| Message ID | 20241009072530.2952143-1-chaitanya.kumar.borah@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Allow fastset for change in HDR infoframe | expand |
On Wed, 09 Oct 2024, Chaitanya Kumar Borah <chaitanya.kumar.borah@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@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(). OTOH the DP path uses a common function, which makes me wonder if there could be less duplication for HDMI too. 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,
> -----Original Message----- > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > Sent: Wednesday, October 9, 2024 12:56 PM > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Shankar, Uma > <uma.shankar@intel.com>; ville.syrjala@linux.intel.com > Subject: [v2] drm/i915: Allow fastset for change in HDR infoframe > > 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@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); > + Please change the naming to be consistent as pointed by Jani. Also at the moment this will unnecessarily update DRM infoframe bit in hardware Even if HDR is not even enabled. Please fix that logic. > 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, > -- > 2.25.1
Hi Jani, Thank you for the review. > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, October 9, 2024 2:59 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Shankar, Uma > <uma.shankar@intel.com>; ville.syrjala@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@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@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
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); + 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,
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@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(-)