[Intel-gfx] [RFC 1/8] drm/i915: use vfuncs for reg_read/write_fw_domains
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Jun 7 08:54:07 UTC 2019
On 06/06/2019 22:52, Daniele Ceraolo Spurio wrote:
> Instead of going through the if-else chain every time, let's save the
> function in the uncore structure. Note that the new functions are
> purposely not used from the reg read/write functions to keep the
> inlining there.
>
> While at it, use the new macro to call the old ones to clean the code a
> bit.
Very neat idea!
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 172 ++++++++-----------
> drivers/gpu/drm/i915/intel_uncore.h | 5 +
> drivers/gpu/drm/i915/selftests/mock_uncore.c | 4 +-
> 3 files changed, 75 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index f78668123f02..c3be79c4957b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -901,6 +901,12 @@ static bool is_gen##x##_shadowed(u32 offset) \
> __is_genX_shadowed(8)
> __is_genX_shadowed(11)
>
> +static enum forcewake_domains
> +gen6_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg)
> +{
> + return FORCEWAKE_RENDER;
> +}
> +
> #define __gen8_reg_write_fw_domains(uncore, offset) \
> ({ \
> enum forcewake_domains __fwd; \
> @@ -1145,26 +1151,23 @@ func##_read##x(struct intel_uncore *uncore, i915_reg_t reg, bool trace) { \
> val = __raw_uncore_read##x(uncore, reg); \
> GEN6_READ_FOOTER; \
> }
> -#define __gen6_read(x) __gen_read(gen6, x)
> -#define __fwtable_read(x) __gen_read(fwtable, x)
> -#define __gen11_fwtable_read(x) __gen_read(gen11_fwtable, x)
> -
> -__gen11_fwtable_read(8)
> -__gen11_fwtable_read(16)
> -__gen11_fwtable_read(32)
> -__gen11_fwtable_read(64)
> -__fwtable_read(8)
> -__fwtable_read(16)
> -__fwtable_read(32)
> -__fwtable_read(64)
> -__gen6_read(8)
> -__gen6_read(16)
> -__gen6_read(32)
> -__gen6_read(64)
> -
> -#undef __gen11_fwtable_read
> -#undef __fwtable_read
> -#undef __gen6_read
> +
> +#define __gen_reg_read_funcs(func) \
> +static enum forcewake_domains \
> +func##_reg_read_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
> + return __##func##_reg_read_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
> +} \
> +\
> +__gen_read(func, 8) \
> +__gen_read(func, 16) \
> +__gen_read(func, 32) \
> +__gen_read(func, 64)
> +
> +__gen_reg_read_funcs(gen11_fwtable);
> +__gen_reg_read_funcs(fwtable);
> +__gen_reg_read_funcs(gen6);
> +
> +#undef __gen_reg_read_funcs
> #undef GEN6_READ_FOOTER
> #undef GEN6_READ_HEADER
>
> @@ -1225,6 +1228,9 @@ gen6_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trace)
> __raw_uncore_write##x(uncore, reg, val); \
> GEN6_WRITE_FOOTER; \
> }
> +__gen6_write(8)
> +__gen6_write(16)
> +__gen6_write(32)
>
> #define __gen_write(func, x) \
> static void \
> @@ -1237,38 +1243,33 @@ func##_write##x(struct intel_uncore *uncore, i915_reg_t reg, u##x val, bool trac
> __raw_uncore_write##x(uncore, reg, val); \
> GEN6_WRITE_FOOTER; \
> }
> -#define __gen8_write(x) __gen_write(gen8, x)
> -#define __fwtable_write(x) __gen_write(fwtable, x)
> -#define __gen11_fwtable_write(x) __gen_write(gen11_fwtable, x)
> -
> -__gen11_fwtable_write(8)
> -__gen11_fwtable_write(16)
> -__gen11_fwtable_write(32)
> -__fwtable_write(8)
> -__fwtable_write(16)
> -__fwtable_write(32)
> -__gen8_write(8)
> -__gen8_write(16)
> -__gen8_write(32)
> -__gen6_write(8)
> -__gen6_write(16)
> -__gen6_write(32)
>
> -#undef __gen11_fwtable_write
> -#undef __fwtable_write
> -#undef __gen8_write
> -#undef __gen6_write
> +#define __gen_reg_write_funcs(func) \
> +static enum forcewake_domains \
> +func##_reg_write_fw_domains(struct intel_uncore *uncore, i915_reg_t reg) { \
> + return __##func##_reg_write_fw_domains(uncore, i915_mmio_reg_offset(reg)); \
> +} \
> +\
> +__gen_write(func, 8) \
> +__gen_write(func, 16) \
> +__gen_write(func, 32)
> +
> +__gen_reg_write_funcs(gen11_fwtable);
> +__gen_reg_write_funcs(fwtable);
> +__gen_reg_write_funcs(gen8);
> +
> +#undef __gen_reg_write_funcs
> #undef GEN6_WRITE_FOOTER
> #undef GEN6_WRITE_HEADER
>
> -#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
> +#define ASSIGN_WRITE_MMIO_VFUNCS_NO_FW(uncore, x) \
I only needed a couple minutes to figure out the NO_FW suffix means
platforms without this feature. Initially I was thinking about the
existing _FW suffix.
How about ASSIGN_RAW_READ/WRITE_MMIO_VFUNCS? Not sure either way, just
talking..
I didn't read it in detail but I think the idea is great and we want it
regardless of verdict on the rest. I can do a detailed review at a later
point.
Regards,
Tvrtko
> do { \
> (uncore)->funcs.mmio_writeb = x##_write8; \
> (uncore)->funcs.mmio_writew = x##_write16; \
> (uncore)->funcs.mmio_writel = x##_write32; \
> } while (0)
>
> -#define ASSIGN_READ_MMIO_VFUNCS(uncore, x) \
> +#define ASSIGN_READ_MMIO_VFUNCS_NO_FW(uncore, x) \
> do { \
> (uncore)->funcs.mmio_readb = x##_read8; \
> (uncore)->funcs.mmio_readw = x##_read16; \
> @@ -1276,6 +1277,17 @@ do { \
> (uncore)->funcs.mmio_readq = x##_read64; \
> } while (0)
>
> +#define ASSIGN_WRITE_MMIO_VFUNCS(uncore, x) \
> +do { \
> + ASSIGN_WRITE_MMIO_VFUNCS_NO_FW((uncore), x); \
> + (uncore)->funcs.write_fw_domains = x##_reg_write_fw_domains; \
> +} while (0)
> +
> +#define ASSIGN_READ_MMIO_VFUNCS(uncore, x) \
> +do { \
> + ASSIGN_READ_MMIO_VFUNCS_NO_FW(uncore, x); \
> + (uncore)->funcs.read_fw_domains = x##_reg_read_fw_domains; \
> +} while (0)
>
> static void fw_domain_init(struct intel_uncore *uncore,
> enum forcewake_domain_id domain_id,
> @@ -1559,11 +1571,11 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>
> if (!intel_uncore_has_forcewake(uncore)) {
> if (IS_GEN(i915, 5)) {
> - ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
> - ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
> + ASSIGN_WRITE_MMIO_VFUNCS_NO_FW(uncore, gen5);
> + ASSIGN_READ_MMIO_VFUNCS_NO_FW(uncore, gen5);
> } else {
> - ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
> - ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
> + ASSIGN_WRITE_MMIO_VFUNCS_NO_FW(uncore, gen2);
> + ASSIGN_READ_MMIO_VFUNCS_NO_FW(uncore, gen2);
> }
> } else if (IS_GEN_RANGE(i915, 6, 7)) {
> ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
> @@ -1594,6 +1606,12 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
> ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
> }
>
> + /* make sure fw funcs are set if and only if we have fw*/
> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_get);
> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_put);
> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
> +
> if (HAS_FPGA_DBG_UNCLAIMED(i915))
> uncore->flags |= UNCORE_HAS_FPGA_DBG_UNCLAIMED;
>
> @@ -1866,62 +1884,6 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
> return ret;
> }
>
> -static enum forcewake_domains
> -intel_uncore_forcewake_for_read(struct intel_uncore *uncore,
> - i915_reg_t reg)
> -{
> - struct drm_i915_private *i915 = uncore_to_i915(uncore);
> - u32 offset = i915_mmio_reg_offset(reg);
> - enum forcewake_domains fw_domains;
> -
> - if (INTEL_GEN(i915) >= 11) {
> - fw_domains = __gen11_fwtable_reg_read_fw_domains(uncore, offset);
> - } else if (HAS_FWTABLE(i915)) {
> - fw_domains = __fwtable_reg_read_fw_domains(uncore, offset);
> - } else if (INTEL_GEN(i915) >= 6) {
> - fw_domains = __gen6_reg_read_fw_domains(uncore, offset);
> - } else {
> - /* on devices with FW we expect to hit one of the above cases */
> - if (intel_uncore_has_forcewake(uncore))
> - MISSING_CASE(INTEL_GEN(i915));
> -
> - fw_domains = 0;
> - }
> -
> - WARN_ON(fw_domains & ~uncore->fw_domains);
> -
> - return fw_domains;
> -}
> -
> -static enum forcewake_domains
> -intel_uncore_forcewake_for_write(struct intel_uncore *uncore,
> - i915_reg_t reg)
> -{
> - struct drm_i915_private *i915 = uncore_to_i915(uncore);
> - u32 offset = i915_mmio_reg_offset(reg);
> - enum forcewake_domains fw_domains;
> -
> - if (INTEL_GEN(i915) >= 11) {
> - fw_domains = __gen11_fwtable_reg_write_fw_domains(uncore, offset);
> - } else if (HAS_FWTABLE(i915) && !IS_VALLEYVIEW(i915)) {
> - fw_domains = __fwtable_reg_write_fw_domains(uncore, offset);
> - } else if (IS_GEN(i915, 8)) {
> - fw_domains = __gen8_reg_write_fw_domains(uncore, offset);
> - } else if (IS_GEN_RANGE(i915, 6, 7)) {
> - fw_domains = FORCEWAKE_RENDER;
> - } else {
> - /* on devices with FW we expect to hit one of the above cases */
> - if (intel_uncore_has_forcewake(uncore))
> - MISSING_CASE(INTEL_GEN(i915));
> -
> - fw_domains = 0;
> - }
> -
> - WARN_ON(fw_domains & ~uncore->fw_domains);
> -
> - return fw_domains;
> -}
> -
> /**
> * intel_uncore_forcewake_for_reg - which forcewake domains are needed to access
> * a register
> @@ -1948,10 +1910,12 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
> return 0;
>
> if (op & FW_REG_READ)
> - fw_domains = intel_uncore_forcewake_for_read(uncore, reg);
> + fw_domains = uncore->funcs.read_fw_domains(uncore, reg);
>
> if (op & FW_REG_WRITE)
> - fw_domains |= intel_uncore_forcewake_for_write(uncore, reg);
> + fw_domains |= uncore->funcs.write_fw_domains(uncore, reg);
> +
> + WARN_ON(fw_domains & ~uncore->fw_domains);
>
> return fw_domains;
> }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index d6af3de70121..72ef8b262930 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -70,6 +70,11 @@ struct intel_uncore_funcs {
> void (*force_wake_put)(struct intel_uncore *uncore,
> enum forcewake_domains domains);
>
> + enum forcewake_domains (*read_fw_domains)(struct intel_uncore *uncore,
> + i915_reg_t r);
> + enum forcewake_domains (*write_fw_domains)(struct intel_uncore *uncore,
> + i915_reg_t r);
> +
> u8 (*mmio_readb)(struct intel_uncore *uncore,
> i915_reg_t r, bool trace);
> u16 (*mmio_readw)(struct intel_uncore *uncore,
> diff --git a/drivers/gpu/drm/i915/selftests/mock_uncore.c b/drivers/gpu/drm/i915/selftests/mock_uncore.c
> index ff8999c63a12..343017215463 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_uncore.c
> @@ -41,6 +41,6 @@ __nop_read(64)
>
> void mock_uncore_init(struct intel_uncore *uncore)
> {
> - ASSIGN_WRITE_MMIO_VFUNCS(uncore, nop);
> - ASSIGN_READ_MMIO_VFUNCS(uncore, nop);
> + ASSIGN_WRITE_MMIO_VFUNCS_NO_FW(uncore, nop);
> + ASSIGN_READ_MMIO_VFUNCS_NO_FW(uncore, nop);
> }
>
More information about the Intel-gfx
mailing list