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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABymUCNuYDjmytbb+HLg1KF5eOyQVNczcq_wqFdo51cr0Y6BdQ@mail.gmail.com>
Date: Fri, 30 May 2025 22:58:51 +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>, 
	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>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 10/12] drm/msm/dpu: support SSPP assignment for
 quad-pipe case

Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年5月29日周四 02:22写道:
>
> On Mon, May 26, 2025 at 05:28:28PM +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_crtc.c  |  11 +++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |   2 +
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 126 ++++++++++++++++++------------
> >  3 files changed, 88 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 85f585206218f4578e18b00452762dbada060e9c..47ab43dfec76acc058fb275d1928603e8e8e7fc6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1562,6 +1562,17 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
> >       return 0;
> >  }
> >
> > +/**
> > + * dpu_crtc_get_num_lm - Get mixer number in this CRTC pipeline
> > + * @state: Pointer to drm crtc state object
> > + */
> > +unsigned int dpu_crtc_get_num_lm(const struct drm_crtc_state *state)
> > +{
> > +     struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
> > +
> > +     return cstate->num_mixers;
> > +}
> > +
> >  #ifdef CONFIG_DEBUG_FS
> >  static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
> >  {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index 94392b9b924546f96e738ae20920cf9afd568e6b..6eaba5696e8e6bd1246a9895c4c8714ca6589b10 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -267,4 +267,6 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
> >
> >  void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
> >
> > +unsigned int dpu_crtc_get_num_lm(const struct drm_crtc_state *state);
> > +
> >  #endif /* _DPU_CRTC_H_ */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 0bb153a71353ca9eaca138ebbee4cd699414771d..f721dc504bbbe3a49986239adee113bfb6790f70 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -961,6 +961,33 @@ static int dpu_plane_is_multirect_parallel_capable(struct dpu_hw_sspp *sspp,
> >               dpu_plane_is_parallel_capable(pipe_cfg, fmt, max_linewidth);
> >  }
> >
> > +static bool dpu_plane_check_single_pipe(struct dpu_plane_state *pstate,
> > +                                     struct dpu_sw_pipe **single_pipe,
> > +                                     struct dpu_sw_pipe_cfg **single_pipe_cfg,
> > +                                     bool config_pipe)
> > +{
> > +     int i, valid_pipe = 0;
> > +     struct dpu_sw_pipe *pipe;
> > +
> > +     for (i = 0; i < PIPES_PER_PLANE; i++) {
> > +             if (drm_rect_width(&pstate->pipe_cfg[i].src_rect) != 0) {
> > +                     valid_pipe++;
> > +                     if (valid_pipe > 1)
> > +                             return false;
> > +                     *single_pipe = &pstate->pipe[i];
> > +                     *single_pipe_cfg = &pstate->pipe_cfg[i];
> > +             } else {
> > +                     if (!config_pipe)
> > +                             continue;
> > +                     pipe = &pstate->pipe[i];
> > +                     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > +                     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +                     pipe->sspp = NULL;
>
> If this function is 'check', then why does it change something in the
> pipe configuration?

I see modification is made in other check functions, like
dpu_plane_atomic_check_nosspp(). So
the name is referenced. Do you think dpu_plane_get_single_pipe() is OK here?
>
> > +             }
> > +     }
> > +
> > +     return true;
> > +}
> >
> >  static int dpu_plane_atomic_check_sspp(struct drm_plane *plane,
> >                                      struct drm_atomic_state *state,
> > @@ -1028,15 +1055,15 @@ static int dpu_plane_try_multirect_shared(struct dpu_plane_state *pstate,
> >                                         const struct msm_format *fmt,
> >                                         uint32_t max_linewidth)
> >  {
> > -     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_check_single_pipe(pstate, &pipe, &pipe_cfg, true))
> > +             return false;
> > +
> > +     if (!dpu_plane_check_single_pipe(prev_adjacent_pstate, &prev_pipe, &prev_pipe_cfg, false) ||
> >           prev_pipe->multirect_mode != DPU_SSPP_MULTIRECT_NONE)
> >               return false;
> >
> > @@ -1050,11 +1077,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 ||
> > @@ -1194,12 +1216,11 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
> >       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_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, *r_pipe;
> > +     struct dpu_sw_pipe_cfg *pipe_cfg, *r_pipe_cfg;
> > +     struct dpu_plane *pdpu = to_dpu_plane(plane);
> >       const struct msm_format *fmt;
> > -     int i;
> > +     int i, num_lm, stage_id, num_stages;
> >
> >       if (plane_state->crtc)
> >               crtc_state = drm_atomic_get_new_crtc_state(state,
> > @@ -1209,11 +1230,6 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
> >       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 < PIPES_PER_PLANE; i++)
> >               pstate->pipe[i].sspp = NULL;
> >
> > @@ -1227,44 +1243,52 @@ 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;
> > +     if (prev_adjacent_pstate &&
> > +         dpu_plane_try_multirect_shared(pstate, prev_adjacent_pstate, fmt,
> > +                                         dpu_kms->catalog->caps->max_linewidth)) {
> > +             goto assigned;
> > +     }
> >
> > -                     r_pipe->sspp = NULL;
> > +     num_lm = dpu_crtc_get_num_lm(crtc_state);
> > +     num_stages = (num_lm + 1) / 2;
> > +     for (stage_id = 0; stage_id < num_stages; stage_id++) {
>
> Can't we just loop through all possible stages? Same result, but makes
> the logic somewhat easier.

Yeah, that's doable. Will revise it in next version.
>
> > +             for (i = stage_id * PIPES_PER_STAGE; i < (stage_id + 1) * PIPES_PER_STAGE; i++) {
>
> I still really don't like this idea (and especially the part with
> (i % PIPES_PER_STAGE == 0) condition). Extract current code which deals
> with two planes and a single stage. Call it for each stage. That's it.

Will do.
>
> > +                     pipe = &pstate->pipe[i];
> > +                     pipe_cfg = &pstate->pipe_cfg[i];
> >
> > -                     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > -                     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +                     if (drm_rect_width(&pipe_cfg->src_rect) == 0)
> > +                             break;
> >
> > -                     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)
> > +                     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;
> > -
> > -                     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > -                     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +                     r_pipe = &pstate->pipe[i + 1];
> > +                     r_pipe_cfg = &pstate->pipe_cfg[i + 1];
> > +
> > +                     /*
> > +                      * If current pipe is the first pipe in pipe pair, check
> > +                      * multi-rect opportunity for the 2nd pipe in the pair.
> > +                      * SSPP multi-rect mode cross mixer pairs is not supported.
> > +                      */
> > +                     if ((i % PIPES_PER_STAGE == 0) &&
> > +                         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)) {
> > +                             i++;
> > +                     } else {
> > +                             /* multirect is not possible, use two SSPP blocks */
> > +                             pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > +                             pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +                             DPU_DEBUG_PLANE(pdpu, "allocating sspp_%d for pipe %d.\n",
> > +                                             pipe->sspp->idx - SSPP_NONE, i);
> > +                     }
> >               }
> >       }
> >
> > +assigned:
> >       return dpu_plane_atomic_check_sspp(plane, state, crtc_state);
> >  }
> >
> >
> > --
> > 2.34.1
> >
>
> --
> With best wishes
> Dmitry

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ