| Message ID | 1400203049-21385-2-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(+)