[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-24 9:43 a.m., Pekka Paalanen wrote:
> On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> <michel at 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 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.
>
> 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
-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
=Pbd2
-----END PGP SIGNATURE-----