Skip to content

[SQUASHME] ASoC: SOF : hda: better split between HDA and non-HDA support #257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 1 commit into from
Closed

[SQUASHME] ASoC: SOF : hda: better split between HDA and non-HDA support #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions sound/soc/sof/intel/hda-bus.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,21 @@ int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,

bus->io_ops = &io_ops;
INIT_LIST_HEAD(&bus->stream_list);
INIT_LIST_HEAD(&bus->codec_list);

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
bus->ops = &bus_ops;
INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
#endif
spin_lock_init(&bus->reg_lock);
mutex_init(&bus->cmd_mutex);
bus->irq = -1;

bus->ext_ops = ext_ops;
Copy link
Member

@plbossart plbossart Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that ext_ops is NULL except for the HDA_CODEC case

INIT_LIST_HEAD(&bus->hlink_list);
bus->idx = idx++;

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
INIT_LIST_HEAD(&bus->codec_list);
INIT_LIST_HEAD(&bus->hlink_list);
spin_lock_init(&bus->reg_lock);
mutex_init(&bus->cmd_mutex);
mutex_init(&bus->lock);
bus->ops = &bus_ops;
INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
bus->cmd_dma_state = true;
#endif
Copy link

@libinyang libinyang Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some members such as hlink_list is a general member and will be used in other places.
Let's consume some other module such as hda-dai.c is compiled while CONFIG_SND_SOC_SOF_HDA is not enabled (Makefile and Kconfig permit this situation although I think there may not such scenario.) What if hda-dai.c uses members?

Copy link
Author

@keyonjie keyonjie Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@libinyang sorry, don't get your point. are you saying that hda-dai.c doesn't compile when CONFIG_SND_SOC_SOF_HDA is unselected?

Copy link

@libinyang libinyang Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. In this case, it is still in the CONFIG_SND_SOC_SOF_HDA.

Copy link
Member

@plbossart plbossart Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not able to follow these comments, too many double-negatives. I also find suspicious that hlink_list can be parked under the SOF_HDA config, @keyonjie can you double check?

Also for the bus_idx it'd be nice to find a solution that doesn't rely on a static variable, maybe by doing the ref-counting at a higher level and passing the argument.

Copy link
Author

@keyonjie keyonjie Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through hlink_list with Libin and confirmed that all hlink_list related codes are embraced in #ifdef CONFIG_SND_SOC_SOF_HDA.

For bus_idx, I can imagine the potential issue here when doing module unload/reload, sorry the time is used up today, let me figure out a solution for it next Monday.

Copy link
Member

@plbossart plbossart Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the list is wrong per the discussion on #279, reg_lock is required in all cases?

Copy link
Author

@keyonjie keyonjie Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let's move reg_lock out.


return 0;
}
4 changes: 4 additions & 0 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ int hda_dsp_core_reset_power_down(struct snd_sof_dev *sdev,
static int hda_suspend(struct snd_sof_dev *sdev, int state)
{
const struct sof_intel_dsp_desc *chip = sdev->hda->desc;
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
struct hdac_bus *bus = sof_to_bus(sdev);
#endif
int ret = 0;

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
Expand Down Expand Up @@ -307,8 +309,10 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state)
static int hda_resume(struct snd_sof_dev *sdev)
{
const struct sof_intel_dsp_desc *chip = sdev->hda->desc;
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
struct hdac_bus *bus = sof_to_bus(sdev);
struct hdac_ext_link *hlink = NULL;
#endif
int ret;

/*
Expand Down
25 changes: 8 additions & 17 deletions sound/soc/sof/intel/hda-stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,6 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
}

/* mem alloc for the position buffer */
/* TODO: check position buffer update */
ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
SOF_HDA_DPIB_ENTRY_SIZE * num_total,
&bus->posbuf);
Expand All @@ -610,13 +609,15 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
return -ENOMEM;
}

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* mem alloc for the CORB/RIRB ringbuffers */
ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
PAGE_SIZE, &bus->rb);
if (ret < 0) {
dev_err(sdev->dev, "error: RB alloc failed\n");
return -ENOMEM;
}
#endif

/* create capture streams */
for (i = 0; i < num_capture; i++) {
Expand All @@ -643,14 +644,6 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
SOF_HDA_SPIB_MAXFIFO;
}

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_DRSM)
/* FIXME: Remove? HDAC doesn't use DRSM so has no drsm_addr */
/* do we support DRSM */
if (sdev->bar[HDA_DSP_DRSM_BAR])
stream->drsm_addr = sdev->bar[HDA_DSP_DRSM_BAR] +
SOF_HDA_DRSM_BASE + SOF_HDA_DRSM_INTERVAL * i;
#endif

hstream = &stream->hstream;
Copy link
Member

@plbossart plbossart Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was this about? I don't even know what DRSM is, maybe worth sharing and educating the rest of us?

Copy link
Author

@keyonjie keyonjie Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is for HDA DMA resume capabilities, which support resume from DPIB/LPIB, we haven't enabled it yet.

Copy link
Member

@plbossart plbossart Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, how would it work since we take the DSP off on suspend. You'd be missing a lot of context and resuming the DMA might not be that useful. What is the expectation here and was it supported in the SKL and legacy drivers?

Copy link
Author

@keyonjie keyonjie Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supported in SKL driver. In SOF driver, it is a missing feature at the moment, we are relying on ALSA resume which means the resume point may be not so accurate in my opinion.

Copy link
Member

@plbossart plbossart Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education, where do you see this supported in SKL?

hstream->bus = bus;
hstream->sd_int_sta_mask = 1 << i;
Expand Down Expand Up @@ -701,14 +694,6 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
SOF_HDA_SPIB_MAXFIFO;
}

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_DRSM)
/* FIXME: Remove? HDAC doesn't use DRSM so has no drsm_addr */
/* do we support DRSM */
if (sdev->bar[HDA_DSP_DRSM_BAR])
stream->drsm_addr = sdev->bar[HDA_DSP_DRSM_BAR] +
SOF_HDA_DRSM_BASE + SOF_HDA_DRSM_INTERVAL * i;
#endif

hstream = &stream->hstream;
hstream->bus = bus;
hstream->sd_int_sta_mask = 1 << i;
Expand Down Expand Up @@ -747,6 +732,12 @@ void hda_dsp_stream_free(struct snd_sof_dev *sdev)
if (bus->posbuf.area)
snd_dma_free_pages(&bus->posbuf);

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* free position buffer */
if (bus->rb.area)
snd_dma_free_pages(&bus->rb);
#endif

Copy link
Member

@plbossart plbossart Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split this RB part to a fix in a separate patch, this is not controversial and can be merged without issues.

Copy link
Author

@keyonjie keyonjie Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Rander is doing this(#280 ) so let me remove this part.

list_for_each_entry_safe(s, _s, &bus->stream_list, list) {
/* TODO: decouple */

Expand Down