| Message ID | 20230904041640.31297-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/i915: Use vblank worker to unpin old legacy cursor fb safely | expand |
I didn't spot anything to nag about. Just hope that warning doesn't become excessively noisy. These two patches are Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> On 4.9.2023 7.16, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The cursor hardware only does sync updates, and thus the hardware > will be scanning out from the old fb until the next start of vblank. > So in order to make the legacy cursor fastpath actually safe we > should not unpin the old fb until we're sure the hardware has > ceased accessing it. The simplest approach is to just use a vblank > work here to do the delayed unpin. > > Not 100% sure it's a good idea to put this onto the same high > priority vblank worker as eg. our timing critical gamma updates. > But let's keep it simple for now, and it we later discover that > this is causing problems we can think about adding a lower > priority worker for such things. > > v2: wait for cursor unpins before turning off the vblank irq > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_cursor.c | 36 +++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_cursor.h | 2 ++ > drivers/gpu/drm/i915/display/intel_display.c | 3 ++ > .../drm/i915/display/intel_display_types.h | 4 +++ > 4 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c > index b342fad180ca..625540fd1dab 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > @@ -603,6 +603,26 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane, > return format == DRM_FORMAT_ARGB8888; > } > > +static void intel_cursor_unpin_work(struct kthread_work *base) > +{ > + struct drm_vblank_work *work = to_drm_vblank_work(base); > + struct intel_plane_state *plane_state = > + container_of(work, typeof(*plane_state), unpin_work); > + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > + > + intel_plane_unpin_fb(plane_state); > + intel_plane_destroy_state(&plane->base, &plane_state->uapi); > + > + if (atomic_dec_and_test(&plane->cursor.pending_unpins)) > + wake_up_var(&plane->cursor.pending_unpins); > +} > + > +void intel_cursor_wait_unpin_works(struct intel_plane *plane) > +{ > + wait_var_event(&plane->cursor.pending_unpins, > + !atomic_read(&plane->cursor.pending_unpins)); > +} > + > static int > intel_legacy_cursor_update(struct drm_plane *_plane, > struct drm_crtc *_crtc, > @@ -730,14 +750,26 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > local_irq_enable(); > > - intel_plane_unpin_fb(old_plane_state); > + if (old_plane_state->hw.fb != new_plane_state->hw.fb) { > + drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base, > + intel_cursor_unpin_work); > + > + atomic_inc(&plane->cursor.pending_unpins); > + drm_vblank_work_schedule(&old_plane_state->unpin_work, > + drm_crtc_accurate_vblank_count(&crtc->base) + 1, > + false); > + > + old_plane_state = NULL; > + } else { > + intel_plane_unpin_fb(old_plane_state); > + } > > out_free: > if (new_crtc_state) > intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi); > if (ret) > intel_plane_destroy_state(&plane->base, &new_plane_state->uapi); > - else > + else if (old_plane_state) > intel_plane_destroy_state(&plane->base, &old_plane_state->uapi); > return ret; > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.h b/drivers/gpu/drm/i915/display/intel_cursor.h > index ce333bf4c2d5..e778aff77129 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.h > +++ b/drivers/gpu/drm/i915/display/intel_cursor.h > @@ -14,4 +14,6 @@ struct intel_plane * > intel_cursor_plane_create(struct drm_i915_private *dev_priv, > enum pipe pipe); > > +void intel_cursor_wait_unpin_works(struct intel_plane *plane); > + > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index f6397462e4c2..90c1ed61ba0e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -63,6 +63,7 @@ > #include "intel_crt.h" > #include "intel_crtc.h" > #include "intel_crtc_state_dump.h" > +#include "intel_cursor.h" > #include "intel_ddi.h" > #include "intel_de.h" > #include "intel_display_driver.h" > @@ -6618,6 +6619,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > > intel_pre_plane_update(state, crtc); > intel_crtc_disable_planes(state, crtc); > + > + intel_cursor_wait_unpin_works(to_intel_plane(crtc->base.cursor)); > } > > /* Only disable port sync and MST slaves */ > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index c21064794f32..f6ca86f1d834 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -702,6 +702,9 @@ struct intel_plane_state { > > struct intel_fb_view view; > > + /* for legacy cursor fb unpin */ > + struct drm_vblank_work unpin_work; > + > /* Plane pxp decryption state */ > bool decrypt; > > @@ -1515,6 +1518,7 @@ struct intel_plane { > > struct { > u32 base, cntl, size; > + atomic_t pending_unpins; > } cursor; > > struct intel_fbc *fbc;
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index b342fad180ca..625540fd1dab 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -603,6 +603,26 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane, return format == DRM_FORMAT_ARGB8888; } +static void intel_cursor_unpin_work(struct kthread_work *base) +{ + struct drm_vblank_work *work = to_drm_vblank_work(base); + struct intel_plane_state *plane_state = + container_of(work, typeof(*plane_state), unpin_work); + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); + + intel_plane_unpin_fb(plane_state); + intel_plane_destroy_state(&plane->base, &plane_state->uapi); + + if (atomic_dec_and_test(&plane->cursor.pending_unpins)) + wake_up_var(&plane->cursor.pending_unpins); +} + +void intel_cursor_wait_unpin_works(struct intel_plane *plane) +{ + wait_var_event(&plane->cursor.pending_unpins, + !atomic_read(&plane->cursor.pending_unpins)); +} + static int intel_legacy_cursor_update(struct drm_plane *_plane, struct drm_crtc *_crtc, @@ -730,14 +750,26 @@ intel_legacy_cursor_update(struct drm_plane *_plane, local_irq_enable(); - intel_plane_unpin_fb(old_plane_state); + if (old_plane_state->hw.fb != new_plane_state->hw.fb) { + drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base, + intel_cursor_unpin_work); + + atomic_inc(&plane->cursor.pending_unpins); + drm_vblank_work_schedule(&old_plane_state->unpin_work, + drm_crtc_accurate_vblank_count(&crtc->base) + 1, + false); + + old_plane_state = NULL; + } else { + intel_plane_unpin_fb(old_plane_state); + } out_free: if (new_crtc_state) intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi); if (ret) intel_plane_destroy_state(&plane->base, &new_plane_state->uapi); - else + else if (old_plane_state) intel_plane_destroy_state(&plane->base, &old_plane_state->uapi); return ret; diff --git a/drivers/gpu/drm/i915/display/intel_cursor.h b/drivers/gpu/drm/i915/display/intel_cursor.h index ce333bf4c2d5..e778aff77129 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.h +++ b/drivers/gpu/drm/i915/display/intel_cursor.h @@ -14,4 +14,6 @@ struct intel_plane * intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe); +void intel_cursor_wait_unpin_works(struct intel_plane *plane); + #endif diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index f6397462e4c2..90c1ed61ba0e 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -63,6 +63,7 @@ #include "intel_crt.h" #include "intel_crtc.h" #include "intel_crtc_state_dump.h" +#include "intel_cursor.h" #include "intel_ddi.h" #include "intel_de.h" #include "intel_display_driver.h" @@ -6618,6 +6619,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) intel_pre_plane_update(state, crtc); intel_crtc_disable_planes(state, crtc); + + intel_cursor_wait_unpin_works(to_intel_plane(crtc->base.cursor)); } /* Only disable port sync and MST slaves */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index c21064794f32..f6ca86f1d834 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -702,6 +702,9 @@ struct intel_plane_state { struct intel_fb_view view; + /* for legacy cursor fb unpin */ + struct drm_vblank_work unpin_work; + /* Plane pxp decryption state */ bool decrypt; @@ -1515,6 +1518,7 @@ struct intel_plane { struct { u32 base, cntl, size; + atomic_t pending_unpins; } cursor; struct intel_fbc *fbc;