On 19/12/2019 20:43, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-12-19 18:00:13)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
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@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxx>
(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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
