[PATCH 3/7] drm/vc4: Mimic drm_atomic_helper_commit() behavior

[Boris Brezillon]

On Tue, 06 Jun 2017 13:27:09 -0700
Eric Anholt <eric at anholt.net> wrote:

> Boris Brezillon <boris.brezillon at free-electrons.com> writes:
> 
> > The VC4 KMS driver is implementing its own ->atomic_commit() but there
> > are a few generic helpers we can use instead of open-coding the logic.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++--------------------------
> >  1 file changed, 12 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index ad7925a9e0ea..f229abc0991b 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c)
> >  	struct drm_device *dev = state->dev;
> >  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >  
> > +	drm_atomic_helper_wait_for_fences(dev, state, false);
> > +
> > +	drm_atomic_helper_wait_for_dependencies(state);  
> 
> With this wait_for_fences() addition and the reservation stuff that
> landed, I think we can rip out the "seqno cb" in vc4, and just use
> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail().  Do you
> see anything missing, with that?

I can't tell. I haven't dig enough to understand what this seqno cb was
used for :-), but Daniel was suggesting the same thing. I'll try to
better understand what seqno cb does and if it's all safe to get rid of
it and use the standard helpers.

> 
> > +
> >  	drm_atomic_helper_commit_modeset_disables(dev, state);
> >  
> >  	drm_atomic_helper_commit_planes(dev, state, 0);
> > @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c)
> >  	 */
> >  	state->legacy_cursor_update = false;
> >  
> > +	drm_atomic_helper_commit_hw_done(state);
> > +
> >  	drm_atomic_helper_wait_for_vblanks(dev, state);
> >  
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> >  
> > +	drm_atomic_helper_commit_cleanup_done(state);
> > +
> >  	drm_atomic_state_put(state);
> >  
> >  	up(&vc4->async_modeset);
> > @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev,
> >  	if (!c)
> >  		return -ENOMEM;
> >  
> > -	/* Make sure that any outstanding modesets have finished. */
> > -	if (nonblock) {
> > -		struct drm_crtc *crtc;
> > -		struct drm_crtc_state *crtc_state;
> > -		unsigned long flags;
> > -		bool busy = false;
> > -
> > -		/*
> > -		 * If there's an undispatched event to send then we're
> > -		 * obviously still busy.  If there isn't, then we can
> > -		 * unconditionally wait for the semaphore because it
> > -		 * shouldn't be contended (for long).
> > -		 *
> > -		 * This is to prevent a race where queuing a new flip
> > -		 * from userspace immediately on receipt of an event
> > -		 * beats our clean-up and returns EBUSY.
> > -		 */
> > -		spin_lock_irqsave(&dev->event_lock, flags);
> > -		for_each_crtc_in_state(state, crtc, crtc_state, i)
> > -			busy |= vc4_event_pending(crtc);
> > -		spin_unlock_irqrestore(&dev->event_lock, flags);
> > -		if (busy) {
> > -			kfree(c);
> > -			return -EBUSY;
> > -		}
> > -	}
> > +	ret = drm_atomic_helper_setup_commit(state, nonblock);
> > +	if (ret)
> > +		return ret;
> > +  
> 
> Looks like vc4_event_pending() should be garbage-collected with this
> commit.

Indeed.

Thanks for the review.