[PATCH] drm/i915: Annotate the execbuffer dma-fence signalling critical path

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx-trybot <intel-gfx-trybot-bounces at lists.freedesktop.org> On Behalf Of Nirmoy Das
Sent: Wednesday, January 15, 2025 7:59 AM
To: intel-gfx-trybot at lists.freedesktop.org
Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
Subject: [PATCH] drm/i915: Annotate the execbuffer dma-fence signalling critical path

Nit:

1. I think it should be drm/i915/gem and not just drm/i915

2. "signalling" reports as misspelled in my text viewer, though I looked it up and I'm
fairly certain that both "signaling" and "signalling" are valid spellings depending on
regional language differences.  It's also what's being used in the code itself, so I
don't think it should be "fixed".

3. Is "Annotate" the right word here?  I think "Annotate" would imply that we're
adding comments to the code, which doesn't seem to be the case?  Or that we're
only adding a prototype version of the critical path, which also seems incorrect?
Should we s/Annotate/Implement ?  Or maybe s/Annotate/Add ?

> 
> From: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> 

We should have a commit message here.  Even if it's relatively small and redundant:
"""
Begin dma-fence signalling in the execbuffer execution path, specifically
during the lifecycle of the exec buffer requests.
"""

> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index f151640c1d13..12186e4724cd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -315,6 +315,7 @@ struct i915_execbuffer {
>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>  	struct i915_capture_list *capture_lists[MAX_ENGINE_INSTANCE + 1];
>  #endif
> +	bool dma_fence_cookie;

Nit:
This new structure member should probably have a comment explaining
its purpose:
"""
	/**
	 * Opaque cookie returned from dma_fence_begin_signalling that must
	 * be passed to dma_fence_end_singalling.
	 */
	bool dma_fence_cookie;
"""
Though admittedly, this explanation is lacking in a lot of ways.  Unfortunately,
this is all the details given when looking at the dma_fence_[begin|end]_signalling
functions (other than that they're "needed by the implementation"), so I don't
think we can get any more specific than that.

>  };
>  
>  static int eb_parse(struct i915_execbuffer *eb);
> @@ -3139,6 +3140,8 @@ static int eb_requests_add(struct i915_execbuffer *eb, int err)
>  		if (!rq)
>  			continue;
>  		err |= eb_request_add(eb, rq, err, i == 0);
> +		if (i == 0)
> +			dma_fence_end_signalling(eb->dma_fence_cookie);
>  	}
>  
>  	return err;
> @@ -3319,6 +3322,9 @@ eb_requests_create(struct i915_execbuffer *eb, struct dma_fence *in_fence,
>  			return out_fence;
>  		}
>  
> +		if (i == 0)
> +			eb->dma_fence_cookie = dma_fence_begin_signalling();
> +
>  		/*
>  		 * Only the first request added (committed to backend) has to
>  		 * take the in fences into account as all subsequent requests
> -- 
> 2.46.0
> 
> 

I don't see any significant reason to block this.  Just add a commit message
and this is:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt