Re: [PATCH 08/11] drm/i915/uc: Simplify firmware path handling

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

On 13/03/2017 13:48, Arkadiusz Hiler wrote:
> On Mon, Mar 13, 2017 at 01:39:45PM +0000, Tvrtko Ursulin wrote:
>>
>> On 13/03/2017 13:15, Arkadiusz Hiler wrote:
>>> Currently fw->path values can represent one of three possible states:
>>>
>>>  1) NULL - device without the uC
>>>  2) '\0' - device with the uC but have no firmware
>>>  3) else - device with the uC and we have firmware
>>>
>>> Second case is used only to WARN at a later stage.
>>>
>>> We can WARN right away and merge cases 1 and 2.
>>>
>>> Code can be even further simplified and common (HuC/GuC logic) happening
>>> right before the fetch can be offloaded to the common function.
>>>
>>> v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
>>> v3: DRM_ERROR instead of WARN (M. Wajdeczko)
>>> v4: coding standard (J. Lahtinen)
>>> v5: non-trivial rebase
>>> v6: remove path check, we are checking fetch status (M. Wajdeczko)
>>>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_guc_loader.c | 36 ++++++++++-----------------------
>>>  drivers/gpu/drm/i915/intel_huc.c        | 21 ++++++-------------
>>>  drivers/gpu/drm/i915/intel_uc.c         |  4 +++-
>>>  3 files changed, 20 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> index 0a29c1b..d731f68 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> @@ -368,13 +368,6 @@ int intel_guc_init_hw(struct intel_guc *guc)
>>>  		intel_uc_fw_status_repr(guc->fw.fetch_status),
>>>  		intel_uc_fw_status_repr(guc->fw.load_status));
>>>
>>> -	if (!fw_path) {
>>> -		return -ENXIO;
>>> -	} else if (*fw_path == '\0') {
>>> -		WARN(1, "No GuC firmware known for this platform!\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>>  	if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
>>>  		return -EIO;
>>>
>>> @@ -399,7 +392,6 @@ int intel_guc_init_hw(struct intel_guc *guc)
>>>  	return 0;
>>>  }
>>>
>>> -
>>>  /**
>>>   * intel_guc_init_fw() - select and prepare firmware for loading
>>>   * @guc:	intel_guc struct
>>> @@ -412,37 +404,31 @@ int intel_guc_init_hw(struct intel_guc *guc)
>>>  void intel_guc_init_fw(struct intel_guc *guc)
>>>  {
>>>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>> -	const char *fw_path;
>>> +
>>> +	guc->fw.path = NULL;
>>> +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
>>> +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
>>> +	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
>>
>> If not too hard on the series and all, maybe bikeshed this field to "type".
>
> Makes sense. Can we do that as a separate patch afterwards?

Fine by me.

>> More importantly, this field wasn't getting set before? I don't see that it
>> got moved in this diff.
>
> Huh. Quick grep on drm-tip revealed that this was not set, only checked
> against. Guess it worked due to pure luck.

:) Okay. This one will be re-spun then?

>>>
>>>  	if (IS_SKYLAKE(dev_priv)) {
>>> -		fw_path = I915_SKL_GUC_UCODE;
>>> +		guc->fw.path = I915_SKL_GUC_UCODE;
>>>  		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
>>>  		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
>>>  	} else if (IS_BROXTON(dev_priv)) {
>>> -		fw_path = I915_BXT_GUC_UCODE;
>>> +		guc->fw.path = I915_BXT_GUC_UCODE;
>>>  		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
>>>  		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
>>>  	} else if (IS_KABYLAKE(dev_priv)) {
>>> -		fw_path = I915_KBL_GUC_UCODE;
>>> +		guc->fw.path = I915_KBL_GUC_UCODE;
>>>  		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
>>>  		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
>>>  	} else {
>>> -		fw_path = "";	/* unknown device */
>>> +		DRM_ERROR("No GuC firmware known for platform with GuC!\n");
>>
>> Quick glance over the series suggests intel_guc_init_fw is called
>> unconditionally from i915_load_modeset_init meaning this error gets logged
>> on all non-GuC platforms? I must be missing something..
>
> intel_uc_init_fw which call that, returns early if !enable_guc_loading.
>
> in intel_uc_snitize_options():
> 	if (!HAS_GUC) enable_guc_loading = 0
>
> The flow is further improved by next patch.

Ah, I was foiled by diff output. It is difficult to follow this series 
if one is not 100% invested into it. Looks good to me then.

Assuming the mysterious fw type (fw.fw) assignment is resolved outside 
this patch:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx