[Intel-gfx] [PATCH v8 3/4] drm/i915: Manipulate DBuf slices properly
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Thu Dec 19 11:30:46 UTC 2019
On Thu, 2019-12-19 at 11:48 +0200, Ville Syrjälä wrote:
> On Thu, Dec 19, 2019 at 09:13:23AM +0000, Lisovskiy, Stanislav wrote:
> > On Wed, 2019-12-18 at 20:00 +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 13, 2019 at 03:02:27PM +0200, Stanislav Lisovskiy
> > > wrote:
> > > > Start manipulating DBuf slices as a mask,
> > > > but not as a total number, as current approach
> > > > doesn't give us full control on all combinations
> > > > of slices, which we might need(like enabling S2
> > > > only can't enabled by setting enabled_slices=1).
> > > >
> > > > Removed wrong code from intel_get_ddb_size as
> > > > it doesn't match to BSpec. For now still just
> > > > use DBuf slice until proper algorithm is implemented.
> > > >
> > > > Other minor code refactoring to get prepared
> > > > for major DBuf assignment changes landed:
> > > > - As now enabled slices contain a mask
> > > > we still need some value which should
> > > > reflect how much DBuf slices are supported
> > > > by the platform, now device info contains
> > > > num_supported_dbuf_slices.
> > > > - Removed unneeded assertion as we are now
> > > > manipulating slices in a more proper way.
> > > >
> > > > v2: Start using enabled_slices in dev_priv
> > > >
> > > > v3: "enabled_slices" is now "enabled_dbuf_slices_mask",
> > > > as this now sits in dev_priv independently.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <
> > > > stanislav.lisovskiy at intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 23 ++--
> > > > .../drm/i915/display/intel_display_power.c | 100 ++++++++
> > > > ----
> > > > ------
> > > > .../drm/i915/display/intel_display_power.h | 5 +
> > > > .../drm/i915/display/intel_display_types.h | 2 +-
> > > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > > drivers/gpu/drm/i915/i915_pci.c | 6 +-
> > > > drivers/gpu/drm/i915/intel_device_info.h | 1 +
> > > > drivers/gpu/drm/i915/intel_pm.c | 49 +++------
> > > > drivers/gpu/drm/i915/intel_pm.h | 2 +-
> > > > 9 files changed, 84 insertions(+), 106 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 0e09d0c23b1d..42a0ea540d4f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -13359,12 +13359,12 @@ static void verify_wm_state(struct
> > > > intel_crtc *crtc,
> >
> > Hi Ville,
> >
> > Thank you for comments, please see replies for some of those
> > inline.
> >
Just going through your comments now - majority seem to be addressed
already in recent series(formatiing, DBUF_CTL automation, redundant
static func). So I will proceed with DBUF asserts and remaining stuff,
being not addressed.
However I would take a courage to leave variable naming same at least
:)
As for example "required_slices" and "hw_enabled_slices" were named
that way even before me. Let's may be first solve all the crucial
issues here and then I would be happy to change those variable
names(which were named that way by somebody else) to whatever in a
follow up patch.
Stan
> > > >
> > > > skl_pipe_ddb_get_hw_state(crtc, hw->ddb_y, hw->ddb_uv);
> > > >
> > > > - hw_enabled_slices =
> > > > intel_enabled_dbuf_slices_num(dev_priv);
> > > > + hw_enabled_slices =
> > > > intel_enabled_dbuf_slices_mask(dev_priv);
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 11 &&
> > > > - hw_enabled_slices != dev_priv-
> > > > >enabled_dbuf_slices_num)
> > > > - DRM_ERROR("mismatch in DBUF Slices (expected
> > > > %u, got
> > > > %u)\n",
> > > > - dev_priv->enabled_dbuf_slices_num,
> > > > + hw_enabled_slices != dev_priv-
> > > > >enabled_dbuf_slices_mask)
> > > > + DRM_ERROR("mismatch in DBUF Slices (expected
> > > > %x, got
> > > > %x)\n",
> > > > + dev_priv->enabled_dbuf_slices_mask,
> > > > hw_enabled_slices);
> > > >
> > > > /* planes */
> > > > @@ -14549,22 +14549,23 @@ static void
> > > > intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc,
> > > > static void icl_dbuf_slice_pre_update(struct
> > > > intel_atomic_state
> > > > *state)
> > > > {
> > > > struct drm_i915_private *dev_priv = to_i915(state-
> > > > >base.dev);
> > > > - u8 hw_enabled_slices = dev_priv-
> > > > >enabled_dbuf_slices_num;
> > > > - u8 required_slices = state->enabled_dbuf_slices_num;
> > > > + u8 hw_enabled_slices = dev_priv-
> > > > >enabled_dbuf_slices_mask;
> > > > + 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 && required_slices >
> > > > hw_enabled_slices)
> > > > - icl_dbuf_slices_update(dev_priv,
> > > > required_slices);
> > > > + if (INTEL_GEN(dev_priv) >= 11 && required_slices !=
> > > > hw_enabled_slices)
> > > > + icl_dbuf_slices_update(dev_priv, slices_union);
> > > > }
> > > >
> > > > static void icl_dbuf_slice_post_update(struct
> > > > intel_atomic_state
> > > > *state)
> > > > {
> > > > struct drm_i915_private *dev_priv = to_i915(state-
> > > > >base.dev);
> > > > - u8 hw_enabled_slices = dev_priv-
> > > > >enabled_dbuf_slices_num;
> > > > - u8 required_slices = state->enabled_dbuf_slices_num;
> > > > + 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)
> > > > + if (INTEL_GEN(dev_priv) >= 11 && required_slices !=
> > > > hw_enabled_slices)
> > >
> > > I would rename the variables to old_slices vs. new_slices or
> > > something
> > > like that. Would match the common naming pattern we use
> > > extensively
> > > all
> > > over.
> >
> > Yep, we just used to have it that way, so I just didn't want to
> > change variable names.
> >
> > >
> > > > icl_dbuf_slices_update(dev_priv,
> > > > required_slices);
> > > > }
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > index b8983422a882..ba384a5315f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > @@ -1031,15 +1031,6 @@ static bool
> > > > gen9_dc_off_power_well_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > (I915_READ(DC_STATE_EN) &
> > > > DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0);
> > > > }
> > > >
> > > > -static void gen9_assert_dbuf_enabled(struct drm_i915_private
> > > > *dev_priv)
> > > > -{
> > > > - u32 tmp = I915_READ(DBUF_CTL);
> > > > -
> > > > - WARN((tmp & (DBUF_POWER_STATE | DBUF_POWER_REQUEST)) !=
> > > > - (DBUF_POWER_STATE | DBUF_POWER_REQUEST),
> > > > - "Unexpected DBuf power power state (0x%08x)\n",
> > > > tmp);
> > > > -}
> > > > -
> > > > static void gen9_disable_dc_states(struct drm_i915_private
> > > > *dev_priv)
> > > > {
> > > > struct intel_cdclk_state cdclk_state = {};
> > > > @@ -1055,8 +1046,6 @@ static void gen9_disable_dc_states(struct
> > > > drm_i915_private *dev_priv)
> > > > /* Can't read out voltage_level so can't use
> > > > intel_cdclk_changed() */
> > > > WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > > > &cdclk_state));
> > > >
> > > > - gen9_assert_dbuf_enabled(dev_priv);
> > >
> > > Why are you removing these? I think you still left the code in
> > > place
> > > to
> > > power up the first slice uncoditionally. Also not sure if DMC
> > > just
> > > powers that sucker up regardless. I think we should try it and if
> > > DMC
> > > isn't insane we should turn all the slices off when we don't need
> > > them.
> >
> > I just didn't get why we do this check here, as we actually have
> > that
> > check in verify_wm_state. Also this hardcoded check seems to always
> > assume that we should have both slices enabled which might be wrong
> > -
> > we could have now different configurations, prior to this call,
> > as this is called for example before suspend which would be
> > intel_power_domains_suspend->icl_display_core_uninit-
>
> The check is there because DMC is supposed to restore power to the
> slices after DC5/6.
>
More information about the Intel-gfx
mailing list