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

[Vlad Zahorodnii <vlad.zahorodnii@xxxxxxx>]

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@xxxxxxx>
>
> 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@xxxxxxx>
>
> 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) {