On Tue, Dec 16, 2025 at 04:22:32PM +0200, Dmitry Baryshkov wrote:
> 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.
I make the mistake.
Sure, I will keep the v2 patch order in next patch.
> >
> >
> > > > .../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.
get it, will drop unrelated whitespaces.
> >
> > > > 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.
get it, will drop unrelated whitespaces.
> >
> > > >
> > > > 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
Thanks,
Yuanjie
