Re: [Intel-gfx] [PATCH] drm/i915: Check EDID before dpcd for possible HDR aux bl support

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2022-04-13 at 17:08 -0400, Lyude Paul wrote:
> On Wed, 2022-04-13 at 08:31 +0000, Hogander, Jouni wrote:
> > 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@redhat.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Tested-by: Filippo Falezza <filippo.falezza@outlook.it>
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  .../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?
> 
> Yes, probably - I think just making it so that
> i915.enable_dpcd_backlight=3
> enables the HDR backlight regardless of the results of the EDID check
> would
> probably be a good idea.

Ok, I think I understood you correctly originally. If you could please
check version 2:

https://patchwork.freedesktop.org/patch/481985/?series=102645&rev=1

> 
> > 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@vger.kernel.org
> > 
> > tag from dim added here using `dim fixes $commit`.
> > 
> > With that fixed:
> > 
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > 
> > +
> >         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
> >