| Message ID | 20240404032931.380887-7-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable Aux Based EDP HDR | expand |
On Thu, 04 Apr 2024, Suraj Kandpal <suraj.kandpal@intel.com> 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 > > --v6 > -Use intel_dp_has_gamut_dip to check if we have capability > to send sdp [Ville] > -Seprate filling of all hdr tcon related bits into it's > own function. > -Check eotf data to make sure we are in HDR mode [Sebastian] > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_dp_aux_backlight.c | 105 ++++++++++++++++-- > 1 file changed, 94 insertions(+), 11 deletions(-) > > 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 2d50a4734823..7af876e2d210 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. > @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (ret != sizeof(tcon_cap)) > return false; > > - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > - return false; > - > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR backlight interface version %d\n", > connector->base.base.id, connector->base.name, > is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", tcon_cap[0]); > @@ -137,6 +129,9 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (!is_intel_tcon_cap(tcon_cap)) > return false; > > + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > + return false; > + > /* > * If we don't have HDR static metadata there is no way to > * runtime detect used range for nits based control. For now > @@ -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, > connector->base.base.id, connector->base.name); > } > > +static bool > +intel_dp_aux_in_hdr_mode(const struct drm_connector_state *conn_state) The naming of this file and the symbols in this file isn't all that great to begin with, but at least all the current intel_dp_aux_* functions have "backlight" somewhere in them, to indicate this is about "aux backlight". "intel_dp_aux_in_hdr_mode" literally says *aux* is in hdr mode, which is misleading and surprising to someone who doesn't know what it's about. And we have enough things that are misleading and surprising! ;) Please let's try to be more specific. (But also, let more review come in before just rolling another version with renames.) BR, Jani. > +{ > + struct hdr_output_metadata *hdr_metadata; > + > + if (!conn_state->hdr_output_metadata) > + return false; > + > + hdr_metadata = conn_state->hdr_output_metadata->data; > + > + return hdr_metadata->hdmi_metadata_type1.eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR; > +} > + > static void > intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level) > { > 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 (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 > } > } > > +static void > +intel_dp_aux_write_content_luminance(struct intel_connector *connector, > + struct hdr_output_metadata *hdr_metadata) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + int ret; > + u8 buf[4]; > + > + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) > + return; > + > + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; > + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) >> 8; > + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; > + buf[3] = (hdr_metadata->hdmi_metadata_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); > +} > + > +static void > +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state *conn_state, u8 *ctrl) > +{ > + struct intel_connector *connector = to_intel_connector(conn_state->connector); > + struct intel_panel *panel = &connector->panel; > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + struct hdr_output_metadata *hdr_metadata = conn_state->hdr_output_metadata->data; > + > + /* According to spec segmented backlight needs to be set whenever panel is in > + * HDR mode. > + */ > + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > + > + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL we disable > + * tone mapping and set 2084 decode. > + */ > + if (hdr_metadata->hdmi_metadata_type1.eotf == > + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > + } else { > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: Cannot decode requested EOTF\n", > + connector->base.base.id, connector->base.name); > + } > + > + if (panel->backlight.edp.intel.supports_2020_gamut && > + (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; > + > + if (panel->backlight.edp.intel.supports_sdp_colorimetry && > + intel_dp_has_gamut_metadata_dip(connector->encoder)) > + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > + else > + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > +} > + > static void > intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state, u32 level) > @@ -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > struct intel_panel *panel = &connector->panel; > struct drm_i915_private *i915 = to_i915(connector->base.dev); > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct hdr_output_metadata *hdr_metadata; > int ret; > u8 old_ctrl, ctrl; > > @@ -261,8 +335,10 @@ 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 (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > + > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > } > > + if (intel_dp_aux_in_hdr_mode(conn_state)) > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > + > if (ctrl != old_ctrl && > drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) > drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to configure DPCD brightness controls\n", > connector->base.base.id, connector->base.name); > + > + if (intel_dp_aux_in_hdr_mode(conn_state)) { > + hdr_metadata = conn_state->hdr_output_metadata->data; > + intel_dp_aux_write_content_luminance(connector, hdr_metadata); > + } > } > > static void > @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > 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;
On Thu, Apr 04, 2024 at 08:59:29AM +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 > > --v6 > -Use intel_dp_has_gamut_dip to check if we have capability > to send sdp [Ville] > -Seprate filling of all hdr tcon related bits into it's > own function. > -Check eotf data to make sure we are in HDR mode [Sebastian] > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_dp_aux_backlight.c | 105 ++++++++++++++++-- > 1 file changed, 94 insertions(+), 11 deletions(-) > > 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 2d50a4734823..7af876e2d210 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. > @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (ret != sizeof(tcon_cap)) > return false; > > - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > - return false; > - > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR backlight interface version %d\n", > connector->base.base.id, connector->base.name, > is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", tcon_cap[0]); > @@ -137,6 +129,9 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > if (!is_intel_tcon_cap(tcon_cap)) > return false; > > + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > + return false; > + > /* > * If we don't have HDR static metadata there is no way to > * runtime detect used range for nits based control. For now > @@ -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, > connector->base.base.id, connector->base.name); > } > > +static bool > +intel_dp_aux_in_hdr_mode(const struct drm_connector_state *conn_state) > +{ > + struct hdr_output_metadata *hdr_metadata; > + > + if (!conn_state->hdr_output_metadata) > + return false; > + > + hdr_metadata = conn_state->hdr_output_metadata->data; > + > + return hdr_metadata->hdmi_metadata_type1.eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR; This line worries me a bit. The TCON only supports PQ HDR mode so when the metadata request HLG or traditional gamma HDR then only the segmented backlight is enable in intel_dp_aux_fill_hdr_tcon_params but it uses HDR backlight. Did you test that this doesn't result in a black screen? Maybe change this to `eotf == HDMI_EOTF_SMPTE_ST2084` instead? > +} > + > static void > intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level) > { > 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 (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 > } > } > > +static void > +intel_dp_aux_write_content_luminance(struct intel_connector *connector, > + struct hdr_output_metadata *hdr_metadata) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + int ret; > + u8 buf[4]; > + > + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) > + return; I don't entirely understand intel_dp_has_gamut_metadata_dip, but I assume it is true when the SDP HDR stuff can be send? Either way, you only enable the HDR_OUTPUT_METADATA property if intel_dp_has_gamut_metadata_dip returns true, so you will only ever have intel_dp_aux_in_hdr_mode return true if the dip exists. So, this doesn't do anything. I think what you need is * attach the HDR_OUTPUT_METADATA property if you can send the SDP thing and the TCON supports the SDP override (>ICL) OR you can control HDR via AUX (>=ICL). * always set the content luminance via AUX > + > + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; > + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) >> 8; > + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; > + buf[3] = (hdr_metadata->hdmi_metadata_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); > +} > + > +static void > +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state *conn_state, u8 *ctrl) > +{ > + struct intel_connector *connector = to_intel_connector(conn_state->connector); > + struct intel_panel *panel = &connector->panel; > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + struct hdr_output_metadata *hdr_metadata = conn_state->hdr_output_metadata->data; > + > + /* According to spec segmented backlight needs to be set whenever panel is in > + * HDR mode. > + */ > + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > + > + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL we disable > + * tone mapping and set 2084 decode. > + */ > + if (hdr_metadata->hdmi_metadata_type1.eotf == > + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > + } else { > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: Cannot decode requested EOTF\n", > + connector->base.base.id, connector->base.name); > + } > + > + if (panel->backlight.edp.intel.supports_2020_gamut && > + (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; > + > + if (panel->backlight.edp.intel.supports_sdp_colorimetry && > + intel_dp_has_gamut_metadata_dip(connector->encoder)) > + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > + else > + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > +} > + > static void > intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state, u32 level) > @@ -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > struct intel_panel *panel = &connector->panel; > struct drm_i915_private *i915 = to_i915(connector->base.dev); > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > + struct hdr_output_metadata *hdr_metadata; > int ret; > u8 old_ctrl, ctrl; > > @@ -261,8 +335,10 @@ 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 (intel_dp_aux_in_hdr_mode(conn_state) || > + panel->backlight.edp.intel.sdr_uses_aux) { > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > + > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > } else { > u32 pwm_level = intel_backlight_level_to_pwm(connector, level); > @@ -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > } > > + if (intel_dp_aux_in_hdr_mode(conn_state)) > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > + > if (ctrl != old_ctrl && > drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) > drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to configure DPCD brightness controls\n", > connector->base.base.id, connector->base.name); > + > + if (intel_dp_aux_in_hdr_mode(conn_state)) { > + hdr_metadata = conn_state->hdr_output_metadata->data; > + intel_dp_aux_write_content_luminance(connector, hdr_metadata); > + } > } > > static void > @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi > 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; >
> -----Original Message----- > From: Sebastian Wick <sebastian.wick@redhat.com> > Sent: Friday, April 5, 2024 11:50 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Borah, Chaitanya Kumar > <chaitanya.kumar.borah@intel.com>; Shankar, Uma > <uma.shankar@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; > Murthy, Arun R <arun.r.murthy@intel.com>; Syrjala, Ville > <ville.syrjala@intel.com>; Kumar, Naveen1 <naveen1.kumar@intel.com> > Subject: Re: [5/7] drm/i915/dp: Enable AUX based backlight for HDR > > On Thu, Apr 04, 2024 at 08:59:29AM +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 > > > > --v6 > > -Use intel_dp_has_gamut_dip to check if we have capability to send sdp > > [Ville] -Seprate filling of all hdr tcon related bits into it's own > > function. > > -Check eotf data to make sure we are in HDR mode [Sebastian] > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../drm/i915/display/intel_dp_aux_backlight.c | 105 > > ++++++++++++++++-- > > 1 file changed, 94 insertions(+), 11 deletions(-) > > > > 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 2d50a4734823..7af876e2d210 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. > > @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct > intel_connector *connector) > > if (ret != sizeof(tcon_cap)) > > return false; > > > > - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > > - return false; > > - > > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR > backlight interface version %d\n", > > connector->base.base.id, connector->base.name, > > is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", > > tcon_cap[0]); @@ -137,6 +129,9 @@ > intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > > if (!is_intel_tcon_cap(tcon_cap)) > > return false; > > > > + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > > + return false; > > + > > /* > > * If we don't have HDR static metadata there is no way to > > * runtime detect used range for nits based control. For now @@ > > -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct > drm_connector_state *conn_state, > > connector->base.base.id, connector->base.name); } > > > > +static bool > > +intel_dp_aux_in_hdr_mode(const struct drm_connector_state > > +*conn_state) { > > + struct hdr_output_metadata *hdr_metadata; > > + > > + if (!conn_state->hdr_output_metadata) > > + return false; > > + > > + hdr_metadata = conn_state->hdr_output_metadata->data; > > + > > + return hdr_metadata->hdmi_metadata_type1.eotf != > > +HDMI_EOTF_TRADITIONAL_GAMMA_SDR; > > This line worries me a bit. The TCON only supports PQ HDR mode so when the > metadata request HLG or traditional gamma HDR then only the segmented > backlight is enable in intel_dp_aux_fill_hdr_tcon_params but it uses HDR > backlight. > > Did you test that this doesn't result in a black screen? Maybe change this to > `eotf == HDMI_EOTF_SMPTE_ST2084` instead? > I can test it to see if we do see a blank screen if so I can add this change in otherwise Ill stick with this. > > +} > > + > > static void > > intel_dp_aux_hdr_set_backlight(const struct drm_connector_state > > *conn_state, u32 level) { > > 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 (intel_dp_aux_in_hdr_mode(conn_state) || > > + panel->backlight.edp.intel.sdr_uses_aux) { > > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > > } else { > > const u32 pwm_level = > intel_backlight_level_to_pwm(connector, > > level); @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const > struct drm_connector_state *conn_state, u32 > > } > > } > > > > +static void > > +intel_dp_aux_write_content_luminance(struct intel_connector *connector, > > + struct hdr_output_metadata > *hdr_metadata) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + int ret; > > + u8 buf[4]; > > + > > + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) > > + return; > > I don't entirely understand intel_dp_has_gamut_metadata_dip, but I assume > it is true when the SDP HDR stuff can be send? > > Either way, you only enable the HDR_OUTPUT_METADATA property if > intel_dp_has_gamut_metadata_dip returns true, so you will only ever have > intel_dp_aux_in_hdr_mode return true if the dip exists. So, this doesn't do > anything. > > I think what you need is > > * attach the HDR_OUTPUT_METADATA property if you can send the SDP thing > and the TCON supports the SDP override (>ICL) OR you can control HDR > via AUX (>=ICL). > * always set the content luminance via AUX Content luminance DPCD register does not need to be written from gen > ICL It would just add an unnecessary DPCD write since specs state these values are directly to be extracted from static_metadata_header. Yes intel_dp_has_gamut_metadata_dip check is won't alone do the work and an extra H/w restriction check needs to be added here. Regards, Suraj Kandpal > > > + > > + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; > > + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) > >> 8; > > + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; > > + buf[3] = (hdr_metadata->hdmi_metadata_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); > > +} > > + > > +static void > > +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state > > +*conn_state, u8 *ctrl) { > > + struct intel_connector *connector = to_intel_connector(conn_state- > >connector); > > + struct intel_panel *panel = &connector->panel; > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + struct hdr_output_metadata *hdr_metadata = > > +conn_state->hdr_output_metadata->data; > > + > > + /* According to spec segmented backlight needs to be set whenever > panel is in > > + * HDR mode. > > + */ > > + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > + > > + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL > we disable > > + * tone mapping and set 2084 decode. > > + */ > > + if (hdr_metadata->hdmi_metadata_type1.eotf == > > + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { > > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > + } else { > > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: > Cannot decode requested EOTF\n", > > + connector->base.base.id, connector->base.name); > > + } > > + > > + if (panel->backlight.edp.intel.supports_2020_gamut && > > + (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; > > + > > + if (panel->backlight.edp.intel.supports_sdp_colorimetry && > > + intel_dp_has_gamut_metadata_dip(connector->encoder)) > > + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > > + else > > + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > > +} > > + > > static void > > intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, > > const struct drm_connector_state > *conn_state, u32 level) @@ > > -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct > intel_crtc_state *crtc_state, > > struct intel_panel *panel = &connector->panel; > > struct drm_i915_private *i915 = to_i915(connector->base.dev); > > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > + struct hdr_output_metadata *hdr_metadata; > > int ret; > > u8 old_ctrl, ctrl; > > > > @@ -261,8 +335,10 @@ 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 (intel_dp_aux_in_hdr_mode(conn_state) || > > + panel->backlight.edp.intel.sdr_uses_aux) { > > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > > + > > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > > } else { > > u32 pwm_level = intel_backlight_level_to_pwm(connector, > level); @@ > > -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct > intel_crtc_state *crtc_state, > > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > > } > > > > + if (intel_dp_aux_in_hdr_mode(conn_state)) > > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > > + > > if (ctrl != old_ctrl && > > drm_dp_dpcd_writeb(&intel_dp->aux, > INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) > > drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to > configure DPCD brightness controls\n", > > connector->base.base.id, connector->base.name); > > + > > + if (intel_dp_aux_in_hdr_mode(conn_state)) { > > + hdr_metadata = conn_state->hdr_output_metadata->data; > > + intel_dp_aux_write_content_luminance(connector, > hdr_metadata); > > + } > > } > > > > static void > > @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct > intel_connector *connector, enum pipe pi > > 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; > >
> -----Original Message----- > From: Sebastian Wick <sebastian.wick@redhat.com> > Sent: Friday, April 5, 2024 11:50 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Borah, Chaitanya Kumar > <chaitanya.kumar.borah@intel.com>; Shankar, Uma > <uma.shankar@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; > Murthy, Arun R <arun.r.murthy@intel.com>; Syrjala, Ville > <ville.syrjala@intel.com>; Kumar, Naveen1 <naveen1.kumar@intel.com> > Subject: Re: [5/7] drm/i915/dp: Enable AUX based backlight for HDR > > On Thu, Apr 04, 2024 at 08:59:29AM +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 > > > > --v6 > > -Use intel_dp_has_gamut_dip to check if we have capability to send sdp > > [Ville] -Seprate filling of all hdr tcon related bits into it's own > > function. > > -Check eotf data to make sure we are in HDR mode [Sebastian] > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../drm/i915/display/intel_dp_aux_backlight.c | 105 > > ++++++++++++++++-- > > 1 file changed, 94 insertions(+), 11 deletions(-) > > > > 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 2d50a4734823..7af876e2d210 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. > > @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct > intel_connector *connector) > > if (ret != sizeof(tcon_cap)) > > return false; > > > > - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > > - return false; > > - > > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR > backlight interface version %d\n", > > connector->base.base.id, connector->base.name, > > is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", > > tcon_cap[0]); @@ -137,6 +129,9 @@ > intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) > > if (!is_intel_tcon_cap(tcon_cap)) > > return false; > > > > + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) > > + return false; > > + > > /* > > * If we don't have HDR static metadata there is no way to > > * runtime detect used range for nits based control. For now @@ > > -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct > drm_connector_state *conn_state, > > connector->base.base.id, connector->base.name); } > > > > +static bool > > +intel_dp_aux_in_hdr_mode(const struct drm_connector_state > > +*conn_state) { > > + struct hdr_output_metadata *hdr_metadata; > > + > > + if (!conn_state->hdr_output_metadata) > > + return false; > > + > > + hdr_metadata = conn_state->hdr_output_metadata->data; > > + > > + return hdr_metadata->hdmi_metadata_type1.eotf != > > +HDMI_EOTF_TRADITIONAL_GAMMA_SDR; > > This line worries me a bit. The TCON only supports PQ HDR mode so when > the metadata request HLG or traditional gamma HDR then only the > segmented backlight is enable in intel_dp_aux_fill_hdr_tcon_params but it > uses HDR backlight. > > Did you test that this doesn't result in a black screen? Maybe change this to > `eotf == HDMI_EOTF_SMPTE_ST2084` instead? Just tested it will update this change Regards, Suraj Kandpal > > > +} > > + > > static void > > intel_dp_aux_hdr_set_backlight(const struct drm_connector_state > > *conn_state, u32 level) { > > 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 (intel_dp_aux_in_hdr_mode(conn_state) || > > + panel->backlight.edp.intel.sdr_uses_aux) { > > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > > } else { > > const u32 pwm_level = > intel_backlight_level_to_pwm(connector, > > level); @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const struct > drm_connector_state *conn_state, u32 > > } > > } > > > > +static void > > +intel_dp_aux_write_content_luminance(struct intel_connector *connector, > > + struct hdr_output_metadata > *hdr_metadata) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + int ret; > > + u8 buf[4]; > > + > > + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) > > + return; > > I don't entirely understand intel_dp_has_gamut_metadata_dip, but I assume > it is true when the SDP HDR stuff can be send? > > Either way, you only enable the HDR_OUTPUT_METADATA property if > intel_dp_has_gamut_metadata_dip returns true, so you will only ever have > intel_dp_aux_in_hdr_mode return true if the dip exists. So, this doesn't do > anything. > > I think what you need is > > * attach the HDR_OUTPUT_METADATA property if you can send the SDP thing > and the TCON supports the SDP override (>ICL) OR you can control HDR > via AUX (>=ICL). > * always set the content luminance via AUX > > > + > > + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; > > + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) > >> 8; > > + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; > > + buf[3] = (hdr_metadata->hdmi_metadata_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); > > +} > > + > > +static void > > +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state > > +*conn_state, u8 *ctrl) { > > + struct intel_connector *connector = to_intel_connector(conn_state- > >connector); > > + struct intel_panel *panel = &connector->panel; > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + struct hdr_output_metadata *hdr_metadata = > > +conn_state->hdr_output_metadata->data; > > + > > + /* According to spec segmented backlight needs to be set whenever > panel is in > > + * HDR mode. > > + */ > > + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > + > > + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL > we disable > > + * tone mapping and set 2084 decode. > > + */ > > + if (hdr_metadata->hdmi_metadata_type1.eotf == > > + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { > > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > + } else { > > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: > Cannot decode requested EOTF\n", > > + connector->base.base.id, connector->base.name); > > + } > > + > > + if (panel->backlight.edp.intel.supports_2020_gamut && > > + (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; > > + > > + if (panel->backlight.edp.intel.supports_sdp_colorimetry && > > + intel_dp_has_gamut_metadata_dip(connector->encoder)) > > + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > > + else > > + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; > > +} > > + > > static void > > intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state > *crtc_state, > > const struct drm_connector_state > *conn_state, u32 level) @@ > > -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct > intel_crtc_state *crtc_state, > > struct intel_panel *panel = &connector->panel; > > struct drm_i915_private *i915 = to_i915(connector->base.dev); > > struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); > > + struct hdr_output_metadata *hdr_metadata; > > int ret; > > u8 old_ctrl, ctrl; > > > > @@ -261,8 +335,10 @@ 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 (intel_dp_aux_in_hdr_mode(conn_state) || > > + panel->backlight.edp.intel.sdr_uses_aux) { > > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > > + > > intel_dp_aux_hdr_set_aux_backlight(conn_state, level); > > } else { > > u32 pwm_level = intel_backlight_level_to_pwm(connector, > level); @@ > > -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct > intel_crtc_state *crtc_state, > > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; > > } > > > > + if (intel_dp_aux_in_hdr_mode(conn_state)) > > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > > + > > if (ctrl != old_ctrl && > > drm_dp_dpcd_writeb(&intel_dp->aux, > INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) > > drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to > configure DPCD brightness controls\n", > > connector->base.base.id, connector->base.name); > > + > > + if (intel_dp_aux_in_hdr_mode(conn_state)) { > > + hdr_metadata = conn_state->hdr_output_metadata->data; > > + intel_dp_aux_write_content_luminance(connector, > hdr_metadata); > > + } > > } > > > > static void > > @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct > intel_connector *connector, enum pipe pi > > 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; > >
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 2d50a4734823..7af876e2d210 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. @@ -127,9 +122,6 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) if (ret != sizeof(tcon_cap)) return false; - if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) - return false; - drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Detected %s HDR backlight interface version %d\n", connector->base.base.id, connector->base.name, is_intel_tcon_cap(tcon_cap) ? "Intel" : "unsupported", tcon_cap[0]); @@ -137,6 +129,9 @@ intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector) if (!is_intel_tcon_cap(tcon_cap)) return false; + if (!(tcon_cap[1] & INTEL_EDP_HDR_TCON_BRIGHTNESS_NITS_CAP)) + return false; + /* * If we don't have HDR static metadata there is no way to * runtime detect used range for nits based control. For now @@ -225,13 +220,27 @@ intel_dp_aux_hdr_set_aux_backlight(const struct drm_connector_state *conn_state, connector->base.base.id, connector->base.name); } +static bool +intel_dp_aux_in_hdr_mode(const struct drm_connector_state *conn_state) +{ + struct hdr_output_metadata *hdr_metadata; + + if (!conn_state->hdr_output_metadata) + return false; + + hdr_metadata = conn_state->hdr_output_metadata->data; + + return hdr_metadata->hdmi_metadata_type1.eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR; +} + static void intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 level) { 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 (intel_dp_aux_in_hdr_mode(conn_state) || + panel->backlight.edp.intel.sdr_uses_aux) { intel_dp_aux_hdr_set_aux_backlight(conn_state, level); } else { const u32 pwm_level = intel_backlight_level_to_pwm(connector, level); @@ -240,6 +249,70 @@ intel_dp_aux_hdr_set_backlight(const struct drm_connector_state *conn_state, u32 } } +static void +intel_dp_aux_write_content_luminance(struct intel_connector *connector, + struct hdr_output_metadata *hdr_metadata) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); + struct drm_i915_private *i915 = to_i915(connector->base.dev); + int ret; + u8 buf[4]; + + if (!intel_dp_has_gamut_metadata_dip(connector->encoder)) + return; + + buf[0] = hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF; + buf[1] = (hdr_metadata->hdmi_metadata_type1.max_cll & 0xFF00) >> 8; + buf[2] = hdr_metadata->hdmi_metadata_type1.max_fall & 0xFF; + buf[3] = (hdr_metadata->hdmi_metadata_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); +} + +static void +intel_dp_aux_fill_hdr_tcon_params(const struct drm_connector_state *conn_state, u8 *ctrl) +{ + struct intel_connector *connector = to_intel_connector(conn_state->connector); + struct intel_panel *panel = &connector->panel; + struct drm_i915_private *i915 = to_i915(connector->base.dev); + struct hdr_output_metadata *hdr_metadata = conn_state->hdr_output_metadata->data; + + /* According to spec segmented backlight needs to be set whenever panel is in + * HDR mode. + */ + *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; + + /* 2084 decode needs to set if eotf suggests so or in case of pre-ICL we disable + * tone mapping and set 2084 decode. + */ + if (hdr_metadata->hdmi_metadata_type1.eotf == + HDMI_EOTF_SMPTE_ST2084 || DISPLAY_VER(i915) < 11) { + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; + } else { + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] TCON: Cannot decode requested EOTF\n", + connector->base.base.id, connector->base.name); + } + + if (panel->backlight.edp.intel.supports_2020_gamut && + (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; + + if (panel->backlight.edp.intel.supports_sdp_colorimetry && + intel_dp_has_gamut_metadata_dip(connector->encoder)) + *ctrl |= INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; + else + *ctrl &= ~INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX; +} + static void intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state, u32 level) @@ -248,6 +321,7 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, struct intel_panel *panel = &connector->panel; struct drm_i915_private *i915 = to_i915(connector->base.dev); struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder); + struct hdr_output_metadata *hdr_metadata; int ret; u8 old_ctrl, ctrl; @@ -261,8 +335,10 @@ 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 (intel_dp_aux_in_hdr_mode(conn_state) || + panel->backlight.edp.intel.sdr_uses_aux) { ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; + intel_dp_aux_hdr_set_aux_backlight(conn_state, level); } else { u32 pwm_level = intel_backlight_level_to_pwm(connector, level); @@ -272,10 +348,18 @@ intel_dp_aux_hdr_enable_backlight(const struct intel_crtc_state *crtc_state, ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE; } + if (intel_dp_aux_in_hdr_mode(conn_state)) + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); + if (ctrl != old_ctrl && drm_dp_dpcd_writeb(&intel_dp->aux, INTEL_EDP_HDR_GETSET_CTRL_PARAMS, ctrl) != 1) drm_err(&i915->drm, "[CONNECTOR:%d:%s] Failed to configure DPCD brightness controls\n", connector->base.base.id, connector->base.name); + + if (intel_dp_aux_in_hdr_mode(conn_state)) { + hdr_metadata = conn_state->hdr_output_metadata->data; + intel_dp_aux_write_content_luminance(connector, hdr_metadata); + } } static void @@ -332,7 +416,6 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe pi 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 --v6 -Use intel_dp_has_gamut_dip to check if we have capability to send sdp [Ville] -Seprate filling of all hdr tcon related bits into it's own function. -Check eotf data to make sure we are in HDR mode [Sebastian] Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- .../drm/i915/display/intel_dp_aux_backlight.c | 105 ++++++++++++++++-- 1 file changed, 94 insertions(+), 11 deletions(-)