drm/i915/display: Enable second VDSC engine for higher moderates

Commit Message

Kulkarni, Vandita Sept. 13, 2021, 2:39 p.m. UTC

Each VDSC operates with 1ppc throughput, hence enable the second
VDSC engine when moderate is higher that the current cdclk.

Signed-off-by: Vandita Kulkarni <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Stanislav Lisovskiy Sept. 14, 2021, 7:19 a.m. UTC | #1

On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> Each VDSC operates with 1ppc throughput, hence enable the second
> VDSC engine when moderate is higher that the current cdclk.
> 
> Signed-off-by: Vandita Kulkarni <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 161c33b2c869..55878f65f724 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -70,6 +70,7 @@
>  #include "intel_tc.h"
>  #include "intel_vdsc.h"
>  #include "intel_vrr.h"
> +#include "intel_cdclk.h"
>  
>  #define DP_DPRX_ESI_LEN 14
>  
> @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				       struct drm_connector_state *conn_state,
>  				       struct link_config_limits *limits)
>  {
> +	struct intel_cdclk_state *cdclk_state;
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	const struct drm_display_mode *adjusted_mode =
>  		&pipe_config->hw.adjusted_mode;
> +	struct intel_atomic_state *state =
> +				to_intel_atomic_state(pipe_config->uapi.state);
>  	int pipe_bpp;
>  	int ret;
>  
> @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  		}
>  	}
>  
> +	cdclk_state = intel_atomic_get_cdclk_state(state);
> +	if (IS_ERR(cdclk_state))
> +		return PTR_ERR(cdclk_state);
> +
>  	/*
>  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> -	 * is greater than the maximum Cdclock and if slice count is even
> +	 * is greater than the current Cdclock and if slice count is even
>  	 * then we need to use 2 VDSC instances.
>  	 */
> -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||

So in the end, we didn't have to bump CDCLK up to get rid of that?

Anyways, checked with BSpec 49259, seems to make sense, was no point in
comparing to max CDCLK, which is not even currently used.

Reviewed-by: Stanislav Lisovskiy <[e-mail]>

>  	    pipe_config->bigjoiner) {
>  		if (pipe_config->dsc.slice_count < 2) {
>  			drm_dbg_kms(&dev_priv->drm,
> -- 
> 2.32.0
>

Ville Syrjälä Sept. 14, 2021, 7:28 a.m. UTC | #2

On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> Each VDSC operates with 1ppc throughput, hence enable the second
> VDSC engine when moderate is higher that the current cdclk.
> 
> Signed-off-by: Vandita Kulkarni <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 161c33b2c869..55878f65f724 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -70,6 +70,7 @@
>  #include "intel_tc.h"
>  #include "intel_vdsc.h"
>  #include "intel_vrr.h"
> +#include "intel_cdclk.h"
>  
>  #define DP_DPRX_ESI_LEN 14
>  
> @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				       struct drm_connector_state *conn_state,
>  				       struct link_config_limits *limits)
>  {
> +	struct intel_cdclk_state *cdclk_state;
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	const struct drm_display_mode *adjusted_mode =
>  		&pipe_config->hw.adjusted_mode;
> +	struct intel_atomic_state *state =
> +				to_intel_atomic_state(pipe_config->uapi.state);
>  	int pipe_bpp;
>  	int ret;
>  
> @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  		}
>  	}
>  
> +	cdclk_state = intel_atomic_get_cdclk_state(state);
> +	if (IS_ERR(cdclk_state))
> +		return PTR_ERR(cdclk_state);
> +
>  	/*
>  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> -	 * is greater than the maximum Cdclock and if slice count is even
> +	 * is greater than the current Cdclock and if slice count is even
>  	 * then we need to use 2 VDSC instances.
>  	 */
> -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||

This is wrong. We compute the cdclk based on the requirements of the
mode/etc., not the other way around.

>  	    pipe_config->bigjoiner) {
>  		if (pipe_config->dsc.slice_count < 2) {
>  			drm_dbg_kms(&dev_priv->drm,
> -- 
> 2.32.0

Kulkarni, Vandita Sept. 14, 2021, 7:31 a.m. UTC | #3

> -----Original Message-----
> From: Ville Syrjälä <[e-mail]>
> Sent: Tuesday, September 14, 2021 12:59 PM
> To: Kulkarni, Vandita <[e-mail]>
> Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> Navare, Manasi D <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> > engine when moderate is higher that the current cdclk.
> >
> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 161c33b2c869..55878f65f724 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -70,6 +70,7 @@
> >  #include "intel_tc.h"
> >  #include "intel_vdsc.h"
> >  #include "intel_vrr.h"
> > +#include "intel_cdclk.h"
> >
> >  #define DP_DPRX_ESI_LEN 14
> >
> > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  				       struct drm_connector_state *conn_state,
> >  				       struct link_config_limits *limits)  {
> > +	struct intel_cdclk_state *cdclk_state;
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> >base.base.dev);
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&pipe_config->hw.adjusted_mode;
> > +	struct intel_atomic_state *state =
> > +				to_intel_atomic_state(pipe_config-
> >uapi.state);
> >  	int pipe_bpp;
> >  	int ret;
> >
> > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  		}
> >  	}
> >
> > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > +	if (IS_ERR(cdclk_state))
> > +		return PTR_ERR(cdclk_state);
> > +
> >  	/*
> >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > -	 * is greater than the maximum Cdclock and if slice count is even
> > +	 * is greater than the current Cdclock and if slice count is even
> >  	 * then we need to use 2 VDSC instances.
> >  	 */
> > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> 
> This is wrong. We compute the cdclk based on the requirements of the
> mode/etc., not the other way around.

Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?

> 
> >  	    pipe_config->bigjoiner) {
> >  		if (pipe_config->dsc.slice_count < 2) {
> >  			drm_dbg_kms(&dev_priv->drm,
> > --
> > 2.32.0
> 
> --
> Ville Syrjälä
> Intel

Kulkarni, Vandita Sept. 14, 2021, 7:37 a.m. UTC | #4

> -----Original Message-----
> From: Lisovskiy, Stanislav <[e-mail]>
> Sent: Tuesday, September 14, 2021 12:49 PM
> To: Kulkarni, Vandita <[e-mail]>
> Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> Navare, Manasi D <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> > engine when moderate is higher that the current cdclk.
> >
> > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 161c33b2c869..55878f65f724 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -70,6 +70,7 @@
> >  #include "intel_tc.h"
> >  #include "intel_vdsc.h"
> >  #include "intel_vrr.h"
> > +#include "intel_cdclk.h"
> >
> >  #define DP_DPRX_ESI_LEN 14
> >
> > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  				       struct drm_connector_state *conn_state,
> >  				       struct link_config_limits *limits)  {
> > +	struct intel_cdclk_state *cdclk_state;
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> >base.base.dev);
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&pipe_config->hw.adjusted_mode;
> > +	struct intel_atomic_state *state =
> > +				to_intel_atomic_state(pipe_config-
> >uapi.state);
> >  	int pipe_bpp;
> >  	int ret;
> >
> > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> intel_dp *intel_dp,
> >  		}
> >  	}
> >
> > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > +	if (IS_ERR(cdclk_state))
> > +		return PTR_ERR(cdclk_state);
> > +
> >  	/*
> >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > -	 * is greater than the maximum Cdclock and if slice count is even
> > +	 * is greater than the current Cdclock and if slice count is even
> >  	 * then we need to use 2 VDSC instances.
> >  	 */
> > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> 
> So in the end, we didn't have to bump CDCLK up to get rid of that?

The solution that could fix the underruns was either of these:  set max cdclk that can drive this or enable the second dsc engine if slice count  > 2  to achieve 2ppc.

> 
> Anyways, checked with BSpec 49259, seems to make sense, was no point in
> comparing to max CDCLK, which is not even currently used.
> 
> Reviewed-by: Stanislav Lisovskiy <[e-mail]>
> 
> >  	    pipe_config->bigjoiner) {
> >  		if (pipe_config->dsc.slice_count < 2) {
> >  			drm_dbg_kms(&dev_priv->drm,
> > --
> > 2.32.0
> >

Ville Syrjälä Sept. 14, 2021, 7:48 a.m. UTC | #5

On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
> > -----Original Message-----
> > From: Ville Syrjälä <[e-mail]>
> > Sent: Tuesday, September 14, 2021 12:59 PM
> > To: Kulkarni, Vandita <[e-mail]>
> > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> > Navare, Manasi D <[e-mail]>
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > engine for higher moderates
> > 
> > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> > > engine when moderate is higher that the current cdclk.
> > >
> > > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 161c33b2c869..55878f65f724 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -70,6 +70,7 @@
> > >  #include "intel_tc.h"
> > >  #include "intel_vdsc.h"
> > >  #include "intel_vrr.h"
> > > +#include "intel_cdclk.h"
> > >
> > >  #define DP_DPRX_ESI_LEN 14
> > >
> > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> > intel_dp *intel_dp,
> > >  				       struct drm_connector_state *conn_state,
> > >  				       struct link_config_limits *limits)  {
> > > +	struct intel_cdclk_state *cdclk_state;
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > >base.base.dev);
> > >  	const struct drm_display_mode *adjusted_mode =
> > >  		&pipe_config->hw.adjusted_mode;
> > > +	struct intel_atomic_state *state =
> > > +				to_intel_atomic_state(pipe_config-
> > >uapi.state);
> > >  	int pipe_bpp;
> > >  	int ret;
> > >
> > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> > intel_dp *intel_dp,
> > >  		}
> > >  	}
> > >
> > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > > +	if (IS_ERR(cdclk_state))
> > > +		return PTR_ERR(cdclk_state);
> > > +
> > >  	/*
> > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > > -	 * is greater than the maximum Cdclock and if slice count is even
> > > +	 * is greater than the current Cdclock and if slice count is even
> > >  	 * then we need to use 2 VDSC instances.
> > >  	 */
> > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> > 
> > This is wrong. We compute the cdclk based on the requirements of the
> > mode/etc., not the other way around.
> 
> Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?

That seems like the easiest solution. Another option might be to come up
with some lower dotclock limit for the use of the second vdsc. But not
sure we know where the tipping point is wrt. powr consumption.

Stanislav Lisovskiy Sept. 14, 2021, 8:59 a.m. UTC | #6

On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä <[e-mail]>
> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > > To: Kulkarni, Vandita <[e-mail]>
> > > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> > > Navare, Manasi D <[e-mail]>
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > > engine for higher moderates
> > > 
> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> > > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> > > > engine when moderate is higher that the current cdclk.
> > > >
> > > > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 161c33b2c869..55878f65f724 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -70,6 +70,7 @@
> > > >  #include "intel_tc.h"
> > > >  #include "intel_vdsc.h"
> > > >  #include "intel_vrr.h"
> > > > +#include "intel_cdclk.h"
> > > >
> > > >  #define DP_DPRX_ESI_LEN 14
> > > >
> > > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> > > intel_dp *intel_dp,
> > > >  				       struct drm_connector_state *conn_state,
> > > >  				       struct link_config_limits *limits)  {
> > > > +	struct intel_cdclk_state *cdclk_state;
> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > > >base.base.dev);
> > > >  	const struct drm_display_mode *adjusted_mode =
> > > >  		&pipe_config->hw.adjusted_mode;
> > > > +	struct intel_atomic_state *state =
> > > > +				to_intel_atomic_state(pipe_config-
> > > >uapi.state);
> > > >  	int pipe_bpp;
> > > >  	int ret;
> > > >
> > > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> > > intel_dp *intel_dp,
> > > >  		}
> > > >  	}
> > > >
> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > > > +	if (IS_ERR(cdclk_state))
> > > > +		return PTR_ERR(cdclk_state);
> > > > +
> > > >  	/*
> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > > > -	 * is greater than the maximum Cdclock and if slice count is even
> > > > +	 * is greater than the current Cdclock and if slice count is even
> > > >  	 * then we need to use 2 VDSC instances.
> > > >  	 */
> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> > > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> > > 
> > > This is wrong. We compute the cdclk based on the requirements of the
> > > mode/etc., not the other way around.

According to BSpec guideline, we decide whether we enable or disable second VDSC engine, based
on that condition. As I understand that one is about DSC config calculation, based on CDCLK
which was calculated. 

If we bump up CDCLK, to avoid this, will we even then use a second VDSC ever?

Another thing is that probably enabling second VDSC is cheaper in terms of power consumption,
than bumping up the CDCLK.

Stan

> > 
> > Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?
> 
> That seems like the easiest solution. Another option might be to come up
> with some lower dotclock limit for the use of the second vdsc. But not
> sure we know where the tipping point is wrt. powr consumption.
> 
> -- 
> Ville Syrjälä
> Intel

Jani Nikula Sept. 14, 2021, 12:04 p.m. UTC | #7

On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]> wrote:
> On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
>> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
>> > > -----Original Message-----
>> > > From: Ville Syrjälä <[e-mail]>
>> > > Sent: Tuesday, September 14, 2021 12:59 PM
>> > > To: Kulkarni, Vandita <[e-mail]>
>> > > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
>> > > Navare, Manasi D <[e-mail]>
>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
>> > > engine for higher moderates
>> > > 
>> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
>> > > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
>> > > > engine when moderate is higher that the current cdclk.
>> > > >
>> > > > Signed-off-by: Vandita Kulkarni <[e-mail]>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
>> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > > index 161c33b2c869..55878f65f724 100644
>> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > > > @@ -70,6 +70,7 @@
>> > > >  #include "intel_tc.h"
>> > > >  #include "intel_vdsc.h"
>> > > >  #include "intel_vrr.h"
>> > > > +#include "intel_cdclk.h"
>> > > >
>> > > >  #define DP_DPRX_ESI_LEN 14
>> > > >
>> > > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
>> > > intel_dp *intel_dp,
>> > > >  				       struct drm_connector_state *conn_state,
>> > > >  				       struct link_config_limits *limits)  {
>> > > > +	struct intel_cdclk_state *cdclk_state;
>> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
>> > > >base.base.dev);
>> > > >  	const struct drm_display_mode *adjusted_mode =
>> > > >  		&pipe_config->hw.adjusted_mode;
>> > > > +	struct intel_atomic_state *state =
>> > > > +				to_intel_atomic_state(pipe_config-
>> > > >uapi.state);
>> > > >  	int pipe_bpp;
>> > > >  	int ret;
>> > > >
>> > > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
>> > > intel_dp *intel_dp,
>> > > >  		}
>> > > >  	}
>> > > >
>> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
>> > > > +	if (IS_ERR(cdclk_state))
>> > > > +		return PTR_ERR(cdclk_state);
>> > > > +
>> > > >  	/*
>> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
>> > > > -	 * is greater than the maximum Cdclock and if slice count is even
>> > > > +	 * is greater than the current Cdclock and if slice count is even
>> > > >  	 * then we need to use 2 VDSC instances.
>> > > >  	 */
>> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
>> > > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
>> > > 
>> > > This is wrong. We compute the cdclk based on the requirements of the
>> > > mode/etc., not the other way around.
>
> According to BSpec guideline, we decide whether we enable or disable second VDSC engine, based
> on that condition. As I understand that one is about DSC config calculation, based on CDCLK
> which was calculated. 

Point is, at the time compute_config gets called, what guarantees are
there that cdclk_state->actual.cdclk contains anything useful? This is
the design we have.

> If we bump up CDCLK, to avoid this, will we even then use a second VDSC ever?

I think we'll eventually need better logic than unconditionally bumping
to max, and it needs to take *both* the cdclk and the number of dsc
engines into account. The referenced bspec only has the vdsc clock
perspective, not overall perspective.

BR,
Jani.

> Another thing is that probably enabling second VDSC is cheaper in terms of power consumption,
> than bumping up the CDCLK.
>
> Stan
>
>> > 
>> > Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?
>> 
>> That seems like the easiest solution. Another option might be to come up
>> with some lower dotclock limit for the use of the second vdsc. But not
>> sure we know where the tipping point is wrt. powr consumption.
>> 
>> -- 
>> Ville Syrjälä
>> Intel

Stanislav Lisovskiy Sept. 14, 2021, 1:04 p.m. UTC | #8

On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]> wrote:
> > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
> >> > > -----Original Message-----
> >> > > From: Ville Syrjälä <[e-mail]>
> >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> >> > > To: Kulkarni, Vandita <[e-mail]>
> >> > > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> >> > > Navare, Manasi D <[e-mail]>
> >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> >> > > engine for higher moderates
> >> > > 
> >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> >> > > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> >> > > > engine when moderate is higher that the current cdclk.
> >> > > >
> >> > > > Signed-off-by: Vandita Kulkarni <[e-mail]>
> >> > > > ---
> >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > > > index 161c33b2c869..55878f65f724 100644
> >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > > > @@ -70,6 +70,7 @@
> >> > > >  #include "intel_tc.h"
> >> > > >  #include "intel_vdsc.h"
> >> > > >  #include "intel_vrr.h"
> >> > > > +#include "intel_cdclk.h"
> >> > > >
> >> > > >  #define DP_DPRX_ESI_LEN 14
> >> > > >
> >> > > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> >> > > intel_dp *intel_dp,
> >> > > >  				       struct drm_connector_state *conn_state,
> >> > > >  				       struct link_config_limits *limits)  {
> >> > > > +	struct intel_cdclk_state *cdclk_state;
> >> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> >> > > >base.base.dev);
> >> > > >  	const struct drm_display_mode *adjusted_mode =
> >> > > >  		&pipe_config->hw.adjusted_mode;
> >> > > > +	struct intel_atomic_state *state =
> >> > > > +				to_intel_atomic_state(pipe_config-
> >> > > >uapi.state);
> >> > > >  	int pipe_bpp;
> >> > > >  	int ret;
> >> > > >
> >> > > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> >> > > intel_dp *intel_dp,
> >> > > >  		}
> >> > > >  	}
> >> > > >
> >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> >> > > > +	if (IS_ERR(cdclk_state))
> >> > > > +		return PTR_ERR(cdclk_state);
> >> > > > +
> >> > > >  	/*
> >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> >> > > > -	 * is greater than the maximum Cdclock and if slice count is even
> >> > > > +	 * is greater than the current Cdclock and if slice count is even
> >> > > >  	 * then we need to use 2 VDSC instances.
> >> > > >  	 */
> >> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> >> > > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> >> > > 
> >> > > This is wrong. We compute the cdclk based on the requirements of the
> >> > > mode/etc., not the other way around.
> >
> > According to BSpec guideline, we decide whether we enable or disable second VDSC engine, based
> > on that condition. As I understand that one is about DSC config calculation, based on CDCLK
> > which was calculated. 
> 
> Point is, at the time compute_config gets called, what guarantees are
> there that cdclk_state->actual.cdclk contains anything useful? This is
> the design we have.

That is actually good question, was willing to check that as well.

> 
> > If we bump up CDCLK, to avoid this, will we even then use a second VDSC ever?
> 
> I think we'll eventually need better logic than unconditionally bumping
> to max, and it needs to take *both* the cdclk and the number of dsc
> engines into account. The referenced bspec only has the vdsc clock
> perspective, not overall perspective.

What we need to clarify here is that how this is supposed to work in theory.
Basically same issue can be fixed by both increasing the CDCLK or enabling
2nd VDSC engine.
There should be some guideline telling us, how to prioritize. 
From overall perspective as I understand, by default, we are able to keep
CDCLK 2 times less than pixel rate(see intel_pixel_rate_to_cdclk), however
due to that VDSC limitation that it can use only 1 ppc this becomes, not
applicable anymore(at least as of BSpec 49259), so we have to increase amount
of VDSC instances then.

So the question is now - what is more optimal here?
Also if we bump up CDCLK(which we have done many times already in fact), we
then need to add some logic to intel_compute_min_cdclk to check if we are using
DSC or not, because otherwise we don't really need to do that.

Stan

> 
> BR,
> Jani.
> 
> > Another thing is that probably enabling second VDSC is cheaper in terms of power consumption,
> > than bumping up the CDCLK.
> >
> > Stan
> >
> >> > 
> >> > Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?
> >> 
> >> That seems like the easiest solution. Another option might be to come up
> >> with some lower dotclock limit for the use of the second vdsc. But not
> >> sure we know where the tipping point is wrt. powr consumption.
> >> 
> >> -- 
> >> Ville Syrjälä
> >> Intel
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Stanislav Lisovskiy Sept. 14, 2021, 1:32 p.m. UTC | #9

On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]> wrote:
> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
> > >> > > -----Original Message-----
> > >> > > From: Ville Syrjälä <[e-mail]>
> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > >> > > To: Kulkarni, Vandita <[e-mail]>
> > >> > > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
> > >> > > Navare, Manasi D <[e-mail]>
> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > >> > > engine for higher moderates
> > >> > > 
> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
> > >> > > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
> > >> > > > engine when moderate is higher that the current cdclk.
> > >> > > >
> > >> > > > Signed-off-by: Vandita Kulkarni <[e-mail]>
> > >> > > > ---
> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >> > > >
> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> > > > index 161c33b2c869..55878f65f724 100644
> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> > > > @@ -70,6 +70,7 @@
> > >> > > >  #include "intel_tc.h"
> > >> > > >  #include "intel_vdsc.h"
> > >> > > >  #include "intel_vrr.h"
> > >> > > > +#include "intel_cdclk.h"
> > >> > > >
> > >> > > >  #define DP_DPRX_ESI_LEN 14
> > >> > > >
> > >> > > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
> > >> > > intel_dp *intel_dp,
> > >> > > >  				       struct drm_connector_state *conn_state,
> > >> > > >  				       struct link_config_limits *limits)  {
> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> > >> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > >> > > >base.base.dev);
> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> > >> > > >  		&pipe_config->hw.adjusted_mode;
> > >> > > > +	struct intel_atomic_state *state =
> > >> > > > +				to_intel_atomic_state(pipe_config-
> > >> > > >uapi.state);
> > >> > > >  	int pipe_bpp;
> > >> > > >  	int ret;
> > >> > > >
> > >> > > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
> > >> > > intel_dp *intel_dp,
> > >> > > >  		}
> > >> > > >  	}
> > >> > > >
> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > >> > > > +	if (IS_ERR(cdclk_state))
> > >> > > > +		return PTR_ERR(cdclk_state);
> > >> > > > +
> > >> > > >  	/*
> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > >> > > > -	 * is greater than the maximum Cdclock and if slice count is even
> > >> > > > +	 * is greater than the current Cdclock and if slice count is even
> > >> > > >  	 * then we need to use 2 VDSC instances.
> > >> > > >  	 */
> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
> > >> > > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
> > >> > > 
> > >> > > This is wrong. We compute the cdclk based on the requirements of the
> > >> > > mode/etc., not the other way around.
> > >
> > > According to BSpec guideline, we decide whether we enable or disable second VDSC engine, based
> > > on that condition. As I understand that one is about DSC config calculation, based on CDCLK
> > > which was calculated. 
> > 
> > Point is, at the time compute_config gets called, what guarantees are
> > there that cdclk_state->actual.cdclk contains anything useful? This is
> > the design we have.
> 
> That is actually good question, was willing to check that as well.
> 
> > 
> > > If we bump up CDCLK, to avoid this, will we even then use a second VDSC ever?
> > 
> > I think we'll eventually need better logic than unconditionally bumping
> > to max, and it needs to take *both* the cdclk and the number of dsc
> > engines into account. The referenced bspec only has the vdsc clock
> > perspective, not overall perspective.
> 
> What we need to clarify here is that how this is supposed to work in theory.
> Basically same issue can be fixed by both increasing the CDCLK or enabling
> 2nd VDSC engine.
> There should be some guideline telling us, how to prioritize. 
> From overall perspective as I understand, by default, we are able to keep
> CDCLK 2 times less than pixel rate(see intel_pixel_rate_to_cdclk), however
> due to that VDSC limitation that it can use only 1 ppc this becomes, not
> applicable anymore(at least as of BSpec 49259), so we have to increase amount
> of VDSC instances then.
> 
> So the question is now - what is more optimal here?
> Also if we bump up CDCLK(which we have done many times already in fact), we
> then need to add some logic to intel_compute_min_cdclk to check if we are using
> DSC or not, because otherwise we don't really need to do that.
> 
> Stan

Checked and indeed, encoder->compute_config is called way before, basically
CDCLK calculation is called almost in the end of atomic_check, so in 
compute_config, there would be an old CDCLK value copied from previous
cdclk state, but not the last one.

Vandita, this means we actually can't do it that way, if you want to do
anything with VDSC based on CDCLK this has to be done _after_ 
intel_compute_min_cdclk was called. Which is not very sweet, I guess.

So as of current architecture, it seems that the easiest way is indeed
to bump the CDCLK or we need to figure the way how to enable 2nd VDSC
somewhere else, after CDCLK was calculated.

Stan

> 
> > 
> > BR,
> > Jani.
> > 
> > > Another thing is that probably enabling second VDSC is cheaper in terms of power consumption,
> > > than bumping up the CDCLK.
> > >
> > > Stan
> > >
> > >> > 
> > >> > Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?
> > >> 
> > >> That seems like the easiest solution. Another option might be to come up
> > >> with some lower dotclock limit for the use of the second vdsc. But not
> > >> sure we know where the tipping point is wrt. powr consumption.
> > >> 
> > >> -- 
> > >> Ville Syrjälä
> > >> Intel
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center

Jani Nikula Sept. 14, 2021, 2:02 p.m. UTC | #10

On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]> wrote:
> On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
>> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
>> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]> wrote:
>> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
>> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
>> > >> > > -----Original Message-----
>> > >> > > From: Ville Syrjälä <[e-mail]>
>> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
>> > >> > > To: Kulkarni, Vandita <[e-mail]>
>> > >> > > Cc: [e-mail]; Nikula, Jani <[e-mail]>;
>> > >> > > Navare, Manasi D <[e-mail]>
>> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
>> > >> > > engine for higher moderates
>> > >> > > 
>> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni wrote:
>> > >> > > > Each VDSC operates with 1ppc throughput, hence enable the second VDSC
>> > >> > > > engine when moderate is higher that the current cdclk.
>> > >> > > >
>> > >> > > > Signed-off-by: Vandita Kulkarni <[e-mail]>
>> > >> > > > ---
>> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
>> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
>> > >> > > >
>> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > >> > > > index 161c33b2c869..55878f65f724 100644
>> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > >> > > > @@ -70,6 +70,7 @@
>> > >> > > >  #include "intel_tc.h"
>> > >> > > >  #include "intel_vdsc.h"
>> > >> > > >  #include "intel_vrr.h"
>> > >> > > > +#include "intel_cdclk.h"
>> > >> > > >
>> > >> > > >  #define DP_DPRX_ESI_LEN 14
>> > >> > > >
>> > >> > > > @@ -1291,10 +1292,13 @@ static int intel_dp_dsc_compute_config(struct
>> > >> > > intel_dp *intel_dp,
>> > >> > > >  				       struct drm_connector_state *conn_state,
>> > >> > > >  				       struct link_config_limits *limits)  {
>> > >> > > > +	struct intel_cdclk_state *cdclk_state;
>> > >> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > >> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
>> > >> > > >base.base.dev);
>> > >> > > >  	const struct drm_display_mode *adjusted_mode =
>> > >> > > >  		&pipe_config->hw.adjusted_mode;
>> > >> > > > +	struct intel_atomic_state *state =
>> > >> > > > +				to_intel_atomic_state(pipe_config-
>> > >> > > >uapi.state);
>> > >> > > >  	int pipe_bpp;
>> > >> > > >  	int ret;
>> > >> > > >
>> > >> > > > @@ -1373,12 +1377,16 @@ static int intel_dp_dsc_compute_config(struct
>> > >> > > intel_dp *intel_dp,
>> > >> > > >  		}
>> > >> > > >  	}
>> > >> > > >
>> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
>> > >> > > > +	if (IS_ERR(cdclk_state))
>> > >> > > > +		return PTR_ERR(cdclk_state);
>> > >> > > > +
>> > >> > > >  	/*
>> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
>> > >> > > > -	 * is greater than the maximum Cdclock and if slice count is even
>> > >> > > > +	 * is greater than the current Cdclock and if slice count is even
>> > >> > > >  	 * then we need to use 2 VDSC instances.
>> > >> > > >  	 */
>> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
>> > >> > > > +	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
>> > >> > > 
>> > >> > > This is wrong. We compute the cdclk based on the requirements of the
>> > >> > > mode/etc., not the other way around.
>> > >
>> > > According to BSpec guideline, we decide whether we enable or disable second VDSC engine, based
>> > > on that condition. As I understand that one is about DSC config calculation, based on CDCLK
>> > > which was calculated. 
>> > 
>> > Point is, at the time compute_config gets called, what guarantees are
>> > there that cdclk_state->actual.cdclk contains anything useful? This is
>> > the design we have.
>> 
>> That is actually good question, was willing to check that as well.
>> 
>> > 
>> > > If we bump up CDCLK, to avoid this, will we even then use a second VDSC ever?
>> > 
>> > I think we'll eventually need better logic than unconditionally bumping
>> > to max, and it needs to take *both* the cdclk and the number of dsc
>> > engines into account. The referenced bspec only has the vdsc clock
>> > perspective, not overall perspective.
>> 
>> What we need to clarify here is that how this is supposed to work in theory.
>> Basically same issue can be fixed by both increasing the CDCLK or enabling
>> 2nd VDSC engine.
>> There should be some guideline telling us, how to prioritize. 
>> From overall perspective as I understand, by default, we are able to keep
>> CDCLK 2 times less than pixel rate(see intel_pixel_rate_to_cdclk), however
>> due to that VDSC limitation that it can use only 1 ppc this becomes, not
>> applicable anymore(at least as of BSpec 49259), so we have to increase amount
>> of VDSC instances then.
>> 
>> So the question is now - what is more optimal here?
>> Also if we bump up CDCLK(which we have done many times already in fact), we
>> then need to add some logic to intel_compute_min_cdclk to check if we are using
>> DSC or not, because otherwise we don't really need to do that.

intel_compute_min_cdclk() already needs to be dsc aware when slice count
is 1 and we can't use two dsc engines anyway. See the recent commit
fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC enabled").

Looking again, I'm not sure that does the right decision for when
dsc.slice_count > 1, but dsc.split == false. It should probably use
dsc.split for the decision.

>> 
>> Stan
>
> Checked and indeed, encoder->compute_config is called way before, basically
> CDCLK calculation is called almost in the end of atomic_check, so in 
> compute_config, there would be an old CDCLK value copied from previous
> cdclk state, but not the last one.
>
> Vandita, this means we actually can't do it that way, if you want to do
> anything with VDSC based on CDCLK this has to be done _after_ 
> intel_compute_min_cdclk was called. Which is not very sweet, I guess.
>
> So as of current architecture, it seems that the easiest way is indeed
> to bump the CDCLK or we need to figure the way how to enable 2nd VDSC
> somewhere else, after CDCLK was calculated.

Alternatively, we could use two dsc engines more aggressively, but that
decision currently can't take overall chosen cdclk into account.

We'll end up sometimes unnecessarily using a too high cdclk or two dsc
engines, just have to pick the poison.

I think trying to do dsc decisions after intel_compute_min_cdclk() gets
way too complicated.

BR,
Jani




>
> Stan
>
>> 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > > Another thing is that probably enabling second VDSC is cheaper in terms of power consumption,
>> > > than bumping up the CDCLK.
>> > >
>> > > Stan
>> > >
>> > >> > 
>> > >> > Okay , So you suggest that we set the cd clock to max when we have such requirement, than enabling the second engine?
>> > >> 
>> > >> That seems like the easiest solution. Another option might be to come up
>> > >> with some lower dotclock limit for the use of the second vdsc. But not
>> > >> sure we know where the tipping point is wrt. powr consumption.
>> > >> 
>> > >> -- 
>> > >> Ville Syrjälä
>> > >> Intel
>> > 
>> > -- 
>> > Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita Sept. 14, 2021, 2:40 p.m. UTC | #11

> -----Original Message-----
> From: Nikula, Jani <[e-mail]>
> Sent: Tuesday, September 14, 2021 7:33 PM
> To: Lisovskiy, Stanislav <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>; Kulkarni, Vandita
> <[e-mail]>; [e-mail]; Navare,
> Manasi D <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]>
> wrote:
> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> <[e-mail]> wrote:
> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
> >> > >> > > -----Original Message-----
> >> > >> > > From: Ville Syrjälä <[e-mail]>
> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
> >> > >> > > Cc: [e-mail]; Nikula, Jani
> >> > >> > > <[e-mail]>; Navare, Manasi D
> >> > >> > > <[e-mail]>
> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable
> >> > >> > > second VDSC engine for higher moderates
> >> > >> > >
> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni
> wrote:
> >> > >> > > > Each VDSC operates with 1ppc throughput, hence enable the
> >> > >> > > > second VDSC engine when moderate is higher that the current
> cdclk.
> >> > >> > > >
> >> > >> > > > Signed-off-by: Vandita Kulkarni
> >> > >> > > > <[e-mail]>
> >> > >> > > > ---
> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> >> > >> > > >
> >> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> > >> > > > @@ -70,6 +70,7 @@
> >> > >> > > >  #include "intel_tc.h"
> >> > >> > > >  #include "intel_vdsc.h"
> >> > >> > > >  #include "intel_vrr.h"
> >> > >> > > > +#include "intel_cdclk.h"
> >> > >> > > >
> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> >> > >> > > >
> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> >> > >> > > > intel_dp_dsc_compute_config(struct
> >> > >> > > intel_dp *intel_dp,
> >> > >> > > >  				       struct drm_connector_state
> *conn_state,
> >> > >> > > >  				       struct link_config_limits *limits)  {
> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> >> > >> > > >  	struct intel_digital_port *dig_port =
> dp_to_dig_port(intel_dp);
> >> > >> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
> >> > >> > > >base.base.dev);
> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> >> > >> > > > +	struct intel_atomic_state *state =
> >> > >> > > > +				to_intel_atomic_state(pipe_config-
> >> > >> > > >uapi.state);
> >> > >> > > >  	int pipe_bpp;
> >> > >> > > >  	int ret;
> >> > >> > > >
> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> >> > >> > > > intel_dp_dsc_compute_config(struct
> >> > >> > > intel_dp *intel_dp,
> >> > >> > > >  		}
> >> > >> > > >  	}
> >> > >> > > >
> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> >> > >> > > > +	if (IS_ERR(cdclk_state))
> >> > >> > > > +		return PTR_ERR(cdclk_state);
> >> > >> > > > +
> >> > >> > > >  	/*
> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel
> rate
> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice count is
> even
> >> > >> > > > +	 * is greater than the current Cdclock and if slice
> >> > >> > > > +count is even
> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> >> > >> > > >  	 */
> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq
> ||
> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> >> > >> > > > +cdclk_state->actual.cdclk ||
> >> > >> > >
> >> > >> > > This is wrong. We compute the cdclk based on the
> >> > >> > > requirements of the mode/etc., not the other way around.
> >> > >
> >> > > According to BSpec guideline, we decide whether we enable or
> >> > > disable second VDSC engine, based on that condition. As I
> >> > > understand that one is about DSC config calculation, based on CDCLK
> which was calculated.
> >> >
> >> > Point is, at the time compute_config gets called, what guarantees
> >> > are there that cdclk_state->actual.cdclk contains anything useful?
> >> > This is the design we have.
> >>
> >> That is actually good question, was willing to check that as well.
> >>
> >> >
> >> > > If we bump up CDCLK, to avoid this, will we even then use a second
> VDSC ever?
> >> >
> >> > I think we'll eventually need better logic than unconditionally
> >> > bumping to max, and it needs to take *both* the cdclk and the
> >> > number of dsc engines into account. The referenced bspec only has
> >> > the vdsc clock perspective, not overall perspective.
> >>
> >> What we need to clarify here is that how this is supposed to work in
> theory.
> >> Basically same issue can be fixed by both increasing the CDCLK or
> >> enabling 2nd VDSC engine.
> >> There should be some guideline telling us, how to prioritize.
> >> From overall perspective as I understand, by default, we are able to
> >> keep CDCLK 2 times less than pixel rate(see
> >> intel_pixel_rate_to_cdclk), however due to that VDSC limitation that
> >> it can use only 1 ppc this becomes, not applicable anymore(at least
> >> as of BSpec 49259), so we have to increase amount of VDSC instances
> then.
> >>
> >> So the question is now - what is more optimal here?
> >> Also if we bump up CDCLK(which we have done many times already in
> >> fact), we then need to add some logic to intel_compute_min_cdclk to
> >> check if we are using DSC or not, because otherwise we don't really need
> to do that.
> 
> intel_compute_min_cdclk() already needs to be dsc aware when slice count
> is 1 and we can't use two dsc engines anyway. See the recent commit
> fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC enabled").
> 
> Looking again, I'm not sure that does the right decision for when
> dsc.slice_count > 1, but dsc.split == false. It should probably use dsc.split for
> the decision.
> 
> >>
> >> Stan
> >
> > Checked and indeed, encoder->compute_config is called way before,
> > basically CDCLK calculation is called almost in the end of
> > atomic_check, so in compute_config, there would be an old CDCLK value
> > copied from previous cdclk state, but not the last one.
> >
> > Vandita, this means we actually can't do it that way, if you want to
> > do anything with VDSC based on CDCLK this has to be done _after_
> > intel_compute_min_cdclk was called. Which is not very sweet, I guess.
> >
> > So as of current architecture, it seems that the easiest way is indeed
> > to bump the CDCLK or we need to figure the way how to enable 2nd VDSC
> > somewhere else, after CDCLK was calculated.
> 
> Alternatively, we could use two dsc engines more aggressively, but that
> decision currently can't take overall chosen cdclk into account.
> 
> We'll end up sometimes unnecessarily using a too high cdclk or two dsc
> engines, just have to pick the poison.
> 
> I think trying to do dsc decisions after intel_compute_min_cdclk() gets way
> too complicated.

In this case, can we just use the 2nd VDSC engine if slice_count is 2 or more?
Which would mean we always operate in joiner enabled mode(small joiner) of all the compression modes of operation
mentioned in the table bspec: 49259
Because we are still going to hit the max cdclk restriction for higher resolutions, and many lower resolutions wouldn’t need max cdclk.
And eventually once we have more details on cd clk vs 2VDSC engine we could add
the logic to choose one over the other?

I see that in case of DSI we do split = true, for slice_count > 1 but that would need a different set of checks, thats a TBD.

Or Do you suggest I just do this for now max cdclk when slice_count =1  (what we are doing now) replace with compression = true and split = false
 
Thanks,
Vandita
> 
> BR,
> Jani
> 
> 
> 
> 
> >
> > Stan
> >
> >>
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >> > > Another thing is that probably enabling second VDSC is cheaper in
> >> > > terms of power consumption, than bumping up the CDCLK.
> >> > >
> >> > > Stan
> >> > >
> >> > >> >
> >> > >> > Okay , So you suggest that we set the cd clock to max when we
> have such requirement, than enabling the second engine?
> >> > >>
> >> > >> That seems like the easiest solution. Another option might be to
> >> > >> come up with some lower dotclock limit for the use of the second
> >> > >> vdsc. But not sure we know where the tipping point is wrt. powr
> consumption.
> >> > >>
> >> > >> --
> >> > >> Ville Syrjälä
> >> > >> Intel
> >> >
> >> > --
> >> > Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Jani Nikula Sept. 14, 2021, 3:10 p.m. UTC | #12

On Tue, 14 Sep 2021, "Kulkarni, Vandita" <[e-mail]> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <[e-mail]>
>> Sent: Tuesday, September 14, 2021 7:33 PM
>> To: Lisovskiy, Stanislav <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>; Kulkarni, Vandita
>> <[e-mail]>; [e-mail]; Navare,
>> Manasi D <[e-mail]>
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
>> engine for higher moderates
>> 
>> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <[e-mail]>
>> wrote:
>> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
>> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
>> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
>> <[e-mail]> wrote:
>> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
>> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita wrote:
>> >> > >> > > -----Original Message-----
>> >> > >> > > From: Ville Syrjälä <[e-mail]>
>> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
>> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
>> >> > >> > > Cc: [e-mail]; Nikula, Jani
>> >> > >> > > <[e-mail]>; Navare, Manasi D
>> >> > >> > > <[e-mail]>
>> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable
>> >> > >> > > second VDSC engine for higher moderates
>> >> > >> > >
>> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita Kulkarni
>> wrote:
>> >> > >> > > > Each VDSC operates with 1ppc throughput, hence enable the
>> >> > >> > > > second VDSC engine when moderate is higher that the current
>> cdclk.
>> >> > >> > > >
>> >> > >> > > > Signed-off-by: Vandita Kulkarni
>> >> > >> > > > <[e-mail]>
>> >> > >> > > > ---
>> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++--
>> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
>> >> > >> > > >
>> >> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > >> > > > index 161c33b2c869..55878f65f724 100644
>> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> > >> > > > @@ -70,6 +70,7 @@
>> >> > >> > > >  #include "intel_tc.h"
>> >> > >> > > >  #include "intel_vdsc.h"
>> >> > >> > > >  #include "intel_vrr.h"
>> >> > >> > > > +#include "intel_cdclk.h"
>> >> > >> > > >
>> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
>> >> > >> > > >
>> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
>> >> > >> > > > intel_dp_dsc_compute_config(struct
>> >> > >> > > intel_dp *intel_dp,
>> >> > >> > > >  				       struct drm_connector_state
>> *conn_state,
>> >> > >> > > >  				       struct link_config_limits *limits)  {
>> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
>> >> > >> > > >  	struct intel_digital_port *dig_port =
>> dp_to_dig_port(intel_dp);
>> >> > >> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port-
>> >> > >> > > >base.base.dev);
>> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
>> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
>> >> > >> > > > +	struct intel_atomic_state *state =
>> >> > >> > > > +				to_intel_atomic_state(pipe_config-
>> >> > >> > > >uapi.state);
>> >> > >> > > >  	int pipe_bpp;
>> >> > >> > > >  	int ret;
>> >> > >> > > >
>> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
>> >> > >> > > > intel_dp_dsc_compute_config(struct
>> >> > >> > > intel_dp *intel_dp,
>> >> > >> > > >  		}
>> >> > >> > > >  	}
>> >> > >> > > >
>> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
>> >> > >> > > > +	if (IS_ERR(cdclk_state))
>> >> > >> > > > +		return PTR_ERR(cdclk_state);
>> >> > >> > > > +
>> >> > >> > > >  	/*
>> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel
>> rate
>> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice count is
>> even
>> >> > >> > > > +	 * is greater than the current Cdclock and if slice
>> >> > >> > > > +count is even
>> >> > >> > > >  	 * then we need to use 2 VDSC instances.
>> >> > >> > > >  	 */
>> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq
>> ||
>> >> > >> > > > +	if (adjusted_mode->crtc_clock >
>> >> > >> > > > +cdclk_state->actual.cdclk ||
>> >> > >> > >
>> >> > >> > > This is wrong. We compute the cdclk based on the
>> >> > >> > > requirements of the mode/etc., not the other way around.
>> >> > >
>> >> > > According to BSpec guideline, we decide whether we enable or
>> >> > > disable second VDSC engine, based on that condition. As I
>> >> > > understand that one is about DSC config calculation, based on CDCLK
>> which was calculated.
>> >> >
>> >> > Point is, at the time compute_config gets called, what guarantees
>> >> > are there that cdclk_state->actual.cdclk contains anything useful?
>> >> > This is the design we have.
>> >>
>> >> That is actually good question, was willing to check that as well.
>> >>
>> >> >
>> >> > > If we bump up CDCLK, to avoid this, will we even then use a second
>> VDSC ever?
>> >> >
>> >> > I think we'll eventually need better logic than unconditionally
>> >> > bumping to max, and it needs to take *both* the cdclk and the
>> >> > number of dsc engines into account. The referenced bspec only has
>> >> > the vdsc clock perspective, not overall perspective.
>> >>
>> >> What we need to clarify here is that how this is supposed to work in
>> theory.
>> >> Basically same issue can be fixed by both increasing the CDCLK or
>> >> enabling 2nd VDSC engine.
>> >> There should be some guideline telling us, how to prioritize.
>> >> From overall perspective as I understand, by default, we are able to
>> >> keep CDCLK 2 times less than pixel rate(see
>> >> intel_pixel_rate_to_cdclk), however due to that VDSC limitation that
>> >> it can use only 1 ppc this becomes, not applicable anymore(at least
>> >> as of BSpec 49259), so we have to increase amount of VDSC instances
>> then.
>> >>
>> >> So the question is now - what is more optimal here?
>> >> Also if we bump up CDCLK(which we have done many times already in
>> >> fact), we then need to add some logic to intel_compute_min_cdclk to
>> >> check if we are using DSC or not, because otherwise we don't really need
>> to do that.
>> 
>> intel_compute_min_cdclk() already needs to be dsc aware when slice count
>> is 1 and we can't use two dsc engines anyway. See the recent commit
>> fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC enabled").
>> 
>> Looking again, I'm not sure that does the right decision for when
>> dsc.slice_count > 1, but dsc.split == false. It should probably use dsc.split for
>> the decision.
>> 
>> >>
>> >> Stan
>> >
>> > Checked and indeed, encoder->compute_config is called way before,
>> > basically CDCLK calculation is called almost in the end of
>> > atomic_check, so in compute_config, there would be an old CDCLK value
>> > copied from previous cdclk state, but not the last one.
>> >
>> > Vandita, this means we actually can't do it that way, if you want to
>> > do anything with VDSC based on CDCLK this has to be done _after_
>> > intel_compute_min_cdclk was called. Which is not very sweet, I guess.
>> >
>> > So as of current architecture, it seems that the easiest way is indeed
>> > to bump the CDCLK or we need to figure the way how to enable 2nd VDSC
>> > somewhere else, after CDCLK was calculated.
>> 
>> Alternatively, we could use two dsc engines more aggressively, but that
>> decision currently can't take overall chosen cdclk into account.
>> 
>> We'll end up sometimes unnecessarily using a too high cdclk or two dsc
>> engines, just have to pick the poison.
>> 
>> I think trying to do dsc decisions after intel_compute_min_cdclk() gets way
>> too complicated.
>
> In this case, can we just use the 2nd VDSC engine if slice_count is 2 or more?
> Which would mean we always operate in joiner enabled mode(small joiner) of all the compression modes of operation
> mentioned in the table bspec: 49259
> Because we are still going to hit the max cdclk restriction for higher resolutions, and many lower resolutions wouldn’t need max cdclk.
> And eventually once we have more details on cd clk vs 2VDSC engine we could add
> the logic to choose one over the other?
>
> I see that in case of DSI we do split = true, for slice_count > 1 but that would need a different set of checks, thats a TBD.
>
> Or Do you suggest I just do this for now max cdclk when slice_count =1  (what we are doing now) replace with compression = true and split = false

I think the check in intel_compute_min_cdclk() should be:

	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)

That's a separate change.

Enabling two dsc engines more aggressively... I don't mind doing it
unconditionally when slice count > 1 for starters. But I think we'll
need to improve this going forward, including fixing the mode valid
checks etc. as we've discussed.

Ville, any objections?

BR,
Jani.


>  
> Thanks,
> Vandita
>> 
>> BR,
>> Jani
>> 
>> 
>> 
>> 
>> >
>> > Stan
>> >
>> >>
>> >> >
>> >> > BR,
>> >> > Jani.
>> >> >
>> >> > > Another thing is that probably enabling second VDSC is cheaper in
>> >> > > terms of power consumption, than bumping up the CDCLK.
>> >> > >
>> >> > > Stan
>> >> > >
>> >> > >> >
>> >> > >> > Okay , So you suggest that we set the cd clock to max when we
>> have such requirement, than enabling the second engine?
>> >> > >>
>> >> > >> That seems like the easiest solution. Another option might be to
>> >> > >> come up with some lower dotclock limit for the use of the second
>> >> > >> vdsc. But not sure we know where the tipping point is wrt. powr
>> consumption.
>> >> > >>
>> >> > >> --
>> >> > >> Ville Syrjälä
>> >> > >> Intel
>> >> >
>> >> > --
>> >> > Jani Nikula, Intel Open Source Graphics Center
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita Jan. 10, 2022, 7:15 a.m. UTC | #13

Revisiting this thread after update from the bspec.

> -----Original Message-----
> From: Nikula, Jani <[e-mail]>
> Sent: Tuesday, September 14, 2021 8:40 PM
> To: Kulkarni, Vandita <[e-mail]>; Lisovskiy, Stanislav
> <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>; intel-
> [e-mail]; Navare, Manasi D <[e-mail]>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> On Tue, 14 Sep 2021, "Kulkarni, Vandita" <[e-mail]>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <[e-mail]>
> >> Sent: Tuesday, September 14, 2021 7:33 PM
> >> To: Lisovskiy, Stanislav <[e-mail]>
> >> Cc: Ville Syrjälä <[e-mail]>; Kulkarni, Vandita
> >> <[e-mail]>; [e-mail];
> >> Navare, Manasi D <[e-mail]>
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> >> engine for higher moderates
> >>
> >> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> >> <[e-mail]>
> >> wrote:
> >> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> >> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> >> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> >> <[e-mail]> wrote:
> >> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> >> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita
> wrote:
> >> >> > >> > > -----Original Message-----
> >> >> > >> > > From: Ville Syrjälä <[e-mail]>
> >> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> >> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
> >> >> > >> > > Cc: [e-mail]; Nikula, Jani
> >> >> > >> > > <[e-mail]>; Navare, Manasi D
> >> >> > >> > > <[e-mail]>
> >> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable
> >> >> > >> > > second VDSC engine for higher moderates
> >> >> > >> > >
> >> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita
> >> >> > >> > > Kulkarni
> >> wrote:
> >> >> > >> > > > Each VDSC operates with 1ppc throughput, hence enable
> >> >> > >> > > > the second VDSC engine when moderate is higher that the
> >> >> > >> > > > current
> >> cdclk.
> >> >> > >> > > >
> >> >> > >> > > > Signed-off-by: Vandita Kulkarni
> >> >> > >> > > > <[e-mail]>
> >> >> > >> > > > ---
> >> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12
> >> >> > >> > > > ++++++++++--
> >> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> >> >> > >> > > >
> >> >> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> >> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >> > >> > > > @@ -70,6 +70,7 @@
> >> >> > >> > > >  #include "intel_tc.h"
> >> >> > >> > > >  #include "intel_vdsc.h"
> >> >> > >> > > >  #include "intel_vrr.h"
> >> >> > >> > > > +#include "intel_cdclk.h"
> >> >> > >> > > >
> >> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> >> >> > >> > > >
> >> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> >> >> > >> > > > intel_dp_dsc_compute_config(struct
> >> >> > >> > > intel_dp *intel_dp,
> >> >> > >> > > >  				       struct
> drm_connector_state
> >> *conn_state,
> >> >> > >> > > >  				       struct link_config_limits
> *limits)  {
> >> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> >> >> > >> > > >  	struct intel_digital_port *dig_port =
> >> dp_to_dig_port(intel_dp);
> >> >> > >> > > >  	struct drm_i915_private *dev_priv =
> to_i915(dig_port-
> >> >> > >> > > >base.base.dev);
> >> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> >> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> >> >> > >> > > > +	struct intel_atomic_state *state =
> >> >> > >> > > > +
> 	to_intel_atomic_state(pipe_config-
> >> >> > >> > > >uapi.state);
> >> >> > >> > > >  	int pipe_bpp;
> >> >> > >> > > >  	int ret;
> >> >> > >> > > >
> >> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> >> >> > >> > > > intel_dp_dsc_compute_config(struct
> >> >> > >> > > intel_dp *intel_dp,
> >> >> > >> > > >  		}
> >> >> > >> > > >  	}
> >> >> > >> > > >
> >> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> >> >> > >> > > > +	if (IS_ERR(cdclk_state))
> >> >> > >> > > > +		return PTR_ERR(cdclk_state);
> >> >> > >> > > > +
> >> >> > >> > > >  	/*
> >> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if
> >> >> > >> > > > peak pixel
> >> rate
> >> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice
> count is
> >> even
> >> >> > >> > > > +	 * is greater than the current Cdclock and if slice
> >> >> > >> > > > +count is even
> >> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> >> >> > >> > > >  	 */
> >> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv-
> >max_cdclk_freq
> >> ||
> >> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> >> >> > >> > > > +cdclk_state->actual.cdclk ||
> >> >> > >> > >
> >> >> > >> > > This is wrong. We compute the cdclk based on the
> >> >> > >> > > requirements of the mode/etc., not the other way around.
> >> >> > >
> >> >> > > According to BSpec guideline, we decide whether we enable or
> >> >> > > disable second VDSC engine, based on that condition. As I
> >> >> > > understand that one is about DSC config calculation, based on
> >> >> > > CDCLK
> >> which was calculated.
> >> >> >
> >> >> > Point is, at the time compute_config gets called, what
> >> >> > guarantees are there that cdclk_state->actual.cdclk contains
> anything useful?
> >> >> > This is the design we have.
> >> >>
> >> >> That is actually good question, was willing to check that as well.
> >> >>
> >> >> >
> >> >> > > If we bump up CDCLK, to avoid this, will we even then use a
> >> >> > > second
> >> VDSC ever?
> >> >> >
> >> >> > I think we'll eventually need better logic than unconditionally
> >> >> > bumping to max, and it needs to take *both* the cdclk and the
> >> >> > number of dsc engines into account. The referenced bspec only
> >> >> > has the vdsc clock perspective, not overall perspective.
> >> >>
> >> >> What we need to clarify here is that how this is supposed to work
> >> >> in
> >> theory.
> >> >> Basically same issue can be fixed by both increasing the CDCLK or
> >> >> enabling 2nd VDSC engine.
> >> >> There should be some guideline telling us, how to prioritize.
> >> >> From overall perspective as I understand, by default, we are able
> >> >> to keep CDCLK 2 times less than pixel rate(see
> >> >> intel_pixel_rate_to_cdclk), however due to that VDSC limitation
> >> >> that it can use only 1 ppc this becomes, not applicable anymore(at
> >> >> least as of BSpec 49259), so we have to increase amount of VDSC
> >> >> instances
> >> then.
> >> >>
> >> >> So the question is now - what is more optimal here?
> >> >> Also if we bump up CDCLK(which we have done many times already in
> >> >> fact), we then need to add some logic to intel_compute_min_cdclk
> >> >> to check if we are using DSC or not, because otherwise we don't
> >> >> really need
> >> to do that.
> >>
> >> intel_compute_min_cdclk() already needs to be dsc aware when slice
> >> count is 1 and we can't use two dsc engines anyway. See the recent
> >> commit fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC enabled").
> >>
> >> Looking again, I'm not sure that does the right decision for when
> >> dsc.slice_count > 1, but dsc.split == false. It should probably use
> >> dsc.split for the decision.
> >>
> >> >>
> >> >> Stan
> >> >
> >> > Checked and indeed, encoder->compute_config is called way before,
> >> > basically CDCLK calculation is called almost in the end of
> >> > atomic_check, so in compute_config, there would be an old CDCLK
> >> > value copied from previous cdclk state, but not the last one.
> >> >
> >> > Vandita, this means we actually can't do it that way, if you want
> >> > to do anything with VDSC based on CDCLK this has to be done _after_
> >> > intel_compute_min_cdclk was called. Which is not very sweet, I guess.
> >> >
> >> > So as of current architecture, it seems that the easiest way is
> >> > indeed to bump the CDCLK or we need to figure the way how to enable
> >> > 2nd VDSC somewhere else, after CDCLK was calculated.
> >>
> >> Alternatively, we could use two dsc engines more aggressively, but
> >> that decision currently can't take overall chosen cdclk into account.
> >>
> >> We'll end up sometimes unnecessarily using a too high cdclk or two
> >> dsc engines, just have to pick the poison.
> >>
> >> I think trying to do dsc decisions after intel_compute_min_cdclk()
> >> gets way too complicated.
> >
> > In this case, can we just use the 2nd VDSC engine if slice_count is 2 or
> more?
> > Which would mean we always operate in joiner enabled mode(small
> > joiner) of all the compression modes of operation mentioned in the
> > table bspec: 49259 Because we are still going to hit the max cdclk restriction
> for higher resolutions, and many lower resolutions wouldn’t need max cdclk.
> > And eventually once we have more details on cd clk vs 2VDSC engine we
> > could add the logic to choose one over the other?
> >
> > I see that in case of DSI we do split = true, for slice_count > 1 but that
> would need a different set of checks, thats a TBD.
> >
> > Or Do you suggest I just do this for now max cdclk when slice_count =1
> > (what we are doing now) replace with compression = true and split =
> > false
> 
> I think the check in intel_compute_min_cdclk() should be:
> 
> 	if (crtc_state->dsc.compression_enable && !crtc_state-
> >dsc.dsc_split)
> 
> That's a separate change.
> 
> Enabling two dsc engines more aggressively... I don't mind doing it
> unconditionally when slice count > 1 for starters. But I think we'll need to
> improve this going forward, including fixing the mode valid checks etc. as
> we've discussed.

Design recommendation is to use 2 VDSC instances while meeting the following constraint so that cdclk can stay as low as possible.
DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth

Thanks,
Vandita
> 
> Ville, any objections?
> 
> BR,
> Jani.
> 
> 
> >
> > Thanks,
> > Vandita
> >>
> >> BR,
> >> Jani
> >>
> >>
> >>
> >>
> >> >
> >> > Stan
> >> >
> >> >>
> >> >> >
> >> >> > BR,
> >> >> > Jani.
> >> >> >
> >> >> > > Another thing is that probably enabling second VDSC is cheaper
> >> >> > > in terms of power consumption, than bumping up the CDCLK.
> >> >> > >
> >> >> > > Stan
> >> >> > >
> >> >> > >> >
> >> >> > >> > Okay , So you suggest that we set the cd clock to max when
> >> >> > >> > we
> >> have such requirement, than enabling the second engine?
> >> >> > >>
> >> >> > >> That seems like the easiest solution. Another option might be
> >> >> > >> to come up with some lower dotclock limit for the use of the
> >> >> > >> second vdsc. But not sure we know where the tipping point is
> >> >> > >> wrt. powr
> >> consumption.
> >> >> > >>
> >> >> > >> --
> >> >> > >> Ville Syrjälä
> >> >> > >> Intel
> >> >> >
> >> >> > --
> >> >> > Jani Nikula, Intel Open Source Graphics Center
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi Jan. 10, 2022, 7:36 p.m. UTC | #14

Thankf for revisiting this thread. The use of max_cdclk is currently in 2 places in DSC code
1. . if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
                if (pipe_config->dsc.slice_count > 1) {
                        pipe_config->dsc.dsc_split = true;
2. if (bigjoiner) {
                u32 max_bpp_bigjoiner =
                        i915->max_cdclk_freq * 48 /
                        intel_dp_mode_to_fec_clock(mode_clock);

                DRM_DEBUG_KMS("Max big joiner bpp: %u\n", max_bpp_bigjoiner);
                bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
        }

In both these places, using max_cdclk can cause problems, like for compressed bpp it can give a higher bpp based
on max_cdclk and we might actually end up chosing lower cdclk at what point this will cause underruns.

So when I was discussing with Ville on this, my first thought was also to use the cdclk_state->actual_cdclk but like
Ville mentioned later in the review comments the challenge there was that actual cdclk does get computed
much later than dsc_compute_config.

So I think as suggested in one of the reviews we just to check if DSC is enabled then we dont allow lowering the cdclk
which would also prevent underruns caused by possibly setting up higher bpp based on max cdclk.

@Ville @Jani does this sound like a good approach. Then @Vandita we can pursue that change.

Regards
Manasi

On Sun, Jan 09, 2022 at 11:15:04PM -0800, Kulkarni, Vandita wrote:
> Revisiting this thread after update from the bspec.
> 
> > -----Original Message-----
> > From: Nikula, Jani <[e-mail]>
> > Sent: Tuesday, September 14, 2021 8:40 PM
> > To: Kulkarni, Vandita <[e-mail]>; Lisovskiy, Stanislav
> > <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>; intel-
> > [e-mail]; Navare, Manasi D <[e-mail]>
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > engine for higher moderates
> > 
> > On Tue, 14 Sep 2021, "Kulkarni, Vandita" <[e-mail]>
> > wrote:
> > >> -----Original Message-----
> > >> From: Nikula, Jani <[e-mail]>
> > >> Sent: Tuesday, September 14, 2021 7:33 PM
> > >> To: Lisovskiy, Stanislav <[e-mail]>
> > >> Cc: Ville Syrjälä <[e-mail]>; Kulkarni, Vandita
> > >> <[e-mail]>; [e-mail];
> > >> Navare, Manasi D <[e-mail]>
> > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > >> engine for higher moderates
> > >>
> > >> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > >> <[e-mail]>
> > >> wrote:
> > >> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> > >> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> > >> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > >> <[e-mail]> wrote:
> > >> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> > >> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni, Vandita
> > wrote:
> > >> >> > >> > > -----Original Message-----
> > >> >> > >> > > From: Ville Syrjälä <[e-mail]>
> > >> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > >> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
> > >> >> > >> > > Cc: [e-mail]; Nikula, Jani
> > >> >> > >> > > <[e-mail]>; Navare, Manasi D
> > >> >> > >> > > <[e-mail]>
> > >> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable
> > >> >> > >> > > second VDSC engine for higher moderates
> > >> >> > >> > >
> > >> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita
> > >> >> > >> > > Kulkarni
> > >> wrote:
> > >> >> > >> > > > Each VDSC operates with 1ppc throughput, hence enable
> > >> >> > >> > > > the second VDSC engine when moderate is higher that the
> > >> >> > >> > > > current
> > >> cdclk.
> > >> >> > >> > > >
> > >> >> > >> > > > Signed-off-by: Vandita Kulkarni
> > >> >> > >> > > > <[e-mail]>
> > >> >> > >> > > > ---
> > >> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12
> > >> >> > >> > > > ++++++++++--
> > >> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >> >> > >> > > >
> > >> >> > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > >> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> > >> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > >> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > >> >> > >> > > > @@ -70,6 +70,7 @@
> > >> >> > >> > > >  #include "intel_tc.h"
> > >> >> > >> > > >  #include "intel_vdsc.h"
> > >> >> > >> > > >  #include "intel_vrr.h"
> > >> >> > >> > > > +#include "intel_cdclk.h"
> > >> >> > >> > > >
> > >> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> > >> >> > >> > > >
> > >> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > >> >> > >> > > intel_dp *intel_dp,
> > >> >> > >> > > >  				       struct
> > drm_connector_state
> > >> *conn_state,
> > >> >> > >> > > >  				       struct link_config_limits
> > *limits)  {
> > >> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> > >> >> > >> > > >  	struct intel_digital_port *dig_port =
> > >> dp_to_dig_port(intel_dp);
> > >> >> > >> > > >  	struct drm_i915_private *dev_priv =
> > to_i915(dig_port-
> > >> >> > >> > > >base.base.dev);
> > >> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> > >> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> > >> >> > >> > > > +	struct intel_atomic_state *state =
> > >> >> > >> > > > +
> > 	to_intel_atomic_state(pipe_config-
> > >> >> > >> > > >uapi.state);
> > >> >> > >> > > >  	int pipe_bpp;
> > >> >> > >> > > >  	int ret;
> > >> >> > >> > > >
> > >> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > >> >> > >> > > intel_dp *intel_dp,
> > >> >> > >> > > >  		}
> > >> >> > >> > > >  	}
> > >> >> > >> > > >
> > >> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > >> >> > >> > > > +	if (IS_ERR(cdclk_state))
> > >> >> > >> > > > +		return PTR_ERR(cdclk_state);
> > >> >> > >> > > > +
> > >> >> > >> > > >  	/*
> > >> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so if
> > >> >> > >> > > > peak pixel
> > >> rate
> > >> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice
> > count is
> > >> even
> > >> >> > >> > > > +	 * is greater than the current Cdclock and if slice
> > >> >> > >> > > > +count is even
> > >> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> > >> >> > >> > > >  	 */
> > >> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv-
> > >max_cdclk_freq
> > >> ||
> > >> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> > >> >> > >> > > > +cdclk_state->actual.cdclk ||
> > >> >> > >> > >
> > >> >> > >> > > This is wrong. We compute the cdclk based on the
> > >> >> > >> > > requirements of the mode/etc., not the other way around.
> > >> >> > >
> > >> >> > > According to BSpec guideline, we decide whether we enable or
> > >> >> > > disable second VDSC engine, based on that condition. As I
> > >> >> > > understand that one is about DSC config calculation, based on
> > >> >> > > CDCLK
> > >> which was calculated.
> > >> >> >
> > >> >> > Point is, at the time compute_config gets called, what
> > >> >> > guarantees are there that cdclk_state->actual.cdclk contains
> > anything useful?
> > >> >> > This is the design we have.
> > >> >>
> > >> >> That is actually good question, was willing to check that as well.
> > >> >>
> > >> >> >
> > >> >> > > If we bump up CDCLK, to avoid this, will we even then use a
> > >> >> > > second
> > >> VDSC ever?
> > >> >> >
> > >> >> > I think we'll eventually need better logic than unconditionally
> > >> >> > bumping to max, and it needs to take *both* the cdclk and the
> > >> >> > number of dsc engines into account. The referenced bspec only
> > >> >> > has the vdsc clock perspective, not overall perspective.
> > >> >>
> > >> >> What we need to clarify here is that how this is supposed to work
> > >> >> in
> > >> theory.
> > >> >> Basically same issue can be fixed by both increasing the CDCLK or
> > >> >> enabling 2nd VDSC engine.
> > >> >> There should be some guideline telling us, how to prioritize.
> > >> >> From overall perspective as I understand, by default, we are able
> > >> >> to keep CDCLK 2 times less than pixel rate(see
> > >> >> intel_pixel_rate_to_cdclk), however due to that VDSC limitation
> > >> >> that it can use only 1 ppc this becomes, not applicable anymore(at
> > >> >> least as of BSpec 49259), so we have to increase amount of VDSC
> > >> >> instances
> > >> then.
> > >> >>
> > >> >> So the question is now - what is more optimal here?
> > >> >> Also if we bump up CDCLK(which we have done many times already in
> > >> >> fact), we then need to add some logic to intel_compute_min_cdclk
> > >> >> to check if we are using DSC or not, because otherwise we don't
> > >> >> really need
> > >> to do that.
> > >>
> > >> intel_compute_min_cdclk() already needs to be dsc aware when slice
> > >> count is 1 and we can't use two dsc engines anyway. See the recent
> > >> commit fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC enabled").
> > >>
> > >> Looking again, I'm not sure that does the right decision for when
> > >> dsc.slice_count > 1, but dsc.split == false. It should probably use
> > >> dsc.split for the decision.
> > >>
> > >> >>
> > >> >> Stan
> > >> >
> > >> > Checked and indeed, encoder->compute_config is called way before,
> > >> > basically CDCLK calculation is called almost in the end of
> > >> > atomic_check, so in compute_config, there would be an old CDCLK
> > >> > value copied from previous cdclk state, but not the last one.
> > >> >
> > >> > Vandita, this means we actually can't do it that way, if you want
> > >> > to do anything with VDSC based on CDCLK this has to be done _after_
> > >> > intel_compute_min_cdclk was called. Which is not very sweet, I guess.
> > >> >
> > >> > So as of current architecture, it seems that the easiest way is
> > >> > indeed to bump the CDCLK or we need to figure the way how to enable
> > >> > 2nd VDSC somewhere else, after CDCLK was calculated.
> > >>
> > >> Alternatively, we could use two dsc engines more aggressively, but
> > >> that decision currently can't take overall chosen cdclk into account.
> > >>
> > >> We'll end up sometimes unnecessarily using a too high cdclk or two
> > >> dsc engines, just have to pick the poison.
> > >>
> > >> I think trying to do dsc decisions after intel_compute_min_cdclk()
> > >> gets way too complicated.
> > >
> > > In this case, can we just use the 2nd VDSC engine if slice_count is 2 or
> > more?
> > > Which would mean we always operate in joiner enabled mode(small
> > > joiner) of all the compression modes of operation mentioned in the
> > > table bspec: 49259 Because we are still going to hit the max cdclk restriction
> > for higher resolutions, and many lower resolutions wouldn’t need max cdclk.
> > > And eventually once we have more details on cd clk vs 2VDSC engine we
> > > could add the logic to choose one over the other?
> > >
> > > I see that in case of DSI we do split = true, for slice_count > 1 but that
> > would need a different set of checks, thats a TBD.
> > >
> > > Or Do you suggest I just do this for now max cdclk when slice_count =1
> > > (what we are doing now) replace with compression = true and split =
> > > false
> > 
> > I think the check in intel_compute_min_cdclk() should be:
> > 
> > 	if (crtc_state->dsc.compression_enable && !crtc_state-
> > >dsc.dsc_split)
> > 
> > That's a separate change.
> > 
> > Enabling two dsc engines more aggressively... I don't mind doing it
> > unconditionally when slice count > 1 for starters. But I think we'll need to
> > improve this going forward, including fixing the mode valid checks etc. as
> > we've discussed.
> 
> Design recommendation is to use 2 VDSC instances while meeting the following constraint so that cdclk can stay as low as possible.
> DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth
> 
> Thanks,
> Vandita
> > 
> > Ville, any objections?
> > 
> > BR,
> > Jani.
> > 
> > 
> > >
> > > Thanks,
> > > Vandita
> > >>
> > >> BR,
> > >> Jani
> > >>
> > >>
> > >>
> > >>
> > >> >
> > >> > Stan
> > >> >
> > >> >>
> > >> >> >
> > >> >> > BR,
> > >> >> > Jani.
> > >> >> >
> > >> >> > > Another thing is that probably enabling second VDSC is cheaper
> > >> >> > > in terms of power consumption, than bumping up the CDCLK.
> > >> >> > >
> > >> >> > > Stan
> > >> >> > >
> > >> >> > >> >
> > >> >> > >> > Okay , So you suggest that we set the cd clock to max when
> > >> >> > >> > we
> > >> have such requirement, than enabling the second engine?
> > >> >> > >>
> > >> >> > >> That seems like the easiest solution. Another option might be
> > >> >> > >> to come up with some lower dotclock limit for the use of the
> > >> >> > >> second vdsc. But not sure we know where the tipping point is
> > >> >> > >> wrt. powr
> > >> consumption.
> > >> >> > >>
> > >> >> > >> --
> > >> >> > >> Ville Syrjälä
> > >> >> > >> Intel
> > >> >> >
> > >> >> > --
> > >> >> > Jani Nikula, Intel Open Source Graphics Center
> > >>
> > >> --
> > >> Jani Nikula, Intel Open Source Graphics Center
> > 
> > --
> > Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita Jan. 11, 2022, 4:24 a.m. UTC | #15

> -----Original Message-----
> From: Navare, Manasi D <[e-mail]>
> Sent: Tuesday, January 11, 2022 1:07 AM
> To: Kulkarni, Vandita <[e-mail]>
> Cc: Nikula, Jani <[e-mail]>; Lisovskiy, Stanislav
> <[e-mail]>; Ville Syrjälä <[e-mail]>;
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> Thankf for revisiting this thread. The use of max_cdclk is currently in 2 places
> in DSC code 1. . if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq)
> {
>                 if (pipe_config->dsc.slice_count > 1) {
>                         pipe_config->dsc.dsc_split = true; 2. if (bigjoiner) {
>                 u32 max_bpp_bigjoiner =
>                         i915->max_cdclk_freq * 48 /
>                         intel_dp_mode_to_fec_clock(mode_clock);
> 
>                 DRM_DEBUG_KMS("Max big joiner bpp: %u\n",
> max_bpp_bigjoiner);
>                 bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>         }
> 
> In both these places, using max_cdclk can cause problems, like for
> compressed bpp it can give a higher bpp based on max_cdclk and we might
> actually end up chosing lower cdclk at what point this will cause underruns.
> 
> So when I was discussing with Ville on this, my first thought was also to use
> the cdclk_state->actual_cdclk but like Ville mentioned later in the review
> comments the challenge there was that actual cdclk does get computed
> much later than dsc_compute_config.
> 
> So I think as suggested in one of the reviews we just to check if DSC is
> enabled then we dont allow lowering the cdclk which would also prevent
> underruns caused by possibly setting up higher bpp based on max cdclk.

Thanks for the review.
This is taken care now, in case if we cannot split, then we already are using max_cdclk.
Regarding the bigjoiner_bpp, you may need to make the change if you need it to be set to max cdclk.
As we all of us here agree that we do not have computed cd clk at that time.

> 
> @Ville @Jani does this sound like a good approach. Then @Vandita we can
> pursue that change.
> 
> Regards
> Manasi
> 
> On Sun, Jan 09, 2022 at 11:15:04PM -0800, Kulkarni, Vandita wrote:
> > Revisiting this thread after update from the bspec.
> >
> > > -----Original Message-----
> > > From: Nikula, Jani <[e-mail]>
> > > Sent: Tuesday, September 14, 2021 8:40 PM
> > > To: Kulkarni, Vandita <[e-mail]>; Lisovskiy,
> > > Stanislav <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>; intel-
> > > [e-mail]; Navare, Manasi D
> > > <[e-mail]>
> > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > VDSC engine for higher moderates
> > >
> > > On Tue, 14 Sep 2021, "Kulkarni, Vandita"
> > > <[e-mail]>
> > > wrote:
> > > >> -----Original Message-----
> > > >> From: Nikula, Jani <[e-mail]>
> > > >> Sent: Tuesday, September 14, 2021 7:33 PM
> > > >> To: Lisovskiy, Stanislav <[e-mail]>
> > > >> Cc: Ville Syrjälä <[e-mail]>; Kulkarni,
> > > >> Vandita <[e-mail]>;
> > > >> [e-mail]; Navare, Manasi D
> > > >> <[e-mail]>
> > > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > >> VDSC engine for higher moderates
> > > >>
> > > >> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > >> <[e-mail]>
> > > >> wrote:
> > > >> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> > > >> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> > > >> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > >> <[e-mail]> wrote:
> > > >> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> > > >> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni,
> > > >> >> > >> Vandita
> > > wrote:
> > > >> >> > >> > > -----Original Message-----
> > > >> >> > >> > > From: Ville Syrjälä <[e-mail]>
> > > >> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > > >> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
> > > >> >> > >> > > Cc: [e-mail]; Nikula, Jani
> > > >> >> > >> > > <[e-mail]>; Navare, Manasi D
> > > >> >> > >> > > <[e-mail]>
> > > >> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display:
> > > >> >> > >> > > Enable second VDSC engine for higher moderates
> > > >> >> > >> > >
> > > >> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita
> > > >> >> > >> > > Kulkarni
> > > >> wrote:
> > > >> >> > >> > > > Each VDSC operates with 1ppc throughput, hence
> > > >> >> > >> > > > enable the second VDSC engine when moderate is
> > > >> >> > >> > > > higher that the current
> > > >> cdclk.
> > > >> >> > >> > > >
> > > >> >> > >> > > > Signed-off-by: Vandita Kulkarni
> > > >> >> > >> > > > <[e-mail]>
> > > >> >> > >> > > > ---
> > > >> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12
> > > >> >> > >> > > > ++++++++++--
> > > >> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >> >> > >> > > >
> > > >> >> > >> > > > diff --git
> > > >> >> > >> > > > a/drivers/gpu/drm/i915/display/intel_dp.c
> > > >> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > >> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> > > >> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > >> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > >> >> > >> > > > @@ -70,6 +70,7 @@
> > > >> >> > >> > > >  #include "intel_tc.h"
> > > >> >> > >> > > >  #include "intel_vdsc.h"
> > > >> >> > >> > > >  #include "intel_vrr.h"
> > > >> >> > >> > > > +#include "intel_cdclk.h"
> > > >> >> > >> > > >
> > > >> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> > > >> >> > >> > > >
> > > >> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > >> >> > >> > > intel_dp *intel_dp,
> > > >> >> > >> > > >  				       struct
> > > drm_connector_state
> > > >> *conn_state,
> > > >> >> > >> > > >  				       struct link_config_limits
> > > *limits)  {
> > > >> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> > > >> >> > >> > > >  	struct intel_digital_port *dig_port =
> > > >> dp_to_dig_port(intel_dp);
> > > >> >> > >> > > >  	struct drm_i915_private *dev_priv =
> > > to_i915(dig_port-
> > > >> >> > >> > > >base.base.dev);
> > > >> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> > > >> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> > > >> >> > >> > > > +	struct intel_atomic_state *state =
> > > >> >> > >> > > > +
> > > 	to_intel_atomic_state(pipe_config-
> > > >> >> > >> > > >uapi.state);
> > > >> >> > >> > > >  	int pipe_bpp;
> > > >> >> > >> > > >  	int ret;
> > > >> >> > >> > > >
> > > >> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > >> >> > >> > > intel_dp *intel_dp,
> > > >> >> > >> > > >  		}
> > > >> >> > >> > > >  	}
> > > >> >> > >> > > >
> > > >> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > > >> >> > >> > > > +	if (IS_ERR(cdclk_state))
> > > >> >> > >> > > > +		return PTR_ERR(cdclk_state);
> > > >> >> > >> > > > +
> > > >> >> > >> > > >  	/*
> > > >> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so
> > > >> >> > >> > > > if peak pixel
> > > >> rate
> > > >> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice
> > > count is
> > > >> even
> > > >> >> > >> > > > +	 * is greater than the current Cdclock and if
> > > >> >> > >> > > > +slice count is even
> > > >> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> > > >> >> > >> > > >  	 */
> > > >> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv-
> > > >max_cdclk_freq
> > > >> ||
> > > >> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> > > >> >> > >> > > > +cdclk_state->actual.cdclk ||
> > > >> >> > >> > >
> > > >> >> > >> > > This is wrong. We compute the cdclk based on the
> > > >> >> > >> > > requirements of the mode/etc., not the other way
> around.
> > > >> >> > >
> > > >> >> > > According to BSpec guideline, we decide whether we enable
> > > >> >> > > or disable second VDSC engine, based on that condition. As
> > > >> >> > > I understand that one is about DSC config calculation,
> > > >> >> > > based on CDCLK
> > > >> which was calculated.
> > > >> >> >
> > > >> >> > Point is, at the time compute_config gets called, what
> > > >> >> > guarantees are there that cdclk_state->actual.cdclk contains
> > > anything useful?
> > > >> >> > This is the design we have.
> > > >> >>
> > > >> >> That is actually good question, was willing to check that as well.
> > > >> >>
> > > >> >> >
> > > >> >> > > If we bump up CDCLK, to avoid this, will we even then use
> > > >> >> > > a second
> > > >> VDSC ever?
> > > >> >> >
> > > >> >> > I think we'll eventually need better logic than
> > > >> >> > unconditionally bumping to max, and it needs to take *both*
> > > >> >> > the cdclk and the number of dsc engines into account. The
> > > >> >> > referenced bspec only has the vdsc clock perspective, not overall
> perspective.
> > > >> >>
> > > >> >> What we need to clarify here is that how this is supposed to
> > > >> >> work in
> > > >> theory.
> > > >> >> Basically same issue can be fixed by both increasing the CDCLK
> > > >> >> or enabling 2nd VDSC engine.
> > > >> >> There should be some guideline telling us, how to prioritize.
> > > >> >> From overall perspective as I understand, by default, we are
> > > >> >> able to keep CDCLK 2 times less than pixel rate(see
> > > >> >> intel_pixel_rate_to_cdclk), however due to that VDSC
> > > >> >> limitation that it can use only 1 ppc this becomes, not
> > > >> >> applicable anymore(at least as of BSpec 49259), so we have to
> > > >> >> increase amount of VDSC instances
> > > >> then.
> > > >> >>
> > > >> >> So the question is now - what is more optimal here?
> > > >> >> Also if we bump up CDCLK(which we have done many times
> already
> > > >> >> in fact), we then need to add some logic to
> > > >> >> intel_compute_min_cdclk to check if we are using DSC or not,
> > > >> >> because otherwise we don't really need
> > > >> to do that.
> > > >>
> > > >> intel_compute_min_cdclk() already needs to be dsc aware when
> > > >> slice count is 1 and we can't use two dsc engines anyway. See the
> > > >> recent commit fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC
> enabled").
> > > >>
> > > >> Looking again, I'm not sure that does the right decision for when
> > > >> dsc.slice_count > 1, but dsc.split == false. It should probably
> > > >> use dsc.split for the decision.
> > > >>
> > > >> >>
> > > >> >> Stan
> > > >> >
> > > >> > Checked and indeed, encoder->compute_config is called way
> > > >> > before, basically CDCLK calculation is called almost in the end
> > > >> > of atomic_check, so in compute_config, there would be an old
> > > >> > CDCLK value copied from previous cdclk state, but not the last one.
> > > >> >
> > > >> > Vandita, this means we actually can't do it that way, if you
> > > >> > want to do anything with VDSC based on CDCLK this has to be
> > > >> > done _after_ intel_compute_min_cdclk was called. Which is not
> very sweet, I guess.
> > > >> >
> > > >> > So as of current architecture, it seems that the easiest way is
> > > >> > indeed to bump the CDCLK or we need to figure the way how to
> > > >> > enable 2nd VDSC somewhere else, after CDCLK was calculated.
> > > >>
> > > >> Alternatively, we could use two dsc engines more aggressively,
> > > >> but that decision currently can't take overall chosen cdclk into account.
> > > >>
> > > >> We'll end up sometimes unnecessarily using a too high cdclk or
> > > >> two dsc engines, just have to pick the poison.
> > > >>
> > > >> I think trying to do dsc decisions after
> > > >> intel_compute_min_cdclk() gets way too complicated.
> > > >
> > > > In this case, can we just use the 2nd VDSC engine if slice_count
> > > > is 2 or
> > > more?
> > > > Which would mean we always operate in joiner enabled mode(small
> > > > joiner) of all the compression modes of operation mentioned in the
> > > > table bspec: 49259 Because we are still going to hit the max cdclk
> > > > restriction
> > > for higher resolutions, and many lower resolutions wouldn’t need max
> cdclk.
> > > > And eventually once we have more details on cd clk vs 2VDSC engine
> > > > we could add the logic to choose one over the other?
> > > >
> > > > I see that in case of DSI we do split = true, for slice_count > 1
> > > > but that
> > > would need a different set of checks, thats a TBD.
> > > >
> > > > Or Do you suggest I just do this for now max cdclk when
> > > > slice_count =1 (what we are doing now) replace with compression =
> > > > true and split = false
> > >
> > > I think the check in intel_compute_min_cdclk() should be:
> > >
> > > 	if (crtc_state->dsc.compression_enable && !crtc_state-
> > > >dsc.dsc_split)
> > >
> > > That's a separate change.
> > >
> > > Enabling two dsc engines more aggressively... I don't mind doing it
> > > unconditionally when slice count > 1 for starters. But I think we'll
> > > need to improve this going forward, including fixing the mode valid
> > > checks etc. as we've discussed.
> >
> > Design recommendation is to use 2 VDSC instances while meeting the
> following constraint so that cdclk can stay as low as possible.
> > DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth

Based on the hw recommendation to me it looks like, as @Nikula, Jani suggested
We can set split to true based on " DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth"

In such cases we can avoid bumping up to max cdclk for vdsc reasons.
Will make this change and float V2.

Thanks,
Vandita
> >
> > Thanks,
> > Vandita
> > >
> > > Ville, any objections?
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > >
> > > > Thanks,
> > > > Vandita
> > > >>
> > > >> BR,
> > > >> Jani
> > > >>
> > > >>
> > > >>
> > > >>
> > > >> >
> > > >> > Stan
> > > >> >
> > > >> >>
> > > >> >> >
> > > >> >> > BR,
> > > >> >> > Jani.
> > > >> >> >
> > > >> >> > > Another thing is that probably enabling second VDSC is
> > > >> >> > > cheaper in terms of power consumption, than bumping up the
> CDCLK.
> > > >> >> > >
> > > >> >> > > Stan
> > > >> >> > >
> > > >> >> > >> >
> > > >> >> > >> > Okay , So you suggest that we set the cd clock to max
> > > >> >> > >> > when we
> > > >> have such requirement, than enabling the second engine?
> > > >> >> > >>
> > > >> >> > >> That seems like the easiest solution. Another option
> > > >> >> > >> might be to come up with some lower dotclock limit for
> > > >> >> > >> the use of the second vdsc. But not sure we know where
> > > >> >> > >> the tipping point is wrt. powr
> > > >> consumption.
> > > >> >> > >>
> > > >> >> > >> --
> > > >> >> > >> Ville Syrjälä
> > > >> >> > >> Intel
> > > >> >> >
> > > >> >> > --
> > > >> >> > Jani Nikula, Intel Open Source Graphics Center
> > > >>
> > > >> --
> > > >> Jani Nikula, Intel Open Source Graphics Center
> > >
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center

Navare, Manasi Jan. 11, 2022, 6:12 a.m. UTC | #16

On Mon, Jan 10, 2022 at 08:24:54PM -0800, Kulkarni, Vandita wrote:
> > -----Original Message-----
> > From: Navare, Manasi D <[e-mail]>
> > Sent: Tuesday, January 11, 2022 1:07 AM
> > To: Kulkarni, Vandita <[e-mail]>
> > Cc: Nikula, Jani <[e-mail]>; Lisovskiy, Stanislav
> > <[e-mail]>; Ville Syrjälä <[e-mail]>;
> > [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> > engine for higher moderates
> > 
> > Thankf for revisiting this thread. The use of max_cdclk is currently in 2 places
> > in DSC code 1. . if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq)
> > {
> >                 if (pipe_config->dsc.slice_count > 1) {
> >                         pipe_config->dsc.dsc_split = true; 2. if (bigjoiner) {
> >                 u32 max_bpp_bigjoiner =
> >                         i915->max_cdclk_freq * 48 /
> >                         intel_dp_mode_to_fec_clock(mode_clock);
> > 
> >                 DRM_DEBUG_KMS("Max big joiner bpp: %u\n",
> > max_bpp_bigjoiner);
> >                 bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> >         }
> > 
> > In both these places, using max_cdclk can cause problems, like for
> > compressed bpp it can give a higher bpp based on max_cdclk and we might
> > actually end up chosing lower cdclk at what point this will cause underruns.
> > 
> > So when I was discussing with Ville on this, my first thought was also to use
> > the cdclk_state->actual_cdclk but like Ville mentioned later in the review
> > comments the challenge there was that actual cdclk does get computed
> > much later than dsc_compute_config.
> > 
> > So I think as suggested in one of the reviews we just to check if DSC is
> > enabled then we dont allow lowering the cdclk which would also prevent
> > underruns caused by possibly setting up higher bpp based on max cdclk.
> 
> Thanks for the review.
> This is taken care now, in case if we cannot split, then we already are using max_cdclk.
> Regarding the bigjoiner_bpp, you may need to make the change if you need it to be set to max cdclk.
> As we all of us here agree that we do not have computed cd clk at that time.
>

So the resolution was to keep it at max_cdclock when we set the dsc.split  and decide to use 2 VDSC engines? Is this change merged upstream now?
For bpp calculation, that was pointed out by Srikanth that they were seeing underruns and was a bug they had found in their code.
And infact there we dont want to set it to max_cdclk because if we lower the cdclk later then that bpp will cause underuns.
One of the comments in here was to then check if DSC enabled in function that tries to lower the cdclk and not allow if DSC enabled.
Are you already working on that change or do I need to follow up on that with Ville/ Jani?

Manasi
 
> > 
> > @Ville @Jani does this sound like a good approach. Then @Vandita we can
> > pursue that change.
> > 
> > Regards
> > Manasi
> > 
> > On Sun, Jan 09, 2022 at 11:15:04PM -0800, Kulkarni, Vandita wrote:
> > > Revisiting this thread after update from the bspec.
> > >
> > > > -----Original Message-----
> > > > From: Nikula, Jani <[e-mail]>
> > > > Sent: Tuesday, September 14, 2021 8:40 PM
> > > > To: Kulkarni, Vandita <[e-mail]>; Lisovskiy,
> > > > Stanislav <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>; intel-
> > > > [e-mail]; Navare, Manasi D
> > > > <[e-mail]>
> > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > > VDSC engine for higher moderates
> > > >
> > > > On Tue, 14 Sep 2021, "Kulkarni, Vandita"
> > > > <[e-mail]>
> > > > wrote:
> > > > >> -----Original Message-----
> > > > >> From: Nikula, Jani <[e-mail]>
> > > > >> Sent: Tuesday, September 14, 2021 7:33 PM
> > > > >> To: Lisovskiy, Stanislav <[e-mail]>
> > > > >> Cc: Ville Syrjälä <[e-mail]>; Kulkarni,
> > > > >> Vandita <[e-mail]>;
> > > > >> [e-mail]; Navare, Manasi D
> > > > >> <[e-mail]>
> > > > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > > >> VDSC engine for higher moderates
> > > > >>
> > > > >> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > > >> <[e-mail]>
> > > > >> wrote:
> > > > >> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav wrote:
> > > > >> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> > > > >> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > > >> <[e-mail]> wrote:
> > > > >> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä wrote:
> > > > >> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni,
> > > > >> >> > >> Vandita
> > > > wrote:
> > > > >> >> > >> > > -----Original Message-----
> > > > >> >> > >> > > From: Ville Syrjälä <[e-mail]>
> > > > >> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > > > >> >> > >> > > To: Kulkarni, Vandita <[e-mail]>
> > > > >> >> > >> > > Cc: [e-mail]; Nikula, Jani
> > > > >> >> > >> > > <[e-mail]>; Navare, Manasi D
> > > > >> >> > >> > > <[e-mail]>
> > > > >> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display:
> > > > >> >> > >> > > Enable second VDSC engine for higher moderates
> > > > >> >> > >> > >
> > > > >> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita
> > > > >> >> > >> > > Kulkarni
> > > > >> wrote:
> > > > >> >> > >> > > > Each VDSC operates with 1ppc throughput, hence
> > > > >> >> > >> > > > enable the second VDSC engine when moderate is
> > > > >> >> > >> > > > higher that the current
> > > > >> cdclk.
> > > > >> >> > >> > > >
> > > > >> >> > >> > > > Signed-off-by: Vandita Kulkarni
> > > > >> >> > >> > > > <[e-mail]>
> > > > >> >> > >> > > > ---
> > > > >> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12
> > > > >> >> > >> > > > ++++++++++--
> > > > >> >> > >> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >> >> > >> > > >
> > > > >> >> > >> > > > diff --git
> > > > >> >> > >> > > > a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > >> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > >> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> > > > >> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > >> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > >> >> > >> > > > @@ -70,6 +70,7 @@
> > > > >> >> > >> > > >  #include "intel_tc.h"
> > > > >> >> > >> > > >  #include "intel_vdsc.h"
> > > > >> >> > >> > > >  #include "intel_vrr.h"
> > > > >> >> > >> > > > +#include "intel_cdclk.h"
> > > > >> >> > >> > > >
> > > > >> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> > > > >> >> > >> > > >
> > > > >> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> > > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > > >> >> > >> > > intel_dp *intel_dp,
> > > > >> >> > >> > > >  				       struct
> > > > drm_connector_state
> > > > >> *conn_state,
> > > > >> >> > >> > > >  				       struct link_config_limits
> > > > *limits)  {
> > > > >> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> > > > >> >> > >> > > >  	struct intel_digital_port *dig_port =
> > > > >> dp_to_dig_port(intel_dp);
> > > > >> >> > >> > > >  	struct drm_i915_private *dev_priv =
> > > > to_i915(dig_port-
> > > > >> >> > >> > > >base.base.dev);
> > > > >> >> > >> > > >  	const struct drm_display_mode *adjusted_mode =
> > > > >> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> > > > >> >> > >> > > > +	struct intel_atomic_state *state =
> > > > >> >> > >> > > > +
> > > > 	to_intel_atomic_state(pipe_config-
> > > > >> >> > >> > > >uapi.state);
> > > > >> >> > >> > > >  	int pipe_bpp;
> > > > >> >> > >> > > >  	int ret;
> > > > >> >> > >> > > >
> > > > >> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> > > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > > >> >> > >> > > intel_dp *intel_dp,
> > > > >> >> > >> > > >  		}
> > > > >> >> > >> > > >  	}
> > > > >> >> > >> > > >
> > > > >> >> > >> > > > +	cdclk_state = intel_atomic_get_cdclk_state(state);
> > > > >> >> > >> > > > +	if (IS_ERR(cdclk_state))
> > > > >> >> > >> > > > +		return PTR_ERR(cdclk_state);
> > > > >> >> > >> > > > +
> > > > >> >> > >> > > >  	/*
> > > > >> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock, so
> > > > >> >> > >> > > > if peak pixel
> > > > >> rate
> > > > >> >> > >> > > > -	 * is greater than the maximum Cdclock and if slice
> > > > count is
> > > > >> even
> > > > >> >> > >> > > > +	 * is greater than the current Cdclock and if
> > > > >> >> > >> > > > +slice count is even
> > > > >> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> > > > >> >> > >> > > >  	 */
> > > > >> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv-
> > > > >max_cdclk_freq
> > > > >> ||
> > > > >> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> > > > >> >> > >> > > > +cdclk_state->actual.cdclk ||
> > > > >> >> > >> > >
> > > > >> >> > >> > > This is wrong. We compute the cdclk based on the
> > > > >> >> > >> > > requirements of the mode/etc., not the other way
> > around.
> > > > >> >> > >
> > > > >> >> > > According to BSpec guideline, we decide whether we enable
> > > > >> >> > > or disable second VDSC engine, based on that condition. As
> > > > >> >> > > I understand that one is about DSC config calculation,
> > > > >> >> > > based on CDCLK
> > > > >> which was calculated.
> > > > >> >> >
> > > > >> >> > Point is, at the time compute_config gets called, what
> > > > >> >> > guarantees are there that cdclk_state->actual.cdclk contains
> > > > anything useful?
> > > > >> >> > This is the design we have.
> > > > >> >>
> > > > >> >> That is actually good question, was willing to check that as well.
> > > > >> >>
> > > > >> >> >
> > > > >> >> > > If we bump up CDCLK, to avoid this, will we even then use
> > > > >> >> > > a second
> > > > >> VDSC ever?
> > > > >> >> >
> > > > >> >> > I think we'll eventually need better logic than
> > > > >> >> > unconditionally bumping to max, and it needs to take *both*
> > > > >> >> > the cdclk and the number of dsc engines into account. The
> > > > >> >> > referenced bspec only has the vdsc clock perspective, not overall
> > perspective.
> > > > >> >>
> > > > >> >> What we need to clarify here is that how this is supposed to
> > > > >> >> work in
> > > > >> theory.
> > > > >> >> Basically same issue can be fixed by both increasing the CDCLK
> > > > >> >> or enabling 2nd VDSC engine.
> > > > >> >> There should be some guideline telling us, how to prioritize.
> > > > >> >> From overall perspective as I understand, by default, we are
> > > > >> >> able to keep CDCLK 2 times less than pixel rate(see
> > > > >> >> intel_pixel_rate_to_cdclk), however due to that VDSC
> > > > >> >> limitation that it can use only 1 ppc this becomes, not
> > > > >> >> applicable anymore(at least as of BSpec 49259), so we have to
> > > > >> >> increase amount of VDSC instances
> > > > >> then.
> > > > >> >>
> > > > >> >> So the question is now - what is more optimal here?
> > > > >> >> Also if we bump up CDCLK(which we have done many times
> > already
> > > > >> >> in fact), we then need to add some logic to
> > > > >> >> intel_compute_min_cdclk to check if we are using DSC or not,
> > > > >> >> because otherwise we don't really need
> > > > >> to do that.
> > > > >>
> > > > >> intel_compute_min_cdclk() already needs to be dsc aware when
> > > > >> slice count is 1 and we can't use two dsc engines anyway. See the
> > > > >> recent commit fe01883fdcef ("drm/i915: Get proper min cdclk if vDSC
> > enabled").
> > > > >>
> > > > >> Looking again, I'm not sure that does the right decision for when
> > > > >> dsc.slice_count > 1, but dsc.split == false. It should probably
> > > > >> use dsc.split for the decision.
> > > > >>
> > > > >> >>
> > > > >> >> Stan
> > > > >> >
> > > > >> > Checked and indeed, encoder->compute_config is called way
> > > > >> > before, basically CDCLK calculation is called almost in the end
> > > > >> > of atomic_check, so in compute_config, there would be an old
> > > > >> > CDCLK value copied from previous cdclk state, but not the last one.
> > > > >> >
> > > > >> > Vandita, this means we actually can't do it that way, if you
> > > > >> > want to do anything with VDSC based on CDCLK this has to be
> > > > >> > done _after_ intel_compute_min_cdclk was called. Which is not
> > very sweet, I guess.
> > > > >> >
> > > > >> > So as of current architecture, it seems that the easiest way is
> > > > >> > indeed to bump the CDCLK or we need to figure the way how to
> > > > >> > enable 2nd VDSC somewhere else, after CDCLK was calculated.
> > > > >>
> > > > >> Alternatively, we could use two dsc engines more aggressively,
> > > > >> but that decision currently can't take overall chosen cdclk into account.
> > > > >>
> > > > >> We'll end up sometimes unnecessarily using a too high cdclk or
> > > > >> two dsc engines, just have to pick the poison.
> > > > >>
> > > > >> I think trying to do dsc decisions after
> > > > >> intel_compute_min_cdclk() gets way too complicated.
> > > > >
> > > > > In this case, can we just use the 2nd VDSC engine if slice_count
> > > > > is 2 or
> > > > more?
> > > > > Which would mean we always operate in joiner enabled mode(small
> > > > > joiner) of all the compression modes of operation mentioned in the
> > > > > table bspec: 49259 Because we are still going to hit the max cdclk
> > > > > restriction
> > > > for higher resolutions, and many lower resolutions wouldn’t need max
> > cdclk.
> > > > > And eventually once we have more details on cd clk vs 2VDSC engine
> > > > > we could add the logic to choose one over the other?
> > > > >
> > > > > I see that in case of DSI we do split = true, for slice_count > 1
> > > > > but that
> > > > would need a different set of checks, thats a TBD.
> > > > >
> > > > > Or Do you suggest I just do this for now max cdclk when
> > > > > slice_count =1 (what we are doing now) replace with compression =
> > > > > true and split = false
> > > >
> > > > I think the check in intel_compute_min_cdclk() should be:
> > > >
> > > > 	if (crtc_state->dsc.compression_enable && !crtc_state-
> > > > >dsc.dsc_split)
> > > >
> > > > That's a separate change.
> > > >
> > > > Enabling two dsc engines more aggressively... I don't mind doing it
> > > > unconditionally when slice count > 1 for starters. But I think we'll
> > > > need to improve this going forward, including fixing the mode valid
> > > > checks etc. as we've discussed.
> > >
> > > Design recommendation is to use 2 VDSC instances while meeting the
> > following constraint so that cdclk can stay as low as possible.
> > > DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth
> 
> Based on the hw recommendation to me it looks like, as @Nikula, Jani suggested
> We can set split to true based on " DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth"
> 
> In such cases we can avoid bumping up to max cdclk for vdsc reasons.
> Will make this change and float V2.
> 
> Thanks,
> Vandita
> > >
> > > Thanks,
> > > Vandita
> > > >
> > > > Ville, any objections?
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Vandita
> > > > >>
> > > > >> BR,
> > > > >> Jani
> > > > >>
> > > > >>
> > > > >>
> > > > >>
> > > > >> >
> > > > >> > Stan
> > > > >> >
> > > > >> >>
> > > > >> >> >
> > > > >> >> > BR,
> > > > >> >> > Jani.
> > > > >> >> >
> > > > >> >> > > Another thing is that probably enabling second VDSC is
> > > > >> >> > > cheaper in terms of power consumption, than bumping up the
> > CDCLK.
> > > > >> >> > >
> > > > >> >> > > Stan
> > > > >> >> > >
> > > > >> >> > >> >
> > > > >> >> > >> > Okay , So you suggest that we set the cd clock to max
> > > > >> >> > >> > when we
> > > > >> have such requirement, than enabling the second engine?
> > > > >> >> > >>
> > > > >> >> > >> That seems like the easiest solution. Another option
> > > > >> >> > >> might be to come up with some lower dotclock limit for
> > > > >> >> > >> the use of the second vdsc. But not sure we know where
> > > > >> >> > >> the tipping point is wrt. powr
> > > > >> consumption.
> > > > >> >> > >>
> > > > >> >> > >> --
> > > > >> >> > >> Ville Syrjälä
> > > > >> >> > >> Intel
> > > > >> >> >
> > > > >> >> > --
> > > > >> >> > Jani Nikula, Intel Open Source Graphics Center
> > > > >>
> > > > >> --
> > > > >> Jani Nikula, Intel Open Source Graphics Center
> > > >
> > > > --
> > > > Jani Nikula, Intel Open Source Graphics Center

Kulkarni, Vandita Jan. 11, 2022, 6:26 a.m. UTC | #17

> -----Original Message-----
> From: Navare, Manasi D <[e-mail]>
> Sent: Tuesday, January 11, 2022 11:42 AM
> To: Kulkarni, Vandita <[e-mail]>
> Cc: Nikula, Jani <[e-mail]>; Lisovskiy, Stanislav
> <[e-mail]>; Ville Syrjälä <[e-mail]>;
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC
> engine for higher moderates
> 
> On Mon, Jan 10, 2022 at 08:24:54PM -0800, Kulkarni, Vandita wrote:
> > > -----Original Message-----
> > > From: Navare, Manasi D <[e-mail]>
> > > Sent: Tuesday, January 11, 2022 1:07 AM
> > > To: Kulkarni, Vandita <[e-mail]>
> > > Cc: Nikula, Jani <[e-mail]>; Lisovskiy, Stanislav
> > > <[e-mail]>; Ville Syrjälä
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > VDSC engine for higher moderates
> > >
> > > Thankf for revisiting this thread. The use of max_cdclk is currently
> > > in 2 places in DSC code 1. . if (adjusted_mode->crtc_clock >
> > > dev_priv->max_cdclk_freq) {
> > >                 if (pipe_config->dsc.slice_count > 1) {
> > >                         pipe_config->dsc.dsc_split = true; 2. if (bigjoiner) {
> > >                 u32 max_bpp_bigjoiner =
> > >                         i915->max_cdclk_freq * 48 /
> > >                         intel_dp_mode_to_fec_clock(mode_clock);
> > >
> > >                 DRM_DEBUG_KMS("Max big joiner bpp: %u\n",
> > > max_bpp_bigjoiner);
> > >                 bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> > >         }
> > >
> > > In both these places, using max_cdclk can cause problems, like for
> > > compressed bpp it can give a higher bpp based on max_cdclk and we
> > > might actually end up chosing lower cdclk at what point this will cause
> underruns.
> > >
> > > So when I was discussing with Ville on this, my first thought was
> > > also to use the cdclk_state->actual_cdclk but like Ville mentioned
> > > later in the review comments the challenge there was that actual
> > > cdclk does get computed much later than dsc_compute_config.
> > >
> > > So I think as suggested in one of the reviews we just to check if
> > > DSC is enabled then we dont allow lowering the cdclk which would
> > > also prevent underruns caused by possibly setting up higher bpp based
> on max cdclk.
> >
> > Thanks for the review.
> > This is taken care now, in case if we cannot split, then we already are using
> max_cdclk.
> > Regarding the bigjoiner_bpp, you may need to make the change if you
> need it to be set to max cdclk.
> > As we all of us here agree that we do not have computed cd clk at that
> time.
> >
> 

Just to clarify this patch doesn’t do anything wrt the big  joiner bpp calclulation.

> So the resolution was to keep it at max_cdclock when we set the dsc.split
> and decide to use 2 VDSC engines? 
 
The solution was to keep it at max_cdclk when we dsc.split is false.
>Is this change merged upstream now?
Yes
> For bpp calculation, that was pointed out by Srikanth that they were seeing
> underruns and was a bug they had found in their code.
> And infact there we dont want to set it to max_cdclk because if we lower the
> cdclk later then that bpp will cause underuns.

I am guessing you are talking about big joiner cases, this patch was made for addressing pure dsc cases
Where we were seeing underruns. Due to 1 ppc limitation.
> One of the comments in here was to then check if DSC enabled in function
> that tries to lower the cdclk and not allow if DSC enabled.
The suggestion was to check if we are not using split = true  (2 dsc engines), then use max cdclk.
PS : intel_crtc_compute_min_cdclk

This whole patch was to make sure that when we cannot use 2 dsc engine, to increase the cdclk.
Or use 2 engines based on the current cdclk, which is not possible.
And we were checking against max cdclk to enable second engine.

This got resolved with the change that is merged now, sets cdclk to max if we cannot enable the second vdsc engine.

The suggestion from the bspec is directing to use 2 vdsc engines based on the condition that I have mentioned below,
And that means we really do not need the check if (adjusted_mode->crtc_clock > dev_priv- max_cdclk_freq) to set split = true.

> Are you already working on that change or do I need to follow up on that
> with Ville/ Jani?

Here in this patch we are not talking about the bpp calculation based on cd clk at all, which comes into picture with big joiner.
If you want to make changes wrt bigjoiner, you may need to make another patch, which I am not working on.

As per my previous email: this is what I will be sending.
"> > Based on the hw recommendation to me it looks like, as @Nikula, Jani
> suggested
> > We can set split to true based on " DP/HDMI PPR spec provided slice size <
> DPCD provided MaxSliceWidth"
> >
> > In such cases we can avoid bumping up to max cdclk for vdsc reasons.
> > Will make this change and float V2.
> >
> > Thanks,
> > Vandita
"

> 
> Manasi
> 
> > >
> > > @Ville @Jani does this sound like a good approach. Then @Vandita we
> > > can pursue that change.
> > >
> > > Regards
> > > Manasi
> > >
> > > On Sun, Jan 09, 2022 at 11:15:04PM -0800, Kulkarni, Vandita wrote:
> > > > Revisiting this thread after update from the bspec.
> > > >
> > > > > -----Original Message-----
> > > > > From: Nikula, Jani <[e-mail]>
> > > > > Sent: Tuesday, September 14, 2021 8:40 PM
> > > > > To: Kulkarni, Vandita <[e-mail]>; Lisovskiy,
> > > > > Stanislav <[e-mail]>
> > > > > Cc: Ville Syrjälä <[e-mail]>; intel-
> > > > > [e-mail]; Navare, Manasi D
> > > > > <[e-mail]>
> > > > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Enable second
> > > > > VDSC engine for higher moderates
> > > > >
> > > > > On Tue, 14 Sep 2021, "Kulkarni, Vandita"
> > > > > <[e-mail]>
> > > > > wrote:
> > > > > >> -----Original Message-----
> > > > > >> From: Nikula, Jani <[e-mail]>
> > > > > >> Sent: Tuesday, September 14, 2021 7:33 PM
> > > > > >> To: Lisovskiy, Stanislav <[e-mail]>
> > > > > >> Cc: Ville Syrjälä <[e-mail]>; Kulkarni,
> > > > > >> Vandita <[e-mail]>;
> > > > > >> [e-mail]; Navare, Manasi D
> > > > > >> <[e-mail]>
> > > > > >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Enable
> > > > > >> second VDSC engine for higher moderates
> > > > > >>
> > > > > >> On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > > > >> <[e-mail]>
> > > > > >> wrote:
> > > > > >> > On Tue, Sep 14, 2021 at 04:04:25PM +0300, Lisovskiy, Stanislav
> wrote:
> > > > > >> >> On Tue, Sep 14, 2021 at 03:04:11PM +0300, Jani Nikula wrote:
> > > > > >> >> > On Tue, 14 Sep 2021, "Lisovskiy, Stanislav"
> > > > > >> <[e-mail]> wrote:
> > > > > >> >> > > On Tue, Sep 14, 2021 at 10:48:46AM +0300, Ville Syrjälä
> wrote:
> > > > > >> >> > >> On Tue, Sep 14, 2021 at 07:31:46AM +0000, Kulkarni,
> > > > > >> >> > >> Vandita
> > > > > wrote:
> > > > > >> >> > >> > > -----Original Message-----
> > > > > >> >> > >> > > From: Ville Syrjälä
> > > > > >> >> > >> > > <[e-mail]>
> > > > > >> >> > >> > > Sent: Tuesday, September 14, 2021 12:59 PM
> > > > > >> >> > >> > > To: Kulkarni, Vandita
> > > > > >> >> > >> > > <[e-mail]>
> > > > > >> >> > >> > > Cc: [e-mail]; Nikula, Jani
> > > > > >> >> > >> > > <[e-mail]>; Navare, Manasi D
> > > > > >> >> > >> > > <[e-mail]>
> > > > > >> >> > >> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display:
> > > > > >> >> > >> > > Enable second VDSC engine for higher moderates
> > > > > >> >> > >> > >
> > > > > >> >> > >> > > On Mon, Sep 13, 2021 at 08:09:23PM +0530, Vandita
> > > > > >> >> > >> > > Kulkarni
> > > > > >> wrote:
> > > > > >> >> > >> > > > Each VDSC operates with 1ppc throughput, hence
> > > > > >> >> > >> > > > enable the second VDSC engine when moderate is
> > > > > >> >> > >> > > > higher that the current
> > > > > >> cdclk.
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > Signed-off-by: Vandita Kulkarni
> > > > > >> >> > >> > > > <[e-mail]>
> > > > > >> >> > >> > > > ---
> > > > > >> >> > >> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 12
> > > > > >> >> > >> > > > ++++++++++--
> > > > > >> >> > >> > > >  1 file changed, 10 insertions(+), 2
> > > > > >> >> > >> > > > deletions(-)
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > diff --git
> > > > > >> >> > >> > > > a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > >> >> > >> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > >> >> > >> > > > index 161c33b2c869..55878f65f724 100644
> > > > > >> >> > >> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > >> >> > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > >> >> > >> > > > @@ -70,6 +70,7 @@  #include "intel_tc.h"
> > > > > >> >> > >> > > >  #include "intel_vdsc.h"
> > > > > >> >> > >> > > >  #include "intel_vrr.h"
> > > > > >> >> > >> > > > +#include "intel_cdclk.h"
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > >  #define DP_DPRX_ESI_LEN 14
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > @@ -1291,10 +1292,13 @@ static int
> > > > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > > > >> >> > >> > > intel_dp *intel_dp,
> > > > > >> >> > >> > > >  				       struct
> > > > > drm_connector_state
> > > > > >> *conn_state,
> > > > > >> >> > >> > > >  				       struct
> link_config_limits
> > > > > *limits)  {
> > > > > >> >> > >> > > > +	struct intel_cdclk_state *cdclk_state;
> > > > > >> >> > >> > > >  	struct intel_digital_port *dig_port =
> > > > > >> dp_to_dig_port(intel_dp);
> > > > > >> >> > >> > > >  	struct drm_i915_private *dev_priv =
> > > > > to_i915(dig_port-
> > > > > >> >> > >> > > >base.base.dev);
> > > > > >> >> > >> > > >  	const struct drm_display_mode
> *adjusted_mode =
> > > > > >> >> > >> > > >  		&pipe_config->hw.adjusted_mode;
> > > > > >> >> > >> > > > +	struct intel_atomic_state *state =
> > > > > >> >> > >> > > > +
> > > > > 	to_intel_atomic_state(pipe_config-
> > > > > >> >> > >> > > >uapi.state);
> > > > > >> >> > >> > > >  	int pipe_bpp;
> > > > > >> >> > >> > > >  	int ret;
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > @@ -1373,12 +1377,16 @@ static int
> > > > > >> >> > >> > > > intel_dp_dsc_compute_config(struct
> > > > > >> >> > >> > > intel_dp *intel_dp,
> > > > > >> >> > >> > > >  		}
> > > > > >> >> > >> > > >  	}
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > +	cdclk_state =
> intel_atomic_get_cdclk_state(state);
> > > > > >> >> > >> > > > +	if (IS_ERR(cdclk_state))
> > > > > >> >> > >> > > > +		return PTR_ERR(cdclk_state);
> > > > > >> >> > >> > > > +
> > > > > >> >> > >> > > >  	/*
> > > > > >> >> > >> > > >  	 * VDSC engine operates at 1 Pixel per clock,
> so
> > > > > >> >> > >> > > > if peak pixel
> > > > > >> rate
> > > > > >> >> > >> > > > -	 * is greater than the maximum Cdclock and if
> slice
> > > > > count is
> > > > > >> even
> > > > > >> >> > >> > > > +	 * is greater than the current Cdclock and if
> > > > > >> >> > >> > > > +slice count is even
> > > > > >> >> > >> > > >  	 * then we need to use 2 VDSC instances.
> > > > > >> >> > >> > > >  	 */
> > > > > >> >> > >> > > > -	if (adjusted_mode->crtc_clock > dev_priv-
> > > > > >max_cdclk_freq
> > > > > >> ||
> > > > > >> >> > >> > > > +	if (adjusted_mode->crtc_clock >
> > > > > >> >> > >> > > > +cdclk_state->actual.cdclk ||
> > > > > >> >> > >> > >
> > > > > >> >> > >> > > This is wrong. We compute the cdclk based on the
> > > > > >> >> > >> > > requirements of the mode/etc., not the other way
> > > around.
> > > > > >> >> > >
> > > > > >> >> > > According to BSpec guideline, we decide whether we
> enable
> > > > > >> >> > > or disable second VDSC engine, based on that condition. As
> > > > > >> >> > > I understand that one is about DSC config calculation,
> > > > > >> >> > > based on CDCLK
> > > > > >> which was calculated.
> > > > > >> >> >
> > > > > >> >> > Point is, at the time compute_config gets called, what
> > > > > >> >> > guarantees are there that cdclk_state->actual.cdclk contains
> > > > > anything useful?
> > > > > >> >> > This is the design we have.
> > > > > >> >>
> > > > > >> >> That is actually good question, was willing to check that as well.
> > > > > >> >>
> > > > > >> >> >
> > > > > >> >> > > If we bump up CDCLK, to avoid this, will we even then use
> > > > > >> >> > > a second
> > > > > >> VDSC ever?
> > > > > >> >> >
> > > > > >> >> > I think we'll eventually need better logic than
> > > > > >> >> > unconditionally bumping to max, and it needs to take *both*
> > > > > >> >> > the cdclk and the number of dsc engines into account. The
> > > > > >> >> > referenced bspec only has the vdsc clock perspective, not
> overall
> > > perspective.
> > > > > >> >>
> > > > > >> >> What we need to clarify here is that how this is supposed to
> > > > > >> >> work in
> > > > > >> theory.
> > > > > >> >> Basically same issue can be fixed by both increasing the CDCLK
> > > > > >> >> or enabling 2nd VDSC engine.
> > > > > >> >> There should be some guideline telling us, how to prioritize.
> > > > > >> >> From overall perspective as I understand, by default, we are
> > > > > >> >> able to keep CDCLK 2 times less than pixel rate(see
> > > > > >> >> intel_pixel_rate_to_cdclk), however due to that VDSC
> > > > > >> >> limitation that it can use only 1 ppc this becomes, not
> > > > > >> >> applicable anymore(at least as of BSpec 49259), so we have to
> > > > > >> >> increase amount of VDSC instances
> > > > > >> then.
> > > > > >> >>
> > > > > >> >> So the question is now - what is more optimal here?
> > > > > >> >> Also if we bump up CDCLK(which we have done many times
> > > already
> > > > > >> >> in fact), we then need to add some logic to
> > > > > >> >> intel_compute_min_cdclk to check if we are using DSC or not,
> > > > > >> >> because otherwise we don't really need
> > > > > >> to do that.
> > > > > >>
> > > > > >> intel_compute_min_cdclk() already needs to be dsc aware when
> > > > > >> slice count is 1 and we can't use two dsc engines anyway. See the
> > > > > >> recent commit fe01883fdcef ("drm/i915: Get proper min cdclk if
> vDSC
> > > enabled").
> > > > > >>
> > > > > >> Looking again, I'm not sure that does the right decision for when
> > > > > >> dsc.slice_count > 1, but dsc.split == false. It should probably
> > > > > >> use dsc.split for the decision.
> > > > > >>
> > > > > >> >>
> > > > > >> >> Stan
> > > > > >> >
> > > > > >> > Checked and indeed, encoder->compute_config is called way
> > > > > >> > before, basically CDCLK calculation is called almost in the end
> > > > > >> > of atomic_check, so in compute_config, there would be an old
> > > > > >> > CDCLK value copied from previous cdclk state, but not the last
> one.
> > > > > >> >
> > > > > >> > Vandita, this means we actually can't do it that way, if you
> > > > > >> > want to do anything with VDSC based on CDCLK this has to be
> > > > > >> > done _after_ intel_compute_min_cdclk was called. Which is not
> > > very sweet, I guess.
> > > > > >> >
> > > > > >> > So as of current architecture, it seems that the easiest way is
> > > > > >> > indeed to bump the CDCLK or we need to figure the way how to
> > > > > >> > enable 2nd VDSC somewhere else, after CDCLK was calculated.
> > > > > >>
> > > > > >> Alternatively, we could use two dsc engines more aggressively,
> > > > > >> but that decision currently can't take overall chosen cdclk into
> account.
> > > > > >>
> > > > > >> We'll end up sometimes unnecessarily using a too high cdclk or
> > > > > >> two dsc engines, just have to pick the poison.
> > > > > >>
> > > > > >> I think trying to do dsc decisions after
> > > > > >> intel_compute_min_cdclk() gets way too complicated.
> > > > > >
> > > > > > In this case, can we just use the 2nd VDSC engine if slice_count
> > > > > > is 2 or
> > > > > more?
> > > > > > Which would mean we always operate in joiner enabled
> mode(small
> > > > > > joiner) of all the compression modes of operation mentioned in the
> > > > > > table bspec: 49259 Because we are still going to hit the max cdclk
> > > > > > restriction
> > > > > for higher resolutions, and many lower resolutions wouldn’t need
> max
> > > cdclk.
> > > > > > And eventually once we have more details on cd clk vs 2VDSC
> engine
> > > > > > we could add the logic to choose one over the other?
> > > > > >
> > > > > > I see that in case of DSI we do split = true, for slice_count > 1
> > > > > > but that
> > > > > would need a different set of checks, thats a TBD.
> > > > > >
> > > > > > Or Do you suggest I just do this for now max cdclk when
> > > > > > slice_count =1 (what we are doing now) replace with compression =
> > > > > > true and split = false
> > > > >
> > > > > I think the check in intel_compute_min_cdclk() should be:
> > > > >
> > > > > 	if (crtc_state->dsc.compression_enable && !crtc_state-
> > > > > >dsc.dsc_split)
> > > > >
> > > > > That's a separate change.
> > > > >
> > > > > Enabling two dsc engines more aggressively... I don't mind doing it
> > > > > unconditionally when slice count > 1 for starters. But I think we'll
> > > > > need to improve this going forward, including fixing the mode valid
> > > > > checks etc. as we've discussed.
> > > >
> > > > Design recommendation is to use 2 VDSC instances while meeting the
> > > following constraint so that cdclk can stay as low as possible.
> > > > DP/HDMI PPR spec provided slice size < DPCD provided MaxSliceWidth
> >
> > Based on the hw recommendation to me it looks like, as @Nikula, Jani
> suggested
> > We can set split to true based on " DP/HDMI PPR spec provided slice size <
> DPCD provided MaxSliceWidth"
> >
> > In such cases we can avoid bumping up to max cdclk for vdsc reasons.
> > Will make this change and float V2.
> >
> > Thanks,
> > Vandita
> > > >
> > > > Thanks,
> > > > Vandita
> > > > >
> > > > > Ville, any objections?
> > > > >
> > > > > BR,
> > > > > Jani.
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Vandita
> > > > > >>
> > > > > >> BR,
> > > > > >> Jani
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> >
> > > > > >> > Stan
> > > > > >> >
> > > > > >> >>
> > > > > >> >> >
> > > > > >> >> > BR,
> > > > > >> >> > Jani.
> > > > > >> >> >
> > > > > >> >> > > Another thing is that probably enabling second VDSC is
> > > > > >> >> > > cheaper in terms of power consumption, than bumping up
> the
> > > CDCLK.
> > > > > >> >> > >
> > > > > >> >> > > Stan
> > > > > >> >> > >
> > > > > >> >> > >> >
> > > > > >> >> > >> > Okay , So you suggest that we set the cd clock to max
> > > > > >> >> > >> > when we
> > > > > >> have such requirement, than enabling the second engine?
> > > > > >> >> > >>
> > > > > >> >> > >> That seems like the easiest solution. Another option
> > > > > >> >> > >> might be to come up with some lower dotclock limit for
> > > > > >> >> > >> the use of the second vdsc. But not sure we know where
> > > > > >> >> > >> the tipping point is wrt. powr
> > > > > >> consumption.
> > > > > >> >> > >>
> > > > > >> >> > >> --
> > > > > >> >> > >> Ville Syrjälä
> > > > > >> >> > >> Intel
> > > > > >> >> >
> > > > > >> >> > --
> > > > > >> >> > Jani Nikula, Intel Open Source Graphics Center
> > > > > >>
> > > > > >> --
> > > > > >> Jani Nikula, Intel Open Source Graphics Center
> > > > >
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 161c33b2c869..55878f65f724 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -70,6 +70,7 @@ 
 #include "intel_tc.h"
 #include "intel_vdsc.h"
 #include "intel_vrr.h"
+#include "intel_cdclk.h"
 
 #define DP_DPRX_ESI_LEN 14
 
@@ -1291,10 +1292,13 @@  static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				       struct drm_connector_state *conn_state,
 				       struct link_config_limits *limits)
 {
+	struct intel_cdclk_state *cdclk_state;
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
 	const struct drm_display_mode *adjusted_mode =
 		&pipe_config->hw.adjusted_mode;
+	struct intel_atomic_state *state =
+				to_intel_atomic_state(pipe_config->uapi.state);
 	int pipe_bpp;
 	int ret;
 
@@ -1373,12 +1377,16 @@  static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 		}
 	}
 
+	cdclk_state = intel_atomic_get_cdclk_state(state);
+	if (IS_ERR(cdclk_state))
+		return PTR_ERR(cdclk_state);
+
 	/*
 	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
-	 * is greater than the maximum Cdclock and if slice count is even
+	 * is greater than the current Cdclock and if slice count is even
 	 * then we need to use 2 VDSC instances.
 	 */
-	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq ||
+	if (adjusted_mode->crtc_clock > cdclk_state->actual.cdclk ||
 	    pipe_config->bigjoiner) {
 		if (pipe_config->dsc.slice_count < 2) {
 			drm_dbg_kms(&dev_priv->drm,