[PATCH 0/4] drm: Resurrect atomic rmfb code.

[PATCH 0/4] drm: Resurrect atomic rmfb code.

[Maarten Lankhorst]

There were some issues in i915 preventing rmfb from working correctly.
A lot of them are related to hw readout, some others due to keeping
crtc active during driver unload:

https://intel-gfx-ci.01.org/CI/Trybot_394/

I fixed the POWER_DOMAIN_AUDIO issue because it was easy to do so,
but ignored the other failures by disabling all crtc's during driver
unload.

Daniel Vetter (1):
  drm: Resurrect atomic rmfb code, v2

Maarten Lankhorst (3):
  drm/atomic: Delete wrong comment.
  drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.
  drm/i915: Disable all crtcs during driver unload.

 drivers/gpu/drm/drm_atomic.c         | 68 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_crtc_internal.h  |  1 +
 drivers/gpu/drm/drm_framebuffer.c    |  7 ++++
 drivers/gpu/drm/i915/i915_drv.c      |  5 +++
 drivers/gpu/drm/i915/intel_ddi.c     | 14 ++------
 drivers/gpu/drm/i915/intel_display.c |  4 +++
 drivers/gpu/drm/i915/intel_dp_mst.c  |  9 ++---
 7 files changed, 85 insertions(+), 23 deletions(-)

-- 
2.7.4

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

[PATCH 1/4] drm/atomic: Delete wrong comment.

[Maarten Lankhorst]

drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
different from commit.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 60697482b94c..d1d252261bf1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2195,10 +2195,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		goto out;
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
-		/*
-		 * Unlike commit, check_only does not clean up state.
-		 * Below we call drm_atomic_state_put for it.
-		 */
 		ret = drm_atomic_check_only(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_nonblocking_commit(state);
-- 
2.7.4

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

[PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Maarten Lankhorst]

If the crtc was brought up with audio before the driver loads,
then crtc_disable will remove a refcount to audio that doesn't exist
before.

Fortunately we already set power domains on readout, so we can just add
the power domain handling to get_crtc_power_domains, which will update
the power domains correctly in all cases.

This was found when testing module reload on CI with the crtc enabled,
which resulted in the following warn after module reload + modeset:

[   24.197041] ------------[ cut here ]------------
[   24.197075] WARNING: CPU: 0 PID: 99 at drivers/gpu/drm/i915/intel_runtime_pm.c:1790 intel_display_power_put+0x134/0x140 [i915]
[   24.197076] Use count on domain AUDIO is already zero
[   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 4.9.0-CI-Trybot_393+ #1
[   24.197099] Hardware name:                  /NUC6i5SYB, BIOS SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
[   24.197102] Workqueue: events_unbound async_run_entry_fn
[   24.197105]  ffffc900003c7688 ffffffff81435b35 ffffc900003c76d8 0000000000000000
[   24.197107]  ffffc900003c76c8 ffffffff8107e4d6 000006fe5dc36f28 ffff88025dc30054
[   24.197109]  ffff88025dc36f28 ffff88025dc30000 ffff88025dc30000 0000000000000015
[   24.197110] Call Trace:
[   24.197113]  [<ffffffff81435b35>] dump_stack+0x67/0x92
[   24.197116]  [<ffffffff8107e4d6>] __warn+0xc6/0xe0
[   24.197118]  [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
[   24.197149]  [<ffffffffa039b4b4>] intel_display_power_put+0x134/0x140 [i915]
[   24.197187]  [<ffffffffa04217dd>] intel_disable_ddi+0x4d/0x80 [i915]
[   24.197223]  [<ffffffffa03f388f>] intel_encoders_disable.isra.74+0x7f/0x90 [i915]
[   24.197257]  [<ffffffffa03f6c05>] haswell_crtc_disable+0x55/0x170 [i915]
[   24.197292]  [<ffffffffa03fec88>] intel_atomic_commit_tail+0x108/0xfd0 [i915]
[   24.197295]  [<ffffffff810d47c6>] ? __lock_is_held+0x66/0x90
[   24.197330]  [<ffffffffa03fff79>] intel_atomic_commit+0x429/0x560 [i915]
[   24.197332]  [<ffffffff81570186>] ?drm_atomic_add_affected_connectors+0x56/0xf0
[   24.197334]  [<ffffffff8156f726>] drm_atomic_commit+0x46/0x50
[   24.197336]  [<ffffffff81553f87>] restore_fbdev_mode+0x147/0x270
[   24.197337]  [<ffffffff81555bee>] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
[   24.197339]  [<ffffffff81555aa8>] drm_fb_helper_set_par+0x28/0x50
[   24.197374]  [<ffffffffa041c7d3>] intel_fbdev_set_par+0x13/0x70 [i915]
[   24.197376]  [<ffffffff8149e07a>] fbcon_init+0x57a/0x600
[   24.197379]  [<ffffffff81514b71>] visual_init+0xd1/0x130
[   24.197381]  [<ffffffff8151603c>] do_bind_con_driver+0x1bc/0x3a0
[   24.197384]  [<ffffffff81516521>] do_take_over_console+0x111/0x180
[   24.197386]  [<ffffffff8149e152>] do_fbcon_takeover+0x52/0xb0
[   24.197387]  [<ffffffff814a12c3>] fbcon_event_notify+0x723/0x850
[   24.197390]  [<ffffffff810a4830>] ?__blocking_notifier_call_chain+0x30/0x70
[   24.197392]  [<ffffffff810a44a4>] notifier_call_chain+0x34/0xa0
[   24.197394]  [<ffffffff810a4848>] __blocking_notifier_call_chain+0x48/0x70
[   24.197397]  [<ffffffff810a4881>] blocking_notifier_call_chain+0x11/0x20
[   24.197398]  [<ffffffff814a4556>] fb_notifier_call_chain+0x16/0x20
[   24.197400]  [<ffffffff814a678c>] register_framebuffer+0x24c/0x330
[   24.197402]  [<ffffffff815558d9>] drm_fb_helper_initial_config+0x219/0x3c0
[   24.197436]  [<ffffffffa041d373>] intel_fbdev_initial_config+0x13/0x30 [i915]
[   24.197438]  [<ffffffff810a5d44>] async_run_entry_fn+0x34/0x140
[   24.197440]  [<ffffffff8109c26c>] process_one_work+0x1ec/0x6b0
[   24.197442]  [<ffffffff8109c1e6>] ? process_one_work+0x166/0x6b0
[   24.197445]  [<ffffffff8109c779>] worker_thread+0x49/0x490
[   24.197447]  [<ffffffff8109c730>] ? process_one_work+0x6b0/0x6b0
[   24.197448]  [<ffffffff810a2a9b>] kthread+0xeb/0x110
[   24.197451]  [<ffffffff810a29b0>] ? kthread_park+0x60/0x60
[   24.197453]  [<ffffffff818241a7>] ret_from_fork+0x27/0x40
[   24.197476] ---[ end trace bda64b683b8e8162 ]---

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 14 ++------------
 drivers/gpu/drm/i915/intel_display.c |  4 ++++
 drivers/gpu/drm/i915/intel_dp_mst.c  |  9 ++-------
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index d808a2ccc29e..8c9ce850760b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1835,8 +1835,6 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
 			     struct drm_connector_state *conn_state)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
 	enum port port = intel_ddi_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
@@ -1863,10 +1861,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
 		intel_edp_drrs_enable(intel_dp, pipe_config);
 	}
 
-	if (intel_crtc->config->has_audio) {
-		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+	if (pipe_config->has_audio)
 		intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
-	}
 }
 
 static void intel_disable_ddi(struct intel_encoder *intel_encoder,
@@ -1874,16 +1870,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 			      struct drm_connector_state *old_conn_state)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int type = intel_encoder->type;
-	struct drm_device *dev = encoder->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (intel_crtc->config->has_audio) {
+	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(intel_encoder);
-		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
-	}
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac587ca3b2af..0daa54633450 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5735,6 +5735,7 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
 					    struct intel_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
@@ -5756,6 +5757,9 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
 		mask |= BIT(intel_display_port_power_domain(intel_encoder));
 	}
 
+	if (HAS_DDI(dev_priv) && crtc_state->has_audio)
+		mask |= BIT(POWER_DOMAIN_AUDIO);
+
 	if (crtc_state->shared_dpll)
 		mask |= BIT(POWER_DOMAIN_PLLS);
 
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 205fe4748ec5..6c5382494fee 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -87,7 +87,6 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int ret;
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
@@ -98,10 +97,8 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
 	if (ret) {
 		DRM_ERROR("failed to update payload %d\n", ret);
 	}
-	if (old_crtc_state->has_audio) {
+	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(encoder);
-		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
-	}
 }
 
 static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
@@ -214,10 +211,8 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
 	ret = drm_dp_check_act_status(&intel_dp->mst_mgr);
 
 	ret = drm_dp_update_payload_part2(&intel_dp->mst_mgr);
-	if (pipe_config->has_audio) {
-		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+	if (pipe_config->has_audio)
 		intel_audio_codec_enable(encoder, pipe_config, conn_state);
-	}
 }
 
 static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
-- 
2.7.4

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

[PATCH 3/4] drm/i915: Disable all crtcs during driver unload.

[Maarten Lankhorst]

We may keep the crtc's enabled when userspace unsets all framebuffers but
keeps the crtc active. This exposes a WARN in fbc_global disable, and
a lot of bugs in our hardware readout code. Solve this by disabling
all crtc's for now.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6428588518aa..bb0d7517b678 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -43,6 +43,7 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/i915_drm.h>
 
 #include "i915_drv.h"
@@ -1282,6 +1283,10 @@ void i915_driver_unload(struct drm_device *dev)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
+	drm_modeset_lock_all(dev);
+	drm_atomic_helper_disable_all(dev, dev->mode_config.acquire_ctx);
+	drm_modeset_unlock_all(dev);
+
 	i915_driver_unregister(dev_priv);
 
 	drm_vblank_cleanup(dev);
-- 
2.7.4

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

[PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Maarten Lankhorst]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

This was somehow lost between v3 and the merged version in Maarten's
patch merged as:

commit f2d580b9a8149735cbc4b59c4a8df60173658140
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Wed May 4 14:38:26 2016 +0200

    drm/core: Do not preserve framebuffer on rmfb, v4.

Actual code copied from Maarten's patch, but with the slight change to
just use dev->mode_config.funcs->atomic_commit to decide whether to
use the atomic path or not.

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.

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        | 64 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_framebuffer.c   |  7 ++++
 3 files changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d1d252261bf1..23a3845542e1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2059,6 +2059,70 @@ 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;
+	int 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;
+		}
+
+		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;
+	}
+
+	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 cdf6860c9d22..121e250853d2 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -178,6 +178,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 cbf0c893f426..c358bf8280a8 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in this manner.
 	 */
 	if (drm_framebuffer_read_refcount(fb) > 1) {
+		if (dev->mode_config.funcs->atomic_commit) {
+			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) {
@@ -787,6 +793,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

Re: [Intel-gfx] [PATCH 1/4] drm/atomic: Delete wrong comment.

[Daniel Vetter]

On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
> different from commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think it'd be good to update the kerneldoc for the atomic_commit
callback to mention that drivers should grab their own references using
drm_atomic_state_get() when they need it.

Applied this one meanwhile, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 60697482b94c..d1d252261bf1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2195,10 +2195,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		goto out;
>  
>  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> -		/*
> -		 * Unlike commit, check_only does not clean up state.
> -		 * Below we call drm_atomic_state_put for it.
> -		 */
>  		ret = drm_atomic_check_only(state);
>  	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>  		ret = drm_atomic_nonblocking_commit(state);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

✗ Fi.CI.BAT: warning for drm: Resurrect atomic rmfb code.

[Patchwork]

== Series Details ==

Series: drm: Resurrect atomic rmfb code.
URL   : https://patchwork.freedesktop.org/series/16849/
State : warning

== Summary ==

Series 16849v1 drm: Resurrect atomic rmfb code.
https://patchwork.freedesktop.org/api/1.0/series/16849/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                dmesg-warn -> PASS       (fi-ivb-3770)
Test core_prop_blob:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-ivb-3770)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                dmesg-warn -> PASS       (fi-ivb-3770)
        Subgroup basic-subslice-total:
                dmesg-warn -> PASS       (fi-ivb-3770)
Test drv_hangman:
        Subgroup error-state-basic:
                dmesg-warn -> PASS       (fi-ivb-3770)
Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-fail -> PASS       (fi-ivb-3770)
        Subgroup basic-reload-final:
                fail       -> PASS       (fi-ivb-3770)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-skl-6260u)
Test gem_basic:
        Subgroup bad-close:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup create-close:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup create-fd-close:
                skip       -> PASS       (fi-ivb-3770)
Test gem_busy:
        Subgroup basic-busy-default:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-hang-default:
                skip       -> PASS       (fi-ivb-3770)
Test gem_close_race:
        Subgroup basic-process:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-threads:
                skip       -> PASS       (fi-ivb-3770)
Test gem_cpu_reloc:
        Subgroup basic:
                skip       -> PASS       (fi-ivb-3770)
Test gem_cs_tlb:
        Subgroup basic-default:
                skip       -> PASS       (fi-ivb-3770)
Test gem_ctx_basic:
                skip       -> PASS       (fi-ivb-3770)
Test gem_ctx_create:
        Subgroup basic:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-files:
                skip       -> PASS       (fi-ivb-3770)
Test gem_ctx_exec:
        Subgroup basic:
                skip       -> PASS       (fi-ivb-3770)
Test gem_ctx_param:
        Subgroup basic:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-default:
                skip       -> PASS       (fi-ivb-3770)
Test gem_ctx_switch:
        Subgroup basic-default:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-default-heavy:
                skip       -> PASS       (fi-ivb-3770)
Test gem_exec_basic:
        Subgroup basic-blt:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-bsd:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-default:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-render:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup gtt-blt:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup gtt-bsd:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup gtt-default:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup gtt-render:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup readonly-blt:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup readonly-bsd:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup readonly-default:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup readonly-render:
                skip       -> PASS       (fi-ivb-3770)
Test gem_exec_create:
        Subgroup basic:
                skip       -> PASS       (fi-ivb-3770)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                skip       -> PASS       (fi-ivb-3770)
        Subgroup basic-batch-kernel-default-uc:
                skip       -> PASS       (fi-ivb-3770)
WARNING: Long output truncated

a16fc233af5c80d1ed23178c94b7fbb77e48ffe0 drm-tip: 2016y-12m-15d-15h-21m-31s UTC integration manifest
b52a651 drm: Resurrect atomic rmfb code, v2
fe01be7 drm/i915: Disable all crtcs during driver unload.
48ad6e1 drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

== Logs ==

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

Re: ✗ Fi.CI.BAT: warning for drm: Resurrect atomic rmfb code.

[Maarten Lankhorst]

Op 15-12-16 om 17:23 schreef Patchwork:
> == Series Details ==
>
> Series: drm: Resurrect atomic rmfb code.
> URL   : https://patchwork.freedesktop.org/series/16849/
> State : warning
>
> == Summary ==
>
> Series 16849v1 drm: Resurrect atomic rmfb code.
> https://patchwork.freedesktop.org/api/1.0/series/16849/revisions/1/mbox/
>
> Test core_auth:
>         Subgroup basic-auth:
>                 dmesg-warn -> PASS       (fi-ivb-3770)
> Test core_prop_blob:
>         Subgroup basic:
>                 dmesg-warn -> PASS       (fi-ivb-3770)
> Test drv_getparams_basic:
>         Subgroup basic-eu-total:
>                 dmesg-warn -> PASS       (fi-ivb-3770)
>         Subgroup basic-subslice-total:
>                 dmesg-warn -> PASS       (fi-ivb-3770)
> Test drv_hangman:
>         Subgroup error-state-basic:
>                 dmesg-warn -> PASS       (fi-ivb-3770)
> Test drv_module_reload:
>         Subgroup basic-reload:
>                 dmesg-fail -> PASS       (fi-ivb-3770)
>         Subgroup basic-reload-final:
>                 fail       -> PASS       (fi-ivb-3770)
>         Subgroup basic-reload-inject:
>                 pass       -> DMESG-WARN (fi-skl-6260u)
> Test gem_basic:
>         Subgroup bad-close:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup create-close:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup create-fd-close:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_busy:
>         Subgroup basic-busy-default:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-hang-default:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_close_race:
>         Subgroup basic-process:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-threads:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_cpu_reloc:
>         Subgroup basic:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_cs_tlb:
>         Subgroup basic-default:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_ctx_basic:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_ctx_create:
>         Subgroup basic:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-files:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_ctx_exec:
>         Subgroup basic:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_ctx_param:
>         Subgroup basic:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-default:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_ctx_switch:
>         Subgroup basic-default:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-default-heavy:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_exec_basic:
>         Subgroup basic-blt:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-bsd:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-default:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-render:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup gtt-blt:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup gtt-bsd:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup gtt-default:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup gtt-render:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup readonly-blt:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup readonly-bsd:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup readonly-default:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup readonly-render:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_exec_create:
>         Subgroup basic:
>                 skip       -> PASS       (fi-ivb-3770)
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-cmd:
>                 skip       -> PASS       (fi-ivb-3770)
>         Subgroup basic-batch-kernel-default-uc:
>                 skip       -> PASS       (fi-ivb-3770)
> WARNING: Long output truncated
>
> a16fc233af5c80d1ed23178c94b7fbb77e48ffe0 drm-tip: 2016y-12m-15d-15h-21m-31s UTC integration manifest
> b52a651 drm: Resurrect atomic rmfb code, v2
> fe01be7 drm/i915: Disable all crtcs during driver unload.
> 48ad6e1 drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.
>
> == Logs ==
>
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3295/

I only see module reload inject failing with dev_priv->gt.awake, which is

https://bugs.freedesktop.org/show_bug.cgi?id=98670

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

[PATCH] drm/atomic: Fix outdated comment.

[Maarten Lankhorst]

Op 15-12-16 om 16:19 schreef Daniel Vetter:
> On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
>> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
>> different from commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> I think it'd be good to update the kerneldoc for the atomic_commit
> callback to mention that drivers should grab their own references using
> drm_atomic_state_get() when they need it.
>
> Applied this one meanwhile, thanks.

--->8---
Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
adds reference counting to atomic state, but didn't update the comments
in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.

Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 681d5f97639d..6492546476b4 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
  * more locks but encountered a deadlock. The caller must then do the usual w/w
  * backoff dance and restart. All other errors are fatal.
  *
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * In earlier versions of the atomic api, the caller was handing its reference
+ * of @state over to this function on success. This is no longer the case, and
+ * callers should always call drm_atomic_state_put().
  *
  * Returns:
  * 0 on success, negative error code on failure.
@@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
  * more locks but encountered a deadlock. The caller must then do the usual w/w
  * backoff dance and restart. All other errors are fatal.
  *
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * In earlier versions of the atomic api, the caller was handing its reference
+ * of @state over to this function on success. This is no longer the case, and
+ * callers should always call drm_atomic_state_put().
  *
  * Returns:
  * 0 on success, negative error code on failure.

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

Re: [PATCH] drm/atomic: Fix outdated comment.

[Chris Wilson]

On Wed, Jan 04, 2017 at 12:15:55PM +0100, Maarten Lankhorst wrote:
> Op 15-12-16 om 16:19 schreef Daniel Vetter:
> > On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
> >> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
> >> different from commit.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > I think it'd be good to update the kerneldoc for the atomic_commit
> > callback to mention that drivers should grab their own references using
> > drm_atomic_state_get() when they need it.
> >
> > Applied this one meanwhile, thanks.
> 
> --->8---
> Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> adds reference counting to atomic state, but didn't update the comments
> in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
> 
> Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 681d5f97639d..6492546476b4 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>   * backoff dance and restart. All other errors are fatal.
>   *
> - * Also note that on successful execution ownership of @state is transferred
> - * from the caller of this function to the function itself. The caller must not
> - * free or in any other way access @state. If the function fails then the caller
> - * must clean up @state itself.
> + * In earlier versions of the atomic api, the caller was handing its reference
> + * of @state over to this function on success. This is no longer the case, and
> + * callers should always call drm_atomic_state_put().
>   *
>   * Returns:
>   * 0 on success, negative error code on failure.
> @@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>   * backoff dance and restart. All other errors are fatal.
>   *
> - * Also note that on successful execution ownership of @state is transferred
> - * from the caller of this function to the function itself. The caller must not
> - * free or in any other way access @state. If the function fails then the caller
> - * must clean up @state itself.
> + * In earlier versions of the atomic api, the caller was handing its reference
> + * of @state over to this function on success. This is no longer the case, and
> + * callers should always call drm_atomic_state_put().

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

But do we really want the confusion of mentioning what the api doesn't do
any more?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Fix outdated comment.

[Daniel Vetter]

On Wed, Jan 04, 2017 at 11:22:54AM +0000, Chris Wilson wrote:
> On Wed, Jan 04, 2017 at 12:15:55PM +0100, Maarten Lankhorst wrote:
> > Op 15-12-16 om 16:19 schreef Daniel Vetter:
> > > On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
> > >> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
> > >> different from commit.
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > I think it'd be good to update the kerneldoc for the atomic_commit
> > > callback to mention that drivers should grab their own references using
> > > drm_atomic_state_get() when they need it.
> > >
> > > Applied this one meanwhile, thanks.
> > 
> > --->8---
> > Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> > adds reference counting to atomic state, but didn't update the comments
> > in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
> > 
> > Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 681d5f97639d..6492546476b4 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
> >   * more locks but encountered a deadlock. The caller must then do the usual w/w
> >   * backoff dance and restart. All other errors are fatal.
> >   *
> > - * Also note that on successful execution ownership of @state is transferred
> > - * from the caller of this function to the function itself. The caller must not
> > - * free or in any other way access @state. If the function fails then the caller
> > - * must clean up @state itself.
> > + * In earlier versions of the atomic api, the caller was handing its reference
> > + * of @state over to this function on success. This is no longer the case, and
> > + * callers should always call drm_atomic_state_put().
> >   *
> >   * Returns:
> >   * 0 on success, negative error code on failure.
> > @@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
> >   * more locks but encountered a deadlock. The caller must then do the usual w/w
> >   * backoff dance and restart. All other errors are fatal.
> >   *
> > - * Also note that on successful execution ownership of @state is transferred
> > - * from the caller of this function to the function itself. The caller must not
> > - * free or in any other way access @state. If the function fails then the caller
> > - * must clean up @state itself.
> > + * In earlier versions of the atomic api, the caller was handing its reference
> > + * of @state over to this function on success. This is no longer the case, and
> > + * callers should always call drm_atomic_state_put().
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> But do we really want the confusion of mentioning what the api doesn't do
> any more?

Agreed, kerneldoc should state what is (and why), maybe with a FIXME if it
only makes sense with some hystorical context and what should be done
instead. git blame is for figuring out what was.

Maarten, can you pls respin?
-Daniel
-- 
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

[PATCH v2] drm/atomic: Fix outdated comment.

[Maarten Lankhorst]

Op 04-01-17 om 12:28 schreef Daniel Vetter:
> On Wed, Jan 04, 2017 at 11:22:54AM +0000, Chris Wilson wrote:
>> On Wed, Jan 04, 2017 at 12:15:55PM +0100, Maarten Lankhorst wrote:
>>> Op 15-12-16 om 16:19 schreef Daniel Vetter:
>>>> On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
>>>>> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
>>>>> different from commit.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> I think it'd be good to update the kerneldoc for the atomic_commit
>>>> callback to mention that drivers should grab their own references using
>>>> drm_atomic_state_get() when they need it.
>>>>
>>>> Applied this one meanwhile, thanks.
>>> --->8---
>>> Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
>>> adds reference counting to atomic state, but didn't update the comments
>>> in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
>>>
>>> Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
>>> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 681d5f97639d..6492546476b4 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
>>>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>>>   * backoff dance and restart. All other errors are fatal.
>>>   *
>>> - * Also note that on successful execution ownership of @state is transferred
>>> - * from the caller of this function to the function itself. The caller must not
>>> - * free or in any other way access @state. If the function fails then the caller
>>> - * must clean up @state itself.
>>> + * In earlier versions of the atomic api, the caller was handing its reference
>>> + * of @state over to this function on success. This is no longer the case, and
>>> + * callers should always call drm_atomic_state_put().
>>>   *
>>>   * Returns:
>>>   * 0 on success, negative error code on failure.
>>> @@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
>>>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>>>   * backoff dance and restart. All other errors are fatal.
>>>   *
>>> - * Also note that on successful execution ownership of @state is transferred
>>> - * from the caller of this function to the function itself. The caller must not
>>> - * free or in any other way access @state. If the function fails then the caller
>>> - * must clean up @state itself.
>>> + * In earlier versions of the atomic api, the caller was handing its reference
>>> + * of @state over to this function on success. This is no longer the case, and
>>> + * callers should always call drm_atomic_state_put().
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> But do we really want the confusion of mentioning what the api doesn't do
>> any more?
> Agreed, kerneldoc should state what is (and why), maybe with a FIXME if it
> only makes sense with some hystorical context and what should be done
> instead. git blame is for figuring out what was.
>
> Maarten, can you pls respin?
> -Daniel

----8<---
Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
adds reference counting to atomic state, but didn't update the comments
in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.

Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 681d5f97639d..672f1de84d6c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1599,10 +1599,8 @@ EXPORT_SYMBOL(drm_atomic_check_only);
  * more locks but encountered a deadlock. The caller must then do the usual w/w
  * backoff dance and restart. All other errors are fatal.
  *
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * This function will take its own reference on @state.
+ * Callers should always release their reference with drm_atomic_state_put().
  *
  * Returns:
  * 0 on success, negative error code on failure.
@@ -1630,10 +1628,8 @@ EXPORT_SYMBOL(drm_atomic_commit);
  * more locks but encountered a deadlock. The caller must then do the usual w/w
  * backoff dance and restart. All other errors are fatal.
  *
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * This function will take its own reference on @state.
+ * Callers should always release their reference with drm_atomic_state_put().
  *
  * Returns:
  * 0 on success, negative error code on failure.

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

Re: [PATCH v2] drm/atomic: Fix outdated comment.

[Daniel Vetter]

On Wed, Jan 04, 2017 at 12:34:00PM +0100, Maarten Lankhorst wrote:
> Op 04-01-17 om 12:28 schreef Daniel Vetter:
> > On Wed, Jan 04, 2017 at 11:22:54AM +0000, Chris Wilson wrote:
> >> On Wed, Jan 04, 2017 at 12:15:55PM +0100, Maarten Lankhorst wrote:
> >>> Op 15-12-16 om 16:19 schreef Daniel Vetter:
> >>>> On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
> >>>>> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
> >>>>> different from commit.
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> I think it'd be good to update the kerneldoc for the atomic_commit
> >>>> callback to mention that drivers should grab their own references using
> >>>> drm_atomic_state_get() when they need it.
> >>>>
> >>>> Applied this one meanwhile, thanks.
> >>> --->8---
> >>> Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> >>> adds reference counting to atomic state, but didn't update the comments
> >>> in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
> >>>
> >>> Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> >>> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 681d5f97639d..6492546476b4 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
> >>>   * more locks but encountered a deadlock. The caller must then do the usual w/w
> >>>   * backoff dance and restart. All other errors are fatal.
> >>>   *
> >>> - * Also note that on successful execution ownership of @state is transferred
> >>> - * from the caller of this function to the function itself. The caller must not
> >>> - * free or in any other way access @state. If the function fails then the caller
> >>> - * must clean up @state itself.
> >>> + * In earlier versions of the atomic api, the caller was handing its reference
> >>> + * of @state over to this function on success. This is no longer the case, and
> >>> + * callers should always call drm_atomic_state_put().
> >>>   *
> >>>   * Returns:
> >>>   * 0 on success, negative error code on failure.
> >>> @@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
> >>>   * more locks but encountered a deadlock. The caller must then do the usual w/w
> >>>   * backoff dance and restart. All other errors are fatal.
> >>>   *
> >>> - * Also note that on successful execution ownership of @state is transferred
> >>> - * from the caller of this function to the function itself. The caller must not
> >>> - * free or in any other way access @state. If the function fails then the caller
> >>> - * must clean up @state itself.
> >>> + * In earlier versions of the atomic api, the caller was handing its reference
> >>> + * of @state over to this function on success. This is no longer the case, and
> >>> + * callers should always call drm_atomic_state_put().
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> But do we really want the confusion of mentioning what the api doesn't do
> >> any more?
> > Agreed, kerneldoc should state what is (and why), maybe with a FIXME if it
> > only makes sense with some hystorical context and what should be done
> > instead. git blame is for figuring out what was.
> >
> > Maarten, can you pls respin?
> > -Daniel
> 
> ----8<---
> Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> adds reference counting to atomic state, but didn't update the comments
> in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
> 
> Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Applied to drm-misc, thanks.
-Daniel

> ---
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 681d5f97639d..672f1de84d6c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1599,10 +1599,8 @@ EXPORT_SYMBOL(drm_atomic_check_only);
>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>   * backoff dance and restart. All other errors are fatal.
>   *
> - * Also note that on successful execution ownership of @state is transferred
> - * from the caller of this function to the function itself. The caller must not
> - * free or in any other way access @state. If the function fails then the caller
> - * must clean up @state itself.
> + * This function will take its own reference on @state.
> + * Callers should always release their reference with drm_atomic_state_put().
>   *
>   * Returns:
>   * 0 on success, negative error code on failure.
> @@ -1630,10 +1628,8 @@ EXPORT_SYMBOL(drm_atomic_commit);
>   * more locks but encountered a deadlock. The caller must then do the usual w/w
>   * backoff dance and restart. All other errors are fatal.
>   *
> - * Also note that on successful execution ownership of @state is transferred
> - * from the caller of this function to the function itself. The caller must not
> - * free or in any other way access @state. If the function fails then the caller
> - * must clean up @state itself.
> + * This function will take its own reference on @state.
> + * Callers should always release their reference with drm_atomic_state_put().
>   *
>   * Returns:
>   * 0 on success, negative error code on failure.
> 

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

Re: [PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Daniel Vetter]

On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
> If the crtc was brought up with audio before the driver loads,
> then crtc_disable will remove a refcount to audio that doesn't exist
> before.
> 
> Fortunately we already set power domains on readout, so we can just add
> the power domain handling to get_crtc_power_domains, which will update
> the power domains correctly in all cases.
> 
> This was found when testing module reload on CI with the crtc enabled,
> which resulted in the following warn after module reload + modeset:
> 
> [   24.197041] ------------[ cut here ]------------
> [   24.197075] WARNING: CPU: 0 PID: 99 at drivers/gpu/drm/i915/intel_runtime_pm.c:1790 intel_display_power_put+0x134/0x140 [i915]
> [   24.197076] Use count on domain AUDIO is already zero
> [   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 4.9.0-CI-Trybot_393+ #1
> [   24.197099] Hardware name:                  /NUC6i5SYB, BIOS SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
> [   24.197102] Workqueue: events_unbound async_run_entry_fn
> [   24.197105]  ffffc900003c7688 ffffffff81435b35 ffffc900003c76d8 0000000000000000
> [   24.197107]  ffffc900003c76c8 ffffffff8107e4d6 000006fe5dc36f28 ffff88025dc30054
> [   24.197109]  ffff88025dc36f28 ffff88025dc30000 ffff88025dc30000 0000000000000015
> [   24.197110] Call Trace:
> [   24.197113]  [<ffffffff81435b35>] dump_stack+0x67/0x92
> [   24.197116]  [<ffffffff8107e4d6>] __warn+0xc6/0xe0
> [   24.197118]  [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
> [   24.197149]  [<ffffffffa039b4b4>] intel_display_power_put+0x134/0x140 [i915]
> [   24.197187]  [<ffffffffa04217dd>] intel_disable_ddi+0x4d/0x80 [i915]
> [   24.197223]  [<ffffffffa03f388f>] intel_encoders_disable.isra.74+0x7f/0x90 [i915]
> [   24.197257]  [<ffffffffa03f6c05>] haswell_crtc_disable+0x55/0x170 [i915]
> [   24.197292]  [<ffffffffa03fec88>] intel_atomic_commit_tail+0x108/0xfd0 [i915]
> [   24.197295]  [<ffffffff810d47c6>] ? __lock_is_held+0x66/0x90
> [   24.197330]  [<ffffffffa03fff79>] intel_atomic_commit+0x429/0x560 [i915]
> [   24.197332]  [<ffffffff81570186>] ?drm_atomic_add_affected_connectors+0x56/0xf0
> [   24.197334]  [<ffffffff8156f726>] drm_atomic_commit+0x46/0x50
> [   24.197336]  [<ffffffff81553f87>] restore_fbdev_mode+0x147/0x270
> [   24.197337]  [<ffffffff81555bee>] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
> [   24.197339]  [<ffffffff81555aa8>] drm_fb_helper_set_par+0x28/0x50
> [   24.197374]  [<ffffffffa041c7d3>] intel_fbdev_set_par+0x13/0x70 [i915]
> [   24.197376]  [<ffffffff8149e07a>] fbcon_init+0x57a/0x600
> [   24.197379]  [<ffffffff81514b71>] visual_init+0xd1/0x130
> [   24.197381]  [<ffffffff8151603c>] do_bind_con_driver+0x1bc/0x3a0
> [   24.197384]  [<ffffffff81516521>] do_take_over_console+0x111/0x180
> [   24.197386]  [<ffffffff8149e152>] do_fbcon_takeover+0x52/0xb0
> [   24.197387]  [<ffffffff814a12c3>] fbcon_event_notify+0x723/0x850
> [   24.197390]  [<ffffffff810a4830>] ?__blocking_notifier_call_chain+0x30/0x70
> [   24.197392]  [<ffffffff810a44a4>] notifier_call_chain+0x34/0xa0
> [   24.197394]  [<ffffffff810a4848>] __blocking_notifier_call_chain+0x48/0x70
> [   24.197397]  [<ffffffff810a4881>] blocking_notifier_call_chain+0x11/0x20
> [   24.197398]  [<ffffffff814a4556>] fb_notifier_call_chain+0x16/0x20
> [   24.197400]  [<ffffffff814a678c>] register_framebuffer+0x24c/0x330
> [   24.197402]  [<ffffffff815558d9>] drm_fb_helper_initial_config+0x219/0x3c0
> [   24.197436]  [<ffffffffa041d373>] intel_fbdev_initial_config+0x13/0x30 [i915]
> [   24.197438]  [<ffffffff810a5d44>] async_run_entry_fn+0x34/0x140
> [   24.197440]  [<ffffffff8109c26c>] process_one_work+0x1ec/0x6b0
> [   24.197442]  [<ffffffff8109c1e6>] ? process_one_work+0x166/0x6b0
> [   24.197445]  [<ffffffff8109c779>] worker_thread+0x49/0x490
> [   24.197447]  [<ffffffff8109c730>] ? process_one_work+0x6b0/0x6b0
> [   24.197448]  [<ffffffff810a2a9b>] kthread+0xeb/0x110
> [   24.197451]  [<ffffffff810a29b0>] ? kthread_park+0x60/0x60
> [   24.197453]  [<ffffffff818241a7>] ret_from_fork+0x27/0x40
> [   24.197476] ---[ end trace bda64b683b8e8162 ]---
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Do we still need this with patch 3? I know it'd be nice if we could
faithfully restore any state we can also program, but then that's also a
lot of complexity ...

Otoh patch 3 means we'll stop testing a lot of the fastboot code while
reloading the driver. But then that's been the thing in the past, and as
long as we still boot up we have at least some test coverage fo the
fastboot code (I'm mostly concerned about the plane/buffer readout code,
since that's not covered by the state checker).

But for now I'd say let's just go with patch 3 only.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 14 ++------------
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  |  9 ++-------
>  3 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index d808a2ccc29e..8c9ce850760b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1835,8 +1835,6 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  			     struct drm_connector_state *conn_state)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> -	struct drm_crtc *crtc = encoder->crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
>  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
> @@ -1863,10 +1861,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  		intel_edp_drrs_enable(intel_dp, pipe_config);
>  	}
>  
> -	if (intel_crtc->config->has_audio) {
> -		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +	if (pipe_config->has_audio)
>  		intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
> -	}
>  }
>  
>  static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> @@ -1874,16 +1870,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>  			      struct drm_connector_state *old_conn_state)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> -	struct drm_crtc *crtc = encoder->crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int type = intel_encoder->type;
> -	struct drm_device *dev = encoder->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	if (intel_crtc->config->has_audio) {
> +	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(intel_encoder);
> -		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> -	}
>  
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac587ca3b2af..0daa54633450 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5735,6 +5735,7 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
>  					    struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_encoder *encoder;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> @@ -5756,6 +5757,9 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
>  		mask |= BIT(intel_display_port_power_domain(intel_encoder));
>  	}
>  
> +	if (HAS_DDI(dev_priv) && crtc_state->has_audio)
> +		mask |= BIT(POWER_DOMAIN_AUDIO);
> +
>  	if (crtc_state->shared_dpll)
>  		mask |= BIT(POWER_DOMAIN_PLLS);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 205fe4748ec5..6c5382494fee 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -87,7 +87,6 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct intel_connector *connector =
>  		to_intel_connector(old_conn_state->connector);
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	int ret;
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> @@ -98,10 +97,8 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
>  	if (ret) {
>  		DRM_ERROR("failed to update payload %d\n", ret);
>  	}
> -	if (old_crtc_state->has_audio) {
> +	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder);
> -		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> -	}
>  }
>  
>  static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> @@ -214,10 +211,8 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
>  	ret = drm_dp_check_act_status(&intel_dp->mst_mgr);
>  
>  	ret = drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> -	if (pipe_config->has_audio) {
> -		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> +	if (pipe_config->has_audio)
>  		intel_audio_codec_enable(encoder, pipe_config, conn_state);
> -	}
>  }
>  
>  static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/4] drm/i915: Disable all crtcs during driver unload.

[Daniel Vetter]

On Thu, Dec 15, 2016 at 03:29:44PM +0100, Maarten Lankhorst wrote:
> We may keep the crtc's enabled when userspace unsets all framebuffers but
> keeps the crtc active. This exposes a WARN in fbc_global disable, and
> a lot of bugs in our hardware readout code. Solve this by disabling
> all crtc's for now.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6428588518aa..bb0d7517b678 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -43,6 +43,7 @@
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/i915_drm.h>
>  
>  #include "i915_drv.h"
> @@ -1282,6 +1283,10 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  
> +	drm_modeset_lock_all(dev);
> +	drm_atomic_helper_disable_all(dev, dev->mode_config.acquire_ctx);
> +	drm_modeset_unlock_all(dev);

Bikeshed: I think we should phase out lock_all and do an explicit acquire
context here. And maybe get a bit better at refactoring the boilerplate
that brings along. But also as-is:

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

> +
>  	i915_driver_unregister(dev_priv);
>  
>  	drm_vblank_cleanup(dev);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Daniel Vetter]

On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> This was somehow lost between v3 and the merged version in Maarten's
> patch merged as:
> 
> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Wed May 4 14:38:26 2016 +0200
> 
>     drm/core: Do not preserve framebuffer on rmfb, v4.
> 
> Actual code copied from Maarten's patch, but with the slight change to
> just use dev->mode_config.funcs->atomic_commit to decide whether to
> use the atomic path or not.
> 
> 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.
> 
> 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>

Would be great if someone else could r-b this, I've proven pretty well
that I don't understand the complexity here :(
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        | 64 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |  1 +
>  drivers/gpu/drm/drm_framebuffer.c   |  7 ++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d1d252261bf1..23a3845542e1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2059,6 +2059,70 @@ 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;
> +	int 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;
> +		}
> +
> +		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;
> +	}
> +
> +	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 cdf6860c9d22..121e250853d2 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -178,6 +178,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 cbf0c893f426..c358bf8280a8 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in this manner.
>  	 */
>  	if (drm_framebuffer_read_refcount(fb) > 1) {
> +		if (dev->mode_config.funcs->atomic_commit) {
> +			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) {
> @@ -787,6 +793,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
> 

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

Re: [PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Maarten Lankhorst]

Op 11-01-17 om 17:13 schreef Daniel Vetter:
> On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
>> If the crtc was brought up with audio before the driver loads,
>> then crtc_disable will remove a refcount to audio that doesn't exist
>> before.
>>
>> Fortunately we already set power domains on readout, so we can just add
>> the power domain handling to get_crtc_power_domains, which will update
>> the power domains correctly in all cases.
>>
>> This was found when testing module reload on CI with the crtc enabled,
>> which resulted in the following warn after module reload + modeset:
>>
>> [   24.197041] ------------[ cut here ]------------
>> [   24.197075] WARNING: CPU: 0 PID: 99 at drivers/gpu/drm/i915/intel_runtime_pm.c:1790 intel_display_power_put+0x134/0x140 [i915]
>> [   24.197076] Use count on domain AUDIO is already zero
>> [   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 4.9.0-CI-Trybot_393+ #1
>> [   24.197099] Hardware name:                  /NUC6i5SYB, BIOS SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
>> [   24.197102] Workqueue: events_unbound async_run_entry_fn
>> [   24.197105]  ffffc900003c7688 ffffffff81435b35 ffffc900003c76d8 0000000000000000
>> [   24.197107]  ffffc900003c76c8 ffffffff8107e4d6 000006fe5dc36f28 ffff88025dc30054
>> [   24.197109]  ffff88025dc36f28 ffff88025dc30000 ffff88025dc30000 0000000000000015
>> [   24.197110] Call Trace:
>> [   24.197113]  [<ffffffff81435b35>] dump_stack+0x67/0x92
>> [   24.197116]  [<ffffffff8107e4d6>] __warn+0xc6/0xe0
>> [   24.197118]  [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
>> [   24.197149]  [<ffffffffa039b4b4>] intel_display_power_put+0x134/0x140 [i915]
>> [   24.197187]  [<ffffffffa04217dd>] intel_disable_ddi+0x4d/0x80 [i915]
>> [   24.197223]  [<ffffffffa03f388f>] intel_encoders_disable.isra.74+0x7f/0x90 [i915]
>> [   24.197257]  [<ffffffffa03f6c05>] haswell_crtc_disable+0x55/0x170 [i915]
>> [   24.197292]  [<ffffffffa03fec88>] intel_atomic_commit_tail+0x108/0xfd0 [i915]
>> [   24.197295]  [<ffffffff810d47c6>] ? __lock_is_held+0x66/0x90
>> [   24.197330]  [<ffffffffa03fff79>] intel_atomic_commit+0x429/0x560 [i915]
>> [   24.197332]  [<ffffffff81570186>] ?drm_atomic_add_affected_connectors+0x56/0xf0
>> [   24.197334]  [<ffffffff8156f726>] drm_atomic_commit+0x46/0x50
>> [   24.197336]  [<ffffffff81553f87>] restore_fbdev_mode+0x147/0x270
>> [   24.197337]  [<ffffffff81555bee>] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
>> [   24.197339]  [<ffffffff81555aa8>] drm_fb_helper_set_par+0x28/0x50
>> [   24.197374]  [<ffffffffa041c7d3>] intel_fbdev_set_par+0x13/0x70 [i915]
>> [   24.197376]  [<ffffffff8149e07a>] fbcon_init+0x57a/0x600
>> [   24.197379]  [<ffffffff81514b71>] visual_init+0xd1/0x130
>> [   24.197381]  [<ffffffff8151603c>] do_bind_con_driver+0x1bc/0x3a0
>> [   24.197384]  [<ffffffff81516521>] do_take_over_console+0x111/0x180
>> [   24.197386]  [<ffffffff8149e152>] do_fbcon_takeover+0x52/0xb0
>> [   24.197387]  [<ffffffff814a12c3>] fbcon_event_notify+0x723/0x850
>> [   24.197390]  [<ffffffff810a4830>] ?__blocking_notifier_call_chain+0x30/0x70
>> [   24.197392]  [<ffffffff810a44a4>] notifier_call_chain+0x34/0xa0
>> [   24.197394]  [<ffffffff810a4848>] __blocking_notifier_call_chain+0x48/0x70
>> [   24.197397]  [<ffffffff810a4881>] blocking_notifier_call_chain+0x11/0x20
>> [   24.197398]  [<ffffffff814a4556>] fb_notifier_call_chain+0x16/0x20
>> [   24.197400]  [<ffffffff814a678c>] register_framebuffer+0x24c/0x330
>> [   24.197402]  [<ffffffff815558d9>] drm_fb_helper_initial_config+0x219/0x3c0
>> [   24.197436]  [<ffffffffa041d373>] intel_fbdev_initial_config+0x13/0x30 [i915]
>> [   24.197438]  [<ffffffff810a5d44>] async_run_entry_fn+0x34/0x140
>> [   24.197440]  [<ffffffff8109c26c>] process_one_work+0x1ec/0x6b0
>> [   24.197442]  [<ffffffff8109c1e6>] ? process_one_work+0x166/0x6b0
>> [   24.197445]  [<ffffffff8109c779>] worker_thread+0x49/0x490
>> [   24.197447]  [<ffffffff8109c730>] ? process_one_work+0x6b0/0x6b0
>> [   24.197448]  [<ffffffff810a2a9b>] kthread+0xeb/0x110
>> [   24.197451]  [<ffffffff810a29b0>] ? kthread_park+0x60/0x60
>> [   24.197453]  [<ffffffff818241a7>] ret_from_fork+0x27/0x40
>> [   24.197476] ---[ end trace bda64b683b8e8162 ]---
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Do we still need this with patch 3? I know it'd be nice if we could
> faithfully restore any state we can also program, but then that's also a
> lot of complexity ...
>
> Otoh patch 3 means we'll stop testing a lot of the fastboot code while
> reloading the driver. But then that's been the thing in the past, and as
> long as we still boot up we have at least some test coverage fo the
> fastboot code (I'm mostly concerned about the plane/buffer readout code,
> since that's not covered by the state checker).
>
> But for now I'd say let's just go with patch 3 only.
We don't need this patch, but it makes the audio power domain tracked like all other power domains. It's a nice cleanup in general and I would like it even without the rest of the series. Could otherwise be merged through the intel tree through.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.

[Daniel Vetter]

On Mon, Jan 16, 2017 at 12:00:46PM +0100, Maarten Lankhorst wrote:
> Op 11-01-17 om 17:13 schreef Daniel Vetter:
> > On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
> >> If the crtc was brought up with audio before the driver loads,
> >> then crtc_disable will remove a refcount to audio that doesn't exist
> >> before.
> >>
> >> Fortunately we already set power domains on readout, so we can just add
> >> the power domain handling to get_crtc_power_domains, which will update
> >> the power domains correctly in all cases.
> >>
> >> This was found when testing module reload on CI with the crtc enabled,
> >> which resulted in the following warn after module reload + modeset:
> >>
> >> [   24.197041] ------------[ cut here ]------------
> >> [   24.197075] WARNING: CPU: 0 PID: 99 at drivers/gpu/drm/i915/intel_runtime_pm.c:1790 intel_display_power_put+0x134/0x140 [i915]
> >> [   24.197076] Use count on domain AUDIO is already zero
> >> [   24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 4.9.0-CI-Trybot_393+ #1
> >> [   24.197099] Hardware name:                  /NUC6i5SYB, BIOS SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
> >> [   24.197102] Workqueue: events_unbound async_run_entry_fn
> >> [   24.197105]  ffffc900003c7688 ffffffff81435b35 ffffc900003c76d8 0000000000000000
> >> [   24.197107]  ffffc900003c76c8 ffffffff8107e4d6 000006fe5dc36f28 ffff88025dc30054
> >> [   24.197109]  ffff88025dc36f28 ffff88025dc30000 ffff88025dc30000 0000000000000015
> >> [   24.197110] Call Trace:
> >> [   24.197113]  [<ffffffff81435b35>] dump_stack+0x67/0x92
> >> [   24.197116]  [<ffffffff8107e4d6>] __warn+0xc6/0xe0
> >> [   24.197118]  [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
> >> [   24.197149]  [<ffffffffa039b4b4>] intel_display_power_put+0x134/0x140 [i915]
> >> [   24.197187]  [<ffffffffa04217dd>] intel_disable_ddi+0x4d/0x80 [i915]
> >> [   24.197223]  [<ffffffffa03f388f>] intel_encoders_disable.isra.74+0x7f/0x90 [i915]
> >> [   24.197257]  [<ffffffffa03f6c05>] haswell_crtc_disable+0x55/0x170 [i915]
> >> [   24.197292]  [<ffffffffa03fec88>] intel_atomic_commit_tail+0x108/0xfd0 [i915]
> >> [   24.197295]  [<ffffffff810d47c6>] ? __lock_is_held+0x66/0x90
> >> [   24.197330]  [<ffffffffa03fff79>] intel_atomic_commit+0x429/0x560 [i915]
> >> [   24.197332]  [<ffffffff81570186>] ?drm_atomic_add_affected_connectors+0x56/0xf0
> >> [   24.197334]  [<ffffffff8156f726>] drm_atomic_commit+0x46/0x50
> >> [   24.197336]  [<ffffffff81553f87>] restore_fbdev_mode+0x147/0x270
> >> [   24.197337]  [<ffffffff81555bee>] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
> >> [   24.197339]  [<ffffffff81555aa8>] drm_fb_helper_set_par+0x28/0x50
> >> [   24.197374]  [<ffffffffa041c7d3>] intel_fbdev_set_par+0x13/0x70 [i915]
> >> [   24.197376]  [<ffffffff8149e07a>] fbcon_init+0x57a/0x600
> >> [   24.197379]  [<ffffffff81514b71>] visual_init+0xd1/0x130
> >> [   24.197381]  [<ffffffff8151603c>] do_bind_con_driver+0x1bc/0x3a0
> >> [   24.197384]  [<ffffffff81516521>] do_take_over_console+0x111/0x180
> >> [   24.197386]  [<ffffffff8149e152>] do_fbcon_takeover+0x52/0xb0
> >> [   24.197387]  [<ffffffff814a12c3>] fbcon_event_notify+0x723/0x850
> >> [   24.197390]  [<ffffffff810a4830>] ?__blocking_notifier_call_chain+0x30/0x70
> >> [   24.197392]  [<ffffffff810a44a4>] notifier_call_chain+0x34/0xa0
> >> [   24.197394]  [<ffffffff810a4848>] __blocking_notifier_call_chain+0x48/0x70
> >> [   24.197397]  [<ffffffff810a4881>] blocking_notifier_call_chain+0x11/0x20
> >> [   24.197398]  [<ffffffff814a4556>] fb_notifier_call_chain+0x16/0x20
> >> [   24.197400]  [<ffffffff814a678c>] register_framebuffer+0x24c/0x330
> >> [   24.197402]  [<ffffffff815558d9>] drm_fb_helper_initial_config+0x219/0x3c0
> >> [   24.197436]  [<ffffffffa041d373>] intel_fbdev_initial_config+0x13/0x30 [i915]
> >> [   24.197438]  [<ffffffff810a5d44>] async_run_entry_fn+0x34/0x140
> >> [   24.197440]  [<ffffffff8109c26c>] process_one_work+0x1ec/0x6b0
> >> [   24.197442]  [<ffffffff8109c1e6>] ? process_one_work+0x166/0x6b0
> >> [   24.197445]  [<ffffffff8109c779>] worker_thread+0x49/0x490
> >> [   24.197447]  [<ffffffff8109c730>] ? process_one_work+0x6b0/0x6b0
> >> [   24.197448]  [<ffffffff810a2a9b>] kthread+0xeb/0x110
> >> [   24.197451]  [<ffffffff810a29b0>] ? kthread_park+0x60/0x60
> >> [   24.197453]  [<ffffffff818241a7>] ret_from_fork+0x27/0x40
> >> [   24.197476] ---[ end trace bda64b683b8e8162 ]---
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Do we still need this with patch 3? I know it'd be nice if we could
> > faithfully restore any state we can also program, but then that's also a
> > lot of complexity ...
> >
> > Otoh patch 3 means we'll stop testing a lot of the fastboot code while
> > reloading the driver. But then that's been the thing in the past, and as
> > long as we still boot up we have at least some test coverage fo the
> > fastboot code (I'm mostly concerned about the plane/buffer readout code,
> > since that's not covered by the state checker).
> >
> > But for now I'd say let's just go with patch 3 only.
> We don't need this patch, but it makes the audio power domain tracked
> like all other power domains. It's a nice cleanup in general and I would
> like it even without the rest of the series. Could otherwise be merged
> through the intel tree through.

Hm, count me convinced.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Matt Roper]

On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > This was somehow lost between v3 and the merged version in Maarten's
> > patch merged as:
> > 
> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
> > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Date:   Wed May 4 14:38:26 2016 +0200
> > 
> >     drm/core: Do not preserve framebuffer on rmfb, v4.
> > 
> > Actual code copied from Maarten's patch, but with the slight change to
> > just use dev->mode_config.funcs->atomic_commit to decide whether to
> > use the atomic path or not.
> > 
> > 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.
> > 
> > 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>
> 
> Would be great if someone else could r-b this, I've proven pretty well
> that I don't understand the complexity here :(
> -Daniel

It looks like this will change the behavior slightly in that rmfb will
cause primary planes to be disabled, but no longer cause the entire CRTC
to be turned off.  You'll probably want to note that in the commit
message, along with the justification on why this is okay ABI-wise.

I know that 13803132818c ("drm/core: Preserve the framebuffer after
removing it.") was initially trying to not only leave the CRTC on, but
also preserve the framebuffer and leave the planes on; that wound up
causing some kind of regression for vmwgfx, but I'm unclear on the
details there.  I'd suggest getting an Ack from one of the vmware guys
to ensure that the less drastic change in behavior here won't cause them
any problems.

Your code looks correct to me, so with an expanded commit message,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

General observation; we specifically test for the presence of
mode_config->funcs.atomic_commit here rather than DRIVER_ATOMIC because
we care about a driver's internal atomic plumbing rather than whether it
exposes atomic to userspace or not.  I can see that causing confusion in
the future, so I wonder if adding macros like
DRM_SUPPORTS_ATOMIC_INTERNAL(dev) vs DRM_SUPPORTS_ATOMIC_USERSPACE(dev)
might help overclarify exactly what/why we're testing in various places
in the driver.


Matt

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 64 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_crtc_internal.h |  1 +
> >  drivers/gpu/drm/drm_framebuffer.c   |  7 ++++
> >  3 files changed, 72 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index d1d252261bf1..23a3845542e1 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -2059,6 +2059,70 @@ 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;
> > +	int 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;
> > +		}
> > +
> > +		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;
> > +	}
> > +
> > +	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 cdf6860c9d22..121e250853d2 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -178,6 +178,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 cbf0c893f426..c358bf8280a8 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >  	 * in this manner.
> >  	 */
> >  	if (drm_framebuffer_read_refcount(fb) > 1) {
> > +		if (dev->mode_config.funcs->atomic_commit) {
> > +			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) {
> > @@ -787,6 +793,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
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Daniel Vetter]

On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
> > On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > This was somehow lost between v3 and the merged version in Maarten's
> > > patch merged as:
> > > 
> > > commit f2d580b9a8149735cbc4b59c4a8df60173658140
> > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Date:   Wed May 4 14:38:26 2016 +0200
> > > 
> > >     drm/core: Do not preserve framebuffer on rmfb, v4.
> > > 
> > > Actual code copied from Maarten's patch, but with the slight change to
> > > just use dev->mode_config.funcs->atomic_commit to decide whether to
> > > use the atomic path or not.
> > > 
> > > 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.
> > > 
> > > 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>
> > 
> > Would be great if someone else could r-b this, I've proven pretty well
> > that I don't understand the complexity here :(
> > -Daniel
> 
> It looks like this will change the behavior slightly in that rmfb will
> cause primary planes to be disabled, but no longer cause the entire CRTC
> to be turned off.  You'll probably want to note that in the commit
> message, along with the justification on why this is okay ABI-wise.
> 
> I know that 13803132818c ("drm/core: Preserve the framebuffer after
> removing it.") was initially trying to not only leave the CRTC on, but
> also preserve the framebuffer and leave the planes on; that wound up
> causing some kind of regression for vmwgfx, but I'm unclear on the
> details there.  I'd suggest getting an Ack from one of the vmware guys
> to ensure that the less drastic change in behavior here won't cause them
> any problems.

Hm, this is a good point since with some atomic drivers (the ones using
the simple pipe helpers at least) disabling the primary plane alone
doesn't work. We might need a fallback path if the atomic_commit fails,
which also disables the crtc on top. I guess that needs to be address?

> Your code looks correct to me, so with an expanded commit message,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> General observation; we specifically test for the presence of
> mode_config->funcs.atomic_commit here rather than DRIVER_ATOMIC because
> we care about a driver's internal atomic plumbing rather than whether it
> exposes atomic to userspace or not.  I can see that causing confusion in
> the future, so I wonder if adding macros like
> DRM_SUPPORTS_ATOMIC_INTERNAL(dev) vs DRM_SUPPORTS_ATOMIC_USERSPACE(dev)
> might help overclarify exactly what/why we're testing in various places
> in the driver.

This already exists and is called drm_drv_uses_atomic_modeset(). Maarten,
can you pls update the patch?
-Daniel

> 
> 
> Matt
> 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c        | 64 +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_crtc_internal.h |  1 +
> > >  drivers/gpu/drm/drm_framebuffer.c   |  7 ++++
> > >  3 files changed, 72 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index d1d252261bf1..23a3845542e1 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -2059,6 +2059,70 @@ 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;
> > > +	int 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;
> > > +		}
> > > +
> > > +		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;
> > > +	}
> > > +
> > > +	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 cdf6860c9d22..121e250853d2 100644
> > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > @@ -178,6 +178,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 cbf0c893f426..c358bf8280a8 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> > >  	 * in this manner.
> > >  	 */
> > >  	if (drm_framebuffer_read_refcount(fb) > 1) {
> > > +		if (dev->mode_config.funcs->atomic_commit) {
> > > +			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) {
> > > @@ -787,6 +793,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
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Thomas Hellstrom]

On 01/25/2017 05:54 AM, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>> patch merged as:
>>>>
>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>
>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>
>>>> Actual code copied from Maarten's patch, but with the slight change to
>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
>>>> use the atomic path or not.
>>>>
>>>> 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.
>>>>
>>>> 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>
>>> Would be great if someone else could r-b this, I've proven pretty well
>>> that I don't understand the complexity here :(
>>> -Daniel
>> It looks like this will change the behavior slightly in that rmfb will
>> cause primary planes to be disabled, but no longer cause the entire CRTC
>> to be turned off.  You'll probably want to note that in the commit
>> message, along with the justification on why this is okay ABI-wise.
>>
>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>> removing it.") was initially trying to not only leave the CRTC on, but
>> also preserve the framebuffer and leave the planes on; that wound up
>> causing some kind of regression for vmwgfx, but I'm unclear on the
>> details there.  I'd suggest getting an Ack from one of the vmware guys
>> to ensure that the less drastic change in behavior here won't cause them
>> any problems.
The vmware Xorg driver is currently relying on rmfb to turn all attached
crtcs off. Even if we were to fix that in the Xorg driver now, older
Xorgs with newer kernels still would break.

/Thomas


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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Maarten Lankhorst]

Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
> On 01/25/2017 05:54 AM, Daniel Vetter wrote:
>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>
>>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>>> patch merged as:
>>>>>
>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>>
>>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>>
>>>>> Actual code copied from Maarten's patch, but with the slight change to
>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
>>>>> use the atomic path or not.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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>
>>>> Would be great if someone else could r-b this, I've proven pretty well
>>>> that I don't understand the complexity here :(
>>>> -Daniel
>>> It looks like this will change the behavior slightly in that rmfb will
>>> cause primary planes to be disabled, but no longer cause the entire CRTC
>>> to be turned off.  You'll probably want to note that in the commit
>>> message, along with the justification on why this is okay ABI-wise.
>>>
>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>>> removing it.") was initially trying to not only leave the CRTC on, but
>>> also preserve the framebuffer and leave the planes on; that wound up
>>> causing some kind of regression for vmwgfx, but I'm unclear on the
>>> details there.  I'd suggest getting an Ack from one of the vmware guys
>>> to ensure that the less drastic change in behavior here won't cause them
>>> any problems.
> The vmware Xorg driver is currently relying on rmfb to turn all attached
> crtcs off. Even if we were to fix that in the Xorg driver now, older
> Xorgs with newer kernels still would break.
Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?

If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.
However for right now vmwgfx uses the legacy rmfb, which does disable all crtc's. :)

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

[PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Maarten Lankhorst]

This was somehow lost between v3 and the merged version in Maarten's
patch merged as:

commit f2d580b9a8149735cbc4b59c4a8df60173658140
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Wed May 4 14:38:26 2016 +0200

    drm/core: Do not preserve framebuffer on rmfb, v4.

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.

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

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.

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        | 106 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |   1 +
 drivers/gpu/drm/drm_framebuffer.c   |   7 +++
 3 files changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 723392fc98c8..c79ab8048435 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2058,6 +2058,112 @@ 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, disable_crtcs = false;
+
+	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;
+		}
+
+		/*
+		 * 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);
+
+			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;
+	}
+
+	/* 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);
+
+		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);
+
+	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);
+
+	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 724c329186d5..d98a683d31d0 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -184,6 +184,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 588ccc3a2218..357b521bbdc3 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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Sinclair Yeh]

On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote:
> Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
> > On 01/25/2017 05:54 AM, Daniel Vetter wrote:
> >> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
> >>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
> >>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> >>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>
> >>>>> This was somehow lost between v3 and the merged version in Maarten's
> >>>>> patch merged as:
> >>>>>
> >>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> >>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Date:   Wed May 4 14:38:26 2016 +0200
> >>>>>
> >>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
> >>>>>
> >>>>> Actual code copied from Maarten's patch, but with the slight change to
> >>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
> >>>>> use the atomic path or not.
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> 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>
> >>>> Would be great if someone else could r-b this, I've proven pretty well
> >>>> that I don't understand the complexity here :(
> >>>> -Daniel
> >>> It looks like this will change the behavior slightly in that rmfb will
> >>> cause primary planes to be disabled, but no longer cause the entire CRTC
> >>> to be turned off.  You'll probably want to note that in the commit
> >>> message, along with the justification on why this is okay ABI-wise.
> >>>
> >>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
> >>> removing it.") was initially trying to not only leave the CRTC on, but
> >>> also preserve the framebuffer and leave the planes on; that wound up
> >>> causing some kind of regression for vmwgfx, but I'm unclear on the
> >>> details there.  I'd suggest getting an Ack from one of the vmware guys
> >>> to ensure that the less drastic change in behavior here won't cause them
> >>> any problems.
> > The vmware Xorg driver is currently relying on rmfb to turn all attached
> > crtcs off. Even if we were to fix that in the Xorg driver now, older
> > Xorgs with newer kernels still would break.
> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?
> 
> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.

FYI, we are in the process of converting things to atomic.  This may happen
around 4.12

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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Maarten Lankhorst]

Op 25-01-17 om 19:05 schreef Sinclair Yeh:
> On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote:
>> Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
>>> On 01/25/2017 05:54 AM, Daniel Vetter wrote:
>>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
>>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>
>>>>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>>>>> patch merged as:
>>>>>>>
>>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>>>>
>>>>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>>>>
>>>>>>> Actual code copied from Maarten's patch, but with the slight change to
>>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
>>>>>>> use the atomic path or not.
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> 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>
>>>>>> Would be great if someone else could r-b this, I've proven pretty well
>>>>>> that I don't understand the complexity here :(
>>>>>> -Daniel
>>>>> It looks like this will change the behavior slightly in that rmfb will
>>>>> cause primary planes to be disabled, but no longer cause the entire CRTC
>>>>> to be turned off.  You'll probably want to note that in the commit
>>>>> message, along with the justification on why this is okay ABI-wise.
>>>>>
>>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>>>>> removing it.") was initially trying to not only leave the CRTC on, but
>>>>> also preserve the framebuffer and leave the planes on; that wound up
>>>>> causing some kind of regression for vmwgfx, but I'm unclear on the
>>>>> details there.  I'd suggest getting an Ack from one of the vmware guys
>>>>> to ensure that the less drastic change in behavior here won't cause them
>>>>> any problems.
>>> The vmware Xorg driver is currently relying on rmfb to turn all attached
>>> crtcs off. Even if we were to fix that in the Xorg driver now, older
>>> Xorgs with newer kernels still would break.
>> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?
>>
>> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.
> FYI, we are in the process of converting things to atomic.  This may happen
> around 4.12
>
Will the driver allow the crtc to be enabled without primary plane?

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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Sinclair Yeh]

On Thu, Jan 26, 2017 at 10:55:51AM +0100, Maarten Lankhorst wrote:
> Op 25-01-17 om 19:05 schreef Sinclair Yeh:
> > On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote:
> >> Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
> >>> On 01/25/2017 05:54 AM, Daniel Vetter wrote:
> >>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
> >>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
> >>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
> >>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>>>
> >>>>>>> This was somehow lost between v3 and the merged version in Maarten's
> >>>>>>> patch merged as:
> >>>>>>>
> >>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> >>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>>> Date:   Wed May 4 14:38:26 2016 +0200
> >>>>>>>
> >>>>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
> >>>>>>>
> >>>>>>> Actual code copied from Maarten's patch, but with the slight change to
> >>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
> >>>>>>> use the atomic path or not.
> >>>>>>>
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> 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>
> >>>>>> Would be great if someone else could r-b this, I've proven pretty well
> >>>>>> that I don't understand the complexity here :(
> >>>>>> -Daniel
> >>>>> It looks like this will change the behavior slightly in that rmfb will
> >>>>> cause primary planes to be disabled, but no longer cause the entire CRTC
> >>>>> to be turned off.  You'll probably want to note that in the commit
> >>>>> message, along with the justification on why this is okay ABI-wise.
> >>>>>
> >>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
> >>>>> removing it.") was initially trying to not only leave the CRTC on, but
> >>>>> also preserve the framebuffer and leave the planes on; that wound up
> >>>>> causing some kind of regression for vmwgfx, but I'm unclear on the
> >>>>> details there.  I'd suggest getting an Ack from one of the vmware guys
> >>>>> to ensure that the less drastic change in behavior here won't cause them
> >>>>> any problems.
> >>> The vmware Xorg driver is currently relying on rmfb to turn all attached
> >>> crtcs off. Even if we were to fix that in the Xorg driver now, older
> >>> Xorgs with newer kernels still would break.
> >> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?
> >>
> >> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.
> > FYI, we are in the process of converting things to atomic.  This may happen
> > around 4.12
> >
> Will the driver allow the crtc to be enabled without primary plane?

Give me a few days to get back to you.  I'm reworking some patches right now.


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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Maarten Lankhorst]

Op 26-01-17 om 19:39 schreef Sinclair Yeh:
> On Thu, Jan 26, 2017 at 10:55:51AM +0100, Maarten Lankhorst wrote:
>> Op 25-01-17 om 19:05 schreef Sinclair Yeh:
>>> On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote:
>>>> Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
>>>>> On 01/25/2017 05:54 AM, Daniel Vetter wrote:
>>>>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>>>>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>>>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
>>>>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>>>
>>>>>>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>>>>>>> patch merged as:
>>>>>>>>>
>>>>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>>>>>>
>>>>>>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>>>>>>
>>>>>>>>> Actual code copied from Maarten's patch, but with the slight change to
>>>>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
>>>>>>>>> use the atomic path or not.
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> 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>
>>>>>>>> Would be great if someone else could r-b this, I've proven pretty well
>>>>>>>> that I don't understand the complexity here :(
>>>>>>>> -Daniel
>>>>>>> It looks like this will change the behavior slightly in that rmfb will
>>>>>>> cause primary planes to be disabled, but no longer cause the entire CRTC
>>>>>>> to be turned off.  You'll probably want to note that in the commit
>>>>>>> message, along with the justification on why this is okay ABI-wise.
>>>>>>>
>>>>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>>>>>>> removing it.") was initially trying to not only leave the CRTC on, but
>>>>>>> also preserve the framebuffer and leave the planes on; that wound up
>>>>>>> causing some kind of regression for vmwgfx, but I'm unclear on the
>>>>>>> details there.  I'd suggest getting an Ack from one of the vmware guys
>>>>>>> to ensure that the less drastic change in behavior here won't cause them
>>>>>>> any problems.
>>>>> The vmware Xorg driver is currently relying on rmfb to turn all attached
>>>>> crtcs off. Even if we were to fix that in the Xorg driver now, older
>>>>> Xorgs with newer kernels still would break.
>>>> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?
>>>>
>>>> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.
>>> FYI, we are in the process of converting things to atomic.  This may happen
>>> around 4.12
>>>
>> Will the driver allow the crtc to be enabled without primary plane?
> Give me a few days to get back to you.  I'm reworking some patches right now.
>
>
Any update on this?

~Maarten

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

Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

[Sinclair Yeh]

I've verified that it doesn't break our existing code, but I'm in the process of rebasing my atomic enabling patch series onto drm-next along with this.  I should be able to get this done by tomorrow morning.
________________________________________
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Sent: Thursday, February 9, 2017 4:29:49 AM
To: Sinclair Yeh
Cc: Thomas Hellstrom; Daniel Vetter; Matt Roper; Daniel Vetter; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Daniel Vetter
Subject: Re: [PATCH 4/4] drm: Resurrect atomic rmfb code, v2

Op 26-01-17 om 19:39 schreef Sinclair Yeh:
> On Thu, Jan 26, 2017 at 10:55:51AM +0100, Maarten Lankhorst wrote:
>> Op 25-01-17 om 19:05 schreef Sinclair Yeh:
>>> On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote:
>>>> Op 25-01-17 om 09:09 schreef Thomas Hellstrom:
>>>>> On 01/25/2017 05:54 AM, Daniel Vetter wrote:
>>>>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote:
>>>>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote:
>>>>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote:
>>>>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>>>
>>>>>>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>>>>>>> patch merged as:
>>>>>>>>>
>>>>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>>>>>>
>>>>>>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>>>>>>
>>>>>>>>> Actual code copied from Maarten's patch, but with the slight change to
>>>>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to
>>>>>>>>> use the atomic path or not.
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> 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>
>>>>>>>> Would be great if someone else could r-b this, I've proven pretty well
>>>>>>>> that I don't understand the complexity here :(
>>>>>>>> -Daniel
>>>>>>> It looks like this will change the behavior slightly in that rmfb will
>>>>>>> cause primary planes to be disabled, but no longer cause the entire CRTC
>>>>>>> to be turned off.  You'll probably want to note that in the commit
>>>>>>> message, along with the justification on why this is okay ABI-wise.
>>>>>>>
>>>>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after
>>>>>>> removing it.") was initially trying to not only leave the CRTC on, but
>>>>>>> also preserve the framebuffer and leave the planes on; that wound up
>>>>>>> causing some kind of regression for vmwgfx, but I'm unclear on the
>>>>>>> details there.  I'd suggest getting an Ack from one of the vmware guys
>>>>>>> to ensure that the less drastic change in behavior here won't cause them
>>>>>>> any problems.
>>>>> The vmware Xorg driver is currently relying on rmfb to turn all attached
>>>>> crtcs off. Even if we were to fix that in the Xorg driver now, older
>>>>> Xorgs with newer kernels still would break.
>>>> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled?
>>>>
>>>> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow.
>>> FYI, we are in the process of converting things to atomic.  This may happen
>>> around 4.12
>>>
>> Will the driver allow the crtc to be enabled without primary plane?
> Give me a few days to get back to you.  I'm reworking some patches right now.
>
>
Any update on this?

~Maarten

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

Re: [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Jani Nikula]

On Wed, 25 Jan 2017, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> This was somehow lost between v3 and the merged version in Maarten's
> patch merged as:
>
> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Wed May 4 14:38:26 2016 +0200
>
>     drm/core: Do not preserve framebuffer on rmfb, v4.
>
> 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.
>
> If this commit is rejected by the driver then we will still fall
> back to the old behavior and turn off the crtc.
>
> 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.
>
> 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>

Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
for the patch.

BR,
Jani.


> ---
>  drivers/gpu/drm/drm_atomic.c        | 106 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |   1 +
>  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
>  3 files changed, 114 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 723392fc98c8..c79ab8048435 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2058,6 +2058,112 @@ 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, disable_crtcs = false;
> +
> +	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;
> +		}
> +
> +		/*
> +		 * 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);
> +
> +			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;
> +	}
> +
> +	/* 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);
> +
> +		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);
> +
> +	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);
> +
> +	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 724c329186d5..d98a683d31d0 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -184,6 +184,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 588ccc3a2218..357b521bbdc3 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);

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

Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Sinclair Yeh]

On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
> On Wed, 25 Jan 2017, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> > This was somehow lost between v3 and the merged version in Maarten's
> > patch merged as:
> >
> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
> > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Date:   Wed May 4 14:38:26 2016 +0200
> >
> >     drm/core: Do not preserve framebuffer on rmfb, v4.
> >
> > 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.
> >
> > If this commit is rejected by the driver then we will still fall
> > back to the old behavior and turn off the crtc.
> >
> > 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.
> >
> > 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>
> 
> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
> for the patch.

I verified yesterday that this patch will cause a regression for vmwgfx
multi-mon.  Can we hold off on this?



> 
> BR,
> Jani.
> 
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 106 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_crtc_internal.h |   1 +
> >  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
> >  3 files changed, 114 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 723392fc98c8..c79ab8048435 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -2058,6 +2058,112 @@ 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, disable_crtcs = false;
> > +
> > +	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;
> > +		}
> > +
> > +		/*
> > +		 * 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);
> > +
> > +			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;
> > +	}
> > +
> > +	/* 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);
> > +
> > +		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);
> > +
> > +	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);
> > +
> > +	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 724c329186d5..d98a683d31d0 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -184,6 +184,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 588ccc3a2218..357b521bbdc3 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);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Jani Nikula]

On Wed, 15 Feb 2017, Sinclair Yeh <syeh@vmware.com> wrote:
> On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
>> On Wed, 25 Jan 2017, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>> > This was somehow lost between v3 and the merged version in Maarten's
>> > patch merged as:
>> >
>> > commit f2d580b9a8149735cbc4b59c4a8df60173658140
>> > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Date:   Wed May 4 14:38:26 2016 +0200
>> >
>> >     drm/core: Do not preserve framebuffer on rmfb, v4.
>> >
>> > 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.
>> >
>> > If this commit is rejected by the driver then we will still fall
>> > back to the old behavior and turn off the crtc.
>> >
>> > 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.
>> >
>> > 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>
>> 
>> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
>> for the patch.
>
> I verified yesterday that this patch will cause a regression for vmwgfx
> multi-mon.  Can we hold off on this?

Turns out it fails some of our tests too. Maybe three weeks old CI
results are not to be trusted, the base moves too fast. *shrug*.

I've dropped the patch, and asked Dave not to pull. Let's go back to the
drawing board...

BR,
Jani.


>
>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> > ---
>> >  drivers/gpu/drm/drm_atomic.c        | 106 ++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/drm_crtc_internal.h |   1 +
>> >  drivers/gpu/drm/drm_framebuffer.c   |   7 +++
>> >  3 files changed, 114 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> > index 723392fc98c8..c79ab8048435 100644
>> > --- a/drivers/gpu/drm/drm_atomic.c
>> > +++ b/drivers/gpu/drm/drm_atomic.c
>> > @@ -2058,6 +2058,112 @@ 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, disable_crtcs = false;
>> > +
>> > +	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;
>> > +		}
>> > +
>> > +		/*
>> > +		 * 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);
>> > +
>> > +			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;
>> > +	}
>> > +
>> > +	/* 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);
>> > +
>> > +		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);
>> > +
>> > +	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);
>> > +
>> > +	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 724c329186d5..d98a683d31d0 100644
>> > --- a/drivers/gpu/drm/drm_crtc_internal.h
>> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> > @@ -184,6 +184,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 588ccc3a2218..357b521bbdc3 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);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

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

Re: [Intel-gfx] [PATCH v3 4/4] drm: Resurrect atomic rmfb code, v3

[Maarten Lankhorst]

Op 16-02-17 om 10:45 schreef Jani Nikula:
> On Wed, 15 Feb 2017, Sinclair Yeh <syeh@vmware.com> wrote:
>> On Wed, Feb 15, 2017 at 03:56:09PM +0200, Jani Nikula wrote:
>>> On Wed, 25 Jan 2017, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>>>> This was somehow lost between v3 and the merged version in Maarten's
>>>> patch merged as:
>>>>
>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140
>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Date:   Wed May 4 14:38:26 2016 +0200
>>>>
>>>>     drm/core: Do not preserve framebuffer on rmfb, v4.
>>>>
>>>> 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.
>>>>
>>>> If this commit is rejected by the driver then we will still fall
>>>> back to the old behavior and turn off the crtc.
>>>>
>>>> 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.
>>>>
>>>> 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>
>>> Pushed to drm-misc-next-fixes, and sent the pull request to Dave. Thanks
>>> for the patch.
>> I verified yesterday that this patch will cause a regression for vmwgfx
>> multi-mon.  Can we hold off on this?
> Turns out it fails some of our tests too. Maybe three weeks old CI
> results are not to be trusted, the base moves too fast. *shrug*.
>
> I've dropped the patch, and asked Dave not to pull. Let's go back to the
> drawing board...
Yeah it's unfortunate. We tend to trigger a lot of bugs in other parts of the code with this change. The reload tests are fixed by fixing drm_atomic_helper_disable_all.
I haven't seen the crc bugs before, would be interesting to know why other tests a're suddenly failing.

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