[PATCH v7 0/3] drm: add explict fencing

[PATCH v7 0/3] drm: add explict fencing

[Gustavo Padovan]

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

Hi,

This is yet another version of the DRM fences patches. Please refer
to the cover letter[1] in a previous version to check for more details.

In v7 we now have split most of the out_fences code into
prepare_crtc_signaling() and unprepare_crtc_signaling() with improved error
handling. More details on the v7 changes are embedded in each commit's
message.

Robert Foss managed to port Android's drm_hwcomposer to the new HWC2 API and
added support to fences. Current patches can be seen here:

https://git.collabora.com/cgit/user/robertfoss/drm_hwcomposer.git/log/?h=hwc2_fence_v1

He managed to run AOSP on top of padovan/fences kernel branch with full fence
support on qemu/virgl and msm db410c. That means we already have a working
open source userspace using the explicit fencing implementation.

Also i-g-t testing are available at:

https://git.collabora.com/cgit/user/padovan/intel-gpu-tools.git/log/

Please review!

Gustavo

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1253822.html

---
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        | 247 ++++++++++++++++++++++++++++++------
 drivers/gpu/drm/drm_atomic_helper.c |   3 +
 drivers/gpu/drm/drm_crtc.c          |  45 +++++++
 drivers/gpu/drm/drm_plane.c         |   1 +
 include/drm/drm_atomic.h            |   1 +
 include/drm/drm_crtc.h              |  55 ++++++++
 7 files changed, 311 insertions(+), 42 deletions(-)

-- 
2.5.5

[PATCH v7 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 IN_FENCE_FD attached to every plane
state that receives sync_file fds from userspace via the atomic commit
IOCTL.

The fd is then translated to a fence (that may be a fence_array
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

v4: Comment from Maarten Lankhorst
	- allow set fence with no related fb

v5: rename FENCE_FD to IN_FENCE_FD

v6: Comments by Daniel Vetter:
	- rename plane_state->in_fence back to "fence"
	- re-introduce WARN_ON if fence set but no fb

     - rebase after fence -> dma_fence rename

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/Kconfig             |  1 +
 drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  3 +++
 drivers/gpu/drm/drm_crtc.c          |  6 ++++++
 drivers/gpu/drm/drm_plane.c         |  1 +
 include/drm/drm_crtc.h              |  5 +++++
 6 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 483059a..43cb33d 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 5e73954..d1ae463 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"
 
@@ -686,6 +687,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_in_fence_fd) {
+		if (U642I64(val) == -1)
+			return 0;
+
+		if (state->fence)
+			dma_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);
@@ -747,6 +759,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_in_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 75ad01d..26a067f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3114,6 +3114,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fence)
+		dma_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 13441e2..7878bfd 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -397,6 +397,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,
+			"IN_FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_in_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 249c0ae..3957ef8 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -137,6 +137,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_in_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);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index fa1aa21..719b6a8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1201,6 +1201,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_fb_id;
 	/**
+	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
+	 * for a Plane.
+	 */
+	struct drm_property *prop_in_fence_fd;
+	/**
 	 * @prop_crtc_id: Default atomic plane property to specify the
 	 * &drm_crtc.
 	 */
-- 
2.5.5

Re: [PATCH v7 1/3] drm/fence: add in-fences support

[Daniel Vetter]

On Tue, Nov 08, 2016 at 03:54:48PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called IN_FENCE_FD attached to every plane
> state that receives sync_file fds from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_array
> 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
> 
> v4: Comment from Maarten Lankhorst
> 	- allow set fence with no related fb
> 
> v5: rename FENCE_FD to IN_FENCE_FD
> 
> v6: Comments by Daniel Vetter:
> 	- rename plane_state->in_fence back to "fence"
> 	- re-introduce WARN_ON if fence set but no fb
> 
>      - rebase after fence -> dma_fence rename
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/Kconfig             |  1 +
>  drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  3 +++
>  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
>  drivers/gpu/drm/drm_plane.c         |  1 +
>  include/drm/drm_crtc.h              |  5 +++++
>  6 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 483059a..43cb33d 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 5e73954..d1ae463 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"
>  
> @@ -686,6 +687,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_in_fence_fd) {
> +		if (U642I64(val) == -1)
> +			return 0;
> +
> +		if (state->fence)
> +			dma_fence_put(state->fence);
> +
> +		state->fence = sync_file_get_fence(val);
> +		if (!state->fence)
> +			return -EINVAL;

Might have missed it, but your igt only seems to test for -1 (i.e. no
fence), not for an entirely invalid fence. I think we need another
testcase which opens some file (/dev/null maybe) and tries to use that as
fence.

Another corner case is error unrolling. I think we need 3 cases for full
coverage:
- Early error unrolling while setting properties. Best way imo would be 1)
  set an in-fence (correctly), then 2) set an invalid property (just set
  the CRTC plane property to something invalid like ~0ULL).
- Late unrolling (i.e. atomic_check fails). I think the reliable way here
  to provoke this on all drivers would be: Atomic commit with in-fence and
  otherwise everything the same (i.e. known to work), but changing the
  crtc's ACTIVE property from false to true without setting ALLOW_MODESET.
- And then also a successful TEST_ONLY commit.

Same is required for out-fences ofc too.

Patch itself looks good, and I couldn't poke any holes into it.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	} 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);
> @@ -747,6 +759,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_in_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 75ad01d..26a067f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3114,6 +3114,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence)
> +		dma_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 13441e2..7878bfd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -397,6 +397,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,
> +			"IN_FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_in_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..3957ef8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -137,6 +137,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_in_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);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index fa1aa21..719b6a8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1201,6 +1201,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_fb_id;
>  	/**
> +	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
> +	 * for a Plane.
> +	 */
> +	struct drm_property *prop_in_fence_fd;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 1/3] drm/fence: add in-fences support

[Brian Starkey]

Hi Gustavo,

On Tue, Nov 08, 2016 at 03:54:48PM +0900, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>There is now a new property called IN_FENCE_FD attached to every plane
>state that receives sync_file fds from userspace via the atomic commit
>IOCTL.
>
>The fd is then translated to a fence (that may be a fence_array
>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
>
>v4: Comment from Maarten Lankhorst
>	- allow set fence with no related fb
>
>v5: rename FENCE_FD to IN_FENCE_FD
>
>v6: Comments by Daniel Vetter:
>	- rename plane_state->in_fence back to "fence"
>	- re-introduce WARN_ON if fence set but no fb
>
>     - rebase after fence -> dma_fence rename
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>---
> drivers/gpu/drm/Kconfig             |  1 +
> drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
> drivers/gpu/drm/drm_atomic_helper.c |  3 +++
> drivers/gpu/drm/drm_crtc.c          |  6 ++++++
> drivers/gpu/drm/drm_plane.c         |  1 +
> include/drm/drm_crtc.h              |  5 +++++
> 6 files changed, 30 insertions(+)
>
>diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>index 483059a..43cb33d 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 5e73954..d1ae463 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"
>
>@@ -686,6 +687,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_in_fence_fd) {
>+		if (U642I64(val) == -1)
>+			return 0;
>+

Sorry, I'm a bit late to bring this up but what's the expected
behaviour of a commit which sets OUT_FENCE twice (first with a valid
fence, then with -1 later in the list)? Maybe you need to actually
clear state->fence for -1.

>+		if (state->fence)
>+			dma_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);
>@@ -747,6 +759,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_in_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 75ad01d..26a067f 100644
>--- a/drivers/gpu/drm/drm_atomic_helper.c
>+++ b/drivers/gpu/drm/drm_atomic_helper.c
>@@ -3114,6 +3114,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> {
> 	if (state->fb)
> 		drm_framebuffer_unreference(state->fb);
>+
>+	if (state->fence)
>+		dma_fence_put(state->fence);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>

It looks like you need to add something in
__drm_atomic_helper_plane_duplicate_state() to either get a reference
on the fence or set state->fence = NULL, because the memcpy() there
will copy the pointer.

Cheers,
-Brian

>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>index 13441e2..7878bfd 100644
>--- a/drivers/gpu/drm/drm_crtc.c
>+++ b/drivers/gpu/drm/drm_crtc.c
>@@ -397,6 +397,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,
>+			"IN_FENCE_FD", -1, INT_MAX);
>+	if (!prop)
>+		return -ENOMEM;
>+	dev->mode_config.prop_in_fence_fd = prop;
>+
> 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> 	if (!prop)
>diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>index 249c0ae..3957ef8 100644
>--- a/drivers/gpu/drm/drm_plane.c
>+++ b/drivers/gpu/drm/drm_plane.c
>@@ -137,6 +137,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_in_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);
>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>index fa1aa21..719b6a8 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -1201,6 +1201,11 @@ struct drm_mode_config {
> 	 */
> 	struct drm_property *prop_fb_id;
> 	/**
>+	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
>+	 * for a Plane.
>+	 */
>+	struct drm_property *prop_in_fence_fd;
>+	/**
> 	 * @prop_crtc_id: Default atomic plane property to specify the
> 	 * &drm_crtc.
> 	 */
>-- 
>2.5.5
>

Re: [PATCH v7 1/3] drm/fence: add in-fences support

[Daniel Vetter]

On Tue, Nov 08, 2016 at 03:27:14PM +0000, Brian Starkey wrote:
> Hi Gustavo,
> 
> On Tue, Nov 08, 2016 at 03:54:48PM +0900, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > There is now a new property called IN_FENCE_FD attached to every plane
> > state that receives sync_file fds from userspace via the atomic commit
> > IOCTL.
> > 
> > The fd is then translated to a fence (that may be a fence_array
> > 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
> > 
> > v4: Comment from Maarten Lankhorst
> > 	- allow set fence with no related fb
> > 
> > v5: rename FENCE_FD to IN_FENCE_FD
> > 
> > v6: Comments by Daniel Vetter:
> > 	- rename plane_state->in_fence back to "fence"
> > 	- re-introduce WARN_ON if fence set but no fb
> > 
> >     - rebase after fence -> dma_fence rename
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > drivers/gpu/drm/Kconfig             |  1 +
> > drivers/gpu/drm/drm_atomic.c        | 14 ++++++++++++++
> > drivers/gpu/drm/drm_atomic_helper.c |  3 +++
> > drivers/gpu/drm/drm_crtc.c          |  6 ++++++
> > drivers/gpu/drm/drm_plane.c         |  1 +
> > include/drm/drm_crtc.h              |  5 +++++
> > 6 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 483059a..43cb33d 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 5e73954..d1ae463 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"
> > 
> > @@ -686,6 +687,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_in_fence_fd) {
> > +		if (U642I64(val) == -1)
> > +			return 0;
> > +
> 
> Sorry, I'm a bit late to bring this up but what's the expected
> behaviour of a commit which sets OUT_FENCE twice (first with a valid
> fence, then with -1 later in the list)? Maybe you need to actually
> clear state->fence for -1.

Good point, we should probaly reject that. Since ->fence should autoclear
(I totally missed that one) we should check for an existing fence and fail
if it's set already.

> > +		if (state->fence)
> > +			dma_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);
> > @@ -747,6 +759,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_in_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 75ad01d..26a067f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3114,6 +3114,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
> > {
> > 	if (state->fb)
> > 		drm_framebuffer_unreference(state->fb);
> > +
> > +	if (state->fence)
> > +		dma_fence_put(state->fence);
> > }
> > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
> > 
> 
> It looks like you need to add something in
> __drm_atomic_helper_plane_duplicate_state() to either get a reference
> on the fence or set state->fence = NULL, because the memcpy() there
> will copy the pointer.

Clearing to NULL would be best, since that matches the meaning of the -1
we return to userspace when reading the property. I guess I get to retract
my r-b.
-Daniel

> 
> Cheers,
> -Brian
> 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 13441e2..7878bfd 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -397,6 +397,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,
> > +			"IN_FENCE_FD", -1, INT_MAX);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.prop_in_fence_fd = prop;
> > +
> > 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> > 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> > 	if (!prop)
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 249c0ae..3957ef8 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -137,6 +137,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_in_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);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index fa1aa21..719b6a8 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1201,6 +1201,11 @@ struct drm_mode_config {
> > 	 */
> > 	struct drm_property *prop_fb_id;
> > 	/**
> > +	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
> > +	 * for a Plane.
> > +	 */
> > +	struct drm_property *prop_in_fence_fd;
> > +	/**
> > 	 * @prop_crtc_id: Default atomic plane property to specify the
> > 	 * &drm_crtc.
> > 	 */
> > -- 
> > 2.5.5
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v7 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

v4: Comments by Brian Starkey
	- Use even more meaninful name for the crtc timeline
	- add doc for timeline_name
    Comment by Daniel Vetter
	- use in-line style for comments

    - rebase after fence -> dma_fence rename

v5: Comment by Daniel Vetter
	- Add doc for drm_crtc_fence_ops

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7878bfd..e2a06c8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc)
 #endif
 }
 
+static const char *drm_crtc_fence_get_driver_name(struct dma_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 dma_fence *fence)
+{
+	struct drm_crtc *crtc = fence_to_crtc(fence);
+
+	return crtc->timeline_name;
+}
+
+static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence)
+{
+	return true;
+}
+
+const struct dma_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 = dma_fence_default_wait,
+};
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return -ENOMEM;
 	}
 
+	crtc->fence_context = dma_fence_context_alloc(1);
+	spin_lock_init(&crtc->fence_lock);
+	snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
+		 "CRTC:%d-%s", crtc->base.id, crtc->name);
+
 	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 719b6a8..30f2401 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/dma-fence.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
 #include <drm/drm_modeset_lock.h>
@@ -726,9 +728,52 @@ struct drm_crtc {
 	 */
 	struct drm_crtc_crc crc;
 #endif
+
+	/**
+	 * @fence_context:
+	 *
+	 * timeline context used for fence operations.
+	 */
+	unsigned int fence_context;
+
+	/**
+	 * @fence_lock:
+	 *
+	 * spinlock to protect the fences in the fence_context.
+	 */
+
+	spinlock_t fence_lock;
+	/**
+	 * @fence_seqno:
+	 *
+	 * Seqno variable used as monotonic counter for the fences
+	 * created on the CRTC's timeline.
+	 */
+	unsigned long fence_seqno;
+
+	/**
+	 * @timeline_name:
+	 *
+	 * The name of the CRTC's fence timeline.
+	 */
+	char timeline_name[32];
 };
 
 /**
+ * dma_crtc_fence_ops - fence ops for the drm_crtc timeline
+ *
+ * It contains the dma_fence_ops that should be called by the dma_fence
+ * code. CRTC core should use this ops when initializing fences.
+ */
+extern const struct dma_fence_ops drm_crtc_fence_ops;
+
+static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
+{
+	BUG_ON(fence->ops != &drm_crtc_fence_ops);
+	return container_of(fence->lock, struct drm_crtc, fence_lock);
+}
+
+/**
  * struct drm_mode_set - new values for a CRTC config change
  * @fb: framebuffer to use for new config
  * @crtc: CRTC whose configuration we're about to change
-- 
2.5.5

[PATCH v7 3/3] drm/fence: add out-fences support

[Gustavo Padovan]

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

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

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.

v4: Create OUT_FENCE_PTR properties and remove old approach.

v5: Comments by Brian Starkey:
	- Remove extra fence_get() in atomic_ioctl()
	- Check ret before iterating on the crtc_state
	- check ret before fd_install
	- set fence_state to NULL at the beginning
	- check fence_state->out_fence_ptr before put_user()
	- change order of fput() and put_unused_fd() on failure

     - Add access_ok() check to the out_fence_ptr received
     - Rebase after fence -> dma_fence rename
     - Store out_fence_ptr in the drm_atomic_state
     - Split crtc_setup_out_fence()
     - return -1 as out_fence with TEST_ONLY flag

v6: Comments by Daniel Vetter
	- Add prepare/unprepare_crtc_signaling()
	- move struct drm_out_fence_state to drm_atomic.c
	- mark get_crtc_fence() as static

    Comments by Brian Starkey
	- proper set fence_ptr fence_state array
	- isolate fence_idx increment

    - improve error handling

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 233 +++++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/drm_crtc.c   |   8 ++
 include/drm/drm_atomic.h     |   1 +
 include/drm/drm_crtc.h       |   7 +-
 4 files changed, 206 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d1ae463..9a7d84c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -289,6 +289,25 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 
+static void
+drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
+				  struct drm_crtc *crtc, u64 __user *fence_ptr)
+{
+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
+}
+
+static u64 __user *
+drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
+			     struct drm_crtc *crtc)
+{
+	u64 __user *fence_ptr;
+
+	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
+	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
+
+	return fence_ptr;
+}
+
 /**
  * drm_atomic_set_mode_for_crtc - set mode for CRTC
  * @state: the CRTC whose incoming state to update
@@ -490,6 +509,12 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 					&replaced);
 		state->color_mgmt_changed |= replaced;
 		return ret;
+	} else if (property == config->prop_out_fence_ptr) {
+		uint64_t __user *fence_ptr = u64_to_user_ptr(val);
+		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
+			return -EFAULT;
+
+		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
 	} else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	else
@@ -532,6 +557,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->ctm) ? state->ctm->base.id : 0;
 	else if (property == config->gamma_lut_property)
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+	else if (property == config->prop_out_fence_ptr)
+		*val = 0;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
@@ -1506,11 +1533,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 dma_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)
@@ -1520,17 +1545,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;
 }
 
@@ -1635,6 +1649,148 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_clean_old_fb);
 
+static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
+					struct drm_crtc_state *crtc_state)
+{
+	struct dma_fence *fence;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return NULL;
+
+	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
+		       crtc->fence_context, ++crtc->fence_seqno);
+
+	return fence;
+}
+
+struct drm_out_fence_state {
+	u64 __user *out_fence_ptr;
+	struct sync_file *sync_file;
+	int fd;
+};
+
+static int setup_out_fence(struct drm_out_fence_state *fence_state,
+			   struct dma_fence *fence)
+{
+	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fence_state->fd < 0)
+		return fence_state->fd;
+
+	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+		return -EFAULT;
+
+	fence_state->sync_file = sync_file_create(fence);
+	if(!fence_state->sync_file)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int prepare_crtc_signaling(struct drm_device *dev,
+				  struct drm_atomic_state *state,
+				  struct drm_mode_atomic *arg,
+				  struct drm_file *file_priv,
+				  struct drm_out_fence_state *fence_state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int i, ret, fence_idx = 0;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		u64 __user *fence_ptr;
+
+		fence_ptr = drm_atomic_get_out_fence_for_crtc(crtc_state->state,
+							      crtc);
+
+		if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY && fence_ptr) {
+			if (put_user(-1, fence_ptr))
+				return -EFAULT;
+
+			continue;
+		}
+
+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
+			struct drm_pending_vblank_event *e;
+
+			e = create_vblank_event(dev, arg->user_data);
+			if (!e)
+				return -ENOMEM;
+
+			crtc_state->event = e;
+		}
+
+		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+			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;
+				return ret;
+			}
+		}
+
+		if (fence_ptr) {
+			struct dma_fence *fence;
+
+			fence_state[fence_idx].out_fence_ptr = fence_ptr;
+
+			fence = get_crtc_fence(crtc, crtc_state);
+			if (!fence)
+				return -ENOMEM;
+
+			ret = setup_out_fence(&fence_state[fence_idx], fence);
+			if (ret) {
+				dma_fence_put(fence);
+				return ret;
+			}
+
+			fence_idx++;
+
+			crtc_state->event->base.fence = fence;
+		}
+	}
+
+	return 0;
+}
+
+static void unprepare_crtc_signaling(struct drm_device *dev,
+				     struct drm_atomic_state *state,
+				     struct drm_out_fence_state *fence_state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
+		 * exclusive, if they weren't, this code should be
+		 * called on success for TEST_ONLY too.
+		 */
+		if (crtc_state->event)
+			drm_event_cancel_free(dev,
+					      &crtc_state->event->base);
+	}
+
+	for (i = 0; fence_state[i].out_fence_ptr; 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);
+
+		/* If this fails log error to the user */
+		if (fence_state[i].out_fence_ptr &&
+		    put_user(-1, fence_state[i].out_fence_ptr))
+			DRM_INFO("Couldn't clear out_fence_ptr\n");
+	}
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1647,8 +1803,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	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;
@@ -1766,21 +1921,18 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		drm_mode_object_unreference(obj);
 	}
 
-	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		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);
-			if (!e) {
-				ret = -ENOMEM;
-				goto out;
-			}
-
-			crtc_state->event = e;
-		}
+	fence_state = kzalloc(dev->mode_config.num_crtc * sizeof(*fence_state),
+			      GFP_KERNEL);
+	if (!fence_state) {
+		ret = -ENOMEM;
+		goto out;
 	}
 
+	ret = prepare_crtc_signaling(dev, state, arg, file_priv,
+					  fence_state);
+	if (ret)
+		goto out;
+
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
 		/*
 		 * Unlike commit, check_only does not clean up state.
@@ -1793,23 +1945,20 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		ret = drm_atomic_commit(state);
 	}
 
+	if (ret)
+		goto out;
+
+	for (i = 0; fence_state[i].sync_file && i < dev->mode_config.num_crtc;
+	     i++)
+		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
+
 out:
 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
-	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-		/*
-		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
-		 * if they weren't, this code should be called on success
-		 * for TEST_ONLY too.
-		 */
-
-		for_each_crtc_in_state(state, crtc, crtc_state, i) {
-			if (!crtc_state->event)
-				continue;
+	if (ret && fence_state)
+		unprepare_crtc_signaling(dev, state, fence_state);
 
-			drm_event_cancel_free(dev, &crtc_state->event->base);
-		}
-	}
+	kfree(fence_state);
 
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e2a06c8..a18d81b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
 		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
+		drm_object_attach_property(&crtc->base,
+					   config->prop_out_fence_ptr, 0);
 	}
 
 	return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_in_fence_fd = prop;
 
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
+			"OUT_FENCE_PTR", 0, U64_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_out_fence_ptr = 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_atomic.h b/include/drm/drm_atomic.h
index 2d1e9c9..ebde60e 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,6 +144,7 @@ struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state;
 	struct drm_crtc_commit *commit;
+	u64 __user *out_fence_ptr;
 };
 
 struct __drm_connnectors_state {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 30f2401..a5a05e8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -773,7 +773,7 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
 	return container_of(fence->lock, struct drm_crtc, fence_lock);
 }
 
-/**
+/*
  * struct drm_mode_set - new values for a CRTC config change
  * @fb: framebuffer to use for new config
  * @crtc: CRTC whose configuration we're about to change
@@ -1251,6 +1251,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *prop_in_fence_fd;
 	/**
+	 * @prop_out_fence_ptr: Sync File fd pointer representing the
+	 * outgoing fences for a CRTC. Userspace should provide a u64 pointer.
+	 */
+	struct drm_property *prop_out_fence_ptr;
+	/**
 	 * @prop_crtc_id: Default atomic plane property to specify the
 	 * &drm_crtc.
 	 */
-- 
2.5.5

Re: [PATCH v7 3/3] drm/fence: add out-fences support

[Daniel Vetter]

On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.
> 
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
> 
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
> 
> 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.
> 
> v4: Create OUT_FENCE_PTR properties and remove old approach.
> 
> v5: Comments by Brian Starkey:
> 	- Remove extra fence_get() in atomic_ioctl()
> 	- Check ret before iterating on the crtc_state
> 	- check ret before fd_install
> 	- set fence_state to NULL at the beginning
> 	- check fence_state->out_fence_ptr before put_user()
> 	- change order of fput() and put_unused_fd() on failure
> 
>      - Add access_ok() check to the out_fence_ptr received
>      - Rebase after fence -> dma_fence rename
>      - Store out_fence_ptr in the drm_atomic_state
>      - Split crtc_setup_out_fence()
>      - return -1 as out_fence with TEST_ONLY flag
> 
> v6: Comments by Daniel Vetter
> 	- Add prepare/unprepare_crtc_signaling()
> 	- move struct drm_out_fence_state to drm_atomic.c
> 	- mark get_crtc_fence() as static
> 
>     Comments by Brian Starkey
> 	- proper set fence_ptr fence_state array
> 	- isolate fence_idx increment
> 
>     - improve error handling
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Found a bunch more nitpicks, and wishlist items for igt coverage.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 233 +++++++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_crtc.c   |   8 ++
>  include/drm/drm_atomic.h     |   1 +
>  include/drm/drm_crtc.h       |   7 +-
>  4 files changed, 206 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d1ae463..9a7d84c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -289,6 +289,25 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
> +static void
> +drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
> +				  struct drm_crtc *crtc, u64 __user *fence_ptr)
> +{
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> +}
> +
> +static u64 __user *
> +drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
> +			     struct drm_crtc *crtc)

Since these two are static I'd drop the long prefix to make the code
easier to read.

> +{
> +	u64 __user *fence_ptr;
> +
> +	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
> +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
> +
> +	return fence_ptr;
> +}
> +
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
> @@ -490,6 +509,12 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  					&replaced);
>  		state->color_mgmt_changed |= replaced;
>  		return ret;
> +	} else if (property == config->prop_out_fence_ptr) {
> +		uint64_t __user *fence_ptr = u64_to_user_ptr(val);

Don't we need to filter out NULL/0 here like we filter out -1 for the
in-fences? Just in case some userspace samples/restores all properties.

Sounds like an igt is missing for this ...

Maybe we even need a special igt which samples _all_ current atomic
properties, and then does an atomic commit which changes none of them
(without setting the ALLOW_MODESET flag ofc). That /should/ work, and
would catch such bugs in the future.

> +		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
> +			return -EFAULT;

Same comment about igt coverage I made for patch 1, but with
s/in-fence/out-fence/, and s/~0ULL/8/. I picked 8 as an invalid address !=
NULL.

And the testcase need to cover all possible combinations of output event
generation, i.e. out-fence, event and out-fence+event. So 3x3=9 testcases
for this I think.

> +
> +		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>  	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	else
> @@ -532,6 +557,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = (state->ctm) ? state->ctm->base.id : 0;
>  	else if (property == config->gamma_lut_property)
>  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +	else if (property == config->prop_out_fence_ptr)
> +		*val = 0;
>  	else if (crtc->funcs->atomic_get_property)
>  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>  	else
> @@ -1506,11 +1533,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 dma_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)
> @@ -1520,17 +1545,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;
>  }
>  
> @@ -1635,6 +1649,148 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
> +					struct drm_crtc_state *crtc_state)
> +{
> +	struct dma_fence *fence;
> +
> +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +	if (!fence)
> +		return NULL;
> +
> +	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> +		       crtc->fence_context, ++crtc->fence_seqno);
> +
> +	return fence;
> +}
> +
> +struct drm_out_fence_state {
> +	u64 __user *out_fence_ptr;

You have a type mess here. The pointer is to an u64, but the value you
store there is an int. Since we want to avoid fund with 32bit userspace
vs. 64bit kernel I think it should be an s64 everywhere.
> +	struct sync_file *sync_file;
> +	int fd;
> +};
> +
> +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> +			   struct dma_fence *fence)
> +{
> +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fence_state->fd < 0)
> +		return fence_state->fd;
> +
> +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +		return -EFAULT;
> +
> +	fence_state->sync_file = sync_file_create(fence);
> +	if(!fence_state->sync_file)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static int prepare_crtc_signaling(struct drm_device *dev,
> +				  struct drm_atomic_state *state,
> +				  struct drm_mode_atomic *arg,
> +				  struct drm_file *file_priv,
> +				  struct drm_out_fence_state *fence_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int i, ret, fence_idx = 0;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		u64 __user *fence_ptr;
> +
> +		fence_ptr = drm_atomic_get_out_fence_for_crtc(crtc_state->state,
> +							      crtc);
> +
> +		if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY && fence_ptr) {
> +			if (put_user(-1, fence_ptr))
> +				return -EFAULT;
> +
> +			continue;
> +		}
> +
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> +			struct drm_pending_vblank_event *e;
> +
> +			e = create_vblank_event(dev, arg->user_data);
> +			if (!e)
> +				return -ENOMEM;
> +
> +			crtc_state->event = e;
> +		}
> +
> +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +			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;
> +				return ret;
> +			}
> +		}
> +
> +		if (fence_ptr) {
> +			struct dma_fence *fence;
> +
> +			fence_state[fence_idx].out_fence_ptr = fence_ptr;
> +
> +			fence = get_crtc_fence(crtc, crtc_state);
> +			if (!fence)
> +				return -ENOMEM;
> +
> +			ret = setup_out_fence(&fence_state[fence_idx], fence);
> +			if (ret) {
> +				dma_fence_put(fence);
> +				return ret;
> +			}
> +
> +			fence_idx++;
> +
> +			crtc_state->event->base.fence = fence;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void unprepare_crtc_signaling(struct drm_device *dev,
> +				     struct drm_atomic_state *state,
> +				     struct drm_out_fence_state *fence_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		/*
> +		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
> +		 * exclusive, if they weren't, this code should be
> +		 * called on success for TEST_ONLY too.
> +		 */
> +		if (crtc_state->event)
> +			drm_event_cancel_free(dev,
> +					      &crtc_state->event->base);
> +	}
> +
> +	for (i = 0; fence_state[i].out_fence_ptr; i++) {

This goes boom if you have fences set for every crtc, because then this
check will walk past the end of the array and do something undefined. You
need to manually count how many of these slots are set (and might want to
switch to a krealloc pattern while at it). Sounds like it needs an igt.

> +		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);
> +
> +		/* If this fails log error to the user */
> +		if (fence_state[i].out_fence_ptr &&
> +		    put_user(-1, fence_state[i].out_fence_ptr))
> +			DRM_INFO("Couldn't clear out_fence_ptr\n");

Userspace can provoke this (race an munmap against an atomic ioctl), and
userspace is not allowed to spam dmesg. Please degrade to
DRM_DEBUG_ATOMIC.

> +	}
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1647,8 +1803,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	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;
> @@ -1766,21 +1921,18 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		drm_mode_object_unreference(obj);
>  	}
>  
> -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		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);
> -			if (!e) {
> -				ret = -ENOMEM;
> -				goto out;
> -			}
> -
> -			crtc_state->event = e;
> -		}
> +	fence_state = kzalloc(dev->mode_config.num_crtc * sizeof(*fence_state),
> +			      GFP_KERNEL);
> +	if (!fence_state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
>  
> +	ret = prepare_crtc_signaling(dev, state, arg, file_priv,
> +					  fence_state);

Personally I'd pass &fence_state here and only krealloc within prepare if
needed. And as mentioned you need an &num_fences here too.

> +	if (ret)
> +		goto out;
> +
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
>  		/*
>  		 * Unlike commit, check_only does not clean up state.
> @@ -1793,23 +1945,20 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		ret = drm_atomic_commit(state);
>  	}
>  
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; fence_state[i].sync_file && i < dev->mode_config.num_crtc;
> +	     i++)
> +		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
> +
>  out:
>  	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  
> -	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> -		/*
> -		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> -		 * if they weren't, this code should be called on success
> -		 * for TEST_ONLY too.
> -		 */
> -
> -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			if (!crtc_state->event)
> -				continue;
> +	if (ret && fence_state)
> +		unprepare_crtc_signaling(dev, state, fence_state);

I'm tempted to rework this to:

	complete_crtc_signalling(dev, state, fence_state, ret)

and push the check for fence_state == NULL into it. And also use ret to
decide whether we need to clean up, or fd_install the fence. That way
everything is in one place. That would also tidy up the control flow a
bit. And s/unprepare/complete since it wouldn't just be the error path
cleanup any more.

>  
> -			drm_event_cancel_free(dev, &crtc_state->event->base);
> -		}
> -	}
> +	kfree(fence_state);
>  
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e2a06c8..a18d81b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&crtc->base, config->prop_active, 0);
>  		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
> +		drm_object_attach_property(&crtc->base,
> +					   config->prop_out_fence_ptr, 0);
>  	}
>  
>  	return 0;
> @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_in_fence_fd = prop;
>  
> +	prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"OUT_FENCE_PTR", 0, U64_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_out_fence_ptr = 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_atomic.h b/include/drm/drm_atomic.h
> index 2d1e9c9..ebde60e 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,6 +144,7 @@ struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state;
>  	struct drm_crtc_commit *commit;
> +	u64 __user *out_fence_ptr;

Same confusion about pointer types here as in fence_state.

>  };
>  
>  struct __drm_connnectors_state {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 30f2401..a5a05e8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -773,7 +773,7 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_fence *fence)
>  	return container_of(fence->lock, struct drm_crtc, fence_lock);
>  }
>  
> -/**
> +/*

Bogus hunk I think here, this breaks the kerneldoc.

>   * struct drm_mode_set - new values for a CRTC config change
>   * @fb: framebuffer to use for new config
>   * @crtc: CRTC whose configuration we're about to change
> @@ -1251,6 +1251,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_in_fence_fd;
>  	/**
> +	 * @prop_out_fence_ptr: Sync File fd pointer representing the
> +	 * outgoing fences for a CRTC. Userspace should provide a u64 pointer.

"... should provide a pointer to a value of type s64, and then cast that
pointer to u64." The type of the value pointed to and the pointer itself
aren't the same!

> +	 */
> +	struct drm_property *prop_out_fence_ptr;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 3/3] drm/fence: add out-fences support

[Gustavo Padovan]

2016-11-08 Daniel Vetter <daniel@ffwll.ch>:

> On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> > 
> > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> > the sync_file fd back to userspace.
> > 
> > The sync_file and fd are allocated/created before commit, but the
> > fd_install operation only happens after we know that commit succeed.
> > 
> > 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.
> > 
> > v4: Create OUT_FENCE_PTR properties and remove old approach.
> > 
> > v5: Comments by Brian Starkey:
> > 	- Remove extra fence_get() in atomic_ioctl()
> > 	- Check ret before iterating on the crtc_state
> > 	- check ret before fd_install
> > 	- set fence_state to NULL at the beginning
> > 	- check fence_state->out_fence_ptr before put_user()
> > 	- change order of fput() and put_unused_fd() on failure
> > 
> >      - Add access_ok() check to the out_fence_ptr received
> >      - Rebase after fence -> dma_fence rename
> >      - Store out_fence_ptr in the drm_atomic_state
> >      - Split crtc_setup_out_fence()
> >      - return -1 as out_fence with TEST_ONLY flag
> > 
> > v6: Comments by Daniel Vetter
> > 	- Add prepare/unprepare_crtc_signaling()
> > 	- move struct drm_out_fence_state to drm_atomic.c
> > 	- mark get_crtc_fence() as static
> > 
> >     Comments by Brian Starkey
> > 	- proper set fence_ptr fence_state array
> > 	- isolate fence_idx increment
> > 
> >     - improve error handling
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Found a bunch more nitpicks, and wishlist items for igt coverage.

Thank you, I've collected the list of the igt tests and will work on
them.


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 233 +++++++++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/drm_crtc.c   |   8 ++
> >  include/drm/drm_atomic.h     |   1 +
> >  include/drm/drm_crtc.h       |   7 +-
> >  4 files changed, 206 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index d1ae463..9a7d84c 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -289,6 +289,25 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
> >  
> > +static void
> > +drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,
> > +				  struct drm_crtc *crtc, u64 __user *fence_ptr)
> > +{
> > +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;
> > +}
> > +
> > +static u64 __user *
> > +drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,
> > +			     struct drm_crtc *crtc)
> 
> Since these two are static I'd drop the long prefix to make the code
> easier to read.
> 
> > +{
> > +	u64 __user *fence_ptr;
> > +
> > +	fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
> > +	state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
> > +
> > +	return fence_ptr;
> > +}
> > +
> >  /**
> >   * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >   * @state: the CRTC whose incoming state to update
> > @@ -490,6 +509,12 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  					&replaced);
> >  		state->color_mgmt_changed |= replaced;
> >  		return ret;
> > +	} else if (property == config->prop_out_fence_ptr) {
> > +		uint64_t __user *fence_ptr = u64_to_user_ptr(val);
> 
> Don't we need to filter out NULL/0 here like we filter out -1 for the
> in-fences? Just in case some userspace samples/restores all properties.
> 
> Sounds like an igt is missing for this ...
> 
> Maybe we even need a special igt which samples _all_ current atomic
> properties, and then does an atomic commit which changes none of them
> (without setting the ALLOW_MODESET flag ofc). That /should/ work, and
> would catch such bugs in the future.
> 
> > +		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
> > +			return -EFAULT;
> 
> Same comment about igt coverage I made for patch 1, but with
> s/in-fence/out-fence/, and s/~0ULL/8/. I picked 8 as an invalid address !=
> NULL.
> 
> And the testcase need to cover all possible combinations of output event
> generation, i.e. out-fence, event and out-fence+event. So 3x3=9 testcases
> for this I think.

out-fence and event. so 2x2=4 ;)

> 
> > +
> > +		drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
> >  	} else if (crtc->funcs->atomic_set_property)
> >  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> >  	else
> > @@ -532,6 +557,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >  		*val = (state->ctm) ? state->ctm->base.id : 0;
> >  	else if (property == config->gamma_lut_property)
> >  		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +	else if (property == config->prop_out_fence_ptr)
> > +		*val = 0;
> >  	else if (crtc->funcs->atomic_get_property)
> >  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> >  	else
> > @@ -1506,11 +1533,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 dma_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)
> > @@ -1520,17 +1545,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;
> >  }
> >  
> > @@ -1635,6 +1649,148 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> >  
> > +static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
> > +					struct drm_crtc_state *crtc_state)
> > +{
> > +	struct dma_fence *fence;
> > +
> > +	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> > +	if (!fence)
> > +		return NULL;
> > +
> > +	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
> > +		       crtc->fence_context, ++crtc->fence_seqno);
> > +
> > +	return fence;
> > +}
> > +
> > +struct drm_out_fence_state {
> > +	u64 __user *out_fence_ptr;
> 
> You have a type mess here. The pointer is to an u64, but the value you
> store there is an int. Since we want to avoid fund with 32bit userspace
> vs. 64bit kernel I think it should be an s64 everywhere.
> > +	struct sync_file *sync_file;
> > +	int fd;
> > +};
> > +
> > +static int setup_out_fence(struct drm_out_fence_state *fence_state,
> > +			   struct dma_fence *fence)
> > +{
> > +	fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> > +	if (fence_state->fd < 0)
> > +		return fence_state->fd;
> > +
> > +	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> > +		return -EFAULT;
> > +
> > +	fence_state->sync_file = sync_file_create(fence);
> > +	if(!fence_state->sync_file)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +
> > +static int prepare_crtc_signaling(struct drm_device *dev,
> > +				  struct drm_atomic_state *state,
> > +				  struct drm_mode_atomic *arg,
> > +				  struct drm_file *file_priv,
> > +				  struct drm_out_fence_state *fence_state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int i, ret, fence_idx = 0;
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		u64 __user *fence_ptr;
> > +
> > +		fence_ptr = drm_atomic_get_out_fence_for_crtc(crtc_state->state,
> > +							      crtc);
> > +
> > +		if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY && fence_ptr) {
> > +			if (put_user(-1, fence_ptr))
> > +				return -EFAULT;
> > +
> > +			continue;
> > +		}
> > +
> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> > +			struct drm_pending_vblank_event *e;
> > +
> > +			e = create_vblank_event(dev, arg->user_data);
> > +			if (!e)
> > +				return -ENOMEM;
> > +
> > +			crtc_state->event = e;
> > +		}
> > +
> > +		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > +			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;
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		if (fence_ptr) {
> > +			struct dma_fence *fence;
> > +
> > +			fence_state[fence_idx].out_fence_ptr = fence_ptr;
> > +
> > +			fence = get_crtc_fence(crtc, crtc_state);
> > +			if (!fence)
> > +				return -ENOMEM;
> > +
> > +			ret = setup_out_fence(&fence_state[fence_idx], fence);
> > +			if (ret) {
> > +				dma_fence_put(fence);
> > +				return ret;
> > +			}
> > +
> > +			fence_idx++;
> > +
> > +			crtc_state->event->base.fence = fence;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void unprepare_crtc_signaling(struct drm_device *dev,
> > +				     struct drm_atomic_state *state,
> > +				     struct drm_out_fence_state *fence_state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> > +	int i;
> > +
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > +		/*
> > +		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
> > +		 * exclusive, if they weren't, this code should be
> > +		 * called on success for TEST_ONLY too.
> > +		 */
> > +		if (crtc_state->event)
> > +			drm_event_cancel_free(dev,
> > +					      &crtc_state->event->base);
> > +	}
> > +
> > +	for (i = 0; fence_state[i].out_fence_ptr; i++) {
> 
> This goes boom if you have fences set for every crtc, because then this
> check will walk past the end of the array and do something undefined. You
> need to manually count how many of these slots are set (and might want to
> switch to a krealloc pattern while at it). Sounds like it needs an igt.

On the fd_install loop I was also checking for i <
dev->mode_config.num_crtcs but forgot to add that here. However having a
num_fences is a better solution, I'll add that.

> 
> > +		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);
> > +
> > +		/* If this fails log error to the user */
> > +		if (fence_state[i].out_fence_ptr &&
> > +		    put_user(-1, fence_state[i].out_fence_ptr))
> > +			DRM_INFO("Couldn't clear out_fence_ptr\n");
> 
> Userspace can provoke this (race an munmap against an atomic ioctl), and
> userspace is not allowed to spam dmesg. Please degrade to
> DRM_DEBUG_ATOMIC.
> 
> > +	}
> > +}
> > +
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv)
> >  {
> > @@ -1647,8 +1803,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  	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;
> > @@ -1766,21 +1921,18 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  		drm_mode_object_unreference(obj);
> >  	}
> >  
> > -	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > -		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);
> > -			if (!e) {
> > -				ret = -ENOMEM;
> > -				goto out;
> > -			}
> > -
> > -			crtc_state->event = e;
> > -		}
> > +	fence_state = kzalloc(dev->mode_config.num_crtc * sizeof(*fence_state),
> > +			      GFP_KERNEL);
> > +	if (!fence_state) {
> > +		ret = -ENOMEM;
> > +		goto out;
> >  	}
> >  
> > +	ret = prepare_crtc_signaling(dev, state, arg, file_priv,
> > +					  fence_state);
> 
> Personally I'd pass &fence_state here and only krealloc within prepare if
> needed. And as mentioned you need an &num_fences here too.
> 
> > +	if (ret)
> > +		goto out;
> > +
> >  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> >  		/*
> >  		 * Unlike commit, check_only does not clean up state.
> > @@ -1793,23 +1945,20 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  		ret = drm_atomic_commit(state);
> >  	}
> >  
> > +	if (ret)
> > +		goto out;
> > +
> > +	for (i = 0; fence_state[i].sync_file && i < dev->mode_config.num_crtc;
> > +	     i++)
> > +		fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
> > +
> >  out:
> >  	drm_atomic_clean_old_fb(dev, plane_mask, ret);
> >  
> > -	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> > -		/*
> > -		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
> > -		 * if they weren't, this code should be called on success
> > -		 * for TEST_ONLY too.
> > -		 */
> > -
> > -		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > -			if (!crtc_state->event)
> > -				continue;
> > +	if (ret && fence_state)
> > +		unprepare_crtc_signaling(dev, state, fence_state);
> 
> I'm tempted to rework this to:
> 
> 	complete_crtc_signalling(dev, state, fence_state, ret)
> 
> and push the check for fence_state == NULL into it. And also use ret to
> decide whether we need to clean up, or fd_install the fence. That way
> everything is in one place. That would also tidy up the control flow a
> bit. And s/unprepare/complete since it wouldn't just be the error path
> cleanup any more.

Yes, this is a lot more cleaner. Thanks for the suggestion.

Gustavo

Re: [PATCH v7 3/3] drm/fence: add out-fences support

[Daniel Vetter]

On Wed, Nov 09, 2016 at 11:39:11AM +0900, Gustavo Padovan wrote:
> > On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote:
> > > +		if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
> > > +			return -EFAULT;
> > 
> > Same comment about igt coverage I made for patch 1, but with
> > s/in-fence/out-fence/, and s/~0ULL/8/. I picked 8 as an invalid address !=
> > NULL.
> > 
> > And the testcase need to cover all possible combinations of output event
> > generation, i.e. out-fence, event and out-fence+event. So 3x3=9 testcases
> > for this I think.
> 
> out-fence and event. so 2x2=4 ;)

3 different igt modes I've counted:
- wrong prop after correct fence prop (early failure)
- atomic_check fails (late failure)
- success

With 3 kinds of events:
- fence only
- event only
- both - which might show up some bug if you bail out after e.g. handling
  fences, but before handling events and then leak.

Hence 3x3 ;-) But if some of these aren't reasonable I'm ok with leaving
them out, too.

> > > +static void unprepare_crtc_signaling(struct drm_device *dev,
> > > +				     struct drm_atomic_state *state,
> > > +				     struct drm_out_fence_state *fence_state)
> > > +{
> > > +	struct drm_crtc *crtc;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	int i;
> > > +
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		/*
> > > +		 * TEST_ONLY and PAGE_FLIP_EVENT are mutually
> > > +		 * exclusive, if they weren't, this code should be
> > > +		 * called on success for TEST_ONLY too.
> > > +		 */
> > > +		if (crtc_state->event)
> > > +			drm_event_cancel_free(dev,
> > > +					      &crtc_state->event->base);
> > > +	}
> > > +
> > > +	for (i = 0; fence_state[i].out_fence_ptr; i++) {
> > 
> > This goes boom if you have fences set for every crtc, because then this
> > check will walk past the end of the array and do something undefined. You
> > need to manually count how many of these slots are set (and might want to
> > switch to a krealloc pattern while at it). Sounds like it needs an igt.
> 
> On the fd_install loop I was also checking for i <
> dev->mode_config.num_crtcs but forgot to add that here. However having a
> num_fences is a better solution, I'll add that.

And adding num_fence will be a good prep for writeback fences from Brian,
too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 3/3] drm/fence: add out-fences support

[Brian Starkey]

On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
>+static struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,
>+					struct drm_crtc_state *crtc_state)
>+{
>+	struct dma_fence *fence;
>+
>+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>+	if (!fence)
>+		return NULL;
>+
>+	dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
>+		       crtc->fence_context, ++crtc->fence_seqno);
>+
>+	return fence;
>+}
>+

crtc_state is unused in this function.

-Brian

Re: [PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> This is yet another version of the DRM fences patches. Please refer
> to the cover letter[1] in a previous version to check for more details.

Explicit fencing is not a superset of the implicit fences. The driver
may be using implicit fences (on a reservation object) to serialise
asynchronous operations wrt to each other (such as dispatching threads
to flush cpu caches to memory, manipulating page tables and the like
before the flip).  Since the user doesn't know about these operations,
they are not included in the explicit fence they provide, at which point
we can't trust their fence to the exclusion of the implicit fences...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v7 0/3] drm: add explict fencing

[Daniel Vetter]

On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Hi,
> > 
> > This is yet another version of the DRM fences patches. Please refer
> > to the cover letter[1] in a previous version to check for more details.
> 
> Explicit fencing is not a superset of the implicit fences. The driver
> may be using implicit fences (on a reservation object) to serialise
> asynchronous operations wrt to each other (such as dispatching threads
> to flush cpu caches to memory, manipulating page tables and the like
> before the flip).  Since the user doesn't know about these operations,
> they are not included in the explicit fence they provide, at which point
> we can't trust their fence to the exclusion of the implicit fences...

My thoughts are that in atomic_check drivers just fill in the fence from
the reservation_object (i.e. the uapi implicit fencing part). If there's
any additional work that's queued up in ->prepare_fb then I guess the
driver needs to track that internally, but _only_ for kernel-internally
queued work.

The reason for that is that with explicit fencing we want to allow
userspace to overwrite any existing implicit fences that might hang
around.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Hi,
> > > 
> > > This is yet another version of the DRM fences patches. Please refer
> > > to the cover letter[1] in a previous version to check for more details.
> > 
> > Explicit fencing is not a superset of the implicit fences. The driver
> > may be using implicit fences (on a reservation object) to serialise
> > asynchronous operations wrt to each other (such as dispatching threads
> > to flush cpu caches to memory, manipulating page tables and the like
> > before the flip).  Since the user doesn't know about these operations,
> > they are not included in the explicit fence they provide, at which point
> > we can't trust their fence to the exclusion of the implicit fences...
> 
> My thoughts are that in atomic_check drivers just fill in the fence from
> the reservation_object (i.e. the uapi implicit fencing part). If there's
> any additional work that's queued up in ->prepare_fb then I guess the
> driver needs to track that internally, but _only_ for kernel-internally
> queued work.

That's not a trivial task to work out which of the fence contexts within
the reservation object are required and which are to be replaced by the
explicit fence, esp. when you have to consider external fences.
 
> The reason for that is that with explicit fencing we want to allow
> userspace to overwrite any existing implicit fences that might hang
> around.

I'm just suggesting the danger of that when userspace doesn't know
everything and the current interfaces do not allow for userspace to know,
we only tell userspace about its own action (more or less).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v7 0/3] drm: add explict fencing

[Daniel Vetter]

On Tue, Nov 08, 2016 at 11:45:51AM +0000, Chris Wilson wrote:
> On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> > > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > Hi,
> > > > 
> > > > This is yet another version of the DRM fences patches. Please refer
> > > > to the cover letter[1] in a previous version to check for more details.
> > > 
> > > Explicit fencing is not a superset of the implicit fences. The driver
> > > may be using implicit fences (on a reservation object) to serialise
> > > asynchronous operations wrt to each other (such as dispatching threads
> > > to flush cpu caches to memory, manipulating page tables and the like
> > > before the flip).  Since the user doesn't know about these operations,
> > > they are not included in the explicit fence they provide, at which point
> > > we can't trust their fence to the exclusion of the implicit fences...
> > 
> > My thoughts are that in atomic_check drivers just fill in the fence from
> > the reservation_object (i.e. the uapi implicit fencing part). If there's
> > any additional work that's queued up in ->prepare_fb then I guess the
> > driver needs to track that internally, but _only_ for kernel-internally
> > queued work.
> 
> That's not a trivial task to work out which of the fence contexts within
> the reservation object are required and which are to be replaced by the
> explicit fence, esp. when you have to consider external fences.

Hm, what kind of async kernel tasks are you thinking off? Atm I don't know
of anyone who does e.g. clflush through the gpu. And ttm bo placement
moves for display should be explicit enough that drivers will deal with
them correctly. At least that seems to have been the conclusion from the
long amdgpu thread.

> > The reason for that is that with explicit fencing we want to allow
> > userspace to overwrite any existing implicit fences that might hang
> > around.
> 
> I'm just suggesting the danger of that when userspace doesn't know
> everything and the current interfaces do not allow for userspace to know,
> we only tell userspace about its own action (more or less).

tools for fools, but yes userspace is expected to get this 100% right (for
any userspace-issued cs at least), and eat the fallout if it doesn't.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 0/3] drm: add explict fencing

[Chris Wilson]

On Tue, Nov 08, 2016 at 01:43:40PM +0100, Daniel Vetter wrote:
> On Tue, Nov 08, 2016 at 11:45:51AM +0000, Chris Wilson wrote:
> > On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> > > > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This is yet another version of the DRM fences patches. Please refer
> > > > > to the cover letter[1] in a previous version to check for more details.
> > > > 
> > > > Explicit fencing is not a superset of the implicit fences. The driver
> > > > may be using implicit fences (on a reservation object) to serialise
> > > > asynchronous operations wrt to each other (such as dispatching threads
> > > > to flush cpu caches to memory, manipulating page tables and the like
> > > > before the flip).  Since the user doesn't know about these operations,
> > > > they are not included in the explicit fence they provide, at which point
> > > > we can't trust their fence to the exclusion of the implicit fences...
> > > 
> > > My thoughts are that in atomic_check drivers just fill in the fence from
> > > the reservation_object (i.e. the uapi implicit fencing part). If there's
> > > any additional work that's queued up in ->prepare_fb then I guess the
> > > driver needs to track that internally, but _only_ for kernel-internally
> > > queued work.
> > 
> > That's not a trivial task to work out which of the fence contexts within
> > the reservation object are required and which are to be replaced by the
> > explicit fence, esp. when you have to consider external fences.
> 
> Hm, what kind of async kernel tasks are you thinking off? Atm I don't know
> of anyone who does e.g. clflush through the gpu. And ttm bo placement
> moves for display should be explicit enough that drivers will deal with
> them correctly. At least that seems to have been the conclusion from the
> long amdgpu thread.

Now that we (i915) serialise on an reservation_object (obj->resv), we
have floated ideas to use that to serialise async tasks (such as
offloading the 100ms clflush to a (cpu) worker, a gpu task would pose a
similar problem with a fence inserted that is not exposed to userspace).
Also tempted to look at using async tasks + fences to do GTT updates
but that is not a common pain point at the moment, and cases where it
is the GTT thrashing itself is the issue.

So how does i915 deal with ttm bo fences?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v7 0/3] drm: add explict fencing

[Christian König]

Am 08.11.2016 um 12:45 schrieb Chris Wilson:
> On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
>> On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
>>> On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> Hi,
>>>>
>>>> This is yet another version of the DRM fences patches. Please refer
>>>> to the cover letter[1] in a previous version to check for more details.
>>> Explicit fencing is not a superset of the implicit fences. The driver
>>> may be using implicit fences (on a reservation object) to serialise
>>> asynchronous operations wrt to each other (such as dispatching threads
>>> to flush cpu caches to memory, manipulating page tables and the like
>>> before the flip).  Since the user doesn't know about these operations,
>>> they are not included in the explicit fence they provide, at which point
>>> we can't trust their fence to the exclusion of the implicit fences...
>> My thoughts are that in atomic_check drivers just fill in the fence from
>> the reservation_object (i.e. the uapi implicit fencing part). If there's
>> any additional work that's queued up in ->prepare_fb then I guess the
>> driver needs to track that internally, but _only_ for kernel-internally
>> queued work.
> That's not a trivial task to work out which of the fence contexts within
> the reservation object are required and which are to be replaced by the
> explicit fence, esp. when you have to consider external fences.
>   
>> The reason for that is that with explicit fencing we want to allow
>> userspace to overwrite any existing implicit fences that might hang
>> around.
> I'm just suggesting the danger of that when userspace doesn't know
> everything and the current interfaces do not allow for userspace to know,
> we only tell userspace about its own action (more or less).

It's even worse than that. See the kernel can for example swap out 
objects any time it wants.

Userspace doesn't know about such operations and so can't provide them 
as explicit fence.

Same is true for example in situations where one userspace process 
doesn't know about operations another process does. E.g. for backward 
compatibility with DRI2/3 for example.

So we will always have a mixture of implicit fences and explicit fences.

The approach we used for amdgpu is that we implicit wait for all fences 
which the initiator of an operation can't know about (e.g. from another 
process or kernel internally) and explicitly wait for all additional 
fences provided by the initiator or an operation.

Regards,
Christian.

> -Chris
>

Re: [PATCH v7 0/3] drm: add explict fencing

[Daniel Vetter]

On Tue, Nov 08, 2016 at 01:44:34PM +0100, Christian König wrote:
> Am 08.11.2016 um 12:45 schrieb Chris Wilson:
> > On Tue, Nov 08, 2016 at 12:32:56PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 08, 2016 at 10:35:08AM +0000, Chris Wilson wrote:
> > > > On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This is yet another version of the DRM fences patches. Please refer
> > > > > to the cover letter[1] in a previous version to check for more details.
> > > > Explicit fencing is not a superset of the implicit fences. The driver
> > > > may be using implicit fences (on a reservation object) to serialise
> > > > asynchronous operations wrt to each other (such as dispatching threads
> > > > to flush cpu caches to memory, manipulating page tables and the like
> > > > before the flip).  Since the user doesn't know about these operations,
> > > > they are not included in the explicit fence they provide, at which point
> > > > we can't trust their fence to the exclusion of the implicit fences...
> > > My thoughts are that in atomic_check drivers just fill in the fence from
> > > the reservation_object (i.e. the uapi implicit fencing part). If there's
> > > any additional work that's queued up in ->prepare_fb then I guess the
> > > driver needs to track that internally, but _only_ for kernel-internally
> > > queued work.
> > That's not a trivial task to work out which of the fence contexts within
> > the reservation object are required and which are to be replaced by the
> > explicit fence, esp. when you have to consider external fences.
> > > The reason for that is that with explicit fencing we want to allow
> > > userspace to overwrite any existing implicit fences that might hang
> > > around.
> > I'm just suggesting the danger of that when userspace doesn't know
> > everything and the current interfaces do not allow for userspace to know,
> > we only tell userspace about its own action (more or less).
> 
> It's even worse than that. See the kernel can for example swap out objects
> any time it wants.
> 
> Userspace doesn't know about such operations and so can't provide them as
> explicit fence.
> 
> Same is true for example in situations where one userspace process doesn't
> know about operations another process does. E.g. for backward compatibility
> with DRI2/3 for example.
> 
> So we will always have a mixture of implicit fences and explicit fences.
> 
> The approach we used for amdgpu is that we implicit wait for all fences
> which the initiator of an operation can't know about (e.g. from another
> process or kernel internally) and explicitly wait for all additional fences
> provided by the initiator or an operation.

On android userspace is also supposed to know about explicit fences from
other users, i.e. shared buffers. So there you shouldn't wait for implicit
fences from other processes, only for kernel-internal stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v7 0/3] drm: add explict fencing

[Daniel Vetter]

On Tue, Nov 08, 2016 at 03:54:47PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> This is yet another version of the DRM fences patches. Please refer
> to the cover letter[1] in a previous version to check for more details.
> 
> In v7 we now have split most of the out_fences code into
> prepare_crtc_signaling() and unprepare_crtc_signaling() with improved error
> handling. More details on the v7 changes are embedded in each commit's
> message.
> 
> Robert Foss managed to port Android's drm_hwcomposer to the new HWC2 API and
> added support to fences. Current patches can be seen here:
> 
> https://git.collabora.com/cgit/user/robertfoss/drm_hwcomposer.git/log/?h=hwc2_fence_v1
> 
> He managed to run AOSP on top of padovan/fences kernel branch with full fence
> support on qemu/virgl and msm db410c. That means we already have a working
> open source userspace using the explicit fencing implementation.
> 
> Also i-g-t testing are available at:
> 
> https://git.collabora.com/cgit/user/padovan/intel-gpu-tools.git/log/
> 
> Please review!

I think we're getting there. Found some issues with patch 3 still. Besides
that I think we need:
- r-b/ack from Sean Paul, as an ack that he's happy with what this means
  for drm_hwcomposer (and that the hwc patches look ok, too).
- acks/t-b from everyone who's run this, the more the better. This is a
  big uabi extension, making the effort by everyone explicit is important.
- ack from Brian that he can use the out-fence stuff for his writeback
  support. Probably need to add a for_each_connector loop to
  prepare/complete_signalling (and drop the crtc_ in there, but Brian can
  do that).

Cheers, Daniel

> 
> Gustavo
> 
> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1253822.html
> 
> ---
> 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        | 247 ++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/drm_atomic_helper.c |   3 +
>  drivers/gpu/drm/drm_crtc.c          |  45 +++++++
>  drivers/gpu/drm/drm_plane.c         |   1 +
>  include/drm/drm_atomic.h            |   1 +
>  include/drm/drm_crtc.h              |  55 ++++++++
>  7 files changed, 311 insertions(+), 42 deletions(-)
> 
> -- 
> 2.5.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch