On Wed, Apr 24, 2024 at 5:59 AM Kandpal, Suraj <suraj.kandpal@xxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
> > Sent: Wednesday, April 24, 2024 2:51 AM
> > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah@xxxxxxxxx>; Shankar, Uma
> > <uma.shankar@xxxxxxxxx>; Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>;
> > Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; Kumar, Naveen1
> > <naveen1.kumar@xxxxxxxxx>
> > Subject: Re: [5/6] drm/i915/dp: Enable AUX based backlight for HDR
> >
> > On Mon, Apr 22, 2024 at 09:02:54AM +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]
> > >
> > > --v8
> > > -Call fill_hdr_tcon_param unconditionally as some parameters may not
> > > be dependent on the fact if we are in hdr mode or not [Sebastian] -Fix
> > > some conditions after change in hdr mode check [Sebastian]
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > > ---
> > > .../drm/i915/display/intel_dp_aux_backlight.c | 98
> > > ++++++++++++++++---
> > > 1 file changed, 87 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..e23694257ea5 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,64 @@ 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;
> >
> > The usage of intel_dp_has_gamut_metadata_dip is all over the place. You
> > happily set INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE and
> > INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE even when it doesn't have
> > the dip but you require it for INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX
> > and INTEL_EDP_HDR_CONTENT_LUMINANCE. Why?
> >
> > Especially because when there is no gamut_metadata_dip, the KMS properties
> > for HDR is not exposed.
> >
>
> As I have iterated multiple times before segmented backlight needs to be set
> regardless of H/W's capabilities to send SDP(the spec demands it). Having it set based on a KMS property
> Is a whole different conversation keeping in mind this is an INTEL specific spec DPCD
> reg bit which needs to be set, do we really want to expose a property that only intel
> H/W will use does not seem right but as I said a discussion that needs to be taken up later
> and should not be a part of this patch series which enables HDR and AUX based backlight.
I don't have the spec and can only speculate what's going on, but I
doubt a lot that you cannot enter HDR mode without segmented
backlight. Did you actually test this?
Control over segmented backlight is part of both Vulkan and DX so I
don't think it's far-fetched to expose this to KMS.
Either way, I agree with you, this is something for the future.
> INTEL_EDP_HDR_CONTENT_LUMINANCE is only set in cases DISPLAY_VER < 11 that's why the
> Dip check there since it return true in case DISPLAY_VER >= 11 or if port is not A (which is used for EDP).
> INTEL_EDP_HDR_TCON_SDP_OVERRIDE_AUX well and this need a dip check which seems obvious since we
> Would only want to override aux if SDP is present.
> So in short I am pretty happy with the bits that have been set and the conditions used to set them.
The dip check is never going to be reached when the dip check fails in
the first place. You only expose the property when the dip check
succeeds, thus you only can be in hdr mode if it succeeds and you only
ever call intel_dp_aux_write_content_luminance if the connector is in
hdr mode. It is redundant here but it's your driver and it still works
just fine as is.
For the whole series:
Reviewed-by: Sebastian Wick <sebastian.wick@xxxxxxxxxx>
>
> > > +
> > > + 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);
> > > +
> > > + /*
> > > + * According to spec segmented backlight needs to be set whenever
> > panel is in
> > > + * HDR mode.
> > > + */
> > > + if (intel_dp_in_hdr_mode(conn_state)) {
> > > + *ctrl |=
> > INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE;
> > > + *ctrl |= INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE;
> > > + }
> > > +
> > > + if (DISPLAY_VER(i915) < 11)
> > > + *ctrl &= ~INTEL_EDP_HDR_TCON_TONE_MAPPING_ENABLE;
> > > +
> > > + 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 +315,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 +329,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 +342,17 @@ intel_dp_aux_hdr_enable_backlight(const struct
> > intel_crtc_state *crtc_state,
> > > ctrl &= ~INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
> > > }
> > >
> > > + 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);
> >
> > Unrelated to the changes here, but why is crtl based on the old value?
> > There is nothing else that sets it so the state is always entirely determined
> > here.
> >
>
> We read the getset_param register and set oldctrl and ctrl. Then ctrl is changed based on
> State and other params. In case ctrl and old ctrl still end up being the same value that means
> No change in DPCD register is required as of now so let's not waste a AUX write operation on that
>
> Regards,
> Suraj Kandpal
> > > +
> > > + 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 +409,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;
> > >
>
