Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1386965 > unrolled thread

[RFC v2 5/8] drm/fence: add in-fences support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC v2 5/8] drm/fence: add in-fences support Gustavo Padovan <gustavo@padovan.org> - 2016-04-26 00:40 +0200
    Re: [RFC v2 5/8] drm/fence: add in-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-26 12:20 +0200
      Re: [RFC v2 5/8] drm/fence: add in-fences support Gustavo Padovan <gustavo.padovan@collabora.co.uk> - 2016-04-26 16:20 +0200
        Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-04-26 16:40 +0200
          Re: [RFC v2 5/8] drm/fence: add in-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-26 18:30 +0200
            Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-04-26 19:30 +0200
              Re: [RFC v2 5/8] drm/fence: add in-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-26 19:50 +0200
                Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-04-26 20:30 +0200
                  Re: [RFC v2 5/8] drm/fence: add in-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-26 21:00 +0200
                    Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-04-26 22:10 +0200
                      Re: [RFC v2 5/8] drm/fence: add in-fences support Greg Hackmann <ghackmann@google.com> - 2016-04-26 22:50 +0200
                        Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Vetter <daniel@ffwll.ch> - 2016-04-27 08:40 +0200
                        Re: [RFC v2 5/8] drm/fence: add in-fences support Daniel Stone <daniel@fooishbar.org> - 2016-04-27 09:00 +0200
        Re: [RFC v2 5/8] drm/fence: add in-fences support Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-04-26 18:30 +0200

#1386965 — [RFC v2 5/8] drm/fence: add in-fences support

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

There is now a new property called FENCE_FD attached to every plane
state that receives the sync_file fd from userspace via the atomic commit
IOCTL.

The fd is then translated to a fence (that may be a fence_collection
subclass or just a normal fence) and then used by DRM to fence_wait() for
all fences in the sync_file to signal. So it only commits when all
framebuffers are ready to scanout.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

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.
---
 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          |  7 +++++++
 include/drm/drm_crtc.h              |  1 +
 5 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f2a74d0..3c987e3 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -12,6 +12,7 @@ menuconfig DRM
 	select I2C
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
+	select SYNC_FILE
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..13674c7 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -30,6 +30,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_plane_helper.h>
+#include <linux/sync_file.h>
 
 /**
  * drm_atomic_state_default_release -
@@ -680,6 +681,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		drm_atomic_set_fb_for_plane(state, fb);
 		if (fb)
 			drm_framebuffer_unreference(fb);
+	} else if (property == config->prop_fence_fd) {
+		if (U642I64(val) == -1)
+			return 0;
+
+		if (state->fence)
+			fence_put(state->fence);
+
+		state->fence = sync_file_fences_get(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);
@@ -737,6 +749,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	if (property == config->prop_fb_id) {
 		*val = (state->fb) ? state->fb->base.id : 0;
+	} else if (property == config->prop_fence_fd) {
+		*val = -1;
 	} else if (property == config->prop_crtc_id) {
 		*val = (state->crtc) ? state->crtc->base.id : 0;
 	} else if (property == config->prop_crtc_x) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f1cfcce..866f332 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2696,6 +2696,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
 {
 	if (state->fb)
 		drm_framebuffer_unreference(state->fb);
+
+	if (state->fence)
+		fence_put(state->fence);
 }
 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 55ffde5..65212ce 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
 		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
 		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
@@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_fb_id = prop;
 
+	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
+			"FENCE_FD", -1, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fence_fd = prop;
+
 	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
 			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
 	if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8cb377c..5ba3cda 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2122,6 +2122,7 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_w;
 	struct drm_property *prop_crtc_h;
 	struct drm_property *prop_fb_id;
+	struct drm_property *prop_fence_fd;
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
 	struct drm_property *prop_mode_id;
-- 
2.5.5

[toc] | [next] | [standalone]

#1387331

On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.

I still don't like this property abuse. Also with atomic, all passed
fences must be waited upon before anything is done, so attaching them
to planes seems like it might just give people the wrong idea.

> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> 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.
> ---
>  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          |  7 +++++++
>  include/drm/drm_crtc.h              |  1 +
>  5 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f2a74d0..3c987e3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..13674c7 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  /**
>   * drm_atomic_state_default_release -
> @@ -680,6 +681,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_fence_fd) {
> +		if (U642I64(val) == -1)
> +			return 0;
> +
> +		if (state->fence)
> +			fence_put(state->fence);
> +
> +		state->fence = sync_file_fences_get(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);
> @@ -737,6 +749,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f1cfcce..866f332 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2696,6 +2696,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence)
> +		fence_put(state->fence);
>  }
>  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 55ffde5..65212ce 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1278,6 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> @@ -1533,6 +1534,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8cb377c..5ba3cda 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2122,6 +2122,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_w;
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_fence_fd;
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  	struct drm_property *prop_mode_id;
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1387521

2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > There is now a new property called FENCE_FD attached to every plane
> > state that receives the sync_file fd from userspace via the atomic commit
> > IOCTL.
> 
> I still don't like this property abuse. Also with atomic, all passed
> fences must be waited upon before anything is done, so attaching them
> to planes seems like it might just give people the wrong idea.

I'm actually fine with this as property, but another solutions is use
an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
we have done for out fences. However the FENCE_FD property is easier to
handle in userspace than the array. Any other idea?

	Gustavo

[toc] | [prev] | [next] | [standalone]

#1387542

On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > There is now a new property called FENCE_FD attached to every plane
> > > state that receives the sync_file fd from userspace via the atomic commit
> > > IOCTL.
> > 
> > I still don't like this property abuse. Also with atomic, all passed
> > fences must be waited upon before anything is done, so attaching them
> > to planes seems like it might just give people the wrong idea.
> 
> I'm actually fine with this as property, but another solutions is use
> an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> we have done for out fences. However the FENCE_FD property is easier to
> handle in userspace than the array. Any other idea?

Imo FENCE_FD is perfectly fine. But what's the concern around giving
people the wrong idea with attaching fences to planes? For nonblocking
commits we need to store them somewhere for the worker, drm_plane_state
seems like an as good place as any other.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1387676

On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > 
> > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > 
> > > > There is now a new property called FENCE_FD attached to every plane
> > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > IOCTL.
> > > 
> > > I still don't like this property abuse. Also with atomic, all passed
> > > fences must be waited upon before anything is done, so attaching them
> > > to planes seems like it might just give people the wrong idea.
> > 
> > I'm actually fine with this as property, but another solutions is use
> > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > we have done for out fences. However the FENCE_FD property is easier to
> > handle in userspace than the array. Any other idea?
> 
> Imo FENCE_FD is perfectly fine. But what's the concern around giving
> people the wrong idea with attaching fences to planes? For nonblocking
> commits we need to store them somewhere for the worker, drm_plane_state
> seems like an as good place as any other.

It gives the impression that each plane might flip as soon as its fence
signals.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1387745

On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > 
> > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > 
> > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > IOCTL.
> > > > 
> > > > I still don't like this property abuse. Also with atomic, all passed
> > > > fences must be waited upon before anything is done, so attaching them
> > > > to planes seems like it might just give people the wrong idea.
> > > 
> > > I'm actually fine with this as property, but another solutions is use
> > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > we have done for out fences. However the FENCE_FD property is easier to
> > > handle in userspace than the array. Any other idea?
> > 
> > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > people the wrong idea with attaching fences to planes? For nonblocking
> > commits we need to store them somewhere for the worker, drm_plane_state
> > seems like an as good place as any other.
> 
> It gives the impression that each plane might flip as soon as its fence
> signals.

That wouldn't be atomic. Not sure how someone could come up with that
idea. I mean we could move FENCE_FD to the crtc (fence fds can be merged),
but that's just a needless difference to what hwc expects. I think
aligning with the only real-world users in this case here makes sense.

Plus docs in case someone has funny ideas.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1387767

On Tue, Apr 26, 2016 at 07:20:49PM +0200, Daniel Vetter wrote:
> On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > 
> > > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > 
> > > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > > IOCTL.
> > > > > 
> > > > > I still don't like this property abuse. Also with atomic, all passed
> > > > > fences must be waited upon before anything is done, so attaching them
> > > > > to planes seems like it might just give people the wrong idea.
> > > > 
> > > > I'm actually fine with this as property, but another solutions is use
> > > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > > we have done for out fences. However the FENCE_FD property is easier to
> > > > handle in userspace than the array. Any other idea?
> > > 
> > > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > > people the wrong idea with attaching fences to planes? For nonblocking
> > > commits we need to store them somewhere for the worker, drm_plane_state
> > > seems like an as good place as any other.
> > 
> > It gives the impression that each plane might flip as soon as its fence
> > signals.
> 
> That wouldn't be atomic. Not sure how someone could come up with that
> idea.

What else would it mean? It's attached to a specific plane, so why would
it affect other planes?

> I mean we could move FENCE_FD to the crtc (fence fds can be merged),
> but that's just a needless difference to what hwc expects. I think
> aligning with the only real-world users in this case here makes sense.

Well it doesn't belong on the crtc either. I would just stick in the
ioctl as a separate thing, then it's clear it's related to the whole
operation rather than any kms object.

> 
> Plus docs in case someone has funny ideas.

Weren't you just quoting rusty's API manifesto recently? ;)

Maybe it was someone else.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1387806

On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 26, 2016 at 07:20:49PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> > > On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > > > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > > > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > > 
> > > > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > > 
> > > > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > > > IOCTL.
> > > > > > 
> > > > > > I still don't like this property abuse. Also with atomic, all passed
> > > > > > fences must be waited upon before anything is done, so attaching them
> > > > > > to planes seems like it might just give people the wrong idea.
> > > > > 
> > > > > I'm actually fine with this as property, but another solutions is use
> > > > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > > > we have done for out fences. However the FENCE_FD property is easier to
> > > > > handle in userspace than the array. Any other idea?
> > > > 
> > > > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > > > people the wrong idea with attaching fences to planes? For nonblocking
> > > > commits we need to store them somewhere for the worker, drm_plane_state
> > > > seems like an as good place as any other.
> > > 
> > > It gives the impression that each plane might flip as soon as its fence
> > > signals.
> > 
> > That wouldn't be atomic. Not sure how someone could come up with that
> > idea.
> 
> What else would it mean? It's attached to a specific plane, so why would
> it affect other planes?
> 
> > I mean we could move FENCE_FD to the crtc (fence fds can be merged),
> > but that's just a needless difference to what hwc expects. I think
> > aligning with the only real-world users in this case here makes sense.
> 
> Well it doesn't belong on the crtc either. I would just stick in the
> ioctl as a separate thing, then it's clear it's related to the whole
> operation rather than any kms object.

We want it per-crtc I'd say, so that you could flip each crtc
individually. But really the reason for per-plane is hw composer from
Android. I don't see any point in designing an api that's needlessly
different from what the main user expects (even if it may be silly).

The other bit is that for implicit syncing you need one fence per fb/plane
anyway, so this also fits nicely on the driver side I think.

> > Plus docs in case someone has funny ideas.
> 
> Weren't you just quoting rusty's API manifesto recently? ;)

I quote it all the time.

http://sweng.the-davies.net/Home/rustys-api-design-manifesto

I think current interface is scoring pretty high since as part of
Gustavo's work there's no also a new atomic helper which will get the
waiting right. There's still the problem that neither for drm_event nor
the fence do we have anything idiot-proof. So for that the solution is
testcases (which are also happening).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1387904

On Tue, Apr 26, 2016 at 08:23:46PM +0200, Daniel Vetter wrote:
> On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 26, 2016 at 07:20:49PM +0200, Daniel Vetter wrote:
> > > On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > > > > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > > > > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > > > 
> > > > > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > > > 
> > > > > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > > > > IOCTL.
> > > > > > > 
> > > > > > > I still don't like this property abuse. Also with atomic, all passed
> > > > > > > fences must be waited upon before anything is done, so attaching them
> > > > > > > to planes seems like it might just give people the wrong idea.
> > > > > > 
> > > > > > I'm actually fine with this as property, but another solutions is use
> > > > > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > > > > we have done for out fences. However the FENCE_FD property is easier to
> > > > > > handle in userspace than the array. Any other idea?
> > > > > 
> > > > > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > > > > people the wrong idea with attaching fences to planes? For nonblocking
> > > > > commits we need to store them somewhere for the worker, drm_plane_state
> > > > > seems like an as good place as any other.
> > > > 
> > > > It gives the impression that each plane might flip as soon as its fence
> > > > signals.
> > > 
> > > That wouldn't be atomic. Not sure how someone could come up with that
> > > idea.
> > 
> > What else would it mean? It's attached to a specific plane, so why would
> > it affect other planes?
> > 
> > > I mean we could move FENCE_FD to the crtc (fence fds can be merged),
> > > but that's just a needless difference to what hwc expects. I think
> > > aligning with the only real-world users in this case here makes sense.
> > 
> > Well it doesn't belong on the crtc either. I would just stick in the
> > ioctl as a separate thing, then it's clear it's related to the whole
> > operation rather than any kms object.
> 
> We want it per-crtc I'd say, so that you could flip each crtc
> individually.

Then you could just issue multiple ioctls. For eg. those nasty 4k MST
display (or just otherwise neatly synced displayes) you want to wait for
all the fences upfront and the flip everything at once, otherwise you'll
get nasty tears at the seams.

> But really the reason for per-plane is hw composer from
> Android. I don't see any point in designing an api that's needlessly
> different from what the main user expects (even if it may be silly).

What are they doing that can't stuff the fences into an array
instead of props?

> 
> The other bit is that for implicit syncing you need one fence per fb/plane
> anyway, so this also fits nicely on the driver side I think.
> 
> > > Plus docs in case someone has funny ideas.
> > 
> > Weren't you just quoting rusty's API manifesto recently? ;)
> 
> I quote it all the time.
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> 
> I think current interface is scoring pretty high since as part of
> Gustavo's work there's no also a new atomic helper which will get the
> waiting right. There's still the problem that neither for drm_event nor
> the fence do we have anything idiot-proof. So for that the solution is
> testcases (which are also happening).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1387991

On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 26, 2016 at 08:23:46PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
> > > On Tue, Apr 26, 2016 at 07:20:49PM +0200, Daniel Vetter wrote:
> > > > On Tue, Apr 26, 2016 at 07:26:21PM +0300, Ville Syrjälä wrote:
> > > > > On Tue, Apr 26, 2016 at 04:36:36PM +0200, Daniel Vetter wrote:
> > > > > > On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> > > > > > > 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > > > > 
> > > > > > > > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > > > > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > > > > > > > 
> > > > > > > > > There is now a new property called FENCE_FD attached to every plane
> > > > > > > > > state that receives the sync_file fd from userspace via the atomic commit
> > > > > > > > > IOCTL.
> > > > > > > > 
> > > > > > > > I still don't like this property abuse. Also with atomic, all passed
> > > > > > > > fences must be waited upon before anything is done, so attaching them
> > > > > > > > to planes seems like it might just give people the wrong idea.
> > > > > > > 
> > > > > > > I'm actually fine with this as property, but another solutions is use
> > > > > > > an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> > > > > > > we have done for out fences. However the FENCE_FD property is easier to
> > > > > > > handle in userspace than the array. Any other idea?
> > > > > > 
> > > > > > Imo FENCE_FD is perfectly fine. But what's the concern around giving
> > > > > > people the wrong idea with attaching fences to planes? For nonblocking
> > > > > > commits we need to store them somewhere for the worker, drm_plane_state
> > > > > > seems like an as good place as any other.
> > > > > 
> > > > > It gives the impression that each plane might flip as soon as its fence
> > > > > signals.
> > > > 
> > > > That wouldn't be atomic. Not sure how someone could come up with that
> > > > idea.
> > > 
> > > What else would it mean? It's attached to a specific plane, so why would
> > > it affect other planes?
> > > 
> > > > I mean we could move FENCE_FD to the crtc (fence fds can be merged),
> > > > but that's just a needless difference to what hwc expects. I think
> > > > aligning with the only real-world users in this case here makes sense.
> > > 
> > > Well it doesn't belong on the crtc either. I would just stick in the
> > > ioctl as a separate thing, then it's clear it's related to the whole
> > > operation rather than any kms object.
> > 
> > We want it per-crtc I'd say, so that you could flip each crtc
> > individually.
> 
> Then you could just issue multiple ioctls. For eg. those nasty 4k MST
> display (or just otherwise neatly synced displayes) you want to wait for
> all the fences upfront and the flip everything at once, otherwise you'll
> get nasty tears at the seams.
> 
> > But really the reason for per-plane is hw composer from
> > Android. I don't see any point in designing an api that's needlessly
> > different from what the main user expects (even if it may be silly).
> 
> What are they doing that can't stuff the fences into an array
> instead of props?

The hw composer interface is one in-fence per plane. That's really the
major reason why the kernel interface is built to match. And I really
don't think we should diverge just because we have a slight different
color preference ;-)

As long as you end up with a pile of fences somehow it'll work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1388076

On 04/26/2016 01:05 PM, Daniel Vetter wrote:
> On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
>> On Tue, Apr 26, 2016 at 08:23:46PM +0200, Daniel Vetter wrote:
>>> On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
>>> But really the reason for per-plane is hw composer from
>>> Android. I don't see any point in designing an api that's needlessly
>>> different from what the main user expects (even if it may be silly).
>>
>> What are they doing that can't stuff the fences into an array
>> instead of props?
>
> The hw composer interface is one in-fence per plane. That's really the
> major reason why the kernel interface is built to match. And I really
> don't think we should diverge just because we have a slight different
> color preference ;-)
>
> As long as you end up with a pile of fences somehow it'll work.
> -Daniel
>

The relationship between layers and fences is only fuzzy and indirect 
though.  The relationship is really between the buffer you're displaying 
on that layer, and the fence representing the work done to render into 
that buffer.  SurfaceFlinger just happens to bundle them together inside 
the same struct hwc_layer_1 as an API convenience.

Which is kind of splitting hairs as long as you have a 1-to-1 
relationship between layers and DRM planes.  But that's not always the case.

A (per-CRTC?) array of fences would be more flexible.  And even in the 
cases where you could make a 1-to-1 mapping between planes and fences, 
it's not that much more work for userspace to assemble those fences into 
an array anyway.

[toc] | [prev] | [next] | [standalone]

#1388695

On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
> On 04/26/2016 01:05 PM, Daniel Vetter wrote:
> >On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
> >>On Tue, Apr 26, 2016 at 08:23:46PM +0200, Daniel Vetter wrote:
> >>>On Tue, Apr 26, 2016 at 08:40:45PM +0300, Ville Syrjälä wrote:
> >>>But really the reason for per-plane is hw composer from
> >>>Android. I don't see any point in designing an api that's needlessly
> >>>different from what the main user expects (even if it may be silly).
> >>
> >>What are they doing that can't stuff the fences into an array
> >>instead of props?
> >
> >The hw composer interface is one in-fence per plane. That's really the
> >major reason why the kernel interface is built to match. And I really
> >don't think we should diverge just because we have a slight different
> >color preference ;-)
> >
> >As long as you end up with a pile of fences somehow it'll work.
> >-Daniel
> >
> 
> The relationship between layers and fences is only fuzzy and indirect
> though.  The relationship is really between the buffer you're displaying on
> that layer, and the fence representing the work done to render into that
> buffer.  SurfaceFlinger just happens to bundle them together inside the same
> struct hwc_layer_1 as an API convenience.
> 
> Which is kind of splitting hairs as long as you have a 1-to-1 relationship
> between layers and DRM planes.  But that's not always the case.
> 
> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
> where you could make a 1-to-1 mapping between planes and fences, it's not
> that much more work for userspace to assemble those fences into an array
> anyway.

I'm ok with an array too if that's what you folks prefer (it's meant to be
used by you after all). I just don't want just 1 fence for the entire op,
forcing userspace to first merge them all together. That seems silly.

One side-effect of that is that we'd also have to rework all the internal
bits and move fences around in atomic. Which means change a pile of
drivers. Not sure that's worth it, but I'd be ok either way really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1388705

Hi,

On 26 April 2016 at 21:48, Greg Hackmann <ghackmann@google.com> wrote:
> On 04/26/2016 01:05 PM, Daniel Vetter wrote:
>> On Tue, Apr 26, 2016 at 09:55:06PM +0300, Ville Syrjälä wrote:
>>> What are they doing that can't stuff the fences into an array
>>> instead of props?
>>
>> The hw composer interface is one in-fence per plane. That's really the
>> major reason why the kernel interface is built to match. And I really
>> don't think we should diverge just because we have a slight different
>> color preference ;-)
>
> The relationship between layers and fences is only fuzzy and indirect
> though.  The relationship is really between the buffer you're displaying on
> that layer, and the fence representing the work done to render into that
> buffer.  SurfaceFlinger just happens to bundle them together inside the same
> struct hwc_layer_1 as an API convenience.

Right, and when using implicit fencing, this comes as a plane
property, by virtue of plane -> fb -> buffer -> fence.

> Which is kind of splitting hairs as long as you have a 1-to-1 relationship
> between layers and DRM planes.  But that's not always the case.

Can you please elaborate?

> A (per-CRTC?) array of fences would be more flexible.  And even in the cases
> where you could make a 1-to-1 mapping between planes and fences, it's not
> that much more work for userspace to assemble those fences into an array
> anyway.

As Ville says, I don't want to go down the path of scheduling CRTC
updates separately, because that breaks MST pretty badly. If you don't
want your updates to display atomically, then don't schedule them
atomically ... ? That's the only reason I can see for making fencing
per-CRTC, rather than just a pile of unassociated fences appended to
the request. Per-CRTC fences also forces userspace to merge fences
before submission when using multiple planes per CRTC, which is pretty
punitive.

I think having it semantically attached to the plane is a little bit
nicer for tracing (why was this request delayed? -> a fence -> which
buffer was that fence for?) at a glance. Also the 'pile of appended
fences' model is a bit awkward for more generic userspace, which
creates a libdrm request and builds it (add a plane, try it out, wind
back) incrementally. Using properties makes that really easy, but
without properties, we'd have to add separate codepaths - and thus
separate ABI, which complicates distribution - to libdrm to account
for a separate plane array which shares a cursor with the properties.
So for that reason if none other, I'd really prefer not to go down
that route.

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1387686

On Tue, Apr 26, 2016 at 11:14:22AM -0300, Gustavo Padovan wrote:
> 2016-04-26 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Mon, Apr 25, 2016 at 07:33:25PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > There is now a new property called FENCE_FD attached to every plane
> > > state that receives the sync_file fd from userspace via the atomic commit
> > > IOCTL.
> > 
> > I still don't like this property abuse. Also with atomic, all passed
> > fences must be waited upon before anything is done, so attaching them
> > to planes seems like it might just give people the wrong idea.
> 
> I'm actually fine with this as property, but another solutions is use
> an array of {plane, fence_fd} and extend drm_atomic_ioctl args just like
> we have done for out fences.

Why do you want to associate these with planes?

> However the FENCE_FD property is easier to
> handle in userspace than the array. Any other idea?
> 
> 	Gustavo

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web