Re: [PATCH v2] drm/amd/display: Use oriented source size when checking cursor scaling

[Vlad Zahorodnii <vlad.zahorodnii@kde.org>]

On 12/2/21 16:54, Kazlauskas, Nicholas wrote:
> On 2021-12-02 7:52 a.m., Vlad Zahorodnii wrote:
>> dm_check_crtc_cursor() doesn't take into account plane transforms when
>> calculating plane scaling, this can result in false positives.
>>
>> For example, if there's an output with resolution 3840x2160 and the
>> output is rotated 90 degrees, CRTC_W and CRTC_H will be 3840 and 2160,
>> respectively, but SRC_W and SRC_H will be 2160 and 3840, respectively.
>>
>> Since the cursor plane usually has a square buffer attached to it, the
>> dm_check_crtc_cursor() will think that there's a scale factor mismatch
>> even though there isn't really.
>>
>> This fixes an issue where kwin fails to use hardware plane transforms.
>>
>> Changes since version 1:
>> - s/orientated/oriented/g
>>
>> Signed-off-by: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
> 
> This looks correct to me. I guess it's also not modifying the actual 
> programming position, just the check to ensure that the cursor is going 
> to be unscaled in the correct orientation.
> 
> Would be good to have some IGT tests for these scaled cases to verify 
> atomic check pass/fail assumptions, but for now:

I'd be glad to add the tests, but I would need some assistance. Can we 
continue this conversation in the radeon irc channel (I'm zzag)?

> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> Regards,
> Nicholas Kazlauskas
> 
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++++++++++++++-----
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> 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 a3c0f2e4f4c1..c009c668fbe2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10736,6 +10736,24 @@ static int dm_update_plane_state(struct dc *dc,
>>       return ret;
>>   }
>> +static void dm_get_oriented_plane_size(struct drm_plane_state 
>> *plane_state,
>> +                       int *src_w, int *src_h)
>> +{
>> +    switch (plane_state->rotation & DRM_MODE_ROTATE_MASK) {
>> +    case DRM_MODE_ROTATE_90:
>> +    case DRM_MODE_ROTATE_270:
>> +        *src_w = plane_state->src_h >> 16;
>> +        *src_h = plane_state->src_w >> 16;
>> +        break;
>> +    case DRM_MODE_ROTATE_0:
>> +    case DRM_MODE_ROTATE_180:
>> +    default:
>> +        *src_w = plane_state->src_w >> 16;
>> +        *src_h = plane_state->src_h >> 16;
>> +        break;
>> +    }
>> +}
>> +
>>   static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>>                   struct drm_crtc *crtc,
>>                   struct drm_crtc_state *new_crtc_state)
>> @@ -10744,6 +10762,8 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>       struct drm_plane_state *new_cursor_state, *new_underlying_state;
>>       int i;
>>       int cursor_scale_w, cursor_scale_h, underlying_scale_w, 
>> underlying_scale_h;
>> +    int cursor_src_w, cursor_src_h;
>> +    int underlying_src_w, underlying_src_h;
>>       /* On DCE and DCN there is no dedicated hardware cursor plane. 
>> We get a
>>        * cursor per pipe but it's going to inherit the scaling and
>> @@ -10755,10 +10775,9 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>           return 0;
>>       }
>> -    cursor_scale_w = new_cursor_state->crtc_w * 1000 /
>> -             (new_cursor_state->src_w >> 16);
>> -    cursor_scale_h = new_cursor_state->crtc_h * 1000 /
>> -             (new_cursor_state->src_h >> 16);
>> +    dm_get_oriented_plane_size(new_cursor_state, &cursor_src_w, 
>> &cursor_src_h);
>> +    cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
>> +    cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
>>       for_each_new_plane_in_state_reverse(state, underlying, 
>> new_underlying_state, i) {
>>           /* Narrow down to non-cursor planes on the same CRTC as the 
>> cursor */
>> @@ -10769,10 +10788,10 @@ static int dm_check_crtc_cursor(struct 
>> drm_atomic_state *state,
>>           if (!new_underlying_state->fb)
>>               continue;
>> -        underlying_scale_w = new_underlying_state->crtc_w * 1000 /
>> -                     (new_underlying_state->src_w >> 16);
>> -        underlying_scale_h = new_underlying_state->crtc_h * 1000 /
>> -                     (new_underlying_state->src_h >> 16);
>> +        dm_get_oriented_plane_size(new_underlying_state,
>> +                       &underlying_src_w, &underlying_src_h);
>> +        underlying_scale_w = new_underlying_state->crtc_w * 1000 / 
>> underlying_src_w;
>> +        underlying_scale_h = new_underlying_state->crtc_h * 1000 / 
>> underlying_src_h;
>>           if (cursor_scale_w != underlying_scale_w ||
>>               cursor_scale_h != underlying_scale_h) {
>>
>