[igt-dev] [PATCH v3 2/3] lib: implement new engine discovery interface

[Tvrtko Ursulin]

On 27/11/2018 20:33, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +	ctx_param.ctx_id = ctx_id;
>>> +	ctx_param.size = size;
>>> +	ctx_param.param = I915_CONTEXT_PARAM_ENGINES;
>>> +	ctx_param.value = to_user_pointer(ctx_engine);
>>> +
>>> +	/* check whether we free the engines */
>>> +	igt_require(!ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &ctx_param));
>>> +
>>> +	free(ctx_engine);
>>
>> Leaks on skip, not that it matters a lot, but the comments makes me think
>> you wanted to handle it.
> 
> The comment is a leftover from the cleanup. I need to fix/remove
> it.
> 
> While as for freeing ctx_engine in case of failure, some ugly code
> would be required. I think it's cleaner to leave it as it is,
> anyway 'igt_require' exits in case of failure.

Yeah, ok.

>>> +#define for_each_engine_ctx(fd, ctx, e) \
>>> +		for (int __m = __gem_setup_ctx_engines(fd, ctx), __e = e = 1; \
>>> +				__e <= __m; e = ++__e)
>>> +
>>
>> Is __e needed? Could just use passed in e?
> 
> I cannot mix declarations and "already declared variables"
> initializations in the first expression in 'for'. If I do
> something like:
> 
>    for (int __m = __gem_setup_ctx_engines(), e = 1; ... )
> 
> I would re-define 'e' and it would be a different variable
> outside the loop.
> 
> So that either I declare '__m' outside, or I initialize 'e'
> outside, or I use the double 'for' loop as it was done
> previously, or do some ugly tricks.
> 
> It looked simplier to define an '__e'.
> 
> Am I missing anything?

No, looks like you're right, makes sense.

>>>    bool gem_ring_is_physical_engine(int fd, unsigned int ring);
>>>    bool gem_ring_has_physical_engine(int fd, unsigned int ring);
>>> +int __gem_setup_ctx_engines(int fd, uint32_t ctx_id);
>>>    bool gem_can_store_dword(int fd, unsigned int engine);
>>>
>>
>> Looks okay. But we need to decide whether we want the iterator to be a
>> struct sooner rather than later now.
>>
>> I think if you go and convert one of the tests which uses
>> for_each_physical_engine to enumerate subtests and so, it will become
>> clearer what approach work better. (struct iterator, or helpers to get data
>> from engine index.)
> 
> All right, I'll try it out and I will post something as a reply to this
> patch.

Ack.

Regards,

Tvrtko