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

[Abhinav Kumar]

On 5/15/2023 12:12 PM, Dmitry Baryshkov wrote:
> On Mon, 15 May 2023 at 21:45, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 5/14/2023 10:01 AM, Dmitry Baryshkov wrote:
>>> 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.
>>>
>>
>> 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.
> 
> I think it is better for the uniformity to check for r_pipe->sspp:
> this is the condition that is used all over the driver to check that
> r_pipe is used.
> 

hmmm .... okay .... not entirely convinced this was the right way to 
begin with then because some places do need a valid sspp for the 
function getting called so thats fine but some do not.

its incorrect uniformity, but I am not going to complain about it now. 
will think of cleaning it up once this lands.

>>
>>>>
>>>> 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?
> 
> I don't think this will work. I explicitly rotate & invert rotation to
> get correct coordinates for both source and destination rectangles.
> Doing it otherwise would require us to manually implement this in the
> DPU driver.
> 

Ok got it, i guess this will need more changes within the if (src_width 
 > max_width) .... this is fine then.

Will ack this once i finish reviews on the others.

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