[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: 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.

Will fix this accordingly and send the next version. Hope this is ok.

> > > Sure,  will fix this and resend the next version.
> > >
> > > > >
> > > > > Also my question "how do we turn off infoframes once enabled?"
> > > > > from
> > > > > https://patchwork.freedesktop.org/patch/351719/?series=72928&rev
> > > > > =1
> > > > > still remains unanswered...
> > >
> > > For the AVI infoframe we generally compute and change the respective
> > > values. If no change is requested and computed we can let the
> > > existing infoframes be transmitted. AFAIK there is no mechanism
> > > called out, to explicitly disable this on Lspcon. Have not seen any
> > > issues due to this, so
> > hoping that it may be safe even if they are enabled.
> >
> > It's not valid to transmit infoframes to DVI sinks.
> 
> With your fix, we won't be enabling or setting the infoframe on DVI sinks.
> If I understand correctly, we may have issue if we connect HDMI (where we
> would have sent the infoframe) and later unplug and plug a DVI sink. With
> unplug if Lspcon is not resetting this internally then this will be a problem. I will
> try to get this information on Lspcon behavior.

Hi Ville,
Searched various docs on LSPCON and couldn't find any mention of disabling infoframe.
Reached out to few folks who were interfacing with Lspcon vendors and got this finally
clarified.  The expectation is that LSPCON will reset stuff on unplug and will not send infoframes to DVI sink,
so from source side we don't need to explicitly disable them. Lspcon will take care of this, this was
tested for compliance as well by them.

I will share the updated series adding your patch stopping infoframes to be sent to DVI sinks.
Please review the same.

Thank & Regards,
Uma Shankar
 
> > >
> > > I am planning to take your patch from the series and float along
> > > with this series, adding check for DRM Infoframes also. Hope that is ok ?
> > >
> > > Thanks Ville for your feedback.
> > >
> > > Regards,
> > > Uma Shankar
> > >
> > > > > > +
> > > > > > +	if (lspcon->hdr_supported) {
> > > > > > +		val = intel_de_read(dev_priv,
> > > > > > +				    HSW_TVIDEO_DIP_CTL(pipe_config-
> > > > >cpu_transcoder));
> > > > > > +		mask |= VIDEO_DIP_ENABLE_GMP_HSW;
> > > > > > +
> > > > > > +		if (val & mask)
> > > > > > +			return val & mask;
> > > > > > +	}
> > > > > > +
> > > > > > +	return false;
> > > > > >  }
> > > > > >
> > > > > >  void lspcon_resume(struct intel_lspcon *lspcon)
> > > > > > --
> > > > > > 2.22.0
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> >
> > --
> > Ville Syrjälä
> > Intel