Re: [PATCH 2/2] drm/xe/gsc: add support for GSC proxy interrupt

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Mon, 2023-12-11 at 17:05 -0800, Daniele Ceraolo Spurio wrote:
> The GSC notifies us of a proxy request via the HECI2 interrupt. The
> interrupt must be enabled both in the HECI layer and in our usual gt irq
> programming; for the latter, the interrupt is enabled via the same enable
> register as the GSC CS, but it does have its own mask register. When the
> interrupt is received, we also need to de-assert it in both layers.
> 
> The handling of the proxy request is deferred to the same worker that we
> use for GSC load. New flags have been added to distinguish between the
> init case and the proxy interrupt.

alan:snip
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 5f5a72e9d0d8..9f5f2150034a 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -450,6 +450,7 @@
>  #define   INTR_ENGINE_CLASS(x)			REG_FIELD_GET(GENMASK(18, 16), x)
>  #define   INTR_ENGINE_INTR(x)			REG_FIELD_GET(GENMASK(15, 0), x)
>  #define   OTHER_GUC_INSTANCE			0
> +#define   OTHER_HECI2_INSTANCE			3
alan: should we stick with OTHER_GSC_HECI2_INSTANCE to match the naming in our hw specs?
>  #define   OTHER_GSC_INSTANCE			6
>  
>  #define RENDER_COPY_INTR_ENABLE			XE_REG(0x190030)
> @@ -462,6 +463,7 @@
>  #define VCS0_VCS1_INTR_MASK			XE_REG(0x1900a8)
>  #define VCS2_VCS3_INTR_MASK			XE_REG(0x1900ac)
>  #define VECS0_VECS1_INTR_MASK			XE_REG(0x1900d0)
> +#define HECI2_RSVD_INTR_MASK			XE_REG(0x1900e4)
alan: similarly, i believe it more maintainable to follow hw spec: CRYPTO_RSVD_INTR_ENABLE

>  #define GUC_SG_INTR_MASK			XE_REG(0x1900e8)
>  #define GPM_WGBOXPERF_INTR_MASK			XE_REG(0x1900ec)
>  #define GUNIT_GSC_INTR_MASK			XE_REG(0x1900f4)
alan:snip

> diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/xe_gsc_proxy.c
>  int xe_gsc_proxy_start(struct xe_gsc *gsc)
>  {
>  	int err;
>  
> +	/* enable the proxy interrupt in the GSC shim layer */
> +	gsc_proxy_irq_toggle(gsc, true);
alan: nit: just a question -> does hw/fw require us to enable the gsc heci2 interrupts if
we are not yet processing event-based requests but rather doing just the proactive-startup-proxy
sequence? (i.e. maybe only enable IRQs after the proxy-request handler call below? but i
guess it doesnt matter either way)
> +
>  	/*
>  	 * The handling of the first proxy request must be manually triggered to
>  	 * notify the GSC that we're ready to support the proxy flow.
>  	 */
> -	err = gsc_proxy_request_handler(gsc);
> +	err = xe_gsc_proxy_request_handler(gsc);
alan:snip

> diff --git a/drivers/gpu/drm/xe/xe_gsc_types.h b/drivers/gpu/drm/xe/xe_gsc_types.h
> index 645166adae1f..310f25809798 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_types.h
> +++ b/drivers/gpu/drm/xe/xe_gsc_types.h
> @@ -39,6 +40,14 @@ struct xe_gsc {
>  	/** @work: delayed load and proxy handling work */
>  	struct work_struct work;
>  
> +	/** @lock: protects access to the work_actions mask */
> +	spinlock_t lock;
> +
> +	/** @work_actions: mask of actions to be performed in the work */
alan: nit: did you mean "in the worker"?
> +	u32 work_actions;
> +#define GSC_ACTION_FW_LOAD BIT(0)
> +#define GSC_ACTION_SW_PROXY BIT(1)
> +
>  	struct {
>  		/** @component: struct for communication with mei component */
>  		struct i915_gsc_proxy_component *component;
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index d1f5ba4bb745..de7cc6e611a2 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -15,6 +15,7 @@
>  #include "xe_display.h"
>  #include "xe_drv.h"
>  #include "xe_gt.h"
> +#include "xe_gsc_proxy.h"
alan: alphabetical order
>  #include "xe_guc.h"
>  #include "xe_hw_engine.h"
>  #include "xe_mmio.h"
> @@ -129,6 +130,7 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
> 
alan:snip