Re: [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader

Daniel Vetter
2015-07-13T15:32:20Z

On Thu, Jul 09, 2015 at 07:29:05PM +0100, Dave Gordon wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> This fetches the required firmware image from the filesystem,
> then loads it into the GuC's memory via a dedicated DMA engine.
>
> This patch is derived from GuC loading work originally done by
> Vinit Azad and Ben Widawsky.
>
> v2:
> Various improvements per review comments by Chris Wilson
>
> v3:
> Removed 'wait' parameter to intel_guc_ucode_load() as firmware
> prefetch is no longer supported in the common firmware loader,
> per Daniel Vetter's request.
> Firmware checker callback fn now returns errno rather than bool.
>
> v4:
> Squash uC-independent code into GuC-specifc loader [Daniel Vetter]
> Don't keep the driver working (by falling back to execlist mode)
> if GuC firmware loading fails [Daniel Vetter]
>
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

I think this is it, one comment below.

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dbbb649..e020309 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5074,6 +5074,19 @@ i915_gem_init_hw(struct drm_device *dev)
> goto out;
> }
>
> + /* We can't enable contexts until all firmware is loaded */
> + ret = intel_guc_ucode_load(dev);

To stay in line with the current flow I think the request_firmware +
create fw bo object code should be move into gem_init so that gem_init_hw
is only responsible for loading the fw in the bo into hw.

That will take care of not trying to re-request the firmware from
userspace in resume/gpu reset code, which should simplify the status
handling a lot.

Also with just declaring the gpu wedged we could instead move the check
below into the init_hw part of the guc load process. That would nicely
encapsulate that decision and I'd expect take care of the other status
codes - in the end callers really only care about -EIO or not here.

But imo we can polish that after merging. All my other higher-level
concerns with this have been addressed, so I think this is good to go in
after detailed code review.

Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation