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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 5/15/2023 12:12 PM, Dmitry Baryshkov wrote:
On Mon, 15 May 2023 at 21:45, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



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

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






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux