[RFC PATCH v2 02/13] drm/msm/dpu: take plane rotation into account for wide planes

[Dmitry Baryshkov]

On Sat, 13 May 2023 at 01:12, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
> > Take into account the plane rotation and flipping when calculating src
> > positions for the wide plane parts.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>
> Do we need to have a fixes tag for this? This means we dont consider
> rotation while calculating src position today which is a bug?

Hmm, I thought that I had a check forbidding rotation with the current
approach, but I don't see it. Most probably I thought about it and
then forgot to add it.
The proper fix should be to disallow it for static SSPP case. I'll
include the patch into v3.

>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 ++++++++++++++---------
> >   1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 2e63eb0a2f3f..d43e04fc4578 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -887,16 +887,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >               return -EINVAL;
> >       }
> >
> > -     pipe_cfg->src_rect = new_plane_state->src;
> > -
> > -     /* state->src is 16.16, src_rect is not */
> > -     pipe_cfg->src_rect.x1 >>= 16;
> > -     pipe_cfg->src_rect.x2 >>= 16;
> > -     pipe_cfg->src_rect.y1 >>= 16;
> > -     pipe_cfg->src_rect.y2 >>= 16;
> > -
> > -     pipe_cfg->dst_rect = new_plane_state->dst;
> > -
> >       fb_rect.x2 = new_plane_state->fb->width;
> >       fb_rect.y2 = new_plane_state->fb->height;
> >
> > @@ -912,6 +902,15 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >
> >       max_linewidth = pdpu->catalog->caps->max_linewidth;
> >
> > +     /* state->src is 16.16, src_rect is not */
> > +     drm_rect_fp_to_int(&pipe_cfg->src_rect, &new_plane_state->src);
> > +
> > +     pipe_cfg->dst_rect = new_plane_state->dst;
> > +
> > +     drm_rect_rotate(&pipe_cfg->src_rect,
> > +                     new_plane_state->fb->width, new_plane_state->fb->height,
> > +                     new_plane_state->rotation);
> > +
> >       if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> >               /*
> >                * In parallel multirect case only the half of the usual width
> > @@ -959,6 +958,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >               r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
> >       }
> >
> > +     drm_rect_rotate_inv(&pipe_cfg->src_rect,
> > +                         new_plane_state->fb->width, new_plane_state->fb->height,
> > +                         new_plane_state->rotation);
> > +     if (r_pipe->sspp)
>
> Dont you need to check for if (r_pipe_cfg) here and not if
> (r_pipe->sspp) because parameter you are passing is the r_pipe_cfg to
> drm_rect_rotate_inv().

Of course not. r_pipe_cfg is a pointer to the field in pstate. We know
that it can not be NULL.

>
> So we rotated the pipe_cfg once, then rotated_inv it to restore the
> rectangle to its original state, but r_pipe_cfg's rectangle was never
> rotated as it was not allocated before this function so it will remain
> in inverse rotated state now right?

No. r_pipe_cfg is set beforehand to the half of the rotated pipe_cfg.

> > +             drm_rect_rotate_inv(&r_pipe_cfg->src_rect,
> > +                                 new_plane_state->fb->width, new_plane_state->fb->height,
> > +                                 new_plane_state->rotation);
> > +
> >       ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
> >       if (ret)
> >               return ret;



--
With best wishes
Dmitry