On Wed, 2023-01-18 at 18:01 -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 1/11/2023 1:42 PM, Alan Previn wrote:
> > Despite KCR subsystem being in the media-tile (close to the
> > GSC-CS), the IRQ controls for it are on GT-0 with other global
> > IRQ controls. Thus, add a helper for KCR hw interrupt
> > enable/disable functions to get the correct gt structure (for
> > uncore) for MTL.
> >
> > In the helper, we get GT-0's handle for uncore when touching
> > IRQ registers despite the pxp->ctrl_gt being the media-tile.
> > No difference for legacy of course.
> >
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 23 +++++++++++++++++---
> > drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 8 +++++++
> > 3 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 4b8e70caa3ad..9f6e300486b4 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -44,7 +44,7 @@ static int pxp_terminate_get(void *data, u64 *val)
> > static int pxp_terminate_set(void *data, u64 val)
> > {
> > struct intel_pxp *pxp = data;
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
>
> This doesn't seem to be required here. The only use of gt in this
> function is an assert below and both the root and media gt point to the
> same irq_lock, so it doesn't matter which one we're using. Should we
> keep it anyway just for safety in case things change in the future? or
> just add a comment instead?
>
I rather we keep this helper for consistency: everything else in front-end
pxp code is using pxp->ctrl_gt, but if we use pxp->[root_gt] here, it would
just read like a bug without understanding the hw details.
As you have pointed out farther down, the helper could just return root gt
always (since they both point to the same IRQ register). I will go ahead and
follow your recommendation for the helper internals (with the comments
explaining in detail) but have the callers continue to use that helper.
> >
> > if (!intel_pxp_is_active(pxp))
> > return -ENODEV;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > index 91e9622c07d0..2eef0c19e91a 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > @@ -8,6 +8,7 @@
> > #include "gt/intel_gt_regs.h"
> > #include "gt/intel_gt_types.h"
> >
> > +#include "i915_drv.h"
> > #include "i915_irq.h"
> > #include "i915_reg.h"
> >
> > @@ -17,6 +18,22 @@
> > #include "intel_pxp_types.h"
> > #include "intel_runtime_pm.h"
> >
> > +/**
> > + * intel_pxp_get_irq_gt - Find the correct GT that owns KCR interrupts
> > + * @pxp: pointer to pxp struct
> > + *
> > + * For platforms with a single GT, we return the pxp->ctrl_gt (as expected)
> > + * but for MTL+ that has a media-tile, although the KCR engine is in the
> > + * media-tile (i.e. pxp->ctrl_gt), the IRQ controls are on the root tile.
> > + */
> > +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> > +{
> > + if (pxp->uses_gsccs)
> > + return to_gt(pxp->ctrl_gt->i915);
> > +
> > + return pxp->ctrl_gt;
>
> AFAICT here we can skip the if and always return the root gt, because
> that's what happens in both cases. If you want to make sure we don't get
> issues in the future maybe instead add a:
>
> GEM_BUG_ON(!i915->media_gt && !gt_is_root(pxp->ctrl_gt))
will do.
>
> > +}
> > +
> > /**
> > * intel_pxp_irq_handler - Handles PXP interrupts.
> > * @pxp: pointer to pxp struct
> > @@ -29,7 +46,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
> > return;
> >
> > - gt = pxp->ctrl_gt;
> > + gt = intel_pxp_get_irq_gt(pxp);
>
> Here also we only have the assert below
i will follow the above recommendation.
>
> Daniele
>
> >
> > lockdep_assert_held(gt->irq_lock);
> >
> > @@ -68,7 +85,7 @@ static inline void pxp_irq_reset(struct intel_gt *gt)
> >
> > void intel_pxp_irq_enable(struct intel_pxp *pxp)
> > {
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
> >
> > spin_lock_irq(gt->irq_lock);
> >
> > @@ -83,7 +100,7 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
> >
> > void intel_pxp_irq_disable(struct intel_pxp *pxp)
> > {
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
> >
> > /*
> > * We always need to submit a global termination when we re-enable the
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > index 8c292dc86f68..eea87c9eb62b 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> >
> > struct intel_pxp;
> > +struct intel_gt;
> >
> > #define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
> > #define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
> > @@ -23,6 +24,8 @@ struct intel_pxp;
> > void intel_pxp_irq_enable(struct intel_pxp *pxp);
> > void intel_pxp_irq_disable(struct intel_pxp *pxp);
> > void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
> > +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp);
> > +
> > #else
> > static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > {
> > @@ -35,6 +38,11 @@ static inline void intel_pxp_irq_enable(struct intel_pxp *pxp)
> > static inline void intel_pxp_irq_disable(struct intel_pxp *pxp)
> > {
> > }
> > +
> > +static inline struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> > +{
> > + return NULL;
> > +}
> > #endif
> >
> > #endif /* __INTEL_PXP_IRQ_H__ */
>
