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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
