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