[Intel-gfx] [PATCH v3] drm/i915/bxt: Broxton decoupled MMIO

[Praveen Paneri]

Hi Chris,

On Wednesday 05 October 2016 01:26 AM, Chris Wilson wrote:
> On Tue, Oct 04, 2016 at 09:16:06PM +0530, Praveen Paneri wrote:
>> +#define HAS_DECOUPLED_MMIO(dev) (INTEL_INFO(dev)->has_decoupled_mmio \
>> +		&& IS_BXT_REVID(dev, BXT_REVID_C0, REVID_FOREVER))
>
> Edit dev_priv->info.has_decoupled_mmio on init.
Can I add this check directly into __intel_uncore_early_sanitize(), like 
below?

@@ -419,6 +419,10 @@ static void __intel_uncore_early_sanitize(struct 
drm_i915_private *dev_priv,
                                    GT_FIFO_CTL_RC6_POLICY_STALL);
         }

+       /* Enable Decoupled MMIO only on BXT C stepping onwards */
+       if (!IS_BXT_REVID(dev_priv, BXT_REVID_C0, REVID_FOREVER))
+               info->has_decoupled_mmio = 0;
+
         intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
  }

>
>> +static void __gen9_decoupled_mmio_access(struct drm_i915_private *dev_priv,
>> +					 u32 reg,
>> +					 enum forcewake_domains fw_engine,
>> +					 enum decoupled_ops operation)
>> +{
>> +	enum decoupled_power_domains dpd_engine;
>> +	u32 ctrl_reg_data = 0;
>> +
>> +	dpd_engine = fw2dpd_engine[fw_engine - 1];
>
> 	enum decoupled_power_domains dpd = fw2dpd_engine[fw_engine - 1];
>
> enum decoupled_power_domain
>
> And don't call it fw_engine. fw_domain, if you must.
I can change it but related existing code still uses fw_engine. Wouldn't 
it look out of the place?

Thanks,
Praveen
>
>> +
>> +	ctrl_reg_data |= reg;
>> +	ctrl_reg_data |= (operation << GEN9_DECOUPLED_OP_SHIFT);
>> +	ctrl_reg_data |= (dpd_engine << GEN9_DECOUPLED_PD_SHIFT);
>> +	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1, ctrl_reg_data);
>> +
>> +	ctrl_reg_data |= GEN9_DECOUPLED_DW1_GO;
>> +	__raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1, ctrl_reg_data);
>> +
>> +	if (wait_for_atomic((__raw_i915_read32(dev_priv,
>> +			GEN9_DECOUPLED_REG0_DW1) & GEN9_DECOUPLED_DW1_GO) == 0,
>> +			FORCEWAKE_ACK_TIMEOUT_MS))
>> +		DRM_ERROR("Decoupled MMIO wait timed out\n");
>> +}
>> +
>> +static inline u32 __gen9_decoupled_mmio_read(struct drm_i915_private *dev_priv,
>> +                                      u32 reg,
>> +                                      enum forcewake_domains fw)
>
> __gen9_decoupeld_mmio_read32()
>
>> +{
>> +	__gen9_decoupled_mmio_access(dev_priv,
>> +			reg,
>> +			fw_engine,
>> +			GEN9_DECOUPLED_OP_READ);
>
> 	__gen9_decoupled_mmio_access(dev_priv, reg, fw, GEN9_DECOUPLED_OP_READ);
>
>> +
>> +	return __raw_i915_read32(dev_priv,
>> +			GEN9_DECOUPLED_REG0_DW0);
>
> Everywhere! Please be careful with alignment.
>
>> +#define __gen9_decoupled_read(x) \
>> +static u##x \
>> +gen9_decoupled_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
>> +	enum forcewake_domains fw_engine; \
>> +	GEN6_READ_HEADER(x); \
>> +	fw_engine = __fwtable_reg_read_fw_domains(offset); \
>> +	if (!fw_engine || !(fw_engine & ~dev_priv->uncore.fw_domains_active)) { \
>> +		val = __raw_i915_read##x(dev_priv, reg); \
>> +	} else { \
>> +		unsigned i; \
>> +		u32 *ptr_data = (u32 *) &val; \
>> +		for (i = 0; i < x/32; i++, offset += sizeof(u32), ptr_data++) \
>> +			*ptr_data = __gen9_decoupled_mmio_read(dev_priv, \
>> +						     offset, \
>> +						     fw_engine); \
>> +	} \
>> +	GEN6_READ_FOOTER; \
>> +}
>
> Reverse it,
>
> 	if (domain & ~dev_priv->uncore.fw_domains_active) {
> 		u32 *ptr = (u32 *)&val;
> 		unsigned i;
>
> 		for (i = 0; i < x/32; i++, offset += sizeof(u32), ptr++)
> 			*ptr = __gen9_decoupled_mmio_read32(dev_priv, offset, domain);
> 	} else {
> 		val = __raw_i915_read##x(dev_priv, reg);
> 	}
>