Re: [PATCH 5/6] drm/i915: Expose RPCS (SSEU) configuration to userspace

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/09/2018 17:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-14 17:09:31)
>> +static int
>> +gen8_modify_rpcs_gpu(struct intel_context *ce,
>> +                    struct intel_engine_cs *engine,
>> +                    struct intel_sseu sseu)
>> +{
>> +       struct drm_i915_private *i915 = engine->i915;
>> +       struct i915_request *rq, *prev;
>> +       int ret;
>> +
>> +       GEM_BUG_ON(!ce->pin_count);
>> +
>> +       lockdep_assert_held(&i915->drm.struct_mutex);
>> +
>> +       /* Submitting requests etc needs the hw awake. */
>> +       intel_runtime_pm_get(i915);
>> +
>> +       rq = i915_request_alloc(engine, i915->kernel_context);
>> +       if (IS_ERR(rq)) {
>> +               ret = PTR_ERR(rq);
>> +               goto out_put;
>> +       }
>> +
>> +       ret = gen8_emit_rpcs_config(rq, ce, sseu);
>> +       if (ret)
>> +               goto out_add;
>> +
>> +       /* Queue this switch after all other activity by this context. */
>> +       prev = i915_gem_active_raw(&ce->ring->timeline->last_request,
>> +                                  &i915->drm.struct_mutex);
>> +       if (prev && !i915_request_completed(prev))
>> +               i915_sw_fence_await_sw_fence_gfp(&rq->submit,
>> +                                                &prev->submit,
>> +                                                I915_FENCE_GFP);
> 
> I guess we really should be respecting the potential error here.
> We should do the await before the emit, and out_add on err < 0.

Yep, completely missed it can return error even though GFP_KERNEL should 
have been a clue enough.

>> +
>> +       /* Order all following requests to be after. */
>> +       i915_timeline_set_barrier(ce->ring->timeline, rq);
>> +
>> +       /*
>> +        * Guarantee context image and the timeline remains pinned until the
>> +        * modifying request is retired by setting the ce activity tracker.
>> +        *
>> +        * But we only need to take one pin on the account of it. Or in other
>> +        * words transfer the pinned ce object to tracked active request.
>> +        */
>> +       if (!i915_gem_active_isset(&ce->active))
>> +               __intel_context_pin(ce);
>> +       i915_gem_active_set(&ce->active, rq);
>> +
>> +out_add:
>> +       i915_request_add(rq);
>> +out_put:
>> +       intel_runtime_pm_put(i915);
>> +
>> +       return ret;
>> +}
>> +
>> +static int
>> +i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
>> +                                 struct intel_engine_cs *engine,
>> +                                 struct intel_sseu sseu)
>> +{
>> +       struct intel_context *ce = to_intel_context(ctx, engine);
>> +       int ret;
>> +
>> +       GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8);
>> +       GEM_BUG_ON(engine->id != RCS);
>> +
>> +       ret = mutex_lock_interruptible(&ctx->i915->drm.struct_mutex);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Nothing to do if unmodified. */
>> +       if (!memcmp(&ce->sseu, &sseu, sizeof(sseu)))
>> +               goto out;
> 
> /* If oa is active, it has already overridden the per-context setting */
> if (oa->active)
> 	goto set;

I don't like sprinkling knowledge of OA to more places than is 
unavoidable. As such I prefer to centralize it to gen8_make_rpcs. Only 
downside to not do it is an useless re-configuration request, but I 
don't think we should care to optimize for the OA enabled case. If 
anything it could make the overall system perform better when analysed 
than when not. :)

Regards,

Tvrtko

>> +
>> +       /*
>> +        * If context is not idle we have to submit an ordered request to modify
>> +        * its context image via the kernel context. Pristine and idle contexts
>> +        * will be configured on pinning.
>> +        */
>> +       if (ce->pin_count)
>> +               ret = gen8_modify_rpcs_gpu(ce, engine, sseu);
>> +
>> +       if (!ret)
> 
> set:
> 
>> +               ce->sseu = sseu;
>> +
>> +out:
>> +       mutex_unlock(&ctx->i915->drm.struct_mutex);
>> +
>> +       return ret;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx