[PATCH 2/2] drm/xe/oa/uapi: Allow preemption to be disabled on the stream exec queue

[Upadhyay, Tejas]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: Wednesday, June 26, 2024 1:22 PM
> To: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Brost, Matthew
> <matthew.brost at intel.com>; Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Subject: Re: [PATCH 2/2] drm/xe/oa/uapi: Allow preemption to be disabled on
> the stream exec queue
> 
> On Tue, 25 Jun 2024 20:07:31 -0700, Umesh Nerlige Ramappa wrote:
> >
> 
> Hi Umesh (and also Matt),
> 
> > On Tue, Jun 25, 2024 at 07:13:43PM -0700, Ashutosh Dixit wrote:
> > > Mesa VK_KHR_performance_query use case requires preemption and
> > > timeslicing to be disabled for the stream exec queue. Implement this
> > > functionality here.
> > >
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_oa.c       | 70
> +++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/xe/xe_oa_types.h |  3 ++
> > > include/uapi/drm/xe_drm.h        |  6 +++
> > > 3 files changed, 78 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > > index a68659fd5386..1147035af039 100644
> > > --- a/drivers/gpu/drm/xe/xe_oa.c
> > > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > > @@ -80,6 +80,7 @@ struct xe_oa_open_param {
> > >	int engine_instance;
> > >	struct xe_exec_queue *exec_q;
> > >	struct xe_hw_engine *hwe;
> > > +	bool no_preempt;
> > > };
> > >
> > > struct xe_oa_config_bo {
> > > @@ -1013,11 +1014,55 @@ static void xe_oa_stream_disable(struct
> xe_oa_stream *stream)
> > >		hrtimer_cancel(&stream->poll_check_timer);
> > > }
> > >
> > > +static int xe_oa_enable_preempt_timeslice(struct xe_oa_stream
> > > +*stream) {
> > > +	struct xe_exec_queue *q = stream->exec_q;
> > > +	int ret1, ret2;
> > > +
> > > +	/* Best effort recovery: try to revert both to original, irrespective of
> error */
> > > +	ret1 = q->ops->set_timeslice(q, stream->hwe->eclass-
> >sched_props.timeslice_us);
> > > +	ret2 = q->ops->set_preempt_timeout(q, stream->hwe->eclass-
> >sched_props.preempt_timeout_us);
> > > +	if (ret1 || ret2)
> > > +		goto err;
> > > +	return 0;
> > > +err:
> > > +	drm_dbg(&stream->oa->xe->drm, "%s failed %d\n", __func__, ret1 ?:
> > > +ret2);
> >
> > Maybe print both rets or something that allows us to know which one
> failed.
> >
> > drm_dbg(&stream->oa->xe->drm, "%s failed ret1 = %d, ret2 = %d\n",
> > __func__, ret1, ret2);
> 
> Sure.

I think its better to handle each return value with dbg message in its own branch with custom message, or append existing dbg message with some readble string based on return value. For example,
drm_dbg(&stream->oa->xe->drm, "set %s failed ret= %d\n", __func__, ret1? "timesliec":"preemt", ret1?:ret2);

> 
> >
> > > +	return ret1 ?: ret2;
> > > +}
> > > +
> > > +static int xe_oa_disable_preempt_timeslice(struct xe_oa_stream
> > > +*stream) {
> > > +	struct xe_exec_queue *q = stream->exec_q;
> > > +	int ret;
> > > +
> > > +	/* Setting values to 0 will disable timeslice and preempt_timeout */
> > > +	ret = q->ops->set_timeslice(q, 0);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > > +	ret = q->ops->set_preempt_timeout(q, 0);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > > +	return 0;
> > > +err:
> > > +	xe_oa_enable_preempt_timeslice(stream);
> > > +	drm_dbg(&stream->oa->xe->drm, "%s failed %d\n", __func__, ret);
> > > +	return ret;
> >
> > 1) I think the same parameters are also exposed via sysfs and they may
> > be applied at the engine or class level by the user. I believe context
> > level granularity is only available to KMD.
> 
> Correct.

Yes, context level granularity is with KMD only during context creation.

> 
> > Hence, for the duration of the OA use case (from open to close of the
> > stream fd), how do you make sure that someone else is not undoing what
> > you did here?  I don't see anyone calling these hooks though. The only
> > place where these params are applied to the context are when the
> > context is registered (likely when user creates the queue). Better to
> > have someone from GuC team or Matt confirm that this is okay as is.
> 
> Correct, this implemenation is assuming that at present there are no other
> callers of set_timeslice() and set_preempt_timeout() ops (except when the
> exec queue is created, which is prior to what is happening here). So the
> question of "someone else undoing what is done here" only arises if there
> were other callers. And also, as you observed, exec queue properties are not
> directly modified via sysfs, sysfs only modifies the engine class attributes. So
> IMO we are good here till other callers of those ops appear and I think it is
> better to handle those scenarios at that time?
> 
> > 2) The other aspect I worry about is synchronization between sysfs
> > modifying the underlying scheduling params and OA trying to modify
> > them at the some other time. The set hooks themselves don't use any
> > locks and I think it is because these params are modified in select
> > places where concurrency may not be an issue. With OA, we may have to
> > do some sync OR at least rule out potential concurrency issues. Again,
> > we should understand when these params may be applied outside of OA.
> 
> Actually, I agree, this is good point and there is a concurrency issue. But it is
> not specific to OA. The issue already exists when an exec queue is created
> (sysfs is written to, concurrently with __xe_exec_queue_alloc). So rightfully
> some locking should be introduced to handle this. We can decide if we want
> to do this before merging this patch, or do it later since it is a pre-existing
> issue (it would need to be done in all places where concurrent read/write's of
> sysfs written values are happening).
> 
> @Brost, Matthew any comments? Also Cc: @@Upadhyay, Tejas.

What we set through user level is context specific setting and what sysfs sets is engine class specific, both are different. However KMD takes first engine class specific timeout setting and then on top of that user extension properties are applied, so I think no sync up issue here.

Thanks,
Tejas
> 
> Thanks.
> --
> Ashutosh
> 
> 
> >
> > Thanks,
> > Umesh
> >
> > > +}
> > > +
> > > static int xe_oa_enable_locked(struct xe_oa_stream *stream)  {
> > >	if (stream->enabled)
> > >		return 0;
> > >
> > > +	if (stream->no_preempt) {
> > > +		int ret = xe_oa_disable_preempt_timeslice(stream);
> > > +
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >	xe_oa_stream_enable(stream);
> > >
> > >	stream->enabled = true;
> > > @@ -1026,13 +1071,18 @@ static int xe_oa_enable_locked(struct
> > >xe_oa_stream *stream)
> > >
> > > static int xe_oa_disable_locked(struct xe_oa_stream *stream) {
> > > +	int ret = 0;
> > > +
> > >	if (!stream->enabled)
> > >		return 0;
> > >
> > >	xe_oa_stream_disable(stream);
> > >
> > > +	if (stream->no_preempt)
> > > +		ret = xe_oa_enable_preempt_timeslice(stream);
> > > +
> > >	stream->enabled = false;
> > > -	return 0;
> > > +	return ret;
> > > }
> > >
> > > static long xe_oa_config_locked(struct xe_oa_stream *stream, u64
> > >arg)  @@ -1307,6 +1357,7 @@ static int xe_oa_stream_init(struct
> xe_oa_stream *stream,
> > >	stream->sample = param->sample;
> > >	stream->periodic = param->period_exponent > 0;
> > >	stream->period_exponent = param->period_exponent;
> > > +	stream->no_preempt = param->no_preempt;
> > >
> > >	/*
> > >	 * For Xe2+, when overrun mode is enabled, there are no partial
> > >reports at the end  @@ -1651,6 +1702,13 @@ static int
> xe_oa_set_prop_engine_instance(struct xe_oa *oa, u64 value,
> > >	return 0;
> > > }
> > >
> > > +static int xe_oa_set_no_preempt(struct xe_oa *oa, u64 value,
> > > +				struct xe_oa_open_param *param) {
> > > +	param->no_preempt = value;
> > > +	return 0;
> > > +}
> > > +
> > > typedef int (*xe_oa_set_property_fn)(struct xe_oa *oa, u64 value,
> > >				     struct xe_oa_open_param *param);  static
> const
> > >xe_oa_set_property_fn xe_oa_set_property_funcs[] = {  @@ -1662,6
> > >+1720,7 @@ static const xe_oa_set_property_fn
> xe_oa_set_property_funcs[] = {
> > >	[DRM_XE_OA_PROPERTY_OA_DISABLED] = xe_oa_set_prop_disabled,
> > >	[DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID] =
> xe_oa_set_prop_exec_queue_id,
> > >	[DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE] =
> > >xe_oa_set_prop_engine_instance,
> > > +	[DRM_XE_OA_PROPERTY_NO_PREEMPT] = xe_oa_set_no_preempt,
> > > };
> > >
> > > static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64
> > >extension,  @@ -1766,6 +1825,15 @@ int xe_oa_stream_open_ioctl(struct
> drm_device *dev, u64 data, struct drm_file *f
> > >	if (param.exec_q && !param.sample)
> > >		privileged_op = false;
> > >
> > > +	if (param.no_preempt) {
> > > +		if (!param.exec_q) {
> > > +			drm_dbg(&oa->xe->drm, "Preemption disable
> without exec_q!\n");
> > > +			ret = -EINVAL;
> > > +			goto err_exec_q;
> > > +		}
> > > +		privileged_op = true;
> > > +	}
> > > +
> > >	if (privileged_op && xe_perf_stream_paranoid &&
> !perfmon_capable()) {
> > >		drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe
> perf stream\n");
> > >		ret = -EACCES;
> > > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h
> > >b/drivers/gpu/drm/xe/xe_oa_types.h
> > > index 706d45577dae..540c3ec53a6d 100644
> > > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > > @@ -235,5 +235,8 @@ struct xe_oa_stream {
> > >
> > >	/** @oa_status: temporary storage for oa_status register value */
> > >	u32 oa_status;
> > > +
> > > +	/** @no_preempt: Whether preemption and timeslicing is disabled
> for stream exec_q */
> > > +	u32 no_preempt;
> > > };
> > > #endif
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index b410553faa9b..12eaa8532b5c 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -1611,6 +1611,12 @@ enum drm_xe_oa_property_id {
> > >	 * pass along with @DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID or will
> default to 0.
> > >	 */
> > >	DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE,
> > > +
> > > +	/**
> > > +	 * @DRM_XE_OA_PROPERTY_NO_PREEMPT: Allow preemption and
> timeslicing
> > > +	 * to be disabled for the stream exec queue.
> > > +	 */
> > > +	DRM_XE_OA_PROPERTY_NO_PREEMPT,
> > > };
> > >
> > > /**
> > > --
> > > 2.41.0
> > >