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

[Lucas Stach <l.stach@pengutronix.de>]

Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
> Hi Gustavo,
> 
> On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> [...]
> > 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.
> 
> Why would we have to fail if somebody feeds us our own fences? Wouldn't
> it be enough to just wait if there are foreign fences in the array?

Yes, skipping the wait if all fences are from our own context is an
optimization and it's certainly not an issue if someone feeds us our own
fences.

> 
> How about something like this:
> 
> ----------8<----------
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 364fe50d020de..0b0bdaf4406d4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -296,6 +296,22 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	kfree(submit);
>  }
>  
> +static bool dma_fence_all_in_context(struct dma_fence *fence, u64 context)

dma_fence_match_context?

> +{
> +	if (dma_fence_is_array(fence)) {
> +		struct dma_fence_array *array = to_dma_fence_array(fence);
> +		int i;
> +
> +		for (i = 0; i < array->num_fences; i++) {
> +			if (array->fences[i]->context != context)
> +				return false;
> +		}
> +		return true;
> +	}
> +
> +	return fence->context == context;
> +}
> +

This looks good to me. Please add this to a common location in the next
submission.

>  int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> @@ -413,12 +429,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  			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..
> +		/*
> +		 * Wait if the fence is from a foreign context, or if the fence
> +		 * array contains any fence from a foreign context.
>  		 */
> -
> -		if (in_fence->context != gpu->fence_context) {
> +		if (!dma_fence_all_in_context(in_fence, gpu->fence_context)) {
>  			ret = dma_fence_wait(in_fence, true);
>  			if (ret)
>  				goto err_submit_objects;
> ---------->8----------
> 
> [...]
> > > > > @@ -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.
> 
> Yes, that would be welcome.


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel