[PATCH 13/21] drm/msm/dpu: Support quad pipe in header files

[Jun Nie]

Dmitry Baryshkov <dmitry.baryshkov at linaro.org> 于2024年8月29日周四 19:30写道：
>
> On Thu, 29 Aug 2024 at 13:20, Jun Nie <jun.nie at linaro.org> wrote:
> >
> > Support 4 pipes and their configs at most. They are for 2 SSPP
> > and their multi-rect mode. Because one SSPP can co-work with
> > 2 mixer at most, 2 pair of mixer are needed for 2 SSPP in quad-
> > pipe case. So 2 mixer configs are needed in quad-pipe case.
>
> As you wrote this is based (depends?) on the virtual planes, then you
> know that the code already uses either one or two SSPP blocks to drive
> one sw_pipe. I'm not sure what do you mean by "2 mixer configs". There
> are 4 LMs and 4 mixer configurations in the quad-pipe case. The commit
> message is thus misleading.

This patch set depends on the virtual plane patch set. The mixer config is
not a proper term per your response. It is from DPU2 branch. Maybe
clip_config is a better term for this. The config is used to split the plane
into 2 mixers pairs and 2 DSI interface with 2 halves of full screen.

>
> >
> > Signed-off-by: Jun Nie <jun.nie at linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h |  2 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 11 ++++++++++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   | 30 +++++++++++++++++++++--------
> >  3 files changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > index a2eff36a2224c..424725303ccad 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > @@ -32,7 +32,7 @@
> >  #define DPU_MAX_PLANES                 4
> >  #endif
> >
> > -#define PIPES_PER_STAGE                        2
> > +#define PIPES_PER_STAGE                        4
> >  #ifndef DPU_MAX_DE_CURVES
> >  #define DPU_MAX_DE_CURVES              3
> >  #endif
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > index fc54625ae5d4f..ae6beff2c294b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > @@ -143,11 +143,20 @@ struct dpu_hw_pixel_ext {
> >   *             such as decimation, flip etc to program this field
> >   * @dest_rect: destination ROI.
> >   * @rotation: simplified drm rotation hint
> > + * @visible: mark this cfg is valid
>
> So is it valid or visible?
Yeah, valid is better than visible.
>
> > + * @mxcfg_id: mixer config ID for left or right half screen.
> > + *             We have single SSPP, dual SSPP, single SSPP+multi_rect or dual
> > + *             SSPP+multi_rect case. mxcfg_id mark current pipe will use
> > + *             which mixer cfg. The first mxcfg is for the left half of screen,
> > + *             the 2nd mxcfg is for the right half screen. The heading cfg may
> > + *             be skipped by pipe with the first mxcfg_id = 1 if the plane is
> > + *             only displayed in the right side, thus SSPP goes to later mixers.
>
> too long description for an unreadable name.

Maybe the clip_id is better per above discussion?
>
> >   */
> >  struct dpu_sw_pipe_cfg {
> >         struct drm_rect src_rect;
> >         struct drm_rect dst_rect;
> > -       unsigned int rotation;
> > +       unsigned int rotation, mxcfg_id;
> > +       bool visible;
> >  };
> >
> >  /**
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > index e225d5baceb09..9e79cf9eba264 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > @@ -14,14 +14,30 @@
> >  #include "dpu_hw_mdss.h"
> >  #include "dpu_hw_sspp.h"
> >
> > +/**
> > + * Max number of mixer configs. Because we support 4 pipes at most,
> > + * the 4 pipes are with 2 SSPP and their multi-rect mode. While one
>
> Or 4 SSPPs. Or 3 SSPPs. Or even a single SSPP if it doesn't cover the
> whole screen.
>
> I'm really sorry to say, but I can not understand this text.

Yeah, lots of usage cases are not mentioned here. It just describe how the
config number come from. It should be the number for screen clip rectangle
in a full screen.

>
> > + * SSPP can co-work with 2 mixer at most, then 2 pair of mixer are
> > + * needed for 2 SSPP in quad-pipe case. Thus 2 mixer configs are
> > + * needed in quad-pipe case.
> > + */
> > +#define MIX_CFGS_IN_CRTC 2
> > +
> >  /**
> >   * 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. Some or all of fields in array can
>
> array has elements, not fields.
>
> > + *             be in use per topology. The heading fields are used first,
> > + *             and the later fields is invalid if visible field of pipe_cfg
> > + *             is not set. For example, the visible fields of pipe_cfg are set
> > + *             in the first 2 pipe_cfg fields, and the mxcfg_id for them are
> > + *             0 and 1. That means the first pipe is for left half screen and
> > + *             the 2nd pipe is for right half. The visible field of the 3rd
> > + *             pipe_cfg is not set, which means the 3rd and 4th pipe are not
> > + *             in use.
>
> NAK. A single LM pair might already need two sw pipes.
> After reading the comment I have doubts that you understand what the
> code is currently doing.

This describes that  a right half only plane will only use the first
pipe/pipe_cfg with
valid flag and clip_id flag. So the later 2 elements of
sw_pipe/pipe_cfg arrary are not
used.

>
> > + * @pipe_cfg:  software pipe configuration. The 4 fields are for SSPP and their
> > +               parallel rect as above pipes.
> >   * @stage:     assigned by crtc blender
> >   * @needs_qos_remap: qos remap settings need to be updated
> >   * @multirect_index: index of the rectangle of SSPP
> > @@ -34,10 +50,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;
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry