| Message ID | 20240131211909.622419-1-navaremanasi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/vdsc: Fix the macro that calculates DSCC_/DSCA_ PPS reg address | expand |
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
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@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@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@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) 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
On Thu, 01 Feb 2024, Manasi Navare <navaremanasi@chromium.org> wrote: > On Thu, Feb 1, 2024 at 1:15 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Wed, 31 Jan 2024, Manasi Navare <navaremanasi@chromium.org> wrote: >> > 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? I've added them in my suggestion. The original had (pps < 12) and (pps + 12), which would fail if someone passed in an expression with a lower precedence than < or +. BR, Jani.
Thanks Jani, that makes sense and thanks for adding them in your suggestion. I have made the necessary changes addressing all your review comments and will send out a V2 for the patch. Regards Manasi On Thu, Feb 1, 2024 at 3:34 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Thu, 01 Feb 2024, Manasi Navare <navaremanasi@chromium.org> wrote: > > On Thu, Feb 1, 2024 at 1:15 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> On Wed, 31 Jan 2024, Manasi Navare <navaremanasi@chromium.org> wrote: > >> > 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? > > I've added them in my suggestion. > > The original had (pps < 12) and (pps + 12), which would fail if someone > passed in an expression with a lower precedence than < or +. > > BR, > Jani. > > > -- > Jani Nikula, Intel
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)) #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
Patch (bd077259d0a9: drm/i915/vdsc: Add function to read any PPS register) defines 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. This patch fixes it by correcting this macro to add the offset of 12 for PPS >=12. Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/10172 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(-)