| Message ID | 20191219180019.25562-3-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Per client engine busyness | expand |
Quoting Tvrtko Ursulin (2019-12-19 18:00:13) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > In the following patches we will develope a need to peek into the client > owned data from any potential leftover contexts. > > To facilitate this add reference counting to file_priv. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/i915_drv.c | 4 ---- > drivers/gpu/drm/i915/i915_drv.h | 4 +++- > drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++- > 4 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index e5a7c6f02a47..b482b2e5f31f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -853,7 +853,7 @@ int i915_gem_context_open(struct drm_i915_private *i915, > void i915_gem_context_close(struct drm_file *file) > { > struct drm_i915_file_private *file_priv = file->driver_priv; > - struct drm_i915_private *i915 = file_priv->dev_priv; > + struct drm_i915_private *i915 = file_priv->i915; > struct i915_gem_context *ctx; > unsigned long idx; > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 8b08cfe30151..0c9c93418068 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1633,13 +1633,9 @@ static void i915_driver_lastclose(struct drm_device *dev) > > static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) > { > - struct drm_i915_file_private *file_priv = file->driver_priv; > - > i915_gem_context_close(file); > i915_gem_release(dev, file); > > - kfree_rcu(file_priv, rcu); As you are moving the kfree_rcu() into the i915_gem_release (via a put), I think it also makes sense to move the call for i915_gem_context_close on this file. Possibly renaming it to i915_gem_file_close() and s/drm_i915_file_private/i915_gem_file/ or i915_gem_client (with corresponding name changes) in the process. For the basic mechanics of this patch though, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (Though I still suggest a bit of playing with i915_gem_context_close, i915_gem_release to tie them together to the notion of the file better.) -Chris
On 19/12/2019 20:43, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-12-19 18:00:13) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> In the following patches we will develope a need to peek into the client >> owned data from any potential leftover contexts. >> >> To facilitate this add reference counting to file_priv. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.c | 4 ---- >> drivers/gpu/drm/i915/i915_drv.h | 4 +++- >> drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++- >> 4 files changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c >> index e5a7c6f02a47..b482b2e5f31f 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c >> @@ -853,7 +853,7 @@ int i915_gem_context_open(struct drm_i915_private *i915, >> void i915_gem_context_close(struct drm_file *file) >> { >> struct drm_i915_file_private *file_priv = file->driver_priv; >> - struct drm_i915_private *i915 = file_priv->dev_priv; >> + struct drm_i915_private *i915 = file_priv->i915; >> struct i915_gem_context *ctx; >> unsigned long idx; >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 8b08cfe30151..0c9c93418068 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -1633,13 +1633,9 @@ static void i915_driver_lastclose(struct drm_device *dev) >> >> static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) >> { >> - struct drm_i915_file_private *file_priv = file->driver_priv; >> - >> i915_gem_context_close(file); >> i915_gem_release(dev, file); >> >> - kfree_rcu(file_priv, rcu); > > As you are moving the kfree_rcu() into the i915_gem_release (via a put), > I think it also makes sense to move the call for i915_gem_context_close > on this file. Possibly renaming it to i915_gem_file_close() and > s/drm_i915_file_private/i915_gem_file/ or i915_gem_client (with > corresponding name changes) in the process. > > For the basic mechanics of this patch though, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > (Though I still suggest a bit of playing with i915_gem_context_close, > i915_gem_release to tie them together to the notion of the file better.) Yes, agreed in principle. But a) I prefer the release name to match with the fops mindset and b) I prefer to leave drm_i915_file_private alone as the top level driver private container. What I am not completely happy with, or say undecided, is whether to move the kref into i915_drm_client. I had it like that at one point, thinking to only have a smallest needed structure pinned in memory, but then I simplified in favour of fewer allocations. Now I think I'd like to move the kref into i915_drm_client again. Any opinion here? In a later patch, when I add the i915_gem_client_get/put helpers they are already named like that. Hm okay, I also have a naming confusion between struct i915_drm_client and i915_gem_client_get/put(). :) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index e5a7c6f02a47..b482b2e5f31f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -853,7 +853,7 @@ int i915_gem_context_open(struct drm_i915_private *i915, void i915_gem_context_close(struct drm_file *file) { struct drm_i915_file_private *file_priv = file->driver_priv; - struct drm_i915_private *i915 = file_priv->dev_priv; + struct drm_i915_private *i915 = file_priv->i915; struct i915_gem_context *ctx; unsigned long idx; diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 8b08cfe30151..0c9c93418068 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1633,13 +1633,9 @@ static void i915_driver_lastclose(struct drm_device *dev) static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) { - struct drm_i915_file_private *file_priv = file->driver_priv; - i915_gem_context_close(file); i915_gem_release(dev, file); - kfree_rcu(file_priv, rcu); - /* Catch up with all the deferred frees from "this" client */ i915_gem_flush_free_objects(to_i915(dev)); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9226c0515506..6f13f0c619e9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -190,13 +190,15 @@ struct i915_mm_struct; struct i915_mmu_object; struct drm_i915_file_private { - struct drm_i915_private *dev_priv; + struct kref kref; union { struct drm_file *file; struct rcu_head rcu; }; + struct drm_i915_private *i915; + struct { spinlock_t lock; struct list_head request_list; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f19c678ebefc..a52fdae95f58 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1518,6 +1518,14 @@ int i915_gem_freeze_late(struct drm_i915_private *i915) return 0; } +static void gem_release(struct kref *kref) +{ + struct drm_i915_file_private *fpriv = + container_of(kref, typeof(*fpriv), kref); + + kfree_rcu(fpriv, rcu); +} + void i915_gem_release(struct drm_device *dev, struct drm_file *file) { struct drm_i915_file_private *file_priv = file->driver_priv; @@ -1531,6 +1539,8 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file) list_for_each_entry(request, &file_priv->mm.request_list, client_link) request->file_priv = NULL; spin_unlock(&file_priv->mm.lock); + + kref_put(&file_priv->kref, gem_release); } int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file) @@ -1544,8 +1554,10 @@ int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file) if (!file_priv) return -ENOMEM; + kref_init(&file_priv->kref); + file->driver_priv = file_priv; - file_priv->dev_priv = i915; + file_priv->i915 = i915; file_priv->file = file; spin_lock_init(&file_priv->mm.lock);