| Message ID | 1462428071-30248-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 11:31:09AM +0530, Vinod Koul wrote: > The HDA links can be switched off when not is use, similarly > command DMA can be stopped as well. This calls for a reference > counting mechanism on the link by it's users to manage the link > power. The DMA can be turned off when all links are off > > For this we add two APIs > snd_hdac_ext_bus_link_get > snd_hdac_ext_bus_link_put > > They help users to turn up/down link and manage the DMA as well And I missed CCing Takashi, sorry about that. Takashi are you okay with this patch and it going thu ASoC tree :) > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > include/sound/hdaudio_ext.h | 13 ++++++++ > sound/hda/ext/hdac_ext_bus.c | 3 ++ > sound/hda/ext/hdac_ext_controller.c | 62 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 78 insertions(+) > > diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h > index 07fa59237feb..56fa65a1195d 100644 > --- a/include/sound/hdaudio_ext.h > +++ b/include/sound/hdaudio_ext.h > @@ -14,6 +14,8 @@ > * @gtscap: gts capabilities pointer > * @drsmcap: dma resume capabilities pointer > * @hlink_list: link list of HDA links > + * @lock: lock for link mgmt > + * @cmd_io: state of cmd_io > */ > struct hdac_ext_bus { > struct hdac_bus bus; > @@ -27,6 +29,9 @@ struct hdac_ext_bus { > void __iomem *drsmcap; > > struct list_head hlink_list; > + > + struct mutex lock; > + int cmd_io; > }; > > int snd_hdac_ext_bus_init(struct hdac_ext_bus *sbus, struct device *dev, > @@ -142,6 +147,9 @@ struct hdac_ext_link { > void __iomem *ml_addr; /* link output stream reg pointer */ > u32 lcaps; /* link capablities */ > u16 lsdiid; /* link sdi identifier */ > + > + int ref_count; > + > struct list_head list; > }; > > @@ -154,6 +162,11 @@ void snd_hdac_ext_link_set_stream_id(struct hdac_ext_link *link, > void snd_hdac_ext_link_clear_stream_id(struct hdac_ext_link *link, > int stream); > > +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, > + struct hdac_ext_link *link); > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > + struct hdac_ext_link *link); > + > /* update register macro */ > #define snd_hdac_updatel(addr, reg, mask, val) \ > writel(((readl(addr + reg) & ~(mask)) | (val)), \ > diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c > index 64de0a3d6d93..0f62b5498c3d 100644 > --- a/sound/hda/ext/hdac_ext_bus.c > +++ b/sound/hda/ext/hdac_ext_bus.c > @@ -105,6 +105,9 @@ int snd_hdac_ext_bus_init(struct hdac_ext_bus *ebus, struct device *dev, > INIT_LIST_HEAD(&ebus->hlink_list); > ebus->idx = idx++; > > + mutex_init(&ebus->lock); > + ebus->cmd_io = 1; > + > return 0; > } > EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); > diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c > index 548cc1e4114b..9b60624d3405 100644 > --- a/sound/hda/ext/hdac_ext_controller.c > +++ b/sound/hda/ext/hdac_ext_controller.c > @@ -186,6 +186,9 @@ int snd_hdac_ext_bus_get_ml_capabilities(struct hdac_ext_bus *ebus) > hlink->lcaps = readl(hlink->ml_addr + AZX_REG_ML_LCAP); > hlink->lsdiid = readw(hlink->ml_addr + AZX_REG_ML_LSDIID); > > + /* since link in On, update the ref */ > + hlink->ref_count = 1; > + > list_add_tail(&hlink->list, &ebus->hlink_list); > } > > @@ -327,3 +330,62 @@ int snd_hdac_ext_bus_link_power_down_all(struct hdac_ext_bus *ebus) > return 0; > } > EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_power_down_all); > + > +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, > + struct hdac_ext_link *link) > +{ > + int ret = 0; > + > + mutex_lock(&ebus->lock); > + > + /* > + * if we move from 0 to 1, count will be 1 so power up this link > + * as well, also check the dma status and trigger that > + */ > + if (++link->ref_count == 1) { > + if (!ebus->cmd_io) { > + snd_hdac_bus_init_cmd_io(&ebus->bus); > + ebus->cmd_io = 1; > + } > + > + ret = snd_hdac_ext_bus_link_power_up(link); > + } > + > + mutex_unlock(&ebus->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_get); > + > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > + struct hdac_ext_link *link) > +{ > + int ret = 0, state = 0; > + struct hdac_ext_link *hlink = NULL; > + > + mutex_lock(&ebus->lock); > + > + /* > + * if we move from 1 to 0, count will be 0 > + * so power down this link as well > + */ > + if (--link->ref_count == 0) { > + ret = snd_hdac_ext_bus_link_power_down(link); > + > + /* > + * now check if all links are off, if so turn off > + * cmd dma as well > + */ > + list_for_each_entry(hlink, &ebus->hlink_list, list) { > + if (hlink->ref_count) > + state++; > + } > + if (!state) { > + snd_hdac_bus_stop_cmd_io(&ebus->bus); > + ebus->cmd_io = 0; > + } > + } > + > + mutex_unlock(&ebus->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_put); > -- > 1.9.1 >
On Mon, 09 May 2016 06:41:00 +0200, Vinod Koul wrote: > > On Thu, May 05, 2016 at 11:31:09AM +0530, Vinod Koul wrote: > > The HDA links can be switched off when not is use, similarly > > command DMA can be stopped as well. This calls for a reference > > counting mechanism on the link by it's users to manage the link > > power. The DMA can be turned off when all links are off > > > > For this we add two APIs > > snd_hdac_ext_bus_link_get > > snd_hdac_ext_bus_link_put > > > > They help users to turn up/down link and manage the DMA as well > > And I missed CCing Takashi, sorry about that. Takashi are you okay with this > patch and it going thu ASoC tree :) > > > > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > include/sound/hdaudio_ext.h | 13 ++++++++ > > sound/hda/ext/hdac_ext_bus.c | 3 ++ > > sound/hda/ext/hdac_ext_controller.c | 62 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 78 insertions(+) > > > > diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h > > index 07fa59237feb..56fa65a1195d 100644 > > --- a/include/sound/hdaudio_ext.h > > +++ b/include/sound/hdaudio_ext.h > > @@ -14,6 +14,8 @@ > > * @gtscap: gts capabilities pointer > > * @drsmcap: dma resume capabilities pointer > > * @hlink_list: link list of HDA links > > + * @lock: lock for link mgmt > > + * @cmd_io: state of cmd_io > > */ > > struct hdac_ext_bus { > > struct hdac_bus bus; > > @@ -27,6 +29,9 @@ struct hdac_ext_bus { > > void __iomem *drsmcap; > > > > struct list_head hlink_list; > > + > > + struct mutex lock; > > + int cmd_io; It'd be better to put some comments what the flag means. Also, a bool would be clearer. > > }; > > > > int snd_hdac_ext_bus_init(struct hdac_ext_bus *sbus, struct device *dev, > > @@ -142,6 +147,9 @@ struct hdac_ext_link { > > void __iomem *ml_addr; /* link output stream reg pointer */ > > u32 lcaps; /* link capablities */ > > u16 lsdiid; /* link sdi identifier */ > > + > > + int ref_count; > > + > > struct list_head list; > > }; > > > > @@ -154,6 +162,11 @@ void snd_hdac_ext_link_set_stream_id(struct hdac_ext_link *link, > > void snd_hdac_ext_link_clear_stream_id(struct hdac_ext_link *link, > > int stream); > > > > +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, > > + struct hdac_ext_link *link); > > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > > + struct hdac_ext_link *link); > > + > > /* update register macro */ > > #define snd_hdac_updatel(addr, reg, mask, val) \ > > writel(((readl(addr + reg) & ~(mask)) | (val)), \ > > diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c > > index 64de0a3d6d93..0f62b5498c3d 100644 > > --- a/sound/hda/ext/hdac_ext_bus.c > > +++ b/sound/hda/ext/hdac_ext_bus.c > > @@ -105,6 +105,9 @@ int snd_hdac_ext_bus_init(struct hdac_ext_bus *ebus, struct device *dev, > > INIT_LIST_HEAD(&ebus->hlink_list); > > ebus->idx = idx++; > > > > + mutex_init(&ebus->lock); > > + ebus->cmd_io = 1; > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); > > diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c > > index 548cc1e4114b..9b60624d3405 100644 > > --- a/sound/hda/ext/hdac_ext_controller.c > > +++ b/sound/hda/ext/hdac_ext_controller.c > > @@ -186,6 +186,9 @@ int snd_hdac_ext_bus_get_ml_capabilities(struct hdac_ext_bus *ebus) > > hlink->lcaps = readl(hlink->ml_addr + AZX_REG_ML_LCAP); > > hlink->lsdiid = readw(hlink->ml_addr + AZX_REG_ML_LSDIID); > > > > + /* since link in On, update the ref */ > > + hlink->ref_count = 1; > > + > > list_add_tail(&hlink->list, &ebus->hlink_list); > > } > > > > @@ -327,3 +330,62 @@ int snd_hdac_ext_bus_link_power_down_all(struct hdac_ext_bus *ebus) > > return 0; > > } > > EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_power_down_all); > > + > > +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, > > + struct hdac_ext_link *link) > > +{ > > + int ret = 0; > > + > > + mutex_lock(&ebus->lock); > > + > > + /* > > + * if we move from 0 to 1, count will be 1 so power up this link > > + * as well, also check the dma status and trigger that > > + */ > > + if (++link->ref_count == 1) { > > + if (!ebus->cmd_io) { > > + snd_hdac_bus_init_cmd_io(&ebus->bus); > > + ebus->cmd_io = 1; > > + } > > + > > + ret = snd_hdac_ext_bus_link_power_up(link); > > + } > > + > > + mutex_unlock(&ebus->lock); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_get); > > + > > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > > + struct hdac_ext_link *link) > > +{ > > + int ret = 0, state = 0; > > + struct hdac_ext_link *hlink = NULL; Why initializing hlink? > > + > > + mutex_lock(&ebus->lock); > > + > > + /* > > + * if we move from 1 to 0, count will be 0 > > + * so power down this link as well > > + */ > > + if (--link->ref_count == 0) { > > + ret = snd_hdac_ext_bus_link_power_down(link); > > + > > + /* > > + * now check if all links are off, if so turn off > > + * cmd dma as well > > + */ > > + list_for_each_entry(hlink, &ebus->hlink_list, list) { > > + if (hlink->ref_count) > > + state++; > > + } Basically you can break at the first match. But it's supposed to be a relatively short list, so no performance impact should be seen. So, take micro-optimization only if it's simple enough. thanks, Takashi > > + if (!state) { > > + snd_hdac_bus_stop_cmd_io(&ebus->bus); > > + ebus->cmd_io = 0; > > + } > > + } > > + > > + mutex_unlock(&ebus->lock); > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_put); > > -- > > 1.9.1 > > > > -- > ~Vinod >
On Mon, May 09, 2016 at 08:40:56AM +0200, Takashi Iwai wrote: > > > * @gtscap: gts capabilities pointer > > > * @drsmcap: dma resume capabilities pointer > > > * @hlink_list: link list of HDA links > > > + * @lock: lock for link mgmt > > > + * @cmd_io: state of cmd_io > > > */ > > > struct hdac_ext_bus { > > > struct hdac_bus bus; > > > @@ -27,6 +29,9 @@ struct hdac_ext_bus { > > > void __iomem *drsmcap; > > > > > > struct list_head hlink_list; > > > + > > > + struct mutex lock; > > > + int cmd_io; > > It'd be better to put some comments what the flag means. > Also, a bool would be clearer. Well we do have comment that it means state of cmd_io but yes we can make it clearer by explicitly saying the state of cmd DMAs CORB and RIRB I will make it a bool > > > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > > > + struct hdac_ext_link *link) > > > +{ > > > + int ret = 0, state = 0; > > > + struct hdac_ext_link *hlink = NULL; > > Why initializing hlink? Not required :) > > > + /* > > > + * if we move from 1 to 0, count will be 0 > > > + * so power down this link as well > > > + */ > > > + if (--link->ref_count == 0) { > > > + ret = snd_hdac_ext_bus_link_power_down(link); > > > + > > > + /* > > > + * now check if all links are off, if so turn off > > > + * cmd dma as well > > > + */ > > > + list_for_each_entry(hlink, &ebus->hlink_list, list) { > > > + if (hlink->ref_count) > > > + state++; > > > + } > > Basically you can break at the first match. But it's supposed to be a > relatively short list, so no performance impact should be seen. > So, take micro-optimization only if it's simple enough. No not in this case, we are scanning the state of all links to see if they are on or not and turning off DMAs when all the links are off, so we can't break at first match in this case. Yes for other places where we find a link, first match break will help, I will check that Thanks
On Mon, 09 May 2016 09:58:23 +0200, Vinod Koul wrote: > > On Mon, May 09, 2016 at 08:40:56AM +0200, Takashi Iwai wrote: > > > > * @gtscap: gts capabilities pointer > > > > * @drsmcap: dma resume capabilities pointer > > > > * @hlink_list: link list of HDA links > > > > + * @lock: lock for link mgmt > > > > + * @cmd_io: state of cmd_io > > > > */ > > > > struct hdac_ext_bus { > > > > struct hdac_bus bus; > > > > @@ -27,6 +29,9 @@ struct hdac_ext_bus { > > > > void __iomem *drsmcap; > > > > > > > > struct list_head hlink_list; > > > > + > > > > + struct mutex lock; > > > > + int cmd_io; > > > > It'd be better to put some comments what the flag means. > > Also, a bool would be clearer. > > Well we do have comment that it means state of cmd_io but yes we can make it > clearer by explicitly saying the state of cmd DMAs CORB and RIRB Or, name it more explicitly. > > I will make it a bool > > > > > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, > > > > + struct hdac_ext_link *link) > > > > +{ > > > > + int ret = 0, state = 0; > > > > + struct hdac_ext_link *hlink = NULL; > > > > Why initializing hlink? > > Not required :) > > > > > + /* > > > > + * if we move from 1 to 0, count will be 0 > > > > + * so power down this link as well > > > > + */ > > > > + if (--link->ref_count == 0) { > > > > + ret = snd_hdac_ext_bus_link_power_down(link); > > > > + > > > > + /* > > > > + * now check if all links are off, if so turn off > > > > + * cmd dma as well > > > > + */ > > > > + list_for_each_entry(hlink, &ebus->hlink_list, list) { > > > > + if (hlink->ref_count) > > > > + state++; > > > > + } > > > > Basically you can break at the first match. But it's supposed to be a > > relatively short list, so no performance impact should be seen. > > So, take micro-optimization only if it's simple enough. > > No not in this case, we are scanning the state of all links to see if they > are on or not and turning off DMAs when all the links are off, so we can't > break at first match in this case. You need to check whether there is any link, not the number of links, right? Then it'd be like: link_up = false; list_for_each_entry(hlink, &ebus->hlink_list, list) { if (hlink->ref_count) { link_up = true; break; } } if (!link_up) { cmd_io_off(); } Takashi > Yes for other places where we find a link, first match break will help, I > will check that > > Thanks > -- > ~Vinod >
On Mon, May 09, 2016 at 10:10:53AM +0200, Takashi Iwai wrote: > > You need to check whether there is any link, not the number of links, > right? Then it'd be like: Yes thinking it from other way, I do agree with this. Will change it > > link_up = false; > list_for_each_entry(hlink, &ebus->hlink_list, list) { > if (hlink->ref_count) { > link_up = true; > break; > } > } > > if (!link_up) { > cmd_io_off(); > } > >
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 07fa59237feb..56fa65a1195d 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -14,6 +14,8 @@ * @gtscap: gts capabilities pointer * @drsmcap: dma resume capabilities pointer * @hlink_list: link list of HDA links + * @lock: lock for link mgmt + * @cmd_io: state of cmd_io */ struct hdac_ext_bus { struct hdac_bus bus; @@ -27,6 +29,9 @@ struct hdac_ext_bus { void __iomem *drsmcap; struct list_head hlink_list; + + struct mutex lock; + int cmd_io; }; int snd_hdac_ext_bus_init(struct hdac_ext_bus *sbus, struct device *dev, @@ -142,6 +147,9 @@ struct hdac_ext_link { void __iomem *ml_addr; /* link output stream reg pointer */ u32 lcaps; /* link capablities */ u16 lsdiid; /* link sdi identifier */ + + int ref_count; + struct list_head list; }; @@ -154,6 +162,11 @@ void snd_hdac_ext_link_set_stream_id(struct hdac_ext_link *link, void snd_hdac_ext_link_clear_stream_id(struct hdac_ext_link *link, int stream); +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, + struct hdac_ext_link *link); +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, + struct hdac_ext_link *link); + /* update register macro */ #define snd_hdac_updatel(addr, reg, mask, val) \ writel(((readl(addr + reg) & ~(mask)) | (val)), \ diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index 64de0a3d6d93..0f62b5498c3d 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -105,6 +105,9 @@ int snd_hdac_ext_bus_init(struct hdac_ext_bus *ebus, struct device *dev, INIT_LIST_HEAD(&ebus->hlink_list); ebus->idx = idx++; + mutex_init(&ebus->lock); + ebus->cmd_io = 1; + return 0; } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c index 548cc1e4114b..9b60624d3405 100644 --- a/sound/hda/ext/hdac_ext_controller.c +++ b/sound/hda/ext/hdac_ext_controller.c @@ -186,6 +186,9 @@ int snd_hdac_ext_bus_get_ml_capabilities(struct hdac_ext_bus *ebus) hlink->lcaps = readl(hlink->ml_addr + AZX_REG_ML_LCAP); hlink->lsdiid = readw(hlink->ml_addr + AZX_REG_ML_LSDIID); + /* since link in On, update the ref */ + hlink->ref_count = 1; + list_add_tail(&hlink->list, &ebus->hlink_list); } @@ -327,3 +330,62 @@ int snd_hdac_ext_bus_link_power_down_all(struct hdac_ext_bus *ebus) return 0; } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_power_down_all); + +int snd_hdac_ext_bus_link_get(struct hdac_ext_bus *ebus, + struct hdac_ext_link *link) +{ + int ret = 0; + + mutex_lock(&ebus->lock); + + /* + * if we move from 0 to 1, count will be 1 so power up this link + * as well, also check the dma status and trigger that + */ + if (++link->ref_count == 1) { + if (!ebus->cmd_io) { + snd_hdac_bus_init_cmd_io(&ebus->bus); + ebus->cmd_io = 1; + } + + ret = snd_hdac_ext_bus_link_power_up(link); + } + + mutex_unlock(&ebus->lock); + return ret; +} +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_get); + +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus, + struct hdac_ext_link *link) +{ + int ret = 0, state = 0; + struct hdac_ext_link *hlink = NULL; + + mutex_lock(&ebus->lock); + + /* + * if we move from 1 to 0, count will be 0 + * so power down this link as well + */ + if (--link->ref_count == 0) { + ret = snd_hdac_ext_bus_link_power_down(link); + + /* + * now check if all links are off, if so turn off + * cmd dma as well + */ + list_for_each_entry(hlink, &ebus->hlink_list, list) { + if (hlink->ref_count) + state++; + } + if (!state) { + snd_hdac_bus_stop_cmd_io(&ebus->bus); + ebus->cmd_io = 0; + } + } + + mutex_unlock(&ebus->lock); + return ret; +} +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_put);