[PATCH 1/3] drm/etnaviv: submit support for in-fences

[Gustavo Padovan]

Hi Philipp,

2017-03-13 Philipp Zabel <p.zabel at pengutronix.de>:

> Hi Gustavo,
> 
> thank you for the review.
> 
> On Wed, 2017-03-08 at 11:37 -0300, Gustavo Padovan wrote:
> [...]
> > > @@ -385,6 +396,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> > >  		goto err_submit_objects;
> > >  	}
> > >  
> > > +	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> > > +		in_fence = sync_file_get_fence(args->fence_fd);
> > > +		if (!in_fence) {
> > > +			ret = -EINVAL;
> > > +			goto err_submit_objects;
> > > +		}
> > > +
> > > +		/* TODO if we get an array-fence due to userspace merging
> > > +		 * multiple fences, we need a way to determine if all the
> > > +		 * backing fences are from our own context..
> > > +		 */
> > 
> > All GPU drivers seem to have the same need on fence_array, so I think we
> > can create a fence array helper to inspect if it has a specific context
> > or not.
> 
> Do you have a pointer where I could read up on how this should be done?

I was thinking on some function that would iterate over all fences in
the fence_array and check their context. The if we find our own gpu
context in there we fail the submit.

> 
> > > +
> > > +		if (in_fence->context != gpu->fence_context) {
> > > +			ret = dma_fence_wait(in_fence, true);
> > > +			if (ret)
> > > +				goto err_submit_objects;
> > 
> > sync_file_get_fence() hold a fence ref, we need to release it here
> > always and not only in case of error.
> 
> We do that already. err_submit_objects is an unfortunate name for patch
> review, but the out label at the end of the function falls right through
> to it.
> 
> > > +		}
> > > +	}
> > > +
> > >  	ret = submit_fence_sync(submit);
> > >  	if (ret)
> > >  		goto err_submit_objects;
> > > @@ -419,6 +449,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> > >  		flush_workqueue(priv->wq);
> > >  
> > >  err_submit_objects:
> > > +	if (in_fence)
> > > +		dma_fence_put(in_fence);
> 
> We pass through here in the non-error case, too.

Cool.

> 
> [...]
> > > @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > >   * one or more cmdstream buffers.  This allows for conditional execution
> > >   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > >   */
> > > +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > > +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > 
> > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > you send and fence_fd to the kernel you are requesting on explicit sync
> > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> 
> I just followed Rob's lead. I'm not sure if there may be a reason to
> submit an in fence still keep implicit fencing enabled at the same time,
> or to allow a submit without any fencing whatsoever. Maybe for testing
> purposes?
> I'm happy to drop the NO_IMPLICIT flag if there is no need for it.

Right. My understanding is that the flags would mean the same thing, but
I'm no expert on the GPU side of things. Maybe Rob can provide us some
insight on why he added NO_IMPLICIT.

Gustavo