Re: [Intel-xe] [PATCH v3 2/2] drm/xe/pmu: Enable PMU interface

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

On Wed, 09 Aug 2023 04:39:58 -0700, Iddamsetty, Aravind wrote:

Hi Aravind,

> On 09-08-2023 13:16, Iddamsetty, Aravind wrote:
> > On 09-08-2023 12:58, Dixit, Ashutosh wrote:
> >> On Tue, 08 Aug 2023 04:54:36 -0700, Aravind Iddamsetty wrote:
> >>> diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/xe_pmu_types.h
> >>> new file mode 100644
> >>> index 000000000000..a950c892e364
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/xe/xe_pmu_types.h
> >>> @@ -0,0 +1,76 @@
> >>> +/* SPDX-License-Identifier: MIT */
> >>> +/*
> >>> + * Copyright © 2023 Intel Corporation
> >>> + */
> >>> +
> >>> +#ifndef _XE_PMU_TYPES_H_
> >>> +#define _XE_PMU_TYPES_H_
> >>> +
> >>> +#include <linux/perf_event.h>
> >>> +#include <linux/spinlock_types.h>
> >>> +#include <uapi/drm/xe_drm.h>
> >>> +
> >>> +enum {
> >>> +	__XE_SAMPLE_RENDER_GROUP_BUSY,
> >>> +	__XE_SAMPLE_COPY_GROUP_BUSY,
> >>> +	__XE_SAMPLE_MEDIA_GROUP_BUSY,
> >>> +	__XE_SAMPLE_ANY_ENGINE_GROUP_BUSY,
> >>> +	__XE_NUM_PMU_SAMPLERS
> >>> +};
> >>> +
> >>> +#define XE_MAX_GT_PER_TILE 2
> >>> +
> >>> +struct xe_pmu {
> >>> +	/**
> >>> +	 * @cpuhp: Struct used for CPU hotplug handling.
> >>> +	 */
> >>> +	struct {
> >>> +		struct hlist_node node;
> >>> +		unsigned int cpu;
> >>> +	} cpuhp;
> >>> +	/**
> >>> +	 * @base: PMU base.
> >>> +	 */
> >>> +	struct pmu base;
> >>> +	/**
> >>> +	 * @closed: xe is unregistering.
> >>> +	 */
> >>> +	bool closed;
> >>> +	/**
> >>> +	 * @name: Name as registered with perf core.
> >>> +	 */
> >>> +	const char *name;
> >>> +	/**
> >>> +	 * @lock: Lock protecting enable mask and ref count handling.
> >>> +	 */
> >>> +	spinlock_t lock;
> >>> +	/**
> >>> +	 * @sample: Current and previous (raw) counters.
> >>> +	 *
> >>> +	 * These counters are updated when the device is awake.
> >>> +	 *
> >>> +	 */
> >>> +	u64 sample[XE_MAX_GT_PER_TILE][__XE_NUM_PMU_SAMPLERS];
> >>
> >> s/XE_MAX_GT_PER_TILE/XE_MAX_GT/ since the PMU is for the entire device not
> >> per tile, as I mentioned earlier.
> >
> > right, so for a device this shall be sample[XE_MAX_TILES_PER_DEVICE *
> > XE_MAX_GT_PER_TILE][__XE_NUM_PMU_SAMPLERS]
>
> on further checking based on (d714e2b698d8 drm/xe: Introduce xe_tile)
> the XE_MAX_TILES_PER_DEVICE is being considered as MAX_GT as well so
> will use similar to that here.

OK, but just to point out the all the code is written in terms of gt's only
(not tiles) so IMO we should keep tiles out of this array definition. Also
note that gt id's are unique, across tiles. That is why I am suggesting
XE_MAX_GT (since gt id's would go from 0 to (XE_MAX_GT - 1)) (irrespective
of the number of tiles or the tiles gt's are on).

Thanks.
--
Ashutosh