Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 于2025年1月16日周四 16:00写道：
>
> On Thu, Jan 16, 2025 at 03:25:59PM +0800, Jun Nie wrote:
> > Store pipes in array with removing dedicated r_pipe. There are
> > 2 pipes in a drm plane at most currently, while 4 pipes are
> > required for quad-pipe case. Generalize the handling to pipe pair
> > and ease handling to another pipe pair later.
>
> With the first sentence being moved to the end of the commit message:
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> Minor issues below, please address them in the next version.
>
> >
> > Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 35 +++----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 167 +++++++++++++++++-------------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 12 +--
> > 3 files changed, 112 insertions(+), 102 deletions(-)
>
> > @@ -853,6 +855,9 @@ static int dpu_plane_atomic_check_nosspp(struct drm_plane *plane,
> > return -EINVAL;
> > }
> >
> > + /* move the assignment here, to ease handling to another pairs later */
>
> Is it a TODO comment? It reads like an order.
>
> > + pipe_cfg = &pstate->pipe_cfg[0];
> > + r_pipe_cfg = &pstate->pipe_cfg[1];
> > /* state->src is 16.16, src_rect is not */
> > drm_rect_fp_to_int(&pipe_cfg->src_rect, &new_plane_state->src);
> >
>
> > @@ -1387,17 +1394,28 @@ static void _dpu_plane_atomic_disable(struct drm_plane *plane)
> > {
> > struct drm_plane_state *state = plane->state;
> > struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> > - struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> > + struct dpu_sw_pipe *pipe;
> > + int i;
> >
> > - trace_dpu_plane_disable(DRMID(plane), false,
> > - pstate->pipe.multirect_mode);
> > + for (i = 0; i < PIPES_PER_STAGE; i += 1) {
> > + pipe = &pstate->pipe[i];
> > + if (!pipe->sspp)
> > + continue;
> >
> > - if (r_pipe->sspp) {
> > - r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > - r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > + trace_dpu_plane_disable(DRMID(plane), false,
> > + pstate->pipe[i].multirect_mode);
> >
> > - if (r_pipe->sspp->ops.setup_multirect)
> > - r_pipe->sspp->ops.setup_multirect(r_pipe);
> > + /*
> > + * clear multirect for the right pipe so that the SSPP
> > + * can be further reused in the solo mode
> > + */
> > + if (pipe->sspp && i == 1) {
>
> Wouldn't it be better to `&& i % 2 != 0`? Then, I think, this condition
> can stay even in quad-pipe case.
If all pipes are in solo mode, there is no need to test ' i %2 != 0 '. Below
test shall be better, right?
if (pipe->sspp && pipe->multirect_index == DPU_SSPP_RECT_1)
>
> > + pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > + pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +
> > + if (pipe->sspp->ops.setup_multirect)
> > + pipe->sspp->ops.setup_multirect(pipe);
> > + }
> > }
> >
> > pstate->pending = true;
>
> --
> With best wishes
> Dmitry
