[Intel-xe] [PATCH] drm/xe/xe2: Add workaround 18032095049

[Matt Roper]

On Mon, Dec 04, 2023 at 07:57:38PM +0530, Tejas Upadhyay wrote:
> This workaround applies to graphics 20.04 A0 step and on
> render engine

There are three parts to this workaround:
 * Post-sync operations vs MI_ATOMIC --- This part isn't relevant to Xe
   (at least not right now) since we don't use MI_ATOMIC anywhere in the
   kernel mode driver.  It's probably still worth mentioning in the
   commit message here in case the situation changes in the future.

 * Memory-based interrupt masking.  I don't think this one is actually
   relevant either for a few reasons:
     - Memory-based interrupt processing isn't supported on physical
       functions, only virtual functions, according to bspec 60352.  So
       this is probably only relevant once SRIOV support lands in the
       driver.
     - Using RTP for this programming isn't going to work since this
       will apply the setting once at init and then whenever the engine
       gets reset.  However the RCS engine context (bspec 65182) contains
       a special MI_LOAD_REGISTER_MEM instruction which will re-loads a
       value for this register from memory whenever a context switch
       happens; that will overwrite whatever you try to program here.
     - In general, the only types of engine interrupts we enable/unmask
       on GuC-based platforms (i.e., all platforms) are USER_INTERRUPT
       and PIPECTL_NOTIFY_INTERRUPT, which are the two exceptions given
       in the workaround text.  See xe_irq_enable_hwe().

 * Disabling CSB/timestamp updates to the ghwsp and pphwsp.

So I think only the third bullet there is necessary, but we should still
mention the others in the commit message (and why we don't need to do
anything for them right now) in case the situation changes in the future
(e.g., as we implement additional features like SRIOV).

> 
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_engine_regs.h |  9 +++++++++
>  drivers/gpu/drm/xe/xe_wa.c               | 10 ++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> index 444ff9b83bb1..5f70eb62db70 100644
> --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> @@ -40,6 +40,10 @@
>  #define RING_NOPID(base)			XE_REG((base) + 0x94)
>  
>  #define RING_IMR(base)				XE_REG((base) + 0xa8)
> +#define   RENDER_INTR_VEC			REG_GENMASK(15, 0)
> +#define   RENDER_INTR_VEC_PIPE_CONTROL_NOTIFY	REG_BIT(4)
> +#define   RENDER_INTR_VEC_MI_USER_INTERRUPT	REG_BIT(0)

As noted above, I don't think we actually need to mess with the
interrupt vector, but if we did we already have some definitions we can
use; they just got copy/pasted to a strange place in the xe_reg.h
header:

  #define   GT_WAIT_SEMAPHORE_INTERRUPT           REG_BIT(11)                                                    
  #define   GT_CONTEXT_SWITCH_INTERRUPT           REG_BIT(8)                                                     
  #define   GT_RENDER_PIPECTL_NOTIFY_INTERRUPT    REG_BIT(4)                                                     
  #define   GT_CS_MASTER_ERROR_INTERRUPT          REG_BIT(3)                                                     
  #define   GT_RENDER_USER_INTERRUPT              REG_BIT(0)


> +
>  #define   RING_MAX_NONPRIV_SLOTS  12
>  
>  #define RING_EIR(base)				XE_REG((base) + 0xb0)
> @@ -47,6 +51,11 @@
>  #define RING_ESR(base)				XE_REG((base) + 0xb8)
>  
>  #define RING_CMD_CCTL(base)			XE_REG((base) + 0xc4, XE_REG_OPTION_MASKED)
> +
> +#define CSFE_CHICKEN1_REG(base)			XE_REG((base) + 0xd4, XE_REG_OPTION_MASKED)
> +#define   PPHWSP_CSB_AND_TIMESTAMP_REPORT_DIS	REG_BIT(14)
> +#define   GHWSP_CSB_REPORT_DISABLE		REG_BIT(15)

Nitpick:  We usually order register bits in descending order.  Also,
it's a bit strange to use "_DIS" for one and "_DISABLE" for the other.
I'd just make them both end with "_DIS" for consistency.


Matt

> +
>  /*
>   * CMD_CCTL read/write fields take a MOCS value and _not_ a table index.
>   * The lsb of each can be considered a separate enabling bit for encryption.
> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> index 63bd4bb1af03..c5c640dcb427 100644
> --- a/drivers/gpu/drm/xe/xe_wa.c
> +++ b/drivers/gpu/drm/xe/xe_wa.c
> @@ -596,6 +596,16 @@ static const struct xe_rtp_entry_sr engine_was[] = {
>  	  XE_RTP_RULES(GRAPHICS_VERSION(2004), FUNC(xe_rtp_match_first_render_or_compute)),
>  	  XE_RTP_ACTIONS(SET(ROW_CHICKEN, EARLY_EOT_DIS))
>  	},
> +	{ XE_RTP_NAME("18032095049"),
> +	  XE_RTP_RULES(GRAPHICS_VERSION(2004), GRAPHICS_STEP(A0, B0), ENGINE_CLASS(RENDER)),
> +	  XE_RTP_ACTIONS(FIELD_SET(RING_IMR(0),
> +				   RENDER_INTR_VEC,
> +				   RENDER_INTR_VEC_PIPE_CONTROL_NOTIFY |
> +				   RENDER_INTR_VEC_MI_USER_INTERRUPT,
> +				   XE_RTP_ACTION_FLAG(ENGINE_BASE)),
> +			 SET(CSFE_CHICKEN1_REG(0), PPHWSP_CSB_AND_TIMESTAMP_REPORT_DIS |
> +			     GHWSP_CSB_REPORT_DISABLE))
> +	},
>  
>  	{}
>  };
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation