[Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Stanislav Lisovskiy]

According to spec, we should check if output_bpp * pixel_rate is less
than DDI clock * 72, if UHBR is used.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fdb..13baf3cb5f934 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1582,6 +1582,17 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
 			    pipe_config->dsc.compressed_bpp,
 			    pipe_config->dsc.slice_count);
+
+		/* wa1406899791 */
+		if (intel_dp_is_uhbr(pipe_config)) {
+			int output_bpp = pipe_config->dsc.compressed_bpp;
+
+			if (output_bpp * adjusted_mode->crtc_clock >=
+			    pipe_config->port_clock * 72) {
+				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");
+				return -EINVAL;
+			}
+		}
 	}
 	/*
 	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Patchwork]

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

== Series Details ==

Series: drm/i915: Implement workaround for DP2 UHBR bandwidth check
URL   : https://patchwork.freedesktop.org/series/112345/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12536 -> Patchwork_112345v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_112345v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_112345v1, 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_112345v1/index.html

Participating hosts (45 -> 43)
------------------------------

  Missing    (2): fi-snb-2520m fi-pnv-d510 

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

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

### IGT changes ###

#### Possible regressions ####

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

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - {bat-rpls-1}:       [DMESG-WARN][3] ([i915#6687]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12536/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112345v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_module_load@load:
    - fi-kbl-soraka:      [DMESG-WARN][5] ([i915#1982]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12536/fi-kbl-soraka/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112345v1/fi-kbl-soraka/igt@i915_module_load@load.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][7] ([i915#5334]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12536/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112345v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
    - fi-kbl-soraka:      [DMESG-FAIL][9] ([i915#5334]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12536/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112345v1/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor@atomic-transitions-varying-size:
    - {bat-adlp-9}:       [FAIL][11] ([i915#4289]) -> [PASS][12] +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12536/bat-adlp-9/igt@kms_cursor_legacy@basic-flip-before-cursor@atomic-transitions-varying-size.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112345v1/bat-adlp-9/igt@kms_cursor_legacy@basic-flip-before-cursor@atomic-transitions-varying-size.html

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

  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4289]: https://gitlab.freedesktop.org/drm/intel/issues/4289
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7554]: https://gitlab.freedesktop.org/drm/intel/issues/7554


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

  * Linux: CI_DRM_12536 -> Patchwork_112345v1

  CI-20190529: 20190529
  CI_DRM_12536: 4c18d8b1c2a1f11e99f865f60fbce9fedd3376fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7105: 305e8d105abf033cb850d1fb118e5cbfb6c9cd40 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112345v1: 4c18d8b1c2a1f11e99f865f60fbce9fedd3376fc @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

aa51e97eae11 drm/i915: Implement workaround for DP2 UHBR bandwidth check

== Logs ==

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

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

Re: [Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Rodrigo Vivi]

on the subject: This is not a hw workaround. Please remove the workaround from
the subject and the wrong comment.

"The HSD given is a 'feature' rather than 'bugeco' so no workaround details are
present here."


On Mon, Jan 02, 2023 at 01:39:37PM +0200, Stanislav Lisovskiy wrote:
> According to spec, we should check if output_bpp * pixel_rate is less
> than DDI clock * 72, if UHBR is used.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fdb..13baf3cb5f934 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1582,6 +1582,17 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
>  			    pipe_config->dsc.compressed_bpp,
>  			    pipe_config->dsc.slice_count);
> +
> +		/* wa1406899791 */

even if it was a bugeco, the notation doesn't follow the standard.

But anyway, as I pointed out, this is not a workaround so
you probably just want a

HSDES: 1406899791
BSPEC: 49259

in your commit msg.

Also maybe a "Fixes:" tag pointing to the commit that added the sequence
but didn't added this part of the sequence?

> +		if (intel_dp_is_uhbr(pipe_config)) {
> +			int output_bpp = pipe_config->dsc.compressed_bpp;
> +
> +			if (output_bpp * adjusted_mode->crtc_clock >=
> +			    pipe_config->port_clock * 72) {
> +				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");

some probably dummy question:
do we need to add a check for the DP 2.0 above as well?
or it is unecessary/redundant?

> +				return -EINVAL;
> +			}
> +		}
>  	}
>  	/*
>  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> -- 
> 2.37.3
> 

Re: [Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Lisovskiy, Stanislav]

On Tue, Jan 03, 2023 at 11:32:28AM -0500, Rodrigo Vivi wrote:
> 
> on the subject: This is not a hw workaround. Please remove the workaround from
> the subject and the wrong comment.
> 
> "The HSD given is a 'feature' rather than 'bugeco' so no workaround details are
> present here."
> 
> 
> On Mon, Jan 02, 2023 at 01:39:37PM +0200, Stanislav Lisovskiy wrote:
> > According to spec, we should check if output_bpp * pixel_rate is less
> > than DDI clock * 72, if UHBR is used.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fdb..13baf3cb5f934 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1582,6 +1582,17 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  		drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
> >  			    pipe_config->dsc.compressed_bpp,
> >  			    pipe_config->dsc.slice_count);
> > +
> > +		/* wa1406899791 */
> 
> even if it was a bugeco, the notation doesn't follow the standard.
> 
> But anyway, as I pointed out, this is not a workaround so
> you probably just want a
> 
> HSDES: 1406899791
> BSPEC: 49259
> 
> in your commit msg.

Ok, will add this thanks.

> 
> Also maybe a "Fixes:" tag pointing to the commit that added the sequence
> but didn't added this part of the sequence?
> 
> > +		if (intel_dp_is_uhbr(pipe_config)) {
> > +			int output_bpp = pipe_config->dsc.compressed_bpp;
> > +
> > +			if (output_bpp * adjusted_mode->crtc_clock >=
> > +			    pipe_config->port_clock * 72) {
> > +				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");
> 
> some probably dummy question:
> do we need to add a check for the DP 2.0 above as well?
> or it is unecessary/redundant?

I think this check is more related to hardware limitation, rather than
DP 2.0 standard. I mean if it was even not DP 2.0, but still UHBR I really doubt
that this limitation wouldn't be essential still.

Stan

> 
> > +				return -EINVAL;
> > +			}
> > +		}
> >  	}
> >  	/*
> >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > -- 
> > 2.37.3
> > 

Re: [Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Lisovskiy, Stanislav]

On Tue, Jan 10, 2023 at 02:12:17PM -0500, Rodrigo Vivi wrote:
> On Tue, Jan 10, 2023 at 02:33:38PM +0200, Stanislav Lisovskiy wrote:
> > According to spec, we should check if output_bpp * pixel_rate is less
> > than DDI clock * 72, if UHBR is used.
> > 
> > HSDES: 1406899791
> > BSPEC: 49259
> > 
> > v2: - Removed wrong comment(Rodrigo Vivi)
> >     - Added HSDES to the commit msg(Rodrigo Vivi)
> >     - Moved UHBR check to the MST specific code
> 
> I'm afraid you forgot to remove the "workaround" from the patch subject.

Ah, right, my bad!

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 8b0e4defa3f1..1f1f7f5f6501 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -339,10 +339,19 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
> >  						  conn_state, &limits,
> >  						  pipe_config->dp_m_n.tu, false);
> > -	}
> > +		if (ret < 0)
> > +			return ret;
> >  
> > -	if (ret)
> > -		return ret;
> > +		if (intel_dp_is_uhbr(pipe_config)) {
> > +			int output_bpp = pipe_config->dsc.compressed_bpp;
> > +
> > +			if (output_bpp * adjusted_mode->crtc_clock >=
> > +			    pipe_config->port_clock * 72) {
> > +				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");
> 
> I'm wondering if I misunderstood your recent reply....  but I believe you told this
> has nothing to do with DP2.0 so why we have DP2 in the msg still?
> 
> I believe that or:
> 1. We are sure this case is only happening on DP2.0 because it is impossible
> 2. or because we are adding a DP2.0 check
> 3. or we don't mention DP2.0

I think we should mention UHBR only, because it is basically more like bandwidth
limitation. It might be that it can happen only on DP2.0, but still I think
it is more correct to link it to UHBR.
I mean that limitation is most likely still valid even with DP1.4, but it simply
becomes relevant once we start using ultra high bit rate, so that in theory we
can exceed that bw limitation.

> 
> With the subject and the comment fixed:
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Yes, thank you!

> 
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> >  
> >  	ret = intel_dp_mst_update_slots(encoder, pipe_config, conn_state);
> >  	if (ret)
> > -- 
> > 2.37.3
> > 

Re: [Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Rodrigo Vivi]

On Tue, Jan 10, 2023 at 02:33:38PM +0200, Stanislav Lisovskiy wrote:
> According to spec, we should check if output_bpp * pixel_rate is less
> than DDI clock * 72, if UHBR is used.
> 
> HSDES: 1406899791
> BSPEC: 49259
> 
> v2: - Removed wrong comment(Rodrigo Vivi)
>     - Added HSDES to the commit msg(Rodrigo Vivi)
>     - Moved UHBR check to the MST specific code

I'm afraid you forgot to remove the "workaround" from the patch subject.

> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 8b0e4defa3f1..1f1f7f5f6501 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -339,10 +339,19 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
>  						  conn_state, &limits,
>  						  pipe_config->dp_m_n.tu, false);
> -	}
> +		if (ret < 0)
> +			return ret;
>  
> -	if (ret)
> -		return ret;
> +		if (intel_dp_is_uhbr(pipe_config)) {
> +			int output_bpp = pipe_config->dsc.compressed_bpp;
> +
> +			if (output_bpp * adjusted_mode->crtc_clock >=
> +			    pipe_config->port_clock * 72) {
> +				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");

I'm wondering if I misunderstood your recent reply....  but I believe you told this
has nothing to do with DP2.0 so why we have DP2 in the msg still?

I believe that or:
1. We are sure this case is only happening on DP2.0 because it is impossible
2. or because we are adding a DP2.0 check
3. or we don't mention DP2.0

With the subject and the comment fixed:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +				return -EINVAL;
> +			}
> +		}
> +	}
>  
>  	ret = intel_dp_mst_update_slots(encoder, pipe_config, conn_state);
>  	if (ret)
> -- 
> 2.37.3
> 

[Intel-gfx] [PATCH] drm/i915: Implement workaround for DP2 UHBR bandwidth check

[Stanislav Lisovskiy]

According to spec, we should check if output_bpp * pixel_rate is less
than DDI clock * 72, if UHBR is used.

HSDES: 1406899791
BSPEC: 49259

v2: - Removed wrong comment(Rodrigo Vivi)
    - Added HSDES to the commit msg(Rodrigo Vivi)
    - Moved UHBR check to the MST specific code

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 8b0e4defa3f1..1f1f7f5f6501 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -339,10 +339,19 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
 						  conn_state, &limits,
 						  pipe_config->dp_m_n.tu, false);
-	}
+		if (ret < 0)
+			return ret;
 
-	if (ret)
-		return ret;
+		if (intel_dp_is_uhbr(pipe_config)) {
+			int output_bpp = pipe_config->dsc.compressed_bpp;
+
+			if (output_bpp * adjusted_mode->crtc_clock >=
+			    pipe_config->port_clock * 72) {
+				drm_dbg_kms(&dev_priv->drm, "DP2 UHBR check failed\n");
+				return -EINVAL;
+			}
+		}
+	}
 
 	ret = intel_dp_mst_update_slots(encoder, pipe_config, conn_state);
 	if (ret)
-- 
2.37.3