[RFC PATCH v2 17/17] WIP: drm/tegra: Implement new UAPI

[Mikko Perttunen]

On 9/11/20 7:30 PM, Dmitry Osipenko wrote:
> 11.09.2020 12:59, Mikko Perttunen пишет:
>> On 9/11/20 12:57 AM, Dmitry Osipenko wrote:
>>> 09.09.2020 11:36, Mikko Perttunen пишет:
>>> ...
>>>>>
>>>>> Does it make sense to have timeout in microseconds?
>>>>>
>>>>
>>>> Not sure, but better have it a bit more fine-grained rather than
>>>> coarse-grained. This still gives a maximum timeout of 71 minutes so I
>>>> don't think it has any negatives compared to milliseconds.
>>>
>>> If there is no good reason to use microseconds right now, then should be
>>> better to default to milliseconds, IMO. It shouldn't be a problem to
>>> extend the IOCLT with a microseconds entry, if ever be needed.
>>>
>>> {
>>>      __u32 timeout_ms;
>>> ...
>>>      __u32 timeout_us;
>>> }
>>>
>>> timeout = timeout_ms + 1000 * timeout_us;
>>>
>>> There shouldn't be a need for a long timeouts, since a job that takes
>>> over 100ms is probably too unpractical. It also should be possible to
>>> detect a progressing job and then defer timeout in the driver. At least
>>> this is what other drivers do, like etnaviv driver for example:
>>>
>>> https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/gpu/drm/etnaviv/etnaviv_sched.c#L107
>>>
>>>
>>
>> I still don't quite understand why it's better to default to
>> milliseconds? As you say, there is no need to have a long timeout, and
>> if we go microseconds now, then there wouldn't be a need to extend in
>> the future.
> 
> It will nicer to avoid unnecessary unit-conversions in the code in order
> to keep it cleaner.

We can change all the internals to use microseconds as well. We 
eventually have to convert it to jiffies anyway, so the unit before that 
shouldn't matter much.

> 
> I'm now also a bit dubious about that the timeout field of the submit
> IOCTL will be in the final UAPI version because it should become
> obsolete once drm-scheduler will be hooked up, since the hung-check
> timeout will be specified per-hardware engine within the kernel driver
> and there won't be much use for the user-defined timeout.
> 

Perhaps we can omit this field for now. Looking at it, it's primarily 
used for tests, and for that we could add a debugfs knob to adjust the 
timeout if needed.

Mikko