Re: [PATCH v8 3/7] drm/xe/eustall: Implement EU stall sampling APIs for Xe_HPC

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

On Wed, 15 Jan 2025 12:02:09 -0800, Harish Chegondi wrote:
>

Hi Harish,

Still reviewing but here's the next set of review comments. Since this is a
huge 800 line patch, I'll just keep sending comments in email about section
of the code I am able to review, so that you can continue to work on
addressing the review comments in parallel.

I will also drop most people on the Cc: from these emails in the
future. Let them read the email on the intel-xe mailing list if interested.

> +static u32
> +gen_eustall_base(struct xe_eu_stall_data_stream *stream, bool enable)
> +{
> +	u32 val = xe_bo_ggtt_addr(stream->bo);
> +	u32 sz;
> +
> +	XE_WARN_ON(!IS_ALIGNED(val, 64));
> +
> +	switch (stream->per_xecore_buf_size) {
> +	case SZ_128K:
> +		sz = 0;
> +		break;
> +	case SZ_256K:
> +		sz = 1;
> +		break;
> +	case SZ_512K:
> +		sz = 2;
> +		break;

Hmm this switch statement is not needed. This is just:

	sz = stream->per_xecore_buf_size / SZ_256K;

> +	default:
> +		xe_gt_warn(stream->gt, "Missing case per XeCore buffer size == %lu)\n",
> +			   (long)(stream->per_xecore_buf_size));
> +		sz = 2;

If you need this check, can we use BUILD_BUG_ON(per_xecore_buf_size ...)
here? Or skip the check, this is an internal variable, we don't get this
from userspace, correct?

> +/**
> + * xe_eu_stall_stream_open_locked - Open a EU stall data stream FD.
> + * @dev: drm device instance
> + * @props: individually validated u64 property value pairs
> + * @file: drm file
> + *
> + * Returns: zero on success or a negative error code.
> + */
> +static int
> +xe_eu_stall_stream_open_locked(struct drm_device *dev,
> +			       struct eu_stall_open_properties *props,
> +			       struct drm_file *file)
> +{
> +	struct xe_device *xe = to_xe_device(dev);
> +	struct xe_eu_stall_data_stream *stream;
> +	struct xe_gt *gt = props->gt;
> +	unsigned long f_flags = 0;
> +	int ret, stream_fd;
> +
> +	if (!has_eu_stall_sampling_support(xe)) {

Wondering if this should be a bit in xe_graphics_desc? Anyway we can live
with this for now.

Also why is this check here in locked()? It should be the first check in
xe_eu_stall_stream_open?

> +		xe_gt_dbg(gt, "EU stall monitoring is not supported on this platform\n");
> +		return -EPERM;
> +	}
> +
> +	if (xe_observation_paranoid && !perfmon_capable()) {

This check should also be xe_eu_stall_stream_open.

Please reorder the checks and distribute them correctly between open and
open _locked. locked() should only contain checks where we need to hold the
lock. See xe_oa.c for similar checks there.

I will review the error rewinding etc in the next rev since it will change
with the reordering.

> @@ -252,10 +964,15 @@ int xe_eu_stall_stream_open(struct drm_device *dev,
>  {
>	struct xe_device *xe = to_xe_device(dev);
>	struct eu_stall_open_properties props;
> -	int ret, stream_fd;
> +	int ret;
>
>	memset(&props, 0, sizeof(struct eu_stall_open_properties));
>
> +	/* Set default values */
> +	props.gt = NULL;

Don't need? Already initialized to 0 above.

> +	props.eu_stall_sampling_rate = 4;

Why not call this sampling_rate_mult or something like that? eu_stall_ is
an unnecessary repetition, everything here is eu_stall!

> +	props.wait_num_reports = 1;
> +
>	ret = xe_eu_stall_user_extensions(xe, data, &props);
>	if (ret)
>		return ret;
> @@ -265,19 +982,9 @@ int xe_eu_stall_stream_open(struct drm_device *dev,
>		return -EINVAL;
>	}
>
> -	if (xe_observation_paranoid && !perfmon_capable()) {
> -		xe_gt_dbg(props.gt, "Insufficient privileges for EU stall monitoring\n");
> -		return -EACCES;
> -	}
> +	mutex_lock(&props.gt->eu_stall->lock);
> +	ret = xe_eu_stall_stream_open_locked(dev, &props, file);
> +	mutex_unlock(&props.gt->eu_stall->lock);
>
> -	if (!has_eu_stall_sampling_support(xe)) {
> -		xe_gt_dbg(props.gt, "EU stall monitoring is not supported on this platform\n");
> -		return -EPERM;
> -	}
> -	stream_fd = anon_inode_getfd("[xe_eu_stall]", &fops_eu_stall,
> -				     NULL, 0);
> -	if (stream_fd < 0)
> -		xe_gt_dbg(props.gt, "EU stall inode get fd failed : %d\n", stream_fd);
> -
> -	return stream_fd;
> +	return ret;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_eu_stall.h b/drivers/gpu/drm/xe/xe_eu_stall.h
> index 3447958a7a22..f97c8bf8e852 100644
> --- a/drivers/gpu/drm/xe/xe_eu_stall.h
> +++ b/drivers/gpu/drm/xe/xe_eu_stall.h
> @@ -6,6 +6,49 @@
>  #ifndef __XE_EU_STALL_H__
>  #define __XE_EU_STALL_H__
>
> +#include "xe_gt_types.h"
> +
> +struct per_xecore_buf {
> +	u8 *vaddr;
> +	u32 write;
> +	u32 read;
> +	/* lock to protect read and write pointers */
> +	struct mutex lock;

Let us call this buf_lock or ptr_lock or something like that.

I don't like generic names like 'lock' because it makes it really hard to
search the code for the locks when they are just named 'lock'.

> +};
> +
> +/**
> + * struct xe_eu_stall_data_stream - state of EU stall data stream FD
> + */
> +struct xe_eu_stall_data_stream {
> +	bool pollin;
> +	bool enabled;
> +	u64 poll_period;
> +	u32 wait_num_reports;
> +	size_t per_xecore_buf_size;
> +	wait_queue_head_t poll_wq;
> +
> +	struct xe_gt *gt;
> +	struct xe_bo *bo;
> +	struct per_xecore_buf *xecore_buf;
> +	struct {
> +		xe_dss_mask_t mask;
> +	} data_drop;
> +	struct hrtimer poll_check_timer;
> +	struct work_struct buf_check_work;
> +	struct workqueue_struct *buf_check_wq;
> +};
> +
> +struct xe_eu_stall_gt {
> +	/* Lock to protect stream */
> +	struct mutex lock;

Similarly here call this stream_lock or something like that.

Thanks.
--
Ashutosh