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

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader
Date: Mon, 13 Jul 2015 17:35:01 +0200	[thread overview]
Message-ID: <20150713153501.GX3736@phenom.ffwll.local> (raw)
In-Reply-To: <1436466554-24806-5-git-send-email-david.s.gordon@intel.com>

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
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx