linux-kernel - Re: [PATCH v3 10/11] drm/msm/dpu: Refactor SSPP to compatible DPU 13.0.0

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <gljkq6jijsprelq7qmgai4g7mqlshezdx755n3ivbxjdf6uw73@dz3mkct7g3ry>
Date: Tue, 16 Dec 2025 16:22:32 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: yuanjiey <yuanjie.yang@....qualcomm.com>
Cc: robin.clark@....qualcomm.com, lumag@...nel.org, abhinav.kumar@...ux.dev,
        jesszhan0024@...il.com, sean@...rly.run, marijn.suijten@...ainline.org,
        airlied@...il.com, simona@...ll.ch, maarten.lankhorst@...ux.intel.com,
        mripard@...nel.org, tzimmermann@...e.de, robh@...nel.org,
        krzk+dt@...nel.org, conor+dt@...nel.org, neil.armstrong@...aro.org,
        yongxing.mou@....qualcomm.com, konrad.dybcio@....qualcomm.com,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, tingwei.zhang@....qualcomm.com,
        aiqun.yu@....qualcomm.com
Subject: Re: [PATCH v3 10/11] drm/msm/dpu: Refactor SSPP to compatible DPU
 13.0.0

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 <yuanjie.yang@....qualcomm.com>
> > > 
> > > 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 <yongxing.mou@....qualcomm.com>
> > > Signed-off-by: Yongxing Mou <yongxing.mou@....qualcomm.com>
> > > Signed-off-by: Yuanjie Yang <yuanjie.yang@....qualcomm.com>
> > > ---
> > 
> > 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