* [Intel-gfx] [PATCH v8 1/7] drm/i915: Decouple cdclk calculation from modeset checks
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 2/7] drm/i915: Extract cdclk requirements checking to separate function Stanislav Lisovskiy
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
We need to calculate cdclk after watermarks/ddb has been calculated
as with recent hw CDCLK needs to be adjusted accordingly to DBuf
requirements, which is not possible with current code organization.
Setting CDCLK according to DBuf BW requirements and not just rejecting
if it doesn't satisfy BW requirements, will allow us to save power when
it is possible and gain additional bandwidth when it's needed - i.e
boosting both our power management and perfomance capabilities.
This patch is preparation for that, first we now extract modeset
calculation from modeset checks, in order to call it after wm/ddb
has been calculated.
v2: - Extract only intel_modeset_calc_cdclk from intel_modeset_checks
(Ville Syrjälä)
v3: - Clear plls after intel_modeset_calc_cdclk
v4: - Added r-b from previous revision to commit message
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 22 +++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 432b4eeaf9f6..005e324d0582 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14493,12 +14493,6 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
return ret;
}
- ret = intel_modeset_calc_cdclk(state);
- if (ret)
- return ret;
-
- intel_modeset_clear_plls(state);
-
if (IS_HASWELL(dev_priv))
return hsw_mode_set_planes_workaround(state);
@@ -14830,10 +14824,6 @@ static int intel_atomic_check(struct drm_device *dev,
goto fail;
}
- ret = intel_atomic_check_crtcs(state);
- if (ret)
- goto fail;
-
intel_fbc_choose_crtc(dev_priv, state);
ret = calc_watermark_data(state);
if (ret)
@@ -14843,6 +14833,18 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
+ if (any_ms) {
+ ret = intel_modeset_calc_cdclk(state);
+ if (ret)
+ return ret;
+
+ intel_modeset_clear_plls(state);
+ }
+
+ ret = intel_atomic_check_crtcs(state);
+ if (ret)
+ goto fail;
+
for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if (!needs_modeset(new_crtc_state) &&
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 2/7] drm/i915: Extract cdclk requirements checking to separate function
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 1/7] drm/i915: Decouple cdclk calculation from modeset checks Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 19:36 ` Manasi Navare
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 3/7] drm/i915: Check plane configuration properly Stanislav Lisovskiy
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
In Gen11+ whenever we might exceed DBuf bandwidth we might need to
recalculate CDCLK which DBuf bandwidth is scaled with.
Total Dbuf bw used might change based on particular plane needs.
Thus to calculate if cdclk needs to be changed it is not enough
anymore to check plane configuration and plane min cdclk, per DBuf
bw can be calculated only after wm/ddb calculation is done and
all required planes are added into the state. In order to keep
all min_cdclk related checks in one place let's extract it into
separate function, checking and modifying any_ms.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 30 ++++++++++++++------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 005e324d0582..e93a553a344d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14572,8 +14572,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
IS_IVYBRIDGE(dev_priv);
}
-static int intel_atomic_check_planes(struct intel_atomic_state *state,
- bool *need_cdclk_calc)
+static int intel_atomic_check_planes(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_crtc_state *old_crtc_state, *new_crtc_state;
@@ -14623,6 +14622,22 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state,
return ret;
}
+ return 0;
+}
+
+static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
+ bool *need_cdclk_calc)
+{
+ struct intel_cdclk_state *new_cdclk_state;
+ int i;
+ struct intel_plane_state *plane_state;
+ struct intel_plane *plane;
+ int ret;
+
+ new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+ if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
+ *need_cdclk_calc = true;
+
/*
* active_planes bitmask has been updated, and potentially
* affected planes are part of the state. We can now
@@ -14685,7 +14700,6 @@ static int intel_atomic_check(struct drm_device *dev,
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_atomic_state *state = to_intel_atomic_state(_state);
struct intel_crtc_state *old_crtc_state, *new_crtc_state;
- struct intel_cdclk_state *new_cdclk_state;
struct intel_crtc *crtc;
int ret, i;
bool any_ms = false;
@@ -14796,14 +14810,10 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
- ret = intel_atomic_check_planes(state, &any_ms);
+ ret = intel_atomic_check_planes(state);
if (ret)
goto fail;
- new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
- if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
- any_ms = true;
-
/*
* distrust_bios_wm will force a full dbuf recomputation
* but the hardware state will only get updated accordingly
@@ -14833,6 +14843,10 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
+ ret = intel_atomic_check_cdclk(state, &any_ms);
+ if (ret)
+ goto fail;
+
if (any_ms) {
ret = intel_modeset_calc_cdclk(state);
if (ret)
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 2/7] drm/i915: Extract cdclk requirements checking to separate function
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 2/7] drm/i915: Extract cdclk requirements checking to separate function Stanislav Lisovskiy
@ 2020-05-14 19:36 ` Manasi Navare
0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2020-05-14 19:36 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
On Thu, May 14, 2020 at 06:21:40PM +0300, Stanislav Lisovskiy wrote:
> In Gen11+ whenever we might exceed DBuf bandwidth we might need to
> recalculate CDCLK which DBuf bandwidth is scaled with.
> Total Dbuf bw used might change based on particular plane needs.
>
> Thus to calculate if cdclk needs to be changed it is not enough
> anymore to check plane configuration and plane min cdclk, per DBuf
> bw can be calculated only after wm/ddb calculation is done and
> all required planes are added into the state. In order to keep
> all min_cdclk related checks in one place let's extract it into
> separate function, checking and modifying any_ms.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Separating the need_cdclk_calc setting into a new function intel_atomic_check_cdclk()
makes sense.
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Manasi
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 30 ++++++++++++++------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 005e324d0582..e93a553a344d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14572,8 +14572,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
> IS_IVYBRIDGE(dev_priv);
> }
>
> -static int intel_atomic_check_planes(struct intel_atomic_state *state,
> - bool *need_cdclk_calc)
> +static int intel_atomic_check_planes(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -14623,6 +14622,22 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state,
> return ret;
> }
>
> + return 0;
> +}
> +
> +static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
> + bool *need_cdclk_calc)
> +{
> + struct intel_cdclk_state *new_cdclk_state;
> + int i;
> + struct intel_plane_state *plane_state;
> + struct intel_plane *plane;
> + int ret;
> +
> + new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> + if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
> + *need_cdclk_calc = true;
> +
> /*
> * active_planes bitmask has been updated, and potentially
> * affected planes are part of the state. We can now
> @@ -14685,7 +14700,6 @@ static int intel_atomic_check(struct drm_device *dev,
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_atomic_state *state = to_intel_atomic_state(_state);
> struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> - struct intel_cdclk_state *new_cdclk_state;
> struct intel_crtc *crtc;
> int ret, i;
> bool any_ms = false;
> @@ -14796,14 +14810,10 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> - ret = intel_atomic_check_planes(state, &any_ms);
> + ret = intel_atomic_check_planes(state);
> if (ret)
> goto fail;
>
> - new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> - if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
> - any_ms = true;
> -
> /*
> * distrust_bios_wm will force a full dbuf recomputation
> * but the hardware state will only get updated accordingly
> @@ -14833,6 +14843,10 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> + ret = intel_atomic_check_cdclk(state, &any_ms);
> + if (ret)
> + goto fail;
> +
> if (any_ms) {
> ret = intel_modeset_calc_cdclk(state);
> if (ret)
> --
> 2.24.1.485.gad05a3d8e5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 3/7] drm/i915: Check plane configuration properly
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 1/7] drm/i915: Decouple cdclk calculation from modeset checks Stanislav Lisovskiy
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 2/7] drm/i915: Extract cdclk requirements checking to separate function Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 19:38 ` Manasi Navare
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 4/7] drm/i915: Plane configuration affects CDCLK in Gen11+ Stanislav Lisovskiy
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
From: Stanislav Lisovskiy <stanislav.lisovskiY@intel.com>
Checking with hweight8 if plane configuration had
changed seems to be wrong as different plane configs
can result in a same hamming weight.
So lets check the bitmask itself.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e93a553a344d..a9ab66d97360 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14614,7 +14614,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
- if (hweight8(old_active_planes) == hweight8(new_active_planes))
+ /*
+ * Not only the number of planes, but if the plane configuration had
+ * changed might already mean we need to recompute min CDCLK,
+ * because different planes might consume different amount of Dbuf bandwidth
+ * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
+ */
+ if (old_active_planes == new_active_planes)
continue;
ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 3/7] drm/i915: Check plane configuration properly
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 3/7] drm/i915: Check plane configuration properly Stanislav Lisovskiy
@ 2020-05-14 19:38 ` Manasi Navare
0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2020-05-14 19:38 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
On Thu, May 14, 2020 at 06:21:41PM +0300, Stanislav Lisovskiy wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiY@intel.com>
>
> Checking with hweight8 if plane configuration had
> changed seems to be wrong as different plane configs
> can result in a same hamming weight.
> So lets check the bitmask itself.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Manasi
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e93a553a344d..a9ab66d97360 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14614,7 +14614,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
>
> - if (hweight8(old_active_planes) == hweight8(new_active_planes))
> + /*
> + * Not only the number of planes, but if the plane configuration had
> + * changed might already mean we need to recompute min CDCLK,
> + * because different planes might consume different amount of Dbuf bandwidth
> + * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> + */
> + if (old_active_planes == new_active_planes)
> continue;
>
> ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> --
> 2.24.1.485.gad05a3d8e5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 4/7] drm/i915: Plane configuration affects CDCLK in Gen11+
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
` (2 preceding siblings ...)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 3/7] drm/i915: Check plane configuration properly Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 19:41 ` Manasi Navare
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 5/7] drm/i915: Introduce for_each_dbuf_slice_in_mask macro Stanislav Lisovskiy
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
From: Stanislav Lisovskiy <stanislav.lisovskiY@intel.com>
So lets support it.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a9ab66d97360..800ae3768841 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14569,7 +14569,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
/* See {hsw,vlv,ivb}_plane_ratio() */
return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
- IS_IVYBRIDGE(dev_priv);
+ IS_IVYBRIDGE(dev_priv) || (INTEL_GEN(dev_priv) >= 11);
}
static int intel_atomic_check_planes(struct intel_atomic_state *state)
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 4/7] drm/i915: Plane configuration affects CDCLK in Gen11+
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 4/7] drm/i915: Plane configuration affects CDCLK in Gen11+ Stanislav Lisovskiy
@ 2020-05-14 19:41 ` Manasi Navare
0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2020-05-14 19:41 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
On Thu, May 14, 2020 at 06:21:42PM +0300, Stanislav Lisovskiy wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiY@intel.com>
>
> So lets support it.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Manasi
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a9ab66d97360..800ae3768841 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14569,7 +14569,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
> /* See {hsw,vlv,ivb}_plane_ratio() */
> return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
> IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> - IS_IVYBRIDGE(dev_priv);
> + IS_IVYBRIDGE(dev_priv) || (INTEL_GEN(dev_priv) >= 11);
> }
>
> static int intel_atomic_check_planes(struct intel_atomic_state *state)
> --
> 2.24.1.485.gad05a3d8e5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 5/7] drm/i915: Introduce for_each_dbuf_slice_in_mask macro
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
` (3 preceding siblings ...)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 4/7] drm/i915: Plane configuration affects CDCLK in Gen11+ Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 19:57 ` Manasi Navare
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs Stanislav Lisovskiy
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
We quite often need now to iterate only particular dbuf slices
in mask, whether they are active or related to particular crtc.
v2: - Minor code refactoring
v3: - Use enum for max slices instead of macro
Let's make our life a bit easier and use a macro for that.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.h | 7 +++++++
drivers/gpu/drm/i915/display/intel_display_power.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index efb4da205ea2..b7a6d56bac5f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -187,6 +187,13 @@ enum plane_id {
for ((__p) = PLANE_PRIMARY; (__p) < I915_MAX_PLANES; (__p)++) \
for_each_if((__crtc)->plane_ids_mask & BIT(__p))
+#define for_each_dbuf_slice_in_mask(__slice, __mask) \
+ for ((__slice) = DBUF_S1; (__slice) < I915_MAX_DBUF_SLICES; (__slice)++) \
+ for_each_if((BIT(__slice)) & (__mask))
+
+#define for_each_dbuf_slice(__slice) \
+ for_each_dbuf_slice_in_mask(__slice, BIT(I915_MAX_DBUF_SLICES) - 1)
+
enum port {
PORT_NONE = -1,
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 6c917699293b..4d0d6f9dad26 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -314,6 +314,7 @@ intel_display_power_put_async(struct drm_i915_private *i915,
enum dbuf_slice {
DBUF_S1,
DBUF_S2,
+ I915_MAX_DBUF_SLICES
};
#define with_intel_display_power(i915, domain, wf) \
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 5/7] drm/i915: Introduce for_each_dbuf_slice_in_mask macro
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 5/7] drm/i915: Introduce for_each_dbuf_slice_in_mask macro Stanislav Lisovskiy
@ 2020-05-14 19:57 ` Manasi Navare
0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2020-05-14 19:57 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
On Thu, May 14, 2020 at 06:21:43PM +0300, Stanislav Lisovskiy wrote:
> We quite often need now to iterate only particular dbuf slices
> in mask, whether they are active or related to particular crtc.
>
> v2: - Minor code refactoring
> v3: - Use enum for max slices instead of macro
>
> Let's make our life a bit easier and use a macro for that.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Manasi
> ---
> drivers/gpu/drm/i915/display/intel_display.h | 7 +++++++
> drivers/gpu/drm/i915/display/intel_display_power.h | 1 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index efb4da205ea2..b7a6d56bac5f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -187,6 +187,13 @@ enum plane_id {
> for ((__p) = PLANE_PRIMARY; (__p) < I915_MAX_PLANES; (__p)++) \
> for_each_if((__crtc)->plane_ids_mask & BIT(__p))
>
> +#define for_each_dbuf_slice_in_mask(__slice, __mask) \
> + for ((__slice) = DBUF_S1; (__slice) < I915_MAX_DBUF_SLICES; (__slice)++) \
> + for_each_if((BIT(__slice)) & (__mask))
> +
> +#define for_each_dbuf_slice(__slice) \
> + for_each_dbuf_slice_in_mask(__slice, BIT(I915_MAX_DBUF_SLICES) - 1)
> +
> enum port {
> PORT_NONE = -1,
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 6c917699293b..4d0d6f9dad26 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -314,6 +314,7 @@ intel_display_power_put_async(struct drm_i915_private *i915,
> enum dbuf_slice {
> DBUF_S1,
> DBUF_S2,
> + I915_MAX_DBUF_SLICES
> };
>
> #define with_intel_display_power(i915, domain, wf) \
> --
> 2.24.1.485.gad05a3d8e5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
` (4 preceding siblings ...)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 5/7] drm/i915: Introduce for_each_dbuf_slice_in_mask macro Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-14 15:43 ` Stanislav Lisovskiy
` (2 more replies)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 7/7] drm/i915: Remove unneeded hack now for CDCLK Stanislav Lisovskiy
2020-05-15 15:11 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Consider DBuf bandwidth when calculating CDCLK (rev13) Patchwork
7 siblings, 3 replies; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
According to BSpec max BW per slice is calculated using formula
Max BW = CDCLK * 64. Currently when calculating min CDCLK we
account only per plane requirements, however in order to avoid
FIFO underruns we need to estimate accumulated BW consumed by
all planes(ddb entries basically) residing on that particular
DBuf slice. This will allow us to put CDCLK lower and save power
when we don't need that much bandwidth or gain additional
performance once plane consumption grows.
v2: - Fix long line warning
- Limited new DBuf bw checks to only gens >= 11
v3: - Lets track used Dbuf bw per slice and per crtc in bw state
(or may be in DBuf state in future), that way we don't need
to have all crtcs in state and those only if we detect if
are actually going to change cdclk, just same way as we
do with other stuff, i.e intel_atomic_serialize_global_state
and co. Just as per Ville's paradigm.
- Made dbuf bw calculation procedure look nicer by introducing
for_each_dbuf_slice_in_mask - we often will now need to iterate
slices using mask.
- According to experimental results CDCLK * 64 accounts for
overall bandwidth across all dbufs, not per dbuf.
v4: - Fixed missing const(Ville)
- Removed spurious whitespaces(Ville)
- Fixed local variable init(reduced scope where not needed)
- Added some comments about data rate for planar formats
- Changed struct intel_crtc_bw to intel_dbuf_bw
- Moved dbuf bw calculation to intel_compute_min_cdclk(Ville)
v5: - Removed unneeded macro
v6: - Prevent too frequent CDCLK switching back and forth:
Always switch to higher CDCLK when needed to prevent bandwidth
issues, however don't switch to lower CDCLK earlier than once
in 30 minutes in order to prevent constant modeset blinking.
We could of course not switch back at all, however this is
bad from power consumption point of view.
v7: - Fixed to track cdclk using bw_state, modeset will be now
triggered only when CDCLK change is really needed.
v8: - Lock global state if bw_state->min_cdclk is changed.
- Try getting bw_state only if there are crtcs in the commit
(need to have read-locked global state)
v9: - Do not do Dbuf bw check for gens < 9 - triggers WARN
as ddb_size is 0.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_bw.c | 114 ++++++++++++++++++-
drivers/gpu/drm/i915/display/intel_bw.h | 9 ++
drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++-
drivers/gpu/drm/i915/display/intel_cdclk.h | 1 -
drivers/gpu/drm/i915/display/intel_display.c | 38 ++++++-
drivers/gpu/drm/i915/intel_pm.c | 31 ++++-
drivers/gpu/drm/i915/intel_pm.h | 3 +
7 files changed, 200 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 6e7cc3a4f1aa..1ee65b756712 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -6,8 +6,10 @@
#include <drm/drm_atomic_state_helper.h>
#include "intel_bw.h"
+#include "intel_pm.h"
#include "intel_display_types.h"
#include "intel_sideband.h"
+#include "intel_cdclk.h"
/* Parameters for Qclk Geyserville (QGV) */
struct intel_qgv_point {
@@ -333,7 +335,6 @@ static unsigned int intel_bw_crtc_data_rate(const struct intel_crtc_state *crtc_
return data_rate;
}
-
void intel_bw_crtc_update(struct intel_bw_state *bw_state,
const struct intel_crtc_state *crtc_state)
{
@@ -410,6 +411,117 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
return to_intel_bw_state(bw_state);
}
+static int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ int i;
+ const struct intel_crtc_state *crtc_state;
+ struct intel_crtc *crtc;
+ int max_bw = 0;
+ int slice_id;
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ enum plane_id plane_id;
+ struct intel_dbuf_bw *crtc_bw;
+ struct intel_bw_state *new_bw_state;
+ struct intel_bw_state *old_bw_state;
+
+ new_bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(new_bw_state))
+ return PTR_ERR(new_bw_state);
+
+ crtc_bw = &new_bw_state->dbuf_bw[crtc->pipe];
+
+ memset(&crtc_bw->used_bw, 0, sizeof(crtc_bw->used_bw));
+
+ for_each_plane_id_on_crtc(crtc, plane_id) {
+ const struct skl_ddb_entry *plane_alloc =
+ &crtc_state->wm.skl.plane_ddb_y[plane_id];
+ const struct skl_ddb_entry *uv_plane_alloc =
+ &crtc_state->wm.skl.plane_ddb_uv[plane_id];
+ unsigned int data_rate = crtc_state->data_rate[plane_id];
+ unsigned int dbuf_mask = 0;
+
+ dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, plane_alloc);
+ dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, uv_plane_alloc);
+
+ /*
+ * FIXME: To calculate that more properly we probably need to
+ * to split per plane data_rate into data_rate_y and data_rate_uv
+ * for multiplanar formats in order not to get accounted those twice
+ * if they happen to reside on different slices.
+ * However for pre-icl this would work anyway because we have only single
+ * slice and for icl+ uv plane has non-zero data rate.
+ * So in worst case those calculation are a bit pessimistic, which
+ * shouldn't pose any significant problem anyway.
+ */
+ for_each_dbuf_slice_in_mask(slice_id, dbuf_mask)
+ crtc_bw->used_bw[slice_id] += data_rate;
+ }
+
+ for_each_dbuf_slice(slice_id) {
+ /*
+ * Current experimental observations show that contrary to BSpec
+ * we get underruns once we exceed 64 * CDCLK for slices in total.
+ * As a temporary measure in order not to keep CDCLK bumped up all the
+ * time we calculate CDCLK according to this formula for overall bw
+ * consumed by slices.
+ */
+ max_bw += crtc_bw->used_bw[slice_id];
+ }
+
+ new_bw_state->min_cdclk = max_bw / 64;
+
+ old_bw_state = intel_atomic_get_old_bw_state(state);
+
+ if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
+ int ret = intel_atomic_lock_global_state(&new_bw_state->base);
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+int intel_bw_calc_min_cdclk(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ int i;
+ const struct intel_crtc_state *crtc_state;
+ struct intel_crtc *crtc;
+
+ /*
+ * Older gens might not have dbuf/ddb and for
+ * compatibility will have same min_cdclk in bw_state
+ * as cdclk state does.
+ */
+ if (INTEL_GEN(dev_priv) >= 9) {
+ return skl_bw_calc_min_cdclk(state);
+ } else {
+ int min_cdclk = 0;
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_bw_state *bw_state;
+ struct intel_cdclk_state *cdclk_state;
+
+ bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(bw_state))
+ return PTR_ERR(bw_state);
+
+ cdclk_state = intel_atomic_get_cdclk_state(state);
+ if (IS_ERR(cdclk_state))
+ return PTR_ERR(cdclk_state);
+
+ min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ bw_state->min_cdclk = min_cdclk;
+ }
+ }
+
+ return 0;
+}
+
int intel_bw_atomic_check(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index 898b4a85ccab..d8c92a59ba49 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -10,13 +10,19 @@
#include "intel_display.h"
#include "intel_global_state.h"
+#include "intel_display_power.h"
struct drm_i915_private;
struct intel_atomic_state;
struct intel_crtc_state;
+struct intel_dbuf_bw {
+ int used_bw[I915_MAX_DBUF_SLICES];
+};
+
struct intel_bw_state {
struct intel_global_state base;
+ struct intel_dbuf_bw dbuf_bw[I915_MAX_PIPES];
/*
* Contains a bit mask, used to determine, whether correspondent
@@ -29,6 +35,8 @@ struct intel_bw_state {
/* bitmask of active pipes */
u8 active_pipes;
+
+ int min_cdclk;
};
#define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base)
@@ -47,5 +55,6 @@ int intel_bw_init(struct drm_i915_private *dev_priv);
int intel_bw_atomic_check(struct intel_atomic_state *state);
void intel_bw_crtc_update(struct intel_bw_state *bw_state,
const struct intel_crtc_state *crtc_state);
+int intel_bw_calc_min_cdclk(struct intel_atomic_state *state);
#endif /* __INTEL_BW_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 979a0241fdcb..28750d1f914b 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -21,10 +21,12 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include <linux/time.h>
#include "intel_atomic.h"
#include "intel_cdclk.h"
#include "intel_display_types.h"
#include "intel_sideband.h"
+#include "intel_bw.h"
/**
* DOC: CDCLK / RAWCLK
@@ -2093,11 +2095,9 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
{
struct intel_atomic_state *state = cdclk_state->base.state;
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_crtc *crtc;
struct intel_crtc_state *crtc_state;
int min_cdclk, i;
- enum pipe pipe;
for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
int ret;
@@ -2117,8 +2117,18 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
}
min_cdclk = cdclk_state->force_min_cdclk;
- for_each_pipe(dev_priv, pipe)
- min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk);
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_bw_state *bw_state;
+
+ min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(bw_state))
+ return PTR_ERR(bw_state);
+
+ min_cdclk = max(bw_state->min_cdclk, min_cdclk);
+ }
return min_cdclk;
}
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 5731806e4cee..d62e11d620c0 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -7,7 +7,6 @@
#define __INTEL_CDCLK_H__
#include <linux/types.h>
-
#include "i915_drv.h"
#include "intel_display.h"
#include "intel_global_state.h"
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 800ae3768841..7d460ccfda8d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14634,16 +14634,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
bool *need_cdclk_calc)
{
- struct intel_cdclk_state *new_cdclk_state;
int i;
struct intel_plane_state *plane_state;
struct intel_plane *plane;
int ret;
-
- new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
- if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
- *need_cdclk_calc = true;
-
+ struct intel_cdclk_state *new_cdclk_state;
+ struct intel_crtc_state *new_crtc_state;
+ struct intel_crtc *crtc;
/*
* active_planes bitmask has been updated, and potentially
* affected planes are part of the state. We can now
@@ -14655,6 +14652,35 @@ static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
return ret;
}
+ new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+
+ if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
+ *need_cdclk_calc = true;
+
+ ret = intel_bw_calc_min_cdclk(state);
+ if (ret)
+ return ret;
+
+ if (!new_cdclk_state)
+ return 0;
+
+ for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+ struct intel_bw_state *bw_state;
+ int min_cdclk = 0;
+
+ min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(bw_state))
+ return PTR_ERR(bw_state);
+
+ /*
+ * Currently do this change only if we need to increase
+ */
+ if (bw_state->min_cdclk > min_cdclk)
+ *need_cdclk_calc = true;
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f7bd1dbb625e..684339f44559 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3939,10 +3939,9 @@ icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
return offset;
}
-static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
+u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
{
u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
-
drm_WARN_ON(&dev_priv->drm, ddb_size == 0);
if (INTEL_GEN(dev_priv) < 11)
@@ -3951,6 +3950,34 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
return ddb_size;
}
+u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
+ const struct skl_ddb_entry *entry)
+{
+ u32 slice_mask = 0;
+ u16 ddb_size = intel_get_ddb_size(dev_priv);
+ u16 num_supported_slices = INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
+ u16 slice_size = ddb_size / num_supported_slices;
+ u16 start_slice;
+ u16 end_slice;
+
+ if (!skl_ddb_entry_size(entry))
+ return 0;
+
+ start_slice = entry->start / slice_size;
+ end_slice = (entry->end - 1) / slice_size;
+
+ /*
+ * Per plane DDB entry can in a really worst case be on multiple slices
+ * but single entry is anyway contigious.
+ */
+ while (start_slice <= end_slice) {
+ slice_mask |= BIT(start_slice);
+ start_slice++;
+ }
+
+ return slice_mask;
+}
+
static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
u8 active_pipes);
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 614ac7f8d4cc..f8fc7eecadb6 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -38,6 +38,9 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
struct skl_ddb_entry *ddb_y,
struct skl_ddb_entry *ddb_uv);
void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
+u16 intel_get_ddb_size(struct drm_i915_private *dev_priv);
+u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
+ const struct skl_ddb_entry *entry);
void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
struct skl_pipe_wm *out);
void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs Stanislav Lisovskiy
@ 2020-05-14 15:43 ` Stanislav Lisovskiy
2020-05-14 20:15 ` Manasi Navare
2020-05-14 18:26 ` kbuild test robot
2020-05-16 21:49 ` kbuild test robot
2 siblings, 1 reply; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:43 UTC (permalink / raw)
To: intel-gfx
According to BSpec max BW per slice is calculated using formula
Max BW = CDCLK * 64. Currently when calculating min CDCLK we
account only per plane requirements, however in order to avoid
FIFO underruns we need to estimate accumulated BW consumed by
all planes(ddb entries basically) residing on that particular
DBuf slice. This will allow us to put CDCLK lower and save power
when we don't need that much bandwidth or gain additional
performance once plane consumption grows.
v2: - Fix long line warning
- Limited new DBuf bw checks to only gens >= 11
v3: - Lets track used Dbuf bw per slice and per crtc in bw state
(or may be in DBuf state in future), that way we don't need
to have all crtcs in state and those only if we detect if
are actually going to change cdclk, just same way as we
do with other stuff, i.e intel_atomic_serialize_global_state
and co. Just as per Ville's paradigm.
- Made dbuf bw calculation procedure look nicer by introducing
for_each_dbuf_slice_in_mask - we often will now need to iterate
slices using mask.
- According to experimental results CDCLK * 64 accounts for
overall bandwidth across all dbufs, not per dbuf.
v4: - Fixed missing const(Ville)
- Removed spurious whitespaces(Ville)
- Fixed local variable init(reduced scope where not needed)
- Added some comments about data rate for planar formats
- Changed struct intel_crtc_bw to intel_dbuf_bw
- Moved dbuf bw calculation to intel_compute_min_cdclk(Ville)
v5: - Removed unneeded macro
v6: - Prevent too frequent CDCLK switching back and forth:
Always switch to higher CDCLK when needed to prevent bandwidth
issues, however don't switch to lower CDCLK earlier than once
in 30 minutes in order to prevent constant modeset blinking.
We could of course not switch back at all, however this is
bad from power consumption point of view.
v7: - Fixed to track cdclk using bw_state, modeset will be now
triggered only when CDCLK change is really needed.
v8: - Lock global state if bw_state->min_cdclk is changed.
- Try getting bw_state only if there are crtcs in the commit
(need to have read-locked global state)
v9: - Do not do Dbuf bw check for gens < 9 - triggers WARN
as ddb_size is 0.
v10: - Lock global state for older gens as well.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_bw.c | 129 ++++++++++++++++++-
drivers/gpu/drm/i915/display/intel_bw.h | 9 ++
drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++-
drivers/gpu/drm/i915/display/intel_cdclk.h | 1 -
drivers/gpu/drm/i915/display/intel_display.c | 38 +++++-
drivers/gpu/drm/i915/intel_pm.c | 31 ++++-
drivers/gpu/drm/i915/intel_pm.h | 3 +
7 files changed, 215 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 6e7cc3a4f1aa..d00338dba7e2 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -6,8 +6,10 @@
#include <drm/drm_atomic_state_helper.h>
#include "intel_bw.h"
+#include "intel_pm.h"
#include "intel_display_types.h"
#include "intel_sideband.h"
+#include "intel_cdclk.h"
/* Parameters for Qclk Geyserville (QGV) */
struct intel_qgv_point {
@@ -333,7 +335,6 @@ static unsigned int intel_bw_crtc_data_rate(const struct intel_crtc_state *crtc_
return data_rate;
}
-
void intel_bw_crtc_update(struct intel_bw_state *bw_state,
const struct intel_crtc_state *crtc_state)
{
@@ -410,6 +411,132 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
return to_intel_bw_state(bw_state);
}
+static int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ int i;
+ const struct intel_crtc_state *crtc_state;
+ struct intel_crtc *crtc;
+ int max_bw = 0;
+ int slice_id;
+ struct intel_bw_state *new_bw_state = NULL;
+ struct intel_bw_state *old_bw_state = NULL;
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ enum plane_id plane_id;
+ struct intel_dbuf_bw *crtc_bw;
+
+ new_bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(new_bw_state))
+ return PTR_ERR(new_bw_state);
+
+ crtc_bw = &new_bw_state->dbuf_bw[crtc->pipe];
+
+ memset(&crtc_bw->used_bw, 0, sizeof(crtc_bw->used_bw));
+
+ for_each_plane_id_on_crtc(crtc, plane_id) {
+ const struct skl_ddb_entry *plane_alloc =
+ &crtc_state->wm.skl.plane_ddb_y[plane_id];
+ const struct skl_ddb_entry *uv_plane_alloc =
+ &crtc_state->wm.skl.plane_ddb_uv[plane_id];
+ unsigned int data_rate = crtc_state->data_rate[plane_id];
+ unsigned int dbuf_mask = 0;
+
+ dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, plane_alloc);
+ dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, uv_plane_alloc);
+
+ /*
+ * FIXME: To calculate that more properly we probably need to
+ * to split per plane data_rate into data_rate_y and data_rate_uv
+ * for multiplanar formats in order not to get accounted those twice
+ * if they happen to reside on different slices.
+ * However for pre-icl this would work anyway because we have only single
+ * slice and for icl+ uv plane has non-zero data rate.
+ * So in worst case those calculation are a bit pessimistic, which
+ * shouldn't pose any significant problem anyway.
+ */
+ for_each_dbuf_slice_in_mask(slice_id, dbuf_mask)
+ crtc_bw->used_bw[slice_id] += data_rate;
+ }
+
+ for_each_dbuf_slice(slice_id) {
+ /*
+ * Current experimental observations show that contrary to BSpec
+ * we get underruns once we exceed 64 * CDCLK for slices in total.
+ * As a temporary measure in order not to keep CDCLK bumped up all the
+ * time we calculate CDCLK according to this formula for overall bw
+ * consumed by slices.
+ */
+ max_bw += crtc_bw->used_bw[slice_id];
+ }
+
+ new_bw_state->min_cdclk = max_bw / 64;
+
+ old_bw_state = intel_atomic_get_old_bw_state(state);
+ }
+
+ if (!old_bw_state)
+ return 0;
+
+ if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
+ int ret = intel_atomic_lock_global_state(&new_bw_state->base);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+int intel_bw_calc_min_cdclk(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ int i;
+ const struct intel_crtc_state *crtc_state;
+ struct intel_crtc *crtc;
+
+ /*
+ * Older gens might not have dbuf/ddb and for
+ * compatibility will have same min_cdclk in bw_state
+ * as cdclk state does.
+ */
+ if (INTEL_GEN(dev_priv) >= 9) {
+ return skl_bw_calc_min_cdclk(state);
+ } else {
+ int min_cdclk = 0;
+ struct intel_bw_state *new_bw_state = NULL;
+ struct intel_bw_state *old_bw_state = NULL;
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_cdclk_state *cdclk_state;
+
+ new_bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(new_bw_state))
+ return PTR_ERR(new_bw_state);
+
+ cdclk_state = intel_atomic_get_cdclk_state(state);
+ if (IS_ERR(cdclk_state))
+ return PTR_ERR(cdclk_state);
+
+ min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ new_bw_state->min_cdclk = min_cdclk;
+
+ old_bw_state = intel_atomic_get_old_bw_state(state);
+ }
+
+ if (!old_bw_state)
+ return 0;
+
+ if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
+ int ret = intel_atomic_lock_global_state(&new_bw_state->base);
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
int intel_bw_atomic_check(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index 898b4a85ccab..d8c92a59ba49 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -10,13 +10,19 @@
#include "intel_display.h"
#include "intel_global_state.h"
+#include "intel_display_power.h"
struct drm_i915_private;
struct intel_atomic_state;
struct intel_crtc_state;
+struct intel_dbuf_bw {
+ int used_bw[I915_MAX_DBUF_SLICES];
+};
+
struct intel_bw_state {
struct intel_global_state base;
+ struct intel_dbuf_bw dbuf_bw[I915_MAX_PIPES];
/*
* Contains a bit mask, used to determine, whether correspondent
@@ -29,6 +35,8 @@ struct intel_bw_state {
/* bitmask of active pipes */
u8 active_pipes;
+
+ int min_cdclk;
};
#define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base)
@@ -47,5 +55,6 @@ int intel_bw_init(struct drm_i915_private *dev_priv);
int intel_bw_atomic_check(struct intel_atomic_state *state);
void intel_bw_crtc_update(struct intel_bw_state *bw_state,
const struct intel_crtc_state *crtc_state);
+int intel_bw_calc_min_cdclk(struct intel_atomic_state *state);
#endif /* __INTEL_BW_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 979a0241fdcb..28750d1f914b 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -21,10 +21,12 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include <linux/time.h>
#include "intel_atomic.h"
#include "intel_cdclk.h"
#include "intel_display_types.h"
#include "intel_sideband.h"
+#include "intel_bw.h"
/**
* DOC: CDCLK / RAWCLK
@@ -2093,11 +2095,9 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
{
struct intel_atomic_state *state = cdclk_state->base.state;
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_crtc *crtc;
struct intel_crtc_state *crtc_state;
int min_cdclk, i;
- enum pipe pipe;
for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
int ret;
@@ -2117,8 +2117,18 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
}
min_cdclk = cdclk_state->force_min_cdclk;
- for_each_pipe(dev_priv, pipe)
- min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk);
+
+ for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+ struct intel_bw_state *bw_state;
+
+ min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(bw_state))
+ return PTR_ERR(bw_state);
+
+ min_cdclk = max(bw_state->min_cdclk, min_cdclk);
+ }
return min_cdclk;
}
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 5731806e4cee..d62e11d620c0 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -7,7 +7,6 @@
#define __INTEL_CDCLK_H__
#include <linux/types.h>
-
#include "i915_drv.h"
#include "intel_display.h"
#include "intel_global_state.h"
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 800ae3768841..7d460ccfda8d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14634,16 +14634,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
bool *need_cdclk_calc)
{
- struct intel_cdclk_state *new_cdclk_state;
int i;
struct intel_plane_state *plane_state;
struct intel_plane *plane;
int ret;
-
- new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
- if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
- *need_cdclk_calc = true;
-
+ struct intel_cdclk_state *new_cdclk_state;
+ struct intel_crtc_state *new_crtc_state;
+ struct intel_crtc *crtc;
/*
* active_planes bitmask has been updated, and potentially
* affected planes are part of the state. We can now
@@ -14655,6 +14652,35 @@ static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
return ret;
}
+ new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+
+ if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
+ *need_cdclk_calc = true;
+
+ ret = intel_bw_calc_min_cdclk(state);
+ if (ret)
+ return ret;
+
+ if (!new_cdclk_state)
+ return 0;
+
+ for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+ struct intel_bw_state *bw_state;
+ int min_cdclk = 0;
+
+ min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
+
+ bw_state = intel_atomic_get_bw_state(state);
+ if (IS_ERR(bw_state))
+ return PTR_ERR(bw_state);
+
+ /*
+ * Currently do this change only if we need to increase
+ */
+ if (bw_state->min_cdclk > min_cdclk)
+ *need_cdclk_calc = true;
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f7bd1dbb625e..684339f44559 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3939,10 +3939,9 @@ icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
return offset;
}
-static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
+u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
{
u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
-
drm_WARN_ON(&dev_priv->drm, ddb_size == 0);
if (INTEL_GEN(dev_priv) < 11)
@@ -3951,6 +3950,34 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
return ddb_size;
}
+u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
+ const struct skl_ddb_entry *entry)
+{
+ u32 slice_mask = 0;
+ u16 ddb_size = intel_get_ddb_size(dev_priv);
+ u16 num_supported_slices = INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
+ u16 slice_size = ddb_size / num_supported_slices;
+ u16 start_slice;
+ u16 end_slice;
+
+ if (!skl_ddb_entry_size(entry))
+ return 0;
+
+ start_slice = entry->start / slice_size;
+ end_slice = (entry->end - 1) / slice_size;
+
+ /*
+ * Per plane DDB entry can in a really worst case be on multiple slices
+ * but single entry is anyway contigious.
+ */
+ while (start_slice <= end_slice) {
+ slice_mask |= BIT(start_slice);
+ start_slice++;
+ }
+
+ return slice_mask;
+}
+
static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
u8 active_pipes);
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 614ac7f8d4cc..f8fc7eecadb6 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -38,6 +38,9 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
struct skl_ddb_entry *ddb_y,
struct skl_ddb_entry *ddb_uv);
void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
+u16 intel_get_ddb_size(struct drm_i915_private *dev_priv);
+u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
+ const struct skl_ddb_entry *entry);
void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
struct skl_pipe_wm *out);
void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
2020-05-14 15:43 ` Stanislav Lisovskiy
@ 2020-05-14 20:15 ` Manasi Navare
0 siblings, 0 replies; 19+ messages in thread
From: Manasi Navare @ 2020-05-14 20:15 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
The logic seems fine, some comments below:
On Thu, May 14, 2020 at 06:43:37PM +0300, Stanislav Lisovskiy wrote:
> According to BSpec max BW per slice is calculated using formula
> Max BW = CDCLK * 64. Currently when calculating min CDCLK we
> account only per plane requirements, however in order to avoid
> FIFO underruns we need to estimate accumulated BW consumed by
> all planes(ddb entries basically) residing on that particular
> DBuf slice. This will allow us to put CDCLK lower and save power
> when we don't need that much bandwidth or gain additional
> performance once plane consumption grows.
>
> v2: - Fix long line warning
> - Limited new DBuf bw checks to only gens >= 11
>
> v3: - Lets track used Dbuf bw per slice and per crtc in bw state
> (or may be in DBuf state in future), that way we don't need
> to have all crtcs in state and those only if we detect if
> are actually going to change cdclk, just same way as we
> do with other stuff, i.e intel_atomic_serialize_global_state
> and co. Just as per Ville's paradigm.
> - Made dbuf bw calculation procedure look nicer by introducing
> for_each_dbuf_slice_in_mask - we often will now need to iterate
> slices using mask.
> - According to experimental results CDCLK * 64 accounts for
> overall bandwidth across all dbufs, not per dbuf.
>
> v4: - Fixed missing const(Ville)
> - Removed spurious whitespaces(Ville)
> - Fixed local variable init(reduced scope where not needed)
> - Added some comments about data rate for planar formats
> - Changed struct intel_crtc_bw to intel_dbuf_bw
> - Moved dbuf bw calculation to intel_compute_min_cdclk(Ville)
>
> v5: - Removed unneeded macro
>
> v6: - Prevent too frequent CDCLK switching back and forth:
> Always switch to higher CDCLK when needed to prevent bandwidth
> issues, however don't switch to lower CDCLK earlier than once
> in 30 minutes in order to prevent constant modeset blinking.
> We could of course not switch back at all, however this is
> bad from power consumption point of view.
>
> v7: - Fixed to track cdclk using bw_state, modeset will be now
> triggered only when CDCLK change is really needed.
>
> v8: - Lock global state if bw_state->min_cdclk is changed.
> - Try getting bw_state only if there are crtcs in the commit
> (need to have read-locked global state)
>
> v9: - Do not do Dbuf bw check for gens < 9 - triggers WARN
> as ddb_size is 0.
>
> v10: - Lock global state for older gens as well.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_bw.c | 129 ++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_bw.h | 9 ++
> drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++-
> drivers/gpu/drm/i915/display/intel_cdclk.h | 1 -
> drivers/gpu/drm/i915/display/intel_display.c | 38 +++++-
> drivers/gpu/drm/i915/intel_pm.c | 31 ++++-
> drivers/gpu/drm/i915/intel_pm.h | 3 +
> 7 files changed, 215 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 6e7cc3a4f1aa..d00338dba7e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -6,8 +6,10 @@
> #include <drm/drm_atomic_state_helper.h>
>
> #include "intel_bw.h"
> +#include "intel_pm.h"
> #include "intel_display_types.h"
> #include "intel_sideband.h"
> +#include "intel_cdclk.h"
>
> /* Parameters for Qclk Geyserville (QGV) */
> struct intel_qgv_point {
> @@ -333,7 +335,6 @@ static unsigned int intel_bw_crtc_data_rate(const struct intel_crtc_state *crtc_
>
> return data_rate;
> }
> -
> void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> const struct intel_crtc_state *crtc_state)
> {
> @@ -410,6 +411,132 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> return to_intel_bw_state(bw_state);
> }
>
> +static int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + int i;
> + const struct intel_crtc_state *crtc_state;
> + struct intel_crtc *crtc;
> + int max_bw = 0;
> + int slice_id;
> + struct intel_bw_state *new_bw_state = NULL;
> + struct intel_bw_state *old_bw_state = NULL;
> +
> + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> + enum plane_id plane_id;
> + struct intel_dbuf_bw *crtc_bw;
> +
> + new_bw_state = intel_atomic_get_bw_state(state);
> + if (IS_ERR(new_bw_state))
> + return PTR_ERR(new_bw_state);
> +
> + crtc_bw = &new_bw_state->dbuf_bw[crtc->pipe];
> +
> + memset(&crtc_bw->used_bw, 0, sizeof(crtc_bw->used_bw));
> +
> + for_each_plane_id_on_crtc(crtc, plane_id) {
> + const struct skl_ddb_entry *plane_alloc =
> + &crtc_state->wm.skl.plane_ddb_y[plane_id];
> + const struct skl_ddb_entry *uv_plane_alloc =
> + &crtc_state->wm.skl.plane_ddb_uv[plane_id];
> + unsigned int data_rate = crtc_state->data_rate[plane_id];
> + unsigned int dbuf_mask = 0;
> +
> + dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, plane_alloc);
> + dbuf_mask |= skl_ddb_dbuf_slice_mask(dev_priv, uv_plane_alloc);
> +
> + /*
> + * FIXME: To calculate that more properly we probably need to
> + * to split per plane data_rate into data_rate_y and data_rate_uv
> + * for multiplanar formats in order not to get accounted those twice
> + * if they happen to reside on different slices.
> + * However for pre-icl this would work anyway because we have only single
> + * slice and for icl+ uv plane has non-zero data rate.
> + * So in worst case those calculation are a bit pessimistic, which
> + * shouldn't pose any significant problem anyway.
> + */
> + for_each_dbuf_slice_in_mask(slice_id, dbuf_mask)
> + crtc_bw->used_bw[slice_id] += data_rate;
> + }
> +
> + for_each_dbuf_slice(slice_id) {
> + /*
> + * Current experimental observations show that contrary to BSpec
> + * we get underruns once we exceed 64 * CDCLK for slices in total.
> + * As a temporary measure in order not to keep CDCLK bumped up all the
> + * time we calculate CDCLK according to this formula for overall bw
> + * consumed by slices.
> + */
> + max_bw += crtc_bw->used_bw[slice_id];
> + }
> +
> + new_bw_state->min_cdclk = max_bw / 64;
> +
> + old_bw_state = intel_atomic_get_old_bw_state(state);
> + }
> +
> + if (!old_bw_state)
> + return 0;
> +
> + if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
> + int ret = intel_atomic_lock_global_state(&new_bw_state->base);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int intel_bw_calc_min_cdclk(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + int i;
> + const struct intel_crtc_state *crtc_state;
> + struct intel_crtc *crtc;
> +
> + /*
> + * Older gens might not have dbuf/ddb and for
> + * compatibility will have same min_cdclk in bw_state
> + * as cdclk state does.
> + */
> + if (INTEL_GEN(dev_priv) >= 9) {
> + return skl_bw_calc_min_cdclk(state);
> + } else {
> + int min_cdclk = 0;
> + struct intel_bw_state *new_bw_state = NULL;
> + struct intel_bw_state *old_bw_state = NULL;
> +
> + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> + struct intel_cdclk_state *cdclk_state;
> +
> + new_bw_state = intel_atomic_get_bw_state(state);
> + if (IS_ERR(new_bw_state))
> + return PTR_ERR(new_bw_state);
> +
> + cdclk_state = intel_atomic_get_cdclk_state(state);
> + if (IS_ERR(cdclk_state))
> + return PTR_ERR(cdclk_state);
> +
> + min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
> +
> + new_bw_state->min_cdclk = min_cdclk;
> +
> + old_bw_state = intel_atomic_get_old_bw_state(state);
> + }
> +
> + if (!old_bw_state)
> + return 0;
> +
> + if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
> + int ret = intel_atomic_lock_global_state(&new_bw_state->base);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
Is it better to define a new hook for intel_bw_calc_min_cdclk() where the bw_calc_min_cdclk() hook for Gen <9 gets assigned to
intel_bw_calc_min_cdclk() and Gen >=9 gets assigned to skl_bw_calc_min_cdclk instead of the if ..else in this function.
This also makes the solution more scalable if this logic changes for further platforms and this also follows
the same terminology as other per platforms hooks.
Manasi
> +
> int intel_bw_atomic_check(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 898b4a85ccab..d8c92a59ba49 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -10,13 +10,19 @@
>
> #include "intel_display.h"
> #include "intel_global_state.h"
> +#include "intel_display_power.h"
>
> struct drm_i915_private;
> struct intel_atomic_state;
> struct intel_crtc_state;
>
> +struct intel_dbuf_bw {
> + int used_bw[I915_MAX_DBUF_SLICES];
> +};
> +
> struct intel_bw_state {
> struct intel_global_state base;
> + struct intel_dbuf_bw dbuf_bw[I915_MAX_PIPES];
>
> /*
> * Contains a bit mask, used to determine, whether correspondent
> @@ -29,6 +35,8 @@ struct intel_bw_state {
>
> /* bitmask of active pipes */
> u8 active_pipes;
> +
> + int min_cdclk;
> };
>
> #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base)
> @@ -47,5 +55,6 @@ int intel_bw_init(struct drm_i915_private *dev_priv);
> int intel_bw_atomic_check(struct intel_atomic_state *state);
> void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> const struct intel_crtc_state *crtc_state);
> +int intel_bw_calc_min_cdclk(struct intel_atomic_state *state);
>
> #endif /* __INTEL_BW_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 979a0241fdcb..28750d1f914b 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -21,10 +21,12 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +#include <linux/time.h>
> #include "intel_atomic.h"
> #include "intel_cdclk.h"
> #include "intel_display_types.h"
> #include "intel_sideband.h"
> +#include "intel_bw.h"
>
> /**
> * DOC: CDCLK / RAWCLK
> @@ -2093,11 +2095,9 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
> {
> struct intel_atomic_state *state = cdclk_state->base.state;
> - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_crtc *crtc;
> struct intel_crtc_state *crtc_state;
> int min_cdclk, i;
> - enum pipe pipe;
>
> for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> int ret;
> @@ -2117,8 +2117,18 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
> }
>
> min_cdclk = cdclk_state->force_min_cdclk;
> - for_each_pipe(dev_priv, pipe)
> - min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk);
> +
> + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> + struct intel_bw_state *bw_state;
> +
> + min_cdclk = max(cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
> +
> + bw_state = intel_atomic_get_bw_state(state);
> + if (IS_ERR(bw_state))
> + return PTR_ERR(bw_state);
> +
> + min_cdclk = max(bw_state->min_cdclk, min_cdclk);
> + }
>
> return min_cdclk;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 5731806e4cee..d62e11d620c0 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -7,7 +7,6 @@
> #define __INTEL_CDCLK_H__
>
> #include <linux/types.h>
> -
> #include "i915_drv.h"
> #include "intel_display.h"
> #include "intel_global_state.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 800ae3768841..7d460ccfda8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14634,16 +14634,13 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
> bool *need_cdclk_calc)
> {
> - struct intel_cdclk_state *new_cdclk_state;
> int i;
> struct intel_plane_state *plane_state;
> struct intel_plane *plane;
> int ret;
> -
> - new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> - if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
> - *need_cdclk_calc = true;
> -
> + struct intel_cdclk_state *new_cdclk_state;
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_crtc *crtc;
> /*
> * active_planes bitmask has been updated, and potentially
> * affected planes are part of the state. We can now
> @@ -14655,6 +14652,35 @@ static int intel_atomic_check_cdclk(struct intel_atomic_state *state,
> return ret;
> }
>
> + new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
> +
> + if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
> + *need_cdclk_calc = true;
> +
> + ret = intel_bw_calc_min_cdclk(state);
> + if (ret)
> + return ret;
> +
> + if (!new_cdclk_state)
> + return 0;
> +
> + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> + struct intel_bw_state *bw_state;
> + int min_cdclk = 0;
> +
> + min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
> +
> + bw_state = intel_atomic_get_bw_state(state);
> + if (IS_ERR(bw_state))
> + return PTR_ERR(bw_state);
> +
> + /*
> + * Currently do this change only if we need to increase
> + */
> + if (bw_state->min_cdclk > min_cdclk)
> + *need_cdclk_calc = true;
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7bd1dbb625e..684339f44559 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3939,10 +3939,9 @@ icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> return offset;
> }
>
> -static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
> +u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
> {
> u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> -
> drm_WARN_ON(&dev_priv->drm, ddb_size == 0);
>
> if (INTEL_GEN(dev_priv) < 11)
> @@ -3951,6 +3950,34 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
> return ddb_size;
> }
>
> +u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
> + const struct skl_ddb_entry *entry)
> +{
> + u32 slice_mask = 0;
> + u16 ddb_size = intel_get_ddb_size(dev_priv);
> + u16 num_supported_slices = INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
> + u16 slice_size = ddb_size / num_supported_slices;
> + u16 start_slice;
> + u16 end_slice;
> +
> + if (!skl_ddb_entry_size(entry))
> + return 0;
> +
> + start_slice = entry->start / slice_size;
> + end_slice = (entry->end - 1) / slice_size;
> +
> + /*
> + * Per plane DDB entry can in a really worst case be on multiple slices
> + * but single entry is anyway contigious.
> + */
> + while (start_slice <= end_slice) {
> + slice_mask |= BIT(start_slice);
> + start_slice++;
> + }
> +
> + return slice_mask;
> +}
> +
> static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
> u8 active_pipes);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 614ac7f8d4cc..f8fc7eecadb6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -38,6 +38,9 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
> struct skl_ddb_entry *ddb_y,
> struct skl_ddb_entry *ddb_uv);
> void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
> +u16 intel_get_ddb_size(struct drm_i915_private *dev_priv);
> +u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *dev_priv,
> + const struct skl_ddb_entry *entry);
> void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> struct skl_pipe_wm *out);
> void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> --
> 2.24.1.485.gad05a3d8e5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs Stanislav Lisovskiy
@ 2020-05-14 18:26 ` kbuild test robot
2020-05-14 18:26 ` kbuild test robot
2020-05-16 21:49 ` kbuild test robot
2 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-05-14 18:26 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6210 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20200514]
[cannot apply to drm-tip/drm-tip v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Stanislav-Lisovskiy/Consider-DBuf-bandwidth-when-calculating-CDCLK/20200514-232752
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_planes':
>> drivers/gpu/drm/i915/display/intel_display.c:14695:2: error: 'new_cdclk_state' undeclared (first use in this function); did you mean 'intel_cdclk_state'?
new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
^~~~~~~~~~~~~~~
intel_cdclk_state
drivers/gpu/drm/i915/display/intel_display.c:14695:2: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/display/intel_display.c:14698:4: error: 'need_cdclk_calc' undeclared (first use in this function); did you mean 'intel_cdclk_vals'?
*need_cdclk_calc = true;
^~~~~~~~~~~~~~~
intel_cdclk_vals
In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/gpu/drm/i915/display/intel_display.c:27:
include/linux/kernel.h:866:2: error: first argument to '__builtin_choose_expr' not a constant
__builtin_choose_expr(__safe_cmp(x, y), ^
include/linux/kernel.h:882:19: note: in expansion of macro '__careful_cmp'
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~
>> drivers/gpu/drm/i915/display/intel_display.c:14711:15: note: in expansion of macro 'max'
min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
^~~
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_cdclk':
drivers/gpu/drm/i915/display/intel_display.c:14736:21: warning: unused variable 'crtc' [-Wunused-variable]
struct intel_crtc *crtc;
^~~~
drivers/gpu/drm/i915/display/intel_display.c:14735:27: warning: unused variable 'new_crtc_state' [-Wunused-variable]
struct intel_crtc_state *new_crtc_state;
^~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14734:28: warning: unused variable 'new_cdclk_state' [-Wunused-variable]
struct intel_cdclk_state *new_cdclk_state;
^~~~~~~~~~~~~~~
vim +14695 drivers/gpu/drm/i915/display/intel_display.c
14638
14639 static int intel_atomic_check_planes(struct intel_atomic_state *state)
14640 {
14641 struct drm_i915_private *dev_priv = to_i915(state->base.dev);
14642 struct intel_crtc_state *old_crtc_state, *new_crtc_state;
14643 struct intel_plane_state *plane_state;
14644 struct intel_plane *plane;
14645 struct intel_crtc *crtc;
14646 int i, ret;
14647
14648 ret = icl_add_linked_planes(state);
14649 if (ret)
14650 return ret;
14651
14652 for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
14653 ret = intel_plane_atomic_check(state, plane);
14654 if (ret) {
14655 drm_dbg_atomic(&dev_priv->drm,
14656 "[PLANE:%d:%s] atomic driver check failed\n",
14657 plane->base.base.id, plane->base.name);
14658 return ret;
14659 }
14660 }
14661
14662 for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
14663 new_crtc_state, i) {
14664 u8 old_active_planes, new_active_planes;
14665
14666 ret = icl_check_nv12_planes(new_crtc_state);
14667 if (ret)
14668 return ret;
14669
14670 /*
14671 * On some platforms the number of active planes affects
14672 * the planes' minimum cdclk calculation. Add such planes
14673 * to the state before we compute the minimum cdclk.
14674 */
14675 if (!active_planes_affects_min_cdclk(dev_priv))
14676 continue;
14677
14678 old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
14679 new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
14680
14681 /*
14682 * Not only the number of planes, but if the plane configuration had
14683 * changed might already mean we need to recompute min CDCLK,
14684 * because different planes might consume different amount of Dbuf bandwidth
14685 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
14686 */
14687 if (old_active_planes == new_active_planes)
14688 continue;
14689
14690 ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
14691 if (ret)
14692 return ret;
14693 }
14694
14695 new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
14696
14697 if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
14698 *need_cdclk_calc = true;
14699
14700 ret = intel_bw_calc_min_cdclk(state);
14701 if (ret)
14702 return ret;
14703
14704 if (!new_cdclk_state)
14705 return 0;
14706
14707 for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
14708 struct intel_bw_state *bw_state;
14709 int min_cdclk = 0;
14710
14711 min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
14712
14713 bw_state = intel_atomic_get_bw_state(state);
14714 if (IS_ERR(bw_state))
14715 return PTR_ERR(bw_state);
14716
14717 /*
14718 * Currently do this change only if we need to increase
14719 */
14720 if (bw_state->min_cdclk > min_cdclk)
14721 *need_cdclk_calc = true;
14722 }
14723
14724 return 0;
14725 }
14726
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73362 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
@ 2020-05-14 18:26 ` kbuild test robot
0 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-05-14 18:26 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6367 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20200514]
[cannot apply to drm-tip/drm-tip v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Stanislav-Lisovskiy/Consider-DBuf-bandwidth-when-calculating-CDCLK/20200514-232752
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_planes':
>> drivers/gpu/drm/i915/display/intel_display.c:14695:2: error: 'new_cdclk_state' undeclared (first use in this function); did you mean 'intel_cdclk_state'?
new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
^~~~~~~~~~~~~~~
intel_cdclk_state
drivers/gpu/drm/i915/display/intel_display.c:14695:2: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/gpu/drm/i915/display/intel_display.c:14698:4: error: 'need_cdclk_calc' undeclared (first use in this function); did you mean 'intel_cdclk_vals'?
*need_cdclk_calc = true;
^~~~~~~~~~~~~~~
intel_cdclk_vals
In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/gpu/drm/i915/display/intel_display.c:27:
include/linux/kernel.h:866:2: error: first argument to '__builtin_choose_expr' not a constant
__builtin_choose_expr(__safe_cmp(x, y), ^
include/linux/kernel.h:882:19: note: in expansion of macro '__careful_cmp'
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~
>> drivers/gpu/drm/i915/display/intel_display.c:14711:15: note: in expansion of macro 'max'
min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
^~~
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_cdclk':
drivers/gpu/drm/i915/display/intel_display.c:14736:21: warning: unused variable 'crtc' [-Wunused-variable]
struct intel_crtc *crtc;
^~~~
drivers/gpu/drm/i915/display/intel_display.c:14735:27: warning: unused variable 'new_crtc_state' [-Wunused-variable]
struct intel_crtc_state *new_crtc_state;
^~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14734:28: warning: unused variable 'new_cdclk_state' [-Wunused-variable]
struct intel_cdclk_state *new_cdclk_state;
^~~~~~~~~~~~~~~
vim +14695 drivers/gpu/drm/i915/display/intel_display.c
14638
14639 static int intel_atomic_check_planes(struct intel_atomic_state *state)
14640 {
14641 struct drm_i915_private *dev_priv = to_i915(state->base.dev);
14642 struct intel_crtc_state *old_crtc_state, *new_crtc_state;
14643 struct intel_plane_state *plane_state;
14644 struct intel_plane *plane;
14645 struct intel_crtc *crtc;
14646 int i, ret;
14647
14648 ret = icl_add_linked_planes(state);
14649 if (ret)
14650 return ret;
14651
14652 for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
14653 ret = intel_plane_atomic_check(state, plane);
14654 if (ret) {
14655 drm_dbg_atomic(&dev_priv->drm,
14656 "[PLANE:%d:%s] atomic driver check failed\n",
14657 plane->base.base.id, plane->base.name);
14658 return ret;
14659 }
14660 }
14661
14662 for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
14663 new_crtc_state, i) {
14664 u8 old_active_planes, new_active_planes;
14665
14666 ret = icl_check_nv12_planes(new_crtc_state);
14667 if (ret)
14668 return ret;
14669
14670 /*
14671 * On some platforms the number of active planes affects
14672 * the planes' minimum cdclk calculation. Add such planes
14673 * to the state before we compute the minimum cdclk.
14674 */
14675 if (!active_planes_affects_min_cdclk(dev_priv))
14676 continue;
14677
14678 old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
14679 new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
14680
14681 /*
14682 * Not only the number of planes, but if the plane configuration had
14683 * changed might already mean we need to recompute min CDCLK,
14684 * because different planes might consume different amount of Dbuf bandwidth
14685 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
14686 */
14687 if (old_active_planes == new_active_planes)
14688 continue;
14689
14690 ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
14691 if (ret)
14692 return ret;
14693 }
14694
14695 new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
14696
14697 if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
14698 *need_cdclk_calc = true;
14699
14700 ret = intel_bw_calc_min_cdclk(state);
14701 if (ret)
14702 return ret;
14703
14704 if (!new_cdclk_state)
14705 return 0;
14706
14707 for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
14708 struct intel_bw_state *bw_state;
14709 int min_cdclk = 0;
14710
14711 min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
14712
14713 bw_state = intel_atomic_get_bw_state(state);
14714 if (IS_ERR(bw_state))
14715 return PTR_ERR(bw_state);
14716
14717 /*
14718 * Currently do this change only if we need to increase
14719 */
14720 if (bw_state->min_cdclk > min_cdclk)
14721 *need_cdclk_calc = true;
14722 }
14723
14724 return 0;
14725 }
14726
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73362 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs Stanislav Lisovskiy
@ 2020-05-16 21:49 ` kbuild test robot
2020-05-14 18:26 ` kbuild test robot
2020-05-16 21:49 ` kbuild test robot
2 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-05-16 21:49 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4282 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to drm-tip/drm-tip v5.7-rc5 next-20200515]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Stanislav-Lisovskiy/Consider-DBuf-bandwidth-when-calculating-CDCLK/20200514-232752
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-allyesconfig (attached as .config)
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-193-gb8fad4bc-dirty
# save the attached .config to linux build tree
make C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_planes':
drivers/gpu/drm/i915/display/intel_display.c:14695:2: error: 'new_cdclk_state' undeclared (first use in this function); did you mean 'intel_cdclk_state'?
new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
^~~~~~~~~~~~~~~
intel_cdclk_state
drivers/gpu/drm/i915/display/intel_display.c:14695:2: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/display/intel_display.c:14698:4: error: 'need_cdclk_calc' undeclared (first use in this function); did you mean 'intel_cdclk_vals'?
*need_cdclk_calc = true;
^~~~~~~~~~~~~~~
intel_cdclk_vals
In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/gpu/drm/i915/display/intel_display.c:27:
>> include/linux/kernel.h:866:2: error: first argument to '__builtin_choose_expr' not a constant
__builtin_choose_expr(__safe_cmp(x, y), ^
include/linux/kernel.h:882:19: note: in expansion of macro '__careful_cmp'
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14711:15: note: in expansion of macro 'max'
min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
^~~
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_cdclk':
drivers/gpu/drm/i915/display/intel_display.c:14736:21: warning: unused variable 'crtc' [-Wunused-variable]
struct intel_crtc *crtc;
^~~~
drivers/gpu/drm/i915/display/intel_display.c:14735:27: warning: unused variable 'new_crtc_state' [-Wunused-variable]
struct intel_crtc_state *new_crtc_state;
^~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14734:28: warning: unused variable 'new_cdclk_state' [-Wunused-variable]
struct intel_cdclk_state *new_cdclk_state;
^~~~~~~~~~~~~~~
vim +/__builtin_choose_expr +866 include/linux/kernel.h
3c8ba0d61d04ce Kees Cook 2018-03-30 859
e9092d0d979611 Linus Torvalds 2018-04-09 860 #define __cmp_once(x, y, unique_x, unique_y, op) ({ \
e9092d0d979611 Linus Torvalds 2018-04-09 861 typeof(x) unique_x = (x); \
e9092d0d979611 Linus Torvalds 2018-04-09 862 typeof(y) unique_y = (y); \
e9092d0d979611 Linus Torvalds 2018-04-09 863 __cmp(unique_x, unique_y, op); })
3c8ba0d61d04ce Kees Cook 2018-03-30 864
3c8ba0d61d04ce Kees Cook 2018-03-30 865 #define __careful_cmp(x, y, op) \
3c8ba0d61d04ce Kees Cook 2018-03-30 @866 __builtin_choose_expr(__safe_cmp(x, y), \
e9092d0d979611 Linus Torvalds 2018-04-09 867 __cmp(x, y, op), \
e9092d0d979611 Linus Torvalds 2018-04-09 868 __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
e8c97af0c1f23d Randy Dunlap 2017-10-13 869
:::::: The code at line 866 was first introduced by commit
:::::: 3c8ba0d61d04ced9f8d9ff93977995a9e4e96e91 kernel.h: Retain constant expression output for max()/min()
:::::: TO: Kees Cook <keescook@chromium.org>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72435 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
@ 2020-05-16 21:49 ` kbuild test robot
0 siblings, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2020-05-16 21:49 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4367 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[cannot apply to drm-tip/drm-tip v5.7-rc5 next-20200515]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Stanislav-Lisovskiy/Consider-DBuf-bandwidth-when-calculating-CDCLK/20200514-232752
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-allyesconfig (attached as .config)
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-193-gb8fad4bc-dirty
# save the attached .config to linux build tree
make C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_planes':
drivers/gpu/drm/i915/display/intel_display.c:14695:2: error: 'new_cdclk_state' undeclared (first use in this function); did you mean 'intel_cdclk_state'?
new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
^~~~~~~~~~~~~~~
intel_cdclk_state
drivers/gpu/drm/i915/display/intel_display.c:14695:2: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/display/intel_display.c:14698:4: error: 'need_cdclk_calc' undeclared (first use in this function); did you mean 'intel_cdclk_vals'?
*need_cdclk_calc = true;
^~~~~~~~~~~~~~~
intel_cdclk_vals
In file included from include/linux/list.h:9:0,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/gpu/drm/i915/display/intel_display.c:27:
>> include/linux/kernel.h:866:2: error: first argument to '__builtin_choose_expr' not a constant
__builtin_choose_expr(__safe_cmp(x, y), ^
include/linux/kernel.h:882:19: note: in expansion of macro '__careful_cmp'
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14711:15: note: in expansion of macro 'max'
min_cdclk = max(new_cdclk_state->min_cdclk[crtc->pipe], min_cdclk);
^~~
drivers/gpu/drm/i915/display/intel_display.c: In function 'intel_atomic_check_cdclk':
drivers/gpu/drm/i915/display/intel_display.c:14736:21: warning: unused variable 'crtc' [-Wunused-variable]
struct intel_crtc *crtc;
^~~~
drivers/gpu/drm/i915/display/intel_display.c:14735:27: warning: unused variable 'new_crtc_state' [-Wunused-variable]
struct intel_crtc_state *new_crtc_state;
^~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display.c:14734:28: warning: unused variable 'new_cdclk_state' [-Wunused-variable]
struct intel_cdclk_state *new_cdclk_state;
^~~~~~~~~~~~~~~
vim +/__builtin_choose_expr +866 include/linux/kernel.h
3c8ba0d61d04ce Kees Cook 2018-03-30 859
e9092d0d979611 Linus Torvalds 2018-04-09 860 #define __cmp_once(x, y, unique_x, unique_y, op) ({ \
e9092d0d979611 Linus Torvalds 2018-04-09 861 typeof(x) unique_x = (x); \
e9092d0d979611 Linus Torvalds 2018-04-09 862 typeof(y) unique_y = (y); \
e9092d0d979611 Linus Torvalds 2018-04-09 863 __cmp(unique_x, unique_y, op); })
3c8ba0d61d04ce Kees Cook 2018-03-30 864
3c8ba0d61d04ce Kees Cook 2018-03-30 865 #define __careful_cmp(x, y, op) \
3c8ba0d61d04ce Kees Cook 2018-03-30 @866 __builtin_choose_expr(__safe_cmp(x, y), \
e9092d0d979611 Linus Torvalds 2018-04-09 867 __cmp(x, y, op), \
e9092d0d979611 Linus Torvalds 2018-04-09 868 __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
e8c97af0c1f23d Randy Dunlap 2017-10-13 869
:::::: The code at line 866 was first introduced by commit
:::::: 3c8ba0d61d04ced9f8d9ff93977995a9e4e96e91 kernel.h: Retain constant expression output for max()/min()
:::::: TO: Kees Cook <keescook@chromium.org>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 72435 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] [PATCH v8 7/7] drm/i915: Remove unneeded hack now for CDCLK
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
` (5 preceding siblings ...)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 6/7] drm/i915: Adjust CDCLK accordingly to our DBuf bw needs Stanislav Lisovskiy
@ 2020-05-14 15:21 ` Stanislav Lisovskiy
2020-05-15 15:11 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Consider DBuf bandwidth when calculating CDCLK (rev13) Patchwork
7 siblings, 0 replies; 19+ messages in thread
From: Stanislav Lisovskiy @ 2020-05-14 15:21 UTC (permalink / raw)
To: intel-gfx
No need to bump up CDCLK now, as it is now correctly
calculated, accounting for DBuf BW as BSpec says.
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 28750d1f914b..a7986df12bd8 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2070,18 +2070,6 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
/* Account for additional needs from the planes */
min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
- /*
- * HACK. Currently for TGL platforms we calculate
- * min_cdclk initially based on pixel_rate divided
- * by 2, accounting for also plane requirements,
- * however in some cases the lowest possible CDCLK
- * doesn't work and causing the underruns.
- * Explicitly stating here that this seems to be currently
- * rather a Hack, than final solution.
- */
- if (IS_TIGERLAKE(dev_priv))
- min_cdclk = max(min_cdclk, (int)crtc_state->pixel_rate);
-
if (min_cdclk > dev_priv->max_cdclk_freq) {
drm_dbg_kms(&dev_priv->drm,
"required cdclk (%d kHz) exceeds max (%d kHz)\n",
--
2.24.1.485.gad05a3d8e5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for Consider DBuf bandwidth when calculating CDCLK (rev13)
2020-05-14 15:21 [Intel-gfx] [PATCH v8 0/7] Consider DBuf bandwidth when calculating CDCLK Stanislav Lisovskiy
` (6 preceding siblings ...)
2020-05-14 15:21 ` [Intel-gfx] [PATCH v8 7/7] drm/i915: Remove unneeded hack now for CDCLK Stanislav Lisovskiy
@ 2020-05-15 15:11 ` Patchwork
7 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2020-05-15 15:11 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
== Series Details ==
Series: Consider DBuf bandwidth when calculating CDCLK (rev13)
URL : https://patchwork.freedesktop.org/series/74739/
State : failure
== Summary ==
Applying: drm/i915: Decouple cdclk calculation from modeset checks
Applying: drm/i915: Extract cdclk requirements checking to separate function
Applying: drm/i915: Check plane configuration properly
Applying: drm/i915: Plane configuration affects CDCLK in Gen11+
Applying: drm/i915: Introduce for_each_dbuf_slice_in_mask macro
Applying: drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/display/intel_bw.c
M drivers/gpu/drm/i915/display/intel_bw.h
M drivers/gpu/drm/i915/display/intel_display.c
M drivers/gpu/drm/i915/intel_pm.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_pm.c
Auto-merging drivers/gpu/drm/i915/display/intel_display.c
Auto-merging drivers/gpu/drm/i915/display/intel_bw.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_bw.h
Auto-merging drivers/gpu/drm/i915/display/intel_bw.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_bw.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0006 drm/i915: Adjust CDCLK accordingly to our DBuf bw needs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread