Re: [PATCH v7] drm/i915: Engine discovery query

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 04/10/2018 15:32, Joonas Lahtinen wrote:
> Some comments below, mostly related to trying to keep the uapi header
> nice and tidy.
> 
> Quoting Tvrtko Ursulin (2018-10-04 14:32:48)
>> @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info {
>>          __u8 data[];
>>   };
>>   
>> +/**
>> + * struct drm_i915_engine_info
>> + *
>> + * Describes one engine known to the driver, whether or not it is an user-
>> + * accessible or hardware only engine, and what are it's capabilities where
>> + * applicable.
>> + */
>> +struct drm_i915_engine_info {
>> +       /** Engine class as in enum drm_i915_gem_engine_class. */
>> +       __u16 class;
>> +
>> +       /** Engine instance number. */
>> +       __u16 instance;
>> +
>> +       /** Reserved field must be cleared to zero. */
>> +       __u32 rsvd0;
> 
> u32 class, u32 instance just to put the padding to good use?

There is some attractiveness to lose the padding, but I think in general 
we trashed it out to be u16:u16. So it is a question of consistency vs 
elegance and I give preference to consistency.

Chris, is your recollection also that we said u16:u16 for class:instance 
in all uAPI?

> 
>> +
>> +       /** Engine flags. */
>> +       __u64 flags;
>> +
>> +       /** Capabilities of this engine. */
>> +       __u64 capabilities;
>> +#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0)
>> +#define I915_VCS_CLASS_CAPABILITY_SFC  (1 << 1)
>> +
>> +       /** Reserved fields must be cleared to zero. */
>> +       __u64 rsvd1[4];
> 
> Why this at the end of the struct? We have flags which we should be able
> to use for new versions.

This is to allow some growing space without complicating the userspace 
array member start calculation.

If we have to grow struct engine_info itself when adding a new member, 
then we have to define the array (via documentation) as potentially not 
aligned to sizeof(struct engine_info) as known by userspace.

I don't have such a big aversion to rsvd fields so for me it seems 
easier to have some, with the benefit of simpler userspace code.

But if the consensus will be to change it, no problem.

> 
>> +};
>> +
>> +/**
>> + * struct drm_i915_query_engine_info
>> + *
>> + * Engine info query enumerates all engines known to the driver by filling in
>> + * an array of struct drm_i915_engine_info structures.
>> + */
>> +struct drm_i915_query_engine_info {
>> +       /** Number of struct drm_i915_engine_info structs following. */
>> +       __u32 num_engines;
>> +
>> +       /** MBZ */
> 
> Just copy the non-abbreviated comment from other reserved fields.

I need to nuke this comment since from v6 code is not mandating MBZ for 
the array.

> 
>> +       __u32 rsvd[3];
>> +
> 
> Might as well do just __u32 flags, which must be zero?

Could do flags..

> 
> I don't think we need to get too excited about adding the reserved
> fields in every spot :)

... but I do like my rsvd! :))

Regards,

Tvrtko

> Regards, Joonas
> 
>> +       /** Marker for drm_i915_engine_info structures. */
>> +       struct drm_i915_engine_info engines[];
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> -- 
>> 2.17.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx