Re: [PATCH v3 1/2] drm/i915/guc: Limit number of scratch registers used for H2G

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 18/10/18 11:30, Michal Wajdeczko wrote:
> We wrongly assumed that GuC is only using last scratch register
> for G2H messages, but in fact it is also using register [14] to
> report sleep state status. Remove that register from our H2G
> send registers pool.
> 
> v2: No message from host to GuC uses more than 8 registers and
> the GuC FW itself uses an 8-element array to store the H2G message,
> so we may reduce our send array to just 8 registers (Daniele)
> v3: use explicit define
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_guc.c      | 3 ++-
>   drivers/gpu/drm/i915/intel_guc_fwif.h | 5 ++++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 4c61eb9..9b00cdf 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -50,7 +50,8 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
>   	unsigned int i;
>   
>   	guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
> -	guc->send_regs.count = SOFT_SCRATCH_COUNT - 1;
> +	guc->send_regs.count = GUC_MMIO_MSG_LEN;
> +	BUILD_BUG_ON(GUC_MMIO_MSG_LEN > SOFT_SCRATCH_COUNT);
>   
>   	for (i = 0; i < guc->send_regs.count; i++) {
>   		fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index d1bbaba..cc997ef 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -601,7 +601,8 @@ struct guc_shared_ctx_data {
>    * registers, where first register holds data treated as message header,
>    * and other registers are used to hold message payload.
>    *
> - * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8
> + * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8, but
> + * the GuC FW itself uses an 8-element array to store the H2G message.

I think our justification here should be that no commands takes more 
than 8 dwords more than what GuC FW does internally (which can reinforce 
the point but not make it IMO).

>    *
>    *      +-----------+---------+---------+---------+
>    *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
> @@ -633,6 +634,8 @@ struct guc_shared_ctx_data {
>    *   field.
>    */
>   
> +#define GUC_MMIO_MSG_LEN		8
> +

I'd prefer GUC_MAX_MMIO_MSG_LEN, but I can live with this.

With the improved comment:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>   #define INTEL_GUC_MSG_TYPE_SHIFT	28
>   #define INTEL_GUC_MSG_TYPE_MASK		(0xF << INTEL_GUC_MSG_TYPE_SHIFT)
>   #define INTEL_GUC_MSG_DATA_SHIFT	16
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx