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 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

> 
> 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);