[Intel-gfx] [PATCH v2 07/20] drm/i915: Unify the low level dbuf code
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Mar 4 18:30:07 UTC 2020
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.
>
>
> Stan
>
> ________________________________
> From: Ville Syrjala <ville.syrjala at linux.intel.com>
> Sent: Tuesday, February 25, 2020 7:11:12 PM
> To: intel-gfx at lists.freedesktop.org
> Cc: Lisovskiy, Stanislav
> Subject: [PATCH v2 07/20] drm/i915: Unify the low level dbuf code
>
> From: Ville Syrjälä <ville.syrjala at 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 at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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
> @@ -15296,9 +15296,8 @@ static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
> u8 required_slices = state->enabled_dbuf_slices_mask;
> u8 slices_union = hw_enabled_slices | required_slices;
>
> - /* 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);
> }
>
> 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,
> --
> 2.24.1
>
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list