| Message ID | 20250116-sm8650-v6-13-hmd-deckard-mdss-quad-upstream-33-v4-10-74749c6eba33@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: Support quad pipe with dual-DSI | expand |
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@linaro.org> Minor issues below, please address them in the next version. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > 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;
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于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@linaro.org> > > Minor issues below, please address them in the next version. > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > --- > > 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
Jun Nie <jun.nie@linaro.org> 于2025年1月16日周四 17:49写道： > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于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@linaro.org> > > > > Minor issues below, please address them in the next version. > > > > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > > --- > > > 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) See your comments for later patch. Let's keep it as: i % PIPES_PER_STAGE != 0 > > > > > > + 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
On Thu, Jan 16, 2025 at 05:49:43PM +0800, Jun Nie wrote: > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于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@linaro.org> > > > > Minor issues below, please address them in the next version. > > > > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > > --- > > > 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) Again, this will not work as expected for the SSPP-sharing case as it will then clear pipe 0 for the sharing planes. Let me think a bit... This code resets multirect for right pipes. It was added back in 4.9 to fix the case of 'master' aka RECT_0 not being a part of the atomic update. However I don't think this is applicable anymore. We use z_pos normalization, so all planes for a CRTC are added to the commit. Please drop this piece in a separate commit. > > > > > > + 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
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年1月16日周四 18:08写道： > > On Thu, Jan 16, 2025 at 05:49:43PM +0800, Jun Nie wrote: > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于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@linaro.org> > > > > > > Minor issues below, please address them in the next version. > > > > > > > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > > > --- > > > > 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) > > Again, this will not work as expected for the SSPP-sharing case as it > will then clear pipe 0 for the sharing planes. > > Let me think a bit... This code resets multirect for right pipes. It was > added back in 4.9 to fix the case of 'master' aka RECT_0 not being a > part of the atomic update. However I don't think this is applicable > anymore. We use z_pos normalization, so all planes for a CRTC are added > to the commit. Please drop this piece in a separate commit. You mean only testing sspp as below? We have to handle the default non-shared case as existing implementation. Maybe we add a flag after sharing SSPP among planes? Otherwise, how to distinct the shared SSPP case and disable multi-rect mode in non-shared case? if (pipe->sspp) { > > > > > > > > > > + 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 > > -- > With best wishes > Dmitry
On Thu, Jan 16, 2025 at 11:36:21PM +0800, Jun Nie wrote: > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2025年1月16日周四 18:08写道： > > > > On Thu, Jan 16, 2025 at 05:49:43PM +0800, Jun Nie wrote: > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于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@linaro.org> > > > > > > > > Minor issues below, please address them in the next version. > > > > > > > > > > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > > > > --- > > > > > 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) > > > > Again, this will not work as expected for the SSPP-sharing case as it > > will then clear pipe 0 for the sharing planes. > > > > Let me think a bit... This code resets multirect for right pipes. It was > > added back in 4.9 to fix the case of 'master' aka RECT_0 not being a > > part of the atomic update. However I don't think this is applicable > > anymore. We use z_pos normalization, so all planes for a CRTC are added > > to the commit. Please drop this piece in a separate commit. > > You mean only testing sspp as below? We have to handle the default > non-shared case as existing implementation. Maybe we add a flag after > sharing SSPP among planes? Otherwise, how to distinct the shared > SSPP case and disable multi-rect mode in non-shared case? > > if (pipe->sspp) { I was thinking about dropping this piece of code completely, but we can do it afterwards. Note, that you check for !pipe->sspp few lines above, so the code can become: for (i = 0; i < PIPES_PER_STAGE; i++) { pipe = &pstate->pipe[i]; if (!pipe->sspp) continue; if (i % PIPES_PER_STAGE == 0) continue; pipe->multirect_index = DPU_SSPP_RECT_SOLO; ... } > > > > > > > > > > > > > > > + 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 > > > > -- > > With best wishes > > Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 05abe2d05d8d8..193818b02197d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -442,7 +442,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, const struct msm_format *format; struct dpu_hw_ctl *ctl = mixer->lm_ctl; - uint32_t lm_idx; + uint32_t lm_idx, i; bool bg_alpha_enable = false; DECLARE_BITMAP(fetch_active, SSPP_MAX); @@ -463,20 +463,15 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable) bg_alpha_enable = true; - set_bit(pstate->pipe.sspp->idx, fetch_active); - _dpu_crtc_blend_setup_pipe(crtc, plane, - mixer, cstate->num_mixers, - pstate->stage, - format, fb ? fb->modifier : 0, - &pstate->pipe, 0, stage_cfg); - - if (pstate->r_pipe.sspp) { - set_bit(pstate->r_pipe.sspp->idx, fetch_active); + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + set_bit(pstate->pipe[i].sspp->idx, fetch_active); _dpu_crtc_blend_setup_pipe(crtc, plane, mixer, cstate->num_mixers, pstate->stage, format, fb ? fb->modifier : 0, - &pstate->r_pipe, 1, stage_cfg); + &pstate->pipe[i], i, stage_cfg); } /* blend config update */ @@ -1440,15 +1435,15 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) seq_printf(s, "\tdst x:%4d dst_y:%4d dst_w:%4d dst_h:%4d\n", state->crtc_x, state->crtc_y, state->crtc_w, state->crtc_h); - seq_printf(s, "\tsspp[0]:%s\n", - pstate->pipe.sspp->cap->name); - seq_printf(s, "\tmultirect[0]: mode: %d index: %d\n", - pstate->pipe.multirect_mode, pstate->pipe.multirect_index); - if (pstate->r_pipe.sspp) { - seq_printf(s, "\tsspp[1]:%s\n", - pstate->r_pipe.sspp->cap->name); - seq_printf(s, "\tmultirect[1]: mode: %d index: %d\n", - pstate->r_pipe.multirect_mode, pstate->r_pipe.multirect_index); + + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + seq_printf(s, "\tsspp[%d]:%s\n", + i, pstate->pipe[i].sspp->cap->name); + seq_printf(s, "\tmultirect[%d]: mode: %d index: %d\n", + i, pstate->pipe[i].multirect_mode, + pstate->pipe[i].multirect_index); } seq_puts(s, "\n"); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 2b75a6cf4e670..1adbf91be850f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -619,6 +619,7 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu, struct msm_drm_private *priv = plane->dev->dev_private; struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state); u32 fill_color = (color & 0xFFFFFF) | ((alpha & 0xFF) << 24); + int i; DPU_DEBUG_PLANE(pdpu, "\n"); @@ -632,12 +633,13 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu, return; /* update sspp */ - _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg.dst_rect, - fill_color, fmt); - - if (pstate->r_pipe.sspp) - _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg.dst_rect, + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + _dpu_plane_color_fill_pipe(pstate, &pstate->pipe[i], + &pstate->pipe_cfg[i].dst_rect, fill_color, fmt); + } } static int dpu_plane_prepare_fb(struct drm_plane *plane, @@ -827,8 +829,8 @@ static int dpu_plane_atomic_check_nosspp(struct drm_plane *plane, struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate; struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); - struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; - struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; + struct dpu_sw_pipe_cfg *pipe_cfg; + struct dpu_sw_pipe_cfg *r_pipe_cfg; struct drm_rect fb_rect = { 0 }; uint32_t max_linewidth; @@ -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 */ + 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); @@ -949,10 +954,10 @@ static int dpu_plane_atomic_check_sspp(struct drm_plane *plane, drm_atomic_get_new_plane_state(state, plane); struct dpu_plane *pdpu = to_dpu_plane(plane); struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); - struct dpu_sw_pipe *pipe = &pstate->pipe; - struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; - struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; - struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; + struct dpu_sw_pipe *pipe = &pstate->pipe[0]; + struct dpu_sw_pipe *r_pipe = &pstate->pipe[1]; + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg[0]; + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->pipe_cfg[1]; int ret = 0; ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, @@ -1011,10 +1016,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, struct dpu_plane *pdpu = to_dpu_plane(plane); struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); - struct dpu_sw_pipe *pipe = &pstate->pipe; - struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; - struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; - struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; + struct dpu_sw_pipe *pipe = &pstate->pipe[0]; + struct dpu_sw_pipe *r_pipe = &pstate->pipe[1]; + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg[0]; + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->pipe_cfg[1]; const struct drm_crtc_state *crtc_state = NULL; uint32_t max_linewidth = dpu_kms->catalog->caps->max_linewidth; @@ -1058,7 +1063,7 @@ static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, drm_atomic_get_old_plane_state(state, plane); struct dpu_plane_state *pstate = to_dpu_plane_state(plane_state); struct drm_crtc_state *crtc_state; - int ret; + int ret, i; if (plane_state->crtc) crtc_state = drm_atomic_get_new_crtc_state(state, @@ -1073,8 +1078,8 @@ static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, * resources are freed by dpu_crtc_assign_plane_resources(), * but clean them here. */ - pstate->pipe.sspp = NULL; - pstate->r_pipe.sspp = NULL; + for (i = 0; i < PIPES_PER_STAGE; i++) + pstate->pipe[i].sspp = NULL; return 0; } @@ -1111,19 +1116,21 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, struct dpu_sw_pipe_cfg *pipe_cfg; struct dpu_sw_pipe_cfg *r_pipe_cfg; const struct msm_format *fmt; + int i; if (plane_state->crtc) crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc); pstate = to_dpu_plane_state(plane_state); - pipe = &pstate->pipe; - r_pipe = &pstate->r_pipe; - pipe_cfg = &pstate->pipe_cfg; - r_pipe_cfg = &pstate->r_pipe_cfg; - pipe->sspp = NULL; - r_pipe->sspp = NULL; + pipe = &pstate->pipe[0]; + r_pipe = &pstate->pipe[1]; + pipe_cfg = &pstate->pipe_cfg[0]; + r_pipe_cfg = &pstate->pipe_cfg[1]; + + for (i = 0; i < PIPES_PER_STAGE; i++) + pstate->pipe[i].sspp = NULL; if (!plane_state->fb) return -EINVAL; @@ -1213,6 +1220,7 @@ void dpu_plane_flush(struct drm_plane *plane) { struct dpu_plane *pdpu; struct dpu_plane_state *pstate; + int i; if (!plane || !plane->state) { DPU_ERROR("invalid plane\n"); @@ -1233,8 +1241,8 @@ void dpu_plane_flush(struct drm_plane *plane) /* force 100% alpha */ _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF); else { - dpu_plane_flush_csc(pdpu, &pstate->pipe); - dpu_plane_flush_csc(pdpu, &pstate->r_pipe); + for (i = 0; i < PIPES_PER_STAGE; i++) + dpu_plane_flush_csc(pdpu, &pstate->pipe[i]); } /* flag h/w flush complete */ @@ -1335,15 +1343,12 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane, struct dpu_plane *pdpu = to_dpu_plane(plane); struct drm_plane_state *state = plane->state; struct dpu_plane_state *pstate = to_dpu_plane_state(state); - struct dpu_sw_pipe *pipe = &pstate->pipe; - struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; struct drm_crtc *crtc = state->crtc; struct drm_framebuffer *fb = state->fb; bool is_rt_pipe; const struct msm_format *fmt = msm_framebuffer_format(fb); - struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; - struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; + int i; pstate->pending = true; @@ -1358,12 +1363,12 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane, crtc->base.id, DRM_RECT_ARG(&state->dst), &fmt->pixel_format, MSM_FORMAT_IS_UBWC(fmt)); - dpu_plane_sspp_update_pipe(plane, pipe, pipe_cfg, fmt, - drm_mode_vrefresh(&crtc->mode), - &pstate->layout); - - if (r_pipe->sspp) { - dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt, + /* move the assignment here, to ease handling to another pairs later */ + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + dpu_plane_sspp_update_pipe(plane, &pstate->pipe[i], + &pstate->pipe_cfg[i], fmt, drm_mode_vrefresh(&crtc->mode), &pstate->layout); } @@ -1371,15 +1376,17 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane, if (pstate->needs_qos_remap) pstate->needs_qos_remap = false; - pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, - &crtc->mode, pipe_cfg); - - pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg); - - if (r_pipe->sspp) { - pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg); + pstate->plane_fetch_bw = 0; + pstate->plane_clk = 0; + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, + &crtc->mode, &pstate->pipe_cfg[i]); - pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg)); + pstate->plane_clk = max(pstate->plane_clk, + _dpu_plane_calc_clk(&crtc->mode, + &pstate->pipe_cfg[i])); } } @@ -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) { + 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; @@ -1492,28 +1510,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p, const struct drm_plane_state *state) { const struct dpu_plane_state *pstate = to_dpu_plane_state(state); - const struct dpu_sw_pipe *pipe = &pstate->pipe; - const struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; - const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; - const struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; + const struct dpu_sw_pipe *pipe; + const struct dpu_sw_pipe_cfg *pipe_cfg; + int i; drm_printf(p, "\tstage=%d\n", pstate->stage); - drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); - drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode)); - drm_printf(p, "\tmultirect_index[0]=%s\n", - dpu_get_multirect_index(pipe->multirect_index)); - drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); - drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect)); - - if (r_pipe->sspp) { - drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name); - drm_printf(p, "\tmultirect_mode[1]=%s\n", - dpu_get_multirect_mode(r_pipe->multirect_mode)); - drm_printf(p, "\tmultirect_index[1]=%s\n", - dpu_get_multirect_index(r_pipe->multirect_index)); - drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect)); - drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect)); + for (i = 0; i < PIPES_PER_STAGE; i++) { + pipe = &pstate->pipe[i]; + if (!pipe->sspp) + continue; + pipe_cfg = &pstate->pipe_cfg[i]; + drm_printf(p, "\tsspp[%d]=%s\n", i, pipe->sspp->cap->name); + drm_printf(p, "\tmultirect_mode[%d]=%s\n", i, + dpu_get_multirect_mode(pipe->multirect_mode)); + drm_printf(p, "\tmultirect_index[%d]=%s\n", i, + dpu_get_multirect_index(pipe->multirect_index)); + drm_printf(p, "\tsrc[%d]=" DRM_RECT_FMT "\n", i, + DRM_RECT_ARG(&pipe_cfg->src_rect)); + drm_printf(p, "\tdst[%d]=" DRM_RECT_FMT "\n", i, + DRM_RECT_ARG(&pipe_cfg->dst_rect)); } } @@ -1551,14 +1567,17 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) struct dpu_plane *pdpu = to_dpu_plane(plane); struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state); struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); + int i; if (!pdpu->is_rt_pipe) return; pm_runtime_get_sync(&dpu_kms->pdev->dev); - _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable); - if (pstate->r_pipe.sspp) - _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable); + for (i = 0; i < PIPES_PER_STAGE; i++) { + if (!pstate->pipe[i].sspp) + continue; + _dpu_plane_set_qos_ctrl(plane, &pstate->pipe[i], enable); + } pm_runtime_put_sync(&dpu_kms->pdev->dev); } #endif diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index acd5725175cdd..052fd046e8463 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -18,10 +18,8 @@ * struct dpu_plane_state: Define dpu extension of drm plane state object * @base: base drm plane state object * @aspace: pointer to address space for input/output buffers - * @pipe: software pipe description - * @r_pipe: software pipe description of the second pipe - * @pipe_cfg: software pipe configuration - * @r_pipe_cfg: software pipe configuration for the second pipe + * @pipe: software pipe description array + * @pipe_cfg: software pipe configuration array * @stage: assigned by crtc blender * @needs_qos_remap: qos remap settings need to be updated * @multirect_index: index of the rectangle of SSPP @@ -35,10 +33,8 @@ struct dpu_plane_state { struct drm_plane_state base; struct msm_gem_address_space *aspace; - struct dpu_sw_pipe pipe; - struct dpu_sw_pipe r_pipe; - struct dpu_sw_pipe_cfg pipe_cfg; - struct dpu_sw_pipe_cfg r_pipe_cfg; + struct dpu_sw_pipe pipe[PIPES_PER_STAGE]; + struct dpu_sw_pipe_cfg pipe_cfg[PIPES_PER_STAGE]; enum dpu_stage stage; bool needs_qos_remap; bool pending;
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. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- 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(-)