* [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
@ 2017-11-01 15:04 Maarten Lankhorst
2017-11-01 15:29 ` [Intel-gfx] " Ville Syrjälä
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-01 15:04 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Daniel Vetter
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.
Vmwgfx' and simple kms helper atomic implementation rejects CRTC
enabled without plane, so we can do this safely.
If the atomic commit is rejected by the driver then we will still
fall back to the old behavior and turn off the crtc.
Changes since v1:
- Restart completely when rmfb with crtc on fails (Sean Paul).
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 2affe53f3fda..f0679468f421 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
struct drm_plane *plane;
struct drm_connector *conn;
struct drm_connector_state *conn_state;
- int i, ret = 0;
+ int i, ret;
unsigned plane_mask;
+ bool disable_crtcs = false;
- state = drm_atomic_state_alloc(dev);
- if (!state)
- return -ENOMEM;
-
+retry_disable:
drm_modeset_acquire_init(&ctx, 0);
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state) {
+ ret = -ENOMEM;
+ goto out;
+ }
state->acquire_ctx = &ctx;
retry:
@@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
goto unlock;
}
- if (plane_state->crtc->primary == plane) {
+ 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);
@@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
plane->old_fb = plane->fb;
}
+ /* This list is only filled when disable_crtcs is set. */
for_each_new_connector_in_state(state, conn, conn_state, i) {
ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
@@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
drm_atomic_state_put(state);
+out:
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
+ if (ret == -EINVAL && !disable_crtcs) {
+ disable_crtcs = true;
+ goto retry_disable;
+ }
+
return ret;
}
--
2.14.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
@ 2017-11-01 15:29 ` Ville Syrjälä
2017-11-01 15:55 ` Maarten Lankhorst
2017-11-01 16:30 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2017-11-01 15:29 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> This introduces a slight behavioral change to rmfb. Instead of
> disabling a crtc when the primary plane is disabled, we try to
> preserve it.
>
> Apart from old versions of the vmwgfx xorg driver, there is
> nothing depending on rmfb disabling a crtc.
>
> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> enabled without plane, so we can do this safely.
>
> If the atomic commit is rejected by the driver then we will still
> fall back to the old behavior and turn off the crtc.
>
> Changes since v1:
> - Restart completely when rmfb with crtc on fails (Sean Paul).
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 2affe53f3fda..f0679468f421 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> struct drm_plane *plane;
> struct drm_connector *conn;
> struct drm_connector_state *conn_state;
> - int i, ret = 0;
> + int i, ret;
> unsigned plane_mask;
> + bool disable_crtcs = false;
>
> - state = drm_atomic_state_alloc(dev);
> - if (!state)
> - return -ENOMEM;
> -
> +retry_disable:
> drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> state->acquire_ctx = &ctx;
>
> retry:
> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> goto unlock;
> }
>
> - if (plane_state->crtc->primary == plane) {
> + 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);
> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> plane->old_fb = plane->fb;
> }
>
> + /* This list is only filled when disable_crtcs is set. */
> for_each_new_connector_in_state(state, conn, conn_state, i) {
WARN_ON(!disable_crtcs) maybe?
> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>
> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>
> drm_atomic_state_put(state);
>
> +out:
> drm_modeset_drop_locks(&ctx);
> drm_modeset_acquire_fini(&ctx);
>
> + if (ret == -EINVAL && !disable_crtcs) {
Hmm. -EINVAL seems rather specific. Not sure if we could just check for
any error?
Or... I'm not sure if we have any central place where we do the
"can I disable the primary plane w/o disabling the crtc?" check. If we
do then we could also add a comment there informing people that the
-EINVAL is important.
> + disable_crtcs = true;
> + goto retry_disable;
> + }
> +
> return ret;
> }
>
> --
> 2.14.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:29 ` [Intel-gfx] " Ville Syrjälä
@ 2017-11-01 15:55 ` Maarten Lankhorst
2017-11-01 17:00 ` Ville Syrjälä
0 siblings, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-01 15:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, dri-devel
Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
>> This introduces a slight behavioral change to rmfb. Instead of
>> disabling a crtc when the primary plane is disabled, we try to
>> preserve it.
>>
>> Apart from old versions of the vmwgfx xorg driver, there is
>> nothing depending on rmfb disabling a crtc.
>>
>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
>> enabled without plane, so we can do this safely.
>>
>> If the atomic commit is rejected by the driver then we will still
>> fall back to the old behavior and turn off the crtc.
>>
>> Changes since v1:
>> - Restart completely when rmfb with crtc on fails (Sean Paul).
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> index 2affe53f3fda..f0679468f421 100644
>> --- a/drivers/gpu/drm/drm_framebuffer.c
>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>> struct drm_plane *plane;
>> struct drm_connector *conn;
>> struct drm_connector_state *conn_state;
>> - int i, ret = 0;
>> + int i, ret;
>> unsigned plane_mask;
>> + bool disable_crtcs = false;
>>
>> - state = drm_atomic_state_alloc(dev);
>> - if (!state)
>> - return -ENOMEM;
>> -
>> +retry_disable:
>> drm_modeset_acquire_init(&ctx, 0);
>> +
>> + state = drm_atomic_state_alloc(dev);
>> + if (!state) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> state->acquire_ctx = &ctx;
>>
>> retry:
>> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>> goto unlock;
>> }
>>
>> - if (plane_state->crtc->primary == plane) {
>> + 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);
>> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>> plane->old_fb = plane->fb;
>> }
>>
>> + /* This list is only filled when disable_crtcs is set. */
>> for_each_new_connector_in_state(state, conn, conn_state, i) {
> WARN_ON(!disable_crtcs) maybe?
Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :)
>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>>
>> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>
>> drm_atomic_state_put(state);
>>
>> +out:
>> drm_modeset_drop_locks(&ctx);
>> drm_modeset_acquire_fini(&ctx);
>>
>> + if (ret == -EINVAL && !disable_crtcs) {
> Hmm. -EINVAL seems rather specific. Not sure if we could just check for
> any error?
>
> Or... I'm not sure if we have any central place where we do the
> "can I disable the primary plane w/o disabling the crtc?" check. If we
> do then we could also add a comment there informing people that the
> -EINVAL is important.
We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
2017-11-01 15:29 ` [Intel-gfx] " Ville Syrjälä
@ 2017-11-01 16:30 ` Patchwork
2017-11-07 10:35 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-11-01 16:30 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
URL : https://patchwork.freedesktop.org/series/32985/
State : warning
== Summary ==
Series 32985v1 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
https://patchwork.freedesktop.org/api/1.0/series/32985/revisions/1/mbox/
Test gem_ctx_switch:
Subgroup basic-default-heavy:
incomplete -> PASS (fi-glk-dsi)
Test gem_sync:
Subgroup basic-all:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-each:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-many-each:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-store-all:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-store-each:
pass -> SKIP (fi-blb-e6850)
Test gem_tiled_blits:
Subgroup basic:
pass -> SKIP (fi-blb-e6850)
Test gem_tiled_fence_blits:
Subgroup basic:
pass -> SKIP (fi-blb-e6850)
Test gem_wait:
Subgroup basic-busy-all:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-wait-all:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-await-all:
pass -> SKIP (fi-blb-e6850)
Test kms_busy:
Subgroup basic-flip-a:
pass -> SKIP (fi-blb-e6850)
Subgroup basic-flip-b:
pass -> SKIP (fi-blb-e6850)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
pass -> SKIP (fi-blb-e6850)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-byt-j1900) fdo#101705 +1
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:442s
fi-blb-e6850 total:289 pass:210 dwarn:1 dfail:0 fail:0 skip:78 time:357s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:535s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:274s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:502s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:500s
fi-byt-j1900 total:289 pass:254 dwarn:0 dfail:0 fail:0 skip:35 time:491s
fi-byt-n2820 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:486s
fi-cfl-s total:289 pass:251 dwarn:5 dfail:0 fail:0 skip:33 time:523s
fi-cnl-y total:217 pass:196 dwarn:0 dfail:0 fail:0 skip:20
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:425s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:263s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:537s
fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:492s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:434s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:432s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:426s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:462s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:475s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:523s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:472s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:534s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:569s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:448s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:541s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:557s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:523s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:496s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:558s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:422s
fff06da054b4752eabf7d4103d30a281f8de96d9 drm-tip: 2017y-11m-01d-13h-47m-20s UTC integration manifest
bc724f51d670 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6295/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:55 ` Maarten Lankhorst
@ 2017-11-01 17:00 ` Ville Syrjälä
2017-11-01 17:23 ` [Intel-gfx] " Maarten Lankhorst
2017-11-02 8:55 ` Maarten Lankhorst
0 siblings, 2 replies; 15+ messages in thread
From: Ville Syrjälä @ 2017-11-01 17:00 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> > On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> >> This introduces a slight behavioral change to rmfb. Instead of
> >> disabling a crtc when the primary plane is disabled, we try to
> >> preserve it.
> >>
> >> Apart from old versions of the vmwgfx xorg driver, there is
> >> nothing depending on rmfb disabling a crtc.
> >>
> >> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> >> enabled without plane, so we can do this safely.
The code for those seems a bit inconsistent. The crtc check requires
that the crtc state and plane state match. But the plane check allows
the plane to be enabled w/o the crtc being enabled. I guess it doesn't
matter really since you can't enable the plane without a crtc, and the
crtc check would then catch the case where the crtc would be disabled.
Oh and looks like drm_plane_helper_check_state() is a bit buggy. It
still uses crtc->enabled instead of crtc_state->enable to check the
state of the crtc. I guess to keep drm_plane_helper_check_update()
working we may have to pass in the crtc state manually.
The vmwgfx plane check looks a bit bogus in other ways too. I guess
I'll have to fire off a couple of patches.
> >>
> >> If the atomic commit is rejected by the driver then we will still
> >> fall back to the old behavior and turn off the crtc.
> >>
> >> Changes since v1:
> >> - Restart completely when rmfb with crtc on fails (Sean Paul).
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> ---
> >> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
> >> 1 file changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >> index 2affe53f3fda..f0679468f421 100644
> >> --- a/drivers/gpu/drm/drm_framebuffer.c
> >> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> struct drm_plane *plane;
> >> struct drm_connector *conn;
> >> struct drm_connector_state *conn_state;
> >> - int i, ret = 0;
> >> + int i, ret;
> >> unsigned plane_mask;
> >> + bool disable_crtcs = false;
> >>
> >> - state = drm_atomic_state_alloc(dev);
> >> - if (!state)
> >> - return -ENOMEM;
> >> -
> >> +retry_disable:
> >> drm_modeset_acquire_init(&ctx, 0);
> >> +
> >> + state = drm_atomic_state_alloc(dev);
> >> + if (!state) {
> >> + ret = -ENOMEM;
> >> + goto out;
> >> + }
> >> state->acquire_ctx = &ctx;
> >>
> >> retry:
> >> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> goto unlock;
> >> }
> >>
> >> - if (plane_state->crtc->primary == plane) {
> >> + 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);
> >> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >> plane->old_fb = plane->fb;
> >> }
> >>
> >> + /* This list is only filled when disable_crtcs is set. */
> >> for_each_new_connector_in_state(state, conn, conn_state, i) {
> > WARN_ON(!disable_crtcs) maybe?
> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :)
It would serve as a way to document that fact, even without the comment.
But I won't insist on it.
> >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> >>
> >> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> >>
> >> drm_atomic_state_put(state);
> >>
> >> +out:
> >> drm_modeset_drop_locks(&ctx);
> >> drm_modeset_acquire_fini(&ctx);
> >>
> >> + if (ret == -EINVAL && !disable_crtcs) {
> > Hmm. -EINVAL seems rather specific. Not sure if we could just check for
> > any error?
> >
> > Or... I'm not sure if we have any central place where we do the
> > "can I disable the primary plane w/o disabling the crtc?" check. If we
> > do then we could also add a comment there informing people that the
> > -EINVAL is important.
> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
Oh well. I guess people just have to be careful with their error
values. I suppoe anyone depending on the retry will notice this
issue rather quickly.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 17:00 ` Ville Syrjälä
@ 2017-11-01 17:23 ` Maarten Lankhorst
2017-11-02 8:55 ` Maarten Lankhorst
1 sibling, 0 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-01 17:23 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, dri-devel
Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
>> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
>>>> This introduces a slight behavioral change to rmfb. Instead of
>>>> disabling a crtc when the primary plane is disabled, we try to
>>>> preserve it.
>>>>
>>>> Apart from old versions of the vmwgfx xorg driver, there is
>>>> nothing depending on rmfb disabling a crtc.
>>>>
>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
>>>> enabled without plane, so we can do this safely.
> The code for those seems a bit inconsistent. The crtc check requires
> that the crtc state and plane state match. But the plane check allows
> the plane to be enabled w/o the crtc being enabled. I guess it doesn't
> matter really since you can't enable the plane without a crtc, and the
> crtc check would then catch the case where the crtc would be disabled.
>
> Oh and looks like drm_plane_helper_check_state() is a bit buggy. It
> still uses crtc->enabled instead of crtc_state->enable to check the
> state of the crtc. I guess to keep drm_plane_helper_check_update()
> working we may have to pass in the crtc state manually.
This is the transitional helper. i915 gets away with it because it passes the flag that ignores crtc->enabled.
> The vmwgfx plane check looks a bit bogus in other ways too. I guess
> I'll have to fire off a couple of patches.
>
>>>> If the atomic commit is rejected by the driver then we will still
>>>> fall back to the old behavior and turn off the crtc.
>>>>
>>>> Changes since v1:
>>>> - Restart completely when rmfb with crtc on fails (Sean Paul).
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
>>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>>> index 2affe53f3fda..f0679468f421 100644
>>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>>> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> struct drm_plane *plane;
>>>> struct drm_connector *conn;
>>>> struct drm_connector_state *conn_state;
>>>> - int i, ret = 0;
>>>> + int i, ret;
>>>> unsigned plane_mask;
>>>> + bool disable_crtcs = false;
>>>>
>>>> - state = drm_atomic_state_alloc(dev);
>>>> - if (!state)
>>>> - return -ENOMEM;
>>>> -
>>>> +retry_disable:
>>>> drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> + state = drm_atomic_state_alloc(dev);
>>>> + if (!state) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>> state->acquire_ctx = &ctx;
>>>>
>>>> retry:
>>>> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> goto unlock;
>>>> }
>>>>
>>>> - if (plane_state->crtc->primary == plane) {
>>>> + 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);
>>>> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> plane->old_fb = plane->fb;
>>>> }
>>>>
>>>> + /* This list is only filled when disable_crtcs is set. */
>>>> for_each_new_connector_in_state(state, conn, conn_state, i) {
>>> WARN_ON(!disable_crtcs) maybe?
>> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :)
> It would serve as a way to document that fact, even without the comment.
> But I won't insist on it.
>
>>>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>>>>
>>>> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>>
>>>> drm_atomic_state_put(state);
>>>>
>>>> +out:
>>>> drm_modeset_drop_locks(&ctx);
>>>> drm_modeset_acquire_fini(&ctx);
>>>>
>>>> + if (ret == -EINVAL && !disable_crtcs) {
>>> Hmm. -EINVAL seems rather specific. Not sure if we could just check for
>>> any error?
>>>
>>> Or... I'm not sure if we have any central place where we do the
>>> "can I disable the primary plane w/o disabling the crtc?" check. If we
>>> do then we could also add a comment there informing people that the
>>> -EINVAL is important.
>> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
> Oh well. I guess people just have to be careful with their error
> values. I suppoe anyone depending on the retry will notice this
> issue rather quickly.
Yes. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 17:00 ` Ville Syrjälä
2017-11-01 17:23 ` [Intel-gfx] " Maarten Lankhorst
@ 2017-11-02 8:55 ` Maarten Lankhorst
2017-11-06 14:01 ` Ville Syrjälä
1 sibling, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-02 8:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, dri-devel
Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
>> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
>>>> This introduces a slight behavioral change to rmfb. Instead of
>>>> disabling a crtc when the primary plane is disabled, we try to
>>>> preserve it.
>>>>
>>>> Apart from old versions of the vmwgfx xorg driver, there is
>>>> nothing depending on rmfb disabling a crtc.
>>>>
>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
>>>> enabled without plane, so we can do this safely.
> The code for those seems a bit inconsistent. The crtc check requires
> that the crtc state and plane state match. But the plane check allows
> the plane to be enabled w/o the crtc being enabled. I guess it doesn't
> matter really since you can't enable the plane without a crtc, and the
> crtc check would then catch the case where the crtc would be disabled.
Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't
be invoked. Hence it's the most accurate way of making sure that
crtc enabled <=> primary plane bound.
If the check was done in the primary plane, an atomic modeset could enable
the crtc without enabling the primary plane, which shouldn't be allowed but
a plane check won't catch it.
This has been a bug in simple-kms-helper, fixed in the below commit:
commit 765831dc27ab141b3a0be1ab55b922b012427902
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date: Wed Jul 12 10:13:29 2017 +0200
drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled.
> Oh and looks like drm_plane_helper_check_state() is a bit buggy. It
> still uses crtc->enabled instead of crtc_state->enable to check the
> state of the crtc. I guess to keep drm_plane_helper_check_update()
> working we may have to pass in the crtc state manually.
>
> The vmwgfx plane check looks a bit bogus in other ways too. I guess
> I'll have to fire off a couple of patches.
>
>>>> If the atomic commit is rejected by the driver then we will still
>>>> fall back to the old behavior and turn off the crtc.
>>>>
>>>> Changes since v1:
>>>> - Restart completely when rmfb with crtc on fails (Sean Paul).
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
>>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>>> index 2affe53f3fda..f0679468f421 100644
>>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>>> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> struct drm_plane *plane;
>>>> struct drm_connector *conn;
>>>> struct drm_connector_state *conn_state;
>>>> - int i, ret = 0;
>>>> + int i, ret;
>>>> unsigned plane_mask;
>>>> + bool disable_crtcs = false;
>>>>
>>>> - state = drm_atomic_state_alloc(dev);
>>>> - if (!state)
>>>> - return -ENOMEM;
>>>> -
>>>> +retry_disable:
>>>> drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> + state = drm_atomic_state_alloc(dev);
>>>> + if (!state) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>> state->acquire_ctx = &ctx;
>>>>
>>>> retry:
>>>> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> goto unlock;
>>>> }
>>>>
>>>> - if (plane_state->crtc->primary == plane) {
>>>> + 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);
>>>> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>> plane->old_fb = plane->fb;
>>>> }
>>>>
>>>> + /* This list is only filled when disable_crtcs is set. */
>>>> for_each_new_connector_in_state(state, conn, conn_state, i) {
>>> WARN_ON(!disable_crtcs) maybe?
>> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :)
> It would serve as a way to document that fact, even without the comment.
> But I won't insist on it.
>
>>>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>>>>
>>>> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>>>>
>>>> drm_atomic_state_put(state);
>>>>
>>>> +out:
>>>> drm_modeset_drop_locks(&ctx);
>>>> drm_modeset_acquire_fini(&ctx);
>>>>
>>>> + if (ret == -EINVAL && !disable_crtcs) {
>>> Hmm. -EINVAL seems rather specific. Not sure if we could just check for
>>> any error?
>>>
>>> Or... I'm not sure if we have any central place where we do the
>>> "can I disable the primary plane w/o disabling the crtc?" check. If we
>>> do then we could also add a comment there informing people that the
>>> -EINVAL is important.
>> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
> Oh well. I guess people just have to be careful with their error
> values. I suppoe anyone depending on the retry will notice this
> issue rather quickly.
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-02 8:55 ` Maarten Lankhorst
@ 2017-11-06 14:01 ` Ville Syrjälä
2017-11-06 14:06 ` Ville Syrjälä
0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2017-11-06 14:01 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote:
> Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
> >> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> >>>> This introduces a slight behavioral change to rmfb. Instead of
> >>>> disabling a crtc when the primary plane is disabled, we try to
> >>>> preserve it.
> >>>>
> >>>> Apart from old versions of the vmwgfx xorg driver, there is
> >>>> nothing depending on rmfb disabling a crtc.
> >>>>
> >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> >>>> enabled without plane, so we can do this safely.
> > The code for those seems a bit inconsistent. The crtc check requires
> > that the crtc state and plane state match. But the plane check allows
> > the plane to be enabled w/o the crtc being enabled. I guess it doesn't
> > matter really since you can't enable the plane without a crtc, and the
> > crtc check would then catch the case where the crtc would be disabled.
>
> Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't
> be invoked. Hence it's the most accurate way of making sure that
> crtc enabled <=> primary plane bound.
>
> If the check was done in the primary plane, an atomic modeset could enable
> the crtc without enabling the primary plane, which shouldn't be allowed but
> a plane check won't catch it.
>
> This has been a bug in simple-kms-helper, fixed in the below commit:
>
> commit 765831dc27ab141b3a0be1ab55b922b012427902
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date: Wed Jul 12 10:13:29 2017 +0200
>
> drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled.
Hmm. OK that part looks OK. What does seem a bit inconsistent is the
fact that we pass can_update_disabled=true, but later on we reject the
update when visible==false. The old code would have accepted that
because it didn't even call drm_plane_helper_check_state() when the
crtc (and thus also the plane) was disabled.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-06 14:01 ` Ville Syrjälä
@ 2017-11-06 14:06 ` Ville Syrjälä
2017-11-06 15:43 ` Maarten Lankhorst
0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2017-11-06 14:06 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote:
> > Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> > > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
> > >> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> > >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> > >>>> This introduces a slight behavioral change to rmfb. Instead of
> > >>>> disabling a crtc when the primary plane is disabled, we try to
> > >>>> preserve it.
> > >>>>
> > >>>> Apart from old versions of the vmwgfx xorg driver, there is
> > >>>> nothing depending on rmfb disabling a crtc.
> > >>>>
> > >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> > >>>> enabled without plane, so we can do this safely.
> > > The code for those seems a bit inconsistent. The crtc check requires
> > > that the crtc state and plane state match. But the plane check allows
> > > the plane to be enabled w/o the crtc being enabled. I guess it doesn't
> > > matter really since you can't enable the plane without a crtc, and the
> > > crtc check would then catch the case where the crtc would be disabled.
> >
> > Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't
> > be invoked. Hence it's the most accurate way of making sure that
> > crtc enabled <=> primary plane bound.
> >
> > If the check was done in the primary plane, an atomic modeset could enable
> > the crtc without enabling the primary plane, which shouldn't be allowed but
> > a plane check won't catch it.
>
> >
> > This has been a bug in simple-kms-helper, fixed in the below commit:
> >
> > commit 765831dc27ab141b3a0be1ab55b922b012427902
> > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Date: Wed Jul 12 10:13:29 2017 +0200
> >
> > drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled.
>
> Hmm. OK that part looks OK. What does seem a bit inconsistent is the
> fact that we pass can_update_disabled=true, but later on we reject the
> update when visible==false. The old code would have accepted that
> because it didn't even call drm_plane_helper_check_state() when the
> crtc (and thus also the plane) was disabled.
Actually how does this work at all? If you turn off both the crtc and
plane, then the plane check will always return -EINVAL on account of
visible==false?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-06 14:06 ` Ville Syrjälä
@ 2017-11-06 15:43 ` Maarten Lankhorst
2017-11-06 16:20 ` Ville Syrjälä
0 siblings, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-06 15:43 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, dri-devel
Op 06-11-17 om 15:06 schreef Ville Syrjälä:
> On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote:
>> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote:
>>> Op 01-11-17 om 18:00 schreef Ville Syrjälä:
>>>> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
>>>>> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
>>>>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
>>>>>>> This introduces a slight behavioral change to rmfb. Instead of
>>>>>>> disabling a crtc when the primary plane is disabled, we try to
>>>>>>> preserve it.
>>>>>>>
>>>>>>> Apart from old versions of the vmwgfx xorg driver, there is
>>>>>>> nothing depending on rmfb disabling a crtc.
>>>>>>>
>>>>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
>>>>>>> enabled without plane, so we can do this safely.
>>>> The code for those seems a bit inconsistent. The crtc check requires
>>>> that the crtc state and plane state match. But the plane check allows
>>>> the plane to be enabled w/o the crtc being enabled. I guess it doesn't
>>>> matter really since you can't enable the plane without a crtc, and the
>>>> crtc check would then catch the case where the crtc would be disabled.
>>> Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't
>>> be invoked. Hence it's the most accurate way of making sure that
>>> crtc enabled <=> primary plane bound.
>>>
>>> If the check was done in the primary plane, an atomic modeset could enable
>>> the crtc without enabling the primary plane, which shouldn't be allowed but
>>> a plane check won't catch it.
>>> This has been a bug in simple-kms-helper, fixed in the below commit:
>>>
>>> commit 765831dc27ab141b3a0be1ab55b922b012427902
>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Date: Wed Jul 12 10:13:29 2017 +0200
>>>
>>> drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled.
>> Hmm. OK that part looks OK. What does seem a bit inconsistent is the
>> fact that we pass can_update_disabled=true, but later on we reject the
>> update when visible==false. The old code would have accepted that
>> because it didn't even call drm_plane_helper_check_state() when the
>> crtc (and thus also the plane) was disabled.
> Actually how does this work at all? If you turn off both the crtc and
> plane, then the plane check will always return -EINVAL on account of
> visible==false?
>
If the crtc is turned off, enabled = false and the primary plane is not in crtc_state->plane_mask.
Visibility is ignored in this check, that part is handled in plane check that the framebuffer has to span the entire crtc if enabled.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-06 15:43 ` Maarten Lankhorst
@ 2017-11-06 16:20 ` Ville Syrjälä
0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2017-11-06 16:20 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Mon, Nov 06, 2017 at 04:43:17PM +0100, Maarten Lankhorst wrote:
> Op 06-11-17 om 15:06 schreef Ville Syrjälä:
> > On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote:
> >> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote:
> >>> Op 01-11-17 om 18:00 schreef Ville Syrjälä:
> >>>> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote:
> >>>>> Op 01-11-17 om 16:29 schreef Ville Syrjälä:
> >>>>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> >>>>>>> This introduces a slight behavioral change to rmfb. Instead of
> >>>>>>> disabling a crtc when the primary plane is disabled, we try to
> >>>>>>> preserve it.
> >>>>>>>
> >>>>>>> Apart from old versions of the vmwgfx xorg driver, there is
> >>>>>>> nothing depending on rmfb disabling a crtc.
> >>>>>>>
> >>>>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> >>>>>>> enabled without plane, so we can do this safely.
> >>>> The code for those seems a bit inconsistent. The crtc check requires
> >>>> that the crtc state and plane state match. But the plane check allows
> >>>> the plane to be enabled w/o the crtc being enabled. I guess it doesn't
> >>>> matter really since you can't enable the plane without a crtc, and the
> >>>> crtc check would then catch the case where the crtc would be disabled.
> >>> Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't
> >>> be invoked. Hence it's the most accurate way of making sure that
> >>> crtc enabled <=> primary plane bound.
> >>>
> >>> If the check was done in the primary plane, an atomic modeset could enable
> >>> the crtc without enabling the primary plane, which shouldn't be allowed but
> >>> a plane check won't catch it.
> >>> This has been a bug in simple-kms-helper, fixed in the below commit:
> >>>
> >>> commit 765831dc27ab141b3a0be1ab55b922b012427902
> >>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Date: Wed Jul 12 10:13:29 2017 +0200
> >>>
> >>> drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled.
> >> Hmm. OK that part looks OK. What does seem a bit inconsistent is the
> >> fact that we pass can_update_disabled=true, but later on we reject the
> >> update when visible==false. The old code would have accepted that
> >> because it didn't even call drm_plane_helper_check_state() when the
> >> crtc (and thus also the plane) was disabled.
> > Actually how does this work at all? If you turn off both the crtc and
> > plane, then the plane check will always return -EINVAL on account of
> > visible==false?
> >
> If the crtc is turned off, enabled = false and the primary plane is not in crtc_state->plane_mask.
>
> Visibility is ignored in this check, that part is handled in plane check that the framebuffer has to span the entire crtc if enabled.
Hmm. I thought the !crtc->enabled check disappeared from
drm_simple_kms_plane_atomic_check() but looks like I was wrong and it's
still there. OK, just totally ignore what I wrote before. I guess one
shouldn't try to figure out what the code is going to be doing while in
the middle of an unrelated bisect.
Though for some extra consistency we might want to change that to check
to look for !fb after calling drm_plane_helper_check_state(). That way
we wouldn't have to change drivers/simple_kms_helper if come up with
something that has to be checked even for disabled planes
in drm_plane_helper_check_state().
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
2017-11-01 15:29 ` [Intel-gfx] " Ville Syrjälä
2017-11-01 16:30 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2017-11-07 10:35 ` Patchwork
2017-11-07 11:57 ` ✓ Fi.CI.IGT: " Patchwork
2017-11-07 14:31 ` [PATCH] " Daniel Vetter
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-11-07 10:35 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
URL : https://patchwork.freedesktop.org/series/32985/
State : success
== Summary ==
Series 32985v1 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
https://patchwork.freedesktop.org/api/1.0/series/32985/revisions/1/mbox/
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
fail -> PASS (fi-gdg-551) fdo#102618
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-byt-j1900) fdo#101705 +1
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:435s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:454s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:380s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:543s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:274s
fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:502s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:506s
fi-byt-j1900 total:289 pass:254 dwarn:0 dfail:0 fail:0 skip:35 time:493s
fi-byt-n2820 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:482s
fi-cfl-s total:289 pass:254 dwarn:3 dfail:0 fail:0 skip:32 time:537s
fi-cnl-y total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:551s
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:435s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:265s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:538s
fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:489s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:433s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:431s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:422s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:473s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:460s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:526s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:474s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:531s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:574s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:457s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:539s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:566s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:516s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:497s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:457s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:555s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:421s
4295e1469b2127d79d2d02ef34d065509bdded97 drm-tip: 2017y-11m-06d-15h-35m-57s UTC integration manifest
b54086959609 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6981/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.IGT: success for drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
` (2 preceding siblings ...)
2017-11-07 10:35 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2017-11-07 11:57 ` Patchwork
2017-11-07 14:31 ` [PATCH] " Daniel Vetter
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-11-07 11:57 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
URL : https://patchwork.freedesktop.org/series/32985/
State : success
== Summary ==
Test kms_flip:
Subgroup plain-flip-ts-check:
fail -> PASS (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
skip -> PASS (shard-hsw)
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
shard-hsw total:2540 pass:1432 dwarn:1 dfail:0 fail:10 skip:1097 time:9271s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6981/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
` (3 preceding siblings ...)
2017-11-07 11:57 ` ✓ Fi.CI.IGT: " Patchwork
@ 2017-11-07 14:31 ` Daniel Vetter
2017-11-08 7:50 ` Maarten Lankhorst
4 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2017-11-07 14:31 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, dri-devel
On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
> This introduces a slight behavioral change to rmfb. Instead of
> disabling a crtc when the primary plane is disabled, we try to
> preserve it.
>
> Apart from old versions of the vmwgfx xorg driver, there is
> nothing depending on rmfb disabling a crtc.
>
> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
> enabled without plane, so we can do this safely.
>
> If the atomic commit is rejected by the driver then we will still
> fall back to the old behavior and turn off the crtc.
>
> Changes since v1:
> - Restart completely when rmfb with crtc on fails (Sean Paul).
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 2affe53f3fda..f0679468f421 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> struct drm_plane *plane;
> struct drm_connector *conn;
> struct drm_connector_state *conn_state;
> - int i, ret = 0;
> + int i, ret;
> unsigned plane_mask;
> + bool disable_crtcs = false;
>
> - state = drm_atomic_state_alloc(dev);
> - if (!state)
> - return -ENOMEM;
> -
> +retry_disable:
> drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> state->acquire_ctx = &ctx;
>
> retry:
> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> goto unlock;
> }
>
> - if (plane_state->crtc->primary == plane) {
> + 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);
> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> plane->old_fb = plane->fb;
> }
>
> + /* This list is only filled when disable_crtcs is set. */
> for_each_new_connector_in_state(state, conn, conn_state, i) {
> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
>
> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
>
> drm_atomic_state_put(state);
>
> +out:
> drm_modeset_drop_locks(&ctx);
> drm_modeset_acquire_fini(&ctx);
>
> + if (ret == -EINVAL && !disable_crtcs) {
> + disable_crtcs = true;
> + goto retry_disable;
> + }
> +
> return ret;
> }
>
> --
> 2.14.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2.
2017-11-07 14:31 ` [PATCH] " Daniel Vetter
@ 2017-11-08 7:50 ` Maarten Lankhorst
0 siblings, 0 replies; 15+ messages in thread
From: Maarten Lankhorst @ 2017-11-08 7:50 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel
Op 07-11-17 om 15:31 schreef Daniel Vetter:
> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote:
>> This introduces a slight behavioral change to rmfb. Instead of
>> disabling a crtc when the primary plane is disabled, we try to
>> preserve it.
>>
>> Apart from old versions of the vmwgfx xorg driver, there is
>> nothing depending on rmfb disabling a crtc.
>>
>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC
>> enabled without plane, so we can do this safely.
>>
>> If the atomic commit is rejected by the driver then we will still
>> fall back to the old behavior and turn off the crtc.
>>
>> Changes since v1:
>> - Restart completely when rmfb with crtc on fails (Sean Paul).
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks, pushed. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-11-08 7:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-01 15:04 [PATCH] drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2 Maarten Lankhorst
2017-11-01 15:29 ` [Intel-gfx] " Ville Syrjälä
2017-11-01 15:55 ` Maarten Lankhorst
2017-11-01 17:00 ` Ville Syrjälä
2017-11-01 17:23 ` [Intel-gfx] " Maarten Lankhorst
2017-11-02 8:55 ` Maarten Lankhorst
2017-11-06 14:01 ` Ville Syrjälä
2017-11-06 14:06 ` Ville Syrjälä
2017-11-06 15:43 ` Maarten Lankhorst
2017-11-06 16:20 ` Ville Syrjälä
2017-11-01 16:30 ` ✗ Fi.CI.BAT: warning for " Patchwork
2017-11-07 10:35 ` ✓ Fi.CI.BAT: success " Patchwork
2017-11-07 11:57 ` ✓ Fi.CI.IGT: " Patchwork
2017-11-07 14:31 ` [PATCH] " Daniel Vetter
2017-11-08 7:50 ` Maarten Lankhorst
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.