Re: [PATCH 11/19] drm/i915/perf: Store a pointer to oa_format in oa_buffer

[Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>]

On 23/08/2022 23:41, Umesh Nerlige Ramappa wrote:
> DG2 introduces OA reports with 64 bit report header fields. Perf OA
> would need more information about the OA format in order to process such
> reports. Store all OA format info in oa_buffer instead of just the size
> and format-id.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_perf.c       | 23 ++++++++++-------------
>   drivers/gpu/drm/i915/i915_perf_types.h |  3 +--
>   2 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index f7621b45966c..9e455bd3bce5 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -483,7 +483,7 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream)
>   static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
>   {
>   	u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
> -	int report_size = stream->oa_buffer.format_size;
> +	int report_size = stream->oa_buffer.format->size;
>   	unsigned long flags;
>   	bool pollin;
>   	u32 hw_tail;
> @@ -630,7 +630,7 @@ static int append_oa_sample(struct i915_perf_stream *stream,
>   			    size_t *offset,
>   			    const u8 *report)
>   {
> -	int report_size = stream->oa_buffer.format_size;
> +	int report_size = stream->oa_buffer.format->size;
>   	struct drm_i915_perf_record_header header;
>   	int report_size_partial;
>   	u8 *oa_buf_end;
> @@ -694,7 +694,7 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
>   				  size_t *offset)
>   {
>   	struct intel_uncore *uncore = stream->uncore;
> -	int report_size = stream->oa_buffer.format_size;
> +	int report_size = stream->oa_buffer.format->size;
>   	u8 *oa_buf_base = stream->oa_buffer.vaddr;
>   	u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
>   	size_t start_offset = *offset;
> @@ -970,7 +970,7 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream,
>   				  size_t *offset)
>   {
>   	struct intel_uncore *uncore = stream->uncore;
> -	int report_size = stream->oa_buffer.format_size;
> +	int report_size = stream->oa_buffer.format->size;
>   	u8 *oa_buf_base = stream->oa_buffer.vaddr;
>   	u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
>   	u32 mask = (OA_BUFFER_SIZE - 1);
> @@ -2517,7 +2517,7 @@ static int gen12_configure_oar_context(struct i915_perf_stream *stream,
>   {
>   	int err;
>   	struct intel_context *ce = stream->pinned_ctx;
> -	u32 format = stream->oa_buffer.format;
> +	u32 format = stream->oa_buffer.format->format;
>   	u32 offset = stream->perf->ctx_oactxctrl_offset;
>   	struct flex regs_context[] = {
>   		{
> @@ -2890,7 +2890,7 @@ static void gen7_oa_enable(struct i915_perf_stream *stream)
>   	u32 ctx_id = stream->specific_ctx_id;
>   	bool periodic = stream->periodic;
>   	u32 period_exponent = stream->period_exponent;
> -	u32 report_format = stream->oa_buffer.format;
> +	u32 report_format = stream->oa_buffer.format->format;
>   
>   	/*
>   	 * Reset buf pointers so we don't forward reports from before now.
> @@ -2916,7 +2916,7 @@ static void gen7_oa_enable(struct i915_perf_stream *stream)
>   static void gen8_oa_enable(struct i915_perf_stream *stream)
>   {
>   	struct intel_uncore *uncore = stream->uncore;
> -	u32 report_format = stream->oa_buffer.format;
> +	u32 report_format = stream->oa_buffer.format->format;
>   
>   	/*
>   	 * Reset buf pointers so we don't forward reports from before now.
> @@ -2942,7 +2942,7 @@ static void gen8_oa_enable(struct i915_perf_stream *stream)
>   static void gen12_oa_enable(struct i915_perf_stream *stream)
>   {
>   	struct intel_uncore *uncore = stream->uncore;
> -	u32 report_format = stream->oa_buffer.format;
> +	u32 report_format = stream->oa_buffer.format->format;
>   
>   	/*
>   	 * If we don't want OA reports from the OA buffer, then we don't even
> @@ -3184,15 +3184,12 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>   	stream->sample_flags = props->sample_flags;
>   	stream->sample_size += format_size;
>   
> -	stream->oa_buffer.format_size = format_size;
> -	if (drm_WARN_ON(&i915->drm, stream->oa_buffer.format_size == 0))
> +	stream->oa_buffer.format = &perf->oa_formats[props->oa_format];
> +	if (drm_WARN_ON(&i915->drm, stream->oa_buffer.format->size == 0))
>   		return -EINVAL;
>   
>   	stream->hold_preemption = props->hold_preemption;
>   
> -	stream->oa_buffer.format =
> -		perf->oa_formats[props->oa_format].format;
> -
>   	stream->periodic = props->oa_periodic;
>   	if (stream->periodic)
>   		stream->period_exponent = props->oa_period_exponent;
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index dc9bfd8086cf..e0c96b44eda8 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -250,11 +250,10 @@ struct i915_perf_stream {
>   	 * @oa_buffer: State of the OA buffer.
>   	 */
>   	struct {
> +		const struct i915_oa_format *format;
>   		struct i915_vma *vma;
>   		u8 *vaddr;
>   		u32 last_ctx_id;
> -		int format;
> -		int format_size;
>   		int size_exponent;
>   
>   		/**