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@xxxxxxxxx>
> > Signed-off-by: Vinod Koul <vinod.koul@xxxxxxxxx>
> > ---
> > 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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
