It's probably allowed to leave old_fb set to garbage when unlocking, but to prevent undefined behavior unset it just in case.

Also crtc_state->event could be NULL on memory allocation failure, in which case event_space is increased for no reason.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 63 ++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index bd7f723c708e..a89f73f1dca1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1463,18 +1463,18 @@ retry:

if (get_user(obj_id, objs_ptr + copied_objs)) { ret = -EFAULT; - goto fail; + goto out; }

obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY); if (!obj || !obj->properties) { ret = -ENOENT; - goto fail; + goto out; }

if (get_user(count_props, count_props_ptr + copied_objs)) { ret = -EFAULT; - goto fail; + goto out; }

copied_objs++; @@ -1486,25 +1486,25 @@ retry:

if (get_user(prop_id, props_ptr + copied_props)) { ret = -EFAULT; - goto fail; + goto out; }

prop = drm_property_find(dev, prop_id); if (!prop) { ret = -ENOENT; - goto fail; + goto out; }

if (copy_from_user(&prop_value, prop_values_ptr + copied_props, sizeof(prop_value))) { ret = -EFAULT; - goto fail; + goto out; }

ret = atomic_set_prop(state, obj, prop, prop_value); if (ret) - goto fail; + goto out;

copied_props++; } @@ -1523,7 +1523,7 @@ retry: e = create_vblank_event(dev, file_priv, arg->user_data); if (!e) { ret = -ENOMEM; - goto fail; + goto out; }

crtc_state->event = e; @@ -1533,13 +1533,15 @@ retry: if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { ret = drm_atomic_check_only(state); /* _check_only() does not free state, unlike _commit() */ - drm_atomic_state_free(state); + if (!ret) + drm_atomic_state_free(state); } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { ret = drm_atomic_async_commit(state); } else { ret = drm_atomic_commit(state); }

+out: /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping * locks (ie. while it is still safe to deref plane->state). We * need to do this here because the driver entry points cannot @@ -1552,41 +1554,34 @@ retry: drm_framebuffer_reference(new_fb); plane->fb = new_fb; plane->crtc = plane->state->crtc; - } else { - plane->old_fb = NULL; - } - if (plane->old_fb) { - drm_framebuffer_unreference(plane->old_fb); - plane->old_fb = NULL; + + if (plane->old_fb) + drm_framebuffer_unreference(plane->old_fb); } + plane->old_fb = NULL; }

- drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); - - return ret; + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + goto retry; + }

-fail: - if (ret == -EDEADLK) - goto backoff; + if (ret) { + if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (!crtc_state->event) + continue;

- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { - destroy_vblank_event(dev, file_priv, crtc_state->event); - crtc_state->event = NULL; + destroy_vblank_event(dev, file_priv, crtc_state->event); + } } - }

- drm_atomic_state_free(state); + drm_atomic_state_free(state); + }

drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);

return ret; - -backoff: - drm_atomic_state_clear(state); - drm_modeset_backoff(&ctx); - - goto retry; }

On Wed, Jun 24, 2015 at 2:59 AM, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:

This change updates the old_fb pointer only after acquiring the plane lock, if there are no properties the fb cannot have been changed either, so this works out correctly.

Found in a discussion with Rob Clark.

Cc: Rob Clark robdclark@gmail.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Reviewed-by: Rob Clark robdclark@gmail.com

---

drivers/gpu/drm/drm_atomic.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f6f2fb58eb37..bd7f723c708e 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1472,12 +1472,6 @@ retry: goto fail; }

•           if (obj->type == DRM_MODE_OBJECT_PLANE) {
  

•                   plane = obj_to_plane(obj);
  

•                   plane_mask |= (1 << drm_plane_index(plane));
  

•                   plane->old_fb = plane->fb;
  

•           }
  

•           if (get_user(count_props, count_props_ptr + copied_objs)) {
                    ret = -EFAULT;
                    goto fail;
  

@@ -1514,6 +1508,12 @@ retry:

                    copied_props++;
            }

•           if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
  

•                   plane = obj_to_plane(obj);
  

•                   plane_mask |= (1 << drm_plane_index(plane));
  

•                   plane->old_fb = plane->fb;
  

•           }
    }
  
    if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
  

-- 2.1.0