| Message ID | 20190313231319.711-8-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Compartmentalize uncore code | expand |
Quoting Daniele Ceraolo Spurio (2019-03-13 23:13:16) > This will allow futher simplifications in the uncore handling. > > RFC: if we want to keep the pointer logically separate from the uncore, > we could also move both the regs pointer and the uncore struct > inside a new structure (intel_mmio?) and pass that around instead, or > just take a copy of the pointer > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 +++--- > drivers/gpu/drm/i915/i915_drv.h | 6 ++---- > drivers/gpu/drm/i915/i915_irq.c | 22 +++++++++++----------- > drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- > drivers/gpu/drm/i915/intel_uncore.c | 5 ++--- > drivers/gpu/drm/i915/intel_uncore.h | 2 ++ > 6 files changed, 23 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index a2e039f523c0..2470c1ef4951 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -949,8 +949,8 @@ static int i915_mmio_setup(struct drm_i915_private *dev_priv) > mmio_size = 512 * 1024; > else > mmio_size = 2 * 1024 * 1024; > - dev_priv->regs = pci_iomap(pdev, mmio_bar, mmio_size); > - if (dev_priv->regs == NULL) { > + dev_priv->uncore.regs = pci_iomap(pdev, mmio_bar, mmio_size); > + if (dev_priv->uncore.regs == NULL) { > DRM_ERROR("failed to map registers\n"); > > return -EIO; > @@ -967,7 +967,7 @@ static void i915_mmio_cleanup(struct drm_i915_private *dev_priv) > struct pci_dev *pdev = dev_priv->drm.pdev; > > intel_teardown_mchbar(dev_priv); > - pci_iounmap(pdev, dev_priv->regs); > + pci_iounmap(pdev, dev_priv->uncore.regs); At the very least with moving to the uncore as owner of regs, the setup and cleanup should be moved to intel_uncore. As it stands, it looks reasonable, as the uncore being the arbitrator and debugger of mmio access. Now, what would make Ville and myself happy would be an artificial split of uncore into gpu and display roles, with an arch-arbiter around forcewake itself. i.e. since display doesn't really forcewake it doesn't need to share the pain of the forcewake spinlock -- it still needs some locks around to serialise mmio, but it wants much finer control as locking is a big hindrance wrt flip latency. On the gpu side, we already avoid using the general uncore routines on the hottest paths, but equally do not want latency from display operations. So even the coarse split between display/gpu should have immediate improvements. I'll leave you to judge if the knowledge and data structures gained from that exercise will pay off in future. -Chris
On 3/15/19 1:50 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2019-03-13 23:13:16) >> This will allow futher simplifications in the uncore handling. >> >> RFC: if we want to keep the pointer logically separate from the uncore, >> we could also move both the regs pointer and the uncore struct >> inside a new structure (intel_mmio?) and pass that around instead, or >> just take a copy of the pointer >> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 6 +++--- >> drivers/gpu/drm/i915/i915_drv.h | 6 ++---- >> drivers/gpu/drm/i915/i915_irq.c | 22 +++++++++++----------- >> drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- >> drivers/gpu/drm/i915/intel_uncore.c | 5 ++--- >> drivers/gpu/drm/i915/intel_uncore.h | 2 ++ >> 6 files changed, 23 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index a2e039f523c0..2470c1ef4951 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -949,8 +949,8 @@ static int i915_mmio_setup(struct drm_i915_private *dev_priv) >> mmio_size = 512 * 1024; >> else >> mmio_size = 2 * 1024 * 1024; >> - dev_priv->regs = pci_iomap(pdev, mmio_bar, mmio_size); >> - if (dev_priv->regs == NULL) { >> + dev_priv->uncore.regs = pci_iomap(pdev, mmio_bar, mmio_size); >> + if (dev_priv->uncore.regs == NULL) { >> DRM_ERROR("failed to map registers\n"); >> >> return -EIO; >> @@ -967,7 +967,7 @@ static void i915_mmio_cleanup(struct drm_i915_private *dev_priv) >> struct pci_dev *pdev = dev_priv->drm.pdev; >> >> intel_teardown_mchbar(dev_priv); >> - pci_iounmap(pdev, dev_priv->regs); >> + pci_iounmap(pdev, dev_priv->uncore.regs); > > At the very least with moving to the uncore as owner of regs, the setup > and cleanup should be moved to intel_uncore. > I knew someone was going to call me out on this :P. I was waiting to confirm the approach was ok before moving stuff around, I should have noted it in the commit message > As it stands, it looks reasonable, as the uncore being the arbitrator > and debugger of mmio access. > > Now, what would make Ville and myself happy would be an artificial > split of uncore into gpu and display roles, with an arch-arbiter around > forcewake itself. i.e. since display doesn't really forcewake it doesn't > need to share the pain of the forcewake spinlock -- it still needs some > locks around to serialise mmio, but it wants much finer control as > locking is a big hindrance wrt flip latency. On the gpu side, we already > avoid using the general uncore routines on the hottest paths, but > equally do not want latency from display operations. So even the coarse > split between display/gpu should have immediate improvements. > Sounds good, I'll look at this after this series stabilizes. Thanks, Daniele > I'll leave you to judge if the knowledge and data structures gained from > that exercise will pay off in future. > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a2e039f523c0..2470c1ef4951 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -949,8 +949,8 @@ static int i915_mmio_setup(struct drm_i915_private *dev_priv) mmio_size = 512 * 1024; else mmio_size = 2 * 1024 * 1024; - dev_priv->regs = pci_iomap(pdev, mmio_bar, mmio_size); - if (dev_priv->regs == NULL) { + dev_priv->uncore.regs = pci_iomap(pdev, mmio_bar, mmio_size); + if (dev_priv->uncore.regs == NULL) { DRM_ERROR("failed to map registers\n"); return -EIO; @@ -967,7 +967,7 @@ static void i915_mmio_cleanup(struct drm_i915_private *dev_priv) struct pci_dev *pdev = dev_priv->drm.pdev; intel_teardown_mchbar(dev_priv); - pci_iounmap(pdev, dev_priv->regs); + pci_iounmap(pdev, dev_priv->uncore.regs); } /** diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8a4b5bbac02e..293bf68fd8ba 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1505,8 +1505,6 @@ struct drm_i915_private { */ resource_size_t stolen_usable_size; /* Total size minus reserved ranges */ - void __iomem *regs; - struct intel_uncore uncore; struct i915_virtual_gpu vgpu; @@ -3490,14 +3488,14 @@ static inline u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv, static inline uint##x##_t __raw_i915_read##x(const struct drm_i915_private *dev_priv, \ i915_reg_t reg) \ { \ - return read##s(dev_priv->regs + i915_mmio_reg_offset(reg)); \ + return read##s(dev_priv->uncore.regs + i915_mmio_reg_offset(reg)); \ } #define __raw_write(x, s) \ static inline void __raw_i915_write##x(const struct drm_i915_private *dev_priv, \ i915_reg_t reg, uint##x##_t val) \ { \ - write##s(val, dev_priv->regs + i915_mmio_reg_offset(reg)); \ + write##s(val, dev_priv->uncore.regs + i915_mmio_reg_offset(reg)); \ } __raw_read(8, b) __raw_read(16, w) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index c823d2e76852..f5a5cdf2f645 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -268,7 +268,7 @@ static bool gen11_reset_one_iir(struct drm_i915_private * const i915, const unsigned int bank, const unsigned int bit) { - void __iomem * const regs = i915->regs; + void __iomem * const regs = i915->uncore.regs; u32 dw; lockdep_assert_held(&i915->irq_lock); @@ -1471,7 +1471,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir) static void gen8_gt_irq_ack(struct drm_i915_private *i915, u32 master_ctl, u32 gt_iir[4]) { - void __iomem * const regs = i915->regs; + void __iomem * const regs = i915->uncore.regs; #define GEN8_GT_IRQS (GEN8_GT_RCS_IRQ | \ GEN8_GT_BCS_IRQ | \ @@ -2868,7 +2868,7 @@ static inline void gen8_master_intr_enable(void __iomem * const regs) static irqreturn_t gen8_irq_handler(int irq, void *arg) { struct drm_i915_private *dev_priv = to_i915(arg); - void __iomem * const regs = dev_priv->regs; + void __iomem * const regs = dev_priv->uncore.regs; u32 master_ctl; u32 gt_iir[4]; @@ -2902,7 +2902,7 @@ static u32 gen11_gt_engine_identity(struct drm_i915_private * const i915, const unsigned int bank, const unsigned int bit) { - void __iomem * const regs = i915->regs; + void __iomem * const regs = i915->uncore.regs; u32 timeout_ts; u32 ident; @@ -2986,7 +2986,7 @@ static void gen11_gt_bank_handler(struct drm_i915_private * const i915, const unsigned int bank) { - void __iomem * const regs = i915->regs; + void __iomem * const regs = i915->uncore.regs; unsigned long intr_dw; unsigned int bit; @@ -3029,7 +3029,7 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915, static u32 gen11_gu_misc_irq_ack(struct drm_i915_private *dev_priv, const u32 master_ctl) { - void __iomem * const regs = dev_priv->regs; + void __iomem * const regs = dev_priv->uncore.regs; u32 iir; if (!(master_ctl & GEN11_GU_MISC_IRQ)) @@ -3070,7 +3070,7 @@ static inline void gen11_master_intr_enable(void __iomem * const regs) static irqreturn_t gen11_irq_handler(int irq, void *arg) { struct drm_i915_private * const i915 = to_i915(arg); - void __iomem * const regs = i915->regs; + void __iomem * const regs = i915->uncore.regs; u32 master_ctl; u32 gu_misc_iir; @@ -3351,7 +3351,7 @@ static void gen8_irq_reset(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); int pipe; - gen8_master_intr_disable(dev_priv->regs); + gen8_master_intr_disable(dev_priv->uncore.regs); gen8_gt_irq_reset(dev_priv); @@ -3393,7 +3393,7 @@ static void gen11_irq_reset(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int pipe; - gen11_master_intr_disable(dev_priv->regs); + gen11_master_intr_disable(dev_priv->uncore.regs); gen11_gt_irq_reset(dev_priv); @@ -3998,7 +3998,7 @@ static int gen8_irq_postinstall(struct drm_device *dev) if (HAS_PCH_SPLIT(dev_priv)) ibx_irq_postinstall(dev); - gen8_master_intr_enable(dev_priv->regs); + gen8_master_intr_enable(dev_priv->uncore.regs); return 0; } @@ -4060,7 +4060,7 @@ static int gen11_irq_postinstall(struct drm_device *dev) I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE); - gen11_master_intr_enable(dev_priv->regs); + gen11_master_intr_enable(dev_priv->uncore.regs); POSTING_READ(GEN11_GFX_MSTR_IRQ); return 0; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index dc3de09c7586..0e6dd9c1365a 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2412,12 +2412,12 @@ static int logical_ring_init(struct intel_engine_cs *engine) intel_engine_init_workarounds(engine); if (HAS_LOGICAL_RING_ELSQ(i915)) { - execlists->submit_reg = i915->regs + + execlists->submit_reg = i915->uncore.regs + i915_mmio_reg_offset(RING_EXECLIST_SQ_CONTENTS(engine)); - execlists->ctrl_reg = i915->regs + + execlists->ctrl_reg = i915->uncore.regs + i915_mmio_reg_offset(RING_EXECLIST_CONTROL(engine)); } else { - execlists->submit_reg = i915->regs + + execlists->submit_reg = i915->uncore.regs + i915_mmio_reg_offset(RING_ELSP(engine)); } diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 32c75337b96d..ce076d046b65 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1328,7 +1328,6 @@ static void fw_domain_init(struct intel_uncore *uncore, i915_reg_t reg_ack) { struct intel_uncore_forcewake_domain *d; - struct drm_i915_private *i915 = uncore_to_i915(uncore); if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT)) return; @@ -1341,8 +1340,8 @@ static void fw_domain_init(struct intel_uncore *uncore, WARN_ON(!i915_mmio_reg_valid(reg_ack)); d->wake_count = 0; - d->reg_set = i915->regs + i915_mmio_reg_offset(reg_set); - d->reg_ack = i915->regs + i915_mmio_reg_offset(reg_ack); + d->reg_set = uncore->regs + i915_mmio_reg_offset(reg_set); + d->reg_ack = uncore->regs + i915_mmio_reg_offset(reg_ack); d->id = domain_id; diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 4d0d7ec785f8..9c32714fa1ab 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -93,6 +93,8 @@ struct intel_forcewake_range { }; struct intel_uncore { + void __iomem *regs; + spinlock_t lock; /** lock is also taken in irq contexts. */ const struct intel_forcewake_range *fw_domains_table;
This will allow futher simplifications in the uncore handling. RFC: if we want to keep the pointer logically separate from the uncore, we could also move both the regs pointer and the uncore struct inside a new structure (intel_mmio?) and pass that around instead, or just take a copy of the pointer Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 6 +++--- drivers/gpu/drm/i915/i915_drv.h | 6 ++---- drivers/gpu/drm/i915/i915_irq.c | 22 +++++++++++----------- drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- drivers/gpu/drm/i915/intel_uncore.c | 5 ++--- drivers/gpu/drm/i915/intel_uncore.h | 2 ++ 6 files changed, 23 insertions(+), 24 deletions(-)