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

[Philipp Zabel <p.zabel@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?

> > +
> > +		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.

[...]
> > @@ -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.

regards
Philipp

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