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@xxxxxxxxx> > > --- > > 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. > > > > 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- >gen9_disable_dc_states->gen9_assert_dbuf_enabled. But prior to suspend we can now have S1 or S2 in any combination enabled or disabled based on pipes config. verify_wm_state does it at least in a smarter way, i.e it checks the actual calculated sw state against hw state and then WARNs about the mismatch. > > > - > > if (IS_GEN9_LP(dev_priv)) > > bxt_verify_ddi_phy_power_wells(dev_priv); > > > > @@ -4254,72 +4243,71 @@ static void gen9_dbuf_disable(struct > > drm_i915_private *dev_priv) > > intel_dbuf_slice_set(dev_priv, DBUF_CTL, false); > > } > > > > -static u8 intel_dbuf_max_slices(struct drm_i915_private *dev_priv) > > +int intel_dbuf_max_slices(struct drm_i915_private *dev_priv) > > { > > - if (INTEL_GEN(dev_priv) < 11) > > - return 1; > > - return 2; > > + return INTEL_INFO(dev_priv)->num_supported_dbuf_slices; > > +} > > I don't see this being used anywhere outside this file in this patch. > So why is it being made non-static? Also it's rather redundant now > and could just be inlined into the single caller we have left. > > > + > > +void icl_program_dbuf_slices(struct drm_i915_private *dev_priv) > > +{ > > + const u8 hw_enabled_slices = dev_priv- > > >enabled_dbuf_slices_mask; > > This whole function is just a one line wrapper now that does nothing > special. So I would not even add it. > > > + > > + icl_dbuf_slices_update(dev_priv, hw_enabled_slices); > > } > > > > void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > > u8 req_slices) > > { > > - const u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num; > > - bool ret; > > + int i; > > + int max_slices = intel_dbuf_max_slices(dev_priv); > > > > - if (req_slices > intel_dbuf_max_slices(dev_priv)) { > > + if (hweight8(req_slices) > intel_dbuf_max_slices(dev_priv)) { > > You already have max_slices in a variable. > > > DRM_ERROR("Invalid number of dbuf slices requested\n"); > > Can't happen unless we messed up in state calculation. We should > drop the check or just WARN and keep going. But that's probably > material for another patch. > > > return; > > } > > > > - if (req_slices == hw_enabled_slices || req_slices == 0) > > - return; > > + DRM_DEBUG_KMS("Updating dbuf slices to %x\n", req_slices); > > Pls use 0x%x so it's obvious it's hex. > > > > > - if (req_slices > hw_enabled_slices) > > - ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, > > true); > > - else > > - ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, > > false); > > + for (i = 0; i < max_slices; i++) { > > + int slice_bit = BIT(i); > > + bool slice_set = (slice_bit & req_slices) != 0; > > + > > + switch (slice_bit) { > > + case DBUF_S1_BIT: > > + intel_dbuf_slice_set(dev_priv, > > + DBUF_CTL_S1, > > + slice_set); > > + break; > > + case DBUF_S2_BIT: > > + intel_dbuf_slice_set(dev_priv, > > + DBUF_CTL_S2, > > + slice_set); > > + break; > > + default: > > + MISSING_CASE(slice_bit); > > + } > > Still can be just: > intel_dbuf_slice_set(dev_priv, DBUF_CTL(i), > req_slices & BIT(i)); I actually have this already in latest patch revision: https://patchwork.freedesktop.org/patch/345308/?series=70059&rev=10 Stan > > > > + } > > > > - if (ret) > > - dev_priv->enabled_dbuf_slices_num = req_slices; > > + dev_priv->enabled_dbuf_slices_mask = req_slices; > > } > > > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv) > > { > > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) | > > DBUF_POWER_REQUEST); > > - I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) | > > DBUF_POWER_REQUEST); > > - POSTING_READ(DBUF_CTL_S2); > > - > > - udelay(10); > > - > > - if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) || > > - !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)) > > - DRM_ERROR("DBuf power enable timeout\n"); > > - else > > - /* > > - * FIXME: for now pretend that we only have 1 slice, > > see > > - * intel_enabled_dbuf_slices_num(). > > - */ > > - dev_priv->enabled_dbuf_slices_num = 1; > > + /* > > + * Just power up 1 slice, we will > > + * figure out later which slices we have and what we need. > > + */ > > + dev_priv->enabled_dbuf_slices_mask = DBUF_S1_BIT; > > + icl_program_dbuf_slices(dev_priv); > > } > > > > static void icl_dbuf_disable(struct drm_i915_private *dev_priv) > > { > > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) & > > ~DBUF_POWER_REQUEST); > > - I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) & > > ~DBUF_POWER_REQUEST); > > - POSTING_READ(DBUF_CTL_S2); > > - > > - udelay(10); > > - > > - if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) || > > - (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)) > > - DRM_ERROR("DBuf power disable timeout!\n"); > > - else > > - /* > > - * FIXME: for now pretend that the first slice is > > always > > - * enabled, see intel_enabled_dbuf_slices_num(). > > - */ > > - dev_priv->enabled_dbuf_slices_num = 1; > > + /* > > + * Disable all slices > > + */ > > + dev_priv->enabled_dbuf_slices_mask = 0; > > + icl_program_dbuf_slices(dev_priv); > > } > > > > static void icl_mbus_init(struct drm_i915_private *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 1da04f3e0fb3..0d9f87607eac 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h > > @@ -311,8 +311,13 @@ intel_display_power_put_async(struct > > drm_i915_private *i915, > > for ((wf) = intel_display_power_get((i915), (domain)); (wf); \ > > intel_display_power_put_async((i915), (domain), (wf)), > > (wf) = 0) > > > > +#define DBUF_S1_BIT BIT(0) > > +#define DBUF_S2_BIT BIT(1) > > + > > void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > > u8 req_slices); > > +void icl_program_dbuf_slices(struct drm_i915_private *dev_priv); > > +int intel_dbuf_max_slices(struct drm_i915_private *dev_priv); > > > > void chv_phy_powergate_lanes(struct intel_encoder *encoder, > > bool override, unsigned int mask); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 70e65c2d525d..ba2e41a03051 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -518,7 +518,7 @@ struct intel_atomic_state { > > struct skl_ddb_values wm_results; > > > > /* Number of enabled DBuf slices */ > > - u8 enabled_dbuf_slices_num; > > + u8 enabled_dbuf_slices_mask; > > > > struct i915_sw_fence commit_ready; > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 7a2d9fa5a9a6..ec4b9e3cef79 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1210,7 +1210,7 @@ struct drm_i915_private { > > bool distrust_bios_wm; > > } wm; > > > > - u8 enabled_dbuf_slices_num; /* GEN11 has configurable 2 slices > > */ > > + u8 enabled_dbuf_slices_mask; /* GEN11 has configurable 2 slices > > */ > > > > struct dram_info { > > bool valid; > > diff --git a/drivers/gpu/drm/i915/i915_pci.c > > b/drivers/gpu/drm/i915/i915_pci.c > > index 877560b1031e..2068aac5ab6a 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -614,7 +614,8 @@ static const struct intel_device_info > > intel_cherryview_info = { > > .has_gt_uc = 1, \ > > .display.has_hdcp = 1, \ > > .display.has_ipc = 1, \ > > - .ddb_size = 896 > > + .ddb_size = 896, \ > > + .num_supported_dbuf_slices = 1 > > > > #define SKL_PLATFORM \ > > GEN9_FEATURES, \ > > @@ -649,6 +650,7 @@ static const struct intel_device_info > > intel_skylake_gt4_info = { > > #define GEN9_LP_FEATURES \ > > GEN(9), \ > > .is_lp = 1, \ > > + .num_supported_dbuf_slices = 1, \ > > .display.has_hotplug = 1, \ > > .engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), > > \ > > .pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C), \ > > @@ -737,6 +739,7 @@ static const struct intel_device_info > > intel_coffeelake_gt3_info = { > > GEN9_FEATURES, \ > > GEN(10), \ > > .ddb_size = 1024, \ > > + .num_supported_dbuf_slices = 1, \ > > .display.has_dsc = 1, \ > > .has_coherent_ggtt = false, \ > > GLK_COLORS > > @@ -773,6 +776,7 @@ static const struct intel_device_info > > intel_cannonlake_info = { > > }, \ > > GEN(11), \ > > .ddb_size = 2048, \ > > + .num_supported_dbuf_slices = 2, \ > > .has_logical_ring_elsq = 1, \ > > .color = { .degamma_lut_size = 33, .gamma_lut_size = 262145 } > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h > > b/drivers/gpu/drm/i915/intel_device_info.h > > index 2725cb7fc169..7d4d122d2182 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -180,6 +180,7 @@ struct intel_device_info { > > } display; > > > > u16 ddb_size; /* in blocks */ > > + u8 num_supported_dbuf_slices; /* number of DBuf slices */ > > Redundant comment. > > > > > /* Register offsets for the various display pipes and > > transcoders */ > > int pipe_offsets[I915_MAX_TRANSCODERS]; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index c2510978ccdf..111bcafd6e4c 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3616,26 +3616,22 @@ bool ilk_disable_lp_wm(struct > > drm_i915_private *dev_priv) > > return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > > } > > > > -u8 intel_enabled_dbuf_slices_num(struct drm_i915_private > > *dev_priv) > > +u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private > > *dev_priv) > > { > > - u8 enabled_dbuf_slices_num; > > - > > - /* Slice 1 will always be enabled */ > > - enabled_dbuf_slices_num = 1; > > + u8 enabled_slices_mask = 0; > > > > /* Gen prior to GEN11 have only one DBuf slice */ > > if (INTEL_GEN(dev_priv) < 11) > > - return enabled_dbuf_slices_num; > > + return DBUF_S1_BIT; > > > > - /* > > - * FIXME: for now we'll only ever use 1 slice; pretend that we > > have > > - * only that 1 slice enabled until we have a proper way for on- > > demand > > - * toggling of the second slice. > > - */ > > - if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE) > > - enabled_dbuf_slices_num++; > > + /* Check if second DBuf slice is enabled */ > > + if (I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) > > + enabled_slices_mask |= DBUF_S1_BIT; > > > > - return enabled_dbuf_slices_num; > > + if (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE) > > + enabled_slices_mask |= DBUF_S2_BIT; > > With parametrized DBUF_CTL this could also just loop over the slices. > > > + > > + return enabled_slices_mask; > > } > > > > /* > > @@ -3843,8 +3839,6 @@ static u16 intel_get_ddb_size(struct > > drm_i915_private *dev_priv, > > { > > struct drm_atomic_state *state = crtc_state->uapi.state; > > struct intel_atomic_state *intel_state = > > to_intel_atomic_state(state); > > - const struct drm_display_mode *adjusted_mode; > > - u64 total_data_bw; > > u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size; > > > > WARN_ON(ddb_size == 0); > > @@ -3852,23 +3846,8 @@ static u16 intel_get_ddb_size(struct > > drm_i915_private *dev_priv, > > if (INTEL_GEN(dev_priv) < 11) > > return ddb_size - 4; /* 4 blocks for bypass path > > allocation */ > > > > - adjusted_mode = &crtc_state->hw.adjusted_mode; > > - total_data_bw = total_data_rate * > > drm_mode_vrefresh(adjusted_mode); > > - > > - /* > > - * 12GB/s is maximum BW supported by single DBuf slice. > > - * > > - * FIXME dbuf slice code is broken: > > - * - must wait for planes to stop using the slice before > > powering it off > > - * - plane straddling both slices is illegal in multi-pipe > > scenarios > > - * - should validate we stay within the hw bandwidth limits > > - */ > > - if (0 && (num_active > 1 || total_data_bw >= GBps(12))) { > > - intel_state->enabled_dbuf_slices_num = 2; > > - } else { > > - intel_state->enabled_dbuf_slices_num = 1; > > - ddb_size /= 2; > > - } > > Aren't we left with loads of pointless function arguments now? > > > + intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT; > > + ddb_size /= 2; > > > > return ddb_size; > > } > > @@ -4065,7 +4044,7 @@ void skl_pipe_ddb_get_hw_state(struct > > intel_crtc *crtc, > > > > void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv) > > { > > - dev_priv->enabled_dbuf_slices_num = > > intel_enabled_dbuf_slices_num(dev_priv); > > + dev_priv->enabled_dbuf_slices_mask = > > intel_enabled_dbuf_slices_mask(dev_priv); > > } > > > > /* > > @@ -5204,7 +5183,7 @@ skl_compute_ddb(struct intel_atomic_state > > *state) > > struct intel_crtc *crtc; > > int ret, i; > > > > - state->enabled_dbuf_slices_num = dev_priv- > > >enabled_dbuf_slices_num; > > + state->enabled_dbuf_slices_mask = dev_priv- > > >enabled_dbuf_slices_mask; > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, > > old_crtc_state, > > new_crtc_state, i) { > > diff --git a/drivers/gpu/drm/i915/intel_pm.h > > b/drivers/gpu/drm/i915/intel_pm.h > > index a476f6c730e9..7a7494d1224f 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.h > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > @@ -33,7 +33,7 @@ void g4x_wm_get_hw_state(struct drm_i915_private > > *dev_priv); > > void vlv_wm_get_hw_state(struct drm_i915_private *dev_priv); > > void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv); > > void skl_wm_get_hw_state(struct drm_i915_private *dev_priv); > > -u8 intel_enabled_dbuf_slices_num(struct drm_i915_private > > *dev_priv); > > +u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private > > *dev_priv); > > void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc, > > struct skl_ddb_entry *ddb_y, > > struct skl_ddb_entry *ddb_uv); > > -- > > 2.17.1 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx