| Message ID | 20240411060925.475456-6-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Aux Based EDP HDR | expand |
On Thu, Apr 11, 2024 at 11:39:24AM +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] > > --v7 > -Fix confusion function name for hdr mode check [Jani] > -Fix the condition which tells us if we are in HDR mode or not > [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 b61bad218994..b13eee250dc4 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_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_SMPTE_ST2084; > +} > + > 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_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) { this function only gets called when eotf == HDMI_EOTF_SMPTE_ST2084 which means all of this is qeuivalent to *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; The comment about pre-ICL is also really confusing to me, especially because it doesn't actually change the semantics. Can you explain what you're trying to achieve here? > + *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; The Colorspace prop can be set independently of the HDR_OUTPUT_METADATA prop but this only enables the bt2020 gamut when in HDR mode (intel_dp_in_hdr_mode() == true). > + > + 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_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_in_hdr_mode(conn_state)) > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > + I think you should call intel_dp_aux_fill_hdr_tcon_params unconditionally and in there have the logic like this: if (hdr_metadata->hdmi_metadata_type1.eotf == HDMI_EOTF_SMPTE_ST2084) { *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; } if (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB ...) { *ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; } > 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_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: Tuesday, April 16, 2024 7:10 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>; Nikula, Jani > <jani.nikula@intel.com>; Kumar, Naveen1 <naveen1.kumar@intel.com> > Subject: Re: [5/6] drm/i915/dp: Enable AUX based backlight for HDR > > On Thu, Apr 11, 2024 at 11:39:24AM +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] > > > > --v7 > > -Fix confusion function name for hdr mode check [Jani] -Fix the > > condition which tells us if we are in HDR mode or not [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 b61bad218994..b13eee250dc4 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_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_SMPTE_ST2084; } > > + > > 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_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) { > > this function only gets called when eotf == HDMI_EOTF_SMPTE_ST2084 which > means all of this is qeuivalent to > > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > Right will drop the SMPTE check altogether > The comment about pre-ICL is also really confusing to me, especially because it > doesn't actually change the semantics. Can you explain what you're trying to > achieve here? > Spec states that we need to disable Tone mapping pre-ICL and after that it will Be TCON's decision to enable TM based on panel override values > > + *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; > > The Colorspace prop can be set independently of the > HDR_OUTPUT_METADATA prop but this only enables the bt2020 gamut when > in HDR mode > (intel_dp_in_hdr_mode() == true). > there's only two states in the DPCD register Standard gamma and bt2020 now this case here is specific to when we are in HDR mode and do not want to write in this register when not in HDR mode. > > + > > + 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_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_in_hdr_mode(conn_state)) > > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > > + > > I think you should call intel_dp_aux_fill_hdr_tcon_params unconditionally and > in there have the logic like this: > > if (hdr_metadata->hdmi_metadata_type1.eotf == > HDMI_EOTF_SMPTE_ST2084) { > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > } > if (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB ...) { > *ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; > } > I'll be dropping the condition inside the intel_dp_aux_fill_hdr_tcon_params As writing to DPCD and filling in the params does not make sense until we are in HDR mode. Regards, Suraj Kandpal > > 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_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 Wed, Apr 17, 2024 at 04:58:06AM +0000, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Sebastian Wick <sebastian.wick@redhat.com> > > Sent: Tuesday, April 16, 2024 7:10 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>; Nikula, Jani > > <jani.nikula@intel.com>; Kumar, Naveen1 <naveen1.kumar@intel.com> > > Subject: Re: [5/6] drm/i915/dp: Enable AUX based backlight for HDR > > > > On Thu, Apr 11, 2024 at 11:39:24AM +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] > > > > > > --v7 > > > -Fix confusion function name for hdr mode check [Jani] -Fix the > > > condition which tells us if we are in HDR mode or not [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 b61bad218994..b13eee250dc4 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_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_SMPTE_ST2084; } > > > + > > > 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_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) { > > > > this function only gets called when eotf == HDMI_EOTF_SMPTE_ST2084 which > > means all of this is qeuivalent to > > > > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > > > Right will drop the SMPTE check altogether > > > The comment about pre-ICL is also really confusing to me, especially because it > > doesn't actually change the semantics. Can you explain what you're trying to > > achieve here? > > > > Spec states that we need to disable Tone mapping pre-ICL and after that it will > Be TCON's decision to enable TM based on panel override values Right, but the TCON will just ignore it so you always set it. > > > > + *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; > > > > The Colorspace prop can be set independently of the > > HDR_OUTPUT_METADATA prop but this only enables the bt2020 gamut when > > in HDR mode > > (intel_dp_in_hdr_mode() == true). > > > > there's only two states in the DPCD register Standard gamma and bt2020 > now this case here is specific to when we are in HDR mode and do not want to write > in this register when not in HDR mode. That doesn't seem right. bt2020 gamut is completely independent of 2084 decode and segmented backlight (i.e. HDR mode). From a uAPI perspective when the Colorspace prop is set to bt2020 we need the sink to expect bt2020 content, no matter any other property. The question really is if on devices with the TCON are you able to get it to process bt2020 without setting INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE? I don't believe so. From the testing we've done it looks like DPCD is ignored unless INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX is set. > > > > + > > > + 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_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_in_hdr_mode(conn_state)) > > > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > > > + > > > > I think you should call intel_dp_aux_fill_hdr_tcon_params unconditionally and > > in there have the logic like this: > > > > if (hdr_metadata->hdmi_metadata_type1.eotf == > > HDMI_EOTF_SMPTE_ST2084) { > > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > } > > if (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB ...) { > > *ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; > > } > > > > I'll be dropping the condition inside the intel_dp_aux_fill_hdr_tcon_params > As writing to DPCD and filling in the params does not make sense until we are in HDR mode. See above. I believe HDR mode is just one thing you have to handle in the TCON and the gamut processing must be set independently. > > Regards, > Suraj Kandpal > > > > 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_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: Wednesday, April 17, 2024 3:54 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>; Nikula, Jani > <jani.nikula@intel.com>; Kumar, Naveen1 <naveen1.kumar@intel.com> > Subject: Re: [5/6] drm/i915/dp: Enable AUX based backlight for HDR > > On Wed, Apr 17, 2024 at 04:58:06AM +0000, Kandpal, Suraj wrote: > > > > > > > -----Original Message----- > > > From: Sebastian Wick <sebastian.wick@redhat.com> > > > Sent: Tuesday, April 16, 2024 7:10 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>; Nikula, Jani <jani.nikula@intel.com>; > > > Kumar, Naveen1 <naveen1.kumar@intel.com> > > > Subject: Re: [5/6] drm/i915/dp: Enable AUX based backlight for HDR > > > > > > On Thu, Apr 11, 2024 at 11:39:24AM +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] > > > > > > > > --v7 > > > > -Fix confusion function name for hdr mode check [Jani] -Fix the > > > > condition which tells us if we are in HDR mode or not [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 b61bad218994..b13eee250dc4 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_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_SMPTE_ST2084; } > > > > + > > > > 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_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) { > > > > > > this function only gets called when eotf == HDMI_EOTF_SMPTE_ST2084 > > > which means all of this is qeuivalent to > > > > > > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > > > > > > Right will drop the SMPTE check altogether > > > > > The comment about pre-ICL is also really confusing to me, especially > > > because it doesn't actually change the semantics. Can you explain > > > what you're trying to achieve here? > > > > > > > Spec states that we need to disable Tone mapping pre-ICL and after > > that it will Be TCON's decision to enable TM based on panel override > > values > > Right, but the TCON will just ignore it so you always set it. > Sure will rework this patch a little > > > > > > + *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; > > > > > > The Colorspace prop can be set independently of the > > > HDR_OUTPUT_METADATA prop but this only enables the bt2020 gamut > when > > > in HDR mode > > > (intel_dp_in_hdr_mode() == true). > > > > > > > there's only two states in the DPCD register Standard gamma and bt2020 > > now this case here is specific to when we are in HDR mode and do not > > want to write in this register when not in HDR mode. > > That doesn't seem right. bt2020 gamut is completely independent of 2084 > decode and segmented backlight (i.e. HDR mode). From a uAPI perspective > when the Colorspace prop is set to bt2020 we need the sink to expect > bt2020 content, no matter any other property. > > The question really is if on devices with the TCON are you able to get it to > process bt2020 without setting > INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE? I don't believe so. From the > testing we've done it looks like DPCD is ignored unless > INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX is set. > > > > > > > + > > > > + 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_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_in_hdr_mode(conn_state)) > > > > + intel_dp_aux_fill_hdr_tcon_params(conn_state, &ctrl); > > > > + > > > > > > I think you should call intel_dp_aux_fill_hdr_tcon_params > > > unconditionally and in there have the logic like this: > > > > > > if (hdr_metadata->hdmi_metadata_type1.eotf == > > > HDMI_EOTF_SMPTE_ST2084) { > > > *ctrl |= INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE; > > > *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE; > > > *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE; > > > } > > > if (conn_state->colorspace == DRM_MODE_COLORIMETRY_BT2020_RGB > ...) { > > > *ctrl |= INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE; > > > } > > > > > > > I'll be dropping the condition inside the > > intel_dp_aux_fill_hdr_tcon_params As writing to DPCD and filling in the > params does not make sense until we are in HDR mode. > > See above. I believe HDR mode is just one thing you have to handle in the > TCON and the gamut processing must be set independently. > Sure got your point will rework it to have bt 2020 gamut set regardess of when in hdr mode or not. Regards, Suraj Kandpal > > > > Regards, > > Suraj Kandpal > > > > > > 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_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 b61bad218994..b13eee250dc4 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_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_SMPTE_ST2084; +} + 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_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_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_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_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] --v7 -Fix confusion function name for hdr mode check [Jani] -Fix the condition which tells us if we are in HDR mode or not [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(-)