[Intel-gfx] [PATCH 1/3] drm/i915/guc: fix GuC loading/submission check

[Dave Gordon]

On 09/06/16 12:04, Tvrtko Ursulin wrote:
>
> On 07/06/16 09:41, Tvrtko Ursulin wrote:
>>
>> On 07/06/16 09:14, Dave Gordon wrote:
>>> The last stage of the GuC loader also sanitises the GuC submission
>>> settings, so should be called unconditionally (even on platforms
>>> without a GuC) to ensure consistent settings; in particular, this
>>> prevents any attempt to use GuC submission on GuCless platforms!
>>>
>>> Also fix error path handling and clarify DRM_INFO fallback message.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c         |  8 +++-----
>>>   drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++++----
>>>   2 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index 1bfc260..eae8d7a 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4930,11 +4930,9 @@ int i915_gem_init_engines(struct drm_device *dev)
>>>       intel_mocs_init_l3cc_table(dev);
>>>
>>>       /* We can't enable contexts until all firmware is loaded */
>>> -    if (HAS_GUC(dev)) {
>>> -        ret = intel_guc_setup(dev);
>>> -        if (ret)
>>> -            goto out;
>>> -    }
>>> +    ret = intel_guc_setup(dev);
>>> +    if (ret)
>>> +        goto out;
>>>
>>>       /*
>>>        * Increment the next seqno by 0x100 so we have a visible break
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> index f2b88c7..4e34c2e 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> @@ -425,9 +425,13 @@ int intel_guc_setup(struct drm_device *dev)
>>>       if (!i915.enable_guc_loading) {
>>>           err = 0;
>>>           goto fail;
>>> -    } else if (fw_path == NULL || *fw_path == '\0') {
>>> -        if (*fw_path == '\0')
>>
>> Ops. I can only assume I meant !fw_path.
>>
>>> -            DRM_INFO("No GuC firmware known for this platform\n");
>>> +    } else if (fw_path == NULL) {
>>> +        /* Device is known to have no uCode (e.g. no GuC) */
>>> +        err = -ENXIO;
>>> +        goto fail;
>>> +    } else if (*fw_path == '\0') {
>>> +        /* Device has a GuC but we don't know what f/w to load? */
>>> +        DRM_INFO("No GuC firmware known for this platform\n");
>>>           err = -ENODEV;
>>>           goto fail;
>>>       }
>>> @@ -535,7 +539,7 @@ int intel_guc_setup(struct drm_device *dev)
>>>           if (fw_path == NULL)
>>>               DRM_INFO("GuC submission without firmware not
>>> supported\n");
>>>           if (ret == 0)
>>> -            DRM_INFO("Falling back to execlist mode\n");
>>> +            DRM_INFO("Falling back from GuC submission to execlist
>>> mode\n");
>>>           else
>>>               DRM_ERROR("GuC init failed: %d\n", ret);
>>>       }
>>>
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Bah now on BDW we get on boot:
>
> [drm:gen8_init_common_ring] Execlists enabled for render ring
> [drm:gen8_init_common_ring] Execlists enabled for blitter ring
> [drm:gen8_init_common_ring] Execlists enabled for bsd ring
> [drm:gen8_init_common_ring] Execlists enabled for video enhancement ring
> [drm:intel_guc_setup] GuC fw status: path (null), fetch NONE, load NONE
> [drm] GuC firmware load skipped
> [drm] GuC submission without firmware not supported
> [drm] Falling back from GuC submission to execlist mode
>
> Which is a bit untidy. :(
>
> Regards,
> Tvrtko

You shouldn't get the second or third [drm] message, if you haven't 
overridden the default values. The default for enable_guc_submission
is (-1) which gets mapped to HAS_GUC_SCHED() which is 0 on BDW; you only 
get the extra messages if you've set it to a non-default value.

.Dave.