[Intel-gfx] [PATCH 1/2] drm/i915: Teach record_defaults to operate on the intel_gt

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-10-22 16:43:17)
> 
> On 22/10/2019 14:56, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-10-22 14:40:42)
> >>
> >> On 22/10/2019 14:02, Chris Wilson wrote:
> >>> Again we wish to operate on the engines, which are owned by the
> >>> intel_gt. As such it is easier, and much more consistent, to pass the
> >>> intel_gt parameter.
> >>>
> >>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++------
> >>>    1 file changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>> index ca64a0c9b762..b882988056bd 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>> @@ -48,6 +48,7 @@
> >>>    #include "gt/intel_engine_user.h"
> >>>    #include "gt/intel_gt.h"
> >>>    #include "gt/intel_gt_pm.h"
> >>> +#include "gt/intel_gt_requests.h"
> >>>    #include "gt/intel_mocs.h"
> >>>    #include "gt/intel_reset.h"
> >>>    #include "gt/intel_renderstate.h"
> >>> @@ -1072,7 +1073,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
> >>>        intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> >>>    }
> >>>    
> >>> -static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>> +static int __intel_engines_record_defaults(struct intel_gt *gt)
> >>>    {
> >>>        struct i915_request *requests[I915_NUM_ENGINES] = {};
> >>>        struct intel_engine_cs *engine;
> >>> @@ -1088,7 +1089,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>         * from the same default HW values.
> >>>         */
> >>>    
> >>> -     for_each_engine(engine, i915, id) {
> >>> +     for_each_engine(engine, gt, id) {
> >>>                struct intel_context *ce;
> >>>                struct i915_request *rq;
> >>>    
> >>> @@ -1096,7 +1097,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>                GEM_BUG_ON(!engine->kernel_context);
> >>>                engine->serial++; /* force the kernel context switch */
> >>>    
> >>> -             ce = intel_context_create(i915->kernel_context, engine);
> >>> +             ce = intel_context_create(engine->kernel_context->gem_context,
> >>> +                                       engine);
> >>>                if (IS_ERR(ce)) {
> >>>                        err = PTR_ERR(ce);
> >>>                        goto out;
> >>> @@ -1125,7 +1127,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>        }
> >>>    
> >>>        /* Flush the default context image to memory, and enable powersaving. */
> >>> -     if (!i915_gem_load_power_context(i915)) {
> >>> +     if (intel_gt_wait_for_idle(gt, I915_GEM_IDLE_TIMEOUT) == -ETIME) {
> >>
> >> What are the plans for i915_gem_load_power_context? It does a little bit
> >> extra. But also becomes confined to i915_gem_pm.c if not needed here any
> >> more so could be unexported.
> > 
> > It's to be subsumed entirely onto the gt. On resume, we simply call
> > intel_gt_resume() which does the power saving setup and ensures we have
> > a kernel_context primed. I'm still waiting on Andi's overhaul of GT
> > powersaving to land before pulling the plug.
> 
> Why it is bad, or not needed, to call intel_gt_pm_wait_for_idle at this 
> point?

It's just an extra delay that isn't required before bringing up
userspace. It should be a miniscule delay, but it's the principle of the
thing! The implicit dependency on the selftests of a completely idle
system has its own shortcomings.
-Chris