Re: [PATCH 2/3] drm/panfrost: Expose HW counters to userspace

[Alyssa Rosenzweig <alyssa@rosenzweig.io>]

> +	/*
> +	 * In v4 HW we have one tiler per core group, with the number
> +	 * of core groups being equal to the number of L2 caches. Other
> +	 * HW versions just have one tiler and the number of L2 caches
> +	 * can be extracted from the mem_features field.
> +	 */

Normalizing layout in kernel sounds like a good idea to me, nice one :)
(kbase does not and userspace is messy as a result)

> +/*
> + * Returns true if the 2 jobs have exactly the same perfcnt context, false
> + * otherwise.
> + */
> +static bool panfrost_perfcnt_job_ctx_cmp(struct panfrost_perfcnt_job_ctx *a,
> +					 struct panfrost_perfcnt_job_ctx *b)
> +{
> +	unsigned int i, j;
> +
> +	if (a->perfmon_count != b->perfmon_count)
> +		return false;
> +
> +	for (i = 0; i < a->perfmon_count; i++) {
> +		for (j = 0; j < b->perfmon_count; j++) {
> +			if (a->perfmons[i] == b->perfmons[j])
> +				break;
> +		}
> +
> +		if (j == b->perfmon_count)
> +			return false;
> +	}
> +

Would using memcmp() be cleaner here?

> +	if (panfrost_model_cmp(pfdev, 0x1000) >= 0)

What does 0x1000 refer to here? I'm assuming maybe Bifrost, but it's not
obvious... probably better to have a #define somewhere and use that (or
an enum equivalently).

> +	/*
> +	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
> +	 * counters.
> +	 */

What on earth is PRLAM-8186? :)

Actually, wait, I can answer that -- old kbase versions had an errata
list:

        /* Write of PRFCNT_CONFIG_MODE_MANUAL to PRFCNT_CONFIG causes a instrumentation dump if
           PRFCNT_TILER_EN is enabled */
        BASE_HW_ISSUE_8186,

So that's why. If people want, I'm considering moving these errata
descriptions back into the kernel where possible, since otherwise code
like this is opaque.

> +		unsigned int nl2c, ncores;
> +
> +		/*
> +		 * TODO: define a macro to extract the number of l2 caches from
> +		 * mem_features.
> +		 */
> +		nl2c = ((pfdev->features.mem_features >> 8) & GENMASK(3, 0)) + 1;
> +
> +		/*
> +		 * The ARM driver is grouping cores per core group and then
> +		 * only using the number of cores in group 0 to calculate the
> +		 * size. Not sure why this is done like that, but I guess
> +		 * shader_present will only show cores in the first group
> +		 * anyway.
> +		 */
> +		ncores = hweight64(pfdev->features.shader_present);
> +

Deja vu. Was this copypaste dmaybe?

> +		  (panfrost_model_cmp(pfdev, 0x1000) >= 0 ?

THere's that pesky 0x1000 again.

> @@ -55,6 +63,15 @@ struct drm_panfrost_submit {
>  
>  	/** A combination of PANFROST_JD_REQ_* */
>  	__u32 requirements;
> +
> +	/** Pointer to a u32 array of perfmons that should be attached to the job. */
> +	__u64 perfmon_handles;
> +
> +	/** Number of perfmon handles passed in (size is that times 4). */
> +	__u32 perfmon_handle_count;
> +
> +	/** Unused field, should be set to 0. */
> +	__u32 padding;

Bleep blorp. If we're modifying _submit, we'll need to be swift about
merging this ahead of the main code to make sure we don't break the
UABI. Although I guess if we're just adding fields at the end, that's a
nonissue.

> +struct drm_panfrost_block_perfcounters {
> +	/*
> +	 * For DRM_IOCTL_PANFROST_GET_PERFCNT_LAYOUT, encodes the available
> +	 * instances for a specific given block type.
> +	 * For DRM_IOCTL_PANFROST_CREATE_PERFMON, encodes the instances the
> +	 * user wants to monitor.
> +	 * Note: the bitmap might be sparse.
> +	 */
> +	__u64 instances;
> +
> +	/*
> +	 * For DRM_IOCTL_PANFROST_GET_PERFCNT_LAYOUT, encodes the available
> +	 * counters attached to a specific block type.
> +	 * For DRM_IOCTL_PANFROST_CREATE_PERFMON, encodes the counters the user
> +	 * wants to monitor.
> +	 * Note: the bitmap might be sparse.
> +	 */
> +	__u64 counters;
> +};

I don't understand this. Aren't there more than 64 counters?

> +struct drm_panfrost_get_perfcnt_layout {
> +	struct drm_panfrost_block_perfcounters counters[PANFROST_NUM_BLOCKS];
> +};

--Oh. It's per-block. Got it.

> + * Used to create a performance monitor. Each perfmonance monitor is assigned an

Typo.

---

Overall, this looks really great! Thank you! :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel