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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 06.06.2024 19:25, Rodrigo Vivi wrote:
> 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.

but note that drm_mm alone does not interact with the hw, it's what we
eventually build on top of it (like here ggtt manager) may touch the hw

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