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

[Harry Wentland]

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:
>>>> 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.
>>
>>> 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
>