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

[Murthy, Arun R]

> -----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.

> > +		/* 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.

> >
> >  	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
--------------------