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

[Rob Clark]

On Wed, Mar 23, 2016 at 2:47 PM, Gustavo Padovan <gustavo@xxxxxxxxxxx> wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
>  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
>