Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup

Dave Gordon
2016-07-14T11:33:54Z

On 13/07/16 14:16, Tvrtko Ursulin wrote:
>
> On 13/07/16 13:23, Daniel Vetter wrote:
>> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>>> From: Dave Gordon <david.s.gordon@intel.com>
>>>
[snip]
>>> {
>>> - const struct logical_ring_info *info = &logical_rings[id];
>>> + const struct engine_info *info = &intel_engines[id];
>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
>>> - enum forcewake_domains fw_domains;
>>>
>>> engine->id = id;
>>> + engine->i915 = dev_priv;
>>> engine->name = info->name;
>>> engine->exec_id = info->exec_id;
>>> - engine->guc_id = info->guc_id;
>>> + engine->hw_id = engine->guc_id = info->guc_id;
>>
>> Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
>> context. Or nuking engine->guc_id.
>
> Someone said we cannot be sure they will be the same in the future. So
> maybe just rename to hw_id for now.
>
> Regards,
> Tvrtko

The GuC firmware *could* use a completely different enumeration of the
engines, but why would it? Since it's closely tied to the hardware, it
*ought* to use the same naming scheme as the h/w. So I have no objection
to getting rid of guc_id entirely, and changing existing uses to use
hw_id instead.

OTOH it seems little benefit and would certainly involve more work to
reverse. The firmware team might, after all, decide that they too want
to decouple the logical-engine-numbers used for the KMD interface from
whatever the hardware team decide is the best way to number engines --
which might after all change between generations or even different SKUs
of the same device!

SO, I think the "best" version of that line is probably:

+ engine->hw_id = info->hw_id;
+
+ /* Current GuC f/w uses hw_id not driver id */
+ engine->guc_id = info->hw_id;

and we'll add "info->guc_id" back again if it ever becomes necessary.

.Dave.