Re: [Intel-gfx] [PATCH] drm/i915: Hold reference to intel_context over life of i915_request

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 28.07.2023 09:54, Andrzej Hajda wrote:
> References to i915_requests may be trapped by userspace inside a
> sync_file or dmabuf (dma-resv) and held indefinitely across different
> proceses. To counter-act the memory leaks, we try to not to keep
> references from the request past their completion.
> On the other side on fence release we need to know if rq->engine
> is valid and points to hw engine (true for non-virtual requests).
> To make it possible extra bit has been added to rq->execution_mask,
> for marking virtual engines.
> 
> Fixes: bcb9aa45d5a0 ("Revert "drm/i915: Hold reference to intel_context over life of i915_request"")
> Signed-off-by: Chris Wilson <chris.p.wilson@linux.intel.com>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>

Ups, I forgot to change the subject, should be:
drm/i915: fix request_pool assignment code on request release

Regards
Andrzej


> ---
> Hi all,
> 
> This is squash of revert of fixed patch with Chris fix for internal
> branch with expanded description.
> 
> Regards
> Andrzej
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_types.h      | 1 +
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 1 +
>   drivers/gpu/drm/i915/i915_request.c               | 7 ++-----
>   3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index e99a6fa03d4539..a7e6775980043c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -58,6 +58,7 @@ struct i915_perf_group;
>   
>   typedef u32 intel_engine_mask_t;
>   #define ALL_ENGINES ((intel_engine_mask_t)~0ul)
> +#define VIRTUAL_ENGINES BIT(BITS_PER_TYPE(intel_engine_mask_t) - 1)
>   
>   struct intel_hw_status_page {
>   	struct list_head timelines;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index a0e3ef1c65d246..e7f748b2102263 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -5469,6 +5469,7 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count,
>   	ve->base.submit_request = guc_submit_request;
>   
>   	ve->base.flags = I915_ENGINE_IS_VIRTUAL;
> +	ve->base.mask = VIRTUAL_ENGINES;
>   
>   	intel_context_init(&ve->context, &ve->base);
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 721e6aefec6b4d..0679863d10244f 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -134,9 +134,7 @@ static void i915_fence_release(struct dma_fence *fence)
>   	i915_sw_fence_fini(&rq->semaphore);
>   
>   	/*
> -	 * Keep one request on each engine for reserved use under mempressure
> -	 * do not use with virtual engines as this really is only needed for
> -	 * kernel contexts.
> +	 * Keep one request on each engine for reserved use under mempressure.
>   	 *
>   	 * We do not hold a reference to the engine here and so have to be
>   	 * very careful in what rq->engine we poke. The virtual engine is
> @@ -166,8 +164,7 @@ static void i915_fence_release(struct dma_fence *fence)
>   	 * know that if the rq->execution_mask is a single bit, rq->engine
>   	 * can be a physical engine with the exact corresponding mask.
>   	 */
> -	if (!intel_engine_is_virtual(rq->engine) &&
> -	    is_power_of_2(rq->execution_mask) &&
> +	if (is_power_of_2(rq->execution_mask) &&
>   	    !cmpxchg(&rq->engine->request_pool, NULL, rq))
>   		return;
>