Hi Jani, Thank you for the review. > -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: Wednesday, October 9, 2024 2:59 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx > Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Shankar, Uma > <uma.shankar@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx > Subject: Re: [v2] drm/i915: Allow fastset for change in HDR infoframe > > On Wed, 09 Oct 2024, Chaitanya Kumar Borah > <chaitanya.kumar.borah@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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
