From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Hi,
Currently the Linux Kernel only have an implicit fencing mechanism, through reservation objects, in which the fences are attached directly to buffers operations and userspace is unaware of what is happening. On the other hand explicit fencing exposes fences to the userspace to handle fencing between producer/consumer explicitely.
To support explicit fencing in the mainline kernel we de-staged the the needed parts of the Android Sync Framework[1] to be able to send and received fences to/from userspace. It has the concept of sync_file that exposes the driver's fences to userspace as file descriptors.
With explicit fencing we have a global mechanism that optimizes the flow of buffers between consumers and producers, avoiding a lot of waiting and some potential desktop freeze. So instead of waiting for a buffer to be processed by the GPU before sending it to DRM in an Atomic IOCTL we can get a sync_file fd from the GPU driver at the moment we submit the buffer processing. The compositor then passes these fds to DRM in an Atomic IOCTL, that will not be displayed until the fences signal, i.e, the GPU finished processing the buffer and it is ready to display. In DRM the fences we wait on before displaying a buffer are called in-fences and they are a per-plane deal.
Vice-versa, we have out-fences to sychronize the return of buffers to GPU (producer) to be processed again. When DRM receives an atomic request with a the OUT_FENCE_PTR property it create a fence attach it to a per-crtc sync_file. It then returns the sync_file fds to userspace. With the fences available userspace can forward these fences to the GPU, where it will wait the fence to signal before starting to process on buffer again. Out-fences are per-crtc.
While these are out-fences in DRM (the consumer) they become in-fences once they get to the GPU (the producer).
DRM explicit fences are opt-in, as the default will still be implicit fencing.
In-fences ---------
In the first discussions on #dri-devel on IRC we decided to hide the Sync Framework from DRM drivers to reduce complexity, so as soon we get the fd via IN_FENCE_FD plane property we convert the sync_file fd to a struct fence. However a sync_file might contain more than one fence, so we created the fence_array concept. struct fence_array is a subclass of struct fence and stores a group of fences that needs to be waited together.
Then we just use the already in place fence waiting support to wait on those fences. Once the producer calls fence_signal() for all fences on wait we can proceed with the atomic commit and display the framebuffers.
Out-fences ----------
Passing a pointer to OUT_FENCE_PTR property in an atomic request enables out-fences. The kernel then creates a fence, attach it to a sync_file and install this file on a unused fd for each crtc. The kernel writes the fd in the memory pointed by the out_fence_ptr provided. In case of error it writes -1.
DRM core use the already in place drm_event infrastructure to help signal fences, we've added a fence pointer to struct drm_pending_event. We signal signal fences when all the buffer related to that CRTC are *on* the screen.
Kernel tree -----------
For those who want all patches on this RFC are in my tree:
https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences
v5 changes ----------
The changes from v5 to v4 are in the patches description.
Userspace ---------
Fences support on drm_hwcomposer is currently a working in progress.
Regards,
Gustavo
[1] https://source.android.com/devices/graphics/implement.html#vsync
--- Gustavo Padovan (4): drm/fence: add in-fences support drm/fence: release fence reference when canceling event drm/fence: add fence timeline to drm_crtc drm/fence: add out-fences support
drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_atomic.c | 138 +++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_atomic_helper.c | 13 ++-- drivers/gpu/drm/drm_crtc.c | 45 ++++++++++++ drivers/gpu/drm/drm_fops.c | 4 ++ drivers/gpu/drm/drm_plane.c | 1 + include/drm/drm_crtc.h | 43 +++++++++++ include/drm/drm_plane.h | 4 +- 8 files changed, 214 insertions(+), 35 deletions(-)
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.
v2: Comments by Daniel Vetter: - remove set state->fence = NULL in destroy phase - accept fence -1 as valid and just return 0 - do not call fence_get() - sync_file_fences_get() already calls it - fence_put() if state->fence is already set, in case userspace set the property more than once.
v3: WARN_ON if fence is set but state has no FB
v4: Comment from Maarten Lankhorst - allow set fence with no related fb
v5: rename FENCE_FD to IN_FENCE_FD
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_atomic.c | 14 ++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------ drivers/gpu/drm/drm_crtc.c | 6 ++++++ drivers/gpu/drm/drm_plane.c | 1 + include/drm/drm_crtc.h | 5 +++++ include/drm/drm_plane.h | 4 ++-- 7 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 483059a..43cb33d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -12,6 +12,7 @@ menuconfig DRM select I2C select I2C_ALGOBIT select DMA_SHARED_BUFFER + select SYNC_FILE help Kernel-level support for the Direct Rendering Infrastructure (DRI) introduced in XFree86 4.0. If you say Y here, you need to select diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5dd7054..b3284b2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_mode.h> #include <drm/drm_plane_helper.h> +#include <linux/sync_file.h>
#include "drm_crtc_internal.h"
@@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, drm_atomic_set_fb_for_plane(state, fb); if (fb) drm_framebuffer_unreference(fb); + } else if (property == config->prop_in_fence_fd) { + if (U642I64(val) == -1) + return 0; + + if (state->in_fence) + fence_put(state->in_fence); + + state->in_fence = sync_file_get_fence(val); + if (!state->in_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); @@ -745,6 +757,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
if (property == config->prop_fb_id) { *val = (state->fb) ? state->fb->base.id : 0; + } else if (property == config->prop_in_fence_fd) { + *val = -1; } else if (property == config->prop_crtc_id) { *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->prop_crtc_x) { diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 07b432f..73464b1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1031,22 +1031,20 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev, if (!pre_swap) plane_state = plane->state;
- if (!plane_state->fence) + if (!plane_state->in_fence) continue;
- WARN_ON(!plane_state->fb); - /* * If waiting for fences pre-swap (ie: nonblock), userspace can * still interrupt the operation. Instead of blocking until the * timer expires, make the wait interruptible. */ - ret = fence_wait(plane_state->fence, pre_swap); + ret = fence_wait(plane_state->in_fence, pre_swap); if (ret) return ret;
- fence_put(plane_state->fence); - plane_state->fence = NULL; + fence_put(plane_state->in_fence); + plane_state->in_fence = NULL; }
return 0; @@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) { if (state->fb) drm_framebuffer_unreference(state->fb); + + if (state->in_fence) + fence_put(state->in_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 60403bf..fcb6453 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_fb_id = prop;
+ prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC, + "IN_FENCE_FD", -1, INT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_in_fence_fd = prop; + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop) diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 249c0ae..3957ef8 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&plane->base, config->prop_fb_id, 0); + drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1); drm_object_attach_property(&plane->base, config->prop_crtc_id, 0); drm_object_attach_property(&plane->base, config->prop_crtc_x, 0); drm_object_attach_property(&plane->base, config->prop_crtc_y, 0); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 284c1b3..279132e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1206,6 +1206,11 @@ struct drm_mode_config { */ struct drm_property *prop_fb_id; /** + * @prop_in_fence_fd: Sync File fd representing the incoming fences + * for a Plane. + */ + struct drm_property *prop_in_fence_fd; + /** * @prop_crtc_id: Default atomic plane property to specify the * &drm_crtc. */ diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 0235390..3cb9bf1 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -34,7 +34,7 @@ struct drm_crtc; * @plane: backpointer to the plane * @crtc: currently bound CRTC, NULL if disabled * @fb: currently bound framebuffer - * @fence: optional fence to wait for before scanning out @fb + * @in_fence: optional fence to wait for before scanning out @fb * @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 @@ -59,7 +59,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; + struct fence *in_fence;
/* Signed dest location allows it to be partially off screen */ int32_t crtc_x, crtc_y;
On Thu, Oct 20, 2016 at 12:50:02PM -0200, 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.
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.
v2: Comments by Daniel Vetter:
- remove set state->fence = NULL in destroy phase
- accept fence -1 as valid and just return 0
- do not call fence_get() - sync_file_fences_get() already calls it
- fence_put() if state->fence is already set, in case userspace
set the property more than once.
v3: WARN_ON if fence is set but state has no FB
v4: Comment from Maarten Lankhorst
- allow set fence with no related fb
v5: rename FENCE_FD to IN_FENCE_FD
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
Since you rename plane_state->fence to in_fence (why exactly?) this breaks compilation for imx and msm. Also there's the question of which fence should win, if there's both explicit and implicit fences. I think the best way to solve that is by adding a helper to set the implicit fence, which drivers are supposed to call. That helper only sets the fence if there's not yet an explicit fence attached. New sequence would be:
- patch1: roll out that helper (e.g. drm_atomic_plane_state_set_fence or whatever) for all drivers using it. - patch2 (this one here): add explicit fences and change the helper to cooporate correctly.
drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_atomic.c | 14 ++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++------ drivers/gpu/drm/drm_crtc.c | 6 ++++++ drivers/gpu/drm/drm_plane.c | 1 + include/drm/drm_crtc.h | 5 +++++ include/drm/drm_plane.h | 4 ++-- 7 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 483059a..43cb33d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -12,6 +12,7 @@ menuconfig DRM select I2C select I2C_ALGOBIT select DMA_SHARED_BUFFER
- select SYNC_FILE help Kernel-level support for the Direct Rendering Infrastructure (DRI) introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5dd7054..b3284b2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_mode.h> #include <drm/drm_plane_helper.h> +#include <linux/sync_file.h>
#include "drm_crtc_internal.h"
@@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, drm_atomic_set_fb_for_plane(state, fb); if (fb) drm_framebuffer_unreference(fb);
- } else if (property == config->prop_in_fence_fd) {
if (U642I64(val) == -1)return 0;if (state->in_fence)fence_put(state->in_fence);state->in_fence = sync_file_get_fence(val);if (!state->in_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);
@@ -745,6 +757,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
if (property == config->prop_fb_id) { *val = (state->fb) ? state->fb->base.id : 0;
- } else if (property == config->prop_in_fence_fd) {
*val = -1;diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 07b432f..73464b1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1031,22 +1031,20 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev, if (!pre_swap) plane_state = plane->state;
if (!plane_state->fence)
if (!plane_state->in_fence) continue;
WARN_ON(!plane_state->fb);- /*
*/
- If waiting for fences pre-swap (ie: nonblock), userspace can
- still interrupt the operation. Instead of blocking until the
- timer expires, make the wait interruptible.
ret = fence_wait(plane_state->fence, pre_swap);
ret = fence_wait(plane_state->in_fence, pre_swap);
fence_put(plane_state->fence);plane_state->fence = NULL;
fence_put(plane_state->in_fence);plane_state->in_fence = NULL;}
return 0;
@@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) { if (state->fb) drm_framebuffer_unreference(state->fb);
- if (state->in_fence)
fence_put(state->in_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 60403bf..fcb6453 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_fb_id = prop;
- prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
"IN_FENCE_FD", -1, INT_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 249c0ae..3957ef8 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1);diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 284c1b3..279132e 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1206,6 +1206,11 @@ struct drm_mode_config { */ struct drm_property *prop_fb_id; /**
* @prop_in_fence_fd: Sync File fd representing the incoming fences* for a Plane.*/- struct drm_property *prop_in_fence_fd;
- /**
*/
- @prop_crtc_id: Default atomic plane property to specify the
- &drm_crtc.
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 0235390..3cb9bf1 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -34,7 +34,7 @@ struct drm_crtc;
- @plane: backpointer to the plane
- @crtc: currently bound CRTC, NULL if disabled
- @fb: currently bound framebuffer
- @fence: optional fence to wait for before scanning out @fb
- @in_fence: optional fence to wait for before scanning out @fb
- @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
@@ -59,7 +59,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;
- struct fence *in_fence;
I think it'd be good to move the kerneldoc into an in-line comment here and expand a bit upon the expected semantics. This is a pretty important part of the atomic uabi!
Otherwise looks all good. -Daniel
/* Signed dest location allows it to be partially off screen */ int32_t crtc_x, crtc_y; -- 2.5.5
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
If the event gets canceled we also need to put away the fence reference it holds.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/drm_fops.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index e84faec..8bed5f4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -663,6 +663,10 @@ void drm_event_cancel_free(struct drm_device *dev, list_del(&p->pending_link); } spin_unlock_irqrestore(&dev->event_lock, flags); + + if (p->fence) + fence_put(p->fence); + kfree(p); } EXPORT_SYMBOL(drm_event_cancel_free);
On Thu, Oct 20, 2016 at 12:50:03PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
If the event gets canceled we also need to put away the fence reference it holds.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
I've broken my local dim scripts right now, so can't apply ;-) -Daniel
drivers/gpu/drm/drm_fops.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index e84faec..8bed5f4 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -663,6 +663,10 @@ void drm_event_cancel_free(struct drm_device *dev, list_del(&p->pending_link); } spin_unlock_irqrestore(&dev->event_lock, flags);
- if (p->fence)
fence_put(p->fence);- kfree(p);
} EXPORT_SYMBOL(drm_event_cancel_free); -- 2.5.5
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Create one timeline context for each CRTC to be able to handle out-fences and signal them. It adds a few members to struct drm_crtc: fence_context, where we store the context we get from fence_context_alloc(), the fence seqno and the fence lock, that we pass in fence_init() to be used by the fence.
v2: Comment by Daniel Stone: - add BUG_ON() to fence_to_crtc() macro
v3: Comment by Ville Syrjälä - Use more meaningful name as crtc timeline name
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 19 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fcb6453..b99090f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif }
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence) +{ + struct drm_crtc *crtc = fence_to_crtc(fence); + + return crtc->dev->driver->name; +} + +static const char *drm_crtc_fence_get_timeline_name(struct fence *fence) +{ + struct drm_crtc *crtc = fence_to_crtc(fence); + + return crtc->timeline_name; +} + +static bool drm_crtc_fence_enable_signaling(struct fence *fence) +{ + return true; +} + +const struct fence_ops drm_crtc_fence_ops = { + .get_driver_name = drm_crtc_fence_get_driver_name, + .get_timeline_name = drm_crtc_fence_get_timeline_name, + .enable_signaling = drm_crtc_fence_enable_signaling, + .wait = fence_default_wait, +}; + /** * drm_crtc_init_with_planes - Initialise a new CRTC object with * specified primary and cursor planes. @@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, return -ENOMEM; }
+ crtc->fence_context = fence_context_alloc(1); + spin_lock_init(&crtc->fence_lock); + snprintf(crtc->timeline_name, sizeof(crtc->timeline_name), + "drm_crtc-%d", crtc->base.id); + crtc->base.properties = &crtc->properties;
list_add_tail(&crtc->head, &config->crtc_list); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 279132e..657a33a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -32,6 +32,8 @@ #include <linux/fb.h> #include <linux/hdmi.h> #include <linux/media-bus-format.h> +#include <linux/srcu.h> +#include <linux/fence.h> #include <uapi/drm/drm_mode.h> #include <uapi/drm/drm_fourcc.h> #include <drm/drm_modeset_lock.h> @@ -618,6 +620,9 @@ struct drm_crtc_funcs { * @gamma_store: gamma ramp values * @helper_private: mid-layer private data * @properties: property tracking for this CRTC + * @fence_context: context for fence signalling + * @fence_lock: fence lock for the fence context + * @fence_seqno: seqno variable to create fences * * Each CRTC may have one or more connectors associated with it. This structure * allows the CRTC to be controlled. @@ -726,8 +731,22 @@ struct drm_crtc { */ struct drm_crtc_crc crc; #endif + + /* fence timelines info for DRM out-fences */ + unsigned int fence_context; + spinlock_t fence_lock; + unsigned long fence_seqno; + char timeline_name[32]; };
+extern const struct fence_ops drm_crtc_fence_ops; + +static inline struct drm_crtc *fence_to_crtc(struct fence *fence) +{ + BUG_ON(fence->ops != &drm_crtc_fence_ops); + return container_of(fence->lock, struct drm_crtc, fence_lock); +} + /** * struct drm_mode_set - new values for a CRTC config change * @fb: framebuffer to use for new config
Hi Gustavo,
On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Create one timeline context for each CRTC to be able to handle out-fences and signal them. It adds a few members to struct drm_crtc: fence_context, where we store the context we get from fence_context_alloc(), the fence seqno and the fence lock, that we pass in fence_init() to be used by the fence.
v2: Comment by Daniel Stone:
- add BUG_ON() to fence_to_crtc() macro
v3: Comment by Ville Syrjälä
- Use more meaningful name as crtc timeline name
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 19 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fcb6453..b99090f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif }
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->dev->driver->name;
+}
+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->timeline_name;
+}
+static bool drm_crtc_fence_enable_signaling(struct fence *fence) +{
- return true;
+}
+const struct fence_ops drm_crtc_fence_ops = {
- .get_driver_name = drm_crtc_fence_get_driver_name,
- .get_timeline_name = drm_crtc_fence_get_timeline_name,
- .enable_signaling = drm_crtc_fence_enable_signaling,
- .wait = fence_default_wait,
+};
/**
- drm_crtc_init_with_planes - Initialise a new CRTC object with
- specified primary and cursor planes.
@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, return -ENOMEM; }
- crtc->fence_context = fence_context_alloc(1);
- spin_lock_init(&crtc->fence_lock);
- snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
"drm_crtc-%d", crtc->base.id);
I wondered about "[CRTC:id:name]" to be consistent with the DRM debug prints.
crtc->base.properties = &crtc->properties;
list_add_tail(&crtc->head, &config->crtc_list);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 279132e..657a33a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -32,6 +32,8 @@ #include <linux/fb.h> #include <linux/hdmi.h> #include <linux/media-bus-format.h> +#include <linux/srcu.h> +#include <linux/fence.h> #include <uapi/drm/drm_mode.h> #include <uapi/drm/drm_fourcc.h> #include <drm/drm_modeset_lock.h> @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
- @gamma_store: gamma ramp values
- @helper_private: mid-layer private data
- @properties: property tracking for this CRTC
- @fence_context: context for fence signalling
- @fence_lock: fence lock for the fence context
- @fence_seqno: seqno variable to create fences
@timeline_name ?
Cheers, Brian
- Each CRTC may have one or more connectors associated with it. This structure
- allows the CRTC to be controlled.
@@ -726,8 +731,22 @@ struct drm_crtc { */ struct drm_crtc_crc crc; #endif
- /* fence timelines info for DRM out-fences */
- unsigned int fence_context;
- spinlock_t fence_lock;
- unsigned long fence_seqno;
- char timeline_name[32];
};
+extern const struct fence_ops drm_crtc_fence_ops;
+static inline struct drm_crtc *fence_to_crtc(struct fence *fence) +{
- BUG_ON(fence->ops != &drm_crtc_fence_ops);
- return container_of(fence->lock, struct drm_crtc, fence_lock);
+}
/**
- struct drm_mode_set - new values for a CRTC config change
- @fb: framebuffer to use for new config
-- 2.5.5
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
2016-10-20 Brian Starkey brian.starkey@arm.com:
Hi Gustavo,
On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Create one timeline context for each CRTC to be able to handle out-fences and signal them. It adds a few members to struct drm_crtc: fence_context, where we store the context we get from fence_context_alloc(), the fence seqno and the fence lock, that we pass in fence_init() to be used by the fence.
v2: Comment by Daniel Stone:
- add BUG_ON() to fence_to_crtc() macro
v3: Comment by Ville Syrjälä
- Use more meaningful name as crtc timeline name
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 19 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fcb6453..b99090f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif }
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->dev->driver->name;
+}
+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->timeline_name;
+}
+static bool drm_crtc_fence_enable_signaling(struct fence *fence) +{
- return true;
+}
+const struct fence_ops drm_crtc_fence_ops = {
- .get_driver_name = drm_crtc_fence_get_driver_name,
- .get_timeline_name = drm_crtc_fence_get_timeline_name,
- .enable_signaling = drm_crtc_fence_enable_signaling,
- .wait = fence_default_wait,
+};
/**
- drm_crtc_init_with_planes - Initialise a new CRTC object with
- specified primary and cursor planes.
@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, return -ENOMEM; }
- crtc->fence_context = fence_context_alloc(1);
- spin_lock_init(&crtc->fence_lock);
- snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
"drm_crtc-%d", crtc->base.id);I wondered about "[CRTC:id:name]" to be consistent with the DRM debug prints.
Yeah, sounds good to me.
crtc->base.properties = &crtc->properties;
list_add_tail(&crtc->head, &config->crtc_list);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 279132e..657a33a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -32,6 +32,8 @@ #include <linux/fb.h> #include <linux/hdmi.h> #include <linux/media-bus-format.h> +#include <linux/srcu.h> +#include <linux/fence.h> #include <uapi/drm/drm_mode.h> #include <uapi/drm/drm_fourcc.h> #include <drm/drm_modeset_lock.h> @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
- @gamma_store: gamma ramp values
- @helper_private: mid-layer private data
- @properties: property tracking for this CRTC
- @fence_context: context for fence signalling
- @fence_lock: fence lock for the fence context
- @fence_seqno: seqno variable to create fences
@timeline_name ?
Sure.
Gustavo
On Thu, Oct 20, 2016 at 12:50:04PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Create one timeline context for each CRTC to be able to handle out-fences and signal them. It adds a few members to struct drm_crtc: fence_context, where we store the context we get from fence_context_alloc(), the fence seqno and the fence lock, that we pass in fence_init() to be used by the fence.
v2: Comment by Daniel Stone:
- add BUG_ON() to fence_to_crtc() macro
v3: Comment by Ville Syrjälä
- Use more meaningful name as crtc timeline name
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 19 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fcb6453..b99090f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -165,6 +165,32 @@ static void drm_crtc_crc_fini(struct drm_crtc *crtc) #endif }
+static const char *drm_crtc_fence_get_driver_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->dev->driver->name;
+}
+static const char *drm_crtc_fence_get_timeline_name(struct fence *fence) +{
- struct drm_crtc *crtc = fence_to_crtc(fence);
- return crtc->timeline_name;
+}
+static bool drm_crtc_fence_enable_signaling(struct fence *fence) +{
- return true;
+}
+const struct fence_ops drm_crtc_fence_ops = {
- .get_driver_name = drm_crtc_fence_get_driver_name,
- .get_timeline_name = drm_crtc_fence_get_timeline_name,
- .enable_signaling = drm_crtc_fence_enable_signaling,
- .wait = fence_default_wait,
+};
/**
- drm_crtc_init_with_planes - Initialise a new CRTC object with
- specified primary and cursor planes.
@@ -222,6 +248,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, return -ENOMEM; }
crtc->fence_context = fence_context_alloc(1);
spin_lock_init(&crtc->fence_lock);
snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),
"drm_crtc-%d", crtc->base.id);crtc->base.properties = &crtc->properties;
list_add_tail(&crtc->head, &config->crtc_list);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 279132e..657a33a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -32,6 +32,8 @@ #include <linux/fb.h> #include <linux/hdmi.h> #include <linux/media-bus-format.h> +#include <linux/srcu.h> +#include <linux/fence.h> #include <uapi/drm/drm_mode.h> #include <uapi/drm/drm_fourcc.h> #include <drm/drm_modeset_lock.h> @@ -618,6 +620,9 @@ struct drm_crtc_funcs {
- @gamma_store: gamma ramp values
- @helper_private: mid-layer private data
- @properties: property tracking for this CRTC
- @fence_context: context for fence signalling
- @fence_lock: fence lock for the fence context
- @fence_seqno: seqno variable to create fences
For new stuff I much prefer in-line kerneldoc with structures, keeps the comments much closer to the code ...
- Each CRTC may have one or more connectors associated with it. This structure
- allows the CRTC to be controlled.
@@ -726,8 +731,22 @@ struct drm_crtc { */ struct drm_crtc_crc crc; #endif
- /* fence timelines info for DRM out-fences */
... and avoids duplicated comments like this one here.
Otherwise lgtm. -Daniel
- unsigned int fence_context;
- spinlock_t fence_lock;
- unsigned long fence_seqno;
- char timeline_name[32];
};
+extern const struct fence_ops drm_crtc_fence_ops;
+static inline struct drm_crtc *fence_to_crtc(struct fence *fence) +{
- BUG_ON(fence->ops != &drm_crtc_fence_ops);
- return container_of(fence->lock, struct drm_crtc, fence_lock);
+}
/**
- struct drm_mode_set - new values for a CRTC config change
- @fb: framebuffer to use for new config
-- 2.5.5
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark: - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter: - Add clean up code for out_fences
v3: Comments by Daniel Vetter: - create DRM_MODE_ATOMIC_EVENT_MASK - userspace should fill out_fences_ptr with the crtc_ids for which it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret; + } else if (property == config->prop_out_fence_ptr) { + state->out_fence_ptr = u64_to_user_ptr(val); } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0; + else if (property == config->prop_out_fence_ptr) + *val = 0; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event( - struct drm_device *dev, struct drm_file *file_priv, - struct fence *fence, uint64_t user_data) + struct drm_device *dev, uint64_t user_data) { struct drm_pending_vblank_event *e = NULL; - int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e) @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) { - ret = drm_event_reserve_init(dev, file_priv, &e->base, - &e->event.base); - if (ret) { - kfree(e); - return NULL; - } - } - - e->base.fence = fence; - return e; }
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state, + struct drm_out_fence_state *fence_state) +{ + struct fence *fence; + + fence_state->fd = get_unused_fd_flags(O_CLOEXEC); + if (fence_state->fd < 0) { + return fence_state->fd; + } + + fence_state->out_fence_ptr = crtc_state->out_fence_ptr; + crtc_state->out_fence_ptr = NULL; + + if (put_user(fence_state->fd, fence_state->out_fence_ptr)) + return -EFAULT; + + fence = kzalloc(sizeof(*fence), GFP_KERNEL); + if (!fence) + return -ENOMEM; + + fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock, + crtc->fence_context, ++crtc->fence_seqno); + + fence_state->sync_file = sync_file_create(fence); + if(!fence_state->sync_file) { + fence_put(fence); + return -ENOMEM; + } + + crtc_state->event->base.fence = fence_get(fence); + return 0; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; + struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0; - unsigned int i, j; + unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { + fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), + GFP_KERNEL); + if (!fence_state) { + ret = -ENOMEM; + goto out; + } + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || + crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
- e = create_vblank_event(dev, file_priv, NULL, - arg->user_data); + e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out; @@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; } + + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + struct drm_pending_vblank_event *e = crtc_state->event; + + if (!file_priv) + continue; + + ret = drm_event_reserve_init(dev, file_priv, &e->base, + &e->event.base); + if (ret) { + kfree(e); + crtc_state->event = NULL; + goto out; + } + } + + if (crtc_state->out_fence_ptr) { + ret = crtc_setup_out_fence(crtc, crtc_state, + &fence_state[fence_idx++]); + if (ret) + goto out; + } }
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { @@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
+ for (i = 0; i < fence_idx; i++) + fd_install(fence_state[i].fd, fence_state[i].sync_file->file); + out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { /* * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, * if they weren't, this code should be called on success * for TEST_ONLY too. */ + if (ret && crtc_state->event) + drm_event_cancel_free(dev, &crtc_state->event->base); + }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc_state->event) - continue; + if (ret && fence_state) { + for (i = 0; i < fence_idx; i++) { + if (fence_state[i].fd >= 0) + put_unused_fd(fence_state[i].fd); + if (fence_state[i].sync_file) + fput(fence_state[i].sync_file->file);
- drm_event_cancel_free(dev, &crtc_state->event->base); + /* If this fails, there's not much we can do about it */ + if (put_user(-1, fence_state->out_fence_ptr)) + DRM_ERROR("Couldn't clear out_fence_ptr\n"); } }
+ kfree(fence_state); + if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); + drm_object_attach_property(&crtc->base, + config->prop_out_fence_ptr, 0); }
return 0; @@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
+ prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC, + "OUT_FENCE_PTR", 0, U64_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_out_fence_ptr = prop; + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
+ u64 __user *out_fence_ptr; + /** * @event: * @@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) }
/** + * struct drm_out_fence_state - store out_fence data for install and clean up + * @out_fence_ptr: user pointer to store the fence fd in. + * @sync_file: sync_file related to the out_fence + * @fd: file descriptor to installed on the sync_file. + */ +struct drm_out_fence_state { + u64 __user *out_fence_ptr; + struct sync_file *sync_file; + int fd; +}; + +/* * struct drm_mode_set - new values for a CRTC config change * @fb: framebuffer to use for new config * @crtc: CRTC whose configuration we're about to change @@ -1230,6 +1244,11 @@ struct drm_mode_config { */ struct drm_property *prop_in_fence_fd; /** + * @prop_out_fence_ptr: Sync File fd pointer representing the + * outgoing fences for a CRTC. + */ + struct drm_property *prop_out_fence_ptr; + /** * @prop_crtc_id: Default atomic plane property to specify the * &drm_crtc. */
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->prop_out_fence_ptr) {
state->out_fence_ptr = u64_to_user_ptr(val);@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->prop_out_fence_ptr)
*val = 0;@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event(
struct drm_device *dev, struct drm_file *file_priv,struct fence *fence, uint64_t user_data)
struct drm_device *dev, uint64_t user_data){ struct drm_pending_vblank_event *e = NULL;
int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) {
ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);return NULL;}- }
- e->base.fence = fence;
- return e;
}
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,struct drm_out_fence_state *fence_state)+{
- struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
return fence_state->fd;- }
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;- fence = kzalloc(sizeof(*fence), GFP_KERNEL);
- if (!fence)
return -ENOMEM;- fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);- fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
fence_put(fence);return -ENOMEM;- }
- crtc_state->event->base.fence = fence_get(fence);
- return 0;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0;
- unsigned int i, j;
unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
ret = -ENOMEM;goto out;- }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
e = create_vblank_event(dev, file_priv, NULL,arg->user_data);
e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out;@@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; }
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {struct drm_pending_vblank_event *e = crtc_state->event;if (!file_priv)continue;ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);crtc_state->event = NULL;goto out;}}if (crtc_state->out_fence_ptr) {ret = crtc_setup_out_fence(crtc, crtc_state,&fence_state[fence_idx++]);if (ret)goto out;}}
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
- for (i = 0; i < fence_idx; i++)
fd_install(fence_state[i].fd, fence_state[i].sync_file->file);out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
- for_each_crtc_in_state(state, crtc, crtc_state, i) { /*
*/
- TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
- if they weren't, this code should be called on success
- for TEST_ONLY too.
if (ret && crtc_state->event)drm_event_cancel_free(dev, &crtc_state->event->base);- }
for_each_crtc_in_state(state, crtc, crtc_state, i) {if (!crtc_state->event)continue;
- if (ret && fence_state) {
for (i = 0; i < fence_idx; i++) {if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);if (fence_state[i].sync_file)fput(fence_state[i].sync_file->file);
drm_event_cancel_free(dev, &crtc_state->event->base);
/* If this fails, there's not much we can do about it */if (put_user(-1, fence_state->out_fence_ptr))DRM_ERROR("Couldn't clear out_fence_ptr\n");} }
kfree(fence_state);
if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
drm_object_attach_property(&crtc->base,config->prop_out_fence_ptr, 0);}
return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
"OUT_FENCE_PTR", 0, U64_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_out_fence_ptr = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
- /**
- @event:
@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) }
/**
- struct drm_out_fence_state - store out_fence data for install and clean up
- @out_fence_ptr: user pointer to store the fence fd in.
- @sync_file: sync_file related to the out_fence
- @fd: file descriptor to installed on the sync_file.
- */
+struct drm_out_fence_state {
- u64 __user *out_fence_ptr;
- struct sync_file *sync_file;
- int fd;
+};
+/*
- struct drm_mode_set - new values for a CRTC config change
- @fb: framebuffer to use for new config
- @crtc: CRTC whose configuration we're about to change
@@ -1230,6 +1244,11 @@ struct drm_mode_config { */ struct drm_property *prop_in_fence_fd; /**
* @prop_out_fence_ptr: Sync File fd pointer representing the* outgoing fences for a CRTC.*/- struct drm_property *prop_out_fence_ptr;
- /**
*/
- @prop_crtc_id: Default atomic plane property to specify the
- &drm_crtc.
-- 2.5.5
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
2016-10-20 Ville Syrjälä ville.syrjala@linux.intel.com:
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
How would the kernel signal these dirty planes then? Right now we signal at the vblank.
Gustavo
On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
2016-10-20 Ville Syrjälä ville.syrjala@linux.intel.com:
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
How would the kernel signal these dirty planes then? Right now we signal at the vblank.
So if I think about it in terms of the previous fbs something like this comes to mind:
starting point: plane a, fb 0 plane b, fb 1
ioctl: plane a, fb 2, fence A plane b, fb 3, fence B
ioctl: plane a, fb 4, fence C -> fb 2 can be reused, so fence C should signal immediately?
vblank: -> fb 0,1 can be reused, so fence A,B signal?
It feels a bit wonky since the fence is effectively associated with the previous fb after the previous fb was already submitted to the kernel. One might assume fence A to be the one signalled early, but that would give the impression that fb 0 would be free for reuse when it's not.
On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
2016-10-20 Ville Syrjälä ville.syrjala@linux.intel.com:
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
How would the kernel signal these dirty planes then? Right now we signal at the vblank.
So if I think about it in terms of the previous fbs something like this comes to mind:
starting point: plane a, fb 0 plane b, fb 1
ioctl: plane a, fb 2, fence A plane b, fb 3, fence B
ioctl: plane a, fb 4, fence C -> fb 2 can be reused, so fence C should signal immediately?
vblank: -> fb 0,1 can be reused, so fence A,B signal?
It feels a bit wonky since the fence is effectively associated with the previous fb after the previous fb was already submitted to the kernel. One might assume fence A to be the one signalled early, but that would give the impression that fb 0 would be free for reuse when it's not.
OK, so after hashing this out on irc with Gustavo and Brian, we came to the conclusion that the per-crtc out fence should be sufficient after all.
The way it can work is that the first ioctl will return the fence, doesn't really matter how many of the planes are involved in this ioctl. Subsequent ioctls that manage to get in before the next vblank will get back a -1 as the fence, even if the set if planes involved is not the same as in the first ioctl. From the -1 userspace can tell what happened, and can then consult its own records to see which still pending flips were overwritten, and thus knows which buffers can be reused immediately.
If userspace plans on sending out the now free buffers to someone else accompanied by a fence, it can just create an already signalled fence to send instead of sending the fence it got back from the atomic ioctl since that one is still associated with the original scanout buffers.
The fence returned by the atomic ioctl will only signal after the vblank, at which point userspace will obviously know that the orginal scanout buffers are now also ready for reuse, and that the last buffer submitted for each plane is now being actively scanned out. So if userspace wants to send out some of the original buffers to someone else, it can send along the fence returned from the atomic ioctl.
So requires a bit more work from userspace perhaps. But obviously it only has to do this if it plans on submitting multiple operations per frame.
So a slightly expanded version of my previous example might look like: starting point: plane a, fb 0 plane b, fb 1
ioctl: crtc: fence A plane a, fb 2 plane b, fb 3
ioctl: crtc: fence -1 plane a, fb 4 -> the previously submitted fb for plane a (fb 2) can be reused
ioctl: crtc: fence -1 plane a, fb 5 -> the previously submitted fb for plane a (fb 4) can be reused
vblank: -> fence A signals, fb 0,1 can be reused fb 3 and 5 being scanned out now
We also had a quick side track w.r.t. variable refresh rate displays, and I think the conclusion was that there the vblank may just happen sooner or later. Nothing else should change. What's unclear is how the refresh rate would be controlled. The two obvious options are explicit control, and automagic based on the submit rate. But that's a separate topic entirely.
On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
2016-10-20 Ville Syrjälä ville.syrjala@linux.intel.com:
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
How would the kernel signal these dirty planes then? Right now we signal at the vblank.
So if I think about it in terms of the previous fbs something like this comes to mind:
starting point: plane a, fb 0 plane b, fb 1
ioctl: plane a, fb 2, fence A plane b, fb 3, fence B
ioctl: plane a, fb 4, fence C -> fb 2 can be reused, so fence C should signal immediately?
vblank: -> fb 0,1 can be reused, so fence A,B signal?
It feels a bit wonky since the fence is effectively associated with the previous fb after the previous fb was already submitted to the kernel. One might assume fence A to be the one signalled early, but that would give the impression that fb 0 would be free for reuse when it's not.
OK, so after hashing this out on irc with Gustavo and Brian, we came to the conclusion that the per-crtc out fence should be sufficient after all.
The way it can work is that the first ioctl will return the fence, doesn't really matter how many of the planes are involved in this ioctl. Subsequent ioctls that manage to get in before the next vblank will get back a -1 as the fence, even if the set if planes involved is not the same as in the first ioctl. From the -1 userspace can tell what happened, and can then consult its own records to see which still pending flips were overwritten, and thus knows which buffers can be reused immediately.
If userspace plans on sending out the now free buffers to someone else accompanied by a fence, it can just create an already signalled fence to send instead of sending the fence it got back from the atomic ioctl since that one is still associated with the original scanout buffers.
The fence returned by the atomic ioctl will only signal after the vblank, at which point userspace will obviously know that the orginal scanout buffers are now also ready for reuse, and that the last buffer submitted for each plane is now being actively scanned out. So if userspace wants to send out some of the original buffers to someone else, it can send along the fence returned from the atomic ioctl.
So requires a bit more work from userspace perhaps. But obviously it only has to do this if it plans on submitting multiple operations per frame.
So a slightly expanded version of my previous example might look like: starting point: plane a, fb 0 plane b, fb 1
ioctl: crtc: fence A plane a, fb 2 plane b, fb 3
ioctl: crtc: fence -1 plane a, fb 4 -> the previously submitted fb for plane a (fb 2) can be reused
ioctl: crtc: fence -1 plane a, fb 5 -> the previously submitted fb for plane a (fb 4) can be reused
vblank: -> fence A signals, fb 0,1 can be reused fb 3 and 5 being scanned out now
We also had a quick side track w.r.t. variable refresh rate displays, and I think the conclusion was that there the vblank may just happen sooner or later. Nothing else should change. What's unclear is how the refresh rate would be controlled. The two obvious options are explicit control, and automagic based on the submit rate. But that's a separate topic entirely.
Thanks a lot for doing this discussion and the detailed write-up. Imo we should bake this into the kerneldoc, as uabi documentation examples. Gustavo, can you pls include this? I'd put it into the kerneldoc for @out_fence, with inline struct comments we can be rather excessive in their lenght ;-) -Daniel
2016-10-21 Daniel Vetter daniel@ffwll.ch:
On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
2016-10-20 Ville Syrjälä ville.syrjala@linux.intel.com:
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
I still maintain the out fence should also be per fb (well, per plane since we can't add it to fbs). Otherwise it's not going to be at all useful if you do fps>vrefresh and don't include the same set of planes in every atomic ioctl, eg. if you only ever include ones that are somehow dirty.
How would the kernel signal these dirty planes then? Right now we signal at the vblank.
So if I think about it in terms of the previous fbs something like this comes to mind:
starting point: plane a, fb 0 plane b, fb 1
ioctl: plane a, fb 2, fence A plane b, fb 3, fence B
ioctl: plane a, fb 4, fence C -> fb 2 can be reused, so fence C should signal immediately?
vblank: -> fb 0,1 can be reused, so fence A,B signal?
It feels a bit wonky since the fence is effectively associated with the previous fb after the previous fb was already submitted to the kernel. One might assume fence A to be the one signalled early, but that would give the impression that fb 0 would be free for reuse when it's not.
OK, so after hashing this out on irc with Gustavo and Brian, we came to the conclusion that the per-crtc out fence should be sufficient after all.
The way it can work is that the first ioctl will return the fence, doesn't really matter how many of the planes are involved in this ioctl. Subsequent ioctls that manage to get in before the next vblank will get back a -1 as the fence, even if the set if planes involved is not the same as in the first ioctl. From the -1 userspace can tell what happened, and can then consult its own records to see which still pending flips were overwritten, and thus knows which buffers can be reused immediately.
If userspace plans on sending out the now free buffers to someone else accompanied by a fence, it can just create an already signalled fence to send instead of sending the fence it got back from the atomic ioctl since that one is still associated with the original scanout buffers.
The fence returned by the atomic ioctl will only signal after the vblank, at which point userspace will obviously know that the orginal scanout buffers are now also ready for reuse, and that the last buffer submitted for each plane is now being actively scanned out. So if userspace wants to send out some of the original buffers to someone else, it can send along the fence returned from the atomic ioctl.
So requires a bit more work from userspace perhaps. But obviously it only has to do this if it plans on submitting multiple operations per frame.
So a slightly expanded version of my previous example might look like: starting point: plane a, fb 0 plane b, fb 1
ioctl: crtc: fence A plane a, fb 2 plane b, fb 3
ioctl: crtc: fence -1 plane a, fb 4 -> the previously submitted fb for plane a (fb 2) can be reused
ioctl: crtc: fence -1 plane a, fb 5 -> the previously submitted fb for plane a (fb 4) can be reused
vblank: -> fence A signals, fb 0,1 can be reused fb 3 and 5 being scanned out now
We also had a quick side track w.r.t. variable refresh rate displays, and I think the conclusion was that there the vblank may just happen sooner or later. Nothing else should change. What's unclear is how the refresh rate would be controlled. The two obvious options are explicit control, and automagic based on the submit rate. But that's a separate topic entirely.
Thanks a lot for doing this discussion and the detailed write-up. Imo we should bake this into the kerneldoc, as uabi documentation examples. Gustavo, can you pls include this? I'd put it into the kerneldoc for @out_fence, with inline struct comments we can be rather excessive in their lenght ;-)
Sure, I'll work on kernel doc for this.
Gustavo
Hi Gustavo,
I notice your branch has the sync_file refcount change in, but this doesn't seem to take account for that. Will you be dropping that change to match the semantics of fence_array?
Couple more comments below.
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->prop_out_fence_ptr) {
state->out_fence_ptr = u64_to_user_ptr(val);@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->prop_out_fence_ptr)
*val = 0;@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event(
struct drm_device *dev, struct drm_file *file_priv,struct fence *fence, uint64_t user_data)
struct drm_device *dev, uint64_t user_data){ struct drm_pending_vblank_event *e = NULL;
int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) {
ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);return NULL;}- }
- e->base.fence = fence;
- return e;
}
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,struct drm_out_fence_state *fence_state)+{
- struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
return fence_state->fd;- }
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;- fence = kzalloc(sizeof(*fence), GFP_KERNEL);
- if (!fence)
return -ENOMEM;- fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);- fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
fence_put(fence);return -ENOMEM;- }
- crtc_state->event->base.fence = fence_get(fence);
- return 0;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0;
- unsigned int i, j;
unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
ret = -ENOMEM;goto out;- }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
e = create_vblank_event(dev, file_priv, NULL,arg->user_data);
e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out;@@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; }
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {struct drm_pending_vblank_event *e = crtc_state->event;if (!file_priv)continue;ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);crtc_state->event = NULL;goto out;}}if (crtc_state->out_fence_ptr) {ret = crtc_setup_out_fence(crtc, crtc_state,&fence_state[fence_idx++]);if (ret)goto out;}}
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
- for (i = 0; i < fence_idx; i++)
fd_install(fence_state[i].fd, fence_state[i].sync_file->file);out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
I think a check on ret is still needed before you iterate the CRTCs. If the commit is successful then state has already been freed by this point, and I get a splat.
/* * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, * if they weren't, this code should be called on success * for TEST_ONLY too. */
if (ret && crtc_state->event)drm_event_cancel_free(dev, &crtc_state->event->base);- }
for_each_crtc_in_state(state, crtc, crtc_state, i) {if (!crtc_state->event)continue;
- if (ret && fence_state) {
for (i = 0; i < fence_idx; i++) {if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);if (fence_state[i].sync_file)fput(fence_state[i].sync_file->file);
drm_event_cancel_free(dev, &crtc_state->event->base);
/* If this fails, there's not much we can do about it */if (put_user(-1, fence_state->out_fence_ptr))DRM_ERROR("Couldn't clear out_fence_ptr\n");} }
kfree(fence_state);
if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
drm_object_attach_property(&crtc->base,config->prop_out_fence_ptr, 0);}
return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
"OUT_FENCE_PTR", 0, U64_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_out_fence_ptr = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
I'm somewhat not convinced about stashing a __user pointer in the CRTC atomic state. I know it gets cleared before the driver sees it, but if anything I think that hints that this isn't the right place to store it.
I've been trying to think of other ways to get from a property to something drm_mode_atomic_ioctl can use - maybe we can store some stuff in drm_atomic_state which only lives for the length of the ioctl call and put this pointer in there.
Thanks, Brian
/** * @event: * @@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) }
/**
- struct drm_out_fence_state - store out_fence data for install and clean up
- @out_fence_ptr: user pointer to store the fence fd in.
- @sync_file: sync_file related to the out_fence
- @fd: file descriptor to installed on the sync_file.
- */
+struct drm_out_fence_state {
- u64 __user *out_fence_ptr;
- struct sync_file *sync_file;
- int fd;
+};
+/*
- struct drm_mode_set - new values for a CRTC config change
- @fb: framebuffer to use for new config
- @crtc: CRTC whose configuration we're about to change
@@ -1230,6 +1244,11 @@ struct drm_mode_config { */ struct drm_property *prop_in_fence_fd; /**
* @prop_out_fence_ptr: Sync File fd pointer representing the* outgoing fences for a CRTC.*/- struct drm_property *prop_out_fence_ptr;
- /**
*/
- @prop_crtc_id: Default atomic plane property to specify the
- &drm_crtc.
-- 2.5.5
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Brian,
2016-10-20 Brian Starkey brian.starkey@arm.com:
Hi Gustavo,
I notice your branch has the sync_file refcount change in, but this doesn't seem to take account for that. Will you be dropping that change to match the semantics of fence_array?
I will drop the fence_get() in the out-fence patch because we already hold the ref when we create the fence. The sync_file refcount patch will remain.
Couple more comments below.
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->prop_out_fence_ptr) {
state->out_fence_ptr = u64_to_user_ptr(val);@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->prop_out_fence_ptr)
*val = 0;@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event(
struct drm_device *dev, struct drm_file *file_priv,struct fence *fence, uint64_t user_data)
struct drm_device *dev, uint64_t user_data){ struct drm_pending_vblank_event *e = NULL;
int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) {
ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);return NULL;}- }
- e->base.fence = fence;
- return e;
}
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,struct drm_out_fence_state *fence_state)+{
- struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
return fence_state->fd;- }
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;- fence = kzalloc(sizeof(*fence), GFP_KERNEL);
- if (!fence)
return -ENOMEM;- fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);- fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
fence_put(fence);return -ENOMEM;- }
- crtc_state->event->base.fence = fence_get(fence);
- return 0;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0;
- unsigned int i, j;
unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
ret = -ENOMEM;goto out;- }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
e = create_vblank_event(dev, file_priv, NULL,arg->user_data);
e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out;@@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; }
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {struct drm_pending_vblank_event *e = crtc_state->event;if (!file_priv)continue;ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);crtc_state->event = NULL;goto out;}}if (crtc_state->out_fence_ptr) {ret = crtc_setup_out_fence(crtc, crtc_state,&fence_state[fence_idx++]);if (ret)goto out;}}
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
- for (i = 0; i < fence_idx; i++)
fd_install(fence_state[i].fd, fence_state[i].sync_file->file);out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
I think a check on ret is still needed before you iterate the CRTCs. If the commit is successful then state has already been freed by this point, and I get a splat.
Yes, I believe I only tested with non-blocking requests.
/* * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, * if they weren't, this code should be called on success * for TEST_ONLY too. */
if (ret && crtc_state->event)drm_event_cancel_free(dev, &crtc_state->event->base);- }
for_each_crtc_in_state(state, crtc, crtc_state, i) {if (!crtc_state->event)continue;
- if (ret && fence_state) {
for (i = 0; i < fence_idx; i++) {if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);if (fence_state[i].sync_file)fput(fence_state[i].sync_file->file);
drm_event_cancel_free(dev, &crtc_state->event->base);
/* If this fails, there's not much we can do about it */if (put_user(-1, fence_state->out_fence_ptr))DRM_ERROR("Couldn't clear out_fence_ptr\n");} }
kfree(fence_state);
if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
drm_object_attach_property(&crtc->base,config->prop_out_fence_ptr, 0);}
return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
"OUT_FENCE_PTR", 0, U64_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_out_fence_ptr = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
I'm somewhat not convinced about stashing a __user pointer in the CRTC atomic state. I know it gets cleared before the driver sees it, but if anything I think that hints that this isn't the right place to store it.
I've been trying to think of other ways to get from a property to something drm_mode_atomic_ioctl can use - maybe we can store some stuff in drm_atomic_state which only lives for the length of the ioctl call and put this pointer in there.
The drm_atomic_state is still visible by the drivers. Between there and crtc_state, I would keep in the crtc_state for now.
Gustavo
Hi Gustavo,
On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
Hi Brian,
2016-10-20 Brian Starkey brian.starkey@arm.com:
Hi Gustavo,
I notice your branch has the sync_file refcount change in, but this doesn't seem to take account for that. Will you be dropping that change to match the semantics of fence_array?
I will drop the fence_get() in the out-fence patch because we already hold the ref when we create the fence. The sync_file refcount patch will remain.
OK, I'll work on that basis, thanks.
Couple more comments below.
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->prop_out_fence_ptr) {
state->out_fence_ptr = u64_to_user_ptr(val);@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->prop_out_fence_ptr)
*val = 0;@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event(
struct drm_device *dev, struct drm_file *file_priv,struct fence *fence, uint64_t user_data)
struct drm_device *dev, uint64_t user_data){ struct drm_pending_vblank_event *e = NULL;
int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) {
ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);return NULL;}- }
- e->base.fence = fence;
- return e;
}
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,struct drm_out_fence_state *fence_state)+{
- struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
return fence_state->fd;- }
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;- fence = kzalloc(sizeof(*fence), GFP_KERNEL);
- if (!fence)
return -ENOMEM;- fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);- fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
fence_put(fence);return -ENOMEM;- }
- crtc_state->event->base.fence = fence_get(fence);
- return 0;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state;
I just hit another crash - looks like fence_state needs to be initialised to NULL here, otherwise if property lookup/set fails we kfree() a bad pointer.
unsigned plane_mask; int ret = 0;
- unsigned int i, j;
unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
ret = -ENOMEM;goto out;- }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
e = create_vblank_event(dev, file_priv, NULL,arg->user_data);
e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out;@@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; }
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {struct drm_pending_vblank_event *e = crtc_state->event;if (!file_priv)continue;ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);crtc_state->event = NULL;goto out;}}if (crtc_state->out_fence_ptr) {ret = crtc_setup_out_fence(crtc, crtc_state,&fence_state[fence_idx++]);if (ret)goto out;}}
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
- for (i = 0; i < fence_idx; i++)
fd_install(fence_state[i].fd, fence_state[i].sync_file->file);out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
I think a check on ret is still needed before you iterate the CRTCs. If the commit is successful then state has already been freed by this point, and I get a splat.
Yes, I believe I only tested with non-blocking requests.
Right, I suppose you should get here before the free in that case.
/* * TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive, * if they weren't, this code should be called on success * for TEST_ONLY too. */
if (ret && crtc_state->event)drm_event_cancel_free(dev, &crtc_state->event->base);- }
for_each_crtc_in_state(state, crtc, crtc_state, i) {if (!crtc_state->event)continue;
- if (ret && fence_state) {
for (i = 0; i < fence_idx; i++) {if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);if (fence_state[i].sync_file)fput(fence_state[i].sync_file->file);
drm_event_cancel_free(dev, &crtc_state->event->base);
/* If this fails, there's not much we can do about it */if (put_user(-1, fence_state->out_fence_ptr))DRM_ERROR("Couldn't clear out_fence_ptr\n");} }
kfree(fence_state);
if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
drm_object_attach_property(&crtc->base,config->prop_out_fence_ptr, 0);}
return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
"OUT_FENCE_PTR", 0, U64_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_out_fence_ptr = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
I'm somewhat not convinced about stashing a __user pointer in the CRTC atomic state. I know it gets cleared before the driver sees it, but if anything I think that hints that this isn't the right place to store it.
I've been trying to think of other ways to get from a property to something drm_mode_atomic_ioctl can use - maybe we can store some stuff in drm_atomic_state which only lives for the length of the ioctl call and put this pointer in there.
The drm_atomic_state is still visible by the drivers. Between there and crtc_state, I would keep in the crtc_state for now.
Mm, yeah I suppose they could get to it if they went looking for it in ->atomic_set_property or something, but I was thinking of freeing it before the drm_atomic_commit.
Anyway, this way is certainly simpler, and setting it to NULL should be enough to limit the damage a driver can do :-)
Thanks, Brian
Gustavo
On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
2016-10-20 Brian Starkey brian.starkey@arm.com:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
I'm somewhat not convinced about stashing a __user pointer in the CRTC atomic state. I know it gets cleared before the driver sees it, but if anything I think that hints that this isn't the right place to store it.
I've been trying to think of other ways to get from a property to something drm_mode_atomic_ioctl can use - maybe we can store some stuff in drm_atomic_state which only lives for the length of the ioctl call and put this pointer in there.
The drm_atomic_state is still visible by the drivers. Between there and crtc_state, I would keep in the crtc_state for now.
Mm, yeah I suppose they could get to it if they went looking for it in ->atomic_set_property or something, but I was thinking of freeing it before the drm_atomic_commit.
Anyway, this way is certainly simpler, and setting it to NULL should be enough to limit the damage a driver can do :-)
+1 on moving this out of drm_crtc_state. drm_atomic_state already has per-crtc structs, so easy to extend them with this. And yes drivers can still see it, but mostly they're not supposed to touch drm_atomic_state internals - the book-keeping is all done by the core.
The per-object state structs otoh are meant to be massively mangled by drivers. -Daniel
Hi,
Sorry, I hit another couple of bugs that originated from my hastebin patch - see below.
On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences by creating a sync_file with a fence for each CRTC that sets the OUT_FENCE_PTR property.
We use the out_fence pointer received in the OUT_FENCE_PTR prop to send the sync_file fd back to userspace.
The sync_file and fd are allocated/created before commit, but the fd_install operation only happens after we know that commit succeed.
In case of error userspace should receive a fd number of -1.
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.
v4: Create OUT_FENCE_PTR properties and remove old approach.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc.c | 8 +++ include/drm/drm_crtc.h | 19 +++++++ 3 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b3284b2..07775fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, &replaced); state->color_mgmt_changed |= replaced; return ret;
- } else if (property == config->prop_out_fence_ptr) {
state->out_fence_ptr = u64_to_user_ptr(val);@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = (state->ctm) ? state->ctm->base.id : 0; else if (property == config->gamma_lut_property) *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
- else if (property == config->prop_out_fence_ptr)
*val = 0;@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit); */
static struct drm_pending_vblank_event *create_vblank_event(
struct drm_device *dev, struct drm_file *file_priv,struct fence *fence, uint64_t user_data)
struct drm_device *dev, uint64_t user_data){ struct drm_pending_vblank_event *e = NULL;
int ret;
e = kzalloc(sizeof *e, GFP_KERNEL); if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event *create_vblank_event( e->event.base.length = sizeof(e->event); e->event.user_data = user_data;
- if (file_priv) {
ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);return NULL;}- }
- e->base.fence = fence;
- return e;
}
@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static int crtc_setup_out_fence(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,struct drm_out_fence_state *fence_state)+{
- struct fence *fence;
- fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
- if (fence_state->fd < 0) {
return fence_state->fd;- }
- fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
- crtc_state->out_fence_ptr = NULL;
- if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;- fence = kzalloc(sizeof(*fence), GFP_KERNEL);
- if (!fence)
return -ENOMEM;- fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,
crtc->fence_context, ++crtc->fence_seqno);- fence_state->sync_file = sync_file_create(fence);
- if(!fence_state->sync_file) {
fence_put(fence);return -ENOMEM;- }
- crtc_state->event->base.fence = fence_get(fence);
- return 0;
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state; unsigned plane_mask; int ret = 0;
- unsigned int i, j;
unsigned int i, j, fence_idx = 0;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1734,12 +1760,19 @@ retry: drm_mode_object_unreference(obj); }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
ret = -ENOMEM;goto out;- }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||crtc_state->out_fence_ptr) { struct drm_pending_vblank_event *e;
e = create_vblank_event(dev, file_priv, NULL,arg->user_data);
e = create_vblank_event(dev, arg->user_data); if (!e) { ret = -ENOMEM; goto out;@@ -1747,6 +1780,28 @@ retry:
crtc_state->event = e; }
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {struct drm_pending_vblank_event *e = crtc_state->event;if (!file_priv)continue;ret = drm_event_reserve_init(dev, file_priv, &e->base,&e->event.base);if (ret) {kfree(e);crtc_state->event = NULL;goto out;}}if (crtc_state->out_fence_ptr) {ret = crtc_setup_out_fence(crtc, crtc_state,&fence_state[fence_idx++]);if (ret)goto out;}}
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
@@ -1761,24 +1816,37 @@ retry: ret = drm_atomic_commit(state); }
- for (i = 0; i < fence_idx; i++)
fd_install(fence_state[i].fd, fence_state[i].sync_file->file);
This loop should be conditional on ret. If drm_atomic_commit fails it doesn't jump over this, it falls into it.
out: drm_atomic_clean_old_fb(dev, plane_mask, ret);
- if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
- for_each_crtc_in_state(state, crtc, crtc_state, i) { /*
*/
- TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive,
- if they weren't, this code should be called on success
- for TEST_ONLY too.
if (ret && crtc_state->event)drm_event_cancel_free(dev, &crtc_state->event->base);- }
for_each_crtc_in_state(state, crtc, crtc_state, i) {if (!crtc_state->event)continue;
- if (ret && fence_state) {
for (i = 0; i < fence_idx; i++) {if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);if (fence_state[i].sync_file)fput(fence_state[i].sync_file->file);
drm_event_cancel_free(dev, &crtc_state->event->base);
/* If this fails, there's not much we can do about it */if (put_user(-1, fence_state->out_fence_ptr))DRM_ERROR("Couldn't clear out_fence_ptr\n");
This should be conditional on fence_state->out_fence_ptr, otherwise it's needlessly noisy for an incompletely filled-in fence_state.
I also wonder if the fput should be first, for symmetry.
Cheers, Brian
}}
- kfree(fence_state);
- if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b99090f..40bce97 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
drm_object_attach_property(&crtc->base,config->prop_out_fence_ptr, 0);}
return 0;
@@ -434,6 +436,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_in_fence_fd = prop;
- prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
"OUT_FENCE_PTR", 0, U64_MAX);- if (!prop)
return -ENOMEM;- dev->mode_config.prop_out_fence_ptr = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, "CRTC_ID", DRM_MODE_OBJECT_CRTC); if (!prop)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 657a33a..b898604 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -165,6 +165,8 @@ struct drm_crtc_state { struct drm_property_blob *ctm; struct drm_property_blob *gamma_lut;
- u64 __user *out_fence_ptr;
- /**
- @event:
@@ -748,6 +750,18 @@ static inline struct drm_crtc *fence_to_crtc(struct fence *fence) }
/**
- struct drm_out_fence_state - store out_fence data for install and clean up
- @out_fence_ptr: user pointer to store the fence fd in.
- @sync_file: sync_file related to the out_fence
- @fd: file descriptor to installed on the sync_file.
- */
+struct drm_out_fence_state {
- u64 __user *out_fence_ptr;
- struct sync_file *sync_file;
- int fd;
+};
+/*
- struct drm_mode_set - new values for a CRTC config change
- @fb: framebuffer to use for new config
- @crtc: CRTC whose configuration we're about to change
@@ -1230,6 +1244,11 @@ struct drm_mode_config { */ struct drm_property *prop_in_fence_fd; /**
* @prop_out_fence_ptr: Sync File fd pointer representing the* outgoing fences for a CRTC.*/- struct drm_property *prop_out_fence_ptr;
- /**
*/
- @prop_crtc_id: Default atomic plane property to specify the
- &drm_crtc.
-- 2.5.5
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Oct 20, 2016 at 12:50:01PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Hi,
Currently the Linux Kernel only have an implicit fencing mechanism, through reservation objects, in which the fences are attached directly to buffers operations and userspace is unaware of what is happening. On the other hand explicit fencing exposes fences to the userspace to handle fencing between producer/consumer explicitely.
To support explicit fencing in the mainline kernel we de-staged the the needed parts of the Android Sync Framework[1] to be able to send and received fences to/from userspace. It has the concept of sync_file that exposes the driver's fences to userspace as file descriptors.
With explicit fencing we have a global mechanism that optimizes the flow of buffers between consumers and producers, avoiding a lot of waiting and some potential desktop freeze. So instead of waiting for a buffer to be processed by the GPU before sending it to DRM in an Atomic IOCTL we can get a sync_file fd from the GPU driver at the moment we submit the buffer processing. The compositor then passes these fds to DRM in an Atomic IOCTL, that will not be displayed until the fences signal, i.e, the GPU finished processing the buffer and it is ready to display. In DRM the fences we wait on before displaying a buffer are called in-fences and they are a per-plane deal.
Vice-versa, we have out-fences to sychronize the return of buffers to GPU (producer) to be processed again. When DRM receives an atomic request with a the OUT_FENCE_PTR property it create a fence attach it to a per-crtc sync_file. It then returns the sync_file fds to userspace. With the fences available userspace can forward these fences to the GPU, where it will wait the fence to signal before starting to process on buffer again. Out-fences are per-crtc.
While these are out-fences in DRM (the consumer) they become in-fences once they get to the GPU (the producer).
DRM explicit fences are opt-in, as the default will still be implicit fencing.
In-fences
In the first discussions on #dri-devel on IRC we decided to hide the Sync Framework from DRM drivers to reduce complexity, so as soon we get the fd via IN_FENCE_FD plane property we convert the sync_file fd to a struct fence. However a sync_file might contain more than one fence, so we created the fence_array concept. struct fence_array is a subclass of struct fence and stores a group of fences that needs to be waited together.
Then we just use the already in place fence waiting support to wait on those fences. Once the producer calls fence_signal() for all fences on wait we can proceed with the atomic commit and display the framebuffers.
Out-fences
Passing a pointer to OUT_FENCE_PTR property in an atomic request enables out-fences. The kernel then creates a fence, attach it to a sync_file and install this file on a unused fd for each crtc. The kernel writes the fd in the memory pointed by the out_fence_ptr provided. In case of error it writes -1.
DRM core use the already in place drm_event infrastructure to help signal fences, we've added a fence pointer to struct drm_pending_event. We signal signal fences when all the buffer related to that CRTC are *on* the screen.
Kernel tree
For those who want all patches on this RFC are in my tree:
https://git.kernel.org/cgit/linux/kernel/git/padovan/linux.git/log/?h=fences
v5 changes
The changes from v5 to v4 are in the patches description.
Userspace
Fences support on drm_hwcomposer is currently a working in progress.
Where are the igts? There's some fairly tricky error recovery and handling code in there, I think we should have testcases for all of them. E.g. out_fence property set, but then some invalid property later on to exercise the error handling paths. Another one would be an atomic update (maybe of a primary plane) which should work, except the fb is way too small. That's checked by core code, but only at ->atomic_check phase, and so could be used to exercise the even later error code.
Other things are mixing up in_fences, out_fences and events in different ways, and making sure it all works out. And then maybe also mix in nonblocking commit vs. blocking commit.
If you need something to generate fences: vgem has them. Chris Wilson can point you at the code he's done in igt for testing the implicit fence support in i915.
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Brian Starkey -
Daniel Vetter -
Gustavo Padovan -
Gustavo Padovan -
Ville Syrjälä