diff mbox series

[RFC] Revert "drm/i915: Disable compression tricks on JSL"

Message ID z6xndjwwwnck67qcv2355v5qejq64qldziqg7saint3eqe6fo2@6sx7xyh5juvc (mailing list archive)
State New
Headers show
Series [RFC] Revert "drm/i915: Disable compression tricks on JSL" | expand

Commit Message

Sebastian Brzezinka March 5, 2025, 2:49 p.m. UTC
This reverts commit 0ddae025ab6cefa9aba757da3cd1d27908d70b0e.

According to bspec 14181, CACHE_MODE_0 is a register that's under userspace
control, and DISABLE_REPACKING_FOR_COMPRESSION workaround should be already
in all recent Mesa releases. So, there is no need to include it in kernel.

Also, this workaround·sporadically fails to load:
```
ERROR GT0: engine workaround lost on application! (reg[7000]=0x0,
relevant bits were 0x0 vs expected 0x8000)
```

Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 1 -
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ---------
 2 files changed, 10 deletions(-)

Comments

Sebastian Brzezinka March 5, 2025, 3:03 p.m. UTC | #1
Hi

The bug appeared recently once, and it is possible that it will pop up
from time to times, so it might be better to get rid of this workaround
from the kernel, especially since it's already in Mesa. I would like to
know, what you think about it ?
Ville Syrjala March 5, 2025, 3:26 p.m. UTC | #2
On Wed, Mar 05, 2025 at 02:49:46PM +0000, Sebastian Brzezinka wrote:
> This reverts commit 0ddae025ab6cefa9aba757da3cd1d27908d70b0e.
> 
> According to bspec 14181, CACHE_MODE_0 is a register that's under userspace
> control, and DISABLE_REPACKING_FOR_COMPRESSION workaround should be already
> in all recent Mesa releases. So, there is no need to include it in kernel.

igt doesn't have it.

> 
> Also, this workaround·sporadically fails to load:
> ```
> ERROR GT0: engine workaround lost on application! (reg[7000]=0x0,
> relevant bits were 0x0 vs expected 0x8000)
> ```

If it somehow fails to load from the kernel why would it
work from userspace?

Hmm, apparently CACHE_MODE_0 needs the mcr steering stuff.
Does that fix the verification fail?

> 
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 1 -
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ---------
>  2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 7421ed18d8d1..52ddfa9f3ad3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -440,7 +440,6 @@
>  #define XEHPG_INSTDONE_GEOM_SVG			MCR_REG(0x666c)
>  
>  #define CACHE_MODE_0_GEN7			_MMIO(0x7000) /* IVB+ */
> -#define   DISABLE_REPACKING_FOR_COMPRESSION	REG_BIT(15) /* jsl+ */
>  #define   RC_OP_FLUSH_ENABLE			(1 << 0)
>  #define   HIZ_RAW_STALL_OPT_DISABLE		(1 << 2)
>  #define CACHE_MODE_1				_MMIO(0x7004) /* IVB+ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 116683ebe074..51839f270d57 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -2306,15 +2306,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>  			     GEN8_RC_SEMA_IDLE_MSG_DISABLE);
>  	}
>  
> -	if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) {
> -		/*
> -		 * "Disable Repacking for Compression (masked R/W access)
> -		 *  before rendering compressed surfaces for display."
> -		 */
> -		wa_masked_en(wal, CACHE_MODE_0_GEN7,
> -			     DISABLE_REPACKING_FOR_COMPRESSION);
> -	}
> -
>  	if (GRAPHICS_VER(i915) == 11) {
>  		/* This is not an Wa. Enable for better image quality */
>  		wa_masked_en(wal,
> -- 
> 2.34.1
Sebastian Brzezinka March 5, 2025, 5 p.m. UTC | #3
Hi Ville

On Wed Mar 5, 2025 at 3:26 PM UTC, Ville Syrjälä wrote:
> On Wed, Mar 05, 2025 at 02:49:46PM +0000, Sebastian Brzezinka wrote:
>> This reverts commit 0ddae025ab6cefa9aba757da3cd1d27908d70b0e.
>> 
>> According to bspec 14181, CACHE_MODE_0 is a register that's under userspace
>> control, and DISABLE_REPACKING_FOR_COMPRESSION workaround should be already
>> in all recent Mesa releases. So, there is no need to include it in kernel.
>
> igt doesn't have it.
>
>> 
>> Also, this workaround·sporadically fails to load:
>> ```
>> ERROR GT0: engine workaround lost on application! (reg[7000]=0x0,
>> relevant bits were 0x0 vs expected 0x8000)
>> ```
>
> If it somehow fails to load from the kernel why would it
> work from userspace?
>
> Hmm, apparently CACHE_MODE_0 needs the mcr steering stuff.
> Does that fix the verification fail?

Thanks for sugestion. Right now I think that I try to move this wa to
icl_ctx_workarounds_init as both Mat and Chriss notice that register
is a part of the context.
Ville Syrjala March 6, 2025, 12:52 p.m. UTC | #4
On Wed, Mar 05, 2025 at 05:00:26PM +0000, Sebastian Brzezinka wrote:
> Hi Ville
> 
> On Wed Mar 5, 2025 at 3:26 PM UTC, Ville Syrjälä wrote:
> > On Wed, Mar 05, 2025 at 02:49:46PM +0000, Sebastian Brzezinka wrote:
> >> This reverts commit 0ddae025ab6cefa9aba757da3cd1d27908d70b0e.
> >> 
> >> According to bspec 14181, CACHE_MODE_0 is a register that's under userspace
> >> control, and DISABLE_REPACKING_FOR_COMPRESSION workaround should be already
> >> in all recent Mesa releases. So, there is no need to include it in kernel.
> >
> > igt doesn't have it.
> >
> >> 
> >> Also, this workaround·sporadically fails to load:
> >> ```
> >> ERROR GT0: engine workaround lost on application! (reg[7000]=0x0,
> >> relevant bits were 0x0 vs expected 0x8000)
> >> ```
> >
> > If it somehow fails to load from the kernel why would it
> > work from userspace?
> >
> > Hmm, apparently CACHE_MODE_0 needs the mcr steering stuff.
> > Does that fix the verification fail?
> 
> Thanks for sugestion. Right now I think that I try to move this wa to
> icl_ctx_workarounds_init as both Mat and Chriss notice that register
> is a part of the context. 

Hmm, didn't realize there was a separate list for that. It looks to
me like there are a bunch of context saved registers handled in
the enging_ctx() stuff currently. I think someone needs to go through
all this stuff and relocate all the registers to their correct spots.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 7421ed18d8d1..52ddfa9f3ad3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -440,7 +440,6 @@ 
 #define XEHPG_INSTDONE_GEOM_SVG			MCR_REG(0x666c)
 
 #define CACHE_MODE_0_GEN7			_MMIO(0x7000) /* IVB+ */
-#define   DISABLE_REPACKING_FOR_COMPRESSION	REG_BIT(15) /* jsl+ */
 #define   RC_OP_FLUSH_ENABLE			(1 << 0)
 #define   HIZ_RAW_STALL_OPT_DISABLE		(1 << 2)
 #define CACHE_MODE_1				_MMIO(0x7004) /* IVB+ */
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 116683ebe074..51839f270d57 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -2306,15 +2306,6 @@  rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 			     GEN8_RC_SEMA_IDLE_MSG_DISABLE);
 	}
 
-	if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) {
-		/*
-		 * "Disable Repacking for Compression (masked R/W access)
-		 *  before rendering compressed surfaces for display."
-		 */
-		wa_masked_en(wal, CACHE_MODE_0_GEN7,
-			     DISABLE_REPACKING_FOR_COMPRESSION);
-	}
-
 	if (GRAPHICS_VER(i915) == 11) {
 		/* This is not an Wa. Enable for better image quality */
 		wa_masked_en(wal,