[Intel-gfx] [PATCH 1/2] drm/i915: Stop accessing crtc->state from the flip done irq

[Murthy, Arun R]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Thursday, December 7, 2023 7:50 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Stop accessing crtc->state from
> the flip done irq
> 
> On Tue, Dec 05, 2023 at 11:16:58PM +0000, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> > > Of Ville Syrjälä
> > > Sent: Tuesday, November 21, 2023 7:21 PM
> > > To: intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Stop accessing
> > > crtc->state from the flip done irq
> > >
> > > On Thu, Sep 28, 2023 at 06:24:49PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > >
> > > > Assuming crtc->state is pointing at the correct thing for the
> > > > async flip commit is nonsense. If we had already queued up
> > > > multiple commits this would point at the very lates crtc state
> > > > even if the older commits hadn't even happened yet.
> > > >
> > > > Instead properly stage/arm the event like we do for async flips.
> > > > Since we don't need to arm multiple of these at the same time we
> > > > don't need a list like the normal vblank even processing uses.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_crtc.c          | 9 ++++++++-
> > > >  drivers/gpu/drm/i915/display/intel_display_irq.c   | 9 ++++-----
> > > >  drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
> > > >  3 files changed, 15 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > index 1fd068e6e26c..8a84a31c7b48 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > > @@ -553,8 +553,15 @@ void intel_pipe_update_start(struct
> > > > intel_atomic_state *state,
> > > >
> > > >  	intel_psr_lock(new_crtc_state);
> > > >
> > > > -	if (new_crtc_state->do_async_flip)
> > > > +	if (new_crtc_state->do_async_flip) {
> > > > +		spin_lock_irq(&crtc->base.dev->event_lock);
> >
> >
> > Would it be better to use irqsave since we are dealing with events.
> 
> One uses irqsave/restore when the we must protect against irq handlers, and
> the code can be called both with irqs enabled and irqs disabled.
> Here we are always called with irqs enabled, so the save/restore would be
> pointless.
> 
That's right, I didn't notice the next patch where irq_save/restore is removed and added this comment.

Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>

Thanks and Regards,
Arun R Murthy
--------------------

> >
> > > > +		/* arm the event for the flip done irq handler */
> > > > +		crtc->flip_done_event = new_crtc_state->uapi.event;
> > > > +		spin_unlock_irq(&crtc->base.dev->event_lock);
> > > > +
> > > > +		new_crtc_state->uapi.event = NULL;
> > > >  		return;
> > > > +	}
> > > >
> > > >  	if (intel_crtc_needs_vblank_work(new_crtc_state))
> > > >  		intel_crtc_vblank_work_init(new_crtc_state);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > index bff4a76310c0..d3df615f0e48 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > > @@ -340,16 +340,15 @@ static void flip_done_handler(struct
> > > drm_i915_private *i915,
> > > >  			      enum pipe pipe)
> > > >  {
> > > >  	struct intel_crtc *crtc = intel_crtc_for_pipe(i915, pipe);
> > > > -	struct drm_crtc_state *crtc_state = crtc->base.state;
> > > > -	struct drm_pending_vblank_event *e = crtc_state->event;
> > > >  	struct drm_device *dev = &i915->drm;
> > > >  	unsigned long irqflags;
> > > >
> > > >  	spin_lock_irqsave(&dev->event_lock, irqflags);
> > > >
> > > > -	crtc_state->event = NULL;
> > > > -
> > > > -	drm_crtc_send_vblank_event(&crtc->base, e);
> > > > +	if (crtc->flip_done_event) {
> > > > +		drm_crtc_send_vblank_event(&crtc->base, crtc-
> > > >flip_done_event);
> > > > +		crtc->flip_done_event = NULL;
> > > > +	}
> > >
> > > I just observed an oops here due to e==NULL with the current code.
> > > I *think* I've seen it once before as well. Pstore also caught what
> > > seemed to some kind of spurious DE interrupt, which might explain
> > > the oops. But not really sure what happened as the machine died before I
> could poke at it more.
> > >
> >
> > Earlier the event was set to NULL and then drm_crtc_send_vblank_event()
> was called.
> 
> The question is "how was this called when the event was NULL?".
> 
> The possible answers are:
> - spurious flip done irq
> - some kind of race with multiple commits, but can't immediately
>   think how that would happen as we still signal hw_done after
>   flip_done, and drm_atomic_helper_swap_state() will block on
>   hw_done, and the flip_done irq will not be enabled otherwise
> 
> >
> > > >
> > > >  	spin_unlock_irqrestore(&dev->event_lock, irqflags);  } diff
> > > > --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 8d8b2f8d37a9..a8ae1a25a550 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1461,6 +1461,9 @@ struct intel_crtc {
> > > >
> > > >  	struct intel_crtc_state *config;
> > > >
> > > > +	/* armed event for async flip */
> > > > +	struct drm_pending_vblank_event *flip_done_event;
> > > > +
> > > >  	/* Access to these should be protected by dev_priv->irq_lock. */
> > > >  	bool cpu_fifo_underrun_disabled;
> > > >  	bool pch_fifo_underrun_disabled;
> > > > --
> > > > 2.41.0
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> 
> --
> Ville Syrjälä
> Intel