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@xxxxxxxxx>
> ---
> .../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.
