[Intel-xe] [PATCH 1/2] drm/xe/device: Prefer the drm-managed mutex_init

[Matt Roper]

On Fri, Feb 24, 2023 at 04:21:37PM -0800, Lucas De Marchi wrote:
> There's inconsistent use of mutex_init(), in xe_device_create(), with
> several of them never calling mutex_destroy() in xe_device_destroy().
> Migrate all of them to drmm_mutex_init(), so the destroy part is
> automatically called.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index a028efa8eebe..5e2f5d93f2ce 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -159,7 +159,6 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
>  
>  	destroy_workqueue(xe->display.hotplug.dp_wq);
>  	destroy_workqueue(xe->ordered_wq);
> -	mutex_destroy(&xe->persitent_engines.lock);
>  	ttm_device_fini(&xe->ttm);
>  }
>  
> @@ -196,10 +195,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  
>  	init_waitqueue_head(&xe->ufence_wq);
>  
> -	mutex_init(&xe->usm.lock);
> +	drmm_mutex_init(&xe->drm, &xe->usm.lock);
>  	xa_init_flags(&xe->usm.asid_to_vm, XA_FLAGS_ALLOC1);
>  
> -	mutex_init(&xe->persitent_engines.lock);
> +	drmm_mutex_init(&xe->drm, &xe->persitent_engines.lock);
>  	INIT_LIST_HEAD(&xe->persitent_engines.list);

I'm guessing this was supposed to be "persistent" rather than
"persitent?"  But the typo has propagated to so many places in the code
now, that changing it would need to be a separate patch.

>  
>  	spin_lock_init(&xe->pinned.lock);
> @@ -213,12 +212,12 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	/* Initialize display parts here.. */
>  	spin_lock_init(&xe->display.fb_tracking.lock);
>  
> -	mutex_init(&xe->sb_lock);
> -	mutex_init(&xe->display.backlight.lock);
> -	mutex_init(&xe->display.audio.mutex);
> -	mutex_init(&xe->display.wm.wm_mutex);
> -	mutex_init(&xe->display.pps.mutex);
> -	mutex_init(&xe->display.hdcp.comp_mutex);
> +	drmm_mutex_init(&xe->drm, &xe->sb_lock);
> +	drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
> +	drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
> +	drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
> +	drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
> +	drmm_mutex_init(&xe->drm, &xe->display.hdcp.comp_mutex);

It looks like we've never bothered to destroy these in i915 either.

I sort of feel like we shouldn't even be initializing display mutexes
like this in xe at all.  Rather i915's shared display code should just
be exposing something like an intel_display_early_init() function that
will take care of doing the basic data structure setup for a 'struct
intel_display' and that can be called by either driver's general init.
Then when someone adds a new display mutex to i915, we don't have to go
duplicate the same initialization on the Xe side as well.

Anway, we can move in that direction with future series, so not a
blocker here.

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

>  	xe->enabled_irq_mask = ~0;
>  
>  	xe->params.invert_brightness = -1;
> @@ -236,7 +235,8 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	if (err)
>  		goto err;
>  
> -	mutex_init(&xe->mem_access.lock);
> +	drmm_mutex_init(&xe->drm, &xe->mem_access.lock);
> +
>  	return xe;
>  
>  err_put:
> -- 
> 2.39.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation