[PATCH v2 0/3] ASoC: Intel: Skylake: Add HDA link management

[PATCH v2 0/3] ASoC: Intel: Skylake: Add HDA link management

[Vinod Koul]

HDA links are always kept On, this patch adds link management
routines in hdac extended core and it's use in Skylake driver and
hdmi codec (runs on HDA link)

The aim of serial is to shut down the links when not in use and
keep them active when a user is active. The user can activate
link by taking reference to the link when required.

When all links are closed, the command DMAs are turned off as
well. On first link power up, the command DMAs are turned on
first.

First patch touches HDA core, and rest two are dependent on it
so Takashi if you are okay, pls ACK it and we can merge thru ASoC
tree

Changes in v2:
 - use mutex instead of spinlock


Vinod Koul (3):
  ALSA: hdac: add link pm and ref counting
  ASoC: Intel: Skylake: add link management
  ASoC: hdac_hdmi: add link management

 include/sound/hdaudio_ext.h         | 13 ++++++++
 sound/hda/ext/hdac_ext_bus.c        |  3 ++
 sound/hda/ext/hdac_ext_controller.c | 62 +++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/hdac_hdmi.c        | 32 +++++++++++++++++--
 sound/soc/intel/skylake/skl-pcm.c   |  1 -
 sound/soc/intel/skylake/skl.c       | 34 ++++++++++++++++++++
 6 files changed, 142 insertions(+), 3 deletions(-)

-- 
1.9.1

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

[Vinod Koul]

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

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

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

[Vinod Koul]

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
> 

-- 
~Vinod

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

[Takashi Iwai]

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
> 

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

[Vinod Koul]

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
-- 
~Vinod

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

[Takashi Iwai]

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
> 

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

[Vinod Koul]

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();
> 	}
> 
> 

-- 
~Vinod

[PATCH v2 2/3] ASoC: Intel: Skylake: add link management

[Vinod Koul]

Use shiny new link APIs to manage the links. Also remove old link
configuration logic from driver.

We need to keep link and cmd dma to off during active suspend
to allow system to enter low power state and turn it on if
the link and cmd dma was on before active suspend in active
resume.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c |  1 -
 sound/soc/intel/skylake/skl.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index c726937321ef..6e71bf34ea2b 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -533,7 +533,6 @@ static int skl_link_pcm_prepare(struct snd_pcm_substream *substream,
 	if (!link)
 		return -EINVAL;
 
-	snd_hdac_ext_bus_link_power_up(link);
 	snd_hdac_ext_link_stream_reset(link_dev);
 
 	snd_hdac_ext_link_stream_setup(link_dev, format_val);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 83e985c0c0c9..c1235ce4ea10 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -229,7 +229,12 @@ static int skl_suspend(struct device *dev)
 	 * running, we need to save the state for these and continue
 	 */
 	if (skl->supend_active) {
+		/* turn off the links and stop the CORB/RIRB DMA if it is On */
 		snd_hdac_ext_bus_link_power_down_all(ebus);
+
+		if (ebus->cmd_io)
+			snd_hdac_bus_stop_cmd_io(&ebus->bus);
+
 		enable_irq_wake(bus->irq);
 		pci_save_state(pci);
 		pci_disable_device(pci);
@@ -255,6 +260,7 @@ static int skl_resume(struct device *dev)
 	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
 	struct skl *skl  = ebus_to_skl(ebus);
 	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
 	/* Turned OFF in HDMI codec driver after codec reconfiguration */
@@ -276,8 +282,29 @@ static int skl_resume(struct device *dev)
 		ret = pci_enable_device(pci);
 		snd_hdac_ext_bus_link_power_up_all(ebus);
 		disable_irq_wake(bus->irq);
+		/*
+		 * turn On the links which are On before active suspend
+		 * and start the CORB/RIRB DMA if On before
+		 * active suspend.
+		 */
+		list_for_each_entry(hlink, &ebus->hlink_list, list) {
+			if (hlink->ref_count)
+				snd_hdac_ext_bus_link_power_up(hlink);
+		}
+
+		if (ebus->cmd_io)
+			snd_hdac_bus_init_cmd_io(&ebus->bus);
 	} else {
 		ret = _skl_resume(ebus);
+
+		/* turn off the links which are off before suspend */
+		list_for_each_entry(hlink, &ebus->hlink_list, list) {
+			if (!hlink->ref_count)
+				snd_hdac_ext_bus_link_power_down(hlink);
+		}
+
+		if (!ebus->cmd_io)
+			snd_hdac_bus_stop_cmd_io(&ebus->bus);
 	}
 
 	return ret;
@@ -613,6 +640,7 @@ static int skl_probe(struct pci_dev *pci,
 	struct skl *skl;
 	struct hdac_ext_bus *ebus = NULL;
 	struct hdac_bus *bus = NULL;
+	struct hdac_ext_link *hlink = NULL;
 	int err;
 
 	/* we use ext core ops, so provide NULL for ops here */
@@ -679,6 +707,12 @@ static int skl_probe(struct pci_dev *pci,
 		}
 	}
 
+	/*
+	 * we are done probling so decrement link counts
+	 */
+	list_for_each_entry(hlink, &ebus->hlink_list, list)
+		snd_hdac_ext_bus_link_put(ebus, hlink);
+
 	/*configure PM */
 	pm_runtime_put_noidle(bus->dev);
 	pm_runtime_allow(bus->dev);
-- 
1.9.1

[PATCH v2 3/3] ASoC: hdac_hdmi: add link management

[Vinod Koul]

Manage the hda idisp link using shiny new link APIs.  We need to
keep link On while we probe and also hold the reference in runtime
resume and drop in suspend

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 7e7ff1b708f4..37332976ccbb 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1464,10 +1464,18 @@ static int hdmi_codec_probe(struct snd_soc_codec *codec)
 	struct snd_soc_dapm_context *dapm =
 		snd_soc_component_get_dapm(&codec->component);
 	struct hdac_hdmi_pin *pin;
+	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
 	edev->scodec = codec;
 
+	/*
+	 * hold the ref while we probe, also no need to drop the ref on
+	 * exit, we call pm_runtime_suspend() so that will do for us
+	 */
+	hlink = snd_hdac_ext_bus_get_link(edev->ebus, dev_name(&edev->hdac.dev));
+	snd_hdac_ext_bus_link_get(edev->ebus, hlink);
+
 	ret = create_fill_widget_route_map(dapm);
 	if (ret < 0)
 		return ret;
@@ -1620,9 +1628,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	struct hdac_device *codec = &edev->hdac;
 	struct hdac_hdmi_priv *hdmi_priv;
 	struct snd_soc_dai_driver *hdmi_dais = NULL;
+	struct hdac_ext_link *hlink = NULL;
 	int num_dais = 0;
 	int ret = 0;
 
+	/* hold the ref while we probe */
+	hlink = snd_hdac_ext_bus_get_link(edev->ebus, dev_name(&edev->hdac.dev));
+	snd_hdac_ext_bus_link_get(edev->ebus, hlink);
+
 	hdmi_priv = devm_kzalloc(&codec->dev, sizeof(*hdmi_priv), GFP_KERNEL);
 	if (hdmi_priv == NULL)
 		return -ENOMEM;
@@ -1661,8 +1674,12 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	}
 
 	/* ASoC specific initialization */
-	return snd_soc_register_codec(&codec->dev, &hdmi_hda_codec,
-			hdmi_dais, num_dais);
+	ret = snd_soc_register_codec(&codec->dev, &hdmi_hda_codec,
+					hdmi_dais, num_dais);
+
+	snd_hdac_ext_bus_link_put(edev->ebus, hlink);
+
+	return ret;
 }
 
 static int hdac_hdmi_dev_remove(struct hdac_ext_device *edev)
@@ -1701,6 +1718,9 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	struct hdac_ext_device *edev = to_hda_ext_device(dev);
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_bus *bus = hdac->bus;
+	unsigned long timeout;
+	struct hdac_ext_bus *ebus = hbus_to_ebus(bus);
+	struct hdac_ext_link *hlink = NULL;
 	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
@@ -1724,6 +1744,9 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 		return err;
 	}
 
+	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
+	snd_hdac_ext_bus_link_put(ebus, hlink);
+
 	return 0;
 }
 
@@ -1732,6 +1755,8 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	struct hdac_ext_device *edev = to_hda_ext_device(dev);
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_bus *bus = hdac->bus;
+	struct hdac_ext_bus *ebus = hbus_to_ebus(bus);
+	struct hdac_ext_link *hlink = NULL;
 	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
@@ -1740,6 +1765,9 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	if (!bus)
 		return 0;
 
+	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
+	snd_hdac_ext_bus_link_get(ebus, hlink);
+
 	err = snd_hdac_display_power(bus, true);
 	if (err < 0) {
 		dev_err(bus->dev, "Cannot turn on display power on i915\n");
-- 
1.9.1

Applied "ASoC: hdac_hdmi: add link management" to the asoc tree

[Mark Brown]

The patch

   ASoC: hdac_hdmi: add link management

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From b2047e996cd88d36eb0f4e84fe6aedab831a4b31 Mon Sep 17 00:00:00 2001
From: Vinod Koul <vinod.koul@intel.com>
Date: Thu, 12 May 2016 08:58:55 +0530
Subject: [PATCH] ASoC: hdac_hdmi: add link management

Manage the hda idisp link using shiny new link APIs.  We need to
keep link On while we probe and also hold the reference in runtime
resume and drop in suspend

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/hdac_hdmi.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index aaa038ffc8a5..13002f33384e 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1378,10 +1378,18 @@ static int hdmi_codec_probe(struct snd_soc_codec *codec)
 	struct snd_soc_dapm_context *dapm =
 		snd_soc_component_get_dapm(&codec->component);
 	struct hdac_hdmi_pin *pin;
+	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
 	edev->scodec = codec;
 
+	/*
+	 * hold the ref while we probe, also no need to drop the ref on
+	 * exit, we call pm_runtime_suspend() so that will do for us
+	 */
+	hlink = snd_hdac_ext_bus_get_link(edev->ebus, dev_name(&edev->hdac.dev));
+	snd_hdac_ext_bus_link_get(edev->ebus, hlink);
+
 	ret = create_fill_widget_route_map(dapm);
 	if (ret < 0)
 		return ret;
@@ -1480,9 +1488,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	struct hdac_device *codec = &edev->hdac;
 	struct hdac_hdmi_priv *hdmi_priv;
 	struct snd_soc_dai_driver *hdmi_dais = NULL;
+	struct hdac_ext_link *hlink = NULL;
 	int num_dais = 0;
 	int ret = 0;
 
+	/* hold the ref while we probe */
+	hlink = snd_hdac_ext_bus_get_link(edev->ebus, dev_name(&edev->hdac.dev));
+	snd_hdac_ext_bus_link_get(edev->ebus, hlink);
+
 	hdmi_priv = devm_kzalloc(&codec->dev, sizeof(*hdmi_priv), GFP_KERNEL);
 	if (hdmi_priv == NULL)
 		return -ENOMEM;
@@ -1516,8 +1529,12 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	}
 
 	/* ASoC specific initialization */
-	return snd_soc_register_codec(&codec->dev, &hdmi_hda_codec,
-			hdmi_dais, num_dais);
+	ret = snd_soc_register_codec(&codec->dev, &hdmi_hda_codec,
+					hdmi_dais, num_dais);
+
+	snd_hdac_ext_bus_link_put(edev->ebus, hlink);
+
+	return ret;
 }
 
 static int hdac_hdmi_dev_remove(struct hdac_ext_device *edev)
@@ -1556,6 +1573,9 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	struct hdac_ext_device *edev = to_hda_ext_device(dev);
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_bus *bus = hdac->bus;
+	unsigned long timeout;
+	struct hdac_ext_bus *ebus = hbus_to_ebus(bus);
+	struct hdac_ext_link *hlink = NULL;
 	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
@@ -1579,6 +1599,9 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 		return err;
 	}
 
+	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
+	snd_hdac_ext_bus_link_put(ebus, hlink);
+
 	return 0;
 }
 
@@ -1587,6 +1610,8 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	struct hdac_ext_device *edev = to_hda_ext_device(dev);
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_bus *bus = hdac->bus;
+	struct hdac_ext_bus *ebus = hbus_to_ebus(bus);
+	struct hdac_ext_link *hlink = NULL;
 	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
@@ -1595,6 +1620,9 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	if (!bus)
 		return 0;
 
+	hlink = snd_hdac_ext_bus_get_link(ebus, dev_name(dev));
+	snd_hdac_ext_bus_link_get(ebus, hlink);
+
 	err = snd_hdac_display_power(bus, true);
 	if (err < 0) {
 		dev_err(bus->dev, "Cannot turn on display power on i915\n");
-- 
2.8.1