| Message ID | 20240315050529.1987425-2-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Enable AUX based backlight for HDR | expand |
On Fri, Mar 15, 2024 at 10:35:30AM +0530, Suraj Kandpal wrote: > As of now whenerver HDR is switched on we use the PWM to change the > backlight as opposed to AUX based backlight changes in terms of nits. > This patch writes to the appropriate DPCD registers to enable aux > based backlight using values in nits. > > --v2 > -Fix max_cll and max_fall assignment [Jani] > -Fix the size sent in drm_dpcd_write [Jani] > > --v3 > -Content Luminance needs to be sent only for pre-ICL after that > it is directly picked up from hdr metadata [Ville] > > --v4 > -Add checks for HDR TCON cap bits [Ville] > -Check eotf of hdr_output_data and sets bits base of that value. > > --v5 > -Fix capability check bits. > -Check colorspace before setting BT2020 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_display_types.h | 3 + > .../drm/i915/display/intel_dp_aux_backlight.c | 57 ++++++++++++++++--- > 2 files changed, 52 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index e67cd5b02e84..271bb609106d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -401,6 +401,9 @@ struct intel_panel { > } vesa; > struct { > bool sdr_uses_aux; > + bool supports_2084_decode; > + bool supports_2020_gamut; > + bool supports_segmented_backlight; > } intel; > } edp; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 4f58efdc688a..f927e259b540 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -40,11 +40,6 @@ > #include "intel_dp.h" > #include "intel_dp_aux_backlight.h" > > -/* TODO: > - * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we > - * can make people's backlights work in the mean time > - */ > - > /* > * DP AUX registers for Intel's proprietary HDR backlight interface. We define > * them here since we'll likely be the only driver to ever use these. > @@ -158,6 +153,12 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > > panel->backlight.edp.intel.sdr_uses_aux = > tcon_cap[2] & INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP; > + panel->backlight.edp.intel.supports_2084_decode = > + tcon_cap[1] & INTEL_EDP_HDR_TCON_2084_DECODE_CAP; > + panel->backlight.edp.intel.supports_2020_gamut = > + tcon_cap[1] & INTEL_EDP_HDR_TCON_2020_GAMUT_CAP; > + panel->backlight.edp.intel.supports_segmented_backlight = > + tcon_cap[1] & INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_CAP; > > return true; > } > @@ -206,6 +207,9 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > u8 buf[4] = {}; > > + if (level < 20) > + level = 20; > + Why are you limiting this to at least 20? > buf[0] = level & 0xFF; > buf[1] = (level & 0xFF00) >> 8; > > @@ -221,7 +225,7 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 > struct intel_connector *connector = to_intel_connector(conn_state->connector); > struct intel_panel *panel = &connector->panel; > > - if (panel->backlight.edp.intel.sdr_uses_aux) { > + if (panel->backlight.edp.intel.sdr_uses_aux || conn_state->hdr_output_metadata) { Just because there is a HDR Static Metadata blob on the connector doesn't mean an HDR mode is set. The TCON seems to consider itself in HDR mode when either INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_ENABLE or INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE are set. If you don't match this up properly, the backlight will be all wrong and the user has an (almost) black screen! > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -251,8 +255,30 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > } > > ctrl = old_ctrl; > - if (panel->backlight.edp.intel.sdr_uses_aux) { > + if (panel->backlight.edp.intel.sdr_uses_aux || conn_state->hdr_output_metadata) { > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; Same problem as before. > + > + if (conn_state->hdr_output_metadata) { > + struct hdr_output_metadata *hdr_metadata = > + conn_state->hdr_output_metadata->data; > + > + if (panel->backlight.edp.intel.supports_segmented_backlight && > + hdr_metadata->hdmi_metadata_type1.eotf >= > + HDMI_EOTF_TRADITIONAL_GAMMA_HDR) > + ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; You cannot enable HLG or any mode other than PQ. Why would you want to enable the segmented backlight in those cases? Also, you said in a gitlab issue that segmented backlight has to be enabled for HDR. This is just entirely wrong. It's a tradeoff between more dynamic range and picture fidelity that ultimately should not be made by a driver but user space. > + if (panel->backlight.edp.intel.supports_2084_decode && > + hdr_metadata->hdmi_metadata_type1.eotf == > + HDMI_EOTF_SMPTE_ST2084) > + ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > + if (panel->backlight.edp.intel.supports_2020_gamut && > + hdr_metadata->hdmi_metadata_type1.eotf >= > + HDMI_EOTF_TRADITIONAL_GAMMA_HDR && Why is this checking for something completely unrelated? This means when we use the Colorspace prop to get BT2020 gamut we don't get the gamut we requested! > + (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB || > + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC || > + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_CYCC)) > + ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; > + } > + > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -292,9 +318,11 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > { > struct drm_i915_private *i915 = to_i915(connector->base.dev); > struct intel_panel *panel = &connector->panel; > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > struct drm_luminance_range_info *luminance_range = > &connector->base.display_info.luminance_range; > int ret; > + u8 buf[4]; > > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] SDR backlight is controlled through %s\n", > connector->base.base.id, connector->base.name, > @@ -318,11 +346,24 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > panel->backlight.min = 0; > } > > + if (DISPLAY_VER(i915) < 11) { This should check for INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP instead! There is no reason to bind this to any hardware version if the hardware itself can tell you if it supports SDP signalling or needs to set it via AUX. Even worse, for DISPLAY_VER(i915) >= 11 you're not setting INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_ENABLE which means the SDP might not get picked up by the TCON and we end up with empty metadata! > + buf[0] = connector->base.hdr_sink_metadata.hdmi_type1.max_cll & 0xFF; > + buf[1] = (connector->base.hdr_sink_metadata.hdmi_type1.max_cll & 0xFF00) >> 8; > + buf[2] = connector->base.hdr_sink_metadata.hdmi_type1.max_fall & 0xFF; > + buf[3] = (connector->base.hdr_sink_metadata.hdmi_type1.max_fall & 0xFF00) >> 8; This is the wrong set of metadata. You have to send it what user space set on the connector. > + > + ret = drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_HDR_CONTENT_LUMINANCE, buf, > + sizeof(buf)); > + if (ret < 0) > + drm_dbg_kms(&i915->drm, > + "Content Luminance DPCD reg write failed, err:-%d\n", > + ret); > + } > + > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Using AUX HDR interface for backlight control (range %d..%d)\n", > connector->base.base.id, connector->base.name, > panel->backlight.min, panel->backlight.max); > > - > panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector, pipe); > panel->backlight.enabled = panel->backlight.level != 0; > Pretty much all of this is wrong or broken. I already pointed you to a fixed up version: https://gitlab.freedesktop.org/swick/linux/-/commits/wip/intel-aux-hdr-backlight We verified that it works as well.
On Fri, Mar 15, 2024 at 12:33:52PM +0100, Sebastian Wick wrote: > On Fri, Mar 15, 2024 at 10:35:30AM +0530, Suraj Kandpal wrote: <snip> > > @@ -318,11 +346,24 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > > panel->backlight.min = 0; > > } > > > > + if (DISPLAY_VER(i915) < 11) { > > This should check for INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP instead! > There is no reason to bind this to any hardware version if the hardware > itself can tell you if it supports SDP signalling or needs to set it via > AUX. That should be OK for icl+. To extend this to pre-icl hardware I think we pretty much need both checks: - has_gamut_metadata_dip() -> can we actually transmit the SDP? - INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP -> can the tcon extract the data from the SDP? and if either is false then we fall back to AUX. We should also change intel_dp_add_properties() to check the tcon caps instead of relying on has_gamut_metadata_dip(), for eDP.
On Fri, Mar 15, 2024 at 01:56:19PM +0200, Ville Syrjälä wrote: > On Fri, Mar 15, 2024 at 12:33:52PM +0100, Sebastian Wick wrote: > > On Fri, Mar 15, 2024 at 10:35:30AM +0530, Suraj Kandpal wrote: > <snip> > > > @@ -318,11 +346,24 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > > > panel->backlight.min = 0; > > > } > > > > > > + if (DISPLAY_VER(i915) < 11) { > > > > This should check for INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP instead! > > There is no reason to bind this to any hardware version if the hardware > > itself can tell you if it supports SDP signalling or needs to set it via > > AUX. > > That should be OK for icl+. > > To extend this to pre-icl hardware I think we pretty much need both checks: > - has_gamut_metadata_dip() -> can we actually transmit the SDP? > - INTEL_EDP_HDR_TCON_SDP_COLORIMETRY_CAP -> can the tcon extract the data from the SDP? > and if either is false then we fall back to AUX. Right, that's something my patch is missing as well. > We should also change intel_dp_add_properties() to check > the tcon caps instead of relying on has_gamut_metadata_dip(), > for eDP. > > -- > Ville Syrjälä > Intel >
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index e67cd5b02e84..271bb609106d 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -401,6 +401,9 @@ struct intel_panel { } vesa; struct { bool sdr_uses_aux; + bool supports_2084_decode; + bool supports_2020_gamut; + bool supports_segmented_backlight; } intel; } edp; diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c index 4f58efdc688a..f927e259b540 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c @@ -40,11 +40,6 @@ #include "intel_dp.h" #include "intel_dp_aux_backlight.h" -/* TODO: - * Implement HDR, right now we just implement the bare minimum to bring us back into SDR mode so we - * can make people's backlights work in the mean time - */ - /* * DP AUX registers for Intel's proprietary HDR backlight interface. We define * them here since we'll likely be the only driver to ever use these. @@ -158,6 +153,12 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) panel->backlight.edp.intel.sdr_uses_aux = tcon_cap[2] & INTEL_EDP_SDR_TCON_BRIGHTNESS_AUX_CAP; + panel->backlight.edp.intel.supports_2084_decode = + tcon_cap[1] & INTEL_EDP_HDR_TCON_2084_DECODE_CAP; + panel->backlight.edp.intel.supports_2020_gamut = + tcon_cap[1] & INTEL_EDP_HDR_TCON_2020_GAMUT_CAP; + panel->backlight.edp.intel.supports_segmented_backlight = + tcon_cap[1] & INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_CAP; return true; } @@ -206,6 +207,9 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); u8 buf[4] = {}; + if (level < 20) + level = 20; + buf[0] = level & 0xFF; buf[1] = (level & 0xFF00) >> 8; @@ -221,7 +225,7 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 struct intel_connector *connector = to_intel_connector(conn_state->connector); struct intel_panel *panel = &connector->panel; - if (panel->backlight.edp.intel.sdr_uses_aux) { + if (panel->backlight.edp.intel.sdr_uses_aux || conn_state->hdr_output_metadata) { intel_dp_aux_hdr_set_aux_backlight(conn_state, level); } else { const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); @@ -251,8 +255,30 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, } ctrl = old_ctrl; - if (panel->backlight.edp.intel.sdr_uses_aux) { + if (panel->backlight.edp.intel.sdr_uses_aux || conn_state->hdr_output_metadata) { ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; + + if (conn_state->hdr_output_metadata) { + struct hdr_output_metadata *hdr_metadata = + conn_state->hdr_output_metadata->data; + + if (panel->backlight.edp.intel.supports_segmented_backlight && + hdr_metadata->hdmi_metadata_type1.eotf >= + HDMI_EOTF_TRADITIONAL_GAMMA_HDR) + ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; + if (panel->backlight.edp.intel.supports_2084_decode && + hdr_metadata->hdmi_metadata_type1.eotf == + HDMI_EOTF_SMPTE_ST2084) + ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; + if (panel->backlight.edp.intel.supports_2020_gamut && + hdr_metadata->hdmi_metadata_type1.eotf >= + HDMI_EOTF_TRADITIONAL_GAMMA_HDR && + (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB || + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC || + conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_CYCC)) + ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; + } + intel_dp_aux_hdr_set_aux_backlight(conn_state, level); } else { u32 pwm_level = intel_backlight_level_to_pwm(connector, level); @@ -292,9 +318,11 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi { struct drm_i915_private *i915 = to_i915(connector->base.dev); struct intel_panel *panel = &connector->panel; + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); struct drm_luminance_range_info *luminance_range = &connector->base.display_info.luminance_range; int ret; + u8 buf[4]; drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] SDR backlight is controlled through %s\n", connector->base.base.id, connector->base.name, @@ -318,11 +346,24 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi panel->backlight.min = 0; } + if (DISPLAY_VER(i915) < 11) { + buf[0] = connector->base.hdr_sink_metadata.hdmi_type1.max_cll & 0xFF; + buf[1] = (connector->base.hdr_sink_metadata.hdmi_type1.max_cll & 0xFF00) >> 8; + buf[2] = connector->base.hdr_sink_metadata.hdmi_type1.max_fall & 0xFF; + buf[3] = (connector->base.hdr_sink_metadata.hdmi_type1.max_fall & 0xFF00) >> 8; + + ret = drm_dp_dpcd_write(&intel_dp->aux, INTEL_EDP_HDR_CONTENT_LUMINANCE, buf, + sizeof(buf)); + if (ret < 0) + drm_dbg_kms(&i915->drm, + "Content Luminance DPCD reg write failed, err:-%d\n", + ret); + } + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Using AUX HDR interface for backlight control (range %d..%d)\n", connector->base.base.id, connector->base.name, panel->backlight.min, panel->backlight.max); - panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector, pipe); panel->backlight.enabled = panel->backlight.level != 0;
As of now whenerver HDR is switched on we use the PWM to change the backlight as opposed to AUX based backlight changes in terms of nits. This patch writes to the appropriate DPCD registers to enable aux based backlight using values in nits. --v2 -Fix max_cll and max_fall assignment [Jani] -Fix the size sent in drm_dpcd_write [Jani] --v3 -Content Luminance needs to be sent only for pre-ICL after that it is directly picked up from hdr metadata [Ville] --v4 -Add checks for HDR TCON cap bits [Ville] -Check eotf of hdr_output_data and sets bits base of that value. --v5 -Fix capability check bits. -Check colorspace before setting BT2020 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- .../drm/i915/display/intel_display_types.h | 3 + .../drm/i915/display/intel_dp_aux_backlight.c | 57 ++++++++++++++++--- 2 files changed, 52 insertions(+), 8 deletions(-)