On Tue, 3 Sept 2024 at 10:51, Jun Nie <jun.nie@xxxxxxxxxx> wrote:
>
> Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 于2024年8月29日周四 19:30写道：
> >
> > On Thu, 29 Aug 2024 at 13:20, Jun Nie <jun.nie@xxxxxxxxxx> 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.
Let's understand why you need it at all. The sw_pipe should be
agnostic of the "left-or-right-half" part of the story.
>
> >
> > >
> > > Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> > > ---
> > > 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?
No. Please use clip when something gets clipped (e.g. because the
plane displays just a part of the framebuffer).
> >
> > > */
> > > 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.
I'd suggest doing it in the opposite way: always use 0/1 for the left
part, 2/3 for the right part. It should save you from all the
"visible" and "mxcfg_id" story.
>
> >
> > > + * @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
--
With best wishes
Dmitry
