Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Robert Foss]

On 05/02/2016 08:57 PM, Eric Anholt wrote:
> robert.foss@xxxxxxxxxxxxx writes:
>
> > From: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> >
> > As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> > update is requested and there is an earlier update pending".
>
> Note: docs cited here are drm_crtc.h, and the whole quote is:
>
> 	 *  - -EBUSY, if an asynchronous updated is requested and there is
> 	 *    an earlier updated pending. Drivers are allowed to support a queue
> 	 *    of outstanding updates, but currently no driver supports that.
> 	 *    Note that drivers must wait for preceding updates to complete if a
> 	 *    synchronous update is requested, they are not allowed to fail the
> 	 *    commit in that case.
>
> > Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
> >   drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
> >   drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
> >   3 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> > index 355ee4b..43193a3 100644
> > --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> > @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
> >   		.of_match_table = vc4_crtc_dt_match,
> >   	},
> >   };
> > +
> > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
> > +{
> > +	assert_spin_locked(&crtc->dev->event_lock);
> > +	return to_vc4_crtc(crtc)->event;
> > +}
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index fa2ad15..54c1fb5 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
> >   int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
> >   void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
> >   int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
> >
> >   /* vc4_debugfs.c */
> >   int vc4_debugfs_init(struct drm_minor *minor);
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 4718ae5..dc157a1e 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
> >   			     bool async)
> >   {
> >   	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > -	int ret;
> > -	int i;
> >   	uint64_t wait_seqno = 0;
> >   	struct vc4_commit *c;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	unsigned long flags;
> > +	int i, ret;
> > +
> > +	if (async) {
> > +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +
> > +			spin_lock_irqsave(&dev->event_lock, flags);
> > +
> > +			if (crtc->state->event ||
> > +				vc4_crtc_has_pending_event(crtc)) {
> > +				spin_unlock_irqrestore(&dev->event_lock, flags);
> > +				return -EBUSY;
> > +			}
> > +			spin_unlock_irqrestore(&dev->event_lock, flags);
> > +		}
> > +	}
>
> By my reading, the point of returning -EBUSY here is just to avoid
> blocking when the compositor asked for nonblocking.  You're checking
> that each CRTC involved in the commit doesn't have a queued pageflip
> event, except that then we immediately proceed to:
>
> 	/* Make sure that any outstanding modesets have finished. */
> 	ret = down_interruptible(&vc4->async_modeset);
> 	if (ret) {
> 		kfree(c);
> 		return ret;
> 	}
>
> so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
> this commit was disjoint from the last one, right?  To implement the
> minimal behavior, I think we'd want to just down_trylock instead in the
> async case, I think.  And to implement the disjoint CRTC thing you were
> trying for, I think we'd need to track a mask kind of like msm's
> pending_crtcs.

So what you're saying is that the set of CRTCs in state
might not contain all CRTCs and that this check might be incomplete for 
that reason?

I'm fairly new to these waters, so don't hesitate to tell me if I seem
to be misunderstanding something or am on a wild goose chase of some sort.

So you're suggesting something like this instead of
the per CRTC check:

- 	/* Make sure that any outstanding modesets have finished. */
- 	ret = down_interruptible(&vc4->async_modeset);
- 	if (ret) {
- 		kfree(c);
- 		return ret;
- 	}
+ 	/* Make sure that any outstanding modesets have finished. */
+ 	ret = down_trylock(&vc4->async_modeset);
+ 	if (ret) {
+ 		kfree(c);
+ 		return -EBUSY;
+ 	}

I've quickly tested the above patch and it does seem to work and fulfill 
all of my requirements.