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

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

[Simon Ser]

Make sure there's an underlying pipe that can be used for the
cursor.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@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 /
-- 
2.30.1


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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@emersion.fr>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@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.


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.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Wed, Mar 3, 2021 at 5:53 PM Michel Dänzer <michel@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@emersion.fr>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Harry Wentland <hwentlan@amd.com>
> > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@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. And then
the dpms on call should still succeed.

atomic_check should never have different state checks depending upon
crtc_state->active.

> 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. At least assuming I'm understanding the hw constraints
correctly here. Otherwise I'm not really seeing how this would work
otherwise for userspace without some big surprises.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

On 2021-03-03 8:17 p.m., Daniel Vetter wrote:
> On Wed, Mar 3, 2021 at 5:53 PM Michel Dänzer <michel@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@emersion.fr>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: Harry Wentland <hwentlan@amd.com>
>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@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.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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@emersion.fr>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: Harry Wentland <hwentlan@amd.com>
>>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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@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?


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


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

On 2021-03-04 7:26 p.m., Kazlauskas, Nicholas wrote:
> 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@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.

They can still be by default?

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

That's unfortunate, but might be the least bad choice overall.

BTW, doesn't Chrome OS try to disable the ARGB overlay plane while there are no UI elements to display? If it does, this series might break it anyway (if the cursor plane can be enabled while the ARGB overlay plane is off).


>>> 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 WARN triggers if atomic_remove_fb returns an error, which is the case if it can't disable an overlay plane. I actually hit this with IGT tests while working on b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is" (I initially tried allowing the cursor plane to be enabled together with an overlay plane while the primary plane is off).

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

Good point, so at least there's no scanout of freed memory. Even so, the overlay plane continues displaying contents which user space apparently doesn't want to be displayed anymore.


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

I'd say it's more than just a little odd. :) Being unable to disable an overlay plane seems very surprising, and could make it tricky for user space (not to mention core DRM code like atomic_remove_fb) to find a solution.

I'd suggest the amdgpu DM code should rather virtualize the KMS API planes somehow such that an overlay plane can always be disabled. While this might incur some short-term pain, it will likely save more pain overall in the long term.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Fri, Mar 5, 2021 at 10:24 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2021-03-04 7:26 p.m., Kazlauskas, Nicholas wrote:
> > 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@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.
>
> They can still be by default?
>
> > 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.
>
> That's unfortunate, but might be the least bad choice overall.
>
> BTW, doesn't Chrome OS try to disable the ARGB overlay plane while there are no UI elements to display? If it does, this series might break it anyway (if the cursor plane can be enabled while the ARGB overlay plane is off).
>
>
> >>> 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 WARN triggers if atomic_remove_fb returns an error, which is the case if it can't disable an overlay plane. I actually hit this with IGT tests while working on b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is" (I initially tried allowing the cursor plane to be enabled together with an overlay plane while the primary plane is off).
>
> > 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.
>
> Good point, so at least there's no scanout of freed memory. Even so, the overlay plane continues displaying contents which user space apparently doesn't want to be displayed anymore.

Hm I do wonder how much we need to care for this. If you use planes,
you better use TEST_ONLY in atomic to it's full extend (including
cursor, if that's a real plane, which it is for every driver except
msm/mdp4). If userspace screws this up and worse, shuts of planes with
an RMFB, I think it's not entirely unreasonable to claim that it
should keep the pieces.

So maybe we should refine the WARN_ON to not trigger if other planes
than crtc->primary and crtc->cursor are enabled right now?

> > 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.
>
> I'd say it's more than just a little odd. :) Being unable to disable an overlay plane seems very surprising, and could make it tricky for user space (not to mention core DRM code like atomic_remove_fb) to find a solution.
>
> I'd suggest the amdgpu DM code should rather virtualize the KMS API planes somehow such that an overlay plane can always be disabled. While this might incur some short-term pain, it will likely save more pain overall in the long term.

Yeah I think this amd dc cursor problem is the first case where
removing a plane can make things worse.

Since the hw is what it is, can't we put a transparent plane with
cursor compatible format in for the case where stuff would fail? So
not fully virtualize the planes (since I don't see how that helps),
but just keeping the plane going underneath it all.

I think that's also what Ville did for i915/gen2, which has the
requirement that the primary plane must always be on iirc.

Ofc since amd display doesn't go through pagetables this needs some
vram, but maybe you can use the scalers to make the requirement a bit
less drastic.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kazlauskas, Nicholas]

On 2021-03-08 3:18 p.m., Daniel Vetter wrote:
> On Fri, Mar 5, 2021 at 10:24 AM Michel Dänzer <michel@daenzer.net> wrote:
>>
>> On 2021-03-04 7:26 p.m., Kazlauskas, Nicholas wrote:
>>> 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@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.
>>
>> They can still be by default?
>>
>>> 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.
>>
>> That's unfortunate, but might be the least bad choice overall.
>>
>> BTW, doesn't Chrome OS try to disable the ARGB overlay plane while there are no UI elements to display? If it does, this series might break it anyway (if the cursor plane can be enabled while the ARGB overlay plane is off).
>>
>>
>>>>> 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 WARN triggers if atomic_remove_fb returns an error, which is the case if it can't disable an overlay plane. I actually hit this with IGT tests while working on b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is" (I initially tried allowing the cursor plane to be enabled together with an overlay plane while the primary plane is off).
>>
>>> 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.
>>
>> Good point, so at least there's no scanout of freed memory. Even so, the overlay plane continues displaying contents which user space apparently doesn't want to be displayed anymore.
> 
> Hm I do wonder how much we need to care for this. If you use planes,
> you better use TEST_ONLY in atomic to it's full extend (including
> cursor, if that's a real plane, which it is for every driver except
> msm/mdp4). If userspace screws this up and worse, shuts of planes with
> an RMFB, I think it's not entirely unreasonable to claim that it
> should keep the pieces.
> 
> So maybe we should refine the WARN_ON to not trigger if other planes
> than crtc->primary and crtc->cursor are enabled right now?
> 
>>> 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.
>>
>> I'd say it's more than just a little odd. :) Being unable to disable an overlay plane seems very surprising, and could make it tricky for user space (not to mention core DRM code like atomic_remove_fb) to find a solution.
>>
>> I'd suggest the amdgpu DM code should rather virtualize the KMS API planes somehow such that an overlay plane can always be disabled. While this might incur some short-term pain, it will likely save more pain overall in the long term.
> 
> Yeah I think this amd dc cursor problem is the first case where
> removing a plane can make things worse.
> 
> Since the hw is what it is, can't we put a transparent plane with
> cursor compatible format in for the case where stuff would fail? So
> not fully virtualize the planes (since I don't see how that helps),
> but just keeping the plane going underneath it all.
> 
> I think that's also what Ville did for i915/gen2, which has the
> requirement that the primary plane must always be on iirc.
> 
> Ofc since amd display doesn't go through pagetables this needs some
> vram, but maybe you can use the scalers to make the requirement a bit
> less drastic.
> -Daniel >

The cursor framebuffer would have to be used as the pipe's primary 
framebuffer in this case because the hardware has nothing underneath to 
scan out along with it.

I'm not sure the atomic async update interface would work well in the 
virtualization case if we had to fallback from using the regular cursor 
programming to using full pipe programming.

This would burn extra power from the secondary pipe but it would also 
add cursor stuttering into the mix because async updates would be blocked.

My preference would be to continue to reject the commits and not 
implement the fallback path because of how suboptimal the whole thing 
is, but this whole thing is just kind of a mess.

Regards,
Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

On 2021-03-08 9:38 p.m., Kazlauskas, Nicholas wrote:
> On 2021-03-08 3:18 p.m., Daniel Vetter wrote:
>> On Fri, Mar 5, 2021 at 10:24 AM Michel Dänzer <michel@daenzer.net> wrote:
>>>
>>> On 2021-03-04 7:26 p.m., Kazlauskas, Nicholas wrote:
>>>>
>>>> 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.
>>>
>>> I'd say it's more than just a little odd. :) Being unable to disable
>>> an overlay plane seems very surprising, and could make it tricky for
>>> user space (not to mention core DRM code like atomic_remove_fb) to
>>> find a solution.
>>>
>>> I'd suggest the amdgpu DM code should rather virtualize the KMS API
>>> planes somehow such that an overlay plane can always be disabled.
>>> While this might incur some short-term pain, it will likely save more
>>> pain overall in the long term.
>>
>> Yeah I think this amd dc cursor problem is the first case where
>> removing a plane can make things worse.
>>
>> Since the hw is what it is, can't we put a transparent plane with
>> cursor compatible format in for the case where stuff would fail? So
>> not fully virtualize the planes (since I don't see how that helps),
>> but just keeping the plane going underneath it all.

What you describe is one way to "virtualize the KMS API planes", since their state would no longer directly correspond to HW state.


>> I think that's also what Ville did for i915/gen2, which has the
>> requirement that the primary plane must always be on iirc.
>>
>> Ofc since amd display doesn't go through pagetables this needs some
>> vram, but maybe you can use the scalers to make the requirement a bit
>> less drastic.
> 
> The cursor framebuffer would have to be used as the pipe's primary
> framebuffer in this case because the hardware has nothing underneath to
> scan out along with it.
> 
> I'm not sure the atomic async update interface would work well in the
> virtualization case if we had to fallback from using the regular cursor
> programming to using full pipe programming.
> 
> This would burn extra power from the secondary pipe but it would also
> add cursor stuttering into the mix because async updates would be blocked.

Why can't the DM code keep the overlay plane enabled in DC, and make it scan out from a minimal buffer which appears fully transparent, as suggested by Daniel (and yours truly before)?


> My preference would be to continue to reject the commits and not
> implement the fallback path because of how suboptimal the whole thing
> is, but this whole thing is just kind of a mess.

It is indeed. Let's keep the mess contained as much as possible, instead of inflicting it on the whole atomic KMS ecosystem.


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx