| Message ID | 20240524133518.976-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DRM-managed drm_mm_init() | expand |
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 >
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 >>
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; + primelockdep(ggtt); - return drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt); + return 0; } static void xe_ggtt_invalidate(struct xe_ggtt *ggtt);
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(-)