[Intel-gfx] [v3 6/8] drm/i915/display: Implement infoframes readback for LSPCON

[Shankar, Uma]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Monday, June 22, 2020 11:10 PM
> To: Shankar, Uma <uma.shankar at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; jani.nikula at linux.intel.com; Mun, Gwan-
> gyeong <gwan-gyeong.mun at intel.com>
> Subject: Re: [v3 6/8] drm/i915/display: Implement infoframes readback for
> LSPCON
> 
> On Mon, Jun 22, 2020 at 05:17:50PM +0000, Shankar, Uma wrote:
> >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > > Sent: Thursday, June 11, 2020 9:31 PM
> > > > > > > > To: Shankar, Uma <uma.shankar at intel.com>
> > > > > > > > Cc: intel-gfx at lists.freedesktop.org;
> > > > > > > > jani.nikula at linux.intel.com; Mun, Gwan- gyeong
> > > > > > > > <gwan-gyeong.mun at intel.com>
> > > > > > > > Subject: Re: [v3 6/8] drm/i915/display: Implement
> > > > > > > > infoframes readback for LSPCON
> > > > > > > >
> > > > > > > > On Thu, Jun 11, 2020 at 06:46:50PM +0300, Ville Syrjälä wrote:
> > > > > > > > > On Thu, Jun 11, 2020 at 12:42:30AM +0530, Uma Shankar wrote:
> > > > > > > > > > Implemented Infoframes enabled readback for LSPCON devices.
> > > > > > > > > > This will help align the implementation with state
> > > > > > > > > > readback infrastructure.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/i915/display/intel_lspcon.c | 63
> > > > > > > > > > ++++++++++++++++++++-
> > > > > > > > > >  1 file changed, 61 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git
> > > > > > > > > > a/drivers/gpu/drm/i915/display/intel_lspcon.c
> > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_lspcon.c
> > > > > > > > > > index 9034ce6f20b9..0ebe9a700291 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> > > > > > > > > > @@ -576,11 +576,70 @@ void
> > > > > > > > > > lspcon_set_infoframes(struct intel_encoder
> > > > > > > > *encoder,
> > > > > > > > > >  				  buf, ret);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +static bool
> > > > > > > > > > +_lspcon_read_avi_infoframe_enabled_mca(struct
> > > > > > > > > > +drm_dp_aux *aux) {
> > > > > > > > > > +	int ret;
> > > > > > > > > > +	u32 val = 0;
> > > > > > > > > > +	u16 reg = LSPCON_MCA_AVI_IF_CTRL;
> > > > > > > > > > +
> > > > > > > > > > +	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
> > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > +		DRM_ERROR("DPCD read failed, address 0x%x\n",
> reg);
> > > > > > > > > > +		return false;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	if (val & LSPCON_MCA_AVI_IF_KICKOFF)
> > > > > > > > > > +		return true;
> > > > > > > > > > +
> > > > > > > > > > +	return false;
> > > > > > > > >
> > > > > > > > > return val & ...;
> > > > > > > > >
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static bool
> > > > > > > > > > +_lspcon_read_avi_infoframe_enabled_parade(struct
> > > > > > > > > > +drm_dp_aux *aux) {
> > > > > > > > > > +	int ret;
> > > > > > > > > > +	u32 val = 0;
> > > > > > > > > > +	u16 reg = LSPCON_PARADE_AVI_IF_CTRL;
> > > > > > > > > > +
> > > > > > > > > > +	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
> > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > +		DRM_ERROR("DPCD read failed, address 0x%x\n",
> reg);
> > > > > > > > > > +		return false;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	if (val & LSPCON_PARADE_AVI_IF_KICKOFF)
> > > > > > > > > > +		return true;
> > > > > > > > > > +
> > > > > > > > > > +	return false;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  u32 lspcon_infoframes_enabled(struct intel_encoder *encoder,
> > > > > > > > > >  			      const struct intel_crtc_state
> *pipe_config)  {
> > > > > > > > > > -	/* FIXME actually read this from the hw */
> > > > > > > > > > -	return 0;
> > > > > > > > > > +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > > > > > +	struct intel_lspcon *lspcon =
> enc_to_intel_lspcon(encoder);
> > > > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
> > > > > > > > > > +	bool infoframes_enabled;
> > > > > > > > > > +	u32 mask = 0;
> > > > > > > > > > +	u32 val;
> > > > > > > > > > +
> > > > > > > > > > +	if (lspcon->vendor == LSPCON_VENDOR_MCA)
> > > > > > > > > > +		infoframes_enabled =
> > > > > > > > _lspcon_read_avi_infoframe_enabled_mca(&intel_dp->aux);
> > > > > > > > > > +	else
> > > > > > > > > > +		infoframes_enabled =
> > > > > > > > > > +_lspcon_read_avi_infoframe_enabled_parade(&intel_dp->
> > > > > > > > > > +aux)
> > > > > > > > > > +;
> > > > > > > > > > +
> > > > > > > > > > +	if (infoframes_enabled)
> > > > > > > > > > +		return true;
> > > > > > > > >
> > > > > > > > > This is supposed to return a bitmask of all enabled infoframes.
> > > > > > >
> > > > > > > > Actually since we're dealing with both the LSPCON specific
> > > > > > > > stuff and DIP stuff for the DRM infoframe I think we
> > > > > > > > should stop using using intel_hdmi_infoframes_enabled(),
> > > > > > > > and instead provide a LSPCON specific replacement for it.
> > > > > > > > That way we can directly return the abstract bitmask
> > > > > > > > instead of pretending to return a bitmask of
> > > > > the DIP bits.
> > > >
> > > > We have DP (VSC etc) packets also managed as HDMI infoframes only.
> > > > We can keep the same with bitmask as VIDEO_DIP_ENABLE_AVI_HSW for
> > > > AVI and similarly VIDEO_DIP_ENABLE_GMP_HSW for DRM (HDR metadata).
> > > > This will
> > > help all the helper align appropriately even in the intel_dump_pipe_config.
> > >
> > > intel_dump_infoframe() does not use any platform specific bitmasks.
> > > So I don't understand what you're talking about here.
> >
> > What I meant is that if we continue to use the existing values and
> > bitmask, we can have lspcon infoframes_enabled return the appropriate
> > type of infoframe which is active (as you suggested) and later with
> > intel_dump_pipe_config when it checks for intel_hdmi_infoframe_enable, we
> will get a matching value in pipe_config->infoframes.enable and be able to dump
> them as well. Hope I am on same page with you here.
> 
> Still don't really get  what you're saying.
> 
> What I am saying is we have something like:
> 
> intel_lspcon_infoframes_enabled()
> {
> 	u32 enabled = 0;
> 
> 	if (lspcon_avi_enabled)
> 		enabled |=
> intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI);
> 
> 	if (dip_gmp_enabled)
> 		enabled |=
> intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA);
> 
> 	return enabled;
> }
> 
> Or I suppose we could even do something like:
> 
> intel_lspcon_infoframes_enabled()
> {
> 	u32 enabled = intel_hdmi_infoframes_enabled();
> 
> 	if (lspcon_avi_enabled)
> 		enabled |=
> intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_AVI);
> 
> 	return enabled;
> }
> 
> which would be more future proof if we start to use any other DIP stuff.

Got the point now, thanks Ville for patiently clarifying. Will incorporate this and send out
the next version.

Regards,
Uma Shankar

> --
> Ville Syrjälä
> Intel