Re: [PATCH 1/9 v6] drm/i915: GuC-specific firmware loader

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 12, 2015 at 03:43:36PM +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]
> 
> v5:
>     Clarify WOPCM-related #defines [Tom O'Rourke]
>     Delete obsolete code no longer required with current h/w & f/w
>         [Tom O'Rourke]
>     Move the call to intel_guc_ucode_init() later, so that it can
>         allocate GEM objects, and have it fetch the firmware; then
> 	intel_guc_ucode_load() doesn't need to fetch it later.
>         [Daniel Vetter].
> 
> v6:
>     Update comment describing intel_guc_ucode_load() [Tom O'Rourke]
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Checkpatch was pretty unhappy with long lines and alignment of
continuations. But didn't look too bad overall so pulled it in.

> +/*
> + * Transfer the firmware image to RAM for execution by the microcontroller.
> + *
> + * GuC Firmware layout:
> + * +-------------------------------+  ----
> + * |          CSS header           |  128B
> + * | contains major/minor version  |
> + * +-------------------------------+  ----
> + * |             uCode             |
> + * +-------------------------------+  ----
> + * |         RSA signature         |  256B
> + * +-------------------------------+  ----
> + * |         RSA public Key        |  256B
> + * +-------------------------------+  ----
> + * |       Public key modulus      |    4B
> + * +-------------------------------+  ----
> + *
> + * Architecturally, the DMA engine is bidirectional, and can potentially even
> + * transfer between GTT locations. This functionality is left out of the API
> + * for now as there is no need for it.
> + *
> + * Note that GuC needs the CSS header plus uKernel code to be copied by the
> + * DMA engine in one operation, whereas the RSA signature is loaded via MMIO.

Imo this should be included in the kerneldoc too, especially now that we
can handle tables with markdown.
-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