Re: [PATCH 1/3] drm/xe/oa: Allow stream enable/disable functions to return error

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

On Tue, Jun 25, 2024 at 01:15:16PM -0700, Ashutosh Dixit wrote:
>Stream enable/disable functions previously had void return because failure
>during function execution was not possible. This will change when we
>introduce functionality to disable preemption on the stream exec
>queue. Therefore, in preparation for this functionality, prepare this code
>to be able to handle error returns.
>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

lgtm

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

>---
> drivers/gpu/drm/xe/xe_oa.c | 38 ++++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>index 9263ae9a864e..a68659fd5386 100644
>--- a/drivers/gpu/drm/xe/xe_oa.c
>+++ b/drivers/gpu/drm/xe/xe_oa.c
>@@ -1013,24 +1013,26 @@ static void xe_oa_stream_disable(struct xe_oa_stream *stream)
> 		hrtimer_cancel(&stream->poll_check_timer);
> }
>
>-static void xe_oa_enable_locked(struct xe_oa_stream *stream)
>+static int xe_oa_enable_locked(struct xe_oa_stream *stream)
> {
> 	if (stream->enabled)
>-		return;
>-
>-	stream->enabled = true;
>+		return 0;
>
> 	xe_oa_stream_enable(stream);
>+
>+	stream->enabled = true;
>+	return 0;
> }
>
>-static void xe_oa_disable_locked(struct xe_oa_stream *stream)
>+static int xe_oa_disable_locked(struct xe_oa_stream *stream)
> {
> 	if (!stream->enabled)
>-		return;
>-
>-	stream->enabled = false;
>+		return 0;
>
> 	xe_oa_stream_disable(stream);
>+
>+	stream->enabled = false;
>+	return 0;
> }
>
> static long xe_oa_config_locked(struct xe_oa_stream *stream, u64 arg)
>@@ -1105,11 +1107,9 @@ static long xe_oa_ioctl_locked(struct xe_oa_stream *stream,
> {
> 	switch (cmd) {
> 	case DRM_XE_PERF_IOCTL_ENABLE:
>-		xe_oa_enable_locked(stream);
>-		return 0;
>+		return xe_oa_enable_locked(stream);
> 	case DRM_XE_PERF_IOCTL_DISABLE:
>-		xe_oa_disable_locked(stream);
>-		return 0;
>+		return xe_oa_disable_locked(stream);
> 	case DRM_XE_PERF_IOCTL_CONFIG:
> 		return xe_oa_config_locked(stream, arg);
> 	case DRM_XE_PERF_IOCTL_STATUS:
>@@ -1432,19 +1432,25 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> 	if (ret)
> 		goto err_free;
>
>+	if (!param->disabled) {
>+		ret = xe_oa_enable_locked(stream);
>+		if (ret)
>+			goto err_destroy;
>+	}
>+
> 	stream_fd = anon_inode_getfd("[xe_oa]", &xe_oa_fops, stream, 0);
> 	if (stream_fd < 0) {
> 		ret = stream_fd;
>-		goto err_destroy;
>+		goto err_disable;
> 	}
>
>-	if (!param->disabled)
>-		xe_oa_enable_locked(stream);
>-
> 	/* Hold a reference on the drm device till stream_fd is released */
> 	drm_dev_get(&stream->oa->xe->drm);
>
> 	return stream_fd;
>+err_disable:
>+	if (!param->disabled)
>+		xe_oa_disable_locked(stream);
> err_destroy:
> 	xe_oa_stream_destroy(stream);
> err_free:
>-- 
>2.41.0
>