linux-kernel - Re: [RFC v4 1/3] drm/fence: add in-fences support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160901161400.GL23577@joana>
Date:   Thu, 1 Sep 2016 12:14:00 -0400
From:   Gustavo Padovan <gustavo@...ovan.org>
To:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Daniel Stone <daniels@...labora.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Rob Clark <robdclark@...il.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        John Harrison <John.C.Harrison@...el.com>,
        laurent.pinchart@...asonboard.com, seanpaul@...gle.com,
        marcheu@...gle.com, m.chehab@...sung.com,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>
Subject: Re: [RFC v4 1/3] drm/fence: add in-fences support

Hi Maarten,

2016-09-01 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>:

> Op 31-08-16 om 21:07 schreef Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@...labora.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.
> >
> > v2: Comments by Daniel Vetter:
> > 	- remove set state->fence = NULL in destroy phase
> > 	- accept fence -1 as valid and just return 0
> > 	- do not call fence_get() - sync_file_fences_get() already calls it
> > 	- fence_put() if state->fence is already set, in case userspace
> > 	set the property more than once.
> >
> > v3: WARN_ON if fence is set but state has no FB
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> > ---
> >  drivers/gpu/drm/Kconfig             |  1 +
> >  drivers/gpu/drm/drm_atomic.c        | 19 +++++++++++++++++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  3 +++
> >  drivers/gpu/drm/drm_crtc.c          |  7 +++++++
> >  include/drm/drm_crtc.h              |  4 ++++
> >  5 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index c02be6a..07f9c60 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 5cb2e22..9e6c4e7 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>
> >  
> >  #include "drm_crtc_internal.h"
> >  
> > @@ -690,6 +691,17 @@ 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) {
> > +		if (U642I64(val) == -1)
> > +			return 0;
> > +
> > +		if (state->fence)
> > +			fence_put(state->fence);
> > +
> > +		state->fence = sync_file_get_fence(val);
> > +		if (!state->fence)
> > +			return -EINVAL;
> > @@ -749,6 +761,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) {
> > @@ -824,6 +838,11 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (WARN_ON(state->fence && !state->fb)) {
> > +		DRM_DEBUG_ATOMIC("in-fence set but no FB\n");
> > +		return -EINVAL;
> > +	}
> Why is this a error? Could be useful to fence a modeset disable.

Yes. I didn't envision that use case.  I'll change that for the next
version.

> 
> It might make more sense to put the fence inside the crtc state, not the plane state. Updates are done per crtc and moving planes
> between multiple crtc's inside a single commit is not allowed. I'd like to know what others think of that.
> 
> I'm not sure this patch is tested, looks like plane_duplicate_state is not clearing ->fence, probably resulting in WARNs.

It is tested, but I'n not seeing any warning there. Which WARNs are you
talking about? And why do we need to clear fence there? we don't clean
anything else on duplicate.

Gustavo