Em qua, 2019-03-13 às 16:13 -0700, Daniele Ceraolo Spurio escreveu: > This allows us to ditch i915 in some more places. > > RFC: should we just make them work directly on the regs pointer instead?
Both options look better than passing God Object dev_priv, so I'm fine with either. To give a parallel with other parts of the driver, in display code we tend to pass struct intel_crtc as much as possible and only pass enum pipe when it doesn't make sense anymore to pass a CRTC. So uncore makes sense here. More below: > > Cc: Paulo Zanoni <[email protected]> > Signed-off-by: Daniele Ceraolo Spurio <[email protected]> > --- > drivers/gpu/drm/i915/i915_drv.h | 14 ++--- > drivers/gpu/drm/i915/i915_vgpu.c | 8 ++- > drivers/gpu/drm/i915/intel_uncore.c | 90 +++++++++++++++-------------- > 3 files changed, 58 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 293bf68fd8ba..a1b8059cd7f8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3485,17 +3485,17 @@ static inline u64 intel_rc6_residency_us(struct > drm_i915_private *dev_priv, > #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) > > #define __raw_read(x, s) \ > -static inline uint##x##_t __raw_i915_read##x(const struct drm_i915_private > *dev_priv, \ > +static inline uint##x##_t __raw_i915_read##x(const struct intel_uncore > *uncore, \ > i915_reg_t reg) \ > { \ > - return read##s(dev_priv->uncore.regs + i915_mmio_reg_offset(reg)); \ > + return read##s(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, \ > +static inline void __raw_i915_write##x(const struct intel_uncore *uncore, \ > i915_reg_t reg, uint##x##_t val) \ > { \ > - write##s(val, dev_priv->uncore.regs + i915_mmio_reg_offset(reg)); \ > + write##s(val, uncore->regs + i915_mmio_reg_offset(reg)); \ > } > __raw_read(8, b) > __raw_read(16, w) > @@ -3536,9 +3536,9 @@ __raw_write(64, q) > * therefore generally be serialised, by either the dev_priv->uncore.lock or > * a more localised lock guarding all access to that bank of registers. > */ > -#define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) > -#define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), > (val__)) > -#define I915_WRITE64_FW(reg__, val__) __raw_i915_write64(dev_priv, (reg__), > (val__)) > +#define I915_READ_FW(reg__) __raw_i915_read32(&dev_priv->uncore, (reg__)) > +#define I915_WRITE_FW(reg__, val__) __raw_i915_write32(&dev_priv->uncore, > (reg__), (val__)) > +#define I915_WRITE64_FW(reg__, val__) __raw_i915_write64(&dev_priv->uncore, > (reg__), (val__)) Looking forward to the day dev_priv won't be implicit in register read/write macros :). > #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__) > > /* "Broadcast RGB" property */ > diff --git a/drivers/gpu/drm/i915/i915_vgpu.c > b/drivers/gpu/drm/i915/i915_vgpu.c > index 869cf4a3b6de..51e5e2630eec 100644 > --- a/drivers/gpu/drm/i915/i915_vgpu.c > +++ b/drivers/gpu/drm/i915/i915_vgpu.c > @@ -65,17 +65,19 @@ void i915_check_vgpu(struct drm_i915_private *dev_priv) You could have gone for a local intel_uncore variable here too. With or without that: Reviewed-by: Paulo Zanoni <[email protected]> > > BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE); > > - magic = __raw_i915_read64(dev_priv, vgtif_reg(magic)); > + magic = __raw_i915_read64(&dev_priv->uncore, vgtif_reg(magic)); > if (magic != VGT_MAGIC) > return; > > - version_major = __raw_i915_read16(dev_priv, vgtif_reg(version_major)); > + version_major = __raw_i915_read16(&dev_priv->uncore, > + vgtif_reg(version_major)); > if (version_major < VGT_VERSION_MAJOR) { > DRM_INFO("VGT interface version mismatch!\n"); > return; > } > > - dev_priv->vgpu.caps = __raw_i915_read32(dev_priv, vgtif_reg(vgt_caps)); > + dev_priv->vgpu.caps = __raw_i915_read32(&dev_priv->uncore, > + vgtif_reg(vgt_caps)); > > dev_priv->vgpu.active = true; > DRM_INFO("Virtual GPU for Intel GVT-g detected.\n"); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > b/drivers/gpu/drm/i915/intel_uncore.c > index ce076d046b65..4c20b2fc33fe 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -31,7 +31,7 @@ > #define FORCEWAKE_ACK_TIMEOUT_MS 50 > #define GT_FIFO_TIMEOUT_MS 10 > > -#define __raw_posting_read(dev_priv__, reg__) > (void)__raw_i915_read32((dev_priv__), (reg__)) > +#define __raw_posting_read(uncore__, reg__) > (void)__raw_i915_read32((uncore__), (reg__)) > > static const char * const forcewake_domain_names[] = { > "render", > @@ -273,23 +273,23 @@ fw_domains_reset(struct intel_uncore *uncore, > fw_domain_reset(d); > } > > -static inline u32 gt_thread_status(struct drm_i915_private *dev_priv) > +static inline u32 gt_thread_status(struct intel_uncore *uncore) > { > u32 val; > > - val = __raw_i915_read32(dev_priv, GEN6_GT_THREAD_STATUS_REG); > + val = __raw_i915_read32(uncore, GEN6_GT_THREAD_STATUS_REG); > val &= GEN6_GT_THREAD_STATUS_CORE_MASK; > > return val; > } > > -static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv) > +static void __gen6_gt_wait_for_thread_c0(struct intel_uncore *uncore) > { > /* > * w/a for a sporadic read returning 0 by waiting for the GT > * thread to wake up. > */ > - WARN_ONCE(wait_for_atomic_us(gt_thread_status(dev_priv) == 0, 5000), > + WARN_ONCE(wait_for_atomic_us(gt_thread_status(uncore) == 0, 5000), > "GT thread status wait timed out\n"); > } > > @@ -299,30 +299,29 @@ static void fw_domains_get_with_thread_status(struct > intel_uncore *uncore, > fw_domains_get(uncore, fw_domains); > > /* WaRsForcewakeWaitTC0:snb,ivb,hsw,bdw,vlv */ > - __gen6_gt_wait_for_thread_c0(uncore_to_i915(uncore)); > + __gen6_gt_wait_for_thread_c0(uncore); > } > > -static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv) > +static inline u32 fifo_free_entries(struct intel_uncore *uncore) > { > - u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL); > + u32 count = __raw_i915_read32(uncore, GTFIFOCTL); > > return count & GT_FIFO_FREE_ENTRIES_MASK; > } > > -static void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv) > +static void __gen6_gt_wait_for_fifo(struct intel_uncore *uncore) > { > - struct intel_uncore *uncore = &dev_priv->uncore; > u32 n; > > /* On VLV, FIFO will be shared by both SW and HW. > * So, we need to read the FREE_ENTRIES everytime */ > - if (IS_VALLEYVIEW(dev_priv)) > - n = fifo_free_entries(dev_priv); > + if (IS_VALLEYVIEW(uncore_to_i915(uncore))) > + n = fifo_free_entries(uncore); > else > n = uncore->fifo_count; > > if (n <= GT_FIFO_NUM_RESERVED_ENTRIES) { > - if (wait_for_atomic((n = fifo_free_entries(dev_priv)) > > + if (wait_for_atomic((n = fifo_free_entries(uncore)) > > GT_FIFO_NUM_RESERVED_ENTRIES, > GT_FIFO_TIMEOUT_MS)) { > DRM_DEBUG("GT_FIFO timeout, entries: %u\n", n); > @@ -450,7 +449,7 @@ static void intel_uncore_edram_detect(struct > drm_i915_private *dev_priv) > if (IS_HASWELL(dev_priv) || > IS_BROADWELL(dev_priv) || > INTEL_GEN(dev_priv) >= 9) { > - dev_priv->edram_cap = __raw_i915_read32(dev_priv, > + dev_priv->edram_cap = __raw_i915_read32(&dev_priv->uncore, > HSW_EDRAM_CAP); > > /* NB: We can't write IDICR yet because we do not have gt funcs > @@ -465,43 +464,43 @@ static void intel_uncore_edram_detect(struct > drm_i915_private *dev_priv) > } > > static bool > -fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > +fpga_check_for_unclaimed_mmio(struct intel_uncore *uncore) > { > u32 dbg; > > - dbg = __raw_i915_read32(dev_priv, FPGA_DBG); > + dbg = __raw_i915_read32(uncore, FPGA_DBG); > if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM))) > return false; > > - __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > + __raw_i915_write32(uncore, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > > return true; > } > > static bool > -vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > +vlv_check_for_unclaimed_mmio(struct intel_uncore *uncore) > { > u32 cer; > > - cer = __raw_i915_read32(dev_priv, CLAIM_ER); > + cer = __raw_i915_read32(uncore, CLAIM_ER); > if (likely(!(cer & (CLAIM_ER_OVERFLOW | CLAIM_ER_CTR_MASK)))) > return false; > > - __raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR); > + __raw_i915_write32(uncore, CLAIM_ER, CLAIM_ER_CLR); > > return true; > } > > static bool > -gen6_check_for_fifo_debug(struct drm_i915_private *dev_priv) > +gen6_check_for_fifo_debug(struct intel_uncore *uncore) > { > u32 fifodbg; > > - fifodbg = __raw_i915_read32(dev_priv, GTFIFODBG); > + fifodbg = __raw_i915_read32(uncore, GTFIFODBG); > > if (unlikely(fifodbg)) { > DRM_DEBUG_DRIVER("GTFIFODBG = 0x08%x\n", fifodbg); > - __raw_i915_write32(dev_priv, GTFIFODBG, fifodbg); > + __raw_i915_write32(uncore, GTFIFODBG, fifodbg); > } > > return fifodbg; > @@ -510,16 +509,17 @@ gen6_check_for_fifo_debug(struct drm_i915_private > *dev_priv) > static bool > check_for_unclaimed_mmio(struct drm_i915_private *dev_priv) > { > + struct intel_uncore *uncore = &dev_priv->uncore; > bool ret = false; > > if (HAS_FPGA_DBG_UNCLAIMED(dev_priv)) > - ret |= fpga_check_for_unclaimed_mmio(dev_priv); > + ret |= fpga_check_for_unclaimed_mmio(uncore); > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - ret |= vlv_check_for_unclaimed_mmio(dev_priv); > + ret |= vlv_check_for_unclaimed_mmio(uncore); > > if (IS_GEN_RANGE(dev_priv, 6, 7)) > - ret |= gen6_check_for_fifo_debug(dev_priv); > + ret |= gen6_check_for_fifo_debug(uncore); > > return ret; > } > @@ -535,8 +535,8 @@ static void __intel_uncore_early_sanitize(struct > intel_uncore *uncore, > > /* WaDisableShadowRegForCpd:chv */ > if (IS_CHERRYVIEW(i915)) { > - __raw_i915_write32(i915, GTFIFOCTL, > - __raw_i915_read32(i915, GTFIFOCTL) | > + __raw_i915_write32(uncore, GTFIFOCTL, > + __raw_i915_read32(uncore, GTFIFOCTL) | > GT_FIFO_CTL_BLOCK_ALL_POLICY_STALL | > GT_FIFO_CTL_RC6_POLICY_STALL); > } > @@ -548,7 +548,7 @@ static void __intel_uncore_early_sanitize(struct > intel_uncore *uncore, > uncore->funcs.force_wake_get(uncore, restore_forcewake); > > if (IS_GEN_RANGE(i915, 6, 7)) > - uncore->fifo_count = fifo_free_entries(i915); > + uncore->fifo_count = fifo_free_entries(uncore); > spin_unlock_irq(&uncore->lock); > } > iosf_mbi_punit_release(); > @@ -1061,12 +1061,12 @@ static const struct intel_forcewake_range > __gen11_fw_ranges[] = { > }; > > static void > -ilk_dummy_write(struct drm_i915_private *dev_priv) > +ilk_dummy_write(struct intel_uncore *uncore) > { > /* WaIssueDummyWriteToWakeupFromRC6:ilk Issue a dummy write to wake up > * the chip from rc6 before touching it for real. MI_MODE is masked, > * hence harmless to write 0 into. */ > - __raw_i915_write32(dev_priv, MI_MODE, 0); > + __raw_i915_write32(uncore, MI_MODE, 0); > } > > static void > @@ -1096,6 +1096,7 @@ unclaimed_reg_debug(struct drm_i915_private *dev_priv, > } > > #define GEN2_READ_HEADER(x) \ > + struct intel_uncore *uncore = &dev_priv->uncore; \ > u##x val = 0; \ > assert_rpm_wakelock_held(dev_priv); > > @@ -1107,7 +1108,7 @@ unclaimed_reg_debug(struct drm_i915_private *dev_priv, > static u##x \ > gen2_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) > { \ > GEN2_READ_HEADER(x); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + val = __raw_i915_read##x(uncore, reg); \ > GEN2_READ_FOOTER; \ > } > > @@ -1115,8 +1116,8 @@ gen2_read##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, bool trace) { \ > static u##x \ > gen5_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) > { \ > GEN2_READ_HEADER(x); \ > - ilk_dummy_write(dev_priv); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + ilk_dummy_write(uncore); \ > + val = __raw_i915_read##x(uncore, reg); \ > GEN2_READ_FOOTER; \ > } > > @@ -1186,7 +1187,7 @@ func##_read##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, bool trace) { > fw_engine = __##func##_reg_read_fw_domains(uncore, offset); \ > if (fw_engine) \ > __force_wake_auto(uncore, fw_engine); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + val = __raw_i915_read##x(uncore, reg); \ > GEN6_READ_FOOTER; \ > } > #define __gen6_read(x) __gen_read(gen6, x) > @@ -1213,6 +1214,7 @@ __gen6_read(64) > #undef GEN6_READ_HEADER > > #define GEN2_WRITE_HEADER \ > + struct intel_uncore *uncore = &dev_priv->uncore; \ > trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ > assert_rpm_wakelock_held(dev_priv); \ > > @@ -1222,7 +1224,7 @@ __gen6_read(64) > static void \ > gen2_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, > bool trace) { \ > GEN2_WRITE_HEADER; \ > - __raw_i915_write##x(dev_priv, reg, val); \ > + __raw_i915_write##x(uncore, reg, val); \ > GEN2_WRITE_FOOTER; \ > } > > @@ -1230,8 +1232,8 @@ gen2_write##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, u##x val, bool > static void \ > gen5_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, > bool trace) { \ > GEN2_WRITE_HEADER; \ > - ilk_dummy_write(dev_priv); \ > - __raw_i915_write##x(dev_priv, reg, val); \ > + ilk_dummy_write(uncore); \ > + __raw_i915_write##x(uncore, reg, val); \ > GEN2_WRITE_FOOTER; \ > } > > @@ -1266,8 +1268,8 @@ static void \ > gen6_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, > bool trace) { \ > GEN6_WRITE_HEADER; \ > if (NEEDS_FORCE_WAKE(offset)) \ > - __gen6_gt_wait_for_fifo(dev_priv); \ > - __raw_i915_write##x(dev_priv, reg, val); \ > + __gen6_gt_wait_for_fifo(uncore); \ > + __raw_i915_write##x(uncore, reg, val); \ > GEN6_WRITE_FOOTER; \ > } > > @@ -1279,7 +1281,7 @@ func##_write##x(struct drm_i915_private *dev_priv, > i915_reg_t reg, u##x val, boo > fw_engine = __##func##_reg_write_fw_domains(uncore, offset); \ > if (fw_engine) \ > __force_wake_auto(uncore, fw_engine); \ > - __raw_i915_write##x(dev_priv, reg, val); \ > + __raw_i915_write##x(uncore, reg, val); \ > GEN6_WRITE_FOOTER; \ > } > #define __gen8_write(x) __gen_write(gen8, x) > @@ -1477,15 +1479,15 @@ static void intel_uncore_fw_domains_init(struct > intel_uncore *uncore) > * before the ecobus check. > */ > > - __raw_i915_write32(i915, FORCEWAKE, 0); > - __raw_posting_read(i915, ECOBUS); > + __raw_i915_write32(uncore, FORCEWAKE, 0); > + __raw_posting_read(uncore, ECOBUS); > > fw_domain_init(uncore, FW_DOMAIN_ID_RENDER, > FORCEWAKE_MT, FORCEWAKE_MT_ACK); > > spin_lock_irq(&uncore->lock); > fw_domains_get_with_thread_status(uncore, FORCEWAKE_RENDER); > - ecobus = __raw_i915_read32(i915, ECOBUS); > + ecobus = __raw_i915_read32(uncore, ECOBUS); > fw_domains_put(uncore, FORCEWAKE_RENDER); > spin_unlock_irq(&uncore->lock); > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
