Re: [PATCH 2/2] drm/xe: Use drm_device managed mutex/mm init helpers in GGTT

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, May 24, 2024 at 03:35:18PM +0200, Michal Wajdeczko wrote:
> There is not need for private release action as there are existing
> drmm_mm_init() and drmm_mutex_init() helpers that can be used.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 17e5066763db..7c91fe212dcb 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -96,14 +96,6 @@ static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
>  	}
>  }
>  
> -static void ggtt_fini_early(struct drm_device *drm, void *arg)
> -{
> -	struct xe_ggtt *ggtt = arg;
> -
> -	mutex_destroy(&ggtt->lock);
> -	drm_mm_takedown(&ggtt->mm);
> -}
> -
>  static void ggtt_fini(struct drm_device *drm, void *arg)
>  {
>  	struct xe_ggtt *ggtt = arg;
> @@ -141,6 +133,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  	struct xe_device *xe = tile_to_xe(ggtt->tile);
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	unsigned int gsm_size;
> +	int err;
>  
>  	if (IS_SRIOV_VF(xe))
>  		gsm_size = SZ_8M; /* GGTT is expected to be 4GiB */
> @@ -189,12 +182,18 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  	else
>  		ggtt->pt_ops = &xelp_pt_ops;
>  
> -	drm_mm_init(&ggtt->mm, xe_wopcm_size(xe),
> -		    ggtt->size - xe_wopcm_size(xe));
> -	mutex_init(&ggtt->lock);
> +	err = drmm_mm_init(&xe->drm, &ggtt->mm, xe_wopcm_size(xe),
> +			   ggtt->size - xe_wopcm_size(xe));
> +	if (err)
> +		return err;
> +
> +	err = drmm_mutex_init(&xe->drm, &ggtt->lock);
> +	if (err)
> +		return err;

My first impression here is that we would have a bug here if drmm_mm_init
works, but drmm_mutex_init fails, but we are likely safe because the
probe will also entirely fail if this mutex init fails.

> +
>  	primelockdep(ggtt);
>  
> -	return drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt);

But my question here is, why drmm and not devm for this ggtt case that
only makes sense if the hardware/device is up and not about the module
or no reason to keep it alive after the probe failure or device removal.

I know that the question is orthogonal to your patch. But if we decide to
change the course later and move this towards devm, then we need to
get back to the exit function and perhaps regular mutex.

I mean, really nothing against this patch itself, specially if we are
confident that drmm is the way to go with this ggtt. So, I'm not blocking
here:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +	return 0;
>  }
>  
>  static void xe_ggtt_invalidate(struct xe_ggtt *ggtt);
> -- 
> 2.43.0
>