diff mbox

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

Message ID 1400279896-26112-1-git-send-email-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matt Roper May 16, 2014, 10:38 p.m. UTC
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(+)

Comments

Daniel Vetter May 16, 2014, 10:43 p.m. UTC | #1
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 May 19, 2014, 7:28 a.m. UTC | #2
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
Matt Roper May 19, 2014, 10:25 p.m. UTC | #3
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
Daniel Vetter May 20, 2014, 7:24 a.m. UTC | #4
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
diff mbox

Patch

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;