From: Ville Syrjälä ville.syrjala@linux.intel.com
No reason that I can see to delay the fb lookup this late. Moving it earlier allows us to keep it outside of the lock retry loop. This makes error handling and whatnot simpler.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_plane.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 6153cbda239f..924830071307 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1009,7 +1009,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_mode_crtc_page_flip_target *page_flip = data; struct drm_crtc *crtc; struct drm_plane *plane; - struct drm_framebuffer *fb = NULL, *old_fb; + struct drm_framebuffer *fb, *old_fb; struct drm_pending_vblank_event *e = NULL; u32 target_vblank = page_flip->sequence; struct drm_modeset_acquire_ctx ctx; @@ -1055,16 +1055,16 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, DRM_DEBUG("Invalid absolute flip target %u, " "must be <= %u\n", target_vblank, current_vblank + 1); - drm_crtc_vblank_put(crtc); - return -EINVAL; + ret = -EINVAL; + goto put_vblank; } break; case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE: if (target_vblank != 0 && target_vblank != 1) { DRM_DEBUG("Invalid relative flip target %u, " "must be 0 or 1\n", target_vblank); - drm_crtc_vblank_put(crtc); - return -EINVAL; + ret = -EINVAL; + goto put_vblank; } target_vblank += current_vblank; break; @@ -1075,7 +1075,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, } } else if (crtc->funcs->page_flip == NULL || (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) { - return -EINVAL; + ret = -EINVAL; + goto put_vblank; + } + + fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id); + if (!fb) { + ret = -ENOENT; + goto put_vblank; }
drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); @@ -1101,12 +1108,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; }
- fb = drm_framebuffer_lookup(dev, file_priv, page_flip->fb_id); - if (!fb) { - ret = -ENOENT; - goto out; - } - if (plane->state) { const struct drm_plane_state *state = plane->state;
@@ -1170,8 +1171,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, }
out: - if (fb) - drm_framebuffer_put(fb); if (plane->old_fb) drm_framebuffer_put(plane->old_fb); plane->old_fb = NULL; @@ -1185,6 +1184,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);
+ drm_framebuffer_put(fb); +put_vblank: if (ret && crtc->funcs->page_flip_target) drm_crtc_vblank_put(crtc);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Can't see why we need to delay the page flip event allocation until the last moment. Move it earlier to simplify error handling.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_plane.c | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 924830071307..d269231d63c5 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1085,6 +1085,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto put_vblank; }
+ if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { + e = kzalloc(sizeof *e, GFP_KERNEL); + if (!e) { + ret = -ENOMEM; + goto put_fb; + } + + e->event.base.type = DRM_EVENT_FLIP_COMPLETE; + e->event.base.length = sizeof(e->event); + e->event.vbl.user_data = page_flip->user_data; + e->event.vbl.crtc_id = crtc->base.id; + + ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base); + if (ret) { + kfree(e); + e = NULL; + goto put_fb; + } + } + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); retry: ret = drm_modeset_lock(&crtc->mutex, &ctx); @@ -1129,26 +1149,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; }
- if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { - e = kzalloc(sizeof *e, GFP_KERNEL); - if (!e) { - ret = -ENOMEM; - goto out; - } - - e->event.base.type = DRM_EVENT_FLIP_COMPLETE; - e->event.base.length = sizeof(e->event); - e->event.vbl.user_data = page_flip->user_data; - e->event.vbl.crtc_id = crtc->base.id; - - ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base); - if (ret) { - kfree(e); - e = NULL; - goto out; - } - } - plane->old_fb = plane->fb; if (crtc->funcs->page_flip_target) ret = crtc->funcs->page_flip_target(crtc, fb, e, @@ -1159,8 +1159,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags, &ctx); if (ret) { - if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) - drm_event_cancel_free(dev, &e->base); /* Keep the old fb, don't unref it. */ plane->old_fb = NULL; } else { @@ -1184,6 +1182,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);
+ if (ret && e) + drm_event_cancel_free(dev, &e->base); +put_fb: drm_framebuffer_put(fb); put_vblank: if (ret && crtc->funcs->page_flip_target)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Yank out the code for the plane->fb/old_fb/crtc handling from the page flip path into page_flip_internal(), and provide a simpler variant for atomic drivers.
We'll also move the fb vs. src viewport checks into the new functions as they are slightly different between the two paths. If the atomic .page_flip() implementations are guaranteed to call drm_atomic_plane_check() we could even drop the check entirely from page_flip_atomic(). For now toss in a FIXME.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_plane.c | 162 ++++++++++++++++++++++++++++---------------- 1 file changed, 105 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d269231d63c5..e3f2ddf7deae 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1003,13 +1003,112 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, return drm_mode_cursor_common(dev, req, file_priv); }
+static int page_flip_check_fbs(const struct drm_framebuffer *fb, + const struct drm_framebuffer *old_fb) +{ + /* The framebuffer is currently unbound, presumably + * due to a hotplug event, that userspace has not + * yet discovered. + */ + if (!old_fb) + return -EBUSY; + + if (old_fb->format != fb->format) { + DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n"); + return -EINVAL; + } + + return 0; +} + +static int page_flip_internal(struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_pending_vblank_event *e, + u32 flags, + u32 target_vblank, + struct drm_modeset_acquire_ctx *ctx) +{ + struct drm_plane *plane = crtc->primary; + int ret; + + WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); + + ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, + &crtc->mode, fb); + if (ret) + return ret; + + ret = page_flip_check_fbs(fb, plane->fb); + if (ret) + return ret; + + plane->old_fb = plane->fb; + + if (crtc->funcs->page_flip_target) + ret = crtc->funcs->page_flip_target(crtc, fb, e, flags, + target_vblank, ctx); + else + ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx); + + if (ret) { + /* Keep the old fb, don't unref it. */ + plane->old_fb = NULL; + } else { + plane->fb = fb; + drm_framebuffer_get(plane->fb); + } + + if (plane->old_fb) + drm_framebuffer_put(plane->old_fb); + plane->old_fb = NULL; + + return ret; +} + +static int page_flip_atomic(struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_pending_vblank_event *e, + u32 flags, + u32 target_vblank, + struct drm_modeset_acquire_ctx *ctx) +{ + struct drm_plane *plane = crtc->primary; + struct drm_plane_state *plane_state = plane->state; + int ret; + + WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev)); + + /* + * FIXME: Can we assume all drivers end up calling + * drm_atomic_plane_check() in their page flip paths? + * If so we could remove this. + */ + ret = drm_framebuffer_check_src_coords(plane_state->src_x, + plane_state->src_y, + plane_state->src_w, + plane_state->src_h, + fb); + if (ret) + return ret; + + ret = page_flip_check_fbs(fb, plane_state->fb); + if (ret) + return ret; + + if (crtc->funcs->page_flip_target) + return crtc->funcs->page_flip_target(crtc, fb, e, flags, + target_vblank, ctx); + else + return crtc->funcs->page_flip(crtc, fb, e, flags, ctx); +} + int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_crtc_page_flip_target *page_flip = data; struct drm_crtc *crtc; struct drm_plane *plane; - struct drm_framebuffer *fb, *old_fb; + struct drm_framebuffer *fb; struct drm_pending_vblank_event *e = NULL; u32 target_vblank = page_flip->sequence; struct drm_modeset_acquire_ctx ctx; @@ -1114,65 +1213,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (ret) goto out;
- if (plane->state) - old_fb = plane->state->fb; - else - old_fb = plane->fb; - - if (old_fb == NULL) { - /* The framebuffer is currently unbound, presumably - * due to a hotplug event, that userspace has not - * yet discovered. - */ - ret = -EBUSY; - goto out; - } - - if (plane->state) { - const struct drm_plane_state *state = plane->state; - - ret = drm_framebuffer_check_src_coords(state->src_x, - state->src_y, - state->src_w, - state->src_h, - fb); - } else { - ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, - &crtc->mode, fb); - } - if (ret) - goto out; - - if (old_fb->format != fb->format) { - DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n"); - ret = -EINVAL; - goto out; - } - - plane->old_fb = plane->fb; - if (crtc->funcs->page_flip_target) - ret = crtc->funcs->page_flip_target(crtc, fb, e, - page_flip->flags, - target_vblank, - &ctx); + if (drm_drv_uses_atomic_modeset(dev)) + ret = page_flip_atomic(crtc, fb, e, page_flip->flags, + target_vblank, &ctx); else - ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags, - &ctx); - if (ret) { - /* Keep the old fb, don't unref it. */ - plane->old_fb = NULL; - } else { - if (!plane->state) { - plane->fb = fb; - drm_framebuffer_get(fb); - } - } + ret = page_flip_internal(crtc, fb, e, page_flip->flags, + target_vblank, &ctx);
out: - if (plane->old_fb) - drm_framebuffer_put(plane->old_fb); - plane->old_fb = NULL; - if (ret == -EDEADLK) { ret = drm_modeset_backoff(&ctx); if (!ret)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Instead of doing the things in a convoluted way with the failure and success paths mixed up let's just clear old_fb when we encounter an error and bail out immediately.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_plane.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index e3f2ddf7deae..406f11281d61 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -655,12 +655,13 @@ static int __setplane_internal(struct drm_plane *plane, if (!fb) { plane->old_fb = plane->fb; ret = plane->funcs->disable_plane(plane, ctx); - if (!ret) { - plane->crtc = NULL; - plane->fb = NULL; - } else { + if (ret) { plane->old_fb = NULL; + return ret; } + + plane->crtc = NULL; + plane->fb = NULL; goto out; }
@@ -668,20 +669,22 @@ static int __setplane_internal(struct drm_plane *plane, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h); if (ret) - goto out; + return ret;
plane->old_fb = plane->fb; + ret = plane->funcs->update_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, ctx); - if (!ret) { - plane->crtc = crtc; - plane->fb = fb; - drm_framebuffer_get(plane->fb); - } else { + if (ret) { plane->old_fb = NULL; + return ret; }
+ plane->crtc = crtc; + plane->fb = fb; + drm_framebuffer_get(plane->fb); + out: if (plane->old_fb) drm_framebuffer_put(plane->old_fb); @@ -1049,15 +1052,14 @@ static int page_flip_internal(struct drm_crtc *crtc, target_vblank, ctx); else ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx); - if (ret) { - /* Keep the old fb, don't unref it. */ plane->old_fb = NULL; - } else { - plane->fb = fb; - drm_framebuffer_get(plane->fb); + return ret; }
+ plane->fb = fb; + drm_framebuffer_get(plane->fb); + if (plane->old_fb) drm_framebuffer_put(plane->old_fb); plane->old_fb = NULL;
dri-devel@lists.freedesktop.org