[PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[Animesh Manna]

Panel Replay VSC SDP not getting sent when VRR is enabled
and W1 and W2 are 0. So Program Set Context Latency in
TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.

HSD: 14015406119

v1: Initial version.
v2: Update timings stored in adjusted_mode struct. [Ville]
v3: Add WA in compute_config(). [Ville]
v4:
- Add DISPLAY_VER() check and improve code comment. [Rodrigo]
- Introduce centralized intel_crtc_vblank_delay(). [Ville]

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  1 +
 drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 00ac65a14029..7f5c42a14196 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3840,6 +3840,23 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state)
 	return true;
 }
 
+void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state)
+{
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	/*
+	 * wa_14015401596 for display versions >= 13.
+	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
+	 * to at least a value of 1 when Panel Replay is enabled with VRR.
+	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by substracting
+	 * crtc_vdisplay from crtc_vblank_start, so incrementing crtc_vblank_start
+	 * by 1 if both are equal.
+	 */
+	if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
+	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay)
+		adjusted_mode->crtc_vblank_start += 1;
+}
+
 int intel_dotclock_calculate(int link_freq,
 			     const struct intel_link_m_n *m_n)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index f4a0773f0fca..23315eced41e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -413,6 +413,7 @@ bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state);
 u8 intel_crtc_bigjoiner_slave_pipes(const struct intel_crtc_state *crtc_state);
 struct intel_crtc *intel_master_crtc(const struct intel_crtc_state *crtc_state);
 bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
+void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state);
 bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 			       const struct intel_crtc_state *pipe_config,
 			       bool fastset);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 72cadad09db5..fccef5434e9c 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1430,6 +1430,10 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 	if (!(crtc_state->has_panel_replay || crtc_state->has_psr))
 		return;
 
+	/* wa_14015401596: display versions 13, 14 */
+	if (DISPLAY_VER(dev_priv) >= 13)
+		intel_crtc_vblank_delay(crtc_state);
+
 	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
 }
 
-- 
2.29.0

✗ Fi.CI.BAT: failure for drm/i915/panelreplay: Panel replay workaround with VRR (rev4)

[Patchwork]

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

== Series Details ==

Series: drm/i915/panelreplay: Panel replay workaround with VRR (rev4)
URL   : https://patchwork.freedesktop.org/series/129632/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14495 -> Patchwork_129632v4
====================================================

Summary
-------

  **FAILURE**

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

Participating hosts (38 -> 39)
------------------------------

  Additional (2): bat-atsm-1 fi-kbl-8809g 
  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@module-reload:
    - bat-adlp-6:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-adlp-6/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-adlp-6/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_timelines:
    - bat-arls-1:         [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-arls-1/igt@i915_selftest@live@gt_timelines.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-arls-1/igt@i915_selftest@live@gt_timelines.html

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

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

### CI changes ###

#### Issues hit ####

  * boot:
    - bat-dg2-11:         [PASS][5] -> [FAIL][6] ([i915#10491])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-dg2-11/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-dg2-11/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - bat-atsm-1:         NOTRUN -> [FAIL][7] ([i915#10563])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@gem_huc_copy@huc-copy.html

  * igt@gem_mmap@basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][8] ([i915#4083])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@gem_mmap@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][9] ([i915#4079]) +1 other test skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-atsm-1:         NOTRUN -> [SKIP][10] ([i915#6621])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gem:
    - bat-atsm-1:         NOTRUN -> [ABORT][11] ([i915#10564])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@i915_selftest@live@gem.html

  * igt@kms_addfb_basic@size-max:
    - bat-atsm-1:         NOTRUN -> [SKIP][12] ([i915#6077]) +37 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_addfb_basic@size-max.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - bat-atsm-1:         NOTRUN -> [SKIP][13] ([i915#6078]) +22 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-atsm-1:         NOTRUN -> [SKIP][14] ([i915#6093]) +4 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24:
    - bat-atsm-1:         NOTRUN -> [SKIP][15] ([i915#1836]) +6 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24.html

  * igt@kms_prop_blob@basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][16] ([i915#7357])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_prop_blob@basic.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-atsm-1:         NOTRUN -> [SKIP][17] ([i915#6094])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-atsm-1:         NOTRUN -> [SKIP][18] ([i915#4077]) +4 other tests skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-atsm-1:         NOTRUN -> [SKIP][19] +2 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-atsm-1/igt@prime_vgem@basic-write.html

  * igt@runner@aborted:
    - fi-kbl-8809g:       NOTRUN -> [FAIL][20] ([i915#4991])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/fi-kbl-8809g/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@dmabuf@all-tests@dma_fence:
    - bat-arls-3:         [DMESG-FAIL][21] ([i915#10602]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-arls-3/igt@dmabuf@all-tests@dma_fence.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-arls-3/igt@dmabuf@all-tests@dma_fence.html

  * igt@dmabuf@all-tests@sanitycheck:
    - bat-arls-3:         [ABORT][23] ([i915#10601]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-arls-3/igt@dmabuf@all-tests@sanitycheck.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-arls-3/igt@dmabuf@all-tests@sanitycheck.html

  * igt@gem_lmem_swapping@basic@lmem0:
    - bat-dg2-14:         [FAIL][25] ([i915#10378]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-dg2-14/igt@gem_lmem_swapping@basic@lmem0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-dg2-14/igt@gem_lmem_swapping@basic@lmem0.html

  * igt@i915_pm_rpm@module-reload:
    - {bat-mtlp-9}:       [WARN][27] ([i915#10436]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-mtlp-9/igt@i915_pm_rpm@module-reload.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-mtlp-9/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@coherency:
    - bat-dg2-9:          [ABORT][29] ([i915#10366]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-dg2-9/igt@i915_selftest@live@coherency.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-dg2-9/igt@i915_selftest@live@coherency.html

  * igt@i915_selftest@live@hangcheck:
    - bat-mtlp-8:         [DMESG-FAIL][31] ([i915#9840]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14495/bat-mtlp-8/igt@i915_selftest@live@hangcheck.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v4/bat-mtlp-8/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).

  [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366
  [i915#10378]: https://gitlab.freedesktop.org/drm/intel/issues/10378
  [i915#10436]: https://gitlab.freedesktop.org/drm/intel/issues/10436
  [i915#10491]: https://gitlab.freedesktop.org/drm/intel/issues/10491
  [i915#10563]: https://gitlab.freedesktop.org/drm/intel/issues/10563
  [i915#10564]: https://gitlab.freedesktop.org/drm/intel/issues/10564
  [i915#10601]: https://gitlab.freedesktop.org/drm/intel/issues/10601
  [i915#10602]: https://gitlab.freedesktop.org/drm/intel/issues/10602
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
  [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
  [i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
  [i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
  [i915#9840]: https://gitlab.freedesktop.org/drm/intel/issues/9840


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

  * Linux: CI_DRM_14495 -> Patchwork_129632v4

  CI-20190529: 20190529
  CI_DRM_14495: 07c774152cf8a034784b40978a77b5ee66e4779b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7786: 1e4a3cd0a4bb3419fb70ed3e01259485b056dcfd @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_129632v4: 07c774152cf8a034784b40978a77b5ee66e4779b @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

efa98f787e1a drm/i915/panelreplay: Panel replay workaround with VRR

== Logs ==

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

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

✓ Fi.CI.IGT: success for drm/i915/panelreplay: Panel replay workaround with VRR (rev4)

[Patchwork]

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

== Series Details ==

Series: drm/i915/panelreplay: Panel replay workaround with VRR (rev4)
URL   : https://patchwork.freedesktop.org/series/129632/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_14495_full -> Patchwork_129632v4_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (9 -> 7)
------------------------------

  Additional (1): shard-snb-0 
  Missing    (3): shard-snb shard-dg2 shard-glk 


Changes
-------

  No changes found


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_14495 -> Patchwork_129632v4
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_14495: 07c774152cf8a034784b40978a77b5ee66e4779b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7786: 1e4a3cd0a4bb3419fb70ed3e01259485b056dcfd @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_129632v4: 07c774152cf8a034784b40978a77b5ee66e4779b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

Re: [PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[Hogander, Jouni]

On Thu, 2024-03-28 at 10:13 +0530, Animesh Manna wrote:
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
> 
> HSD: 14015406119
> 
> v1: Initial version.
> v2: Update timings stored in adjusted_mode struct. [Ville]
> v3: Add WA in compute_config(). [Ville]
> v4:
> - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  1 +
>  drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 00ac65a14029..7f5c42a14196 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3840,6 +3840,23 @@ bool intel_crtc_get_pipe_config(struct
> intel_crtc_state *crtc_state)
>         return true;
>  }
>  
> +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state)
> +{
> +       struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> +
> +       /*
> +        * wa_14015401596 for display versions >= 13.
> +        * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> register
> +        * to at least a value of 1 when Panel Replay is enabled with
> VRR.
> +        * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> substracting
> +        * crtc_vdisplay from crtc_vblank_start, so incrementing
> crtc_vblank_start
> +        * by 1 if both are equal.
> +        */
> +       if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
> +           adjusted_mode->crtc_vblank_start == adjusted_mode-
> >crtc_vdisplay)
> +               adjusted_mode->crtc_vblank_start += 1;
> +}
> +

Do you have some specific reason to have this in intel_display.c? How
about move it to intel_psr.c? You could also use more descriptive name.
Current name somehow made me think it is some generic function to
calculate vblank delay. It is actually only for this workaround.

BR,

Jouni Högander
 

>  int intel_dotclock_calculate(int link_freq,
>                              const struct intel_link_m_n *m_n)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index f4a0773f0fca..23315eced41e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -413,6 +413,7 @@ bool intel_crtc_is_bigjoiner_master(const struct
> intel_crtc_state *crtc_state);
>  u8 intel_crtc_bigjoiner_slave_pipes(const struct intel_crtc_state
> *crtc_state);
>  struct intel_crtc *intel_master_crtc(const struct intel_crtc_state
> *crtc_state);
>  bool intel_crtc_get_pipe_config(struct intel_crtc_state
> *crtc_state);
> +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state);
>  bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
>                                const struct intel_crtc_state
> *pipe_config,
>                                bool fastset);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 72cadad09db5..fccef5434e9c 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1430,6 +1430,10 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>         if (!(crtc_state->has_panel_replay || crtc_state->has_psr))
>                 return;
>  
> +       /* wa_14015401596: display versions 13, 14 */
> +       if (DISPLAY_VER(dev_priv) >= 13)
> +               intel_crtc_vblank_delay(crtc_state);
> +
>         crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> crtc_state);
>  }
>  

RE: [PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[Manna, Animesh]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Wednesday, April 10, 2024 12:54 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: Re: [PATCH v4] drm/i915/panelreplay: Panel replay workaround with
> VRR
> 
> On Thu, 2024-03-28 at 10:13 +0530, Animesh Manna wrote:
> > Panel Replay VSC SDP not getting sent when VRR is enabled and W1 and
> > W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY
> > register to at least a value of 1.
> >
> > HSD: 14015406119
> >
> > v1: Initial version.
> > v2: Update timings stored in adjusted_mode struct. [Ville]
> > v3: Add WA in compute_config(). [Ville]
> > v4:
> > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 00ac65a14029..7f5c42a14196 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -3840,6 +3840,23 @@ bool intel_crtc_get_pipe_config(struct
> > intel_crtc_state *crtc_state)
> >         return true;
> >  }
> >
> > +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state) {
> > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > >hw.adjusted_mode;
> > +
> > +       /*
> > +        * wa_14015401596 for display versions >= 13.
> > +        * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> > register
> > +        * to at least a value of 1 when Panel Replay is enabled with
> > VRR.
> > +        * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> > substracting
> > +        * crtc_vdisplay from crtc_vblank_start, so incrementing
> > crtc_vblank_start
> > +        * by 1 if both are equal.
> > +        */
> > +       if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
> > +           adjusted_mode->crtc_vblank_start == adjusted_mode-
> > >crtc_vdisplay)
> > +               adjusted_mode->crtc_vblank_start += 1; }
> > +
> 
> Do you have some specific reason to have this in intel_display.c? How about
> move it to intel_psr.c? You could also use more descriptive name.
> Current name somehow made me think it is some generic function to
> calculate vblank delay. It is actually only for this workaround.

Thanks for review.
As per feedback from rev3 I have added in intel_display.c. Going forward all vbalnk related adjustment will be added into this function.
https://patchwork.freedesktop.org/series/129632/#rev3
I can move to intel_psr.c if the current version is not acceptable.

Regards,
Animesh

> 
> BR,
> 
> Jouni Högander
> 
> 
> >  int intel_dotclock_calculate(int link_freq,
> >                              const struct intel_link_m_n *m_n)
> >  {
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index f4a0773f0fca..23315eced41e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -413,6 +413,7 @@ bool intel_crtc_is_bigjoiner_master(const struct
> > intel_crtc_state *crtc_state);
> >  u8 intel_crtc_bigjoiner_slave_pipes(const struct intel_crtc_state
> > *crtc_state);
> >  struct intel_crtc *intel_master_crtc(const struct intel_crtc_state
> > *crtc_state);
> >  bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> > +void intel_crtc_vblank_delay(struct intel_crtc_state *crtc_state);
> >  bool intel_pipe_config_compare(const struct intel_crtc_state
> > *current_config,
> >                                const struct intel_crtc_state
> > *pipe_config,
> >                                bool fastset); diff --git
> > a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 72cadad09db5..fccef5434e9c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1430,6 +1430,10 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >         if (!(crtc_state->has_panel_replay || crtc_state->has_psr))
> >                 return;
> >
> > +       /* wa_14015401596: display versions 13, 14 */
> > +       if (DISPLAY_VER(dev_priv) >= 13)
> > +               intel_crtc_vblank_delay(crtc_state);
> > +
> >         crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > crtc_state);
> >  }
> >

Re: [PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[Hogander, Jouni]

On Wed, 2024-04-10 at 07:35 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Wednesday, April 10, 2024 12:54 PM
> > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: ville.syrjala@linux.intel.com; Murthy, Arun R
> > <arun.r.murthy@intel.com>
> > Subject: Re: [PATCH v4] drm/i915/panelreplay: Panel replay
> > workaround with
> > VRR
> > 
> > On Thu, 2024-03-28 at 10:13 +0530, Animesh Manna wrote:
> > > Panel Replay VSC SDP not getting sent when VRR is enabled and W1
> > > and
> > > W2 are 0. So Program Set Context Latency in
> > TRANS_SET_CONTEXT_LATENCY
> > > register to at least a value of 1.
> > > 
> > > HSD: 14015406119
> > > 
> > > v1: Initial version.
> > > v2: Update timings stored in adjusted_mode struct. [Ville]
> > > v3: Add WA in compute_config(). [Ville]
> > > v4:
> > > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> > > 
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 17
> > > +++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> > >  drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
> > >  3 files changed, 22 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 00ac65a14029..7f5c42a14196 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -3840,6 +3840,23 @@ bool intel_crtc_get_pipe_config(struct
> > > intel_crtc_state *crtc_state)
> > >         return true;
> > >  }
> > > 
> > > +void intel_crtc_vblank_delay(struct intel_crtc_state
> > > *crtc_state) {
> > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > hw.adjusted_mode;
> > > +
> > > +       /*
> > > +        * wa_14015401596 for display versions >= 13.
> > > +        * Program Set Context Latency in
> > > TRANS_SET_CONTEXT_LATENCY
> > > register
> > > +        * to at least a value of 1 when Panel Replay is enabled
> > > with
> > > VRR.
> > > +        * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> > > substracting
> > > +        * crtc_vdisplay from crtc_vblank_start, so incrementing
> > > crtc_vblank_start
> > > +        * by 1 if both are equal.
> > > +        */
> > > +       if (crtc_state->vrr.enable && crtc_state-
> > > >has_panel_replay &&
> > > +           adjusted_mode->crtc_vblank_start == adjusted_mode-
> > > > crtc_vdisplay)
> > > +               adjusted_mode->crtc_vblank_start += 1; }
> > > +
> > 
> > Do you have some specific reason to have this in intel_display.c?
> > How about
> > move it to intel_psr.c? You could also use more descriptive name.
> > Current name somehow made me think it is some generic function to
> > calculate vblank delay. It is actually only for this workaround.
> 
> Thanks for review.
> As per feedback from rev3 I have added in intel_display.c. Going
> forward all vbalnk related adjustment will be added into this
> function.
> https://patchwork.freedesktop.org/series/129632/#rev3
> I can move to intel_psr.c if the current version is not acceptable.

Ok, thank you for providing the context. If it's intended use is
generic then I think calling it shouldn't happen from
intel_psr_compute_config. Maybe Ville can comment where it should be
called from.

BR,

Jouni Högander

> 
> Regards,
> Animesh
> 
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > 
> > >  int intel_dotclock_calculate(int link_freq,
> > >                              const struct intel_link_m_n *m_n)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > index f4a0773f0fca..23315eced41e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -413,6 +413,7 @@ bool intel_crtc_is_bigjoiner_master(const
> > > struct
> > > intel_crtc_state *crtc_state);
> > >  u8 intel_crtc_bigjoiner_slave_pipes(const struct
> > > intel_crtc_state
> > > *crtc_state);
> > >  struct intel_crtc *intel_master_crtc(const struct
> > > intel_crtc_state
> > > *crtc_state);
> > >  bool intel_crtc_get_pipe_config(struct intel_crtc_state
> > > *crtc_state);
> > > +void intel_crtc_vblank_delay(struct intel_crtc_state
> > > *crtc_state);
> > >  bool intel_pipe_config_compare(const struct intel_crtc_state
> > > *current_config,
> > >                                const struct intel_crtc_state
> > > *pipe_config,
> > >                                bool fastset); diff --git
> > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 72cadad09db5..fccef5434e9c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1430,6 +1430,10 @@ void intel_psr_compute_config(struct
> > > intel_dp
> > > *intel_dp,
> > >         if (!(crtc_state->has_panel_replay || crtc_state-
> > > >has_psr))
> > >                 return;
> > > 
> > > +       /* wa_14015401596: display versions 13, 14 */
> > > +       if (DISPLAY_VER(dev_priv) >= 13)
> > > +               intel_crtc_vblank_delay(crtc_state);
> > > +
> > >         crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > > crtc_state);
> > >  }
> > > 
> 

Re: [PATCH v4] drm/i915/panelreplay: Panel replay workaround with VRR

[Ville Syrjälä]

On Wed, Apr 10, 2024 at 07:41:42AM +0000, Hogander, Jouni wrote:
> On Wed, 2024-04-10 at 07:35 +0000, Manna, Animesh wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Hogander, Jouni <jouni.hogander@intel.com>
> > > Sent: Wednesday, April 10, 2024 12:54 PM
> > > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: ville.syrjala@linux.intel.com; Murthy, Arun R
> > > <arun.r.murthy@intel.com>
> > > Subject: Re: [PATCH v4] drm/i915/panelreplay: Panel replay
> > > workaround with
> > > VRR
> > > 
> > > On Thu, 2024-03-28 at 10:13 +0530, Animesh Manna wrote:
> > > > Panel Replay VSC SDP not getting sent when VRR is enabled and W1
> > > > and
> > > > W2 are 0. So Program Set Context Latency in
> > > TRANS_SET_CONTEXT_LATENCY
> > > > register to at least a value of 1.
> > > > 
> > > > HSD: 14015406119
> > > > 
> > > > v1: Initial version.
> > > > v2: Update timings stored in adjusted_mode struct. [Ville]
> > > > v3: Add WA in compute_config(). [Ville]
> > > > v4:
> > > > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > > > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> > > > 
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 17
> > > > +++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_display.h |  1 +
> > > >  drivers/gpu/drm/i915/display/intel_psr.c     |  4 ++++
> > > >  3 files changed, 22 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 00ac65a14029..7f5c42a14196 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -3840,6 +3840,23 @@ bool intel_crtc_get_pipe_config(struct
> > > > intel_crtc_state *crtc_state)
> > > >         return true;
> > > >  }
> > > > 
> > > > +void intel_crtc_vblank_delay(struct intel_crtc_state
> > > > *crtc_state) {
> > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > > hw.adjusted_mode;
> > > > +
> > > > +       /*
> > > > +        * wa_14015401596 for display versions >= 13.
> > > > +        * Program Set Context Latency in
> > > > TRANS_SET_CONTEXT_LATENCY
> > > > register
> > > > +        * to at least a value of 1 when Panel Replay is enabled
> > > > with
> > > > VRR.
> > > > +        * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> > > > substracting
> > > > +        * crtc_vdisplay from crtc_vblank_start, so incrementing
> > > > crtc_vblank_start
> > > > +        * by 1 if both are equal.
> > > > +        */
> > > > +       if (crtc_state->vrr.enable && crtc_state-
> > > > >has_panel_replay &&
> > > > +           adjusted_mode->crtc_vblank_start == adjusted_mode-
> > > > > crtc_vdisplay)
> > > > +               adjusted_mode->crtc_vblank_start += 1; }
> > > > +
> > > 
> > > Do you have some specific reason to have this in intel_display.c?
> > > How about
> > > move it to intel_psr.c? You could also use more descriptive name.
> > > Current name somehow made me think it is some generic function to
> > > calculate vblank delay. It is actually only for this workaround.
> > 
> > Thanks for review.
> > As per feedback from rev3 I have added in intel_display.c. Going
> > forward all vbalnk related adjustment will be added into this
> > function.
> > https://patchwork.freedesktop.org/series/129632/#rev3
> > I can move to intel_psr.c if the current version is not acceptable.
> 
> Ok, thank you for providing the context. If it's intended use is
> generic then I think calling it shouldn't happen from
> intel_psr_compute_config. Maybe Ville can comment where it should be
> called from.

It's a potential chicken vs. egg situation.

The best place would be somewhere after .compute_config() since
we need this this for DSB in general. I think I have it in
intel_crtc_compute_config() in my current WIP DSB plane update
branch.

The problem with PSR is that it seems to want to use it
before that. The question no one has yet answered is
whether PSR actually wants to consult the undelayed
transcoder vblank or the delayed vblank. I'm think the
former is more likely (with PSR being a transcoder level
feature), in which case the chicken vs. egg sitation
will simply disappear. But someone will need to confirm
that.

The other potential chicken vs. egg looks to be VRR on
setups where TRANS_SET_CONTEXT_LATENCY isn't a thing.
The question there is how the vblank delay is configured
when using VRR. I'm pretty sure I did figure this out
already, but can't remember right now what the conclusion
was. So that needs to be double checked as well.

I was also pondering whether we should try to leave
adjusted_mode.crtc_vblank_start alone and instead just
reflect the delayed vblank in pipe_mode.crtc_vblank_start?
That might be useful if there are other places later on
in the atomic_check/etc. that needs to know the position
of the undelayed vblank. But that would have big implications
to eg. the vblank timestamping code, so probably not entirely
feasible.

-- 
Ville Syrjälä
Intel