Re: [PATCH 2/4] drm: adp: Handle drm_crtc_vblank_get() errors

[Janne Grunau <j@jannau.net>]

On Wed, Apr 16, 2025 at 04:58:18PM -0400, Alyssa Rosenzweig wrote:
> > -	spin_lock_irqsave(&crtc->dev->event_lock, flags);
> >  	if (crtc->state->event) {
> > -		drm_crtc_vblank_get(crtc);
> > -		adp->event = crtc->state->event;
> > +		spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +
> > +		if (drm_crtc_vblank_get(crtc) != 0)
> > +			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +		else
> > +			adp->event = crtc->state->event;
> > +
> > +		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> >  	}
> >  	crtc->state->event = NULL;
> > -	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> 
> Kind of confused about
> 
> >  	crtc->state->event = NULL;
> 
> now being out of the lock. Should we set to NULL in the if, since
> if we don't take the if, we know event is already NULL? Or should we
> hold the lock for the whole time, the way the code did before your
> change? I'm not sure between the two, but the in-between here smells
> wrong.

I struggled with this as well. To my understanding event_lock is
necessary for drm_crtc_send_vblank_event(), adp->event and
drm_crtc_vblank_get(). The first according to event_lock's
documentation, the second to avoid avoid races with the irq handler and
the third to ensure vblank interrupts are not disabled.
Based on examples in other drivers I assumed `crtc->state->event` is
protected by another lock held externally. Not sure about that now. To
my understanding sruct drm_crtc::mutex protects `crtc->state`.

I did not move "crtc->state->event = NULL;" to avoid churn. No point
setting it to NULL if it is already NULL.

I'll look tomorrow if the locking for crtc->state->event is sufficient.

Janne