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

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

[Vinod Govindapillai]

Initial patchset to support dirty rect capability for FBC in Xe3.
This is the initial design suggestion for the feedback. This
is for non PSR cases. Seective fetch is not allowed with the
dirty rectangle feature.

More FC use case scenarios might need to be handled - such as
frontbuffer flush, invalidate and more cases we would need to
do a full region update etc.

Vinod Govindapillai (4):
  drm/i915/display: update intel_fbc_atomic_check for dirty_fbc support
  drm/i915/display: 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_fbc.c      | 145 +++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fbc.h      |   3 +
 drivers/gpu/drm/i915/display/intel_fbc_regs.h |   9 ++
 .../drm/i915/display/skl_universal_plane.c    |   2 +
 4 files changed, 155 insertions(+), 4 deletions(-)

-- 
2.34.1

[RFC PATCH 1/4] drm/i915/display: update intel_fbc_atomic_check for dirty_fbc support

[Vinod Govindapillai]

For the follw-up patches to enable the dirty rect support for
FBC, we need to have a provision to compare certain FBC specific
parameters between old plane state and new plane state. Update
intel_fbc_atomic_check() to get the old plane state as well.

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

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index df05904bac8a..1fdb1a3e3fbe 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1666,11 +1666,13 @@ void intel_fbc_flush(struct drm_i915_private *i915,
 
 int intel_fbc_atomic_check(struct intel_atomic_state *state)
 {
-	struct intel_plane_state __maybe_unused *plane_state;
+	struct intel_plane_state __maybe_unused *new_plane_state;
+	struct intel_plane_state __maybe_unused *old_plane_state;
 	struct intel_plane *plane;
 	int i;
 
-	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
+					     new_plane_state, i) {
 		int ret;
 
 		ret = intel_fbc_check_plane(state, plane);
-- 
2.34.1

[RFC PATCH 2/4] drm/i915/display: 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.34.1

[RFC PATCH 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.

Bspec: 71675, 73424
Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c      | 130 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_fbc.h      |   3 +
 .../drm/i915/display/skl_universal_plane.c    |   2 +
 3 files changed, 135 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 1fdb1a3e3fbe..01080171790f 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"
@@ -126,6 +128,8 @@ struct intel_fbc {
 	 */
 	struct intel_fbc_state state;
 	const char *no_fbc_reason;
+
+	struct drm_rect dirty_rect;
 };
 
 /* plane stride in pixels */
@@ -670,6 +674,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);
 }
@@ -1664,6 +1672,113 @@ void intel_fbc_flush(struct drm_i915_private *i915,
 		__intel_fbc_flush(fbc, frontbuffer_bits, origin);
 }
 
+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;
+
+	if (DISPLAY_VER(display) < 30)
+		return;
+
+	if (!fbc)
+		return;
+
+	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
+			   FBC_DIRTY_RECT_START_LINE(fbc->dirty_rect.y1) |
+			   FBC_DIRTY_RECT_END_LINE(fbc->dirty_rect.y2));
+}
+
+static bool
+intel_fbc_need_full_region_update(struct intel_plane_state *old_plane_state,
+				  struct intel_plane_state *new_plane_state)
+{
+	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
+	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
+
+	if (!old_fb || !new_fb)
+		return true;
+
+	if (old_fb->format->format != new_fb->format->format)
+		return true;
+
+	if (old_fb->modifier != new_fb->modifier)
+		return true;
+
+	if (intel_fbc_plane_stride(old_plane_state) !=
+	    intel_fbc_plane_stride(new_plane_state))
+		return true;
+
+	if (intel_fbc_cfb_stride(old_plane_state) !=
+	    intel_fbc_cfb_stride(new_plane_state))
+		return true;
+
+	if (intel_fbc_cfb_size(old_plane_state) !=
+	    intel_fbc_cfb_size(new_plane_state))
+		return true;
+
+	return false;
+}
+
+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;
+}
+
+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,
+					bool need_full_region_update)
+{
+	struct intel_display *display = to_intel_display(plane);
+	struct intel_fbc *fbc = plane->fbc;
+	struct drm_rect *fbc_dirty_rect = &fbc->dirty_rect;
+
+	if (need_full_region_update) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Full region update needed\n",
+			    plane->base.base.id, plane->base.name);
+		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
+		goto out;
+	}
+
+	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 {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Damage clips merge cal failed. Use full region\n",
+			    plane->base.base.id, plane->base.name);
+
+		/* TODO! if the drm call failed, update full region? */
+		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
+	}
+
+out:
+	drm_dbg_kms(display->drm,
+		    "[PLANE:%d:%s] Dirty rect start line: %d End line: %d\n",
+		    plane->base.base.id, plane->base.name, fbc_dirty_rect->y1,
+		    fbc_dirty_rect->y2);
+}
+
 int intel_fbc_atomic_check(struct intel_atomic_state *state)
 {
 	struct intel_plane_state __maybe_unused *new_plane_state;
@@ -1673,11 +1788,26 @@ int intel_fbc_atomic_check(struct intel_atomic_state *state)
 
 	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
 					     new_plane_state, i) {
+		bool full_region_update;
 		int ret;
 
+		if (!plane->fbc)
+			continue;
+
 		ret = intel_fbc_check_plane(state, plane);
 		if (ret)
 			return ret;
+
+		if (!new_plane_state->no_fbc_reason)
+			continue;
+
+		full_region_update =
+			intel_fbc_need_full_region_update(old_plane_state,
+							  new_plane_state);
+
+		intel_fbc_compute_dirty_rect(plane, old_plane_state,
+					     new_plane_state,
+					     full_region_update);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
index ceae55458e14..073d671ea94d 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,7 @@ 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_plane *plane);
 
 #endif /* __INTEL_FBC_H__ */
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 4c7bcf6806ff..fd3611323ec0 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1510,6 +1510,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
 		icl_plane_csc_load_black(dsb, plane, crtc_state);
 
 	icl_plane_update_sel_fetch_noarm(dsb, plane, crtc_state, plane_state, color_plane);
+
+	intel_fbc_program_dirty_rect(dsb, plane);
 }
 
 static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,
-- 
2.34.1

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

[Vinod Govindapillai]

FBC dirty rect support and PSR2 selective fetch canno be enabled
together. In xe3 driver enables the FBC dirty rect feature by
default. So PSR2 is enabled, then mark that plane as FBC cannot
be enabled. Later on we need to find a way to select between
FBC and PSR2 based on amount of damaged areas.

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 01080171790f..e1d55f5f2938 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1346,9 +1346,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.34.1

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

[Patchwork]

== Series Details ==

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

== Summary ==

Error: dim checkpatch failed
222407216c36 drm/i915/display: update intel_fbc_atomic_check for dirty_fbc support
23854babc8aa drm/i915/display: 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
502e65cdddca drm/i915/xe3: add dirty rect support for FBC
-:129: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#129: FILE: drivers/gpu/drm/i915/display/intel_fbc.c:1746:
+static void intel_fbc_compute_dirty_rect(struct intel_plane *plane,
+					struct intel_plane_state *old_plane_state,

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

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

[Patchwork]

== Series Details ==

Series: drm/i915/xe3: FBC Dirty rect feature support
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.

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

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_15717 -> Patchwork_141527v1
====================================================

Summary
-------

  **FAILURE**

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

Participating hosts (46 -> 45)
------------------------------

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-skl-6600u:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-skl-6600u/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-skl-6600u/igt@i915_module_load@load.html
    - fi-hsw-4770:        [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-hsw-4770/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-hsw-4770/igt@i915_module_load@load.html
    - fi-ivb-3770:        [PASS][5] -> [DMESG-WARN][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-ivb-3770/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-ivb-3770/igt@i915_module_load@load.html

  * igt@i915_pm_rpm@module-reload:
    - bat-adls-6:         [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-adls-6/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-adls-6/igt@i915_pm_rpm@module-reload.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12:
    - fi-cfl-8700k:       [PASS][9] -> [DMESG-WARN][10] +1 other test dmesg-warn
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-cfl-8700k/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-cfl-8700k/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-dp-1:
    - fi-cfl-8109u:       [PASS][11] -> [DMESG-WARN][12] +1 other test dmesg-warn
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-dp-1.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-dp-1.html
    - fi-kbl-7567u:       [PASS][13] -> [DMESG-WARN][14] +1 other test dmesg-warn
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-kbl-7567u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-dp-1.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-kbl-7567u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-dp-1.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-hdmi-a-1:
    - fi-cfl-guc:         [PASS][15] -> [DMESG-WARN][16] +1 other test dmesg-warn
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-cfl-guc/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-hdmi-a-1.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-cfl-guc/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-hdmi-a-1.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@load:
    - bat-apl-1:          [PASS][17] -> [DMESG-WARN][18] ([i915#1982])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-apl-1/igt@i915_module_load@load.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-apl-1/igt@i915_module_load@load.html

  * igt@i915_selftest@live:
    - bat-arlh-3:         [PASS][19] -> [ABORT][20] ([i915#12829])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-3/igt@i915_selftest@live.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-arlh-3/igt@i915_selftest@live.html
    - bat-adlp-9:         [PASS][21] -> [INCOMPLETE][22] ([i915#12829] / [i915#9413])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-adlp-9/igt@i915_selftest@live.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-adlp-9/igt@i915_selftest@live.html

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

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [PASS][25] -> [ABORT][26] ([i915#12061])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html

  * igt@runner@aborted:
    - bat-dg2-13:         NOTRUN -> [FAIL][27] ([i915#12658])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-dg2-13/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@core_auth@basic-auth:
    - bat-twl-1:          [DMESG-WARN][28] ([i915#1982]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-twl-1/igt@core_auth@basic-auth.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-twl-1/igt@core_auth@basic-auth.html

  * igt@dmabuf@all-tests:
    - bat-apl-1:          [INCOMPLETE][30] -> [PASS][31] +1 other test pass
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-apl-1/igt@dmabuf@all-tests.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-apl-1/igt@dmabuf@all-tests.html

  * igt@i915_pm_rpm@module-reload:
    - bat-rpls-4:         [FAIL][32] -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-rpls-4/igt@i915_pm_rpm@module-reload.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-rpls-4/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live:
    - bat-arlh-2:         [ABORT][34] ([i915#12829]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-2/igt@i915_selftest@live.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-arlh-2/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-2:         [ABORT][36] ([i915#12061]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-arlh-2/igt@i915_selftest@live@workarounds.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-arlh-2/igt@i915_selftest@live@workarounds.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1:
    - fi-cfl-8109u:       [DMESG-WARN][38] -> [PASS][39] +2 other tests pass
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/fi-cfl-8109u/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/fi-cfl-8109u/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         [SKIP][40] ([i915#9197]) -> [PASS][41] +3 other tests pass
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15717/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_141527v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

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

  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12658]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12658
  [i915#12829]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12829
  [i915#1982]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1982
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197
  [i915#9413]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9413


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

  * Linux: CI_DRM_15717 -> Patchwork_141527v1

  CI-20190529: 20190529
  CI_DRM_15717: 1fe9a6cc7d136c9a34c47ccd6ee5a2b7d02c0bd6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8115: 4942fc57c20f9cb2195e70991c4e4df03dd3db21 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_141527v1: 1fe9a6cc7d136c9a34c47ccd6ee5a2b7d02c0bd6 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

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

[Jani Nikula]

On Tue, 19 Nov 2024, Vinod Govindapillai <vinod.govindapillai@intel.com> 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.
>
> Bspec: 71675, 73424
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 130 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fbc.h      |   3 +
>  .../drm/i915/display/skl_universal_plane.c    |   2 +
>  3 files changed, 135 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1fdb1a3e3fbe..01080171790f 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"
> @@ -126,6 +128,8 @@ struct intel_fbc {
>  	 */
>  	struct intel_fbc_state state;
>  	const char *no_fbc_reason;
> +
> +	struct drm_rect dirty_rect;
>  };
>  
>  /* plane stride in pixels */
> @@ -670,6 +674,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);
>  }
> @@ -1664,6 +1672,113 @@ void intel_fbc_flush(struct drm_i915_private *i915,
>  		__intel_fbc_flush(fbc, frontbuffer_bits, origin);
>  }
>  
> +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;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	if (!fbc)
> +		return;
> +
> +	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
> +			   FBC_DIRTY_RECT_START_LINE(fbc->dirty_rect.y1) |
> +			   FBC_DIRTY_RECT_END_LINE(fbc->dirty_rect.y2));
> +}
> +
> +static bool
> +intel_fbc_need_full_region_update(struct intel_plane_state *old_plane_state,
> +				  struct intel_plane_state *new_plane_state)
> +{
> +	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
> +	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
> +
> +	if (!old_fb || !new_fb)
> +		return true;
> +
> +	if (old_fb->format->format != new_fb->format->format)
> +		return true;
> +
> +	if (old_fb->modifier != new_fb->modifier)
> +		return true;
> +
> +	if (intel_fbc_plane_stride(old_plane_state) !=
> +	    intel_fbc_plane_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_stride(old_plane_state) !=
> +	    intel_fbc_cfb_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_size(old_plane_state) !=
> +	    intel_fbc_cfb_size(new_plane_state))
> +		return true;
> +
> +	return false;
> +}
> +
> +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;
> +}
> +
> +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,
> +					bool need_full_region_update)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	struct intel_fbc *fbc = plane->fbc;
> +	struct drm_rect *fbc_dirty_rect = &fbc->dirty_rect;
> +
> +	if (need_full_region_update) {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Full region update needed\n",
> +			    plane->base.base.id, plane->base.name);
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +		goto out;
> +	}
> +
> +	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 {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Damage clips merge cal failed. Use full region\n",
> +			    plane->base.base.id, plane->base.name);
> +
> +		/* TODO! if the drm call failed, update full region? */
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +	}
> +
> +out:
> +	drm_dbg_kms(display->drm,
> +		    "[PLANE:%d:%s] Dirty rect start line: %d End line: %d\n",
> +		    plane->base.base.id, plane->base.name, fbc_dirty_rect->y1,
> +		    fbc_dirty_rect->y2);
> +}
> +
>  int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  {
>  	struct intel_plane_state __maybe_unused *new_plane_state;
> @@ -1673,11 +1788,26 @@ int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  
>  	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
>  					     new_plane_state, i) {
> +		bool full_region_update;
>  		int ret;
>  
> +		if (!plane->fbc)
> +			continue;
> +
>  		ret = intel_fbc_check_plane(state, plane);
>  		if (ret)
>  			return ret;
> +
> +		if (!new_plane_state->no_fbc_reason)
> +			continue;
> +
> +		full_region_update =
> +			intel_fbc_need_full_region_update(old_plane_state,
> +							  new_plane_state);

Drive-by comment, you could drop the __maybe_unused annotations with
this.

> +
> +		intel_fbc_compute_dirty_rect(plane, old_plane_state,
> +					     new_plane_state,
> +					     full_region_update);
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index ceae55458e14..073d671ea94d 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,7 @@ 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_plane *plane);
>  
>  #endif /* __INTEL_FBC_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4c7bcf6806ff..fd3611323ec0 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1510,6 +1510,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
>  		icl_plane_csc_load_black(dsb, plane, crtc_state);
>  
>  	icl_plane_update_sel_fetch_noarm(dsb, plane, crtc_state, plane_state, color_plane);
> +
> +	intel_fbc_program_dirty_rect(dsb, plane);
>  }
>  
>  static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,

-- 
Jani Nikula, Intel

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

[Jani Nikula]

On Tue, 19 Nov 2024, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> FBC dirty rect support and PSR2 selective fetch canno be enabled
> together. In xe3 driver enables the FBC dirty rect feature by
> default. So PSR2 is enabled, then mark that plane as FBC cannot
> be enabled. Later on we need to find a way to select between
> FBC and PSR2 based on amount of damaged areas.
>
> 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 01080171790f..e1d55f5f2938 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1346,9 +1346,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 &&

This will never be true.

BR,
Jani.

> +	    crtc_state->has_sel_update && !crtc_state->has_panel_replay) {
>  		plane_state->no_fbc_reason = "PSR2 enabled";
>  		return 0;
>  	}

-- 
Jani Nikula, Intel

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

[Govindapillai, Vinod]

On Tue, 2024-11-19 at 10:51 +0200, Jani Nikula wrote:
> On Tue, 19 Nov 2024, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> > FBC dirty rect support and PSR2 selective fetch canno be enabled
> > together. In xe3 driver enables the FBC dirty rect feature by
> > default. So PSR2 is enabled, then mark that plane as FBC cannot
> > be enabled. Later on we need to find a way to select between
> > FBC and PSR2 based on amount of damaged areas.
> > 
> > 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 01080171790f..e1d55f5f2938 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -1346,9 +1346,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 &&
> 
> This will never be true.
> 
> BR,
> Jani.

oops! Thanks! Will fix that!

BR
vinod
> 
> > +           crtc_state->has_sel_update && !crtc_state->has_panel_replay) {
> >                 plane_state->no_fbc_reason = "PSR2 enabled";
> >                 return 0;
> >         }
> 

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

[Ville Syrjälä]

On Tue, Nov 19, 2024 at 01:53:24AM +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.
> 
> Bspec: 71675, 73424
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 130 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fbc.h      |   3 +
>  .../drm/i915/display/skl_universal_plane.c    |   2 +
>  3 files changed, 135 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1fdb1a3e3fbe..01080171790f 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"
> @@ -126,6 +128,8 @@ struct intel_fbc {
>  	 */
>  	struct intel_fbc_state state;
>  	const char *no_fbc_reason;
> +
> +	struct drm_rect dirty_rect;

That belongs in the fbc state.

>  };
>  
>  /* plane stride in pixels */
> @@ -670,6 +674,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);
>  }
> @@ -1664,6 +1672,113 @@ void intel_fbc_flush(struct drm_i915_private *i915,
>  		__intel_fbc_flush(fbc, frontbuffer_bits, origin);
>  }
>  
> +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;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	if (!fbc)
> +		return;
> +
> +	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
> +			   FBC_DIRTY_RECT_START_LINE(fbc->dirty_rect.y1) |
> +			   FBC_DIRTY_RECT_END_LINE(fbc->dirty_rect.y2));

I suspect the end line needs a -1. But that needs to be confirmed on
actual hardware.

> +}
> +
> +static bool
> +intel_fbc_need_full_region_update(struct intel_plane_state *old_plane_state,
> +				  struct intel_plane_state *new_plane_state)
> +{
> +	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
> +	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
> +
> +	if (!old_fb || !new_fb)
> +		return true;
> +
> +	if (old_fb->format->format != new_fb->format->format)
> +		return true;
> +
> +	if (old_fb->modifier != new_fb->modifier)
> +		return true;
> +
> +	if (intel_fbc_plane_stride(old_plane_state) !=
> +	    intel_fbc_plane_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_stride(old_plane_state) !=
> +	    intel_fbc_cfb_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_size(old_plane_state) !=
> +	    intel_fbc_cfb_size(new_plane_state))
> +		return true;
> +
> +	return false;
> +}

If any of those fail then I think we're going to be
disabling+re-enabling FBC anyway. So we might not need to check
any of this, or if we do we could perhaps just use can_flip_nuke().

> +
> +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;
> +}
> +
> +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,
> +					bool need_full_region_update)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	struct intel_fbc *fbc = plane->fbc;
> +	struct drm_rect *fbc_dirty_rect = &fbc->dirty_rect;
> +
> +	if (need_full_region_update) {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Full region update needed\n",
> +			    plane->base.base.id, plane->base.name);
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +		goto out;
> +	}
> +
> +	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);

Your fbc_dirty_rect here will still be in a different coordinate space
(fb relative) than the plane src coordinates (PLANE_SURF relative).

> +	} else {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Damage clips merge cal failed. Use full region\n",
> +			    plane->base.base.id, plane->base.name);
> +

That's going to create excessive spam.

> +		/* TODO! if the drm call failed, update full region? */
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +	}
> +
> +out:
> +	drm_dbg_kms(display->drm,
> +		    "[PLANE:%d:%s] Dirty rect start line: %d End line: %d\n",
> +		    plane->base.base.id, plane->base.name, fbc_dirty_rect->y1,
> +		    fbc_dirty_rect->y2);a

ditto

> +}
> +
>  int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  {
>  	struct intel_plane_state __maybe_unused *new_plane_state;
> @@ -1673,11 +1788,26 @@ int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  
>  	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
>  					     new_plane_state, i) {
> +		bool full_region_update;
>  		int ret;
>  
> +		if (!plane->fbc)
> +			continue;
> +
>  		ret = intel_fbc_check_plane(state, plane);
>  		if (ret)
>  			return ret;
> +
> +		if (!new_plane_state->no_fbc_reason)
> +			continue;
> +
> +		full_region_update =
> +			intel_fbc_need_full_region_update(old_plane_state,
> +							  new_plane_state);
> +
> +		intel_fbc_compute_dirty_rect(plane, old_plane_state,
> +					     new_plane_state,
> +					     full_region_update);

That belongs in intel_fbc_update_state()

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index ceae55458e14..073d671ea94d 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,7 @@ 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_plane *plane);
>  
>  #endif /* __INTEL_FBC_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4c7bcf6806ff..fd3611323ec0 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1510,6 +1510,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
>  		icl_plane_csc_load_black(dsb, plane, crtc_state);
>  
>  	icl_plane_update_sel_fetch_noarm(dsb, plane, crtc_state, plane_state, color_plane);
> +
> +	intel_fbc_program_dirty_rect(dsb, plane);

I think we want a completely separate call to FBC around where call
intel_crtc_planes_update_arm().

It's going to be a bit dodgy becase we won't be able take fbc->lock
and thus accessing the fbc state needs to be done locklessly. But
I *think* it should be safe because the fbc state should only be
updated from intel_fbc_{update,disable}() which are done from the
atomic commit path only.

We are going to need some real tests for this:
- update region A -> flip with dirty rect A -> update region B -> flip with dirty rect B
- update region A -> flip with dirty rect A -> update region B -> flip without dirty rect
- update region A -> flip with dirty rect A -> update region B -> dirtyfb
- probably some other ones which involve format changes/etc. as well.

>  }
>  
>  static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel