| Message ID | 20241209074702.3975702-1-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/display: Adjust Added Wake Time with PKG_C_LATENCY | expand |
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 >
> -----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
> -----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
> -----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
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);
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(+)