| Message ID | 20241017082348.3413727-5-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 3 VDSC engines 12 slices | expand |
> -----Original Message----- > From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> > Sent: Thursday, October 17, 2024 1:54 PM > To: intel-gfx@lists.freedesktop.org > Cc: intel-xe@lists.freedesktop.org; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: [PATCH 04/10] drm/i915/vdsc: Add support for read/write PPS for > DSC3 > > With BMG each pipe has 3 DSC engines, so add bits to read/write the PPS > registers for the 3rd VDSC engine. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++--- > drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 6 ++++++ > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c > b/drivers/gpu/drm/i915/display/intel_vdsc.c > index e34483d5be36..718e1b400af5 100644 > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > @@ -411,8 +411,10 @@ static void intel_dsc_get_pps_reg(const struct > intel_crtc_state *crtc_state, int > > pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder); > > - if (dsc_reg_num >= 3) > + if (dsc_reg_num >= 4) > MISSING_CASE(dsc_reg_num); > + if (dsc_reg_num >= 3) > + dsc_reg[2] = BMG_DSC2_PPS(pipe, pps); > if (dsc_reg_num >= 2) Quick question why is this condition not == that would make sense only the first condition Should have been >= Maybe another patch to fix this Rest LGTM, Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> > dsc_reg[1] = pipe_dsc ? ICL_DSC1_PPS(pipe, pps) : > DSCC_PPS(pps); > if (dsc_reg_num >= 1) > @@ -424,7 +426,7 @@ static void intel_dsc_pps_write(const struct > intel_crtc_state *crtc_state, { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > - i915_reg_t dsc_reg[2]; > + i915_reg_t dsc_reg[3]; > int i, vdsc_per_pipe, dsc_reg_num; > > vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state); > @@ -824,7 +826,7 @@ static u32 intel_dsc_pps_read(struct intel_crtc_state > *crtc_state, int pps, { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > - i915_reg_t dsc_reg[2]; > + i915_reg_t dsc_reg[3]; > int i, vdsc_per_pipe, dsc_reg_num; > u32 val; > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h > b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h > index 941f4ff6b940..efaeb5e0aea3 100644 > --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h > +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h > @@ -61,8 +61,10 @@ > #define DSCC_PPS(pps) _MMIO(_DSCC_PPS_0 > + ((pps) < 12 ? (pps) : (pps) + 12) * 4) > #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB 0x78270 > #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB 0x78370 > +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB 0x78970 > #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC 0x78470 > #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC 0x78570 > +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC 0x78A70 > #define ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe) _MMIO_PIPE((pipe) - > PIPE_B, \ > > _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB, \ > > _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC) > @@ -75,8 +77,12 @@ > #define _ICL_DSC1_PPS_0(pipe) _PICK_EVEN((pipe) - > PIPE_B, \ > > _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB, \ > > _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC) > +#define _BMG_DSC2_PPS_0(pipe) _PICK_EVEN((pipe) - > PIPE_B, \ > + > _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB, \ > + > _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC) > #define ICL_DSC0_PPS(pipe, pps) > _MMIO(_ICL_DSC0_PPS_0(pipe) + ((pps) * 4)) > #define ICL_DSC1_PPS(pipe, pps) > _MMIO(_ICL_DSC1_PPS_0(pipe) + ((pps) * 4)) > +#define BMG_DSC2_PPS(pipe, pps) > _MMIO(_BMG_DSC2_PPS_0(pipe) + ((pps) * 4)) > > /* PPS 0 */ > #define DSC_PPS0_NATIVE_422_ENABLE REG_BIT(23) > -- > 2.45.2
Hi Suraj, Thanks for the review and comments. I agree with most of the comments on the series and will send new version with those addressed. Please find my explanation inline for your query: On 10/18/2024 7:47 AM, Kandpal, Suraj wrote: > >> -----Original Message----- >> From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> >> Sent: Thursday, October 17, 2024 1:54 PM >> To: intel-gfx@lists.freedesktop.org >> Cc: intel-xe@lists.freedesktop.org; Kandpal, Suraj >> <suraj.kandpal@intel.com> >> Subject: [PATCH 04/10] drm/i915/vdsc: Add support for read/write PPS for >> DSC3 >> >> With BMG each pipe has 3 DSC engines, so add bits to read/write the PPS >> registers for the 3rd VDSC engine. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++--- >> drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 6 ++++++ >> 2 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c >> b/drivers/gpu/drm/i915/display/intel_vdsc.c >> index e34483d5be36..718e1b400af5 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> @@ -411,8 +411,10 @@ static void intel_dsc_get_pps_reg(const struct >> intel_crtc_state *crtc_state, int >> >> pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder); >> >> - if (dsc_reg_num >= 3) >> + if (dsc_reg_num >= 4) >> MISSING_CASE(dsc_reg_num); >> + if (dsc_reg_num >= 3) >> + dsc_reg[2] = BMG_DSC2_PPS(pipe, pps); >> if (dsc_reg_num >= 2) > Quick question why is this condition not == that would make sense only the first condition > Should have been >= We want to set dsc_reg[1] for case where dsc_reg_num is 2 as well as when its 3 as we need to write PPS for 3 DSC engines. Changing to 'if (dsc_reg_num == 2)' will only set dsc_reg[2], but skip dsc_reg[1] when dsc_reg_num is 3. Thanks & Regards, Ankit > Maybe another patch to fix this > Rest LGTM, > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> > >> dsc_reg[1] = pipe_dsc ? ICL_DSC1_PPS(pipe, pps) : >> DSCC_PPS(pps); >> if (dsc_reg_num >= 1) >> @@ -424,7 +426,7 @@ static void intel_dsc_pps_write(const struct >> intel_crtc_state *crtc_state, { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> struct drm_i915_private *i915 = to_i915(crtc->base.dev); >> - i915_reg_t dsc_reg[2]; >> + i915_reg_t dsc_reg[3]; >> int i, vdsc_per_pipe, dsc_reg_num; >> >> vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state); >> @@ -824,7 +826,7 @@ static u32 intel_dsc_pps_read(struct intel_crtc_state >> *crtc_state, int pps, { >> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> struct drm_i915_private *i915 = to_i915(crtc->base.dev); >> - i915_reg_t dsc_reg[2]; >> + i915_reg_t dsc_reg[3]; >> int i, vdsc_per_pipe, dsc_reg_num; >> u32 val; >> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h >> b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h >> index 941f4ff6b940..efaeb5e0aea3 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h >> @@ -61,8 +61,10 @@ >> #define DSCC_PPS(pps) _MMIO(_DSCC_PPS_0 >> + ((pps) < 12 ? (pps) : (pps) + 12) * 4) >> #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB 0x78270 >> #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB 0x78370 >> +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB 0x78970 >> #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC 0x78470 >> #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC 0x78570 >> +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC 0x78A70 >> #define ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe) _MMIO_PIPE((pipe) - >> PIPE_B, \ >> >> _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB, \ >> >> _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC) >> @@ -75,8 +77,12 @@ >> #define _ICL_DSC1_PPS_0(pipe) _PICK_EVEN((pipe) - >> PIPE_B, \ >> >> _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB, \ >> >> _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC) >> +#define _BMG_DSC2_PPS_0(pipe) _PICK_EVEN((pipe) - >> PIPE_B, \ >> + >> _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB, \ >> + >> _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC) >> #define ICL_DSC0_PPS(pipe, pps) >> _MMIO(_ICL_DSC0_PPS_0(pipe) + ((pps) * 4)) >> #define ICL_DSC1_PPS(pipe, pps) >> _MMIO(_ICL_DSC1_PPS_0(pipe) + ((pps) * 4)) >> +#define BMG_DSC2_PPS(pipe, pps) >> _MMIO(_BMG_DSC2_PPS_0(pipe) + ((pps) * 4)) >> >> /* PPS 0 */ >> #define DSC_PPS0_NATIVE_422_ENABLE REG_BIT(23) >> -- >> 2.45.2
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index e34483d5be36..718e1b400af5 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -411,8 +411,10 @@ static void intel_dsc_get_pps_reg(const struct intel_crtc_state *crtc_state, int pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder); - if (dsc_reg_num >= 3) + if (dsc_reg_num >= 4) MISSING_CASE(dsc_reg_num); + if (dsc_reg_num >= 3) + dsc_reg[2] = BMG_DSC2_PPS(pipe, pps); if (dsc_reg_num >= 2) dsc_reg[1] = pipe_dsc ? ICL_DSC1_PPS(pipe, pps) : DSCC_PPS(pps); if (dsc_reg_num >= 1) @@ -424,7 +426,7 @@ static void intel_dsc_pps_write(const struct intel_crtc_state *crtc_state, { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *i915 = to_i915(crtc->base.dev); - i915_reg_t dsc_reg[2]; + i915_reg_t dsc_reg[3]; int i, vdsc_per_pipe, dsc_reg_num; vdsc_per_pipe = intel_dsc_get_vdsc_per_pipe(crtc_state); @@ -824,7 +826,7 @@ static u32 intel_dsc_pps_read(struct intel_crtc_state *crtc_state, int pps, { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *i915 = to_i915(crtc->base.dev); - i915_reg_t dsc_reg[2]; + i915_reg_t dsc_reg[3]; int i, vdsc_per_pipe, dsc_reg_num; u32 val; diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h index 941f4ff6b940..efaeb5e0aea3 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h @@ -61,8 +61,10 @@ #define DSCC_PPS(pps) _MMIO(_DSCC_PPS_0 + ((pps) < 12 ? (pps) : (pps) + 12) * 4) #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB 0x78270 #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB 0x78370 +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB 0x78970 #define _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC 0x78470 #define _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC 0x78570 +#define _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC 0x78A70 #define ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe) _MMIO_PIPE((pipe) - PIPE_B, \ _ICL_DSC0_PICTURE_PARAMETER_SET_0_PB, \ _ICL_DSC0_PICTURE_PARAMETER_SET_0_PC) @@ -75,8 +77,12 @@ #define _ICL_DSC1_PPS_0(pipe) _PICK_EVEN((pipe) - PIPE_B, \ _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB, \ _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC) +#define _BMG_DSC2_PPS_0(pipe) _PICK_EVEN((pipe) - PIPE_B, \ + _BMG_DSC2_PICTURE_PARAMETER_SET_0_PB, \ + _BMG_DSC2_PICTURE_PARAMETER_SET_0_PC) #define ICL_DSC0_PPS(pipe, pps) _MMIO(_ICL_DSC0_PPS_0(pipe) + ((pps) * 4)) #define ICL_DSC1_PPS(pipe, pps) _MMIO(_ICL_DSC1_PPS_0(pipe) + ((pps) * 4)) +#define BMG_DSC2_PPS(pipe, pps) _MMIO(_BMG_DSC2_PPS_0(pipe) + ((pps) * 4)) /* PPS 0 */ #define DSC_PPS0_NATIVE_422_ENABLE REG_BIT(23)
With BMG each pipe has 3 DSC engines, so add bits to read/write the PPS registers for the 3rd VDSC engine. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- drivers/gpu/drm/i915/display/intel_vdsc.c | 8 +++++--- drivers/gpu/drm/i915/display/intel_vdsc_regs.h | 6 ++++++ 2 files changed, 11 insertions(+), 3 deletions(-)