| Message ID | 20230306141638.196359-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe/display: Do not use i915 frontbuffer tracking implementation | expand |
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
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 >
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
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.
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
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.
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
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.
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
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
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.
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
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
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(-)