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

[Ville Syrjälä]

On Mon, Jun 22, 2020 at 05:21: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->au
> > > > > > > > > > +x)
> > > > > > > > > > +;
> > > > > > > > > > +
> > > > > > > > > > +	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.
> > 
> > > >
> > > > 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=729
> > > > > > > > > 28
> > > > > > > > > &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.
> > >
> > > Unplug is irrelevant. We also have the property by which the user can
> > > force the inforframes off.
> 
> Oh ok, but it seems we don't have an option to disable them on LSPCON. They have not
> added any interface for that unfortunately.

That seems a bit crazy. What happens if we just zero out the whole
thing? The parade version at least seems to have that "token" byte
which maybe could be used to specify whether the infoframe is
enabled?

-- 
Ville Syrjälä
Intel