| Message ID | 20180705065653.20449-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/2018 07:56, Chris Wilson wrote: > We can now use the full release mechanism (i915_ppgtt_put) for our local > ppgtt allocation in igt_ppgtt_alloc. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > index 4bfb0537f9be..e108fe4e0fd9 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > @@ -202,9 +202,8 @@ static int igt_ppgtt_alloc(void *arg) > > err_ppgtt_cleanup: > mutex_lock(&dev_priv->drm.struct_mutex); > - ppgtt->vm.cleanup(&ppgtt->vm); > + i915_ppgtt_put(ppgtt); > mutex_unlock(&dev_priv->drm.struct_mutex); > - kfree(ppgtt); > return err; > } > > Hm, it's not obvious why this works, if it does. This ppgtt it creates does not call kref_init on ppgtt->ref, or i915_address_space_init. Colour me confused. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-07-05 10:50:36) > > On 05/07/2018 07:56, Chris Wilson wrote: > > We can now use the full release mechanism (i915_ppgtt_put) for our local > > ppgtt allocation in igt_ppgtt_alloc. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > > index 4bfb0537f9be..e108fe4e0fd9 100644 > > --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > > @@ -202,9 +202,8 @@ static int igt_ppgtt_alloc(void *arg) > > > > err_ppgtt_cleanup: > > mutex_lock(&dev_priv->drm.struct_mutex); > > - ppgtt->vm.cleanup(&ppgtt->vm); > > + i915_ppgtt_put(ppgtt); > > mutex_unlock(&dev_priv->drm.struct_mutex); > > - kfree(ppgtt); > > return err; > > } > > > > > > Hm, it's not obvious why this works, if it does. This ppgtt it creates > does not call kref_init on ppgtt->ref, or i915_address_space_init. > Colour me confused. Are you sure? There were a few ordering issues that required pushing i915_address_space_init earlier into __hw_ppgtt_create() itself that dragged the kref_init along for the fun. -Chris
On 05/07/2018 10:50, Tvrtko Ursulin wrote: > > On 05/07/2018 07:56, Chris Wilson wrote: >> We can now use the full release mechanism (i915_ppgtt_put) for our local >> ppgtt allocation in igt_ppgtt_alloc. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c >> b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c >> index 4bfb0537f9be..e108fe4e0fd9 100644 >> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c >> @@ -202,9 +202,8 @@ static int igt_ppgtt_alloc(void *arg) >> err_ppgtt_cleanup: >> mutex_lock(&dev_priv->drm.struct_mutex); >> - ppgtt->vm.cleanup(&ppgtt->vm); >> + i915_ppgtt_put(ppgtt); >> mutex_unlock(&dev_priv->drm.struct_mutex); >> - kfree(ppgtt); >> return err; >> } >> > > Hm, it's not obvious why this works, if it does. This ppgtt it creates > does not call kref_init on ppgtt->ref, or i915_address_space_init. > Colour me confused. I was looking at the too old tree, in the meantime you sneaked in v4 of the wc stash patch which changes these things. Slightly unfortunate that common parts of the init are now duplicated.. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-07-05 10:55:23) > > On 05/07/2018 10:50, Tvrtko Ursulin wrote: > > > > On 05/07/2018 07:56, Chris Wilson wrote: > >> We can now use the full release mechanism (i915_ppgtt_put) for our local > >> ppgtt allocation in igt_ppgtt_alloc. > >> > >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> --- > >> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > >> b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > >> index 4bfb0537f9be..e108fe4e0fd9 100644 > >> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > >> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c > >> @@ -202,9 +202,8 @@ static int igt_ppgtt_alloc(void *arg) > >> err_ppgtt_cleanup: > >> mutex_lock(&dev_priv->drm.struct_mutex); > >> - ppgtt->vm.cleanup(&ppgtt->vm); > >> + i915_ppgtt_put(ppgtt); > >> mutex_unlock(&dev_priv->drm.struct_mutex); > >> - kfree(ppgtt); > >> return err; > >> } > >> > > > > Hm, it's not obvious why this works, if it does. This ppgtt it creates > > does not call kref_init on ppgtt->ref, or i915_address_space_init. > > Colour me confused. > > I was looking at the too old tree, in the meantime you sneaked in v4 of > the wc stash patch which changes these things. I thought it was clean and rushed before checking the selftests. > Slightly unfortunate that common parts of the init are now duplicated.. I think we should just pull the kref into i915_address_space, then all the common parts are inited together. The only reason it is not there is to avoid refcounting ggtt, which I'm not that concerned about as we should only be tweaking ppgtt krefs on the fly. -Chris
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c index 4bfb0537f9be..e108fe4e0fd9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -202,9 +202,8 @@ static int igt_ppgtt_alloc(void *arg) err_ppgtt_cleanup: mutex_lock(&dev_priv->drm.struct_mutex); - ppgtt->vm.cleanup(&ppgtt->vm); + i915_ppgtt_put(ppgtt); mutex_unlock(&dev_priv->drm.struct_mutex); - kfree(ppgtt); return err; }
We can now use the full release mechanism (i915_ppgtt_put) for our local ppgtt allocation in igt_ppgtt_alloc. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)