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

[Lucas De Marchi]

On Mon, Feb 27, 2023 at 02:59:28PM -0800, Matt Roper wrote:
>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.

yep, looks like it.

>
>>
>>  	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.

that is exactly what I was doing with some patches on top. These 2
patches are moving it to a single block that I could take out to a

	xe->display = xe_display_create();

with the goal of leaving an opaque `struct xe_display` that would wrap
the i915 one.  I also moved the device infor stuff to be set there.
Code-wise I think I got 90% of that done, but then I gave
up: too many issues were popping up with the ifdefs on the i915 side and
the partial copies we did in display/ext/.  I may try to revive part of
that, but the overall display direction reusing i915 is not very clear
for me to commit much more time on that.

Here I only sent what I had split in small sane commits.

>
>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>

thanks
Lucas De Marchi


>
>>  	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