Re: [Intel-gfx] [PATCH v3 04/16] drm/i915/perf: Determine gen12 oa ctx offset at runtime

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 10 Oct 2022 11:14:22 -0700, Umesh Nerlige Ramappa wrote:

Hi Umesh,

> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index cd57b5836386..b292aa39633e 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1358,6 +1358,68 @@ static int gen12_get_render_context_id(struct i915_perf_stream *stream)
>	return 0;
>  }
>
> +#define valid_oactxctrl_offset(x) ((x) && (x) != U32_MAX)
> +static bool oa_find_reg_in_lri(u32 *state, u32 reg, u32 *offset, u32 end)
> +{
> +	u32 idx = *offset;
> +	u32 len = min(MI_LRI_LEN(state[idx]) + idx, end);
> +	bool found = false;

My recommendation would be to put something like this here:

	if (MI_LRI_LEN(state[idx]) & 0x1)
		drm_warn("MI_LRI instruction with odd length\n");

Because we expect the MI_LRI length to even and I am not sure what is in
the context image. But maybe not needed?

> @@ -2436,15 +2514,18 @@ static int gen12_configure_oar_context(struct i915_perf_stream *stream,
>		},
>	};
>
> -	/* Modify the context image of pinned context with regs_context*/
> -	err = intel_context_lock_pinned(ce);
> -	if (err)
> -		return err;
> +	/* Modify the context image of pinned context with regs_context */
> +	if (valid_oactxctrl_offset(offset)) {

As I said before, this check is not needed, if we didn't have valid a
offset we should return error from oa_get_render_ctx_id. For if we have
this check and offset is not valid, can we skip this code and will things
still work? Or do we need to return an error from
gen12_configure_oar_context?

> +		err = intel_context_lock_pinned(ce);
> +		if (err)
> +			return err;
>
> -	err = gen8_modify_context(ce, regs_context, ARRAY_SIZE(regs_context));
> -	intel_context_unlock_pinned(ce);
> -	if (err)
> -		return err;
> +		err = gen8_modify_context(ce, regs_context,
> +					  ARRAY_SIZE(regs_context));
> +		intel_context_unlock_pinned(ce);
> +		if (err)
> +			return err;
> +	}

With the if statement removed or handled otherwise, this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>