[Intel-gfx] [PATCH 0/2] Update DSC Bigjoiner BW check

[Intel-gfx] [PATCH 0/2] Update DSC Bigjoiner BW check

[Ankit Nautiyal]

Update the DSC Bigjoiner BW check for computing compressed bpp:
-As per latest Bspec update bigjoiner interface bits for DP for
DISPLAY > 12 is 36.
-Use current cdclk for the calculation of compressed_bpp instead of
maxcdclk.

Ankit Nautiyal (2):
  drm/i915/dp: Update Bigjoiner interface bits for computing compressed
    bpp
  drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

 drivers/gpu/drm/i915/display/intel_dp.c     | 10 +++++++---
 drivers/gpu/drm/i915/display/intel_dp.h     |  1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c |  1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.25.1

[Intel-gfx] [PATCH 1/2] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ankit Nautiyal]

In Bigjoiner check for DSC, bigjoiner interface bits for DP for
DISPLAY > 12 is 36 (Bspec: 49259).

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index da1c00ee92fb..3fe651a8f5d0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -756,8 +756,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
 
 	if (bigjoiner) {
+		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
 		u32 max_bpp_bigjoiner =
-			i915->display.cdclk.max_cdclk_freq * 48 /
+			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
 			intel_dp_mode_to_fec_clock(mode_clock);
 
 		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
-- 
2.25.1

[Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Ankit Nautiyal]

As per Bspec, Big Joiner BW check is:
Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
Pixel clock

Currently we always use max_cdclk in the check for both modevalid
and compute config steps.

During modevalid use max_cdclk_freq for the check.
During compute config step use current cdclk for the check.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
 drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 3fe651a8f5d0..d6600de1ab49 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
 u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 				u32 link_clock, u32 lane_count,
 				u32 mode_clock, u32 mode_hdisplay,
+				unsigned int cdclk,
 				bool bigjoiner,
 				u32 pipe_bpp,
 				u32 timeslots)
@@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 
 	if (bigjoiner) {
 		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
-		u32 max_bpp_bigjoiner =
-			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
-			intel_dp_mode_to_fec_clock(mode_clock);
+
+		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
+					intel_dp_mode_to_fec_clock(mode_clock);
 
 		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
 	}
@@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
 							    max_lanes,
 							    target_clock,
 							    mode->hdisplay,
+							    dev_priv->display.cdclk.max_cdclk_freq,
 							    bigjoiner,
 							    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
@@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 							    pipe_config->lane_count,
 							    adjusted_mode->crtc_clock,
 							    adjusted_mode->crtc_hdisplay,
+							    dev_priv->display.cdclk.hw.cdclk,
 							    pipe_config->bigjoiner_pipes,
 							    pipe_bpp,
 							    timeslots);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index ef39e4f7a329..d150bfe8abf4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
 u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 				u32 link_clock, u32 lane_count,
 				u32 mode_clock, u32 mode_hdisplay,
+				unsigned int cdclk,
 				bool bigjoiner,
 				u32 pipe_bpp,
 				u32 timeslots);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index a860cbc5dbea..266e31b78729 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
 							    max_lanes,
 							    target_clock,
 							    mode->hdisplay,
+							    dev_priv->display.cdclk.max_cdclk_freq,
 							    bigjoiner,
 							    pipe_bpp, 64) >> 4;
 			dsc_slice_count =
-- 
2.25.1

[Intel-gfx] [PATCH v2 1/2] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ankit Nautiyal]

In Bigjoiner check for DSC, bigjoiner interface bits for DP for
DISPLAY > 13 is 36 (Bspec: 49259).

v2: Corrected Display ver to 13.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index da1c00ee92fb..0b59c1e53678 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -756,8 +756,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
 	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
 
 	if (bigjoiner) {
+		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 13 ? 24 : 36;
 		u32 max_bpp_bigjoiner =
-			i915->display.cdclk.max_cdclk_freq * 48 /
+			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
 			intel_dp_mode_to_fec_clock(mode_clock);
 
 		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
-- 
2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> As per Bspec, Big Joiner BW check is:
> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> Pixel clock
> 
> Currently we always use max_cdclk in the check for both modevalid
> and compute config steps.
> 
> During modevalid use max_cdclk_freq for the check.
> During compute config step use current cdclk for the check.

Nak. cdclk is computed much later based on what is actually needed.
The cdclk freq you are using here is essentially a random number.

> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>  drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 3fe651a8f5d0..d6600de1ab49 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>  u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  				u32 link_clock, u32 lane_count,
>  				u32 mode_clock, u32 mode_hdisplay,
> +				unsigned int cdclk,
>  				bool bigjoiner,
>  				u32 pipe_bpp,
>  				u32 timeslots)
> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  
>  	if (bigjoiner) {
>  		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
> -		u32 max_bpp_bigjoiner =
> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> -			intel_dp_mode_to_fec_clock(mode_clock);
> +
> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
> +					intel_dp_mode_to_fec_clock(mode_clock);
>  
>  		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>  	}
> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  							    max_lanes,
>  							    target_clock,
>  							    mode->hdisplay,
> +							    dev_priv->display.cdclk.max_cdclk_freq,
>  							    bigjoiner,
>  							    pipe_bpp, 64) >> 4;
>  			dsc_slice_count =
> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  							    pipe_config->lane_count,
>  							    adjusted_mode->crtc_clock,
>  							    adjusted_mode->crtc_hdisplay,
> +							    dev_priv->display.cdclk.hw.cdclk,
>  							    pipe_config->bigjoiner_pipes,
>  							    pipe_bpp,
>  							    timeslots);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index ef39e4f7a329..d150bfe8abf4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>  u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  				u32 link_clock, u32 lane_count,
>  				u32 mode_clock, u32 mode_hdisplay,
> +				unsigned int cdclk,
>  				bool bigjoiner,
>  				u32 pipe_bpp,
>  				u32 timeslots);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index a860cbc5dbea..266e31b78729 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  							    max_lanes,
>  							    target_clock,
>  							    mode->hdisplay,
> +							    dev_priv->display.cdclk.max_cdclk_freq,
>  							    bigjoiner,
>  							    pipe_bpp, 64) >> 4;
>  			dsc_slice_count =
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Nautiyal, Ankit K]

On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
>> As per Bspec, Big Joiner BW check is:
>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
>> Pixel clock
>>
>> Currently we always use max_cdclk in the check for both modevalid
>> and compute config steps.
>>
>> During modevalid use max_cdclk_freq for the check.
>> During compute config step use current cdclk for the check.
> Nak. cdclk is computed much later based on what is actually needed.
> The cdclk freq you are using here is essentially a random number.

Oh I didn't realise that, perhaps I was lucky when I tested this.

So this check where CDCLK is mentioned, actually expects max_cdclk_freq?

If it doesnt then, we might have a compressed_bpp value, that might be 
violating the big joiner bw check.

Should this be handled while computing cdclk?

Regards,

Ankit

>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>>   drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 3fe651a8f5d0..d6600de1ab49 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>>   u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>   				u32 link_clock, u32 lane_count,
>>   				u32 mode_clock, u32 mode_hdisplay,
>> +				unsigned int cdclk,
>>   				bool bigjoiner,
>>   				u32 pipe_bpp,
>>   				u32 timeslots)
>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>   
>>   	if (bigjoiner) {
>>   		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
>> -		u32 max_bpp_bigjoiner =
>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>> -			intel_dp_mode_to_fec_clock(mode_clock);
>> +
>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
>> +					intel_dp_mode_to_fec_clock(mode_clock);
>>   
>>   		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>>   	}
>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>>   							    max_lanes,
>>   							    target_clock,
>>   							    mode->hdisplay,
>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>   							    bigjoiner,
>>   							    pipe_bpp, 64) >> 4;
>>   			dsc_slice_count =
>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>   							    pipe_config->lane_count,
>>   							    adjusted_mode->crtc_clock,
>>   							    adjusted_mode->crtc_hdisplay,
>> +							    dev_priv->display.cdclk.hw.cdclk,
>>   							    pipe_config->bigjoiner_pipes,
>>   							    pipe_bpp,
>>   							    timeslots);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>> index ef39e4f7a329..d150bfe8abf4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>>   u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>   				u32 link_clock, u32 lane_count,
>>   				u32 mode_clock, u32 mode_hdisplay,
>> +				unsigned int cdclk,
>>   				bool bigjoiner,
>>   				u32 pipe_bpp,
>>   				u32 timeslots);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> index a860cbc5dbea..266e31b78729 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>   							    max_lanes,
>>   							    target_clock,
>>   							    mode->hdisplay,
>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>   							    bigjoiner,
>>   							    pipe_bpp, 64) >> 4;
>>   			dsc_slice_count =
>> -- 
>> 2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> > On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> >> As per Bspec, Big Joiner BW check is:
> >> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> >> Pixel clock
> >>
> >> Currently we always use max_cdclk in the check for both modevalid
> >> and compute config steps.
> >>
> >> During modevalid use max_cdclk_freq for the check.
> >> During compute config step use current cdclk for the check.
> > Nak. cdclk is computed much later based on what is actually needed.
> > The cdclk freq you are using here is essentially a random number.
> 
> Oh I didn't realise that, perhaps I was lucky when I tested this.
> 
> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
> 
> If it doesnt then, we might have a compressed_bpp value, that might be 
> violating the big joiner bw check.
> 
> Should this be handled while computing cdclk?

Yes. I suggest adding something like intel_vdsc_min_cdclk() that
handles all of it.

> 
> Regards,
> 
> Ankit
> 
> >
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
> >>   drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
> >>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
> >>   3 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 3fe651a8f5d0..d6600de1ab49 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
> >>   u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>   				u32 link_clock, u32 lane_count,
> >>   				u32 mode_clock, u32 mode_hdisplay,
> >> +				unsigned int cdclk,
> >>   				bool bigjoiner,
> >>   				u32 pipe_bpp,
> >>   				u32 timeslots)
> >> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>   
> >>   	if (bigjoiner) {
> >>   		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
> >> -		u32 max_bpp_bigjoiner =
> >> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> >> -			intel_dp_mode_to_fec_clock(mode_clock);
> >> +
> >> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
> >> +					intel_dp_mode_to_fec_clock(mode_clock);
> >>   
> >>   		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> >>   	}
> >> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> >>   							    max_lanes,
> >>   							    target_clock,
> >>   							    mode->hdisplay,
> >> +							    dev_priv->display.cdclk.max_cdclk_freq,
> >>   							    bigjoiner,
> >>   							    pipe_bpp, 64) >> 4;
> >>   			dsc_slice_count =
> >> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >>   							    pipe_config->lane_count,
> >>   							    adjusted_mode->crtc_clock,
> >>   							    adjusted_mode->crtc_hdisplay,
> >> +							    dev_priv->display.cdclk.hw.cdclk,
> >>   							    pipe_config->bigjoiner_pipes,
> >>   							    pipe_bpp,
> >>   							    timeslots);
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> >> index ef39e4f7a329..d150bfe8abf4 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> >> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
> >>   u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>   				u32 link_clock, u32 lane_count,
> >>   				u32 mode_clock, u32 mode_hdisplay,
> >> +				unsigned int cdclk,
> >>   				bool bigjoiner,
> >>   				u32 pipe_bpp,
> >>   				u32 timeslots);
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> index a860cbc5dbea..266e31b78729 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> >>   							    max_lanes,
> >>   							    target_clock,
> >>   							    mode->hdisplay,
> >> +							    dev_priv->display.cdclk.max_cdclk_freq,
> >>   							    bigjoiner,
> >>   							    pipe_bpp, 64) >> 4;
> >>   			dsc_slice_count =
> >> -- 
> >> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 02:37:45PM +0530, Ankit Nautiyal wrote:
> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
> DISPLAY > 13 is 36 (Bspec: 49259).
> 
> v2: Corrected Display ver to 13.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index da1c00ee92fb..0b59c1e53678 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -756,8 +756,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>  	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>  
>  	if (bigjoiner) {
> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 13 ? 24 : 36;

We generally prefer "new -> old" order. So please flip that around.

>  		u32 max_bpp_bigjoiner =
> -			i915->display.cdclk.max_cdclk_freq * 48 /
> +			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>  			intel_dp_mode_to_fec_clock(mode_clock);

Hmm. Why is this using the FEC adjusted clock here?

>  
>  		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Nautiyal, Ankit K]

On 3/29/2023 4:24 PM, Ville Syrjälä wrote:
> On Wed, Mar 29, 2023 at 02:37:45PM +0530, Ankit Nautiyal wrote:
>> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
>> DISPLAY > 13 is 36 (Bspec: 49259).
>>
>> v2: Corrected Display ver to 13.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index da1c00ee92fb..0b59c1e53678 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -756,8 +756,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>   	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>>   
>>   	if (bigjoiner) {
>> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 13 ? 24 : 36;
> We generally prefer "new -> old" order. So please flip that around.

Noted. Will do this in next version.


>
>>   		u32 max_bpp_bigjoiner =
>> -			i915->display.cdclk.max_cdclk_freq * 48 /
>> +			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>   			intel_dp_mode_to_fec_clock(mode_clock);
> Hmm. Why is this using the FEC adjusted clock here?

AFAIU we always use FEC for DP when DSC is enabled, provided sink and 
source both support FEC.

Bspec mentions : For cases where FEC is enabled, pixel clock is replaced 
by pixel clock/0.972261 in the above calculations.

But it seems in this whole function we need a check for 
intel_dp_supports_fec() before using adjusted value.

Regards,

Ankit

>
>>   
>>   		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>> -- 
>> 2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Nautiyal, Ankit K]

On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
> On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
>> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
>>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
>>>> As per Bspec, Big Joiner BW check is:
>>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
>>>> Pixel clock
>>>>
>>>> Currently we always use max_cdclk in the check for both modevalid
>>>> and compute config steps.
>>>>
>>>> During modevalid use max_cdclk_freq for the check.
>>>> During compute config step use current cdclk for the check.
>>> Nak. cdclk is computed much later based on what is actually needed.
>>> The cdclk freq you are using here is essentially a random number.
>> Oh I didn't realise that, perhaps I was lucky when I tested this.
>>
>> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
>>
>> If it doesnt then, we might have a compressed_bpp value, that might be
>> violating the big joiner bw check.
>>
>> Should this be handled while computing cdclk?
> Yes. I suggest adding something like intel_vdsc_min_cdclk() that
> handles all of it.


I can try that out.

Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency * 
Number of pipes joined, which seems to be missing.

So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of 
pipes joined)

In addition, as per bigjoiner check it should be >= compressed_bpp / 
(PPC * bigjoiner interface bits).

Regards,

Ankit

>> Regards,
>>
>> Ankit
>>
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>>>>    drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>>>>    3 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index 3fe651a8f5d0..d6600de1ab49 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>    				u32 link_clock, u32 lane_count,
>>>>    				u32 mode_clock, u32 mode_hdisplay,
>>>> +				unsigned int cdclk,
>>>>    				bool bigjoiner,
>>>>    				u32 pipe_bpp,
>>>>    				u32 timeslots)
>>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>    
>>>>    	if (bigjoiner) {
>>>>    		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
>>>> -		u32 max_bpp_bigjoiner =
>>>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>>> -			intel_dp_mode_to_fec_clock(mode_clock);
>>>> +
>>>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
>>>> +					intel_dp_mode_to_fec_clock(mode_clock);
>>>>    
>>>>    		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>>>>    	}
>>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>>>>    							    max_lanes,
>>>>    							    target_clock,
>>>>    							    mode->hdisplay,
>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>    							    bigjoiner,
>>>>    							    pipe_bpp, 64) >> 4;
>>>>    			dsc_slice_count =
>>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>>    							    pipe_config->lane_count,
>>>>    							    adjusted_mode->crtc_clock,
>>>>    							    adjusted_mode->crtc_hdisplay,
>>>> +							    dev_priv->display.cdclk.hw.cdclk,
>>>>    							    pipe_config->bigjoiner_pipes,
>>>>    							    pipe_bpp,
>>>>    							    timeslots);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>>>> index ef39e4f7a329..d150bfe8abf4 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>    				u32 link_clock, u32 lane_count,
>>>>    				u32 mode_clock, u32 mode_hdisplay,
>>>> +				unsigned int cdclk,
>>>>    				bool bigjoiner,
>>>>    				u32 pipe_bpp,
>>>>    				u32 timeslots);
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> index a860cbc5dbea..266e31b78729 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>>>    							    max_lanes,
>>>>    							    target_clock,
>>>>    							    mode->hdisplay,
>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>    							    bigjoiner,
>>>>    							    pipe_bpp, 64) >> 4;
>>>>    			dsc_slice_count =
>>>> -- 
>>>> 2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
> > On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
> >> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> >>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> >>>> As per Bspec, Big Joiner BW check is:
> >>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> >>>> Pixel clock
> >>>>
> >>>> Currently we always use max_cdclk in the check for both modevalid
> >>>> and compute config steps.
> >>>>
> >>>> During modevalid use max_cdclk_freq for the check.
> >>>> During compute config step use current cdclk for the check.
> >>> Nak. cdclk is computed much later based on what is actually needed.
> >>> The cdclk freq you are using here is essentially a random number.
> >> Oh I didn't realise that, perhaps I was lucky when I tested this.
> >>
> >> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
> >>
> >> If it doesnt then, we might have a compressed_bpp value, that might be
> >> violating the big joiner bw check.
> >>
> >> Should this be handled while computing cdclk?
> > Yes. I suggest adding something like intel_vdsc_min_cdclk() that
> > handles all of it.
> 
> 
> I can try that out.
> 
> Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency * 
> Number of pipes joined, which seems to be missing.

That is already accounted for in the pixel rate.

> 
> So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of 
> pipes joined)
> 
> In addition, as per bigjoiner check it should be >= compressed_bpp / 
> (PPC * bigjoiner interface bits).
> 
> Regards,
> 
> Ankit
> 
> >> Regards,
> >>
> >> Ankit
> >>
> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
> >>>>    drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
> >>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
> >>>>    3 files changed, 8 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> index 3fe651a8f5d0..d6600de1ab49 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
> >>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>    				u32 link_clock, u32 lane_count,
> >>>>    				u32 mode_clock, u32 mode_hdisplay,
> >>>> +				unsigned int cdclk,
> >>>>    				bool bigjoiner,
> >>>>    				u32 pipe_bpp,
> >>>>    				u32 timeslots)
> >>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>    
> >>>>    	if (bigjoiner) {
> >>>>    		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
> >>>> -		u32 max_bpp_bigjoiner =
> >>>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> >>>> -			intel_dp_mode_to_fec_clock(mode_clock);
> >>>> +
> >>>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
> >>>> +					intel_dp_mode_to_fec_clock(mode_clock);
> >>>>    
> >>>>    		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> >>>>    	}
> >>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> >>>>    							    max_lanes,
> >>>>    							    target_clock,
> >>>>    							    mode->hdisplay,
> >>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
> >>>>    							    bigjoiner,
> >>>>    							    pipe_bpp, 64) >> 4;
> >>>>    			dsc_slice_count =
> >>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >>>>    							    pipe_config->lane_count,
> >>>>    							    adjusted_mode->crtc_clock,
> >>>>    							    adjusted_mode->crtc_hdisplay,
> >>>> +							    dev_priv->display.cdclk.hw.cdclk,
> >>>>    							    pipe_config->bigjoiner_pipes,
> >>>>    							    pipe_bpp,
> >>>>    							    timeslots);
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> >>>> index ef39e4f7a329..d150bfe8abf4 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> >>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
> >>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>    				u32 link_clock, u32 lane_count,
> >>>>    				u32 mode_clock, u32 mode_hdisplay,
> >>>> +				unsigned int cdclk,
> >>>>    				bool bigjoiner,
> >>>>    				u32 pipe_bpp,
> >>>>    				u32 timeslots);
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>> index a860cbc5dbea..266e31b78729 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> >>>>    							    max_lanes,
> >>>>    							    target_clock,
> >>>>    							    mode->hdisplay,
> >>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
> >>>>    							    bigjoiner,
> >>>>    							    pipe_bpp, 64) >> 4;
> >>>>    			dsc_slice_count =
> >>>> -- 
> >>>> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 04:53:13PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/29/2023 4:24 PM, Ville Syrjälä wrote:
> > On Wed, Mar 29, 2023 at 02:37:45PM +0530, Ankit Nautiyal wrote:
> >> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
> >> DISPLAY > 13 is 36 (Bspec: 49259).
> >>
> >> v2: Corrected Display ver to 13.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index da1c00ee92fb..0b59c1e53678 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -756,8 +756,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>   	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
> >>   
> >>   	if (bigjoiner) {
> >> +		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 13 ? 24 : 36;
> > We generally prefer "new -> old" order. So please flip that around.
> 
> Noted. Will do this in next version.
> 
> 
> >
> >>   		u32 max_bpp_bigjoiner =
> >> -			i915->display.cdclk.max_cdclk_freq * 48 /
> >> +			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> >>   			intel_dp_mode_to_fec_clock(mode_clock);
> > Hmm. Why is this using the FEC adjusted clock here?
> 
> AFAIU we always use FEC for DP when DSC is enabled, provided sink and 
> source both support FEC.
> 
> Bspec mentions : For cases where FEC is enabled, pixel clock is replaced 
> by pixel clock/0.972261 in the above calculations.

That doesn't make sense to me. FEC happens in the PHY
layer, DSC compression happens earlier.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Lisovskiy, Stanislav]

On Wed, Mar 29, 2023 at 02:35:38PM +0300, Ville Syrjälä wrote:
> On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
> > 
> > On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
> > > On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
> > >> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> > >>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> > >>>> As per Bspec, Big Joiner BW check is:
> > >>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> > >>>> Pixel clock
> > >>>>
> > >>>> Currently we always use max_cdclk in the check for both modevalid
> > >>>> and compute config steps.
> > >>>>
> > >>>> During modevalid use max_cdclk_freq for the check.
> > >>>> During compute config step use current cdclk for the check.
> > >>> Nak. cdclk is computed much later based on what is actually needed.
> > >>> The cdclk freq you are using here is essentially a random number.
> > >> Oh I didn't realise that, perhaps I was lucky when I tested this.
> > >>
> > >> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
> > >>

We use max_cdclk_freq basically as a "hack" to estimate what could be the max
amount of the CDCLK, because for the reasons, Ville mentioned, we can't use
CDCLK directly here, because it hasn't been yet calculated.

However we anyway know CDCLK will be aligned accordingly to pixel rate.

> > >> If it doesnt then, we might have a compressed_bpp value, that might be
> > >> violating the big joiner bw check.
> > >>
> > >> Should this be handled while computing cdclk?
> > > Yes. I suggest adding something like intel_vdsc_min_cdclk() that
> > > handles all of it.
> > 
> > 
> > I can try that out.

It is all again about that same chicken&egg problem. 
Our paradigm is that CDCLK is the last thing that we calculate, however that
check instructs us to choose the output bpp which obeys

Output bpp <= PPC * CDCLK frequency * Big joiner interface bits / pixel clock

rule.

If we choose to adjust CDCLK accordingly, we loose an option to actually change
the ourpur bpp to save the power, because theoretically we could avoid increasing
CDCLK to match that rule, by decreasing the output bpp..

So this kinda leads us to possibly waste more power.

Stan

> > 
> > Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency * 
> > Number of pipes joined, which seems to be missing.
> 
> That is already accounted for in the pixel rate.
> 
> > 
> > So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of 
> > pipes joined)
> > 
> > In addition, as per bigjoiner check it should be >= compressed_bpp / 
> > (PPC * bigjoiner interface bits).
> > 
> > Regards,
> > 
> > Ankit
> > 
> > >> Regards,
> > >>
> > >> Ankit
> > >>
> > >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > >>>> ---
> > >>>>    drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
> > >>>>    drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
> > >>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
> > >>>>    3 files changed, 8 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > >>>> index 3fe651a8f5d0..d6600de1ab49 100644
> > >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > >>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
> > >>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> > >>>>    				u32 link_clock, u32 lane_count,
> > >>>>    				u32 mode_clock, u32 mode_hdisplay,
> > >>>> +				unsigned int cdclk,
> > >>>>    				bool bigjoiner,
> > >>>>    				u32 pipe_bpp,
> > >>>>    				u32 timeslots)
> > >>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> > >>>>    
> > >>>>    	if (bigjoiner) {
> > >>>>    		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
> > >>>> -		u32 max_bpp_bigjoiner =
> > >>>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> > >>>> -			intel_dp_mode_to_fec_clock(mode_clock);
> > >>>> +
> > >>>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
> > >>>> +					intel_dp_mode_to_fec_clock(mode_clock);
> > >>>>    
> > >>>>    		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> > >>>>    	}
> > >>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> > >>>>    							    max_lanes,
> > >>>>    							    target_clock,
> > >>>>    							    mode->hdisplay,
> > >>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
> > >>>>    							    bigjoiner,
> > >>>>    							    pipe_bpp, 64) >> 4;
> > >>>>    			dsc_slice_count =
> > >>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >>>>    							    pipe_config->lane_count,
> > >>>>    							    adjusted_mode->crtc_clock,
> > >>>>    							    adjusted_mode->crtc_hdisplay,
> > >>>> +							    dev_priv->display.cdclk.hw.cdclk,
> > >>>>    							    pipe_config->bigjoiner_pipes,
> > >>>>    							    pipe_bpp,
> > >>>>    							    timeslots);
> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > >>>> index ef39e4f7a329..d150bfe8abf4 100644
> > >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > >>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
> > >>>>    u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> > >>>>    				u32 link_clock, u32 lane_count,
> > >>>>    				u32 mode_clock, u32 mode_hdisplay,
> > >>>> +				unsigned int cdclk,
> > >>>>    				bool bigjoiner,
> > >>>>    				u32 pipe_bpp,
> > >>>>    				u32 timeslots);
> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > >>>> index a860cbc5dbea..266e31b78729 100644
> > >>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > >>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> > >>>>    							    max_lanes,
> > >>>>    							    target_clock,
> > >>>>    							    mode->hdisplay,
> > >>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
> > >>>>    							    bigjoiner,
> > >>>>    							    pipe_bpp, 64) >> 4;
> > >>>>    			dsc_slice_count =
> > >>>> -- 
> > >>>> 2.25.1
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Ville Syrjälä]

On Wed, Mar 29, 2023 at 04:44:12PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, Mar 29, 2023 at 02:35:38PM +0300, Ville Syrjälä wrote:
> > On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
> > > 
> > > On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
> > > > On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
> > > >> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> > > >>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> > > >>>> As per Bspec, Big Joiner BW check is:
> > > >>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> > > >>>> Pixel clock
> > > >>>>
> > > >>>> Currently we always use max_cdclk in the check for both modevalid
> > > >>>> and compute config steps.
> > > >>>>
> > > >>>> During modevalid use max_cdclk_freq for the check.
> > > >>>> During compute config step use current cdclk for the check.
> > > >>> Nak. cdclk is computed much later based on what is actually needed.
> > > >>> The cdclk freq you are using here is essentially a random number.
> > > >> Oh I didn't realise that, perhaps I was lucky when I tested this.
> > > >>
> > > >> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
> > > >>
> 
> We use max_cdclk_freq basically as a "hack" to estimate what could be the max
> amount of the CDCLK, because for the reasons, Ville mentioned, we can't use
> CDCLK directly here, because it hasn't been yet calculated.
> 
> However we anyway know CDCLK will be aligned accordingly to pixel rate.
> 
> > > >> If it doesnt then, we might have a compressed_bpp value, that might be
> > > >> violating the big joiner bw check.
> > > >>
> > > >> Should this be handled while computing cdclk?
> > > > Yes. I suggest adding something like intel_vdsc_min_cdclk() that
> > > > handles all of it.
> > > 
> > > 
> > > I can try that out.
> 
> It is all again about that same chicken&egg problem. 
> Our paradigm is that CDCLK is the last thing that we calculate, however that
> check instructs us to choose the output bpp which obeys
> 
> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits / pixel clock
> 
> rule.
> 
> If we choose to adjust CDCLK accordingly, we loose an option to actually change
> the ourpur bpp to save the power, because theoretically we could avoid increasing
> CDCLK to match that rule, by decreasing the output bpp..
> 
> So this kinda leads us to possibly waste more power.

The main questions in selecting the bpp are what kind of
quality is achievable and/or acceptable. The rest (link
rate/cdclk/etc.) are all derived based on that. Doing it
the other way around would essentially result in 
non-deterministic behaviour.

Currently the only way to affect the quality criteria
is the max_bpc property which isn't really properly defined
when it comes to compression. Also it just specifies the
max, not the the min (or what the user would consider
acceptable).

Every now and then I muse about introducing some kind
of abstract "power vs. quality/performance" knob that would
give the driver better hints as to which the user values more.
With something like that we could try to reduce the quality
in places to achieve better power savings.

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✗ Fi.CI.BUILD: failure for Update DSC Bigjoiner BW check (rev2)

[Patchwork]

== Series Details ==

Series: Update DSC Bigjoiner BW check (rev2)
URL   : https://patchwork.freedesktop.org/series/115773/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/115773/revisions/2/mbox/ not applied
Applying: drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp
Applying: drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/display/intel_dp.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check
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

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Nautiyal, Ankit K]

On 3/29/2023 7:14 PM, Lisovskiy, Stanislav wrote:
> On Wed, Mar 29, 2023 at 02:35:38PM +0300, Ville Syrjälä wrote:
>> On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
>>> On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
>>>> On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
>>>>> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
>>>>>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
>>>>>>> As per Bspec, Big Joiner BW check is:
>>>>>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
>>>>>>> Pixel clock
>>>>>>>
>>>>>>> Currently we always use max_cdclk in the check for both modevalid
>>>>>>> and compute config steps.
>>>>>>>
>>>>>>> During modevalid use max_cdclk_freq for the check.
>>>>>>> During compute config step use current cdclk for the check.
>>>>>> Nak. cdclk is computed much later based on what is actually needed.
>>>>>> The cdclk freq you are using here is essentially a random number.
>>>>> Oh I didn't realise that, perhaps I was lucky when I tested this.
>>>>>
>>>>> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
>>>>>
> We use max_cdclk_freq basically as a "hack" to estimate what could be the max
> amount of the CDCLK, because for the reasons, Ville mentioned, we can't use
> CDCLK directly here, because it hasn't been yet calculated.
>
> However we anyway know CDCLK will be aligned accordingly to pixel rate.
>
>>>>> If it doesnt then, we might have a compressed_bpp value, that might be
>>>>> violating the big joiner bw check.
>>>>>
>>>>> Should this be handled while computing cdclk?
>>>> Yes. I suggest adding something like intel_vdsc_min_cdclk() that
>>>> handles all of it.
>>>
>>> I can try that out.
> It is all again about that same chicken&egg problem.
> Our paradigm is that CDCLK is the last thing that we calculate, however that
> check instructs us to choose the output bpp which obeys
>
> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits / pixel clock
>
> rule.
>
> If we choose to adjust CDCLK accordingly, we loose an option to actually change
> the ourpur bpp to save the power, because theoretically we could avoid increasing
> CDCLK to match that rule, by decreasing the output bpp..
>
> So this kinda leads us to possibly waste more power.

I understand there is a tradeoff of power and performance, and what we 
are trying to do is to set the maximum compressed bpp (to have minimum 
compression).

But there is a possibility with bigjoiner case, where we set the 
compressed_bpp as per max_cdclk_freq, but the check fails as 
compressed_bpp is too high for the computed cdclk as per the Bigjoiner 
BW check.

This might not be faced every time bigjoiner + dsc are involved, as 
there are link_bw_check, and other dsc checks that might limit the 
compressed_bpp and the computed cdclk is sufficient.

However, there are cases where the bigjoiner check is actually failing, 
and resulting in pipe fifo underruns. I have seen in my testing with an 
8k setup via PCON->HDMI2.1:

For 8k@30 the compressed_bpp is set to 21 which according to 
max_cdclk_freq is fine, but the with actual cdclock of 307200 KHz, the 
compressed_bpp of 21 bpp is getting too much, resulting is bigjoiner bw 
check failure, causing underruns and no-display.

For 8k@60 we do not see this issue, as the compressed_bpp is limited by 
link bandwidth check to 10bpp, and also the cdclock is computed is 
actually max_cdclk_freq.

For 4k@120 again we see compressed_bpp is set to 21, and cdclk computed 
to be max_cdclk_freq so the check doesnt fail.

So IMHO, we do need a check to avoid the issue mentioned above.

I think let the existing check in dp_get_output_bpp remain as it is, 
with max_cdclk_freq. This will help to get an acceptable compressed_bpp, 
and if other dsc constraint come into play we will get a lesser value 
for the bpp.

Let’s add a check in intel_crtc_compute_min_cdclk  : 
intel_vdsc_min_cdclk() (as suggested by Ville), where, if bigjoiner is 
used, update the min_cdclk to accommodate the compressed_bpp. In worst 
case we have to use max_cdclk_freq.

I have tested something like this with the above mentioned setup. The 
compressed_bpp is set as 21 as before, but the cdclck computed is 556800 
KHz, which enough to honor the check, and is still less than the max 
cdclk of 652800 KHz.

If this approach makes sense, I can float the changes.

Regards,

Ankit


>
> Stan
>
>>> Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency *
>>> Number of pipes joined, which seems to be missing.
>> That is already accounted for in the pixel rate.
>>
>>> So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of
>>> pipes joined)
>>>
>>> In addition, as per bigjoiner check it should be >= compressed_bpp /
>>> (PPC * bigjoiner interface bits).
>>>
>>> Regards,
>>>
>>> Ankit
>>>
>>>>> Regards,
>>>>>
>>>>> Ankit
>>>>>
>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>>>>>>>     drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>>>>>>>     drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>>>>>>>     3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>> index 3fe651a8f5d0..d6600de1ab49 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>>>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>     				u32 link_clock, u32 lane_count,
>>>>>>>     				u32 mode_clock, u32 mode_hdisplay,
>>>>>>> +				unsigned int cdclk,
>>>>>>>     				bool bigjoiner,
>>>>>>>     				u32 pipe_bpp,
>>>>>>>     				u32 timeslots)
>>>>>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>     
>>>>>>>     	if (bigjoiner) {
>>>>>>>     		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
>>>>>>> -		u32 max_bpp_bigjoiner =
>>>>>>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>>>>>> -			intel_dp_mode_to_fec_clock(mode_clock);
>>>>>>> +
>>>>>>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
>>>>>>> +					intel_dp_mode_to_fec_clock(mode_clock);
>>>>>>>     
>>>>>>>     		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>>>>>>>     	}
>>>>>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>>>>>>>     							    max_lanes,
>>>>>>>     							    target_clock,
>>>>>>>     							    mode->hdisplay,
>>>>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>>     							    bigjoiner,
>>>>>>>     							    pipe_bpp, 64) >> 4;
>>>>>>>     			dsc_slice_count =
>>>>>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>>>>>     							    pipe_config->lane_count,
>>>>>>>     							    adjusted_mode->crtc_clock,
>>>>>>>     							    adjusted_mode->crtc_hdisplay,
>>>>>>> +							    dev_priv->display.cdclk.hw.cdclk,
>>>>>>>     							    pipe_config->bigjoiner_pipes,
>>>>>>>     							    pipe_bpp,
>>>>>>>     							    timeslots);
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>> index ef39e4f7a329..d150bfe8abf4 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>>>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>     				u32 link_clock, u32 lane_count,
>>>>>>>     				u32 mode_clock, u32 mode_hdisplay,
>>>>>>> +				unsigned int cdclk,
>>>>>>>     				bool bigjoiner,
>>>>>>>     				u32 pipe_bpp,
>>>>>>>     				u32 timeslots);
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>> index a860cbc5dbea..266e31b78729 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>>>>>>     							    max_lanes,
>>>>>>>     							    target_clock,
>>>>>>>     							    mode->hdisplay,
>>>>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>>     							    bigjoiner,
>>>>>>>     							    pipe_bpp, 64) >> 4;
>>>>>>>     			dsc_slice_count =
>>>>>>> -- 
>>>>>>> 2.25.1
>> -- 
>> Ville Syrjälä
>> Intel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Nautiyal, Ankit K]

On 3/29/2023 5:05 PM, Ville Syrjälä wrote:
> On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
>> On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
>>> On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
>>>> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
>>>>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
>>>>>> As per Bspec, Big Joiner BW check is:
>>>>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
>>>>>> Pixel clock
>>>>>>
>>>>>> Currently we always use max_cdclk in the check for both modevalid
>>>>>> and compute config steps.
>>>>>>
>>>>>> During modevalid use max_cdclk_freq for the check.
>>>>>> During compute config step use current cdclk for the check.
>>>>> Nak. cdclk is computed much later based on what is actually needed.
>>>>> The cdclk freq you are using here is essentially a random number.
>>>> Oh I didn't realise that, perhaps I was lucky when I tested this.
>>>>
>>>> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
>>>>
>>>> If it doesnt then, we might have a compressed_bpp value, that might be
>>>> violating the big joiner bw check.
>>>>
>>>> Should this be handled while computing cdclk?
>>> Yes. I suggest adding something like intel_vdsc_min_cdclk() that
>>> handles all of it.
>>
>> I can try that out.
>>
>> Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency *
>> Number of pipes joined, which seems to be missing.
> That is already accounted for in the pixel rate.

Indeed, will club this check and the other bigjoiner_bw check in 
intel_vdsc_min_cdclk, if the approach mentioned in the other mail is 
acceptable.

Regards,

Ankit

>
>> So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of
>> pipes joined)
>>
>> In addition, as per bigjoiner check it should be >= compressed_bpp /
>> (PPC * bigjoiner interface bits).
>>
>> Regards,
>>
>> Ankit
>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>>>>>>     drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>>>>>>     drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>>>>>>     3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>> index 3fe651a8f5d0..d6600de1ab49 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>     				u32 link_clock, u32 lane_count,
>>>>>>     				u32 mode_clock, u32 mode_hdisplay,
>>>>>> +				unsigned int cdclk,
>>>>>>     				bool bigjoiner,
>>>>>>     				u32 pipe_bpp,
>>>>>>     				u32 timeslots)
>>>>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>     
>>>>>>     	if (bigjoiner) {
>>>>>>     		int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
>>>>>> -		u32 max_bpp_bigjoiner =
>>>>>> -			i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>>>>> -			intel_dp_mode_to_fec_clock(mode_clock);
>>>>>> +
>>>>>> +		u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
>>>>>> +					intel_dp_mode_to_fec_clock(mode_clock);
>>>>>>     
>>>>>>     		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>>>>>>     	}
>>>>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>>>>>>     							    max_lanes,
>>>>>>     							    target_clock,
>>>>>>     							    mode->hdisplay,
>>>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>     							    bigjoiner,
>>>>>>     							    pipe_bpp, 64) >> 4;
>>>>>>     			dsc_slice_count =
>>>>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>>>>     							    pipe_config->lane_count,
>>>>>>     							    adjusted_mode->crtc_clock,
>>>>>>     							    adjusted_mode->crtc_hdisplay,
>>>>>> +							    dev_priv->display.cdclk.hw.cdclk,
>>>>>>     							    pipe_config->bigjoiner_pipes,
>>>>>>     							    pipe_bpp,
>>>>>>     							    timeslots);
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>> index ef39e4f7a329..d150bfe8abf4 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>     				u32 link_clock, u32 lane_count,
>>>>>>     				u32 mode_clock, u32 mode_hdisplay,
>>>>>> +				unsigned int cdclk,
>>>>>>     				bool bigjoiner,
>>>>>>     				u32 pipe_bpp,
>>>>>>     				u32 timeslots);
>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>> index a860cbc5dbea..266e31b78729 100644
>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>>>>>     							    max_lanes,
>>>>>>     							    target_clock,
>>>>>>     							    mode->hdisplay,
>>>>>> +							    dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>     							    bigjoiner,
>>>>>>     							    pipe_bpp, 64) >> 4;
>>>>>>     			dsc_slice_count =
>>>>>> -- 
>>>>>> 2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Manasi Navare]

On Thu, Mar 30, 2023 at 4:11 AM Nautiyal, Ankit K
<ankit.k.nautiyal@intel.com> wrote:
>
>
> On 3/29/2023 5:05 PM, Ville Syrjälä wrote:
> > On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
> >> On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
> >>> On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
> >>>> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
> >>>>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
> >>>>>> As per Bspec, Big Joiner BW check is:
> >>>>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
> >>>>>> Pixel clock
> >>>>>>
> >>>>>> Currently we always use max_cdclk in the check for both modevalid
> >>>>>> and compute config steps.
> >>>>>>
> >>>>>> During modevalid use max_cdclk_freq for the check.
> >>>>>> During compute config step use current cdclk for the check.
> >>>>> Nak. cdclk is computed much later based on what is actually needed.
> >>>>> The cdclk freq you are using here is essentially a random number.
> >>>> Oh I didn't realise that, perhaps I was lucky when I tested this.
> >>>>
> >>>> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
> >>>>
> >>>> If it doesnt then, we might have a compressed_bpp value, that might be
> >>>> violating the big joiner bw check.
> >>>>
> >>>> Should this be handled while computing cdclk?
> >>> Yes. I suggest adding something like intel_vdsc_min_cdclk() that
> >>> handles all of it.
> >>
> >> I can try that out.
> >>
> >> Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency *
> >> Number of pipes joined, which seems to be missing.
> > That is already accounted for in the pixel rate.
>
> Indeed, will club this check and the other bigjoiner_bw check in
> intel_vdsc_min_cdclk, if the approach mentioned in the other mail is
> acceptable.
>
> Regards,
>
> Ankit

Hi Ankit,

Yes I think it makes sense to add the vdsc_min_cdclk check while
computing the min cdclk.
This will hopefully let us exercise all allowed compressed bpps all
the way to 27, which have caused failures earlier
possibly due to the cdcclk being reduced beyond the pipe bw required
with the max compressed bpp that we use.
So best would be to check against vdsc_mind_cdclk required if dsc is
enabled for that configuration, if not then skip
the check

Regards
Manasi
>
> >
> >> So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of
> >> pipes joined)
> >>
> >> In addition, as per bigjoiner check it should be >= compressed_bpp /
> >> (PPC * bigjoiner interface bits).
> >>
> >> Regards,
> >>
> >> Ankit
> >>
> >>>> Regards,
> >>>>
> >>>> Ankit
> >>>>
> >>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
> >>>>>>     drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
> >>>>>>     drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
> >>>>>>     3 files changed, 8 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>>>> index 3fe651a8f5d0..d6600de1ab49 100644
> >>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >>>>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
> >>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>>>                                  u32 link_clock, u32 lane_count,
> >>>>>>                                  u32 mode_clock, u32 mode_hdisplay,
> >>>>>> +                                unsigned int cdclk,
> >>>>>>                                  bool bigjoiner,
> >>>>>>                                  u32 pipe_bpp,
> >>>>>>                                  u32 timeslots)
> >>>>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>>>
> >>>>>>          if (bigjoiner) {
> >>>>>>                  int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
> >>>>>> -                u32 max_bpp_bigjoiner =
> >>>>>> -                        i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
> >>>>>> -                        intel_dp_mode_to_fec_clock(mode_clock);
> >>>>>> +
> >>>>>> +                u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
> >>>>>> +                                        intel_dp_mode_to_fec_clock(mode_clock);
> >>>>>>
> >>>>>>                  bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> >>>>>>          }
> >>>>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> >>>>>>                                                              max_lanes,
> >>>>>>                                                              target_clock,
> >>>>>>                                                              mode->hdisplay,
> >>>>>> +                                                            dev_priv->display.cdclk.max_cdclk_freq,
> >>>>>>                                                              bigjoiner,
> >>>>>>                                                              pipe_bpp, 64) >> 4;
> >>>>>>                          dsc_slice_count =
> >>>>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >>>>>>                                                              pipe_config->lane_count,
> >>>>>>                                                              adjusted_mode->crtc_clock,
> >>>>>>                                                              adjusted_mode->crtc_hdisplay,
> >>>>>> +                                                            dev_priv->display.cdclk.hw.cdclk,
> >>>>>>                                                              pipe_config->bigjoiner_pipes,
> >>>>>>                                                              pipe_bpp,
> >>>>>>                                                              timeslots);
> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> >>>>>> index ef39e4f7a329..d150bfe8abf4 100644
> >>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> >>>>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
> >>>>>>     u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
> >>>>>>                                  u32 link_clock, u32 lane_count,
> >>>>>>                                  u32 mode_clock, u32 mode_hdisplay,
> >>>>>> +                                unsigned int cdclk,
> >>>>>>                                  bool bigjoiner,
> >>>>>>                                  u32 pipe_bpp,
> >>>>>>                                  u32 timeslots);
> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>>>> index a860cbc5dbea..266e31b78729 100644
> >>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >>>>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> >>>>>>                                                              max_lanes,
> >>>>>>                                                              target_clock,
> >>>>>>                                                              mode->hdisplay,
> >>>>>> +                                                            dev_priv->display.cdclk.max_cdclk_freq,
> >>>>>>                                                              bigjoiner,
> >>>>>>                                                              pipe_bpp, 64) >> 4;
> >>>>>>                          dsc_slice_count =
> >>>>>> --
> >>>>>> 2.25.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Use current cdclk for DSC Bigjoiner BW check

[Nautiyal, Ankit K]

On 4/4/2023 3:03 AM, Manasi Navare wrote:
> On Thu, Mar 30, 2023 at 4:11 AM Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com> wrote:
>>
>> On 3/29/2023 5:05 PM, Ville Syrjälä wrote:
>>> On Wed, Mar 29, 2023 at 05:00:55PM +0530, Nautiyal, Ankit K wrote:
>>>> On 3/29/2023 4:23 PM, Ville Syrjälä wrote:
>>>>> On Wed, Mar 29, 2023 at 04:06:21PM +0530, Nautiyal, Ankit K wrote:
>>>>>> On 3/29/2023 3:27 PM, Ville Syrjälä wrote:
>>>>>>> On Wed, Mar 29, 2023 at 02:14:49PM +0530, Ankit Nautiyal wrote:
>>>>>>>> As per Bspec, Big Joiner BW check is:
>>>>>>>> Output bpp <= PPC * CDCLK frequency * Big joiner interface bits /
>>>>>>>> Pixel clock
>>>>>>>>
>>>>>>>> Currently we always use max_cdclk in the check for both modevalid
>>>>>>>> and compute config steps.
>>>>>>>>
>>>>>>>> During modevalid use max_cdclk_freq for the check.
>>>>>>>> During compute config step use current cdclk for the check.
>>>>>>> Nak. cdclk is computed much later based on what is actually needed.
>>>>>>> The cdclk freq you are using here is essentially a random number.
>>>>>> Oh I didn't realise that, perhaps I was lucky when I tested this.
>>>>>>
>>>>>> So this check where CDCLK is mentioned, actually expects max_cdclk_freq?
>>>>>>
>>>>>> If it doesnt then, we might have a compressed_bpp value, that might be
>>>>>> violating the big joiner bw check.
>>>>>>
>>>>>> Should this be handled while computing cdclk?
>>>>> Yes. I suggest adding something like intel_vdsc_min_cdclk() that
>>>>> handles all of it.
>>>> I can try that out.
>>>>
>>>> Will also add *Pipe BW check*: Pixel clock < PPC * CDCLK frequency *
>>>> Number of pipes joined, which seems to be missing.
>>> That is already accounted for in the pixel rate.
>> Indeed, will club this check and the other bigjoiner_bw check in
>> intel_vdsc_min_cdclk, if the approach mentioned in the other mail is
>> acceptable.
>>
>> Regards,
>>
>> Ankit
> Hi Ankit,
>
> Yes I think it makes sense to add the vdsc_min_cdclk check while
> computing the min cdclk.
> This will hopefully let us exercise all allowed compressed bpps all
> the way to 27, which have caused failures earlier
> possibly due to the cdcclk being reduced beyond the pipe bw required
> with the max compressed bpp that we use.
> So best would be to check against vdsc_mind_cdclk required if dsc is
> enabled for that configuration, if not then skip
> the check
>
> Regards
> Manasi


Right, I will send a new patch with intel_vdsc_min_cdclk as discussed.

Will be good to have further discussion on that.

Thanks & Regards,

Ankit

>>>> So with pipe bw_check cdclk should be >  Pixel clock / (PPC * Number of
>>>> pipes joined)
>>>>
>>>> In addition, as per bigjoiner check it should be >= compressed_bpp /
>>>> (PPC * bigjoiner interface bits).
>>>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Ankit
>>>>>>
>>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/i915/display/intel_dp.c     | 9 ++++++---
>>>>>>>>      drivers/gpu/drm/i915/display/intel_dp.h     | 1 +
>>>>>>>>      drivers/gpu/drm/i915/display/intel_dp_mst.c | 1 +
>>>>>>>>      3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>>> index 3fe651a8f5d0..d6600de1ab49 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>>>>> @@ -711,6 +711,7 @@ u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, u32 p
>>>>>>>>      u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>>                                   u32 link_clock, u32 lane_count,
>>>>>>>>                                   u32 mode_clock, u32 mode_hdisplay,
>>>>>>>> +                                unsigned int cdclk,
>>>>>>>>                                   bool bigjoiner,
>>>>>>>>                                   u32 pipe_bpp,
>>>>>>>>                                   u32 timeslots)
>>>>>>>> @@ -757,9 +758,9 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>>
>>>>>>>>           if (bigjoiner) {
>>>>>>>>                   int bigjoiner_interface_bits = DISPLAY_VER(i915) <= 12 ? 24 : 36;
>>>>>>>> -                u32 max_bpp_bigjoiner =
>>>>>>>> -                        i915->display.cdclk.max_cdclk_freq * 2 * bigjoiner_interface_bits /
>>>>>>>> -                        intel_dp_mode_to_fec_clock(mode_clock);
>>>>>>>> +
>>>>>>>> +                u32 max_bpp_bigjoiner = cdclk * 2 * bigjoiner_interface_bits /
>>>>>>>> +                                        intel_dp_mode_to_fec_clock(mode_clock);
>>>>>>>>
>>>>>>>>                   bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>>>>>>>>           }
>>>>>>>> @@ -1073,6 +1074,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>>>>>>>>                                                               max_lanes,
>>>>>>>>                                                               target_clock,
>>>>>>>>                                                               mode->hdisplay,
>>>>>>>> +                                                            dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>>>                                                               bigjoiner,
>>>>>>>>                                                               pipe_bpp, 64) >> 4;
>>>>>>>>                           dsc_slice_count =
>>>>>>>> @@ -1580,6 +1582,7 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>>>>>>                                                               pipe_config->lane_count,
>>>>>>>>                                                               adjusted_mode->crtc_clock,
>>>>>>>>                                                               adjusted_mode->crtc_hdisplay,
>>>>>>>> +                                                            dev_priv->display.cdclk.hw.cdclk,
>>>>>>>>                                                               pipe_config->bigjoiner_pipes,
>>>>>>>>                                                               pipe_bpp,
>>>>>>>>                                                               timeslots);
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>>> index ef39e4f7a329..d150bfe8abf4 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>>>>>>>> @@ -106,6 +106,7 @@ int intel_dp_dsc_compute_bpp(struct intel_dp *intel_dp, u8 dsc_max_bpc);
>>>>>>>>      u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>>>>>>>                                   u32 link_clock, u32 lane_count,
>>>>>>>>                                   u32 mode_clock, u32 mode_hdisplay,
>>>>>>>> +                                unsigned int cdclk,
>>>>>>>>                                   bool bigjoiner,
>>>>>>>>                                   u32 pipe_bpp,
>>>>>>>>                                   u32 timeslots);
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>>> index a860cbc5dbea..266e31b78729 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>>>>>>> @@ -925,6 +925,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>>>>>>>>                                                               max_lanes,
>>>>>>>>                                                               target_clock,
>>>>>>>>                                                               mode->hdisplay,
>>>>>>>> +                                                            dev_priv->display.cdclk.max_cdclk_freq,
>>>>>>>>                                                               bigjoiner,
>>>>>>>>                                                               pipe_bpp, 64) >> 4;
>>>>>>>>                           dsc_slice_count =
>>>>>>>> --
>>>>>>>> 2.25.1