On Thu, Oct 27, 2016 at 05:37:09PM -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
v6: Comments by Daniel Vetter:
rename plane_state->in_fence back to "fence"
- rebase after fence -> dma_fence rename
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
Looks good, but I'll wait with full review with all the fence patches until the igts show up. It's much easier to check for gabs in input validation code if you also have the testcases at hand. A few comments below. -Daniel
drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_atomic.c | 15 +++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 5 +++-- drivers/gpu/drm/drm_crtc.c | 6 ++++++ drivers/gpu/drm/drm_plane.c | 1 + include/drm/drm_crtc.h | 5 +++++ 6 files changed, 31 insertions(+), 2 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 5e73954..28d9366 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->fence)dma_fence_put(state->fence);state->fence = sync_file_get_fence(val);if (!state->fence)return -EINVAL;- } else if (property == config->prop_crtc_id) { struct drm_crtc *crtc = drm_crtc_find(dev, val); return drm_atomic_set_crtc_for_plane(state, crtc);
@@ -747,6 +759,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;@@ -1752,6 +1766,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, drm_mode_object_unreference(obj); }
Spurios whitespace.
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { for_each_crtc_in_state(state, crtc, crtc_state, i) { struct drm_pending_vblank_event *e; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 75ad01d..c34da9e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1034,8 +1034,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev, if (!plane_state->fence) continue;
WARN_ON(!plane_state->fb);
Why this? We don't allow a plane to be enabled without an fb, and adding a fence to a plane which is disabled sounds likea bug. We probably should have a bit of code in the core atomic check code to make sure userspace never asks for a fence when the plane is off.
/* * If waiting for fences pre-swap (ie: nonblock), userspace can * still interrupt the operation. Instead of blocking until the@@ -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->fence)
dma_fence_put(state->fence);} EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 13441e2..7878bfd 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 fa1aa21..719b6a8 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1201,6 +1201,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.
-- 2.5.5