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

Wed Sep 2 09:26:25 UTC 2020

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 at amd.com> wrote:
>>>>
>>>>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>>> 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?

-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer