[v2,07/20] drm/i915: Unify the low level dbuf code
diff mbox series

Message ID 20200225171125.28885-8-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Proper dbuf global state
Related show

Commit Message

Ville Syrjälä Feb. 25, 2020, 5:11 p.m. UTC
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(-)

Comments

Stanislav Lisovskiy March 4, 2020, 5:14 p.m. UTC | #1
>-       /* 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! :)


Stan
Stanislav Lisovskiy March 4, 2020, 5:23 p.m. UTC | #2
>-       /* 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! :)


Stan
Ville Syrjälä March 4, 2020, 6:30 p.m. UTC | #3
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@linux.intel.com>
> Sent: Tuesday, February 25, 2020 7:11:12 PM
> To: intel-gfx@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@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
> @@ -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
>
Stanislav Lisovskiy March 5, 2020, 8:28 a.m. UTC | #4
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

> 
> > 
> > 
> > Stan
> > 
> > ________________________________
> > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Sent: Tuesday, February 25, 2020 7:11:12 PM
> > To: intel-gfx@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@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
> > @@ -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
> > 
> 
>
Stanislav Lisovskiy March 5, 2020, 8:46 a.m. UTC | #5
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,
Ville Syrjälä March 5, 2020, 1:37 p.m. UTC | #6
On Thu, Mar 05, 2020 at 08:28:30AM +0000, Lisovskiy, Stanislav wrote:
> 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. 

No, the function works just fine for all skl+ but no real requirement
that it gets called on all of them.  It's just part of the standard set
of gen9_dbuf (which should really be skl_dbuf since this is about
display stuff).

Anyways, IIRC this check is going away in a later patch, so the
discussion is a bit moot.

> 
> -       /* 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
> 
> > 
> > > 
> > > 
> > > Stan
> > > 
> > > ________________________________
> > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Sent: Tuesday, February 25, 2020 7:11:12 PM
> > > To: intel-gfx@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@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
> > > @@ -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
> > > 
> > 
> >
Stanislav Lisovskiy March 5, 2020, 2:01 p.m. UTC | #7
On Thu, 2020-03-05 at 15:37 +0200, Ville Syrjälä wrote:
> On Thu, Mar 05, 2020 at 08:28:30AM +0000, Lisovskiy, Stanislav wrote:
> > 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. 
> 
> No, the function works just fine for all skl+ but no real requirement
> that it gets called on all of them.  It's just part of the standard
> set
> of gen9_dbuf (which should really be skl_dbuf since this is about
> display stuff).
> 
> Anyways, IIRC this check is going away in a later patch, so the
> discussion is a bit moot.

Ahh, I didn't simply get to that patch yet - no questions then :)

Would just remove it here right away though, but whatever.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> > 
> > -       /* 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
> > 
> > > 
> > > > 
> > > > 
> > > > Stan
> > > > 
> > > > ________________________________
> > > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Sent: Tuesday, February 25, 2020 7:11:12 PM
> > > > To: intel-gfx@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@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
> > > > @@ -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
> > > > 
> > > 
> > > 
> 
>

Patch
diff mbox series

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,