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

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	Stephen Boyd <swboyd@chromium.org>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Andersson <andersson@kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<dri-devel@lists.freedesktop.org>,
	<freedreno@lists.freedesktop.org>
Subject: Re: [RFC PATCH v2 02/13] drm/msm/dpu: take plane rotation into account for wide planes
Date: Mon, 15 May 2023 11:45:06 -0700	[thread overview]
Message-ID: <94d4bc54-74c5-f565-a75e-766fdc458f75@quicinc.com> (raw)
In-Reply-To: <CAA8EJppbXavJCT4ErBoW2cBjRoabFK58UQ39T6h96Ovm8yMdEQ@mail.gmail.com>

On 5/14/2023 10:01 AM, Dmitry Baryshkov wrote:
> On Sat, 13 May 2023 at 01:12, Abhinav Kumar <quic_abhinavk@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@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.
> 

Ack, and my bad for not checking that r_pipe_cfg points to a field in 
pstate but .... it was just weird though that you are checking for 
r_pipe->sspp before calling a method which really doesnt care if its 
null or not. How about you use drm_rect_visible(r_pipe_cfg->src_rect)

If its not set, it wont be visible too.

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

Ok i got it now. Instead of directly operating on the plane_state's 
rectangle which makes you to invert again why not just use a temporary 
drm_rect which stores the rotated pipe_cfg->src_rect. That way you dont 
have to invert anything?

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