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

[Kazlauskas, Nicholas]

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:
>>>
>>> On 2021-02-19 7:58 p.m., Simon Ser wrote:
>>>> Make sure there's an underlying pipe that can be used for the
>>>> cursor.
>>>>
>>>> Signed-off-by: Simon Ser <contact at emersion.fr>
>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>> Cc: Harry Wentland <hwentlan at amd.com>
>>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index acbe1537e7cf..a5d6010405bf 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -9226,9 +9226,14 @@ static int dm_check_crtc_cursor(struct 
>>>> drm_atomic_state *state,
>>>>        }
>>>>
>>>>        new_cursor_state = drm_atomic_get_new_plane_state(state, 
>>>> crtc->cursor);
>>>> -     if (!new_cursor_state || !new_underlying_state || 
>>>> !new_cursor_state->fb)
>>>> +     if (!new_cursor_state || !new_cursor_state->fb)
>>>>                return 0;
>>>>
>>>> +     if (!new_underlying_state || !new_underlying_state->fb) {
>>>> +             drm_dbg_atomic(crtc->dev, "Cursor plane can't be 
>>>> enabled without underlying plane\n");
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>>        cursor_scale_w = new_cursor_state->crtc_w * 1000 /
>>>>                         (new_cursor_state->src_w >> 16);
>>>>        cursor_scale_h = new_cursor_state->crtc_h * 1000 /
>>>>
>>>
>>> Houston, we have a problem I'm afraid. Adding Daniel.
>>>
>>>
>>> If the primary plane is enabled with a format which isn't compatible 
>>> with the HW cursor,
>>> and no overlay plane is enabled, the same issues as described in 
>>> b836a274b797
>>> "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC 
>>> is" can again occur:
>>>
>>>
>>> * The legacy cursor ioctl fails with EINVAL for a non-0 cursor FB ID
>>>   (which enables the cursor plane).
>>>
>>> * If the cursor plane is enabled (e.g. using the legacy cursor ioctl
>>>   during DPMS off), changing the legacy DPMS property value from off to
>>>   on fails with EINVAL.
>>
>> atomic_check should still be run when the crtc is off, so the legacy
>> cursor ioctl should fail when dpms off in this case already.
> 
> Good point. This could already be problematic though. E.g. mutter treats
> EINVAL from the cursor ioctl as the driver not supporting HW cursors at
> all, so it falls back to SW cursor and never tries to use the HW cursor
> again. (I don't think mutter could hit this particular case with an
> incompatible format though, but there might be other similar user space)
> 
> 
>>> 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.

When ChromeOS disables MPO it doesn't do it plane by plane, it does it 
in one go from NV12+ARGB8888 -> ARGB88888.

Regards,
Nicholas Kazlauskas