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(-)
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);
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.*/-- 2.7.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.
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
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
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.
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.
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,
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
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.
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
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);
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
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);
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
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
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
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
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
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
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?
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.
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
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
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);
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);
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
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
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@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Jani Nikula -
Maarten Lankhorst -
Matt Roper -
Sinclair Yeh -
Thomas Hellstrom