Re: [RFC PATCH v2 1/3] drm/syncobj: Add flag DRM_SYNCOBJ_QUERY_FLAGS_ERROR to query errors

[Yicong Hui]

> > +
> >   /**
> >    * dma_fence_chain_find_seqno - find fence chain node by seqno
> >    * @pfence: pointer to the chain node where to start
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 2d4ab745fdad..322f64b72775 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -1654,14 +1654,17 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> >   {
> >   	struct drm_syncobj_timeline_array *args = data;
> >   	struct drm_syncobj **syncobjs;
> > +	unsigned int valid_flags = DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED |
> > +				   DRM_SYNCOBJ_QUERY_FLAGS_ERROR;
> >   	uint64_t __user *points = u64_to_user_ptr(args->points);
> > +	uint64_t __user *handles = u64_to_user_ptr(args->handles);
> >   	uint32_t i;
> >   	int ret;
> >   
> >   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> >   		return -EOPNOTSUPP;
> >   
> > -	if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED)
> > +	if (args->flags & ~valid_flags)
> >   		return -EINVAL;
> >   
> >   	if (args->count_handles == 0)
> > @@ -1680,6 +1683,22 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> >   		uint64_t point;
>
> Make a local variable "int error" here.
>
> ...
>
> > +			int64_t error = 0;
>
> The error code is an int and only 32bits.
Understood, will change that!

> > +
> > +			if (fence)
> > +				error = dma_fence_chain_find_error(fence);
> > +
> > +			ret = copy_to_user(&handles[i], &error, sizeof(int64_t));
>
> The handles are also only 32bits!
Ah, that's my mistake. Was thrown off by the __u64 in the struct definition but it is obvious now that it is a u32. Fixed as well!

> > +			ret = ret ? -EFAULT : 0;
> > +			if (ret) {
> > +				dma_fence_put(fence);
> > +				break;
> > +			}
> > +
> > +		}
> > +
> >   		chain = to_dma_fence_chain(fence);
> >   		if (chain) {
>
> In this code path whenever point is assigned something do a "error = dma_fence_get_status(fence);" and then eventually copy the error to userspace after copying the point.

Hi! Were you thinking something that looks a little bit like this?

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 2d4ab745fdad..b74e491f9d8b 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1654,14 +1654,17 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_syncobj_timeline_array *args = data;
 	struct drm_syncobj **syncobjs;
+	unsigned int valid_flags = DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED |
+				  DRM_SYNCOBJ_QUERY_FLAGS_ERROR;
 	uint64_t __user *points = u64_to_user_ptr(args->points);
+	uint32_t __user *handles = u64_to_user_ptr(args->handles);
 	uint32_t i;
-	int ret;
+	int ret, error;
 
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
 		return -EOPNOTSUPP;
 
-	if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED)
+	if (args->flags & ~valid_flags)
 		return -EINVAL;
 
 	if (args->count_handles == 0)
@@ -1681,6 +1684,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 
 		fence = drm_syncobj_fence_get(syncobjs[i]);
 		chain = to_dma_fence_chain(fence);
+
 		if (chain) {
 			struct dma_fence *iter, *last_signaled =
 				dma_fence_get(fence);
@@ -1688,6 +1692,8 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 			if (args->flags &
 			   DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
 				point = fence->seqno;
+				error = dma_fence_get_status(fence);
+
 			} else {
 				dma_fence_chain_for_each(iter, fence) {
 					if (iter->context != fence->context) {
@@ -1702,16 +1708,28 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 				point = dma_fence_is_signaled(last_signaled) ?
 					last_signaled->seqno :
 					to_dma_fence_chain(last_signaled)->prev_seqno;
+
+				error = dma_fence_get_status(last_signaled);
 			}
 			dma_fence_put(last_signaled);
 		} else {
 			point = 0;
+			error = fence ? dma_fence_get_status(fence) : 0;
 		}
 		dma_fence_put(fence);
+
 		ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
 		ret = ret ? -EFAULT : 0;
 		if (ret)
 			break;
+
+		if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
+			ret = copy_to_user(&handles[i], &error, sizeof(*handles));
+
+			ret = ret ? -EFAULT : 0;
+			if (ret)
+				break;
+		}
 	}
 	drm_syncobj_array_free(syncobjs, args->count_handles);