diff mbox

[v2] ASoC: hdac_hdmi: Program pin-port-mux during jack report event

Message ID 1531290643-29312-1-git-send-email-sriramx.periyasamy@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sriram Periyasamy July 11, 2018, 6:30 a.m. UTC 
During d3/d0 cycle, the connection selection index of all pins points
to the default value. This needs to be restored to ensure audio is
restored after d3/d0 cycle.

So store the connection selection index and program it during jack
report event which gets invoked in cases like d3/d0 cycle, hot plug
detection when multiple displays are connected.

Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
---

v1 -> v2:
	- Runtime resume the device explicitly in order to use
	  snd_hdac_codec_write()

 sound/soc/codecs/hdac_hdmi.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Takashi Iwai July 11, 2018, 6:45 a.m. UTC | #1 
On Wed, 11 Jul 2018 08:30:43 +0200,
Sriram Periyasamy wrote:
> 
> @@ -167,6 +171,30 @@ static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
>  
>  	if (is_connect) {
>  		/*
> +		 * Increment the usage count to ensure that the device
> +		 * is runtime active to send verb across.
> +		 */
> +		pm_runtime_get_sync(&edev->hdev.dev);

This looks more reasonable now, but...

> +		/* set the device if pin is mst_capable */
> +		if (hdac_hdmi_port_select_set(edev, port) < 0) {
> +			dev_err(&edev->hdev.dev,
> +				"port %d device select fail\n", port->id);
> +			return;

It appears like an unbalanced pm refcount here in the error path.

> +		}
> +
> +		/*
> +		 * Restore the connection selection index of the
> +		 * respective pin.
> +		 */
> +		if (port->pin->conn_index > 0)
> +			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
> +						0, AC_VERB_SET_CONNECT_SEL,
> +						port->pin->conn_index - 1);

And, here checks conn_index > 0 while...

> @@ -903,6 +931,9 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
>  		}
>  	}
>  
> +	if (ucontrol->value.enumerated.item[0] > 0)
> +		port->pin->conn_index =	ucontrol->value.enumerated.item[0];

... conn_index is set only non-zero here.

That is, once after a non-zero is passed, conn_index can't any longer
back to zero.  I guess it's not intentional?


thanks,

Takashi
Sriram Periyasamy July 11, 2018, 9:25 a.m. UTC | #2 
On Wed, Jul 11, 2018 at 08:45:05AM +0200, Takashi Iwai wrote:
> On Wed, 11 Jul 2018 08:30:43 +0200,
> Sriram Periyasamy wrote:
> > 
> > @@ -167,6 +171,30 @@ static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
> >  
> >  	if (is_connect) {
> >  		/*
> > +		 * Increment the usage count to ensure that the device
> > +		 * is runtime active to send verb across.
> > +		 */
> > +		pm_runtime_get_sync(&edev->hdev.dev);
> 
> This looks more reasonable now, but...
> 
> > +		/* set the device if pin is mst_capable */
> > +		if (hdac_hdmi_port_select_set(edev, port) < 0) {
> > +			dev_err(&edev->hdev.dev,
> > +				"port %d device select fail\n", port->id);
> > +			return;
> 
> It appears like an unbalanced pm refcount here in the error path.
> 

Thanks for pointing out. Will fix it.

> > +		}
> > +
> > +		/*
> > +		 * Restore the connection selection index of the
> > +		 * respective pin.
> > +		 */
> > +		if (port->pin->conn_index > 0)
> > +			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
> > +						0, AC_VERB_SET_CONNECT_SEL,
> > +						port->pin->conn_index - 1);
> 
> And, here checks conn_index > 0 while...
> 
> > @@ -903,6 +931,9 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
> >  		}
> >  	}
> >  
> > +	if (ucontrol->value.enumerated.item[0] > 0)
> > +		port->pin->conn_index =	ucontrol->value.enumerated.item[0];
> 
> ... conn_index is set only non-zero here.
> 
> That is, once after a non-zero is passed, conn_index can't any longer
> back to zero.  I guess it's not intentional?
> 

No, it is intentional. For example, two ports are connected to the
display and user land set mux only for one port. Hence other port's
conn_index is set to default 0. When jack report happens for that port,
we would be writing invalid connection select index which will lead to
undefined hardware behaviour as per the HDA spec.

Though it is userland's mistake, it is better to take care in the driver.

Thanks,
Sriram.
Takashi Iwai July 11, 2018, 10:03 a.m. UTC | #3 
On Wed, 11 Jul 2018 11:25:46 +0200,
Sriram Periyasamy wrote:
> 
> On Wed, Jul 11, 2018 at 08:45:05AM +0200, Takashi Iwai wrote:
> > On Wed, 11 Jul 2018 08:30:43 +0200,
> > Sriram Periyasamy wrote:
> > > +		}
> > > +
> > > +		/*
> > > +		 * Restore the connection selection index of the
> > > +		 * respective pin.
> > > +		 */
> > > +		if (port->pin->conn_index > 0)
> > > +			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
> > > +						0, AC_VERB_SET_CONNECT_SEL,
> > > +						port->pin->conn_index - 1);
> > 
> > And, here checks conn_index > 0 while...
> > 
> > > @@ -903,6 +931,9 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
> > >  		}
> > >  	}
> > >  
> > > +	if (ucontrol->value.enumerated.item[0] > 0)
> > > +		port->pin->conn_index =	ucontrol->value.enumerated.item[0];
> > 
> > ... conn_index is set only non-zero here.
> > 
> > That is, once after a non-zero is passed, conn_index can't any longer
> > back to zero.  I guess it's not intentional?
> > 
> 
> No, it is intentional. For example, two ports are connected to the
> display and user land set mux only for one port. Hence other port's
> conn_index is set to default 0. When jack report happens for that port,
> we would be writing invalid connection select index which will lead to
> undefined hardware behaviour as per the HDA spec.
> 
> Though it is userland's mistake, it is better to take care in the driver.

Then wouldn't it be better to remember the last set value in
hdac_hdmi_pin_mux_widget_event()?  The purpose of the patch is to
restore the previous state.


thanks,

Takashi
Sriram Periyasamy July 11, 2018, 10:56 a.m. UTC | #4 
On Wed, Jul 11, 2018 at 12:03:14PM +0200, Takashi Iwai wrote:
> On Wed, 11 Jul 2018 11:25:46 +0200,
> Sriram Periyasamy wrote:
> > 
> > On Wed, Jul 11, 2018 at 08:45:05AM +0200, Takashi Iwai wrote:
> > > On Wed, 11 Jul 2018 08:30:43 +0200,
> > > Sriram Periyasamy wrote:
> > > > +		if (port->pin->conn_index > 0)
> > > > +			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
> > > > +						0, AC_VERB_SET_CONNECT_SEL,
> > > > +						port->pin->conn_index - 1);
> > > 
> > > And, here checks conn_index > 0 while...
> > > 
> > > > @@ -903,6 +931,9 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	if (ucontrol->value.enumerated.item[0] > 0)
> > > > +		port->pin->conn_index =	ucontrol->value.enumerated.item[0];
> > > 
> > > ... conn_index is set only non-zero here.
> > > 
> > > That is, once after a non-zero is passed, conn_index can't any longer
> > > back to zero.  I guess it's not intentional?
> > > 
> > 
> > No, it is intentional. For example, two ports are connected to the
> > display and user land set mux only for one port. Hence other port's
> > conn_index is set to default 0. When jack report happens for that port,
> > we would be writing invalid connection select index which will lead to
> > undefined hardware behaviour as per the HDA spec.
> > 
> > Though it is userland's mistake, it is better to take care in the driver.
> 
> Then wouldn't it be better to remember the last set value in
> hdac_hdmi_pin_mux_widget_event()?  The purpose of the patch is to
> restore the previous state.
> 

Since hdac_hdmi_pin_mux_widget_event() invoked only for current playback
pin-port-mux path, we wouldn't restore other port's indices. So
hdac_hdmi_jack_report() chosen which will restore other ports also in
all use cases.

Thanks,
Sriram.
Takashi Iwai July 11, 2018, 11:06 a.m. UTC | #5 
On Wed, 11 Jul 2018 12:56:03 +0200,
Sriram Periyasamy wrote:
> 
> On Wed, Jul 11, 2018 at 12:03:14PM +0200, Takashi Iwai wrote:
> > On Wed, 11 Jul 2018 11:25:46 +0200,
> > Sriram Periyasamy wrote:
> > > 
> > > On Wed, Jul 11, 2018 at 08:45:05AM +0200, Takashi Iwai wrote:
> > > > On Wed, 11 Jul 2018 08:30:43 +0200,
> > > > Sriram Periyasamy wrote:
> > > > > +		if (port->pin->conn_index > 0)
> > > > > +			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
> > > > > +						0, AC_VERB_SET_CONNECT_SEL,
> > > > > +						port->pin->conn_index - 1);
> > > > 
> > > > And, here checks conn_index > 0 while...
> > > > 
> > > > > @@ -903,6 +931,9 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	if (ucontrol->value.enumerated.item[0] > 0)
> > > > > +		port->pin->conn_index =	ucontrol->value.enumerated.item[0];
> > > > 
> > > > ... conn_index is set only non-zero here.
> > > > 
> > > > That is, once after a non-zero is passed, conn_index can't any longer
> > > > back to zero.  I guess it's not intentional?
> > > > 
> > > 
> > > No, it is intentional. For example, two ports are connected to the
> > > display and user land set mux only for one port. Hence other port's
> > > conn_index is set to default 0. When jack report happens for that port,
> > > we would be writing invalid connection select index which will lead to
> > > undefined hardware behaviour as per the HDA spec.
> > > 
> > > Though it is userland's mistake, it is better to take care in the driver.
> > 
> > Then wouldn't it be better to remember the last set value in
> > hdac_hdmi_pin_mux_widget_event()?  The purpose of the patch is to
> > restore the previous state.
> > 
> 
> Since hdac_hdmi_pin_mux_widget_event() invoked only for current playback
> pin-port-mux path, we wouldn't restore other port's indices. So
> hdac_hdmi_jack_report() chosen which will restore other ports also in
> all use cases.

Hm, it sounds like really more black magic behind the scene.
Please give more descriptions to the patch and/or codes.


Takashi
diff mbox

Patch

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 84f7a7a36e4b..dd5863830eef 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -83,6 +83,7 @@  struct hdac_hdmi_pin {
 	struct list_head head;
 	hda_nid_t nid;
 	bool mst_capable;
+	int conn_index;
 	struct hdac_hdmi_port *ports;
 	int num_ports;
 	struct hdac_ext_device *edev;
@@ -141,6 +142,9 @@  struct hdac_hdmi_priv {
 
 #define hdev_to_hdmi_priv(_hdev) ((to_ehdac_device(_hdev))->private_data)
 
+static int hdac_hdmi_port_select_set(struct hdac_ext_device *edev,
+					struct hdac_hdmi_port *port);
+
 static struct hdac_hdmi_pcm *
 hdac_hdmi_get_pcm_from_cvt(struct hdac_hdmi_priv *hdmi,
 			   struct hdac_hdmi_cvt *cvt)
@@ -167,6 +171,30 @@  static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
 
 	if (is_connect) {
 		/*
+		 * Increment the usage count to ensure that the device
+		 * is runtime active to send verb across.
+		 */
+		pm_runtime_get_sync(&edev->hdev.dev);
+
+		/* set the device if pin is mst_capable */
+		if (hdac_hdmi_port_select_set(edev, port) < 0) {
+			dev_err(&edev->hdev.dev,
+				"port %d device select fail\n", port->id);
+			return;
+		}
+
+		/*
+		 * Restore the connection selection index of the
+		 * respective pin.
+		 */
+		if (port->pin->conn_index > 0)
+			snd_hdac_codec_write(&edev->hdev, port->pin->nid,
+						0, AC_VERB_SET_CONNECT_SEL,
+						port->pin->conn_index - 1);
+
+		pm_runtime_put_sync(&edev->hdev.dev);
+
+		/*
 		 * Report Jack connect event when a device is connected
 		 * for the first time where same PCM is attached to multiple
 		 * ports.
@@ -903,6 +931,9 @@  static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol,
 		}
 	}
 
+	if (ucontrol->value.enumerated.item[0] > 0)
+		port->pin->conn_index =	ucontrol->value.enumerated.item[0];
+
 	/*
 	 * Jack status is not reported during device probe as the
 	 * PCMs are not registered by then. So report it here.