[PATCH 0/6] Cursor support with universal planes (v4)

[PATCH 0/6] Cursor support with universal planes (v4)

[Matt Roper]

Rebasing to the latest drm-intel-nightly and resending at the request of the
reviewers.  Daniel has already provided an r-b for the first five patches here,
so #6 is the main functionality awaiting review.

Matt Roper (6):
  drm: Refactor framebuffer creation to allow internal use (v2)
  drm: Refactor setplane to allow internal use (v3)
  drm: Support legacy cursor ioctls via universal planes when possible
    (v4)
  drm: Allow drivers to register cursor planes with crtc
  drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer (v2)
  drm/i915: Switch to unified plane cursor handling (v4)

 drivers/gpu/drm/drm_crtc.c           | 357 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c | 170 +++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |   1 -
 include/drm/drm_crtc.h               |   6 +-
 4 files changed, 390 insertions(+), 144 deletions(-)

-- 
1.8.5.1

[PATCH 1/6] drm: Refactor framebuffer creation to allow internal use (v2)

[Matt Roper]

Refactor DRM framebuffer creation into a new function that returns a
struct drm_framebuffer directly.  The upcoming universal cursor support
will want to create framebuffers internally to wrap cursor buffers, so
we want to be able to share that framebuffer creation with the
drm_mode_addfb2 ioctl handler.

v2: Take struct drm_mode_fb_cmd2 parameter directly rather than void*

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 69 ++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fe94cc1..5a88267 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -41,6 +41,10 @@
 
 #include "drm_crtc_internal.h"
 
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+							struct drm_mode_fb_cmd2 *r,
+							struct drm_file *file_priv);
+
 /**
  * drm_modeset_lock_all - take all modeset locks
  * @dev: drm device
@@ -2827,56 +2831,38 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 	return 0;
 }
 
-/**
- * drm_mode_addfb2 - add an FB to the graphics configuration
- * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
- *
- * Add a new FB to the specified CRTC, given a user request with format. This is
- * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
- * and uses fourcc codes as pixel format specifiers.
- *
- * Called by the user via ioctl.
- *
- * Returns:
- * Zero on success, errno on failure.
- */
-int drm_mode_addfb2(struct drm_device *dev,
-		    void *data, struct drm_file *file_priv)
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+							struct drm_mode_fb_cmd2 *r,
+							struct drm_file *file_priv)
 {
-	struct drm_mode_fb_cmd2 *r = data;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_framebuffer *fb;
 	int ret;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return -EINVAL;
-
 	if (r->flags & ~DRM_MODE_FB_INTERLACED) {
 		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	if ((config->min_width > r->width) || (r->width > config->max_width)) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
 			  r->width, config->min_width, config->max_width);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 	if ((config->min_height > r->height) || (r->height > config->max_height)) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
 			  r->height, config->min_height, config->max_height);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	ret = framebuffer_check(r);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
-		return PTR_ERR(fb);
+		return fb;
 	}
 
 	mutex_lock(&file_priv->fbs_lock);
@@ -2885,8 +2871,37 @@ int drm_mode_addfb2(struct drm_device *dev,
 	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
 	mutex_unlock(&file_priv->fbs_lock);
 
+	return fb;
+}
 
-	return ret;
+/**
+ * drm_mode_addfb2 - add an FB to the graphics configuration
+ * @dev: drm device for the ioctl
+ * @data: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Add a new FB to the specified CRTC, given a user request with format. This is
+ * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
+ * and uses fourcc codes as pixel format specifiers.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_addfb2(struct drm_device *dev,
+		    void *data, struct drm_file *file_priv)
+{
+	struct drm_framebuffer *fb;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	fb = add_framebuffer_internal(dev, data, file_priv);
+	if (IS_ERR(fb))
+		return PTR_ERR(fb);
+
+	return 0;
 }
 
 /**
-- 
1.8.5.1

[PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Matt Roper]

Refactor DRM setplane code into a new setplane_internal() function that
takes DRM objects directly as parameters rather than looking them up by
ID.  We'll use this in a future patch when we implement legacy cursor
ioctls on top of the universal plane interface.

v3:
 - Move integer overflow checking from setplane_internal to setplane
   ioctl.  The upcoming legacy cursor support via universal planes needs
   to maintain current cursor ioctl semantics and not return error for
   these extreme values (found via intel-gpu-tools kms_cursor_crc test).
v2:
 - Allow planes to be disabled without a valid crtc again (and add
   mention of this to setplane's kerneldoc, since it doesn't seem to be
   mentioned anywhere else).
 - Reformat some parameter line wrap

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 176 ++++++++++++++++++++++++++-------------------
 1 file changed, 102 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5a88267..27eae03 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2122,45 +2122,32 @@ out:
 	return ret;
 }
 
-/**
- * drm_mode_setplane - configure a plane's configuration
- * @dev: DRM device
- * @data: ioctl data*
- * @file_priv: DRM file info
+/*
+ * setplane_internal - setplane handler for internal callers
  *
- * Set plane configuration, including placement, fb, scaling, and other factors.
- * Or pass a NULL fb to disable.
+ * Note that we assume an extra reference has already been taken on fb.  If the
+ * update fails, this reference will be dropped before return; if it succeeds,
+ * the previous framebuffer (if any) will be unreferenced instead.
  *
- * Returns:
- * Zero on success, errno on failure.
+ * src_{x,y,w,h} are provided in 16.16 fixed point format
  */
-int drm_mode_setplane(struct drm_device *dev, void *data,
-		      struct drm_file *file_priv)
+static int setplane_internal(struct drm_crtc *crtc,
+			     struct drm_plane *plane,
+			     struct drm_framebuffer *fb,
+			     int32_t crtc_x, int32_t crtc_y,
+			     uint32_t crtc_w, uint32_t crtc_h,
+			     /* src_{x,y,w,h} values are 16.16 fixed point */
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
 {
-	struct drm_mode_set_plane *plane_req = data;
-	struct drm_plane *plane;
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *old_fb = NULL;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return -EINVAL;
-
-	/*
-	 * First, find the plane, crtc, and fb objects.  If not available,
-	 * we don't bother to call the driver.
-	 */
-	plane = drm_plane_find(dev, plane_req->plane_id);
-	if (!plane) {
-		DRM_DEBUG_KMS("Unknown plane ID %d\n",
-			      plane_req->plane_id);
-		return -ENOENT;
-	}
-
 	/* No fb means shut it down */
-	if (!plane_req->fb_id) {
+	if (!fb) {
 		drm_modeset_lock_all(dev);
 		old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane);
@@ -2174,14 +2161,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	crtc = drm_crtc_find(dev, plane_req->crtc_id);
-	if (!crtc) {
-		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
-			      plane_req->crtc_id);
-		ret = -ENOENT;
-		goto out;
-	}
-
 	/* Check whether this plane is usable on this CRTC */
 	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
 		DRM_DEBUG_KMS("Invalid crtc for plane\n");
@@ -2189,14 +2168,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
-	if (!fb) {
-		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
-			      plane_req->fb_id);
-		ret = -ENOENT;
-		goto out;
-	}
-
 	/* Check whether this plane supports the fb pixel format. */
 	for (i = 0; i < plane->format_count; i++)
 		if (fb->pixel_format == plane->format_types[i])
@@ -2212,43 +2183,25 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	fb_height = fb->height << 16;
 
 	/* Make sure source coordinates are inside the fb. */
-	if (plane_req->src_w > fb_width ||
-	    plane_req->src_x > fb_width - plane_req->src_w ||
-	    plane_req->src_h > fb_height ||
-	    plane_req->src_y > fb_height - plane_req->src_h) {
+	if (src_w > fb_width ||
+	    src_x > fb_width - src_w ||
+	    src_h > fb_height ||
+	    src_y > fb_height - src_h) {
 		DRM_DEBUG_KMS("Invalid source coordinates "
 			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
-			      plane_req->src_w >> 16,
-			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
-			      plane_req->src_h >> 16,
-			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
-			      plane_req->src_x >> 16,
-			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
-			      plane_req->src_y >> 16,
-			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
+			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
+			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
+			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
+			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
 		ret = -ENOSPC;
 		goto out;
 	}
 
-	/* Give drivers some help against integer overflows */
-	if (plane_req->crtc_w > INT_MAX ||
-	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
-	    plane_req->crtc_h > INT_MAX ||
-	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
-		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      plane_req->crtc_w, plane_req->crtc_h,
-			      plane_req->crtc_x, plane_req->crtc_y);
-		ret = -ERANGE;
-		goto out;
-	}
-
 	drm_modeset_lock_all(dev);
 	old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
-					 plane_req->crtc_x, plane_req->crtc_y,
-					 plane_req->crtc_w, plane_req->crtc_h,
-					 plane_req->src_x, plane_req->src_y,
-					 plane_req->src_w, plane_req->src_h);
+					 crtc_x, crtc_y, crtc_w, crtc_h,
+					 src_x, src_y, src_w, src_h);
 	if (!ret) {
 		plane->crtc = crtc;
 		plane->fb = fb;
@@ -2265,6 +2218,81 @@ out:
 		drm_framebuffer_unreference(old_fb);
 
 	return ret;
+
+}
+
+/**
+ * drm_mode_setplane - configure a plane's configuration
+ * @dev: DRM device
+ * @data: ioctl data*
+ * @file_priv: DRM file info
+ *
+ * Set plane configuration, including placement, fb, scaling, and other factors.
+ * Or pass a NULL fb to disable (planes may be disabled without providing a
+ * valid crtc).
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_setplane(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv)
+{
+	struct drm_mode_set_plane *plane_req = data;
+	struct drm_mode_object *obj;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc = NULL;
+	struct drm_framebuffer *fb = NULL;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	/* Give drivers some help against integer overflows */
+	if (plane_req->crtc_w > INT_MAX ||
+	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
+	    plane_req->crtc_h > INT_MAX ||
+	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
+		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
+			      plane_req->crtc_w, plane_req->crtc_h,
+			      plane_req->crtc_x, plane_req->crtc_y);
+		return -ERANGE;
+	}
+
+	/*
+	 * First, find the plane, crtc, and fb objects.  If not available,
+	 * we don't bother to call the driver.
+	 */
+	obj = drm_mode_object_find(dev, plane_req->plane_id,
+				   DRM_MODE_OBJECT_PLANE);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown plane ID %d\n",
+			      plane_req->plane_id);
+		return -ENOENT;
+	}
+	plane = obj_to_plane(obj);
+
+	if (plane_req->fb_id) {
+		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
+		if (!fb) {
+			DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
+				      plane_req->fb_id);
+			return -ENOENT;
+		}
+
+		obj = drm_mode_object_find(dev, plane_req->crtc_id,
+					   DRM_MODE_OBJECT_CRTC);
+		if (!obj) {
+			DRM_DEBUG_KMS("Unknown crtc ID %d\n",
+				      plane_req->crtc_id);
+			return -ENOENT;
+		}
+		crtc = obj_to_crtc(obj);
+	}
+
+	return setplane_internal(crtc, plane, fb,
+				 plane_req->crtc_x, plane_req->crtc_y,
+				 plane_req->crtc_w, plane_req->crtc_h,
+				 plane_req->src_x, plane_req->src_y,
+				 plane_req->src_w, plane_req->src_h);
 }
 
 /**
-- 
1.8.5.1

[PATCH 3/6] drm: Support legacy cursor ioctls via universal planes when possible (v4)

[Matt Roper]

If drivers support universal planes and have registered a cursor plane
with the DRM core, we should use that universal plane support when
handling legacy cursor ioctls.  Drivers that transition to universal
planes won't have to maintain separate legacy ioctl handling; drivers
that don't transition to universal planes will continue to operate
without any change to behavior.

Note that there's a bit of a mismatch between the legacy cursor ioctls
and the universal plane API's --- legacy ioctl's use driver buffer
handles directly whereas the universal plane API takes drm_framebuffers.
Since there's no way to recover the driver handle from a
drm_framebuffer, we can implement legacy ioctl's in terms of universal
plane interfaces, but cannot implement universal plane interfaces in
terms of legacy ioctls.  Specifically, there's no way to create a
general cursor helper in the way we previously created a primary plane
helper.

It's important to land this patch before any patches that add universal
cursor support to individual drivers so that drivers don't have to worry
about juggling two different styles of reference counting for cursor
buffers when userspace mixes and matches legacy and universal cursor
calls.  With this patch, a driver that switches to universal cursor
support may assume that all cursor buffers are wrapped in a
drm_framebuffer and can rely on framebuffer reference counting for all
cursor operations.

v4:
 - Add comments pointing out setplane_internal's reference-eating
   semantics.
v3:
 - Drop drm_mode_rmfb() call that is no longer needed now that we're
   using setplane_internal(), which takes care of deref'ing the
   appropriate framebuffer.
v2:
 - Use new add_framebuffer_internal() function to create framebuffer
   rather than trying to call directly into the ioctl interface and
   look up the handle returned.
 - Use new setplane_internal() function to update the cursor plane
   rather than calling through the ioctl interface.  Note that since
   we're no longer looking up an fb_id, no extra reference will be
   taken here.
 - Grab extra reference to fb under lock in !BO case to avoid issues
   where racing userspace could cause the fb to be destroyed out from
   under us after we grab the fb pointer.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 107 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |   4 ++
 2 files changed, 111 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 27eae03..b5bce5b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2288,6 +2288,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		crtc = obj_to_crtc(obj);
 	}
 
+	/*
+	 * setplane_internal will take care of deref'ing either the old or new
+	 * framebuffer depending on success.
+	 */
 	return setplane_internal(crtc, plane, fb,
 				 plane_req->crtc_x, plane_req->crtc_y,
 				 plane_req->crtc_w, plane_req->crtc_h,
@@ -2541,6 +2545,102 @@ out:
 	return ret;
 }
 
+/**
+ * drm_mode_cursor_universal - translate legacy cursor ioctl call into a
+ *     universal plane handler call
+ * @crtc: crtc to update cursor for
+ * @req: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Legacy cursor ioctl's work directly with driver buffer handles.  To
+ * translate legacy ioctl calls into universal plane handler calls, we need to
+ * wrap the native buffer handle in a drm_framebuffer.
+ *
+ * Note that we assume any handle passed to the legacy ioctls was a 32-bit ARGB
+ * buffer with a pitch of 4*width; the universal plane interface should be used
+ * directly in cases where the hardware can support other buffer settings and
+ * userspace wants to make use of these capabilities.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+static int drm_mode_cursor_universal(struct drm_crtc *crtc,
+				     struct drm_mode_cursor2 *req,
+				     struct drm_file *file_priv)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *fb = NULL;
+	struct drm_mode_fb_cmd2 fbreq = {
+		.width = req->width,
+		.height = req->height,
+		.pixel_format = DRM_FORMAT_ARGB8888,
+		.pitches = { req->width * 4 },
+		.handles = { req->handle },
+	};
+	int32_t crtc_x, crtc_y;
+	uint32_t crtc_w = 0, crtc_h = 0;
+	uint32_t src_w = 0, src_h = 0;
+	int ret = 0;
+
+	BUG_ON(!crtc->cursor);
+
+	/*
+	 * Obtain fb we'll be using (either new or existing) and take an extra
+	 * reference to it if fb != null.  setplane will take care of dropping
+	 * the reference if the plane update fails.
+	 */
+	if (req->flags & DRM_MODE_CURSOR_BO) {
+		if (req->handle) {
+			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
+			if (IS_ERR(fb)) {
+				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
+				return PTR_ERR(fb);
+			}
+
+			drm_framebuffer_reference(fb);
+		} else {
+			fb = NULL;
+		}
+	} else {
+		mutex_lock(&dev->mode_config.mutex);
+		fb = crtc->cursor->fb;
+		if (fb)
+			drm_framebuffer_reference(fb);
+		mutex_unlock(&dev->mode_config.mutex);
+	}
+
+	if (req->flags & DRM_MODE_CURSOR_MOVE) {
+		crtc_x = req->x;
+		crtc_y = req->y;
+	} else {
+		crtc_x = crtc->cursor_x;
+		crtc_y = crtc->cursor_y;
+	}
+
+	if (fb) {
+		crtc_w = fb->width;
+		crtc_h = fb->height;
+		src_w = fb->width << 16;
+		src_h = fb->height << 16;
+	}
+
+	/*
+	 * setplane_internal will take care of deref'ing either the old or new
+	 * framebuffer depending on success.
+	 */
+	ret = setplane_internal(crtc, crtc->cursor, fb,
+				crtc_x, crtc_y, crtc_w, crtc_h,
+				0, 0, src_w, src_h);
+
+	/* Update successful; save new cursor position, if necessary */
+	if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
+		crtc->cursor_x = req->x;
+		crtc->cursor_y = req->y;
+	}
+
+	return ret;
+}
+
 static int drm_mode_cursor_common(struct drm_device *dev,
 				  struct drm_mode_cursor2 *req,
 				  struct drm_file *file_priv)
@@ -2560,6 +2660,13 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		return -ENOENT;
 	}
 
+	/*
+	 * If this crtc has a universal cursor plane, call that plane's update
+	 * handler rather than using legacy cursor handlers.
+	 */
+	if (crtc->cursor)
+		return drm_mode_cursor_universal(crtc, req, file_priv);
+
 	drm_modeset_lock(&crtc->mutex, NULL);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 251b75e..b8c7a9a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -331,6 +331,10 @@ struct drm_crtc {
 	struct drm_plane *primary;
 	struct drm_plane *cursor;
 
+	/* position of cursor plane on crtc */
+	int cursor_x;
+	int cursor_y;
+
 	/* Temporary tracking of the old fb while a modeset is ongoing. Used
 	 * by drm_mode_set_config_internal to implement correct refcounting. */
 	struct drm_framebuffer *old_fb;
-- 
1.8.5.1

[PATCH 4/6] drm: Allow drivers to register cursor planes with crtc

[Matt Roper]

Universal plane support had placeholders for cursor planes, but didn't
actually do anything with them.  Save the cursor plane reference inside
the crtc and update the cursor plane parameter from void* to drm_plane.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 5 ++++-
 include/drm/drm_crtc.h     | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b5bce5b..74ad72f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -727,7 +727,7 @@ DEFINE_WW_CLASS(crtc_ww_class);
  */
 int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 			      struct drm_plane *primary,
-			      void *cursor,
+			      struct drm_plane *cursor,
 			      const struct drm_crtc_funcs *funcs)
 {
 	struct drm_mode_config *config = &dev->mode_config;
@@ -752,8 +752,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	config->num_crtc++;
 
 	crtc->primary = primary;
+	crtc->cursor = cursor;
 	if (primary)
 		primary->possible_crtcs = 1 << drm_crtc_index(crtc);
+	if (cursor)
+		cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
 
  out:
 	drm_modeset_unlock_all(dev);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b8c7a9a..4ee7e26 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -856,7 +856,7 @@ struct drm_prop_enum_list {
 extern int drm_crtc_init_with_planes(struct drm_device *dev,
 				     struct drm_crtc *crtc,
 				     struct drm_plane *primary,
-				     void *cursor,
+				     struct drm_plane *cursor,
 				     const struct drm_crtc_funcs *funcs);
 extern int drm_crtc_init(struct drm_device *dev,
 			 struct drm_crtc *crtc,
-- 
1.8.5.1

[PATCH 5/6] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer (v2)

[Matt Roper]

Refactor cursor buffer setting such that the code to actually update the
cursor lives in a new function, intel_crtc_cursor_set_obj(), and takes
a GEM object as a parameter.  The existing legacy cursor ioctl handler,
intel_crtc_cursor_set() will now perform the userspace handle lookup and
then call this new function.

This refactoring is in preparation for the universal plane cursor
support where we'll want to update the cursor with an actual GEM buffer
object (obtained via drm_framebuffer) rather than a userspace handle.

v2:  Drop obvious kerneldoc and replace with note about function's
     reference consumption

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..1beeb2a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8089,21 +8089,26 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	intel_crtc->cursor_base = base;
 }
 
-static int intel_crtc_cursor_set(struct drm_crtc *crtc,
-				 struct drm_file *file,
-				 uint32_t handle,
-				 uint32_t width, uint32_t height)
+/*
+ * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
+ *
+ * Note that the object's reference will be consumed if the update fails.  If
+ * the update succeeds, the reference of the old object (if any) will be
+ * consumed.
+ */
+static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+				     struct drm_i915_gem_object *obj,
+				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_gem_object *obj;
 	unsigned old_width;
 	uint32_t addr;
 	int ret;
 
 	/* if we want to turn off the cursor ignore width and height */
-	if (!handle) {
+	if (!obj) {
 		DRM_DEBUG_KMS("cursor off\n");
 		addr = 0;
 		obj = NULL;
@@ -8119,12 +8124,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
-	if (&obj->base == NULL)
-		return -ENOENT;
-
 	if (obj->base.size < width * height * 4) {
-		DRM_DEBUG_KMS("buffer is to small\n");
+		DRM_DEBUG_KMS("buffer is too small\n");
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -8207,6 +8208,25 @@ fail:
 	return ret;
 }
 
+static int intel_crtc_cursor_set(struct drm_crtc *crtc,
+				 struct drm_file *file,
+				 uint32_t handle,
+				 uint32_t width, uint32_t height)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_gem_object *obj;
+
+	if (handle) {
+		obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
+		if (&obj->base == NULL)
+			return -ENOENT;
+	} else {
+		obj = NULL;
+	}
+
+	return intel_crtc_cursor_set_obj(crtc, obj, width, height);
+}
+
 static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-- 
1.8.5.1

[PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

[Matt Roper]

The DRM core will translate calls to legacy cursor ioctls into universal
cursor calls automatically, so there's no need to maintain the legacy
cursor support.  This greatly simplifies the transition since we don't
have to handle reference counting differently depending on which cursor
interface was called.

The aim here is to transition to the universal plane interface with
minimal code change.  There's a lot of cleanup that can be done (e.g.,
using state stored in crtc->cursor->fb rather than intel_crtc) that is
left to future patches.

v4:
 - Drop drm_gem_object_unreference() that is no longer needed now that
   we receive the GEM obj directly rather than looking up the ID.
v3:
 - Pass cursor obj to intel_crtc_cursor_set_obj() if cursor fb changes,
   even if 'visible' is false.  intel_crtc_cursor_set_obj() will notice
   that the cursor isn't visible and disable it properly, but we still
   need to get intel_crtc->cursor_addr set properly so that we behave
   properly if the cursor becomes visible again in the future without
   changing the cursor buffer (noted by Chris Wilson and verified
   via i-g-t kms_cursor_crc).
 - s/drm_plane_init/drm_universal_plane_init/.  Due to type
   compatibility between enum and bool, everything actually works
   correctly with the wrong init call, except for the type of plane that
   gets exposed to userspace (it shows up as type 'primary' rather than
   type 'cursor').
v2:
 - Remove duplicate dimension checks on cursor
 - Drop explicit cursor disable from crtc destroy (fb & plane
   destruction will take care of that now)
 - Use DRM plane helper to check update parameters

Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 166 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   1 -
 2 files changed, 118 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1beeb2a..1880c18 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
 	DRM_FORMAT_ABGR2101010,
 };
 
+/* Cursor formats */
+static const uint32_t intel_cursor_formats[] = {
+	DRM_FORMAT_ARGB8888,
+};
+
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
 ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
@@ -8044,8 +8049,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	int x = intel_crtc->cursor_x;
-	int y = intel_crtc->cursor_y;
+	int x = crtc->cursor_x;
+	int y = crtc->cursor_y;
 	u32 base = 0, pos = 0;
 
 	if (on)
@@ -8180,7 +8185,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	if (intel_crtc->cursor_bo) {
 		if (!INTEL_INFO(dev)->cursor_needs_physical)
 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-		drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
 	}
 
 	mutex_unlock(&dev->struct_mutex);
@@ -8208,38 +8212,6 @@ fail:
 	return ret;
 }
 
-static int intel_crtc_cursor_set(struct drm_crtc *crtc,
-				 struct drm_file *file,
-				 uint32_t handle,
-				 uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_gem_object *obj;
-
-	if (handle) {
-		obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
-		if (&obj->base == NULL)
-			return -ENOENT;
-	} else {
-		obj = NULL;
-	}
-
-	return intel_crtc_cursor_set_obj(crtc, obj, width, height);
-}
-
-static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
-{
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
-	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
-
-	if (intel_crtc->active)
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-	return 0;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 		kfree(work);
 	}
 
-	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
-
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
@@ -10942,8 +10912,6 @@ out_config:
 }
 
 static const struct drm_crtc_funcs intel_crtc_funcs = {
-	.cursor_set = intel_crtc_cursor_set,
-	.cursor_move = intel_crtc_cursor_move,
 	.gamma_set = intel_crtc_gamma_set,
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
@@ -11196,7 +11164,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	return 0;
 }
 
-static void intel_primary_plane_destroy(struct drm_plane *plane)
+/* Common destruction function for both primary and cursor planes */
+static void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	drm_plane_cleanup(plane);
@@ -11206,7 +11175,7 @@ static void intel_primary_plane_destroy(struct drm_plane *plane)
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
 	.update_plane = intel_primary_plane_setplane,
 	.disable_plane = intel_primary_plane_disable,
-	.destroy = intel_primary_plane_destroy,
+	.destroy = intel_plane_destroy,
 };
 
 static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
@@ -11242,11 +11211,100 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	return &primary->base;
 }
 
+static int
+intel_cursor_plane_disable(struct drm_plane *plane)
+{
+	if (!plane->fb)
+		return 0;
+
+	BUG_ON(!plane->crtc);
+
+	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+}
+
+static int
+intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			  unsigned int crtc_w, unsigned int crtc_h,
+			  uint32_t src_x, uint32_t src_y,
+			  uint32_t src_w, uint32_t src_h)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_rect dest = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	const struct drm_rect clip = {
+		/* integer pixels */
+		.x2 = intel_crtc->config.pipe_src_w,
+		.y2 = intel_crtc->config.pipe_src_h,
+	};
+	bool visible;
+	int ret;
+
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    true, true, &visible);
+	if (ret)
+		return ret;
+
+	crtc->cursor_x = crtc_x;
+	crtc->cursor_y = crtc_y;
+	if (fb != crtc->cursor->fb) {
+		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+	} else {
+		intel_crtc_update_cursor(crtc, visible);
+		return 0;
+	}
+}
+static const struct drm_plane_funcs intel_cursor_plane_funcs = {
+	.update_plane = intel_cursor_plane_update,
+	.disable_plane = intel_cursor_plane_disable,
+	.destroy = intel_plane_destroy,
+};
+
+static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
+						   int pipe)
+{
+	struct intel_plane *cursor;
+
+	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
+	if (cursor == NULL)
+		return NULL;
+
+	cursor->can_scale = false;
+	cursor->max_downscale = 1;
+	cursor->pipe = pipe;
+	cursor->plane = pipe;
+
+	drm_universal_plane_init(dev, &cursor->base, 0,
+				 &intel_cursor_plane_funcs,
+				 intel_cursor_formats,
+				 ARRAY_SIZE(intel_cursor_formats),
+				 DRM_PLANE_TYPE_CURSOR);
+	return &cursor->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	struct drm_plane *primary;
+	struct drm_plane *primary = NULL;
+	struct drm_plane *cursor = NULL;
 	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
@@ -11254,13 +11312,17 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		return;
 
 	primary = intel_primary_plane_create(dev, pipe);
+	if (!primary)
+		goto fail;
+
+	cursor = intel_cursor_plane_create(dev, pipe);
+	if (!cursor)
+		goto fail;
+
 	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
-					NULL, &intel_crtc_funcs);
-	if (ret) {
-		drm_plane_cleanup(primary);
-		kfree(intel_crtc);
-		return;
-	}
+					cursor, &intel_crtc_funcs);
+	if (ret)
+		goto fail;
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
@@ -11293,6 +11355,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+	return;
+
+fail:
+	if (primary)
+		drm_plane_cleanup(primary);
+	if (cursor)
+		drm_plane_cleanup(cursor);
+	kfree(intel_crtc);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 78d4124..57a36ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -384,7 +384,6 @@ struct intel_crtc {
 
 	struct drm_i915_gem_object *cursor_bo;
 	uint32_t cursor_addr;
-	int16_t cursor_x, cursor_y;
 	int16_t cursor_width, cursor_height;
 	uint32_t cursor_cntl;
 	uint32_t cursor_base;
-- 
1.8.5.1

Re: [PATCH 1/6] drm: Refactor framebuffer creation to allow internal use (v2)

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> Refactor DRM framebuffer creation into a new function that returns a
> struct drm_framebuffer directly.  The upcoming universal cursor support
> will want to create framebuffers internally to wrap cursor buffers, so
> we want to be able to share that framebuffer creation with the
> drm_mode_addfb2 ioctl handler.
> 
> v2: Take struct drm_mode_fb_cmd2 parameter directly rather than void*
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 69 ++++++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fe94cc1..5a88267 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -41,6 +41,10 @@
>  
>  #include "drm_crtc_internal.h"
>  
> +static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> +							struct drm_mode_fb_cmd2 *r,
> +							struct drm_file *file_priv);
> +
>  /**
>   * drm_modeset_lock_all - take all modeset locks
>   * @dev: drm device
> @@ -2827,56 +2831,38 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  	return 0;
>  }
>  
> -/**
> - * drm_mode_addfb2 - add an FB to the graphics configuration
> - * @dev: drm device for the ioctl
> - * @data: data pointer for the ioctl
> - * @file_priv: drm file for the ioctl call
> - *
> - * Add a new FB to the specified CRTC, given a user request with format. This is
> - * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
> - * and uses fourcc codes as pixel format specifiers.
> - *
> - * Called by the user via ioctl.
> - *
> - * Returns:
> - * Zero on success, errno on failure.
> - */
> -int drm_mode_addfb2(struct drm_device *dev,
> -		    void *data, struct drm_file *file_priv)
> +static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> +							struct drm_mode_fb_cmd2 *r,
> +							struct drm_file *file_priv)
>  {
> -	struct drm_mode_fb_cmd2 *r = data;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_framebuffer *fb;
>  	int ret;
>  
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> -		return -EINVAL;
> -
>  	if (r->flags & ~DRM_MODE_FB_INTERLACED) {
>  		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	if ((config->min_width > r->width) || (r->width > config->max_width)) {
>  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
>  			  r->width, config->min_width, config->max_width);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  	if ((config->min_height > r->height) || (r->height > config->max_height)) {
>  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
>  			  r->height, config->min_height, config->max_height);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	ret = framebuffer_check(r);
>  	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
>  
>  	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
>  	if (IS_ERR(fb)) {
>  		DRM_DEBUG_KMS("could not create framebuffer\n");
> -		return PTR_ERR(fb);
> +		return fb;
>  	}
>  
>  	mutex_lock(&file_priv->fbs_lock);
> @@ -2885,8 +2871,37 @@ int drm_mode_addfb2(struct drm_device *dev,
>  	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> +	return fb;
> +}
>  
> -	return ret;
> +/**
> + * drm_mode_addfb2 - add an FB to the graphics configuration
> + * @dev: drm device for the ioctl
> + * @data: data pointer for the ioctl
> + * @file_priv: drm file for the ioctl call
> + *
> + * Add a new FB to the specified CRTC, given a user request with format. This is
> + * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
> + * and uses fourcc codes as pixel format specifiers.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, errno on failure.
> + */
> +int drm_mode_addfb2(struct drm_device *dev,
> +		    void *data, struct drm_file *file_priv)
> +{
> +	struct drm_framebuffer *fb;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	fb = add_framebuffer_internal(dev, data, file_priv);
> +	if (IS_ERR(fb))
> +		return PTR_ERR(fb);
> +
> +	return 0;
>  }
>  
>  /**

Reviewed-by: Pallavi G<pallavi.g@intel.com>

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> Refactor DRM setplane code into a new setplane_internal() function that
> takes DRM objects directly as parameters rather than looking them up by
> ID.  We'll use this in a future patch when we implement legacy cursor
> ioctls on top of the universal plane interface.
> 
> v3:
>  - Move integer overflow checking from setplane_internal to setplane
>    ioctl.  The upcoming legacy cursor support via universal planes needs
>    to maintain current cursor ioctl semantics and not return error for
>    these extreme values (found via intel-gpu-tools kms_cursor_crc test).
> v2:
>  - Allow planes to be disabled without a valid crtc again (and add
>    mention of this to setplane's kerneldoc, since it doesn't seem to be
>    mentioned anywhere else).
>  - Reformat some parameter line wrap
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 176 ++++++++++++++++++++++++++-------------------
>  1 file changed, 102 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5a88267..27eae03 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2122,45 +2122,32 @@ out:
>  	return ret;
>  }
>  
> -/**
> - * drm_mode_setplane - configure a plane's configuration
> - * @dev: DRM device
> - * @data: ioctl data*
> - * @file_priv: DRM file info
> +/*
> + * setplane_internal - setplane handler for internal callers
>   *
> - * Set plane configuration, including placement, fb, scaling, and other factors.
> - * Or pass a NULL fb to disable.
> + * Note that we assume an extra reference has already been taken on fb.  If the
> + * update fails, this reference will be dropped before return; if it succeeds,
> + * the previous framebuffer (if any) will be unreferenced instead.
>   *
> - * Returns:
> - * Zero on success, errno on failure.
> + * src_{x,y,w,h} are provided in 16.16 fixed point format
>   */
> -int drm_mode_setplane(struct drm_device *dev, void *data,
> -		      struct drm_file *file_priv)
> +static int setplane_internal(struct drm_crtc *crtc,
> +			     struct drm_plane *plane,
> +			     struct drm_framebuffer *fb,
> +			     int32_t crtc_x, int32_t crtc_y,
> +			     uint32_t crtc_w, uint32_t crtc_h,
> +			     /* src_{x,y,w,h} values are 16.16 fixed point */
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
>  {
> -	struct drm_mode_set_plane *plane_req = data;
> -	struct drm_plane *plane;
> -	struct drm_crtc *crtc;
> -	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_framebuffer *old_fb = NULL;
>  	int ret = 0;
>  	unsigned int fb_width, fb_height;
>  	int i;
>  
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> -		return -EINVAL;
> -
> -	/*
> -	 * First, find the plane, crtc, and fb objects.  If not available,
> -	 * we don't bother to call the driver.
> -	 */
> -	plane = drm_plane_find(dev, plane_req->plane_id);
> -	if (!plane) {
> -		DRM_DEBUG_KMS("Unknown plane ID %d\n",
> -			      plane_req->plane_id);
> -		return -ENOENT;
> -	}
> -
>  	/* No fb means shut it down */
> -	if (!plane_req->fb_id) {
> +	if (!fb) {
>  		drm_modeset_lock_all(dev);
>  		old_fb = plane->fb;
>  		ret = plane->funcs->disable_plane(plane);
> @@ -2174,14 +2161,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	crtc = drm_crtc_find(dev, plane_req->crtc_id);
> -	if (!crtc) {
> -		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> -			      plane_req->crtc_id);
> -		ret = -ENOENT;
> -		goto out;
> -	}
> -
>  	/* Check whether this plane is usable on this CRTC */
>  	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
>  		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> @@ -2189,14 +2168,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
> -	if (!fb) {
> -		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
> -			      plane_req->fb_id);
> -		ret = -ENOENT;
> -		goto out;
> -	}
> -
>  	/* Check whether this plane supports the fb pixel format. */
>  	for (i = 0; i < plane->format_count; i++)
>  		if (fb->pixel_format == plane->format_types[i])
> @@ -2212,43 +2183,25 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	fb_height = fb->height << 16;
>  
>  	/* Make sure source coordinates are inside the fb. */
> -	if (plane_req->src_w > fb_width ||
> -	    plane_req->src_x > fb_width - plane_req->src_w ||
> -	    plane_req->src_h > fb_height ||
> -	    plane_req->src_y > fb_height - plane_req->src_h) {
> +	if (src_w > fb_width ||
> +	    src_x > fb_width - src_w ||
> +	    src_h > fb_height ||
> +	    src_y > fb_height - src_h) {
>  		DRM_DEBUG_KMS("Invalid source coordinates "
>  			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> -			      plane_req->src_w >> 16,
> -			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
> -			      plane_req->src_h >> 16,
> -			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
> -			      plane_req->src_x >> 16,
> -			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
> -			      plane_req->src_y >> 16,
> -			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
> +			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
> +			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
> +			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
> +			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
>  		ret = -ENOSPC;
>  		goto out;
>  	}
>  
> -	/* Give drivers some help against integer overflows */
> -	if (plane_req->crtc_w > INT_MAX ||
> -	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
> -	    plane_req->crtc_h > INT_MAX ||
> -	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
> -		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> -			      plane_req->crtc_w, plane_req->crtc_h,
> -			      plane_req->crtc_x, plane_req->crtc_y);
> -		ret = -ERANGE;
> -		goto out;
> -	}
> -
>  	drm_modeset_lock_all(dev);
>  	old_fb = plane->fb;
>  	ret = plane->funcs->update_plane(plane, crtc, fb,
> -					 plane_req->crtc_x, plane_req->crtc_y,
> -					 plane_req->crtc_w, plane_req->crtc_h,
> -					 plane_req->src_x, plane_req->src_y,
> -					 plane_req->src_w, plane_req->src_h);
> +					 crtc_x, crtc_y, crtc_w, crtc_h,
> +					 src_x, src_y, src_w, src_h);
>  	if (!ret) {
>  		plane->crtc = crtc;
>  		plane->fb = fb;
> @@ -2265,6 +2218,81 @@ out:
>  		drm_framebuffer_unreference(old_fb);
>  
>  	return ret;
> +
> +}
> +
> +/**
> + * drm_mode_setplane - configure a plane's configuration
> + * @dev: DRM device
> + * @data: ioctl data*
> + * @file_priv: DRM file info
> + *
> + * Set plane configuration, including placement, fb, scaling, and other factors.
> + * Or pass a NULL fb to disable (planes may be disabled without providing a
> + * valid crtc).
> + *
> + * Returns:
> + * Zero on success, errno on failure.
> + */
> +int drm_mode_setplane(struct drm_device *dev, void *data,
> +		      struct drm_file *file_priv)
> +{
> +	struct drm_mode_set_plane *plane_req = data;
> +	struct drm_mode_object *obj;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc = NULL;
> +	struct drm_framebuffer *fb = NULL;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	/* Give drivers some help against integer overflows */
> +	if (plane_req->crtc_w > INT_MAX ||
> +	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
> +	    plane_req->crtc_h > INT_MAX ||
> +	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
> +		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> +			      plane_req->crtc_w, plane_req->crtc_h,
> +			      plane_req->crtc_x, plane_req->crtc_y);
> +		return -ERANGE;
> +	}
> +
> +	/*
> +	 * First, find the plane, crtc, and fb objects.  If not available,
> +	 * we don't bother to call the driver.
> +	 */
> +	obj = drm_mode_object_find(dev, plane_req->plane_id,
> +				   DRM_MODE_OBJECT_PLANE);
> +	if (!obj) {
> +		DRM_DEBUG_KMS("Unknown plane ID %d\n",
> +			      plane_req->plane_id);
> +		return -ENOENT;
> +	}
> +	plane = obj_to_plane(obj);
> +
> +	if (plane_req->fb_id) {
> +		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
> +		if (!fb) {
> +			DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
> +				      plane_req->fb_id);
> +			return -ENOENT;
> +		}
> +
> +		obj = drm_mode_object_find(dev, plane_req->crtc_id,
> +					   DRM_MODE_OBJECT_CRTC);
> +		if (!obj) {
> +			DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> +				      plane_req->crtc_id);
> +			return -ENOENT;
> +		}
> +		crtc = obj_to_crtc(obj);
> +	}
> +
> +	return setplane_internal(crtc, plane, fb,
> +				 plane_req->crtc_x, plane_req->crtc_y,
> +				 plane_req->crtc_w, plane_req->crtc_h,
> +				 plane_req->src_x, plane_req->src_y,
> +				 plane_req->src_w, plane_req->src_h);
>  }
>  
>  /**



Reviewed-by: Pallavi G <pallavi.g@intel.com>

Re: [PATCH 3/6] drm: Support legacy cursor ioctls via universal planes when possible (v4)

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> If drivers support universal planes and have registered a cursor plane
> with the DRM core, we should use that universal plane support when
> handling legacy cursor ioctls.  Drivers that transition to universal
> planes won't have to maintain separate legacy ioctl handling; drivers
> that don't transition to universal planes will continue to operate
> without any change to behavior.
> 
> Note that there's a bit of a mismatch between the legacy cursor ioctls
> and the universal plane API's --- legacy ioctl's use driver buffer
> handles directly whereas the universal plane API takes drm_framebuffers.
> Since there's no way to recover the driver handle from a
> drm_framebuffer, we can implement legacy ioctl's in terms of universal
> plane interfaces, but cannot implement universal plane interfaces in
> terms of legacy ioctls.  Specifically, there's no way to create a
> general cursor helper in the way we previously created a primary plane
> helper.
> 
> It's important to land this patch before any patches that add universal
> cursor support to individual drivers so that drivers don't have to worry
> about juggling two different styles of reference counting for cursor
> buffers when userspace mixes and matches legacy and universal cursor
> calls.  With this patch, a driver that switches to universal cursor
> support may assume that all cursor buffers are wrapped in a
> drm_framebuffer and can rely on framebuffer reference counting for all
> cursor operations.
> 
> v4:
>  - Add comments pointing out setplane_internal's reference-eating
>    semantics.
> v3:
>  - Drop drm_mode_rmfb() call that is no longer needed now that we're
>    using setplane_internal(), which takes care of deref'ing the
>    appropriate framebuffer.
> v2:
>  - Use new add_framebuffer_internal() function to create framebuffer
>    rather than trying to call directly into the ioctl interface and
>    look up the handle returned.
>  - Use new setplane_internal() function to update the cursor plane
>    rather than calling through the ioctl interface.  Note that since
>    we're no longer looking up an fb_id, no extra reference will be
>    taken here.
>  - Grab extra reference to fb under lock in !BO case to avoid issues
>    where racing userspace could cause the fb to be destroyed out from
>    under us after we grab the fb pointer.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 107 +++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |   4 ++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 27eae03..b5bce5b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2288,6 +2288,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		crtc = obj_to_crtc(obj);
>  	}
>  
> +	/*
> +	 * setplane_internal will take care of deref'ing either the old or new
> +	 * framebuffer depending on success.
> +	 */
>  	return setplane_internal(crtc, plane, fb,
>  				 plane_req->crtc_x, plane_req->crtc_y,
>  				 plane_req->crtc_w, plane_req->crtc_h,
> @@ -2541,6 +2545,102 @@ out:
>  	return ret;
>  }
>  
> +/**
> + * drm_mode_cursor_universal - translate legacy cursor ioctl call into a
> + *     universal plane handler call
> + * @crtc: crtc to update cursor for
> + * @req: data pointer for the ioctl
> + * @file_priv: drm file for the ioctl call
> + *
> + * Legacy cursor ioctl's work directly with driver buffer handles.  To
> + * translate legacy ioctl calls into universal plane handler calls, we need to
> + * wrap the native buffer handle in a drm_framebuffer.
> + *
> + * Note that we assume any handle passed to the legacy ioctls was a 32-bit ARGB
> + * buffer with a pitch of 4*width; the universal plane interface should be used
> + * directly in cases where the hardware can support other buffer settings and
> + * userspace wants to make use of these capabilities.
> + *
> + * Returns:
> + * Zero on success, errno on failure.
> + */
> +static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> +				     struct drm_mode_cursor2 *req,
> +				     struct drm_file *file_priv)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_framebuffer *fb = NULL;
> +	struct drm_mode_fb_cmd2 fbreq = {
> +		.width = req->width,
> +		.height = req->height,
> +		.pixel_format = DRM_FORMAT_ARGB8888,
> +		.pitches = { req->width * 4 },
> +		.handles = { req->handle },
> +	};
> +	int32_t crtc_x, crtc_y;
> +	uint32_t crtc_w = 0, crtc_h = 0;
> +	uint32_t src_w = 0, src_h = 0;
> +	int ret = 0;
> +
> +	BUG_ON(!crtc->cursor);
> +
> +	/*
> +	 * Obtain fb we'll be using (either new or existing) and take an extra
> +	 * reference to it if fb != null.  setplane will take care of dropping
> +	 * the reference if the plane update fails.
> +	 */
> +	if (req->flags & DRM_MODE_CURSOR_BO) {
> +		if (req->handle) {
> +			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
> +			if (IS_ERR(fb)) {
> +				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
> +				return PTR_ERR(fb);
> +			}
> +
> +			drm_framebuffer_reference(fb);
> +		} else {
> +			fb = NULL;
> +		}
> +	} else {
> +		mutex_lock(&dev->mode_config.mutex);
> +		fb = crtc->cursor->fb;
> +		if (fb)
> +			drm_framebuffer_reference(fb);
> +		mutex_unlock(&dev->mode_config.mutex);
> +	}
> +
> +	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> +		crtc_x = req->x;
> +		crtc_y = req->y;
> +	} else {
> +		crtc_x = crtc->cursor_x;
> +		crtc_y = crtc->cursor_y;
> +	}
> +
> +	if (fb) {
> +		crtc_w = fb->width;
> +		crtc_h = fb->height;
> +		src_w = fb->width << 16;
> +		src_h = fb->height << 16;
> +	}
> +
> +	/*
> +	 * setplane_internal will take care of deref'ing either the old or new
> +	 * framebuffer depending on success.
> +	 */
> +	ret = setplane_internal(crtc, crtc->cursor, fb,
> +				crtc_x, crtc_y, crtc_w, crtc_h,
> +				0, 0, src_w, src_h);
> +
> +	/* Update successful; save new cursor position, if necessary */
> +	if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
> +		crtc->cursor_x = req->x;
> +		crtc->cursor_y = req->y;
> +	}
> +
> +	return ret;
> +}
> +
>  static int drm_mode_cursor_common(struct drm_device *dev,
>  				  struct drm_mode_cursor2 *req,
>  				  struct drm_file *file_priv)
> @@ -2560,6 +2660,13 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * If this crtc has a universal cursor plane, call that plane's update
> +	 * handler rather than using legacy cursor handlers.
> +	 */
> +	if (crtc->cursor)
> +		return drm_mode_cursor_universal(crtc, req, file_priv);
> +
>  	drm_modeset_lock(&crtc->mutex, NULL);
>  	if (req->flags & DRM_MODE_CURSOR_BO) {
>  		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 251b75e..b8c7a9a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -331,6 +331,10 @@ struct drm_crtc {
>  	struct drm_plane *primary;
>  	struct drm_plane *cursor;
>  
> +	/* position of cursor plane on crtc */
> +	int cursor_x;
> +	int cursor_y;
> +
>  	/* Temporary tracking of the old fb while a modeset is ongoing. Used
>  	 * by drm_mode_set_config_internal to implement correct refcounting. */
>  	struct drm_framebuffer *old_fb;

Reviewed-by: Pallavi G <pallavi.g@intel.com>

Re: [PATCH 4/6] drm: Allow drivers to register cursor planes with crtc

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> Universal plane support had placeholders for cursor planes, but didn't
> actually do anything with them.  Save the cursor plane reference inside
> the crtc and update the cursor plane parameter from void* to drm_plane.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 5 ++++-
>  include/drm/drm_crtc.h     | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b5bce5b..74ad72f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -727,7 +727,7 @@ DEFINE_WW_CLASS(crtc_ww_class);
>   */
>  int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  			      struct drm_plane *primary,
> -			      void *cursor,
> +			      struct drm_plane *cursor,
>  			      const struct drm_crtc_funcs *funcs)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> @@ -752,8 +752,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	config->num_crtc++;
>  
>  	crtc->primary = primary;
> +	crtc->cursor = cursor;
>  	if (primary)
>  		primary->possible_crtcs = 1 << drm_crtc_index(crtc);
> +	if (cursor)
> +		cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>  
>   out:
>  	drm_modeset_unlock_all(dev);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b8c7a9a..4ee7e26 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -856,7 +856,7 @@ struct drm_prop_enum_list {
>  extern int drm_crtc_init_with_planes(struct drm_device *dev,
>  				     struct drm_crtc *crtc,
>  				     struct drm_plane *primary,
> -				     void *cursor,
> +				     struct drm_plane *cursor,
>  				     const struct drm_crtc_funcs *funcs);
>  extern int drm_crtc_init(struct drm_device *dev,
>  			 struct drm_crtc *crtc,

Reviewed-by: Pallavi G <pallavi.g@intel.com>

Re: [PATCH 5/6] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer (v2)

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> Refactor cursor buffer setting such that the code to actually update the
> cursor lives in a new function, intel_crtc_cursor_set_obj(), and takes
> a GEM object as a parameter.  The existing legacy cursor ioctl handler,
> intel_crtc_cursor_set() will now perform the userspace handle lookup and
> then call this new function.
> 
> This refactoring is in preparation for the universal plane cursor
> support where we'll want to update the cursor with an actual GEM buffer
> object (obtained via drm_framebuffer) rather than a userspace handle.
> 
> v2:  Drop obvious kerneldoc and replace with note about function's
>      reference consumption
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..1beeb2a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8089,21 +8089,26 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	intel_crtc->cursor_base = base;
>  }
>  
> -static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> -				 struct drm_file *file,
> -				 uint32_t handle,
> -				 uint32_t width, uint32_t height)
> +/*
> + * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
> + *
> + * Note that the object's reference will be consumed if the update fails.  If
> + * the update succeeds, the reference of the old object (if any) will be
> + * consumed.
> + */
> +static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> +				     struct drm_i915_gem_object *obj,
> +				     uint32_t width, uint32_t height)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct drm_i915_gem_object *obj;
>  	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
>  	/* if we want to turn off the cursor ignore width and height */
> -	if (!handle) {
> +	if (!obj) {
>  		DRM_DEBUG_KMS("cursor off\n");
>  		addr = 0;
>  		obj = NULL;
> @@ -8119,12 +8124,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> -	if (&obj->base == NULL)
> -		return -ENOENT;
> -
>  	if (obj->base.size < width * height * 4) {
> -		DRM_DEBUG_KMS("buffer is to small\n");
> +		DRM_DEBUG_KMS("buffer is too small\n");
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
> @@ -8207,6 +8208,25 @@ fail:
>  	return ret;
>  }
>  
> +static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> +				 struct drm_file *file,
> +				 uint32_t handle,
> +				 uint32_t width, uint32_t height)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_gem_object *obj;
> +
> +	if (handle) {
> +		obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> +		if (&obj->base == NULL)
> +			return -ENOENT;
> +	} else {
> +		obj = NULL;
> +	}
> +
> +	return intel_crtc_cursor_set_obj(crtc, obj, width, height);
> +}
> +
>  static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);


Reviewed-by: Pallavi G <pallavi.g@intel.com>

Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

[G, Pallavi]

On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> The DRM core will translate calls to legacy cursor ioctls into universal
> cursor calls automatically, so there's no need to maintain the legacy
> cursor support.  This greatly simplifies the transition since we don't
> have to handle reference counting differently depending on which cursor
> interface was called.
> 
> The aim here is to transition to the universal plane interface with
> minimal code change.  There's a lot of cleanup that can be done (e.g.,
> using state stored in crtc->cursor->fb rather than intel_crtc) that is
> left to future patches.
> 
> v4:
>  - Drop drm_gem_object_unreference() that is no longer needed now that
>    we receive the GEM obj directly rather than looking up the ID.
> v3:
>  - Pass cursor obj to intel_crtc_cursor_set_obj() if cursor fb changes,
>    even if 'visible' is false.  intel_crtc_cursor_set_obj() will notice
>    that the cursor isn't visible and disable it properly, but we still
>    need to get intel_crtc->cursor_addr set properly so that we behave
>    properly if the cursor becomes visible again in the future without
>    changing the cursor buffer (noted by Chris Wilson and verified
>    via i-g-t kms_cursor_crc).
>  - s/drm_plane_init/drm_universal_plane_init/.  Due to type
>    compatibility between enum and bool, everything actually works
>    correctly with the wrong init call, except for the type of plane that
>    gets exposed to userspace (it shows up as type 'primary' rather than
>    type 'cursor').
> v2:
>  - Remove duplicate dimension checks on cursor
>  - Drop explicit cursor disable from crtc destroy (fb & plane
>    destruction will take care of that now)
>  - Use DRM plane helper to check update parameters
> 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 166 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  2 files changed, 118 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1beeb2a..1880c18 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
>  	DRM_FORMAT_ABGR2101010,
>  };
>  
> +/* Cursor formats */
> +static const uint32_t intel_cursor_formats[] = {
> +	DRM_FORMAT_ARGB8888,
> +};
> +
>  #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
>  ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
>  
> @@ -8044,8 +8049,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	int x = intel_crtc->cursor_x;
> -	int y = intel_crtc->cursor_y;
> +	int x = crtc->cursor_x;
> +	int y = crtc->cursor_y;
>  	u32 base = 0, pos = 0;
>  
>  	if (on)
> @@ -8180,7 +8185,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	if (intel_crtc->cursor_bo) {
>  		if (!INTEL_INFO(dev)->cursor_needs_physical)
>  			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -		drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
>  	}
>  
>  	mutex_unlock(&dev->struct_mutex);
> @@ -8208,38 +8212,6 @@ fail:
>  	return ret;
>  }
>  
> -static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> -				 struct drm_file *file,
> -				 uint32_t handle,
> -				 uint32_t width, uint32_t height)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_gem_object *obj;
> -
> -	if (handle) {
> -		obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> -		if (&obj->base == NULL)
> -			return -ENOENT;
> -	} else {
> -		obj = NULL;
> -	}
> -
> -	return intel_crtc_cursor_set_obj(crtc, obj, width, height);
> -}
> -
> -static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> -{
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> -	intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
> -	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
> -
> -	if (intel_crtc->active)
> -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -
> -	return 0;
> -}
> -
>  static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
>  				 u16 *blue, uint32_t start, uint32_t size)
>  {
> @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)

        Please help me to understand how the cursor enable/disable will
handled in the legacy path if we remove the cursor disable from
intel_crtc_destroy
> -
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> @@ -10942,8 +10912,6 @@ out_config:
>  }
>  
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
> -	.cursor_set = intel_crtc_cursor_set,
> -	.cursor_move = intel_crtc_cursor_move,

I don't find the corresponding changes in the drm layer related to the
cursor_set and cursor_move removal. Even in the patch 3 only for the
universal plane drm_mode_cursor_universal is called what about the
legacy path?

>  	.gamma_set = intel_crtc_gamma_set,
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
>  -11196,7 +11164,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> -static void intel_primary_plane_destroy(struct drm_plane *plane)
> +/* Common destruction function for both primary and cursor planes */
> +static void intel_plane_destroy(struct drm_plane *plane)
>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	drm_plane_cleanup(plane);
> @@ -11206,7 +11175,7 @@ static void intel_primary_plane_destroy(struct drm_plane *plane)
>  static const struct drm_plane_funcs intel_primary_plane_funcs = {
>  	.update_plane = intel_primary_plane_setplane,
>  	.disable_plane = intel_primary_plane_disable,
> -	.destroy = intel_primary_plane_destroy,
> +	.destroy = intel_plane_destroy,
>  };
>  
>  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11242,11 +11211,100 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	return &primary->base;
>  }
>  
> +static int
> +intel_cursor_plane_disable(struct drm_plane *plane)
> +{
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);
> +
> +	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +}
> +
> +static int
> +intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
> +			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			  unsigned int crtc_w, unsigned int crtc_h,
> +			  uint32_t src_x, uint32_t src_y,
> +			  uint32_t src_w, uint32_t src_h)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct drm_rect dest = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect src = {
> +		/* 16.16 fixed point */
> +		.x1 = src_x,
> +		.y1 = src_y,
> +		.x2 = src_x + src_w,
> +		.y2 = src_y + src_h,
> +	};
> +	const struct drm_rect clip = {
> +		/* integer pixels */
> +		.x2 = intel_crtc->config.pipe_src_w,
> +		.y2 = intel_crtc->config.pipe_src_h,
> +	};
> +	bool visible;
> +	int ret;
> +
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    true, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	crtc->cursor_x = crtc_x;
> +	crtc->cursor_y = crtc_y;
> +	if (fb != crtc->cursor->fb) {
> +		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> +	} else {
> +		intel_crtc_update_cursor(crtc, visible);
> +		return 0;
> +	}
> +}
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> +	.update_plane = intel_cursor_plane_update,
> +	.disable_plane = intel_cursor_plane_disable,
> +	.destroy = intel_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> +						   int pipe)
> +{
> +	struct intel_plane *cursor;
> +
> +	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> +	if (cursor == NULL)
> +		return NULL;
> +
> +	cursor->can_scale = false;
> +	cursor->max_downscale = 1;
> +	cursor->pipe = pipe;
> +	cursor->plane = pipe;
> +
> +	drm_universal_plane_init(dev, &cursor->base, 0,
> +				 &intel_cursor_plane_funcs,
> +				 intel_cursor_formats,
> +				 ARRAY_SIZE(intel_cursor_formats),
> +				 DRM_PLANE_TYPE_CURSOR);
> +	return &cursor->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	struct drm_plane *primary;
> +	struct drm_plane *primary = NULL;
> +	struct drm_plane *cursor = NULL;
>  	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> @@ -11254,13 +11312,17 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		return;
>  
>  	primary = intel_primary_plane_create(dev, pipe);
> +	if (!primary)
> +		goto fail;
> +
> +	cursor = intel_cursor_plane_create(dev, pipe);
> +	if (!cursor)
> +		goto fail;
> +
>  	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> -					NULL, &intel_crtc_funcs);
> -	if (ret) {
> -		drm_plane_cleanup(primary);
> -		kfree(intel_crtc);
> -		return;
> -	}
> +					cursor, &intel_crtc_funcs);
> +	if (ret)
> +		goto fail;
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> @@ -11293,6 +11355,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> +	return;
> +
> +fail:
> +	if (primary)
> +		drm_plane_cleanup(primary);
> +	if (cursor)
> +		drm_plane_cleanup(cursor);
> +	kfree(intel_crtc);
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124..57a36ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,7 +384,6 @@ struct intel_crtc {
>  
>  	struct drm_i915_gem_object *cursor_bo;
>  	uint32_t cursor_addr;
> -	int16_t cursor_x, cursor_y;
>  	int16_t cursor_width, cursor_height;
>  	uint32_t cursor_cntl;
>  	uint32_t cursor_base;

Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

[Matt Roper]

On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote:
> On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
...
> > @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> >  		kfree(work);
> >  	}
> >  
> > -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)
> 
>         Please help me to understand how the cursor enable/disable will
> handled in the legacy path if we remove the cursor disable from
> intel_crtc_destroy

Good question.  When the driver is shutting down the DRM core tears down
all the KMS stuff associated with the driver.  One of those steps is
destroying the CRTC which, as you note, previously would take care of
turning off the cursor plane.  However now that the cursor exists as its
own drm_plane, and whatever is being scanned out by the cursor is a real
drm_framebuffer that the DRM core knows about, the cursor's FB should
get destroyed by the DRM core, which will trigger the cursor disable
entrypoint on the driver.  The call sequence is

drm_framebuffer_remove() -> drm_force_plane_disable()
                         -> intel_cursor_plane_disable()


> > -
> >  	drm_crtc_cleanup(crtc);
> >  
> >  	kfree(intel_crtc);
> > @@ -10942,8 +10912,6 @@ out_config:
> >  }
> >  
> >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> > -	.cursor_set = intel_crtc_cursor_set,
> > -	.cursor_move = intel_crtc_cursor_move,
> 
> I don't find the corresponding changes in the drm layer related to the
> cursor_set and cursor_move removal. Even in the patch 3 only for the
> universal plane drm_mode_cursor_universal is called what about the
> legacy path?

Right, the driver entrypoints remain in the drm_crtc_funcs structure
because other non-i915 drivers may not have implemented universal cursor
planes, so the DRM core will still call into their .cursor_set and
.cursor_move entrypoints when they issue a legacy cursor ioctl.  We
don't want to force other driver authors to update to universal planes
until they're ready, so we want to keep the old code paths working until
everyone has updated.

Drivers like i915 that do get updated to have universal cursor planes
will no longer receive calls into these legacy entrypoints anymore
(since everything will come into the universal entrypoint), so there's
no need for us to keep the legacy .cursor_set and .cursor_move around in
our own driver.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

[G, Pallavi]

On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote:
> On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote:
> > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> ...
> > > @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> > >  		kfree(work);
> > >  	}
> > >  
> > > -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)
> > 
> >         Please help me to understand how the cursor enable/disable will
> > handled in the legacy path if we remove the cursor disable from
> > intel_crtc_destroy
> 
> Good question.  When the driver is shutting down the DRM core tears down
> all the KMS stuff associated with the driver.  One of those steps is
> destroying the CRTC which, as you note, previously would take care of
> turning off the cursor plane.  However now that the cursor exists as its
> own drm_plane, and whatever is being scanned out by the cursor is a real
> drm_framebuffer that the DRM core knows about, the cursor's FB should
> get destroyed by the DRM core, which will trigger the cursor disable
> entrypoint on the driver.  The call sequence is
> 
> drm_framebuffer_remove() -> drm_force_plane_disable()
>                          -> intel_cursor_plane_disable()
> 
> 
> > > -
> > >  	drm_crtc_cleanup(crtc);
> > >  
> > >  	kfree(intel_crtc);
> > > @@ -10942,8 +10912,6 @@ out_config:
> > >  }
> > >  
> > >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> > > -	.cursor_set = intel_crtc_cursor_set,
> > > -	.cursor_move = intel_crtc_cursor_move,
> > 
> > I don't find the corresponding changes in the drm layer related to the
> > cursor_set and cursor_move removal. Even in the patch 3 only for the
> > universal plane drm_mode_cursor_universal is called what about the
> > legacy path?
> 
> Right, the driver entrypoints remain in the drm_crtc_funcs structure
> because other non-i915 drivers may not have implemented universal cursor
> planes, so the DRM core will still call into their .cursor_set and
> .cursor_move entrypoints when they issue a legacy cursor ioctl.  We
> don't want to force other driver authors to update to universal planes
> until they're ready, so we want to keep the old code paths working until
> everyone has updated.
> 
> Drivers like i915 that do get updated to have universal cursor planes
> will no longer receive calls into these legacy entrypoints anymore
> (since everything will come into the universal entrypoint), so there's
> no need for us to keep the legacy .cursor_set and .cursor_move around in
> our own driver.
> 
> 
> Matt
> 
Thanks for the clarification.
with this my doubts are clear. I reviewed the entire patch series with
respect to the legacy support as well as the universal cursor plane
support in general. I dont find any issues and design gaps. I hope Matt
is working on the igt based test for the universal plane support (seen
some patches related to that in the mail thread).

Reviewed-by: Pallavi G <pallavi.g@intel.com>

Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)

[Daniel Vetter]

On Fri, Jun 13, 2014 at 04:35:38AM +0000, G, Pallavi wrote:
> On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote:
> > On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote:
> > > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote:
> > ...
> > > > @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> > > >  		kfree(work);
> > > >  	}
> > > >  
> > > > -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)
> > > 
> > >         Please help me to understand how the cursor enable/disable will
> > > handled in the legacy path if we remove the cursor disable from
> > > intel_crtc_destroy
> > 
> > Good question.  When the driver is shutting down the DRM core tears down
> > all the KMS stuff associated with the driver.  One of those steps is
> > destroying the CRTC which, as you note, previously would take care of
> > turning off the cursor plane.  However now that the cursor exists as its
> > own drm_plane, and whatever is being scanned out by the cursor is a real
> > drm_framebuffer that the DRM core knows about, the cursor's FB should
> > get destroyed by the DRM core, which will trigger the cursor disable
> > entrypoint on the driver.  The call sequence is
> > 
> > drm_framebuffer_remove() -> drm_force_plane_disable()
> >                          -> intel_cursor_plane_disable()
> > 
> > 
> > > > -
> > > >  	drm_crtc_cleanup(crtc);
> > > >  
> > > >  	kfree(intel_crtc);
> > > > @@ -10942,8 +10912,6 @@ out_config:
> > > >  }
> > > >  
> > > >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> > > > -	.cursor_set = intel_crtc_cursor_set,
> > > > -	.cursor_move = intel_crtc_cursor_move,
> > > 
> > > I don't find the corresponding changes in the drm layer related to the
> > > cursor_set and cursor_move removal. Even in the patch 3 only for the
> > > universal plane drm_mode_cursor_universal is called what about the
> > > legacy path?
> > 
> > Right, the driver entrypoints remain in the drm_crtc_funcs structure
> > because other non-i915 drivers may not have implemented universal cursor
> > planes, so the DRM core will still call into their .cursor_set and
> > .cursor_move entrypoints when they issue a legacy cursor ioctl.  We
> > don't want to force other driver authors to update to universal planes
> > until they're ready, so we want to keep the old code paths working until
> > everyone has updated.
> > 
> > Drivers like i915 that do get updated to have universal cursor planes
> > will no longer receive calls into these legacy entrypoints anymore
> > (since everything will come into the universal entrypoint), so there's
> > no need for us to keep the legacy .cursor_set and .cursor_move around in
> > our own driver.
> > 
> > 
> > Matt
> > 
> Thanks for the clarification.
> with this my doubts are clear. I reviewed the entire patch series with
> respect to the legacy support as well as the universal cursor plane
> support in general. I dont find any issues and design gaps. I hope Matt
> is working on the igt based test for the universal plane support (seen
> some patches related to that in the mail thread).
> 
> Reviewed-by: Pallavi G <pallavi.g@intel.com>

Thanks a lot for the review and patches, all merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Daniel Vetter]

On Fri, May 23, 2014 at 12:57 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> When we switch to atomic modeset in i-g-t, everything will be coming in
> via the propertyset interface, right?  Since the range limiting check is
> in the setplane ioctl code now, I'd expect us to bypass it completely.

Yeah, I guess we can look at that again when it happens.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Matt Roper]

On Fri, May 23, 2014 at 12:51:29AM +0200, Daniel Vetter wrote:
> On Thu, May 22, 2014 at 03:48:16PM -0700, Matt Roper wrote:
> > On Fri, May 23, 2014 at 12:37:52AM +0200, Daniel Vetter wrote:
> > > On Thu, May 22, 2014 at 6:33 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > > v3:
> > > >  - Move integer overflow checking from setplane_internal to setplane
> > > >    ioctl.  The upcoming legacy cursor support via universal planes needs
> > > >    to maintain current cursor ioctl semantics and not return error for
> > > >    these extreme values (found via intel-gpu-tools kms_cursor_crc test).
> > > 
> > > Imo that's just the test being silly. I think it's valid to reject
> > > such nonsense with -EINVAL and limit the random-walk range of our
> > > testcase a bit. The igt testcases are just guidelines for what our ABI
> > > is, but not the hard rules. And clarifying the ABI in such cases is
> > > imo the right approach.
> > > 
> > > That random-walk testcase just was meant to test correctly clamping.
> > > E.g. on hardware with just 12bit for the cursor placement we'd wrap
> > > around and show a cursor again that should be off. So as long as you
> > > still test that you can restrict the range of the test a bit.
> > > -Daniel
> > 
> > I was a bit torn on this.  Since we accept offscreen plane positions,
> > I'm not sure it makes sense to really treat "offscreen" differently than
> > "really, really offscreen."  The legacy cursor ioctls have never cared
> > about this in the past so I didn't want to add in artificial range
> > limiting just because the setplane ioctl has had it for a while.
> > 
> > There's actually an i-g-t test that specifically tries to program
> > specifically at INT_MAX (not just the random walk test that might get
> > lucky and go out of bounds), so I figured the best course was just to
> > preserve the current behavior; I can't really see any downsides to not
> > range-limiting the cursors.
> 
> Ok, if you think this won't create an unnecessary fuss in the code then
> I'm ok with this, too. After all most hw has limited offset fields, so
> drivers must clamp anyway. So dunno really how much sense that limit check
> makes in set_plane. But we have it, so doesn't hurt too much really.
> 
> A different consideration though is that with the igt_kms library the plan
> is to just switch to atomic modeset eventually, which also means using
> universal planes for cursors and primary planes. Which means as soon as we
> do that we'll likely hit that restriction again.
> 
> Not sure what to do now here.
> -Daniel

When we switch to atomic modeset in i-g-t, everything will be coming in
via the propertyset interface, right?  Since the range limiting check is
in the setplane ioctl code now, I'd expect us to bypass it completely.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Daniel Vetter]

On Thu, May 22, 2014 at 03:48:16PM -0700, Matt Roper wrote:
> On Fri, May 23, 2014 at 12:37:52AM +0200, Daniel Vetter wrote:
> > On Thu, May 22, 2014 at 6:33 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > v3:
> > >  - Move integer overflow checking from setplane_internal to setplane
> > >    ioctl.  The upcoming legacy cursor support via universal planes needs
> > >    to maintain current cursor ioctl semantics and not return error for
> > >    these extreme values (found via intel-gpu-tools kms_cursor_crc test).
> > 
> > Imo that's just the test being silly. I think it's valid to reject
> > such nonsense with -EINVAL and limit the random-walk range of our
> > testcase a bit. The igt testcases are just guidelines for what our ABI
> > is, but not the hard rules. And clarifying the ABI in such cases is
> > imo the right approach.
> > 
> > That random-walk testcase just was meant to test correctly clamping.
> > E.g. on hardware with just 12bit for the cursor placement we'd wrap
> > around and show a cursor again that should be off. So as long as you
> > still test that you can restrict the range of the test a bit.
> > -Daniel
> 
> I was a bit torn on this.  Since we accept offscreen plane positions,
> I'm not sure it makes sense to really treat "offscreen" differently than
> "really, really offscreen."  The legacy cursor ioctls have never cared
> about this in the past so I didn't want to add in artificial range
> limiting just because the setplane ioctl has had it for a while.
> 
> There's actually an i-g-t test that specifically tries to program
> specifically at INT_MAX (not just the random walk test that might get
> lucky and go out of bounds), so I figured the best course was just to
> preserve the current behavior; I can't really see any downsides to not
> range-limiting the cursors.

Ok, if you think this won't create an unnecessary fuss in the code then
I'm ok with this, too. After all most hw has limited offset fields, so
drivers must clamp anyway. So dunno really how much sense that limit check
makes in set_plane. But we have it, so doesn't hurt too much really.

A different consideration though is that with the igt_kms library the plan
is to just switch to atomic modeset eventually, which also means using
universal planes for cursors and primary planes. Which means as soon as we
do that we'll likely hit that restriction again.

Not sure what to do now here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Matt Roper]

On Fri, May 23, 2014 at 12:37:52AM +0200, Daniel Vetter wrote:
> On Thu, May 22, 2014 at 6:33 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > v3:
> >  - Move integer overflow checking from setplane_internal to setplane
> >    ioctl.  The upcoming legacy cursor support via universal planes needs
> >    to maintain current cursor ioctl semantics and not return error for
> >    these extreme values (found via intel-gpu-tools kms_cursor_crc test).
> 
> Imo that's just the test being silly. I think it's valid to reject
> such nonsense with -EINVAL and limit the random-walk range of our
> testcase a bit. The igt testcases are just guidelines for what our ABI
> is, but not the hard rules. And clarifying the ABI in such cases is
> imo the right approach.
> 
> That random-walk testcase just was meant to test correctly clamping.
> E.g. on hardware with just 12bit for the cursor placement we'd wrap
> around and show a cursor again that should be off. So as long as you
> still test that you can restrict the range of the test a bit.
> -Daniel

I was a bit torn on this.  Since we accept offscreen plane positions,
I'm not sure it makes sense to really treat "offscreen" differently than
"really, really offscreen."  The legacy cursor ioctls have never cared
about this in the past so I didn't want to add in artificial range
limiting just because the setplane ioctl has had it for a while.

There's actually an i-g-t test that specifically tries to program
specifically at INT_MAX (not just the random walk test that might get
lucky and go out of bounds), so I figured the best course was just to
preserve the current behavior; I can't really see any downsides to not
range-limiting the cursors.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Daniel Vetter]

On Thu, May 22, 2014 at 6:33 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> v3:
>  - Move integer overflow checking from setplane_internal to setplane
>    ioctl.  The upcoming legacy cursor support via universal planes needs
>    to maintain current cursor ioctl semantics and not return error for
>    these extreme values (found via intel-gpu-tools kms_cursor_crc test).

Imo that's just the test being silly. I think it's valid to reject
such nonsense with -EINVAL and limit the random-walk range of our
testcase a bit. The igt testcases are just guidelines for what our ABI
is, but not the hard rules. And clarifying the ABI in such cases is
imo the right approach.

That random-walk testcase just was meant to test correctly clamping.
E.g. on hardware with just 12bit for the cursor placement we'd wrap
around and show a cursor again that should be off. So as long as you
still test that you can restrict the range of the test a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/6] drm: Refactor setplane to allow internal use (v3)

[Matt Roper]

Refactor DRM setplane code into a new setplane_internal() function that
takes DRM objects directly as parameters rather than looking them up by
ID.  We'll use this in a future patch when we implement legacy cursor
ioctls on top of the universal plane interface.

v3:
 - Move integer overflow checking from setplane_internal to setplane
   ioctl.  The upcoming legacy cursor support via universal planes needs
   to maintain current cursor ioctl semantics and not return error for
   these extreme values (found via intel-gpu-tools kms_cursor_crc test).
v2:
 - Allow planes to be disabled without a valid crtc again (and add
   mention of this to setplane's kerneldoc, since it doesn't seem to be
   mentioned anywhere else).
 - Reformat some parameter line wrap

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 181 +++++++++++++++++++++++++--------------------
 1 file changed, 102 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index bbc1604..a362738 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2092,48 +2092,32 @@ out:
 	return ret;
 }
 
-/**
- * drm_mode_setplane - configure a plane's configuration
- * @dev: DRM device
- * @data: ioctl data*
- * @file_priv: DRM file info
+/*
+ * setplane_internal - setplane handler for internal callers
  *
- * Set plane configuration, including placement, fb, scaling, and other factors.
- * Or pass a NULL fb to disable.
+ * Note that we assume an extra reference has already been taken on fb.  If the
+ * update fails, this reference will be dropped before return; if it succeeds,
+ * the previous framebuffer (if any) will be unreferenced instead.
  *
- * Returns:
- * Zero on success, errno on failure.
+ * src_{x,y,w,h} are provided in 16.16 fixed point format
  */
-int drm_mode_setplane(struct drm_device *dev, void *data,
-		      struct drm_file *file_priv)
+static int setplane_internal(struct drm_crtc *crtc,
+			     struct drm_plane *plane,
+			     struct drm_framebuffer *fb,
+			     int32_t crtc_x, int32_t crtc_y,
+			     uint32_t crtc_w, uint32_t crtc_h,
+			     /* src_{x,y,w,h} values are 16.16 fixed point */
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
 {
-	struct drm_mode_set_plane *plane_req = data;
-	struct drm_mode_object *obj;
-	struct drm_plane *plane;
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *old_fb = NULL;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return -EINVAL;
-
-	/*
-	 * First, find the plane, crtc, and fb objects.  If not available,
-	 * we don't bother to call the driver.
-	 */
-	obj = drm_mode_object_find(dev, plane_req->plane_id,
-				   DRM_MODE_OBJECT_PLANE);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown plane ID %d\n",
-			      plane_req->plane_id);
-		return -ENOENT;
-	}
-	plane = obj_to_plane(obj);
-
 	/* No fb means shut it down */
-	if (!plane_req->fb_id) {
+	if (!fb) {
 		drm_modeset_lock_all(dev);
 		old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane);
@@ -2147,16 +2131,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	obj = drm_mode_object_find(dev, plane_req->crtc_id,
-				   DRM_MODE_OBJECT_CRTC);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
-			      plane_req->crtc_id);
-		ret = -ENOENT;
-		goto out;
-	}
-	crtc = obj_to_crtc(obj);
-
 	/* Check whether this plane is usable on this CRTC */
 	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
 		DRM_DEBUG_KMS("Invalid crtc for plane\n");
@@ -2164,14 +2138,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
-	if (!fb) {
-		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
-			      plane_req->fb_id);
-		ret = -ENOENT;
-		goto out;
-	}
-
 	/* Check whether this plane supports the fb pixel format. */
 	for (i = 0; i < plane->format_count; i++)
 		if (fb->pixel_format == plane->format_types[i])
@@ -2187,43 +2153,25 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	fb_height = fb->height << 16;
 
 	/* Make sure source coordinates are inside the fb. */
-	if (plane_req->src_w > fb_width ||
-	    plane_req->src_x > fb_width - plane_req->src_w ||
-	    plane_req->src_h > fb_height ||
-	    plane_req->src_y > fb_height - plane_req->src_h) {
+	if (src_w > fb_width ||
+	    src_x > fb_width - src_w ||
+	    src_h > fb_height ||
+	    src_y > fb_height - src_h) {
 		DRM_DEBUG_KMS("Invalid source coordinates "
 			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
-			      plane_req->src_w >> 16,
-			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
-			      plane_req->src_h >> 16,
-			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
-			      plane_req->src_x >> 16,
-			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
-			      plane_req->src_y >> 16,
-			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
+			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
+			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
+			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
+			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
 		ret = -ENOSPC;
 		goto out;
 	}
 
-	/* Give drivers some help against integer overflows */
-	if (plane_req->crtc_w > INT_MAX ||
-	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
-	    plane_req->crtc_h > INT_MAX ||
-	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
-		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      plane_req->crtc_w, plane_req->crtc_h,
-			      plane_req->crtc_x, plane_req->crtc_y);
-		ret = -ERANGE;
-		goto out;
-	}
-
 	drm_modeset_lock_all(dev);
 	old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
-					 plane_req->crtc_x, plane_req->crtc_y,
-					 plane_req->crtc_w, plane_req->crtc_h,
-					 plane_req->src_x, plane_req->src_y,
-					 plane_req->src_w, plane_req->src_h);
+					 crtc_x, crtc_y, crtc_w, crtc_h,
+					 src_x, src_y, src_w, src_h);
 	if (!ret) {
 		plane->crtc = crtc;
 		plane->fb = fb;
@@ -2240,6 +2188,81 @@ out:
 		drm_framebuffer_unreference(old_fb);
 
 	return ret;
+
+}
+
+/**
+ * drm_mode_setplane - configure a plane's configuration
+ * @dev: DRM device
+ * @data: ioctl data*
+ * @file_priv: DRM file info
+ *
+ * Set plane configuration, including placement, fb, scaling, and other factors.
+ * Or pass a NULL fb to disable (planes may be disabled without providing a
+ * valid crtc).
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_setplane(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv)
+{
+	struct drm_mode_set_plane *plane_req = data;
+	struct drm_mode_object *obj;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc = NULL;
+	struct drm_framebuffer *fb = NULL;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	/* Give drivers some help against integer overflows */
+	if (plane_req->crtc_w > INT_MAX ||
+	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
+	    plane_req->crtc_h > INT_MAX ||
+	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
+		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
+			      plane_req->crtc_w, plane_req->crtc_h,
+			      plane_req->crtc_x, plane_req->crtc_y);
+		return -ERANGE;
+	}
+
+	/*
+	 * First, find the plane, crtc, and fb objects.  If not available,
+	 * we don't bother to call the driver.
+	 */
+	obj = drm_mode_object_find(dev, plane_req->plane_id,
+				   DRM_MODE_OBJECT_PLANE);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown plane ID %d\n",
+			      plane_req->plane_id);
+		return -ENOENT;
+	}
+	plane = obj_to_plane(obj);
+
+	if (plane_req->fb_id) {
+		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
+		if (!fb) {
+			DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
+				      plane_req->fb_id);
+			return -ENOENT;
+		}
+
+		obj = drm_mode_object_find(dev, plane_req->crtc_id,
+					   DRM_MODE_OBJECT_CRTC);
+		if (!obj) {
+			DRM_DEBUG_KMS("Unknown crtc ID %d\n",
+				      plane_req->crtc_id);
+			return -ENOENT;
+		}
+		crtc = obj_to_crtc(obj);
+	}
+
+	return setplane_internal(crtc, plane, fb,
+				 plane_req->crtc_x, plane_req->crtc_y,
+				 plane_req->crtc_w, plane_req->crtc_h,
+				 plane_req->src_x, plane_req->src_y,
+				 plane_req->src_w, plane_req->src_h);
 }
 
 /**
-- 
1.8.5.1