Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Prepare for error propagation

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

On 2/7/2022 11:01 PM, Lucas De Marchi wrote:
> Currently guc_mmio_reg_add() relies on having enough memory available in
> the array to add a new slot. It uses
> `GEM_BUG_ON(count >= regset->size);` to protect going above the
> threshold.
>
> In order to allow guc_mmio_reg_add() to handle the memory allocation by
> itself, it must return an error in case of failures.  Adjust return code
> so this error can be propagated to the callers of guc_mmio_reg_add() and
> guc_mmio_regset_init().
>
> No intended change in behavior.
>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 31 +++++++++++++---------
>   1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index e61150adcbe9..5290253b9132 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -226,8 +226,8 @@ static int guc_mmio_reg_cmp(const void *a, const void *b)
>   	return (int)ra->offset - (int)rb->offset;
>   }
>   
> -static void guc_mmio_reg_add(struct temp_regset *regset,
> -			     u32 offset, u32 flags)
> +static long __must_check guc_mmio_reg_add(struct temp_regset *regset,
> +					  u32 offset, u32 flags)
>   {
>   	u32 count = regset->used;
>   	struct guc_mmio_reg reg = {
> @@ -246,7 +246,7 @@ static void guc_mmio_reg_add(struct temp_regset *regset,
>   	 */
>   	if (bsearch(&reg, regset->registers, count,
>   		    sizeof(reg), guc_mmio_reg_cmp))
> -		return;
> +		return 0;
>   
>   	slot = &regset->registers[count];
>   	regset->used++;
> @@ -259,6 +259,8 @@ static void guc_mmio_reg_add(struct temp_regset *regset,
>   
>   		swap(slot[1], slot[0]);
>   	}
> +
> +	return 0;
>   }
>   
>   #define GUC_MMIO_REG_ADD(regset, reg, masked) \
> @@ -266,32 +268,35 @@ static void guc_mmio_reg_add(struct temp_regset *regset,
>   			 i915_mmio_reg_offset((reg)), \
>   			 (masked) ? GUC_REGSET_MASKED : 0)
>   
> -static void guc_mmio_regset_init(struct temp_regset *regset,
> -				 struct intel_engine_cs *engine)
> +static int guc_mmio_regset_init(struct temp_regset *regset,
> +				struct intel_engine_cs *engine)
>   {
>   	const u32 base = engine->mmio_base;
>   	struct i915_wa_list *wal = &engine->wa_list;
>   	struct i915_wa *wa;
>   	unsigned int i;
> +	int ret = 0;
>   
>   	regset->used = 0;
>   
> -	GUC_MMIO_REG_ADD(regset, RING_MODE_GEN7(base), true);
> -	GUC_MMIO_REG_ADD(regset, RING_HWS_PGA(base), false);
> -	GUC_MMIO_REG_ADD(regset, RING_IMR(base), false);
> +	ret |= GUC_MMIO_REG_ADD(regset, RING_MODE_GEN7(base), true);
> +	ret |= GUC_MMIO_REG_ADD(regset, RING_HWS_PGA(base), false);
> +	ret |= GUC_MMIO_REG_ADD(regset, RING_IMR(base), false);

I was thinking we could modify this to break after the first error 
because if a realloc fails it's unlikely a second one immediately after 
will succeed, but on the other side it doesn't really matter and the 
code is simpler like this, so:

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

Daniele

>   
>   	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> -		GUC_MMIO_REG_ADD(regset, wa->reg, wa->masked_reg);
> +		ret |= GUC_MMIO_REG_ADD(regset, wa->reg, wa->masked_reg);
>   
>   	/* Be extra paranoid and include all whitelist registers. */
>   	for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++)
> -		GUC_MMIO_REG_ADD(regset,
> -				 RING_FORCE_TO_NONPRIV(base, i),
> -				 false);
> +		ret |= GUC_MMIO_REG_ADD(regset,
> +					RING_FORCE_TO_NONPRIV(base, i),
> +					false);
>   
>   	/* add in local MOCS registers */
>   	for (i = 0; i < GEN9_LNCFCMOCS_REG_COUNT; i++)
> -		GUC_MMIO_REG_ADD(regset, GEN9_LNCFCMOCS(i), false);
> +		ret |= GUC_MMIO_REG_ADD(regset, GEN9_LNCFCMOCS(i), false);
> +
> +	return ret ? -1 : 0;
>   }
>   
>   static int guc_mmio_reg_state_query(struct intel_guc *guc)