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@xxxxxxxxxxxxxxx> wrote: > > > Please use "drm/i915/dsc: " as the subject prefix. Okay I will change this > > On Wed, 31 Jan 2024, Manasi Navare <navaremanasi@xxxxxxxxxxxx> 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@xxxxxxxxx> > > Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> > > Cc: Animesh Manna <animesh.manna@xxxxxxxxx> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > Cc: Sean Paul <sean@xxxxxxxxxx> > > Cc: Drew Davenport <ddavenport@xxxxxxxxxxxx> > > Signed-off-by: Manasi Navare <navaremanasi@xxxxxxxxxxxx> > > --- > > 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