Re: [Intel-gfx] [v6 02/11] drm/i915/display: Enable HDR on gen9 devices with MCA Lspcon

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, September 29, 2020 9:42 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [v6 02/11] drm/i915/display: Enable HDR on gen9 devices with MCA
> Lspcon
> 
> On Tue, Sep 15, 2020 at 02:30:38AM +0530, Uma Shankar wrote:
> > Gen9 hardware supports HDMI2.0 through LSPCON chips.
> > Extending HDR support for MCA LSPCON based GEN9 devices.
> >
> > SOC will drive LSPCON as DP and send HDR metadata as standard DP SDP
> > packets. LSPCON will be set to operate in PCON mode, will receive the
> > metadata and create Dynamic Range and Mastering Infoframe (DRM
> > packets) and send it to HDR capable HDMI sink devices.
> >
> > v2: Re-used hsw infoframe write implementation for HDR metadata for
> > LSPCON as per Ville's suggestion.
> >
> > v3: Addressed Jani Nikula's review comments.
> >
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_hdmi.c   | 10 ++++++
> >  drivers/gpu/drm/i915/display/intel_lspcon.c | 37
> > +++++++++++++++------  drivers/gpu/drm/i915/display/intel_lspcon.h |
> > 5 ++-
> >  3 files changed, 40 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index 0978b0d8f4c6..1e40ed473fb9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -590,6 +590,16 @@ static u32 hsw_infoframes_enabled(struct
> intel_encoder *encoder,
> >  	return val & mask;
> >  }
> >
> > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder,
> > +				const struct intel_crtc_state *crtc_state,
> > +				unsigned int type,
> > +				const void *frame, ssize_t len)
> > +{
> > +	drm_dbg_kms(encoder->base.dev, "Update HDR metadata for lspcon\n");
> > +	/* It uses the legacy hsw implementation for the same */
> > +	hsw_write_infoframe(encoder, crtc_state, type, frame, len); }
> 
> This wrapper seems quite pointless.

Hmm yeah, will drop this.

> > +
> >  static const u8 infoframe_type_to_idx[] = {
> >  	HDMI_PACKET_TYPE_GENERAL_CONTROL,
> >  	HDMI_PACKET_TYPE_GAMUT_METADATA,
> > diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c
> > b/drivers/gpu/drm/i915/display/intel_lspcon.c
> > index 8e8c7a02ab51..5e2d7ca1d20f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> > @@ -461,27 +461,42 @@ void lspcon_write_infoframe(struct intel_encoder
> *encoder,
> >  			    unsigned int type,
> >  			    const void *frame, ssize_t len)  {
> > -	bool ret;
> > +	bool ret = true;
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
> >
> > -	/* LSPCON only needs AVI IF */
> > -	if (type != HDMI_INFOFRAME_TYPE_AVI)
> > +	/*
> > +	 * Supporting HDR on MCA LSPCON
> > +	 * Todo: Add support for Parade later
> > +	 */
> > +	if (type == HDMI_PACKET_TYPE_GAMUT_METADATA &&
> > +	    lspcon->vendor != LSPCON_VENDOR_MCA)
> >  		return;
> 
> We shouldn't have the infoframe flagged as enabled if we don't support it. So
> this check seems pointless, or there's a bug somewhere else.

Sure, will drop this check. It's not required.
> >
> > -	if (lspcon->vendor == LSPCON_VENDOR_MCA)
> > -		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
> > -						      frame, len);
> > -	else
> > -		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
> > -							 frame, len);
> > +	switch (type) {
> > +	case HDMI_INFOFRAME_TYPE_AVI:
> > +		if (lspcon->vendor == LSPCON_VENDOR_MCA)
> > +			ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
> > +							      frame, len);
> > +		else
> > +			ret = _lspcon_write_avi_infoframe_parade(&intel_dp-
> >aux,
> > +								 frame, len);
> > +		break;
> > +	case HDMI_PACKET_TYPE_GAMUT_METADATA:
> > +		lspcon_drm_write_infoframe(encoder, crtc_state,
> > +
> HDMI_PACKET_TYPE_GAMUT_METADATA,
> > +					   frame, VIDEO_DIP_DATA_SIZE);
> 
> Why are we hardocoding the parameters here? Just pass them through?

Ok, will rectify this.

> > +		break;
> > +	default:
> > +		return;
> > +	}
> >
> >  	if (!ret) {
> > -		DRM_ERROR("Failed to write AVI infoframes\n");
> > +		DRM_ERROR("Failed to write infoframes\n");
> >  		return;
> >  	}
> >
> > -	DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n");
> > +	DRM_DEBUG_DRIVER("Infoframes updated successfully\n");
> 
> That pointless debug should probably be just nuked.

Ok, will drop it.
> >  }
> >
> >  void lspcon_read_infoframe(struct intel_encoder *encoder, diff --git
> > a/drivers/gpu/drm/i915/display/intel_lspcon.h
> > b/drivers/gpu/drm/i915/display/intel_lspcon.h
> > index 1cffe8a42a08..3fac05535731 100644
> > --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
> > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
> > @@ -34,5 +34,8 @@ u32 lspcon_infoframes_enabled(struct intel_encoder
> *encoder,
> >  			      const struct intel_crtc_state *pipe_config);  void
> > lspcon_ycbcr420_config(struct drm_connector *connector,
> >  			    struct intel_crtc_state *crtc_state);
> > -
> > +void lspcon_drm_write_infoframe(struct intel_encoder *encoder,
> > +				const struct intel_crtc_state *crtc_state,
> > +				unsigned int type,
> > +				const void *frame, ssize_t len);
> 
> I think we ususally leave a blank line here.

Will do it.
> >  #endif /* __INTEL_LSPCON_H__ */
> > --
> > 2.26.2
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx