| 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: <eshnauruu4sybpgsfrrwlvk3cpb2zg4mykg4agwong3dbiduic@nvupoe6aoyzu>
Date: Fri, 3 Jan 2025 19:51:35 +0200
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>,
Simona Vetter <simona@...ll.ch>, linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 15/15] drm/msm/dpu: Enable quad-pipe for DSC and
dual-DSI case
On Fri, Jan 03, 2025 at 11:49:07PM +0800, Jun Nie wrote:
> Dmitry Baryshkov <dmitry.baryshkov@...aro.org> 于2024年12月20日周五 07:46写道：
> >
> > On Thu, Dec 19, 2024 at 03:49:33PM +0800, Jun Nie wrote:
> > > Request 4 mixers and 4 DSC for the case that both dual-DSI and DSC are
> > > enabled. We prefer to use 4 pipes for dual DSI case for it is power optimal
> > > for DSC.
> > >
> > > Signed-off-by: Jun Nie <jun.nie@...aro.org>
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 ++---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 ++++++++++++++++++------
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 3 ++-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 +
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
> > > 7 files changed, 30 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index bad75af4e50ab..3c51c199f3e05 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -200,7 +200,7 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc,
> > > struct dpu_crtc_state *crtc_state)
> > > {
> > > struct dpu_crtc_mixer *m;
> > > - u32 crcs[CRTC_DUAL_MIXERS];
> > > + u32 crcs[CRTC_QUAD_MIXERS];
> > >
> > > int rc = 0;
> > > int i;
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > index d1bb3f84fe440..ce41fb364f3db 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > > @@ -210,7 +210,7 @@ struct dpu_crtc_state {
> > >
> > > bool bw_control;
> > > bool bw_split_vote;
> > > - struct drm_rect lm_bounds[CRTC_DUAL_MIXERS];
> > > + struct drm_rect lm_bounds[CRTC_QUAD_MIXERS];
> > >
> > > uint64_t input_fence_timeout_ns;
> > >
> > > @@ -218,10 +218,10 @@ struct dpu_crtc_state {
> > >
> > > /* HW Resources reserved for the crtc */
> > > u32 num_mixers;
> > > - struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
> > > + struct dpu_crtc_mixer mixers[CRTC_QUAD_MIXERS];
> > >
> > > u32 num_ctls;
> > > - struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
> > > + struct dpu_hw_ctl *hw_ctls[CRTC_QUAD_MIXERS];
> > >
> > > enum dpu_crtc_crc_source crc_source;
> > > int crc_frame_skip_count;
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 96d06db3e4be5..6e54ddeaffacd 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -54,7 +54,7 @@
> > > #define MAX_PHYS_ENCODERS_PER_VIRTUAL \
> > > (MAX_H_TILES_PER_DISPLAY * NUM_PHYS_ENCODER_TYPES)
> > >
> > > -#define MAX_CHANNELS_PER_ENC 2
> > > +#define MAX_CHANNELS_PER_ENC 4
> > >
> > > #define IDLE_SHORT_TIMEOUT 1
> > >
> > > @@ -664,15 +664,19 @@ static struct msm_display_topology dpu_encoder_get_topology(
> > >
> > > /* Datapath topology selection
> > > *
> > > - * Dual display
> > > + * Dual display without DSC
> > > * 2 LM, 2 INTF ( Split display using 2 interfaces)
> > > *
> > > + * Dual display with DSC
> > > + * 4 LM, 2 INTF ( Split display using 2 interfaces)
> > > + *
> > > * Single display
> > > * 1 LM, 1 INTF
> > > * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
> > > *
> > > * Add dspps to the reservation requirements if ctm is requested
> > > */
> > > +
> > > if (intf_count == 2)
> > > topology.num_lm = 2;
> > > else if (!dpu_kms->catalog->caps->has_3d_merge)
> > > @@ -691,10 +695,20 @@ static struct msm_display_topology dpu_encoder_get_topology(
> > > * 2 DSC encoders, 2 layer mixers and 1 interface
> > > * this is power optimal and can drive up to (including) 4k
> > > * screens
> > > + * But for dual display case, we prefer 4 layer mixers. Because
> > > + * the resolution is always high in the case and 4 DSCs are more
> > > + * power optimal.
> > > */
> > > - topology.num_dsc = 2;
> > > - topology.num_lm = 2;
> > > - topology.num_intf = 1;
> > > +
> > > + if (intf_count == 2) {
> > > + topology.num_dsc = 4;
> > > + topology.num_lm = 4;
> > > + topology.num_intf = 2;
> > > + } else {
> > > + topology.num_dsc = 2;
> > > + topology.num_lm = 2;
> > > + topology.num_intf = 1;
> >
> > Why is it only enabled for the DSC case? Also I'd like to point out
> > platforms like sm7150 or msm8998 which have only 2 DSC blocks. The
> > condition here needs more work to work with those platforms too.
>
> Because the DSC + quad-pipe is assumed with wide LCD case that dual
> pipe can not handle due to width limitation. If DSC is not involved, the
> resolution is not too big to involve DSC together with 2 interfaces. Do you
> think there is need to support quad-pipe without DSC?
Yes, of course. The same logic: ultra wide resolutions. If two LMs are
not enough for the panel / monitor.
>
> Of course, it is a valid case to use 2 DSC with 2 interfaces. Below logic shall
> be better.
>
> if (intf_count == 2) {
> topology.num_dsc = dpu_kms->catalog->dsc_count >= 4 ? 4 : 2;
> topology.num_lm = topology.num_dsc;
> topology.num_intf = 2;
> } else {
> topology.num_dsc = 2;
> topology.num_lm = 2;
> topology.num_intf = 1;
> }
It all starts to look like a collection of use-case quirks. But indeed,
it should work.
>
> >
> > > + }
> > > }
> > >
> > > return topology;
> > > @@ -2195,8 +2209,8 @@ static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys *phys_enc)
> > > struct dpu_hw_mixer_cfg mixer;
> > > int i, num_lm;
> > > struct dpu_global_state *global_state;
> > > - struct dpu_hw_blk *hw_lm[2];
> > > - struct dpu_hw_mixer *hw_mixer[2];
> > > + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
> > > + struct dpu_hw_mixer *hw_mixer[MAX_CHANNELS_PER_ENC];
> > > struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
> > >
> > > memset(&mixer, 0, sizeof(mixer));
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > index 63f09857025c2..d378a990cc0fb 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > @@ -302,7 +302,8 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
> > >
> > > /* Use merge_3d unless DSC MERGE topology is used */
> > > if (phys_enc->split_role == ENC_ROLE_SOLO &&
> > > - dpu_cstate->num_mixers == CRTC_DUAL_MIXERS &&
> > > + (dpu_cstate->num_mixers == CRTC_DUAL_MIXERS ||
> > > + dpu_cstate->num_mixers == CRTC_QUAD_MIXERS) &&
> >
> > Misaligned. Also isn't it enough to check that num_mixers != 1?
>
> Yeah, num_mixers != 1 should work.
> >
> > > !dpu_encoder_use_dsc_merge(phys_enc->parent))
> > > return BLEND_3D_H_ROW_INT;
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > index 3ab79092a7f25..d9cc84b081b1f 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > @@ -25,6 +25,7 @@
> > > #define DPU_MAX_IMG_HEIGHT 0x3fff
> > >
> > > #define CRTC_DUAL_MIXERS 2
> >
> > Do we still need this define?
> >
> > > +#define CRTC_QUAD_MIXERS 4
> > >
> > > #define MAX_XIN_COUNT 16
> > >
> > > 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 27ef0771da5d2..1fe21087a141a 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> > > @@ -33,8 +33,8 @@
> > > #endif
> > >
> > > #define STAGES_PER_PLANE 2
> > > -#define PIPES_PER_PLANE 2
> > > #define PIPES_PER_STAGE 2
> > > +#define PIPES_PER_PLANE (STAGES_PER_PLANE * STAGES_PER_PLANE)
> >
> > This is incorrect.
> >
> > > #ifndef DPU_MAX_DE_CURVES
> > > #define DPU_MAX_DE_CURVES 3
> > > #endif
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > index 57ccb73c45683..b5c1ad2a75594 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > @@ -1474,7 +1474,7 @@ static void _dpu_plane_atomic_disable(struct drm_plane *plane)
> > > trace_dpu_plane_disable(DRMID(plane), false,
> > > pstate->pipe[i].multirect_mode);
> > >
> > > - if (pipe->sspp && i == 1) {
> > > + if (pipe->sspp && pipe->multirect_index == DPU_SSPP_RECT_1) {
> >
> > Separate change, please. Also I'm not sure how does that work with the
> > shared SSPP case that I pointed to in one of the previous replies.
>
> Maybe we can add a peer member in the pipe to reference each other, then we have
> chance to use multirect across all pipes in all planes.
I'd rather not. We have pairs of pipes. I'd rather see the code stay the
same way: processing one pair at the same time.
>
>
> >
> > > pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > > pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > >
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
Powered by blists - more mailing lists