On Wed, Jan 15, 2020 at 11:50:52AM +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.
>
> v4: - Fixed debug print formatting to hex(Matt Roper)
> - Optimized dbuf slice updates to be used only
> if slice union is different from current conf(Matt Roper)
> - Fixed some functions to be static(Matt Roper)
> - Created a parameterized version for DBUF_CTL to
> simplify DBuf programming cycle(Matt Roper)
> - Removed unrequred field from GEN10_FEATURES(Matt Roper)
>
> v5: - Removed redundant programming dbuf slices helper(Ville Syrjälä)
> - Started to use parameterized loop for hw readout to get slices
> (Ville Syrjälä)
> - Added back assertion checking amount of DBUF slices enabled
> after DC states 5/6 transition, also added new assertion
> as starting from ICL DMC seems to restore the last DBuf
> power state set, rather than power up all dbuf slices
> as assertion was previously expecting(Ville Syrjälä)
>
> v6: - Now using enum for DBuf slices in this patch (Ville Syrjälä)
> - Removed gen11_assert_dbuf_enabled and put gen9_assert_dbuf_enabled
> back, as we really need to have a single unified assert here
> however currently enabling always slice 1 is enforced by BSpec,
> so we will have to OR enabled slices mask with 1 in order
> to be consistent with BSpec, that way we can unify that
> assertion and against the actual state from the driver, but
> not some hardcoded value.(concluded with Ville)
> - Remove parameterized DBUF_CTL version, to extract it to another
> patch.(Ville Syrjälä)
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 23 ++---
> .../drm/i915/display/intel_display_power.c | 95 +++++++++----------
> .../drm/i915/display/intel_display_power.h | 6 ++
> .../drm/i915/display/intel_display_types.h | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_pci.c | 5 +-
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 66 ++++++-------
> drivers/gpu/drm/i915/intel_pm.h | 2 +-
> 9 files changed, 99 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 441bbf67bace..c2ff17358858 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13769,12 +13769,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 0x%x, got 0x%x)\n",
> + dev_priv->enabled_dbuf_slices_mask,
> hw_enabled_slices);
>
> /* planes */
> @@ -15118,22 +15118,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 && slices_union != 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 5e1c601f0f99..4503f5fab63d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -15,6 +15,7 @@
> #include "intel_display_types.h"
> #include "intel_dpio_phy.h"
> #include "intel_hotplug.h"
> +#include "intel_pm.h"
> #include "intel_sideband.h"
> #include "intel_tc.h"
> #include "intel_vga.h"
> @@ -1028,11 +1029,12 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
>
> static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> {
> - u32 tmp = I915_READ(DBUF_CTL);
> + u8 enabled_dbuf_slices = intel_enabled_dbuf_slices_mask(dev_priv);
>
> - WARN((tmp & (DBUF_POWER_STATE | DBUF_POWER_REQUEST)) !=
> - (DBUF_POWER_STATE | DBUF_POWER_REQUEST),
> - "Unexpected DBuf power power state (0x%08x)\n", tmp);
> + WARN(enabled_dbuf_slices != dev_priv->enabled_dbuf_slices_mask,
> + "Unexpected DBuf power power state (0x%08x, expected 0x%08x)\n",
> + enabled_dbuf_slices,
> + dev_priv->enabled_dbuf_slices_mask);
> }
>
> static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
> @@ -4396,72 +4398,61 @@ 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;
> }
Maybe we should just inline this into the callsites now that it's just a
single line?
>
> 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)) {
> - DRM_ERROR("Invalid number of dbuf slices requested\n");
> - return;
> - }
> + WARN(hweight8(req_slices) > max_slices,
> + "Invalid number of dbuf slices requested\n");
>
> - if (req_slices == hw_enabled_slices || req_slices == 0)
> - return;
> + DRM_DEBUG_KMS("Updating dbuf slices to 0x%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++) {
> + u8 slice_set = req_slices & BIT(i);
> +
> + switch (i) {
> + case DBUF_S1:
> + intel_dbuf_slice_set(dev_priv,
> + DBUF_CTL_S1,
> + slice_set);
> + break;
> + case DBUF_S2:
> + intel_dbuf_slice_set(dev_priv,
> + DBUF_CTL_S2,
> + slice_set);
> + break;
> + default:
> + MISSING_CASE(i);
> + }
> + }
Now that we're not using the return value of intel_dbuf_slice_set()
anywhere, we could make it return void. But you can do that as a
followup patch if you like.
>
> - 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 = BIT(DBUF_S1);
> + icl_dbuf_slices_update(dev_priv, dev_priv->enabled_dbuf_slices_mask);
> }
>
> 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_dbuf_slices_update(dev_priv, dev_priv->enabled_dbuf_slices_mask);
> }
>
> 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 2608a65af7fa..d07bdbe2040a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -307,12 +307,18 @@ intel_display_power_put_async(struct drm_i915_private *i915,
> }
> #endif
>
> +enum dbuf_slice {
> + DBUF_S1,
> + DBUF_S2,
> +};
> +
> #define with_intel_display_power(i915, domain, wf) \
> for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
> intel_display_power_put_async((i915), (domain), (wf)), (wf) = 0)
>
> void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> u8 req_slices);
> +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 6dca90ebca35..09f14e9e7863 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 120c9f1090c4..23688e904e12 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1208,7 +1208,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 83f01401b8b5..6c6daa44c439 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -615,7 +615,8 @@ static const struct intel_device_info chv_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, \
> @@ -650,6 +651,7 @@ static const struct intel_device_info skl_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), \
> @@ -774,6 +776,7 @@ static const struct intel_device_info cnl_info = {
> }, \
> GEN(11), \
> .ddb_size = 2048, \
> + .num_supported_dbuf_slices = 2, \
Maybe we should save this change for a patch of its own at the very end
of the series to "flip the switch?" That way if there are regressions
it's less invasive to just revert that while we debug and tweak the rest
of the dbuf logic.
> .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 8f6f6472626a..6ba72a6be7cb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3644,26 +3644,37 @@ 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;
> + int i;
> + int max_slices = intel_dbuf_max_slices(dev_priv);
> + 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 BIT(DBUF_S1);
Is there any reason to handle this case separately? If we just let it
continue on to below, max_slices will be 1, so it will only test
DBUF_CTL_S1 (which is the same register as DBUF_CTL on gen9)
If we hardcode this, then we've neutered gen9_assert_dbuf_enabled() on
gen9/gen10 since we'd now just be comparing expected vs expected and
never actually reading out how the hardware was actually programmed
(either by us or the DMC).
Matt
>
> - /*
> - * 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++;
> + for (i = 0; i < max_slices; i++) {
> + u8 slice_bit = BIT(i);
> + bool res;
> +
> + switch (i) {
> +
> + case DBUF_S1:
> + res = I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE;
> + break;
> + case DBUF_S2:
> + res = I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE;
> + break;
> + default:
> + MISSING_CASE(slice_bit);
> + }
>
> - return enabled_dbuf_slices_num;
> + if (res)
> + enabled_slices_mask |= slice_bit;
> + }
> +
> + return enabled_slices_mask;
> }
>
> /*
> @@ -3871,8 +3882,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);
> @@ -3880,23 +3889,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 = BIT(DBUF_S1);
> + ddb_size /= 2;
>
> return ddb_size;
> }
> @@ -4093,8 +4087,8 @@ 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);
> }
>
> /*
> @@ -5236,7 +5230,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 22fd2daf608e..d60a85421c5a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -32,7 +32,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.24.1.485.gad05a3d8e5
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
