Re: [PATCH v2 1/3] ALSA: hdac: add link pm and ref counting

[Takashi Iwai <tiwai@suse.de>]

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
>