Re: [PATCH] drm/xe/oa/uapi: Expose an unblock after N reports OA property

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, Dec 13, 2024 at 05:08:18PM -0800, Dixit, Ashutosh wrote:
>On Fri, 13 Dec 2024 16:54:41 -0800, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> On Thu, Dec 12, 2024 at 02:49:03PM -0800, Ashutosh Dixit wrote:
>> > Expose an "unblock after N reports" OA property, to allow userspace threads
>> > to be woken up less frequently.
>> >
>> > Co-developed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_oa.c       | 30 ++++++++++++++++++++++++++----
>> > drivers/gpu/drm/xe/xe_oa_types.h |  3 +++
>> > drivers/gpu/drm/xe/xe_query.c    |  2 +-
>> > include/uapi/drm/xe_drm.h        |  7 +++++++
>> > 4 files changed, 37 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>> > index ec88b18e9baa2..56bf375a9d4bc 100644
>> > --- a/drivers/gpu/drm/xe/xe_oa.c
>> > +++ b/drivers/gpu/drm/xe/xe_oa.c
>> > @@ -97,6 +97,7 @@ struct xe_oa_open_param {
>> >	int num_syncs;
>> >	struct xe_sync_entry *syncs;
>> >	size_t oa_buffer_size;
>> > +	int wait_num_reports;
>> > };
>> >
>> > struct xe_oa_config_bo {
>> > @@ -241,11 +242,10 @@ static void oa_timestamp_clear(struct xe_oa_stream *stream, u32 *report)
>> > static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
>> > {
>> >	u32 gtt_offset = xe_bo_ggtt_addr(stream->oa_buffer.bo);
>> > +	u32 tail, hw_tail, partial_report_size, available;
>> >	int report_size = stream->oa_buffer.format->size;
>> > -	u32 tail, hw_tail;
>> >	unsigned long flags;
>> >	bool pollin;
>> > -	u32 partial_report_size;
>> >
>> >	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
>> >
>> > @@ -289,8 +289,8 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
>> >
>> >	stream->oa_buffer.tail = tail;
>> >
>> > -	pollin = xe_oa_circ_diff(stream, stream->oa_buffer.tail,
>> > -				 stream->oa_buffer.head) >= report_size;
>> > +	available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head);
>> > +	pollin = available >= stream->wait_num_reports * report_size;
>> >
>> >	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>> >
>> > @@ -1285,6 +1285,17 @@ static int xe_oa_set_prop_oa_buffer_size(struct xe_oa *oa, u64 value,
>> >	return 0;
>> > }
>> >
>> > +static int xe_oa_set_prop_wait_num_reports(struct xe_oa *oa, u64 value,
>> > +					   struct xe_oa_open_param *param)
>> > +{
>> > +	if (!value) {
>> > +		drm_dbg(&oa->xe->drm, "wait_num_reports %llu\n", value);
>> > +		return -EINVAL;
>> > +	}
>> > +	param->wait_num_reports = value;
>> > +	return 0;
>> > +}
>> > +
>> > static int xe_oa_set_prop_ret_inval(struct xe_oa *oa, u64 value,
>> >				    struct xe_oa_open_param *param)
>> > {
>> > @@ -1306,6 +1317,7 @@ static const xe_oa_set_property_fn xe_oa_set_property_funcs_open[] = {
>> >	[DRM_XE_OA_PROPERTY_NUM_SYNCS] = xe_oa_set_prop_num_syncs,
>> >	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user,
>> >	[DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE] = xe_oa_set_prop_oa_buffer_size,
>> > +	[DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS] = xe_oa_set_prop_wait_num_reports,
>> > };
>> >
>> > static const xe_oa_set_property_fn xe_oa_set_property_funcs_config[] = {
>> > @@ -1321,6 +1333,7 @@ static const xe_oa_set_property_fn xe_oa_set_property_funcs_config[] = {
>> >	[DRM_XE_OA_PROPERTY_NUM_SYNCS] = xe_oa_set_prop_num_syncs,
>> >	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user,
>> >	[DRM_XE_OA_PROPERTY_OA_BUFFER_SIZE] = xe_oa_set_prop_ret_inval,
>> > +	[DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS] = xe_oa_set_prop_ret_inval,
>> > };
>> >
>> > static int xe_oa_user_ext_set_property(struct xe_oa *oa, enum xe_oa_user_extn_from from,
>> > @@ -1797,6 +1810,7 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
>> >	stream->periodic = param->period_exponent > 0;
>> >	stream->period_exponent = param->period_exponent;
>> >	stream->no_preempt = param->no_preempt;
>> > +	stream->wait_num_reports = param->wait_num_reports;
>> >
>> >	stream->xef = xe_file_get(param->xef);
>> >	stream->num_syncs = param->num_syncs;
>> > @@ -2156,6 +2170,14 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
>> >	if (!param.oa_buffer_size)
>> >		param.oa_buffer_size = DEFAULT_XE_OA_BUFFER_SIZE;
>> >
>> > +	if (!param.wait_num_reports)
>> > +		param.wait_num_reports = 1;
>> > +	if (param.wait_num_reports > param.oa_buffer_size / f->size) {
>> > +		drm_dbg(&oa->xe->drm, "wait_num_reports %d\n", param.wait_num_reports);
>> > +		ret = -EINVAL;
>> > +		goto err_exec_q;
>> > +	}
>>
>> If possible, I think this check where wait_num_reports has an upper limit
>> should be moved to xe_oa_set_prop_wait_num_reports().
>
>It's not possible since the properties can come in any order, so both the
>OA buffer size as well as the format might not be available when
>wait_num_reports property is parsed. So they could both still be 0 when
>xe_oa_set_prop_wait_num_reports() is called.
>
>That is why the code which checks for consistency between multiple
>properties comes after the code which parses individual properties.

Oh, makes sense. Then this is:

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

as is.

Thanks,
Umesh