[1/2] drm/i915: fix get digital port issue in intel_audio

Submitted by libin.yang@linux.intel.com on Jan. 6, 2016, 2:26 a.m.

Details

Message ID 1452047202-103072-1-git-send-email-libin.yang@linux.intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

libin.yang@linux.intel.com Jan. 6, 2016, 2:26 a.m.
From: Libin Yang <libin.yang@linux.intel.com>

For DP MST, use enc_to_mst(encoder)->primary to get intel_digital_port,
instead of using enc_to_dig_port(encoder).

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 31f6d21..431487a0 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -187,6 +187,16 @@  static bool intel_eld_uptodate(struct drm_connector *connector,
 	return true;
 }
 
+static struct intel_digital_port *
+intel_encoder_to_dig_port(struct intel_encoder *intel_encoder)
+{
+	struct drm_encoder *encoder = &intel_encoder->base;
+
+	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
+		return enc_to_mst(encoder)->primary;
+	return enc_to_dig_port(encoder);
+}
+
 static void g4x_audio_codec_disable(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
@@ -286,7 +296,7 @@  static void hsw_audio_codec_enable(struct drm_connector *connector,
 	struct i915_audio_component *acomp = dev_priv->audio_component;
 	const uint8_t *eld = connector->eld;
 	struct intel_digital_port *intel_dig_port =
-		enc_to_dig_port(&encoder->base);
+		intel_encoder_to_dig_port(encoder);
 	enum port port = intel_dig_port->port;
 	uint32_t tmp;
 	int len, i;
@@ -500,7 +510,8 @@  void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_audio_component *acomp = dev_priv->audio_component;
-	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	struct intel_digital_port *intel_dig_port =
+		intel_encoder_to_dig_port(intel_encoder);
 	enum port port = intel_dig_port->port;
 
 	connector = drm_select_eld(encoder);
@@ -546,7 +557,8 @@  void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_audio_component *acomp = dev_priv->audio_component;
-	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	struct intel_digital_port *intel_dig_port =
+		intel_encoder_to_dig_port(intel_encoder);
 	enum port port = intel_dig_port->port;
 
 	if (dev_priv->display.audio_codec_disable)
@@ -724,7 +736,8 @@  static int i915_audio_component_get_eld(struct device *dev, int port,
 	/* intel_encoder might be NULL for DP MST */
 	if (intel_encoder) {
 		ret = 0;
-		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+		intel_dig_port =
+			intel_encoder_to_dig_port(intel_encoder);
 		*enabled = intel_dig_port->audio_connector != NULL;
 		if (*enabled) {
 			eld = intel_dig_port->audio_connector->eld;

Comments

On Wed, Jan 06, 2016 at 10:26:41AM +0800, libin.yang@linux.intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> For DP MST, use enc_to_mst(encoder)->primary to get intel_digital_port,
> instead of using enc_to_dig_port(encoder).
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 31f6d21..431487a0 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -187,6 +187,16 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
>  	return true;
>  }
>  
> +static struct intel_digital_port *
> +intel_encoder_to_dig_port(struct intel_encoder *intel_encoder)
> +{
> +	struct drm_encoder *encoder = &intel_encoder->base;
> +
> +	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> +		return enc_to_mst(encoder)->primary;
> +	return enc_to_dig_port(encoder);
> +}
> +
>  static void g4x_audio_codec_disable(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -286,7 +296,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>  	const uint8_t *eld = connector->eld;
>  	struct intel_digital_port *intel_dig_port =
> -		enc_to_dig_port(&encoder->base);
> +		intel_encoder_to_dig_port(encoder);

This hunk makes sense since we just look at intel_dig_port->port. Might
make sense to entirely eliminate the local inte_dig_port variable from
these hooks so that there's no confusion whether it points at the fake
or primary encoder.

>  	enum port port = intel_dig_port->port;
>  	uint32_t tmp;
>  	int len, i;
> @@ -500,7 +510,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_audio_component *acomp = dev_priv->audio_component;
> -	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> +	struct intel_digital_port *intel_dig_port =
> +		intel_encoder_to_dig_port(intel_encoder);
>  	enum port port = intel_dig_port->port;
>  
>  	connector = drm_select_eld(encoder);
> @@ -546,7 +557,8 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_audio_component *acomp = dev_priv->audio_component;
> -	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> +	struct intel_digital_port *intel_dig_port =
> +		intel_encoder_to_dig_port(intel_encoder);

>  	enum port port = intel_dig_port->port;
>  
>  	if (dev_priv->display.audio_codec_disable)
> @@ -724,7 +736,8 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  	/* intel_encoder might be NULL for DP MST */
>  	if (intel_encoder) {
>  		ret = 0;
> -		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> +		intel_dig_port =
> +			intel_encoder_to_dig_port(intel_encoder);
>  		*enabled = intel_dig_port->audio_connector != NULL;

These not so much. We'd clobber 'intel_dig_port->audio_connector' for
the primary encoder whenever a new MST stream is enabled.

Was the intention just to fix the port information passed to
.pin_eld_notify()?

This whole thing seems highlight a rather big issue with the current
component stuff; How do you tell the streams apart when all are using
the same DDI port?

>  		if (*enabled) {
>  			eld = intel_dig_port->audio_connector->eld;
> -- 
> 1.9.1
Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, January 08, 2016 12:50 AM
> To: libin.yang@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com;
> jani.nikula@linux.intel.com; Vetter, Daniel; tiwai@suse.de; Yang, Libin
> Subject: Re: [PATCH 1/2] drm/i915: fix get digital port issue in intel_audio
> 
> On Wed, Jan 06, 2016 at 10:26:41AM +0800, libin.yang@linux.intel.com
> wrote:
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > For DP MST, use enc_to_mst(encoder)->primary to get
> intel_digital_port,
> > instead of using enc_to_dig_port(encoder).
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index 31f6d21..431487a0 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -187,6 +187,16 @@ static bool intel_eld_uptodate(struct
> drm_connector *connector,
> >  	return true;
> >  }
> >
> > +static struct intel_digital_port *
> > +intel_encoder_to_dig_port(struct intel_encoder *intel_encoder)
> > +{
> > +	struct drm_encoder *encoder = &intel_encoder->base;
> > +
> > +	if (intel_encoder->type == INTEL_OUTPUT_DP_MST)
> > +		return enc_to_mst(encoder)->primary;
> > +	return enc_to_dig_port(encoder);
> > +}
> > +
> >  static void g4x_audio_codec_disable(struct intel_encoder *encoder)
> >  {
> >  	struct drm_i915_private *dev_priv = encoder->base.dev-
> >dev_private;
> > @@ -286,7 +296,7 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	struct i915_audio_component *acomp = dev_priv-
> >audio_component;
> >  	const uint8_t *eld = connector->eld;
> >  	struct intel_digital_port *intel_dig_port =
> > -		enc_to_dig_port(&encoder->base);
> > +		intel_encoder_to_dig_port(encoder);
> 
> This hunk makes sense since we just look at intel_dig_port->port. Might
> make sense to entirely eliminate the local inte_dig_port variable from
> these hooks so that there's no confusion whether it points at the fake
> or primary encoder.

Do you mean we can still use enc_to_dig_port()? Maybe the new code
is better. What do you think we use the wrapper
intel_encoder_to_dig_port() here?

> 
> >  	enum port port = intel_dig_port->port;
> >  	uint32_t tmp;
> >  	int len, i;
> > @@ -500,7 +510,8 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder)
> >  	struct drm_device *dev = encoder->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct i915_audio_component *acomp = dev_priv-
> >audio_component;
> > -	struct intel_digital_port *intel_dig_port =
> enc_to_dig_port(encoder);
> > +	struct intel_digital_port *intel_dig_port =
> > +		intel_encoder_to_dig_port(intel_encoder);
> >  	enum port port = intel_dig_port->port;
> >
> >  	connector = drm_select_eld(encoder);
> > @@ -546,7 +557,8 @@ void intel_audio_codec_disable(struct
> intel_encoder *intel_encoder)
> >  	struct drm_device *dev = encoder->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct i915_audio_component *acomp = dev_priv-
> >audio_component;
> > -	struct intel_digital_port *intel_dig_port =
> enc_to_dig_port(encoder);
> > +	struct intel_digital_port *intel_dig_port =
> > +		intel_encoder_to_dig_port(intel_encoder);
> 
> >  	enum port port = intel_dig_port->port;
> >
> >  	if (dev_priv->display.audio_codec_disable)
> > @@ -724,7 +736,8 @@ static int
> i915_audio_component_get_eld(struct device *dev, int port,
> >  	/* intel_encoder might be NULL for DP MST */
> >  	if (intel_encoder) {
> >  		ret = 0;
> > -		intel_dig_port = enc_to_dig_port(&intel_encoder-
> >base);
> > +		intel_dig_port =
> > +			intel_encoder_to_dig_port(intel_encoder);
> >  		*enabled = intel_dig_port->audio_connector != NULL;
> 
> These not so much. We'd clobber 'intel_dig_port->audio_connector' for
> the primary encoder whenever a new MST stream is enabled.

Yes. If we are using i915_audio_component_get_eld() for MST audio,
we need a parameter device_entry_id in the function. I remember
David has sent a patch to support device entry before. But MST was
not supported and he removed the device_entry_id parameter.

> 
> Was the intention just to fix the port information passed to
> .pin_eld_notify()?

No. It's based on device entry.

> 
> This whole thing seems highlight a rather big issue with the current
> component stuff; How do you tell the streams apart when all are using
> the same DDI port?

If we need support other device entries, we need get
the other ports besides primary port. Do you know how
to get the ports?

As Takashi has changed to get eld_info from unsol_event to using this
callback, it seems this is a must to support MST audio.

> 
> >  		if (*enabled) {
> >  			eld = intel_dig_port->audio_connector->eld;
> > --
> > 1.9.1
> 
> --
> Ville Syrjälä
> Intel OTC