| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABymUCO63-V7MoWpgCTEV_8R_4rVHM-1=eyRP34=OdKGpYSLDQ@mail.gmail.com>
Date: Thu, 31 Jul 2025 23:37:14 +0800
From: Jun Nie <jun.nie@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Rob Clark <robdclark@...il.com>, Abhinav Kumar <quic_abhinavk@...cinc.com>,
Dmitry Baryshkov <lumag@...nel.org>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Jessica Zhang <quic_jesszhan@...cinc.com>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v13 10/12] drm/msm/dpu: support SSPP assignment for
quad-pipe case
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年7月31日周四 22:22写道：
>
> On 31/07/2025 13:52, Jun Nie wrote:
> > Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年7月31日周四 02:52写道：
> >>
> >> On Mon, Jul 28, 2025 at 09:14:34PM +0800, Jun Nie wrote:
> >>> Currently, SSPPs are assigned to a maximum of two pipes. However,
> >>> quad-pipe usage scenarios require four pipes and involve configuring
> >>> two stages. In quad-pipe case, the first two pipes share a set of
> >>> mixer configurations and enable multi-rect mode when certain
> >>> conditions are met. The same applies to the subsequent two pipes.
> >>>
> >>> Assign SSPPs to the pipes in each stage using a unified method and
> >>> to loop the stages accordingly.
> >>>
> >>> Signed-off-by: Jun Nie <jun.nie@...aro.org>
> >>> ---
> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 160 ++++++++++++++++++------------
> >>> 1 file changed, 99 insertions(+), 61 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> index 55429f29a4b95594771d930efe42aaa4126f6f07..e1e16a8d5ac55ba52a0f460d62901dced65e3a9e 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> @@ -959,6 +959,30 @@ static int dpu_plane_is_multirect_parallel_capable(struct dpu_hw_sspp *sspp,
> >>> }
> >>>
> >>>
> >>> +static bool dpu_plane_get_single_pipe_in_stage(struct dpu_plane_state *pstate,
> >>> + struct dpu_sw_pipe **single_pipe,
> >>> + struct dpu_sw_pipe_cfg **single_pipe_cfg,
> >>> + int stage_index)
> >>> +{
> >>> + int pipe_idx, i, valid_pipe = 0;
> >>> +
> >>> + for (i = 0; i < PIPES_PER_STAGE; i++) {
> >>
> >> Why do you need to loop here? Is there a case when pipe 0 is not
> >> assigned, but pipe 1 is?
> >
> > Loop the pipe in a stage to count the valid pipes. If there are 2 valid
> > pipes in stage of the current plane, this function will return false.
> > Or you prefer the below coding?
> >
> > 1029 pipe_idx = stage_index * PIPES_PER_STAGE;
> > 1030 if (drm_rect_width(&pstate->pipe_cfg[pipe_idx].src_rect) != 0 &&
> > 1031 drm_rect_width(&pstate->pipe_cfg[pipe_idx +
> > 1].src_rect) == 0) {
>
> Yes, this is better from my POV. But the bigger question is why do you
> need it at all? What is wrong with the existing check? Can it be that
> pipe0 is not used, but pipe1 is used?
There is no case that pipe0 is not used, but pipe1 is used. Existing check
does not filter the plane which contains single pipe in a stage, which can
be a candidate for SSPP sharing. If the stage contains 2 valid pipes or
no valid pipes, it is skipped in dpu_plane_try_multirect_shared(), or it is
skipped to be stored in prev_adjacent_plane_state[].
>
> > 1032 if (single_pipe)
>
> You don't need these ifs. You always pass a valid pointer.
OK, a valid pointer can be passed though the return value may not be needed.
>
> > 1033 *single_pipe = &pstate->pipe[pipe_idx];
> > 1034 if (single_pipe_cfg)
> > 1035 *single_pipe_cfg =
> > &pstate->pipe_cfg[pipe_idx];
> > 1036 return true;
> > 1037 }
> > 1038
> > 1039 return false;
> >
> >>
> >>> + pipe_idx = stage_index * PIPES_PER_STAGE + i;
> >>> + if (drm_rect_width(&pstate->pipe_cfg[pipe_idx].src_rect) != 0) {
> >>> + valid_pipe++;
> >>> + if (valid_pipe > 1)
> >>> + return false;
> >>> +
> >>> + if (single_pipe)
> >>> + *single_pipe = &pstate->pipe[pipe_idx];
> >>> + if (single_pipe_cfg)
> >>> + *single_pipe_cfg = &pstate->pipe_cfg[pipe_idx];
> >>> + }
> >>> + }
> >>> +
> >>> + return valid_pipe == 1;
> >>> +}
> >>> +
> >>> static int dpu_plane_atomic_check_sspp(struct drm_plane *plane,
> >>> struct drm_atomic_state *state,
> >>> const struct drm_crtc_state *crtc_state)
> >>> @@ -1023,17 +1047,20 @@ static bool dpu_plane_try_multirect_parallel(struct dpu_sw_pipe *pipe, struct dp
> >>> static int dpu_plane_try_multirect_shared(struct dpu_plane_state *pstate,
> >>> struct dpu_plane_state *prev_adjacent_pstate,
> >>> const struct msm_format *fmt,
> >>> - uint32_t max_linewidth)
> >>> + uint32_t max_linewidth, int stage_index)
> >>> {
> >>> - 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 *prev_pipe = &prev_adjacent_pstate->pipe[0];
> >>> - struct dpu_sw_pipe_cfg *prev_pipe_cfg = &prev_adjacent_pstate->pipe_cfg[0];
> >>> + struct dpu_sw_pipe *pipe, *prev_pipe;
> >>> + struct dpu_sw_pipe_cfg *pipe_cfg, *prev_pipe_cfg;
> >>> const struct msm_format *prev_fmt = msm_framebuffer_format(prev_adjacent_pstate->base.fb);
> >>> u16 max_tile_height = 1;
> >>>
> >>> - if (prev_adjacent_pstate->pipe[1].sspp != NULL ||
> >>> + if (!dpu_plane_get_single_pipe_in_stage(pstate, &pipe,
> >>> + &pipe_cfg, stage_index))
> >>> + return false;
> >>> +
> >>> + if (!dpu_plane_get_single_pipe_in_stage(prev_adjacent_pstate,
> >>> + &prev_pipe, &prev_pipe_cfg,
> >>> + stage_index) ||
> >>> prev_pipe->multirect_mode != DPU_SSPP_MULTIRECT_NONE)
> >>> return false;
> >>>
> >>> @@ -1048,11 +1075,6 @@ static int dpu_plane_try_multirect_shared(struct dpu_plane_state *pstate,
> >>> if (MSM_FORMAT_IS_UBWC(prev_fmt))
> >>> max_tile_height = max(max_tile_height, prev_fmt->tile_height);
> >>>
> >>> - r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> - r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> -
> >>> - r_pipe->sspp = NULL;
> >>> -
> >>> if (dpu_plane_is_parallel_capable(pipe_cfg, fmt, max_linewidth) &&
> >>> dpu_plane_is_parallel_capable(prev_pipe_cfg, prev_fmt, max_linewidth) &&
> >>> (pipe_cfg->dst_rect.x1 >= prev_pipe_cfg->dst_rect.x2 ||
> >>> @@ -1181,36 +1203,69 @@ static int dpu_plane_virtual_atomic_check(struct drm_plane *plane,
> >>> return 0;
> >>> }
> >>>
> >>> +static int dpu_plane_assign_resource_in_stage(struct dpu_sw_pipe *pipe,
> >>> + struct dpu_sw_pipe_cfg *pipe_cfg,
> >>> + struct drm_plane_state *plane_state,
> >>> + struct dpu_global_state *global_state,
> >>> + struct drm_crtc *crtc,
> >>> + struct dpu_rm_sspp_requirements *reqs)
> >>> +{
> >>> + struct drm_plane *plane = plane_state->plane;
> >>> + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
> >>> + struct dpu_sw_pipe *r_pipe = pipe + 1;
> >>> + struct dpu_sw_pipe_cfg *r_pipe_cfg = pipe_cfg + 1;
> >>> +
> >>> + if (drm_rect_width(&pipe_cfg->src_rect) != 0) {
> >>> + pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, reqs);
> >>> + if (!pipe->sspp)
> >>> + return -ENODEV;
> >>> + pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> + pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> + }
> >>> +
> >>> + if (drm_rect_width(&r_pipe_cfg->src_rect) != 0 &&
> >>> + dpu_plane_try_multirect_parallel(pipe, pipe_cfg, r_pipe, r_pipe_cfg,
> >>> + pipe->sspp,
> >>> + msm_framebuffer_format(plane_state->fb),
> >>> + dpu_kms->catalog->caps->max_linewidth))
> >>> + goto stage_assinged;
> >>> +
> >>> + if (drm_rect_width(&r_pipe_cfg->src_rect) != 0) {
> >>> + r_pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, reqs);
> >>> + if (!r_pipe->sspp)
> >>> + return -ENODEV;
> >>> + r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> + r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> + }
> >>> +
> >>> +stage_assinged:
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
> >>> struct dpu_global_state *global_state,
> >>> struct drm_atomic_state *state,
> >>> struct drm_plane_state *plane_state,
> >>> - struct drm_plane_state *prev_adjacent_plane_state)
> >>> + struct drm_plane_state **prev_adjacent_plane_state)
> >>> {
> >>> const struct drm_crtc_state *crtc_state = NULL;
> >>> struct drm_plane *plane = plane_state->plane;
> >>> struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
> >>> struct dpu_rm_sspp_requirements reqs;
> >>> - struct dpu_plane_state *pstate, *prev_adjacent_pstate;
> >>> + struct dpu_plane_state *pstate, *prev_adjacent_pstate[STAGES_PER_PLANE];
> >>> 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;
> >>> const struct msm_format *fmt;
> >>> - int i;
> >>> + int i, ret;
> >>>
> >>> if (plane_state->crtc)
> >>> crtc_state = drm_atomic_get_new_crtc_state(state,
> >>> plane_state->crtc);
> >>>
> >>> pstate = to_dpu_plane_state(plane_state);
> >>> - prev_adjacent_pstate = prev_adjacent_plane_state ?
> >>> - to_dpu_plane_state(prev_adjacent_plane_state) : 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 < STAGES_PER_PLANE; i++)
> >>> + prev_adjacent_pstate[i] = prev_adjacent_plane_state[i] ?
> >>> + to_dpu_plane_state(prev_adjacent_plane_state[i]) : NULL;
> >>>
> >>> for (i = 0; i < PIPES_PER_PLANE; i++)
> >>> pstate->pipe[i].sspp = NULL;
> >>> @@ -1225,42 +1280,27 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
> >>>
> >>> reqs.rot90 = drm_rotation_90_or_270(plane_state->rotation);
> >>>
> >>> - if (drm_rect_width(&r_pipe_cfg->src_rect) == 0) {
> >>> - if (!prev_adjacent_pstate ||
> >>> - !dpu_plane_try_multirect_shared(pstate, prev_adjacent_pstate, fmt,
> >>> - dpu_kms->catalog->caps->max_linewidth)) {
> >>> - pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs);
> >>> - if (!pipe->sspp)
> >>> - return -ENODEV;
> >>> -
> >>> - r_pipe->sspp = NULL;
> >>> + for (i = 0; i < STAGES_PER_PLANE; i++) {
> >>> + if (!prev_adjacent_pstate[i])
> >>> + goto assignment;
> >>>
> >>> - pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> - pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> -
> >>> - r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> - r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> - }
> >>> - } else {
> >>> - pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs);
> >>> - if (!pipe->sspp)
> >>> - return -ENODEV;
> >>> -
> >>> - if (!dpu_plane_try_multirect_parallel(pipe, pipe_cfg, r_pipe, r_pipe_cfg,
> >>> - pipe->sspp,
> >>> - msm_framebuffer_format(plane_state->fb),
> >>> - dpu_kms->catalog->caps->max_linewidth)) {
> >>> - /* multirect is not possible, use two SSPP blocks */
> >>> - r_pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs);
> >>> - if (!r_pipe->sspp)
> >>> - return -ENODEV;
> >>> + if (dpu_plane_try_multirect_shared(pstate, prev_adjacent_pstate[i], fmt,
> >>> + dpu_kms->catalog->caps->max_linewidth,
> >>> + i))
> >>> + continue;
> >>
> >>
> >> if (prev_adjacent_pstate[i] &&
> >> dpu_plane_try_multirect_shared())
> >> continue;
> >>
> >> No need for the goto.
> >
> > Right, it will be simpler.
> >>
> >>>
> >>> - pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> - pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> +assignment:
> >>> + if (dpu_plane_get_single_pipe_in_stage(pstate, NULL, NULL, i))
> >>> + prev_adjacent_plane_state[i] = plane_state;
> >>>
> >>> - r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> - r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> - }
> >>> + pipe = &pstate->pipe[i * PIPES_PER_STAGE];
> >>> + pipe_cfg = &pstate->pipe_cfg[i * PIPES_PER_STAGE];
> >>> + ret = dpu_plane_assign_resource_in_stage(pipe, pipe_cfg,
> >>> + plane_state,
> >>> + global_state,
> >>> + crtc, &reqs);
> >>> + if (ret)
> >>> + return ret;
> >>> }
> >>>
> >>> return dpu_plane_atomic_check_sspp(plane, state, crtc_state);
> >>> @@ -1273,7 +1313,7 @@ int dpu_assign_plane_resources(struct dpu_global_state *global_state,
> >>> unsigned int num_planes)
> >>> {
> >>> unsigned int i;
> >>> - struct drm_plane_state *prev_adjacent_plane_state = NULL;
> >>> + struct drm_plane_state *prev_adjacent_plane_state[STAGES_PER_PLANE] = { NULL };
> >>>
> >>> for (i = 0; i < num_planes; i++) {
> >>> struct drm_plane_state *plane_state = states[i];
> >>> @@ -1284,11 +1324,9 @@ int dpu_assign_plane_resources(struct dpu_global_state *global_state,
> >>>
> >>> int ret = dpu_plane_virtual_assign_resources(crtc, global_state,
> >>> state, plane_state,
> >>> - prev_adjacent_plane_state);
> >>> + &prev_adjacent_plane_state[0]);
> >>
> >> It's exactly the prev_adjacent_plane_state.
> >
> > Yes, I will change it.
> >
> >>
> >>> if (ret)
> >>> break;
> >>> -
> >>> - prev_adjacent_plane_state = plane_state;
> >>> }
> >>>
> >>> return 0;
> >>>
> >>> --
> >>> 2.34.1
> >>>
> >>
> >> --
> >> With best wishes
> >> Dmitry
>
>
> --
> With best wishes
> Dmitry
Powered by blists - more mailing lists