Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 9 additions & 22 deletions src/audio/dai.c
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ static void dai_free(struct comp_dev *dev)
if (dd->chan) {
notifier_unregister(dev, dd->chan, NOTIFIER_ID_DMA_COPY);
dma_channel_put(dd->chan);
dd->chan->dev_data = NULL;
}

dma_put(dd->dma);
Expand Down Expand Up @@ -599,6 +600,8 @@ static int dai_config_prepare(struct comp_dev *dev)
return -EIO;
}

dd->chan->dev_data = dd;

comp_info(dev, "dai_config_prepare(): new configured dma channel index %d",
dd->chan->index);

Expand All @@ -609,27 +612,6 @@ static int dai_config_prepare(struct comp_dev *dev)
return 0;
}

static void dai_config_reset(struct comp_dev *dev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

in a way this commit seems to limit dai_config_reset() to IPC3 only, while the description says, that it should become common. Wouldn't it be better to just make it global here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its not longer limited to IPC3 only with my new changes

{
struct dai_data *dd = comp_get_drvdata(dev);

/* cannot configure DAI while active */
if (dev->state == COMP_STATE_ACTIVE) {
comp_info(dev, "dai_config(): Component is in active state. Ignore resetting");
return;
}

/* put the allocated DMA channel first */
if (dd->chan) {
dma_channel_put(dd->chan);
dd->chan = NULL;

/* remove callback */
notifier_unregister(dev, dd->chan,
NOTIFIER_ID_DMA_COPY);
}
}

static int dai_prepare(struct comp_dev *dev)
{
struct dai_data *dd = comp_get_drvdata(dev);
Expand Down Expand Up @@ -686,7 +668,12 @@ static int dai_reset(struct comp_dev *dev)

comp_info(dev, "dai_reset()");

dai_config_reset(dev);
/*
* DMA channel release should be skipped now for DAI's that support the two-step stop option.
* It will be done when the host sends the DAI_CONFIG IPC during hw_free.
*/
if (!dd->delayed_dma_stop)
dai_dma_release(dev);

dma_sg_free(&config->elem_array);

Expand Down
6 changes: 3 additions & 3 deletions src/audio/host.c
Original file line number Diff line number Diff line change
Expand Up @@ -847,9 +847,12 @@ static int host_reset(struct comp_dev *dev)
comp_dbg(dev, "host_reset()");

if (hd->chan) {
dma_stop_delayed(hd->chan);

Copy link
Member

Choose a reason for hiding this comment

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

for my education, what does a 'reset' have to do with 'stop'? Or is this yet another abuse of language and 'reset' is the dual of 'params' so reset means 'hw_free'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it is not clear in my commit message and I'll fix that. But basically, we need to stop the DMA in the FW after the host has cleared the RUN bit. This applies to both the host DMA and link DMA. For the link DMA, we use the DAI_CONFIG IPC which is send after the RUN bit is cleared to achieve this. For the host DMA, we use the PCM_FREE IPC to do the same thing. A PCM_FREE IPC results in resetting all components in the pipeline and I use this reset op for the host to stop the DMA. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

yes ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ranj063 @plbossart I think you need to update src/include/sof/lib/dma.h as well. We need to be clear on reset() and stop() semantics.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we add a new call as second step stop, then should not be added to other places before dma_channel_put() is called? But if they are coming in pairs why not handle it directly in dma_channel_put() as @kv2019i suggested?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujfalusi so this has the "pause problem" (when pausing, there will not be a call to channeLput() but stop needs to be two-step still)

/* remove callback */
notifier_unregister(dev, hd->chan, NOTIFIER_ID_DMA_COPY);
dma_channel_put(hd->chan);
hd->chan = NULL;
}

/* free all DMA elements */
Expand All @@ -863,9 +866,6 @@ static int host_reset(struct comp_dev *dev)
hd->dma_buffer = NULL;
}

/* reset dma channel as we have put it */
hd->chan = NULL;

host_pointer_reset(dev);
hd->copy_type = COMP_COPY_NORMAL;
hd->source = NULL;
Expand Down
149 changes: 123 additions & 26 deletions src/drivers/intel/hda/hda-dma.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sof/lib/alloc.h>
#include <sof/lib/clk.h>
#include <sof/lib/cpu.h>
#include <sof/lib/dai.h>
#include <sof/lib/dma.h>
#include <sof/lib/pm_runtime.h>
#include <sof/lib/notifier.h>
Expand All @@ -28,6 +29,77 @@
#include <stddef.h>
#include <stdint.h>

/*
* HD-Audio DMA programming sequence
*
* START stream (Playback):
* 1. Host sends the DAI_CONFIG IPC with SOF_DAI_CONFIG_FLAGS_2_STEP_STOP_PAUSE flag along with
* SOF_DAI_CONFIG_FLAGS_HW_PARAMS
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's no firmware action after this IPC, that means, that all the action there is in software? No hardware access?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is just a notification to the FW to perform stop in 2 steps. There's no action but to acknowledge this.

* 2. Host sets DGCS.RUN bit for link DMA. This step would be skipped if link DMA is already
* running with mixer-based pipelines
* 3. Host sets DGCS.RUN bit for host DMA
* 4. Host sends the STREAM_TRIG_START IPC to the DSP
* 5. FW starts the pipeline and sets DGCS.GEN bit to 1
*
* START stream (Capture):
* 1. Host sends the DAI_CONFIG IPC with SOF_DAI_CONFIG_FLAGS_2_STEP_STOP_PAUSE flag along with
* SOF_DAI_CONFIG_FLAGS_HW_PARAMS
* 2. Host sets DGCS.RUN bit for host DMA
* 3. Host sends the STREAM_TRIG_START IPC to the DSP
* 4. FW starts the pipeline and sets DGCS.GEN bit to 1
* 5. Host sets DGCS.RUN bit for link DMA
*
* PAUSE_PUSH an active stream (Playback):
* 1. Host sends the STREAM_TRIG_PAUSE IPC to the DSP
* 2. FW pauses the pipeline but keeps the DGCS.GEN bit set to 1 for host DMA
* 3. Host clears DGCS.RUN bit for host DMA
* 4. Host clears DGCS.RUN bit for link DMA
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this assume HDA DMA on both host and link sides?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right yes.

* 5. Host sends the DAI_CONFIG IPC with only the SOF_DAI_CONFIG_FLAGS_PAUSE command flag
* 6. FW clears the DGCS.GEN bit for link DMA
*
* PAUSE_PUSH an active stream (Capture):
* 1. Host clears DGCS.RUN bit for link DMA
* 2. Host sends the DAI_CONFIG IPC with only the SOF_DAI_CONFIG_FLAGS_PAUSE command flag
* 3. FW clears the DGCS.GEN bit for link DMA
* 4. Host sends the STREAM_TRIG_PAUSE IPC to the DSP
* 5. FW pauses the pipeline but keeps DGCS.GEN bit set to 1 for host DMA
* 6. Host clears DGCS.RUN bit for host DMA
*
* PAUSE_RELEASE a paused stream (Playback):
* 1. Host sets DGCS.RUN bit for link DMA
* 2. Host sets DGCS.RUN bit for host DMA
* 3. Host sends the STREAM_TRIG_PAUSE_RELEASE IPC to the DSP
* 4. FW releases the pipeline and sets the DGCS.GEN bit to 1
*
* PAUSE_RELEASE a paused stream (Capture):
* 1. Host sets DGCS.RUN bit for host DMA
* 2. Host sends the STREAM_TRIG_PAUSE_RELEASE IPC to the DSP
* 3. FW releases the pipeline and sets the DGCS.GEN bit to 1
* 4. Host sets DGCS.RUN bit for link DMA
*
* STOP an active/paused stream (Playback):
* 1. Host sends the STREAM_TRIG_STOP IPC to the DSP
* 2. FW stops the pipeline
* 3. Host clears the DGCS.RUN bit for host DMA
* 4. Host sends the PCM_FREE IPC
* 5. FW clears DGCS.GEN bit for host DMA during host component reset and checks DGCS.GBUSY bit
* to ensure DMA is idle
* 6. Host clears DGCS.RUN bit for link DMA
* 7. Host sends the DAI_CONFIG IPC with the SOF_DAI_CONFIG_FLAGS_HW_FREE flag
* 8. FW clears the DGCS.GEN bit for link DMA and checks DGCS.GBUSY bit to ensure DMA is idle
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use different numbering:

 * 1. Host sends the STREAM_TRIG_STOP IPC to the DSP
 * 1.1. FW stops the pipeline
 * 2. Host clears the DGCS.RUN bit for host DMA
 * 3. Host sends the PCM_FREE IPC
 * 3.1. FW clears DGCS.GEN bit for host DMA during host component reset and checks DGCS.GBUSY bit
 *    to ensure DMA is idle
 * 4. Host clears DGCS.RUN bit for link DMA
 * 5. Host sends the DAI_CONFIG IPC with the SOF_DAI_CONFIG_FLAGS_HW_FREE flag
 * 5.1. FW clears the DGCS.GEN bit for link DMA and checks DGCS.GBUSY bit to ensure DMA is idle

The x.1 is done while processing the message sent at x. and the reply is only sent after said step (or steps) are completed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IPC's are always sequential and so are the steps above. I don't think the numbering change to x and x.1/2 adds much value but then again if there's consensus I dont mind changing it.

Copy link
Member

Choose a reason for hiding this comment

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

Lets keep as is for later compare & alignments if needed.

*
* STOP an active/paused stream (Capture):
* 1. Host clears DGCS.RUN bit for link DMA
* 2. Host sends the DAI_CONFIG IPC with the SOF_DAI_CONFIG_FLAGS_HW_FREE flag
Copy link
Collaborator

Choose a reason for hiding this comment

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

incidentally, this means, that once we get back to STOP-in-task, we'll have to handle DAI_CONFIG in the task too - when it's sent while the task is running... @lgirdwood

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lyakh but the pipeline has already been stopped. Is the taslk still running?

Copy link
Member

Choose a reason for hiding this comment

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

@lyakh yes your right, I think this also needed for ipc4 flows.

* 3. FW clears the DGCS.GEN bit for link DMA and checks GBUSY bit to ensure DMA is idle
* 4. Host sends the STREAM_TRIG_STOP IPC to the DSP
* 5. FW stops the pipeline
* 6. Host clears the DGCS.RUN bit for host DMA
* 7. Host sends the PCM_FREE IPC
* 8. FW clears DGCS.GEN bit for host DMA during host component reset and checks DGCS.GBUSY bit
* to ensure DMA is idle
*/

/* ee12fa71-4579-45d7-bde2-b32c6893a122 */
DECLARE_SOF_UUID("hda-dma", hda_dma_uuid, 0xee12fa71, 0x4579, 0x45d7,
0xbd, 0xe2, 0xb3, 0x2c, 0x68, 0x93, 0xa1, 0x22);
Expand Down Expand Up @@ -57,6 +129,7 @@ DECLARE_TR_CTX(hdma_tr, SOF_UUID(hda_dma_uuid), LOG_LEVEL_INFO);
#define DGCS_BF BIT(9) /* buffer full */
#define DGCS_BNE BIT(8) /* buffer not empty */
#define DGCS_FIFORDY BIT(5) /* enable FIFO */
#define DGCS_GBUSY BIT(15) /* indicates if the DMA channel is idle or not */

/* DGBBA */
#define DGBBA_MASK 0xffff80
Expand Down Expand Up @@ -148,7 +221,7 @@ struct hda_chan_data {
#endif
};

static int hda_dma_stop(struct dma_chan_data *channel);
static int hda_dma_stop_common(struct dma_chan_data *channel);

static inline void hda_dma_inc_fp(struct dma_chan_data *chan,
uint32_t value)
Expand Down Expand Up @@ -570,30 +643,10 @@ static int hda_dma_release(struct dma_chan_data *channel)
return 0;
}

static int hda_dma_pause(struct dma_chan_data *channel)
{
uint32_t flags;

irq_local_disable(flags);

tr_dbg(&hdma_tr, "hda-dmac: %d channel %d -> pause",
channel->dma->plat_data.id, channel->index);

if (channel->status != COMP_STATE_ACTIVE)
goto out;

/* stop the channel */
hda_dma_stop(channel);

out:
irq_local_enable(flags);
return 0;
}

static int hda_dma_stop(struct dma_chan_data *channel)
static int hda_dma_stop_common(struct dma_chan_data *channel)
{
struct hda_chan_data *hda_chan;
uint32_t flags;
uint32_t flags, dgcs;

irq_local_disable(flags);

Expand All @@ -613,6 +666,15 @@ static int hda_dma_stop(struct dma_chan_data *channel)
} else {
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN, 0);
}

/* check if channel is idle. No need to wait after clearing the GEN bit */
dgcs = dma_chan_reg_read(channel, DGCS);
if (dgcs & DGCS_GBUSY) {
tr_err(&hdma_tr, "hda-dmac: %d channel %d not idle after stop",
channel->dma->plat_data.id, channel->index);
irq_local_enable(flags);
return -EBUSY;
}
channel->status = COMP_STATE_PREPARE;
hda_chan = dma_chan_get_data(channel);
hda_chan->state = 0;
Expand All @@ -624,6 +686,40 @@ static int hda_dma_stop(struct dma_chan_data *channel)
return 0;
}

static int hda_dma_link_stop(struct dma_chan_data *channel)
{
struct dai_data *dd = channel->dev_data;

if (!dd) {
tr_err(&hdma_tr, "hda-dmac: %d channel %d no device data",
channel->dma->plat_data.id, channel->index);
return -EINVAL;
}

/*
* The delayed_dma_stop flag will be set with the newer kernel and DMA will be stopped during
* reset. With older kernel, the DMA will be stopped during the stop/pause trigger.
*/
if (!dd->delayed_dma_stop)
return hda_dma_stop_common(channel);

return 0;
}

static int hda_dma_link_pause(struct dma_chan_data *channel)
{
/*
* with two-step pause, DMA will be stopped when the DAI_CONFIG IPC is sent with the
* SOF_DAI_CONFIG_FLAGS_TWO_STEP_STOP flag
*/
return hda_dma_link_stop(channel);
}

static int hda_dma_stop_delayed(struct dma_chan_data *channel)
{
return hda_dma_stop_common(channel);
}

/* fill in "status" with current DMA channel state and position */
static int hda_dma_status(struct dma_chan_data *channel,
struct dma_chan_status *status, uint8_t direction)
Expand Down Expand Up @@ -974,7 +1070,7 @@ const struct dma_ops hda_host_dma_ops = {
.channel_get = hda_dma_channel_get,
.channel_put = hda_dma_channel_put,
.start = hda_dma_start,
.stop = hda_dma_stop,
.stop_delayed = hda_dma_stop_delayed,
.copy = hda_dma_host_copy,
.status = hda_dma_status,
.set_config = hda_dma_set_config,
Expand All @@ -991,9 +1087,10 @@ const struct dma_ops hda_link_dma_ops = {
.channel_get = hda_dma_channel_get,
.channel_put = hda_dma_channel_put,
.start = hda_dma_start,
.stop = hda_dma_stop,
.stop = hda_dma_link_stop,
.stop_delayed = hda_dma_stop_delayed,
.copy = hda_dma_link_copy,
.pause = hda_dma_pause,
.pause = hda_dma_link_pause,
.release = hda_dma_release,
.status = hda_dma_status,
.set_config = hda_dma_set_config,
Expand Down
2 changes: 1 addition & 1 deletion src/drivers/intel/ssp/ssp.c
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ static int ssp_set_config_tplg(struct dai *dai, struct ipc_config_dai *common_co
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_PREPARE;

clk:
switch (config->flags & SOF_DAI_CONFIG_FLAGS_MASK) {
switch (config->flags & SOF_DAI_CONFIG_FLAGS_CMD_MASK) {
case SOF_DAI_CONFIG_FLAGS_HW_PARAMS:
if (ssp->params.clks_control & SOF_DAI_INTEL_SSP_CLKCTRL_MCLK_ES) {
ret = ssp_mclk_prepare_enable(dai);
Expand Down
28 changes: 22 additions & 6 deletions src/include/ipc/dai.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,28 @@
#define SOF_DAI_FMT_INV_MASK 0x0f00
#define SOF_DAI_FMT_CLOCK_PROVIDER_MASK 0xf000

/* DAI_CONFIG flags */
#define SOF_DAI_CONFIG_FLAGS_MASK 0x3
#define SOF_DAI_CONFIG_FLAGS_NONE (0 << 0) /**< DAI_CONFIG sent without stage information */
#define SOF_DAI_CONFIG_FLAGS_HW_PARAMS (1 << 0) /**< DAI_CONFIG sent during hw_params stage */
#define SOF_DAI_CONFIG_FLAGS_HW_FREE (2 << 0) /**< DAI_CONFIG sent during hw_free stage */
#define SOF_DAI_CONFIG_FLAGS_RFU (3 << 0) /**< not used, reserved for future use */
/*
* DAI_CONFIG flags. The 4 LSB bits are used for the commands, HW_PARAMS, HW_FREE and PAUSE
* representing when the IPC is sent. The 4 MSB bits are used to add quirks along with the above
* commands.
*/
#define SOF_DAI_CONFIG_FLAGS_CMD_MASK 0xF
#define SOF_DAI_CONFIG_FLAGS_NONE 0 /**< DAI_CONFIG sent without stage information */
#define SOF_DAI_CONFIG_FLAGS_HW_PARAMS BIT(0) /**< DAI_CONFIG sent during hw_params stage */
#define SOF_DAI_CONFIG_FLAGS_HW_FREE BIT(1) /**< DAI_CONFIG sent during hw_free stage */
/**< DAI_CONFIG sent during pause trigger. Only available ABI 3.20 onwards */
#define SOF_DAI_CONFIG_FLAGS_PAUSE BIT(2)
#define SOF_DAI_CONFIG_FLAGS_QUIRK_SHIFT 4
#define SOF_DAI_CONFIG_FLAGS_QUIRK_MASK (0xF << SOF_DAI_CONFIG_FLAGS_QUIRK_SHIFT)
/*
* This should be used along with the SOF_DAI_CONFIG_FLAGS_HW_PARAMS to indicate that pipeline
* stop/pause and DAI DMA stop/pause should happen in two steps. This change is only available
* ABI 3.20 onwards.
*/
#define SOF_DAI_CONFIG_FLAGS_2_STEP_STOP BIT(0)

#define SOF_DAI_QUIRK_IS_SET(flags, quirk) \
(((flags & SOF_DAI_CONFIG_FLAGS_QUIRK_MASK) >> SOF_DAI_CONFIG_FLAGS_QUIRK_SHIFT) & quirk)

/** \brief Types of DAI */
enum sof_ipc_dai_type {
Expand Down
2 changes: 1 addition & 1 deletion src/include/kernel/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

/** \brief SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 19
#define SOF_ABI_MINOR 20
#define SOF_ABI_PATCH 0

/** \brief SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down
14 changes: 14 additions & 0 deletions src/include/sof/lib/dai.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ struct dai_data {
void *dai_spec_config; /* dai specific config from the host */

uint64_t wallclock; /* wall clock at stream start */

/*
* flag indicating two-step stop/pause for DAI comp and DAI DMA.
* DAI stop occurs during STREAM_TRIG_STOP IPC and DMA stop during DAI_CONFIG IPC with
* the SOF_DAI_CONFIG_FLAGS_HW_FREE flag.
* DAI pause occurs during STREAM_TRIG_PAUSE IPC and DMA pause during DAI_CONFIG IPC with
* the SOF_DAI_CONFIG_FLAGS_PAUSE flag.
*/
bool delayed_dma_stop;
};

struct dai {
Expand Down Expand Up @@ -508,6 +517,11 @@ static inline const struct dai_info *dai_info_get(void)
*/
int dai_config_dma_channel(struct comp_dev *dev, void *config);

/**
* \brief Reset DAI DMA config
*/
void dai_dma_release(struct comp_dev *dev);

/**
* \brief Configure DAI physical interface.
*/
Expand Down
Loading