[PATCH v3 0/4] drm/i915/xe3: FBC Dirty rect feature support

[PATCH v3 0/4] drm/i915/xe3: FBC Dirty rect feature support

[Vinod Govindapillai]

Dirty rect support for FBC in xe3 onwards based on the comments after the
initial RFC series.

v2: Dirty rect related compute and storage moved to fbc state (Ville)

V3: Dont call fbc activate if FBC is already active

Vinod Govindapillai (4):
  drm/i915/display: avoid calling fbc activate if fbc is active
  drm/i915/xe: add register definitions for fbc dirty rect support
  drm/i915/xe3: add dirty rect support for FBC
  drm/i915/xe3: disable FBC if PSR2 selective fetch is enabled

 drivers/gpu/drm/i915/display/intel_display.c  |   4 +
 drivers/gpu/drm/i915/display/intel_fbc.c      | 108 +++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fbc.h      |   4 +
 drivers/gpu/drm/i915/display/intel_fbc_regs.h |   9 ++
 4 files changed, 121 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] drm/i915/display: avoid calling fbc activate if fbc is active

[Vinod Govindapillai]

If FBC is already active, we don't need to call FBC activate
routine again during the post plane update. As this will
explicitly call the nuke and also rewrite the FBC ctl registers.
"intel_atomic_commit_tail-> intel_post_plane_update->
intel_fbc_post_update-> _intel_fbc_post_update" path will be
executed during the normal flip cases. FBC HW will nuke on sync
flip event and driver do not need to call the nuke explicitly.
This is much more relevant in case of dirty rectangle support
in FBC with the followup patches. Nuke on flip in that case will
remove all the benefits of fetching only the modified region.

The front buffer rendering sequence will call intel_fbc_flush()
and which will call intel_fbc_nuke() or intel_fbc_activate()
based on FBC status explicitly and won't get impacted by this
change.

Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index df05904bac8a..fd540ff5e57e 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1561,7 +1561,8 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc)
 	fbc->flip_pending = false;
 	fbc->busy_bits = 0;
 
-	intel_fbc_activate(fbc);
+	if (!fbc->active)
+		intel_fbc_activate(fbc);
 }
 
 void intel_fbc_post_update(struct intel_atomic_state *state,
-- 
2.43.0

[PATCH v3 2/4] drm/i915/xe: add register definitions for fbc dirty rect support

[Vinod Govindapillai]

Register definitions for FBC dirty rect support

Bspec: 71675, 73424
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc_regs.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
index ae0699c3c2fe..b1d0161a3196 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
@@ -100,6 +100,15 @@
 #define   FBC_STRIDE_MASK	REG_GENMASK(14, 0)
 #define   FBC_STRIDE(x)		REG_FIELD_PREP(FBC_STRIDE_MASK, (x))
 
+#define XE3_FBC_DIRTY_RECT(fbc_id)	_MMIO_PIPE((fbc_id), 0x43230, 0x43270)
+#define   FBC_DIRTY_RECT_END_LINE_MASK		REG_GENMASK(31, 16)
+#define   FBC_DIRTY_RECT_END_LINE(val)		REG_FIELD_PREP(FBC_DIRTY_RECT_END_LINE_MASK, (val))
+#define   FBC_DIRTY_RECT_START_LINE_MASK	REG_GENMASK(15, 0)
+#define   FBC_DIRTY_RECT_START_LINE(val)	REG_FIELD_PREP(FBC_DIRTY_RECT_START_LINE_MASK, (val))
+
+#define XE3_FBC_DIRTY_CTL(fbc_id)	_MMIO_PIPE((fbc_id), 0x43234, 0x43274)
+#define   FBC_DIRTY_RECT_EN		REG_BIT(31)
+
 #define ILK_FBC_RT_BASE		_MMIO(0x2128)
 #define   ILK_FBC_RT_VALID	REG_BIT(0)
 #define   SNB_FBC_FRONT_BUFFER	REG_BIT(1)
-- 
2.43.0

[PATCH v3 3/4] drm/i915/xe3: add dirty rect support for FBC

[Vinod Govindapillai]

Dirty rectangle feature allows FBC to recompress a subsection
of a frame. When this feature is enabled, display will read
the scan lines between dirty rectangle start line and dirty
rectangle end line in subsequent frames.

v2: Move dirty rect handling to fbc state (Ville)

Bspec: 71675, 73424
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  4 +
 drivers/gpu/drm/i915/display/intel_fbc.c     | 96 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fbc.h     |  4 +
 3 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4271da219b41..d381dce04755 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7209,6 +7209,8 @@ static void intel_update_crtc(struct intel_atomic_state *state,
 
 		commit_pipe_pre_planes(state, crtc);
 
+		intel_fbc_program_dirty_rect(NULL, state, crtc);
+
 		intel_crtc_planes_update_arm(NULL, state, crtc);
 
 		commit_pipe_post_planes(state, crtc);
@@ -7678,6 +7680,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 					       new_crtc_state);
 		bdw_set_pipe_misc(new_crtc_state->dsb_commit,
 				  new_crtc_state);
+		intel_fbc_program_dirty_rect(new_crtc_state->dsb_commit,
+					     state, crtc);
 		intel_crtc_planes_update_arm(new_crtc_state->dsb_commit,
 					     state, crtc);
 
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index fd540ff5e57e..f15cddba9bb0 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -42,6 +42,7 @@
 #include <linux/string_helpers.h>
 
 #include <drm/drm_blend.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
 
 #include "gem/i915_gem_stolen.h"
@@ -58,6 +59,7 @@
 #include "intel_display_trace.h"
 #include "intel_display_types.h"
 #include "intel_display_wa.h"
+#include "intel_dsb.h"
 #include "intel_fbc.h"
 #include "intel_fbc_regs.h"
 #include "intel_frontbuffer.h"
@@ -88,6 +90,7 @@ struct intel_fbc_state {
 	u16 override_cfb_stride;
 	u16 interval;
 	s8 fence_id;
+	struct drm_rect dirty_rect;
 };
 
 struct intel_fbc {
@@ -527,6 +530,9 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
 	struct intel_display *display = fbc->display;
 	u32 dpfc_ctl;
 
+	if (DISPLAY_VER(display) >= 30)
+		intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id), 0);
+
 	/* Disable compression */
 	dpfc_ctl = intel_de_read(display, ILK_DPFC_CONTROL(fbc->id));
 	if (dpfc_ctl & DPFC_CTL_EN) {
@@ -670,6 +676,10 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
 	if (DISPLAY_VER(display) >= 20)
 		intel_de_write(display, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
 
+	if (DISPLAY_VER(display) >= 30)
+		intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id),
+			       FBC_DIRTY_RECT_EN);
+
 	intel_de_write(display, ILK_DPFC_CONTROL(fbc->id),
 		       DPFC_CTL_EN | dpfc_ctl);
 }
@@ -1203,6 +1213,85 @@ static bool tiling_is_valid(const struct intel_plane_state *plane_state)
 		return i8xx_fbc_tiling_valid(plane_state);
 }
 
+static void
+__intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
+{
+	struct intel_display *display = to_intel_display(plane);
+	struct intel_fbc *fbc = plane->fbc;
+	struct intel_fbc_state *fbc_state = &fbc->state;
+
+	if (fbc_state->plane != plane)
+		return;
+
+	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
+			   FBC_DIRTY_RECT_START_LINE(fbc_state->dirty_rect.y1) |
+			   FBC_DIRTY_RECT_END_LINE(fbc_state->dirty_rect.y2));
+}
+
+void
+intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
+			     struct intel_atomic_state *state,
+			     struct intel_crtc *crtc)
+{
+	struct intel_display *display = to_intel_display(state);
+	struct intel_plane_state __maybe_unused *plane_state;
+	struct intel_plane *plane;
+	int i;
+
+	if (DISPLAY_VER(display) < 30)
+		return;
+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		struct intel_fbc *fbc = plane->fbc;
+
+		if (!fbc || plane->pipe != crtc->pipe)
+			continue;
+
+		__intel_fbc_program_dirty_rect(dsb, plane);
+	}
+}
+
+
+static void
+update_dirty_rect_to_full_region(struct intel_plane_state *plane_state,
+				 struct drm_rect *dirty_rect)
+{
+	int y_offset = plane_state->view.color_plane[0].y;
+	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
+
+	dirty_rect->y1 = y_offset;
+	dirty_rect->y2 = y_offset + plane_height - 1;
+}
+
+static void
+validate_and_clip_dirty_rect(struct intel_plane_state *plane_state,
+			     struct drm_rect *dirty_rect)
+{
+	int y_offset = plane_state->view.color_plane[0].y;
+	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
+	int max_endline = y_offset + plane_height;
+
+	dirty_rect->y1 = clamp(dirty_rect->y1, y_offset, max_endline);
+	dirty_rect->y2 = clamp(dirty_rect->y2, dirty_rect->y1, max_endline);
+}
+
+static void
+intel_fbc_compute_dirty_rect(struct intel_plane *plane,
+			     struct intel_plane_state *old_plane_state,
+			     struct intel_plane_state *new_plane_state)
+{
+	struct intel_fbc *fbc = plane->fbc;
+	struct intel_fbc_state *fbc_state = &fbc->state;
+	struct drm_rect *fbc_dirty_rect = &fbc_state->dirty_rect;
+
+	if (drm_atomic_helper_damage_merged(&old_plane_state->uapi,
+					    &new_plane_state->uapi,
+					    fbc_dirty_rect))
+		validate_and_clip_dirty_rect(new_plane_state, fbc_dirty_rect);
+	else
+		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
+}
+
 static void intel_fbc_update_state(struct intel_atomic_state *state,
 				   struct intel_crtc *crtc,
 				   struct intel_plane *plane)
@@ -1210,8 +1299,10 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
 	struct intel_display *display = to_intel_display(state->base.dev);
 	const struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
-	const struct intel_plane_state *plane_state =
+	struct intel_plane_state *plane_state =
 		intel_atomic_get_new_plane_state(state, plane);
+	struct intel_plane_state *old_plane_state =
+		intel_atomic_get_old_plane_state(state, plane);
 	struct intel_fbc *fbc = plane->fbc;
 	struct intel_fbc_state *fbc_state = &fbc->state;
 
@@ -1236,6 +1327,9 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
 	fbc_state->cfb_stride = intel_fbc_cfb_stride(plane_state);
 	fbc_state->cfb_size = intel_fbc_cfb_size(plane_state);
 	fbc_state->override_cfb_stride = intel_fbc_override_cfb_stride(plane_state);
+
+	if (DISPLAY_VER(display) >= 30)
+		intel_fbc_compute_dirty_rect(plane, old_plane_state, plane_state);
 }
 
 static bool intel_fbc_is_fence_ok(const struct intel_plane_state *plane_state)
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
index ceae55458e14..acaebe15f312 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.h
+++ b/drivers/gpu/drm/i915/display/intel_fbc.h
@@ -14,6 +14,7 @@ struct intel_atomic_state;
 struct intel_crtc;
 struct intel_crtc_state;
 struct intel_display;
+struct intel_dsb;
 struct intel_fbc;
 struct intel_plane;
 struct intel_plane_state;
@@ -48,5 +49,8 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
 void intel_fbc_reset_underrun(struct intel_display *display);
 void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
 void intel_fbc_debugfs_register(struct intel_display *display);
+void intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
+				  struct intel_atomic_state *state,
+				  struct intel_crtc *crtc);
 
 #endif /* __INTEL_FBC_H__ */
-- 
2.43.0

[PATCH v3 4/4] drm/i915/xe3: disable FBC if PSR2 selective fetch is enabled

[Vinod Govindapillai]

It is not recommended to have both FBC and PSR2 selective fetch
be enabled at the same time in a plane. If PSR2 selective fetch
or panel replay is on, mark FBC as not possible in that plane.

v2: fix the condition to disable FBC if PSR2 enabled (Jani)

Bspec: 68881
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index f15cddba9bb0..5f4809f1359d 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1432,9 +1432,14 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
 	 * Display 12+ is not supporting FBC with PSR2.
 	 * Recommendation is to keep this combination disabled
 	 * Bspec: 50422 HSD: 14010260002
+	 *
+	 * In Xe3, PSR2 selective fetch and FBC dirty rect feature cannot
+	 * coexist. So if PSR2 selective fetch is supported then mark that
+	 * FBC is not supported.
+	 * TODO: Need a logic to decide between PSR2 and FBC Dirty rect
 	 */
-	if (IS_DISPLAY_VER(display, 12, 14) && crtc_state->has_sel_update &&
-	    !crtc_state->has_panel_replay) {
+	if ((IS_DISPLAY_VER(display, 12, 14) || DISPLAY_VER(display) >= 30) &&
+	    crtc_state->has_sel_update && !crtc_state->has_panel_replay) {
 		plane_state->no_fbc_reason = "PSR2 enabled";
 		return 0;
 	}
-- 
2.43.0

✗ Fi.CI.CHECKPATCH: warning for drm/i915/xe3: FBC Dirty rect feature support (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/xe3: FBC Dirty rect feature support (rev3)
URL   : https://patchwork.freedesktop.org/series/141527/
State : warning

== Summary ==

Error: dim checkpatch failed
8e21777a49d3 drm/i915/display: avoid calling fbc activate if fbc is active
78cc53ab5ece drm/i915/xe: add register definitions for fbc dirty rect support
-:24: WARNING:LONG_LINE: line length of 101 exceeds 100 columns
#24: FILE: drivers/gpu/drm/i915/display/intel_fbc_regs.h:107:
+#define   FBC_DIRTY_RECT_START_LINE(val)	REG_FIELD_PREP(FBC_DIRTY_RECT_START_LINE_MASK, (val))

total: 0 errors, 1 warnings, 0 checks, 15 lines checked
4bb7b9f31357 drm/i915/xe3: add dirty rect support for FBC
-:129: CHECK:LINE_SPACING: Please don't use multiple blank lines
#129: FILE: drivers/gpu/drm/i915/display/intel_fbc.c:1254:
+
+

total: 0 errors, 0 warnings, 1 checks, 176 lines checked
c2ff3abafcf0 drm/i915/xe3: disable FBC if PSR2 selective fetch is enabled

✗ Fi.CI.SPARSE: warning for drm/i915/xe3: FBC Dirty rect feature support (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/xe3: FBC Dirty rect feature support (rev3)
URL   : https://patchwork.freedesktop.org/series/141527/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ i915.CI.BAT: failure for drm/i915/xe3: FBC Dirty rect feature support (rev3)

[Patchwork]

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

== Series Details ==

Series: drm/i915/xe3: FBC Dirty rect feature support (rev3)
URL   : https://patchwork.freedesktop.org/series/141527/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15950 -> Patchwork_141527v3
====================================================

Summary
-------

  **FAILURE**

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

Participating hosts (44 -> 41)
------------------------------

  Missing    (3): fi-skl-6600u fi-snb-2520m fi-pnv-d510 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip@basic-flip-vs-dpms@a-dp1:
    - bat-apl-1:          [PASS][1] -> [DMESG-WARN][2] +1 other test dmesg-warn
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-apl-1/igt@kms_flip@basic-flip-vs-dpms@a-dp1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-apl-1/igt@kms_flip@basic-flip-vs-dpms@a-dp1.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-mtlp-9}:       [DMESG-WARN][3] ([i915#13494]) -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-mtlp-9/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-mtlp-9/igt@i915_module_load@load.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         [PASS][5] -> [SKIP][6] ([i915#9197]) +3 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  
#### Possible fixes ####

  * igt@core_hotunplug@unbind-rebind:
    - bat-rpls-4:         [DMESG-WARN][7] ([i915#13400]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-rpls-4/igt@core_hotunplug@unbind-rebind.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-rpls-4/igt@core_hotunplug@unbind-rebind.html

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [DMESG-FAIL][9] ([i915#13393]) -> [PASS][10] +1 other test pass
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-mtlp-8/igt@i915_selftest@live.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-mtlp-8/igt@i915_selftest@live.html
    - bat-adlp-6:         [ABORT][11] ([i915#13399]) -> [PASS][12] +1 other test pass
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-adlp-6/igt@i915_selftest@live.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-adlp-6/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [DMESG-FAIL][13] ([i915#13393]) -> [PASS][14] +1 other test pass
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-arlh-3/igt@i915_selftest@live@workarounds.html
    - bat-arls-5:         [DMESG-FAIL][15] ([i915#13393]) -> [PASS][16] +1 other test pass
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15950/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v3/bat-arls-5/igt@i915_selftest@live@workarounds.html

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

  [i915#13393]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13393
  [i915#13399]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13399
  [i915#13400]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13400
  [i915#13494]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13494
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197


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

  * Linux: CI_DRM_15950 -> Patchwork_141527v3

  CI-20190529: 20190529
  CI_DRM_15950: 3deaca65541212fd09c8832090815813e6128caa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8189: e036190dc1730ecb94cb393f88378e734db4b1d6 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_141527v3: 3deaca65541212fd09c8832090815813e6128caa @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

Re: [PATCH v3 1/4] drm/i915/display: avoid calling fbc activate if fbc is active

[Ville Syrjälä]

On Tue, Jan 14, 2025 at 02:07:16PM +0200, Vinod Govindapillai wrote:
> If FBC is already active, we don't need to call FBC activate
> routine again during the post plane update. As this will
> explicitly call the nuke and also rewrite the FBC ctl registers.
> "intel_atomic_commit_tail-> intel_post_plane_update->
> intel_fbc_post_update-> _intel_fbc_post_update" path will be
> executed during the normal flip cases. FBC HW will nuke on sync
> flip event and driver do not need to call the nuke explicitly.
> This is much more relevant in case of dirty rectangle support
> in FBC with the followup patches. Nuke on flip in that case will
> remove all the benefits of fetching only the modified region.
> 
> The front buffer rendering sequence will call intel_fbc_flush()
> and which will call intel_fbc_nuke() or intel_fbc_activate()
> based on FBC status explicitly and won't get impacted by this
> change.
> 
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index df05904bac8a..fd540ff5e57e 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1561,7 +1561,8 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc)
>  	fbc->flip_pending = false;
>  	fbc->busy_bits = 0;
>  
> -	intel_fbc_activate(fbc);
> +	if (!fbc->active)
> +		intel_fbc_activate(fbc);

We'll need to keep the actual activate part (eg. to update the fence).
But we should be able to elide the explicit nuke if FBC was already
active (that implies a flip nuke has occurred anyway, vs. if FBC was
previously disabled then it might have been disabled by a frontbuffer
invalidate and if it hasn't been disabled for a full frame then the
hardware won't automagically cause a nuke when we reactivate it).

>  }
>  
>  void intel_fbc_post_update(struct intel_atomic_state *state,
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 3/4] drm/i915/xe3: add dirty rect support for FBC

[Ville Syrjälä]

On Tue, Jan 14, 2025 at 02:07:18PM +0200, Vinod Govindapillai wrote:
> Dirty rectangle feature allows FBC to recompress a subsection
> of a frame. When this feature is enabled, display will read
> the scan lines between dirty rectangle start line and dirty
> rectangle end line in subsequent frames.
> 
> v2: Move dirty rect handling to fbc state (Ville)
> 
> Bspec: 71675, 73424
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  4 +
>  drivers/gpu/drm/i915/display/intel_fbc.c     | 96 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_fbc.h     |  4 +
>  3 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4271da219b41..d381dce04755 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7209,6 +7209,8 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>  
>  		commit_pipe_pre_planes(state, crtc);
>  
> +		intel_fbc_program_dirty_rect(NULL, state, crtc);
> +

The problem for the DSB case is that we haven't done the
fbc_update() thing yet, so we don't actually know what the
FBC state should be here.

So we probably need to make some kind of early dirty rect only
state update before we build the DSB commands. I think it should
be sufficient to check intel_fbc_can_flip_nuke(), and if that
passes then we update the dirty rect ahead of time. We may
also want a flag to indicate whether the dirty rect is valid
or not so that we don't end up programming a bunch of garbage
into the hardware when we're not doing a flip nuke (dunno if
dirty rect == empty can be used instead for that?).

>  		intel_crtc_planes_update_arm(NULL, state, crtc);
>  
>  		commit_pipe_post_planes(state, crtc);
> @@ -7678,6 +7680,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>  					       new_crtc_state);
>  		bdw_set_pipe_misc(new_crtc_state->dsb_commit,
>  				  new_crtc_state);
> +		intel_fbc_program_dirty_rect(new_crtc_state->dsb_commit,
> +					     state, crtc);
>  		intel_crtc_planes_update_arm(new_crtc_state->dsb_commit,
>  					     state, crtc);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index fd540ff5e57e..f15cddba9bb0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -42,6 +42,7 @@
>  #include <linux/string_helpers.h>
>  
>  #include <drm/drm_blend.h>
> +#include <drm/drm_damage_helper.h>
>  #include <drm/drm_fourcc.h>
>  
>  #include "gem/i915_gem_stolen.h"
> @@ -58,6 +59,7 @@
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_display_wa.h"
> +#include "intel_dsb.h"
>  #include "intel_fbc.h"
>  #include "intel_fbc_regs.h"
>  #include "intel_frontbuffer.h"
> @@ -88,6 +90,7 @@ struct intel_fbc_state {
>  	u16 override_cfb_stride;
>  	u16 interval;
>  	s8 fence_id;
> +	struct drm_rect dirty_rect;
>  };
>  
>  struct intel_fbc {
> @@ -527,6 +530,9 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
>  	struct intel_display *display = fbc->display;
>  	u32 dpfc_ctl;
>  
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id), 0);
> +
>  	/* Disable compression */
>  	dpfc_ctl = intel_de_read(display, ILK_DPFC_CONTROL(fbc->id));
>  	if (dpfc_ctl & DPFC_CTL_EN) {
> @@ -670,6 +676,10 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
>  	if (DISPLAY_VER(display) >= 20)
>  		intel_de_write(display, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
>  
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id),
> +			       FBC_DIRTY_RECT_EN);
> +
>  	intel_de_write(display, ILK_DPFC_CONTROL(fbc->id),
>  		       DPFC_CTL_EN | dpfc_ctl);
>  }
> @@ -1203,6 +1213,85 @@ static bool tiling_is_valid(const struct intel_plane_state *plane_state)
>  		return i8xx_fbc_tiling_valid(plane_state);
>  }
>  
> +static void
> +__intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	struct intel_fbc *fbc = plane->fbc;
> +	struct intel_fbc_state *fbc_state = &fbc->state;
> +
> +	if (fbc_state->plane != plane)
> +		return;
> +
> +	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
> +			   FBC_DIRTY_RECT_START_LINE(fbc_state->dirty_rect.y1) |
> +			   FBC_DIRTY_RECT_END_LINE(fbc_state->dirty_rect.y2));
> +}
> +
> +void
> +intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
> +			     struct intel_atomic_state *state,
> +			     struct intel_crtc *crtc)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_plane_state __maybe_unused *plane_state;
> +	struct intel_plane *plane;
> +	int i;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> +		struct intel_fbc *fbc = plane->fbc;
> +
> +		if (!fbc || plane->pipe != crtc->pipe)
> +			continue;
> +
> +		__intel_fbc_program_dirty_rect(dsb, plane);
> +	}
> +}
> +
> +
> +static void
> +update_dirty_rect_to_full_region(struct intel_plane_state *plane_state,
> +				 struct drm_rect *dirty_rect)
> +{
> +	int y_offset = plane_state->view.color_plane[0].y;
> +	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
> +
> +	dirty_rect->y1 = y_offset;
> +	dirty_rect->y2 = y_offset + plane_height - 1;
> +}
> +
> +static void
> +validate_and_clip_dirty_rect(struct intel_plane_state *plane_state,
> +			     struct drm_rect *dirty_rect)
> +{
> +	int y_offset = plane_state->view.color_plane[0].y;
> +	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
> +	int max_endline = y_offset + plane_height;
> +
> +	dirty_rect->y1 = clamp(dirty_rect->y1, y_offset, max_endline);
> +	dirty_rect->y2 = clamp(dirty_rect->y2, dirty_rect->y1, max_endline);
> +}
> +
> +static void
> +intel_fbc_compute_dirty_rect(struct intel_plane *plane,
> +			     struct intel_plane_state *old_plane_state,
> +			     struct intel_plane_state *new_plane_state)
> +{
> +	struct intel_fbc *fbc = plane->fbc;
> +	struct intel_fbc_state *fbc_state = &fbc->state;
> +	struct drm_rect *fbc_dirty_rect = &fbc_state->dirty_rect;
> +
> +	if (drm_atomic_helper_damage_merged(&old_plane_state->uapi,
> +					    &new_plane_state->uapi,
> +					    fbc_dirty_rect))
> +		validate_and_clip_dirty_rect(new_plane_state, fbc_dirty_rect);
> +	else
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +}
> +
>  static void intel_fbc_update_state(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc,
>  				   struct intel_plane *plane)
> @@ -1210,8 +1299,10 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
>  	struct intel_display *display = to_intel_display(state->base.dev);
>  	const struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	const struct intel_plane_state *plane_state =
> +	struct intel_plane_state *plane_state =
>  		intel_atomic_get_new_plane_state(state, plane);
> +	struct intel_plane_state *old_plane_state =
> +		intel_atomic_get_old_plane_state(state, plane);
>  	struct intel_fbc *fbc = plane->fbc;
>  	struct intel_fbc_state *fbc_state = &fbc->state;
>  
> @@ -1236,6 +1327,9 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
>  	fbc_state->cfb_stride = intel_fbc_cfb_stride(plane_state);
>  	fbc_state->cfb_size = intel_fbc_cfb_size(plane_state);
>  	fbc_state->override_cfb_stride = intel_fbc_override_cfb_stride(plane_state);
> +
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_fbc_compute_dirty_rect(plane, old_plane_state, plane_state);
>  }
>  
>  static bool intel_fbc_is_fence_ok(const struct intel_plane_state *plane_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index ceae55458e14..acaebe15f312 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -14,6 +14,7 @@ struct intel_atomic_state;
>  struct intel_crtc;
>  struct intel_crtc_state;
>  struct intel_display;
> +struct intel_dsb;
>  struct intel_fbc;
>  struct intel_plane;
>  struct intel_plane_state;
> @@ -48,5 +49,8 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
>  void intel_fbc_reset_underrun(struct intel_display *display);
>  void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
>  void intel_fbc_debugfs_register(struct intel_display *display);
> +void intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
> +				  struct intel_atomic_state *state,
> +				  struct intel_crtc *crtc);
>  
>  #endif /* __INTEL_FBC_H__ */
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 1/4] drm/i915/display: avoid calling fbc activate if fbc is active

[Govindapillai, Vinod]

On Fri, 2025-01-17 at 14:49 +0200, Ville Syrjälä wrote:
> On Tue, Jan 14, 2025 at 02:07:16PM +0200, Vinod Govindapillai wrote:
> > If FBC is already active, we don't need to call FBC activate
> > routine again during the post plane update. As this will
> > explicitly call the nuke and also rewrite the FBC ctl registers.
> > "intel_atomic_commit_tail-> intel_post_plane_update->
> > intel_fbc_post_update-> _intel_fbc_post_update" path will be
> > executed during the normal flip cases. FBC HW will nuke on sync
> > flip event and driver do not need to call the nuke explicitly.
> > This is much more relevant in case of dirty rectangle support
> > in FBC with the followup patches. Nuke on flip in that case will
> > remove all the benefits of fetching only the modified region.
> > 
> > The front buffer rendering sequence will call intel_fbc_flush()
> > and which will call intel_fbc_nuke() or intel_fbc_activate()
> > based on FBC status explicitly and won't get impacted by this
> > change.
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index df05904bac8a..fd540ff5e57e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -1561,7 +1561,8 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc)
> >  	fbc->flip_pending = false;
> >  	fbc->busy_bits = 0;
> >  
> > -	intel_fbc_activate(fbc);
> > +	if (!fbc->active)
> > +		intel_fbc_activate(fbc);
> 
> We'll need to keep the actual activate part (eg. to update the fence).
> But we should be able to elide the explicit nuke if FBC was already
> active (that implies a flip nuke has occurred anyway, vs. if FBC was
> previously disabled then it might have been disabled by a frontbuffer
> invalidate and if it hasn't been disabled for a full frame then the
> hardware won't automagically cause a nuke when we reactivate it).

Thanks Ville!

Okay! I have something like this now! But facing some weird issues!

index df05904bac8a..f05c61040d19 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -739,10 +739,22 @@ static void intel_fbc_nuke(struct intel_fbc *fbc)
 
 static void intel_fbc_activate(struct intel_fbc *fbc)
 {
+       bool fbc_already_active;
+
        lockdep_assert_held(&fbc->lock);
 
+       fbc_already_active = fbc->active;
+
        intel_fbc_hw_activate(fbc);
-       intel_fbc_nuke(fbc);
+
+       /*
+        * If FBC is already active, don't nuke.
+        * In normal flips after FBC is enabled, FBC hw will nuke on flip
+        * In case of frontbuffer rendering cases, invalidate, flush sequence
+        * will handle the nuke
+        */
+       if (!fbc_already_active)
+               intel_fbc_nuke(fbc);

So the nuke won't be called from normal flips if fbc is already active. So the intel_fbc_hw_activate
will be called always - which programs override stride, no fences in case of xe, and reprograms
FBC_CTL and FBC_DIRTYRECT_CTL registers with the same values!

But the weird thing is, with this damaged area update don't have any effect. The whole region is
getting updated! But if I avoid calling the intel_fbc_hw_activate() completely, i can see only those
damaged rect area being updated!

Initially I thought as we rewrite FBC_DIRTYRECT_CTL enable again, that could cause the first frame
being taking the whole plane size as the update region. But after experimenting with  those,
narrowed it to glk_fbc_program_cfb_stride() call! Somehow programming glk_fbc_program_cfb_stride()
is causing entire region being updated!  Do you have any pointers on this?

Thanks
Vinod


> 
> >  }
> >  
> >  void intel_fbc_post_update(struct intel_atomic_state *state,
> > -- 
> > 2.43.0
> 

✗ Fi.CI.BUILD: failure for drm/i915/xe3: FBC Dirty rect feature support (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/xe3: FBC Dirty rect feature support (rev4)
URL   : https://patchwork.freedesktop.org/series/141527/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/141527/revisions/4/mbox/ not applied
Applying: drm/i915/display: avoid calling fbc activate if fbc is active
error: corrupt patch at line 25
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915/display: avoid calling fbc activate if fbc is active
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced