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

[Daniele Ceraolo Spurio]

On 1/8/2024 11:32 AM, Teres Alexis, Alan Previn wrote:
> 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?

AFAICS the HW specs actually have "Security Engine HECI2"; I abbreviated 
it to just HECI2 since the mask register was like that as well (see 
below comment)

>>   #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

This is HECI2_RSVD_INTR_MASK in the MTL specs.

>
>>   #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)

Not right now, but there was talk a while back to move from a loop to a 
fully irq-driven flow (i.e. the GSC would generate a new interrupt for 
each message instead of chaining them as it does now). If we ever go 
that way, that would require us to have irq enabled before the first 
proxy is sent, which is why I went with that ordering here.

>> +
>>   	/*
>>   	 * 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"?

we usually call the functions just _work(), not _worker(), so I matched here

>> +	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

will fix.

Daniele

>>   #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