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@xxxxxxxxx>
>> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>
>> + return 0;
>> }
>>
>> static void xe_ggtt_invalidate(struct xe_ggtt *ggtt);
>> --
>> 2.43.0
>>
