[PATCH] drm/amd/display: Fix overlay validation by considering cursors

[Harry Wentland]

On 2021-06-08 3:47 a.m., Michel Dänzer wrote:
> On 2021-06-07 8:45 p.m., Sean Paul wrote:
>>
>>
>> On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland <harry.wentland at amd.com <mailto:harry.wentland at amd.com>> wrote:
>>
>>     On 2021-06-07 2:19 p.m., Sean Paul wrote:
>>     > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira
>>     > <Rodrigo.Siqueira at amd.com <mailto:Rodrigo.Siqueira at amd.com>> wrote:
>>     >>

<snip>

>>     >> Hi Mark and Sean,
>>     >>
>>     >> I don't know if I fully comprehended the scenario which in my patch
>>     >> might cause a ChromeOS crash, but this is what I understood:
>>     >>
>>     >
>>     > Chrome compositor only does an atomic test when the layout geometry
>>     > changes (ie: plane is added/removed/resized/moved). This does not take
>>     > into consideration the cursor. Once the atomic test has been validated
>>     > for a given layout geometry (set of planes/fbs along with their sizes
>>     > and locations), Chrome assumes this will continue to be valid.
>>     >
>>     > So the situation I'm envisioning is if the cursor is hidden, an
>>     > overlay-based strategy will pass in the kernel. If at any time the
>>     > cursor becomes visible, the kernel will start failing commits which
>>     > causes Chrome's GPU process to crash.
>>     >
>>     > In general I'm uneasy with using the cursor in the atomic check since
>>     > it feels like it could be racy independent of the issue above. I
>>     > haven't dove into the helper code enough to prove it, just a
>>     > spidey-sense.
>>     >
>>
>>     Isn't the cursor supposed to be just another plane? If so, shouldn't
>>     adding/removing the cursor trigger an atomic test?
>>
>>
>> Chrome is using legacy cursor.
>>
>> Yes it will trigger an atomic test in the kernel, and that atomic test will fail. Unfortunately Chrome does not expect a failure so it will crash.
> 
> The solution is probably indeed for atomic check to reject state which couldn't work if the cursor was enabled, even if the cursor is currently disabled. Otherwise one can hit various surprising errors via legacy APIs, as described in b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is".
> 

Agreed.

It's a bit unfortunate but the only way to deal with this better is if we had some way for DRM master to tell us whether it will only use the atomic IOCTL or use legacy IOCTLs (including a combination of atomic and legacy).

Harry

>