Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
On Mon, 2024-02-19 at 12:06 +0530, Suraj Kandpal wrote:
> If fixed refresh rate program the PKGC_LATENCY register
> with the highest latency from level 1 and above LP registers
> and program ADDED_WAKE_TIME = DSB execution time.
> else program PKGC_LATENCY with all 1's and ADDED_WAKE_TIME as 0.
> This is used to improve package C residency by sending the highest
> latency tolerance requirement (LTR) when the planes are done with the
> frame until the next frame programming window (set context latency,
> window 2) starts.
> Bspec: 68986
>
> --v2
> -Fix indentation [Chaitanya]
>
> --v3
> -Take into account if fixed refrersh rate or not [Vinod]
> -Added wake time dependengt on DSB execution time [Vinod]
> -Use REG_FIELD_PREP [Jani]
> -Call program_pkgc_latency from appropriate place [Jani]
> -no need for the ~0 while setting max latency [Jani]
> -change commit message to add the new changes made in.
>
> --v4
> -Remove extra blank line [Vinod]
> -move the vrr.enable check to previous loop [Vinod]
>
> Signed-off-by: Suraj Kandpal
> Reviewed-by: Chaitanya Kumar Borah
> ---
> drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
> drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++-
> drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-
> 3 files changed, 55 insertions(+), 5 deletions(-)
Reviewed-by: Vinod Govindapillai
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index a6c7122fd671..d62e050185e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -325,7 +325,7 @@ static int intel_dsb_dewake_scanline(const struct
> intel_crtc_state
> *crtc_state)
> {
> struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> const struct drm_display_mode *adjusted_mode =
> &crtc_state->hw.adjusted_mode;
> - unsigned int latency = skl_watermark_max_latency(i915);
> + unsigned int latency = skl_watermark_max_latency(i915, 0);
> int vblank_start;
>
> if (crtc_state->vrr.enable) {
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 614f319d754e..c6b9be80d83c 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -23,6 +23,12 @@
> #include "skl_watermark.h"
> #include "skl_watermark_regs.h"
>
> +/*It is expected that DSB can do posted writes to every register in
> + * the pipe and planes within 100us. For flip queue use case, the
> + * recommended DSB execution time is 100us + one SAGV block time.
> + */
> +#define DSB_EXE_TIME 100
> +
> static void skl_sagv_disable(struct drm_i915_private *i915);
>
> /* Stores plane specific WM parameters */
> @@ -2904,12 +2910,51 @@ static int skl_wm_add_affected_planes(struct
> intel_atomic_state *state,
> return 0;
> }
>
> +/*
> + * If Fixed Refresh Rate:
> + * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = DSB execution time
> + * If Variable Refresh Rate:
> + * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0
> + */
> +static void
> +skl_program_dpkgc_latency(struct drm_i915_private *i915, bool vrr_enabled)
> +{
> + u32 max_latency = 0;
> + u32 clear = 0, val = 0;
> + u32 added_wake_time = 0;
> +
> + if (DISPLAY_VER(i915) < 20)
> + return;
> +
> + if (vrr_enabled) {
> + max_latency = LNL_PKG_C_LATENCY_MASK;
> + added_wake_time = 0;
> + } else {
> + max_latency = skl_watermark_max_latency(i915, 1);
> + if (max_latency == 0)
> + max_latency = LNL_PKG_C_LATENCY_MASK;
> + added_wake_time = DSB_EXE_TIME +
> + i915->display.sagv.block_time_us;
> + }
> +
> + clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
> + val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_latency);
> + val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time);
> +
> + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
> +}
> +
> static int
> skl_compute_wm(struct intel_atomic_state *state)
> {
> struct intel_crtc *crtc;
> struct intel_crtc_state __maybe_unused *new_crtc_state;
> int ret, i;
> + bool vrr_enabled = false;
>
> for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> ret = skl_build_pipe_wm(state, crtc);
> @@ -2934,8 +2979,13 @@ skl_compute_wm(struct intel_atomic_state *state)
> ret = skl_wm_add_affected_planes(state, crtc);
> if (ret)
> return ret;
> +
> + if (new_crtc_state->vrr.enable)
> +
[PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
If fixed refresh rate program the PKGC_LATENCY register
with the highest latency from level 1 and above LP registers
and program ADDED_WAKE_TIME = DSB execution time.
else program PKGC_LATENCY with all 1's and ADDED_WAKE_TIME as 0.
This is used to improve package C residency by sending the highest
latency tolerance requirement (LTR) when the planes are done with the
frame until the next frame programming window (set context latency,
window 2) starts.
Bspec: 68986
--v2
-Fix indentation [Chaitanya]
--v3
-Take into account if fixed refrersh rate or not [Vinod]
-Added wake time dependengt on DSB execution time [Vinod]
-Use REG_FIELD_PREP [Jani]
-Call program_pkgc_latency from appropriate place [Jani]
-no need for the ~0 while setting max latency [Jani]
-change commit message to add the new changes made in.
--v4
-Remove extra blank line [Vinod]
-move the vrr.enable check to previous loop [Vinod]
Signed-off-by: Suraj Kandpal
Reviewed-by: Chaitanya Kumar Borah
---
drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++-
drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-
3 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index a6c7122fd671..d62e050185e7 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -325,7 +325,7 @@ static int intel_dsb_dewake_scanline(const struct
intel_crtc_state *crtc_state)
{
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
const struct drm_display_mode *adjusted_mode =
&crtc_state->hw.adjusted_mode;
- unsigned int latency = skl_watermark_max_latency(i915);
+ unsigned int latency = skl_watermark_max_latency(i915, 0);
int vblank_start;
if (crtc_state->vrr.enable) {
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index 614f319d754e..c6b9be80d83c 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -23,6 +23,12 @@
#include "skl_watermark.h"
#include "skl_watermark_regs.h"
+/*It is expected that DSB can do posted writes to every register in
+ * the pipe and planes within 100us. For flip queue use case, the
+ * recommended DSB execution time is 100us + one SAGV block time.
+ */
+#define DSB_EXE_TIME 100
+
static void skl_sagv_disable(struct drm_i915_private *i915);
/* Stores plane specific WM parameters */
@@ -2904,12 +2910,51 @@ static int skl_wm_add_affected_planes(struct
intel_atomic_state *state,
return 0;
}
+/*
+ * If Fixed Refresh Rate:
+ * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
+ * watermark level1 and up and above. If watermark level 1 is
+ * invalid program it with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = DSB execution time
+ * If Variable Refresh Rate:
+ * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = 0
+ */
+static void
+skl_program_dpkgc_latency(struct drm_i915_private *i915, bool vrr_enabled)
+{
+ u32 max_latency = 0;
+ u32 clear = 0, val = 0;
+ u32 added_wake_time = 0;
+
+ if (DISPLAY_VER(i915) < 20)
+ return;
+
+ if (vrr_enabled) {
+ max_latency = LNL_PKG_C_LATENCY_MASK;
+ added_wake_time = 0;
+ } else {
+ max_latency = skl_watermark_max_latency(i915, 1);
+ if (max_latency == 0)
+ max_latency = LNL_PKG_C_LATENCY_MASK;
+ added_wake_time = DSB_EXE_TIME +
+ i915->display.sagv.block_time_us;
+ }
+
+ clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
+ val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_latency);
+ val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time);
+
+ intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
+}
+
static int
skl_compute_wm(struct intel_atomic_state *state)
{
struct intel_crtc *crtc;
struct intel_crtc_state __maybe_unused *new_crtc_state;
int ret, i;
+ bool vrr_enabled = false;
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
ret = skl_build_pipe_wm(state, crtc);
@@ -2934,8 +2979,13 @@ skl_compute_wm(struct intel_atomic_state *state)
ret = skl_wm_add_affected_planes(state, crtc);
if (ret)
return ret;
+
+ if (new_crtc_state->vrr.enable)
+ vrr_enabled = true;
}
+ skl_program_dpkgc_latency(to_i915(state->base.dev), vrr_enabled);
+
skl_print_wm_changes(state);
return 0;
@@ -3731,11 +3781,11 @@ void skl_watermark_debugfs_register(struct
drm_i915_private *i915)
&intel_sagv_status_fops);
}
-uns
Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Hi Suraj,
+Ville - please comment if you see anything wrong with my explanation here
I think there could be a problem with the current logic and a possibility for
underrun. As per the
bspec we need to configure highest latency to soc.
As per this patch, skl_program_dpkgc_latency() is called from skl_compute_wm()
and
skl_program_dpkgc_latency() uses skl_watermark_max_latency() to get the maximum
memory latency. As
per the current implementation this skl_watermark_max_latency() always return
latency from the
highest level.
I think, that might not be the right approach. In skl_compute_wm() we calculate
the basic wm levels
using skl_build_pipe_wm(). Then based on the ddb availability these initial wm
levels are adjusted
again and some of the higher wm levels are pruned (skl_compute_ddb()). As far
as I understood from
the purpose of the bspec 68986, we need to configure the latency of
corresponding the available wm
level.
These wm levels are plane specific. So to find the best - highest wm levels
used, you have to
iterate through all the planes and find out the "least" of highest enabled wm
levels in these
planes. Then use the corresponding memory latency of that level to program this
register.
Otherwise you might end up programming a higher latency value than what is used
and can cause a FIFO
underrun I think.
In case of async flip cases, minimal wm0 is used and that makes wm level 1 to
above not enabled and
in that case you need to set all 1's to this register. This is what I meant
previously. This async
flip case will be covered automatically if the logic as per the above comments.
BR
Vinod
On Tue, 2024-02-13 at 11:58 +0530, Suraj Kandpal wrote:
> If fixed refresh rate program the PKGC_LATENCY register
> with the highest latency from level 1 and above LP registers
> and program ADDED_WAKE_TIME = DSB execution time.
> else program PKGC_LATENCY with all 1's and ADDED_WAKE_TIME as 0.
> This is used to improve package C residency by sending the highest
> latency tolerance requirement (LTR) when the planes are done with the
> frame until the next frame programming window (set context latency,
> window 2) starts.
> Bspec: 68986
>
> --v2
> -Fix indentation [Chaitanya]
>
> --v3
> -Take into account if fixed refrersh rate or not [Vinod]
> -Added wake time dependengt on DSB execution time [Vinod]
> -Use REG_FIELD_PREP [Jani]
> -Call program_pkgc_latency from appropriate place [Jani]
> -no need for the ~0 while setting max latency [Jani]
> -change commit message to add the new changes made in.
>
> Signed-off-by: Suraj Kandpal
> Reviewed-by: Chaitanya Kumar Borah
> ---
> drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
> drivers/gpu/drm/i915/display/skl_watermark.c | 59 +++-
> drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-
> 3 files changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index a6c7122fd671..d62e050185e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -325,7 +325,7 @@ static int intel_dsb_dewake_scanline(const struct
> intel_crtc_state
> *crtc_state)
> {
> struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> const struct drm_display_mode *adjusted_mode =
> &crtc_state->hw.adjusted_mode;
> - unsigned int latency = skl_watermark_max_latency(i915);
> + unsigned int latency = skl_watermark_max_latency(i915, 0);
> int vblank_start;
>
> if (crtc_state->vrr.enable) {
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 614f319d754e..1d423ce0f42d 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -23,6 +23,12 @@
> #include "skl_watermark.h"
> #include "skl_watermark_regs.h"
>
> +/*It is expected that DSB can do posted writes to every register in
> + * the pipe and planes within 100us. For flip queue use case, the
> + * recommended DSB execution time is 100us + one SAGV block time.
> + */
> +#define DSB_EXE_TIME 100
> +
> static void skl_sagv_disable(struct drm_i915_private *i915);
>
> /* Stores plane specific WM parameters */
> @@ -2904,12 +2910,51 @@ static int skl_wm_add_affected_planes(struct
> intel_atomic_state *state,
> return 0;
> }
>
> +/*
> + * If Fixed Refresh Rate:
> + * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = DSB execution time
> + * If Variable Refresh Rate:
> + * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0
> + */
> +static void
> +skl_program_dpkgc_latency(struct drm_i915_private *i915, bool vrr_enabled)
> +{
> + u32 max_latency = 0;
> +
[PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
If fixed refresh rate program the PKGC_LATENCY register
with the highest latency from level 1 and above LP registers
and program ADDED_WAKE_TIME = DSB execution time.
else program PKGC_LATENCY with all 1's and ADDED_WAKE_TIME as 0.
This is used to improve package C residency by sending the highest
latency tolerance requirement (LTR) when the planes are done with the
frame until the next frame programming window (set context latency,
window 2) starts.
Bspec: 68986
--v2
-Fix indentation [Chaitanya]
--v3
-Take into account if fixed refrersh rate or not [Vinod]
-Added wake time dependengt on DSB execution time [Vinod]
-Use REG_FIELD_PREP [Jani]
-Call program_pkgc_latency from appropriate place [Jani]
-no need for the ~0 while setting max latency [Jani]
-change commit message to add the new changes made in.
Signed-off-by: Suraj Kandpal
Reviewed-by: Chaitanya Kumar Borah
---
drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
drivers/gpu/drm/i915/display/skl_watermark.c | 59 +++-
drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-
3 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index a6c7122fd671..d62e050185e7 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -325,7 +325,7 @@ static int intel_dsb_dewake_scanline(const struct
intel_crtc_state *crtc_state)
{
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
const struct drm_display_mode *adjusted_mode =
&crtc_state->hw.adjusted_mode;
- unsigned int latency = skl_watermark_max_latency(i915);
+ unsigned int latency = skl_watermark_max_latency(i915, 0);
int vblank_start;
if (crtc_state->vrr.enable) {
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index 614f319d754e..1d423ce0f42d 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -23,6 +23,12 @@
#include "skl_watermark.h"
#include "skl_watermark_regs.h"
+/*It is expected that DSB can do posted writes to every register in
+ * the pipe and planes within 100us. For flip queue use case, the
+ * recommended DSB execution time is 100us + one SAGV block time.
+ */
+#define DSB_EXE_TIME 100
+
static void skl_sagv_disable(struct drm_i915_private *i915);
/* Stores plane specific WM parameters */
@@ -2904,12 +2910,51 @@ static int skl_wm_add_affected_planes(struct
intel_atomic_state *state,
return 0;
}
+/*
+ * If Fixed Refresh Rate:
+ * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
+ * watermark level1 and up and above. If watermark level 1 is
+ * invalid program it with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = DSB execution time
+ * If Variable Refresh Rate:
+ * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = 0
+ */
+static void
+skl_program_dpkgc_latency(struct drm_i915_private *i915, bool vrr_enabled)
+{
+ u32 max_latency = 0;
+ u32 clear = 0, val = 0;
+ u32 added_wake_time = 0;
+
+ if (DISPLAY_VER(i915) < 20)
+ return;
+
+ if (vrr_enabled) {
+ max_latency = LNL_PKG_C_LATENCY_MASK;
+ added_wake_time = 0;
+ } else {
+ max_latency = skl_watermark_max_latency(i915, 1);
+ if (max_latency == 0)
+ max_latency = LNL_PKG_C_LATENCY_MASK;
+ added_wake_time = DSB_EXE_TIME +
+ i915->display.sagv.block_time_us;
+ }
+
+ clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
+ val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_latency);
+ val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time);
+
+ intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
+}
+
static int
skl_compute_wm(struct intel_atomic_state *state)
{
struct intel_crtc *crtc;
struct intel_crtc_state __maybe_unused *new_crtc_state;
int ret, i;
+ bool vrr_enabled = false;
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
ret = skl_build_pipe_wm(state, crtc);
@@ -2936,6 +2981,15 @@ skl_compute_wm(struct intel_atomic_state *state)
return ret;
}
+ for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+ if (new_crtc_state->vrr.enable) {
+ vrr_enabled = true;
+ break;
+ }
+ }
+
+ skl_program_dpkgc_latency(to_i915(state->base.dev), vrr_enabled);
+
skl_print_wm_changes(state);
return 0;
@@ -3415,6 +3469,7 @@ static void skl_setup_wm_latency(struct drm_i915_private
*i915)
skl_read_wm_latency(i915, i915->display.wm.skl_latency);
intel_print_wm_latency(i915, "Gen9 P
RE: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
On Mon, 05 Feb 2024, "Kandpal, Suraj" wrote:
>> On Mon, 05 Feb 2024, Suraj Kandpal wrote:
>> > + if (wm_latency[i] == 0)
>> > + break;
>> > + else if (wm_latency[i] > max_value)
>> > + max_value = wm_latency[i];
>> > + }
>> > +
>> > + if (max_value == 0)
>> > + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
>>
>> What does "~0 &" gain you here?
>>
>
> So max value is 0 for all bits except 0-12 as we need to set them as all 1's
> to disable deep pkgc State
How is ~0 & LNL_PKG_C_LATENCY_MASK different from
LNL_PKG_C_LATENCY_MASK?
>> > +
>> > + clear |= LNL_ADDED_WAKE_TIME_MASK |
>> LNL_PKG_C_LATENCY_MASK;
>> > + val |= max_value;
>>
>> If you have fields defined for the register, why not use it for setting max
>> value
>> too?
>
> Sorry I didn't get you here .
val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_value);
>
>>
>> > + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); }
>> > +
>> > static void skl_setup_wm_latency(struct drm_i915_private *i915) {
>> >if (HAS_HW_SAGV_WM(i915))
>> > @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
>> drm_i915_private *i915)
>> >skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>> >
>> >intel_print_wm_latency(i915, "Gen9 Plane",
>> > i915->display.wm.skl_latency);
>> > +
>> > + if (DISPLAY_VER(i915) >= 20)
>> > + intel_program_pkgc_latency(i915, i915-
>> >display.wm.skl_latency);
>>
>> Before this, nothing in the skl_wm_init() path actually writes any
>> registers, it's
>> all readout. Is this the right place to be doing this?
>>
>
> Yes since all latency values are all ready and available for use which
> we can program in the deep pkgc register.
Is that a good reason to change a function that only reads hardware to
something writes the hardware?
BR,
Jani.
>
> Regards,
> Suraj Kandpal
>> > }
>> >
>> > static const struct intel_wm_funcs skl_wm_funcs = {
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Hi Suraj,
On Mon, 2024-02-05 at 13:31 +0530, Suraj Kandpal wrote:
> Program the PKGC_LATENCY register with the highest latency from
> level 1 and above LP registers else program with all 1's.
> This is used to improve package C residency by sending the highest
> latency tolerance requirement (LTR) when the planes are done with the
> frame until the next frame programming window (set context latency,
> window 2) starts.
> Bspec: 68986
>
> --v2
> -Fix indentation [Chaitanya]
>
> Signed-off-by: Suraj Kandpal
> Reviewed-by: Chaitanya Kumar Borah
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 31
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 051a02ac01a4..1ce4b33a407a 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> adjust_wm_latency(i915, wm, num_levels, read_latency);
> }
>
> +/*
> + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0.
> + */
Could you please confirm if the added wake time = 0 always? The Bspec says the
otherway!
> +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> + u16 wm_latency[])
> +{
> + u16 max_value = 0;
> + u32 clear = 0, val = 0;
> + int max_level = i915->display.wm.num_levels, i;
> +
> + for (i = 1; i <= max_level; i++) {
> + if (wm_latency[i] == 0)
> + break;
> + else if (wm_latency[i] > max_value)
> + max_value = wm_latency[i];
> + }
May be efficient to iterate max to 1 (skl_max_wm_level_for_vblank())
Also better to call skl_wm_lateny() instead of accessing the lantency values
directly as there are
some latency adjustments been done per platforms.
> +
> + if (max_value == 0)
> + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
> +
> + clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
As mentioned above the waketime is cleared here. Please double check if it is
as expected.
> + val |= max_value;
> + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
> +}
> +
> static void skl_setup_wm_latency(struct drm_i915_private *i915)
> {
> if (HAS_HW_SAGV_WM(i915))
> @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>
> intel_print_wm_latency(i915, "Gen9 Plane",
> i915->display.wm.skl_latency);
> +
> + if (DISPLAY_VER(i915) >= 20)
> + intel_program_pkgc_latency(i915,
> i915->display.wm.skl_latency);
skl_setup_wm_latency() gets called only at the init time. Though latency values
dont change, but as
per bspec you need to disable this for VRR. I guess you would need to have
provision to update this
based on that. So should this check be moved to intel_atomic_check()?
BR
Vinod
> }
>
> static const struct intel_wm_funcs skl_wm_funcs = {
RE: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
> Subject: Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
>
> On Mon, 05 Feb 2024, Suraj Kandpal wrote:
> > Program the PKGC_LATENCY register with the highest latency from level
> > 1 and above LP registers else program with all 1's.
> > This is used to improve package C residency by sending the highest
> > latency tolerance requirement (LTR) when the planes are done with the
> > frame until the next frame programming window (set context latency,
> > window 2) starts.
> > Bspec: 68986
> >
> > --v2
> > -Fix indentation [Chaitanya]
> >
> > Signed-off-by: Suraj Kandpal
> > Reviewed-by: Chaitanya Kumar Borah
> > ---
> > drivers/gpu/drm/i915/display/skl_watermark.c | 31
> >
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 051a02ac01a4..1ce4b33a407a 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> > adjust_wm_latency(i915, wm, num_levels, read_latency); }
> >
> > +/*
> > + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> > + * watermark level1 and up and above. If watermark level 1 is
> > + * invalid program it with all 1's.
> > + * Program PKG_C_LATENCY Added Wake Time = 0.
> > + */
> > +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> > + u16 wm_latency[])
> > +{
> > + u16 max_value = 0;
> > + u32 clear = 0, val = 0;
> > + int max_level = i915->display.wm.num_levels, i;
>
> max_level seems useless, only used once.
Sure will fix this.
>
> > +
> > + for (i = 1; i <= max_level; i++) {
>
> Array access goes out of bounds. Boom.
>
Will fix this
> > + if (wm_latency[i] == 0)
> > + break;
> > + else if (wm_latency[i] > max_value)
> > + max_value = wm_latency[i];
> > + }
> > +
> > + if (max_value == 0)
> > + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
>
> What does "~0 &" gain you here?
>
So max value is 0 for all bits except 0-12 as we need to set them as all 1's to
disable deep pkgc State
> > +
> > + clear |= LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
> > + val |= max_value;
>
> If you have fields defined for the register, why not use it for setting max
> value
> too?
Sorry I didn't get you here .
>
> > + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); }
> > +
> > static void skl_setup_wm_latency(struct drm_i915_private *i915) {
> > if (HAS_HW_SAGV_WM(i915))
> > @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> > skl_read_wm_latency(i915, i915->display.wm.skl_latency);
> >
> > intel_print_wm_latency(i915, "Gen9 Plane",
> > i915->display.wm.skl_latency);
> > +
> > + if (DISPLAY_VER(i915) >= 20)
> > + intel_program_pkgc_latency(i915, i915-
> >display.wm.skl_latency);
>
> Before this, nothing in the skl_wm_init() path actually writes any registers,
> it's
> all readout. Is this the right place to be doing this?
>
Yes since all latency values are all ready and available for use which we can
program in the deep pkgc register.
Regards,
Suraj Kandpal
> > }
> >
> > static const struct intel_wm_funcs skl_wm_funcs = {
>
> --
> Jani Nikula, Intel
Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
On Mon, 05 Feb 2024, Suraj Kandpal wrote:
> Program the PKGC_LATENCY register with the highest latency from
> level 1 and above LP registers else program with all 1's.
> This is used to improve package C residency by sending the highest
> latency tolerance requirement (LTR) when the planes are done with the
> frame until the next frame programming window (set context latency,
> window 2) starts.
> Bspec: 68986
>
> --v2
> -Fix indentation [Chaitanya]
>
> Signed-off-by: Suraj Kandpal
> Reviewed-by: Chaitanya Kumar Borah
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 31
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 051a02ac01a4..1ce4b33a407a 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> adjust_wm_latency(i915, wm, num_levels, read_latency);
> }
>
> +/*
> + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0.
> + */
> +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> +u16 wm_latency[])
> +{
> + u16 max_value = 0;
> + u32 clear = 0, val = 0;
> + int max_level = i915->display.wm.num_levels, i;
max_level seems useless, only used once.
> +
> + for (i = 1; i <= max_level; i++) {
Array access goes out of bounds. Boom.
> + if (wm_latency[i] == 0)
> + break;
> + else if (wm_latency[i] > max_value)
> + max_value = wm_latency[i];
> + }
> +
> + if (max_value == 0)
> + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
What does "~0 &" gain you here?
> +
> + clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
> + val |= max_value;
If you have fields defined for the register, why not use it for setting
max value too?
> + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
> +}
> +
> static void skl_setup_wm_latency(struct drm_i915_private *i915)
> {
> if (HAS_HW_SAGV_WM(i915))
> @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>
> intel_print_wm_latency(i915, "Gen9 Plane",
> i915->display.wm.skl_latency);
> +
> + if (DISPLAY_VER(i915) >= 20)
> + intel_program_pkgc_latency(i915, i915->display.wm.skl_latency);
Before this, nothing in the skl_wm_init() path actually writes any
registers, it's all readout. Is this the right place to be doing this?
> }
>
> static const struct intel_wm_funcs skl_wm_funcs = {
--
Jani Nikula, Intel
[PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Program the PKGC_LATENCY register with the highest latency from
level 1 and above LP registers else program with all 1's.
This is used to improve package C residency by sending the highest
latency tolerance requirement (LTR) when the planes are done with the
frame until the next frame programming window (set context latency,
window 2) starts.
Bspec: 68986
--v2
-Fix indentation [Chaitanya]
Signed-off-by: Suraj Kandpal
Reviewed-by: Chaitanya Kumar Borah
---
drivers/gpu/drm/i915/display/skl_watermark.c | 31
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index 051a02ac01a4..1ce4b33a407a 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct drm_i915_private
*i915, u16 wm[])
adjust_wm_latency(i915, wm, num_levels, read_latency);
}
+/*
+ * Program PKG_C_LATENCY Pkg C with highest valid latency from
+ * watermark level1 and up and above. If watermark level 1 is
+ * invalid program it with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = 0.
+ */
+static void intel_program_pkgc_latency(struct drm_i915_private *i915,
+ u16 wm_latency[])
+{
+ u16 max_value = 0;
+ u32 clear = 0, val = 0;
+ int max_level = i915->display.wm.num_levels, i;
+
+ for (i = 1; i <= max_level; i++) {
+ if (wm_latency[i] == 0)
+ break;
+ else if (wm_latency[i] > max_value)
+ max_value = wm_latency[i];
+ }
+
+ if (max_value == 0)
+ max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
+
+ clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
+ val |= max_value;
+ intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
+}
+
static void skl_setup_wm_latency(struct drm_i915_private *i915)
{
if (HAS_HW_SAGV_WM(i915))
@@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct drm_i915_private
*i915)
skl_read_wm_latency(i915, i915->display.wm.skl_latency);
intel_print_wm_latency(i915, "Gen9 Plane",
i915->display.wm.skl_latency);
+
+ if (DISPLAY_VER(i915) >= 20)
+ intel_program_pkgc_latency(i915, i915->display.wm.skl_latency);
}
static const struct intel_wm_funcs skl_wm_funcs = {
--
2.25.1
RE: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
> -Original Message-
> From: Kandpal, Suraj
> Sent: Thursday, February 1, 2024 2:22 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Borah, Chaitanya Kumar ; Kandpal,
> Suraj
> Subject: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
>
> Program the PKGC_LATENCY register with the highest latency from level 1 and
> above LP registers else program with all 1's.
> This is used to improve package C residency by sending the highest latency
> tolerance requirement (LTR) when the planes are done with the frame until
> the next frame programming window (set context latency, window 2) starts.
> Bspec: 68986
>
> Signed-off-by: Suraj Kandpal
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 31
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 051a02ac01a4..c07376f37baa 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> adjust_wm_latency(i915, wm, num_levels, read_latency); }
>
> +/*
> + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0.
> + */
> +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> +u16 wm_latency[])
> +{
> + u16 max_value = 0;
> + u32 clear = 0, val = 0;
> + int max_level = i915->display.wm.num_levels, i;
> +
> + for (i = 1; i <= max_level; i++) {
> + if (wm_latency[i] == 0)
> + break;
> + else if (wm_latency[i] > max_value)
Fix the indentation here.
With the understanding that this is a bare bones implementation and pending
work will be floated in a different series.
LGTM
Reviewed-by: Chaitanya Kumar Borah
> + max_value = wm_latency[i];
> + }
> +
> + if (max_value == 0)
> + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
> +
> + clear |= LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
> + val |= max_value;
> + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); }
> +
> static void skl_setup_wm_latency(struct drm_i915_private *i915) {
> if (HAS_HW_SAGV_WM(i915))
> @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>
> intel_print_wm_latency(i915, "Gen9 Plane", i915-
> >display.wm.skl_latency);
> +
> + if (DISPLAY_VER(i915) >= 20)
> + intel_program_pkgc_latency(i915, i915-
> >display.wm.skl_latency);
> }
>
> static const struct intel_wm_funcs skl_wm_funcs = {
> --
> 2.25.1
[PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Program the PKGC_LATENCY register with the highest latency from
level 1 and above LP registers else program with all 1's.
This is used to improve package C residency by sending the highest
latency tolerance requirement (LTR) when the planes are done with the
frame until the next frame programming window (set context latency,
window 2) starts.
Bspec: 68986
Signed-off-by: Suraj Kandpal
---
drivers/gpu/drm/i915/display/skl_watermark.c | 31
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
b/drivers/gpu/drm/i915/display/skl_watermark.c
index 051a02ac01a4..c07376f37baa 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct drm_i915_private
*i915, u16 wm[])
adjust_wm_latency(i915, wm, num_levels, read_latency);
}
+/*
+ * Program PKG_C_LATENCY Pkg C with highest valid latency from
+ * watermark level1 and up and above. If watermark level 1 is
+ * invalid program it with all 1's.
+ * Program PKG_C_LATENCY Added Wake Time = 0.
+ */
+static void intel_program_pkgc_latency(struct drm_i915_private *i915,
+ u16 wm_latency[])
+{
+ u16 max_value = 0;
+ u32 clear = 0, val = 0;
+ int max_level = i915->display.wm.num_levels, i;
+
+ for (i = 1; i <= max_level; i++) {
+ if (wm_latency[i] == 0)
+ break;
+ else if (wm_latency[i] > max_value)
+ max_value = wm_latency[i];
+ }
+
+ if (max_value == 0)
+ max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
+
+ clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
+ val |= max_value;
+ intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
+}
+
static void skl_setup_wm_latency(struct drm_i915_private *i915)
{
if (HAS_HW_SAGV_WM(i915))
@@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct drm_i915_private
*i915)
skl_read_wm_latency(i915, i915->display.wm.skl_latency);
intel_print_wm_latency(i915, "Gen9 Plane",
i915->display.wm.skl_latency);
+
+ if (DISPLAY_VER(i915) >= 20)
+ intel_program_pkgc_latency(i915, i915->display.wm.skl_latency);
}
static const struct intel_wm_funcs skl_wm_funcs = {
--
2.25.1
