Re: [PATCH 04/15] drm/i915: Add GuC-related header files

Dave Gordon
2015-06-17T15:02:04Z

On 15/06/15 21:20, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 07:36:22PM +0100, Dave Gordon wrote:
>> From: Alex Dai <yu.dai@intel.com>
>>
>> intel_guc_api.h contains the subset of the GuC interface that we
>> will need for submission of commands through the GuC. These MUST
>> be kept in sync with the definitions used by the GuC firmware.
>
> intel_guc_hw.h or intel_guc_abi.h then. Calling it API doesn't make it
> clear whose API you are talking about.

It's not 'hw' -- the hw register definitions are elsewhere, because they
don't depend on the firmware. What it defines is a set of interfaces
between the GuC firmware and the KMD, so I'll rename it to reflect that
("intel_guc_fwif.h", for FirmWareInterFace).

>> intel_guc.h defines structures and parameters relevant to loading
>> the GuC firmware and setting it running. Some of these also need
>> to be kept in sync with the firmware.
>
> intel_guc.h should be developed organically as features are added in the
> series so that it is possible to track against implementation.

> Certainly not in a patch that adds the entirety of the firmware ABI.

What may not be obvious is that intel_guc_api.h (or intel_guc_fwif.h, as
I'm now going to call it) is autogenerated from the non-Linux-friendly
version actually used in building the GuC firmware. (Or at least, that's
the PoR; in practice Alex has hacked^W hand-tuned this version.) So it
makes no sense to break it into parts.

We /could/ do that with the purely KMD-defined structures in intel_guc.h
such as intel_guc, and /maybe/ i915_guc_client. OTOH when it's a new
file, containing a new structure, it's easier to see that the layout is
sensible when it's all added in one go, rather than repeatedly adding
bits here and there, especially if the logical order of fields in a
structure isn't going to be the same as the order of addition of the
code that uses them.

I'll see how it looks ...

.Dave.