RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor updates
> -Original Message- > From: Ville Syrjälä > Sent: Wednesday, December 20, 2023 8:19 PM > To: Shankar, Uma > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor > updates > > On Wed, Dec 20, 2023 at 11:45:44AM +, Shankar, Uma wrote: > > > > > > > -Original Message- > > > From: Shankar, Uma > > > Sent: Wednesday, December 20, 2023 5:11 PM > > > To: Ville Syrjala ; > > > intel-gfx@lists.freedesktop.org > > > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around > > > legacy cursor updates > > > > > > > > > > > > > -Original Message- > > > > From: Intel-gfx On > > > > Behalf Of Ville Syrjala > > > > Sent: Wednesday, December 13, 2023 3:55 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Subject: [PATCH 8/9] drm/i915: Perform vblank evasion around > > > > legacy cursor updates > > > > > > > > From: Ville Syrjälä > > > > > > > > Our legacy cursor updates are actually mailbox updates. > > > > Ie. the hardware latches things once per frame on start of vblank, > > > > but we issue an number of updates per frame, withough any attempt > > > > to synchronize against the vblank in software. So in theory only > > > > the last update issued during the frame will latch, and the previous > > > > ones are > discarded. > > > > > > > > However this can lead to problems with maintaining the ggtt/iommu > > > > mappings as we have no idea which updates will actually latch. > > > > > > > > The problem is exacerbated by the hardware's annoying disarming > > > > behaviour; any non-arming register write will disarm an already > > > > armed update, only to be rearmed later by the arming register > > > > (CURBASE in case of cursors). If a disarming write happens just > > > > before the start of vblank, and the arming write happens after > > > > start of vblank we have effectively prevented the hardware from > > > > latching anything. And if we manage to straddle multiple > > > > sequential vblank starts in this manner we effectively prevent the > > > > hardware from latching any new registers for an arbitrary amount > > > > of time. This provides more time for the (potentially still in > > > use by the hardware) gtt/iommu mappings to be torn down. > > > > > > > > A partial solution, of course, is to use vblank evasion to avoid > > > > the register writes from spreading on both sides of the start of vblank. > > > > > > > > I've previously highlighted this problem as a general issue > > > > affecting mailbox updates. I even added some notes to the > > > > {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm > > > > and arm phases both need to pulled into the vblank evasion > > > > critical section if we actually decided to implement mailbox > > > > updates in general. But as I never impelemented the > > > > noarm+arm split for cursors we don't have to worry about that for the > moment. > > > > > > > > We've been lucky enough so far that this hasn't really caused > > > > problems. One thing that does help is that Xorg generally sticks > > > > to the same cursor BO. But igt seems pretty good at hitting this > > > > on MTL now, so apparently we have to start thinking about this. > > > > > > Was not aware that a disarming update will disarm an armed update > > > and make the vblank sync irrelevant. Thanks for a good writeup > > > highlighting the issue, really helps. > > > > > > Yeah, this should help maintain consistency with cursor updates and > > > ensure the sync at vblank. Change looks Good to me. > > > Reviewed-by: Uma Shankar > > > > > > > Signed-off-by: Ville Syrjälä > > > > --- > > > > drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++-- > > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > > > > b/drivers/gpu/drm/i915/display/intel_cursor.c > > > > index 926e2de00eb5..77531838001f 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > >
Re: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor updates
On Wed, Dec 20, 2023 at 11:45:44AM +, Shankar, Uma wrote: > > > > -Original Message- > > From: Shankar, Uma > > Sent: Wednesday, December 20, 2023 5:11 PM > > To: Ville Syrjala ; > > intel-gfx@lists.freedesktop.org > > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy > > cursor > > updates > > > > > > > > > -Original Message- > > > From: Intel-gfx On Behalf Of > > > Ville Syrjala > > > Sent: Wednesday, December 13, 2023 3:55 PM > > > To: intel-gfx@lists.freedesktop.org > > > Subject: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy > > > cursor updates > > > > > > From: Ville Syrjälä > > > > > > Our legacy cursor updates are actually mailbox updates. > > > Ie. the hardware latches things once per frame on start of vblank, but > > > we issue an number of updates per frame, withough any attempt to > > > synchronize against the vblank in software. So in theory only the last > > > update issued during the frame will latch, and the previous ones are > > > discarded. > > > > > > However this can lead to problems with maintaining the ggtt/iommu > > > mappings as we have no idea which updates will actually latch. > > > > > > The problem is exacerbated by the hardware's annoying disarming > > > behaviour; any non-arming register write will disarm an already armed > > > update, only to be rearmed later by the arming register (CURBASE in > > > case of cursors). If a disarming write happens just before the start > > > of vblank, and the arming write happens after start of vblank we have > > > effectively prevented the hardware from latching anything. And if we > > > manage to straddle multiple sequential vblank starts in this manner we > > > effectively prevent the hardware from latching any new registers for > > > an arbitrary amount of time. This provides more time for the (potentially > > > still in > > use by the hardware) gtt/iommu mappings to be torn down. > > > > > > A partial solution, of course, is to use vblank evasion to avoid the > > > register writes from spreading on both sides of the start of vblank. > > > > > > I've previously highlighted this problem as a general issue affecting > > > mailbox updates. I even added some notes to the > > > {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm and > > > arm phases both need to pulled into the vblank evasion critical > > > section if we actually decided to implement mailbox updates in > > > general. But as I never impelemented the > > > noarm+arm split for cursors we don't have to worry about that for the > > > moment. > > > > > > We've been lucky enough so far that this hasn't really caused > > > problems. One thing that does help is that Xorg generally sticks to > > > the same cursor BO. But igt seems pretty good at hitting this on MTL > > > now, so apparently we have to start thinking about this. > > > > Was not aware that a disarming update will disarm an armed update and make > > the vblank sync irrelevant. Thanks for a good writeup highlighting the > > issue, really > > helps. > > > > Yeah, this should help maintain consistency with cursor updates and ensure > > the > > sync at vblank. Change looks Good to me. > > Reviewed-by: Uma Shankar > > > > > Signed-off-by: Ville Syrjälä > > > --- > > > drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++-- > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > > > b/drivers/gpu/drm/i915/display/intel_cursor.c > > > index 926e2de00eb5..77531838001f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > > @@ -22,6 +22,7 @@ > > > #include "intel_frontbuffer.h" > > > #include "intel_psr.h" > > > #include "intel_psr_regs.h" > > > +#include "intel_vblank.h" > > > #include "skl_watermark.h" > > > > > > #include "gem/i915_gem_object.h" > > > @@ -647,12 +648,14 @@ intel_legacy_cursor_update(struct drm_plane > > > *_plane, { > > > struct intel_plane *plane = to_intel_plane(_plane); > > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > > + struct dr
RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor updates
> -Original Message- > From: Shankar, Uma > Sent: Wednesday, December 20, 2023 5:11 PM > To: Ville Syrjala ; > intel-gfx@lists.freedesktop.org > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor > updates > > > > > -Original Message- > > From: Intel-gfx On Behalf Of > > Ville Syrjala > > Sent: Wednesday, December 13, 2023 3:55 PM > > To: intel-gfx@lists.freedesktop.org > > Subject: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy > > cursor updates > > > > From: Ville Syrjälä > > > > Our legacy cursor updates are actually mailbox updates. > > Ie. the hardware latches things once per frame on start of vblank, but > > we issue an number of updates per frame, withough any attempt to > > synchronize against the vblank in software. So in theory only the last > > update issued during the frame will latch, and the previous ones are > > discarded. > > > > However this can lead to problems with maintaining the ggtt/iommu > > mappings as we have no idea which updates will actually latch. > > > > The problem is exacerbated by the hardware's annoying disarming > > behaviour; any non-arming register write will disarm an already armed > > update, only to be rearmed later by the arming register (CURBASE in > > case of cursors). If a disarming write happens just before the start > > of vblank, and the arming write happens after start of vblank we have > > effectively prevented the hardware from latching anything. And if we > > manage to straddle multiple sequential vblank starts in this manner we > > effectively prevent the hardware from latching any new registers for > > an arbitrary amount of time. This provides more time for the (potentially > > still in > use by the hardware) gtt/iommu mappings to be torn down. > > > > A partial solution, of course, is to use vblank evasion to avoid the > > register writes from spreading on both sides of the start of vblank. > > > > I've previously highlighted this problem as a general issue affecting > > mailbox updates. I even added some notes to the > > {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm and > > arm phases both need to pulled into the vblank evasion critical > > section if we actually decided to implement mailbox updates in > > general. But as I never impelemented the > > noarm+arm split for cursors we don't have to worry about that for the > > moment. > > > > We've been lucky enough so far that this hasn't really caused > > problems. One thing that does help is that Xorg generally sticks to > > the same cursor BO. But igt seems pretty good at hitting this on MTL > > now, so apparently we have to start thinking about this. > > Was not aware that a disarming update will disarm an armed update and make > the vblank sync irrelevant. Thanks for a good writeup highlighting the issue, > really > helps. > > Yeah, this should help maintain consistency with cursor updates and ensure the > sync at vblank. Change looks Good to me. > Reviewed-by: Uma Shankar > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++-- > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > > b/drivers/gpu/drm/i915/display/intel_cursor.c > > index 926e2de00eb5..77531838001f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > @@ -22,6 +22,7 @@ > > #include "intel_frontbuffer.h" > > #include "intel_psr.h" > > #include "intel_psr_regs.h" > > +#include "intel_vblank.h" > > #include "skl_watermark.h" > > > > #include "gem/i915_gem_object.h" > > @@ -647,12 +648,14 @@ intel_legacy_cursor_update(struct drm_plane > > *_plane, { > > struct intel_plane *plane = to_intel_plane(_plane); > > struct intel_crtc *crtc = to_intel_crtc(_crtc); > > + struct drm_i915_private *i915 = to_i915(plane->base.dev); > > struct intel_plane_state *old_plane_state = > > to_intel_plane_state(plane->base.state); > > struct intel_plane_state *new_plane_state; > > struct intel_crtc_state *crtc_state = > > to_intel_crtc_state(crtc->base.state); > > struct intel_crtc_state *new_crtc_state; > > + struct intel_vblank_evade_ctx evade; > > int ret; > > > > /* > > @@ -745,14 +748,15 @@ intel_legac
RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor updates
> -Original Message- > From: Intel-gfx On Behalf Of Ville > Syrjala > Sent: Wednesday, December 13, 2023 3:55 PM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor > updates > > From: Ville Syrjälä > > Our legacy cursor updates are actually mailbox updates. > Ie. the hardware latches things once per frame on start of vblank, but we > issue an > number of updates per frame, withough any attempt to synchronize against the > vblank in software. So in theory only the last update issued during the frame > will > latch, and the previous ones are discarded. > > However this can lead to problems with maintaining the ggtt/iommu mappings as > we have no idea which updates will actually latch. > > The problem is exacerbated by the hardware's annoying disarming behaviour; any > non-arming register write will disarm an already armed update, only to be > rearmed later by the arming register (CURBASE in case of cursors). If a > disarming > write happens just before the start of vblank, and the arming write happens > after > start of vblank we have effectively prevented the hardware from latching > anything. And if we manage to straddle multiple sequential vblank starts in > this > manner we effectively prevent the hardware from latching any new registers for > an arbitrary amount of time. This provides more time for the (potentially > still in > use by the hardware) gtt/iommu mappings to be torn down. > > A partial solution, of course, is to use vblank evasion to avoid the register > writes > from spreading on both sides of the start of vblank. > > I've previously highlighted this problem as a general issue affecting mailbox > updates. I even added some notes to the > {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm and arm phases > both need to pulled into the vblank evasion critical section if we actually > decided > to implement mailbox updates in general. But as I never impelemented the > noarm+arm split for cursors we don't have to worry about that for the moment. > > We've been lucky enough so far that this hasn't really caused problems. One > thing > that does help is that Xorg generally sticks to the same cursor BO. But igt > seems > pretty good at hitting this on MTL now, so apparently we have to start > thinking > about this. Was not aware that a disarming update will disarm an armed update and make the vblank sync irrelevant. Thanks for a good writeup highlighting the issue, really helps. Yeah, this should help maintain consistency with cursor updates and ensure the sync at vblank. Change looks Good to me. Reviewed-by: Uma Shankar > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++-- > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > b/drivers/gpu/drm/i915/display/intel_cursor.c > index 926e2de00eb5..77531838001f 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > @@ -22,6 +22,7 @@ > #include "intel_frontbuffer.h" > #include "intel_psr.h" > #include "intel_psr_regs.h" > +#include "intel_vblank.h" > #include "skl_watermark.h" > > #include "gem/i915_gem_object.h" > @@ -647,12 +648,14 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > { > struct intel_plane *plane = to_intel_plane(_plane); > struct intel_crtc *crtc = to_intel_crtc(_crtc); > + struct drm_i915_private *i915 = to_i915(plane->base.dev); > struct intel_plane_state *old_plane_state = > to_intel_plane_state(plane->base.state); > struct intel_plane_state *new_plane_state; > struct intel_crtc_state *crtc_state = > to_intel_crtc_state(crtc->base.state); > struct intel_crtc_state *new_crtc_state; > + struct intel_vblank_evade_ctx evade; > int ret; > > /* > @@ -745,14 +748,15 @@ intel_legacy_cursor_update(struct drm_plane *_plane, >*/ > crtc_state->active_planes = new_crtc_state->active_planes; > > - /* > - * Technically we should do a vblank evasion here to make > - * sure all the cursor registers update on the same frame. > - * For now just make sure the register writes happen as > - * quickly as possible to minimize the race window. > - */ > + intel_vblank_evade_init(crtc_state, crtc_state, ); > + > local_irq_disable(); > > + if (!drm_WARN_ON(>drm, drm_crtc_vblank_get(>base))) { > + intel_vblank_evade(); > + drm_crtc_vblank_put(>base); > + } > + > if (new_plane_state->uapi.visible) { > intel_plane_update_noarm(plane, crtc_state, new_plane_state); > intel_plane_update_arm(plane, crtc_state, new_plane_state); > -- > 2.41.0