[PATCH 1/2] drm/atomic: Update old_fb after setting a property.

[PATCH 1/2] drm/atomic: Update old_fb after setting a property.

[Maarten Lankhorst]

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>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/atomic: Cleanup on error properly in the atomic ioctl.

[Maarten Lankhorst]

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;
 }
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/atomic: Cleanup on error properly in the atomic ioctl.

[Daniel Vetter]

On Wed, Jun 24, 2015 at 08:59:25AM +0200, Maarten Lankhorst wrote:
> 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>

Grumpy reviewer comment: You've failed to mention that you rework a few
things just because, makes it harder to spot the actual changes. Anyway,
applied to drm-misc.
-Daniel

> ---
>  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;
>  }
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/atomic: Update old_fb after setting a property.

[Rob Clark]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel