Re: [PATCH v6 09/10] drm/i915: Update bits per component for display info

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 02, 2016 at 02:41:25PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 02:23:13PM +0300, Mika Kahola wrote:
> > On Tue, 2016-07-12 at 15:51 +0200, Daniel Vetter wrote:
> > > On Wed, Jul 06, 2016 at 02:04:53PM +0300, Mika Kahola wrote:
> > > > DisplayPort branch device may define max supported bits per
> > > > component. Update display info based on this value if bpc
> > > > is defined.
> > > > 
> > > > v2: cleanup to match the drm_dp_helper.c patches introduced
> > > >     earlier in this series
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 76a654e..53ec844 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3932,6 +3932,14 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> > > >  	uint8_t *dpcd = intel_dp->dpcd;
> > > >  	uint8_t type;
> > > >  
> > > > +	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
> > > > +		int bpc = drm_dp_downstream_max_bpc(dpcd,
> > > > +						    intel_dp->downstream_ports);
> > > > +
> > > > +		if (bpc > 0)
> > > > +			intel_dp->attached_connector->base.display_info.bpc = bpc;
> > > > +	}
> > > 
> > > I think a function in the dp helpers to correctly fill out the connector's
> > > display info would be neater.
> > > -Daniel
> > Ok. I can move this stuff to be part of dp helper routine.
> 
> I'm not sure frobbing with display_info is a good idea here since that
> that one has so far been the EDID parsers domain. The ordering between
> all this stuff is already very poorly defined/buggy, so some bigger
> cleanup is probably needed. I have some patches to the EDID parser that
> I should send out that at least make it a bit clearer where it fills out
> the display_info stuff.

+1 for bigger cleanup. A helper which does all the dp sink detection
(grabs edid, dpcd and everything else and then computes restrictions)
would be _real_ awesome I think. Even better if you can trick some other
driver into using it too.

Tomeu from collabora just volunteered himself to clean up the analogix dp
support and switch over to the helpers. He's rather active on irc, so
great guinea-pig to volutneer for this and work together with.
-Daniel

> 
> > Cheers,
> > Mika
> > > 
> > > > +
> > > >  	if (!intel_dp_get_dpcd(intel_dp))
> > > >  		return connector_status_disconnected;
> > > >  
> > > > @@ -3968,6 +3976,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> > > >  			return connector_status_unknown;
> > > >  	}
> > > >  
> > > > +
> > > >  	/* Anything else is out of spec, warn and ignore */
> > > >  	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
> > > >  	return connector_status_disconnected;
> > > > -- 
> > > > 1.9.1
> > > > 
> > > 
> > 
> > -- 
> > Mika Kahola - Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel