Re: [Intel-gfx] [PATCH v2 6/9] drm/i915/perf: Parse 64bit report header formats correctly

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

On Thu, 16 Feb 2023 16:58:47 -0800, Umesh Nerlige Ramappa wrote:
>
> Now that OA formats come in flavor of 64 bit reports, the report header
> has 64 bit report-id, timestamp, context-id and gpu-ticks fields. When
> filtering these reports, use the right width for these fields.
>
> Note that upper dword of context id is reserved, so squash lower dword
> only.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c       | 105 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_perf_types.h |   6 ++
>  2 files changed, 92 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 3306653c0b85..9715b964aa1e 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -441,6 +441,75 @@ static u32 gen7_oa_hw_tail_read(struct i915_perf_stream *stream)
>	return oastatus1 & GEN7_OASTATUS1_TAIL_MASK;
>  }
>
> +#define oa_report_header_64bit(__s) \
> +	((__s)->oa_buffer.format->header == HDR_64_BIT)
> +
> +static inline u64
> +oa_report_id(struct i915_perf_stream *stream, void *report)
> +{
> +	return oa_report_header_64bit(stream) ? *(u64 *)report : *(u32 *)report;
> +}
> +
> +static inline u64
> +oa_report_reason(struct i915_perf_stream *stream, void *report)
> +{
> +	return (oa_report_id(stream, report) >> OAREPORT_REASON_SHIFT) &
> +	       (GRAPHICS_VER(stream->perf->i915) == 12 ?
> +		OAREPORT_REASON_MASK_EXTENDED :
> +		OAREPORT_REASON_MASK);
> +}
> +
> +static inline void
> +oa_report_id_clear(struct i915_perf_stream *stream, u32 *report)
> +{
> +	if (oa_report_header_64bit(stream))
> +		*(u64 *)report = 0;
> +	else
> +		*report = 0;
> +}
> +
> +static inline bool
> +oa_report_ctx_invalid(struct i915_perf_stream *stream, void *report)
> +{
> +	return !(oa_report_id(stream, report) &
> +	       stream->perf->gen8_valid_ctx_bit) &&
> +	       GRAPHICS_VER(stream->perf->i915) <= 11;
> +}
> +
> +static inline u64
> +oa_timestamp(struct i915_perf_stream *stream, void *report)
> +{
> +	return oa_report_header_64bit(stream) ?
> +		*((u64 *)report + 1) :
> +		*((u32 *)report + 1);
> +}
> +
> +static inline void
> +oa_timestamp_clear(struct i915_perf_stream *stream, u32 *report)
> +{
> +	if (oa_report_header_64bit(stream))
> +		*(u64 *)&report[2] = 0;
> +	else
> +		report[1] = 0;
> +}
> +
> +static inline u32
> +oa_context_id(struct i915_perf_stream *stream, u32 *report)
> +{
> +	u32 ctx_id = oa_report_header_64bit(stream) ? report[4] : report[2];
> +
> +	return ctx_id & stream->specific_ctx_id_mask;
> +}
> +
> +static inline void
> +oa_context_id_squash(struct i915_perf_stream *stream, u32 *report)
> +{
> +	if (oa_report_header_64bit(stream))
> +		report[4] = INVALID_CTX_ID;
> +	else
> +		report[2] = INVALID_CTX_ID;
> +}

Let's drop 'inline' from all these functions, the convention is to let
compiler decide to inline.

> +
>  /**
>   * oa_buffer_check_unlocked - check for data and update tail ptr state
>   * @stream: i915 stream instance
> @@ -521,9 +590,10 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
>		 * If not : (╯°□°）╯︵ ┻━┻
>		 */

Might as well fix up the comment above to say report_id and timestamp
instead of dword 0 and 1.

With the above changes, this is:

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