[PATCH] drm/syncobj: Deal with signalled fences in transfer.

[Lionel Landwerlin]

On 07/12/2021 03:32, Bas Nieuwenhuizen wrote:
> See the comments in the code. Basically if the seqno is already
> signalled then we get a NULL fence. If we then put the NULL fence
> in a binary syncobj it counts as unsignalled, making that syncobj
> pretty much useless for all expected uses.
>
> Not 100% sure about the transfer to a timeline syncobj but I
> believe it is needed there too, as AFAICT the add_point function
> assumes the fence isn't NULL.
>
> Fixes: ea569910cbab ("drm/syncobj: add transition iotcls between binary and timeline v2")
> Cc: stable at vger.kernel.org
> Signed-off-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index fdd2ec87cdd1..eb28a40400d2 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -861,6 +861,19 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private,
>   				     &fence);
>   	if (ret)
>   		goto err;
> +
> +	/* If the requested seqno is already signaled drm_syncobj_find_fence may
> +	 * return a NULL fence. To make sure the recipient gets signalled, use
> +	 * a new fence instead.
> +	 */
> +	if (!fence) {
> +		fence = dma_fence_allocate_private_stub();
> +		if (!fence) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +	}
> +


Shouldn't we fix drm_syncobj_find_fence() instead?

By returning a stub fence for the timeline case if there isn't one.


Because the same NULL fence check appears missing in amdgpu (and 
probably other drivers).


Also we should have tests for this in IGT.

AMD contributed some tests when this code was written but they never got 
reviewed :(


-Lionel


>   	chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL);
>   	if (!chain) {
>   		ret = -ENOMEM;
> @@ -890,6 +903,19 @@ drm_syncobj_transfer_to_binary(struct drm_file *file_private,
>   				     args->src_point, args->flags, &fence);
>   	if (ret)
>   		goto err;
> +
> +	/* If the requested seqno is already signaled drm_syncobj_find_fence may
> +	 * return a NULL fence. To make sure the recipient gets signalled, use
> +	 * a new fence instead.
> +	 */
> +	if (!fence) {
> +		fence = dma_fence_allocate_private_stub();
> +		if (!fence) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +	}
> +
>   	drm_syncobj_replace_fence(binary_syncobj, fence);
>   	dma_fence_put(fence);
>   err: