[RFC 0/1] Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[RFC 0/1] Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[Zaeem Mohamed]

Current patch to prevent index-out-of-bounds when cursor plane is
required and plane_count is MAX_SURFACES. This check needs to occur in
dm_atomic_check where failing is safe. Need help with finding a better
location for the bounds check within dm_atomic_commit

Zaeem Mohamed (1):
  SWDEV476969 - dm: Fail dm_atomic_check if cursor overlay is required
    at MAX_SURFACES

 amdgpu_dm/amdgpu_dm.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.34.1

[RFC 1/1] SWDEV476969 - dm: Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[Zaeem Mohamed]

[why]
Prevent index-out-of-bounds due to requiring cursor overlay when
plane_count is MAX_SURFACES.

[how]
Bounds check on plane_count when requiring overlay cursor.

Co-developed-by: Melissa Wen <mwen@igalia.com>
Signed-off-by: Zaeem Mohamed <zaeem.mohamed@amd.com>
---
 amdgpu_dm/amdgpu_dm.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/amdgpu_dm/amdgpu_dm.c b/amdgpu_dm/amdgpu_dm.c
index df83e7b42b..c2595efb74 100644
--- a/amdgpu_dm/amdgpu_dm.c
+++ b/amdgpu_dm/amdgpu_dm.c
@@ -11676,6 +11676,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 		 * need to be added for DC to not disable a plane by mistake
 		 */
 		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE) {
+			if(dc->current_state->stream_status->plane_count >= MAX_SURFACES){
+				drm_dbg_driver(crtc->dev,
+				       "Can't enable cursor plane with %d planes\n", MAX_SURFACES);
+				ret = -EINVAL;
+				goto fail;
+			}
 			ret = drm_atomic_add_affected_planes(state, crtc);
 			if (ret)
 				goto fail;
@@ -11769,8 +11775,16 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 
 		/* Overlay cusor not subject to native cursor restrictions */
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
-		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE)
+		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE){
+			if(dc->current_state->stream_status->plane_count > MAX_SURFACES){
+				drm_dbg_driver(crtc->dev,
+				       "Can't enable cursor plane with %d planes\n", MAX_SURFACES);
+				ret = -EINVAL;
+				goto fail;
+			}
+			
 			continue;
+		}
 
 		/* Check if rotation or scaling is enabled on DCN401 */
 		if ((drm_plane_mask(crtc->cursor) & new_crtc_state->plane_mask) &&
-- 
2.34.1

Re: [RFC 1/1] SWDEV476969 - dm: Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[Melissa Wen]

On 25/10/2024 16:37, Zaeem Mohamed wrote:
> [why]
> Prevent index-out-of-bounds due to requiring cursor overlay when
> plane_count is MAX_SURFACES.
Hi Zaeem,

Thanks for working on this fix.
>
> [how]
> Bounds check on plane_count when requiring overlay cursor.
I agree. Atomic check makes sense.

1) Since the native cursor mode was previously the unique mode avaliable, I
wonder if the driver should fall to native cursor mode in favor of the 
overlay
planes advertised. I.e. if driver says it supports two overlay planes and
the userspace requested both, cursor overlay mode should not be available or
should switch to native cursor mode, as before the introduction of cursor
overlay mode.

2) Then my second question: can we increase the number of surfaces to 4 
first to
accommodate more than one active overlay plane with cursor overly mode 
enabled.
If four is still possible, this increase can reduce the number of commit
failure scenarios and mitigate current userspace issues first. After 
addressing
current array-out-of-bounds, follow-up patches can do the proper changes and
checks.

3) IMHO, the incoherence between MAX_SURFACE_NUM and MAX_SURFACE should be
addressed before adding debugging points. For example, there are part of the
DC code using MAX_SURFACE_NUM == MAX_PLANE == 6 to allocate 
dc_surface_update
arrays, instead of using MAX_SURFACE value. You can find one of this 
case here:
https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc/core/dc.c#L4507
It doesn't make sense to me and it can contribute to an incomplete solution.

Also, please add the references of bugs reported in the amd tracker, so far:

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3693
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594
> Co-developed-by: Melissa Wen <mwen@igalia.com>
I don't think I contributed enough to your code to get any credits.
Thanks, but you can remove my co-dev-by :)

Best Regards,

Melissa
> Signed-off-by: Zaeem Mohamed <zaeem.mohamed@amd.com>
> ---
>   amdgpu_dm/amdgpu_dm.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/amdgpu_dm/amdgpu_dm.c b/amdgpu_dm/amdgpu_dm.c
> index df83e7b42b..c2595efb74 100644
> --- a/amdgpu_dm/amdgpu_dm.c
> +++ b/amdgpu_dm/amdgpu_dm.c
> @@ -11676,6 +11676,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   		 * need to be added for DC to not disable a plane by mistake
>   		 */
>   		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE) {
> +			if(dc->current_state->stream_status->plane_count >= MAX_SURFACES){
> +				drm_dbg_driver(crtc->dev,
> +				       "Can't enable cursor plane with %d planes\n", MAX_SURFACES);
> +				ret = -EINVAL;
> +				goto fail;
> +			}
>   			ret = drm_atomic_add_affected_planes(state, crtc);
>   			if (ret)
>   				goto fail;
> @@ -11769,8 +11775,16 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   
>   		/* Overlay cusor not subject to native cursor restrictions */
>   		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE)
> +		if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE){
> +			if(dc->current_state->stream_status->plane_count > MAX_SURFACES){
> +				drm_dbg_driver(crtc->dev,
> +				       "Can't enable cursor plane with %d planes\n", MAX_SURFACES);
> +				ret = -EINVAL;
> +				goto fail;
> +			}
> +			
>   			continue;
> +		}
>   
>   		/* Check if rotation or scaling is enabled on DCN401 */
>   		if ((drm_plane_mask(crtc->cursor) & new_crtc_state->plane_mask) &&

Re: [RFC 1/1] SWDEV476969 - dm: Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[Leo Li]

On 2024-10-25 22:01, Melissa Wen wrote:
> 
> 
> 
> On 25/10/2024 16:37, Zaeem Mohamed wrote:
>> [why]
>> Prevent index-out-of-bounds due to requiring cursor overlay when
>> plane_count is MAX_SURFACES.
> Hi Zaeem,
> 
> Thanks for working on this fix.
>>
>> [how]
>> Bounds check on plane_count when requiring overlay cursor.
> I agree. Atomic check makes sense.
> 
> 1) Since the native cursor mode was previously the unique mode avaliable, I
> wonder if the driver should fall to native cursor mode in favor of the overlay
> planes advertised. I.e. if driver says it supports two overlay planes and
> the userspace requested both, cursor overlay mode should not be available or
> should switch to native cursor mode, as before the introduction of cursor
> overlay mode.

Hey Melissa,

The overlay cursor implementation today should still do native cursor in all
cases, except for when it is not possible: if there is a underlying scaled or
YUV plane.

In such cases, we previously rejected the atomic commit, since the hw won't be
able to produce the rendering intent. Now, we try to accommodate it by using a
dedicated overlay plane. IOW, fallback to native here is equivalent to an atomic
reject.

> 
> 2) Then my second question: can we increase the number of surfaces to 4 first to
> accommodate more than one active overlay plane with cursor overly mode enabled.
> If four is still possible, this increase can reduce the number of commit
> failure scenarios and mitigate current userspace issues first. After addressing
> current array-out-of-bounds, follow-up patches can do the proper changes and
> checks.
> 

My initial thought was to merge the proper fix first to address the current
issues. But if increasing MAX_SURFACES->4 also helps, I don't have a strong
opinion about it :)

I think Zaeem is working on MAX_SURFACES->4 as well, but there's some detangling
work required in DC to accommodate another OS that dc supports. I have a feeling
this fix may land earlier than the ->4 patch. (see my patch comments below)

> 3) IMHO, the incoherence between MAX_SURFACE_NUM and MAX_SURFACE should be
> addressed before adding debugging points. For example, there are part of the
> DC code using MAX_SURFACE_NUM == MAX_PLANE == 6 to allocate dc_surface_update
> arrays, instead of using MAX_SURFACE value. You can find one of this case here:
> https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/ 
> gpu/drm/amd/display/dc/core/dc.c#L4507
> It doesn't make sense to me and it can contribute to an incomplete solution.

Right, also see below

> 
> Also, please add the references of bugs reported in the amd tracker, so far:
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3693
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594
>> Co-developed-by: Melissa Wen <mwen@igalia.com>
> I don't think I contributed enough to your code to get any credits.
> Thanks, but you can remove my co-dev-by :)
> 
> Best Regards,
> 
> Melissa
>> Signed-off-by: Zaeem Mohamed <zaeem.mohamed@amd.com>
>> ---
>>   amdgpu_dm/amdgpu_dm.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/amdgpu_dm/amdgpu_dm.c b/amdgpu_dm/amdgpu_dm.c
>> index df83e7b42b..c2595efb74 100644
>> --- a/amdgpu_dm/amdgpu_dm.c
>> +++ b/amdgpu_dm/amdgpu_dm.c
>> @@ -11676,6 +11676,12 @@ static int amdgpu_dm_atomic_check(struct drm_device 
>> *dev,
>>            * need to be added for DC to not disable a plane by mistake
>>            */
>>           if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE) {
>> +            if(dc->current_state->stream_status->plane_count >= MAX_SURFACES){
>> +                drm_dbg_driver(crtc->dev,
>> +                       "Can't enable cursor plane with %d planes\n", 
>> MAX_SURFACES);
>> +                ret = -EINVAL;
>> +                goto fail;
>> +            }

Hey Zaeem,

I took a tour through DC, and it seems to me that MAX_SURFACE_NUM can be made
equal to MAX_SURFACES in all cases. I wonder, if we simply replace
MAX_SURFACE_NUM with MAX_SURFACES = 3, will we still need these explicit fails?
FWICT, `dc_state_add_plane` should fail for us.

Thanks,
Leo

>>               ret = drm_atomic_add_affected_planes(state, crtc);
>>               if (ret)
>>                   goto fail;
>> @@ -11769,8 +11775,16 @@ static int amdgpu_dm_atomic_check(struct drm_device 
>> *dev,
>>           /* Overlay cusor not subject to native cursor restrictions */
>>           dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>> -        if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE)
>> +        if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE){
>> +            if(dc->current_state->stream_status->plane_count > MAX_SURFACES){
>> +                drm_dbg_driver(crtc->dev,
>> +                       "Can't enable cursor plane with %d planes\n", 
>> MAX_SURFACES);
>> +                ret = -EINVAL;
>> +                goto fail;
>> +            }
>> +
>>               continue;
>> +        }
>>           /* Check if rotation or scaling is enabled on DCN401 */
>>           if ((drm_plane_mask(crtc->cursor) & new_crtc_state->plane_mask) &&
> 

Re: [RFC 1/1] SWDEV476969 - dm: Fail dm_atomic_check if cursor overlay is required at MAX_SURFACES

[Melissa Wen]

On 28/10/2024 16:04, Leo Li wrote:
>
>
>
> On 2024-10-25 22:01, Melissa Wen wrote:
>>
>>
>>
>> On 25/10/2024 16:37, Zaeem Mohamed wrote:
>>> [why]
>>> Prevent index-out-of-bounds due to requiring cursor overlay when
>>> plane_count is MAX_SURFACES.
>> Hi Zaeem,
>>
>> Thanks for working on this fix.
>>>
>>> [how]
>>> Bounds check on plane_count when requiring overlay cursor.
>> I agree. Atomic check makes sense.
>>
>> 1) Since the native cursor mode was previously the unique mode 
>> avaliable, I
>> wonder if the driver should fall to native cursor mode in favor of 
>> the overlay
>> planes advertised. I.e. if driver says it supports two overlay planes 
>> and
>> the userspace requested both, cursor overlay mode should not be 
>> available or
>> should switch to native cursor mode, as before the introduction of 
>> cursor
>> overlay mode.
>
> Hey Melissa,
>
> The overlay cursor implementation today should still do native cursor 
> in all
> cases, except for when it is not possible: if there is a underlying 
> scaled or
> YUV plane.
>
> In such cases, we previously rejected the atomic commit, since the hw 
> won't be
> able to produce the rendering intent. Now, we try to accommodate it by 
> using a
> dedicated overlay plane. IOW, fallback to native here is equivalent to 
> an atomic
> reject.
>
>>
>> 2) Then my second question: can we increase the number of surfaces to 
>> 4 first to
>> accommodate more than one active overlay plane with cursor overly 
>> mode enabled.
>> If four is still possible, this increase can reduce the number of commit
>> failure scenarios and mitigate current userspace issues first. After 
>> addressing
>> current array-out-of-bounds, follow-up patches can do the proper 
>> changes and
>> checks.
>>
>
> My initial thought was to merge the proper fix first to address the 
> current
> issues. But if increasing MAX_SURFACES->4 also helps, I don't have a 
> strong
> opinion about it :)
>
> I think Zaeem is working on MAX_SURFACES->4 as well, but there's some 
> detangling
> work required in DC to accommodate another OS that dc supports. I have 
> a feeling
> this fix may land earlier than the ->4 patch. (see my patch comments 
> below)

Hi Leo,

Thanks for explaining these issues.

I thought changing MAX_SURFACES -> 4 would be faster than reworking 
atomic checks for properly handling active planes, but I understand your 
approach now, since this change impacts other OSes.

BTW, I've been away for the last few weeks and may have missed some 
updates. Any news on this?

Melissa

>
>> 3) IMHO, the incoherence between MAX_SURFACE_NUM and MAX_SURFACE 
>> should be
>> addressed before adding debugging points. For example, there are part 
>> of the
>> DC code using MAX_SURFACE_NUM == MAX_PLANE == 6 to allocate 
>> dc_surface_update
>> arrays, instead of using MAX_SURFACE value. You can find one of this 
>> case here:
>> https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/ 
>> gpu/drm/amd/display/dc/core/dc.c#L4507
>> It doesn't make sense to me and it can contribute to an incomplete 
>> solution.
>
> Right, also see below
>
>>
>> Also, please add the references of bugs reported in the amd tracker, 
>> so far:
>>
>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3693
>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3594
>>> Co-developed-by: Melissa Wen <mwen@igalia.com>
>> I don't think I contributed enough to your code to get any credits.
>> Thanks, but you can remove my co-dev-by :)
>>
>> Best Regards,
>>
>> Melissa
>>> Signed-off-by: Zaeem Mohamed <zaeem.mohamed@amd.com>
>>> ---
>>>   amdgpu_dm/amdgpu_dm.c | 16 +++++++++++++++-
>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/amdgpu_dm/amdgpu_dm.c b/amdgpu_dm/amdgpu_dm.c
>>> index df83e7b42b..c2595efb74 100644
>>> --- a/amdgpu_dm/amdgpu_dm.c
>>> +++ b/amdgpu_dm/amdgpu_dm.c
>>> @@ -11676,6 +11676,12 @@ static int amdgpu_dm_atomic_check(struct 
>>> drm_device *dev,
>>>            * need to be added for DC to not disable a plane by mistake
>>>            */
>>>           if (dm_new_crtc_state->cursor_mode == 
>>> DM_CURSOR_OVERLAY_MODE) {
>>> + if(dc->current_state->stream_status->plane_count >= MAX_SURFACES){
>>> +                drm_dbg_driver(crtc->dev,
>>> +                       "Can't enable cursor plane with %d 
>>> planes\n", MAX_SURFACES);
>>> +                ret = -EINVAL;
>>> +                goto fail;
>>> +            }
>
> Hey Zaeem,
>
> I took a tour through DC, and it seems to me that MAX_SURFACE_NUM can 
> be made
> equal to MAX_SURFACES in all cases. I wonder, if we simply replace
> MAX_SURFACE_NUM with MAX_SURFACES = 3, will we still need these 
> explicit fails?
> FWICT, `dc_state_add_plane` should fail for us.
>
> Thanks,
> Leo
>
>>>               ret = drm_atomic_add_affected_planes(state, crtc);
>>>               if (ret)
>>>                   goto fail;
>>> @@ -11769,8 +11775,16 @@ static int amdgpu_dm_atomic_check(struct 
>>> drm_device *dev,
>>>           /* Overlay cusor not subject to native cursor restrictions */
>>>           dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>> -        if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE)
>>> +        if (dm_new_crtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE){
>>> + if(dc->current_state->stream_status->plane_count > MAX_SURFACES){
>>> +                drm_dbg_driver(crtc->dev,
>>> +                       "Can't enable cursor plane with %d 
>>> planes\n", MAX_SURFACES);
>>> +                ret = -EINVAL;
>>> +                goto fail;
>>> +            }
>>> +
>>>               continue;
>>> +        }
>>>           /* Check if rotation or scaling is enabled on DCN401 */
>>>           if ((drm_plane_mask(crtc->cursor) & 
>>> new_crtc_state->plane_mask) &&
>>
>