[PATCH 0/5] Atomic core fixes.

[PATCH 0/5] Atomic core fixes.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This small series fixes some problems in bookkeeping with old_fb.
The common code that's run after committing is factored out in a
small helper, and re-used by the atomic fb helpers. It should
be targeted for v4.4 because it's being mostly bugfixes for the
atomic fbdev helpers.

Maarten Lankhorst (5):
  drm/core: Set legacy_cursor_update in drm_atomic_helper_disable_plane.
  drm/core: Fix old_fb handling in drm_mode_atomic_ioctl.
  drm/atomic: add a drm_atomic_clean_old_fb helper.
  drm/core: Fix old_fb handling in restore_fbdev_mode_atomic.
  drm/core: Fix old_fb handling in pan_display_atomic.

 drivers/gpu/drm/drm_atomic.c        | 61 +++++++++++++++++++++++++------------
 drivers/gpu/drm/drm_atomic_helper.c |  7 +++--
 drivers/gpu/drm/drm_fb_helper.c     | 51 +++++++++----------------------
 include/drm/drm_atomic.h            |  3 ++
 4 files changed, 63 insertions(+), 59 deletions(-)

-- 
2.1.0

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

[PATCH 1/5] drm/core: Set legacy_cursor_update in drm_atomic_helper_disable_plane.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

legacy_cursor_update was being set in restore_fbdev_mode_atomic which was
probably unintended. Fix this by only setting it in the function that needs it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0c6f62168776..02d363ad35c9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1553,6 +1553,10 @@ retry:
 		goto fail;
 	}
 
+	if (plane_state->crtc && (plane == plane->crtc->cursor))
+		plane_state->state->legacy_cursor_update = true;
+
+
 	ret = __drm_atomic_helper_disable_plane(plane, plane_state);
 	if (ret != 0)
 		goto fail;
@@ -1605,9 +1609,6 @@ int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
 	plane_state->src_h = 0;
 	plane_state->src_w = 0;
 
-	if (plane->crtc && (plane == plane->crtc->cursor))
-		plane_state->state->legacy_cursor_update = true;
-
 	return 0;
 }
 
-- 
2.1.0

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

Re: [PATCH 1/5] drm/core: Set legacy_cursor_update in drm_atomic_helper_disable_plane.

[Daniel Vetter]

On Wed, Nov 11, 2015 at 11:29:07AM +0100, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> legacy_cursor_update was being set in restore_fbdev_mode_atomic which was
> probably unintended. Fix this by only setting it in the function that needs it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This oversight was introduced in

commit bbb1e52402b2a288b09ae37e8182599931c7e9df
Author: Rob Clark <robdclark@gmail.com>
Date:   Tue Aug 25 15:35:58 2015 -0400

    drm/fb-helper: atomic restore_fbdev_mode()...

With that added to the commit message:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0c6f62168776..02d363ad35c9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1553,6 +1553,10 @@ retry:
>  		goto fail;
>  	}
>  
> +	if (plane_state->crtc && (plane == plane->crtc->cursor))
> +		plane_state->state->legacy_cursor_update = true;
> +
> +
>  	ret = __drm_atomic_helper_disable_plane(plane, plane_state);
>  	if (ret != 0)
>  		goto fail;
> @@ -1605,9 +1609,6 @@ int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
>  	plane_state->src_h = 0;
>  	plane_state->src_w = 0;
>  
> -	if (plane->crtc && (plane == plane->crtc->cursor))
> -		plane_state->state->legacy_cursor_update = true;
> -
>  	return 0;
>  }
>  
> -- 
> 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

[PATCH 2/5] drm/core: Fix old_fb handling in drm_mode_atomic_ioctl.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

plane_mask should be cleared inside the retry loop,
because it gets reset on every retry.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: stable@vger.kernel.org #v4.3
---
 drivers/gpu/drm/drm_atomic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7bb3845d9974..0ac31b1ecb67 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1446,7 +1446,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	unsigned plane_mask = 0;
+	unsigned plane_mask;
 	int ret = 0;
 	unsigned int i, j;
 
@@ -1486,6 +1486,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
 
 retry:
+	plane_mask = 0;
 	copied_objs = 0;
 	copied_props = 0;
 
-- 
2.1.0

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

Re: [PATCH 2/5] drm/core: Fix old_fb handling in drm_mode_atomic_ioctl.

[Daniel Vetter]

On Wed, Nov 11, 2015 at 11:29:08AM +0100, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> plane_mask should be cleared inside the retry loop,
> because it gets reset on every retry.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org #v4.3

Nice catch, but a bit a terse commit message. We should add "Without this
fix the plane->fb refcounting might get out of sync on retries, resulting
in either leaked memory or use-after-free." With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7bb3845d9974..0ac31b1ecb67 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1446,7 +1446,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	unsigned plane_mask = 0;
> +	unsigned plane_mask;
>  	int ret = 0;
>  	unsigned int i, j;
>  
> @@ -1486,6 +1486,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
>  
>  retry:
> +	plane_mask = 0;
>  	copied_objs = 0;
>  	copied_props = 0;
>  
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/atomic: add a drm_atomic_clean_old_fb helper.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This is useful for all the boilerplate code about cleaning old_fb.
---
 drivers/gpu/drm/drm_atomic.c | 58 ++++++++++++++++++++++++++++++--------------
 include/drm/drm_atomic.h     |  3 +++
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 0ac31b1ecb67..aeee083c7f95 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1432,6 +1432,45 @@ static int atomic_set_prop(struct drm_atomic_state *state,
 	return ret;
 }
 
+/**
+ * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
+ *
+ * @dev: drm device to check.
+ * @plane_mask: plane mask for planes that were updated.
+ * @ret: return value, can be -EDEADLK for a retry.
+ *
+ * Before doing an update plane->old_fb is set to plane->fb,
+ * but before dropping the locks old_fb needs to be set to NULL
+ * and plane->fb updated. This is a common operation for each
+ * atomic update, so this call is split off as a helper.
+ */
+void drm_atomic_clean_old_fb(struct drm_device *dev,
+			     unsigned plane_mask,
+			     int ret)
+{
+	struct drm_plane *plane;
+
+	/* 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
+	 * distinguish between legacy and atomic ioctls.
+	 */
+	drm_for_each_plane_mask(plane, dev, plane_mask) {
+		if (ret == 0) {
+			struct drm_framebuffer *new_fb = plane->state->fb;
+			if (new_fb)
+				drm_framebuffer_reference(new_fb);
+			plane->fb = new_fb;
+			plane->crtc = plane->state->crtc;
+
+			if (plane->old_fb)
+				drm_framebuffer_unreference(plane->old_fb);
+		}
+		plane->old_fb = NULL;
+	}
+}
+EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
@@ -1577,24 +1616,7 @@ retry:
 	}
 
 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
-	 * distinguish between legacy and atomic ioctls.
-	 */
-	drm_for_each_plane_mask(plane, dev, plane_mask) {
-		if (ret == 0) {
-			struct drm_framebuffer *new_fb = plane->state->fb;
-			if (new_fb)
-				drm_framebuffer_reference(new_fb);
-			plane->fb = new_fb;
-			plane->crtc = plane->state->crtc;
-
-			if (plane->old_fb)
-				drm_framebuffer_unreference(plane->old_fb);
-		}
-		plane->old_fb = NULL;
-	}
+	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
 	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 		/*
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e67aeac2aee0..4b74c97d297a 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -136,6 +136,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
 
 void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
 
+void
+drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret);
+
 int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
 int __must_check drm_atomic_commit(struct drm_atomic_state *state);
 int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
-- 
2.1.0

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

Re: [PATCH 3/5] drm/atomic: add a drm_atomic_clean_old_fb helper.

[Daniel Vetter]

On Wed, Nov 11, 2015 at 11:29:09AM +0100, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> This is useful for all the boilerplate code about cleaning old_fb.
> ---
>  drivers/gpu/drm/drm_atomic.c | 58 ++++++++++++++++++++++++++++++--------------
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0ac31b1ecb67..aeee083c7f95 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1432,6 +1432,45 @@ static int atomic_set_prop(struct drm_atomic_state *state,
>  	return ret;
>  }
>  
> +/**
> + * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
> + *
> + * @dev: drm device to check.
> + * @plane_mask: plane mask for planes that were updated.
> + * @ret: return value, can be -EDEADLK for a retry.
> + *
> + * Before doing an update plane->old_fb is set to plane->fb,
> + * but before dropping the locks old_fb needs to be set to NULL
> + * and plane->fb updated. This is a common operation for each
> + * atomic update, so this call is split off as a helper.
> + */
> +void drm_atomic_clean_old_fb(struct drm_device *dev,
> +			     unsigned plane_mask,
> +			     int ret)

A drm_atomic_prepare_old_fb inline to encapsulate the prep step would have
been neat imo. Anyway: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +{
> +	struct drm_plane *plane;
> +
> +	/* 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
> +	 * distinguish between legacy and atomic ioctls.
> +	 */
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		if (ret == 0) {
> +			struct drm_framebuffer *new_fb = plane->state->fb;
> +			if (new_fb)
> +				drm_framebuffer_reference(new_fb);
> +			plane->fb = new_fb;
> +			plane->crtc = plane->state->crtc;
> +
> +			if (plane->old_fb)
> +				drm_framebuffer_unreference(plane->old_fb);
> +		}
> +		plane->old_fb = NULL;
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> @@ -1577,24 +1616,7 @@ retry:
>  	}
>  
>  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
> -	 * distinguish between legacy and atomic ioctls.
> -	 */
> -	drm_for_each_plane_mask(plane, dev, plane_mask) {
> -		if (ret == 0) {
> -			struct drm_framebuffer *new_fb = plane->state->fb;
> -			if (new_fb)
> -				drm_framebuffer_reference(new_fb);
> -			plane->fb = new_fb;
> -			plane->crtc = plane->state->crtc;
> -
> -			if (plane->old_fb)
> -				drm_framebuffer_unreference(plane->old_fb);
> -		}
> -		plane->old_fb = NULL;
> -	}
> +	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  
>  	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>  		/*
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e67aeac2aee0..4b74c97d297a 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -136,6 +136,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
>  
>  void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
>  
> +void
> +drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret);
> +
>  int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
>  int __must_check drm_atomic_commit(struct drm_atomic_state *state);
>  int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
> -- 
> 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 3/5] drm/atomic: add a drm_atomic_clean_old_fb helper.

[Maarten Lankhorst]

Op 11-11-15 om 11:29 schreef Maarten Lankhorst:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This is useful for all the boilerplate code about cleaning old_fb.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

[PATCH 4/5] drm/core: Fix old_fb handling in restore_fbdev_mode_atomic.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Don't touch plane->old_fb/fb without having the right locks held.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e673c13c7391..abd50863506e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -342,6 +342,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 	struct drm_plane *plane;
 	struct drm_atomic_state *state;
 	int i, ret;
+	unsigned plane_mask;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -349,11 +350,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 
 	state->acquire_ctx = dev->mode_config.acquire_ctx;
 retry:
+	plane_mask = 0;
 	drm_for_each_plane(plane, dev) {
 		struct drm_plane_state *plane_state;
 
-		plane->old_fb = plane->fb;
-
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
 			ret = PTR_ERR(plane_state);
@@ -362,6 +362,9 @@ retry:
 
 		plane_state->rotation = BIT(DRM_ROTATE_0);
 
+		plane->old_fb = plane->fb;
+		plane_mask |= 1 << drm_plane_index(plane);
+
 		/* disable non-primary: */
 		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
 			continue;
@@ -382,19 +385,7 @@ retry:
 	ret = drm_atomic_commit(state);
 
 fail:
-	drm_for_each_plane(plane, dev) {
-		if (ret == 0) {
-			struct drm_framebuffer *new_fb = plane->state->fb;
-			if (new_fb)
-				drm_framebuffer_reference(new_fb);
-			plane->fb = new_fb;
-			plane->crtc = plane->state->crtc;
-
-			if (plane->old_fb)
-				drm_framebuffer_unreference(plane->old_fb);
-		}
-		plane->old_fb = NULL;
-	}
+	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
 	if (ret == -EDEADLK)
 		goto backoff;
-- 
2.1.0

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

[PATCH 5/5] drm/core: Fix old_fb handling in pan_display_atomic.

[Maarten Lankhorst]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index abd50863506e..69cbab5e5c81 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1227,7 +1227,9 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_atomic_state *state;
+	struct drm_plane *plane;
 	int i, ret;
+	unsigned plane_mask;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -1235,19 +1237,22 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 
 	state->acquire_ctx = dev->mode_config.acquire_ctx;
 retry:
+	plane_mask = 0;
 	for(i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set;
 
 		mode_set = &fb_helper->crtc_info[i].mode_set;
 
-		mode_set->crtc->primary->old_fb = mode_set->crtc->primary->fb;
-
 		mode_set->x = var->xoffset;
 		mode_set->y = var->yoffset;
 
 		ret = __drm_atomic_helper_set_config(mode_set, state);
 		if (ret != 0)
 			goto fail;
+
+		plane = mode_set->crtc->primary;
+		plane_mask |= drm_plane_index(plane);
+		plane->old_fb = plane->fb;
 	}
 
 	ret = drm_atomic_commit(state);
@@ -1259,26 +1264,7 @@ retry:
 
 
 fail:
-	for(i = 0; i < fb_helper->crtc_count; i++) {
-		struct drm_mode_set *mode_set;
-		struct drm_plane *plane;
-
-		mode_set = &fb_helper->crtc_info[i].mode_set;
-		plane = mode_set->crtc->primary;
-
-		if (ret == 0) {
-			struct drm_framebuffer *new_fb = plane->state->fb;
-
-			if (new_fb)
-				drm_framebuffer_reference(new_fb);
-			plane->fb = new_fb;
-			plane->crtc = plane->state->crtc;
-
-			if (plane->old_fb)
-				drm_framebuffer_unreference(plane->old_fb);
-		}
-		plane->old_fb = NULL;
-	}
+	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
 	if (ret == -EDEADLK)
 		goto backoff;
-- 
2.1.0

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

Re: [PATCH 5/5] drm/core: Fix old_fb handling in pan_display_atomic.

[Daniel Vetter]

On Wed, Nov 11, 2015 at 11:29:11AM +0100, Maarten Lankhorst wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Needs "Don't touch plane->old_fb/fb without having the right locks held."
like the previous patch in the commit message. With that for patches 4&5:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index abd50863506e..69cbab5e5c81 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1227,7 +1227,9 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
>  	struct drm_fb_helper *fb_helper = info->par;
>  	struct drm_device *dev = fb_helper->dev;
>  	struct drm_atomic_state *state;
> +	struct drm_plane *plane;
>  	int i, ret;
> +	unsigned plane_mask;
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
> @@ -1235,19 +1237,22 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
>  
>  	state->acquire_ctx = dev->mode_config.acquire_ctx;
>  retry:
> +	plane_mask = 0;
>  	for(i = 0; i < fb_helper->crtc_count; i++) {
>  		struct drm_mode_set *mode_set;
>  
>  		mode_set = &fb_helper->crtc_info[i].mode_set;
>  
> -		mode_set->crtc->primary->old_fb = mode_set->crtc->primary->fb;
> -
>  		mode_set->x = var->xoffset;
>  		mode_set->y = var->yoffset;
>  
>  		ret = __drm_atomic_helper_set_config(mode_set, state);
>  		if (ret != 0)
>  			goto fail;
> +
> +		plane = mode_set->crtc->primary;
> +		plane_mask |= drm_plane_index(plane);
> +		plane->old_fb = plane->fb;
>  	}
>  
>  	ret = drm_atomic_commit(state);
> @@ -1259,26 +1264,7 @@ retry:
>  
>  
>  fail:
> -	for(i = 0; i < fb_helper->crtc_count; i++) {
> -		struct drm_mode_set *mode_set;
> -		struct drm_plane *plane;
> -
> -		mode_set = &fb_helper->crtc_info[i].mode_set;
> -		plane = mode_set->crtc->primary;
> -
> -		if (ret == 0) {
> -			struct drm_framebuffer *new_fb = plane->state->fb;
> -
> -			if (new_fb)
> -				drm_framebuffer_reference(new_fb);
> -			plane->fb = new_fb;
> -			plane->crtc = plane->state->crtc;
> -
> -			if (plane->old_fb)
> -				drm_framebuffer_unreference(plane->old_fb);
> -		}
> -		plane->old_fb = NULL;
> -	}
> +	drm_atomic_clean_old_fb(dev, plane_mask, ret);
>  
>  	if (ret == -EDEADLK)
>  		goto backoff;
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/core: Fix old_fb handling in pan_display_atomic.

[Jani Nikula]

On Tue, 17 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 11, 2015 at 11:29:11AM +0100, Maarten Lankhorst wrote:
>> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> 
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Needs "Don't touch plane->old_fb/fb without having the right locks held."
> like the previous patch in the commit message. With that for patches 4&5:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed the series to topic/drm-fixes, with the comments added, thanks
for the patches and review.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx