On 01/04/2019 16:37, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-01 16:22:55)
On 25/03/2019 09:03, Chris Wilson wrote:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11803d485275..7c7afe99986c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2008,7 +2008,8 @@ struct drm_i915_private {
intel_engine_mask_t active_engines;
struct list_head active_rings;
struct list_head closed_vma;
- u32 active_requests;
+ atomic_t active_requests;
+ struct mutex active_mutex;
My initial reaction is why not gem_pm_mutex to match where code was
moved and what the commit message says?
Because we are inheriting the name from active_requests. If we renane
that to active_count, and maybe pm_wake_count?
pm_active_count?
pm_wake_count sounds good from the PM angle. But it's then wrong for all
places which query active requests. Don't know.
+ mutex_lock(&i915->gt.active_mutex);
if (!work_pending(&i915->gt.idle_work.work) &&
- !i915->gt.active_requests) {
- ++i915->gt.active_requests; /* don't requeue idle */
+ !atomic_read(&i915->gt.active_requests)) {
+ atomic_inc(&i915->gt.active_requests); /* don't requeue idle */
atomic_inc_not_zero?
atomicity of the read & inc are not strictly required, once
active_requests is zero it cannot be raised without holding
active_mutex.
Yeah my bad. atomic_inc_not_zero would be wrong, it is the opposite. You
would need atomic_inc_if_zero here.
@@ -191,11 +183,29 @@ void i915_gem_unpark(struct drm_i915_private *i915)
round_jiffies_up_relative(HZ));
}
+void i915_gem_unpark(struct drm_i915_private *i915)
+{
+ if (atomic_add_unless(&i915->gt.active_requests, 1, 0))
+ return;
This looks wrong - how can it be okay to maybe not increment
active_requests on unpark? What am I missing?
If the add succeeds, active_requests was non-zero, and we can skip waking
up the device. If the add fails, active_requests might be zero, so we
take the mutex and check.
True true.. so this one is atomic_inc_not_zero! :)
I would expect here you would need
"atomic_inc_and_return_true_if_OLD_value_was_zero" but I don't think
there is such API.
+
+ mutex_lock(&i915->gt.active_mutex);
+ if (!atomic_read(&i915->gt.active_requests)) {
+ GEM_TRACE("\n");
+ __i915_gem_unpark(i915);
+ smp_mb__before_atomic();
Why is this needed? I have no idea.. but I think we want comments with
all memory barriers.
Because atomic_inc() is not regarded as a strong barrier, so we turn it
into one. (In documentation at least, on x86 it's just a compiler
barrier().)
Document why we need it please.
+ }
+ atomic_inc(&i915->gt.active_requests);
+ mutex_unlock(&i915->gt.active_mutex);
+}
+
bool i915_gem_load_power_context(struct drm_i915_private *i915)
{
+ mutex_lock(&i915->gt.active_mutex);
+ atomic_inc(&i915->gt.active_requests);
Why this function has to manually manage active_requests? Can it be
written in a simpler way?
It's so that this function has control over parking.
This is the first request, this is the place where we will make the
explicit calls to set up the powersaving contexts and state -- currently,
it's implicit on the first request.
Why does it need control over parking? Does it need a comment perhaps? :)
Regards,
Tvrtko
/* Force loading the kernel context on all engines */
if (!switch_to_kernel_context_sync(i915, ALL_ENGINES))
- return false;
+ goto err_active;
/*
* Immediately park the GPU so that we enable powersaving and
@@ -203,9 +213,20 @@ bool i915_gem_load_power_context(struct drm_i915_private *i915)
* unpark and start using the engine->pinned_default_state, otherwise
* it is in limbo and an early reset may fail.
*/
+
+ if (!atomic_dec_and_test(&i915->gt.active_requests))
+ goto err_unlock;
+
__i915_gem_park(i915);
+ mutex_unlock(&i915->gt.active_mutex);
return true;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
