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

["Yang, Libin" <libin.yang@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx