[PATCH 3/6] amd/display: fail on cursor plane without an underlying plane

[Kazlauskas, Nicholas]

On 2021-03-04 10:35 a.m., Michel Dänzer wrote:
> On 2021-03-04 4:09 p.m., Kazlauskas, Nicholas wrote:
>> On 2021-03-04 4:05 a.m., Michel Dänzer wrote:
>>> On 2021-03-03 8:17 p.m., Daniel Vetter wrote:
>>>> On Wed, Mar 3, 2021 at 5:53 PM Michel Dänzer <michel at daenzer.net> wrote:
>>>>>
>>>>> Moreover, in the same scenario plus an overlay plane enabled with a
>>>>> HW cursor compatible format, if the FB bound to the overlay plane is
>>>>> destroyed, the common DRM code will attempt to disable the overlay
>>>>> plane, but dm_check_crtc_cursor will reject that now. I can't remember
>>>>> exactly what the result is, but AFAIR it's not pretty.
>>>>
>>>> CRTC gets disabled instead. That's why we went with the "always
>>>> require primary plane" hack. I think the only solution here would be
>>>> to enable the primary plane (but not in userspace-visible state, so
>>>> this needs to be done in the dc derived state objects only) that scans
>>>> out black any time we're in such a situation with cursor with no
>>>> planes.
>>>
>>> This is about a scenario described by Nicholas earlier:
>>>
>>> Cursor Plane - ARGB8888
>>>
>>> Overlay Plane - ARGB8888 Desktop/UI with cutout for video
>>>
>>> Primary Plane - NV12 video
>>>
>>> And destroying the FB bound to the overlay plane. The fallback to disable
>>> the CRTC in atomic_remove_fb only kicks in for the primary plane, so it
>>> wouldn't in this case and would fail. Which would in turn trigger the
>>> WARN in drm_framebuffer_remove (and leave the overlay plane scanning out
>>> from freed memory?).
>>>
>>>
>>> The cleanest solution might be not to allow any formats incompatible with
>>> the HW cursor for the primary plane.
>>
>> Legacy X userspace doesn't use overlays but Chrome OS does.
>>
>> This would regress ChromeOS MPO support because it relies on the NV12
>> video plane being on the bottom.
> 
> Could it use the NV12 overlay plane below the ARGB primary plane?

Plane ordering was previously undefined in DRM so we have userspace that 
assumes overlays are on top.

Today we have the z-order property in DRM that defines where it is in 
the stack, so technically it could but we'd also be regressing existing 
behavior on Chrome OS today.

> 
> 
>> When ChromeOS disables MPO it doesn't do it plane by plane, it does it
>> in one go from NV12+ARGB8888 -> ARGB88888.
> 
> Even so, we cannot expect all user space to do the same, and we cannot
> allow any user space to trigger a WARN and scanout from freed memory.
> 
>

The WARN doesn't trigger because there's still a reference on the FB - 
the reference held by DRM since it's still scanning out the overlay. 
Userspace can't reclaim this memory with another buffer allocation 
because it's still in use.

It's a little odd that a disable commit can fail, but I don't think 
there's anything in DRM core that specifies that this can't happen for 
planes.

Regards,
Nicholas Kazlauskas