[PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again.

[PATCH 0/3] drm/atomic: Make drm_framebuffer_remove atomic, again.

[Maarten Lankhorst]

This fixes a connector leak first when disabling the device,
and then converts rmfb to atomic. The behavior change is done as
a separate patch, so it can be reverted if it breaks, again..

Maarten Lankhorst (3):
  drm/atomic: Make disable_all helper fully disable the crtc.
  drm: Convert drm_framebuffer_remove to atomic, v4.
  drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb.

 drivers/gpu/drm/drm_atomic.c              | 106 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
 drivers/gpu/drm/drm_crtc_internal.h       |   1 +
 drivers/gpu/drm/drm_framebuffer.c         |   7 ++
 drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
 5 files changed, 152 insertions(+), 126 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Maarten Lankhorst]

It seems that nouveau requires this, so best to do this in the helper.
This allows nouveau to use the atomic suspend helper.

Cc: nouveau@lists.freedesktop.org
Acked-by: Ben Skeggs <bskeggs@redhat.com> #irc
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
 drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
 2 files changed, 38 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9203f3e933f7..ff361066381e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2427,9 +2427,13 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
-	struct drm_connector_list_iter conn_iter;
-	int err;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int ret, i;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -2437,29 +2441,48 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 
 	state->acquire_ctx = ctx;
 
-	drm_connector_list_iter_get(dev, &conn_iter);
-	drm_for_each_connector_iter(conn, &conn_iter) {
-		struct drm_crtc *crtc = conn->state->crtc;
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
-			continue;
-
+	drm_for_each_crtc(crtc, dev) {
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state)) {
-			err = PTR_ERR(crtc_state);
+			ret = PTR_ERR(crtc_state);
 			goto free;
 		}
 
 		crtc_state->active = false;
+
+		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret < 0)
+			goto free;
 	}
 
-	err = drm_atomic_commit(state);
+	for_each_connector_in_state(state, conn, conn_state, i) {
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+		if (ret < 0)
+			goto free;
+	}
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+	}
+
+	ret = drm_atomic_commit(state);
 free:
-	drm_connector_list_iter_put(&conn_iter);
 	drm_atomic_state_put(state);
-	return err;
+	return ret;
 }
+
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index d479aad97cd4..820f44bef0bd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -625,117 +625,6 @@ nouveau_display_destroy(struct drm_device *dev)
 	kfree(disp);
 }
 
-static int
-nouveau_atomic_disable_connector(struct drm_atomic_state *state,
-				 struct drm_connector *connector)
-{
-	struct drm_connector_state *connector_state;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane;
-	int ret;
-
-	if (!(crtc = connector->state->crtc))
-		return 0;
-
-	connector_state = drm_atomic_get_connector_state(state, connector);
-	if (IS_ERR(connector_state))
-		return PTR_ERR(connector_state);
-
-	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state->active = false;
-
-	drm_for_each_plane_mask(plane, connector->dev, crtc_state->plane_mask) {
-		plane_state = drm_atomic_get_plane_state(state, plane);
-		if (IS_ERR(plane_state))
-			return PTR_ERR(plane_state);
-
-		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
-		if (ret)
-			return ret;
-
-		drm_atomic_set_fb_for_plane(plane_state, NULL);
-	}
-
-	return 0;
-}
-
-static int
-nouveau_atomic_disable(struct drm_device *dev,
-		       struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_atomic_state *state;
-	struct drm_connector *connector;
-	int ret;
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-
-	drm_for_each_connector(connector, dev) {
-		ret = nouveau_atomic_disable_connector(state, connector);
-		if (ret)
-			break;
-	}
-
-	if (ret == 0)
-		ret = drm_atomic_commit(state);
-	drm_atomic_state_put(state);
-	return ret;
-}
-
-static struct drm_atomic_state *
-nouveau_atomic_suspend(struct drm_device *dev)
-{
-	struct drm_modeset_acquire_ctx ctx;
-	struct drm_atomic_state *state;
-	int ret;
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-retry:
-	ret = drm_modeset_lock_all_ctx(dev, &ctx);
-	if (ret < 0) {
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-	state = drm_atomic_helper_duplicate_state(dev, &ctx);
-	if (IS_ERR(state))
-		goto unlock;
-
-	ret = nouveau_atomic_disable(dev, &ctx);
-	if (ret < 0) {
-		drm_atomic_state_put(state);
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-unlock:
-	if (PTR_ERR(state) == -EDEADLK) {
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	return state;
-}
-
 int
 nouveau_display_suspend(struct drm_device *dev, bool runtime)
 {
@@ -744,7 +633,7 @@ nouveau_display_suspend(struct drm_device *dev, bool runtime)
 
 	if (drm_drv_uses_atomic_modeset(dev)) {
 		if (!runtime) {
-			disp->suspend = nouveau_atomic_suspend(dev);
+			disp->suspend = drm_atomic_helper_suspend(dev);
 			if (IS_ERR(disp->suspend)) {
 				int ret = PTR_ERR(disp->suspend);
 				disp->suspend = NULL;
-- 
2.7.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4.

[Maarten Lankhorst]

Instead of trying to do everything in 1 go, just do a basic safe
conversion first. We've been bitten by too many regressions in the
past.

This patch only converts drm_framebuffer_remove to atomic. The
regression sensitive part is split out to a separate patch.

v2:
- Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
- Add WARN_ON when atomic_remove_fb fails.
- Always call drm_atomic_state_put.
v3:
- Use drm_drv_uses_atomic_modeset
- Handle the case where the first plane-disable-only commit fails
  with -EINVAL. Some drivers do not support this, fall back to
  disabling all crtc's in this case.
v4:
- Solve vmwgfx compatibility issue in their driver, was fixed in this
  patch by v3.
- Move only disabling primary to a separate patch.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 88 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_framebuffer.c   |  7 +++
 3 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index afec53832145..285e1c23e8c9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev,
 	kfree(fence_state);
 }
 
+int drm_atomic_remove_fb(struct drm_framebuffer *fb)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_device *dev = fb->dev;
+	struct drm_atomic_state *state;
+	struct drm_plane *plane;
+	struct drm_connector *conn;
+	struct drm_connector_state *conn_state;
+	int i, ret = 0;
+	unsigned plane_mask;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;
+
+retry:
+	plane_mask = 0;
+	ret = drm_modeset_lock_all_ctx(dev, &ctx);
+	if (ret)
+		goto unlock;
+
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		if (plane->state->fb != fb)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto unlock;
+		}
+
+		if (plane_state->crtc->primary == plane) {
+			struct drm_crtc_state *crtc_state;
+
+			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
+
+			ret = drm_atomic_add_affected_connectors(state, plane_state->crtc);
+			if (ret)
+				goto unlock;
+
+			crtc_state->active = false;
+			ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+			if (ret)
+				goto unlock;
+		}
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret)
+			goto unlock;
+
+		plane_mask |= BIT(drm_plane_index(plane));
+
+		plane->old_fb = plane->fb;
+	}
+
+	for_each_connector_in_state(state, conn, conn_state, i) {
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+
+		if (ret)
+			goto unlock;
+	}
+
+	if (plane_mask)
+		ret = drm_atomic_commit(state);
+
+unlock:
+	if (plane_mask)
+		drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv)
 {
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..e0678f8a51cf 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
 			    struct drm_property *property, uint64_t *val);
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv);
+int drm_atomic_remove_fb(struct drm_framebuffer *fb);
 
 
 /* drm_plane.c */
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 28a0108a1ab8..c0e593a7f9b4 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in this manner.
 	 */
 	if (drm_framebuffer_read_refcount(fb) > 1) {
+		if (drm_drv_uses_atomic_modeset(dev)) {
+			int ret = drm_atomic_remove_fb(fb);
+			WARN(ret, "atomic remove_fb failed with %i\n", ret);
+			goto out;
+		}
+
 		drm_modeset_lock_all(dev);
 		/* remove from any CRTC */
 		drm_for_each_crtc(crtc, dev) {
@@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 		drm_modeset_unlock_all(dev);
 	}
 
+out:
 	drm_framebuffer_unreference(fb);
 }
 EXPORT_SYMBOL(drm_framebuffer_remove);
-- 
2.7.4

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

[PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb.

[Maarten Lankhorst]

This introduces a slight behavioral change to rmfb. Instead of
disabling a crtc when the primary plane is disabled, we try to
preserve it.

Apart from old versions of the vmwgfx xorg driver, there is
nothing depending on rmfb disabling a crtc. Since vmwgfx is
a legacy driver we can safely only disable the plane with atomic.
In the conversion to atomic the driver will reject the config
without primary plane.

If this commit is rejected by the driver then we will still fall
back to the old behavior and turn off the crtc.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 285e1c23e8c9..0d5b0065208e 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
 	struct drm_connector *conn;
 	struct drm_connector_state *conn_state;
 	int i, ret = 0;
-	unsigned plane_mask;
+	unsigned plane_mask, disable_crtcs = false;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
 			goto unlock;
 		}
 
-		if (plane_state->crtc->primary == plane) {
+		/*
+		 * Some drivers do not support keeping crtc active with the
+		 * primary plane disabled. If we fail to commit with -EINVAL
+		 * then we will try to perform the same commit but with all
+		 * crtc's disabled for primary planes as well.
+		 */
+		if (disable_crtcs && plane_state->crtc->primary == plane) {
 			struct drm_crtc_state *crtc_state;
 
 			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
@@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
 		plane->old_fb = plane->fb;
 	}
 
+	/* This list is only not empty when disable_crtcs is set. */
 	for_each_connector_in_state(state, conn, conn_state, i) {
 		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
 
@@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
 
 	drm_atomic_state_put(state);
 
+	if (ret == -EINVAL && !disable_crtcs) {
+		disable_crtcs = true;
+
+		state = drm_atomic_state_alloc(dev);
+		if (state) {
+			state->acquire_ctx = &ctx;
+			goto retry;
+		}
+		ret = -ENOMEM;
+	}
+
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-- 
2.7.4

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

✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again.

[Patchwork]

== Series Details ==

Series: drm/atomic: Make drm_framebuffer_remove atomic, again.
URL   : https://patchwork.freedesktop.org/series/20008/
State : failure

== Summary ==

Series 20008v1 drm/atomic: Make drm_framebuffer_remove atomic, again.
https://patchwork.freedesktop.org/api/1.0/series/20008/revisions/1/mbox/

Test gem_exec_basic:
        Subgroup gtt-render:
                incomplete -> PASS       (fi-byt-j1900)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> DMESG-FAIL (fi-snb-2520m)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-FAIL (fi-snb-2520m)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-FAIL (fi-snb-2520m)
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-FAIL (fi-snb-2520m)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> INCOMPLETE (fi-snb-2520m)

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:205  pass:179  dwarn:0   dfail:4   fail:0   skip:21 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

3d47f9476bdc68afdb73ddbc96375366ea689a79 drm-tip: 2017y-02m-21d-14h-13m-06s UTC integration manifest
b55a0ee drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb.
fad2fa6 drm: Convert drm_framebuffer_remove to atomic, v4.
ee2d848 drm/atomic: Make disable_all helper fully disable the crtc.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3913/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/atomic: Make drm_framebuffer_remove atomic, again.

[Maarten Lankhorst]

Op 21-02-17 om 16:23 schreef Patchwork:
> == Series Details ==
>
> Series: drm/atomic: Make drm_framebuffer_remove atomic, again.
> URL   : https://patchwork.freedesktop.org/series/20008/
> State : failure
>
> == Summary ==
>
> Series 20008v1 drm/atomic: Make drm_framebuffer_remove atomic, again.
> https://patchwork.freedesktop.org/api/1.0/series/20008/revisions/1/mbox/
>
> Test gem_exec_basic:
>         Subgroup gtt-render:
>                 incomplete -> PASS       (fi-byt-j1900)
> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                 pass       -> DMESG-FAIL (fi-snb-2520m)
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> DMESG-FAIL (fi-snb-2520m)
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 pass       -> DMESG-FAIL (fi-snb-2520m)
>         Subgroup read-crc-pipe-a:
>                 pass       -> DMESG-FAIL (fi-snb-2520m)
>         Subgroup read-crc-pipe-a-frame-sequence:
>                 pass       -> INCOMPLETE (fi-snb-2520m)
Sigh, last patch regresses on the crc tests again. snb-2600 is fine so something specific to the configuration there..

https://patchwork.freedesktop.org/series/20012/ are the ci results for this series without the last patch, so
please apply patch 1 and 2 only?

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

Re: [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Sean Paul]

On Tue, Feb 21, 2017 at 02:51:40PM +0100, Maarten Lankhorst wrote:
> It seems that nouveau requires this, so best to do this in the helper.
> This allows nouveau to use the atomic suspend helper.

Pardon the stupid question, but why can't nouveau just do the right thing when
crtc_state->active becomes false?

Sean

> 
> Cc: nouveau@lists.freedesktop.org
> Acked-by: Ben Skeggs <bskeggs@redhat.com> #irc
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
>  drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
>  2 files changed, 38 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9203f3e933f7..ff361066381e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2427,9 +2427,13 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  				  struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_atomic_state *state;
> +	struct drm_connector_state *conn_state;
>  	struct drm_connector *conn;
> -	struct drm_connector_list_iter conn_iter;
> -	int err;
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int ret, i;
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
> @@ -2437,29 +2441,48 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  
>  	state->acquire_ctx = ctx;
>  
> -	drm_connector_list_iter_get(dev, &conn_iter);
> -	drm_for_each_connector_iter(conn, &conn_iter) {
> -		struct drm_crtc *crtc = conn->state->crtc;
> -		struct drm_crtc_state *crtc_state;
> -
> -		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
> -			continue;
> -
> +	drm_for_each_crtc(crtc, dev) {
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state)) {
> -			err = PTR_ERR(crtc_state);
> +			ret = PTR_ERR(crtc_state);
>  			goto free;
>  		}
>  
>  		crtc_state->active = false;
> +
> +		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +
> +		ret = drm_atomic_add_affected_planes(state, crtc);
> +		if (ret < 0)
> +			goto free;
> +
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret < 0)
> +			goto free;
>  	}
>  
> -	err = drm_atomic_commit(state);
> +	for_each_connector_in_state(state, conn, conn_state, i) {
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +	}
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret < 0)
> +			goto free;
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +	}
> +
> +	ret = drm_atomic_commit(state);
>  free:
> -	drm_connector_list_iter_put(&conn_iter);
>  	drm_atomic_state_put(state);
> -	return err;
> +	return ret;
>  }
> +
>  EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>  
>  /**
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index d479aad97cd4..820f44bef0bd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -625,117 +625,6 @@ nouveau_display_destroy(struct drm_device *dev)
>  	kfree(disp);
>  }
>  
> -static int
> -nouveau_atomic_disable_connector(struct drm_atomic_state *state,
> -				 struct drm_connector *connector)
> -{
> -	struct drm_connector_state *connector_state;
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_plane_state *plane_state;
> -	struct drm_plane *plane;
> -	int ret;
> -
> -	if (!(crtc = connector->state->crtc))
> -		return 0;
> -
> -	connector_state = drm_atomic_get_connector_state(state, connector);
> -	if (IS_ERR(connector_state))
> -		return PTR_ERR(connector_state);
> -
> -	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> -
> -	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state->active = false;
> -
> -	drm_for_each_plane_mask(plane, connector->dev, crtc_state->plane_mask) {
> -		plane_state = drm_atomic_get_plane_state(state, plane);
> -		if (IS_ERR(plane_state))
> -			return PTR_ERR(plane_state);
> -
> -		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> -		if (ret)
> -			return ret;
> -
> -		drm_atomic_set_fb_for_plane(plane_state, NULL);
> -	}
> -
> -	return 0;
> -}
> -
> -static int
> -nouveau_atomic_disable(struct drm_device *dev,
> -		       struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_atomic_state *state;
> -	struct drm_connector *connector;
> -	int ret;
> -
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	state->acquire_ctx = ctx;
> -
> -	drm_for_each_connector(connector, dev) {
> -		ret = nouveau_atomic_disable_connector(state, connector);
> -		if (ret)
> -			break;
> -	}
> -
> -	if (ret == 0)
> -		ret = drm_atomic_commit(state);
> -	drm_atomic_state_put(state);
> -	return ret;
> -}
> -
> -static struct drm_atomic_state *
> -nouveau_atomic_suspend(struct drm_device *dev)
> -{
> -	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_atomic_state *state;
> -	int ret;
> -
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -retry:
> -	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> -	if (ret < 0) {
> -		state = ERR_PTR(ret);
> -		goto unlock;
> -	}
> -
> -	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> -	if (IS_ERR(state))
> -		goto unlock;
> -
> -	ret = nouveau_atomic_disable(dev, &ctx);
> -	if (ret < 0) {
> -		drm_atomic_state_put(state);
> -		state = ERR_PTR(ret);
> -		goto unlock;
> -	}
> -
> -unlock:
> -	if (PTR_ERR(state) == -EDEADLK) {
> -		drm_modeset_backoff(&ctx);
> -		goto retry;
> -	}
> -
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -	return state;
> -}
> -
>  int
>  nouveau_display_suspend(struct drm_device *dev, bool runtime)
>  {
> @@ -744,7 +633,7 @@ nouveau_display_suspend(struct drm_device *dev, bool runtime)
>  
>  	if (drm_drv_uses_atomic_modeset(dev)) {
>  		if (!runtime) {
> -			disp->suspend = nouveau_atomic_suspend(dev);
> +			disp->suspend = drm_atomic_helper_suspend(dev);
>  			if (IS_ERR(disp->suspend)) {
>  				int ret = PTR_ERR(disp->suspend);
>  				disp->suspend = NULL;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4.

[Sean Paul]

On Tue, Feb 21, 2017 at 02:51:41PM +0100, Maarten Lankhorst wrote:
> Instead of trying to do everything in 1 go, just do a basic safe
> conversion first. We've been bitten by too many regressions in the
> past.
> 
> This patch only converts drm_framebuffer_remove to atomic. The
> regression sensitive part is split out to a separate patch.
> 
> v2:
> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
> - Add WARN_ON when atomic_remove_fb fails.
> - Always call drm_atomic_state_put.
> v3:
> - Use drm_drv_uses_atomic_modeset
> - Handle the case where the first plane-disable-only commit fails
>   with -EINVAL. Some drivers do not support this, fall back to
>   disabling all crtc's in this case.
> v4:
> - Solve vmwgfx compatibility issue in their driver, was fixed in this
>   patch by v3.
> - Move only disabling primary to a separate patch.
> 

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 88 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |  1 +
>  drivers/gpu/drm/drm_framebuffer.c   |  7 +++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index afec53832145..285e1c23e8c9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev,
>  	kfree(fence_state);
>  }
>  
> +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_device *dev = fb->dev;
> +	struct drm_atomic_state *state;
> +	struct drm_plane *plane;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *conn_state;
> +	int i, ret = 0;
> +	unsigned plane_mask;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	plane_mask = 0;
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +	if (ret)
> +		goto unlock;
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (plane->state->fb != fb)
> +			continue;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto unlock;
> +		}
> +
> +		if (plane_state->crtc->primary == plane) {
> +			struct drm_crtc_state *crtc_state;
> +
> +			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
> +
> +			ret = drm_atomic_add_affected_connectors(state, plane_state->crtc);
> +			if (ret)
> +				goto unlock;
> +
> +			crtc_state->active = false;
> +			ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> +			if (ret)
> +				goto unlock;
> +		}
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret)
> +			goto unlock;
> +
> +		plane_mask |= BIT(drm_plane_index(plane));
> +
> +		plane->old_fb = plane->fb;
> +	}
> +
> +	for_each_connector_in_state(state, conn, conn_state, i) {
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	if (plane_mask)
> +		ret = drm_atomic_commit(state);
> +
> +unlock:
> +	if (plane_mask)
> +		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv)
>  {
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 955c5690bf64..e0678f8a51cf 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
>  			    struct drm_property *property, uint64_t *val);
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv);
> +int drm_atomic_remove_fb(struct drm_framebuffer *fb);
>  
>  
>  /* drm_plane.c */
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 28a0108a1ab8..c0e593a7f9b4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in this manner.
>  	 */
>  	if (drm_framebuffer_read_refcount(fb) > 1) {
> +		if (drm_drv_uses_atomic_modeset(dev)) {
> +			int ret = drm_atomic_remove_fb(fb);
> +			WARN(ret, "atomic remove_fb failed with %i\n", ret);
> +			goto out;
> +		}
> +
>  		drm_modeset_lock_all(dev);
>  		/* remove from any CRTC */
>  		drm_for_each_crtc(crtc, dev) {
> @@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  		drm_modeset_unlock_all(dev);
>  	}
>  
> +out:
>  	drm_framebuffer_unreference(fb);
>  }
>  EXPORT_SYMBOL(drm_framebuffer_remove);
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb.

[Sean Paul]

On Tue, Feb 21, 2017 at 02:51:42PM +0100, Maarten Lankhorst wrote:
> This introduces a slight behavioral change to rmfb. Instead of
> disabling a crtc when the primary plane is disabled, we try to
> preserve it.
> 
> Apart from old versions of the vmwgfx xorg driver, there is
> nothing depending on rmfb disabling a crtc. Since vmwgfx is
> a legacy driver we can safely only disable the plane with atomic.
> In the conversion to atomic the driver will reject the config
> without primary plane.
> 
> If this commit is rejected by the driver then we will still fall
> back to the old behavior and turn off the crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 285e1c23e8c9..0d5b0065208e 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>  	struct drm_connector *conn;
>  	struct drm_connector_state *conn_state;
>  	int i, ret = 0;
> -	unsigned plane_mask;
> +	unsigned plane_mask, disable_crtcs = false;
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
> @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>  			goto unlock;
>  		}
>  
> -		if (plane_state->crtc->primary == plane) {
> +		/*
> +		 * Some drivers do not support keeping crtc active with the
> +		 * primary plane disabled. If we fail to commit with -EINVAL
> +		 * then we will try to perform the same commit but with all
> +		 * crtc's disabled for primary planes as well.
> +		 */
> +		if (disable_crtcs && plane_state->crtc->primary == plane) {
>  			struct drm_crtc_state *crtc_state;
>  
>  			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
> @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>  		plane->old_fb = plane->fb;
>  	}
>  
> +	/* This list is only not empty when disable_crtcs is set. */
>  	for_each_connector_in_state(state, conn, conn_state, i) {
>  		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>  
> @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>  
>  	drm_atomic_state_put(state);
>  
> +	if (ret == -EINVAL && !disable_crtcs) {
> +		disable_crtcs = true;
> +
> +		state = drm_atomic_state_alloc(dev);
> +		if (state) {
> +			state->acquire_ctx = &ctx;
> +			goto retry;

Re-using the retry label is a little fishy here. You need to re-allocate state,
but we don't drop the locks, which makes things a little unbalanced (I realize
modeset_lock will s/EALREADY/0/).

Perhaps something like this above would be more clear:

+       drm_modeset_acquire_init(&ctx, 0);
+retry:
+       plane_mask = 0;
+       ret = drm_modeset_lock_all_ctx(dev, &ctx);
+       if (ret)
+               goto unlock;
+
+retry_disable:
+       state = drm_atomic_state_alloc(dev);
+       if (!state)
+               return -ENOMEM;
+       state->acquire_ctx = &ctx;
+

Then you can do:


+	if (ret == -EINVAL && !disable_crtcs) {
+		disable_crtcs = true;
+		goto retry_disable;
+       }

(you would also need to ensure you moved the state_put above the EDEADLK
handling).

Thoughts?

Sean

> +		}
> +		ret = -ENOMEM;
> +	}
> +
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Maarten Lankhorst]

Op 23-02-17 om 16:03 schreef Sean Paul:
> On Tue, Feb 21, 2017 at 02:51:40PM +0100, Maarten Lankhorst wrote:
>> It seems that nouveau requires this, so best to do this in the helper.
>> This allows nouveau to use the atomic suspend helper.
> Pardon the stupid question, but why can't nouveau just do the right thing when
> crtc_state->active becomes false?
This patch is also required to clean up a connector leak in i915 driver unload as well, and also to clean up plane state destruction there. Nouveau needs it to unpin some framebuffers during suspend, but even if none of this was true, this is the right way to handle disable_all.

Looking at DPMS is fragile.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb.

[Maarten Lankhorst]

Op 23-02-17 om 16:24 schreef Sean Paul:
> On Tue, Feb 21, 2017 at 02:51:42PM +0100, Maarten Lankhorst wrote:
>> This introduces a slight behavioral change to rmfb. Instead of
>> disabling a crtc when the primary plane is disabled, we try to
>> preserve it.
>>
>> Apart from old versions of the vmwgfx xorg driver, there is
>> nothing depending on rmfb disabling a crtc. Since vmwgfx is
>> a legacy driver we can safely only disable the plane with atomic.
>> In the conversion to atomic the driver will reject the config
>> without primary plane.
>>
>> If this commit is rejected by the driver then we will still fall
>> back to the old behavior and turn off the crtc.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 285e1c23e8c9..0d5b0065208e 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>>  	struct drm_connector *conn;
>>  	struct drm_connector_state *conn_state;
>>  	int i, ret = 0;
>> -	unsigned plane_mask;
>> +	unsigned plane_mask, disable_crtcs = false;
>>  
>>  	state = drm_atomic_state_alloc(dev);
>>  	if (!state)
>> @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>>  			goto unlock;
>>  		}
>>  
>> -		if (plane_state->crtc->primary == plane) {
>> +		/*
>> +		 * Some drivers do not support keeping crtc active with the
>> +		 * primary plane disabled. If we fail to commit with -EINVAL
>> +		 * then we will try to perform the same commit but with all
>> +		 * crtc's disabled for primary planes as well.
>> +		 */
>> +		if (disable_crtcs && plane_state->crtc->primary == plane) {
>>  			struct drm_crtc_state *crtc_state;
>>  
>>  			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
>> @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>>  		plane->old_fb = plane->fb;
>>  	}
>>  
>> +	/* This list is only not empty when disable_crtcs is set. */
>>  	for_each_connector_in_state(state, conn, conn_state, i) {
>>  		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>>  
>> @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb)
>>  
>>  	drm_atomic_state_put(state);
>>  
>> +	if (ret == -EINVAL && !disable_crtcs) {
>> +		disable_crtcs = true;
>> +
>> +		state = drm_atomic_state_alloc(dev);
>> +		if (state) {
>> +			state->acquire_ctx = &ctx;
>> +			goto retry;
> Re-using the retry label is a little fishy here. You need to re-allocate state,
> but we don't drop the locks, which makes things a little unbalanced (I realize
> modeset_lock will s/EALREADY/0/).
drm_modeset_lock can be called any arbitrary numer of times. I'm not worried about it. :)
> Perhaps something like this above would be more clear:
>
> +       drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +       plane_mask = 0;
> +       ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +       if (ret)
> +               goto unlock;
> +
> +retry_disable:
> +       state = drm_atomic_state_alloc(dev);
> +       if (!state)
> +               return -ENOMEM;
> +       state->acquire_ctx = &ctx;
> +
>
> Then you can do:
>
>
> +	if (ret == -EINVAL && !disable_crtcs) {
> +		disable_crtcs = true;
> +		goto retry_disable;
> +       }
>
> (you would also need to ensure you moved the state_put above the EDEADLK
> handling).
>
> Thoughts?
In general we try to call drm_atomic_state_clear instead of drm_atomic_state_put, this would be a source of confusion for being different from other users of atomic.

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

Re: [PATCH 2/3] drm: Convert drm_framebuffer_remove to atomic, v4.

[Daniel Vetter]

On Thu, Feb 23, 2017 at 10:09:27AM -0500, Sean Paul wrote:
> On Tue, Feb 21, 2017 at 02:51:41PM +0100, Maarten Lankhorst wrote:
> > Instead of trying to do everything in 1 go, just do a basic safe
> > conversion first. We've been bitten by too many regressions in the
> > past.
> > 
> > This patch only converts drm_framebuffer_remove to atomic. The
> > regression sensitive part is split out to a separate patch.
> > 
> > v2:
> > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
> > - Add WARN_ON when atomic_remove_fb fails.
> > - Always call drm_atomic_state_put.
> > v3:
> > - Use drm_drv_uses_atomic_modeset
> > - Handle the case where the first plane-disable-only commit fails
> >   with -EINVAL. Some drivers do not support this, fall back to
> >   disabling all crtc's in this case.
> > v4:
> > - Solve vmwgfx compatibility issue in their driver, was fixed in this
> >   patch by v3.
> > - Move only disabling primary to a separate patch.
> > 
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Ok, merged the first 2 from this series to drm-misc-next.

Thanks, Daniel

> 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 88 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_crtc_internal.h |  1 +
> >  drivers/gpu/drm/drm_framebuffer.c   |  7 +++
> >  3 files changed, 96 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index afec53832145..285e1c23e8c9 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -2061,6 +2061,94 @@ static void complete_crtc_signaling(struct drm_device *dev,
> >  	kfree(fence_state);
> >  }
> >  
> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb)
> > +{
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_device *dev = fb->dev;
> > +	struct drm_atomic_state *state;
> > +	struct drm_plane *plane;
> > +	struct drm_connector *conn;
> > +	struct drm_connector_state *conn_state;
> > +	int i, ret = 0;
> > +	unsigned plane_mask;
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state)
> > +		return -ENOMEM;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	plane_mask = 0;
> > +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	drm_for_each_plane(plane, dev) {
> > +		struct drm_plane_state *plane_state;
> > +
> > +		if (plane->state->fb != fb)
> > +			continue;
> > +
> > +		plane_state = drm_atomic_get_plane_state(state, plane);
> > +		if (IS_ERR(plane_state)) {
> > +			ret = PTR_ERR(plane_state);
> > +			goto unlock;
> > +		}
> > +
> > +		if (plane_state->crtc->primary == plane) {
> > +			struct drm_crtc_state *crtc_state;
> > +
> > +			crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc);
> > +
> > +			ret = drm_atomic_add_affected_connectors(state, plane_state->crtc);
> > +			if (ret)
> > +				goto unlock;
> > +
> > +			crtc_state->active = false;
> > +			ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> > +			if (ret)
> > +				goto unlock;
> > +		}
> > +
> > +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> > +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> > +		if (ret)
> > +			goto unlock;
> > +
> > +		plane_mask |= BIT(drm_plane_index(plane));
> > +
> > +		plane->old_fb = plane->fb;
> > +	}
> > +
> > +	for_each_connector_in_state(state, conn, conn_state, i) {
> > +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> > +
> > +		if (ret)
> > +			goto unlock;
> > +	}
> > +
> > +	if (plane_mask)
> > +		ret = drm_atomic_commit(state);
> > +
> > +unlock:
> > +	if (plane_mask)
> > +		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> > +
> > +	if (ret == -EDEADLK) {
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +}
> > +
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv)
> >  {
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > index 955c5690bf64..e0678f8a51cf 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -183,6 +183,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
> >  			    struct drm_property *property, uint64_t *val);
> >  int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  			  void *data, struct drm_file *file_priv);
> > +int drm_atomic_remove_fb(struct drm_framebuffer *fb);
> >  
> >  
> >  /* drm_plane.c */
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 28a0108a1ab8..c0e593a7f9b4 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -773,6 +773,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >  	 * in this manner.
> >  	 */
> >  	if (drm_framebuffer_read_refcount(fb) > 1) {
> > +		if (drm_drv_uses_atomic_modeset(dev)) {
> > +			int ret = drm_atomic_remove_fb(fb);
> > +			WARN(ret, "atomic remove_fb failed with %i\n", ret);
> > +			goto out;
> > +		}
> > +
> >  		drm_modeset_lock_all(dev);
> >  		/* remove from any CRTC */
> >  		drm_for_each_crtc(crtc, dev) {
> > @@ -790,6 +796,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >  		drm_modeset_unlock_all(dev);
> >  	}
> >  
> > +out:
> >  	drm_framebuffer_unreference(fb);
> >  }
> >  EXPORT_SYMBOL(drm_framebuffer_remove);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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