[PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

As a fallback if we decide not to merge the frontbuffer tracking, allow
i915 to keep its own implementation, and do the right thing in Xe.

The frontbuffer tracking for Xe is still done per-fb, while i915 can
keep doing the weird intel_frontbuffer + i915_active thing without
blocking Xe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
 drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
 drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
 drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
 .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
 .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
 drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
 .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
 8 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f2918bb07107..a4a57aa24422 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -133,7 +133,11 @@ struct intel_fb_view {
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
+#ifdef I915
 	struct intel_frontbuffer *frontbuffer;
+#else
+	atomic_t bits;
+#endif
 
 	/* Params to remap the FB pages and program the plane registers in each view. */
 	struct intel_fb_view normal_view;
@@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
 #endif
 }
 
+#ifdef I915
 static inline struct intel_frontbuffer *
 to_intel_frontbuffer(struct drm_framebuffer *fb)
 {
 	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
 }
+#else
+static inline struct intel_framebuffer *
+to_intel_frontbuffer(struct drm_framebuffer *fb)
+{
+	return fb ? to_intel_framebuffer(fb) : NULL;
+}
+#endif
 
 #endif /*  __INTEL_DISPLAY_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
index 5b9e44443814..3503d112387d 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -9,6 +9,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_drrs.h"
+#include "intel_frontbuffer.h"
 #include "intel_panel.h"
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index 8c357a4098f6..e67c71f9b29d 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 #ifdef I915
 	if (intel_fb_uses_dpt(fb))
 		intel_dpt_destroy(intel_fb->dpt_vm);
+
+	intel_frontbuffer_put(intel_fb->frontbuffer);
 #else
 	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
 		struct xe_bo *bo = intel_fb_obj(fb);
@@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 	}
 #endif
 
-	intel_frontbuffer_put(intel_fb->frontbuffer);
-
 	kfree(intel_fb);
 }
 
@@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		obj->flags |= XE_BO_SCANOUT_BIT;
 	}
 	ttm_bo_unreserve(&obj->ttm);
-#endif
 
 	atomic_set(&intel_fb->bits, 0);
+#endif
 
 	if (!drm_any_plane_has_format(&dev_priv->drm,
 				      mode_cmd->pixel_format,
@@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return 0;
 
 err:
+#ifdef I915
 	intel_frontbuffer_put(intel_fb->frontbuffer);
+#endif
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 75d8029185f0..2682b26b511f 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -82,7 +82,7 @@ struct intel_fbdev {
 
 static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
 {
-	return ifbdev->fb->frontbuffer;
+	return to_intel_frontbuffer(&ifbdev->fb->base);
 }
 
 static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
index 17a7aa8b28c2..64fe73d2ac4d 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
@@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
 	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
 }
 
+#ifdef I915
+#define intel_front_obj(front) ((front)->obj)
+#else
+#define intel_front_obj(front) (front)
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		      enum fb_op_origin origin,
 		      unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		frontbuffer_flush(i915, frontbuffer_bits, origin);
 }
 
+#ifdef I915
 static int frontbuffer_active(struct i915_active *ref)
 {
 	struct intel_frontbuffer *front =
@@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
 		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
 }
 
+#endif
+
 /**
  * intel_frontbuffer_track - update frontbuffer tracking
  * @old: current buffer for the frontbuffer slots
@@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
 	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
 
 	if (old) {
-		drm_WARN_ON(old->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(old)->base.dev,
 			    !(atomic_read(&old->bits) & frontbuffer_bits));
 		atomic_andnot(frontbuffer_bits, &old->bits);
 	}
 
 	if (new) {
-		drm_WARN_ON(new->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(new)->base.dev,
 			    atomic_read(&new->bits) & frontbuffer_bits);
 		atomic_or(frontbuffer_bits, &new->bits);
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
index 3c474ed937fb..a79e5ca419ec 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
@@ -28,8 +28,6 @@
 #include <linux/bits.h>
 #include <linux/kref.h>
 
-#include "gem/i915_gem_object_types.h"
-#include "i915_active_types.h"
 
 struct drm_i915_private;
 
@@ -41,6 +39,10 @@ enum fb_op_origin {
 	ORIGIN_CURSOR_UPDATE,
 };
 
+#ifdef I915
+#include "gem/i915_gem_object_types.h"
+#include "i915_active_types.h"
+
 struct intel_frontbuffer {
 	struct kref ref;
 	atomic_t bits;
@@ -48,6 +50,9 @@ struct intel_frontbuffer {
 	struct drm_i915_gem_object *obj;
 	struct rcu_head rcu;
 };
+#else
+#define intel_frontbuffer intel_framebuffer
+#endif
 
 /*
  * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
@@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
 void intel_frontbuffer_flip(struct drm_i915_private *i915,
 			    unsigned frontbuffer_bits);
 
+#ifdef I915
 void intel_frontbuffer_put(struct intel_frontbuffer *front);
 
 static inline struct intel_frontbuffer *
@@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
 struct intel_frontbuffer *
 intel_frontbuffer_get(struct drm_i915_gem_object *obj);
 
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 9820e5fdd087..bc998b526d88 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -33,6 +33,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dp_aux.h"
+#include "intel_frontbuffer.h"
 #include "intel_hdmi.h"
 #include "intel_psr.h"
 #include "intel_snps_phy.h"
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 38f70f27ff1b..86d022e195c7 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -16,11 +16,13 @@
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_fbc.h"
+#include "intel_frontbuffer.h"
 #include "intel_psr.h"
 #include "intel_sprite.h"
 #include "skl_scaler.h"
 #include "skl_universal_plane.h"
 #include "skl_watermark.h"
+
 #ifdef I915
 #include "pxp/intel_pxp.h"
 #else
-- 
2.34.1

[Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

As a fallback if we decide not to merge the frontbuffer tracking, allow
i915 to keep its own implementation, and do the right thing in Xe.

The frontbuffer tracking for Xe is still done per-fb, while i915 can
keep doing the weird intel_frontbuffer + i915_active thing without
blocking Xe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
 drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
 drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
 drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
 .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
 .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
 drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
 .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
 8 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f2918bb07107..a4a57aa24422 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -133,7 +133,11 @@ struct intel_fb_view {
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
+#ifdef I915
 	struct intel_frontbuffer *frontbuffer;
+#else
+	atomic_t bits;
+#endif
 
 	/* Params to remap the FB pages and program the plane registers in each view. */
 	struct intel_fb_view normal_view;
@@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
 #endif
 }
 
+#ifdef I915
 static inline struct intel_frontbuffer *
 to_intel_frontbuffer(struct drm_framebuffer *fb)
 {
 	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
 }
+#else
+static inline struct intel_framebuffer *
+to_intel_frontbuffer(struct drm_framebuffer *fb)
+{
+	return fb ? to_intel_framebuffer(fb) : NULL;
+}
+#endif
 
 #endif /*  __INTEL_DISPLAY_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
index 5b9e44443814..3503d112387d 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -9,6 +9,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_drrs.h"
+#include "intel_frontbuffer.h"
 #include "intel_panel.h"
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index 8c357a4098f6..e67c71f9b29d 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 #ifdef I915
 	if (intel_fb_uses_dpt(fb))
 		intel_dpt_destroy(intel_fb->dpt_vm);
+
+	intel_frontbuffer_put(intel_fb->frontbuffer);
 #else
 	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
 		struct xe_bo *bo = intel_fb_obj(fb);
@@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 	}
 #endif
 
-	intel_frontbuffer_put(intel_fb->frontbuffer);
-
 	kfree(intel_fb);
 }
 
@@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		obj->flags |= XE_BO_SCANOUT_BIT;
 	}
 	ttm_bo_unreserve(&obj->ttm);
-#endif
 
 	atomic_set(&intel_fb->bits, 0);
+#endif
 
 	if (!drm_any_plane_has_format(&dev_priv->drm,
 				      mode_cmd->pixel_format,
@@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return 0;
 
 err:
+#ifdef I915
 	intel_frontbuffer_put(intel_fb->frontbuffer);
+#endif
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 75d8029185f0..2682b26b511f 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -82,7 +82,7 @@ struct intel_fbdev {
 
 static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
 {
-	return ifbdev->fb->frontbuffer;
+	return to_intel_frontbuffer(&ifbdev->fb->base);
 }
 
 static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
index 17a7aa8b28c2..64fe73d2ac4d 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
@@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
 	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
 }
 
+#ifdef I915
+#define intel_front_obj(front) ((front)->obj)
+#else
+#define intel_front_obj(front) (front)
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		      enum fb_op_origin origin,
 		      unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		frontbuffer_flush(i915, frontbuffer_bits, origin);
 }
 
+#ifdef I915
 static int frontbuffer_active(struct i915_active *ref)
 {
 	struct intel_frontbuffer *front =
@@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
 		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
 }
 
+#endif
+
 /**
  * intel_frontbuffer_track - update frontbuffer tracking
  * @old: current buffer for the frontbuffer slots
@@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
 	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
 
 	if (old) {
-		drm_WARN_ON(old->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(old)->base.dev,
 			    !(atomic_read(&old->bits) & frontbuffer_bits));
 		atomic_andnot(frontbuffer_bits, &old->bits);
 	}
 
 	if (new) {
-		drm_WARN_ON(new->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(new)->base.dev,
 			    atomic_read(&new->bits) & frontbuffer_bits);
 		atomic_or(frontbuffer_bits, &new->bits);
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
index 3c474ed937fb..a79e5ca419ec 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
@@ -28,8 +28,6 @@
 #include <linux/bits.h>
 #include <linux/kref.h>
 
-#include "gem/i915_gem_object_types.h"
-#include "i915_active_types.h"
 
 struct drm_i915_private;
 
@@ -41,6 +39,10 @@ enum fb_op_origin {
 	ORIGIN_CURSOR_UPDATE,
 };
 
+#ifdef I915
+#include "gem/i915_gem_object_types.h"
+#include "i915_active_types.h"
+
 struct intel_frontbuffer {
 	struct kref ref;
 	atomic_t bits;
@@ -48,6 +50,9 @@ struct intel_frontbuffer {
 	struct drm_i915_gem_object *obj;
 	struct rcu_head rcu;
 };
+#else
+#define intel_frontbuffer intel_framebuffer
+#endif
 
 /*
  * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
@@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
 void intel_frontbuffer_flip(struct drm_i915_private *i915,
 			    unsigned frontbuffer_bits);
 
+#ifdef I915
 void intel_frontbuffer_put(struct intel_frontbuffer *front);
 
 static inline struct intel_frontbuffer *
@@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
 struct intel_frontbuffer *
 intel_frontbuffer_get(struct drm_i915_gem_object *obj);
 
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 9820e5fdd087..bc998b526d88 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -33,6 +33,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dp_aux.h"
+#include "intel_frontbuffer.h"
 #include "intel_hdmi.h"
 #include "intel_psr.h"
 #include "intel_snps_phy.h"
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 38f70f27ff1b..86d022e195c7 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -16,11 +16,13 @@
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_fbc.h"
+#include "intel_frontbuffer.h"
 #include "intel_psr.h"
 #include "intel_sprite.h"
 #include "skl_scaler.h"
 #include "skl_universal_plane.h"
 #include "skl_watermark.h"
+
 #ifdef I915
 #include "pxp/intel_pxp.h"
 #else
-- 
2.34.1

[Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

As a fallback if we decide not to merge the frontbuffer tracking, allow
i915 to keep its own implementation, and do the right thing in Xe.

The frontbuffer tracking for Xe is still done per-fb, while i915 can
keep doing the weird intel_frontbuffer + i915_active thing without
blocking Xe.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
 drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
 drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
 drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
 .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
 .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
 drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
 .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
 8 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index f2918bb07107..a4a57aa24422 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -133,7 +133,11 @@ struct intel_fb_view {
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
+#ifdef I915
 	struct intel_frontbuffer *frontbuffer;
+#else
+	atomic_t bits;
+#endif
 
 	/* Params to remap the FB pages and program the plane registers in each view. */
 	struct intel_fb_view normal_view;
@@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
 #endif
 }
 
+#ifdef I915
 static inline struct intel_frontbuffer *
 to_intel_frontbuffer(struct drm_framebuffer *fb)
 {
 	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
 }
+#else
+static inline struct intel_framebuffer *
+to_intel_frontbuffer(struct drm_framebuffer *fb)
+{
+	return fb ? to_intel_framebuffer(fb) : NULL;
+}
+#endif
 
 #endif /*  __INTEL_DISPLAY_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
index 5b9e44443814..3503d112387d 100644
--- a/drivers/gpu/drm/i915/display/intel_drrs.c
+++ b/drivers/gpu/drm/i915/display/intel_drrs.c
@@ -9,6 +9,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_drrs.h"
+#include "intel_frontbuffer.h"
 #include "intel_panel.h"
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index 8c357a4098f6..e67c71f9b29d 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 #ifdef I915
 	if (intel_fb_uses_dpt(fb))
 		intel_dpt_destroy(intel_fb->dpt_vm);
+
+	intel_frontbuffer_put(intel_fb->frontbuffer);
 #else
 	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
 		struct xe_bo *bo = intel_fb_obj(fb);
@@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 	}
 #endif
 
-	intel_frontbuffer_put(intel_fb->frontbuffer);
-
 	kfree(intel_fb);
 }
 
@@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		obj->flags |= XE_BO_SCANOUT_BIT;
 	}
 	ttm_bo_unreserve(&obj->ttm);
-#endif
 
 	atomic_set(&intel_fb->bits, 0);
+#endif
 
 	if (!drm_any_plane_has_format(&dev_priv->drm,
 				      mode_cmd->pixel_format,
@@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return 0;
 
 err:
+#ifdef I915
 	intel_frontbuffer_put(intel_fb->frontbuffer);
+#endif
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 75d8029185f0..2682b26b511f 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -82,7 +82,7 @@ struct intel_fbdev {
 
 static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
 {
-	return ifbdev->fb->frontbuffer;
+	return to_intel_frontbuffer(&ifbdev->fb->base);
 }
 
 static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
index 17a7aa8b28c2..64fe73d2ac4d 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
@@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
 	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
 }
 
+#ifdef I915
+#define intel_front_obj(front) ((front)->obj)
+#else
+#define intel_front_obj(front) (front)
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		      enum fb_op_origin origin,
 		      unsigned int frontbuffer_bits)
 {
-	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
+	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
 
 	if (origin == ORIGIN_CS) {
 		spin_lock(&i915->display.fb_tracking.lock);
@@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
 		frontbuffer_flush(i915, frontbuffer_bits, origin);
 }
 
+#ifdef I915
 static int frontbuffer_active(struct i915_active *ref)
 {
 	struct intel_frontbuffer *front =
@@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
 		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
 }
 
+#endif
+
 /**
  * intel_frontbuffer_track - update frontbuffer tracking
  * @old: current buffer for the frontbuffer slots
@@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
 	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
 
 	if (old) {
-		drm_WARN_ON(old->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(old)->base.dev,
 			    !(atomic_read(&old->bits) & frontbuffer_bits));
 		atomic_andnot(frontbuffer_bits, &old->bits);
 	}
 
 	if (new) {
-		drm_WARN_ON(new->obj->base.dev,
+		drm_WARN_ON(intel_front_obj(new)->base.dev,
 			    atomic_read(&new->bits) & frontbuffer_bits);
 		atomic_or(frontbuffer_bits, &new->bits);
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
index 3c474ed937fb..a79e5ca419ec 100644
--- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
+++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
@@ -28,8 +28,6 @@
 #include <linux/bits.h>
 #include <linux/kref.h>
 
-#include "gem/i915_gem_object_types.h"
-#include "i915_active_types.h"
 
 struct drm_i915_private;
 
@@ -41,6 +39,10 @@ enum fb_op_origin {
 	ORIGIN_CURSOR_UPDATE,
 };
 
+#ifdef I915
+#include "gem/i915_gem_object_types.h"
+#include "i915_active_types.h"
+
 struct intel_frontbuffer {
 	struct kref ref;
 	atomic_t bits;
@@ -48,6 +50,9 @@ struct intel_frontbuffer {
 	struct drm_i915_gem_object *obj;
 	struct rcu_head rcu;
 };
+#else
+#define intel_frontbuffer intel_framebuffer
+#endif
 
 /*
  * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
@@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
 void intel_frontbuffer_flip(struct drm_i915_private *i915,
 			    unsigned frontbuffer_bits);
 
+#ifdef I915
 void intel_frontbuffer_put(struct intel_frontbuffer *front);
 
 static inline struct intel_frontbuffer *
@@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
 struct intel_frontbuffer *
 intel_frontbuffer_get(struct drm_i915_gem_object *obj);
 
+#endif
+
 void __intel_fb_invalidate(struct intel_frontbuffer *front,
 			   enum fb_op_origin origin,
 			   unsigned int frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 9820e5fdd087..bc998b526d88 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -33,6 +33,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dp_aux.h"
+#include "intel_frontbuffer.h"
 #include "intel_hdmi.h"
 #include "intel_psr.h"
 #include "intel_snps_phy.h"
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 38f70f27ff1b..86d022e195c7 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -16,11 +16,13 @@
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_fbc.h"
+#include "intel_frontbuffer.h"
 #include "intel_psr.h"
 #include "intel_sprite.h"
 #include "skl_scaler.h"
 #include "skl_universal_plane.h"
 #include "skl_watermark.h"
+
 #ifdef I915
 #include "pxp/intel_pxp.h"
 #else
-- 
2.34.1

[Intel-xe] ✗ CI.Patch_applied: failure for drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Patchwork]

== Series Details ==

Series: drm/xe/display: Do not use i915 frontbuffer tracking implementation
URL   : https://patchwork.freedesktop.org/series/114712/
State : failure

== Summary ==

error: patch failed: drivers/gpu/drm/i915/display/intel_display_types.h:133
error: drivers/gpu/drm/i915/display/intel_display_types.h: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_drrs.c:9
error: drivers/gpu/drm/i915/display/intel_drrs.c: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_fb.c:1857
error: drivers/gpu/drm/i915/display/intel_fb.c: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_fbdev.c:82
error: drivers/gpu/drm/i915/display/intel_fbdev.c: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_frontbuffer.c:163
error: drivers/gpu/drm/i915/display/intel_frontbuffer.c: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_frontbuffer.h:28
error: drivers/gpu/drm/i915/display/intel_frontbuffer.h: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/intel_psr.c:33
error: drivers/gpu/drm/i915/display/intel_psr.c: patch does not apply
error: patch failed: drivers/gpu/drm/i915/display/skl_universal_plane.c:16
error: drivers/gpu/drm/i915/display/skl_universal_plane.c: patch does not apply

Re: [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Souza, Jose]

On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> As a fallback if we decide not to merge the frontbuffer tracking, allow
> i915 to keep its own implementation, and do the right thing in Xe.
> 
> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> keep doing the weird intel_frontbuffer + i915_active thing without
> blocking Xe.

Please also disable PSR and FBC with this or at least add a way for users to disable those features.
Without frontbuffer tracker those two features will break in some cases.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
>  drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
>  drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
>  drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
>  .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
>  .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
>  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
>  .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
>  8 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index f2918bb07107..a4a57aa24422 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -133,7 +133,11 @@ struct intel_fb_view {
>  
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
> +#ifdef I915
>  	struct intel_frontbuffer *frontbuffer;
> +#else
> +	atomic_t bits;
> +#endif
>  
>  	/* Params to remap the FB pages and program the plane registers in each view. */
>  	struct intel_fb_view normal_view;
> @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
>  #endif
>  }
>  
> +#ifdef I915
>  static inline struct intel_frontbuffer *
>  to_intel_frontbuffer(struct drm_framebuffer *fb)
>  {
>  	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
>  }
> +#else
> +static inline struct intel_framebuffer *
> +to_intel_frontbuffer(struct drm_framebuffer *fb)
> +{
> +	return fb ? to_intel_framebuffer(fb) : NULL;
> +}
> +#endif
>  
>  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 5b9e44443814..3503d112387d 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -9,6 +9,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_drrs.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_panel.h"
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index 8c357a4098f6..e67c71f9b29d 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  #ifdef I915
>  	if (intel_fb_uses_dpt(fb))
>  		intel_dpt_destroy(intel_fb->dpt_vm);
> +
> +	intel_frontbuffer_put(intel_fb->frontbuffer);
>  #else
>  	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
>  		struct xe_bo *bo = intel_fb_obj(fb);
> @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  	}
>  #endif
>  
> -	intel_frontbuffer_put(intel_fb->frontbuffer);
> -
>  	kfree(intel_fb);
>  }
>  
> @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		obj->flags |= XE_BO_SCANOUT_BIT;
>  	}
>  	ttm_bo_unreserve(&obj->ttm);
> -#endif
>  
>  	atomic_set(&intel_fb->bits, 0);
> +#endif
>  
>  	if (!drm_any_plane_has_format(&dev_priv->drm,
>  				      mode_cmd->pixel_format,
> @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	return 0;
>  
>  err:
> +#ifdef I915
>  	intel_frontbuffer_put(intel_fb->frontbuffer);
> +#endif
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 75d8029185f0..2682b26b511f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -82,7 +82,7 @@ struct intel_fbdev {
>  
>  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>  {
> -	return ifbdev->fb->frontbuffer;
> +	return to_intel_frontbuffer(&ifbdev->fb->base);
>  }
>  
>  static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index 17a7aa8b28c2..64fe73d2ac4d 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
>  	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
>  }
>  
> +#ifdef I915
> +#define intel_front_obj(front) ((front)->obj)
> +#else
> +#define intel_front_obj(front) (front)
> +#endif
> +
>  void __intel_fb_invalidate(struct intel_frontbuffer *front,
>  			   enum fb_op_origin origin,
>  			   unsigned int frontbuffer_bits)
>  {
> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>  
>  	if (origin == ORIGIN_CS) {
>  		spin_lock(&i915->display.fb_tracking.lock);
> @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>  		      enum fb_op_origin origin,
>  		      unsigned int frontbuffer_bits)
>  {
> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>  
>  	if (origin == ORIGIN_CS) {
>  		spin_lock(&i915->display.fb_tracking.lock);
> @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>  		frontbuffer_flush(i915, frontbuffer_bits, origin);
>  }
>  
> +#ifdef I915
>  static int frontbuffer_active(struct i915_active *ref)
>  {
>  	struct intel_frontbuffer *front =
> @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
>  		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
>  }
>  
> +#endif
> +
>  /**
>   * intel_frontbuffer_track - update frontbuffer tracking
>   * @old: current buffer for the frontbuffer slots
> @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
>  	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
>  
>  	if (old) {
> -		drm_WARN_ON(old->obj->base.dev,
> +		drm_WARN_ON(intel_front_obj(old)->base.dev,
>  			    !(atomic_read(&old->bits) & frontbuffer_bits));
>  		atomic_andnot(frontbuffer_bits, &old->bits);
>  	}
>  
>  	if (new) {
> -		drm_WARN_ON(new->obj->base.dev,
> +		drm_WARN_ON(intel_front_obj(new)->base.dev,
>  			    atomic_read(&new->bits) & frontbuffer_bits);
>  		atomic_or(frontbuffer_bits, &new->bits);
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> index 3c474ed937fb..a79e5ca419ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> @@ -28,8 +28,6 @@
>  #include <linux/bits.h>
>  #include <linux/kref.h>
>  
> -#include "gem/i915_gem_object_types.h"
> -#include "i915_active_types.h"
>  
>  struct drm_i915_private;
>  
> @@ -41,6 +39,10 @@ enum fb_op_origin {
>  	ORIGIN_CURSOR_UPDATE,
>  };
>  
> +#ifdef I915
> +#include "gem/i915_gem_object_types.h"
> +#include "i915_active_types.h"
> +
>  struct intel_frontbuffer {
>  	struct kref ref;
>  	atomic_t bits;
> @@ -48,6 +50,9 @@ struct intel_frontbuffer {
>  	struct drm_i915_gem_object *obj;
>  	struct rcu_head rcu;
>  };
> +#else
> +#define intel_frontbuffer intel_framebuffer
> +#endif
>  
>  /*
>   * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>  void intel_frontbuffer_flip(struct drm_i915_private *i915,
>  			    unsigned frontbuffer_bits);
>  
> +#ifdef I915
>  void intel_frontbuffer_put(struct intel_frontbuffer *front);
>  
>  static inline struct intel_frontbuffer *
> @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
>  struct intel_frontbuffer *
>  intel_frontbuffer_get(struct drm_i915_gem_object *obj);
>  
> +#endif
> +
>  void __intel_fb_invalidate(struct intel_frontbuffer *front,
>  			   enum fb_op_origin origin,
>  			   unsigned int frontbuffer_bits);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 9820e5fdd087..bc998b526d88 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -33,6 +33,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_dp_aux.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_hdmi.h"
>  #include "intel_psr.h"
>  #include "intel_snps_phy.h"
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 38f70f27ff1b..86d022e195c7 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -16,11 +16,13 @@
>  #include "intel_display_types.h"
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_psr.h"
>  #include "intel_sprite.h"
>  #include "skl_scaler.h"
>  #include "skl_universal_plane.h"
>  #include "skl_watermark.h"
> +
>  #ifdef I915
>  #include "pxp/intel_pxp.h"
>  #else

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Souza, Jose]

On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> As a fallback if we decide not to merge the frontbuffer tracking, allow
> i915 to keep its own implementation, and do the right thing in Xe.
> 
> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> keep doing the weird intel_frontbuffer + i915_active thing without
> blocking Xe.

Please also disable PSR and FBC with this or at least add a way for users to disable those features.
Without frontbuffer tracker those two features will break in some cases.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
>  drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
>  drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
>  drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
>  .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
>  .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
>  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
>  .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
>  8 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index f2918bb07107..a4a57aa24422 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -133,7 +133,11 @@ struct intel_fb_view {
>  
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
> +#ifdef I915
>  	struct intel_frontbuffer *frontbuffer;
> +#else
> +	atomic_t bits;
> +#endif
>  
>  	/* Params to remap the FB pages and program the plane registers in each view. */
>  	struct intel_fb_view normal_view;
> @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
>  #endif
>  }
>  
> +#ifdef I915
>  static inline struct intel_frontbuffer *
>  to_intel_frontbuffer(struct drm_framebuffer *fb)
>  {
>  	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
>  }
> +#else
> +static inline struct intel_framebuffer *
> +to_intel_frontbuffer(struct drm_framebuffer *fb)
> +{
> +	return fb ? to_intel_framebuffer(fb) : NULL;
> +}
> +#endif
>  
>  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 5b9e44443814..3503d112387d 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -9,6 +9,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_drrs.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_panel.h"
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index 8c357a4098f6..e67c71f9b29d 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  #ifdef I915
>  	if (intel_fb_uses_dpt(fb))
>  		intel_dpt_destroy(intel_fb->dpt_vm);
> +
> +	intel_frontbuffer_put(intel_fb->frontbuffer);
>  #else
>  	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
>  		struct xe_bo *bo = intel_fb_obj(fb);
> @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  	}
>  #endif
>  
> -	intel_frontbuffer_put(intel_fb->frontbuffer);
> -
>  	kfree(intel_fb);
>  }
>  
> @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  		obj->flags |= XE_BO_SCANOUT_BIT;
>  	}
>  	ttm_bo_unreserve(&obj->ttm);
> -#endif
>  
>  	atomic_set(&intel_fb->bits, 0);
> +#endif
>  
>  	if (!drm_any_plane_has_format(&dev_priv->drm,
>  				      mode_cmd->pixel_format,
> @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	return 0;
>  
>  err:
> +#ifdef I915
>  	intel_frontbuffer_put(intel_fb->frontbuffer);
> +#endif
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 75d8029185f0..2682b26b511f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -82,7 +82,7 @@ struct intel_fbdev {
>  
>  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>  {
> -	return ifbdev->fb->frontbuffer;
> +	return to_intel_frontbuffer(&ifbdev->fb->base);
>  }
>  
>  static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index 17a7aa8b28c2..64fe73d2ac4d 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
>  	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
>  }
>  
> +#ifdef I915
> +#define intel_front_obj(front) ((front)->obj)
> +#else
> +#define intel_front_obj(front) (front)
> +#endif
> +
>  void __intel_fb_invalidate(struct intel_frontbuffer *front,
>  			   enum fb_op_origin origin,
>  			   unsigned int frontbuffer_bits)
>  {
> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>  
>  	if (origin == ORIGIN_CS) {
>  		spin_lock(&i915->display.fb_tracking.lock);
> @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>  		      enum fb_op_origin origin,
>  		      unsigned int frontbuffer_bits)
>  {
> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>  
>  	if (origin == ORIGIN_CS) {
>  		spin_lock(&i915->display.fb_tracking.lock);
> @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>  		frontbuffer_flush(i915, frontbuffer_bits, origin);
>  }
>  
> +#ifdef I915
>  static int frontbuffer_active(struct i915_active *ref)
>  {
>  	struct intel_frontbuffer *front =
> @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
>  		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
>  }
>  
> +#endif
> +
>  /**
>   * intel_frontbuffer_track - update frontbuffer tracking
>   * @old: current buffer for the frontbuffer slots
> @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
>  	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
>  
>  	if (old) {
> -		drm_WARN_ON(old->obj->base.dev,
> +		drm_WARN_ON(intel_front_obj(old)->base.dev,
>  			    !(atomic_read(&old->bits) & frontbuffer_bits));
>  		atomic_andnot(frontbuffer_bits, &old->bits);
>  	}
>  
>  	if (new) {
> -		drm_WARN_ON(new->obj->base.dev,
> +		drm_WARN_ON(intel_front_obj(new)->base.dev,
>  			    atomic_read(&new->bits) & frontbuffer_bits);
>  		atomic_or(frontbuffer_bits, &new->bits);
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> index 3c474ed937fb..a79e5ca419ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> @@ -28,8 +28,6 @@
>  #include <linux/bits.h>
>  #include <linux/kref.h>
>  
> -#include "gem/i915_gem_object_types.h"
> -#include "i915_active_types.h"
>  
>  struct drm_i915_private;
>  
> @@ -41,6 +39,10 @@ enum fb_op_origin {
>  	ORIGIN_CURSOR_UPDATE,
>  };
>  
> +#ifdef I915
> +#include "gem/i915_gem_object_types.h"
> +#include "i915_active_types.h"
> +
>  struct intel_frontbuffer {
>  	struct kref ref;
>  	atomic_t bits;
> @@ -48,6 +50,9 @@ struct intel_frontbuffer {
>  	struct drm_i915_gem_object *obj;
>  	struct rcu_head rcu;
>  };
> +#else
> +#define intel_frontbuffer intel_framebuffer
> +#endif
>  
>  /*
>   * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>  void intel_frontbuffer_flip(struct drm_i915_private *i915,
>  			    unsigned frontbuffer_bits);
>  
> +#ifdef I915
>  void intel_frontbuffer_put(struct intel_frontbuffer *front);
>  
>  static inline struct intel_frontbuffer *
> @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
>  struct intel_frontbuffer *
>  intel_frontbuffer_get(struct drm_i915_gem_object *obj);
>  
> +#endif
> +
>  void __intel_fb_invalidate(struct intel_frontbuffer *front,
>  			   enum fb_op_origin origin,
>  			   unsigned int frontbuffer_bits);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 9820e5fdd087..bc998b526d88 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -33,6 +33,7 @@
>  #include "intel_de.h"
>  #include "intel_display_types.h"
>  #include "intel_dp_aux.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_hdmi.h"
>  #include "intel_psr.h"
>  #include "intel_snps_phy.h"
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 38f70f27ff1b..86d022e195c7 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -16,11 +16,13 @@
>  #include "intel_display_types.h"
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
> +#include "intel_frontbuffer.h"
>  #include "intel_psr.h"
>  #include "intel_sprite.h"
>  #include "skl_scaler.h"
>  #include "skl_universal_plane.h"
>  #include "skl_watermark.h"
> +
>  #ifdef I915
>  #include "pxp/intel_pxp.h"
>  #else

Re: [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Rodrigo Vivi]

On Mon, Mar 06, 2023 at 03:23:08PM +0000, Souza, Jose wrote:
> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > As a fallback if we decide not to merge the frontbuffer tracking, allow
> > i915 to keep its own implementation, and do the right thing in Xe.
> > 
> > The frontbuffer tracking for Xe is still done per-fb, while i915 can
> > keep doing the weird intel_frontbuffer + i915_active thing without
> > blocking Xe.
> 
> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> Without frontbuffer tracker those two features will break in some cases.

I'm afraid we cannot have this solution then. We will need FBC and PSR.
Should we then add a new IOCTL where userspace can request the PSR/FBC,
and then commit to always use the drity_fb calls on any frontbuffer update?

> 
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
> >  drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
> >  drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
> >  drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
> >  .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
> >  .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
> >  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
> >  .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
> >  8 files changed, 45 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index f2918bb07107..a4a57aa24422 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -133,7 +133,11 @@ struct intel_fb_view {
> >  
> >  struct intel_framebuffer {
> >  	struct drm_framebuffer base;
> > +#ifdef I915
> >  	struct intel_frontbuffer *frontbuffer;
> > +#else
> > +	atomic_t bits;
> > +#endif
> >  
> >  	/* Params to remap the FB pages and program the plane registers in each view. */
> >  	struct intel_fb_view normal_view;
> > @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
> >  #endif
> >  }
> >  
> > +#ifdef I915
> >  static inline struct intel_frontbuffer *
> >  to_intel_frontbuffer(struct drm_framebuffer *fb)
> >  {
> >  	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
> >  }
> > +#else
> > +static inline struct intel_framebuffer *
> > +to_intel_frontbuffer(struct drm_framebuffer *fb)
> > +{
> > +	return fb ? to_intel_framebuffer(fb) : NULL;
> > +}
> > +#endif
> >  
> >  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> > index 5b9e44443814..3503d112387d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> > @@ -9,6 +9,7 @@
> >  #include "intel_de.h"
> >  #include "intel_display_types.h"
> >  #include "intel_drrs.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_panel.h"
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 8c357a4098f6..e67c71f9b29d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >  #ifdef I915
> >  	if (intel_fb_uses_dpt(fb))
> >  		intel_dpt_destroy(intel_fb->dpt_vm);
> > +
> > +	intel_frontbuffer_put(intel_fb->frontbuffer);
> >  #else
> >  	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
> >  		struct xe_bo *bo = intel_fb_obj(fb);
> > @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >  	}
> >  #endif
> >  
> > -	intel_frontbuffer_put(intel_fb->frontbuffer);
> > -
> >  	kfree(intel_fb);
> >  }
> >  
> > @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >  		obj->flags |= XE_BO_SCANOUT_BIT;
> >  	}
> >  	ttm_bo_unreserve(&obj->ttm);
> > -#endif
> >  
> >  	atomic_set(&intel_fb->bits, 0);
> > +#endif
> >  
> >  	if (!drm_any_plane_has_format(&dev_priv->drm,
> >  				      mode_cmd->pixel_format,
> > @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >  	return 0;
> >  
> >  err:
> > +#ifdef I915
> >  	intel_frontbuffer_put(intel_fb->frontbuffer);
> > +#endif
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 75d8029185f0..2682b26b511f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -82,7 +82,7 @@ struct intel_fbdev {
> >  
> >  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
> >  {
> > -	return ifbdev->fb->frontbuffer;
> > +	return to_intel_frontbuffer(&ifbdev->fb->base);
> >  }
> >  
> >  static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > index 17a7aa8b28c2..64fe73d2ac4d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
> >  	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
> >  }
> >  
> > +#ifdef I915
> > +#define intel_front_obj(front) ((front)->obj)
> > +#else
> > +#define intel_front_obj(front) (front)
> > +#endif
> > +
> >  void __intel_fb_invalidate(struct intel_frontbuffer *front,
> >  			   enum fb_op_origin origin,
> >  			   unsigned int frontbuffer_bits)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> > +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
> >  
> >  	if (origin == ORIGIN_CS) {
> >  		spin_lock(&i915->display.fb_tracking.lock);
> > @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
> >  		      enum fb_op_origin origin,
> >  		      unsigned int frontbuffer_bits)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> > +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
> >  
> >  	if (origin == ORIGIN_CS) {
> >  		spin_lock(&i915->display.fb_tracking.lock);
> > @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
> >  		frontbuffer_flush(i915, frontbuffer_bits, origin);
> >  }
> >  
> > +#ifdef I915
> >  static int frontbuffer_active(struct i915_active *ref)
> >  {
> >  	struct intel_frontbuffer *front =
> > @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
> >  		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
> >  }
> >  
> > +#endif
> > +
> >  /**
> >   * intel_frontbuffer_track - update frontbuffer tracking
> >   * @old: current buffer for the frontbuffer slots
> > @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
> >  	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
> >  
> >  	if (old) {
> > -		drm_WARN_ON(old->obj->base.dev,
> > +		drm_WARN_ON(intel_front_obj(old)->base.dev,
> >  			    !(atomic_read(&old->bits) & frontbuffer_bits));
> >  		atomic_andnot(frontbuffer_bits, &old->bits);
> >  	}
> >  
> >  	if (new) {
> > -		drm_WARN_ON(new->obj->base.dev,
> > +		drm_WARN_ON(intel_front_obj(new)->base.dev,
> >  			    atomic_read(&new->bits) & frontbuffer_bits);
> >  		atomic_or(frontbuffer_bits, &new->bits);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > index 3c474ed937fb..a79e5ca419ec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > @@ -28,8 +28,6 @@
> >  #include <linux/bits.h>
> >  #include <linux/kref.h>
> >  
> > -#include "gem/i915_gem_object_types.h"
> > -#include "i915_active_types.h"
> >  
> >  struct drm_i915_private;
> >  
> > @@ -41,6 +39,10 @@ enum fb_op_origin {
> >  	ORIGIN_CURSOR_UPDATE,
> >  };
> >  
> > +#ifdef I915
> > +#include "gem/i915_gem_object_types.h"
> > +#include "i915_active_types.h"
> > +
> >  struct intel_frontbuffer {
> >  	struct kref ref;
> >  	atomic_t bits;
> > @@ -48,6 +50,9 @@ struct intel_frontbuffer {
> >  	struct drm_i915_gem_object *obj;
> >  	struct rcu_head rcu;
> >  };
> > +#else
> > +#define intel_frontbuffer intel_framebuffer
> > +#endif
> >  
> >  /*
> >   * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> > @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
> >  void intel_frontbuffer_flip(struct drm_i915_private *i915,
> >  			    unsigned frontbuffer_bits);
> >  
> > +#ifdef I915
> >  void intel_frontbuffer_put(struct intel_frontbuffer *front);
> >  
> >  static inline struct intel_frontbuffer *
> > @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
> >  struct intel_frontbuffer *
> >  intel_frontbuffer_get(struct drm_i915_gem_object *obj);
> >  
> > +#endif
> > +
> >  void __intel_fb_invalidate(struct intel_frontbuffer *front,
> >  			   enum fb_op_origin origin,
> >  			   unsigned int frontbuffer_bits);
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9820e5fdd087..bc998b526d88 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -33,6 +33,7 @@
> >  #include "intel_de.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_aux.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_hdmi.h"
> >  #include "intel_psr.h"
> >  #include "intel_snps_phy.h"
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 38f70f27ff1b..86d022e195c7 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -16,11 +16,13 @@
> >  #include "intel_display_types.h"
> >  #include "intel_fb.h"
> >  #include "intel_fbc.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_psr.h"
> >  #include "intel_sprite.h"
> >  #include "skl_scaler.h"
> >  #include "skl_universal_plane.h"
> >  #include "skl_watermark.h"
> > +
> >  #ifdef I915
> >  #include "pxp/intel_pxp.h"
> >  #else
> 

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Rodrigo Vivi]

On Mon, Mar 06, 2023 at 03:23:08PM +0000, Souza, Jose wrote:
> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > As a fallback if we decide not to merge the frontbuffer tracking, allow
> > i915 to keep its own implementation, and do the right thing in Xe.
> > 
> > The frontbuffer tracking for Xe is still done per-fb, while i915 can
> > keep doing the weird intel_frontbuffer + i915_active thing without
> > blocking Xe.
> 
> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> Without frontbuffer tracker those two features will break in some cases.

I'm afraid we cannot have this solution then. We will need FBC and PSR.
Should we then add a new IOCTL where userspace can request the PSR/FBC,
and then commit to always use the drity_fb calls on any frontbuffer update?

> 
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
> >  drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
> >  drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
> >  drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
> >  .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
> >  .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
> >  drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
> >  .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
> >  8 files changed, 45 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index f2918bb07107..a4a57aa24422 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -133,7 +133,11 @@ struct intel_fb_view {
> >  
> >  struct intel_framebuffer {
> >  	struct drm_framebuffer base;
> > +#ifdef I915
> >  	struct intel_frontbuffer *frontbuffer;
> > +#else
> > +	atomic_t bits;
> > +#endif
> >  
> >  	/* Params to remap the FB pages and program the plane registers in each view. */
> >  	struct intel_fb_view normal_view;
> > @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
> >  #endif
> >  }
> >  
> > +#ifdef I915
> >  static inline struct intel_frontbuffer *
> >  to_intel_frontbuffer(struct drm_framebuffer *fb)
> >  {
> >  	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
> >  }
> > +#else
> > +static inline struct intel_framebuffer *
> > +to_intel_frontbuffer(struct drm_framebuffer *fb)
> > +{
> > +	return fb ? to_intel_framebuffer(fb) : NULL;
> > +}
> > +#endif
> >  
> >  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> > index 5b9e44443814..3503d112387d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> > @@ -9,6 +9,7 @@
> >  #include "intel_de.h"
> >  #include "intel_display_types.h"
> >  #include "intel_drrs.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_panel.h"
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 8c357a4098f6..e67c71f9b29d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >  #ifdef I915
> >  	if (intel_fb_uses_dpt(fb))
> >  		intel_dpt_destroy(intel_fb->dpt_vm);
> > +
> > +	intel_frontbuffer_put(intel_fb->frontbuffer);
> >  #else
> >  	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
> >  		struct xe_bo *bo = intel_fb_obj(fb);
> > @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >  	}
> >  #endif
> >  
> > -	intel_frontbuffer_put(intel_fb->frontbuffer);
> > -
> >  	kfree(intel_fb);
> >  }
> >  
> > @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >  		obj->flags |= XE_BO_SCANOUT_BIT;
> >  	}
> >  	ttm_bo_unreserve(&obj->ttm);
> > -#endif
> >  
> >  	atomic_set(&intel_fb->bits, 0);
> > +#endif
> >  
> >  	if (!drm_any_plane_has_format(&dev_priv->drm,
> >  				      mode_cmd->pixel_format,
> > @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >  	return 0;
> >  
> >  err:
> > +#ifdef I915
> >  	intel_frontbuffer_put(intel_fb->frontbuffer);
> > +#endif
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 75d8029185f0..2682b26b511f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -82,7 +82,7 @@ struct intel_fbdev {
> >  
> >  static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
> >  {
> > -	return ifbdev->fb->frontbuffer;
> > +	return to_intel_frontbuffer(&ifbdev->fb->base);
> >  }
> >  
> >  static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > index 17a7aa8b28c2..64fe73d2ac4d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
> >  	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
> >  }
> >  
> > +#ifdef I915
> > +#define intel_front_obj(front) ((front)->obj)
> > +#else
> > +#define intel_front_obj(front) (front)
> > +#endif
> > +
> >  void __intel_fb_invalidate(struct intel_frontbuffer *front,
> >  			   enum fb_op_origin origin,
> >  			   unsigned int frontbuffer_bits)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> > +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
> >  
> >  	if (origin == ORIGIN_CS) {
> >  		spin_lock(&i915->display.fb_tracking.lock);
> > @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
> >  		      enum fb_op_origin origin,
> >  		      unsigned int frontbuffer_bits)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
> > +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
> >  
> >  	if (origin == ORIGIN_CS) {
> >  		spin_lock(&i915->display.fb_tracking.lock);
> > @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
> >  		frontbuffer_flush(i915, frontbuffer_bits, origin);
> >  }
> >  
> > +#ifdef I915
> >  static int frontbuffer_active(struct i915_active *ref)
> >  {
> >  	struct intel_frontbuffer *front =
> > @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
> >  		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
> >  }
> >  
> > +#endif
> > +
> >  /**
> >   * intel_frontbuffer_track - update frontbuffer tracking
> >   * @old: current buffer for the frontbuffer slots
> > @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
> >  	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
> >  
> >  	if (old) {
> > -		drm_WARN_ON(old->obj->base.dev,
> > +		drm_WARN_ON(intel_front_obj(old)->base.dev,
> >  			    !(atomic_read(&old->bits) & frontbuffer_bits));
> >  		atomic_andnot(frontbuffer_bits, &old->bits);
> >  	}
> >  
> >  	if (new) {
> > -		drm_WARN_ON(new->obj->base.dev,
> > +		drm_WARN_ON(intel_front_obj(new)->base.dev,
> >  			    atomic_read(&new->bits) & frontbuffer_bits);
> >  		atomic_or(frontbuffer_bits, &new->bits);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > index 3c474ed937fb..a79e5ca419ec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> > @@ -28,8 +28,6 @@
> >  #include <linux/bits.h>
> >  #include <linux/kref.h>
> >  
> > -#include "gem/i915_gem_object_types.h"
> > -#include "i915_active_types.h"
> >  
> >  struct drm_i915_private;
> >  
> > @@ -41,6 +39,10 @@ enum fb_op_origin {
> >  	ORIGIN_CURSOR_UPDATE,
> >  };
> >  
> > +#ifdef I915
> > +#include "gem/i915_gem_object_types.h"
> > +#include "i915_active_types.h"
> > +
> >  struct intel_frontbuffer {
> >  	struct kref ref;
> >  	atomic_t bits;
> > @@ -48,6 +50,9 @@ struct intel_frontbuffer {
> >  	struct drm_i915_gem_object *obj;
> >  	struct rcu_head rcu;
> >  };
> > +#else
> > +#define intel_frontbuffer intel_framebuffer
> > +#endif
> >  
> >  /*
> >   * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
> > @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
> >  void intel_frontbuffer_flip(struct drm_i915_private *i915,
> >  			    unsigned frontbuffer_bits);
> >  
> > +#ifdef I915
> >  void intel_frontbuffer_put(struct intel_frontbuffer *front);
> >  
> >  static inline struct intel_frontbuffer *
> > @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
> >  struct intel_frontbuffer *
> >  intel_frontbuffer_get(struct drm_i915_gem_object *obj);
> >  
> > +#endif
> > +
> >  void __intel_fb_invalidate(struct intel_frontbuffer *front,
> >  			   enum fb_op_origin origin,
> >  			   unsigned int frontbuffer_bits);
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9820e5fdd087..bc998b526d88 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -33,6 +33,7 @@
> >  #include "intel_de.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_aux.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_hdmi.h"
> >  #include "intel_psr.h"
> >  #include "intel_snps_phy.h"
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 38f70f27ff1b..86d022e195c7 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -16,11 +16,13 @@
> >  #include "intel_display_types.h"
> >  #include "intel_fb.h"
> >  #include "intel_fbc.h"
> > +#include "intel_frontbuffer.h"
> >  #include "intel_psr.h"
> >  #include "intel_sprite.h"
> >  #include "skl_scaler.h"
> >  #include "skl_universal_plane.h"
> >  #include "skl_watermark.h"
> > +
> >  #ifdef I915
> >  #include "pxp/intel_pxp.h"
> >  #else
> 

[Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Patchwork]

== Series Details ==

Series: drm/xe/display: Do not use i915 frontbuffer tracking implementation
URL   : https://patchwork.freedesktop.org/series/114714/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/114714/revisions/1/mbox/ not applied
Applying: drm/xe/display: Do not use i915 frontbuffer tracking implementation
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/display/intel_display_types.h).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/xe/display: Do not use i915 frontbuffer tracking implementation
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".

Re: [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

Hey,

On 2023-03-06 16:23, Souza, Jose wrote:
> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>> i915 to keep its own implementation, and do the right thing in Xe.
>>
>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>> keep doing the weird intel_frontbuffer + i915_active thing without
>> blocking Xe.
> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> Without frontbuffer tracker those two features will break in some cases.

FBC and PSR work completely as expected. I don't remove frontbuffer 
tracking; I only remove the GEM parts.

Explicit invalidation using pageflip or CPU rendering + DirtyFB continue 
to work, as I validated on my laptop with FBC.

>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
>>   drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
>>   drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
>>   drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
>>   .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
>>   .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
>>   drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
>>   .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
>>   8 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index f2918bb07107..a4a57aa24422 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -133,7 +133,11 @@ struct intel_fb_view {
>>   
>>   struct intel_framebuffer {
>>   	struct drm_framebuffer base;
>> +#ifdef I915
>>   	struct intel_frontbuffer *frontbuffer;
>> +#else
>> +	atomic_t bits;
>> +#endif
>>   
>>   	/* Params to remap the FB pages and program the plane registers in each view. */
>>   	struct intel_fb_view normal_view;
>> @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
>>   #endif
>>   }
>>   
>> +#ifdef I915
>>   static inline struct intel_frontbuffer *
>>   to_intel_frontbuffer(struct drm_framebuffer *fb)
>>   {
>>   	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
>>   }
>> +#else
>> +static inline struct intel_framebuffer *
>> +to_intel_frontbuffer(struct drm_framebuffer *fb)
>> +{
>> +	return fb ? to_intel_framebuffer(fb) : NULL;
>> +}
>> +#endif
>>   
>>   #endif /*  __INTEL_DISPLAY_TYPES_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
>> index 5b9e44443814..3503d112387d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
>> @@ -9,6 +9,7 @@
>>   #include "intel_de.h"
>>   #include "intel_display_types.h"
>>   #include "intel_drrs.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_panel.h"
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
>> index 8c357a4098f6..e67c71f9b29d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
>> @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>   #ifdef I915
>>   	if (intel_fb_uses_dpt(fb))
>>   		intel_dpt_destroy(intel_fb->dpt_vm);
>> +
>> +	intel_frontbuffer_put(intel_fb->frontbuffer);
>>   #else
>>   	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
>>   		struct xe_bo *bo = intel_fb_obj(fb);
>> @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>   	}
>>   #endif
>>   
>> -	intel_frontbuffer_put(intel_fb->frontbuffer);
>> -
>>   	kfree(intel_fb);
>>   }
>>   
>> @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>   		obj->flags |= XE_BO_SCANOUT_BIT;
>>   	}
>>   	ttm_bo_unreserve(&obj->ttm);
>> -#endif
>>   
>>   	atomic_set(&intel_fb->bits, 0);
>> +#endif
>>   
>>   	if (!drm_any_plane_has_format(&dev_priv->drm,
>>   				      mode_cmd->pixel_format,
>> @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>   	return 0;
>>   
>>   err:
>> +#ifdef I915
>>   	intel_frontbuffer_put(intel_fb->frontbuffer);
>> +#endif
>>   	return ret;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index 75d8029185f0..2682b26b511f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -82,7 +82,7 @@ struct intel_fbdev {
>>   
>>   static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>>   {
>> -	return ifbdev->fb->frontbuffer;
>> +	return to_intel_frontbuffer(&ifbdev->fb->base);
>>   }
>>   
>>   static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> index 17a7aa8b28c2..64fe73d2ac4d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
>>   	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
>>   }
>>   
>> +#ifdef I915
>> +#define intel_front_obj(front) ((front)->obj)
>> +#else
>> +#define intel_front_obj(front) (front)
>> +#endif
>> +
>>   void __intel_fb_invalidate(struct intel_frontbuffer *front,
>>   			   enum fb_op_origin origin,
>>   			   unsigned int frontbuffer_bits)
>>   {
>> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>>   
>>   	if (origin == ORIGIN_CS) {
>>   		spin_lock(&i915->display.fb_tracking.lock);
>> @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>>   		      enum fb_op_origin origin,
>>   		      unsigned int frontbuffer_bits)
>>   {
>> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>>   
>>   	if (origin == ORIGIN_CS) {
>>   		spin_lock(&i915->display.fb_tracking.lock);
>> @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>>   		frontbuffer_flush(i915, frontbuffer_bits, origin);
>>   }
>>   
>> +#ifdef I915
>>   static int frontbuffer_active(struct i915_active *ref)
>>   {
>>   	struct intel_frontbuffer *front =
>> @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
>>   		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
>>   }
>>   
>> +#endif
>> +
>>   /**
>>    * intel_frontbuffer_track - update frontbuffer tracking
>>    * @old: current buffer for the frontbuffer slots
>> @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
>>   	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
>>   
>>   	if (old) {
>> -		drm_WARN_ON(old->obj->base.dev,
>> +		drm_WARN_ON(intel_front_obj(old)->base.dev,
>>   			    !(atomic_read(&old->bits) & frontbuffer_bits));
>>   		atomic_andnot(frontbuffer_bits, &old->bits);
>>   	}
>>   
>>   	if (new) {
>> -		drm_WARN_ON(new->obj->base.dev,
>> +		drm_WARN_ON(intel_front_obj(new)->base.dev,
>>   			    atomic_read(&new->bits) & frontbuffer_bits);
>>   		atomic_or(frontbuffer_bits, &new->bits);
>>   	}
>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> index 3c474ed937fb..a79e5ca419ec 100644
>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> @@ -28,8 +28,6 @@
>>   #include <linux/bits.h>
>>   #include <linux/kref.h>
>>   
>> -#include "gem/i915_gem_object_types.h"
>> -#include "i915_active_types.h"
>>   
>>   struct drm_i915_private;
>>   
>> @@ -41,6 +39,10 @@ enum fb_op_origin {
>>   	ORIGIN_CURSOR_UPDATE,
>>   };
>>   
>> +#ifdef I915
>> +#include "gem/i915_gem_object_types.h"
>> +#include "i915_active_types.h"
>> +
>>   struct intel_frontbuffer {
>>   	struct kref ref;
>>   	atomic_t bits;
>> @@ -48,6 +50,9 @@ struct intel_frontbuffer {
>>   	struct drm_i915_gem_object *obj;
>>   	struct rcu_head rcu;
>>   };
>> +#else
>> +#define intel_frontbuffer intel_framebuffer
>> +#endif
>>   
>>   /*
>>    * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
>> @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>>   void intel_frontbuffer_flip(struct drm_i915_private *i915,
>>   			    unsigned frontbuffer_bits);
>>   
>> +#ifdef I915
>>   void intel_frontbuffer_put(struct intel_frontbuffer *front);
>>   
>>   static inline struct intel_frontbuffer *
>> @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
>>   struct intel_frontbuffer *
>>   intel_frontbuffer_get(struct drm_i915_gem_object *obj);
>>   
>> +#endif
>> +
>>   void __intel_fb_invalidate(struct intel_frontbuffer *front,
>>   			   enum fb_op_origin origin,
>>   			   unsigned int frontbuffer_bits);
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9820e5fdd087..bc998b526d88 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -33,6 +33,7 @@
>>   #include "intel_de.h"
>>   #include "intel_display_types.h"
>>   #include "intel_dp_aux.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_hdmi.h"
>>   #include "intel_psr.h"
>>   #include "intel_snps_phy.h"
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 38f70f27ff1b..86d022e195c7 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -16,11 +16,13 @@
>>   #include "intel_display_types.h"
>>   #include "intel_fb.h"
>>   #include "intel_fbc.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_psr.h"
>>   #include "intel_sprite.h"
>>   #include "skl_scaler.h"
>>   #include "skl_universal_plane.h"
>>   #include "skl_watermark.h"
>> +
>>   #ifdef I915
>>   #include "pxp/intel_pxp.h"
>>   #else

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

Hey,

On 2023-03-06 16:23, Souza, Jose wrote:
> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>> i915 to keep its own implementation, and do the right thing in Xe.
>>
>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>> keep doing the weird intel_frontbuffer + i915_active thing without
>> blocking Xe.
> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> Without frontbuffer tracker those two features will break in some cases.

FBC and PSR work completely as expected. I don't remove frontbuffer 
tracking; I only remove the GEM parts.

Explicit invalidation using pageflip or CPU rendering + DirtyFB continue 
to work, as I validated on my laptop with FBC.

>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   .../gpu/drm/i915/display/intel_display_types.h  | 12 ++++++++++++
>>   drivers/gpu/drm/i915/display/intel_drrs.c       |  1 +
>>   drivers/gpu/drm/i915/display/intel_fb.c         |  8 +++++---
>>   drivers/gpu/drm/i915/display/intel_fbdev.c      |  2 +-
>>   .../gpu/drm/i915/display/intel_frontbuffer.c    | 17 +++++++++++++----
>>   .../gpu/drm/i915/display/intel_frontbuffer.h    | 12 ++++++++++--
>>   drivers/gpu/drm/i915/display/intel_psr.c        |  1 +
>>   .../gpu/drm/i915/display/skl_universal_plane.c  |  2 ++
>>   8 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index f2918bb07107..a4a57aa24422 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -133,7 +133,11 @@ struct intel_fb_view {
>>   
>>   struct intel_framebuffer {
>>   	struct drm_framebuffer base;
>> +#ifdef I915
>>   	struct intel_frontbuffer *frontbuffer;
>> +#else
>> +	atomic_t bits;
>> +#endif
>>   
>>   	/* Params to remap the FB pages and program the plane registers in each view. */
>>   	struct intel_fb_view normal_view;
>> @@ -2074,10 +2078,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *plane_
>>   #endif
>>   }
>>   
>> +#ifdef I915
>>   static inline struct intel_frontbuffer *
>>   to_intel_frontbuffer(struct drm_framebuffer *fb)
>>   {
>>   	return fb ? to_intel_framebuffer(fb)->frontbuffer : NULL;
>>   }
>> +#else
>> +static inline struct intel_framebuffer *
>> +to_intel_frontbuffer(struct drm_framebuffer *fb)
>> +{
>> +	return fb ? to_intel_framebuffer(fb) : NULL;
>> +}
>> +#endif
>>   
>>   #endif /*  __INTEL_DISPLAY_TYPES_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
>> index 5b9e44443814..3503d112387d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
>> @@ -9,6 +9,7 @@
>>   #include "intel_de.h"
>>   #include "intel_display_types.h"
>>   #include "intel_drrs.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_panel.h"
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
>> index 8c357a4098f6..e67c71f9b29d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
>> @@ -1846,6 +1846,8 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>   #ifdef I915
>>   	if (intel_fb_uses_dpt(fb))
>>   		intel_dpt_destroy(intel_fb->dpt_vm);
>> +
>> +	intel_frontbuffer_put(intel_fb->frontbuffer);
>>   #else
>>   	if (intel_fb_obj(fb)->flags & XE_BO_CREATE_PINNED_BIT) {
>>   		struct xe_bo *bo = intel_fb_obj(fb);
>> @@ -1857,8 +1859,6 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>   	}
>>   #endif
>>   
>> -	intel_frontbuffer_put(intel_fb->frontbuffer);
>> -
>>   	kfree(intel_fb);
>>   }
>>   
>> @@ -1966,9 +1966,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>   		obj->flags |= XE_BO_SCANOUT_BIT;
>>   	}
>>   	ttm_bo_unreserve(&obj->ttm);
>> -#endif
>>   
>>   	atomic_set(&intel_fb->bits, 0);
>> +#endif
>>   
>>   	if (!drm_any_plane_has_format(&dev_priv->drm,
>>   				      mode_cmd->pixel_format,
>> @@ -2085,7 +2085,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>   	return 0;
>>   
>>   err:
>> +#ifdef I915
>>   	intel_frontbuffer_put(intel_fb->frontbuffer);
>> +#endif
>>   	return ret;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index 75d8029185f0..2682b26b511f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -82,7 +82,7 @@ struct intel_fbdev {
>>   
>>   static struct intel_frontbuffer *to_frontbuffer(struct intel_fbdev *ifbdev)
>>   {
>> -	return ifbdev->fb->frontbuffer;
>> +	return to_intel_frontbuffer(&ifbdev->fb->base);
>>   }
>>   
>>   static void intel_fbdev_invalidate(struct intel_fbdev *ifbdev)
>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> index 17a7aa8b28c2..64fe73d2ac4d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>> @@ -163,11 +163,17 @@ void intel_frontbuffer_flip(struct drm_i915_private *i915,
>>   	frontbuffer_flush(i915, frontbuffer_bits, ORIGIN_FLIP);
>>   }
>>   
>> +#ifdef I915
>> +#define intel_front_obj(front) ((front)->obj)
>> +#else
>> +#define intel_front_obj(front) (front)
>> +#endif
>> +
>>   void __intel_fb_invalidate(struct intel_frontbuffer *front,
>>   			   enum fb_op_origin origin,
>>   			   unsigned int frontbuffer_bits)
>>   {
>> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>>   
>>   	if (origin == ORIGIN_CS) {
>>   		spin_lock(&i915->display.fb_tracking.lock);
>> @@ -188,7 +194,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>>   		      enum fb_op_origin origin,
>>   		      unsigned int frontbuffer_bits)
>>   {
>> -	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>> +	struct drm_i915_private *i915 = to_i915(intel_front_obj(front)->base.dev);
>>   
>>   	if (origin == ORIGIN_CS) {
>>   		spin_lock(&i915->display.fb_tracking.lock);
>> @@ -202,6 +208,7 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>>   		frontbuffer_flush(i915, frontbuffer_bits, origin);
>>   }
>>   
>> +#ifdef I915
>>   static int frontbuffer_active(struct i915_active *ref)
>>   {
>>   	struct intel_frontbuffer *front =
>> @@ -289,6 +296,8 @@ void intel_frontbuffer_put(struct intel_frontbuffer *front)
>>   		      &to_i915(front->obj->base.dev)->display.fb_tracking.lock);
>>   }
>>   
>> +#endif
>> +
>>   /**
>>    * intel_frontbuffer_track - update frontbuffer tracking
>>    * @old: current buffer for the frontbuffer slots
>> @@ -315,13 +324,13 @@ void intel_frontbuffer_track(struct intel_frontbuffer *old,
>>   	BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE);
>>   
>>   	if (old) {
>> -		drm_WARN_ON(old->obj->base.dev,
>> +		drm_WARN_ON(intel_front_obj(old)->base.dev,
>>   			    !(atomic_read(&old->bits) & frontbuffer_bits));
>>   		atomic_andnot(frontbuffer_bits, &old->bits);
>>   	}
>>   
>>   	if (new) {
>> -		drm_WARN_ON(new->obj->base.dev,
>> +		drm_WARN_ON(intel_front_obj(new)->base.dev,
>>   			    atomic_read(&new->bits) & frontbuffer_bits);
>>   		atomic_or(frontbuffer_bits, &new->bits);
>>   	}
>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> index 3c474ed937fb..a79e5ca419ec 100644
>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
>> @@ -28,8 +28,6 @@
>>   #include <linux/bits.h>
>>   #include <linux/kref.h>
>>   
>> -#include "gem/i915_gem_object_types.h"
>> -#include "i915_active_types.h"
>>   
>>   struct drm_i915_private;
>>   
>> @@ -41,6 +39,10 @@ enum fb_op_origin {
>>   	ORIGIN_CURSOR_UPDATE,
>>   };
>>   
>> +#ifdef I915
>> +#include "gem/i915_gem_object_types.h"
>> +#include "i915_active_types.h"
>> +
>>   struct intel_frontbuffer {
>>   	struct kref ref;
>>   	atomic_t bits;
>> @@ -48,6 +50,9 @@ struct intel_frontbuffer {
>>   	struct drm_i915_gem_object *obj;
>>   	struct rcu_head rcu;
>>   };
>> +#else
>> +#define intel_frontbuffer intel_framebuffer
>> +#endif
>>   
>>   /*
>>    * Frontbuffer tracking bits. Set in obj->frontbuffer_bits while a gem bo is
>> @@ -73,6 +78,7 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>>   void intel_frontbuffer_flip(struct drm_i915_private *i915,
>>   			    unsigned frontbuffer_bits);
>>   
>> +#ifdef I915
>>   void intel_frontbuffer_put(struct intel_frontbuffer *front);
>>   
>>   static inline struct intel_frontbuffer *
>> @@ -105,6 +111,8 @@ __intel_frontbuffer_get(const struct drm_i915_gem_object *obj)
>>   struct intel_frontbuffer *
>>   intel_frontbuffer_get(struct drm_i915_gem_object *obj);
>>   
>> +#endif
>> +
>>   void __intel_fb_invalidate(struct intel_frontbuffer *front,
>>   			   enum fb_op_origin origin,
>>   			   unsigned int frontbuffer_bits);
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9820e5fdd087..bc998b526d88 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -33,6 +33,7 @@
>>   #include "intel_de.h"
>>   #include "intel_display_types.h"
>>   #include "intel_dp_aux.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_hdmi.h"
>>   #include "intel_psr.h"
>>   #include "intel_snps_phy.h"
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 38f70f27ff1b..86d022e195c7 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -16,11 +16,13 @@
>>   #include "intel_display_types.h"
>>   #include "intel_fb.h"
>>   #include "intel_fbc.h"
>> +#include "intel_frontbuffer.h"
>>   #include "intel_psr.h"
>>   #include "intel_sprite.h"
>>   #include "skl_scaler.h"
>>   #include "skl_universal_plane.h"
>>   #include "skl_watermark.h"
>> +
>>   #ifdef I915
>>   #include "pxp/intel_pxp.h"
>>   #else

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> On 2023-03-06 16:23, Souza, Jose wrote:
> > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >> i915 to keep its own implementation, and do the right thing in Xe.
> >>
> >> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >> keep doing the weird intel_frontbuffer + i915_active thing without
> >> blocking Xe.
> > Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> > Without frontbuffer tracker those two features will break in some cases.
> 
> FBC and PSR work completely as expected. I don't remove frontbuffer 
> tracking; I only remove the GEM parts.
> 
> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue 
> to work, as I validated on my laptop with FBC.

Neither of which are relevant to the removal of the gem hooks.

Like I already said ~10 times in the last meeting, we need a proper
testcase. Here's a rough idea what it should do:

prepare a batch with
1. spinner
2. something that clobbers the fb

Then
1. grab reference crc
2. execbuffer
3. dirtyfb
4. wait long enough for fbc to recompress
5. terminate spinner
6. gem_sync
7. grab crc and compare with reference

No idea what the current status of PSR+CRC is, so not sure
whether we can actually test PSR or not.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> On 2023-03-06 16:23, Souza, Jose wrote:
> > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >> i915 to keep its own implementation, and do the right thing in Xe.
> >>
> >> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >> keep doing the weird intel_frontbuffer + i915_active thing without
> >> blocking Xe.
> > Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> > Without frontbuffer tracker those two features will break in some cases.
> 
> FBC and PSR work completely as expected. I don't remove frontbuffer 
> tracking; I only remove the GEM parts.
> 
> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue 
> to work, as I validated on my laptop with FBC.

Neither of which are relevant to the removal of the gem hooks.

Like I already said ~10 times in the last meeting, we need a proper
testcase. Here's a rough idea what it should do:

prepare a batch with
1. spinner
2. something that clobbers the fb

Then
1. grab reference crc
2. execbuffer
3. dirtyfb
4. wait long enough for fbc to recompress
5. terminate spinner
6. gem_sync
7. grab crc and compare with reference

No idea what the current status of PSR+CRC is, so not sure
whether we can actually test PSR or not.

-- 
Ville Syrjälä
Intel

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> On 2023-03-06 16:23, Souza, Jose wrote:
> > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >> i915 to keep its own implementation, and do the right thing in Xe.
> >>
> >> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >> keep doing the weird intel_frontbuffer + i915_active thing without
> >> blocking Xe.
> > Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> > Without frontbuffer tracker those two features will break in some cases.
> 
> FBC and PSR work completely as expected. I don't remove frontbuffer 
> tracking; I only remove the GEM parts.
> 
> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue 
> to work, as I validated on my laptop with FBC.

Neither of which are relevant to the removal of the gem hooks.

Like I already said ~10 times in the last meeting, we need a proper
testcase. Here's a rough idea what it should do:

prepare a batch with
1. spinner
2. something that clobbers the fb

Then
1. grab reference crc
2. execbuffer
3. dirtyfb
4. wait long enough for fbc to recompress
5. terminate spinner
6. gem_sync
7. grab crc and compare with reference

No idea what the current status of PSR+CRC is, so not sure
whether we can actually test PSR or not.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-06 21:58, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> On 2023-03-06 16:23, Souza, Jose wrote:
>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>
>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>> blocking Xe.
>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>> Without frontbuffer tracker those two features will break in some cases.
>> FBC and PSR work completely as expected. I don't remove frontbuffer
>> tracking; I only remove the GEM parts.
>>
>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>> to work, as I validated on my laptop with FBC.
> Neither of which are relevant to the removal of the gem hooks.
>
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
>
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
>
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
>
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.

This test doesn't make sense. DirtyFB should simply not return before 
execbuffer finishes.

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-06 21:58, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> On 2023-03-06 16:23, Souza, Jose wrote:
>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>
>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>> blocking Xe.
>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>> Without frontbuffer tracker those two features will break in some cases.
>> FBC and PSR work completely as expected. I don't remove frontbuffer
>> tracking; I only remove the GEM parts.
>>
>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>> to work, as I validated on my laptop with FBC.
> Neither of which are relevant to the removal of the gem hooks.
>
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
>
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
>
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
>
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.

This test doesn't make sense. DirtyFB should simply not return before 
execbuffer finishes.

~Maarten

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-06 21:58, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> On 2023-03-06 16:23, Souza, Jose wrote:
>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>
>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>> blocking Xe.
>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>> Without frontbuffer tracker those two features will break in some cases.
>> FBC and PSR work completely as expected. I don't remove frontbuffer
>> tracking; I only remove the GEM parts.
>>
>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>> to work, as I validated on my laptop with FBC.
> Neither of which are relevant to the removal of the gem hooks.
>
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
>
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
>
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
>
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.

This test doesn't make sense. DirtyFB should simply not return before 
execbuffer finishes.

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-06 21:58, Ville Syrjälä wrote:
> > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> On 2023-03-06 16:23, Souza, Jose wrote:
> >>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>
> >>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>> blocking Xe.
> >>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>> Without frontbuffer tracker those two features will break in some cases.
> >> FBC and PSR work completely as expected. I don't remove frontbuffer
> >> tracking; I only remove the GEM parts.
> >>
> >> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >> to work, as I validated on my laptop with FBC.
> > Neither of which are relevant to the removal of the gem hooks.
> >
> > Like I already said ~10 times in the last meeting, we need a proper
> > testcase. Here's a rough idea what it should do:
> >
> > prepare a batch with
> > 1. spinner
> > 2. something that clobbers the fb
> >
> > Then
> > 1. grab reference crc
> > 2. execbuffer
> > 3. dirtyfb
> > 4. wait long enough for fbc to recompress
> > 5. terminate spinner
> > 6. gem_sync
> > 7. grab crc and compare with reference
> >
> > No idea what the current status of PSR+CRC is, so not sure
> > whether we can actually test PSR or not.
> 
> This test doesn't make sense. DirtyFB should simply not return before 
> execbuffer finishes.

Of course it should. It's not a blocking ioctl, and can't
be because that will make X unusable.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-06 21:58, Ville Syrjälä wrote:
> > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> On 2023-03-06 16:23, Souza, Jose wrote:
> >>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>
> >>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>> blocking Xe.
> >>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>> Without frontbuffer tracker those two features will break in some cases.
> >> FBC and PSR work completely as expected. I don't remove frontbuffer
> >> tracking; I only remove the GEM parts.
> >>
> >> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >> to work, as I validated on my laptop with FBC.
> > Neither of which are relevant to the removal of the gem hooks.
> >
> > Like I already said ~10 times in the last meeting, we need a proper
> > testcase. Here's a rough idea what it should do:
> >
> > prepare a batch with
> > 1. spinner
> > 2. something that clobbers the fb
> >
> > Then
> > 1. grab reference crc
> > 2. execbuffer
> > 3. dirtyfb
> > 4. wait long enough for fbc to recompress
> > 5. terminate spinner
> > 6. gem_sync
> > 7. grab crc and compare with reference
> >
> > No idea what the current status of PSR+CRC is, so not sure
> > whether we can actually test PSR or not.
> 
> This test doesn't make sense. DirtyFB should simply not return before 
> execbuffer finishes.

Of course it should. It's not a blocking ioctl, and can't
be because that will make X unusable.

-- 
Ville Syrjälä
Intel

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-06 21:58, Ville Syrjälä wrote:
> > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> On 2023-03-06 16:23, Souza, Jose wrote:
> >>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>
> >>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>> blocking Xe.
> >>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>> Without frontbuffer tracker those two features will break in some cases.
> >> FBC and PSR work completely as expected. I don't remove frontbuffer
> >> tracking; I only remove the GEM parts.
> >>
> >> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >> to work, as I validated on my laptop with FBC.
> > Neither of which are relevant to the removal of the gem hooks.
> >
> > Like I already said ~10 times in the last meeting, we need a proper
> > testcase. Here's a rough idea what it should do:
> >
> > prepare a batch with
> > 1. spinner
> > 2. something that clobbers the fb
> >
> > Then
> > 1. grab reference crc
> > 2. execbuffer
> > 3. dirtyfb
> > 4. wait long enough for fbc to recompress
> > 5. terminate spinner
> > 6. gem_sync
> > 7. grab crc and compare with reference
> >
> > No idea what the current status of PSR+CRC is, so not sure
> > whether we can actually test PSR or not.
> 
> This test doesn't make sense. DirtyFB should simply not return before 
> execbuffer finishes.

Of course it should. It's not a blocking ioctl, and can't
be because that will make X unusable.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

Hey,


On 2023-03-08 14:36, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
>> On 2023-03-06 21:58, Ville Syrjälä wrote:
>>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>>>
>>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>>>> blocking Xe.
>>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>>>> Without frontbuffer tracker those two features will break in some cases.
>>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>>> tracking; I only remove the GEM parts.
>>>>
>>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>>>> to work, as I validated on my laptop with FBC.
>>> Neither of which are relevant to the removal of the gem hooks.
>>>
>>> Like I already said ~10 times in the last meeting, we need a proper
>>> testcase. Here's a rough idea what it should do:
>>>
>>> prepare a batch with
>>> 1. spinner
>>> 2. something that clobbers the fb
>>>
>>> Then
>>> 1. grab reference crc
>>> 2. execbuffer
>>> 3. dirtyfb
>>> 4. wait long enough for fbc to recompress
>>> 5. terminate spinner
>>> 6. gem_sync
>>> 7. grab crc and compare with reference
>>>
>>> No idea what the current status of PSR+CRC is, so not sure
>>> whether we can actually test PSR or not.
>> This test doesn't make sense. DirtyFB should simply not return before
>> execbuffer finishes.
> Of course it should. It's not a blocking ioctl, and can't
> be because that will make X unusable.

Except it actually is.

DirtyFB blocks in its default implementation, and waits for the next vblank.

drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
plane update.

Considering every driver except i915 uses it, it works just fine. :)

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

Hey,


On 2023-03-08 14:36, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
>> On 2023-03-06 21:58, Ville Syrjälä wrote:
>>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>>>
>>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>>>> blocking Xe.
>>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>>>> Without frontbuffer tracker those two features will break in some cases.
>>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>>> tracking; I only remove the GEM parts.
>>>>
>>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>>>> to work, as I validated on my laptop with FBC.
>>> Neither of which are relevant to the removal of the gem hooks.
>>>
>>> Like I already said ~10 times in the last meeting, we need a proper
>>> testcase. Here's a rough idea what it should do:
>>>
>>> prepare a batch with
>>> 1. spinner
>>> 2. something that clobbers the fb
>>>
>>> Then
>>> 1. grab reference crc
>>> 2. execbuffer
>>> 3. dirtyfb
>>> 4. wait long enough for fbc to recompress
>>> 5. terminate spinner
>>> 6. gem_sync
>>> 7. grab crc and compare with reference
>>>
>>> No idea what the current status of PSR+CRC is, so not sure
>>> whether we can actually test PSR or not.
>> This test doesn't make sense. DirtyFB should simply not return before
>> execbuffer finishes.
> Of course it should. It's not a blocking ioctl, and can't
> be because that will make X unusable.

Except it actually is.

DirtyFB blocks in its default implementation, and waits for the next vblank.

drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
plane update.

Considering every driver except i915 uses it, it works just fine. :)

~Maarten

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

Hey,


On 2023-03-08 14:36, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
>> On 2023-03-06 21:58, Ville Syrjälä wrote:
>>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
>>>>>> i915 to keep its own implementation, and do the right thing in Xe.
>>>>>>
>>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
>>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
>>>>>> blocking Xe.
>>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
>>>>> Without frontbuffer tracker those two features will break in some cases.
>>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>>> tracking; I only remove the GEM parts.
>>>>
>>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
>>>> to work, as I validated on my laptop with FBC.
>>> Neither of which are relevant to the removal of the gem hooks.
>>>
>>> Like I already said ~10 times in the last meeting, we need a proper
>>> testcase. Here's a rough idea what it should do:
>>>
>>> prepare a batch with
>>> 1. spinner
>>> 2. something that clobbers the fb
>>>
>>> Then
>>> 1. grab reference crc
>>> 2. execbuffer
>>> 3. dirtyfb
>>> 4. wait long enough for fbc to recompress
>>> 5. terminate spinner
>>> 6. gem_sync
>>> 7. grab crc and compare with reference
>>>
>>> No idea what the current status of PSR+CRC is, so not sure
>>> whether we can actually test PSR or not.
>> This test doesn't make sense. DirtyFB should simply not return before
>> execbuffer finishes.
> Of course it should. It's not a blocking ioctl, and can't
> be because that will make X unusable.

Except it actually is.

DirtyFB blocks in its default implementation, and waits for the next vblank.

drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
plane update.

Considering every driver except i915 uses it, it works just fine. :)

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> 
> On 2023-03-08 14:36, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> >> On 2023-03-06 21:58, Ville Syrjälä wrote:
> >>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>>> Hey,
> >>>>
> >>>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>>>
> >>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>>>> blocking Xe.
> >>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>>>> Without frontbuffer tracker those two features will break in some cases.
> >>>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>>> tracking; I only remove the GEM parts.
> >>>>
> >>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >>>> to work, as I validated on my laptop with FBC.
> >>> Neither of which are relevant to the removal of the gem hooks.
> >>>
> >>> Like I already said ~10 times in the last meeting, we need a proper
> >>> testcase. Here's a rough idea what it should do:
> >>>
> >>> prepare a batch with
> >>> 1. spinner
> >>> 2. something that clobbers the fb
> >>>
> >>> Then
> >>> 1. grab reference crc
> >>> 2. execbuffer
> >>> 3. dirtyfb
> >>> 4. wait long enough for fbc to recompress
> >>> 5. terminate spinner
> >>> 6. gem_sync
> >>> 7. grab crc and compare with reference
> >>>
> >>> No idea what the current status of PSR+CRC is, so not sure
> >>> whether we can actually test PSR or not.
> >> This test doesn't make sense. DirtyFB should simply not return before
> >> execbuffer finishes.
> > Of course it should. It's not a blocking ioctl, and can't
> > be because that will make X unusable.
> 
> Except it actually is.
> 
> DirtyFB blocks in its default implementation, and waits for the next vblank.
> 
> drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
> plane update.
> 
> Considering every driver except i915 uses it, it works just fine. :)

No it doesn't. We tries to use it, but that was an utter failure.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> 
> On 2023-03-08 14:36, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> >> On 2023-03-06 21:58, Ville Syrjälä wrote:
> >>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>>> Hey,
> >>>>
> >>>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>>>
> >>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>>>> blocking Xe.
> >>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>>>> Without frontbuffer tracker those two features will break in some cases.
> >>>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>>> tracking; I only remove the GEM parts.
> >>>>
> >>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >>>> to work, as I validated on my laptop with FBC.
> >>> Neither of which are relevant to the removal of the gem hooks.
> >>>
> >>> Like I already said ~10 times in the last meeting, we need a proper
> >>> testcase. Here's a rough idea what it should do:
> >>>
> >>> prepare a batch with
> >>> 1. spinner
> >>> 2. something that clobbers the fb
> >>>
> >>> Then
> >>> 1. grab reference crc
> >>> 2. execbuffer
> >>> 3. dirtyfb
> >>> 4. wait long enough for fbc to recompress
> >>> 5. terminate spinner
> >>> 6. gem_sync
> >>> 7. grab crc and compare with reference
> >>>
> >>> No idea what the current status of PSR+CRC is, so not sure
> >>> whether we can actually test PSR or not.
> >> This test doesn't make sense. DirtyFB should simply not return before
> >> execbuffer finishes.
> > Of course it should. It's not a blocking ioctl, and can't
> > be because that will make X unusable.
> 
> Except it actually is.
> 
> DirtyFB blocks in its default implementation, and waits for the next vblank.
> 
> drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
> plane update.
> 
> Considering every driver except i915 uses it, it works just fine. :)

No it doesn't. We tries to use it, but that was an utter failure.

-- 
Ville Syrjälä
Intel

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> 
> On 2023-03-08 14:36, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote:
> >> On 2023-03-06 21:58, Ville Syrjälä wrote:
> >>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>>> Hey,
> >>>>
> >>>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>>> As a fallback if we decide not to merge the frontbuffer tracking, allow
> >>>>>> i915 to keep its own implementation, and do the right thing in Xe.
> >>>>>>
> >>>>>> The frontbuffer tracking for Xe is still done per-fb, while i915 can
> >>>>>> keep doing the weird intel_frontbuffer + i915_active thing without
> >>>>>> blocking Xe.
> >>>>> Please also disable PSR and FBC with this or at least add a way for users to disable those features.
> >>>>> Without frontbuffer tracker those two features will break in some cases.
> >>>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>>> tracking; I only remove the GEM parts.
> >>>>
> >>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue
> >>>> to work, as I validated on my laptop with FBC.
> >>> Neither of which are relevant to the removal of the gem hooks.
> >>>
> >>> Like I already said ~10 times in the last meeting, we need a proper
> >>> testcase. Here's a rough idea what it should do:
> >>>
> >>> prepare a batch with
> >>> 1. spinner
> >>> 2. something that clobbers the fb
> >>>
> >>> Then
> >>> 1. grab reference crc
> >>> 2. execbuffer
> >>> 3. dirtyfb
> >>> 4. wait long enough for fbc to recompress
> >>> 5. terminate spinner
> >>> 6. gem_sync
> >>> 7. grab crc and compare with reference
> >>>
> >>> No idea what the current status of PSR+CRC is, so not sure
> >>> whether we can actually test PSR or not.
> >> This test doesn't make sense. DirtyFB should simply not return before
> >> execbuffer finishes.
> > Of course it should. It's not a blocking ioctl, and can't
> > be because that will make X unusable.
> 
> Except it actually is.
> 
> DirtyFB blocks in its default implementation, and waits for the next vblank.
> 
> drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous 
> plane update.
> 
> Considering every driver except i915 uses it, it works just fine. :)

No it doesn't. We tries to use it, but that was an utter failure.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > On 2023-03-06 16:23, Souza, Jose wrote:
> > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > As a fallback if we decide not to merge the frontbuffer
> > > > tracking, allow
> > > > i915 to keep its own implementation, and do the right thing in
> > > > Xe.
> > > > 
> > > > The frontbuffer tracking for Xe is still done per-fb, while
> > > > i915 can
> > > > keep doing the weird intel_frontbuffer + i915_active thing
> > > > without
> > > > blocking Xe.
> > > Please also disable PSR and FBC with this or at least add a way
> > > for users to disable those features.
> > > Without frontbuffer tracker those two features will break in some
> > > cases.
> > 
> > FBC and PSR work completely as expected. I don't remove frontbuffer
> > tracking; I only remove the GEM parts.
> > 
> > Explicit invalidation using pageflip or CPU rendering + DirtyFB
> > continue 
> > to work, as I validated on my laptop with FBC.
> 
> Neither of which are relevant to the removal of the gem hooks.
> 
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
> 
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
> 
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
> 
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.
> 

CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
capture is requested.

Are we supposed to support frontbuffer rendering using GPU?

BR,

Jouni Högander

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > On 2023-03-06 16:23, Souza, Jose wrote:
> > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > As a fallback if we decide not to merge the frontbuffer
> > > > tracking, allow
> > > > i915 to keep its own implementation, and do the right thing in
> > > > Xe.
> > > > 
> > > > The frontbuffer tracking for Xe is still done per-fb, while
> > > > i915 can
> > > > keep doing the weird intel_frontbuffer + i915_active thing
> > > > without
> > > > blocking Xe.
> > > Please also disable PSR and FBC with this or at least add a way
> > > for users to disable those features.
> > > Without frontbuffer tracker those two features will break in some
> > > cases.
> > 
> > FBC and PSR work completely as expected. I don't remove frontbuffer
> > tracking; I only remove the GEM parts.
> > 
> > Explicit invalidation using pageflip or CPU rendering + DirtyFB
> > continue 
> > to work, as I validated on my laptop with FBC.
> 
> Neither of which are relevant to the removal of the gem hooks.
> 
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
> 
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
> 
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
> 
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.
> 

CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
capture is requested.

Are we supposed to support frontbuffer rendering using GPU?

BR,

Jouni Högander

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> > Hey,
> > 
> > On 2023-03-06 16:23, Souza, Jose wrote:
> > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > As a fallback if we decide not to merge the frontbuffer
> > > > tracking, allow
> > > > i915 to keep its own implementation, and do the right thing in
> > > > Xe.
> > > > 
> > > > The frontbuffer tracking for Xe is still done per-fb, while
> > > > i915 can
> > > > keep doing the weird intel_frontbuffer + i915_active thing
> > > > without
> > > > blocking Xe.
> > > Please also disable PSR and FBC with this or at least add a way
> > > for users to disable those features.
> > > Without frontbuffer tracker those two features will break in some
> > > cases.
> > 
> > FBC and PSR work completely as expected. I don't remove frontbuffer
> > tracking; I only remove the GEM parts.
> > 
> > Explicit invalidation using pageflip or CPU rendering + DirtyFB
> > continue 
> > to work, as I validated on my laptop with FBC.
> 
> Neither of which are relevant to the removal of the gem hooks.
> 
> Like I already said ~10 times in the last meeting, we need a proper
> testcase. Here's a rough idea what it should do:
> 
> prepare a batch with
> 1. spinner
> 2. something that clobbers the fb
> 
> Then
> 1. grab reference crc
> 2. execbuffer
> 3. dirtyfb
> 4. wait long enough for fbc to recompress
> 5. terminate spinner
> 6. gem_sync
> 7. grab crc and compare with reference
> 
> No idea what the current status of PSR+CRC is, so not sure
> whether we can actually test PSR or not.
> 

CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
capture is requested.

Are we supposed to support frontbuffer rendering using GPU?

BR,

Jouni Högander

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-09 12:04, Hogander, Jouni wrote:
> On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>> As a fallback if we decide not to merge the frontbuffer
>>>>> tracking, allow
>>>>> i915 to keep its own implementation, and do the right thing in
>>>>> Xe.
>>>>>
>>>>> The frontbuffer tracking for Xe is still done per-fb, while
>>>>> i915 can
>>>>> keep doing the weird intel_frontbuffer + i915_active thing
>>>>> without
>>>>> blocking Xe.
>>>> Please also disable PSR and FBC with this or at least add a way
>>>> for users to disable those features.
>>>> Without frontbuffer tracker those two features will break in some
>>>> cases.
>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>> tracking; I only remove the GEM parts.
>>>
>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
>>> continue
>>> to work, as I validated on my laptop with FBC.
>> Neither of which are relevant to the removal of the gem hooks.
>>
>> Like I already said ~10 times in the last meeting, we need a proper
>> testcase. Here's a rough idea what it should do:
>>
>> prepare a batch with
>> 1. spinner
>> 2. something that clobbers the fb
>>
>> Then
>> 1. grab reference crc
>> 2. execbuffer
>> 3. dirtyfb
>> 4. wait long enough for fbc to recompress
>> 5. terminate spinner
>> 6. gem_sync
>> 7. grab crc and compare with reference
>>
>> No idea what the current status of PSR+CRC is, so not sure
>> whether we can actually test PSR or not.
>>
> CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> capture is requested.
>
> Are we supposed to support frontbuffer rendering using GPU?

No other driver does that. It's fine if DirtyFB hangs instead until the 
job it waits on completes.

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-09 12:04, Hogander, Jouni wrote:
> On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>> As a fallback if we decide not to merge the frontbuffer
>>>>> tracking, allow
>>>>> i915 to keep its own implementation, and do the right thing in
>>>>> Xe.
>>>>>
>>>>> The frontbuffer tracking for Xe is still done per-fb, while
>>>>> i915 can
>>>>> keep doing the weird intel_frontbuffer + i915_active thing
>>>>> without
>>>>> blocking Xe.
>>>> Please also disable PSR and FBC with this or at least add a way
>>>> for users to disable those features.
>>>> Without frontbuffer tracker those two features will break in some
>>>> cases.
>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>> tracking; I only remove the GEM parts.
>>>
>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
>>> continue
>>> to work, as I validated on my laptop with FBC.
>> Neither of which are relevant to the removal of the gem hooks.
>>
>> Like I already said ~10 times in the last meeting, we need a proper
>> testcase. Here's a rough idea what it should do:
>>
>> prepare a batch with
>> 1. spinner
>> 2. something that clobbers the fb
>>
>> Then
>> 1. grab reference crc
>> 2. execbuffer
>> 3. dirtyfb
>> 4. wait long enough for fbc to recompress
>> 5. terminate spinner
>> 6. gem_sync
>> 7. grab crc and compare with reference
>>
>> No idea what the current status of PSR+CRC is, so not sure
>> whether we can actually test PSR or not.
>>
> CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> capture is requested.
>
> Are we supposed to support frontbuffer rendering using GPU?

No other driver does that. It's fine if DirtyFB hangs instead until the 
job it waits on completes.

~Maarten

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Maarten Lankhorst]

On 2023-03-09 12:04, Hogander, Jouni wrote:
> On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> On 2023-03-06 16:23, Souza, Jose wrote:
>>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
>>>>> As a fallback if we decide not to merge the frontbuffer
>>>>> tracking, allow
>>>>> i915 to keep its own implementation, and do the right thing in
>>>>> Xe.
>>>>>
>>>>> The frontbuffer tracking for Xe is still done per-fb, while
>>>>> i915 can
>>>>> keep doing the weird intel_frontbuffer + i915_active thing
>>>>> without
>>>>> blocking Xe.
>>>> Please also disable PSR and FBC with this or at least add a way
>>>> for users to disable those features.
>>>> Without frontbuffer tracker those two features will break in some
>>>> cases.
>>> FBC and PSR work completely as expected. I don't remove frontbuffer
>>> tracking; I only remove the GEM parts.
>>>
>>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
>>> continue
>>> to work, as I validated on my laptop with FBC.
>> Neither of which are relevant to the removal of the gem hooks.
>>
>> Like I already said ~10 times in the last meeting, we need a proper
>> testcase. Here's a rough idea what it should do:
>>
>> prepare a batch with
>> 1. spinner
>> 2. something that clobbers the fb
>>
>> Then
>> 1. grab reference crc
>> 2. execbuffer
>> 3. dirtyfb
>> 4. wait long enough for fbc to recompress
>> 5. terminate spinner
>> 6. gem_sync
>> 7. grab crc and compare with reference
>>
>> No idea what the current status of PSR+CRC is, so not sure
>> whether we can actually test PSR or not.
>>
> CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> capture is requested.
>
> Are we supposed to support frontbuffer rendering using GPU?

No other driver does that. It's fine if DirtyFB hangs instead until the 
job it waits on completes.

~Maarten

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-09 12:04, Hogander, Jouni wrote:
> > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> >> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>> Hey,
> >>>
> >>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>> As a fallback if we decide not to merge the frontbuffer
> >>>>> tracking, allow
> >>>>> i915 to keep its own implementation, and do the right thing in
> >>>>> Xe.
> >>>>>
> >>>>> The frontbuffer tracking for Xe is still done per-fb, while
> >>>>> i915 can
> >>>>> keep doing the weird intel_frontbuffer + i915_active thing
> >>>>> without
> >>>>> blocking Xe.
> >>>> Please also disable PSR and FBC with this or at least add a way
> >>>> for users to disable those features.
> >>>> Without frontbuffer tracker those two features will break in some
> >>>> cases.
> >>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>> tracking; I only remove the GEM parts.
> >>>
> >>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
> >>> continue
> >>> to work, as I validated on my laptop with FBC.
> >> Neither of which are relevant to the removal of the gem hooks.
> >>
> >> Like I already said ~10 times in the last meeting, we need a proper
> >> testcase. Here's a rough idea what it should do:
> >>
> >> prepare a batch with
> >> 1. spinner
> >> 2. something that clobbers the fb
> >>
> >> Then
> >> 1. grab reference crc
> >> 2. execbuffer
> >> 3. dirtyfb
> >> 4. wait long enough for fbc to recompress
> >> 5. terminate spinner
> >> 6. gem_sync
> >> 7. grab crc and compare with reference
> >>
> >> No idea what the current status of PSR+CRC is, so not sure
> >> whether we can actually test PSR or not.
> >>
> > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> > capture is requested.
> >
> > Are we supposed to support frontbuffer rendering using GPU?
> 
> No other driver does that.

Every driver does that when you run X w/o a compositor. Assuming
there is an actual GPU in there.

> It's fine if DirtyFB hangs instead until the 
> job it waits on completes.

No one tried to make it just wait for the fence(s) w/o doing
a full blown atomic commit. It might work, but might also
still suck too much. I guess depends on how overloaded the GPU
is.

What we could do is do a frontbuffer invalidate on dirtyfb
invocation, and then once the fence(s) signal we do a frontbuffer
flush. That would most closely match the gem hook behaviour, except
the invalidate comes in a bit later. The alternative would be to
skip the invalidate, which should still guarantee correctness in
the end, just with possibly jankier interactivity.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-09 12:04, Hogander, Jouni wrote:
> > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> >> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>> Hey,
> >>>
> >>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>> As a fallback if we decide not to merge the frontbuffer
> >>>>> tracking, allow
> >>>>> i915 to keep its own implementation, and do the right thing in
> >>>>> Xe.
> >>>>>
> >>>>> The frontbuffer tracking for Xe is still done per-fb, while
> >>>>> i915 can
> >>>>> keep doing the weird intel_frontbuffer + i915_active thing
> >>>>> without
> >>>>> blocking Xe.
> >>>> Please also disable PSR and FBC with this or at least add a way
> >>>> for users to disable those features.
> >>>> Without frontbuffer tracker those two features will break in some
> >>>> cases.
> >>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>> tracking; I only remove the GEM parts.
> >>>
> >>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
> >>> continue
> >>> to work, as I validated on my laptop with FBC.
> >> Neither of which are relevant to the removal of the gem hooks.
> >>
> >> Like I already said ~10 times in the last meeting, we need a proper
> >> testcase. Here's a rough idea what it should do:
> >>
> >> prepare a batch with
> >> 1. spinner
> >> 2. something that clobbers the fb
> >>
> >> Then
> >> 1. grab reference crc
> >> 2. execbuffer
> >> 3. dirtyfb
> >> 4. wait long enough for fbc to recompress
> >> 5. terminate spinner
> >> 6. gem_sync
> >> 7. grab crc and compare with reference
> >>
> >> No idea what the current status of PSR+CRC is, so not sure
> >> whether we can actually test PSR or not.
> >>
> > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> > capture is requested.
> >
> > Are we supposed to support frontbuffer rendering using GPU?
> 
> No other driver does that.

Every driver does that when you run X w/o a compositor. Assuming
there is an actual GPU in there.

> It's fine if DirtyFB hangs instead until the 
> job it waits on completes.

No one tried to make it just wait for the fence(s) w/o doing
a full blown atomic commit. It might work, but might also
still suck too much. I guess depends on how overloaded the GPU
is.

What we could do is do a frontbuffer invalidate on dirtyfb
invocation, and then once the fence(s) signal we do a frontbuffer
flush. That would most closely match the gem hook behaviour, except
the invalidate comes in a bit later. The alternative would be to
skip the invalidate, which should still guarantee correctness in
the end, just with possibly jankier interactivity.

-- 
Ville Syrjälä
Intel

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Ville Syrjälä]

On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> 
> On 2023-03-09 12:04, Hogander, Jouni wrote:
> > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> >> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote:
> >>> Hey,
> >>>
> >>> On 2023-03-06 16:23, Souza, Jose wrote:
> >>>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> >>>>> As a fallback if we decide not to merge the frontbuffer
> >>>>> tracking, allow
> >>>>> i915 to keep its own implementation, and do the right thing in
> >>>>> Xe.
> >>>>>
> >>>>> The frontbuffer tracking for Xe is still done per-fb, while
> >>>>> i915 can
> >>>>> keep doing the weird intel_frontbuffer + i915_active thing
> >>>>> without
> >>>>> blocking Xe.
> >>>> Please also disable PSR and FBC with this or at least add a way
> >>>> for users to disable those features.
> >>>> Without frontbuffer tracker those two features will break in some
> >>>> cases.
> >>> FBC and PSR work completely as expected. I don't remove frontbuffer
> >>> tracking; I only remove the GEM parts.
> >>>
> >>> Explicit invalidation using pageflip or CPU rendering + DirtyFB
> >>> continue
> >>> to work, as I validated on my laptop with FBC.
> >> Neither of which are relevant to the removal of the gem hooks.
> >>
> >> Like I already said ~10 times in the last meeting, we need a proper
> >> testcase. Here's a rough idea what it should do:
> >>
> >> prepare a batch with
> >> 1. spinner
> >> 2. something that clobbers the fb
> >>
> >> Then
> >> 1. grab reference crc
> >> 2. execbuffer
> >> 3. dirtyfb
> >> 4. wait long enough for fbc to recompress
> >> 5. terminate spinner
> >> 6. gem_sync
> >> 7. grab crc and compare with reference
> >>
> >> No idea what the current status of PSR+CRC is, so not sure
> >> whether we can actually test PSR or not.
> >>
> > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC
> > capture is requested.
> >
> > Are we supposed to support frontbuffer rendering using GPU?
> 
> No other driver does that.

Every driver does that when you run X w/o a compositor. Assuming
there is an actual GPU in there.

> It's fine if DirtyFB hangs instead until the 
> job it waits on completes.

No one tried to make it just wait for the fence(s) w/o doing
a full blown atomic commit. It might work, but might also
still suck too much. I guess depends on how overloaded the GPU
is.

What we could do is do a frontbuffer invalidate on dirtyfb
invocation, and then once the fence(s) signal we do a frontbuffer
flush. That would most closely match the gem hook behaviour, except
the invalidate comes in a bit later. The alternative would be to
skip the invalidate, which should still guarantee correctness in
the end, just with possibly jankier interactivity.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Thu, 2023-03-09 at 14:34 +0200, Ville Syrjälä wrote:
> On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> > 
> > On 2023-03-09 12:04, Hogander, Jouni wrote:
> > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst
> > > > wrote:
> > > > > Hey,
> > > > > 
> > > > > On 2023-03-06 16:23, Souza, Jose wrote:
> > > > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > > > > As a fallback if we decide not to merge the frontbuffer
> > > > > > > tracking, allow
> > > > > > > i915 to keep its own implementation, and do the right
> > > > > > > thing in
> > > > > > > Xe.
> > > > > > > 
> > > > > > > The frontbuffer tracking for Xe is still done per-fb,
> > > > > > > while
> > > > > > > i915 can
> > > > > > > keep doing the weird intel_frontbuffer + i915_active
> > > > > > > thing
> > > > > > > without
> > > > > > > blocking Xe.
> > > > > > Please also disable PSR and FBC with this or at least add a
> > > > > > way
> > > > > > for users to disable those features.
> > > > > > Without frontbuffer tracker those two features will break
> > > > > > in some
> > > > > > cases.
> > > > > FBC and PSR work completely as expected. I don't remove
> > > > > frontbuffer
> > > > > tracking; I only remove the GEM parts.
> > > > > 
> > > > > Explicit invalidation using pageflip or CPU rendering +
> > > > > DirtyFB
> > > > > continue
> > > > > to work, as I validated on my laptop with FBC.
> > > > Neither of which are relevant to the removal of the gem hooks.
> > > > 
> > > > Like I already said ~10 times in the last meeting, we need a
> > > > proper
> > > > testcase. Here's a rough idea what it should do:
> > > > 
> > > > prepare a batch with
> > > > 1. spinner
> > > > 2. something that clobbers the fb
> > > > 
> > > > Then
> > > > 1. grab reference crc
> > > > 2. execbuffer
> > > > 3. dirtyfb
> > > > 4. wait long enough for fbc to recompress
> > > > 5. terminate spinner
> > > > 6. gem_sync
> > > > 7. grab crc and compare with reference
> > > > 
> > > > No idea what the current status of PSR+CRC is, so not sure
> > > > whether we can actually test PSR or not.
> > > > 
> > > CRC calculation doesn't work with PSR currently. PSR is disabled
> > > if CRC
> > > capture is requested.
> > > 
> > > Are we supposed to support frontbuffer rendering using GPU?
> > 
> > No other driver does that.
> 
> Every driver does that when you run X w/o a compositor. Assuming
> there is an actual GPU in there.
> 
> > It's fine if DirtyFB hangs instead until the 
> > job it waits on completes.
> 
> No one tried to make it just wait for the fence(s) w/o doing
> a full blown atomic commit. It might work, but might also
> still suck too much. I guess depends on how overloaded the GPU
> is.
> 
> What we could do is do a frontbuffer invalidate on dirtyfb
> invocation, and then once the fence(s) signal we do a frontbuffer
> flush. That would most closely match the gem hook behaviour, except
> the invalidate comes in a bit later. The alternative would be to
> skip the invalidate, which should still guarantee correctness in
> the end, just with possibly jankier interactivity.
> 

I have implemented this approach here:

https://patchwork.freedesktop.org/series/116620/

Review/comments are welcome.

BR,

Jouni Högander

Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Thu, 2023-03-09 at 14:34 +0200, Ville Syrjälä wrote:
> On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> > 
> > On 2023-03-09 12:04, Hogander, Jouni wrote:
> > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst
> > > > wrote:
> > > > > Hey,
> > > > > 
> > > > > On 2023-03-06 16:23, Souza, Jose wrote:
> > > > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > > > > As a fallback if we decide not to merge the frontbuffer
> > > > > > > tracking, allow
> > > > > > > i915 to keep its own implementation, and do the right
> > > > > > > thing in
> > > > > > > Xe.
> > > > > > > 
> > > > > > > The frontbuffer tracking for Xe is still done per-fb,
> > > > > > > while
> > > > > > > i915 can
> > > > > > > keep doing the weird intel_frontbuffer + i915_active
> > > > > > > thing
> > > > > > > without
> > > > > > > blocking Xe.
> > > > > > Please also disable PSR and FBC with this or at least add a
> > > > > > way
> > > > > > for users to disable those features.
> > > > > > Without frontbuffer tracker those two features will break
> > > > > > in some
> > > > > > cases.
> > > > > FBC and PSR work completely as expected. I don't remove
> > > > > frontbuffer
> > > > > tracking; I only remove the GEM parts.
> > > > > 
> > > > > Explicit invalidation using pageflip or CPU rendering +
> > > > > DirtyFB
> > > > > continue
> > > > > to work, as I validated on my laptop with FBC.
> > > > Neither of which are relevant to the removal of the gem hooks.
> > > > 
> > > > Like I already said ~10 times in the last meeting, we need a
> > > > proper
> > > > testcase. Here's a rough idea what it should do:
> > > > 
> > > > prepare a batch with
> > > > 1. spinner
> > > > 2. something that clobbers the fb
> > > > 
> > > > Then
> > > > 1. grab reference crc
> > > > 2. execbuffer
> > > > 3. dirtyfb
> > > > 4. wait long enough for fbc to recompress
> > > > 5. terminate spinner
> > > > 6. gem_sync
> > > > 7. grab crc and compare with reference
> > > > 
> > > > No idea what the current status of PSR+CRC is, so not sure
> > > > whether we can actually test PSR or not.
> > > > 
> > > CRC calculation doesn't work with PSR currently. PSR is disabled
> > > if CRC
> > > capture is requested.
> > > 
> > > Are we supposed to support frontbuffer rendering using GPU?
> > 
> > No other driver does that.
> 
> Every driver does that when you run X w/o a compositor. Assuming
> there is an actual GPU in there.
> 
> > It's fine if DirtyFB hangs instead until the 
> > job it waits on completes.
> 
> No one tried to make it just wait for the fence(s) w/o doing
> a full blown atomic commit. It might work, but might also
> still suck too much. I guess depends on how overloaded the GPU
> is.
> 
> What we could do is do a frontbuffer invalidate on dirtyfb
> invocation, and then once the fence(s) signal we do a frontbuffer
> flush. That would most closely match the gem hook behaviour, except
> the invalidate comes in a bit later. The alternative would be to
> skip the invalidate, which should still guarantee correctness in
> the end, just with possibly jankier interactivity.
> 

I have implemented this approach here:

https://patchwork.freedesktop.org/series/116620/

Review/comments are welcome.

BR,

Jouni Högander

Re: [Intel-xe] [Intel-gfx] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation

[Hogander, Jouni]

On Thu, 2023-03-09 at 14:34 +0200, Ville Syrjälä wrote:
> On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote:
> > 
> > On 2023-03-09 12:04, Hogander, Jouni wrote:
> > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst
> > > > wrote:
> > > > > Hey,
> > > > > 
> > > > > On 2023-03-06 16:23, Souza, Jose wrote:
> > > > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote:
> > > > > > > As a fallback if we decide not to merge the frontbuffer
> > > > > > > tracking, allow
> > > > > > > i915 to keep its own implementation, and do the right
> > > > > > > thing in
> > > > > > > Xe.
> > > > > > > 
> > > > > > > The frontbuffer tracking for Xe is still done per-fb,
> > > > > > > while
> > > > > > > i915 can
> > > > > > > keep doing the weird intel_frontbuffer + i915_active
> > > > > > > thing
> > > > > > > without
> > > > > > > blocking Xe.
> > > > > > Please also disable PSR and FBC with this or at least add a
> > > > > > way
> > > > > > for users to disable those features.
> > > > > > Without frontbuffer tracker those two features will break
> > > > > > in some
> > > > > > cases.
> > > > > FBC and PSR work completely as expected. I don't remove
> > > > > frontbuffer
> > > > > tracking; I only remove the GEM parts.
> > > > > 
> > > > > Explicit invalidation using pageflip or CPU rendering +
> > > > > DirtyFB
> > > > > continue
> > > > > to work, as I validated on my laptop with FBC.
> > > > Neither of which are relevant to the removal of the gem hooks.
> > > > 
> > > > Like I already said ~10 times in the last meeting, we need a
> > > > proper
> > > > testcase. Here's a rough idea what it should do:
> > > > 
> > > > prepare a batch with
> > > > 1. spinner
> > > > 2. something that clobbers the fb
> > > > 
> > > > Then
> > > > 1. grab reference crc
> > > > 2. execbuffer
> > > > 3. dirtyfb
> > > > 4. wait long enough for fbc to recompress
> > > > 5. terminate spinner
> > > > 6. gem_sync
> > > > 7. grab crc and compare with reference
> > > > 
> > > > No idea what the current status of PSR+CRC is, so not sure
> > > > whether we can actually test PSR or not.
> > > > 
> > > CRC calculation doesn't work with PSR currently. PSR is disabled
> > > if CRC
> > > capture is requested.
> > > 
> > > Are we supposed to support frontbuffer rendering using GPU?
> > 
> > No other driver does that.
> 
> Every driver does that when you run X w/o a compositor. Assuming
> there is an actual GPU in there.
> 
> > It's fine if DirtyFB hangs instead until the 
> > job it waits on completes.
> 
> No one tried to make it just wait for the fence(s) w/o doing
> a full blown atomic commit. It might work, but might also
> still suck too much. I guess depends on how overloaded the GPU
> is.
> 
> What we could do is do a frontbuffer invalidate on dirtyfb
> invocation, and then once the fence(s) signal we do a frontbuffer
> flush. That would most closely match the gem hook behaviour, except
> the invalidate comes in a bit later. The alternative would be to
> skip the invalidate, which should still guarantee correctness in
> the end, just with possibly jankier interactivity.
> 

I have implemented this approach here:

https://patchwork.freedesktop.org/series/116620/

Review/comments are welcome.

BR,

Jouni Högander