Re: [PATCH v7 4/5] drm: Update bits per component for display info

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 11, 2016 at 10:22:27AM +0300, Ville Syrjälä wrote:
> On Mon, Aug 08, 2016 at 04:00:29PM +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
> > v3: Fill bpc for connector's display info in separate
> >     drm_dp_helper function (Daniel)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 24 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c |  3 +++
> >  include/drm/drm_dp_helper.h     |  4 ++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 1f36016..a2c46ed 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -514,6 +514,30 @@ int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  EXPORT_SYMBOL(drm_dp_downstream_max_bpc);
> >  
> >  /**
> > + * drm_dp_downstream_update_max_bpc() - extract branch device max
> > + *                                      bits per component and update
> > + *                                      connector max bpc
> > + * @dpcd: DisplayPort configuration data
> > + * @port_cap: port capabilities
> > + * @connector: Connector info
> > + * Returns current max bpc on success
> > + */
> > +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +				     const u8 port_cap[4],
> > +				     struct drm_connector *connector)
> > +{
> > +	if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT) {
> > +		int bpc = drm_dp_downstream_max_bpc(dpcd, port_cap);
> > +
> > +		if (bpc > 0)
> > +			connector->display_info.bpc = bpc;
> > +	}
> > +
> > +	return connector->display_info.bpc;
> > +}
> > +EXPORT_SYMBOL(drm_dp_downstream_update_max_bpc);
> > +
> > +/**
> >   * drm_dp_downstream_hw_rev() - read DP branch device HW revision
> >   * @aux: DisplayPort AUX channel
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e990c8b..763e2f5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3985,6 +3985,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >  	uint8_t *dpcd = intel_dp->dpcd;
> >  	uint8_t type;
> >  
> > +	drm_dp_downstream_update_max_bpc(dpcd, intel_dp->downstream_ports,
> > +					 &intel_dp->attached_connector->base);
> > +
> 
> And it will be promptly overwritten by the EDID parser. Like I said
> before, touching display_info from elsewhere is a bad idea right now.
> We need to lay out some real rules on how the EDID parser vs. DP
> helpers must be called if we are to share that information between
> the two.
> 
> For now, I would just utilize the DP helper in the compute_config
> to limit the bpc.

I think we need one dp helper which does _all_ the sink detection, and in
the right order. Edid reading dpcd decoding, all that. And then stuff is
all into relevant structures like display_info, but also some new (or well
I think we have parts of it already) dp_sink struct. Otherwise this will
stay fragile, especially once other drivers start using it.
-Daniel

> 
> >  	if (!intel_dp_get_dpcd(intel_dp))
> >  		return connector_status_disconnected;
> >  
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 45366aa..5491a9b 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/types.h>
> >  #include <linux/i2c.h>
> >  #include <linux/delay.h>
> > +#include <drm/drm_crtc.h>
> >  
> >  /*
> >   * Unless otherwise noted, all values are from the DP 1.1a spec.  Note that
> > @@ -827,6 +828,9 @@ int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  				const u8 port_cap[4]);
> >  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  			      const u8 port_cap[4]);
> > +int drm_dp_downstream_update_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> > +				     const u8 port_cap[4],
> > +				     struct drm_connector *connector);
> >  int drm_dp_downstream_id(struct drm_dp_aux *aux, char id[6]);
> >  int drm_dp_downstream_hw_rev(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> >  			     struct drm_dp_aux *aux, struct drm_dp_link *link);
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC

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