[Intel-gfx] [PATCH 0/5] Add DSC PPS readout

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Intel-gfx] [PATCH 0/5] Add DSC PPS readout
@ 2023-07-10 10:09 Suraj Kandpal
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

Up until now we only verified one or two of the dsc pps
params like bits_per_component and bits_per_pixel this
patch series aim to readout almost all PPS param and get
them compared.
Along with that some work on making a common function to
read and write PPS param regiters is also done.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>


Suraj Kandpal (5):
  drm/i915/dsc: Add PPS enum
  drm/i915/vdsc: Add function to read any PPS register
  drm/i915/vdsc: Add function to write in PPS registers
  drm/i915/vdsc: Fill the intel_dsc_get_pps_config function
  drm/i915/display: Compare the readout dsc pps params

 drivers/gpu/drm/i915/display/intel_display.c  |  55 +
 drivers/gpu/drm/i915/display/intel_vdsc.c     | 944 +++++++++++++-----
 .../gpu/drm/i915/display/intel_vdsc_regs.h    |  53 +-
 3 files changed, 762 insertions(+), 290 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
@ 2023-07-10 10:09 ` Suraj Kandpal
  2023-07-10 10:45   ` Jani Nikula
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

Add PPS enum so that we can later on use it to distinguish which
PPS is being read or written onto.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index bd9116d2cd76..1a8272324c36 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -19,6 +19,23 @@
 #include "intel_vdsc.h"
 #include "intel_vdsc_regs.h"
 
+enum intel_dsc_pps {
+	PPS_0 = 0,
+	PPS_1,
+	PPS_2,
+	PPS_3,
+	PPS_4,
+	PPS_5,
+	PPS_6,
+	PPS_7,
+	PPS_8,
+	PPS_9,
+	PPS_10,
+	PPS_16,
+	PPS_17,
+	PPS_18,
+};
+
 bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state)
 {
 	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
@ 2023-07-10 10:45   ` Jani Nikula
  2023-07-10 10:45     ` Jani Nikula
  0 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 10:45 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Add PPS enum so that we can later on use it to distinguish which
> PPS is being read or written onto.

The patch adding the enum alone isn't useful, should be squashed with
something that uses it.

BR,
Jani.

>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index bd9116d2cd76..1a8272324c36 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -19,6 +19,23 @@
>  #include "intel_vdsc.h"
>  #include "intel_vdsc_regs.h"
>  
> +enum intel_dsc_pps {
> +	PPS_0 = 0,
> +	PPS_1,
> +	PPS_2,
> +	PPS_3,
> +	PPS_4,
> +	PPS_5,
> +	PPS_6,
> +	PPS_7,
> +	PPS_8,
> +	PPS_9,
> +	PPS_10,
> +	PPS_16,
> +	PPS_17,
> +	PPS_18,
> +};
> +
>  bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state)
>  {
>  	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum
  2023-07-10 10:45   ` Jani Nikula
@ 2023-07-10 10:45     ` Jani Nikula
  2023-07-11  6:12       ` Kandpal, Suraj
  0 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 10:45 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> Add PPS enum so that we can later on use it to distinguish which
>> PPS is being read or written onto.
>
> The patch adding the enum alone isn't useful, should be squashed with
> something that uses it.

Also, maybe you could just use an int?

>
> BR,
> Jani.
>
>>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_vdsc.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index bd9116d2cd76..1a8272324c36 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -19,6 +19,23 @@
>>  #include "intel_vdsc.h"
>>  #include "intel_vdsc_regs.h"
>>  
>> +enum intel_dsc_pps {
>> +	PPS_0 = 0,
>> +	PPS_1,
>> +	PPS_2,
>> +	PPS_3,
>> +	PPS_4,
>> +	PPS_5,
>> +	PPS_6,
>> +	PPS_7,
>> +	PPS_8,
>> +	PPS_9,
>> +	PPS_10,
>> +	PPS_16,
>> +	PPS_17,
>> +	PPS_18,
>> +};
>> +
>>  bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state)
>>  {
>>  	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum
  2023-07-10 10:45     ` Jani Nikula
@ 2023-07-11  6:12       ` Kandpal, Suraj
  0 siblings, 0 replies; 17+ messages in thread
From: Kandpal, Suraj @ 2023-07-11  6:12 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx@lists.freedesktop.org

> On Mon, 10 Jul 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> >> Add PPS enum so that we can later on use it to distinguish which PPS
> >> is being read or written onto.
> >
> > The patch adding the enum alone isn't useful, should be squashed with
> > something that uses it.
> 
> Also, maybe you could just use an int?
> 

I see will drop this patch and use int instead

Regards,
Suraj Kandpal
> >
> > BR,
> > Jani.
> >
> >>
> >> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_vdsc.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> index bd9116d2cd76..1a8272324c36 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> >> @@ -19,6 +19,23 @@
> >>  #include "intel_vdsc.h"
> >>  #include "intel_vdsc_regs.h"
> >>
> >> +enum intel_dsc_pps {
> >> +	PPS_0 = 0,
> >> +	PPS_1,
> >> +	PPS_2,
> >> +	PPS_3,
> >> +	PPS_4,
> >> +	PPS_5,
> >> +	PPS_6,
> >> +	PPS_7,
> >> +	PPS_8,
> >> +	PPS_9,
> >> +	PPS_10,
> >> +	PPS_16,
> >> +	PPS_17,
> >> +	PPS_18,
> >> +};
> >> +
> >>  bool intel_dsc_source_support(const struct intel_crtc_state
> >> *crtc_state)  {
> >>  	const struct intel_crtc *crtc =
> >> to_intel_crtc(crtc_state->uapi.crtc);
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
@ 2023-07-10 10:09 ` Suraj Kandpal
  2023-07-10 10:52   ` Jani Nikula
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers Suraj Kandpal
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

Add function to read any PPS register based on the
intel_dsc_pps enum provided. Add a function which will call the
new pps read function and place it in crtc state. Only PPS0 and
PPS1 are readout the rest of the registers will be read in upcoming
patches.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c | 293 ++++++++++++++++++++--
 1 file changed, 272 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 1a8272324c36..53eac8d9c80f 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -940,16 +940,284 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
 	}
 }
 
+static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
+				   enum intel_dsc_pps pps, u32 *pps_val)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 pps_temp;
+
+	switch (pps) {
+	case PPS_0:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_0);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_0);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_1:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_1);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_1);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_2:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_2);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_2);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_3:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_3);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_3);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_4:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_4);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_4);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_5:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_5);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_5);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_6:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_6);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_6);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_7:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_7);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_7);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_8:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_8);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_8);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_9:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_9);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_9);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_10:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_10);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_10);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	case PPS_16:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_16);
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_16);
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		} else {
+			*pps_val = intel_de_read(i915,
+						 ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe));
+			if (crtc_state->dsc.dsc_split) {
+				pps_temp =
+					intel_de_read(i915,
+						      ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe));
+				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+			}
+		}
+		break;
+	/* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
+	case PPS_17:
+		*pps_val = intel_de_read(i915,
+					 MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe));
+		if (crtc_state->dsc.dsc_split) {
+			pps_temp =
+				intel_de_read(i915,
+					      MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe));
+			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+		}
+		break;
+	case PPS_18:
+		*pps_val = intel_de_read(i915,
+					 MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe));
+		if (crtc_state->dsc.dsc_split) {
+			pps_temp =
+				intel_de_read(i915,
+					      MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe));
+			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
+		}
+		break;
+	default:
+		drm_err(&i915->drm, "PPS register does not exist\n");
+		break;
+	}
+}
+
+static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
+{
+	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
+	u32 pps_temp1, pps_temp2;
+
+	/* Readout PPS_0 and PPS_1 registers */
+	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
+	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
+
+	vdsc_cfg->bits_per_pixel = pps_temp2;
+
+	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
+		vdsc_cfg->bits_per_pixel >>= 1;
+
+	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
+}
+
 void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	enum pipe pipe = crtc->pipe;
 	enum intel_display_power_domain power_domain;
 	intel_wakeref_t wakeref;
-	u32 dss_ctl1, dss_ctl2, pps0 = 0, pps1 = 0;
+	u32 dss_ctl1, dss_ctl2;
 
 	if (!intel_dsc_source_support(crtc_state))
 		return;
@@ -970,24 +1238,7 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
 	crtc_state->dsc.dsc_split = (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE) &&
 		(dss_ctl1 & JOINER_ENABLE);
 
-	/* FIXME: add more state readout as needed */
-
-	/* PPS0 & PPS1 */
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		pps1 = intel_de_read(dev_priv, DSCA_PICTURE_PARAMETER_SET_1);
-	} else {
-		pps0 = intel_de_read(dev_priv,
-				     ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
-		pps1 = intel_de_read(dev_priv,
-				     ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
-	}
-
-	vdsc_cfg->bits_per_pixel = pps1;
-
-	if (pps0 & DSC_NATIVE_420_ENABLE)
-		vdsc_cfg->bits_per_pixel >>= 1;
-
-	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
+	intel_dsc_get_pps_config(crtc_state);
 out:
 	intel_display_power_put(dev_priv, power_domain, wakeref);
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
@ 2023-07-10 10:52   ` Jani Nikula
  2023-07-10 10:56     ` Jani Nikula
  0 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 10:52 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Add function to read any PPS register based on the
> intel_dsc_pps enum provided. Add a function which will call the
> new pps read function and place it in crtc state. Only PPS0 and
> PPS1 are readout the rest of the registers will be read in upcoming
> patches.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 293 ++++++++++++++++++++--
>  1 file changed, 272 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 1a8272324c36..53eac8d9c80f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -940,16 +940,284 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
>  	}
>  }
>  
> +static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
> +				   enum intel_dsc_pps pps, u32 *pps_val)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 pps_temp;
> +
> +	switch (pps) {
> +	case PPS_0:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_0);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_0);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}

This function is just a huge amount of duplicated copy-paste.

What you need is a function to return the *registers* for each pps
index. And yeah, I don't think the enum is needed.

We should also switch the negative !is_pipe_dsc() positive
is_pipe_dsc().

Refactoring should never introduce functional changes. You should
separate the two. This adds the if (crtc_state->dsc.dsc_split)
conditions which doesn't exist in the original.

BR,
Jani.


> +		break;
> +	case PPS_1:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_1);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_1);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_2:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_2);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_2);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_3:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_3);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_3);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_4:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_4);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_4);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_5:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_5);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_5);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_6:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_6);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_6);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_7:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_7);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_7);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_8:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_8);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_8);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_9:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_9);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_9);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_10:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_10);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_10);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	case PPS_16:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_16);
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_16);
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		} else {
> +			*pps_val = intel_de_read(i915,
> +						 ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe));
> +			if (crtc_state->dsc.dsc_split) {
> +				pps_temp =
> +					intel_de_read(i915,
> +						      ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe));
> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +			}
> +		}
> +		break;
> +	/* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
> +	case PPS_17:
> +		*pps_val = intel_de_read(i915,
> +					 MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe));
> +		if (crtc_state->dsc.dsc_split) {
> +			pps_temp =
> +				intel_de_read(i915,
> +					      MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe));
> +			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +		}
> +		break;
> +	case PPS_18:
> +		*pps_val = intel_de_read(i915,
> +					 MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe));
> +		if (crtc_state->dsc.dsc_split) {
> +			pps_temp =
> +				intel_de_read(i915,
> +					      MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe));
> +			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> +		}
> +		break;
> +	default:
> +		drm_err(&i915->drm, "PPS register does not exist\n");
> +		break;
> +	}
> +}
> +
> +static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> +	u32 pps_temp1, pps_temp2;
> +
> +	/* Readout PPS_0 and PPS_1 registers */
> +	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
> +	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
> +
> +	vdsc_cfg->bits_per_pixel = pps_temp2;
> +
> +	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
> +		vdsc_cfg->bits_per_pixel >>= 1;
> +
> +	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> +}
> +
>  void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	enum pipe pipe = crtc->pipe;
>  	enum intel_display_power_domain power_domain;
>  	intel_wakeref_t wakeref;
> -	u32 dss_ctl1, dss_ctl2, pps0 = 0, pps1 = 0;
> +	u32 dss_ctl1, dss_ctl2;
>  
>  	if (!intel_dsc_source_support(crtc_state))
>  		return;
> @@ -970,24 +1238,7 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
>  	crtc_state->dsc.dsc_split = (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE) &&
>  		(dss_ctl1 & JOINER_ENABLE);
>  
> -	/* FIXME: add more state readout as needed */
> -
> -	/* PPS0 & PPS1 */
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		pps1 = intel_de_read(dev_priv, DSCA_PICTURE_PARAMETER_SET_1);
> -	} else {
> -		pps0 = intel_de_read(dev_priv,
> -				     ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
> -		pps1 = intel_de_read(dev_priv,
> -				     ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
> -	}
> -
> -	vdsc_cfg->bits_per_pixel = pps1;
> -
> -	if (pps0 & DSC_NATIVE_420_ENABLE)
> -		vdsc_cfg->bits_per_pixel >>= 1;
> -
> -	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> +	intel_dsc_get_pps_config(crtc_state);
>  out:
>  	intel_display_power_put(dev_priv, power_domain, wakeref);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register
  2023-07-10 10:52   ` Jani Nikula
@ 2023-07-10 10:56     ` Jani Nikula
  0 siblings, 0 replies; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 10:56 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
>> Add function to read any PPS register based on the
>> intel_dsc_pps enum provided. Add a function which will call the
>> new pps read function and place it in crtc state. Only PPS0 and
>> PPS1 are readout the rest of the registers will be read in upcoming
>> patches.
>>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_vdsc.c | 293 ++++++++++++++++++++--
>>  1 file changed, 272 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index 1a8272324c36..53eac8d9c80f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -940,16 +940,284 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
>>  	}
>>  }
>>  
>> +static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
>> +				   enum intel_dsc_pps pps, u32 *pps_val)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 pps_temp;
>> +
>> +	switch (pps) {
>> +	case PPS_0:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_0);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_0);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>
> This function is just a huge amount of duplicated copy-paste.
>
> What you need is a function to return the *registers* for each pps
> index. And yeah, I don't think the enum is needed.
>
> We should also switch the negative !is_pipe_dsc() positive
> is_pipe_dsc().
>
> Refactoring should never introduce functional changes. You should
> separate the two. This adds the if (crtc_state->dsc.dsc_split)
> conditions which doesn't exist in the original.

Moreover, are you sure a verification with a warning belongs at the low
level read function? They belong at different abstraction layers.

Add the low level read function, then wrap it with something that
verifies.

BR,
Jani.


>
> BR,
> Jani.
>
>
>> +		break;
>> +	case PPS_1:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_1);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_1);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_2:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_2);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_2);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_3:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_3);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_3);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_4:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_4);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_4);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_5:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_5);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_5);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_6:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_6);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_6);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_7:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_7);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_7);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_8:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_8);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_8);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_9:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_9);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_9);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_10:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_10);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_10);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	case PPS_16:
>> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> +			*pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_16);
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_16);
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		} else {
>> +			*pps_val = intel_de_read(i915,
>> +						 ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe));
>> +			if (crtc_state->dsc.dsc_split) {
>> +				pps_temp =
>> +					intel_de_read(i915,
>> +						      ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe));
>> +				drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +			}
>> +		}
>> +		break;
>> +	/* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
>> +	case PPS_17:
>> +		*pps_val = intel_de_read(i915,
>> +					 MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe));
>> +		if (crtc_state->dsc.dsc_split) {
>> +			pps_temp =
>> +				intel_de_read(i915,
>> +					      MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe));
>> +			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +		}
>> +		break;
>> +	case PPS_18:
>> +		*pps_val = intel_de_read(i915,
>> +					 MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe));
>> +		if (crtc_state->dsc.dsc_split) {
>> +			pps_temp =
>> +				intel_de_read(i915,
>> +					      MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe));
>> +			drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
>> +		}
>> +		break;
>> +	default:
>> +		drm_err(&i915->drm, "PPS register does not exist\n");
>> +		break;
>> +	}
>> +}
>> +
>> +static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
>> +{
>> +	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
>> +	u32 pps_temp1, pps_temp2;
>> +
>> +	/* Readout PPS_0 and PPS_1 registers */
>> +	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
>> +	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
>> +
>> +	vdsc_cfg->bits_per_pixel = pps_temp2;
>> +
>> +	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
>> +		vdsc_cfg->bits_per_pixel >>= 1;
>> +
>> +	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
>> +}
>> +
>>  void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
>>  {
>>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> -	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
>>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> -	enum pipe pipe = crtc->pipe;
>>  	enum intel_display_power_domain power_domain;
>>  	intel_wakeref_t wakeref;
>> -	u32 dss_ctl1, dss_ctl2, pps0 = 0, pps1 = 0;
>> +	u32 dss_ctl1, dss_ctl2;
>>  
>>  	if (!intel_dsc_source_support(crtc_state))
>>  		return;
>> @@ -970,24 +1238,7 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
>>  	crtc_state->dsc.dsc_split = (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE) &&
>>  		(dss_ctl1 & JOINER_ENABLE);
>>  
>> -	/* FIXME: add more state readout as needed */
>> -
>> -	/* PPS0 & PPS1 */
>> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
>> -		pps1 = intel_de_read(dev_priv, DSCA_PICTURE_PARAMETER_SET_1);
>> -	} else {
>> -		pps0 = intel_de_read(dev_priv,
>> -				     ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
>> -		pps1 = intel_de_read(dev_priv,
>> -				     ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
>> -	}
>> -
>> -	vdsc_cfg->bits_per_pixel = pps1;
>> -
>> -	if (pps0 & DSC_NATIVE_420_ENABLE)
>> -		vdsc_cfg->bits_per_pixel >>= 1;
>> -
>> -	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
>> +	intel_dsc_get_pps_config(crtc_state);
>>  out:
>>  	intel_display_power_put(dev_priv, power_domain, wakeref);
>>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
@ 2023-07-10 10:09 ` Suraj Kandpal
  2023-07-10 10:57   ` Jani Nikula
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

Now that we have a function that reads any PPS register based
on intel_dsc_pps enum provided lets create a function that can
write on any PPS.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c | 494 +++++++++++-----------
 1 file changed, 252 insertions(+), 242 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 53eac8d9c80f..274d82360c1a 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -310,6 +310,244 @@ intel_dsc_power_domain(struct intel_crtc *crtc, enum transcoder cpu_transcoder)
 		return POWER_DOMAIN_TRANSCODER_VDSC_PW2;
 }
 
+static void intel_dsc_write_pps_reg(const struct intel_crtc_state *crtc_state,
+				    enum intel_dsc_pps pps, u32 pps_val)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	switch (pps) {
+	case PPS_0:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_0,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_0,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_1:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_1,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_1,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_2:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_2,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_2,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_3:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_3,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_3,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_4:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_4,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_4,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_5:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_5,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_5,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_6:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_6,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_6,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_7:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_7,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_7,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_8:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_8,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_8,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_9:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_9,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_9,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_10:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_10,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_10,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe),
+					       pps_val);
+		}
+		break;
+	case PPS_16:
+		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
+			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_16,
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_16,
+					       pps_val);
+		} else {
+			intel_de_write(i915,
+				       ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe),
+				       pps_val);
+			if (crtc_state->dsc.dsc_split)
+				intel_de_write(i915,
+					       ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe),
+					       pps_val);
+		}
+		break;
+	/* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
+	case PPS_17:
+		intel_de_write(i915,
+			       MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe),
+			       pps_val);
+		if (crtc_state->dsc.dsc_split)
+			intel_de_write(i915,
+				       MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe),
+				       pps_val);
+		break;
+	case PPS_18:
+		intel_de_write(i915,
+			       MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe),
+			       pps_val);
+		if (crtc_state->dsc.dsc_split)
+			intel_de_write(i915,
+				       MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe),
+				       pps_val);
+		break;
+	default:
+		drm_err(&i915->drm, "PPS register does not exist\n");
+		break;
+	}
+}
+
 static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -347,149 +585,41 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 	if (vdsc_cfg->vbr_enable)
 		pps_val |= DSC_VBR_ENABLE;
 	drm_dbg_kms(&dev_priv->drm, "PPS0 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_0,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_0,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_0, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_1 registers */
 	pps_val = 0;
 	pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel);
 	drm_dbg_kms(&dev_priv->drm, "PPS1 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_1,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_1,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_1, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_2 registers */
 	pps_val = 0;
 	pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) |
 		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances);
 	drm_dbg_kms(&dev_priv->drm, "PPS2 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_2,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_2,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_2, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_3 registers */
 	pps_val = 0;
 	pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) |
 		DSC_SLICE_WIDTH(vdsc_cfg->slice_width);
 	drm_dbg_kms(&dev_priv->drm, "PPS3 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_3,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_3,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_3, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_4 registers */
 	pps_val = 0;
 	pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay) |
 		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay);
 	drm_dbg_kms(&dev_priv->drm, "PPS4 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_4,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_4,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_4, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_5 registers */
 	pps_val = 0;
 	pps_val |= DSC_SCALE_INC_INT(vdsc_cfg->scale_increment_interval) |
 		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval);
 	drm_dbg_kms(&dev_priv->drm, "PPS5 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_5,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_5,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_5, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_6 registers */
 	pps_val = 0;
@@ -498,100 +628,28 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) |
 		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp);
 	drm_dbg_kms(&dev_priv->drm, "PPS6 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_6,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_6,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_6, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_7 registers */
 	pps_val = 0;
 	pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) |
 		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset);
 	drm_dbg_kms(&dev_priv->drm, "PPS7 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_7,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_7,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_7, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_8 registers */
 	pps_val = 0;
 	pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) |
 		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset);
 	drm_dbg_kms(&dev_priv->drm, "PPS8 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_8,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_8,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_8, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_9 registers */
 	pps_val = 0;
 	pps_val |= DSC_RC_MODEL_SIZE(vdsc_cfg->rc_model_size) |
 		DSC_RC_EDGE_FACTOR(DSC_RC_EDGE_FACTOR_CONST);
 	drm_dbg_kms(&dev_priv->drm, "PPS9 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_9,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_9,
-				       pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_9, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_10 registers */
 	pps_val = 0;
@@ -600,25 +658,7 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 		DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) |
 		DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST);
 	drm_dbg_kms(&dev_priv->drm, "PPS10 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_10,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       DSCC_PICTURE_PARAMETER_SET_10, pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_10, pps_val);
 
 	/* Populate Picture parameter set 16 */
 	pps_val = 0;
@@ -628,51 +668,21 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 		DSC_SLICE_ROW_PER_FRAME(vdsc_cfg->pic_height /
 					vdsc_cfg->slice_height);
 	drm_dbg_kms(&dev_priv->drm, "PPS16 = 0x%08x\n", pps_val);
-	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
-		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_16,
-			       pps_val);
-		/*
-		 * If 2 VDSC instances are needed, configure PPS for second
-		 * VDSC
-		 */
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       DSCC_PICTURE_PARAMETER_SET_16, pps_val);
-	} else {
-		intel_de_write(dev_priv,
-			       ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe),
-				       pps_val);
-	}
+	intel_dsc_write_pps_reg(crtc_state, PPS_16, pps_val);
 
 	if (DISPLAY_VER(dev_priv) >= 14) {
 		/* Populate PICTURE_PARAMETER_SET_17 registers */
 		pps_val = 0;
 		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset);
 		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n", pps_val);
-		intel_de_write(dev_priv,
-			       MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe),
-				       pps_val);
+		intel_dsc_write_pps_reg(crtc_state, PPS_17, pps_val);
 
 		/* Populate PICTURE_PARAMETER_SET_18 registers */
 		pps_val = 0;
 		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset) |
 			   DSC_SL_OFFSET_ADJ(vdsc_cfg->second_line_offset_adj);
 		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n", pps_val);
-		intel_de_write(dev_priv,
-			       MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe),
-			       pps_val);
-		if (crtc_state->dsc.dsc_split)
-			intel_de_write(dev_priv,
-				       MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe),
-				       pps_val);
+		intel_dsc_write_pps_reg(crtc_state, PPS_18, pps_val);
 	}
 
 	/* Populate the RC_BUF_THRESH registers */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers Suraj Kandpal
@ 2023-07-10 10:57   ` Jani Nikula
  0 siblings, 0 replies; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 10:57 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Now that we have a function that reads any PPS register based
> on intel_dsc_pps enum provided lets create a function that can
> write on any PPS.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 494 +++++++++++-----------
>  1 file changed, 252 insertions(+), 242 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 53eac8d9c80f..274d82360c1a 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -310,6 +310,244 @@ intel_dsc_power_domain(struct intel_crtc *crtc, enum transcoder cpu_transcoder)
>  		return POWER_DOMAIN_TRANSCODER_VDSC_PW2;
>  }
>  
> +static void intel_dsc_write_pps_reg(const struct intel_crtc_state *crtc_state,
> +				    enum intel_dsc_pps pps, u32 pps_val)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +
> +	switch (pps) {
> +	case PPS_0:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_0,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_0,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe),
> +					       pps_val);
> +		}

Ditto here, lots of duplication that just shouldn't exist.

BR,
Jani.

> +		break;
> +	case PPS_1:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_1,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_1,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_2:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_2,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_2,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_3:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_3,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_3,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_4:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_4,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_4,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_5:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_5,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_5,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_6:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_6,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_6,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_7:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_7,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_7,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_8:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_8,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_8,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_9:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_9,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_9,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_10:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_10,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_10,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	case PPS_16:
> +		if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> +			intel_de_write(i915, DSCA_PICTURE_PARAMETER_SET_16,
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915, DSCC_PICTURE_PARAMETER_SET_16,
> +					       pps_val);
> +		} else {
> +			intel_de_write(i915,
> +				       ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe),
> +				       pps_val);
> +			if (crtc_state->dsc.dsc_split)
> +				intel_de_write(i915,
> +					       ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe),
> +					       pps_val);
> +		}
> +		break;
> +	/* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
> +	case PPS_17:
> +		intel_de_write(i915,
> +			       MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe),
> +			       pps_val);
> +		if (crtc_state->dsc.dsc_split)
> +			intel_de_write(i915,
> +				       MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe),
> +				       pps_val);
> +		break;
> +	case PPS_18:
> +		intel_de_write(i915,
> +			       MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe),
> +			       pps_val);
> +		if (crtc_state->dsc.dsc_split)
> +			intel_de_write(i915,
> +				       MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe),
> +				       pps_val);
> +		break;
> +	default:
> +		drm_err(&i915->drm, "PPS register does not exist\n");
> +		break;
> +	}
> +}
> +
>  static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -347,149 +585,41 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  	if (vdsc_cfg->vbr_enable)
>  		pps_val |= DSC_VBR_ENABLE;
>  	drm_dbg_kms(&dev_priv->drm, "PPS0 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_0,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_0,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_0, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_1 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel);
>  	drm_dbg_kms(&dev_priv->drm, "PPS1 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_1,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_1,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_1, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_2 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) |
>  		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances);
>  	drm_dbg_kms(&dev_priv->drm, "PPS2 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_2,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_2,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_2, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_3 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) |
>  		DSC_SLICE_WIDTH(vdsc_cfg->slice_width);
>  	drm_dbg_kms(&dev_priv->drm, "PPS3 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_3,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_3,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_3, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_4 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay) |
>  		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay);
>  	drm_dbg_kms(&dev_priv->drm, "PPS4 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_4,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_4,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_4, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_5 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SCALE_INC_INT(vdsc_cfg->scale_increment_interval) |
>  		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval);
>  	drm_dbg_kms(&dev_priv->drm, "PPS5 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_5,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_5,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_5, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_6 registers */
>  	pps_val = 0;
> @@ -498,100 +628,28 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) |
>  		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp);
>  	drm_dbg_kms(&dev_priv->drm, "PPS6 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_6,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_6,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_6, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_7 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) |
>  		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset);
>  	drm_dbg_kms(&dev_priv->drm, "PPS7 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_7,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_7,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_7, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_8 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) |
>  		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset);
>  	drm_dbg_kms(&dev_priv->drm, "PPS8 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_8,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_8,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_8, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_9 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_RC_MODEL_SIZE(vdsc_cfg->rc_model_size) |
>  		DSC_RC_EDGE_FACTOR(DSC_RC_EDGE_FACTOR_CONST);
>  	drm_dbg_kms(&dev_priv->drm, "PPS9 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_9,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv, DSCC_PICTURE_PARAMETER_SET_9,
> -				       pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_9, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_10 registers */
>  	pps_val = 0;
> @@ -600,25 +658,7 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  		DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) |
>  		DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST);
>  	drm_dbg_kms(&dev_priv->drm, "PPS10 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_10,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       DSCC_PICTURE_PARAMETER_SET_10, pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_10, pps_val);
>  
>  	/* Populate Picture parameter set 16 */
>  	pps_val = 0;
> @@ -628,51 +668,21 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  		DSC_SLICE_ROW_PER_FRAME(vdsc_cfg->pic_height /
>  					vdsc_cfg->slice_height);
>  	drm_dbg_kms(&dev_priv->drm, "PPS16 = 0x%08x\n", pps_val);
> -	if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> -		intel_de_write(dev_priv, DSCA_PICTURE_PARAMETER_SET_16,
> -			       pps_val);
> -		/*
> -		 * If 2 VDSC instances are needed, configure PPS for second
> -		 * VDSC
> -		 */
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       DSCC_PICTURE_PARAMETER_SET_16, pps_val);
> -	} else {
> -		intel_de_write(dev_priv,
> -			       ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe),
> -				       pps_val);
> -	}
> +	intel_dsc_write_pps_reg(crtc_state, PPS_16, pps_val);
>  
>  	if (DISPLAY_VER(dev_priv) >= 14) {
>  		/* Populate PICTURE_PARAMETER_SET_17 registers */
>  		pps_val = 0;
>  		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset);
>  		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n", pps_val);
> -		intel_de_write(dev_priv,
> -			       MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe),
> -				       pps_val);
> +		intel_dsc_write_pps_reg(crtc_state, PPS_17, pps_val);
>  
>  		/* Populate PICTURE_PARAMETER_SET_18 registers */
>  		pps_val = 0;
>  		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset) |
>  			   DSC_SL_OFFSET_ADJ(vdsc_cfg->second_line_offset_adj);
>  		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n", pps_val);
> -		intel_de_write(dev_priv,
> -			       MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe),
> -			       pps_val);
> -		if (crtc_state->dsc.dsc_split)
> -			intel_de_write(dev_priv,
> -				       MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe),
> -				       pps_val);
> +		intel_dsc_write_pps_reg(crtc_state, PPS_18, pps_val);
>  	}
>  
>  	/* Populate the RC_BUF_THRESH registers */

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
                   ` (2 preceding siblings ...)
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers Suraj Kandpal
@ 2023-07-10 10:09 ` Suraj Kandpal
  2023-07-10 11:11   ` Jani Nikula
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
  2023-07-10 13:45 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Add DSC PPS readout Patchwork
  5 siblings, 1 reply; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

We have setup both the read and write functions so we can
move ahead and fill in all the readout state from PPS register
into the crtc_state so we can send it for comparision.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c     | 152 +++++++++++++++---
 .../gpu/drm/i915/display/intel_vdsc_regs.h    |  53 ++++--
 2 files changed, 172 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 274d82360c1a..a4f5b4aceb33 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -596,51 +596,51 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 	/* Populate PICTURE_PARAMETER_SET_2 registers */
 	pps_val = 0;
 	pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) |
-		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances);
+		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS2 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_2, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_3 registers */
 	pps_val = 0;
 	pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) |
-		DSC_SLICE_WIDTH(vdsc_cfg->slice_width);
+		DSC_SLICE_WIDTH(vdsc_cfg->slice_width, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS3 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_3, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_4 registers */
 	pps_val = 0;
 	pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay) |
-		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay);
+		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS4 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_4, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_5 registers */
 	pps_val = 0;
 	pps_val |= DSC_SCALE_INC_INT(vdsc_cfg->scale_increment_interval) |
-		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval);
+		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS5 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_5, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_6 registers */
 	pps_val = 0;
 	pps_val |= DSC_INITIAL_SCALE_VALUE(vdsc_cfg->initial_scale_value) |
-		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg->first_line_bpg_offset) |
-		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) |
-		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp);
+		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg->first_line_bpg_offset, true) |
+		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp, true) |
+		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS6 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_6, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_7 registers */
 	pps_val = 0;
 	pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) |
-		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset);
+		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS7 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_7, pps_val);
 
 	/* Populate PICTURE_PARAMETER_SET_8 registers */
 	pps_val = 0;
 	pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) |
-		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset);
+		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset, true);
 	drm_dbg_kms(&dev_priv->drm, "PPS8 = 0x%08x\n", pps_val);
 	intel_dsc_write_pps_reg(crtc_state, PPS_8, pps_val);
 
@@ -654,7 +654,7 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 	/* Populate PICTURE_PARAMETER_SET_10 registers */
 	pps_val = 0;
 	pps_val |= DSC_RC_QUANT_INC_LIMIT0(vdsc_cfg->rc_quant_incr_limit0) |
-		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg->rc_quant_incr_limit1) |
+		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg->rc_quant_incr_limit1, true) |
 		DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) |
 		DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST);
 	drm_dbg_kms(&dev_priv->drm, "PPS10 = 0x%08x\n", pps_val);
@@ -673,13 +673,14 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
 	if (DISPLAY_VER(dev_priv) >= 14) {
 		/* Populate PICTURE_PARAMETER_SET_17 registers */
 		pps_val = 0;
-		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset);
+		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset,
+					     true);
 		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n", pps_val);
 		intel_dsc_write_pps_reg(crtc_state, PPS_17, pps_val);
 
 		/* Populate PICTURE_PARAMETER_SET_18 registers */
 		pps_val = 0;
-		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset) |
+		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset, true) |
 			   DSC_SL_OFFSET_ADJ(vdsc_cfg->second_line_offset_adj);
 		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n", pps_val);
 		intel_dsc_write_pps_reg(crtc_state, PPS_18, pps_val);
@@ -1206,18 +1207,133 @@ static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
 static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
 {
 	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
-	u32 pps_temp1, pps_temp2;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	u32 pps_temp;
+
+	/* Readout PPS_0 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp);
 
-	/* Readout PPS_0 and PPS_1 registers */
-	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
-	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
+	vdsc_cfg->bits_per_component = (pps_temp & DSC_BPC_MASK) >> DSC_BPC_SHIFT;
+	vdsc_cfg->line_buf_depth =
+		(pps_temp & DSC_LINE_BUF_DEPTH_MASK) >> DSC_LINE_BUF_DEPTH_SHIFT;
+	vdsc_cfg->block_pred_enable = pps_temp & DSC_BLOCK_PREDICTION ? true : false;
+	vdsc_cfg->convert_rgb = pps_temp & DSC_COLOR_SPACE_CONVERSION ? true : false;
+	vdsc_cfg->simple_422 = pps_temp & DSC_422_ENABLE ? true : false;
+	vdsc_cfg->native_422 = pps_temp & DSC_NATIVE_422_ENABLE ? true : false;
+	vdsc_cfg->native_420 = pps_temp & DSC_NATIVE_420_ENABLE ? true : false;
+	vdsc_cfg->vbr_enable = pps_temp & DSC_VBR_ENABLE ? true : false;
 
-	vdsc_cfg->bits_per_pixel = pps_temp2;
+	/* Readout PPS_1 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp);
 
-	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
+	vdsc_cfg->bits_per_pixel = pps_temp;
+
+	if (vdsc_cfg->native_420)
 		vdsc_cfg->bits_per_pixel >>= 1;
 
 	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
+
+	/* Readout PPS_2 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_2, &pps_temp);
+
+	vdsc_cfg->pic_width =
+		DSC_PIC_WIDTH(pps_temp & DSC_PIC_WIDTH_MASK, false);
+	vdsc_cfg->pic_height = pps_temp & ~DSC_PIC_WIDTH_MASK;
+
+	/* Readout PPS_3 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_3, &pps_temp);
+
+	vdsc_cfg->slice_width =
+		DSC_SLICE_WIDTH(pps_temp & DSC_SLICE_WIDTH_MASK, false);
+	vdsc_cfg->slice_height = pps_temp & ~DSC_SLICE_WIDTH_MASK;
+
+	/* Readout PPS_4 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_4, &pps_temp);
+
+	vdsc_cfg->initial_dec_delay =
+		DSC_INITIAL_DEC_DELAY(pps_temp & DSC_INITIAL_DEC_DELAY_MASK, false);
+	vdsc_cfg->initial_xmit_delay = pps_temp & DSC_INITIAL_XMIT_DELAY_MASK;
+
+	/* Readout PPS_5 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_5, &pps_temp);
+
+	vdsc_cfg->scale_decrement_interval =
+		DSC_SCALE_DEC_INT(pps_temp & DSC_SCALE_DEC_INT_MASK, false);
+	vdsc_cfg->scale_increment_interval = pps_temp & DSC_SCALE_INC_INT_MASK;
+
+	/* Readout PPS_6 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_6, &pps_temp);
+
+	vdsc_cfg->initial_scale_value = pps_temp & DSC_INITIAL_SCALE_VALUE_MASK;
+	vdsc_cfg->first_line_bpg_offset =
+		DSC_FIRST_LINE_BPG_OFFSET(pps_temp &
+					  DSC_FIRST_LINE_BPG_OFFSET_MASK, false);
+	vdsc_cfg->flatness_min_qp =
+		DSC_FLATNESS_MIN_QP(pps_temp & DSC_FLATNESS_MIN_QP_MASK, false);
+	vdsc_cfg->flatness_max_qp =
+		DSC_FLATNESS_MAX_QP(pps_temp & DSC_FLATNESS_MAX_QP_MASK, false);
+
+	/* Readout PPS_7 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_7, &pps_temp);
+
+	vdsc_cfg->nfl_bpg_offset =
+		DSC_NFL_BPG_OFFSET(pps_temp & DSC_NFL_BPG_OFFSET_MASK, false);
+	vdsc_cfg->slice_bpg_offset = pps_temp & ~DSC_NFL_BPG_OFFSET_MASK;
+
+	/* Readout PPS_8 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_8, &pps_temp);
+
+	vdsc_cfg->initial_offset =
+		DSC_INITIAL_OFFSET(pps_temp & DSC_INITIAL_OFFSET_MASK, false);
+	vdsc_cfg->final_offset = pps_temp & ~DSC_INITIAL_OFFSET_MASK;
+
+	/* Readout PPS_9 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_9, &pps_temp);
+
+	vdsc_cfg->rc_model_size = pps_temp & DSC_RC_MODEL_SIZE_MASK;
+
+	/* Readout PPS_10 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_10, &pps_temp);
+
+	vdsc_cfg->rc_quant_incr_limit0 = pps_temp & DSC_RC_QUANT_INC_LIMIT0_MASK;
+	vdsc_cfg->rc_quant_incr_limit1 =
+		DSC_RC_QUANT_INC_LIMIT1(pps_temp & DSC_RC_QUANT_INC_LIMIT1_MASK, false);
+
+	/* Readout PPS_16 register */
+	pps_temp = 0;
+	intel_dsc_read_pps_reg(crtc_state, PPS_16, &pps_temp);
+
+	vdsc_cfg->slice_chunk_size = pps_temp & DSC_SLICE_CHUNK_SIZE_MASK;
+
+	if (DISPLAY_VER(i915) >= 14) {
+		/* Readout PPS_17 register */
+		pps_temp = 0;
+		intel_dsc_read_pps_reg(crtc_state, PPS_17, &pps_temp);
+
+		vdsc_cfg->second_line_bpg_offset =
+			DSC_SL_BPG_OFFSET(pps_temp & DSC_SL_BPG_OFFSET_MASK, false);
+
+		/* Readout PPS_18 register */
+		pps_temp = 0;
+		intel_dsc_read_pps_reg(crtc_state, PPS_18, &pps_temp);
+
+		vdsc_cfg->nsl_bpg_offset =
+			DSC_NSL_BPG_OFFSET(pps_temp & DSC_NSL_BPG_OFFSET_MASK, false);
+		vdsc_cfg->second_line_offset_adj =
+			pps_temp & ~DSC_NSL_BPG_OFFSET_MASK;
+	}
 }
 
 void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
index b71f00b5c761..605d37c2978b 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
@@ -57,7 +57,8 @@
 #define MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _MTL_DSC1_PICTURE_PARAMETER_SET_17_PB, \
 							   _MTL_DSC1_PICTURE_PARAMETER_SET_17_PC)
-#define DSC_SL_BPG_OFFSET(offset)		((offset) << 27)
+#define DSC_SL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 27 : (offset) >> 27)
+#define DSC_SL_BPG_OFFSET_MASK			REG_GENMASK(31, 27)
 
 #define _MTL_DSC0_PICTURE_PARAMETER_SET_18_PB	0x782B8
 #define _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB	0x783B8
@@ -69,8 +70,9 @@
 #define MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB, \
 							   _MTL_DSC1_PICTURE_PARAMETER_SET_18_PC)
-#define DSC_NSL_BPG_OFFSET(offset)		((offset) << 16)
+#define DSC_NSL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 16 : (offset) >> 16)
 #define DSC_SL_OFFSET_ADJ(offset)		((offset) << 0)
+#define DSC_NSL_BPG_OFFSET_MASK			REG_GENMASK(31, 16)
 
 /* Icelake Display Stream Compression Registers */
 #define DSCA_PICTURE_PARAMETER_SET_0		_MMIO(0x6B200)
@@ -96,6 +98,9 @@
 #define  DSC_BPC_SHIFT			8
 #define  DSC_VER_MIN_SHIFT		4
 #define  DSC_VER_MAJ			(0x1 << 0)
+#define  DSC_LINE_BUF_DEPTH_MASK	REG_GENMASK(15, 12)
+#define  DSC_BPC_MASK			REG_GENMASK(11, 8)
+
 
 #define DSCA_PICTURE_PARAMETER_SET_1		_MMIO(0x6B204)
 #define DSCC_PICTURE_PARAMETER_SET_1		_MMIO(0x6BA04)
@@ -123,8 +128,9 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 					    _ICL_DSC1_PICTURE_PARAMETER_SET_2_PB, \
 					    _ICL_DSC1_PICTURE_PARAMETER_SET_2_PC)
-#define  DSC_PIC_WIDTH(pic_width)	((pic_width) << 16)
-#define  DSC_PIC_HEIGHT(pic_height)	((pic_height) << 0)
+#define  DSC_PIC_WIDTH(pic_width, shift)	(shift ? (pic_width) << 16 : (pic_width >> 16))
+#define  DSC_PIC_HEIGHT(pic_height)		((pic_height) << 0)
+#define  DSC_PIC_WIDTH_MASK			REG_GENMASK(31, 16)
 
 #define DSCA_PICTURE_PARAMETER_SET_3		_MMIO(0x6B20C)
 #define DSCC_PICTURE_PARAMETER_SET_3		_MMIO(0x6BA0C)
@@ -138,8 +144,9 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_3_PB, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_3_PC)
-#define  DSC_SLICE_WIDTH(slice_width)   ((slice_width) << 16)
+#define  DSC_SLICE_WIDTH(slice_width, shift)   (shift ? (slice_width) << 16 : (slice_width >> 16))
 #define  DSC_SLICE_HEIGHT(slice_height) ((slice_height) << 0)
+#define  DSC_SLICE_WIDTH_MASK			REG_GENMASK(31, 16)
 
 #define DSCA_PICTURE_PARAMETER_SET_4		_MMIO(0x6B210)
 #define DSCC_PICTURE_PARAMETER_SET_4		_MMIO(0x6BA10)
@@ -153,8 +160,11 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_4_PB, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_4_PC)
-#define  DSC_INITIAL_DEC_DELAY(dec_delay)       ((dec_delay) << 16)
-#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)     ((xmit_delay) << 0)
+#define  DSC_INITIAL_DEC_DELAY(dec_delay, shift)	(shift ? (dec_delay) << 16 : \
+							 (dec_delay) >> 16)
+#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)		((xmit_delay) << 0)
+#define  DSC_INITIAL_DEC_DELAY_MASK			REG_GENMASK(31, 16)
+#define  DSC_INITIAL_XMIT_DELAY_MASK			REG_GENMASK(9, 0)
 
 #define DSCA_PICTURE_PARAMETER_SET_5		_MMIO(0x6B214)
 #define DSCC_PICTURE_PARAMETER_SET_5		_MMIO(0x6BA14)
@@ -168,8 +178,10 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_5_PB, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_5_PC)
-#define  DSC_SCALE_DEC_INT(scale_dec)	((scale_dec) << 16)
+#define  DSC_SCALE_DEC_INT(scale_dec, shift)	(shift ? (scale_dec) << 16 : (scale_dec) >> 16)
 #define  DSC_SCALE_INC_INT(scale_inc)		((scale_inc) << 0)
+#define  DSC_SCALE_DEC_INT_MASK			REG_GENMASK(27, 16)
+#define  DSC_SCALE_INC_INT_MASK			REG_GENMASK(15, 0)
 
 #define DSCA_PICTURE_PARAMETER_SET_6		_MMIO(0x6B218)
 #define DSCC_PICTURE_PARAMETER_SET_6		_MMIO(0x6BA18)
@@ -183,10 +195,14 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_6_PB, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_6_PC)
-#define  DSC_FLATNESS_MAX_QP(max_qp)		((max_qp) << 24)
-#define  DSC_FLATNESS_MIN_QP(min_qp)		((min_qp) << 16)
-#define  DSC_FIRST_LINE_BPG_OFFSET(offset)	((offset) << 8)
-#define  DSC_INITIAL_SCALE_VALUE(value)		((value) << 0)
+#define  DSC_FLATNESS_MAX_QP(max_qp, shift)		(shift ? (max_qp) << 24 : (max_qp) >> 24)
+#define  DSC_FLATNESS_MIN_QP(min_qp, shift)		(shift ? (min_qp) << 16 : (min_qp) >> 16)
+#define  DSC_FIRST_LINE_BPG_OFFSET(offset, shift)	(shift ? (offset) << 8 : (offset) >> 8)
+#define  DSC_INITIAL_SCALE_VALUE(value)			((value) << 0)
+#define  DSC_FLATNESS_MAX_QP_MASK			REG_GENMASK(28, 24)
+#define  DSC_FLATNESS_MIN_QP_MASK			REG_GENMASK(20, 16)
+#define  DSC_FIRST_LINE_BPG_OFFSET_MASK			REG_GENMASK(12, 8)
+#define  DSC_INITIAL_SCALE_VALUE_MASK			REG_GENMASK(5, 0)
 
 #define DSCA_PICTURE_PARAMETER_SET_7		_MMIO(0x6B21C)
 #define DSCC_PICTURE_PARAMETER_SET_7		_MMIO(0x6BA1C)
@@ -200,8 +216,9 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							    _ICL_DSC1_PICTURE_PARAMETER_SET_7_PB, \
 							    _ICL_DSC1_PICTURE_PARAMETER_SET_7_PC)
-#define  DSC_NFL_BPG_OFFSET(bpg_offset)		((bpg_offset) << 16)
+#define  DSC_NFL_BPG_OFFSET(bpg_offset, shift)	(shift ? (bpg_offset) << 16 : (bpg_offset) >> 16)
 #define  DSC_SLICE_BPG_OFFSET(bpg_offset)	((bpg_offset) << 0)
+#define  DSC_NFL_BPG_OFFSET_MASK		REG_GENMASK(31, 16)
 
 #define DSCA_PICTURE_PARAMETER_SET_8		_MMIO(0x6B220)
 #define DSCC_PICTURE_PARAMETER_SET_8		_MMIO(0x6BA20)
@@ -215,8 +232,10 @@
 #define ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_8_PB, \
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_8_PC)
-#define  DSC_INITIAL_OFFSET(initial_offset)		((initial_offset) << 16)
+#define  DSC_INITIAL_OFFSET(initial_offset, shift)	(shift ? (initial_offset) << 16 : \
+							 (initial_offset) >> 16)
 #define  DSC_FINAL_OFFSET(final_offset)			((final_offset) << 0)
+#define  DSC_INITIAL_OFFSET_MASK			REG_GENMASK(31, 16)
 
 #define DSCA_PICTURE_PARAMETER_SET_9		_MMIO(0x6B224)
 #define DSCC_PICTURE_PARAMETER_SET_9		_MMIO(0x6BA24)
@@ -232,6 +251,7 @@
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_9_PC)
 #define  DSC_RC_EDGE_FACTOR(rc_edge_fact)	((rc_edge_fact) << 16)
 #define  DSC_RC_MODEL_SIZE(rc_model_size)	((rc_model_size) << 0)
+#define  DSC_RC_MODEL_SIZE_MASK			REG_GENMASK(15, 0)
 
 #define DSCA_PICTURE_PARAMETER_SET_10		_MMIO(0x6B228)
 #define DSCC_PICTURE_PARAMETER_SET_10		_MMIO(0x6BA28)
@@ -247,8 +267,10 @@
 							   _ICL_DSC1_PICTURE_PARAMETER_SET_10_PC)
 #define  DSC_RC_TARGET_OFF_LOW(rc_tgt_off_low)		((rc_tgt_off_low) << 20)
 #define  DSC_RC_TARGET_OFF_HIGH(rc_tgt_off_high)	((rc_tgt_off_high) << 16)
-#define  DSC_RC_QUANT_INC_LIMIT1(lim)			((lim) << 8)
+#define  DSC_RC_QUANT_INC_LIMIT1(lim, shift)		(shift ? (lim) << 8 : (lim) >> 8)
 #define  DSC_RC_QUANT_INC_LIMIT0(lim)			((lim) << 0)
+#define  DSC_RC_QUANT_INC_LIMIT1_MASK			REG_GENMASK(12, 8)
+#define  DSC_RC_QUANT_INC_LIMIT0_MASK			REG_GENMASK(4, 0)
 
 #define DSCA_PICTURE_PARAMETER_SET_11		_MMIO(0x6B22C)
 #define DSCC_PICTURE_PARAMETER_SET_11		_MMIO(0x6BA2C)
@@ -330,6 +352,7 @@
 #define  DSC_SLICE_ROW_PER_FRAME(slice_row_per_frame)	((slice_row_per_frame) << 20)
 #define  DSC_SLICE_PER_LINE(slice_per_line)		((slice_per_line) << 16)
 #define  DSC_SLICE_CHUNK_SIZE(slice_chunk_size)		((slice_chunk_size) << 0)
+#define  DSC_SLICE_CHUNK_SIZE_MASK		REG_GENMASK(15, 0)
 
 /* Icelake Rate Control Buffer Threshold Registers */
 #define DSCA_RC_BUF_THRESH_0			_MMIO(0x6B230)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
@ 2023-07-10 11:11   ` Jani Nikula
  2023-07-11  9:42     ` Kandpal, Suraj
  0 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 11:11 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> We have setup both the read and write functions so we can
> move ahead and fill in all the readout state from PPS register
> into the crtc_state so we can send it for comparision.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c     | 152 +++++++++++++++---
>  .../gpu/drm/i915/display/intel_vdsc_regs.h    |  53 ++++--
>  2 files changed, 172 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 274d82360c1a..a4f5b4aceb33 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -596,51 +596,51 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  	/* Populate PICTURE_PARAMETER_SET_2 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) |
> -		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances);
> +		DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances, true);

So absolutely *no* register macros with boolean flags to shift or
not. Just no.

This is totally unreadable.

Define the macros in terms of the REG_FIELD_MASK and REG_FIELD_PREP, and
use REG_FIELD_GET to read the values.

#define   DSC_PIC_WIDTH(pic_width)    REG_FIELD_PREP(DSC_PIC_WIDTH_MASK, pic_width)

	pps_val |= DSC_PIC_WIDTH(vdsc_cfg->pic_width / num_vdsc_instances)

This should probably be a first cleanup patch before the other changes.

>  	drm_dbg_kms(&dev_priv->drm, "PPS2 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_2, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_3 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) |
> -		DSC_SLICE_WIDTH(vdsc_cfg->slice_width);
> +		DSC_SLICE_WIDTH(vdsc_cfg->slice_width, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS3 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_3, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_4 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay) |
> -		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay);
> +		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS4 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_4, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_5 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SCALE_INC_INT(vdsc_cfg->scale_increment_interval) |
> -		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval);
> +		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS5 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_5, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_6 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_INITIAL_SCALE_VALUE(vdsc_cfg->initial_scale_value) |
> -		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg->first_line_bpg_offset) |
> -		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) |
> -		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp);
> +		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg->first_line_bpg_offset, true) |
> +		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp, true) |
> +		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS6 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_6, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_7 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) |
> -		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset);
> +		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS7 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_7, pps_val);
>  
>  	/* Populate PICTURE_PARAMETER_SET_8 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) |
> -		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset);
> +		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset, true);
>  	drm_dbg_kms(&dev_priv->drm, "PPS8 = 0x%08x\n", pps_val);
>  	intel_dsc_write_pps_reg(crtc_state, PPS_8, pps_val);
>  
> @@ -654,7 +654,7 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  	/* Populate PICTURE_PARAMETER_SET_10 registers */
>  	pps_val = 0;
>  	pps_val |= DSC_RC_QUANT_INC_LIMIT0(vdsc_cfg->rc_quant_incr_limit0) |
> -		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg->rc_quant_incr_limit1) |
> +		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg->rc_quant_incr_limit1, true) |
>  		DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) |
>  		DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST);
>  	drm_dbg_kms(&dev_priv->drm, "PPS10 = 0x%08x\n", pps_val);
> @@ -673,13 +673,14 @@ static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
>  	if (DISPLAY_VER(dev_priv) >= 14) {
>  		/* Populate PICTURE_PARAMETER_SET_17 registers */
>  		pps_val = 0;
> -		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset);
> +		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg->second_line_bpg_offset,
> +					     true);
>  		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n", pps_val);
>  		intel_dsc_write_pps_reg(crtc_state, PPS_17, pps_val);
>  
>  		/* Populate PICTURE_PARAMETER_SET_18 registers */
>  		pps_val = 0;
> -		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset) |
> +		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg->nsl_bpg_offset, true) |
>  			   DSC_SL_OFFSET_ADJ(vdsc_cfg->second_line_offset_adj);
>  		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n", pps_val);
>  		intel_dsc_write_pps_reg(crtc_state, PPS_18, pps_val);
> @@ -1206,18 +1207,133 @@ static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
>  static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> -	u32 pps_temp1, pps_temp2;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	u32 pps_temp;
> +
> +	/* Readout PPS_0 register */

The comments may be helpful as headings, but a simple /* PPS 0 */ etc
will do.

> +	pps_temp = 0;

Please define the read function so that you don't have to initialize
this to zero every time.

> +	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp);
>  
> -	/* Readout PPS_0 and PPS_1 registers */
> -	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
> -	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
> +	vdsc_cfg->bits_per_component = (pps_temp & DSC_BPC_MASK) >> DSC_BPC_SHIFT;

Use REG_FIELD_GET(), throw out the shifts.

> +	vdsc_cfg->line_buf_depth =
> +		(pps_temp & DSC_LINE_BUF_DEPTH_MASK) >> DSC_LINE_BUF_DEPTH_SHIFT;
> +	vdsc_cfg->block_pred_enable = pps_temp & DSC_BLOCK_PREDICTION ? true : false;

For assigning booleans just pps_temp & DSC_BLOCK_PREDICTION is
enough. The ternary operator is superfluous. Ditto below.

> +	vdsc_cfg->convert_rgb = pps_temp & DSC_COLOR_SPACE_CONVERSION ? true : false;
> +	vdsc_cfg->simple_422 = pps_temp & DSC_422_ENABLE ? true : false;
> +	vdsc_cfg->native_422 = pps_temp & DSC_NATIVE_422_ENABLE ? true : false;
> +	vdsc_cfg->native_420 = pps_temp & DSC_NATIVE_420_ENABLE ? true : false;
> +	vdsc_cfg->vbr_enable = pps_temp & DSC_VBR_ENABLE ? true : false;
>  
> -	vdsc_cfg->bits_per_pixel = pps_temp2;
> +	/* Readout PPS_1 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp);
>  
> -	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
> +	vdsc_cfg->bits_per_pixel = pps_temp;
> +
> +	if (vdsc_cfg->native_420)
>  		vdsc_cfg->bits_per_pixel >>= 1;
>  
>  	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> +
> +	/* Readout PPS_2 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_2, &pps_temp);
> +
> +	vdsc_cfg->pic_width =
> +		DSC_PIC_WIDTH(pps_temp & DSC_PIC_WIDTH_MASK, false);

This is the way to go:

	vdsc_cfg->pic_width = REG_FIELD_GET(DSC_PIC_WIDTH_MASK, pps_temp);

> +	vdsc_cfg->pic_height = pps_temp & ~DSC_PIC_WIDTH_MASK;

Nope, just use REG_FIELD_GET() for all of it.

> +
> +	/* Readout PPS_3 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_3, &pps_temp);
> +
> +	vdsc_cfg->slice_width =
> +		DSC_SLICE_WIDTH(pps_temp & DSC_SLICE_WIDTH_MASK, false);
> +	vdsc_cfg->slice_height = pps_temp & ~DSC_SLICE_WIDTH_MASK;
> +
> +	/* Readout PPS_4 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_4, &pps_temp);
> +
> +	vdsc_cfg->initial_dec_delay =
> +		DSC_INITIAL_DEC_DELAY(pps_temp & DSC_INITIAL_DEC_DELAY_MASK, false);
> +	vdsc_cfg->initial_xmit_delay = pps_temp & DSC_INITIAL_XMIT_DELAY_MASK;
> +
> +	/* Readout PPS_5 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_5, &pps_temp);
> +
> +	vdsc_cfg->scale_decrement_interval =
> +		DSC_SCALE_DEC_INT(pps_temp & DSC_SCALE_DEC_INT_MASK, false);
> +	vdsc_cfg->scale_increment_interval = pps_temp & DSC_SCALE_INC_INT_MASK;
> +
> +	/* Readout PPS_6 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_6, &pps_temp);
> +
> +	vdsc_cfg->initial_scale_value = pps_temp & DSC_INITIAL_SCALE_VALUE_MASK;
> +	vdsc_cfg->first_line_bpg_offset =
> +		DSC_FIRST_LINE_BPG_OFFSET(pps_temp &
> +					  DSC_FIRST_LINE_BPG_OFFSET_MASK, false);
> +	vdsc_cfg->flatness_min_qp =
> +		DSC_FLATNESS_MIN_QP(pps_temp & DSC_FLATNESS_MIN_QP_MASK, false);
> +	vdsc_cfg->flatness_max_qp =
> +		DSC_FLATNESS_MAX_QP(pps_temp & DSC_FLATNESS_MAX_QP_MASK, false);
> +
> +	/* Readout PPS_7 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_7, &pps_temp);
> +
> +	vdsc_cfg->nfl_bpg_offset =
> +		DSC_NFL_BPG_OFFSET(pps_temp & DSC_NFL_BPG_OFFSET_MASK, false);
> +	vdsc_cfg->slice_bpg_offset = pps_temp & ~DSC_NFL_BPG_OFFSET_MASK;
> +
> +	/* Readout PPS_8 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_8, &pps_temp);
> +
> +	vdsc_cfg->initial_offset =
> +		DSC_INITIAL_OFFSET(pps_temp & DSC_INITIAL_OFFSET_MASK, false);
> +	vdsc_cfg->final_offset = pps_temp & ~DSC_INITIAL_OFFSET_MASK;
> +
> +	/* Readout PPS_9 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_9, &pps_temp);
> +
> +	vdsc_cfg->rc_model_size = pps_temp & DSC_RC_MODEL_SIZE_MASK;
> +
> +	/* Readout PPS_10 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_10, &pps_temp);
> +
> +	vdsc_cfg->rc_quant_incr_limit0 = pps_temp & DSC_RC_QUANT_INC_LIMIT0_MASK;
> +	vdsc_cfg->rc_quant_incr_limit1 =
> +		DSC_RC_QUANT_INC_LIMIT1(pps_temp & DSC_RC_QUANT_INC_LIMIT1_MASK, false);
> +
> +	/* Readout PPS_16 register */
> +	pps_temp = 0;
> +	intel_dsc_read_pps_reg(crtc_state, PPS_16, &pps_temp);
> +
> +	vdsc_cfg->slice_chunk_size = pps_temp & DSC_SLICE_CHUNK_SIZE_MASK;
> +
> +	if (DISPLAY_VER(i915) >= 14) {
> +		/* Readout PPS_17 register */
> +		pps_temp = 0;
> +		intel_dsc_read_pps_reg(crtc_state, PPS_17, &pps_temp);
> +
> +		vdsc_cfg->second_line_bpg_offset =
> +			DSC_SL_BPG_OFFSET(pps_temp & DSC_SL_BPG_OFFSET_MASK, false);
> +
> +		/* Readout PPS_18 register */
> +		pps_temp = 0;
> +		intel_dsc_read_pps_reg(crtc_state, PPS_18, &pps_temp);
> +
> +		vdsc_cfg->nsl_bpg_offset =
> +			DSC_NSL_BPG_OFFSET(pps_temp & DSC_NSL_BPG_OFFSET_MASK, false);
> +		vdsc_cfg->second_line_offset_adj =
> +			pps_temp & ~DSC_NSL_BPG_OFFSET_MASK;
> +	}
>  }
>  
>  void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> index b71f00b5c761..605d37c2978b 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> @@ -57,7 +57,8 @@
>  #define MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _MTL_DSC1_PICTURE_PARAMETER_SET_17_PB, \
>  							   _MTL_DSC1_PICTURE_PARAMETER_SET_17_PC)
> -#define DSC_SL_BPG_OFFSET(offset)		((offset) << 27)
> +#define DSC_SL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 27 : (offset) >> 27)
> +#define DSC_SL_BPG_OFFSET_MASK			REG_GENMASK(31, 27)
>  
>  #define _MTL_DSC0_PICTURE_PARAMETER_SET_18_PB	0x782B8
>  #define _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB	0x783B8
> @@ -69,8 +70,9 @@
>  #define MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB, \
>  							   _MTL_DSC1_PICTURE_PARAMETER_SET_18_PC)
> -#define DSC_NSL_BPG_OFFSET(offset)		((offset) << 16)
> +#define DSC_NSL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 16 : (offset) >> 16)
>  #define DSC_SL_OFFSET_ADJ(offset)		((offset) << 0)
> +#define DSC_NSL_BPG_OFFSET_MASK			REG_GENMASK(31, 16)
>  
>  /* Icelake Display Stream Compression Registers */
>  #define DSCA_PICTURE_PARAMETER_SET_0		_MMIO(0x6B200)
> @@ -96,6 +98,9 @@
>  #define  DSC_BPC_SHIFT			8
>  #define  DSC_VER_MIN_SHIFT		4
>  #define  DSC_VER_MAJ			(0x1 << 0)
> +#define  DSC_LINE_BUF_DEPTH_MASK	REG_GENMASK(15, 12)
> +#define  DSC_BPC_MASK			REG_GENMASK(11, 8)
> +
>  
>  #define DSCA_PICTURE_PARAMETER_SET_1		_MMIO(0x6B204)
>  #define DSCC_PICTURE_PARAMETER_SET_1		_MMIO(0x6BA04)
> @@ -123,8 +128,9 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  					    _ICL_DSC1_PICTURE_PARAMETER_SET_2_PB, \
>  					    _ICL_DSC1_PICTURE_PARAMETER_SET_2_PC)
> -#define  DSC_PIC_WIDTH(pic_width)	((pic_width) << 16)
> -#define  DSC_PIC_HEIGHT(pic_height)	((pic_height) << 0)
> +#define  DSC_PIC_WIDTH(pic_width, shift)	(shift ? (pic_width) << 16 : (pic_width >> 16))
> +#define  DSC_PIC_HEIGHT(pic_height)		((pic_height) << 0)
> +#define  DSC_PIC_WIDTH_MASK			REG_GENMASK(31, 16)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_3		_MMIO(0x6B20C)
>  #define DSCC_PICTURE_PARAMETER_SET_3		_MMIO(0x6BA0C)
> @@ -138,8 +144,9 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_3_PB, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_3_PC)
> -#define  DSC_SLICE_WIDTH(slice_width)   ((slice_width) << 16)
> +#define  DSC_SLICE_WIDTH(slice_width, shift)   (shift ? (slice_width) << 16 : (slice_width >> 16))
>  #define  DSC_SLICE_HEIGHT(slice_height) ((slice_height) << 0)
> +#define  DSC_SLICE_WIDTH_MASK			REG_GENMASK(31, 16)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_4		_MMIO(0x6B210)
>  #define DSCC_PICTURE_PARAMETER_SET_4		_MMIO(0x6BA10)
> @@ -153,8 +160,11 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_4_PB, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_4_PC)
> -#define  DSC_INITIAL_DEC_DELAY(dec_delay)       ((dec_delay) << 16)
> -#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)     ((xmit_delay) << 0)
> +#define  DSC_INITIAL_DEC_DELAY(dec_delay, shift)	(shift ? (dec_delay) << 16 : \
> +							 (dec_delay) >> 16)
> +#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)		((xmit_delay) << 0)
> +#define  DSC_INITIAL_DEC_DELAY_MASK			REG_GENMASK(31, 16)
> +#define  DSC_INITIAL_XMIT_DELAY_MASK			REG_GENMASK(9, 0)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_5		_MMIO(0x6B214)
>  #define DSCC_PICTURE_PARAMETER_SET_5		_MMIO(0x6BA14)
> @@ -168,8 +178,10 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_5_PB, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_5_PC)
> -#define  DSC_SCALE_DEC_INT(scale_dec)	((scale_dec) << 16)
> +#define  DSC_SCALE_DEC_INT(scale_dec, shift)	(shift ? (scale_dec) << 16 : (scale_dec) >> 16)
>  #define  DSC_SCALE_INC_INT(scale_inc)		((scale_inc) << 0)
> +#define  DSC_SCALE_DEC_INT_MASK			REG_GENMASK(27, 16)
> +#define  DSC_SCALE_INC_INT_MASK			REG_GENMASK(15, 0)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_6		_MMIO(0x6B218)
>  #define DSCC_PICTURE_PARAMETER_SET_6		_MMIO(0x6BA18)
> @@ -183,10 +195,14 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_6_PB, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_6_PC)
> -#define  DSC_FLATNESS_MAX_QP(max_qp)		((max_qp) << 24)
> -#define  DSC_FLATNESS_MIN_QP(min_qp)		((min_qp) << 16)
> -#define  DSC_FIRST_LINE_BPG_OFFSET(offset)	((offset) << 8)
> -#define  DSC_INITIAL_SCALE_VALUE(value)		((value) << 0)
> +#define  DSC_FLATNESS_MAX_QP(max_qp, shift)		(shift ? (max_qp) << 24 : (max_qp) >> 24)
> +#define  DSC_FLATNESS_MIN_QP(min_qp, shift)		(shift ? (min_qp) << 16 : (min_qp) >> 16)
> +#define  DSC_FIRST_LINE_BPG_OFFSET(offset, shift)	(shift ? (offset) << 8 : (offset) >> 8)
> +#define  DSC_INITIAL_SCALE_VALUE(value)			((value) << 0)
> +#define  DSC_FLATNESS_MAX_QP_MASK			REG_GENMASK(28, 24)
> +#define  DSC_FLATNESS_MIN_QP_MASK			REG_GENMASK(20, 16)
> +#define  DSC_FIRST_LINE_BPG_OFFSET_MASK			REG_GENMASK(12, 8)
> +#define  DSC_INITIAL_SCALE_VALUE_MASK			REG_GENMASK(5, 0)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_7		_MMIO(0x6B21C)
>  #define DSCC_PICTURE_PARAMETER_SET_7		_MMIO(0x6BA1C)
> @@ -200,8 +216,9 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							    _ICL_DSC1_PICTURE_PARAMETER_SET_7_PB, \
>  							    _ICL_DSC1_PICTURE_PARAMETER_SET_7_PC)
> -#define  DSC_NFL_BPG_OFFSET(bpg_offset)		((bpg_offset) << 16)
> +#define  DSC_NFL_BPG_OFFSET(bpg_offset, shift)	(shift ? (bpg_offset) << 16 : (bpg_offset) >> 16)
>  #define  DSC_SLICE_BPG_OFFSET(bpg_offset)	((bpg_offset) << 0)
> +#define  DSC_NFL_BPG_OFFSET_MASK		REG_GENMASK(31, 16)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_8		_MMIO(0x6B220)
>  #define DSCC_PICTURE_PARAMETER_SET_8		_MMIO(0x6BA20)
> @@ -215,8 +232,10 @@
>  #define ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe)	_MMIO_PIPE((pipe) - PIPE_B, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_8_PB, \
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_8_PC)
> -#define  DSC_INITIAL_OFFSET(initial_offset)		((initial_offset) << 16)
> +#define  DSC_INITIAL_OFFSET(initial_offset, shift)	(shift ? (initial_offset) << 16 : \
> +							 (initial_offset) >> 16)
>  #define  DSC_FINAL_OFFSET(final_offset)			((final_offset) << 0)
> +#define  DSC_INITIAL_OFFSET_MASK			REG_GENMASK(31, 16)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_9		_MMIO(0x6B224)
>  #define DSCC_PICTURE_PARAMETER_SET_9		_MMIO(0x6BA24)
> @@ -232,6 +251,7 @@
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_9_PC)
>  #define  DSC_RC_EDGE_FACTOR(rc_edge_fact)	((rc_edge_fact) << 16)
>  #define  DSC_RC_MODEL_SIZE(rc_model_size)	((rc_model_size) << 0)
> +#define  DSC_RC_MODEL_SIZE_MASK			REG_GENMASK(15, 0)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_10		_MMIO(0x6B228)
>  #define DSCC_PICTURE_PARAMETER_SET_10		_MMIO(0x6BA28)
> @@ -247,8 +267,10 @@
>  							   _ICL_DSC1_PICTURE_PARAMETER_SET_10_PC)
>  #define  DSC_RC_TARGET_OFF_LOW(rc_tgt_off_low)		((rc_tgt_off_low) << 20)
>  #define  DSC_RC_TARGET_OFF_HIGH(rc_tgt_off_high)	((rc_tgt_off_high) << 16)
> -#define  DSC_RC_QUANT_INC_LIMIT1(lim)			((lim) << 8)
> +#define  DSC_RC_QUANT_INC_LIMIT1(lim, shift)		(shift ? (lim) << 8 : (lim) >> 8)
>  #define  DSC_RC_QUANT_INC_LIMIT0(lim)			((lim) << 0)
> +#define  DSC_RC_QUANT_INC_LIMIT1_MASK			REG_GENMASK(12, 8)
> +#define  DSC_RC_QUANT_INC_LIMIT0_MASK			REG_GENMASK(4, 0)
>  
>  #define DSCA_PICTURE_PARAMETER_SET_11		_MMIO(0x6B22C)
>  #define DSCC_PICTURE_PARAMETER_SET_11		_MMIO(0x6BA2C)
> @@ -330,6 +352,7 @@
>  #define  DSC_SLICE_ROW_PER_FRAME(slice_row_per_frame)	((slice_row_per_frame) << 20)
>  #define  DSC_SLICE_PER_LINE(slice_per_line)		((slice_per_line) << 16)
>  #define  DSC_SLICE_CHUNK_SIZE(slice_chunk_size)		((slice_chunk_size) << 0)
> +#define  DSC_SLICE_CHUNK_SIZE_MASK		REG_GENMASK(15, 0)
>  
>  /* Icelake Rate Control Buffer Threshold Registers */
>  #define DSCA_RC_BUF_THRESH_0			_MMIO(0x6B230)

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function
  2023-07-10 11:11   ` Jani Nikula
@ 2023-07-11  9:42     ` Kandpal, Suraj
  0 siblings, 0 replies; 17+ messages in thread
From: Kandpal, Suraj @ 2023-07-11  9:42 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx@lists.freedesktop.org

> On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> > We have setup both the read and write functions so we can move ahead
> > and fill in all the readout state from PPS register into the
> > crtc_state so we can send it for comparision.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_vdsc.c     | 152 +++++++++++++++---
> >  .../gpu/drm/i915/display/intel_vdsc_regs.h    |  53 ++++--
> >  2 files changed, 172 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index 274d82360c1a..a4f5b4aceb33 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -596,51 +596,51 @@ static void intel_dsc_pps_configure(const struct
> intel_crtc_state *crtc_state)
> >  	/* Populate PICTURE_PARAMETER_SET_2 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_PIC_HEIGHT(vdsc_cfg->pic_height) |
> > -		DSC_PIC_WIDTH(vdsc_cfg->pic_width /
> num_vdsc_instances);
> > +		DSC_PIC_WIDTH(vdsc_cfg->pic_width /
> num_vdsc_instances, true);
> 
> So absolutely *no* register macros with boolean flags to shift or not. Just no.
> 
> This is totally unreadable.
> 
> Define the macros in terms of the REG_FIELD_MASK and REG_FIELD_PREP,
> and use REG_FIELD_GET to read the values.
> 
> #define   DSC_PIC_WIDTH(pic_width)
> REG_FIELD_PREP(DSC_PIC_WIDTH_MASK, pic_width)
> 
> 	pps_val |= DSC_PIC_WIDTH(vdsc_cfg->pic_width /
> num_vdsc_instances)
> 
> This should probably be a first cleanup patch before the other changes.
> 

Hmm I see thanks for pointing me out to this solution will add a new refactor patch
At the start in the next revision

> >  	drm_dbg_kms(&dev_priv->drm, "PPS2 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_2, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_3 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_SLICE_HEIGHT(vdsc_cfg->slice_height) |
> > -		DSC_SLICE_WIDTH(vdsc_cfg->slice_width);
> > +		DSC_SLICE_WIDTH(vdsc_cfg->slice_width, true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS3 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_3, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_4 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_INITIAL_XMIT_DELAY(vdsc_cfg->initial_xmit_delay)
> |
> > -		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay);
> > +		DSC_INITIAL_DEC_DELAY(vdsc_cfg->initial_dec_delay, true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS4 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_4, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_5 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_SCALE_INC_INT(vdsc_cfg-
> >scale_increment_interval) |
> > -		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval);
> > +		DSC_SCALE_DEC_INT(vdsc_cfg->scale_decrement_interval,
> true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS5 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_5, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_6 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_INITIAL_SCALE_VALUE(vdsc_cfg-
> >initial_scale_value) |
> > -		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg-
> >first_line_bpg_offset) |
> > -		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp) |
> > -		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp);
> > +		DSC_FIRST_LINE_BPG_OFFSET(vdsc_cfg-
> >first_line_bpg_offset, true) |
> > +		DSC_FLATNESS_MIN_QP(vdsc_cfg->flatness_min_qp, true)
> |
> > +		DSC_FLATNESS_MAX_QP(vdsc_cfg->flatness_max_qp,
> true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS6 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_6, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_7 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_SLICE_BPG_OFFSET(vdsc_cfg->slice_bpg_offset) |
> > -		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset);
> > +		DSC_NFL_BPG_OFFSET(vdsc_cfg->nfl_bpg_offset, true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS7 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_7, pps_val);
> >
> >  	/* Populate PICTURE_PARAMETER_SET_8 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_FINAL_OFFSET(vdsc_cfg->final_offset) |
> > -		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset);
> > +		DSC_INITIAL_OFFSET(vdsc_cfg->initial_offset, true);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS8 = 0x%08x\n", pps_val);
> >  	intel_dsc_write_pps_reg(crtc_state, PPS_8, pps_val);
> >
> > @@ -654,7 +654,7 @@ static void intel_dsc_pps_configure(const struct
> intel_crtc_state *crtc_state)
> >  	/* Populate PICTURE_PARAMETER_SET_10 registers */
> >  	pps_val = 0;
> >  	pps_val |= DSC_RC_QUANT_INC_LIMIT0(vdsc_cfg-
> >rc_quant_incr_limit0) |
> > -		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg-
> >rc_quant_incr_limit1) |
> > +		DSC_RC_QUANT_INC_LIMIT1(vdsc_cfg-
> >rc_quant_incr_limit1, true) |
> >
> 	DSC_RC_TARGET_OFF_HIGH(DSC_RC_TGT_OFFSET_HI_CONST) |
> >
> 	DSC_RC_TARGET_OFF_LOW(DSC_RC_TGT_OFFSET_LO_CONST);
> >  	drm_dbg_kms(&dev_priv->drm, "PPS10 = 0x%08x\n", pps_val); @@
> -673,13
> > +673,14 @@ static void intel_dsc_pps_configure(const struct
> intel_crtc_state *crtc_state)
> >  	if (DISPLAY_VER(dev_priv) >= 14) {
> >  		/* Populate PICTURE_PARAMETER_SET_17 registers */
> >  		pps_val = 0;
> > -		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg-
> >second_line_bpg_offset);
> > +		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg-
> >second_line_bpg_offset,
> > +					     true);
> >  		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n",
> pps_val);
> >  		intel_dsc_write_pps_reg(crtc_state, PPS_17, pps_val);
> >
> >  		/* Populate PICTURE_PARAMETER_SET_18 registers */
> >  		pps_val = 0;
> > -		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg-
> >nsl_bpg_offset) |
> > +		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg-
> >nsl_bpg_offset, true) |
> >  			   DSC_SL_OFFSET_ADJ(vdsc_cfg-
> >second_line_offset_adj);
> >  		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n",
> pps_val);
> >  		intel_dsc_write_pps_reg(crtc_state, PPS_18, pps_val); @@ -
> 1206,18
> > +1207,133 @@ static void intel_dsc_read_pps_reg(struct
> > intel_crtc_state *crtc_state,  static void
> > intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)  {
> >  	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> > -	u32 pps_temp1, pps_temp2;
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	u32 pps_temp;
> > +
> > +	/* Readout PPS_0 register */
> 
> The comments may be helpful as headings, but a simple /* PPS 0 */ etc will
> do.
> 

Sure 
> > +	pps_temp = 0;
> 
> Please define the read function so that you don't have to initialize this to zero
> every time.
> 

Right seem to have missed that

> > +	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp);
> >
> > -	/* Readout PPS_0 and PPS_1 registers */
> > -	intel_dsc_read_pps_reg(crtc_state, PPS_0, &pps_temp1);
> > -	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp2);
> > +	vdsc_cfg->bits_per_component = (pps_temp & DSC_BPC_MASK) >>
> > +DSC_BPC_SHIFT;
> 
> Use REG_FIELD_GET(), throw out the shifts.
> 

Will Update this in the next revision

> > +	vdsc_cfg->line_buf_depth =
> > +		(pps_temp & DSC_LINE_BUF_DEPTH_MASK) >>
> DSC_LINE_BUF_DEPTH_SHIFT;
> > +	vdsc_cfg->block_pred_enable = pps_temp &
> DSC_BLOCK_PREDICTION ? true
> > +: false;
> 
> For assigning booleans just pps_temp & DSC_BLOCK_PREDICTION is enough.
> The ternary operator is superfluous. Ditto below.
> 

Ahh correct got it will fix this

Regards,
Suraj Kandpal
> > +	vdsc_cfg->convert_rgb = pps_temp &
> DSC_COLOR_SPACE_CONVERSION ? true : false;
> > +	vdsc_cfg->simple_422 = pps_temp & DSC_422_ENABLE ? true : false;
> > +	vdsc_cfg->native_422 = pps_temp & DSC_NATIVE_422_ENABLE ?
> true : false;
> > +	vdsc_cfg->native_420 = pps_temp & DSC_NATIVE_420_ENABLE ?
> true : false;
> > +	vdsc_cfg->vbr_enable = pps_temp & DSC_VBR_ENABLE ? true :
> false;
> >
> > -	vdsc_cfg->bits_per_pixel = pps_temp2;
> > +	/* Readout PPS_1 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_1, &pps_temp);
> >
> > -	if (pps_temp1 & DSC_NATIVE_420_ENABLE)
> > +	vdsc_cfg->bits_per_pixel = pps_temp;
> > +
> > +	if (vdsc_cfg->native_420)
> >  		vdsc_cfg->bits_per_pixel >>= 1;
> >
> >  	crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> > +
> > +	/* Readout PPS_2 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_2, &pps_temp);
> > +
> > +	vdsc_cfg->pic_width =
> > +		DSC_PIC_WIDTH(pps_temp & DSC_PIC_WIDTH_MASK,
> false);
> 
> This is the way to go:
> 
> 	vdsc_cfg->pic_width = REG_FIELD_GET(DSC_PIC_WIDTH_MASK,
> pps_temp);
> 
> > +	vdsc_cfg->pic_height = pps_temp & ~DSC_PIC_WIDTH_MASK;
> 
> Nope, just use REG_FIELD_GET() for all of it.
> 
> > +
> > +	/* Readout PPS_3 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_3, &pps_temp);
> > +
> > +	vdsc_cfg->slice_width =
> > +		DSC_SLICE_WIDTH(pps_temp & DSC_SLICE_WIDTH_MASK,
> false);
> > +	vdsc_cfg->slice_height = pps_temp & ~DSC_SLICE_WIDTH_MASK;
> > +
> > +	/* Readout PPS_4 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_4, &pps_temp);
> > +
> > +	vdsc_cfg->initial_dec_delay =
> > +		DSC_INITIAL_DEC_DELAY(pps_temp &
> DSC_INITIAL_DEC_DELAY_MASK, false);
> > +	vdsc_cfg->initial_xmit_delay = pps_temp &
> > +DSC_INITIAL_XMIT_DELAY_MASK;
> > +
> > +	/* Readout PPS_5 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_5, &pps_temp);
> > +
> > +	vdsc_cfg->scale_decrement_interval =
> > +		DSC_SCALE_DEC_INT(pps_temp &
> DSC_SCALE_DEC_INT_MASK, false);
> > +	vdsc_cfg->scale_increment_interval = pps_temp &
> > +DSC_SCALE_INC_INT_MASK;
> > +
> > +	/* Readout PPS_6 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_6, &pps_temp);
> > +
> > +	vdsc_cfg->initial_scale_value = pps_temp &
> DSC_INITIAL_SCALE_VALUE_MASK;
> > +	vdsc_cfg->first_line_bpg_offset =
> > +		DSC_FIRST_LINE_BPG_OFFSET(pps_temp &
> > +
> DSC_FIRST_LINE_BPG_OFFSET_MASK, false);
> > +	vdsc_cfg->flatness_min_qp =
> > +		DSC_FLATNESS_MIN_QP(pps_temp &
> DSC_FLATNESS_MIN_QP_MASK, false);
> > +	vdsc_cfg->flatness_max_qp =
> > +		DSC_FLATNESS_MAX_QP(pps_temp &
> DSC_FLATNESS_MAX_QP_MASK, false);
> > +
> > +	/* Readout PPS_7 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_7, &pps_temp);
> > +
> > +	vdsc_cfg->nfl_bpg_offset =
> > +		DSC_NFL_BPG_OFFSET(pps_temp &
> DSC_NFL_BPG_OFFSET_MASK, false);
> > +	vdsc_cfg->slice_bpg_offset = pps_temp &
> ~DSC_NFL_BPG_OFFSET_MASK;
> > +
> > +	/* Readout PPS_8 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_8, &pps_temp);
> > +
> > +	vdsc_cfg->initial_offset =
> > +		DSC_INITIAL_OFFSET(pps_temp &
> DSC_INITIAL_OFFSET_MASK, false);
> > +	vdsc_cfg->final_offset = pps_temp & ~DSC_INITIAL_OFFSET_MASK;
> > +
> > +	/* Readout PPS_9 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_9, &pps_temp);
> > +
> > +	vdsc_cfg->rc_model_size = pps_temp &
> DSC_RC_MODEL_SIZE_MASK;
> > +
> > +	/* Readout PPS_10 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_10, &pps_temp);
> > +
> > +	vdsc_cfg->rc_quant_incr_limit0 = pps_temp &
> DSC_RC_QUANT_INC_LIMIT0_MASK;
> > +	vdsc_cfg->rc_quant_incr_limit1 =
> > +		DSC_RC_QUANT_INC_LIMIT1(pps_temp &
> DSC_RC_QUANT_INC_LIMIT1_MASK,
> > +false);
> > +
> > +	/* Readout PPS_16 register */
> > +	pps_temp = 0;
> > +	intel_dsc_read_pps_reg(crtc_state, PPS_16, &pps_temp);
> > +
> > +	vdsc_cfg->slice_chunk_size = pps_temp &
> DSC_SLICE_CHUNK_SIZE_MASK;
> > +
> > +	if (DISPLAY_VER(i915) >= 14) {
> > +		/* Readout PPS_17 register */
> > +		pps_temp = 0;
> > +		intel_dsc_read_pps_reg(crtc_state, PPS_17, &pps_temp);
> > +
> > +		vdsc_cfg->second_line_bpg_offset =
> > +			DSC_SL_BPG_OFFSET(pps_temp &
> DSC_SL_BPG_OFFSET_MASK, false);
> > +
> > +		/* Readout PPS_18 register */
> > +		pps_temp = 0;
> > +		intel_dsc_read_pps_reg(crtc_state, PPS_18, &pps_temp);
> > +
> > +		vdsc_cfg->nsl_bpg_offset =
> > +			DSC_NSL_BPG_OFFSET(pps_temp &
> DSC_NSL_BPG_OFFSET_MASK, false);
> > +		vdsc_cfg->second_line_offset_adj =
> > +			pps_temp & ~DSC_NSL_BPG_OFFSET_MASK;
> > +	}
> >  }
> >
> >  void intel_dsc_get_config(struct intel_crtc_state *crtc_state) diff
> > --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> > b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> > index b71f00b5c761..605d37c2978b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h
> > @@ -57,7 +57,8 @@
> >  #define MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _MTL_DSC1_PICTURE_PARAMETER_SET_17_PB, \
> >
> _MTL_DSC1_PICTURE_PARAMETER_SET_17_PC)
> > -#define DSC_SL_BPG_OFFSET(offset)		((offset) << 27)
> > +#define DSC_SL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 27 :
> (offset) >> 27)
> > +#define DSC_SL_BPG_OFFSET_MASK
> 	REG_GENMASK(31, 27)
> >
> >  #define _MTL_DSC0_PICTURE_PARAMETER_SET_18_PB	0x782B8
> >  #define _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB	0x783B8
> > @@ -69,8 +70,9 @@
> >  #define MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB, \
> >
> _MTL_DSC1_PICTURE_PARAMETER_SET_18_PC)
> > -#define DSC_NSL_BPG_OFFSET(offset)		((offset) << 16)
> > +#define DSC_NSL_BPG_OFFSET(offset, shift)	(shift ? (offset) << 16
> : (offset) >> 16)
> >  #define DSC_SL_OFFSET_ADJ(offset)		((offset) << 0)
> > +#define DSC_NSL_BPG_OFFSET_MASK
> 	REG_GENMASK(31, 16)
> >
> >  /* Icelake Display Stream Compression Registers */
> >  #define DSCA_PICTURE_PARAMETER_SET_0		_MMIO(0x6B200)
> > @@ -96,6 +98,9 @@
> >  #define  DSC_BPC_SHIFT			8
> >  #define  DSC_VER_MIN_SHIFT		4
> >  #define  DSC_VER_MAJ			(0x1 << 0)
> > +#define  DSC_LINE_BUF_DEPTH_MASK	REG_GENMASK(15, 12)
> > +#define  DSC_BPC_MASK			REG_GENMASK(11, 8)
> > +
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_1		_MMIO(0x6B204)
> >  #define DSCC_PICTURE_PARAMETER_SET_1		_MMIO(0x6BA04)
> > @@ -123,8 +128,9 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_2_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_2_PC)
> > -#define  DSC_PIC_WIDTH(pic_width)	((pic_width) << 16)
> > -#define  DSC_PIC_HEIGHT(pic_height)	((pic_height) << 0)
> > +#define  DSC_PIC_WIDTH(pic_width, shift)	(shift ? (pic_width) << 16 :
> (pic_width >> 16))
> > +#define  DSC_PIC_HEIGHT(pic_height)		((pic_height) << 0)
> > +#define  DSC_PIC_WIDTH_MASK			REG_GENMASK(31,
> 16)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_3		_MMIO(0x6B20C)
> >  #define DSCC_PICTURE_PARAMETER_SET_3		_MMIO(0x6BA0C)
> > @@ -138,8 +144,9 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_3_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_3_PC)
> > -#define  DSC_SLICE_WIDTH(slice_width)   ((slice_width) << 16)
> > +#define  DSC_SLICE_WIDTH(slice_width, shift)   (shift ? (slice_width) << 16
> : (slice_width >> 16))
> >  #define  DSC_SLICE_HEIGHT(slice_height) ((slice_height) << 0)
> > +#define  DSC_SLICE_WIDTH_MASK			REG_GENMASK(31,
> 16)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_4		_MMIO(0x6B210)
> >  #define DSCC_PICTURE_PARAMETER_SET_4		_MMIO(0x6BA10)
> > @@ -153,8 +160,11 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_4_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_4_PC)
> > -#define  DSC_INITIAL_DEC_DELAY(dec_delay)       ((dec_delay) << 16)
> > -#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)     ((xmit_delay) << 0)
> > +#define  DSC_INITIAL_DEC_DELAY(dec_delay, shift)	(shift ? (dec_delay)
> << 16 : \
> > +							 (dec_delay) >> 16)
> > +#define  DSC_INITIAL_XMIT_DELAY(xmit_delay)		((xmit_delay)
> << 0)
> > +#define  DSC_INITIAL_DEC_DELAY_MASK
> 	REG_GENMASK(31, 16)
> > +#define  DSC_INITIAL_XMIT_DELAY_MASK
> 	REG_GENMASK(9, 0)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_5		_MMIO(0x6B214)
> >  #define DSCC_PICTURE_PARAMETER_SET_5		_MMIO(0x6BA14)
> > @@ -168,8 +178,10 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_5_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_5_PC)
> > -#define  DSC_SCALE_DEC_INT(scale_dec)	((scale_dec) << 16)
> > +#define  DSC_SCALE_DEC_INT(scale_dec, shift)	(shift ? (scale_dec) <<
> 16 : (scale_dec) >> 16)
> >  #define  DSC_SCALE_INC_INT(scale_inc)		((scale_inc) << 0)
> > +#define  DSC_SCALE_DEC_INT_MASK
> 	REG_GENMASK(27, 16)
> > +#define  DSC_SCALE_INC_INT_MASK
> 	REG_GENMASK(15, 0)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_6		_MMIO(0x6B218)
> >  #define DSCC_PICTURE_PARAMETER_SET_6		_MMIO(0x6BA18)
> > @@ -183,10 +195,14 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_6_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_6_PC)
> > -#define  DSC_FLATNESS_MAX_QP(max_qp)		((max_qp) << 24)
> > -#define  DSC_FLATNESS_MIN_QP(min_qp)		((min_qp) << 16)
> > -#define  DSC_FIRST_LINE_BPG_OFFSET(offset)	((offset) << 8)
> > -#define  DSC_INITIAL_SCALE_VALUE(value)		((value) << 0)
> > +#define  DSC_FLATNESS_MAX_QP(max_qp, shift)		(shift ?
> (max_qp) << 24 : (max_qp) >> 24)
> > +#define  DSC_FLATNESS_MIN_QP(min_qp, shift)		(shift ?
> (min_qp) << 16 : (min_qp) >> 16)
> > +#define  DSC_FIRST_LINE_BPG_OFFSET(offset, shift)	(shift ?
> (offset) << 8 : (offset) >> 8)
> > +#define  DSC_INITIAL_SCALE_VALUE(value)			((value) << 0)
> > +#define  DSC_FLATNESS_MAX_QP_MASK
> 	REG_GENMASK(28, 24)
> > +#define  DSC_FLATNESS_MIN_QP_MASK
> 	REG_GENMASK(20, 16)
> > +#define  DSC_FIRST_LINE_BPG_OFFSET_MASK
> 	REG_GENMASK(12, 8)
> > +#define  DSC_INITIAL_SCALE_VALUE_MASK
> 	REG_GENMASK(5, 0)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_7		_MMIO(0x6B21C)
> >  #define DSCC_PICTURE_PARAMETER_SET_7		_MMIO(0x6BA1C)
> > @@ -200,8 +216,9 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_7_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_7_PC)
> > -#define  DSC_NFL_BPG_OFFSET(bpg_offset)		((bpg_offset)
> << 16)
> > +#define  DSC_NFL_BPG_OFFSET(bpg_offset, shift)	(shift ? (bpg_offset)
> << 16 : (bpg_offset) >> 16)
> >  #define  DSC_SLICE_BPG_OFFSET(bpg_offset)	((bpg_offset) << 0)
> > +#define  DSC_NFL_BPG_OFFSET_MASK		REG_GENMASK(31,
> 16)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_8		_MMIO(0x6B220)
> >  #define DSCC_PICTURE_PARAMETER_SET_8		_MMIO(0x6BA20)
> > @@ -215,8 +232,10 @@
> >  #define ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe)
> 	_MMIO_PIPE((pipe) - PIPE_B, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_8_PB, \
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_8_PC)
> > -#define  DSC_INITIAL_OFFSET(initial_offset)		((initial_offset) << 16)
> > +#define  DSC_INITIAL_OFFSET(initial_offset, shift)	(shift ? (initial_offset)
> << 16 : \
> > +							 (initial_offset) >> 16)
> >  #define  DSC_FINAL_OFFSET(final_offset)			((final_offset)
> << 0)
> > +#define  DSC_INITIAL_OFFSET_MASK
> 	REG_GENMASK(31, 16)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_9		_MMIO(0x6B224)
> >  #define DSCC_PICTURE_PARAMETER_SET_9		_MMIO(0x6BA24)
> > @@ -232,6 +251,7 @@
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_9_PC)
> >  #define  DSC_RC_EDGE_FACTOR(rc_edge_fact)	((rc_edge_fact) <<
> 16)
> >  #define  DSC_RC_MODEL_SIZE(rc_model_size)	((rc_model_size) <<
> 0)
> > +#define  DSC_RC_MODEL_SIZE_MASK
> 	REG_GENMASK(15, 0)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_10
> 	_MMIO(0x6B228)
> >  #define DSCC_PICTURE_PARAMETER_SET_10
> 	_MMIO(0x6BA28)
> > @@ -247,8 +267,10 @@
> >
> _ICL_DSC1_PICTURE_PARAMETER_SET_10_PC)
> >  #define  DSC_RC_TARGET_OFF_LOW(rc_tgt_off_low)
> 	((rc_tgt_off_low) << 20)
> >  #define  DSC_RC_TARGET_OFF_HIGH(rc_tgt_off_high)
> 	((rc_tgt_off_high) << 16)
> > -#define  DSC_RC_QUANT_INC_LIMIT1(lim)			((lim) << 8)
> > +#define  DSC_RC_QUANT_INC_LIMIT1(lim, shift)		(shift ? (lim)
> << 8 : (lim) >> 8)
> >  #define  DSC_RC_QUANT_INC_LIMIT0(lim)			((lim) << 0)
> > +#define  DSC_RC_QUANT_INC_LIMIT1_MASK
> 	REG_GENMASK(12, 8)
> > +#define  DSC_RC_QUANT_INC_LIMIT0_MASK
> 	REG_GENMASK(4, 0)
> >
> >  #define DSCA_PICTURE_PARAMETER_SET_11
> 	_MMIO(0x6B22C)
> >  #define DSCC_PICTURE_PARAMETER_SET_11
> 	_MMIO(0x6BA2C)
> > @@ -330,6 +352,7 @@
> >  #define  DSC_SLICE_ROW_PER_FRAME(slice_row_per_frame)
> 	((slice_row_per_frame) << 20)
> >  #define  DSC_SLICE_PER_LINE(slice_per_line)
> 	((slice_per_line) << 16)
> >  #define  DSC_SLICE_CHUNK_SIZE(slice_chunk_size)
> 	((slice_chunk_size) << 0)
> > +#define  DSC_SLICE_CHUNK_SIZE_MASK		REG_GENMASK(15,
> 0)
> >
> >  /* Icelake Rate Control Buffer Threshold Registers */
> >  #define DSCA_RC_BUF_THRESH_0			_MMIO(0x6B230)
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
                   ` (3 preceding siblings ...)
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
@ 2023-07-10 10:09 ` Suraj Kandpal
  2023-07-10 11:15   ` Jani Nikula
  2023-07-10 13:45 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Add DSC PPS readout Patchwork
  5 siblings, 1 reply; 17+ messages in thread
From: Suraj Kandpal @ 2023-07-10 10:09 UTC (permalink / raw)
  To: intel-gfx

With the dsc config being readout and filled in crtc_state add
macros and use them to compare current and previous PPS param in
DSC.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index eed01957bdb9..5c1596d7cd92 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5007,6 +5007,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 {
 	struct drm_i915_private *dev_priv = to_i915(current_config->uapi.crtc->dev);
 	struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
+	const struct drm_dsc_config *dsc_current_config = &current_config->dsc.config;
+	const struct drm_dsc_config *dsc_pipe_config = &pipe_config->dsc.config;
 	bool ret = true;
 	bool fixup_inherited = fastset &&
 		current_config->inherited && !pipe_config->inherited;
@@ -5202,6 +5204,26 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 #define PIPE_CONF_QUIRK(quirk) \
 	((current_config->quirks | pipe_config->quirks) & (quirk))
 
+#define PIPE_DSC_CONF_CHECK_I(name) do { \
+	if (dsc_current_config->name != dsc_pipe_config->name) { \
+		pipe_config_mismatch(fastset, crtc, __stringify(name), \
+				     "(expected %i, found %i)", \
+				     dsc_current_config->name, \
+				     dsc_pipe_config->name); \
+		ret = false; \
+	} \
+} while (0)
+
+#define PIPE_DSC_CONF_CHECK_BOOL(name) do { \
+	if (dsc_current_config->name != dsc_pipe_config->name) { \
+		pipe_config_mismatch(fastset, crtc,  __stringify(name), \
+				     "(expected %s, found %s)", \
+				     str_yes_no(dsc_current_config->name), \
+				     str_yes_no(dsc_pipe_config->name)); \
+		ret = false; \
+	} \
+} while (0)
+
 	PIPE_CONF_CHECK_I(hw.enable);
 	PIPE_CONF_CHECK_I(hw.active);
 
@@ -5378,6 +5400,39 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_I(master_transcoder);
 	PIPE_CONF_CHECK_X(bigjoiner_pipes);
 
+	PIPE_DSC_CONF_CHECK_BOOL(block_pred_enable);
+	PIPE_DSC_CONF_CHECK_BOOL(convert_rgb);
+	PIPE_DSC_CONF_CHECK_BOOL(simple_422);
+	PIPE_DSC_CONF_CHECK_BOOL(native_422);
+	PIPE_DSC_CONF_CHECK_BOOL(native_420);
+	PIPE_DSC_CONF_CHECK_BOOL(vbr_enable);
+	PIPE_DSC_CONF_CHECK_I(line_buf_depth);
+	PIPE_DSC_CONF_CHECK_I(bits_per_component);
+	PIPE_DSC_CONF_CHECK_I(pic_width);
+	PIPE_DSC_CONF_CHECK_I(pic_height);
+	PIPE_DSC_CONF_CHECK_I(slice_width);
+	PIPE_DSC_CONF_CHECK_I(slice_height);
+	PIPE_DSC_CONF_CHECK_I(initial_dec_delay);
+	PIPE_DSC_CONF_CHECK_I(initial_xmit_delay);
+	PIPE_DSC_CONF_CHECK_I(scale_decrement_interval);
+	PIPE_DSC_CONF_CHECK_I(scale_increment_interval);
+	PIPE_DSC_CONF_CHECK_I(initial_scale_value);
+	PIPE_DSC_CONF_CHECK_I(first_line_bpg_offset);
+	PIPE_DSC_CONF_CHECK_I(flatness_min_qp);
+	PIPE_DSC_CONF_CHECK_I(flatness_max_qp);
+	PIPE_DSC_CONF_CHECK_I(slice_bpg_offset);
+	PIPE_DSC_CONF_CHECK_I(nfl_bpg_offset);
+	PIPE_DSC_CONF_CHECK_I(initial_offset);
+	PIPE_DSC_CONF_CHECK_I(final_offset);
+	PIPE_DSC_CONF_CHECK_I(rc_model_size);
+	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit0);
+	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit1);
+	PIPE_DSC_CONF_CHECK_I(slice_chunk_size);
+	if (DISPLAY_VER(dev_priv) >= 14) {
+		PIPE_DSC_CONF_CHECK_I(second_line_bpg_offset);
+		PIPE_DSC_CONF_CHECK_I(nsl_bpg_offset);
+	}
+
 	PIPE_CONF_CHECK_I(dsc.compression_enable);
 	PIPE_CONF_CHECK_I(dsc.dsc_split);
 	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
@ 2023-07-10 11:15   ` Jani Nikula
  2023-07-11  5:15     ` Kandpal, Suraj
  0 siblings, 1 reply; 17+ messages in thread
From: Jani Nikula @ 2023-07-10 11:15 UTC (permalink / raw)
  To: Suraj Kandpal, intel-gfx

On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> With the dsc config being readout and filled in crtc_state add
> macros and use them to compare current and previous PPS param in
> DSC.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 55 ++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index eed01957bdb9..5c1596d7cd92 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5007,6 +5007,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(current_config->uapi.crtc->dev);
>  	struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> +	const struct drm_dsc_config *dsc_current_config = &current_config->dsc.config;
> +	const struct drm_dsc_config *dsc_pipe_config = &pipe_config->dsc.config;
>  	bool ret = true;
>  	bool fixup_inherited = fastset &&
>  		current_config->inherited && !pipe_config->inherited;
> @@ -5202,6 +5204,26 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  #define PIPE_CONF_QUIRK(quirk) \
>  	((current_config->quirks | pipe_config->quirks) & (quirk))
>  
> +#define PIPE_DSC_CONF_CHECK_I(name) do { \
> +	if (dsc_current_config->name != dsc_pipe_config->name) { \
> +		pipe_config_mismatch(fastset, crtc, __stringify(name), \
> +				     "(expected %i, found %i)", \
> +				     dsc_current_config->name, \
> +				     dsc_pipe_config->name); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
> +#define PIPE_DSC_CONF_CHECK_BOOL(name) do { \
> +	if (dsc_current_config->name != dsc_pipe_config->name) { \
> +		pipe_config_mismatch(fastset, crtc,  __stringify(name), \
> +				     "(expected %s, found %s)", \
> +				     str_yes_no(dsc_current_config->name), \
> +				     str_yes_no(dsc_pipe_config->name)); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
>  	PIPE_CONF_CHECK_I(hw.enable);
>  	PIPE_CONF_CHECK_I(hw.active);
>  
> @@ -5378,6 +5400,39 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(master_transcoder);
>  	PIPE_CONF_CHECK_X(bigjoiner_pipes);
>  
> +	PIPE_DSC_CONF_CHECK_BOOL(block_pred_enable);

You should be able to pass the dsc substruct as name, no need to define
dupe macros for DSC. See e.g. PIPE_CONF_CHECK_I(hw.enable); above in the
patch context above.

> +	PIPE_DSC_CONF_CHECK_BOOL(convert_rgb);
> +	PIPE_DSC_CONF_CHECK_BOOL(simple_422);
> +	PIPE_DSC_CONF_CHECK_BOOL(native_422);
> +	PIPE_DSC_CONF_CHECK_BOOL(native_420);
> +	PIPE_DSC_CONF_CHECK_BOOL(vbr_enable);
> +	PIPE_DSC_CONF_CHECK_I(line_buf_depth);
> +	PIPE_DSC_CONF_CHECK_I(bits_per_component);
> +	PIPE_DSC_CONF_CHECK_I(pic_width);
> +	PIPE_DSC_CONF_CHECK_I(pic_height);
> +	PIPE_DSC_CONF_CHECK_I(slice_width);
> +	PIPE_DSC_CONF_CHECK_I(slice_height);
> +	PIPE_DSC_CONF_CHECK_I(initial_dec_delay);
> +	PIPE_DSC_CONF_CHECK_I(initial_xmit_delay);
> +	PIPE_DSC_CONF_CHECK_I(scale_decrement_interval);
> +	PIPE_DSC_CONF_CHECK_I(scale_increment_interval);
> +	PIPE_DSC_CONF_CHECK_I(initial_scale_value);
> +	PIPE_DSC_CONF_CHECK_I(first_line_bpg_offset);
> +	PIPE_DSC_CONF_CHECK_I(flatness_min_qp);
> +	PIPE_DSC_CONF_CHECK_I(flatness_max_qp);
> +	PIPE_DSC_CONF_CHECK_I(slice_bpg_offset);
> +	PIPE_DSC_CONF_CHECK_I(nfl_bpg_offset);
> +	PIPE_DSC_CONF_CHECK_I(initial_offset);
> +	PIPE_DSC_CONF_CHECK_I(final_offset);
> +	PIPE_DSC_CONF_CHECK_I(rc_model_size);
> +	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit0);
> +	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit1);
> +	PIPE_DSC_CONF_CHECK_I(slice_chunk_size);
> +	if (DISPLAY_VER(dev_priv) >= 14) {
> +		PIPE_DSC_CONF_CHECK_I(second_line_bpg_offset);
> +		PIPE_DSC_CONF_CHECK_I(nsl_bpg_offset);
> +	}

I'd prefer it if we didn't have version checks here. Just check the
values anyway, it should be zeros in both hw and sw states for display <
14, and if it's not, the state checker caught a bug.


> +
>  	PIPE_CONF_CHECK_I(dsc.compression_enable);
>  	PIPE_CONF_CHECK_I(dsc.dsc_split);
>  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params
  2023-07-10 11:15   ` Jani Nikula
@ 2023-07-11  5:15     ` Kandpal, Suraj
  0 siblings, 0 replies; 17+ messages in thread
From: Kandpal, Suraj @ 2023-07-11  5:15 UTC (permalink / raw)
  To: Jani Nikula, intel-gfx@lists.freedesktop.org

> On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> > With the dsc config being readout and filled in crtc_state add macros
> > and use them to compare current and previous PPS param in DSC.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 55
> > ++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index eed01957bdb9..5c1596d7cd92 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5007,6 +5007,8 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config,  {
> >  	struct drm_i915_private *dev_priv = to_i915(current_config-
> >uapi.crtc->dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> > +	const struct drm_dsc_config *dsc_current_config = &current_config-
> >dsc.config;
> > +	const struct drm_dsc_config *dsc_pipe_config =
> > +&pipe_config->dsc.config;
> >  	bool ret = true;
> >  	bool fixup_inherited = fastset &&
> >  		current_config->inherited && !pipe_config->inherited; @@ -
> 5202,6
> > +5204,26 @@ intel_pipe_config_compare(const struct intel_crtc_state
> > *current_config,  #define PIPE_CONF_QUIRK(quirk) \
> >  	((current_config->quirks | pipe_config->quirks) & (quirk))
> >
> > +#define PIPE_DSC_CONF_CHECK_I(name) do { \
> > +	if (dsc_current_config->name != dsc_pipe_config->name) { \
> > +		pipe_config_mismatch(fastset, crtc, __stringify(name), \
> > +				     "(expected %i, found %i)", \
> > +				     dsc_current_config->name, \
> > +				     dsc_pipe_config->name); \
> > +		ret = false; \
> > +	} \
> > +} while (0)
> > +
> > +#define PIPE_DSC_CONF_CHECK_BOOL(name) do { \
> > +	if (dsc_current_config->name != dsc_pipe_config->name) { \
> > +		pipe_config_mismatch(fastset, crtc,  __stringify(name), \
> > +				     "(expected %s, found %s)", \
> > +				     str_yes_no(dsc_current_config->name), \
> > +				     str_yes_no(dsc_pipe_config->name)); \
> > +		ret = false; \
> > +	} \
> > +} while (0)
> > +
> >  	PIPE_CONF_CHECK_I(hw.enable);
> >  	PIPE_CONF_CHECK_I(hw.active);
> >
> > @@ -5378,6 +5400,39 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_I(master_transcoder);
> >  	PIPE_CONF_CHECK_X(bigjoiner_pipes);
> >
> > +	PIPE_DSC_CONF_CHECK_BOOL(block_pred_enable);
> 
> You should be able to pass the dsc substruct as name, no need to define
> dupe macros for DSC. See e.g. PIPE_CONF_CHECK_I(hw.enable); above in
> the patch context above.
> 
Hi 
 
Thanks for the review will get this fixed in the next revision.

> > +	PIPE_DSC_CONF_CHECK_BOOL(convert_rgb);
> > +	PIPE_DSC_CONF_CHECK_BOOL(simple_422);
> > +	PIPE_DSC_CONF_CHECK_BOOL(native_422);
> > +	PIPE_DSC_CONF_CHECK_BOOL(native_420);
> > +	PIPE_DSC_CONF_CHECK_BOOL(vbr_enable);
> > +	PIPE_DSC_CONF_CHECK_I(line_buf_depth);
> > +	PIPE_DSC_CONF_CHECK_I(bits_per_component);
> > +	PIPE_DSC_CONF_CHECK_I(pic_width);
> > +	PIPE_DSC_CONF_CHECK_I(pic_height);
> > +	PIPE_DSC_CONF_CHECK_I(slice_width);
> > +	PIPE_DSC_CONF_CHECK_I(slice_height);
> > +	PIPE_DSC_CONF_CHECK_I(initial_dec_delay);
> > +	PIPE_DSC_CONF_CHECK_I(initial_xmit_delay);
> > +	PIPE_DSC_CONF_CHECK_I(scale_decrement_interval);
> > +	PIPE_DSC_CONF_CHECK_I(scale_increment_interval);
> > +	PIPE_DSC_CONF_CHECK_I(initial_scale_value);
> > +	PIPE_DSC_CONF_CHECK_I(first_line_bpg_offset);
> > +	PIPE_DSC_CONF_CHECK_I(flatness_min_qp);
> > +	PIPE_DSC_CONF_CHECK_I(flatness_max_qp);
> > +	PIPE_DSC_CONF_CHECK_I(slice_bpg_offset);
> > +	PIPE_DSC_CONF_CHECK_I(nfl_bpg_offset);
> > +	PIPE_DSC_CONF_CHECK_I(initial_offset);
> > +	PIPE_DSC_CONF_CHECK_I(final_offset);
> > +	PIPE_DSC_CONF_CHECK_I(rc_model_size);
> > +	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit0);
> > +	PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit1);
> > +	PIPE_DSC_CONF_CHECK_I(slice_chunk_size);
> > +	if (DISPLAY_VER(dev_priv) >= 14) {
> > +		PIPE_DSC_CONF_CHECK_I(second_line_bpg_offset);
> > +		PIPE_DSC_CONF_CHECK_I(nsl_bpg_offset);
> > +	}
> 
> I'd prefer it if we didn't have version checks here. Just check the values
> anyway, it should be zeros in both hw and sw states for display < 14, and if it's
> not, the state checker caught a bug.
> 

Oh got it will remove the version check from here.

Regards,
Suraj Kandpal
> 
> > +
> >  	PIPE_CONF_CHECK_I(dsc.compression_enable);
> >  	PIPE_CONF_CHECK_I(dsc.dsc_split);
> >  	PIPE_CONF_CHECK_I(dsc.compressed_bpp);
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BUILD: failure for Add DSC PPS readout
  2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
                   ` (4 preceding siblings ...)
  2023-07-10 10:09 ` [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
@ 2023-07-10 13:45 ` Patchwork
  5 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2023-07-10 13:45 UTC (permalink / raw)
  To: Suraj Kandpal; +Cc: intel-gfx

== Series Details ==

Series: Add DSC PPS readout
URL   : https://patchwork.freedesktop.org/series/120456/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/120456/revisions/1/mbox/ not applied
Applying: drm/i915/dsc: Add PPS enum
Applying: drm/i915/vdsc: Add function to read any PPS register
Applying: drm/i915/vdsc: Add function to write in PPS registers
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/display/intel_vdsc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/display/intel_vdsc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_vdsc.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 drm/i915/vdsc: Add function to write in PPS registers
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-07-11  9:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
2023-07-10 10:45   ` Jani Nikula
2023-07-10 10:45     ` Jani Nikula
2023-07-11  6:12       ` Kandpal, Suraj
2023-07-10 10:09 ` [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
2023-07-10 10:52   ` Jani Nikula
2023-07-10 10:56     ` Jani Nikula
2023-07-10 10:09 ` [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers Suraj Kandpal
2023-07-10 10:57   ` Jani Nikula
2023-07-10 10:09 ` [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
2023-07-10 11:11   ` Jani Nikula
2023-07-11  9:42     ` Kandpal, Suraj
2023-07-10 10:09 ` [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
2023-07-10 11:15   ` Jani Nikula
2023-07-11  5:15     ` Kandpal, Suraj
2023-07-10 13:45 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Add DSC PPS readout Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.