| Message ID | 20191213130228.29509-4-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable second DBuf slice for ICL and TGL | expand |
On Fri, 2019-12-13 at 11:01 -0800, Matt Roper 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@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, > > > > 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", Thanks for good comments, just as I wrote in previous patch, most of the issues I've addressed in a recent series, for some which I didn't, I will reply inline here with explanation. > > Minor nitpick: I'd write these with "0x%x" to make it more obvious > to > the person reading the message that the value is hex and thus a > bitmask > rather than a count. > > > + 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) > > Should this be hw_enabled_slices != slices_union so that we only > process > it when we're adding new slices and not in cases where we're only > going > to be taking slices away? > > > + 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) > > 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); > > - > > I'm undecided on whether dropping the assertion completely is a good > idea or not. If I understand correctly, it was never meant to check > that our own handling of the dbuf is correct (and it was added back > when > we only ever had a single dbuf slice on gen9), but rather than the > DMC > did the right thing when enabling/disabling the dbuf around DC state > transitions. As far as I know, the DMC has been pretty solid > recently, > but extra paranoia around firmware we don't directly control still > seems > like a good idea to me. With this added we seem to get soem sporadical dmesg-warns on suspend resume tests for ICL as this assertion now is not fully correct as we now might not enable both DBuf slices as it was previsously hardcoded in icl_dbuf_enable, now we only enable dbuf slices based on the actual need. Also I think we already have similar check in verify_wm_state where is also compares sw state and the actual hardware state. > > > > 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) > > I don't see any new usage of this function outside this file. I > think > we can keep this static? > > > { > > - if (INTEL_GEN(dev_priv) < 11) > > - return 1; > > - return 2; > > + return INTEL_INFO(dev_priv)->num_supported_dbuf_slices; > > +} > > + > > +void icl_program_dbuf_slices(struct drm_i915_private *dev_priv) > > It looks like this never gets used outside this file either, so it > could > probably be static. Or it's simple enough that we could just drop it > completely and inline the icl_dbuf_slices_update() directly at the > callsites. > > > +{ > > + const u8 hw_enabled_slices = dev_priv- > > >enabled_dbuf_slices_mask; > > + > > + 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)) { > > DRM_ERROR("Invalid number of dbuf slices requested\n"); > > return; > > } > > > > - if (req_slices == hw_enabled_slices || req_slices == 0) > > - return; > > + DRM_DEBUG_KMS("Updating dbuf slices to %x\n", req_slices); > > As noted above, I prefer "0x%x" to make it more obvious the number is > a > bitmask. Up to you though. > > > > > - 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; > > Technically you could drop the "!= 0" part since anything non-zero > will > be evaluated as 'true.' Doesn't really matter though. > > > + > > + 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); > > + } > > It might be worth parameterizing DBUF_CTL so that we can simplify > code > like this to just > > intel_dbuf_slice_set(dev_priv, DBUF_CTL(i), slice_set); > > or even just > > intel_dbuf_slice_set(dev_priv, DBUF_CTL(i), req_slices & > BIT(i)) > > as we do with other types of registers. That will also future-proof > a > bit in case we ever get platforms with more dbuf slices. > > > + } > > > > - 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, \ > > GEN10_FEATURES inherits everything from GEN9_FEATURES, so I think we > can > drop this one. > > > > .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 */ > > > > /* 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; > > Do we want to assume this or should we just read it anyway to make > sure? > As noted before, we might get into a disagreement with the DMC or > something and it would be good to know how the hardware is actually > set. As I see the current code prefers to assume this for any platforms < 11, which probably need somekind of different hw state readout. I would prefer to leave it as is for previous platforms not to make too many things messy at the same time. Could be good idea to refactor this in subsequent patches though. Stan > > > - /* > > - * 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; > > You have the comment about the second slice before your check of the > first slice... I think you can just drop the comment in this case. > > > > > - return enabled_dbuf_slices_num; > > + if (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE) > > + enabled_slices_mask |= DBUF_S2_BIT; > > + > > + 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; > > - } > > + 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 > > > >
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@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, > > 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. > 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. > - > 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)); > + } > > - 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
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@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. > > > > 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 > >
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@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. > > > > > > > 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.
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@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. >
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, 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) 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); - 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; +} + +void icl_program_dbuf_slices(struct drm_i915_private *dev_priv) +{ + const u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_mask; + + 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)) { DRM_ERROR("Invalid number of dbuf slices requested\n"); return; } - if (req_slices == hw_enabled_slices || req_slices == 0) - return; + DRM_DEBUG_KMS("Updating dbuf slices to %x\n", req_slices); - 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); + } + } - 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 */ /* 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; + + 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; - } + 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);
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@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(-)