Re: [PATCH v2 2/7] drm/mediatek: fix CCORR mtk_ctm_s31_32_to_s1_n function issue

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Thu, 2025-08-07 at 06:19 +0000, Jay Liu (刘博) wrote:
> On Wed, 2025-08-06 at 06:37 +0000, CK Hu (胡俊光) wrote:
> > On Sun, 2025-07-27 at 15:15 +0800, Jay Liu wrote:
> > > if matrixbit is 11,
> > > The range of color matrix is from 0 to (BIT(12) - 1).
> > > Values from 0 to (BIT(11) - 1) represent positive numbers,
> > > values from BIT(11) to (BIT(12) - 1) represent negative numbers.
> > > For example, -1 need converted to 8191.
> > > so convert S31.32 to HW Q2.11 format by
> > > drm_color_ctm_s31_32_to_qm_n,
> > > and set int_bits to 2.
> > 
> > You change the behavior of MT8183 CCORR and MT8192 CCORR.
> > These two SoC has work for a long time.
> > Does both SoC really have bug?
> > 
> > In comment below, it shows that HW S1.n format.
> > The patch sender has much information about the hardware information.
> > Would they make mistake?
> > If only MT8196 CCORR has the format qm_n, use private data to
> > distinguish the behavior.
> > 
> > Regards,
> > CK
> > 
> Yes. We received an email from customer, they found that the previous
> patch failed to handle negative values correctly, so the result was not
> as expected and they would like this issue fixed in the current update.
> We also consulted with the DE; the hardware itself supports negative
> numbers and all ICs behave the same in this regard. The current patch
> converts negative values into a format that the hardware can properly
> recognize.

OK, move this patch out of this series.
Send this patch independently.
And this is a fixup patch, so Add 'Fixes' tag.

Regards,
CK

> 
> Best Regards,
> Jay Liu
> 
> > > 
> > > Fixes: 738ed4156fba ("drm/mediatek: Add matrix_bits private data
> > > for ccorr")
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> > > Signed-off-by: 20220315152503 created <jay.liu@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 24 ++-----------------
> > > ----
> > >  1 file changed, 2 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > > b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > > index 85ba109d6383..b097c20877f3 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
> > > @@ -80,27 +80,6 @@ void mtk_ccorr_stop(struct device *dev)
> > >  	writel_relaxed(0x0, ccorr->regs + DISP_CCORR_EN);
> > >  }
> > >  
> > > -/* Converts a DRM S31.32 value to the HW S1.n format. */
> > > -static u16 mtk_ctm_s31_32_to_s1_n(u64 in, u32 n)
> > > -{
> > > -	u16 r;
> > > -
> > > -	/* Sign bit. */
> > > -	r = in & BIT_ULL(63) ? BIT(n + 1) : 0;
> > > -
> > > -	if ((in & GENMASK_ULL(62, 33)) > 0) {
> > > -		/* identity value 0x100000000 -> 0x400(mt8183), */
> > > -		/* identity value 0x100000000 -> 0x800(mt8192), */
> > > -		/* if bigger this, set it to max 0x7ff. */
> > > -		r |= GENMASK(n, 0);
> > > -	} else {
> > > -		/* take the n+1 most important bits. */
> > > -		r |= (in >> (32 - n)) & GENMASK(n, 0);
> > > -	}
> > > -
> > > -	return r;
> > > -}
> > > -
> > >  bool mtk_ccorr_ctm_set(struct device *dev, struct drm_crtc_state
> > > *state)
> > >  {
> > >  	struct mtk_disp_ccorr *ccorr = dev_get_drvdata(dev);
> > > @@ -109,6 +88,7 @@ bool mtk_ccorr_ctm_set(struct device *dev,
> > > struct drm_crtc_state *state)
> > >  	const u64 *input;
> > >  	uint16_t coeffs[9] = { 0 };
> > >  	int i;
> > > +	int int_bits = 2;
> > >  	struct cmdq_pkt *cmdq_pkt = NULL;
> > >  	u32 matrix_bits = ccorr->data->matrix_bits;
> > >  
> > > @@ -119,7 +99,7 @@ bool mtk_ccorr_ctm_set(struct device *dev,
> > > struct drm_crtc_state *state)
> > >  	input = ctm->matrix;
> > >  
> > >  	for (i = 0; i < ARRAY_SIZE(coeffs); i++)
> > > -		coeffs[i] = mtk_ctm_s31_32_to_s1_n(input[i],
> > > matrix_bits);
> > > +		coeffs[i] = drm_color_ctm_s31_32_to_qm_n(input[i],
> > > int_bits, matrix_bits);
> > >  
> > >  	mtk_ddp_write(cmdq_pkt, coeffs[0] << 16 | coeffs[1],
> > >  		      &ccorr->cmdq_reg, ccorr->regs,
> > > DISP_CCORR_COEF_0);
> > 
> >