Hello Lyude,
See my respose below.
On Tue, 2022-04-12 at 13:50 -0400, Lyude Paul wrote:
> On Tue, 2022-04-12 at 08:25 +0300, Jouni Högander wrote:
> > We have now seen panel (XMG Core 15 e21 laptop) avertizing support
> > for Intel proprietary eDP backlight control via DPCD registers, but
> > actually working only with legacy pwm control.
> >
> > This patch adds panel EDID check for possible HDR static metadata
> > and
> > does detection from DPCD registers only if this data block exists.
> >
> > Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight
> > interface
> > (only SDR for now)")
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5284
> > Cc: Lyude Paul <lyude@xxxxxxxxxx>
> > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Tested-by: Filippo Falezza <filippo.falezza@xxxxxxxxxx>
> > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > ---
> > .../gpu/drm/i915/display/intel_dp_aux_backlight.c | 13
> > +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > 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 97cf3cac0105..f69e185b58c1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> > @@ -108,6 +108,19 @@ intel_dp_aux_supports_hdr_backlight(struct
> > intel_connector *connector)
> > int ret;
> > u8 tcon_cap[4];
> >
> > + /*
> > + * If we don't have HDR static metadata there is no way to
> > + * runtime detect used range for nits based control. For
> > now
> > + * do not use Intel proprietary eDP backlight control if we
> > + * don't have this data in panel EDID. In case we find
> > panel
> > + * which supports only nits based control, but doesn't
> > provide
> > + * HDR static metadata we need to start maintaining table
> > of
> > + * ranges for such panels.
> > + */
> > + if (!(connector-
> > >base.hdr_sink_metadata.hdmi_type1.metadata_type &
> > + BIT(HDMI_STATIC_METADATA_TYPE1)))
> > + return false;
>
> The block used for this is actually for HDMI?? How bizarre…
>
> Anyway yeah - patch looks good to me, but I think we should print a
> debugging
> message of some sort when we determine that there's no HDR backlight
> because
> of the EDID - along with printing instructions for how the user can
> override
> it if we've made the wrong choice along with reporting a bug. Also -
> we should
> have the
hmm, currently there is no override possibility
in intel_dp_aux_supports_hdr_backlight. Do you think I should add one?
I sent version 2. Didn't add your rb there as I wasn't sure if I
understood your comment correctly. Please check new version.
> Cc: stable@xxxxxxxxxxxxxxx
tag from dim added here using `dim fixes $commit`.
With that fixed:
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
+
intel_dp_wait_source_oui(intel_dp);
ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0,
tcon_cap,
sizeof(tcon_cap));
BR,
Jouni Högander
