Re: [RFC 4/8] drm/fence: add in-fences support

[Gustavo Padovan]

2016-04-15 Daniel Vetter <daniel@xxxxxxxx>:

> On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > 
> > There is now a new property called FENCE_FD attached to every plane
> > state that receives the sync_file fd from userspace via the atomic commit
> > IOCTL.
> > 
> > The fd is then translated to a fence (that may be a fence_collection
> > subclass or just a normal fence) and then used by DRM to fence_wait() for
> > all fences in the sync_file to signal. So it only commits when all
> > framebuffers are ready to scanout.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/Kconfig             | 1 +
> >  drivers/gpu/drm/drm_atomic.c        | 8 ++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
> >  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
> >  include/drm/drm_crtc.h              | 1 +
> >  5 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index f2a74d0..3c987e3 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -12,6 +12,7 @@ menuconfig DRM
> >  	select I2C
> >  	select I2C_ALGOBIT
> >  	select DMA_SHARED_BUFFER
> > +	select SYNC_FILE
> >  	help
> >  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
> >  	  introduced in XFree86 4.0. If you say Y here, you need to select
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 8ee1db8..6702502 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -30,6 +30,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_plane_helper.h>
> > +#include <linux/sync_file.h>
> >  
> >  /**
> >   * drm_atomic_state_default_release -
> > @@ -680,6 +681,11 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		drm_atomic_set_fb_for_plane(state, fb);
> >  		if (fb)
> >  			drm_framebuffer_unreference(fb);
> > +	} else if (property == config->prop_fence_fd) {
> > +		state->fence = sync_file_fences_get(val);
> > +		if (!state->fence)
> > +			return -EINVAL;
> > +		fence_get(state->fence);
> 
> Yeah, this fence_get must be part of sync_file_fences_get, this code here
> has a race (exercise for the reader to describe where things go wrong
> here). Also, you need to explicitly filter out -1 here first I think.

Right, I missed this race. Move it sync_file_fences_get() just fixes it.
I'll add the filter for -1 too and a testcase.

> 
> Needs an atomic testcase to make sure setting FENCE_FD to -1 doesn't fall
> over (since generic userspace might do this in an attempt to restore all
> the state).
> 
> >  	} else if (property == config->prop_crtc_id) {
> >  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> >  		return drm_atomic_set_crtc_for_plane(state, crtc);
> > @@ -737,6 +743,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >  
> >  	if (property == config->prop_fb_id) {
> >  		*val = (state->fb) ? state->fb->base.id : 0;
> > +	} else if (property == config->prop_fence_fd) {
> > +		*val = -1;
> >  	} else if (property == config->prop_crtc_id) {
> >  		*val = (state->crtc) ? state->crtc->base.id : 0;
> >  	} else if (property == config->prop_crtc_x) {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f85ef8c..6ed8339 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2687,6 +2687,11 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
> >  {
> >  	if (state->fb)
> >  		drm_framebuffer_unreference(state->fb);
> > +
> > +	if (state->fence) {
> > +		fence_put(state->fence);
> > +		state->fence = NULL;
> 
> No need to set to NULL, we don't do that for ->fb either.

Ok.

	Gustavo