On Tue, Jul 29, 2014 at 11:32:19PM +0200, Daniel Vetter wrote:
> Atomic implemenations for legacy ioctls must be able to drop locks.
> Which doesn't cause havoc since we only do that while constructing
> the new state, so no driver or hardware state change has happened.
>
> The only troubling bit is the fb refcounting the core does - if
> someone else has snuck in then it might potentially unref an
> outdated framebuffer. To fix that move the old_fb temporary storage
> into struct drm_plane for all ioctls, so that the atomic helpers can
> update it.
>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> drivers/gpu/drm/drm_crtc.c | 40 ++++++++++++++++++++++++----------------
> include/drm/drm_crtc.h | 8 ++++----
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c09374038f9a..bacf565449d5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1200,19 +1200,21 @@ EXPORT_SYMBOL(drm_plane_index);
> */
> void drm_plane_force_disable(struct drm_plane *plane)
> {
> - struct drm_framebuffer *old_fb = plane->fb;
> int ret;
>
> - if (!old_fb)
> + if (!plane->fb)
> return;
>
> + plane->old_fb = plane->fb;
> ret = plane->funcs->disable_plane(plane);
> if (ret) {
> DRM_ERROR("failed to disable plane with busy fb\n");
> + plane->old_fb = NULL;
> return;
> }
> /* disconnect the plane from the fb and crtc: */
> - __drm_framebuffer_unreference(old_fb);
> + __drm_framebuffer_unreference(plane->old_fb);
> + plane->old_fb = NULL;
> plane->fb = NULL;
> plane->crtc = NULL;
> }
> @@ -2188,7 +2190,7 @@ static int setplane_internal(struct drm_plane *plane,
> uint32_t src_w, uint32_t src_h)
> {
> struct drm_device *dev = plane->dev;
> - struct drm_framebuffer *old_fb = NULL;
> + struct drm_framebuffer *old_fb;
I think there may be cases where old_fb gets unref'd without ever being
set if we drop the NULL assignment. E.g., if the possible_crtcs test or
the format test fail, we jump down to out and then test the value +
unref which could be garbage.
Would it be simpler to just drm_modeset_lock_all() unconditionally at
the start of the function and then just unlock after the unrefs at the
end of the function so that we don't need a local old_fb?
> int ret = 0;
> unsigned int fb_width, fb_height;
> int i;
> @@ -2196,14 +2198,16 @@ static int setplane_internal(struct drm_plane *plane,
> /* No fb means shut it down */
> if (!fb) {
> drm_modeset_lock_all(dev);
> - old_fb = plane->fb;
> + plane->old_fb = plane->fb;
> ret = plane->funcs->disable_plane(plane);
> if (!ret) {
> plane->crtc = NULL;
> plane->fb = NULL;
> } else {
> - old_fb = NULL;
> + plane->old_fb = NULL;
> }
> + old_fb = plane->old_fb;
> + plane->old_fb = NULL;
> drm_modeset_unlock_all(dev);
> goto out;
> }
> @@ -2245,7 +2249,7 @@ static int setplane_internal(struct drm_plane *plane,
> }
>
> drm_modeset_lock_all(dev);
> - old_fb = plane->fb;
> + 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);
> @@ -2254,8 +2258,10 @@ static int setplane_internal(struct drm_plane *plane,
> plane->fb = fb;
> fb = NULL;
> } else {
> - old_fb = NULL;
> + plane->old_fb = NULL;
> }
> + old_fb = plane->old_fb;
> + plane->old_fb = NULL;
> drm_modeset_unlock_all(dev);
>
> out:
> @@ -2369,7 +2375,7 @@ int drm_mode_set_config_internal(struct drm_mode_set
> *set)
> * crtcs. Atomic modeset will have saner semantics ...
> */
> list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head)
> - tmp->old_fb = tmp->primary->fb;
> + tmp->primary->old_fb = tmp->primary->fb;
>
> fb = set->fb;
>
> @@ -2382,8 +2388,9 @@ int drm_mode_set_config_internal(struct drm_mode_set
> *set)
> list_for_each_entry(tmp, &crtc->dev->mode_config.crtc_list, head) {
> if (tmp->primary->fb)
> drm_framebuffer_reference(tmp->primary->fb);
> - if (tmp->old_fb)
> - drm_framebuffer_unreference(tmp->old_fb);
> + if (tmp->primary->old_fb)
> + drm_framebuffer_unreference(tmp->primary->old_fb);
> + tmp->primary->old_fb = NULL;
> }
>
> return ret;
> @@ -4458,7 +4465,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> {
> struct drm_mode_crtc_page_flip *page_flip = data;
> struct drm_crtc *crtc;
> - struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> + struct drm_framebuffer *fb = NULL;
> struct drm_pending_vblank_event *e = NULL;
> unsigned long flags;
> int ret = -EINVAL;
> @@ -4530,7 +4537,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> (void (*) (struct drm_pending_event *)) kfree;
> }
>
> - old_fb = crtc->primary->fb;
> + crtc->primary->old_fb = crtc->primary->fb;
> ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> if (ret) {
> if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> @@ -4540,7 +4547,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> kfree(e);
> }
> /* Keep the old fb, don't unref it. */
> - old_fb = NULL;
> + crtc->primary->old_fb = NULL;
> } else {
> /*
> * Warn if the driver hasn't properly updated the crtc->fb
> @@ -4556,8 +4563,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> out:
> if (fb)
> drm_framebuffer_unreference(fb);
> - if (old_fb)
> - drm_framebuffer_unreference(old_fb);
> + if (crtc->primary->old_fb)
> + drm_framebuffer_unreference(crtc->primary->old_fb);
> + crtc->primary->old_fb = NULL;
> drm_modeset_unlock_crtc(crtc);
>
> return ret;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b0e30c5526ce..5fffb5c53ba6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -341,10 +341,6 @@ struct drm_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;
> -
> bool enabled;
>
> /* Requested mode from modesetting. */
> @@ -622,6 +618,10 @@ struct drm_plane {
> struct drm_crtc *crtc;
> struct drm_framebuffer *fb;
>
> + /* Temporary tracking of the old fb while a modeset is ongoing. Used
> + * by drm_mode_set_config_internal to implement correct refcounting. */
Might want to update the wording of this comment slightly since it isn't
just for drm_mode_set_config_internal (or modesets) anymore.
Matt
> + struct drm_framebuffer *old_fb;
> +
> const struct drm_plane_funcs *funcs;
>
> struct drm_object_properties properties;
> --
> 2.0.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
