Re: [Intel-gfx] [PATCH 1/5] drm/i915: Move intel_engine_free_request_pool to i915_request.c

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 09/06/2021 22:29, Jason Ekstrand wrote:
> This appears to break encapsulation by moving an intel_engine_cs
> function to a i915_request file.  However, this function is
> intrinsically tied to the lifetime rules and allocation scheme of
> i915_request and having it in intel_engine_cs.c leaks details of
> i915_request.  We have an abstraction leak either way.  Since
> i915_request's allocation scheme is far more subtle than the simple
> pointer that is intel_engine_cs.request_pool, it's probably better to
> keep i915_request's details to itself.
> 
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 8 --------
>   drivers/gpu/drm/i915/i915_request.c       | 7 +++++--
>   drivers/gpu/drm/i915/i915_request.h       | 2 --
>   3 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 9ceddfbb1687d..df6b80ec84199 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -422,14 +422,6 @@ void intel_engines_release(struct intel_gt *gt)
>   	}
>   }
>   
> -void intel_engine_free_request_pool(struct intel_engine_cs *engine)
> -{
> -	if (!engine->request_pool)
> -		return;
> -
> -	kmem_cache_free(i915_request_slab_cache(), engine->request_pool);

Argument that the slab cache shouldn't be exported from i915_request.c 
sounds good to me.

But I think step better than simply reversing the break of encapsulation 
(And it's even worse because it leaks much higher level object!) could 
be to export a freeing helper from i915_request.c, engine pool would 
then use:

void __i915_request_free(...)
{
	kmem_cache_free(...);
}

?

Regards,

Tvrtko

> -}
> -
>   void intel_engines_free(struct intel_gt *gt)
>   {
>   	struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 1014c71cf7f52..48c5f8527854b 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -106,9 +106,12 @@ static signed long i915_fence_wait(struct dma_fence *fence,
>   				 timeout);
>   }
>   
> -struct kmem_cache *i915_request_slab_cache(void)
> +void intel_engine_free_request_pool(struct intel_engine_cs *engine)
>   {
> -	return global.slab_requests;
> +	if (!engine->request_pool)
> +		return;
> +
> +	kmem_cache_free(global.slab_requests, engine->request_pool);
>   }
>   
>   static void i915_fence_release(struct dma_fence *fence)
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 270f6cd37650c..f84c38d29f988 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -300,8 +300,6 @@ static inline bool dma_fence_is_i915(const struct dma_fence *fence)
>   	return fence->ops == &i915_fence_ops;
>   }
>   
> -struct kmem_cache *i915_request_slab_cache(void);
> -
>   struct i915_request * __must_check
>   __i915_request_create(struct intel_context *ce, gfp_t gfp);
>   struct i915_request * __must_check
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx