[RFC] drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

[RFC] drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

[Charlton Lin]

Driver currently limits link rate up to HBR3 in SST mode. Log a
message with monitor vendor, product id, and MSTM_CAP to
help understand what monitors are being downgraded by this limit.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Charlton Lin <charlton.lin@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 6ece2c563c7a..0b2d6d88fd37 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 						     false,
 						     &limits);
 
+	if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) {
+		u8 mstm_cap;
+		u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc);
+		char vend[4];
+		u16 product_id;
+
+		drm_dbg_kms(&i915->drm,
+			    "Limiting LR from max common rate %d to %d\n",
+			    intel_dp_max_common_rate(intel_dp), limits.max_rate);
+
+		drm_edid_decode_panel_id(panel_id, vend, &product_id);
+
+		if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 &&
+		    drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1)
+			drm_dbg_kms(&i915->drm,
+				    "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n",
+				    vend, product_id, mstm_cap);
+	}
+
 	if (!dsc_needed) {
 		/*
 		 * Optimize for slow and wide for everything, because there are some
-- 
2.25.1

Re: [RFC] drm/i915/dp: Log message when limiting SST link rate

[Jani Nikula]

On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@intel.com> wrote:
> Driver currently limits link rate up to HBR3 in SST mode. Log a
> message with monitor vendor, product id, and MSTM_CAP to
> help understand what monitors are being downgraded by this limit.

Any logging of the sink details should be done exactly once at detect
time. No matter what happens after that, there's no need to spam the
dmesg with duplicate information. If the information currently logged is
insufficient, please add it at detect, where it helps *all* debugging,
not just a single restricted case.

More details inline.

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Charlton Lin <charlton.lin@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6ece2c563c7a..0b2d6d88fd37 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  						     false,
>  						     &limits);
>  
> +	if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) {

If link training has failed and the max rate is limited due to that,
you'll hit this on all retraining attempts.

And, of course, this is hit at every single modeset for 128b/132b
capable displays that don't support MST. That's just excessive.

> +		u8 mstm_cap;
> +		u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc);

That reads the entire EDID base block again in the middle of compute
config. A big no. We've also taken care to cache the EDID to avoid
duplicate reads otherwise.

Moreover, it ignores any EDID overrides etc.

> +		char vend[4];
> +		u16 product_id;
> +
> +		drm_dbg_kms(&i915->drm,
> +			    "Limiting LR from max common rate %d to %d\n",
> +			    intel_dp_max_common_rate(intel_dp), limits.max_rate);
> +
> +		drm_edid_decode_panel_id(panel_id, vend, &product_id);
> +
> +		if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 &&
> +		    drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1)

We shouldn't add extra DPCD reads nilly willy either. This should be
debug logged once at detect. I've got a WIP series that should address
this [1], once I fix it.

> +			drm_dbg_kms(&i915->drm,
> +				    "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n",
> +				    vend, product_id, mstm_cap);

Ville and I have discussed adding entire EDID debug logging at
drm_edid.c level, which would address a lot more things than this.

BR,
Jani.


[1] https://patchwork.freedesktop.org/series/129468/



> +	}
> +
>  	if (!dsc_needed) {
>  		/*
>  		 * Optimize for slow and wide for everything, because there are some

-- 
Jani Nikula, Intel

Re: [RFC] drm/i915/dp: Log message when limiting SST link rate

[Ville Syrjälä]

On Fri, Mar 01, 2024 at 11:07:43AM +0200, Jani Nikula wrote:
> On Thu, 29 Feb 2024, Charlton Lin <charlton.lin@intel.com> wrote:
> > Driver currently limits link rate up to HBR3 in SST mode. Log a
> > message with monitor vendor, product id, and MSTM_CAP to
> > help understand what monitors are being downgraded by this limit.
> 
> Any logging of the sink details should be done exactly once at detect
> time. No matter what happens after that, there's no need to spam the
> dmesg with duplicate information. If the information currently logged is
> insufficient, please add it at detect, where it helps *all* debugging,
> not just a single restricted case.
> 
> More details inline.
> 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Charlton Lin <charlton.lin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 6ece2c563c7a..0b2d6d88fd37 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  						     false,
> >  						     &limits);
> >  
> > +	if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) {
> 
> If link training has failed and the max rate is limited due to that,
> you'll hit this on all retraining attempts.
> 
> And, of course, this is hit at every single modeset for 128b/132b
> capable displays that don't support MST. That's just excessive.
> 
> > +		u8 mstm_cap;
> > +		u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc);
> 
> That reads the entire EDID base block again in the middle of compute
> config. A big no. We've also taken care to cache the EDID to avoid
> duplicate reads otherwise.
> 
> Moreover, it ignores any EDID overrides etc.
> 
> > +		char vend[4];
> > +		u16 product_id;
> > +
> > +		drm_dbg_kms(&i915->drm,
> > +			    "Limiting LR from max common rate %d to %d\n",
> > +			    intel_dp_max_common_rate(intel_dp), limits.max_rate);
> > +
> > +		drm_edid_decode_panel_id(panel_id, vend, &product_id);
> > +
> > +		if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 &&
> > +		    drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1)
> 
> We shouldn't add extra DPCD reads nilly willy either. This should be
> debug logged once at detect. I've got a WIP series that should address
> this [1], once I fix it.

Also the stone tablets say "thou shalt not touch the hardware
in atomic_check()/.compute_config()".

This stuff gets executed for every TEST_ONLY commit, which means it:
- potentially runs while the hardware is asleep
- should be relatively fast

> 
> > +			drm_dbg_kms(&i915->drm,
> > +				    "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n",
> > +				    vend, product_id, mstm_cap);
> 
> Ville and I have discussed adding entire EDID debug logging at
> drm_edid.c level, which would address a lot more things than this.
> 
> BR,
> Jani.
> 
> 
> [1] https://patchwork.freedesktop.org/series/129468/
> 
> 
> 
> > +	}
> > +
> >  	if (!dsc_needed) {
> >  		/*
> >  		 * Optimize for slow and wide for everything, because there are some
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel

✗ Fi.CI.BAT: failure for drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

[Patchwork]

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

== Series Details ==

Series: drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
URL   : https://patchwork.freedesktop.org/series/130597/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14376 -> Patchwork_130597v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_130597v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_130597v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) 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_130597v1/index.html

Participating hosts (42 -> 39)
------------------------------

  Missing    (3): bat-mtlp-8 bat-kbl-2 fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@dmabuf@all-tests@dma_fence:
    - bat-arls-1:         [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/bat-arls-1/igt@dmabuf@all-tests@dma_fence.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/bat-arls-1/igt@dmabuf@all-tests@dma_fence.html

  * igt@dmabuf@all-tests@sanitycheck:
    - bat-arls-1:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/bat-arls-1/igt@dmabuf@all-tests@sanitycheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/bat-arls-1/igt@dmabuf@all-tests@sanitycheck.html

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

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

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-tgl-1115g4:      [PASS][5] -> [FAIL][6] ([i915#8293])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/fi-tgl-1115g4/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/fi-tgl-1115g4/boot.html
    - fi-cfl-8109u:       [PASS][7] -> [FAIL][8] ([i915#8293])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/fi-cfl-8109u/boot.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/fi-cfl-8109u/boot.html

  
#### Possible fixes ####

  * boot:
    - fi-apl-guc:         [FAIL][9] ([i915#8293]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/fi-apl-guc/boot.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/fi-apl-guc/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@basic:
    - fi-apl-guc:         NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#4613]) +3 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/fi-apl-guc/igt@gem_lmem_swapping@basic.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-apl-guc:         NOTRUN -> [SKIP][12] ([fdo#109271]) +17 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/fi-apl-guc/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pipe_crc_basic@nonblocking-crc:
    - bat-dg2-11:         NOTRUN -> [SKIP][13] ([i915#9197])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - bat-adlp-9:         [INCOMPLETE][14] ([i915#9413]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@hangcheck:
    - {bat-rpls-3}:       [DMESG-WARN][16] ([i915#5591]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14376/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130597v1/bat-rpls-3/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#9197]: https://gitlab.freedesktop.org/drm/intel/issues/9197
  [i915#9413]: https://gitlab.freedesktop.org/drm/intel/issues/9413


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

  * Linux: CI_DRM_14376 -> Patchwork_130597v1

  CI-20190529: 20190529
  CI_DRM_14376: 320f2374b2d52629a6a52c6c3059f02efd73c0a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7741: 7741
  Patchwork_130597v1: 320f2374b2d52629a6a52c6c3059f02efd73c0a5 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

e2461da545ba drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

== Logs ==

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

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

Re: [RFC] drm/i915/dp: Log message when limiting SST link rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

[Manasi Navare]

Thanks Charlton for the patch.
I think in general it is a good idea to log this when the max rate is
dropped to HBR3 for SST case.

Please find my comments below,


On Thu, Feb 29, 2024 at 11:49 PM Charlton Lin <charlton.lin@intel.com> wrote:
>
> Driver currently limits link rate up to HBR3 in SST mode. Log a
> message with monitor vendor, product id, and MSTM_CAP to
> help understand what monitors are being downgraded by this limit.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Charlton Lin <charlton.lin@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6ece2c563c7a..0b2d6d88fd37 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2437,6 +2437,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>                                                      false,
>                                                      &limits);
>
> +       if (intel_dp_max_common_rate(intel_dp) > limits.max_rate) {
> +               u8 mstm_cap;
> +               u32 panel_id = drm_edid_get_panel_id(&intel_dp->aux.ddc);
> +               char vend[4];
> +               u16 product_id;
> +
> +               drm_dbg_kms(&i915->drm,
> +                           "Limiting LR from max common rate %d to %d\n",

We dont use LR acronym anywhere in the kernel for link rate, just say
link rate here.
Also I think would be good to log the reason why we are dropping this
to HBR3 or add a comment with a Todo
for this

Manasi

> +                           intel_dp_max_common_rate(intel_dp), limits.max_rate);
> +
> +               drm_edid_decode_panel_id(panel_id, vend, &product_id);
> +
> +               if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12 &&
> +                   drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) == 1)
> +                       drm_dbg_kms(&i915->drm,
> +                                   "Manufacturer=%s Model=%x Sink MSTM_CAP=%x\n",
> +                                   vend, product_id, mstm_cap);
> +       }
> +
>         if (!dsc_needed) {
>                 /*
>                  * Optimize for slow and wide for everything, because there are some
> --
> 2.25.1
>