| Message ID | 20210825005840.170796-8-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Drop frontbuffer rendering support from Skylake and newer | expand |
On 8/25/21 3:58 AM, José Roberto de Souza wrote: > By now all the userspace applications should have migrated to atomic > or at least be calling DRM_IOCTL_MODE_DIRTYFB. > > With that we can kill frontbuffer rendering support in i915 for > modern platforms. > > So here converting legacy APIs into atomic commits so it can be > properly handled by driver i915. > > Several IGT tests will fail with this changes, because some tests > were stressing those frontbuffer rendering scenarios that no userspace > should be using by now, fixes to IGT should be sent soon. > > v2: > - return earlier to not set fb_tracking.busy/flip_bits > - added a warn on to make sure we are not setting the busy/flip_bits > > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++---- > drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++- > .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > 4 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c > index c7618fef01439..5aa996c3b7980 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > u32 src_w, u32 src_h, > struct drm_modeset_acquire_ctx *ctx) > { > + struct drm_i915_private *i915 = to_i915(_crtc->dev); > struct intel_plane *plane = to_intel_plane(_plane); > struct intel_crtc *crtc = to_intel_crtc(_crtc); > struct intel_plane_state *old_plane_state = > @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > * PSR2 selective fetch also requires the slow path as > * PSR2 plane and transcoder registers can only be updated during > * vblank. > - * > - * FIXME bigjoiner fastpath would be good > */ > if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) || > - crtc_state->update_pipe || crtc_state->bigjoiner || > - crtc_state->enable_psr2_sel_fetch) > + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915)) > goto slow; > > /* > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c > index e4b8602ec0cd2..3eb60785c9f29 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb.c > +++ b/drivers/gpu/drm/i915/display/intel_fb.c > @@ -3,6 +3,7 @@ > * Copyright © 2021 Intel Corporation > */ > > +#include <drm/drm_damage_helper.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_modeset_helper.h> > > @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > unsigned int num_clips) > { > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > > i915_gem_object_flush_if_display(obj); > - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips, > + num_clips); Hi, Even if the userspace informs us of the dirty (damage) region of the front buffer being used, GEN9 to GEN12 still uses the HW Tracking function for PSR and FBC. And if you look at the description of the intel_psr_flush() function, you can see that there are the following restrictions. "Since the hardware frontbuffer tracking has gaps we need to integrate with the software frontbuffer tracking." If this restriction is still valid from GEN9 to GEN12, even if the existing frontbuffer tracking function is not used, when intel_user_framebuffer_dirty() is called, in the case of PSR, psr_force_hw_tracking_exit() is called or intel_psr_exit() and schedule_work(psr.work) seems to be required. In the case of FBC, it seems that calls to FBC deactive / FBC activate should be added. If GEN9 to GEN12 do not have the above restrictions, please ignore this comment. G.G. > + > + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > return 0; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > index 0492446cd04ad..3860f87dac31c 100644 > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915, > void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > unsigned frontbuffer_bits) > { > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > + return; > + > spin_lock(&i915->fb_tracking.lock); > i915->fb_tracking.flip_bits |= frontbuffer_bits; > /* Remove stale busy bits due to the old buffer. */ > @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > unsigned frontbuffer_bits) > { > + if (!HAS_FRONTBUFFER_RENDERING(i915)) { > + drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits | > + i915->fb_tracking.busy_bits); > + return; > + } > + > spin_lock(&i915->fb_tracking.lock); > /* Mask any cancelled flips. */ > frontbuffer_bits &= i915->fb_tracking.flip_bits; > @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > void intel_frontbuffer_flip(struct drm_i915_private *i915, > unsigned frontbuffer_bits) > { > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > + return; > + > spin_lock(&i915->fb_tracking.lock); > /* Remove stale busy bits due to the old buffer. */ > i915->fb_tracking.busy_bits &= ~frontbuffer_bits; > @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front, > { > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > + return; > + > if (origin == ORIGIN_CS) { > spin_lock(&i915->fb_tracking.lock); > i915->fb_tracking.busy_bits |= frontbuffer_bits; > @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front, > { > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > + return; > + > if (origin == ORIGIN_CS) { > spin_lock(&i915->fb_tracking.lock); > /* Filter out new bits since rendering started. */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 91453f7dbd656..20c135a2bba33 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > #define HAS_ASYNC_FLIPS(i915) (DISPLAY_VER(i915) >= 5) > > +#define HAS_FRONTBUFFER_RENDERING(i915) (DISPLAY_VER(i915) < 9) > + > /* Only valid when HAS_DISPLAY() is true */ > #define INTEL_DISPLAY_ENABLED(dev_priv) \ > (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display) >
On Thu, 2021-09-02 at 21:42 +0300, Gwan-gyeong Mun wrote: > > On 8/25/21 3:58 AM, José Roberto de Souza wrote: > > By now all the userspace applications should have migrated to atomic > > or at least be calling DRM_IOCTL_MODE_DIRTYFB. > > > > With that we can kill frontbuffer rendering support in i915 for > > modern platforms. > > > > So here converting legacy APIs into atomic commits so it can be > > properly handled by driver i915. > > > > Several IGT tests will fail with this changes, because some tests > > were stressing those frontbuffer rendering scenarios that no userspace > > should be using by now, fixes to IGT should be sent soon. > > > > v2: > > - return earlier to not set fb_tracking.busy/flip_bits > > - added a warn on to make sure we are not setting the busy/flip_bits > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++---- > > drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++- > > .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++ > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > 4 files changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c > > index c7618fef01439..5aa996c3b7980 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > u32 src_w, u32 src_h, > > struct drm_modeset_acquire_ctx *ctx) > > { > > + struct drm_i915_private *i915 = to_i915(_crtc->dev); > > struct intel_plane *plane = to_intel_plane(_plane); > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > struct intel_plane_state *old_plane_state = > > @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > * PSR2 selective fetch also requires the slow path as > > * PSR2 plane and transcoder registers can only be updated during > > * vblank. > > - * > > - * FIXME bigjoiner fastpath would be good > > */ > > if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) || > > - crtc_state->update_pipe || crtc_state->bigjoiner || > > - crtc_state->enable_psr2_sel_fetch) > > + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915)) > > goto slow; > > > > /* > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c > > index e4b8602ec0cd2..3eb60785c9f29 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fb.c > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c > > @@ -3,6 +3,7 @@ > > * Copyright © 2021 Intel Corporation > > */ > > > > +#include <drm/drm_damage_helper.h> > > #include <drm/drm_framebuffer.h> > > #include <drm/drm_modeset_helper.h> > > > > @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > > unsigned int num_clips) > > { > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > > > > i915_gem_object_flush_if_display(obj); > > - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips, > > + num_clips); > Hi, > Even if the userspace informs us of the dirty (damage) region of the > front buffer being used, GEN9 to GEN12 still uses the HW Tracking > function for PSR and FBC. > And if you look at the description of the intel_psr_flush() function, > you can see that there are the following restrictions. > > "Since the hardware frontbuffer tracking has gaps we need to integrate > with the software frontbuffer tracking." > > If this restriction is still valid from GEN9 to GEN12, even if the > existing frontbuffer tracking function is not used, when > intel_user_framebuffer_dirty() is called, in the case of PSR, > psr_force_hw_tracking_exit() is called or intel_psr_exit() and > schedule_work(psr.work) seems to be required. As this will trigger calls to the functions that write the plane registers PSR HW tracking and FBC tracking will understand a page flip happened even if going from and to the same surface. But will double check it. > > In the case of FBC, it seems that calls to FBC deactive / FBC activate > should be added. > > If GEN9 to GEN12 do not have the above restrictions, please ignore this > comment. > > G.G. > > + > > + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > > return 0; > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > index 0492446cd04ad..3860f87dac31c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915, > > void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > > unsigned frontbuffer_bits) > > { > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > + return; > > + > > spin_lock(&i915->fb_tracking.lock); > > i915->fb_tracking.flip_bits |= frontbuffer_bits; > > /* Remove stale busy bits due to the old buffer. */ > > @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > > void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > > unsigned frontbuffer_bits) > > { > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) { > > + drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits | > > + i915->fb_tracking.busy_bits); > > + return; > > + } > > + > > spin_lock(&i915->fb_tracking.lock); > > /* Mask any cancelled flips. */ > > frontbuffer_bits &= i915->fb_tracking.flip_bits; > > @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > > void intel_frontbuffer_flip(struct drm_i915_private *i915, > > unsigned frontbuffer_bits) > > { > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > + return; > > + > > spin_lock(&i915->fb_tracking.lock); > > /* Remove stale busy bits due to the old buffer. */ > > i915->fb_tracking.busy_bits &= ~frontbuffer_bits; > > @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front, > > { > > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > + return; > > + > > if (origin == ORIGIN_CS) { > > spin_lock(&i915->fb_tracking.lock); > > i915->fb_tracking.busy_bits |= frontbuffer_bits; > > @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front, > > { > > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > + return; > > + > > if (origin == ORIGIN_CS) { > > spin_lock(&i915->fb_tracking.lock); > > /* Filter out new bits since rendering started. */ > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 91453f7dbd656..20c135a2bba33 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > > > #define HAS_ASYNC_FLIPS(i915) (DISPLAY_VER(i915) >= 5) > > > > +#define HAS_FRONTBUFFER_RENDERING(i915) (DISPLAY_VER(i915) < 9) > > + > > /* Only valid when HAS_DISPLAY() is true */ > > #define INTEL_DISPLAY_ENABLED(dev_priv) \ > > (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display) > >
On Fri, 2021-09-03 at 22:09 +0000, Souza, Jose wrote: > On Thu, 2021-09-02 at 21:42 +0300, Gwan-gyeong Mun wrote: > > > > On 8/25/21 3:58 AM, José Roberto de Souza wrote: > > > By now all the userspace applications should have migrated to atomic > > > or at least be calling DRM_IOCTL_MODE_DIRTYFB. > > > > > > With that we can kill frontbuffer rendering support in i915 for > > > modern platforms. > > > > > > So here converting legacy APIs into atomic commits so it can be > > > properly handled by driver i915. > > > > > > Several IGT tests will fail with this changes, because some tests > > > were stressing those frontbuffer rendering scenarios that no userspace > > > should be using by now, fixes to IGT should be sent soon. > > > > > > v2: > > > - return earlier to not set fb_tracking.busy/flip_bits > > > - added a warn on to make sure we are not setting the busy/flip_bits > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++---- > > > drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++- > > > .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > > 4 files changed, 29 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c > > > index c7618fef01439..5aa996c3b7980 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > > @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > > u32 src_w, u32 src_h, > > > struct drm_modeset_acquire_ctx *ctx) > > > { > > > + struct drm_i915_private *i915 = to_i915(_crtc->dev); > > > struct intel_plane *plane = to_intel_plane(_plane); > > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > > struct intel_plane_state *old_plane_state = > > > @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > > * PSR2 selective fetch also requires the slow path as > > > * PSR2 plane and transcoder registers can only be updated during > > > * vblank. > > > - * > > > - * FIXME bigjoiner fastpath would be good > > > */ > > > if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) || > > > - crtc_state->update_pipe || crtc_state->bigjoiner || > > > - crtc_state->enable_psr2_sel_fetch) > > > + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915)) > > > goto slow; > > > > > > /* > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c > > > index e4b8602ec0cd2..3eb60785c9f29 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c > > > @@ -3,6 +3,7 @@ > > > * Copyright © 2021 Intel Corporation > > > */ > > > > > > +#include <drm/drm_damage_helper.h> > > > #include <drm/drm_framebuffer.h> > > > #include <drm/drm_modeset_helper.h> > > > > > > @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, > > > unsigned int num_clips) > > > { > > > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > > > > > > i915_gem_object_flush_if_display(obj); > > > - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > > > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > > + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips, > > > + num_clips); > > Hi, > > Even if the userspace informs us of the dirty (damage) region of the > > front buffer being used, GEN9 to GEN12 still uses the HW Tracking > > function for PSR and FBC. > > And if you look at the description of the intel_psr_flush() function, > > you can see that there are the following restrictions. > > > > "Since the hardware frontbuffer tracking has gaps we need to integrate > > with the software frontbuffer tracking." > > > > If this restriction is still valid from GEN9 to GEN12, even if the > > existing frontbuffer tracking function is not used, when > > intel_user_framebuffer_dirty() is called, in the case of PSR, > > psr_force_hw_tracking_exit() is called or intel_psr_exit() and > > schedule_work(psr.work) seems to be required. > > As this will trigger calls to the functions that write the plane registers PSR HW tracking and FBC tracking will understand a page flip happened even > if going from and to the same surface. > > But will double check it. Yep, no issues with PSR or FBC. HW understand as a flip and it is properly handled. > > > > > In the case of FBC, it seems that calls to FBC deactive / FBC activate > > should be added. > > > > If GEN9 to GEN12 do not have the above restrictions, please ignore this > > comment. > > > > G.G. > > > + > > > + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); > > > return 0; > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > > index 0492446cd04ad..3860f87dac31c 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > > > @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915, > > > void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > > > unsigned frontbuffer_bits) > > > { > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > > + return; > > > + > > > spin_lock(&i915->fb_tracking.lock); > > > i915->fb_tracking.flip_bits |= frontbuffer_bits; > > > /* Remove stale busy bits due to the old buffer. */ > > > @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, > > > void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > > > unsigned frontbuffer_bits) > > > { > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) { > > > + drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits | > > > + i915->fb_tracking.busy_bits); > > > + return; > > > + } > > > + > > > spin_lock(&i915->fb_tracking.lock); > > > /* Mask any cancelled flips. */ > > > frontbuffer_bits &= i915->fb_tracking.flip_bits; > > > @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, > > > void intel_frontbuffer_flip(struct drm_i915_private *i915, > > > unsigned frontbuffer_bits) > > > { > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > > + return; > > > + > > > spin_lock(&i915->fb_tracking.lock); > > > /* Remove stale busy bits due to the old buffer. */ > > > i915->fb_tracking.busy_bits &= ~frontbuffer_bits; > > > @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front, > > > { > > > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > > + return; > > > + > > > if (origin == ORIGIN_CS) { > > > spin_lock(&i915->fb_tracking.lock); > > > i915->fb_tracking.busy_bits |= frontbuffer_bits; > > > @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front, > > > { > > > struct drm_i915_private *i915 = to_i915(front->obj->base.dev); > > > > > > + if (!HAS_FRONTBUFFER_RENDERING(i915)) > > > + return; > > > + > > > if (origin == ORIGIN_CS) { > > > spin_lock(&i915->fb_tracking.lock); > > > /* Filter out new bits since rendering started. */ > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 91453f7dbd656..20c135a2bba33 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > > > > > #define HAS_ASYNC_FLIPS(i915) (DISPLAY_VER(i915) >= 5) > > > > > > +#define HAS_FRONTBUFFER_RENDERING(i915) (DISPLAY_VER(i915) < 9) > > > + > > > /* Only valid when HAS_DISPLAY() is true */ > > > #define INTEL_DISPLAY_ENABLED(dev_priv) \ > > > (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display) > > > >
Looks good to me. Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> On 9/4/21 3:26 AM, Souza, Jose wrote: > On Fri, 2021-09-03 at 22:09 +0000, Souza, Jose wrote: >> On Thu, 2021-09-02 at 21:42 +0300, Gwan-gyeong Mun wrote: >>> >>> On 8/25/21 3:58 AM, José Roberto de Souza wrote: >>>> By now all the userspace applications should have migrated to atomic >>>> or at least be calling DRM_IOCTL_MODE_DIRTYFB. >>>> >>>> With that we can kill frontbuffer rendering support in i915 for >>>> modern platforms. >>>> >>>> So here converting legacy APIs into atomic commits so it can be >>>> properly handled by driver i915. >>>> >>>> Several IGT tests will fail with this changes, because some tests >>>> were stressing those frontbuffer rendering scenarios that no userspace >>>> should be using by now, fixes to IGT should be sent soon. >>>> >>>> v2: >>>> - return earlier to not set fb_tracking.busy/flip_bits >>>> - added a warn on to make sure we are not setting the busy/flip_bits >>>> >>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++---- >>>> drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++- >>>> .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++ >>>> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >>>> 4 files changed, 29 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c >>>> index c7618fef01439..5aa996c3b7980 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c >>>> @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, >>>> u32 src_w, u32 src_h, >>>> struct drm_modeset_acquire_ctx *ctx) >>>> { >>>> +struct drm_i915_private *i915 = to_i915(_crtc->dev); >>>> struct intel_plane *plane = to_intel_plane(_plane); >>>> struct intel_crtc *crtc = to_intel_crtc(_crtc); >>>> struct intel_plane_state *old_plane_state = >>>> @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, >>>> * PSR2 selective fetch also requires the slow path as >>>> * PSR2 plane and transcoder registers can only be updated during >>>> * vblank. >>>> - * >>>> - * FIXME bigjoiner fastpath would be good >>>> */ >>>> if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) || >>>> - crtc_state->update_pipe || crtc_state->bigjoiner || >>>> - crtc_state->enable_psr2_sel_fetch) >>>> + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915)) >>>> goto slow; >>>> >>>> /* >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c >>>> index e4b8602ec0cd2..3eb60785c9f29 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_fb.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_fb.c >>>> @@ -3,6 +3,7 @@ >>>> * Copyright © 2021 Intel Corporation >>>> */ >>>> >>>> +#include <drm/drm_damage_helper.h> >>>> #include <drm/drm_framebuffer.h> >>>> #include <drm/drm_modeset_helper.h> >>>> >>>> @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, >>>> unsigned int num_clips) >>>> { >>>> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >>>> +struct drm_i915_private *i915 = to_i915(obj->base.dev); >>>> >>>> i915_gem_object_flush_if_display(obj); >>>> -intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); >>>> >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) >>>> +return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips, >>>> + num_clips); >>> Hi, >>> Even if the userspace informs us of the dirty (damage) region of the >>> front buffer being used, GEN9 to GEN12 still uses the HW Tracking >>> function for PSR and FBC. >>> And if you look at the description of the intel_psr_flush() function, >>> you can see that there are the following restrictions. >>> >>> "Since the hardware frontbuffer tracking has gaps we need to integrate >>> with the software frontbuffer tracking." >>> >>> If this restriction is still valid from GEN9 to GEN12, even if the >>> existing frontbuffer tracking function is not used, when >>> intel_user_framebuffer_dirty() is called, in the case of PSR, >>> psr_force_hw_tracking_exit() is called or intel_psr_exit() and >>> schedule_work(psr.work) seems to be required. >> >> As this will trigger calls to the functions that write the plane registers PSR HW tracking and FBC tracking will understand a page flip happened even >> if going from and to the same surface. >> >> But will double check it. > > Yep, no issues with PSR or FBC. HW understand as a flip and it is properly handled. > >> >>> >>> In the case of FBC, it seems that calls to FBC deactive / FBC activate >>> should be added. >>> >>> If GEN9 to GEN12 do not have the above restrictions, please ignore this >>> comment. >>> >>> G.G. >>>> + >>>> +intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); >>>> return 0; >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c >>>> index 0492446cd04ad..3860f87dac31c 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c >>>> @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915, >>>> void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, >>>> unsigned frontbuffer_bits) >>>> { >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) >>>> +return; >>>> + >>>> spin_lock(&i915->fb_tracking.lock); >>>> i915->fb_tracking.flip_bits |= frontbuffer_bits; >>>> /* Remove stale busy bits due to the old buffer. */ >>>> @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, >>>> void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, >>>> unsigned frontbuffer_bits) >>>> { >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) { >>>> +drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits | >>>> + i915->fb_tracking.busy_bits); >>>> +return; >>>> +} >>>> + >>>> spin_lock(&i915->fb_tracking.lock); >>>> /* Mask any cancelled flips. */ >>>> frontbuffer_bits &= i915->fb_tracking.flip_bits; >>>> @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, >>>> void intel_frontbuffer_flip(struct drm_i915_private *i915, >>>> unsigned frontbuffer_bits) >>>> { >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) >>>> +return; >>>> + >>>> spin_lock(&i915->fb_tracking.lock); >>>> /* Remove stale busy bits due to the old buffer. */ >>>> i915->fb_tracking.busy_bits &= ~frontbuffer_bits; >>>> @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front, >>>> { >>>> struct drm_i915_private *i915 = to_i915(front->obj->base.dev); >>>> >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) >>>> +return; >>>> + >>>> if (origin == ORIGIN_CS) { >>>> spin_lock(&i915->fb_tracking.lock); >>>> i915->fb_tracking.busy_bits |= frontbuffer_bits; >>>> @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front, >>>> { >>>> struct drm_i915_private *i915 = to_i915(front->obj->base.dev); >>>> >>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) >>>> +return; >>>> + >>>> if (origin == ORIGIN_CS) { >>>> spin_lock(&i915->fb_tracking.lock); >>>> /* Filter out new bits since rendering started. */ >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 91453f7dbd656..20c135a2bba33 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >>>> >>>> #define HAS_ASYNC_FLIPS(i915)(DISPLAY_VER(i915) >= 5) >>>> >>>> +#define HAS_FRONTBUFFER_RENDERING(i915)(DISPLAY_VER(i915) < 9) >>>> + >>>> /* Only valid when HAS_DISPLAY() is true */ >>>> #define INTEL_DISPLAY_ENABLED(dev_priv) \ >>>> (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display) >>>> >> >
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index c7618fef01439..5aa996c3b7980 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, u32 src_w, u32 src_h, struct drm_modeset_acquire_ctx *ctx) { + struct drm_i915_private *i915 = to_i915(_crtc->dev); struct intel_plane *plane = to_intel_plane(_plane); struct intel_crtc *crtc = to_intel_crtc(_crtc); struct intel_plane_state *old_plane_state = @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, * PSR2 selective fetch also requires the slow path as * PSR2 plane and transcoder registers can only be updated during * vblank. - * - * FIXME bigjoiner fastpath would be good */ if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) || - crtc_state->update_pipe || crtc_state->bigjoiner || - crtc_state->enable_psr2_sel_fetch) + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915)) goto slow; /* diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index e4b8602ec0cd2..3eb60785c9f29 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -3,6 +3,7 @@ * Copyright © 2021 Intel Corporation */ +#include <drm/drm_damage_helper.h> #include <drm/drm_framebuffer.h> #include <drm/drm_modeset_helper.h> @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb, unsigned int num_clips) { struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct drm_i915_private *i915 = to_i915(obj->base.dev); i915_gem_object_flush_if_display(obj); - intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); + if (!HAS_FRONTBUFFER_RENDERING(i915)) + return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips, + num_clips); + + intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB); return 0; } diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c index 0492446cd04ad..3860f87dac31c 100644 --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915, void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, unsigned frontbuffer_bits) { + if (!HAS_FRONTBUFFER_RENDERING(i915)) + return; + spin_lock(&i915->fb_tracking.lock); i915->fb_tracking.flip_bits |= frontbuffer_bits; /* Remove stale busy bits due to the old buffer. */ @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915, void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, unsigned frontbuffer_bits) { + if (!HAS_FRONTBUFFER_RENDERING(i915)) { + drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits | + i915->fb_tracking.busy_bits); + return; + } + spin_lock(&i915->fb_tracking.lock); /* Mask any cancelled flips. */ frontbuffer_bits &= i915->fb_tracking.flip_bits; @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915, void intel_frontbuffer_flip(struct drm_i915_private *i915, unsigned frontbuffer_bits) { + if (!HAS_FRONTBUFFER_RENDERING(i915)) + return; + spin_lock(&i915->fb_tracking.lock); /* Remove stale busy bits due to the old buffer. */ i915->fb_tracking.busy_bits &= ~frontbuffer_bits; @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front, { struct drm_i915_private *i915 = to_i915(front->obj->base.dev); + if (!HAS_FRONTBUFFER_RENDERING(i915)) + return; + if (origin == ORIGIN_CS) { spin_lock(&i915->fb_tracking.lock); i915->fb_tracking.busy_bits |= frontbuffer_bits; @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front, { struct drm_i915_private *i915 = to_i915(front->obj->base.dev); + if (!HAS_FRONTBUFFER_RENDERING(i915)) + return; + if (origin == ORIGIN_CS) { spin_lock(&i915->fb_tracking.lock); /* Filter out new bits since rendering started. */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 91453f7dbd656..20c135a2bba33 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define HAS_ASYNC_FLIPS(i915) (DISPLAY_VER(i915) >= 5) +#define HAS_FRONTBUFFER_RENDERING(i915) (DISPLAY_VER(i915) < 9) + /* Only valid when HAS_DISPLAY() is true */ #define INTEL_DISPLAY_ENABLED(dev_priv) \ (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display)
By now all the userspace applications should have migrated to atomic or at least be calling DRM_IOCTL_MODE_DIRTYFB. With that we can kill frontbuffer rendering support in i915 for modern platforms. So here converting legacy APIs into atomic commits so it can be properly handled by driver i915. Several IGT tests will fail with this changes, because some tests were stressing those frontbuffer rendering scenarios that no userspace should be using by now, fixes to IGT should be sent soon. v2: - return earlier to not set fb_tracking.busy/flip_bits - added a warn on to make sure we are not setting the busy/flip_bits Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++---- drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++- .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/i915_drv.h | 2 ++ 4 files changed, 29 insertions(+), 5 deletions(-)