Re: [Intel-gfx] [PATCH v6] drm/i915: Enable second dbuf slice for ICL and TGL
On Fri, Nov 08, 2019 at 03:45:00PM +0200, Stanislav Lisovskiy wrote:
> Also implemented algorithm for choosing DBuf slice configuration
> based on active pipes, pipe ratio as stated in BSpec 12716.
>
> Now pipe allocation still stays proportional to pipe width as before,
> however within allowed DBuf slice for this particular configuration.
>
> v2: Remove unneeded check from commit as ddb enabled slices might
> now differ from hw state.
>
> v3: - Added new field "supported_slices" to ddb to separate max
> supported slices vs currently enabled, to avoid confusion.
> - Removed obsolete comments and code related to DBuf(Matthew Roper).
> - Some code style and long line removal(Matthew Roper).
> - Added WARN_ON to new ddb range offset calc function(Matthew Roper).
> - Removed platform specific call to calc pipe ratio from ddb
> allocation function and fixed the return value(Matthew Roper)
> - Refactored DBUF slice allocation table to improve readability
> - Added DBUF slice allocation for TGL as well.
> - ICL(however not TGL) seems to voluntarily enable second DBuf slice
> after pm suspend/resume causing a mismatch failure, because we
> update DBuf slices only if we do a modeset, however this check
> is done always. Fixed it to be done only when modeset for ICL.
>
> v4: - Now enabled slices is not actually a number, but a bitmask,
> because we might need to enable second slice only and number
> of slices would still 1 and that current code doesn't allow.
> - Remove redundant duplicate code to have some unified way of
> enabling dbuf slices instead of hardcoding.
>
> v5: - Fix failing gen9_assert_dbuf_enabled as it was naively thinking
> that we have only one DBUF_CTL slice. Now another version for
> gen11 will check both slices as only second one can be enabled,
> to keep CI happy.
>
> v6: - Removed enabled dbuf assertion completely. Previous code
> was keeping dbuf enabled, even(!) when _dbuf_disable is called.
> Currently we enable/disable dbuf slices based on requrement
> so no point in asserting this here.
> - Removed unnecessary modeset check from verify_wm_state(Matthew Roper)
> - Slices intersection after union is same as final result(Matthew Roper)
> - Moved DBuf slices to intel_device_info(Matthew Roper)
> - Some macros added(Matthew Roper)
> - ddb range is now always less or equal to ddb size - no need for
> additional
> checks(previously needed as we had some bandwidth checks in there which
> could "suddenly" return ddb_size smaller than it is.
>
> Signed-off-by: Stanislav Lisovskiy
> Cc: Matthew Roper
> Cc: Ville Syrjälä
> Cc: James Ausmus
A handful of comments inline below but most of them are cosmetic, so
I'll leave it up to you whether you agree or not. The only one I see
that doesn't appear to match the spec is on your TGL pipe A+B dbuf
assignment.
Otherwise,
Reviewed-by: Matt Roper
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 26 +-
> .../drm/i915/display/intel_display_power.c| 98 ++---
> .../drm/i915/display/intel_display_power.h| 2 +
> drivers/gpu/drm/i915/i915_drv.c | 5 +
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_pci.c | 7 +-
> drivers/gpu/drm/i915/i915_reg.h | 8 +-
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 384 --
> 9 files changed, 429 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 876fc25968bf..bd7aff675198 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13387,7 +13387,7 @@ static void verify_wm_state(struct intel_crtc *crtc,
>
> if (INTEL_GEN(dev_priv) >= 11 &&
> hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> - DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> + DRM_ERROR("mismatch in DBUF Slices (expected %x, got %x)\n",
> sw_ddb->enabled_slices,
> hw->ddb.enabled_slices);
>
> @@ -14614,15 +14614,24 @@ static void skl_commit_modeset_enables(struct
> intel_atomic_state *state)
> u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> u8 required_slices = state->wm_results.ddb.enabled_slices;
> struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> + u8 slices_union = hw_enabled_slices | required_slices;
> + u8 slices_intersection = required_slices;
We can probably just drop this variable and use required_slices directly
in the one place below.
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i)
> /* ignore allocations for crtc's that have been
[Intel-gfx] [PATCH v6] drm/i915: Enable second dbuf slice for ICL and TGL
Also implemented algorithm for choosing DBuf slice configuration
based on active pipes, pipe ratio as stated in BSpec 12716.
Now pipe allocation still stays proportional to pipe width as before,
however within allowed DBuf slice for this particular configuration.
v2: Remove unneeded check from commit as ddb enabled slices might
now differ from hw state.
v3: - Added new field "supported_slices" to ddb to separate max
supported slices vs currently enabled, to avoid confusion.
- Removed obsolete comments and code related to DBuf(Matthew Roper).
- Some code style and long line removal(Matthew Roper).
- Added WARN_ON to new ddb range offset calc function(Matthew Roper).
- Removed platform specific call to calc pipe ratio from ddb
allocation function and fixed the return value(Matthew Roper)
- Refactored DBUF slice allocation table to improve readability
- Added DBUF slice allocation for TGL as well.
- ICL(however not TGL) seems to voluntarily enable second DBuf slice
after pm suspend/resume causing a mismatch failure, because we
update DBuf slices only if we do a modeset, however this check
is done always. Fixed it to be done only when modeset for ICL.
v4: - Now enabled slices is not actually a number, but a bitmask,
because we might need to enable second slice only and number
of slices would still 1 and that current code doesn't allow.
- Remove redundant duplicate code to have some unified way of
enabling dbuf slices instead of hardcoding.
v5: - Fix failing gen9_assert_dbuf_enabled as it was naively thinking
that we have only one DBUF_CTL slice. Now another version for
gen11 will check both slices as only second one can be enabled,
to keep CI happy.
v6: - Removed enabled dbuf assertion completely. Previous code
was keeping dbuf enabled, even(!) when _dbuf_disable is called.
Currently we enable/disable dbuf slices based on requrement
so no point in asserting this here.
- Removed unnecessary modeset check from verify_wm_state(Matthew Roper)
- Slices intersection after union is same as final result(Matthew Roper)
- Moved DBuf slices to intel_device_info(Matthew Roper)
- Some macros added(Matthew Roper)
- ddb range is now always less or equal to ddb size - no need for additional
checks(previously needed as we had some bandwidth checks in there which
could "suddenly" return ddb_size smaller than it is.
Signed-off-by: Stanislav Lisovskiy
Cc: Matthew Roper
Cc: Ville Syrjälä
Cc: James Ausmus
---
drivers/gpu/drm/i915/display/intel_display.c | 26 +-
.../drm/i915/display/intel_display_power.c| 98 ++---
.../drm/i915/display/intel_display_power.h| 2 +
drivers/gpu/drm/i915/i915_drv.c | 5 +
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_pci.c | 7 +-
drivers/gpu/drm/i915/i915_reg.h | 8 +-
drivers/gpu/drm/i915/intel_device_info.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 384 --
9 files changed, 429 insertions(+), 104 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 876fc25968bf..bd7aff675198 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13387,7 +13387,7 @@ static void verify_wm_state(struct intel_crtc *crtc,
if (INTEL_GEN(dev_priv) >= 11 &&
hw->ddb.enabled_slices != sw_ddb->enabled_slices)
- DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
+ DRM_ERROR("mismatch in DBUF Slices (expected %x, got %x)\n",
sw_ddb->enabled_slices,
hw->ddb.enabled_slices);
@@ -14614,15 +14614,24 @@ static void skl_commit_modeset_enables(struct
intel_atomic_state *state)
u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
u8 required_slices = state->wm_results.ddb.enabled_slices;
struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
+ u8 slices_union = hw_enabled_slices | required_slices;
+ u8 slices_intersection = required_slices;
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i)
/* ignore allocations for crtc's that have been turned off. */
if (new_crtc_state->hw.active)
entries[i] = old_crtc_state->wm.skl.ddb;
- /* 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);
+ DRM_DEBUG_KMS("DBuf req slices %d hw slices %d\n",
+ required_slices, hw_enabled_slices);
+
+ /*
+* If some other DBuf slice required, enable it here,
+* as here we only add more slices, but not
