Re: [PATCH v6] drm/i915/guc: Simplify intel_guc_init_hw()

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2017-03-10 at 12:46 +0100, Arkadiusz Hiler wrote:
> Current version of intel_guc_init_hw() does a lot:
>  - cares about submission
>  - loads huc
>  - implement WA
> 
> This change offloads some of the logic to intel_uc_init_hw(), which now
> cares about the above.
> 
> v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> v3: rename once again
> v4: remove spurious comments and add some style (J. Lahtinen)
> v5: flow changes, got rid of dead checks (M. Wajdeczko)
> v6: rebase
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

<SNIP>

> +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> +{
> +	int ret, attempts;
> +	const int guc_wa_hash_check_not_set_attempts = 3;

I wouldn't bother with this const, the Wa comment makes it pretty clear
already.

Extra newline here, too.

<SNIP>

> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	DRM_ERROR("GuC init failed\n");
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +

I'd drop the above code to the bottom.

> +	i915_ggtt_disable_guc(dev_priv);
> +	intel_guc_release_interrupts(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);

And make this teardown oniony with some more labels.

With those changes, looks good to me.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx