[RFC v4 0/3] drm: add fences support through Sync Files

[RFC v4 0/3] drm: add fences support through Sync Files

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

Currently the Linux Kernel only have an implicit fencing mechanism where the
fence are attached directly to buffers and userspace is unaware of the inner
kernel workings. However by enabling explicit fencing, where fences travels
all the way up the userspace we can provide insightful information to
compositors so smarter decisions when scheduling framebuffers.

For that we adapted the Android Sync Framework[1], a explicit fencing mechanism
that helps the userspace handles fences directly. It has the concept of
sync_file (called sync_fence in Android) that expose the driver's fences to
userspace via file descriptors, than can then be shared between process.

With explicit fencing we have a global mechanism that optimizes the flow of
buffers between consumers and producers, avoid a lot of waiting. So instead
of waiting for a buffer to be processed by the GPU before sending it to DRM/KMS
in an Atomic IOCTL we can get a sync_file fd from the GPU driver at the moment
we submit the buffer processing. The compositor then passes these fds to DRM/KMS 
in a Atomic Commit request, that will not be displayed until the fences signal,
i.e, the GPU finished processing the buffer and it is ready to be displayed.

In DRM/KMS the fences we wait on before displaying a buffer are called
*in-fences*.  Vice-versa, we have *out-fences*, to sychronize the return of
buffers to GPU (producer) to be processed again. When DRM/KMS receives an
atomic request with a special flag set it generates one fence per-crtc and
attach it to a per-crtc sync_file.  It then returns the array of sync_file fds
to userspace as an atomic_ioctl out arg. With the fences available userspace
can forward these fences to the GPU, where it will wait the fence to signal
before starting to process on the shared buffer again.

Explicit fencing with the Sync Framework provides better traceability and 
debuggabilty of the kernel and allows efficient buffer suballocation by
Mesa/Vulkan.

DRM/KMS explicit fences are opt-in, as the default will still be implicit
fencing.  To enable explicit in-fences one just need to pass a sync_file fd in
the FENCE_FD plane property. *In-fences are per-plane*, i.e., per framebuffer.

For out-fences, just enabling DRM_MODE_ATOMIC_OUT_FENCE flag is enough.
*Out-fences are per-crtc*.

In-fences
---------

In the first discussions on #dri-devel on IRC we decided to hide the Sync
Framework from DRM drivers to reduce complexity, so as soon we get the fd
via FENCE_FD plane property we convert the sync_file fd to a struct fence.

Then we just use the already in place fence support to wait on those fences.
Once the producer calls fence_signal() for all fences on wait we can proceed
with the atomic commit and scanout the framebuffers.

Out-fences
----------

Passing the DRM_MODE_ATOMIC_OUT_FENCE flag to an atomic request enables
out-fences. The kernel then creates a fence, attach it to a sync_file and
install this file on a unused fd. The process is repeated for each crtc.
Userspace get the fence back as an array of per-crtc sync_file fds.

DRM core use the already in place drm_event infrastructure to help signal
fences, we've added a fence pointer to struct drm_pending_event. On vblank we
just call fence_signal() to signal that the buffer related to this fence is
*now* on the screen. Note that this is exactly the opposite behaviour from
Android, where the fences are signaled when they are not on display anymore, so
free to be reused.

No changes are required to DRM drivers to have out-fences support, apart from
atomic support of course.

Kernel tree
-----------

For those who want all patches on this RFC are in my tree. The tree includes a
few other patches necessary to run it, like the interruptible wait_for_fences
patch that I sent last week to the mainling list:

https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences


Testing
-------

For testing it Rob Clark add fences support to mesa and kmscube, Rob has
patch for the Mesa core native fence work and freedeno and I have added 
support of virgl as well:

git://github.com/freedreno/libdrm.git fences

git://git.collabora.com/git/user/padovan/mesa.git fences

git://github.com/robclark/kmscube.git atomic-fence


Changes in RFC v4
-----------------

Rebased against latest drm-misc

Regards,

        Gustavo
---

[1] https://source.android.com/devices/graphics/implement.html#vsync


Gustavo Padovan (3):
  drm/fence: add in-fences support
  drm/fence: add fence timeline to drm_crtc
  drm/fence: add out-fences support

 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_atomic.c        | 233 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_atomic_helper.c |   3 +
 drivers/gpu/drm/drm_crtc.c          |  38 ++++++
 include/drm/drm_crtc.h              |  33 +++++
 include/uapi/drm/drm_mode.h         |  15 ++-
 6 files changed, 301 insertions(+), 22 deletions(-)

-- 
2.5.5

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

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

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@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.

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@collabora.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;
+
 	} 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);
@@ -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;
+	}
+
 	/* if disabled, we don't care about the rest of the state: */
 	if (!state->crtc)
 		return 0;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index dff2389..f817452 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3156,6 +3156,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fence)
+		fence_put(state->fence);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b95c48ac..6eaeb73 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -338,6 +338,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
 		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
@@ -610,6 +611,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_fb_id = prop;
 
+	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
+			"FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3d33c90..2dc7e79 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1809,6 +1809,10 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_fb_id;
 	/**
+ 	 * @prop_fence_fd: Sync File fd reprenseting the fences for this plane
+	 */
+	struct drm_property *prop_fence_fd;
+	/**
 	 * @prop_crtc_id: Default atomic plane property to specify the
 	 * &drm_crtc.
 	 */
-- 
2.5.5

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

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

[Maarten Lankhorst]

Op 31-08-16 om 21:07 schreef Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@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.
>
> 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@collabora.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.

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.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index dff2389..f817452 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3156,6 +3156,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence)
> +		fence_put(state->fence);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b95c48ac..6eaeb73 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -338,6 +338,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -610,6 +611,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3d33c90..2dc7e79 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1809,6 +1809,10 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_fb_id;
>  	/**
> + 	 * @prop_fence_fd: Sync File fd reprenseting the fences for this plane
> +	 */
> +	struct drm_property *prop_fence_fd;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */


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

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

[Gustavo Padovan]

Hi Maarten,

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

> Op 31-08-16 om 21:07 schreef Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@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.
> >
> > 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@collabora.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

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

[RFC v4 2/3] drm/fence: add fence timeline to drm_crtc

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Create one timeline context for each CRTC to be able to handle out-fences
and signal them. It adds a few members to struct drm_crtc: fence_context,
where we store the context we get from fence_context_alloc(), the
fence seqno and the fence lock, that we pass in fence_init() to be
used by the fence.

v2: Comment by Daniel Stone:
	- add BUG_ON() to fence_to_crtc() macro

v3: Comment by Ville Syrjälä
	- Use more meaningful name as crtc timeline name

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 19 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6eaeb73..28e49a5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -141,6 +141,32 @@ static void drm_crtc_unregister_all(struct drm_device *dev)
 	}
 }
 
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->dev->driver->name;
+}
+
+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->timeline_name;
+}
+
+static bool drm_crtc_fence_enable_signaling(struct fence *fence)
+{
+	return true;
+}
+
+const struct fence_ops drm_crtc_fence_ops = {
+	.get_driver_name = drm_crtc_fence_get_driver_name,
+	.get_timeline_name = drm_crtc_fence_get_timeline_name,
+	.enable_signaling = drm_crtc_fence_enable_signaling,
+	.wait = fence_default_wait,
+};
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
@@ -194,6 +220,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return -ENOMEM;
 	}
 
+	crtc->fence_context = fence_context_alloc(1);
+	spin_lock_init(&crtc->fence_lock);
+	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
+		 "drm_crtc-%d", crtc->base.id);
+
 	crtc->base.properties = &crtc->properties;
 
 	list_add_tail(&crtc->head, &config->crtc_list);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 2dc7e79..e92fcb2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,8 @@
 #include <linux/fb.h>
 #include <linux/hdmi.h>
 #include <linux/media-bus-format.h>
+#include <linux/srcu.h>
+#include <linux/fence.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -542,6 +544,9 @@ struct drm_crtc_funcs {
  * @gamma_store: gamma ramp values
  * @helper_private: mid-layer private data
  * @properties: property tracking for this CRTC
+ * @fence_context: context for fence signalling
+ * @fence_lock: fence lock for the fence context
+ * @fence_seqno: seqno variable to create fences
  *
  * Each CRTC may have one or more connectors associated with it.  This structure
  * allows the CRTC to be controlled.
@@ -634,8 +639,22 @@ struct drm_crtc {
 	 * context.
 	 */
 	struct drm_modeset_acquire_ctx *acquire_ctx;
+
+	/* fence timelines info for DRM out-fences */
+	unsigned int fence_context;
+	spinlock_t fence_lock;
+	unsigned long fence_seqno;
+	char timeline_name[32];
 };
 
+extern const struct fence_ops drm_crtc_fence_ops;
+
+static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
+{
+	BUG_ON(fence->ops != &drm_crtc_fence_ops);
+	return container_of(fence->lock, struct drm_crtc, fence_lock);
+}
+
 /**
  * struct drm_plane_state - mutable plane state
  * @plane: backpointer to the plane
-- 
2.5.5

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

[RFC v4 3/3] drm/fence: add out-fences support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Support DRM out-fences creating a sync_file with a fence for each crtc
update with the DRM_MODE_ATOMIC_OUT_FENCE flag.

We then send an struct drm_out_fences array with the out-fences fds back in
the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.

struct drm_out_fences {
	__u32   crtc_id;
	__u32   fd;
};

v2: Comment by Rob Clark:
	- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

    Comment by Daniel Vetter:
	- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
	- create DRM_MODE_ATOMIC_EVENT_MASK
	- userspace should fill out_fences_ptr with the crtc_ids for which
	it wants fences back.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 214 ++++++++++++++++++++++++++++++++++++++-----
 include/drm/drm_crtc.h       |  10 ++
 include/uapi/drm/drm_mode.h  |  15 ++-
 3 files changed, 217 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9e6c4e7..8732c3d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1483,11 +1483,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
  */
 
 static struct drm_pending_vblank_event *create_vblank_event(
-		struct drm_device *dev, struct drm_file *file_priv,
-		struct fence *fence, uint64_t user_data)
+		struct drm_device *dev, uint64_t user_data)
 {
 	struct drm_pending_vblank_event *e = NULL;
-	int ret;
 
 	e = kzalloc(sizeof *e, GFP_KERNEL);
 	if (!e)
@@ -1497,17 +1495,6 @@ static struct drm_pending_vblank_event *create_vblank_event(
 	e->event.base.length = sizeof(e->event);
 	e->event.user_data = user_data;
 
-	if (file_priv) {
-		ret = drm_event_reserve_init(dev, file_priv, &e->base,
-					     &e->event.base);
-		if (ret) {
-			kfree(e);
-			return NULL;
-		}
-	}
-
-	e->base.fence = fence;
-
 	return e;
 }
 
@@ -1612,6 +1599,147 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+static struct drm_out_fence_state *get_out_fence(struct drm_device *dev,
+						 struct drm_atomic_state *state,
+						 uint32_t __user *out_fences_ptr,
+						 uint64_t count_out_fences,
+						 uint64_t user_data)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct drm_out_fences *out_fences;
+	struct drm_out_fence_state *fence_state;
+	int i, ret;
+
+	if (count_out_fences > dev->mode_config.num_crtc)
+		return ERR_PTR(-EINVAL);
+
+	out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
+			     GFP_KERNEL);
+	if (!out_fences)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(out_fences, out_fences_ptr,
+			   count_out_fences * sizeof(*out_fences))) {
+		kfree(out_fences);
+		return ERR_PTR(-EFAULT);
+	}
+
+	fence_state = kcalloc(count_out_fences, sizeof(*fence_state),
+			     GFP_KERNEL);
+	if (!fence_state) {
+		kfree(out_fences);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0 ; i < count_out_fences ; i++)
+		fence_state[i].fd = -1;
+
+	for (i = 0 ; i < count_out_fences ; i++) {
+		struct fence *fence;
+
+		if (out_fences[i].fd != -1) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		crtc = drm_crtc_find(dev, out_fences[i].crtc_id);
+		if (!crtc) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(crtc_state);
+			goto out;
+		}
+
+		fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+		if (!fence) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
+			   crtc->fence_context, ++crtc->fence_seqno);
+
+		fence_state[i].fd = get_unused_fd_flags(O_CLOEXEC);
+		if (fence_state[i].fd < 0) {
+			fence_put(fence);
+			ret = fence_state[i].fd;
+			goto out;
+		}
+
+		fence_state[i].sync_file = sync_file_create(fence);
+		if(!fence_state[i].sync_file) {
+			fence_put(fence);
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		crtc_state->event->base.fence = fence;
+
+		out_fences[i].fd = fence_state[i].fd;
+	}
+
+	if (copy_to_user(out_fences_ptr, out_fences,
+			 count_out_fences * sizeof(*out_fences))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	kfree(out_fences);
+
+	return fence_state;
+
+out:
+	for (i = 0 ; i < count_out_fences ; i++) {
+		if (fence_state[i].sync_file)
+			fput(fence_state[i].sync_file->file);
+		if (fence_state[i].fd >= 0)
+			put_unused_fd(fence_state[i].fd);
+	}
+
+	kfree(fence_state);
+	kfree(out_fences);
+
+	return ERR_PTR(ret);
+}
+
+static void install_out_fence(struct drm_atomic_state *state,
+			      uint64_t count_out_fences,
+			      struct drm_out_fence_state *fence_state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for (i = 0 ; i < count_out_fences ; i++) {
+		if (fence_state[i].sync_file)
+			fd_install(fence_state[i].fd,
+				   fence_state[i].sync_file->file);
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->event->base.fence)
+			fence_get(crtc_state->event->base.fence);
+	}
+}
+
+static void release_out_fence(uint64_t count_out_fences,
+			      struct drm_out_fence_state *state)
+{
+	int i;
+
+	for (i = 0 ; i < count_out_fences ; i++) {
+		if (state->sync_file)
+			fput(state->sync_file->file);
+		if (state->fd >= 0)
+			put_unused_fd(state->fd);
+	}
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1620,12 +1748,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
 	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
 	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
+	uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr);
 	unsigned int copied_objs, copied_props;
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_out_fence_state *fence_state = NULL;
 	unsigned plane_mask;
 	int ret = 0;
 	unsigned int i, j;
@@ -1651,9 +1781,12 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			!dev->mode_config.async_page_flip)
 		return -EINVAL;
 
+	if ((arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) && !arg->count_out_fences)
+		return -EINVAL;
+
 	/* can't test and expect an event at the same time. */
 	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
-			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+			(arg->flags & DRM_MODE_ATOMIC_EVENT_MASK))
 		return -EINVAL;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -1743,12 +1876,11 @@ retry:
 		drm_mode_object_unreference(obj);
 	}
 
-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+	if (arg->flags & DRM_MODE_ATOMIC_EVENT_MASK) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			struct drm_pending_vblank_event *e;
 
-			e = create_vblank_event(dev, file_priv, NULL,
-						arg->user_data);
+			e = create_vblank_event(dev, arg->user_data);
 			if (!e) {
 				ret = -ENOMEM;
 				goto out;
@@ -1758,16 +1890,48 @@ retry:
 		}
 	}
 
+	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			struct drm_pending_vblank_event *e = crtc_state->event;
+
+			if (!file_priv)
+				continue;
+
+			ret = drm_event_reserve_init(dev, file_priv, &e->base,
+						     &e->event.base);
+			if (ret) {
+				kfree(e);
+				crtc_state->event = NULL;
+				goto out;
+			}
+		}
+	}
+
+	if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) {
+		fence_state = get_out_fence(dev, state, out_fences_ptr,
+					    arg->count_out_fences,
+					    arg->user_data);
+		if (IS_ERR(fence_state)) {
+			ret = PTR_ERR(fence_state);
+			goto out;
+		}
+	}
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		/*
 		 * Unlike commit, check_only does not clean up state.
 		 * Below we call drm_atomic_state_free for it.
 		 */
 		ret = drm_atomic_check_only(state);
-	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
-		ret = drm_atomic_nonblocking_commit(state);
 	} else {
-		ret = drm_atomic_commit(state);
+		if (arg->flags & DRM_MODE_ATOMIC_OUT_FENCE)
+			install_out_fence(state, arg->count_out_fences,
+					  fence_state);
+
+		if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK)
+			ret = drm_atomic_nonblocking_commit(state);
+		else
+			ret = drm_atomic_commit(state);
 	}
 
 out:
@@ -1788,6 +1952,14 @@ out:
 		}
 	}
 
+	if ((arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) &&
+	    !IS_ERR_OR_NULL(fence_state)) {
+		if (ret)
+			release_out_fence(arg->count_out_fences, fence_state);
+
+		kfree(fence_state);
+	}
+
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e92fcb2..010a0f9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -656,6 +656,16 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence)
 }
 
 /**
+ * struct drm_out_fence_state - store out_fence data for clean up
+ * @sync_file: sync_file related to the out_fence
+ * @fd: file descriptor to installed on the sync_file.
+ */
+struct drm_out_fence_state {
+	struct sync_file *sync_file;
+	int fd;
+};
+
+/**
  * struct drm_plane_state - mutable plane state
  * @plane: backpointer to the plane
  * @crtc: currently bound CRTC, NULL if disabled
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 49a7265..ce71ad5 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -588,13 +588,24 @@ struct drm_mode_destroy_dumb {
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
+#define DRM_MODE_ATOMIC_OUT_FENCE 0x0800
 
 #define DRM_MODE_ATOMIC_FLAGS (\
 		DRM_MODE_PAGE_FLIP_EVENT |\
 		DRM_MODE_PAGE_FLIP_ASYNC |\
 		DRM_MODE_ATOMIC_TEST_ONLY |\
 		DRM_MODE_ATOMIC_NONBLOCK |\
-		DRM_MODE_ATOMIC_ALLOW_MODESET)
+		DRM_MODE_ATOMIC_ALLOW_MODESET |\
+		DRM_MODE_ATOMIC_OUT_FENCE)
+
+#define DRM_MODE_ATOMIC_EVENT_MASK (\
+		DRM_MODE_PAGE_FLIP_EVENT |\
+		DRM_MODE_ATOMIC_OUT_FENCE)
+
+struct drm_out_fences {
+	__u32	crtc_id;
+	__u32	fd;
+};
 
 struct drm_mode_atomic {
 	__u32 flags;
@@ -605,6 +616,8 @@ struct drm_mode_atomic {
 	__u64 prop_values_ptr;
 	__u64 reserved;
 	__u64 user_data;
+	__u64 count_out_fences;
+	__u64 out_fences_ptr;
 };
 
 /**
-- 
2.5.5

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