2016-10-28 Daniel Vetter daniel@ffwll.ch:
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.
This was Maarten's suggestion that we should be able to sent a fence to determine when then plane should be disabled. Not sure which usecase he had in mind.
Gustavo