diff mbox series

[8/9] drm/i915: Perform vblank evasion around legacy cursor updates

Message ID 20231213102519.13500-9-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Cursor vblank evasion | expand

Commit Message

Ville Syrjälä Dec. 13, 2023, 10:25 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Shankar, Uma Dec. 20, 2023, 11:41 a.m. UTC | #1 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> 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ä <ville.syrjala@linux.intel.com>
> 
> 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 <uma.shankar@intel.com>
 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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, &evade);
> +
>  	local_irq_disable();
> 
> +	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
> +		intel_vblank_evade(&evade);
> +		drm_crtc_vblank_put(&crtc->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
Shankar, Uma Dec. 20, 2023, 11:45 a.m. UTC | #2 
> -----Original Message-----
> From: Shankar, Uma
> Sent: Wednesday, December 20, 2023 5:11 PM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor
> updates
> 
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> 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ä <ville.syrjala@linux.intel.com>
> >
> > 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 <uma.shankar@intel.com>
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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, &evade);

Missed to update: 
Should the 2nd argument not be new_crtc_state ?

> >  	local_irq_disable();
> >
> > +	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
> > +		intel_vblank_evade(&evade);
> > +		drm_crtc_vblank_put(&crtc->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
Ville Syrjälä Dec. 20, 2023, 2:48 p.m. UTC | #3 
On Wed, Dec 20, 2023 at 11:45:44AM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Shankar, Uma
> > Sent: Wednesday, December 20, 2023 5:11 PM
> > To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor
> > updates
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> 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ä <ville.syrjala@linux.intel.com>
> > >
> > > 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 <uma.shankar@intel.com>
> > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  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, &evade);
> 
> Missed to update: 
> Should the 2nd argument not be new_crtc_state ?

We'll discard 'new_crtc_state' at the end and we just update
'crtc_state' in place (only active_planes actually). So essentially
'crtc_state' here is the new state already. That should be perfectly
fine for the purposes of vblank evasion since that only cares about
the old state when modesets are involved (which is never the case here).

> 
> > >  	local_irq_disable();
> > >
> > > +	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
> > > +		intel_vblank_evade(&evade);
> > > +		drm_crtc_vblank_put(&crtc->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
>
Shankar, Uma Dec. 22, 2023, 5:07 a.m. UTC | #4 
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, December 20, 2023 8:19 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> 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 +0000, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: Shankar, Uma
> > > Sent: Wednesday, December 20, 2023 5:11 PM
> > > To: Ville Syrjala <ville.syrjala@linux.intel.com>;
> > > intel-gfx@lists.freedesktop.org
> > > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around
> > > legacy cursor updates
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> 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ä <ville.syrjala@linux.intel.com>
> > > >
> > > > 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 <uma.shankar@intel.com>
> > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  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, &evade);
> >
> > Missed to update:
> > Should the 2nd argument not be new_crtc_state ?
> 
> We'll discard 'new_crtc_state' at the end and we just update 'crtc_state' in place
> (only active_planes actually). So essentially 'crtc_state' here is the new state
> already. That should be perfectly fine for the purposes of vblank evasion since
> that only cares about the old state when modesets are involved (which is never
> the case here).

Got it, thanks for clarifying Ville.

Regards,
Uma Shankar

> >
> > > >  	local_irq_disable();
> > > >
> > > > +	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
> > > > +		intel_vblank_evade(&evade);
> > > > +		drm_crtc_vblank_put(&crtc->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
> >
> 
> --
> Ville Syrjälä
> Intel
diff mbox series

Patch

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, &evade);
+
 	local_irq_disable();
 
+	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
+		intel_vblank_evade(&evade);
+		drm_crtc_vblank_put(&crtc->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);