Re: [Intel-gfx] [PATCH 04/18] drm/i915: add context information to objects
On Thu, 29 Mar 2012 10:47:56 +0200 Daniel Vetter wrote: > On Wed, Mar 28, 2012 at 05:20:11PM -0700, Ben Widawsky wrote: > > On Thu, 29 Mar 2012 00:36:21 +0200 > > Daniel Vetter wrote: > > > > > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote: > > > > Handy mostly for assertions. > > > > > > > > Signed-off-by: Ben Widawsky > > > > > > I don't see the point of carrying around a obj->context_id - > > > context_id's are file_priv, so they're not that useful in the > > > tracepoint. I suggest you just drop the contex_id arg there - the > > > obj pointers are global and imo good enough for identification. > > > And the few BUG_ON's don't seem really useful either. > > > -Daniel > > > > obj->context_id was requested by Chris to clarify the trace events. > > I vaguely recall telling him that you won't like it. > > That's easily shot down on the grounds that: > - we currently don't dump the id/handles of gem objects into our > traces > - your tracepoints dump the context_id at create/destroy time, so > with a bit of python this can be re-added in userspace. Yes, but gem handles are not specific to i915, whereas context id numbers are. Also the tracepoint that is a problem is switch, not create/destroy > > > I personally dislike using object pointer though I do agree it > > serves the same purpose. > > > > The other nice thing about having the context id is it makes it > > easy in debug situations to find out if a certain object is a > > context object. But no use case for that yet. > > My gripes with obj->context_id is that it smells like a layering > violation. We don't have such special stuff for other special things > like rings. The only exception is framebuffer when pageflipping, but > I expect that we'll need to clean this up sooner or later (because we > need to be able to move framebuffers sooner or later anyway, so they > need better integration with the gem eviction code). > -Daniel I still think the pros (easy debug) outweigh the cons (not really on exactly what you don't like). I don't see any harm that can come from this. /me braces for harm Anyway, as you said in the other email, I'm expecting more serious feedback. If I end up redoing a bunch of stuff, this can go too if you really don't like it. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/18] drm/i915: add context information to objects
On Wed, Mar 28, 2012 at 05:20:11PM -0700, Ben Widawsky wrote: > On Thu, 29 Mar 2012 00:36:21 +0200 > Daniel Vetter wrote: > > > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote: > > > Handy mostly for assertions. > > > > > > Signed-off-by: Ben Widawsky > > > > I don't see the point of carrying around a obj->context_id - > > context_id's are file_priv, so they're not that useful in the > > tracepoint. I suggest you just drop the contex_id arg there - the obj > > pointers are global and imo good enough for identification. And the > > few BUG_ON's don't seem really useful either. > > -Daniel > > obj->context_id was requested by Chris to clarify the trace events. I > vaguely recall telling him that you won't like it. That's easily shot down on the grounds that: - we currently don't dump the id/handles of gem objects into our traces - your tracepoints dump the context_id at create/destroy time, so with a bit of python this can be re-added in userspace. > I personally dislike using object pointer though I do agree it serves > the same purpose. > > The other nice thing about having the context id is it makes it easy in > debug situations to find out if a certain object is a context object. > But no use case for that yet. My gripes with obj->context_id is that it smells like a layering violation. We don't have such special stuff for other special things like rings. The only exception is framebuffer when pageflipping, but I expect that we'll need to clean this up sooner or later (because we need to be able to move framebuffers sooner or later anyway, so they need better integration with the gem eviction code). -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/18] drm/i915: add context information to objects
On Thu, 29 Mar 2012 00:36:21 +0200 Daniel Vetter wrote: > On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote: > > Handy mostly for assertions. > > > > Signed-off-by: Ben Widawsky > > I don't see the point of carrying around a obj->context_id - > context_id's are file_priv, so they're not that useful in the > tracepoint. I suggest you just drop the contex_id arg there - the obj > pointers are global and imo good enough for identification. And the > few BUG_ON's don't seem really useful either. > -Daniel obj->context_id was requested by Chris to clarify the trace events. I vaguely recall telling him that you won't like it. I personally dislike using object pointer though I do agree it serves the same purpose. The other nice thing about having the context id is it makes it easy in debug situations to find out if a certain object is a context object. But no use case for that yet. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/18] drm/i915: add context information to objects
On Sun, Mar 18, 2012 at 01:39:44PM -0700, Ben Widawsky wrote:
> Handy mostly for assertions.
>
> Signed-off-by: Ben Widawsky
I don't see the point of carrying around a obj->context_id - context_id's
are file_priv, so they're not that useful in the tracepoint. I suggest you
just drop the contex_id arg there - the obj pointers are global and imo
good enough for identification. And the few BUG_ON's don't seem really
useful either.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h |5 +
> drivers/gpu/drm/i915/i915_gem.c |1 +
> drivers/gpu/drm/i915/i915_gem_context.c |3 +++
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e49e2f7..f458a8f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -953,6 +953,11 @@ struct drm_i915_gem_object {
>* reaches 0, dev_priv->pending_flip_queue will be woken up.
>*/
> atomic_t pending_flip;
> +
> + /**
> + * >= 0 if this object is the object for a context.
> + */
> + int context_id;
> };
>
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6343a82..0985aa5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3629,6 +3629,7 @@ struct drm_i915_gem_object
> *i915_gem_alloc_object(struct drm_device *dev,
> obj->madv = I915_MADV_WILLNEED;
> /* Avoid an unnecessary call to unbind on the first bind. */
> obj->map_and_fenceable = true;
> + obj->context_id = -1;
>
> return obj;
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2c9116d..321bafd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -127,6 +127,7 @@ again:
> } else if (ret)
> goto err_out;
>
> + (*ctx_out)->obj->context_id = (*ctx_out)->id;
> return 0;
>
> err_out:
> @@ -171,6 +172,8 @@ static int create_default_context(struct drm_i915_private
> *dev_priv)
>* default context.
>*/
> ctx = dev_priv->ring[RCS].default_context;
> + ctx->id = DEFAULT_CONTEXT_ID;
> + ctx->obj->context_id = DEFAULT_CONTEXT_ID;
> ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> if (ret)
> do_destroy(ctx);
> --
> 1.7.9.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/18] drm/i915: add context information to objects
Handy mostly for assertions.
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h |5 +
drivers/gpu/drm/i915/i915_gem.c |1 +
drivers/gpu/drm/i915/i915_gem_context.c |3 +++
3 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e49e2f7..f458a8f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -953,6 +953,11 @@ struct drm_i915_gem_object {
* reaches 0, dev_priv->pending_flip_queue will be woken up.
*/
atomic_t pending_flip;
+
+ /**
+* >= 0 if this object is the object for a context.
+*/
+ int context_id;
};
#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6343a82..0985aa5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3629,6 +3629,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct
drm_device *dev,
obj->madv = I915_MADV_WILLNEED;
/* Avoid an unnecessary call to unbind on the first bind. */
obj->map_and_fenceable = true;
+ obj->context_id = -1;
return obj;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 2c9116d..321bafd 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -127,6 +127,7 @@ again:
} else if (ret)
goto err_out;
+ (*ctx_out)->obj->context_id = (*ctx_out)->id;
return 0;
err_out:
@@ -171,6 +172,8 @@ static int create_default_context(struct drm_i915_private
*dev_priv)
* default context.
*/
ctx = dev_priv->ring[RCS].default_context;
+ ctx->id = DEFAULT_CONTEXT_ID;
+ ctx->obj->context_id = DEFAULT_CONTEXT_ID;
ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
if (ret)
do_destroy(ctx);
--
1.7.9.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
