[Sound-open-firmware] [RFC PATCH 3/6] dma: introduce new API for requesting DMAC
Ranjani Sridharan
ranjani.sridharan at linux.intel.com
Wed Jun 6 08:00:23 CEST 2018
On Wed, 2018-06-06 at 11:20 +0800, Keyon Jie wrote:
>
> On 2018年06月06日 03:18, Ranjani Sridharan wrote:
> > On Tue, 2018-06-05 at 19:05 +0800, Keyon Jie wrote:
> > >
> > > On 2018年06月05日 12:23, Ranjani Sridharan wrote:
> > > > This patch introduces a new API for procuring DMAC for various
> > > > user based on the type of DMAC, copy direction and other flags
> > > > for
> > > > share/exclusive access. It defines the necessary DMAC user
> > > > types,
> > > > copy direction and the flags to suuport the new API.It also
> > > > adds
> > > > two new members to dma_plat_data to differentiate the
> > > > DMAC based on usage and copy direction.
> > > >
> > > > Finally, it also contains the platform specific implementation
> > > > of the new dma_get() API.
> > > >
> > > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel
> > > > .com
> > > > >
> > > >
> > > > ---
> > > > src/include/sof/dma.h | 26 ++++++++++++++++-
> > > > src/platform/apollolake/dma.c | 53
> > > > ++++++++++++++++++++++++++++++++---
> > > > src/platform/baytrail/dma.c | 41 ++++++++++++++++++++++++-
> > > > --
> > > > src/platform/cannonlake/dma.c | 53
> > > > ++++++++++++++++++++++++++++++++---
> > > > src/platform/haswell/dma.c | 49
> > > > ++++++++++++++++++++++++++--
> > > > ----
> > > > 5 files changed, 201 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/src/include/sof/dma.h b/src/include/sof/dma.h
> > > > index 1de9efc..25581e6 100644
> > > > --- a/src/include/sof/dma.h
> > > > +++ b/src/include/sof/dma.h
> > > > @@ -48,6 +48,28 @@ enum dma_copy_dir {
> > > > DMA_DIR_DEV_TO_DEV,
> > > > };
> > > >
> > > > +/*
> > > > + * DMAC copy directions
> > > > + * These are used to specify the copy direction while
> > > > requesting a
> > > > DMAC
> > > > + */
> > > > +enum dmac_copy_dir {
> > > > + DMAC_DIR_READ = 0,
> > > > + DMAC_DIR_WRITE,
> > > > + DMAC_DIR_DUPLEX,
> > > > +};
> > >
> > > I can't understand this, what is read and what is write?
> > > For DMA, it always read from some buffer and write them to
> > > another
> > > buffer.
> > >
> > > [edit] after reading the patch, I got your point. maybe naming it
> > > with
> > >
> > > +enum dmac_dir_cap {
> > > + DMAC_CAP_READ = 0,
> > > + DMAC_CAP_WRITE,
> > > + DMAC_CAP_DUPLEX,
> > > +};
> > >
> > > or we can use bit mask for that, which can be reused with user
> > > type
> > > below also, e.g.
> > >
> > > bit 0: cap_read
> > > bit 1: cap_write
> > > bit 2: host/link
> > > bit 3: lp/hp.
> > > bit 4: share/exclusive
> >
> > I can rename the enum but I'd like to stick with the arguments for
> > the
> > dma_get API. It is really only 3 arguments and it makes deciphering
> > code much easier than applying bitmasks.
> >
> > Unless, you have a compelling reason for me not to use separate
> > arguments?
>
> I think using the bitmasks is more object oriented, e.g. for
> direction,
> what directions does your dmac supported? host_mem_to_mem(1)?
> mem_to_mem(2)? mem_to_dev(3)? dev_to_dev(4)? mem_to_link(5)? or the
> opposite? The combination of these can be up to 10.
Is it only 8 or 10? mem_to hostmem (6), dev_to_mem(7) and
link_to_mem(8). What am I missing?
>
> for byt/bdw, we can support (1)(2)(3)(4)(6)(7)(8)(9);
> for apl/cnl, host_dmac_output can support (1) only, host_dmac_input
> can
> support (6) only, gpdma can support (2)(3)(4)(7)(8)(9),
> link_dmac_output
> can support (5), link_dmac_input can support (10) only.
>
> if you tell these clear enough, you will be able to use unify logic
> in a
With the above definitions, I can unify the type/dir arguments but I'd
still need the access flags.
Thanks for this. I can incorporate this change in v2.
> common dma_get():
> /* check DMAC copy dir */
> if (dma[i].plat_data.caps & require_cap == require_cap)
> retrun dma[i]; // very simple, isn't it?
>
> for the user:
> byt/bdw: playback,
> host: dma_get(host_mem_to_mem);
> dai: dma_get(mem_to_dev);
> capture,
> host: dma_get(mem_to_hostmem);
> dai: dma_get(dev_to_mem_);
> page_table_parsing,
> dma_get(host_mem_to_mem);
> dma_trace,
> dma_get(mem_to_host_mem).
> apl/cnl: playback,
> host: dma_get(host_mem_to_mem);
> dai: dma_get(mem_to_dev);
> capture,
> host: dma_get(mem_to_hostmem);
> dai: dma_get(dev_to_mem_);
> page_table_parsing,
> dma_get(host_mem_to_mem);
> dma_trace,
> dma_get(mem_to_host_mem).
>
>
> can you see what? with this, you can make the code very clean/unified
> in
> host/dai/dma_trace...
>
>
> >
> > > ...
> > >
> > > > +
> > > > +/* DMAC user types */
> > > > +enum dmac_user {
> > > > + DMAC_USER_HOST_DMA = 0,
> > > > + DMAC_USER_LINK_DMA,
> > >
> > > do we need split into output and input for these?
> > >
> > > > + DMAC_USER_GP_LP_DMA,
> > > > + DMAC_USER_GP_HP_DMA,
> > > > +};
> > > > +
> > > > +/* DMAC flags */
> > > > +#define DMAC_FLAGS_SHARED 0
> > > > +#define DMAC_FLAGS_EXCLUSIVE (1 << 0)
> > >
> > > Can you explain what is shared and how channels are shared?
> > > Are you meaning channel reservation for specific user with
> > > exclusive?
> > > I can't image the use case for this.
> >
> > Shared is the most common usage, where we share DMAC between users.
> >
> > Exclusive as Liam explained will be fore critical usages that
> > require
> > that only one user will use the DMAC at any time. ie there will
> > only be
> > one channel draining at any time.
>
> Hi Liam, can you explain more about this? Where is the limitation
> about
> only one channel draining at the same time, from DMAC or the user?
> Per
> my understanding, channels of the same DMAC are quite independent,
> they
> can in draining at the same time from the point of DMAC. And, we
> event
> don't use the DMA draining feature yet.
>
> Thanks,
> ~Keyon
>
> >
> > I havent seen the need for exclusive access so far. But we might
> > need
> > it in the future.
> >
> > >
> > >
> > > > +
> > > > /* DMA IRQ types */
> > > > #define DMA_IRQ_TYPE_BLOCK (1 << 0)
> > > > #define DMA_IRQ_TYPE_LLIST (1 << 1)
> > > > @@ -119,6 +141,8 @@ struct dma_ops {
> > > > /* DMA platform data */
> > > > struct dma_plat_data {
> > > > uint32_t id;
> > > > + enum dmac_user type;
> > > > + enum dmac_copy_dir copy_dir;
> > > > uint32_t base;
> > > > uint32_t channels;
> > > > uint32_t irq;
> > > > @@ -139,7 +163,7 @@ struct dma_int {
> > > > uint32_t irq;
> > > > };
> > > >
> > > > -struct dma *dma_get(int dmac_id);
> > > > +struct dma *dma_get(enum dmac_user, enum dmac_copy_dir, int
> > > > flags);
> > > >
> > > > /* initialize all platform DMAC's */
> > > > void dmac_init(void);
> > > > diff --git a/src/platform/apollolake/dma.c
> > > > b/src/platform/apollolake/dma.c
> > > > index 001bb67..1ae097e 100644
> > > > --- a/src/platform/apollolake/dma.c
> > > > +++ b/src/platform/apollolake/dma.c
> > > > @@ -112,6 +112,8 @@ static struct dma dma[] = {
> > > > { /* Low Power GP DMAC 0 */
> > > > .plat_data = {
> > > > .id = DMA_GP_LP_DMAC0,
> > > > + .type = DMAC_USER_GP_LP_DMA,
> > > > + .copy_dir = DMAC_DIR_DUPLEX,
> > > > .base = LP_GP_DMA_BASE(0),
> > > > .channels = 8,
> > > > .irq =
> > > > IRQ_EXT_LP_GPDMA0_LVL5(0,
> > > > 0),
> > > > @@ -122,6 +124,8 @@ static struct dma dma[] = {
> > > > { /* Low Power GP DMAC 1 */
> > > > .plat_data = {
> > > > .id = DMA_GP_LP_DMAC1,
> > > > + .type = DMAC_USER_GP_LP_DMA,
> > > > + .copy_dir = DMAC_DIR_DUPLEX,
> > > > .base = LP_GP_DMA_BASE(1),
> > > > .channels = 8,
> > > > .irq =
> > > > IRQ_EXT_LP_GPDMA1_LVL5(0,
> > > > 0),
> > > > @@ -132,6 +136,8 @@ static struct dma dma[] = {
> > > > { /* Host In DMAC */
> > > > .plat_data = {
> > > > .id = DMA_HOST_IN_DMAC,
> > > > + .type = DMAC_USER_HOST_DMA,
> > > > + .copy_dir = DMAC_DIR_WRITE,
> > > > .base =
> > > > GTW_HOST_IN_STREAM_BASE(0),
> > > > .channels = 7,
> > > > .irq =
> > > > IRQ_EXT_HOST_DMA_IN_LVL3(0,
> > > > 0),
> > > > @@ -142,6 +148,8 @@ static struct dma dma[] = {
> > > > { /* Host out DMAC */
> > > > .plat_data = {
> > > > .id = DMA_HOST_OUT_DMAC,
> > > > + .type = DMAC_USER_HOST_DMA,
> > > > + .copy_dir = DMAC_DIR_READ,
> > > > .base =
> > > > GTW_HOST_OUT_STREAM_BASE(0),
> > > > .channels = 6,
> > > > .irq =
> > > > IRQ_EXT_HOST_DMA_OUT_LVL3(0, 0),
> > > > @@ -152,6 +160,8 @@ static struct dma dma[] = {
> > > > { /* Link In DMAC */
> > > > .plat_data = {
> > > > .id = DMA_LINK_IN_DMAC,
> > > > + .type = DMAC_USER_LINK_DMA,
> > > > + .copy_dir = DMAC_DIR_WRITE,
> > > > .base =
> > > > GTW_LINK_IN_STREAM_BASE(0),
> > > > .channels = 8,
> > > > .irq =
> > > > IRQ_EXT_LINK_DMA_IN_LVL4(0,
> > > > 0),
> > > > @@ -162,6 +172,8 @@ static struct dma dma[] = {
> > > > { /* Link out DMAC */
> > > > .plat_data = {
> > > > .id = DMA_LINK_OUT_DMAC,
> > > > + .type = DMAC_USER_LINK_DMA,
> > > > + .copy_dir = DMAC_DIR_READ,
> > > > .base =
> > > > GTW_LINK_OUT_STREAM_BASE(0),
> > > > .channels = 8,
> > > > .irq =
> > > > IRQ_EXT_LINK_DMA_OUT_LVL4(0, 0),
> > > > @@ -170,15 +182,48 @@ static struct dma dma[] = {
> > > > .ops = &hda_link_dma_ops,
> > > > },};
> > > >
> > > > -struct dma *dma_get(int dmac_id)
> > > > +/*
> > > > + * get DMAC based on user type, copy dir and flags
> > > > + * "flags" is used to set the type of access requested
> > > > + * (ex: shared/exclusive access)
> > > > + */
> > > > +struct dma *dma_get(enum dmac_user user, enum dmac_copy_dir
> > > > dir,
> > > > int flags)
> > > > {
> > > > - int i;
> > > > + int i, ch_count;
> > > > + int dma_index = -1;
> > > > + int min_ch_count = INT32_MAX;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(dma); i++) {
> > > > - if (dma[i].plat_data.id == dmac_id)
> > > > - return &dma[i];
> > > > +
> > > > + /* check DMAC user type */
> > > > + if (dma[i].plat_data.type != user)
> > > > + continue;
> > > > +
> > > > + /* check DMAC copy dir */
> > > > + if (dma[i].plat_data.copy_dir != dir)
> > > > + continue;
> > > > +
> > > > + /* if exclusive access is requested */
> > > > + if (flags & DMAC_FLAGS_EXCLUSIVE) {
> > > > +
> > > > + /* ret DMA with no channel in use */
> > > > + if (!dma_channel_status(&dma[i]))
> > > > + return &dma[i];
> > > > + } else {
> > > > + /* get number of channels in use */
> > > > + ch_count =
> > > > dma_channel_status(&dma[i]);
> > > > +
> > > > + /* Use this DMAC if its channel count
> > > > is
> > > > lower */
> > > > + if (ch_count < min_ch_count) {
> > > > + dma_index = i;
> > > > + min_ch_count = ch_count;
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > + if (dma_index >= 0)
> > > > + return &dma[dma_index];
> > > > +
> > > > return NULL;
> > > > }
> > > >
> > > > diff --git a/src/platform/baytrail/dma.c
> > > > b/src/platform/baytrail/dma.c
> > > > index 5a0b650..71ea241 100644
> > > > --- a/src/platform/baytrail/dma.c
> > > > +++ b/src/platform/baytrail/dma.c
> > > > @@ -175,15 +175,48 @@ static struct dma dma[] = {
> > > > #endif
> > > > };
> > > >
> > > > -struct dma *dma_get(int dmac_id)
> > > > +/*
> > > > + * get DMAC based on user type, copy dir and flags
> > > > + * For BYT/CHT, ignore the dmac_user and copy_dir arguments
> > > > + * "flags" is used to set the type of access requested
> > > > + * (ex: shared/exclusive access)
> > > > + */
> > > > +struct dma *dma_get(enum dmac_user user, enum dmac_copy_dir
> > > > dir,
> > > > int flags)
> > > > {
> > > > - int i;
> > > > + int i, ch_count;
> > > > + int min_ch_count = INT32_MAX;
> > > > + int dma_index = -1;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(dma); i++) {
> > > > - if (dma[i].plat_data.id == dmac_id)
> > > > - return &dma[i];
> > > > +
> > > > + /* if exclusive access is requested */
> > > > + if (flags & DMAC_FLAGS_EXCLUSIVE) {
> > > > +
> > > > + /* ret DMA with no channel in use */
> > > > + if (!dma_channel_status(&dma[i]))
> > > > + return &dma[i];
> > > > + } else {
> > > > +
> > > > + /*
> > > > + * for shared access requests, return
> > > > DMAC
> > > > + * with the least number of channels
> > > > in
> > > > use
> > > > + */
> > > > +
> > > > + /* get number of channels in use for
> > > > this
> > > > DMAC*/
> > > > + ch_count =
> > > > dma_channel_status(&dma[i]);
> > > > +
> > > > + /* Use this DMAC if its channel count
> > > > is
> > > > lower */
> > > > + if (ch_count < min_ch_count) {
> > > > + dma_index = i;
> > > > + min_ch_count = ch_count;
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > + /* return DMAC */
> > > > + if (dma_index >= 0)
> > > > + return &dma[dma_index];
> > > > +
> > > > return NULL;
> > > > }
> > >
> > > again, this function can be shared also?
> > >
> > > Thanks,
> > > ~Keyon
> > >
> > > >
> > > > diff --git a/src/platform/cannonlake/dma.c
> > > > b/src/platform/cannonlake/dma.c
> > > > index 9031c17..697b9a8 100644
> > > > --- a/src/platform/cannonlake/dma.c
> > > > +++ b/src/platform/cannonlake/dma.c
> > > > @@ -113,6 +113,8 @@ static struct dma dma[] = {
> > > > { /* Low Power GP DMAC 0 */
> > > > .plat_data = {
> > > > .id = DMA_GP_LP_DMAC0,
> > > > + .type = DMAC_USER_GP_LP_DMA,
> > > > + .copy_dir = DMAC_DIR_DUPLEX,
> > > > .base = LP_GP_DMA_BASE(0),
> > > > .channels = 8,
> > > > .irq = IRQ_EXT_LP_GPDMA0_LVL5(0, 0),
> > > > @@ -123,6 +125,8 @@ static struct dma dma[] = {
> > > > { /* Low Power GP DMAC 1 */
> > > > .plat_data = {
> > > > .id = DMA_GP_LP_DMAC1,
> > > > + .type = DMAC_USER_GP_LP_DMA,
> > > > + .copy_dir = DMAC_DIR_DUPLEX,
> > > > .base = LP_GP_DMA_BASE(1),
> > > > .channels = 8,
> > > > .irq = IRQ_EXT_LP_GPDMA1_LVL5(0, 0),
> > > > @@ -133,6 +137,8 @@ static struct dma dma[] = {
> > > > { /* Host In DMAC */
> > > > .plat_data = {
> > > > .id = DMA_HOST_IN_DMAC,
> > > > + .type = DMAC_USER_HOST_DMA,
> > > > + .copy_dir = DMAC_DIR_WRITE,
> > > > .base =
> > > > GTW_HOST_IN_STREAM_BASE(0),
> > > > .channels = 7,
> > > > .irq = IRQ_EXT_HOST_DMA_IN_LVL3(0, 0),
> > > > @@ -143,6 +149,8 @@ static struct dma dma[] = {
> > > > { /* Host out DMAC */
> > > > .plat_data = {
> > > > .id = DMA_HOST_OUT_DMAC,
> > > > + .type = DMAC_USER_HOST_DMA,
> > > > + .copy_dir = DMAC_DIR_READ,
> > > > .base =
> > > > GTW_HOST_OUT_STREAM_BASE(0),
> > > > .channels = 9,
> > > > .irq = IRQ_EXT_HOST_DMA_OUT_LVL3(0, 0),
> > > > @@ -153,6 +161,8 @@ static struct dma dma[] = {
> > > > { /* Link In DMAC */
> > > > .plat_data = {
> > > > .id = DMA_LINK_IN_DMAC,
> > > > + .type = DMAC_USER_LINK_DMA,
> > > > + .copy_dir = DMAC_DIR_WRITE,
> > > > .base =
> > > > GTW_LINK_IN_STREAM_BASE(0),
> > > > .channels = 9,
> > > > .irq = IRQ_EXT_LINK_DMA_IN_LVL4(0, 0),
> > > > @@ -163,6 +173,8 @@ static struct dma dma[] = {
> > > > { /* Link out DMAC */
> > > > .plat_data = {
> > > > .id = DMA_LINK_OUT_DMAC,
> > > > + .type = DMAC_USER_LINK_DMA,
> > > > + .copy_dir = DMAC_DIR_READ,
> > > > .base =
> > > > GTW_LINK_OUT_STREAM_BASE(0),
> > > > .channels = 7,
> > > > .irq = IRQ_EXT_LINK_DMA_OUT_LVL4(0, 0),
> > > > @@ -171,15 +183,48 @@ static struct dma dma[] = {
> > > > .ops = &hda_link_dma_ops,
> > > > },};
> > > >
> > > > -struct dma *dma_get(int dmac_id)
> > > > +/*
> > > > + * get DMAC based on user type, copy dir and flags
> > > > + * "flags" is used to set the type of access requested
> > > > + * (ex: shared/exclusive access)
> > > > + */
> > > > +struct dma *dma_get(enum dmac_user user, enum dmac_copy_dir
> > > > dir,
> > > > int flags)
> > > > {
> > > > - int i;
> > > > + int i, ch_count;
> > > > + int dma_index = -1;
> > > > + int min_ch_count = INT32_MAX;
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(dma); i++) {
> > > > - if (dma[i].plat_data.id == dmac_id)
> > > > - return &dma[i];
> > > > +
> > > > + /* check DMAC user type */
> > > > + if (dma[i].plat_data.type != user)
> > > > + continue;
> > > > +
> > > > + /* check DMAC copy dir */
> > > > + if (dma[i].plat_data.copy_dir != dir)
> > > > + continue;
> > > > +
> > > > + /* if exclusive access is requested */
> > > > + if (flags & DMAC_FLAGS_EXCLUSIVE) {
> > > > +
> > > > + /* ret DMA with no channel in use */
> > > > + if (!dma_channel_status(&dma[i]))
> > > > + return &dma[i];
> > > > + } else {
> > > > + /* get number of channels in use */
> > > > + ch_count =
> > > > dma_channel_status(&dma[i]);
> > > > +
> > > > + /* Use this DMAC if its channel count
> > > > is
> > > > lower */
> > > > + if (ch_count < min_ch_count) {
> > > > + dma_index = i;
> > > > + min_ch_count = ch_count;
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > + if (dma_index >= 0)
> > > > + return &dma[dma_index];
> > > > +
> > > > return NULL;
> > > > }
> > > >
> > > > diff --git a/src/platform/haswell/dma.c
> > > > b/src/platform/haswell/dma.c
> > > > index 5d1e308..9138e78 100644
> > > > --- a/src/platform/haswell/dma.c
> > > > +++ b/src/platform/haswell/dma.c
> > > > @@ -124,16 +124,49 @@ static struct dma dma[] = {
> > > > .ops = &dw_dma_ops,
> > > > },};
> > > >
> > > > -struct dma *dma_get(int dmac_id)
> > > > +/*
> > > > + * get DMAC based on user type, copy dir and flags
> > > > + * For BYT/CHT, ignore the dmac_user and copy_dir arguments
> > > > + * "flags" is used to set the type of access requested
> > > > + * (ex: shared/exclusive access)
> > > > + */
> > > > +struct dma *dma_get(enum dmac_user user, enum dmac_copy_dir
> > > > dir,
> > > > int flags)
> > > > {
> > > > - switch (dmac_id) {
> > > > - case DMA_ID_DMAC0:
> > > > - return &dma[0];
> > > > - case DMA_ID_DMAC1:
> > > > - return &dma[1];
> > > > - default:
> > > > - return NULL;
> > > > + int i, ch_count;
> > > > + int min_ch_count = INT32_MAX;
> > > > + int dma_index = -1;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(dma); i++) {
> > > > +
> > > > + /* if exclusive access is requested */
> > > > + if (flags & DMAC_FLAGS_EXCLUSIVE) {
> > > > +
> > > > + /* ret DMA with no channel in use */
> > > > + if (!dma_channel_status(&dma[i]))
> > > > + return &dma[i];
> > > > + } else {
> > > > +
> > > > + /*
> > > > + * for shared access requests, return
> > > > DMAC
> > > > + * with the least number of channels
> > > > in
> > > > use
> > > > + */
> > > > +
> > > > + /* get number of channels in use for
> > > > this
> > > > DMAC*/
> > > > + ch_count =
> > > > dma_channel_status(&dma[i]);
> > > > +
> > > > + /* Use this DMAC if its channel count
> > > > is
> > > > lower */
> > > > + if (ch_count < min_ch_count) {
> > > > + dma_index = i;
> > > > + min_ch_count = ch_count;
> > > > + }
> > > > + }
> > > > }
> > > > +
> > > > + /* return DMAC */
> > > > + if (dma_index >= 0)
> > > > + return &dma[dma_index];
> > > > +
> > > > + return NULL;
> > > > }
> > > >
> > > > /* Initialize all platform DMAC's */
> > > >
> > >
> > > _______________________________________________
> > > Sound-open-firmware mailing list
> > > Sound-open-firmware at alsa-project.org
> > > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmw
> > > are
>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list