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@xxxxxxxxxx>
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?
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
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
