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, 2024-01-08 at 11:48 -0800, Daniele Ceraolo Spurio wrote:
> 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:
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)
alan: hmm...  i think there is a hw spec page that just lists the various engine instances
 - hope i'm not mistaken - will sync with u offline.
alan:snip


> > >   #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.
alan: okay thanks - I see that.

> > >   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.
alan: sounds good - sounds like something we dont want to invest in unless we really
have a use-case for it considering the gsc-proxy-interaction is basically a fixed fsm.

> > > +	/** @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
alan: sounds good - its a nit anyway.