Re: [Intel-gfx] [PATCH] drm/i915/display: Enable second VDSC engine for higher moderates

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 14 Sep 2021, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> 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ä <ville.syrjala@linux.intel.com>
>> > > Sent: Tuesday, September 14, 2021 12:59 PM
>> > > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>
>> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
>> > > Navare, Manasi D <manasi.d.navare@intel.com>
>> > > 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 <vandita.kulkarni@intel.com>
>> > > > ---
>> > > >  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

-- 
Jani Nikula, Intel Open Source Graphics Center