Quoting Tvrtko Ursulin (2019-04-01 16:54:53)
>
> 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 had forgotten about that one!
> >> 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.
Imagine if there was an atomic_inc_unlock() or atomic_inc_release(). I
suppose atomic_add_return_release(), with the caveat that we only need
the release if we actually did the deed.
> >>> + }
> >>> + 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? :)
There is a comment! See "Immediately park the GPU..."
> >>> /* 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
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
