diff mbox

[RFC,1/6] drm/fence: add FENCE_FD property to planes

Message ID 1458758847-21170-2-git-send-email-gustavo@padovan.org (mailing list archive)
State New, archived
Headers show

Commit Message

Gustavo Padovan March 23, 2016, 6:47 p.m. UTC
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

FENCE_FD can now be set by the user during an atomic IOCTL, it
will be used by atomic_commit to wait until the sync_file is signalled,
i.e., the framebuffer is ready for scanout.

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

Comments

Inki Dae March 24, 2016, 7:52 a.m. UTC | #1
Hi,

2016? 03? 24? 03:47? Gustavo Padovan ?(?) ? ?:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> FENCE_FD can now be set by the user during an atomic IOCTL, it
> will be used by atomic_commit to wait until the sync_file is signalled,
> i.e., the framebuffer is ready for scanout.

It seems that this patch makes Linux DRM to be dependent of Android which uses explicit sync interfaces.
So was there any a consensus that Android sync driver is used for DMA buffer synchronization as a Linux standard?

I see the Android sync driver exists in staging yet. Isn't the implicit sync interface used for DMA device as a Linux standard?
I don't see why Android specific things are spreaded into Linux DRM.

Thanks,
Inki Dae

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 4 ++++
>  drivers/gpu/drm/drm_atomic_helper.c | 1 +
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 3 +++
>  4 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8fb469c..8bc364c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -609,6 +609,8 @@ 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) {
> +		state->fence_fd = val;
>  	} 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);
> @@ -666,6 +668,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 = state->fence_fd;
>  	} 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 2b430b0..4f91f84 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2594,6 +2594,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>  	if (plane->state) {
>  		plane->state->plane = plane;
>  		plane->state->rotation = BIT(DRM_ROTATE_0);
> +		plane->state->fence_fd = -1;
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258ac..165f199 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1291,6 +1291,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);
> @@ -1546,6 +1547,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 8c7fb3d..a8f6ec0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1239,6 +1239,7 @@ struct drm_connector {
>   * @crtc: currently bound CRTC, NULL if disabled
>   * @fb: currently bound framebuffer
>   * @fence: optional fence to wait for before scanning out @fb
> + * @fence_fd: fd representing the sync_fence
>   * @crtc_x: left position of visible portion of plane on crtc
>   * @crtc_y: upper position of visible portion of plane on crtc
>   * @crtc_w: width of visible portion of plane on crtc
> @@ -1257,6 +1258,7 @@ struct drm_plane_state {
>  	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
>  	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
>  	struct fence *fence;
> +	int fence_fd;
>  
>  	/* Signed dest location allows it to be partially off screen */
>  	int32_t crtc_x, crtc_y;
> @@ -2098,6 +2100,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;
>
Rob Clark April 5, 2016, 12:36 p.m. UTC | #2
On Wed, Mar 23, 2016 at 2:47 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> FENCE_FD can now be set by the user during an atomic IOCTL, it
> will be used by atomic_commit to wait until the sync_file is signalled,
> i.e., the framebuffer is ready for scanout.

ok, so I've been slacking on writing up the reasons that I don't like
the idea of using a property for fd's (including fence fd's).. I did
at one point assume we'd use properties for fence fd's, but that idea
falls apart pretty quickly when you think about the 'struct file' vs
fd lifecycle.  It could *possibly* work if it was a write-only
property, but I don't think that is a good solution.

The issue is that 'struct file' / 'int fd' have a fundamentally
different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
objects (like framebuffers) where we can use them with properties, the
id is tied to the kernel side object.  This is not true for file
descriptors.  Resulting in a few problems:

1) if it was a readable property, reading it would (should) take a
reference..  we can't just return fence_fd that was previously set
(since in the mean time userspace might have close()d the fd).  So we
have to return a fresh fd.  And if userspace (like modetest) doesn't
realize it is responsible to close() that fd we have a leak

2) basically we shouldn't be tracking fd's on the kernel side, since
we can only count on them being valid during the duration of the ioctl
call.  Once the call returns, you must assume userspace has close()d
the fd.  So in the ioctl we need to convert fd -> file, and from then
on out track the file object (or in this case the underlying fence
object).

I guess we *could* have something analogous to dmabuf, where we import
the fence fd and get a kms drm_fence object (with an id tied to the
kernel side object), and then use a property to attach the drm_fence
object.. but that seems kind of pointless and just trying to force the
'everything is a property' mantra.

I think it is really better to pass in an array of 'struct { u32
plane; int fd }' (which the atomic ioctl code converts into 'struct
fence's and attaches to the plane state) and an array of 'struct { u32
crtc; int fd }' filled in on the kernel side for the out-fences.

(I guess I can't think of any case where we'd have more out-fences
than in-fences so we could possibly re-use the same array.. but I
don't think we have to care about that sort of micro-optimization..
on the gpu side, the submit/execbuf ioctl already passes in a larger
table of gem obj id's and reloc info, and that happens even more
frequently than atomic ioctl.)

BR,
-R

> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 4 ++++
>  drivers/gpu/drm/drm_atomic_helper.c | 1 +
>  drivers/gpu/drm/drm_crtc.c          | 7 +++++++
>  include/drm/drm_crtc.h              | 3 +++
>  4 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8fb469c..8bc364c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -609,6 +609,8 @@ 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) {
> +               state->fence_fd = val;
>         } 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);
> @@ -666,6 +668,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 = state->fence_fd;
>         } 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 2b430b0..4f91f84 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2594,6 +2594,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>         if (plane->state) {
>                 plane->state->plane = plane;
>                 plane->state->rotation = BIT(DRM_ROTATE_0);
> +               plane->state->fence_fd = -1;
>         }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258ac..165f199 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1291,6 +1291,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);
> @@ -1546,6 +1547,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 8c7fb3d..a8f6ec0 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1239,6 +1239,7 @@ struct drm_connector {
>   * @crtc: currently bound CRTC, NULL if disabled
>   * @fb: currently bound framebuffer
>   * @fence: optional fence to wait for before scanning out @fb
> + * @fence_fd: fd representing the sync_fence
>   * @crtc_x: left position of visible portion of plane on crtc
>   * @crtc_y: upper position of visible portion of plane on crtc
>   * @crtc_w: width of visible portion of plane on crtc
> @@ -1257,6 +1258,7 @@ struct drm_plane_state {
>         struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
>         struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
>         struct fence *fence;
> +       int fence_fd;
>
>         /* Signed dest location allows it to be partially off screen */
>         int32_t crtc_x, crtc_y;
> @@ -2098,6 +2100,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.0
>
Daniel Stone April 5, 2016, 12:57 p.m. UTC | #3
Hi,

On 5 April 2016 at 13:36, Rob Clark <robdclark@gmail.com> wrote:
> ok, so I've been slacking on writing up the reasons that I don't like
> the idea of using a property for fd's (including fence fd's).. I did
> at one point assume we'd use properties for fence fd's, but that idea
> falls apart pretty quickly when you think about the 'struct file' vs
> fd lifecycle.  It could *possibly* work if it was a write-only
> property, but I don't think that is a good solution.

I've been assuming that it would have to be write-only; I don't
believe there would be any meaningful usecases for read. Do you have
any in mind, or is it just a symmetry/cleanliness thing?

> The issue is that 'struct file' / 'int fd' have a fundamentally
> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
> objects (like framebuffers) where we can use them with properties, the
> id is tied to the kernel side object.  This is not true for file
> descriptors.  Resulting in a few problems:

Surely the fence FD tied to the kernel-side struct fence, in exactly
the same way, no?

> 1) if it was a readable property, reading it would (should) take a
> reference..  we can't just return fence_fd that was previously set
> (since in the mean time userspace might have close()d the fd).  So we
> have to return a fresh fd.  And if userspace (like modetest) doesn't
> realize it is responsible to close() that fd we have a leak

Again, assuming that read would always return -1.

> 2) basically we shouldn't be tracking fd's on the kernel side, since
> we can only count on them being valid during the duration of the ioctl
> call.  Once the call returns, you must assume userspace has close()d
> the fd.  So in the ioctl we need to convert fd -> file, and from then
> on out track the file object (or in this case the underlying fence
> object).

Right, it would have to be the same as KMS objects, where userspace
passes in an ID (in this case an entry in a non-IDR table, but still),
and the kernel maps that to struct fence and handles the lifetime. So,
almost exactly the same as what we do with KMS objects ...

> I guess we *could* have something analogous to dmabuf, where we import
> the fence fd and get a kms drm_fence object (with an id tied to the
> kernel side object), and then use a property to attach the drm_fence
> object.. but that seems kind of pointless and just trying to force the
> 'everything is a property' mantra.

I think that would be pointless indirection as well.

> I think it is really better to pass in an array of 'struct { u32
> plane; int fd }' (which the atomic ioctl code converts into 'struct
> fence's and attaches to the plane state) and an array of 'struct { u32
> crtc; int fd }' filled in on the kernel side for the out-fences.

Mmm, it definitely makes ioctl parsing harder, and still you rely on
drivers to implement the more-difficult-to-not-screw-up part, which
(analagous to crtc_state->event) is the driver managing the lifecycle
of that component of the state. We already enforce this with events
though, and the difficult part wasn't in the userspace interface, but
the backend handling. This doesn't change at all regardless of whether
we use a property or an external array, so ...

Cheers,
Daniel
Rob Clark April 5, 2016, 2:04 p.m. UTC | #4
On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi,
>
> On 5 April 2016 at 13:36, Rob Clark <robdclark@gmail.com> wrote:
>> ok, so I've been slacking on writing up the reasons that I don't like
>> the idea of using a property for fd's (including fence fd's).. I did
>> at one point assume we'd use properties for fence fd's, but that idea
>> falls apart pretty quickly when you think about the 'struct file' vs
>> fd lifecycle.  It could *possibly* work if it was a write-only
>> property, but I don't think that is a good solution.
>
> I've been assuming that it would have to be write-only; I don't
> believe there would be any meaningful usecases for read. Do you have
> any in mind, or is it just a symmetry/cleanliness thing?

no, don't see a use-case for it.. but this patch didn't seem to be
preventing it.  And it was storing the fence_fd on the kernel side
which is a no-no as well.

>> The issue is that 'struct file' / 'int fd' have a fundamentally
>> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
>> objects (like framebuffers) where we can use them with properties, the
>> id is tied to the kernel side object.  This is not true for file
>> descriptors.  Resulting in a few problems:
>
> Surely the fence FD tied to the kernel-side struct fence, in exactly
> the same way, no?

well, what I mean is you can't keep around the int fd on the kernel
side, like this patch does

A write-only property, which immediately (ie. during the ioctl call)
is converted into a fence object, would work.  Although given that we
need to handle fences differently (ie. not a property) for out-fences,
it seems odd to shoehorn them into a property for in-fences.

>> 1) if it was a readable property, reading it would (should) take a
>> reference..  we can't just return fence_fd that was previously set
>> (since in the mean time userspace might have close()d the fd).  So we
>> have to return a fresh fd.  And if userspace (like modetest) doesn't
>> realize it is responsible to close() that fd we have a leak
>
> Again, assuming that read would always return -1.
>
>> 2) basically we shouldn't be tracking fd's on the kernel side, since
>> we can only count on them being valid during the duration of the ioctl
>> call.  Once the call returns, you must assume userspace has close()d
>> the fd.  So in the ioctl we need to convert fd -> file, and from then
>> on out track the file object (or in this case the underlying fence
>> object).
>
> Right, it would have to be the same as KMS objects, where userspace
> passes in an ID (in this case an entry in a non-IDR table, but still),
> and the kernel maps that to struct fence and handles the lifetime. So,
> almost exactly the same as what we do with KMS objects ...
>
>> I guess we *could* have something analogous to dmabuf, where we import
>> the fence fd and get a kms drm_fence object (with an id tied to the
>> kernel side object), and then use a property to attach the drm_fence
>> object.. but that seems kind of pointless and just trying to force the
>> 'everything is a property' mantra.
>
> I think that would be pointless indirection as well.
>
>> I think it is really better to pass in an array of 'struct { u32
>> plane; int fd }' (which the atomic ioctl code converts into 'struct
>> fence's and attaches to the plane state) and an array of 'struct { u32
>> crtc; int fd }' filled in on the kernel side for the out-fences.
>
> Mmm, it definitely makes ioctl parsing harder, and still you rely on
> drivers to implement the more-difficult-to-not-screw-up part, which
> (analagous to crtc_state->event) is the driver managing the lifecycle
> of that component of the state. We already enforce this with events
> though, and the difficult part wasn't in the userspace interface, but
> the backend handling. This doesn't change at all regardless of whether
> we use a property or an external array, so ...

hmm, I'm assuming that the in/out arrays are handled in
drm_mode_atomic_ioctl() and the drivers never see 'em..

BR,
-R

> Cheers,
> Daniel
Daniel Stone April 5, 2016, 2:19 p.m. UTC | #5
Hi,

On 5 April 2016 at 15:04, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone <daniel@fooishbar.org> wrote:
>> I've been assuming that it would have to be write-only; I don't
>> believe there would be any meaningful usecases for read. Do you have
>> any in mind, or is it just a symmetry/cleanliness thing?
>
> no, don't see a use-case for it.. but this patch didn't seem to be
> preventing it.  And it was storing the fence_fd on the kernel side
> which is a no-no as well.

Yeah, both should be fixed. :)

>>> The issue is that 'struct file' / 'int fd' have a fundamentally
>>> different lifecycle compared to 'kms obj' / 'u32 obj_id'.  For the kms
>>> objects (like framebuffers) where we can use them with properties, the
>>> id is tied to the kernel side object.  This is not true for file
>>> descriptors.  Resulting in a few problems:
>>
>> Surely the fence FD tied to the kernel-side struct fence, in exactly
>> the same way, no?
>
> well, what I mean is you can't keep around the int fd on the kernel
> side, like this patch does
>
> A write-only property, which immediately (ie. during the ioctl call)
> is converted into a fence object, would work.  Although given that we
> need to handle fences differently (ie. not a property) for out-fences,
> it seems odd to shoehorn them into a property for in-fences.

Depends on how you look at it, I guess. From the point of view of all
fence-like things being consistent, yes, it falls down. But from the
point of view of in-fences being attached to an FB, and out-fences
(like events) being separately attached to a request, it's a lot more
consistent.

>>> I think it is really better to pass in an array of 'struct { u32
>>> plane; int fd }' (which the atomic ioctl code converts into 'struct
>>> fence's and attaches to the plane state) and an array of 'struct { u32
>>> crtc; int fd }' filled in on the kernel side for the out-fences.
>>
>> Mmm, it definitely makes ioctl parsing harder, and still you rely on
>> drivers to implement the more-difficult-to-not-screw-up part, which
>> (analagous to crtc_state->event) is the driver managing the lifecycle
>> of that component of the state. We already enforce this with events
>> though, and the difficult part wasn't in the userspace interface, but
>> the backend handling. This doesn't change at all regardless of whether
>> we use a property or an external array, so ...
>
> hmm, I'm assuming that the in/out arrays are handled in
> drm_mode_atomic_ioctl() and the drivers never see 'em..

True, but it complicates the (already not hugely straightforward)
parsing that the ioctl has to do. It also makes extending the ioctl a
little harder to do in future, because you're adding in two
variable-size elements, and have to do some fairly complicated parsing
to even figure out what the size _should_ be. So I'd rather not do it
if there was any way out of it; at the very least it'd have to be
userptr rather than array.

Cheers,
Daniel
Rob Clark April 5, 2016, 3:23 p.m. UTC | #6
On Tue, Apr 5, 2016 at 10:19 AM, Daniel Stone <daniel@fooishbar.org> wrote:
>
>> hmm, I'm assuming that the in/out arrays are handled in
>> drm_mode_atomic_ioctl() and the drivers never see 'em..
>
> True, but it complicates the (already not hugely straightforward)
> parsing that the ioctl has to do. It also makes extending the ioctl a
> little harder to do in future, because you're adding in two
> variable-size elements, and have to do some fairly complicated parsing
> to even figure out what the size _should_ be. So I'd rather not do it
> if there was any way out of it; at the very least it'd have to be
> userptr rather than array.


oh, I was assuming ptr to a user array (rather than var length ioctl
struct) for both in and out fences.  I guess my wording didn't make
that clear.  But yeah, it would be madness to do anything else.

BR,
-R
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8fb469c..8bc364c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -609,6 +609,8 @@  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) {
+		state->fence_fd = val;
 	} 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);
@@ -666,6 +668,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 = state->fence_fd;
 	} 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 2b430b0..4f91f84 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2594,6 +2594,7 @@  void drm_atomic_helper_plane_reset(struct drm_plane *plane)
 	if (plane->state) {
 		plane->state->plane = plane;
 		plane->state->rotation = BIT(DRM_ROTATE_0);
+		plane->state->fence_fd = -1;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65258ac..165f199 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1291,6 +1291,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);
@@ -1546,6 +1547,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 8c7fb3d..a8f6ec0 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1239,6 +1239,7 @@  struct drm_connector {
  * @crtc: currently bound CRTC, NULL if disabled
  * @fb: currently bound framebuffer
  * @fence: optional fence to wait for before scanning out @fb
+ * @fence_fd: fd representing the sync_fence
  * @crtc_x: left position of visible portion of plane on crtc
  * @crtc_y: upper position of visible portion of plane on crtc
  * @crtc_w: width of visible portion of plane on crtc
@@ -1257,6 +1258,7 @@  struct drm_plane_state {
 	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
 	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
 	struct fence *fence;
+	int fence_fd;
 
 	/* Signed dest location allows it to be partially off screen */
 	int32_t crtc_x, crtc_y;
@@ -2098,6 +2100,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;