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

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

[Jouni Högander]

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;
+
 	intel_dp_wait_source_oui(intel_dp);
 
 	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));
-- 
2.25.1

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

[Jani Nikula]

On Tue, 12 Apr 2022, Jouni Högander <jouni.hogander@intel.com> 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;

Considering the complexities around this, I'd probably start gathering
the info in variables, then debug log all of it, with the conclusion the
driver makes. It's makes future debugging much easier.

Other than that, I guess

Acked-by: Jani Nikula <jani.nikula@intel.com>

because I don't really know what's going on with these...

BR,
Jani.

> +
>  	intel_dp_wait_source_oui(intel_dp);
>  
>  	ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap, sizeof(tcon_cap));

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Check EDID before dpcd for possible HDR aux bl support

[Patchwork]

== Series Details ==

Series: drm/i915: Check EDID before dpcd for possible HDR aux bl support
URL   : https://patchwork.freedesktop.org/series/102571/
State : warning

== Summary ==

Error: git fetch origin failed

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Check EDID before dpcd for possible HDR aux bl support

[Patchwork]

== Series Details ==

Series: drm/i915: Check EDID before dpcd for possible HDR aux bl support
URL   : https://patchwork.freedesktop.org/series/102571/
State : warning

== Summary ==

Error: git fetch origin failed

[Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915: Check EDID before dpcd for possible HDR aux bl support

[Patchwork]

== Series Details ==

Series: drm/i915: Check EDID before dpcd for possible HDR aux bl support
URL   : https://patchwork.freedesktop.org/series/102571/
State : warning

== Summary ==

Error: git fetch origin failed

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Check EDID before dpcd for possible HDR aux bl support

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4589 bytes --]

== Series Details ==

Series: drm/i915: Check EDID before dpcd for possible HDR aux bl support
URL   : https://patchwork.freedesktop.org/series/102571/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11486 -> Patchwork_102571v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_102571v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_102571v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/index.html

Participating hosts (47 -> 36)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (12): fi-bdw-samus bat-dg1-6 bat-dg1-5 fi-icl-u2 bat-dg2-9 bat-adlp-6 fi-bsw-cyan bat-adlp-4 bat-rpls-1 bat-rpls-2 bat-jsl-2 bat-jsl-1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_102571v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-kbl-soraka:      [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11486/fi-kbl-soraka/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-kbl-soraka/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@vma:
    - fi-bdw-5557u:       NOTRUN -> [INCOMPLETE][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-bdw-5557u/igt@i915_selftest@live@vma.html

  
Known issues
------------

  Here are the changes found in Patchwork_102571v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@dp-crc-fast:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][4] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-bdw-5557u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - fi-pnv-d510:        NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#5341])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-pnv-d510/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][6] ([fdo#109271]) +14 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-bdw-5557u/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-pnv-d510:        NOTRUN -> [SKIP][7] ([fdo#109271]) +39 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-pnv-d510/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b:
    - fi-cfl-8109u:       [DMESG-WARN][8] ([i915#62]) -> [PASS][9] +11 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11486/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - fi-cfl-8109u:       [DMESG-WARN][10] ([i915#5341] / [i915#62]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11486/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#5341]: https://gitlab.freedesktop.org/drm/intel/issues/5341
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62


Build changes
-------------

  * Linux: CI_DRM_11486 -> Patchwork_102571v1

  CI-20190529: 20190529
  CI_DRM_11486: 39e55253777647f016e2330635c475d672390bfb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6419: 33a5adf20dc435cc2c6dd584caa3674c89032762 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_102571v1: 102571v1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

913d08420f21 drm/i915: Check EDID before dpcd for possible HDR aux bl support

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v1/index.html

[-- Attachment #2: Type: text/html, Size: 5751 bytes --]

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Check EDID before dpcd for possible HDR aux bl support (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5574 bytes --]

== Series Details ==

Series: drm/i915: Check EDID before dpcd for possible HDR aux bl support (rev2)
URL   : https://patchwork.freedesktop.org/series/102571/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11488 -> Patchwork_102571v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_102571v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_102571v2, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/index.html

Participating hosts (46 -> 37)
------------------------------

  Additional (2): fi-icl-u2 fi-pnv-d510 
  Missing    (11): fi-bdw-samus bat-dg1-6 bat-dg2-8 bat-dg2-9 fi-bsw-cyan bat-adlp-6 bat-adlp-4 bat-rpls-1 bat-rpls-2 bat-jsl-2 bat-jsl-1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_102571v2:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_create@basic:
    - fi-snb-2600:        NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-snb-2600/igt@gem_exec_create@basic.html

  
Known issues
------------

  Here are the changes found in Patchwork_102571v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-icl-u2:          NOTRUN -> [SKIP][2] ([i915#2190])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-icl-u2:          NOTRUN -> [SKIP][3] ([i915#4613]) +3 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          NOTRUN -> [SKIP][4] ([fdo#111827]) +8 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-icl-u2:          NOTRUN -> [SKIP][5] ([fdo#109278]) +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u2:          NOTRUN -> [SKIP][6] ([fdo#109285])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - fi-pnv-d510:        NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#5341])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-pnv-d510/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-icl-u2:          NOTRUN -> [SKIP][8] ([i915#3555])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-pnv-d510:        NOTRUN -> [SKIP][9] ([fdo#109271]) +39 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-pnv-d510/igt@prime_vgem@basic-userptr.html
    - fi-icl-u2:          NOTRUN -> [SKIP][10] ([i915#3301])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-icl-u2/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-bdw-5557u:       NOTRUN -> [FAIL][11] ([i915#4312])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-bdw-5557u/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-bdw-5557u:       [INCOMPLETE][12] ([i915#146]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11488/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5341]: https://gitlab.freedesktop.org/drm/intel/issues/5341


Build changes
-------------

  * Linux: CI_DRM_11488 -> Patchwork_102571v2

  CI-20190529: 20190529
  CI_DRM_11488: 72ba03880bae2830ad7651a0156c415271712618 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6420: a3885810ccc0ce9e6552a20c910a0a322eca466c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_102571v2: 102571v2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

27ef3a20ae5d drm/i915: Check EDID before dpcd for possible HDR aux bl support

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102571v2/index.html

[-- Attachment #2: Type: text/html, Size: 6550 bytes --]

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

[Lyude Paul]

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

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));

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Hogander, Jouni]

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?

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

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

[Lyude Paul]

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.

> 
> 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
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Hogander, Jouni]

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
> >