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

From: "Shankar, Uma" <uma.shankar@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [v3 6/8] drm/i915/display: Implement infoframes readback for LSPCON
Date: Mon, 22 Jun 2020 20:05:06 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F8256D6EC@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <20200622193923.GT6112@intel.com>

> > > > > > > > > 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@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?

Hmm, vendor has not defined his action in case we zero out stuff. Most definitely it should
fail and hopefully would be able to recover without crashing himself (as this will be something he
won't be expecting). For parade yeah we have the token byte which it expects to be non-zero,
may be making that to 0 will trigger a failure. But no documentation regarding this is available, so cant
say for sure. 

For the windows design which are productized, they have not disabled the infoframes. Just they try to ensure they
always send appropriate values as infoframes.  But yeah, this is something which was not thought of as part of Lspcon
design.

> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx