[Intel-gfx] [PATCH 4/4] drm/i915/gsc: add support for GSC proxy interrupt

[Teres Alexis, Alan Previn]

I think we are also bottom-ing on the opens fo this patch too:


On Thu, 2023-04-20 at 13:21 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 11:49 AM, Teres Alexis, Alan Previn wrote:
> > On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> > > The GSC notifies us of a proxy request via the HECI2 interrupt. The
> > alan:snip
> > 
> > > @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> > >   	u32 irqs = GT_RENDER_USER_INTERRUPT;
> > >   	u32 guc_mask = intel_uc_wants_guc(&gt->uc) ? GUC_INTR_GUC2HOST : 0;
> > >   	u32 gsc_mask = 0;
> > > +	u32 heci_mask = 0;
> > alan: nit: should we call this heci2_mask - uniquely identifiable from legacy.
> 
> The mask is theoretically not just for HECI2, the bit we set in it is. 
> If future platforms add back the HECI1 interrupt then it'd go in the 
> same mask.
alan: yeah - im good with that - no change needed then - was a nit anyways.


> > alan:snip
> > 
> > > -	else if (HAS_HECI_GSC(gt->i915))
> > > +		heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
> > alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2?
> > i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14?
> 
> GSC_IRQ_INTF(1) is the HECI2 interrupt on DG2 and the bit has remained 
> the same exactly to allow SW to re-use the same code/defines, so IMO we 
> should do so.
alan: okay sure - sounds good.


> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index 4aecb5a7b631..da11ce5ca99e 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -1577,6 +1577,7 @@
> > >   
> > >   #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
> > >   #define   GEN11_CSME				(31)
> > > +#define   GEN12_HECI_2				(30)
> > alan: I dont see this being used anywhere - should remove this.
> 
> A few of the defines for this register here are not used. I've added 
> this one in as a way of documenting where the bit was, but I can remove 
> it if you think it's unneeded.
alan: Oh i actually didnt notice that earlier - in that case, please keep it
would be nice to add a comment for all of those such bits (consider this a nit).

> > 

> > > +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
> > > +{
> > > +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> > > +
> > > +	if (unlikely(!iir))
> > > +		return;
> > > +
> > > +	lockdep_assert_held(gt->irq_lock);
> > > +
> > > +	if (!gsc->proxy.component) {
> > > +		gt_err(gt, "GSC proxy irq received without the component being bound!\n");
> > alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we
> > know that proxy runtime irqs could happen anytime (depending on other gpu-security-related
> > system interactions). However, would the component driver be bound/unbound through a
> > suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request
> > came too early after a resume cycle. If yes, then should we not do somethign here to ensure that
> > when the component gets bound later, we know there is something waiting to be processed?
> > (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger
> > the worker from there, i.e. the bind func?)
> 
> A proxy request can only be triggered in response to a userspace ask, 
> which in turn can only happen once we've completed the resume flow and 
> the component is re-bound. Therefore, we should never have a situation 
> where we get a proxy interrupt without the component being bound.
alan: thanks -my bad.
> > 


> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > @@ -23,6 +23,9 @@ struct intel_gsc_uc {
> > >   	/* for delayed load and proxy handling */
> > >   	struct workqueue_struct *wq;
> > >   	struct work_struct work;
> > > +	u32 gsc_work_actions; /* protected by gt->irq_lock */
> > > +#define GSC_ACTION_FW_LOAD BIT(0)
> > > +#define GSC_ACTION_SW_PROXY BIT(1
> > > 
> > alan: perhaps its time to have a substruct for "proxy_worker" and include
> > 'wq' and 'work' in additional to actions?
> 
> The worker is not just for proxy, we use it for a GSC and HuC loading as 
> well. It's the main way we handle the gsc_uc, so IMO it's better off 
> staying in the main struct. However, if you still think a substructure 
> will make things cleaner I can add it in, but please suggest a name 
> because I have no idea what to call it (something like handler?).
alan: i was thinking "task_handler" - but since its not specific to proxy,
then let's not change it.