Re: [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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__ */
> 





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux