Re: [Intel-gfx] [PATCH v10 09/17] drm/i915/pxp: Implement PXP irq handler

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Sep 20, 2021 at 04:18:10PM +0000, Teres Alexis, Alan Previn wrote:
> 
> On Mon, 2021-09-20 at 12:04 -0400, Rodrigo Vivi wrote:
> > On Fri, Sep 17, 2021 at 09:20:00PM -0700, Alan Previn wrote:
> > > From: "Huang, Sean Z" <sean.z.huang@intel.com>
> > > 
> > > The HW will generate a teardown interrupt when session termination is
> > > required, which requires i915 to submit a terminating batch. Once the HW
> > > is done with the termination it will generate another interrupt, at
> > > which point it is safe to re-create the session.
> > > 
> > > Since the termination and re-creation flow is something we want to
> > > trigger from the driver as well, use a common work function that can be
> > > called both from the irq handler and from the driver set-up flows, which
> > > has the addded benefit of allowing us to skip any extra locks because
> > > the work itself serializes the operations.
> > > 
> > > v2: use struct completion instead of bool (Chris)
> > > v3: drop locks, clean up functions and improve comments (Chris),
> > >     move to common work function.
> > > v4: improve comments, simplify wait logic (Rodrigo)
> > > v5: unconditionally set interrupts, rename state_attacked var (Rodrigo)
> > > 
> > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > +#include <linux/workqueue.h>
> > > +#include "intel_pxp.h"
> > > +#include "intel_pxp_irq.h"
> > > +#include "intel_pxp_session.h"
> > > +#include "gt/intel_gt_irq.h"
> > > +#include "gt/intel_gt_types.h"
> > > +#include "i915_irq.h"
> > > +#include "i915_reg.h"
> > > +
> > > +/**
> > > + * intel_pxp_irq_handler - Handles PXP interrupts.
> > > + * @pxp: pointer to pxp struct
> > > + * @iir: interrupt vector
> > > + */
> > > +void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > > +{
> > > +	struct intel_gt *gt = pxp_to_gt(pxp);
> > 
> > this compiles, but I don't see how this can work.
> > 
> > shouldn't we use the container_of here directly instead of trying
> > to use something that is not properly defined?
> > 
> Its now a function that's abstracted in .c file and prototype defined in the intel_pxp.h. Unless i
> misunderstood something, i thought this was one of the options discussed in last rev - i can change it
> again to use the container_of directly in all the instances if you like. side note: inline was still
> defined but only for the CONFIG_PXP==off case that doesn't require the additional header inclusions.

Oh, indeed.... Sorry
If this was not working we would get an "Undefined referrence" in the face.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

for this v as well.

> 
> > > +