[RFC 0/4] Cursor support with universal planes

[RFC 0/4] Cursor support with universal planes

[Matt Roper]

Cursor planes are a bit trickier to support via the universal plane interface
than primary planes were.  Legacy cursor ioctls take handles to driver buffers
directly whereas the universal plane API takes drm_framebuffer's that represent
a buffer; due to this mismatch it isn't possible to implement a set of cursor
helpers than provide "free" generic universal cursor support without driver
changes as we did with primary planes.  Drivers will need to be updated to
receive cursor support via the universal plane API.

If a driver tries to implement two interfaces for cursor support (legacy ioctl
and universal planes), the reference counting can get very tricky/messy since
we need to take into account userspace that may mix and match calls to both
interfaces.  To address that, patch #1 in this set causes legacy cursor ioctls
to be implemented using a driver's universal plane support if the driver
registers a primary plane.  Calls to the legacy set_cursor ioctl will wrap the
provided driver buffer in a drm_framebuffer and then pass that along to the
universal plane interface.  From a driver's point of view, this causes all
cursor operations to arrive on the universal plane interface, regardless of
which userspace API was used, which simplifies things greatly for the driver.

Patch #2 makes some minor changes to ensure drivers can successfully register a
cursor plane with the DRM core.

Patch #3 does some minor i915 refactoring in preparation for the move to
universal planes.

Patch #4 transitions the i915 driver to universal planes.  The changes here are
intentionally minimal for ease of review.  It's likely that we could perform
further cleanup in the future to eliminate some of the cursor state tracked in
intel_crtc (e.g., cursor_width/cursor_height) since that information can be
also be derived from crtc->cursor->fb.


Matt Roper (4):
  drm: Support legacy cursor ioctls via universal planes when possible
  drm: Allow drivers to register cursor planes with crtc
  drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer
  drm/i915: Switch to unified plane cursor handling

 drivers/gpu/drm/drm_crtc.c           | 113 ++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c | 188 ++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |   1 -
 include/drm/drm_crtc.h               |   6 +-
 4 files changed, 267 insertions(+), 41 deletions(-)

-- 
1.8.5.1

[RFC 1/4] drm: Support legacy cursor ioctls via universal planes when possible

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

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 108 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |   4 ++
 2 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b6d6c04..3afdc45 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2476,6 +2476,107 @@ 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 },
+	};
+	struct drm_mode_set_plane planereq = { 0 };
+	int ret = 0;
+
+	BUG_ON(!crtc->cursor);
+
+	if (req->flags & DRM_MODE_CURSOR_BO) {
+		if (req->handle) {
+			ret = drm_mode_addfb2(dev, &fbreq, file_priv);
+			if (ret) {
+				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
+				return ret;
+			}
+
+			/*
+			 * Get framebuffer we just created (but use
+			 * __drm_framebuffer_lookup() so that we don't take an
+			 * extra reference to it
+			 */
+			mutex_lock(&dev->mode_config.fb_lock);
+			fb = __drm_framebuffer_lookup(dev, fbreq.fb_id);
+			mutex_unlock(&dev->mode_config.fb_lock);
+
+			/*
+			 * We just created this; we shouldn't be able to fail
+			 * the lookup
+			 */
+			BUG_ON(!fb);
+		} else {
+			fb = NULL;
+		}
+	} else {
+		fb = crtc->cursor->fb;
+	}
+
+	planereq.plane_id = crtc->cursor->base.id;
+	planereq.crtc_id = crtc->base.id;
+	planereq.fb_id = fb ? fb->base.id : 0;
+
+	if (req->flags & DRM_MODE_CURSOR_MOVE) {
+		planereq.crtc_x = req->x;
+		planereq.crtc_y = req->y;
+	} else {
+		planereq.crtc_x = crtc->cursor_x;
+		planereq.crtc_y = crtc->cursor_y;
+	}
+
+	if (fb) {
+		planereq.crtc_w = fb->width;
+		planereq.crtc_h = fb->height;
+		planereq.src_w = fb->width << 16;
+		planereq.src_h = fb->height << 16;
+	}
+
+	ret = drm_mode_setplane(dev, &planereq, file_priv);
+	if (ret) {
+		if (req->flags & DRM_MODE_CURSOR_BO)
+			drm_mode_rmfb(dev, &fb->base.id, file_priv);
+		return ret;
+	}
+
+	/* Update successful; save new cursor position, if necessary */
+	if (req->flags & DRM_MODE_CURSOR_MOVE) {
+		crtc->cursor_x = req->x;
+		crtc->cursor_y = req->y;
+	}
+
+	return 0;
+}
+
 static int drm_mode_cursor_common(struct drm_device *dev,
 				  struct drm_mode_cursor2 *req,
 				  struct drm_file *file_priv)
@@ -2497,6 +2598,13 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	}
 	crtc = obj_to_crtc(obj);
 
+	/*
+	 * 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);
+
 	mutex_lock(&crtc->mutex);
 	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 c061bb3..e5d22ff 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -311,6 +311,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

Re: [Intel-gfx] [RFC 1/4] drm: Support legacy cursor ioctls via universal planes when possible

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:26PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Some comments for polish and race avoidance below, but looks sane overall.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 108 +++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |   4 ++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b6d6c04..3afdc45 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2476,6 +2476,107 @@ 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 },
> +	};
> +	struct drm_mode_set_plane planereq = { 0 };
> +	int ret = 0;
> +
> +	BUG_ON(!crtc->cursor);
> +
> +	if (req->flags & DRM_MODE_CURSOR_BO) {
> +		if (req->handle) {
> +			ret = drm_mode_addfb2(dev, &fbreq, file_priv);
> +			if (ret) {
> +				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
> +				return ret;
> +			}
> +
> +			/*
> +			 * Get framebuffer we just created (but use
> +			 * __drm_framebuffer_lookup() so that we don't take an
> +			 * extra reference to it
> +			 */
> +			mutex_lock(&dev->mode_config.fb_lock);
> +			fb = __drm_framebuffer_lookup(dev, fbreq.fb_id);
> +			mutex_unlock(&dev->mode_config.fb_lock);

Imo use the normal function here instead of open-coding and also grab a
reference for the else case. Then drop the acquired reference again later
on.

Actually I think we should extract just the parts we need from addfb2 and
directly return the drm_framebuffer *, reference included - jumping
through this lookup indirection looks fraught with races since userspace
could sneak in and rip it out from underneath you.

So the sequence would be:
1. Call ->fb_create directly.
2. Grab additional reference so the fb doesn't disappear.
3. Register fb in the idr (could be extracted from addfb2), this will take
one of the references.
1.-3. alternate for !BO: Just grab reference to crtc->cursor->fb.
4. Call into cursor->set_plane.
5. Drop temporary reference to fb.


> +
> +			/*
> +			 * We just created this; we shouldn't be able to fail
> +			 * the lookup
> +			 */
> +			BUG_ON(!fb);
> +		} else {
> +			fb = NULL;
> +		}
> +	} else {
> +		fb = crtc->cursor->fb;

You need to (temporarily) grab the crtc->mutex lock here to read
crtc->cursor->fb and acquire the reference. Otherwise racing userspace
might remove the fb while this function runs. This locking is only
required in the else branch here.

> +	}
> +
> +	planereq.plane_id = crtc->cursor->base.id;
> +	planereq.crtc_id = crtc->base.id;
> +	planereq.fb_id = fb ? fb->base.id : 0;
> +
> +	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> +		planereq.crtc_x = req->x;
> +		planereq.crtc_y = req->y;
> +	} else {
> +		planereq.crtc_x = crtc->cursor_x;
> +		planereq.crtc_y = crtc->cursor_y;
> +	}
> +
> +	if (fb) {
> +		planereq.crtc_w = fb->width;
> +		planereq.crtc_h = fb->height;
> +		planereq.src_w = fb->width << 16;
> +		planereq.src_h = fb->height << 16;
> +	}
> +

We might want to store the new hot_x/y positions in the crtc too before
calling setplane. Otoh that can wait until we have a cursor plane enabled
driver that needs it.

> +	ret = drm_mode_setplane(dev, &planereq, file_priv);

I think a setplane_internal which directly takes the struct
drm_framebuffer *fb would be much better. We could extract that from the
setplane ioctl function if we first unify the !fb path with the normal
one.

> +	if (ret) {
> +		if (req->flags & DRM_MODE_CURSOR_BO)
> +			drm_mode_rmfb(dev, &fb->base.id, file_priv);
> +		return ret;
> +	}
> +
> +	/* Update successful; save new cursor position, if necessary */
> +	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> +		crtc->cursor_x = req->x;
> +		crtc->cursor_y = req->y;
> +	}
> +
> +	return 0;
> +}
> +
>  static int drm_mode_cursor_common(struct drm_device *dev,
>  				  struct drm_mode_cursor2 *req,
>  				  struct drm_file *file_priv)
> @@ -2497,6 +2598,13 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  	}
>  	crtc = obj_to_crtc(obj);
>  
> +	/*
> +	 * 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);
> +
>  	mutex_lock(&crtc->mutex);
>  	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 c061bb3..e5d22ff 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -311,6 +311,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)

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

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.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 108 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |   4 ++
 2 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 201c317..9f6f93c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -40,6 +40,10 @@
 
 #include "drm_crtc_internal.h"
 
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+							void *data,
+							struct drm_file *file_priv);
+
 /**
  * drm_modeset_lock_all - take all modeset locks
  * @dev: drm device
@@ -2498,6 +2502,103 @@ 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;
+	}
+
+	ret = setplane_internal(crtc, crtc->cursor, fb,
+				crtc_x, crtc_y, crtc_w, crtc_h,
+				0, 0, src_w, src_h);
+	if (ret) {
+		if (req->flags & DRM_MODE_CURSOR_BO)
+			drm_mode_rmfb(dev, &fb->base.id, file_priv);
+		return ret;
+	}
+
+	/* Update successful; save new cursor position, if necessary */
+	if (req->flags & DRM_MODE_CURSOR_MOVE) {
+		crtc->cursor_x = req->x;
+		crtc->cursor_y = req->y;
+	}
+
+	return 0;
+}
+
 static int drm_mode_cursor_common(struct drm_device *dev,
 				  struct drm_mode_cursor2 *req,
 				  struct drm_file *file_priv)
@@ -2519,6 +2620,13 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	}
 	crtc = obj_to_crtc(obj);
 
+	/*
+	 * 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);
+
 	mutex_lock(&crtc->mutex);
 	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 c061bb3..e5d22ff 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -311,6 +311,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

Re: [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)

[Daniel Vetter]

On Sat, May 17, 2014 at 12:38 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> +       if (ret) {
> +               if (req->flags & DRM_MODE_CURSOR_BO)
> +                       drm_mode_rmfb(dev, &fb->base.id, file_priv);
> +               return ret;
> +       }

With the new refcount logic an unconditional
drm_framebuffer_unreference should be here instead of this. I think,
but please double-check since it's late here ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)

[Daniel Vetter]

I didn't spot anything else, so with the below issue addressed this patch
is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On Sat, May 17, 2014 at 12:43:04AM +0200, Daniel Vetter wrote:
> On Sat, May 17, 2014 at 12:38 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > +       if (ret) {
> > +               if (req->flags & DRM_MODE_CURSOR_BO)
> > +                       drm_mode_rmfb(dev, &fb->base.id, file_priv);
> > +               return ret;
> > +       }
> 
> With the new refcount logic an unconditional
> drm_framebuffer_unreference should be here instead of this. I think,
> but please double-check since it's late here ;-)
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)

[Matt Roper]

On Sat, May 17, 2014 at 12:43:04AM +0200, Daniel Vetter wrote:
> On Sat, May 17, 2014 at 12:38 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > +       if (ret) {
> > +               if (req->flags & DRM_MODE_CURSOR_BO)
> > +                       drm_mode_rmfb(dev, &fb->base.id, file_priv);
> > +               return ret;
> > +       }
> 
> With the new refcount logic an unconditional
> drm_framebuffer_unreference should be here instead of this. I think,
> but please double-check since it's late here ;-)
> -Daniel

Actually, I'm not sure if we want the unreference at all.
setplane_internal() unrefs fb on failure (and old_fb on success), so I
think that's already taken care of.


Matt

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

Re: [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)

[Daniel Vetter]

On Mon, May 19, 2014 at 03:25:45PM -0700, Matt Roper wrote:
> On Sat, May 17, 2014 at 12:43:04AM +0200, Daniel Vetter wrote:
> > On Sat, May 17, 2014 at 12:38 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > +       if (ret) {
> > > +               if (req->flags & DRM_MODE_CURSOR_BO)
> > > +                       drm_mode_rmfb(dev, &fb->base.id, file_priv);
> > > +               return ret;
> > > +       }
> > 
> > With the new refcount logic an unconditional
> > drm_framebuffer_unreference should be here instead of this. I think,
> > but please double-check since it's late here ;-)
> > -Daniel
> 
> Actually, I'm not sure if we want the unreference at all.
> setplane_internal() unrefs fb on failure (and old_fb on success), so I
> think that's already taken care of.

Oh right, I've forgotten about that part. A one-line comment here that
setplane_internal will eat the reference would be good - you already have
a corresponding comment around setplane_internal but I guess that's not
good enough for silly me ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC 2/4] 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.

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 3afdc45..036acb2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -707,7 +707,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
  */
 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)
 {
 	int ret;
@@ -730,8 +730,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	dev->mode_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 e5d22ff..ede384a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -834,7 +834,7 @@ extern void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
 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

Re: [Intel-gfx] [RFC 2/4] drm: Allow drivers to register cursor planes with crtc

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:27PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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 3afdc45..036acb2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -707,7 +707,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
>   */
>  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)
>  {
>  	int ret;
> @@ -730,8 +730,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	dev->mode_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 e5d22ff..ede384a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -834,7 +834,7 @@ extern void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
>  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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC 3/4] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer

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

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f196e..7e75b13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8015,21 +8015,30 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	}
 }
 
-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
+ * @crtc: crtc to set cursor for
+ * @obj: GEM object to set cursor to
+ * @width: cursor width
+ * @height: cursor height
+ *
+ * Programs a crtc's cursor plane with the specified GEM object.
+ * intel_crtc_cursor_set() should be used instead if we have a userspace buffer
+ * handle rather than the actual GEM object.
+ */
+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;
@@ -8045,12 +8054,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;
 	}
@@ -8138,6 +8143,35 @@ fail:
 	return ret;
 }
 
+/**
+ * intel_crtc_cursor_set - Update cursor buffer
+ * @crtc: crtc to set cursor for
+ * @handle: userspace handle of buffer to set cursor to
+ * @width: cursor width
+ * @height: cursor height
+ *
+ * Programs a crtc's cursor plane with the buffer referred to by userspace
+ * handle @handle.
+ */
+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

Re: [RFC 3/4] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:28PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9f196e..7e75b13 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8015,21 +8015,30 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	}
>  }
>  
> -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
> + * @crtc: crtc to set cursor for
> + * @obj: GEM object to set cursor to
> + * @width: cursor width
> + * @height: cursor height
> + *
> + * Programs a crtc's cursor plane with the specified GEM object.
> + * intel_crtc_cursor_set() should be used instead if we have a userspace buffer
> + * handle rather than the actual GEM object.
> + */

Imo doing kerneldoc for simple static functions doesn't add too much
value. The interesting stuff tends to be functions used all over the place
(and hence exported to files) and used to implement core driver
infrastructure.

Comments always have a cost attached since we need to keep them
up-to-date, and my impression here is that these are just stating the
obvious. Most of the kerneldoc for shared functions is also pretty much
stating the obvious, but it helps to clarify the exact semantics of a
function. Which is much more important for something shared than something
used in just one 1 file at 2 places.

If there's something tricky it's much better to have a comment right next
to the tricky code in-line, with no other fluff to distract people.

Here imo the important bit is that cursor_set will eat the reference for
the passed-in obj, for both the success and failure case.

With that addressed the patch is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +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;
> @@ -8045,12 +8054,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;
>  	}
> @@ -8138,6 +8143,35 @@ fail:
>  	return ret;
>  }
>  
> +/**
> + * intel_crtc_cursor_set - Update cursor buffer
> + * @crtc: crtc to set cursor for
> + * @handle: userspace handle of buffer to set cursor to
> + * @width: cursor width
> + * @height: cursor height
> + *
> + * Programs a crtc's cursor plane with the buffer referred to by userspace
> + * handle @handle.
> + */
> +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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/4] 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 e9f196e..11c9493 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8015,21 +8015,26 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	}
 }
 
-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;
@@ -8045,12 +8050,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;
 	}
@@ -8138,6 +8139,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

[RFC 4/4] drm/i915: Switch to unified plane cursor handling

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

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 194 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |   1 -
 2 files changed, 136 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e75b13..d145130 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; })
 
@@ -7967,8 +7972,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;
 	bool visible;
 
@@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
  * @height: cursor height
  *
  * Programs a crtc's cursor plane with the specified GEM object.
- * intel_crtc_cursor_set() should be used instead if we have a userspace buffer
- * handle rather than the actual GEM object.
  */
 static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     struct drm_i915_gem_object *obj,
@@ -8143,48 +8146,6 @@ fail:
 	return ret;
 }
 
-/**
- * intel_crtc_cursor_set - Update cursor buffer
- * @crtc: crtc to set cursor for
- * @handle: userspace handle of buffer to set cursor to
- * @width: cursor width
- * @height: cursor height
- *
- * Programs a crtc's cursor plane with the buffer referred to by userspace
- * handle @handle.
- */
-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)
 {
@@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 		kfree(work);
 	}
 
-	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
+	intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);
 
 	drm_crtc_cleanup(crtc);
 
@@ -10784,8 +10745,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,
@@ -11018,7 +10977,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);
@@ -11028,7 +10988,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,
@@ -11064,11 +11024,116 @@ 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 drm_device *dev = crtc->dev;
+	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,
+	};
+	int hscale, vscale;
+	bool visible;
+
+	/* Check scaling */
+	hscale = drm_rect_calc_hscale(&src, &dest,
+				      DRM_PLANE_HELPER_NO_SCALING,
+				      DRM_PLANE_HELPER_NO_SCALING);
+	vscale = drm_rect_calc_vscale(&src, &dest,
+				      DRM_PLANE_HELPER_NO_SCALING,
+				      DRM_PLANE_HELPER_NO_SCALING);
+	if (hscale < 0 || vscale < 0) {
+		DRM_DEBUG_KMS("Invalid scaling of cursor plane\n");
+		return -ERANGE;
+	}
+
+	/* Check dimensions */
+	if (!((crtc_w == 64 && crtc_h == 64) ||
+			(crtc_w == 128 && crtc_h == 128 && !IS_GEN2(dev)) ||
+			(crtc_w == 256 && crtc_h == 256 && !IS_GEN2(dev)))) {
+		DRM_DEBUG_KMS("Cursor dimension not supported: %dx%d\n",
+			      crtc_w, crtc_h);
+		return -EINVAL;
+	}
+
+	/* Clip to display; if no longer visible after clipping, disable */
+	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
+
+	crtc->cursor_x = crtc_x;
+	crtc->cursor_y = crtc_y;
+	if (fb != crtc->cursor->fb) {
+		return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
+						 crtc_w, crtc_h);
+	} else {
+		intel_crtc_update_cursor(crtc, true);
+		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_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);
@@ -11076,13 +11141,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++) {
@@ -11110,6 +11179,15 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	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 32a74e1..8374f37a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -378,7 +378,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;
 	bool cursor_visible;
 
-- 
1.8.5.1

Re: [RFC 4/4] drm/i915: Switch to unified plane cursor handling

[Chris Wilson]

On Thu, May 15, 2014 at 06:17:29PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> +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 drm_device *dev = crtc->dev;
> +	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,
> +	};
> +	int hscale, vscale;
> +	bool visible;
> +
> +	/* Check scaling */
> +	hscale = drm_rect_calc_hscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	vscale = drm_rect_calc_vscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	if (hscale < 0 || vscale < 0) {
> +		DRM_DEBUG_KMS("Invalid scaling of cursor plane\n");
> +		return -ERANGE;
> +	}
> +
> +	/* Check dimensions */
> +	if (!((crtc_w == 64 && crtc_h == 64) ||
> +			(crtc_w == 128 && crtc_h == 128 && !IS_GEN2(dev)) ||
> +			(crtc_w == 256 && crtc_h == 256 && !IS_GEN2(dev)))) {
> +		DRM_DEBUG_KMS("Cursor dimension not supported: %dx%d\n",
> +			      crtc_w, crtc_h);
> +		return -EINVAL;
> +	}
> +
> +	/* Clip to display; if no longer visible after clipping, disable */
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +
> +	crtc->cursor_x = crtc_x;
> +	crtc->cursor_y = crtc_y;
> +	if (fb != crtc->cursor->fb) {
> +		return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
> +						 crtc_w, crtc_h);
> +	} else {
> +		intel_crtc_update_cursor(crtc, true);
> +		return 0;
> +	}

Does this do the right thing for a cursor that is no longer visible, and
vice versa? It is not immediately clear how clipping now works with
intel_crtc_update_cursor().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC 4/4] drm/i915: Switch to unified plane cursor handling

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:29PM -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.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Bunch of comments below. I think for testing we have good coverage of
functional cursor tests already, and with the backwards-compat code in the
drm core we'll excerise the cursor plane code sufficiently imo.

But there's a bit of corner-case checking:
- Interactions between legacy cursor ioctl and the new stuff to check the
  refcounting and correctness wrt set_bo and visible and fun like that.
- Maybe some checks for the no-upscaling and no-clipping stuff. Hopefully
  you can easily copy&paste this from the primary plane tests.

But in the end you're worked on this and are the expert, so if you
think a different focus gives a better payoff then I'm all ears - we want
to write tests that have a good chance of catching real bugs after all ;-)

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 194 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  2 files changed, 136 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e75b13..d145130 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; })
>  
> @@ -7967,8 +7972,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;
>  	bool visible;
>  
> @@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>   * @height: cursor height
>   *
>   * Programs a crtc's cursor plane with the specified GEM object.
> - * intel_crtc_cursor_set() should be used instead if we have a userspace buffer
> - * handle rather than the actual GEM object.
>   */
>  static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  				     struct drm_i915_gem_object *obj,
> @@ -8143,48 +8146,6 @@ fail:
>  	return ret;
>  }
>  
> -/**
> - * intel_crtc_cursor_set - Update cursor buffer
> - * @crtc: crtc to set cursor for
> - * @handle: userspace handle of buffer to set cursor to
> - * @width: cursor width
> - * @height: cursor height
> - *
> - * Programs a crtc's cursor plane with the buffer referred to by userspace
> - * handle @handle.
> - */
> -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)
>  {
> @@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> -	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
> +	intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);

This shouldn't be needed any more with the plane cleanup code - all the
refcounting is now done with fbs.
>  
>  	drm_crtc_cleanup(crtc);
>  
> @@ -10784,8 +10745,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,
> @@ -11018,7 +10977,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)

Actually a review comment for your primary plane series: We have this
function as intel_destroy_plane in intel_sprite.c already ;-)

>  {
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	drm_plane_cleanup(plane);
> @@ -11028,7 +10988,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,
> @@ -11064,11 +11024,116 @@ 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 drm_device *dev = crtc->dev;
> +	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,
> +	};
> +	int hscale, vscale;
> +	bool visible;
> +
> +	/* Check scaling */
> +	hscale = drm_rect_calc_hscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	vscale = drm_rect_calc_vscale(&src, &dest,
> +				      DRM_PLANE_HELPER_NO_SCALING,
> +				      DRM_PLANE_HELPER_NO_SCALING);
> +	if (hscale < 0 || vscale < 0) {
> +		DRM_DEBUG_KMS("Invalid scaling of cursor plane\n");
> +		return -ERANGE;
> +	}

Can't we reuse the check helpers for primary planes here? We again want to
scaling and no additional clipping (besides the viewport clipping).
> +
> +	/* Check dimensions */
> +	if (!((crtc_w == 64 && crtc_h == 64) ||
> +			(crtc_w == 128 && crtc_h == 128 && !IS_GEN2(dev)) ||
> +			(crtc_w == 256 && crtc_h == 256 && !IS_GEN2(dev)))) {
> +		DRM_DEBUG_KMS("Cursor dimension not supported: %dx%d\n",
> +			      crtc_w, crtc_h);
> +		return -EINVAL;
> +	}

Isn't this check redudnant with the one in cursor_set_obj?

> +
> +	/* Clip to display; if no longer visible after clipping, disable */
> +	visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +
> +	crtc->cursor_x = crtc_x;
> +	crtc->cursor_y = crtc_y;
> +	if (fb != crtc->cursor->fb) {
> +		return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
> +						 crtc_w, crtc_h);
> +	} else {
> +		intel_crtc_update_cursor(crtc, true);
> +		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_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);
> @@ -11076,13 +11141,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++) {
> @@ -11110,6 +11179,15 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> +
> +	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 32a74e1..8374f37a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -378,7 +378,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;
>  	bool cursor_visible;
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 0/4] Cursor support with universal planes

[Daniel Vetter]

On Thu, May 15, 2014 at 06:17:25PM -0700, Matt Roper wrote:
> Cursor planes are a bit trickier to support via the universal plane interface
> than primary planes were.  Legacy cursor ioctls take handles to driver buffers
> directly whereas the universal plane API takes drm_framebuffer's that represent
> a buffer; due to this mismatch it isn't possible to implement a set of cursor
> helpers than provide "free" generic universal cursor support without driver
> changes as we did with primary planes.  Drivers will need to be updated to
> receive cursor support via the universal plane API.
> 
> If a driver tries to implement two interfaces for cursor support (legacy ioctl
> and universal planes), the reference counting can get very tricky/messy since
> we need to take into account userspace that may mix and match calls to both
> interfaces.  To address that, patch #1 in this set causes legacy cursor ioctls
> to be implemented using a driver's universal plane support if the driver
> registers a primary plane.  Calls to the legacy set_cursor ioctl will wrap the
              ^cursor plane I guess?

-Daniel

> provided driver buffer in a drm_framebuffer and then pass that along to the
> universal plane interface.  From a driver's point of view, this causes all
> cursor operations to arrive on the universal plane interface, regardless of
> which userspace API was used, which simplifies things greatly for the driver.
> 
> Patch #2 makes some minor changes to ensure drivers can successfully register a
> cursor plane with the DRM core.
> 
> Patch #3 does some minor i915 refactoring in preparation for the move to
> universal planes.
> 
> Patch #4 transitions the i915 driver to universal planes.  The changes here are
> intentionally minimal for ease of review.  It's likely that we could perform
> further cleanup in the future to eliminate some of the cursor state tracked in
> intel_crtc (e.g., cursor_width/cursor_height) since that information can be
> also be derived from crtc->cursor->fb.
> 
> 
> Matt Roper (4):
>   drm: Support legacy cursor ioctls via universal planes when possible
>   drm: Allow drivers to register cursor planes with crtc
>   drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer
>   drm/i915: Switch to unified plane cursor handling
> 
>  drivers/gpu/drm/drm_crtc.c           | 113 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c | 188 ++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  include/drm/drm_crtc.h               |   6 +-
>  4 files changed, 267 insertions(+), 41 deletions(-)
> 
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch