Re: [Intel-gfx] [PATCH v2 07/20] drm/i915: Unify the low level dbuf code

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The low level dbuf slice code is rather inconsitent with its
> functiona naming and organization. Make it more consistent.
> 
> Also share the enable/disable functions between all platforms
> since the same code works just fine for all of them.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  6 +--
>  .../drm/i915/display/intel_display_power.c    | 44 ++++++++---------
> --
>  .../drm/i915/display/intel_display_power.h    |  6 +--
>  3 files changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 3031e64ee518..6952c398cc43 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c

> > On Wed, 2020-03-04 at 20:30 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 04, 2020 at 05:23:05PM +0000, Lisovskiy, Stanislav
> > wrote:
> > > > 
> > > > > -       /* If 2nd DBuf slice required, enable it here */
> > > > >        if (INTEL_GEN(dev_priv) >= 11 && slices_union !=
> > > > > hw_enabled_slices)
> > > > > -               icl_dbuf_slices_update(dev_priv,
> > slices_union);
> > > > > +               gen9_dbuf_slices_update(dev_priv,
> > slices_union);
> > > > > }
> > > > > static void icl_dbuf_slice_post_update(struct
> > intel_atomic_state
> > > > > *state)
> > > > > @@ -15307,9 +15306,8 @@ static void
> > > > > icl_dbuf_slice_post_update(struct intel_atomic_state *state)
> > > > >        u8 hw_enabled_slices = dev_priv-
> > >enabled_dbuf_slices_mask;
> > > > >        u8 required_slices = state->enabled_dbuf_slices_mask;
> > > > > -       /* If 2nd DBuf slice is no more required disable it
> > */
> > > > >         if (INTEL_GEN(dev_priv) >= 11 && required_slices !=
> > > > > hw_enabled_slices)
> > > > > -               icl_dbuf_slices_update(dev_priv,
> > > > > required_slices);
> > > > > +               gen9_dbuf_slices_update(dev_priv,
> > > > > required_slices);
> > > > 
> > > > 
> > > > Doesn't make much sense. Just look - previously we were
> > checking if
> > > > INTEL_GEN is >= than 11(which _is_ ICL)
> > > > 
> > > > and now we still _do_ check if INTEL_GEN is >= 11, but... call
> > now
> > > > function renamed to gen9
> > > > 
> > > > 
> > > > I guess you either need to change INTEL_GEN check to be >=9 to
> > at
> > > > least look somewhat consistent
> > > > 
> > > > or leave it as is. Or at least rename icl_ prefix to gen11_
> > > > otherwise that looks inconsistent, i.e
> > > > 
> > > > you are now checking that gen is >= 11 and then OK - now let's
> > call
> > > > gen 9! :)
> > > 
> > > The standard practice is to name things based on the oldest
> > platform
> > > that introduced the thing.
> > 

And that is fine - but then you need to change the check above from 
INTEL_GEN >= 11 to INTEL_GEN >= 9, right - if you gen9 is the oldest
platform. 

> > > -       /* If 2nd DBuf slice required, enable it here */
> > >        if (INTEL_GEN(dev_priv) >= 11 && slices_union !=
> > > hw_enabled_slices)
> > > -               icl_dbuf_slices_update(dev_priv, slices_union);
> > > +               gen9_dbuf_slices_update(dev_priv, slices_union);
> > > }

I mean previously we were checking INTEL_GEN to be at least 11 and
called function prefixed with icl_ - which was consistent and logical.

Now you changed this to gen9(oldest platform which introduced the
thing), however then the check above makes no sense - it should be
changed to INTEL_GEN >= 9 as well. Otherwise this
"gen9_dbuf_slices_update" function will not be actually ever called for
gen9.

Or do you want function prefixed as gen9_ to be only called for gen 11,
why we then prefix it..

Stan

> 
>  static void skl_commit_modeset_enables(struct intel_atomic_state
> *state)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index e81e561e8ac0..ce3bbc4c7a27 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -4433,15 +4433,18 @@ static void
> intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&power_domains->lock);
>  }
>  
> -static void intel_dbuf_slice_set(struct drm_i915_private *dev_priv,
> -				 enum dbuf_slice slice, bool enable)
> +static void gen9_dbuf_slice_set(struct drm_i915_private *dev_priv,
> +				enum dbuf_slice slice, bool enable)
>  {
>  	i915_reg_t reg = DBUF_CTL_S(slice);
>  	bool state;
>  	u32 val;
>  
>  	val = intel_de_read(dev_priv, reg);
> -	val = enable ? (val | DBUF_POWER_REQUEST) : (val &
> ~DBUF_POWER_REQUEST);
> +	if (enable)
> +		val |= DBUF_POWER_REQUEST;
> +	else
> +		val &= ~DBUF_POWER_REQUEST;
>  	intel_de_write(dev_priv, reg, val);
>  	intel_de_posting_read(dev_priv, reg);
>  	udelay(10);
> @@ -4452,18 +4455,8 @@ static void intel_dbuf_slice_set(struct
> drm_i915_private *dev_priv,
>  		 slice, enable ? "enable" : "disable");
>  }
>  
> -static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
> -{
> -	icl_dbuf_slices_update(dev_priv, BIT(DBUF_S1));
> -}
> -
> -static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
> -{
> -	icl_dbuf_slices_update(dev_priv, 0);
> -}
> -
> -void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> -			    u8 req_slices)
> +void gen9_dbuf_slices_update(struct drm_i915_private *dev_priv,
> +			     u8 req_slices)
>  {
>  	int num_slices = INTEL_INFO(dev_priv)-
> >num_supported_dbuf_slices;
>  	struct i915_power_domains *power_domains = &dev_priv-
> >power_domains;
> @@ -4486,28 +4479,29 @@ void icl_dbuf_slices_update(struct
> drm_i915_private *dev_priv,
>  	mutex_lock(&power_domains->lock);
>  
>  	for (slice = DBUF_S1; slice < num_slices; slice++)
> -		intel_dbuf_slice_set(dev_priv, slice,
> -				     req_slices & BIT(slice));
> +		gen9_dbuf_slice_set(dev_priv, slice, req_slices &
> BIT(slice));
>  
>  	dev_priv->enabled_dbuf_slices_mask = req_slices;
>  
>  	mutex_unlock(&power_domains->lock);
>  }
>  
> -static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> +static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
>  {
> -	skl_ddb_get_hw_state(dev_priv);
> +	dev_priv->enabled_dbuf_slices_mask =
> +		intel_enabled_dbuf_slices_mask(dev_priv);
> +
>  	/*
>  	 * Just power up at least 1 slice, we will
>  	 * figure out later which slices we have and what we need.
>  	 */
> -	icl_dbuf_slices_update(dev_priv, dev_priv-
> >enabled_dbuf_slices_mask |
> -			       BIT(DBUF_S1));
> +	gen9_dbuf_slices_update(dev_priv, BIT(DBUF_S1) |
> +				dev_priv->enabled_dbuf_slices_mask);
>  }
>  
> -static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> +static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
>  {
> -	icl_dbuf_slices_update(dev_priv, 0);
> +	gen9_dbuf_slices_update(dev_priv, 0);
>  }
>  
>  static void icl_mbus_init(struct drm_i915_private *dev_priv)
> @@ -5067,7 +5061,7 @@ static void icl_display_core_init(struct
> drm_i915_private *dev_priv,
>  	intel_cdclk_init_hw(dev_priv);
>  
>  	/* 5. Enable DBUF. */
> -	icl_dbuf_enable(dev_priv);
> +	gen9_dbuf_enable(dev_priv);
>  
>  	/* 6. Setup MBUS. */
>  	icl_mbus_init(dev_priv);
> @@ -5090,7 +5084,7 @@ static void icl_display_core_uninit(struct
> drm_i915_private *dev_priv)
>  	/* 1. Disable all display engine functions -> aready done */
>  
>  	/* 2. Disable DBUF */
> -	icl_dbuf_disable(dev_priv);
> +	gen9_dbuf_disable(dev_priv);
>  
>  	/* 3. Disable CD clock */
>  	intel_cdclk_uninit_hw(dev_priv);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 601e000ffd0d..1a275611241e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -312,13 +312,13 @@ enum dbuf_slice {
>  	DBUF_S2,
>  };
>  
> +void gen9_dbuf_slices_update(struct drm_i915_private *dev_priv,
> +			     u8 req_slices);
> +
>  #define with_intel_display_power(i915, domain, wf) \
>  	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
>  	     intel_display_power_put_async((i915), (domain), (wf)),
> (wf) = 0)
>  
> -void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> -			    u8 req_slices);
> -
>  void chv_phy_powergate_lanes(struct intel_encoder *encoder,
>  			     bool override, unsigned int mask);
>  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum
> dpio_phy phy,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx