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@xxxxxxxxx>
>
> >
> > - /* 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@xxxxxxxxxxxxxxx>
> > > > Sent: Tuesday, February 25, 2020 7:11:12 PM
> > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Lisovskiy, Stanislav
> > > > Subject: [PATCH v2 07/20] drm/i915: Unify the low level dbuf
> > > > code
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
> > > >
> > >
> > >
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
