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

[Manasi Navare]

Thanks a lot Jani for your feedback and review. Please find my
comments below inline,

On Thu, Feb 1, 2024 at 1:15 AM Jani Nikula <jani.nikula at linux.intel.com> wrote:
>
>
> Please use "drm/i915/dsc: " as the subject prefix.

Okay I will change this

>
> On Wed, 31 Jan 2024, Manasi Navare <navaremanasi at 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")

Yes agreed

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

Okay will reword this

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

Okay will change this accordingly
.
>
> > Fixes: bd077259d0a9 ("drm/i915/vdsc: Add function to read any PPS register")
> > Cc: Suraj Kandpal <suraj.kandpal at intel.com>
> > Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> > Cc: Animesh Manna <animesh.manna at intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Sean Paul <sean at poorly.run>
> > Cc: Drew Davenport <ddavenport at chromium.org>
> > Signed-off-by: Manasi Navare <navaremanasi at 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)

Yes thanks for suggesting the simplification


>
> Also the macro arguments need to be wrapped in braces.

Your above suggestion should work as is right?
#define DSCC_PPS(pps)                 _MMIO(_DSCC_PPS_0 + ((pps) < 12
? (pps) : (pps) + 12) * 4)

Where are you suggesting extra braces?

Regards
Manasi

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