[PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Animesh Manna]

The PKG_C_LATENCY Added Wake Time field is not working.
When added wake time is needed, such as for flip queue
DSB execution, increase the PKG_C_LATENCY Pkg C Latency
field by the added wake time.

WA: 22020432604

v1: Initial version.
v2: Rebase and cosmetic changes.

Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index d93f6786db0e..f6f7205e06eb 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct intel_atomic_state *state)
 			display->sagv.block_time_us;
 	}
 
+	/* Wa_22020432604 */
+	if (DISPLAY_VER(i915) == 30) {
+		latency += added_wake_time;
+		added_wake_time = 0;
+	}
+
 	clear = LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
 	val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
 		REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time);
-- 
2.29.0

Re: [PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Gustavo Sousa]

Quoting Animesh Manna (2024-12-09 04:47:02-03:00)
>The PKG_C_LATENCY Added Wake Time field is not working.
>When added wake time is needed, such as for flip queue
>DSB execution, increase the PKG_C_LATENCY Pkg C Latency
>field by the added wake time.
>
>WA: 22020432604
>
>v1: Initial version.
>v2: Rebase and cosmetic changes.
>
>Cc: Suraj Kandpal <suraj.kandpal@intel.com>
>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>---
> drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
>index d93f6786db0e..f6f7205e06eb 100644
>--- a/drivers/gpu/drm/i915/display/skl_watermark.c
>+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
>@@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct intel_atomic_state *state)
>                         display->sagv.block_time_us;
>         }
> 
>+        /* Wa_22020432604 */
>+        if (DISPLAY_VER(i915) == 30) {

This WA also applies to Xe2_LPD (i.e. display version 20).

--
Gustavo Sousa

>+                latency += added_wake_time;
>+                added_wake_time = 0;
>+        }
>+
>         clear = LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
>         val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
>                 REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time);
>-- 
>2.29.0
>

RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Kandpal, Suraj]

> -----Original Message-----
> From: Manna, Animesh <animesh.manna@intel.com>
> Sent: Monday, December 9, 2024 1:17 PM
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: Manna, Animesh <animesh.manna@intel.com>; Kandpal, Suraj
> <suraj.kandpal@intel.com>
> Subject: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> PKG_C_LATENCY
> 
> The PKG_C_LATENCY Added Wake Time field is not working.
> When added wake time is needed, such as for flip queue DSB execution,
> increase the PKG_C_LATENCY Pkg C Latency field by the added wake time.

No need to mention the issue when It comes to WA only what the patch is doing the
Rest of the info is present in the WA

> 
> WA: 22020432604

This needs to come just above the CC with no new line in between CC and WA no.

> 
> v1: Initial version.
> v2: Rebase and cosmetic changes.
> 
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index d93f6786db0e..f6f7205e06eb 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct
> intel_atomic_state *state)
>  			display->sagv.block_time_us;
>  	}
> 
> +	/* Wa_22020432604 */
> +	if (DISPLAY_VER(i915) == 30) {
> +		latency += added_wake_time;

This wouldn't be the correct place to place it in since this would change the value in case the latency fetched is 0 
From skl_watermark_max_latency and we actually want to write all 1's and want to disable the deep pkgc
The best place would be right after fetching  max_latency  so it plays nice with the other WA and makes sure that pkgc latency 
Is a multiple of max line time when latency is not 0
So something like

If (display_ver && !latency)
	latency += added_wake_time;

this may also require you to move around where added_wake_time is assigned so that's a different patch
	
	
> +		added_wake_time = 0;

Also lets not re assign 0 to added wake time variable let it just be written its not going to be used anyways and wil
Not have any extra writes from our side

Regards,
Suraj Kandpal
 
> +	}


> +
>  	clear = LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
>  	val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
>  		REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> added_wake_time);
> --
> 2.29.0

✓ CI.Patch_applied: success for drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Patchwork]

== Series Details ==

Series: drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY
URL   : https://patchwork.freedesktop.org/series/142276/
State : success

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: c266f2fbb9c0 drm-tip: 2024y-12m-09d-14h-13m-53s UTC integration manifest
=== git am output follows ===
Applying: drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

✓ CI.checkpatch: success for drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Patchwork]

== Series Details ==

Series: drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY
URL   : https://patchwork.freedesktop.org/series/142276/
State : success

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
30ab6715fc09baee6cc14cb3c89ad8858688d474
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit e3168ee2948bdab4b9d067604013ff1248f074cd
Author: Animesh Manna <animesh.manna@intel.com>
Date:   Mon Dec 9 13:17:02 2024 +0530

    drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY
    
    The PKG_C_LATENCY Added Wake Time field is not working.
    When added wake time is needed, such as for flip queue
    DSB execution, increase the PKG_C_LATENCY Pkg C Latency
    field by the added wake time.
    
    WA: 22020432604
    
    v1: Initial version.
    v2: Rebase and cosmetic changes.
    
    Cc: Suraj Kandpal <suraj.kandpal@intel.com>
    Signed-off-by: Animesh Manna <animesh.manna@intel.com>
+ /mt/dim checkpatch c266f2fbb9c0250af17b14788364766adf802a4a drm-intel
e3168ee2948b drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

✗ CI.KUnit: failure for drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Patchwork]

== Series Details ==

Series: drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY
URL   : https://patchwork.freedesktop.org/series/142276/
State : failure

== Summary ==

+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
In file included from ../include/linux/module.h:22,
                 from ../include/linux/device/driver.h:21,
                 from ../include/linux/device.h:32,
                 from ../include/linux/auxiliary_bus.h:11,
                 from ../include/linux/intel_vsec.h:5,
                 from ../drivers/gpu/drm/xe/xe_vsec.c:7:
../drivers/gpu/drm/xe/xe_vsec.c:233:18: error: expected ‘,’ or ‘;’ before ‘INTEL_VSEC’
  233 | MODULE_IMPORT_NS(INTEL_VSEC);
      |                  ^~~~~~~~~~
../include/linux/moduleparam.h:26:61: note: in definition of macro ‘__MODULE_INFO’
   26 |                 = __MODULE_INFO_PREFIX __stringify(tag) "=" info
      |                                                             ^~~~
../include/linux/module.h:299:33: note: in expansion of macro ‘MODULE_INFO’
  299 | #define MODULE_IMPORT_NS(ns)    MODULE_INFO(import_ns, ns)
      |                                 ^~~~~~~~~~~
../drivers/gpu/drm/xe/xe_vsec.c:233:1: note: in expansion of macro ‘MODULE_IMPORT_NS’
  233 | MODULE_IMPORT_NS(INTEL_VSEC);
      | ^~~~~~~~~~~~~~~~
make[7]: *** [../scripts/Makefile.build:194: drivers/gpu/drm/xe/xe_vsec.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:440: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:440: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:440: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:440: drivers] Error 2
make[2]: *** [/kernel/Makefile:1989: .] Error 2
make[1]: *** [/kernel/Makefile:251: __sub-make] Error 2
make: *** [Makefile:251: __sub-make] Error 2

[14:17:13] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[14:17:17] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel

RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Manna, Animesh]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Monday, December 9, 2024 7:19 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Subject: RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> PKG_C_LATENCY
> 
> 
> 
> > -----Original Message-----
> > From: Manna, Animesh <animesh.manna@intel.com>
> > Sent: Monday, December 9, 2024 1:17 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: Manna, Animesh <animesh.manna@intel.com>; Kandpal, Suraj
> > <suraj.kandpal@intel.com>
> > Subject: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> > PKG_C_LATENCY
> >
> > The PKG_C_LATENCY Added Wake Time field is not working.
> > When added wake time is needed, such as for flip queue DSB execution,
> > increase the PKG_C_LATENCY Pkg C Latency field by the added wake time.
> 
> No need to mention the issue when It comes to WA only what the patch is
> doing the Rest of the info is present in the WA

Thanks for review.
Is it generalized rule? Not sure if want to add the background/workaround details what is the issue.

> 
> >
> > WA: 22020432604
> 
> This needs to come just above the CC with no new line in between CC and
> WA no.

Is it generalized rule? Good to know if captured somewhere, I see from git log many are not following the above.
 
> 
> >
> > v1: Initial version.
> > v2: Rebase and cosmetic changes.
> >
> > Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index d93f6786db0e..f6f7205e06eb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct
> > intel_atomic_state *state)
> >  			display->sagv.block_time_us;
> >  	}
> >
> > +	/* Wa_22020432604 */
> > +	if (DISPLAY_VER(i915) == 30) {
> > +		latency += added_wake_time;
> 
> This wouldn't be the correct place to place it in since this would change the
> value in case the latency fetched is 0 From skl_watermark_max_latency and
> we actually want to write all 1's and want to disable the deep pkgc The best
> place would be right after fetching  max_latency  so it plays nice with the
> other WA and makes sure that pkgc latency Is a multiple of max line time
> when latency is not 0 So something like
> 
> If (display_ver && !latency)
> 	latency += added_wake_time;

Got your point, Will take care in next version.

> 
> this may also require you to move around where added_wake_time is
> assigned so that's a different patch
> 
> 
> > +		added_wake_time = 0;
> 
> Also lets not re assign 0 to added wake time variable let it just be written its
> not going to be used anyways and wil Not have any extra writes from our
> side

If added_wake_time is adjusted to pkgc latency then writing the same in register is not logically correct atleast from code readability POV, so better to reset to zero.

Regards,
Animesh

> 
> Regards,
> Suraj Kandpal
> 
> > +	}
> 
> 
> > +
> >  	clear = LNL_ADDED_WAKE_TIME_MASK |
> > LNL_PKG_C_LATENCY_MASK;
> >  	val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
> >  		REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > added_wake_time);
> > --
> > 2.29.0

RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY

[Kandpal, Suraj]

> -----Original Message-----
> From: Manna, Animesh <animesh.manna@intel.com>
> Sent: Wednesday, December 18, 2024 4:04 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Subject: RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> PKG_C_LATENCY
> 
> 
> 
> > -----Original Message-----
> > From: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Sent: Monday, December 9, 2024 7:19 PM
> > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Subject: RE: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> > PKG_C_LATENCY
> >
> >
> >
> > > -----Original Message-----
> > > From: Manna, Animesh <animesh.manna@intel.com>
> > > Sent: Monday, December 9, 2024 1:17 PM
> > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kandpal, Suraj
> > > <suraj.kandpal@intel.com>
> > > Subject: [PATCH v2] drm/i915/display: Adjust Added Wake Time with
> > > PKG_C_LATENCY
> > >
> > > The PKG_C_LATENCY Added Wake Time field is not working.
> > > When added wake time is needed, such as for flip queue DSB
> > > execution, increase the PKG_C_LATENCY Pkg C Latency field by the added
> wake time.
> >
> > No need to mention the issue when It comes to WA only what the patch
> > is doing the Rest of the info is present in the WA
> 
> Thanks for review.
> Is it generalized rule? Not sure if want to add the background/workaround
> details what is the issue.

I think this was a generalized rule as Matt Roper had pointed out we really don't want to tell other what faults ended up causing us to have this WA

> 
> >
> > >
> > > WA: 22020432604
> >
> > This needs to come just above the CC with no new line in between CC
> > and WA no.
> 
> Is it generalized rule? Good to know if captured somewhere, I see from git log
> many are not following the above.
> 
> >
> > >
> > > v1: Initial version.
> > > v2: Rebase and cosmetic changes.
> > >
> > > Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/skl_watermark.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index d93f6786db0e..f6f7205e06eb 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -2894,6 +2894,12 @@ intel_program_dpkgc_latency(struct
> > > intel_atomic_state *state)
> > >  			display->sagv.block_time_us;
> > >  	}
> > >
> > > +	/* Wa_22020432604 */
> > > +	if (DISPLAY_VER(i915) == 30) {
> > > +		latency += added_wake_time;
> >
> > This wouldn't be the correct place to place it in since this would
> > change the value in case the latency fetched is 0 From
> > skl_watermark_max_latency and we actually want to write all 1's and
> > want to disable the deep pkgc The best place would be right after
> > fetching  max_latency  so it plays nice with the other WA and makes
> > sure that pkgc latency Is a multiple of max line time when latency is
> > not 0 So something like
> >
> > If (display_ver && !latency)
> > 	latency += added_wake_time;
> 
> Got your point, Will take care in next version.
> 
> >
> > this may also require you to move around where added_wake_time is
> > assigned so that's a different patch
> >
> >
> > > +		added_wake_time = 0;
> >
> > Also lets not re assign 0 to added wake time variable let it just be
> > written its not going to be used anyways and wil Not have any extra
> > writes from our side
> 
> If added_wake_time is adjusted to pkgc latency then writing the same in
> register is not logically correct atleast from code readability POV, so better to
> reset to zero.
> 

Sure I have no problem with that

Regards,
Suraj Kandpal

> Regards,
> Animesh
> 
> >
> > Regards,
> > Suraj Kandpal
> >
> > > +	}
> >
> >
> > > +
> > >  	clear = LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
> > >  	val = REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, latency) |
> > >  		REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > > added_wake_time);
> > > --
> > > 2.29.0