Re: [Intel-gfx] [v3] drm/i915/display: Extend DP HDR support to hsw+

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

On Thu, Mar 24, 2022 at 11:58:15AM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Thursday, March 24, 2022 5:12 PM
> > To: Shankar, Uma <uma.shankar@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [v3] drm/i915/display: Extend DP HDR support to hsw+
> > 
> > On Thu, Mar 24, 2022 at 04:39:59PM +0530, Uma Shankar wrote:
> > > HSW+ platforms are able to send out HDR Metadata SDP DIP
> > > packet as GMP. Hence, extending the support for HDR on DP encoders for
> > > the same.
> > >
> > > v2: Limited to non eDP ports on hsw/bdw and removed it for lspcon as
> > > it is done separately (suggested by Ville)
> > >
> > > v3: Added helper and limited eDP restriction to port A (Ville)
> > >
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5389
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 9e19165fd175..c993d82c7ec9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -4913,6 +4913,26 @@ bool intel_dp_is_port_edp(struct drm_i915_private
> > *dev_priv, enum port port)
> > >  	return intel_bios_is_port_edp(dev_priv, port);  }
> > >
> > > +static bool
> > > +has_gamut_metadata_dip(struct drm_i915_private *dev_priv,
> > 
> > s/dev_priv/i915/ for modern style
> 
> Ok, will update
> 
> > 
> > > +		       struct intel_dp *intel_dp, enum port port) {
> > > +	if (intel_bios_is_lspcon_present(dev_priv, port))
> > > +		return false;
> > > +
> > > +	if (DISPLAY_VER(dev_priv) >= 10 && !IS_GEMINILAKE(dev_priv))
> > > +		return true;
> > 
> > DISPLAY_VER >= 11
> > 
> > > +
> > > +	if (port == PORT_A && intel_dp_is_edp(intel_dp))
> > 
> > The is_edp check is still wrong. Should be just port==A.
> > Also allows you to drop the intel_dp argument to the function.
> 
> In the register description for VIDEO_DIP_CTL, (Bspec:7748)
> BitField: VDIP Enable GMP
> [BDW, SKL, BXT, KBL, KBLH, GLK, GLV, CFL, WHL, AML, CML, CNL, LKFR]
> GMP is not supported on transcoder EDP going to DDI A.
> 
> That's why was checking for eDP. But port A check should be good enough.

Transcoder EDP is hardwired to DDI A. Despite the name it has
nothing to do with eDP vs. DP.

Having the eDP check would be just wrong because it would then
attach the property to DDI A when it is used as an external DP
port. And yes, such machines do in fact exist.

> Will drop it.
> 
> > > +		return false;
> > > +
> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv) ||
> > > +	    IS_GEMINILAKE(dev_priv) || DISPLAY_VER(dev_priv) >= 9)
> > > +		return true;
> > 
> > The IS_GLK check is redundant.
> 
> Yeah will drop it.
> 
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  static void
> > >  intel_dp_add_properties(struct intel_dp *intel_dp, struct
> > > drm_connector *connector)  { @@ -4939,7 +4959,7 @@
> > > intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> > >  		intel_attach_dp_colorspace_property(connector);
> > >  	}
> > >
> > > -	if (IS_GEMINILAKE(dev_priv) || DISPLAY_VER(dev_priv) >= 11)
> > > +	if (has_gamut_metadata_dip(dev_priv, intel_dp, port))
> > >  		drm_object_attach_property(&connector->base,
> > >  					   connector->dev-
> > >mode_config.hdr_output_metadata_property,
> > >  					   0);
> > > --
> > > 2.25.1
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel