2016-04-15 Daniel Vetter <daniel at ffwll.ch>:
> On Thu, Apr 14, 2016 at 06:29:37PM -0700, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> >
> > 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 at collabora.co.uk>
> > ---
> > 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
