Re: [PATCH] drm/i915/display/vdsc: Fix the macro that calculates DSCC_/DSCA_ PPS reg address

[Jani Nikula <jani.nikula@linux.intel.com>]

Please use "drm/i915/dsc: " as the subject prefix.

On Wed, 31 Jan 2024, Manasi Navare <navaremanasi@chromium.org> wrote:
> Patch (bd077259d0a9: drm/i915/vdsc: Add function to read any PPS register) defines

Please use the usual style to refer to commits:

Commit bd077259d0a9 ("drm/i915/vdsc: Add function to read any PPS register")

> a new macro to calculate the DSC PPS register addresses with PPS number as an
> input. This macro correctly calculates the addresses till PPS 11 since the
> addresses increment by 4. So in that case the following macro works correctly
> to give correct register address:
> _MMIO(_DSCA_PPS_0 + (pps) * 4)
>
> However after PPS 11, the register address for PPS 12 increments by 12 because
> of RC Buffer memory allocation in between. Because of this discontinuity
> in the address space, the macro calculates wrong addresses for PPS 12 - 16
> resulting into incorrect DSC PPS parameter value read/writes causing DSC
> corruption.

Thanks for catching and debugging this!

> This patch fixes it by correcting this macro to add the offset of 12 for
> PPS >=12.

Please just say "Fix it ...". Once committed, this is no longer a patch.

> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/10172

Closes: instead of Bug:.

> Fixes: bd077259d0a9 ("drm/i915/vdsc: Add function to read any PPS register")
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Drew Davenport <ddavenport@chromium.org>
> Signed-off-by: Manasi Navare <navaremanasi@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> index 64f440fdc22b..db29660b74f3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> @@ -51,8 +51,8 @@
>  #define DSCC_PICTURE_PARAMETER_SET_0		_MMIO(0x6BA00)
>  #define _DSCA_PPS_0				0x6B200
>  #define _DSCC_PPS_0				0x6BA00
> -#define DSCA_PPS(pps)				_MMIO(_DSCA_PPS_0 + (pps) * 4)
> -#define DSCC_PPS(pps)				_MMIO(_DSCC_PPS_0 + (pps) * 4)
> +#define DSCA_PPS(pps)				((pps < 12) ? _MMIO(_DSCA_PPS_0 + (pps) * 4):_MMIO(_DSCA_PPS_0 + (pps + 12) * 4))
> +#define DSCC_PPS(pps)				((pps < 12) ? _MMIO(_DSCC_PPS_0 + (pps) * 4):_MMIO(_DSCC_PPS_0 + (pps + 12) * 4))

There's no need to duplicate so much here, this could be just:

	_MMIO(_DSCC_PPS_0 + ((pps) < 12 ? (pps) : (pps) + 12) * 4)

Also the macro arguments need to be wrapped in braces.

With the nitpicks fixed, LGTM.

BR,
Jani.

>  #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB	0x78270
>  #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB	0x78370
>  #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC	0x78470

-- 
Jani Nikula, Intel