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

Daniel Vetter daniel at ffwll.ch
Fri May 16 18:00:36 CEST 2014


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



More information about the Intel-gfx mailing list