Message-ID: <CAA8EJpqDWZiGawPtMXgY9evOWOZ6bTqmgBgks7rXxr4VC5tCXw@mail.gmail.com>
Date: Tue, 3 Sep 2024 11:59:11 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jun Nie <jun.nie@...aro.org>
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>, Daniel Vetter <daniel@...ll.ch>, 
	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 13/21] drm/msm/dpu: Support quad pipe in header files

On Tue, 3 Sept 2024 at 10:51, Jun Nie <jun.nie@...aro.org> wrote:
>
> Dmitry Baryshkov <dmitry.baryshkov@...aro.org> 于2024年8月29日周四 19:30写道：
> >
> > On Thu, 29 Aug 2024 at 13:20, Jun Nie <jun.nie@...aro.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.

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@...aro.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?

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

Powered by blists - more mailing lists