Re: [PATCH v2 2/3] drm/i915/guc: Make scratch register base and count flexible

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 04 May 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> We are using some scratch registers in MMIO based send function.
> Make their base and count flexible in preparation of upcoming
> GuC firmware/hardware changes. While around, change cmd len
> parameter verification from WARN_ON to GEM_BUG_ON as we don't
> need this all the time.

I'm not generally fond of caching the registers like this or adding
_MMIO() wrapping outside of i915_reg.h. Sure, we have some of that here
and there, but here it's hard to see the rationale because you do this
in preparation for something that we you're not sharing.

BR,
Jani.

>
> v2: call out WARN/GEM_BUG change in the commit msg (Daniele)
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uc.c | 41 ++++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
>  2 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 72f49e6..9d11c42 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -260,9 +260,36 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  	__intel_uc_fw_fini(&dev_priv->huc.fw);
>  }
>  
> +static inline i915_reg_t guc_send_reg(struct intel_guc *guc, u32 i)
> +{
> +	GEM_BUG_ON(!guc->send_regs.base);
> +	GEM_BUG_ON(!guc->send_regs.count);
> +	GEM_BUG_ON(i >= guc->send_regs.count);
> +
> +	return _MMIO(guc->send_regs.base + 4 * i);
> +}
> +
> +static void guc_init_send_regs(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	enum forcewake_domains fw_domains = 0;
> +	u32 i;
> +
> +	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> +	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +
> +	for (i = 0; i < guc->send_regs.count; i++) {
> +		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> +					guc_send_reg(guc, i),
> +					FW_REG_READ | FW_REG_WRITE);
> +	}
> +	guc->send_regs.fw_domains = fw_domains;
> +}
> +
>  static int guc_enable_communication(struct intel_guc *guc)
>  {
>  	/* XXX: placeholder for alternate setup */
> +	guc_init_send_regs(guc);
>  	guc->send = intel_guc_send_mmio;
>  	return 0;
>  }
> @@ -407,19 +434,19 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	int i;
>  	int ret;
>  
> -	if (WARN_ON(len < 1 || len > 15))
> -		return -EINVAL;
> +	GEM_BUG_ON(!len);
> +	GEM_BUG_ON(len > guc->send_regs.count);
>  
>  	mutex_lock(&guc->send_mutex);
> -	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
>  
>  	dev_priv->guc.action_count += 1;
>  	dev_priv->guc.action_cmd = action[0];
>  
>  	for (i = 0; i < len; i++)
> -		I915_WRITE(SOFT_SCRATCH(i), action[i]);
> +		I915_WRITE(guc_send_reg(guc, i), action[i]);
>  
> -	POSTING_READ(SOFT_SCRATCH(i - 1));
> +	POSTING_READ(guc_send_reg(guc, i - 1));
>  
>  	intel_guc_notify(guc);
>  
> @@ -428,7 +455,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	 * Fast commands should still complete in 10us.
>  	 */
>  	ret = __intel_wait_for_register_fw(dev_priv,
> -					   SOFT_SCRATCH(0),
> +					   guc_send_reg(guc, 0),
>  					   INTEL_GUC_RECV_MASK,
>  					   INTEL_GUC_RECV_MASK,
>  					   10, 10, &status);
> @@ -450,7 +477,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>  	}
>  	dev_priv->guc.action_status = status;
>  
> -	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
> +	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
>  	mutex_unlock(&guc->send_mutex);
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 097289b..a37a8cc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -205,6 +205,13 @@ struct intel_guc {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  	uint32_t last_seqno[I915_NUM_ENGINES];
>  
> +	/* GuC's FW specific registers used in MMIO send */
> +	struct {
> +		u32 base;
> +		u32 count;
> +		u32 fw_domains; /* enum forcewake_domains */
> +	} send_regs;
> +
>  	/* To serialize the intel_guc_send actions */
>  	struct mutex send_mutex;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx