From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
* Toggling CRTC active to 1 failed if the cursor plane was enabled (e.g. via legacy DPMS property & cursor ioctl). * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com ---
Note that this will cause some IGT tests to fail without https://patchwork.freedesktop.org/series/80904/ .
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 897d60ade1e4..33c5739e221b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5262,19 +5262,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) { }
-static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state) -{ - struct drm_device *dev = new_crtc_state->crtc->dev; - struct drm_plane *plane; - - drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) { - if (plane->type == DRM_PLANE_TYPE_CURSOR) - return true; - } - - return false; -} - static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state) { struct drm_atomic_state *state = new_crtc_state->state; @@ -5338,19 +5325,21 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, return ret; }
- /* In some use cases, like reset, no stream is attached */ - if (!dm_crtc_state->stream) - return 0; - /* - * We want at least one hardware plane enabled to use - * the stream with a cursor enabled. + * We require the primary plane to be enabled whenever the CRTC is, + * otherwise the legacy cursor ioctl helper may end up trying to enable + * the cursor plane while the primary plane is disabled, which is not + * supported by the hardware. And there is legacy userspace which stops + * using the HW cursor altogether in response to the resulting EINVAL. */ - if (state->enable && state->active && - does_crtc_have_active_cursor(state) && - dm_crtc_state->active_planes == 0) + if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL;
+ /* In some use cases, like reset, no stream is attached */ + if (!dm_crtc_state->stream) + return 0; + if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK) return 0;
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various
- &drm_mode_config_funcs.atomic_check callback to reject an atomic
- commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled (e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
Regards, Nicholas Kazlauskas
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Note that this will cause some IGT tests to fail without https://patchwork.freedesktop.org/series/80904/ .
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 897d60ade1e4..33c5739e221b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5262,19 +5262,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) { }
-static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state) -{
- struct drm_device *dev = new_crtc_state->crtc->dev;
- struct drm_plane *plane;
- drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
if (plane->type == DRM_PLANE_TYPE_CURSOR)return true;- }
- return false;
-}
- static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state) { struct drm_atomic_state *state = new_crtc_state->state;
@@ -5338,19 +5325,21 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, return ret; }
- /* In some use cases, like reset, no stream is attached */
- if (!dm_crtc_state->stream)
return 0;- /*
* We want at least one hardware plane enabled to use* the stream with a cursor enabled.
* We require the primary plane to be enabled whenever the CRTC is,* otherwise the legacy cursor ioctl helper may end up trying to enable* the cursor plane while the primary plane is disabled, which is not* supported by the hardware. And there is legacy userspace which stops* using the HW cursor altogether in response to the resulting EINVAL.
- if (state->enable && state->active &&
does_crtc_have_active_cursor(state) &&dm_crtc_state->active_planes == 0)
if (state->enable &&
!(state->plane_mask & drm_plane_mask(crtc->primary)))return -EINVAL;
/* In some use cases, like reset, no stream is attached */
if (!dm_crtc_state->stream)
return 0;if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK) return 0;
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
1) Currently the checks are invalid (atomic_check must not decide based on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
2) Accurately rejecting only atomic states where the cursor plane is enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
3) This patch addresses 1) & 2) but may break existing atomic userspace which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer michel@daenzer.net wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled.
Hi,
my opinion as a userspace developer is that enabling a CRTC without a primary plane has traditionally not worked, so userspace cannot *rely* on it ever working. I think legacy KMS API does not even allow to express that really, or has built-in assumptions that it doesn't work - you can call the legacy cursor ioctls, they don't fail, but also the CRTC remains off. Correct me if this is not true.
Atomic userspace OTOH is required to test the strange (and all!) cases like this to see if they work or not, and carry on with a fallback if they don't. Userspace that wants to use a CRTC without a primary plane likely needs other CRTC properties as well, like background color.
I would guess that when two regressions conflict, the older userspace would win. Hence legacy KMS using Mutter should keep working, while atomic userspace is left with increased power consumption. Not using a hardware cursor (Mutter) is much more easily an end-user visible regression than slightly shorter battery life.
Atomic test commits are allowed to fail at any time for any reason and something that previously worked can fail on the next frame or on the next kernel, as far as I understand.
Sounds like the helpers you refer to are inadequate for your case. Can't you fix the helpers in the long run and land this patch as an immediate fix?
Thanks, pq
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer michel@daenzer.net wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various *
&drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was
enabled (e.g. via legacy DPMS property & cursor ioctl). * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not
decide based on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor
plane is enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic
userspace which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled.
Hi,
my opinion as a userspace developer is that enabling a CRTC without a primary plane has traditionally not worked, so userspace cannot *rely* on it ever working. I think legacy KMS API does not even allow to express that really, or has built-in assumptions that it doesn't work - you can call the legacy cursor ioctls, they don't fail, but also the CRTC remains off. Correct me if this is not true.
Atomic userspace OTOH is required to test the strange (and all!) cases like this to see if they work or not, and carry on with a fallback if they don't. Userspace that wants to use a CRTC without a primary plane likely needs other CRTC properties as well, like background color.
I would guess that when two regressions conflict, the older userspace would win. Hence legacy KMS using Mutter should keep working, while atomic userspace is left with increased power consumption. Not using a hardware cursor (Mutter) is much more easily an end-user visible regression than slightly shorter battery life.
Atomic test commits are allowed to fail at any time for any reason and something that previously worked can fail on the next frame or on the next kernel, as far as I understand.
All of this basically matches my understanding.
Sounds like the helpers you refer to are inadequate for your case. Can't you fix the helpers in the long run and land this patch as an immediate fix?
I'd rather leave working on those helpers to KMS developers. :)
- -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer
On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer michel@daenzer.net wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various *
&drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was
enabled (e.g. via legacy DPMS property & cursor ioctl). * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not
decide based on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor
plane is enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic
userspace which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled.
Hi,
my opinion as a userspace developer is that enabling a CRTC without a primary plane has traditionally not worked, so userspace cannot *rely* on it ever working. I think legacy KMS API does not even allow to express that really, or has built-in assumptions that it doesn't work - you can call the legacy cursor ioctls, they don't fail, but also the CRTC remains off. Correct me if this is not true.
Atomic userspace OTOH is required to test the strange (and all!) cases like this to see if they work or not, and carry on with a fallback if they don't. Userspace that wants to use a CRTC without a primary plane likely needs other CRTC properties as well, like background color.
I would guess that when two regressions conflict, the older userspace would win. Hence legacy KMS using Mutter should keep working, while atomic userspace is left with increased power consumption. Not using a hardware cursor (Mutter) is much more easily an end-user visible regression than slightly shorter battery life.
Atomic test commits are allowed to fail at any time for any reason and something that previously worked can fail on the next frame or on the next kernel, as far as I understand.
All of this basically matches my understanding.
Sounds like the helpers you refer to are inadequate for your case. Can't you fix the helpers in the long run and land this patch as an immediate fix?
I'd rather leave working on those helpers to KMS developers. :)
I thought the issue is the rmfb ioctl trickery, which has this assumption fully backed in. The wiggle room in there for semantic changes is iirc pretty much nil, it took us a pile of patches to just get to the current state. So it's not helper code, it's core legacy ioctl code for atomic drivers. -Daniel
Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer -----BEGIN PGP SIGNATURE-----
iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs= =Pbd2 -----END PGP SIGNATURE----- _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
Sounds like the helpers you refer to are inadequate for your case. Can't you fix the helpers in the long run and land this patch as an immediate fix?
I'd rather leave working on those helpers to KMS developers. :)
I thought the issue is the rmfb ioctl trickery, which has this assumption fully backed in. The wiggle room in there for semantic changes is iirc pretty much nil, it took us a pile of patches to just get to the current state. So it's not helper code, it's core legacy ioctl code for atomic drivers.
My bad. Should I respin with an amended commit log?
On Tue, Sep 01, 2020 at 10:56:42AM +0200, Michel Dänzer wrote:
On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
Sounds like the helpers you refer to are inadequate for your case. Can't you fix the helpers in the long run and land this patch as an immediate fix?
I'd rather leave working on those helpers to KMS developers. :)
I thought the issue is the rmfb ioctl trickery, which has this assumption fully backed in. The wiggle room in there for semantic changes is iirc pretty much nil, it took us a pile of patches to just get to the current state. So it's not helper code, it's core legacy ioctl code for atomic drivers.
My bad. Should I respin with an amended commit log?
Yeah if it's not yet merged then I think would be good to clarify that this assumption is baked into rmfb, and that together with the assumption the the legacy cursor ioctls just work we have fallout.
If (and hey I've been on vacations for 2 weeks, so good chances I remebers this all wrong) this is indeed what we discussed a few weeks ago.
Cheers, Daniel
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Regards, Nicholas Kazlauskas
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Why would you bother implementing that?
Is there really an IGT test that unconditionally demands cursor plane to be usable without any other planes?
Thanks, pq
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 2020-08-26 10:24 a.m., Pekka Paalanen wrote:
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various *
&drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was
enabled (e.g. via legacy DPMS property & cursor ioctl). * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not
decide based on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor
plane is enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic
userspace which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
This would be my case 2) above, so still requires figuring out how the atomic KMS helpers should deal with corner cases such as:
* CRTC on, primary plane off, overlay & cursor planes on * RmFB of FB assigned to overlay plane → need to disable overlay plane, but driver rejects that (because it would leave only the cursor plane on)
- -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer
On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Why would you bother implementing that?
Is there really an IGT test that unconditionally demands cursor plane to be usable without any other planes?
The cursor plane isn't anything else than any other plane, aside from the legacy uapi implication that it's used for the legacy cursor ioctls.
Which means the cursor plane could actually be a full-featured plane, and it's totally legit to use just that without anything else enabled.
So yeah if you allow that, it better show something :-)
Personally I'd lean towards merging this patch to close the gap (oldest regressions wins and all that) and then implement the black plane hack on top. -Daniel
On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
The atomic helpers disable the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- Toggling CRTC active to 1 failed if the cursor plane was enabled
(e.g. via legacy DPMS property & cursor ioctl).
- Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Why would you bother implementing that?
Is there really an IGT test that unconditionally demands cursor plane to be usable without any other planes?
The cursor plane isn't anything else than any other plane, aside from the legacy uapi implication that it's used for the legacy cursor ioctls.
Which means the cursor plane could actually be a full-featured plane, and it's totally legit to use just that without anything else enabled.
So yeah if you allow that, it better show something :-)
Personally I'd lean towards merging this patch to close the gap (oldest regressions wins and all that) and then implement the black plane hack on top.
Not sure I'm a big fan of the black plane hack. Is there any way we could allow the (non-displayed) cursor for the legacy IOCTL but not for the atomic IOCTL? I assume that would require a change to core code in the atomic helpers that convert legacy IOCTLs to atomic for drivers.
Harry
-Daniel
On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
On 2020-08-21 12:57 p.m., Michel Dänzer wrote: > From: Michel Dänzer mdaenzer@redhat.com > > Don't check drm_crtc_state::active for this either, per its > documentation in include/drm/drm_crtc.h: > > * Hence drivers must not consult @active in their various > * &drm_mode_config_funcs.atomic_check callback to reject an atomic > * commit. > > The atomic helpers disable the CRTC as needed for disabling the primary > plane. > > This prevents at least the following problems if the primary plane gets > disabled (e.g. due to destroying the FB assigned to the primary plane, > as happens e.g. with mutter in Wayland mode): > > * Toggling CRTC active to 1 failed if the cursor plane was enabled > (e.g. via legacy DPMS property & cursor ioctl). > * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
We previously had the requirement that the primary plane must be enabled but some userspace expects that they can enable just the overlay plane without anything else.
I think the chromuiumos atomictest validates that this works as well:
So is DRM going forward then with the expectation that this is wrong behavior from userspace?
We require at least one plane to be enabled to display a cursor, but it doesn't necessarily need to be the primary.
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Why would you bother implementing that?
Is there really an IGT test that unconditionally demands cursor plane to be usable without any other planes?
The cursor plane isn't anything else than any other plane, aside from the legacy uapi implication that it's used for the legacy cursor ioctls.
Which means the cursor plane could actually be a full-featured plane, and it's totally legit to use just that without anything else enabled.
So yeah if you allow that, it better show something :-)
Personally I'd lean towards merging this patch to close the gap (oldest regressions wins and all that) and then implement the black plane hack on top.
Not sure I'm a big fan of the black plane hack. Is there any way we could allow the (non-displayed) cursor for the legacy IOCTL but not for the atomic IOCTL? I assume that would require a change to core code in the atomic helpers that convert legacy IOCTLs to atomic for drivers.
That's the "just dont show the cursor when it's not possible" hack, which is also rather iffy imo.
The other side is that this is all kinda uapi, or at least we've spent a lot of attempts trying to needle all this through rmfb and cursor ioctls, and I'm not sure what exactly you can change without breaking something. Yeah it's not helper stuff as in the commit message, it's core ioctl code unfortunately. -Daniel
Harry
-Daniel
On 2020-09-02 9:02 a.m., Daniel Vetter wrote:
On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas" nicholas.kazlauskas@amd.com wrote:
On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
It's a "pick your poison" situation:
- Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to accidentally hit errors trying to enable/move the cursor or switch DPMS off → on.
- Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code, which can only deal with the "CRTC on & primary plane off is not allowed" case specifically.
- This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.
I do think in principle atomic userspace is expected to handle case 3) and leave the primary plane enabled. However, this is not ideal from an energy consumption PoV. Therefore, here's another idea for a possible way out of this quagmire:
amdgpu_dm does not reject any atomic states based on which planes are enabled in it. If the cursor plane is enabled but all other planes are off, amdgpu_dm internally either:
a) Enables an overlay plane and makes it invisible, e.g. by assigning a minimum size FB with alpha = 0.
b) Enables the primary plane and assigns a minimum size FB (scaled up to the required size) containing all black, possibly using compression. (Trying to minimize the memory bandwidth)
Does either of these seem feasible? If both do, which one would be preferable?
It's really the same solution since DCN doesn't make a distinction between primary or overlay planes in hardware. DCE doesn't have overlay planes enabled so this is not relevant there.
The old behavior (pre 5.1?) was to silently accept the commit even though the screen would be completely black instead of outright rejecting the commit.
I almost wonder if that makes more sense in the short term here since the only "userspace" affected here is IGT. We'll fail the CRC checks, but no userspace actually tries to actively use a cursor with no primary plane enabled from my understanding.
Hi,
I believe that there exists userspace that will *accidentally* attempt to update the cursor plane while primary plane or whole CRTC is off. Some versions of Mutter might do that on racy conditions, I suspect. These are legacy KMS users, not atomic KMS.
However, I do not believe there exists any userspace that would actually expect the display to show the cursor plane alone without a primary plane. Therefore I'd be ok with legacy cursor ioctls silently succeeding. Atomic commits not. So the difference has to be in the translation from legacy UAPI to kernel internal atomic interface.
In the long term I think we can work on getting cursor actually on the screen in this case, though I can't say I really like having to reserve some small buffer (eg. 16x16) for allowing lightup on this corner case.
Why would you bother implementing that?
Is there really an IGT test that unconditionally demands cursor plane to be usable without any other planes?
The cursor plane isn't anything else than any other plane, aside from the legacy uapi implication that it's used for the legacy cursor ioctls.
Which means the cursor plane could actually be a full-featured plane, and it's totally legit to use just that without anything else enabled.
So yeah if you allow that, it better show something :-)
Personally I'd lean towards merging this patch to close the gap (oldest regressions wins and all that) and then implement the black plane hack on top.
Not sure I'm a big fan of the black plane hack. Is there any way we could allow the (non-displayed) cursor for the legacy IOCTL but not for the atomic IOCTL? I assume that would require a change to core code in the atomic helpers that convert legacy IOCTLs to atomic for drivers.
That's the "just dont show the cursor when it's not possible" hack, which is also rather iffy imo.
The other side is that this is all kinda uapi, or at least we've spent a lot of attempts trying to needle all this through rmfb and cursor ioctls, and I'm not sure what exactly you can change without breaking something. Yeah it's not helper stuff as in the commit message, it's core ioctl code unfortunately.
Yeah, and I'm not sure how it could work.
E.g. I don't think the core code for the legacy cursor ioctl can just ignore an error from the driver when trying to enable the cursor plane, as there can be genuine errors for other reasons? So instead the driver code would have to special-case somehow for certain legacy ioctls, which sounds very iffy, if it's possible at all.
Also, what should happen if atomic user-space destroys the FB assigned to an overlay plane, and disabling that plane leaves only the cursor plane enabled?
There would need to be a more specific / less hand-wavy proposal how exactly this is envisioned to work.
Anyway, this sub-thread is now about the next steps after this patch, not about alternatives to it, right?
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
* The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane). * If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL.
v2: * Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 45f5f4b7024d..c5f9452490d6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5269,19 +5269,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) { }
-static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state) -{ - struct drm_device *dev = new_crtc_state->crtc->dev; - struct drm_plane *plane; - - drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) { - if (plane->type == DRM_PLANE_TYPE_CURSOR) - return true; - } - - return false; -} - static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state) { struct drm_atomic_state *state = new_crtc_state->state; @@ -5345,19 +5332,20 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, return ret; }
- /* In some use cases, like reset, no stream is attached */ - if (!dm_crtc_state->stream) - return 0; - /* - * We want at least one hardware plane enabled to use - * the stream with a cursor enabled. + * We require the primary plane to be enabled whenever the CRTC is, otherwise + * drm_mode_cursor_universal may end up trying to enable the cursor plane while all other + * planes are disabled, which is not supported by the hardware. And there is legacy + * userspace which stops using the HW cursor altogether in response to the resulting EINVAL. */ - if (state->enable && state->active && - does_crtc_have_active_cursor(state) && - dm_crtc_state->active_planes == 0) + if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL;
+ /* In some use cases, like reset, no stream is attached */ + if (!dm_crtc_state->stream) + return 0; + if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK) return 0;
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various
- &drm_mode_config_funcs.atomic_check callback to reject an atomic
- commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 45f5f4b7024d..c5f9452490d6 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5269,19 +5269,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) { }
-static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state) -{
- struct drm_device *dev = new_crtc_state->crtc->dev;
- struct drm_plane *plane;
- drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
if (plane->type == DRM_PLANE_TYPE_CURSOR)return true;- }
- return false;
-}
static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state) { struct drm_atomic_state *state = new_crtc_state->state; @@ -5345,19 +5332,20 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, return ret; }
- /* In some use cases, like reset, no stream is attached */
- if (!dm_crtc_state->stream)
return 0;- /*
* We want at least one hardware plane enabled to use* the stream with a cursor enabled.
* We require the primary plane to be enabled whenever the CRTC is, otherwise* drm_mode_cursor_universal may end up trying to enable the cursor plane while all other* planes are disabled, which is not supported by the hardware. And there is legacy* userspace which stops using the HW cursor altogether in response to the resulting EINVAL.
- if (state->enable && state->active &&
does_crtc_have_active_cursor(state) &&dm_crtc_state->active_planes == 0)
if (state->enable &&
!(state->plane_mask & drm_plane_mask(crtc->primary)))return -EINVAL;
/* In some use cases, like reset, no stream is attached */
if (!dm_crtc_state->stream)
return 0;if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK) return 0;
-- 2.28.0
On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various
- &drm_mode_config_funcs.atomic_check callback to reject an atomic
- commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Daniel!
Nick / Harry, any more feedback? If not, can you apply this?
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID
(which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property
value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Daniel!
Nick / Harry, any more feedback? If not, can you apply this?
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration.
Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess since the blank plane hack is going to add a significant amount of complexity to DM.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Regards, Nicholas Kazlauskas
On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote:
On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID
(which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property
value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165 GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344 Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Daniel!
Nick / Harry, any more feedback? If not, can you apply this?
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration.
Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's).
Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...]
I'm not sure what you mean by that / how it could work.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Thanks!
On 2020-09-14 11:22 a.m., Michel Dänzer wrote:
On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote:
On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
* Hence drivers must not consult @active in their various * &drm_mode_config_funcs.atomic_check callback to reject an atomic * commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID
(which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property
value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Daniel!
Nick / Harry, any more feedback? If not, can you apply this?
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration.
Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's).
We only expose support for NV12 on the primary plane so we wouldn't be hitting this case at least.
Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...]
I'm not sure what you mean by that / how it could work.
Dropping the check you added in this patch:
+ if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL;
That way we can still allow the cursor plane to be enabled while primary/overlay are not, it's just not going to be actually visible on the screen. It'll fail some IGT tests but nothing really uses this configuration as more than just a temporary state.
Regards, Nicholas Kazlauskas
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Thanks!
On 2020-09-14 5:33 p.m., Kazlauskas, Nicholas wrote:
On 2020-09-14 11:22 a.m., Michel Dänzer wrote:
On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote:
On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration.
Someone mentioned that ChromiumOS uses this for video playback with black bars (when the video aspect ratio doesn't match the display's).
We only expose support for NV12 on the primary plane so we wouldn't be hitting this case at least.
Interesting, so if we're lucky this really won't affect any real-world apps.
We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess [...]
I'm not sure what you mean by that / how it could work.
Dropping the check you added in this patch:
+ if (state->enable && + !(state->plane_mask & drm_plane_mask(crtc->primary))) return -EINVAL;
That way we can still allow the cursor plane to be enabled while primary/overlay are not, it's just not going to be actually visible on the screen. It'll fail some IGT tests but nothing really uses this configuration as more than just a temporary state.
As Daniel pointed out in 20200901075432.GW2352366@phenom.ffwll.local in the v1 patch thread, that won't fly, since atomic userspace can legitimately expect the cursor plane to be visible in that case.
Applied. Thanks!
Alex
On Mon, Sep 14, 2020 at 10:37 AM Kazlauskas, Nicholas nicholas.kazlauskas@amd.com wrote:
On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
From: Michel Dänzer mdaenzer@redhat.com
Don't check drm_crtc_state::active for this either, per its documentation in include/drm/drm_crtc.h:
- Hence drivers must not consult @active in their various
- &drm_mode_config_funcs.atomic_check callback to reject an atomic
- commit.
atomic_remove_fb disables the CRTC as needed for disabling the primary plane.
This prevents at least the following problems if the primary plane gets disabled (e.g. due to destroying the FB assigned to the primary plane, as happens e.g. with mutter in Wayland mode):
- The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID (which enables the cursor plane).
- If the cursor plane was enabled, changing the legacy DPMS property value from off to on returned EINVAL.
v2:
- Minor changes to code comment and commit log, per review feedback.
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165
GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer mdaenzer@redhat.com
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Daniel!
Nick / Harry, any more feedback? If not, can you apply this?
P.S. Since DCN doesn't make a distinction between primary or overlay planes in hardware, could ChromiumOS achieve the same effect with only the primary plane instead of only an overlay plane? If so, maybe there's no need for the "black plane hack".
I only know that atomictest tries to enable this configuration. Not sure if ChromiumOS or other "real" userspace tries this configuration.
Maybe for now this can go in and if something breaks we can deal with the fallout then. We can always go back to lying to userspace about the cursor being visible if the commit fails in that case I guess since the blank plane hack is going to add a significant amount of complexity to DM.
Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com
Regards, Nicholas Kazlauskas _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Harry Wentland -
Kazlauskas, Nicholas -
Michel Dänzer -
Pekka Paalanen