Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > For eDP panels all the DPCD and EDID data is cached when
> > > > initializing
> > > > the eDP connector so in futher detection it do not call
> > > > intel_dp_detect_dpcd() for eDP.
> > > > The problem is on the first short pulse interruption it calls
> > > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > > sink
> > > > count, causing a mismatch between old sink count and the new one
> > > > triggering a full detection without needed.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > >  		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > > +	u8 val;
> > > >  
> > > >  	/* this function is meant to be called only once */
> > > >  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  	intel_dp_set_common_rates(intel_dp);
> > > >  
> > > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > > 0)
> > > > +		return false;
> > > > +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > > 
> > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > compare
> > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > > eDP.
> > 
> > I'm not sure as DP specs for DP_SINK_COUNT says:
> > 
> > The Sink device shall add one more if it has a local Rendering
> > Function.
> > 
> > and eDP spec do not redefine or alter this, so I guess is more safe
> > also read for eDP too.
> > 
> 
> We already special case eDP in several places, for example, don't
> update link rates from the short pulse handler etc. And also don't
> support hotplug, I don't see a point.

IIRC some conformance test or something required that we read this.
I guess what we could do is still read it but just not update
intel_dp->sink_count. We already seem to have a special case which
ignores a zero sink_count on eDP. Might as well extend that a bit
I suppose.

In general I think special cases are bad, so IMO we should try
hard not add more unless really necessary. In this case it seems
the special case is warranted. Unfortunately commit 1034ce707b57
("drm/i915: Fixing eDP detection on certain platforms") failed to add
a comment explaining why. I'd appreciate if someone could add that
comment now so that we don't forget this in the future.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx