On Tue, Dec 16, 2025 at 02:56:31PM +0800, yuanjiey wrote:
> On Mon, Dec 15, 2025 at 10:08:22PM +0200, Dmitry Baryshkov wrote:
> > On Mon, Dec 15, 2025 at 04:38:53PM +0800, yuanjie yang wrote:
> > > From: Yuanjie Yang <[email protected]>
> > >
> > > DPU version 13.0.0 introduces structural changes including
> > > register additions, removals, and relocations.
> > >
> > > Refactor SSPP-related code to be compatible with DPU 13.0.0
> > > modifications.
> > >
> > > Co-developed-by: Yongxing Mou <[email protected]>
> > > Signed-off-by: Yongxing Mou <[email protected]>
> > > Signed-off-by: Yuanjie Yang <[email protected]>
> > > ---
> >
> > We've fixed the order of the interrupts patch. Now you are adding SSPP
> > customization for 13.x _after_ adding the first 13.x support. Is that
> > supposed to work?
>
> Yes, will reorganize order.
And after comparing with v2, I'm really surprised. It was better before
and then you changed the order of the patches. Why? You were asked to
split it, but not to move it to the end.
>
>
> > > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 15 +-
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 155 ++++++++++--------
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 52 ++++++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 18 ++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 +
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 17 +-
> > > 6 files changed, 191 insertions(+), 69 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > > switch (ctx->ubwc->ubwc_enc_version) {
> > > case UBWC_1_0:
> > > fast_clear = fmt->alpha_enable ? BIT(31) : 0;
> > > - DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> > > - fast_clear | (ctx->ubwc->ubwc_swizzle &
> > > 0x1) |
> > > - BIT(8) |
> > > - (ctx->ubwc->highest_bank_bit << 4));
> > > + DPU_REG_WRITE(c, ubwc_ctrl_off,
> > > + fast_clear | (ctx->ubwc->ubwc_swizzle &
> > > 0x1) |
> > > + BIT(8) |
> > > + (ctx->ubwc->highest_bank_bit << 4));
> >
> > I have asked to drop unrelated changes. You didn't. Why? You are
> > changing whitespaces for no reason. It's just a noise which hides the
> > actual change here.
>
> here ubwc reg layout change in DPU 13.
>
> ubwc_ctrl_off
> veriosn < 13
> reg: SSPP_UBWC_STATIC_CTRL
> verison >= 13
> reg: SSPP_REC_UBWC_STATIC_CTRL
>
> So I do some fix.
What does it have to do with the whitespaces? Fix _one_ line.
>
> > > break;
> > > case UBWC_2_0:
> > > fast_clear = fmt->alpha_enable ? BIT(31) : 0;
> > > - DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> > > - fast_clear | (ctx->ubwc->ubwc_swizzle) |
> > > - (ctx->ubwc->highest_bank_bit << 4));
> > > + DPU_REG_WRITE(c, ubwc_ctrl_off,
> > > + fast_clear | (ctx->ubwc->ubwc_swizzle) |
> > > + (ctx->ubwc->highest_bank_bit << 4));
> > > break;
> > > case UBWC_3_0:
> > > - DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> > > - BIT(30) | (ctx->ubwc->ubwc_swizzle) |
> > > - (ctx->ubwc->highest_bank_bit << 4));
> > > + DPU_REG_WRITE(c, ubwc_ctrl_off,
> > > + BIT(30) | (ctx->ubwc->ubwc_swizzle) |
> > > + (ctx->ubwc->highest_bank_bit << 4));
> > > break;
> > > case UBWC_4_0:
> > > - DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> > > - MSM_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
> > > + DPU_REG_WRITE(c, ubwc_ctrl_off,
> > > + MSM_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
> > > break;
> > > }
> > > }
> > > @@ -313,19 +337,18 @@ static void dpu_hw_sspp_setup_format(struct
> > > dpu_sw_pipe *pipe,
> > >
> > > /* update scaler opmode, if appropriate */
> > > if (test_bit(DPU_SSPP_CSC, &ctx->cap->features))
> > > - _sspp_setup_opmode(ctx, VIG_OP_CSC_EN | VIG_OP_CSC_SRC_DATAFMT,
> > > - MSM_FORMAT_IS_YUV(fmt));
> > > + dpu_hw_sspp_setup_opmode(ctx, VIG_OP_CSC_EN |
> > > VIG_OP_CSC_SRC_DATAFMT,
> > > + MSM_FORMAT_IS_YUV(fmt));
> > > else if (test_bit(DPU_SSPP_CSC_10BIT, &ctx->cap->features))
> > > - _sspp_setup_csc10_opmode(ctx,
> > > - VIG_CSC_10_EN | VIG_CSC_10_SRC_DATAFMT,
> > > - MSM_FORMAT_IS_YUV(fmt));
> > > + dpu_hw_sspp_setup_csc10_opmode(ctx,
> > > + VIG_CSC_10_EN |
> > > VIG_CSC_10_SRC_DATAFMT,
> > > + MSM_FORMAT_IS_YUV(fmt));
> >
> > Again, useless whitespace changes.
> checkpatch.pl says here is alignment issuse, so I do this fix.
The issue was present before your patch. If you want to fix it, fix it
in the separate patch or ignore it.
>
> > >
> > > DPU_REG_WRITE(c, format_off, src_format);
> > > DPU_REG_WRITE(c, unpack_pat_off, unpack);
> > > DPU_REG_WRITE(c, op_mode_off, opmode);
> > > -
> >
> > Why?
>
> yes, will drop "-" diff.
>
> > > /* clear previous UBWC error */
> > > - DPU_REG_WRITE(c, SSPP_UBWC_ERROR_STATUS, BIT(31));
> > > + DPU_REG_WRITE(c, ubwc_err_off, BIT(31));
> > > }
> > >
> > > static void dpu_hw_sspp_setup_pe_config(struct dpu_hw_sspp *ctx,
> > > @@ -385,9 +408,9 @@ static void dpu_hw_sspp_setup_pe_config(struct
> > > dpu_hw_sspp *ctx,
> > > tot_req_pixels[3]);
> > > }
> > >
> > > -static void _dpu_hw_sspp_setup_scaler3(struct dpu_hw_sspp *ctx,
> > > - struct dpu_hw_scaler3_cfg *scaler3_cfg,
> > > - const struct msm_format *format)
> > > +void dpu_hw_sspp_setup_scaler3(struct dpu_hw_sspp *ctx,
> > > + struct dpu_hw_scaler3_cfg *scaler3_cfg,
> > > + const struct msm_format *format)
> >
> > And here...
> checkpatch.pl says here is alignment issuse, so I do this fix.
And I'm asking you to don't do it. Don't clutter the patch with
unrelated changes (and whitespace / alignment changes are generally
unrelated).
--
With best wishes
Dmitry
