> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Wednesday, March 6, 2024 5:57 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915/dp: Enable AUX based backlight for HDR
>
> On Wed, Mar 06, 2024 at 02:29:15PM +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]
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > .../drm/i915/display/intel_dp_aux_backlight.c | 30
> > ++++++++++++++-----
> > 1 file changed, 22 insertions(+), 8 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 4f58efdc688a..1b6f14dacf3b 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.
> > @@ -221,7 +216,7 @@ intel_dp_aux_hdr_set_backlight(const struct
> drm_connector_state *conn_state, u32
> > struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> > struct intel_panel *panel = &connector->panel;
> >
> > - if (panel->backlight.edp.intel.sdr_uses_aux) {
> > + if (panel->backlight.edp.intel.sdr_uses_aux ||
> > +conn_state->hdr_output_metadata) {
> > intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
> > } else {
> > const u32 pwm_level =
> intel_backlight_level_to_pwm(connector,
> > level); @@ -251,8 +246,15 @@ intel_dp_aux_hdr_enable_backlight(const
> struct intel_crtc_state *crtc_state,
> > }
> >
> > ctrl = old_ctrl;
> > - if (panel->backlight.edp.intel.sdr_uses_aux) {
> > + if (panel->backlight.edp.intel.sdr_uses_aux ||
> > +conn_state->hdr_output_metadata) {
> > ctrl |= INTEL_EDP_HDR_TCON_BRIGHTNESS_AUX_ENABLE;
> > +
> > + if (conn_state->hdr_output_metadata) {
> > + ctrl |=
> INTEL_EDP_HDR_TCON_SEGMENTED_BACKLIGHT_ENABLE;
> > + ctrl |=
> INTEL_EDP_HDR_TCON_2084_DECODE_ENABLE;
> > + ctrl |=
> INTEL_EDP_HDR_TCON_2020_GAMUT_ENABLE;
> > + }
>
> Wasn't bunch of this stuff supposed to be only used with older hw, and more
> recent panels were supposed to pick this up more or less automagically from
> the HDR metadata?
>
> I seem to also recall that there are a bunch of capability bits we should
> probably be checking somewhere...
>
Hi Ville,
Thanks for the review
You are correct I had a look into it, content luminance will will fill from the static metadata from ICL onwards will get that corrected
As for capability check that was already being done in the function intel_dp_aux_supports_hdr_backlight()
I soon send a revision with the fixes.
Regards,
Suraj Kandpal
> > +
> > intel_dp_aux_hdr_set_aux_backlight(conn_state, level);
> > } else {
> > u32 pwm_level = intel_backlight_level_to_pwm(connector,
> level); @@
> > -292,9 +294,11 @@ intel_dp_aux_hdr_setup_backlight(struct
> > intel_connector *connector, enum pipe pi {
> > struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > struct intel_panel *panel = &connector->panel;
> > + struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
> > struct drm_luminance_range_info *luminance_range =
> > &connector->base.display_info.luminance_range;
> > int ret;
> > + u8 buf[4];
> >
> > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] SDR backlight is
> controlled through %s\n",
> > connector->base.base.id, connector->base.name, @@ -
> 318,11
> > +322,21 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector
> *connector, enum pipe pi
> > panel->backlight.min = 0;
> > }
> >
> > + buf[0] = connector->base.hdr_sink_metadata.hdmi_type1.max_cll &
> 0xFF;
> > + buf[1] = (connector->base.hdr_sink_metadata.hdmi_type1.max_cll &
> 0xFF00) >> 8;
> > + buf[2] = connector->base.hdr_sink_metadata.hdmi_type1.max_fall &
> 0xFF;
> > + buf[3] = (connector->base.hdr_sink_metadata.hdmi_type1.max_fall
> &
> > +0xFF00) >> 8;
> > +
> > + ret = drm_dp_dpcd_write(&intel_dp->aux,
> INTEL_EDP_HDR_CONTENT_LUMINANCE, buf,
> > + sizeof(buf));
> > + if (ret < 0)
> > + drm_dbg_kms(&i915->drm,
> > + "Not able to write content luminence to DPCD
> register
> > +err:-%d\n", ret);
> > +
> > drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] Using AUX HDR
> interface for backlight control (range %d..%d)\n",
> > connector->base.base.id, connector->base.name,
> > panel->backlight.min, panel->backlight.max);
> >
> > -
> > panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector,
> pipe);
> > panel->backlight.enabled = panel->backlight.level != 0;
> >
> > --
> > 2.43.2
>
> --
> Ville Syrjälä
> Intel
