diff mbox

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

Message ID 1461623608-29538-6-git-send-email-gustavo@padovan.org (mailing list archive)
State New, archived
Headers show

Commit Message

Gustavo Padovan April 25, 2016, 10:33 p.m. UTC
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(+)

Comments

Ville Syrjälä April 26, 2016, 10:10 a.m. UTC | #1
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
Gustavo Padovan April 26, 2016, 2:14 p.m. UTC | #2
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
Daniel Vetter April 26, 2016, 2:36 p.m. UTC | #3
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
Ville Syrjälä April 26, 2016, 4:25 p.m. UTC | #4
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ä April 26, 2016, 4:26 p.m. UTC | #5
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.
Daniel Vetter April 26, 2016, 5:20 p.m. UTC | #6
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
Ville Syrjälä April 26, 2016, 5:40 p.m. UTC | #7
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.
Daniel Vetter April 26, 2016, 6:23 p.m. UTC | #8
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
Ville Syrjälä April 26, 2016, 6:55 p.m. UTC | #9
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
Daniel Vetter April 26, 2016, 8:05 p.m. UTC | #10
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
Greg Hackmann April 26, 2016, 8:48 p.m. UTC | #11
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.
Daniel Vetter April 27, 2016, 6:39 a.m. UTC | #12
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 Stone April 27, 2016, 6:57 a.m. UTC | #13
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
Gustavo Padovan April 28, 2016, 2:36 p.m. UTC | #14
2016-04-27 Daniel Stone <daniel@fooishbar.org>:

> 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.

I also agree to have it as FENCE_FD prop on the plane. Summarizing the
arguments on this thread, they are:

 - implicit fences also needs one fence per plane/fb, so it will be good to     
   match with that.                                                             
 - requires userspace to always merge fences                                    
 - can use standard plane properties, making kernel and userspace life easier,  
   an array brings more work to build the atomic request plus extra checkings   
   on the kernel.                                                               
 - do not need to changes to drivers                                            
 - better for tracing, can identify the buffer/fence promptly

	Gustavo
Daniel Vetter April 28, 2016, 2:38 p.m. UTC | #15
On Thu, Apr 28, 2016 at 11:36:44AM -0300, Gustavo Padovan wrote:
> 2016-04-27 Daniel Stone <daniel@fooishbar.org>:
> 
> > 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.
> 
> I also agree to have it as FENCE_FD prop on the plane. Summarizing the
> arguments on this thread, they are:
> 
>  - implicit fences also needs one fence per plane/fb, so it will be good to     
>    match with that.                                                             
>  - requires userspace to always merge fences                                    

"does not require" I presume?

>  - can use standard plane properties, making kernel and userspace life easier,  
>    an array brings more work to build the atomic request plus extra checkings   
>    on the kernel.                                                               
>  - do not need to changes to drivers                                            
>  - better for tracing, can identify the buffer/fence promptly

- Fits in well with the libdrm atomic rollback support - no need to manage
  fences separately when incrementally building an atomic commit.
 
> 
> 	Gustavo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä April 28, 2016, 4:56 p.m. UTC | #16
On Thu, Apr 28, 2016 at 11:36:44AM -0300, Gustavo Padovan wrote:
> 2016-04-27 Daniel Stone <daniel@fooishbar.org>:
> 
> > 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.
> 
> I also agree to have it as FENCE_FD prop on the plane. Summarizing the
> arguments on this thread, they are:

Your "summary" forgot to include any counter arguments.

> 
>  - implicit fences also needs one fence per plane/fb, so it will be good to     
>    match with that.                                                             

We would actually need a fence per object rather than per fb.

>  - requires userspace to always merge fences                                    

"doesn't?" but that's not true if it's an array. It would be true if
you had just one fence for the whole thing, or one per crtc.

>  - can use standard plane properties, making kernel and userspace life easier,  
>    an array brings more work to build the atomic request plus extra checkings   
>    on the kernel.                                                               

I don't really get this one. The objects and props are arrays too. Why is
another array so problematic?

>  - do not need to changes to drivers                                            
>  - better for tracing, can identify the buffer/fence promptly

Can fences be reused somehow while still attached to a plane, or ever?
That might cause some oddness if you, say, leave a fence attached to one
plane and then do a modeset on another crtc perhaps which needs to turn
the first crtc off+on to reconfigure something.
Daniel Vetter April 28, 2016, 5:43 p.m. UTC | #17
On Thu, Apr 28, 2016 at 6:56 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>>  - better for tracing, can identify the buffer/fence promptly
>
> Can fences be reused somehow while still attached to a plane, or ever?
> That might cause some oddness if you, say, leave a fence attached to one
> plane and then do a modeset on another crtc perhaps which needs to turn
> the first crtc off+on to reconfigure something.

Fences auto-disappear of course and don't stick around when you
duplicate the drm_plane_state again. I still don't really get the real
concerns though ... In the end it's purely a transport question, and
both ABI ideas work out semantically exactly the same in the end. It's
just that at least in my opinion FENCE_FD prop is a lot more
convenient.
-Daniel
Ville Syrjälä April 28, 2016, 5:51 p.m. UTC | #18
On Thu, Apr 28, 2016 at 07:43:16PM +0200, Daniel Vetter wrote:
> On Thu, Apr 28, 2016 at 6:56 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >>  - better for tracing, can identify the buffer/fence promptly
> >
> > Can fences be reused somehow while still attached to a plane, or ever?
> > That might cause some oddness if you, say, leave a fence attached to one
> > plane and then do a modeset on another crtc perhaps which needs to turn
> > the first crtc off+on to reconfigure something.
> 
> Fences auto-disappear of course and don't stick around when you
> duplicate the drm_plane_state again. I still don't really get the real
> concerns though ...

Properties that magically change values shouldn't exist IMO. I guess if
you could have write-only properties or something it migth be sensible?

> In the end it's purely a transport question, and
> both ABI ideas work out semantically exactly the same in the end. It's
> just that at least in my opinion FENCE_FD prop is a lot more
> convenient.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Gustavo Padovan April 28, 2016, 5:55 p.m. UTC | #19
2016-04-28 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Thu, Apr 28, 2016 at 07:43:16PM +0200, Daniel Vetter wrote:
> > On Thu, Apr 28, 2016 at 6:56 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >>  - better for tracing, can identify the buffer/fence promptly
> > >
> > > Can fences be reused somehow while still attached to a plane, or ever?
> > > That might cause some oddness if you, say, leave a fence attached to one
> > > plane and then do a modeset on another crtc perhaps which needs to turn
> > > the first crtc off+on to reconfigure something.
> > 
> > Fences auto-disappear of course and don't stick around when you
> > duplicate the drm_plane_state again. I still don't really get the real
> > concerns though ...
> 
> Properties that magically change values shouldn't exist IMO. I guess if
> you could have write-only properties or something it migth be sensible?

We can just not return FENCE_FD on get_props, that would make it
write-only.

	Gustavo
Daniel Vetter April 28, 2016, 6:02 p.m. UTC | #20
On Thu, Apr 28, 2016 at 7:55 PM, Gustavo Padovan
<gustavo.padovan@collabora.co.uk> wrote:
> 2016-04-28 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>
>> On Thu, Apr 28, 2016 at 07:43:16PM +0200, Daniel Vetter wrote:
>> > On Thu, Apr 28, 2016 at 6:56 PM, Ville Syrjälä
>> > <ville.syrjala@linux.intel.com> wrote:
>> > >>  - better for tracing, can identify the buffer/fence promptly
>> > >
>> > > Can fences be reused somehow while still attached to a plane, or ever?
>> > > That might cause some oddness if you, say, leave a fence attached to one
>> > > plane and then do a modeset on another crtc perhaps which needs to turn
>> > > the first crtc off+on to reconfigure something.
>> >
>> > Fences auto-disappear of course and don't stick around when you
>> > duplicate the drm_plane_state again. I still don't really get the real
>> > concerns though ...
>>
>> Properties that magically change values shouldn't exist IMO. I guess if
>> you could have write-only properties or something it migth be sensible?
>
> We can just not return FENCE_FD on get_props, that would make it
> write-only.

We do actually return a value for get_props, but it's -1 which for fds
means "no fd". That's to make sure userspace can save&restore any prop
without causing harm.
-Daniel
Ville Syrjälä April 28, 2016, 6:17 p.m. UTC | #21
On Thu, Apr 28, 2016 at 07:56:19PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 28, 2016 at 11:36:44AM -0300, Gustavo Padovan wrote:
> > 2016-04-27 Daniel Stone <daniel@fooishbar.org>:
> > 
> > > 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.
> > 
> > I also agree to have it as FENCE_FD prop on the plane. Summarizing the
> > arguments on this thread, they are:
> 
> Your "summary" forgot to include any counter arguments.
> 
> > 
> >  - implicit fences also needs one fence per plane/fb, so it will be good to     
> >    match with that.                                                             
> 
> We would actually need a fence per object rather than per fb.

I guess you could overcome this by automagically creating a merged fence
for a multi-obj fb?
Daniel Vetter April 28, 2016, 8:40 p.m. UTC | #22
On Thu, Apr 28, 2016 at 8:17 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> >  - implicit fences also needs one fence per plane/fb, so it will be good to
>> >    match with that.
>>
>> We would actually need a fence per object rather than per fb.
>
> I guess you could overcome this by automagically creating a merged fence
> for a multi-obj fb?

Yeah, and the android hwc does this for you already. You get passed a
surface (or whatever it's called exactly) plus a fence, and the
surface contains the gralloc/native buffer thing, which would contain
multiple dma-buf handles if your hw does planar stuff that way.

I think everyone else who wants explicit fencing will go with the same
or similar model, so it's just about implicit fencing. And there we
can easily construct the fence_collection from a drm_framebuffer
ourselves with a small helper in each driver (or shared one in cma,
although cma doesn't yet grok reserverations/implicitly attached
fences).
-Daniel
Rob Clark April 28, 2016, 9:28 p.m. UTC | #23
On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.

I was kinda more a fan of array too, if for no other reason that to be
consistent w/ how out-fences work.  (And using property just for
in-fence seemed slightly weird/abusive to me)

> 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.

hmm, well we could keep the array per-plane (and if one layer is using
multiple planes, just list the same fd multiple times).. then it
mostly comes down to changes in the ioctl fxn itself.

BR,
-R


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Stone April 29, 2016, 7:48 a.m. UTC | #24
Hi,

On 28 April 2016 at 23:28, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
>>> 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.
>
> I was kinda more a fan of array too, if for no other reason that to be
> consistent w/ how out-fences work.  (And using property just for
> in-fence seemed slightly weird/abusive to me)

I don't think it's really useful to look for much consistency between
the two, beyond the name. I'm more concerned with consistency between
in-fences and the implicit fences on buffers/FBs, and between
out-fences and the page_flip_events.

>> 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.
>
> hmm, well we could keep the array per-plane (and if one layer is using
> multiple planes, just list the same fd multiple times).. then it
> mostly comes down to changes in the ioctl fxn itself.

... and new API in libdrm, which is going to be a serious #ifdef and
distribution pain. The core property API has been available since
2.4.62 last June, but for this we'd have to write the code, wait for
the kernel code, wait for HWC, get everything together, and then merge
and release. That gives minimum one year of libdrm releases which have
had atomic but not in-fence API support, if we're adding a new array.
And I just don't really see what it buys us, apart from the need for
the core atomic_get_property helper to statically return -1 when
requesting FENCE_FD.

Cheers,
Daniel
Greg Hackmann April 29, 2016, 9:14 p.m. UTC | #25
On 04/26/2016 11:39 PM, Daniel Vetter wrote:
>> 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
>

It's not a strong preference on my end.  The 1:1 plane-to-layer mapping 
breaks down somewhat on hardware where you need to split large 
hwcomposer layers across multiple DRM planes.

That said, you can force that case to fit by just dup()ing the fence a 
bunch of times or arbitrarily picking one of the planes to assign the 
fence to.  Either is kludgey, but I can't argue it's kludgey enough to 
justify refactoring a bunch of existing driver code.
Rob Clark April 29, 2016, 10:23 p.m. UTC | #26
On Fri, Apr 29, 2016 at 3:48 AM, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi,
>
> On 28 April 2016 at 23:28, Rob Clark <robdclark@gmail.com> wrote:
>> On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
>>>> 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.
>>
>> I was kinda more a fan of array too, if for no other reason that to be
>> consistent w/ how out-fences work.  (And using property just for
>> in-fence seemed slightly weird/abusive to me)
>
> I don't think it's really useful to look for much consistency between
> the two, beyond the name. I'm more concerned with consistency between
> in-fences and the implicit fences on buffers/FBs, and between
> out-fences and the page_flip_events.
>
>>> 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.
>>
>> hmm, well we could keep the array per-plane (and if one layer is using
>> multiple planes, just list the same fd multiple times).. then it
>> mostly comes down to changes in the ioctl fxn itself.
>
> ... and new API in libdrm, which is going to be a serious #ifdef and
> distribution pain. The core property API has been available since
> 2.4.62 last June, but for this we'd have to write the code, wait for
> the kernel code, wait for HWC, get everything together, and then merge
> and release. That gives minimum one year of libdrm releases which have
> had atomic but not in-fence API support, if we're adding a new array.
> And I just don't really see what it buys us, apart from the need for
> the core atomic_get_property helper to statically return -1 when
> requesting FENCE_FD.

don't we have the same issue for out-fences anyway?

ofc, I suspect we could handle making fences look like properties in
userspace in libdrm (at least if there was a sane way that libdrm
could track and eventually close() old out-fence fd's).  I'm not
entirely sure this matters, I mean how do we make implicit vs explicit
fencing transparent to the compositor and the proto between
compositor<->app?

Admittedly I haven't given *too* much thought yet about the
implications to libdrm and it's users, but it seems like we need to
make a v2 API rev anyway for out-fences, and the compositor is going
to need different codepaths for explicit vs implicit (if it supports
both).  So I don't think in-fences as something other than property
really costs us anything additional?

(Unless there is some sane reason to have an intermediate state w/
in-fences but pageflip events instead of out-fences?  But that seems
odd..)

BR,
-R


> Cheers,
> Daniel
Dominik Behr July 12, 2016, 9:14 p.m. UTC | #27
Gustavo, you added fence_put()
in __drm_atomic_helper_plane_destroy_state(), shouldn't we also add a
corresponding fence_get() in __drm_atomic_helper_plane_duplicate_state() ?

On Fri, Apr 29, 2016 at 3:23 PM, Rob Clark <robdclark@gmail.com> wrote:

> On Fri, Apr 29, 2016 at 3:48 AM, Daniel Stone <daniel@fooishbar.org>
> wrote:
> > Hi,
> >
> > On 28 April 2016 at 23:28, Rob Clark <robdclark@gmail.com> wrote:
> >> On Wed, Apr 27, 2016 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>> On Tue, Apr 26, 2016 at 01:48:02PM -0700, Greg Hackmann wrote:
> >>>> 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.
> >>
> >> I was kinda more a fan of array too, if for no other reason that to be
> >> consistent w/ how out-fences work.  (And using property just for
> >> in-fence seemed slightly weird/abusive to me)
> >
> > I don't think it's really useful to look for much consistency between
> > the two, beyond the name. I'm more concerned with consistency between
> > in-fences and the implicit fences on buffers/FBs, and between
> > out-fences and the page_flip_events.
> >
> >>> 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.
> >>
> >> hmm, well we could keep the array per-plane (and if one layer is using
> >> multiple planes, just list the same fd multiple times).. then it
> >> mostly comes down to changes in the ioctl fxn itself.
> >
> > ... and new API in libdrm, which is going to be a serious #ifdef and
> > distribution pain. The core property API has been available since
> > 2.4.62 last June, but for this we'd have to write the code, wait for
> > the kernel code, wait for HWC, get everything together, and then merge
> > and release. That gives minimum one year of libdrm releases which have
> > had atomic but not in-fence API support, if we're adding a new array.
> > And I just don't really see what it buys us, apart from the need for
> > the core atomic_get_property helper to statically return -1 when
> > requesting FENCE_FD.
>
> don't we have the same issue for out-fences anyway?
>
> ofc, I suspect we could handle making fences look like properties in
> userspace in libdrm (at least if there was a sane way that libdrm
> could track and eventually close() old out-fence fd's).  I'm not
> entirely sure this matters, I mean how do we make implicit vs explicit
> fencing transparent to the compositor and the proto between
> compositor<->app?
>
> Admittedly I haven't given *too* much thought yet about the
> implications to libdrm and it's users, but it seems like we need to
> make a v2 API rev anyway for out-fences, and the compositor is going
> to need different codepaths for explicit vs implicit (if it supports
> both).  So I don't think in-fences as something other than property
> really costs us anything additional?
>
> (Unless there is some sane reason to have an intermediate state w/
> in-fences but pageflip events instead of out-fences?  But that seems
> odd..)
>
> BR,
> -R
>
>
> > Cheers,
> > Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Gustavo Padovan July 12, 2016, 9:21 p.m. UTC | #28
2016-07-12 Dominik Behr <dbehr@chromium.org>:

> Gustavo, you added fence_put()
> in __drm_atomic_helper_plane_destroy_state(), shouldn't we also add a
> corresponding fence_get() in __drm_atomic_helper_plane_duplicate_state() ?

Yes, my new version of the patches that already have an extra
fence_get()

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

	Gustavo
diff mbox

Patch

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;