Re: [Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()
Quoting Tvrtko Ursulin (2019-04-10 12:06:49) > > On 10/04/2019 11:13, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-04-10 11:05:13) > >> And some lockdep_assert_held in all three? > > > > Read on :( > > > > The plan is for intel_context_enter/_exit to be under the > > timeline->mutex, but that isn't realised for about another 30 patches. > > > > mark_active has special protection because it gets used from the > > serialised portion of intel_engine_park. > > Ok, but bug on for zero active_count makes sense right? Maybe even > intel_context_pin_active to signify that it can only act on an already > active context? It get's called on the kernel_context with ce->active_count==0 and not even holding the mutex from inside the wakeref handling code. (On no, I've invented a new BKL --- well a parking brake.) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()
On 10/04/2019 11:13, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-10 11:05:13)
On 08/04/2019 10:17, Chris Wilson wrote:
+void __intel_context_enter(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ if (!i915->gt.active_requests++)
+ i915_gem_unpark(i915);
+}
+
+void __intel_context_exit(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ GEM_BUG_ON(!i915->gt.active_requests);
+ if (!--i915->gt.active_requests)
+ i915_gem_park(i915);
+}
In our normal nomenclature __intel_context_enter would be expected to be
directly called by intel_context_enter on the 0 -> 1 refcnt transition.
Here they are in fact common helpers, so one level of indirection, via a
different layer. I found this a bit confusing at first. My usual remedy
here is to prescribe a better name. __intel_gt_context_enter/exit? To
designate the glue between backend and GT.
The use of gt/gem here is a placeholder. Here we will be iterating over
the engines (so just the one for the usual contexts, and num_siblings for
virtual).
intel_context_enter_engine() ?
And virtual_context_enter remains distinct.
I did not find it at first since from the commit message I expected you
would be assigning backend specific enter/exit callbacks to ce->ops. I
see now that in "drm/i915: Invert the GEM wakeref hierarchy" you in fact
just change the bodies of __intel_context_enter/exit. The only vfunc
override comes with the virtual engine as you wrote.
intel_context_enter_engine sounds okay consider all this.
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index ebc861b1a49e..95b1fdc5826a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct intel_context
*ce)
void intel_context_unpin(struct intel_context *ce);
+void __intel_context_enter(struct intel_context *ce);
+void __intel_context_exit(struct intel_context *ce);
+
+static inline void intel_context_enter(struct intel_context *ce)
+{
+ if (!ce->active_count++)
+ ce->ops->enter(ce);
+}
+
+static inline void intel_context_mark_active(struct intel_context *ce)
+{
GEM_BUG_ON(ce->active_count == 0) ?
We're no longer in Kansas. I was wondering if I could start GT_BUG_ON
and include "intel_gt.h" with a goal of doing a search and replace within
gt/. That gives us some separation from GEM, and maybe we could even
start filtering based on need.
I fear it would be too much for time being.
And some lockdep_assert_held in all three?
Read on :(
The plan is for intel_context_enter/_exit to be under the
timeline->mutex, but that isn't realised for about another 30 patches.
mark_active has special protection because it gets used from the
serialised portion of intel_engine_park.
Ok, but bug on for zero active_count makes sense right? Maybe even
intel_context_pin_active to signify that it can only act on an already
active context?
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()
Quoting Tvrtko Ursulin (2019-04-10 11:05:13)
>
> On 08/04/2019 10:17, Chris Wilson wrote:
> > +void __intel_context_enter(struct intel_context *ce)
> > +{
> > + struct drm_i915_private *i915 = ce->gem_context->i915;
> > +
> > + if (!i915->gt.active_requests++)
> > + i915_gem_unpark(i915);
> > +}
> > +
> > +void __intel_context_exit(struct intel_context *ce)
> > +{
> > + struct drm_i915_private *i915 = ce->gem_context->i915;
> > +
> > + GEM_BUG_ON(!i915->gt.active_requests);
> > + if (!--i915->gt.active_requests)
> > + i915_gem_park(i915);
> > +}
>
> In our normal nomenclature __intel_context_enter would be expected to be
> directly called by intel_context_enter on the 0 -> 1 refcnt transition.
> Here they are in fact common helpers, so one level of indirection, via a
> different layer. I found this a bit confusing at first. My usual remedy
> here is to prescribe a better name. __intel_gt_context_enter/exit? To
> designate the glue between backend and GT.
The use of gt/gem here is a placeholder. Here we will be iterating over
the engines (so just the one for the usual contexts, and num_siblings for
virtual).
intel_context_enter_engine() ?
And virtual_context_enter remains distinct.
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
> > b/drivers/gpu/drm/i915/gt/intel_context.h
> > index ebc861b1a49e..95b1fdc5826a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > @@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct
> > intel_context *ce)
> >
> > void intel_context_unpin(struct intel_context *ce);
> >
> > +void __intel_context_enter(struct intel_context *ce);
> > +void __intel_context_exit(struct intel_context *ce);
> > +
> > +static inline void intel_context_enter(struct intel_context *ce)
> > +{
> > + if (!ce->active_count++)
> > + ce->ops->enter(ce);
> > +}
> > +
> > +static inline void intel_context_mark_active(struct intel_context *ce)
> > +{
>
> GEM_BUG_ON(ce->active_count == 0) ?
We're no longer in Kansas. I was wondering if I could start GT_BUG_ON
and include "intel_gt.h" with a goal of doing a search and replace within
gt/. That gives us some separation from GEM, and maybe we could even
start filtering based on need.
> And some lockdep_assert_held in all three?
Read on :(
The plan is for intel_context_enter/_exit to be under the
timeline->mutex, but that isn't realised for about another 30 patches.
mark_active has special protection because it gets used from the
serialised portion of intel_engine_park.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()
On 08/04/2019 10:17, Chris Wilson wrote:
We wish to start segregating the power management into different control
domains, both with respect to the hardware and the user interface. The
first step is that at the lowest level flow of requests, we want to
process a context event (and not a global GEM operation). In this patch,
we introduce the context callbacks that in future patches will be
redirected to per-engine interfaces leading to global operations as
required.
The intent is that this will be guarded by the timeline->mutex, except
that retiring has not quite finished transitioning over from being
guarded by struct_mutex. So at the moment it is protected by
struct_mutex with a reminded to switch.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/gt/intel_context.c | 17 ++
drivers/gpu/drm/i915/gt/intel_context.h | 20 +
drivers/gpu/drm/i915/gt/intel_context_types.h | 5 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 3 +++
drivers/gpu/drm/i915/gt/intel_ringbuffer.c| 3 +++
drivers/gpu/drm/i915/gt/mock_engine.c | 3 +++
drivers/gpu/drm/i915/i915_request.c | 22 ---
7 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index ebd1e5919a4a..bf16f00a10a4 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -266,3 +266,20 @@ int __init i915_global_context_init(void)
i915_global_register();
return 0;
}
+
+void __intel_context_enter(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ if (!i915->gt.active_requests++)
+ i915_gem_unpark(i915);
+}
+
+void __intel_context_exit(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ GEM_BUG_ON(!i915->gt.active_requests);
+ if (!--i915->gt.active_requests)
+ i915_gem_park(i915);
+}
In our normal nomenclature __intel_context_enter would be expected to be
directly called by intel_context_enter on the 0 -> 1 refcnt transition.
Here they are in fact common helpers, so one level of indirection, via a
different layer. I found this a bit confusing at first. My usual remedy
here is to prescribe a better name. __intel_gt_context_enter/exit? To
designate the glue between backend and GT.
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index ebc861b1a49e..95b1fdc5826a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct intel_context
*ce)
void intel_context_unpin(struct intel_context *ce);
+void __intel_context_enter(struct intel_context *ce);
+void __intel_context_exit(struct intel_context *ce);
+
+static inline void intel_context_enter(struct intel_context *ce)
+{
+ if (!ce->active_count++)
+ ce->ops->enter(ce);
+}
+
+static inline void intel_context_mark_active(struct intel_context *ce)
+{
GEM_BUG_ON(ce->active_count == 0) ?
And some lockdep_assert_held in all three?
+ ++ce->active_count;
+}
+
+static inline void intel_context_exit(struct intel_context *ce)
+{
+ if (!--ce->active_count)
+ ce->ops->exit(ce);
+}
+
static inline struct intel_context *intel_context_get(struct intel_context
*ce)
{
kref_get(>ref);
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 9ec4f787c908..f02d27734e3b 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -25,6 +25,9 @@ struct intel_context_ops {
int (*pin)(struct intel_context *ce);
void (*unpin)(struct intel_context *ce);
+ void (*enter)(struct intel_context *ce);
+ void (*exit)(struct intel_context *ce);
+
void (*reset)(struct intel_context *ce);
void (*destroy)(struct kref *kref);
};
@@ -46,6 +49,8 @@ struct intel_context {
u32 *lrc_reg_state;
u64 lrc_desc;
+ unsigned int active_count; /* notionally protected by timeline->mutex */
+
atomic_t pin_count;
struct mutex pin_mutex; /* guards pinning and associated on-gpuing */
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 1565b11b15f6..7da5e49dd385 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,6 +1404,9 @@ static const struct intel_context_ops
execlists_context_ops = {
.pin = execlists_context_pin,
.unpin = execlists_context_unpin,
+ .enter = __intel_context_enter,
+ .exit = __intel_context_exit,
+
.reset = execlists_context_reset,
.destroy = execlists_context_destroy,
};
diff --git
[Intel-gfx] [PATCH 10/29] drm/i915: Introduce context->enter() and context->exit()
We wish to start segregating the power management into different control
domains, both with respect to the hardware and the user interface. The
first step is that at the lowest level flow of requests, we want to
process a context event (and not a global GEM operation). In this patch,
we introduce the context callbacks that in future patches will be
redirected to per-engine interfaces leading to global operations as
required.
The intent is that this will be guarded by the timeline->mutex, except
that retiring has not quite finished transitioning over from being
guarded by struct_mutex. So at the moment it is protected by
struct_mutex with a reminded to switch.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/gt/intel_context.c | 17 ++
drivers/gpu/drm/i915/gt/intel_context.h | 20 +
drivers/gpu/drm/i915/gt/intel_context_types.h | 5 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 3 +++
drivers/gpu/drm/i915/gt/intel_ringbuffer.c| 3 +++
drivers/gpu/drm/i915/gt/mock_engine.c | 3 +++
drivers/gpu/drm/i915/i915_request.c | 22 ---
7 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index ebd1e5919a4a..bf16f00a10a4 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -266,3 +266,20 @@ int __init i915_global_context_init(void)
i915_global_register();
return 0;
}
+
+void __intel_context_enter(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ if (!i915->gt.active_requests++)
+ i915_gem_unpark(i915);
+}
+
+void __intel_context_exit(struct intel_context *ce)
+{
+ struct drm_i915_private *i915 = ce->gem_context->i915;
+
+ GEM_BUG_ON(!i915->gt.active_requests);
+ if (!--i915->gt.active_requests)
+ i915_gem_park(i915);
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index ebc861b1a49e..95b1fdc5826a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -73,6 +73,26 @@ static inline void __intel_context_pin(struct intel_context
*ce)
void intel_context_unpin(struct intel_context *ce);
+void __intel_context_enter(struct intel_context *ce);
+void __intel_context_exit(struct intel_context *ce);
+
+static inline void intel_context_enter(struct intel_context *ce)
+{
+ if (!ce->active_count++)
+ ce->ops->enter(ce);
+}
+
+static inline void intel_context_mark_active(struct intel_context *ce)
+{
+ ++ce->active_count;
+}
+
+static inline void intel_context_exit(struct intel_context *ce)
+{
+ if (!--ce->active_count)
+ ce->ops->exit(ce);
+}
+
static inline struct intel_context *intel_context_get(struct intel_context *ce)
{
kref_get(>ref);
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 9ec4f787c908..f02d27734e3b 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -25,6 +25,9 @@ struct intel_context_ops {
int (*pin)(struct intel_context *ce);
void (*unpin)(struct intel_context *ce);
+ void (*enter)(struct intel_context *ce);
+ void (*exit)(struct intel_context *ce);
+
void (*reset)(struct intel_context *ce);
void (*destroy)(struct kref *kref);
};
@@ -46,6 +49,8 @@ struct intel_context {
u32 *lrc_reg_state;
u64 lrc_desc;
+ unsigned int active_count; /* notionally protected by timeline->mutex */
+
atomic_t pin_count;
struct mutex pin_mutex; /* guards pinning and associated on-gpuing */
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 1565b11b15f6..7da5e49dd385 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,6 +1404,9 @@ static const struct intel_context_ops
execlists_context_ops = {
.pin = execlists_context_pin,
.unpin = execlists_context_unpin,
+ .enter = __intel_context_enter,
+ .exit = __intel_context_exit,
+
.reset = execlists_context_reset,
.destroy = execlists_context_destroy,
};
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index bcc842b8bbe7..5788e5ab818f 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1516,6 +1516,9 @@ static const struct intel_context_ops ring_context_ops = {
.pin = ring_context_pin,
.unpin = ring_context_unpin,
+ .enter = __intel_context_enter,
+ .exit = __intel_context_exit,
+
.reset = ring_context_reset,
.destroy = ring_context_destroy,
};
diff --git
