| Message ID | 20200610191232.11620-7-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable HDR on MCA LSPCON based Gen9 devices | expand |
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->aux); > + > + if (infoframes_enabled) > + return true; This is supposed to return a bitmask of all enabled infoframes. 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... > + > + 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
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->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. > > 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... > > > + > > + 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
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, June 11, 2020 9:31 PM > To: Shankar, Uma <uma.shankar@intel.com> > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Mun, Gwan- > gyeong <gwan-gyeong.mun@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@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. 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. 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
On Mon, Jun 15, 2020 at 08:39:55PM +0000, Shankar, Uma wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Thursday, June 11, 2020 9:31 PM > > To: Shankar, Uma <uma.shankar@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Mun, Gwan- > > gyeong <gwan-gyeong.mun@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@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. > > 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. > > 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
> > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: Thursday, June 11, 2020 9:31 PM > > > To: Shankar, Uma <uma.shankar@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; > > > Mun, Gwan- gyeong <gwan-gyeong.mun@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@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. > > > > 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. > > > > 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
> > > > -----Original Message----- > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; > > > > Mun, Gwan- gyeong <gwan-gyeong.mun@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@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
On Mon, Jun 22, 2020 at 11:44:58AM +0000, Shankar, Uma wrote: > > > > > > -----Original Message----- > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; > > > > > Mun, Gwan- gyeong <gwan-gyeong.mun@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@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. > > 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. Unplug is irrelevant. We also have the property by which the user can force the inforframes off. > > 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
> > > > > > -----Original Message----- > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > jani.nikula@linux.intel.com; Mun, Gwan- gyeong > > > > > > <gwan-gyeong.mun@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@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. > > > > 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. > > Unplug is irrelevant. We also have the property by which the user can force the > inforframes off. > > > > > 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 > > -- > Ville Syrjälä > Intel
> > > > > > > -----Original Message----- > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > jani.nikula@linux.intel.com; Mun, Gwan- gyeong > > > > > > > <gwan-gyeong.mun@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@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. Regards, Uma Shankar > > > > > > 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 > > > > -- > > Ville Syrjälä > > Intel
On Mon, Jun 22, 2020 at 05:17:50PM +0000, Shankar, Uma wrote: > > > > > > > > -----Original Message----- > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > jani.nikula@linux.intel.com; Mun, Gwan- gyeong > > > > > > > <gwan-gyeong.mun@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@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.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Monday, June 22, 2020 11:10 PM > To: Shankar, Uma <uma.shankar@intel.com> > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Mun, Gwan- > gyeong <gwan-gyeong.mun@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@linux.intel.com> > > > > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > > jani.nikula@linux.intel.com; Mun, Gwan- gyeong > > > > > > > > <gwan-gyeong.mun@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@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
On Mon, Jun 22, 2020 at 05:21:50PM +0000, Shankar, Uma wrote: > > > > > > > > > -----Original Message----- > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Sent: Thursday, June 11, 2020 9:31 PM > > > > > > > > To: Shankar, Uma <uma.shankar@intel.com> > > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > > jani.nikula@linux.intel.com; Mun, Gwan- gyeong > > > > > > > > <gwan-gyeong.mun@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@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?
> > > > > > > > > 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
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; +} + +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; + + 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)
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(-)