[PATCH] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

[Michel Dänzer]

-----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 at 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 at 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?
>>
>> 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
-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0YkIwAKCRBaga+Oatuy
AMWrAJ9IDMIj2eGXERYDZfwraOCHebwE9QCfR/nOaoLGfIOii6r1H4JLn9sraFI=
=D/LX
-----END PGP SIGNATURE-----