Re: [PATCH 12/18] drm/i915: Unify request submission

Dave Gordon
2016-07-28T10:25:41Z

On 27/07/16 19:09, Chris Wilson wrote:
> On Wed, Jul 27, 2016 at 06:51:35PM +0100, Dave Gordon wrote:
>>>> @@ -1006,6 +1005,10 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>>>> host2guc_sample_forcewake(guc, client);
>>>> guc_init_doorbell_hw(guc);
>>>>
>>>> + /* Take over from manual control of ELSP (execlists) */
>>>> + for_each_engine(engine, dev_priv)
>>>> + engine->submit_request = i915_guc_submit;
>>
>> This doesn't get undone in i915_guc_submission_disable().
>> That will prevent the runtime fallback from working.
>
> I honestly wasn't sure if that was supported. (runtime enabling would be
> nice...)

Any time the GuC (re)loading process fails, it will revert (forever) to
execlists mode. At present there's no way to switch back to GuC mode
thereafter. Of course, we don't actually *expect* it to fail on a
reload, but we did observe this in action a while back, before we
discovered why reload on resume didn't always work. That's fixed now,
but the fallback is still there just to make sure that any undiscovered
issues around GuC reload don't leave you with a blank screen and an
unusable machine.

> Would calling a hypothetical intel_execlists_submission_enable() be ok?
> -Chris

Something like that, I guess, although since GuC submission mode is
treated as execlist mode for almost all purposes then _enable() and its
presumed complement _disable() probably aren't the right terms (we don't
want to suggest that enabling GuC mode involves disabling execlist
mode). Perhaps _activate() for a function which simply (re)installs the
vfunc pointers, with no corresponding _deactivate() required, as
activating a new mode implicitly deactivates the old mode (i.e. makes it
unreachable). That little loop can then be moved into its own trivial
guc_submission_activate() function for clarity; and the GuC code would
call intel_execlists_submission_activate() as part of reverting from GuC
to execlists (at the moment it relies on just clearing
enable_guc_submission, but presumably you'd like as few runtime tests of
that as possible).

.Dave.