[v2] drm/i915: Allow fastset for change in HDR infoframe

[Borah, Chaitanya Kumar]

Hi Jani,

Thank you for the review.

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, October 9, 2024 2:59 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>; intel-
> gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>; Shankar, Uma
> <uma.shankar at intel.com>; ville.syrjala at linux.intel.com
> Subject: Re: [v2] drm/i915: Allow fastset for change in HDR infoframe
> 
> On Wed, 09 Oct 2024, Chaitanya Kumar Borah
> <chaitanya.kumar.borah at intel.com> wrote:
> > Changes in Dynamic Range and Mastering infoframe should not trigger a
> > full modeset. Therefore, allow fastset. DP SDP programming is already
> > hooked up in the fastset flow but HDMI AVI infoframe update is not,
> > add it.
> > Any other infoframe that can be fastset should be added to the helper
> > intel_hdmi_fastset_infoframes().
> >
> > v2:
> >  - Update HDMI AVI infoframe during fastset.
> >
> > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c     |  3 +++
> >  drivers/gpu/drm/i915/display/intel_display.c |  3 ++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 19 +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_hdmi.h    |  3 +++
> >  4 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index fe1ded6707f9..3195c1125ac3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3489,6 +3489,9 @@ void intel_ddi_update_pipe(struct
> intel_atomic_state *state,
> >  		intel_ddi_update_pipe_dp(state, encoder, crtc_state,
> >  					 conn_state);
> >
> > +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> > +		intel_hdmi_fastset_infoframes(encoder, crtc_state,
> conn_state);
> 
> I don't know if the patch at hand is the right thing to do, but if it is, please let's
> stick to uniform naming here. If you add stuff specifically for the encoder-
> >update_pipe path, please name it
> *_update_pipe() i.e. intel_hdmi_infoframes_update_pipe().
> 

Ack.

> OTOH the DP path uses a common function, which makes me wonder if there
> could be less duplication for HDMI too.
> 

Considering you are talking about the common function for updating all the HDMI infoframes,
it will need a bit more investigation to determine which infoframes actually can be updated in a fastest.

We know that at least one of them can't. See [1].

The tests we ran suggests that DRM info frame can be successfully updated during a fastest. This also seems
to be a valid use case.

Will it be prudent to have a common function intel_hdmi_fastset_infoframes and add other infoframes to it
as and when needed?

Regards

Chaitanya

[1] https://patchwork.freedesktop.org/patch/229325/

> BR,
> Jani.
> 
> 
> > +
> >  	intel_hdcp_update_pipe(state, encoder, crtc_state, conn_state);  }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index e1f6255e918b..e8f8f55f75d2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5683,7 +5683,8 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_INFOFRAME(avi);
> >  	PIPE_CONF_CHECK_INFOFRAME(spd);
> >  	PIPE_CONF_CHECK_INFOFRAME(hdmi);
> > -	PIPE_CONF_CHECK_INFOFRAME(drm);
> > +	if (!fastset)
> > +		PIPE_CONF_CHECK_INFOFRAME(drm);
> >  	PIPE_CONF_CHECK_DP_VSC_SDP(vsc);
> >  	PIPE_CONF_CHECK_DP_AS_SDP(as_sdp);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index 72ac910bf6ec..eba79f14d4e2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -1211,6 +1211,25 @@ static void vlv_set_infoframes(struct
> intel_encoder *encoder,
> >  			      &crtc_state->infoframes.hdmi);  }
> >
> > +void intel_hdmi_fastset_infoframes(struct intel_encoder *encoder,
> > +				   const struct intel_crtc_state *crtc_state,
> > +				   const struct drm_connector_state
> *conn_state) {
> > +	struct intel_display *display = to_intel_display(encoder);
> > +	i915_reg_t reg = HSW_TVIDEO_DIP_CTL(display,
> > +					    crtc_state->cpu_transcoder);
> > +	u32 val = intel_de_read(display, reg);
> > +
> > +	val &= ~(VIDEO_DIP_ENABLE_DRM_GLK);
> > +
> > +	intel_de_write(display, reg, val);
> > +	intel_de_posting_read(display, reg);
> > +
> > +	intel_write_infoframe(encoder, crtc_state,
> > +			      HDMI_INFOFRAME_TYPE_DRM,
> > +			      &crtc_state->infoframes.drm); }
> > +
> >  static void hsw_set_infoframes(struct intel_encoder *encoder,
> >  			       bool enable,
> >  			       const struct intel_crtc_state *crtc_state, diff --git
> > a/drivers/gpu/drm/i915/display/intel_hdmi.h
> > b/drivers/gpu/drm/i915/display/intel_hdmi.h
> > index 9b97623665c5..466f48df8a74 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.h
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.h
> > @@ -42,6 +42,9 @@ u32 intel_hdmi_infoframes_enabled(struct
> > intel_encoder *encoder,
> >  u32 intel_hdmi_infoframe_enable(unsigned int type);  void
> > intel_hdmi_read_gcp_infoframe(struct intel_encoder *encoder,
> >  				   struct intel_crtc_state *crtc_state);
> > +void intel_hdmi_fastset_infoframes(struct intel_encoder *encoder,
> > +				   const struct intel_crtc_state *crtc_state,
> > +				   const struct drm_connector_state
> *conn_state);
> >  void intel_read_infoframe(struct intel_encoder *encoder,
> >  			  const struct intel_crtc_state *crtc_state,
> >  			  enum hdmi_infoframe_type type,
> 
> --
> Jani Nikula, Intel