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

Daniel Vetter
2016-08-11T08:53:42Z

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