On Fri, Oct 28, 2016 at 09:30:26AM +0200, Daniel Vetter wrote:

On Thu, Oct 27, 2016 at 05:37:06PM -0200, Gustavo Padovan wrote:

...

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

This new function should be used by drivers when setting a implicit fence for the plane. It abstracts the fact that the user might have chosen explicit fencing instead.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

...

---

drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 2 ++ include/drm/drm_plane.h | 2 +- 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index c32fb3c..5e73954 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1147,6 +1147,36 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);

/**

• • drm_atomic_set_fence_for_plane - set fence for plane
• • @plane_state: atomic state object for the plane
• • @fence: dma_fence to use for the plane
• • Helper to setup the plane_state fence in case it is not set yet.
• • By using this drivers doesn't need to worry if the user choose
• • implicit or explicit fencing.
• • This function will not set the fence to the state if it was set
• • via explicit fencing interfaces on the atomic ioctl. It will
• • all drope the reference to the fence as we not storing it
• • anywhere.
• • Otherwise, if plane_state->fence is not set this function we
• • just set it with the received implict fence.
• */

+void +drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,

• 	       struct dma_fence *fence)
  

+{

• if (plane_state->fence) {

• dma_fence_put(fence);
  

• return;
  

• }
• plane_state->fence = fence;

+} +EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);

+/**

• drm_atomic_set_crtc_for_connector - set crtc for connector
• @conn_state: atomic state object for the connector
• @crtc: crtc to use for the connector

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index fc8af53..2d1e9c9 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -345,6 +345,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, struct drm_framebuffer *fb); +void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,

• 		    struct dma_fence *fence);
  

int __must_check drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, struct drm_crtc *crtc); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index c5e8a0d..68f6d22 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -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 dma_fence *fence;

• struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */

Ok, small bikeshed maybe: When you feel the need to add more comments in-line in a struct, then please switch to the in-line kernel-doc member documentation style, and merge the existing kerneldoc together with these additional comments. Would be great to do that with all the others here too. Follow-up patch to address this would be great. -Daniel

...

/* Signed dest location allows it to be partially off screen */ int32_t crtc_x, crtc_y; -- 2.5.5

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Thu, Oct 27, 2016 at 05:37:07PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

drm_atomic_set_fence_for_plane() is smart and won't overwrite plane_state->fence if the user already set an explicit fence there.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Process nit: Please make sure you Cc: driver maintainers for driver patches. Best done by putting the Cc: into the patch, then you never forget ;-)

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/imx/imx-drm-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 98df09c..07fe955 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -158,6 +158,7 @@ static int imx_drm_atomic_commit(struct drm_device *dev, struct drm_plane_state *plane_state; struct drm_plane *plane; struct dma_buf *dma_buf;

• struct dma_fence *fence; int i;

  /*

@@ -170,8 +171,9 @@ static int imx_drm_atomic_commit(struct drm_device *dev, 0)->base.dma_buf; if (!dma_buf) continue;

• 	plane_state->fence =
  

• 		reservation_object_get_excl_rcu(dma_buf->resv);
  

• 	fence = reservation_object_get_excl_rcu(dma_buf->resv);
  

• 	drm_atomic_set_fence_for_plane(plane_state, fence);
  

  } }

-- 2.5.5

On Thu, Oct 27, 2016 at 05:37:08PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

drm_atomic_set_fence_for_plane() is smart and won't overwrite plane_state->fence if the user already set an explicit fence there.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/msm/msm_atomic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index db193f8..4e21e1d 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -217,8 +217,9 @@ int msm_atomic_commit(struct drm_device *dev, if ((plane->state->fb != plane_state->fb) && plane_state->fb) { struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0); struct msm_gem_object *msm_obj = to_msm_bo(obj);

• 	struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
  
  

• 	plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
  

• 	drm_atomic_set_fence_for_plane(plane_state, fence);
  

  } }

-- 2.5.5

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;
  

  } else if (property == config->prop_crtc_id) { *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->prop_crtc_x) {

@@ -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);
  

  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 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

On Fri, Oct 28, 2016 at 09:42:12AM +0200, Daniel Vetter wrote:

On Thu, Oct 27, 2016 at 05:37:10PM -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

v4: Comments by Brian Starkey

• Use even more meaninful name for the crtc timeline
• add doc for timeline_name

Comment by Daniel Vetter

• use in-line style for comments

• rebase after fence -> dma_fence rename

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7878bfd..e2a06c8 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 dma_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 dma_fence *fence) +{

• struct drm_crtc *crtc = fence_to_crtc(fence);
• return crtc->timeline_name;

+}

+static bool drm_crtc_fence_enable_signaling(struct dma_fence *fence) +{

• return true;

+}

+const struct dma_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 = dma_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 = dma_fence_context_alloc(1);

• spin_lock_init(&crtc->fence_lock);

• snprintf(crtc->timeline_name, sizeof(crtc->timeline_name),

•  "CRTC:%d-%s", crtc->base.id, crtc->name);
  

• 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 719b6a8..278dbdd 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/dma-fence.h> #include <uapi/drm/drm_mode.h> #include <uapi/drm/drm_fourcc.h> #include <drm/drm_modeset_lock.h> @@ -726,8 +728,45 @@ struct drm_crtc { */ struct drm_crtc_crc crc; #endif

• /**

• * @fence_context:
  

• *
  

• * timeline context used for fence operations.
  

• */
  

• unsigned int fence_context;
• /**

• * @fence_lock:
  

• *
  

• * spinlock to protect the fences in the fence_context.
  

• */
  

• spinlock_t fence_lock;
• /**

• * @fence_seqno:
  

• *
  

• * Seqno variable used as monotonic counter for the fences
  

• * created on the CRTC's timeline.
  

• */
  

• unsigned long fence_seqno;
• /**

• * @timeline_name:
  

• *
  

• * The name of the CRTC's fence timeline.
  

• */
  

• char timeline_name[32];

};

+extern const struct dma_fence_ops drm_crtc_fence_ops;

Kerneldoc please. With that addressed:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

For my connector fences I exported a function to get the fence, then you can keep the ops and fence_to_crtc private to drm_crtc.c. In that case I think you can drop the BUG_ON.

Either way, lgtm.

Reviewed-by: Brian Starkey brian.starkey@arm.com

...

+static inline struct drm_crtc *fence_to_crtc(struct dma_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

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Thu, Oct 27, 2016 at 05:37:11PM -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.

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.

v5: Comments by Brian Starkey:

• Remove extra fence_get() in atomic_ioctl()

• Check ret before iterating on the crtc_state

• check ret before fd_install

• set fence_state to NULL at the beginning

• check fence_state->out_fence_ptr before put_user()

• change order of fput() and put_unused_fd() on failure

  • Add access_ok() check to the out_fence_ptr received
  • Rebase after fence -> dma_fence rename
  • Store out_fence_ptr in the drm_atomic_state
  • Split crtc_setup_out_fence()
  • return -1 as out_fence with TEST_ONLY flag

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

I think this looks good. Again I'll wait with full in-depth review of all the input validation until the igt shows up, but I think we're mostly covereed. A few smaller comments for polish below. -Daniel

---

drivers/gpu/drm/drm_atomic.c | 199 ++++++++++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_crtc.c | 8 ++ include/drm/drm_atomic.h | 1 + include/drm/drm_crtc.h | 17 ++++ 4 files changed, 196 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 28d9366..9b70a27 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -290,6 +290,46 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_get_crtc_state);

/**

• • drm_atomic_set_out_fence_for_crtc - set CRTC out fence pointer
• • @state: global atomic state object
• • @crtc: crtc to set the out fence pointer
• • @fence_ptr: the userspace pointer to user
• • This function stores the out fence pointer in the atomic state.
• */

+static void +drm_atomic_set_out_fence_for_crtc(struct drm_atomic_state *state,

• 		  struct drm_crtc *crtc, u64 __user *fence_ptr)
  

+{

• state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = fence_ptr;

+}

+/**

• • drm_atomic_get_out_fence_for_crtc - get CRTC out fence pointer
• • @state: global atomic state object
• • @crtc: crtc to set the out fence pointer
• • Get the user pointer that should be used for to store the out fence fd.
• • This function should be called only once per atomic state as it clears
• • the out_fence_ptr store there to prevent drivers to access them.
• • Returns:
• • The out fence user pointer.
• */

We don't document non-driver-visble functions and structs, and especially static functions should be self explanatory. I feel bad for your effort in typing kernel-doc here :(

+static u64 __user * +drm_atomic_get_out_fence_for_crtc(struct drm_atomic_state *state,

• 	     struct drm_crtc *crtc)
  

+{

• u64 __user *fence_ptr;
• fence_ptr = state->crtcs[drm_crtc_index(crtc)].out_fence_ptr;
• state->crtcs[drm_crtc_index(crtc)].out_fence_ptr = NULL;
• return fence_ptr;

+}

+/**

• drm_atomic_set_mode_for_crtc - set mode for CRTC
• @state: the CRTC whose incoming state to update
• @mode: kernel-internal mode to use for the CRTC, or NULL to disable

@@ -490,6 +530,12 @@ 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) {

• uint64_t __user *fence_ptr = u64_to_user_ptr(val);
  

• if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr)))
  

• 	return -EFAULT;
  

• drm_atomic_set_out_fence_for_crtc(state->state, crtc, fence_ptr);
  

  } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else

@@ -532,6 +578,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

@@ -1506,11 +1554,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 dma_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)

@@ -1520,17 +1566,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;

}

@@ -1635,6 +1670,38 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+struct dma_fence *get_crtc_fence(struct drm_crtc *crtc,

• 		 struct drm_crtc_state *crtc_state)
  

static?

+{

• struct dma_fence *fence;
• fence = kzalloc(sizeof(*fence), GFP_KERNEL);
• if (!fence)

• return NULL;
  

• dma_fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,

•        crtc->fence_context, ++crtc->fence_seqno);
  

• return fence;

+}

+static int setup_out_fence(struct drm_out_fence_state *fence_state,

• 	   struct dma_fence *fence)
  

+{

• fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
• if (fence_state->fd < 0)

• return fence_state->fd;
  

• if (put_user(fence_state->fd, fence_state->out_fence_ptr))

• return -EFAULT;
  

• fence_state->sync_file = sync_file_create(fence);
• if(!fence_state->sync_file)

• return -ENOMEM;
  

• return 0;

+}

int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1649,9 +1716,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 = NULL; 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))

@@ -1766,13 +1834,33 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, drm_mode_object_unreference(obj); }

• fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state),

• 	      GFP_KERNEL);
  

• if (!fence_state) {

• ret = -ENOMEM;
  

• goto out;
  

• }

• if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {

• for_each_crtc_in_state(state, crtc, crtc_state, i) {
  

• for_each_crtc_in_state(state, crtc, crtc_state, i) {

• fence_state->out_fence_ptr =
  

• 	drm_atomic_get_out_fence_for_crtc(crtc_state->state, crtc);
  

• if (fence_state->out_fence_ptr &&
  

•     (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
  

• 	if (put_user(-1, fence_state->out_fence_ptr)) {
  

• 		ret = -EFAULT;
  

• 		goto out;
  

• 	}
  

• 	continue;
  

• }
  

• if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
  

•     fence_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;
  

@@ -1780,6 +1868,39 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,

	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 (fence_state->out_fence_ptr) {
  

• 	struct dma_fence *fence;
  

• 	fence = get_crtc_fence(crtc, crtc_state);
  

• 	if (!fence) {
  

• 		ret = -ENOMEM;
  

• 		goto out;
  

• 	}
  

• 	ret = setup_out_fence(&fence_state[fence_idx++], fence);
  

• 	if (ret) {
  

• 		dma_fence_put(fence);
  

• 		goto out;
  

• 	}
  

• 	crtc_state->event->base.fence = fence;
  

• }
  

I think the entire above loop body should be extracted into a helper function, like prepare_crtc_signalling or similar. drm_mode_atomic_ioctl is huge already as-is ;-)

}

if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { @@ -1794,24 +1915,44 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, ret = drm_atomic_commit(state); }

• if (ret)

• goto out;
  

• 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) {

• /*
  

•  * 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) { for_each_crtc_in_state(state, crtc, crtc_state, i) {

• 	if (!crtc_state->event)
  

• 		continue;
  

• 	/*
  

• 	 * 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 (crtc_state->event)
  

• 		drm_event_cancel_free(dev,
  

• 				      &crtc_state->event->base);
  

• }
  
  

• 	drm_event_cancel_free(dev, &crtc_state->event->base);
  

• if (fence_state) {
  

• 	for (i = 0; i < fence_idx; i++) {
  

• 		if (fence_state[i].sync_file)
  

• 			fput(fence_state[i].sync_file->file);
  

• 		if (fence_state[i].fd >= 0)
  

• 			put_unused_fd(fence_state[i].fd);
  

• 		/* If this fails log error to the user */
  

• 		if (fence_state[i].out_fence_ptr &&
  

• 		    put_user(-1, fence_state[i].out_fence_ptr))
  

• 			DRM_INFO("Couldn't clear out_fence_ptr\n");
  

• 	}
  

  }

Same here, for symmetry maybe a unprepare_crtc_signalling or so, to make it clear that this undoes what the first one set up.

}

• 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 e2a06c8..a18d81b 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_atomic.h b/include/drm/drm_atomic.h index 2d1e9c9..ebde60e 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -144,6 +144,7 @@ struct __drm_crtcs_state { struct drm_crtc *ptr; struct drm_crtc_state *state; struct drm_crtc_commit *commit;

• u64 __user *out_fence_ptr;

};

struct __drm_connnectors_state { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 278dbdd..2539394 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -768,6 +768,18 @@ static inline struct drm_crtc *fence_to_crtc(struct dma_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;

+};

You only use this in drm_atomic.c, no need to have it in the header and document it. Please move it into drm_atomic.c, right befoer it's used.

+/*

• 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

@@ -1245,6 +1257,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