Re: [PATCH] drm/i915: Replace some more busy waits with normal ones

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes:

> [ text/plain ]
>
> Should have sent this as RFC..
>
> On 23/03/16 14:32, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> When I added an assert to catch non-atomic users of
>> wait_for_atomic_us in 0351b93992aa463cc3e7f358ddec2709f9390756
>> ("drm/i915: Do not lie about atomic timeout granularity"),
>> I have missed some callers which use it from obviously
>> non-atomic context.
>>
>> Replace them with sleeping waits which support micro-second
>> timeout granularity since 3f177625ee896f5d3c62fa6a49554a9c0243bceb
>> ("drm/i915: Add wait_for_us").
>>
>> Note however than a fix for wait_for is needed to a clock with
>> more granularity than jiffies. In the above referenced patch
>> I have switched the arguments to micro-seconds, but failed to
>> upgrade the clock as well, as Mika has later discovered.
>>
>> Open question here is whether we should allow sleeping waits
>> of less than 10us which usleep_range recommends against. And
>> this patch actually touches one call site which asks for 1us
>> timeout.
>>
>> These might be better served with wait_for_atomic_us, in which
>> case the inatomic warning there should be made dependant on
>> the requested timeout.
>
> For discussion - does the above sound like a better plan than this 
> patch? To sum up my proposal:
>

What I have aimed for was that we only have wait_for and wait_for_atomic.

The sleeping one operates on 1ms granularity and the nonsleeping
one on usecs.

> 1. Allow wait for_atomic_us for < 10us waits and keep using it for such 
> waiters.

I have modified the wait_for to do few busy cycles on the
start of the wait and then adaptive backoff if condition is not
yet met. In hopes that we could convert few atomic_waits for this.

> 2. Upgrade the clock in wait_for to something more precise than jiffies 
> so timeouts from 10us and up can be handled properly. Note that 
> currently this is  only and issue in the failure/timeout mode. In the 
> expected case the current implementation is fine.
>

I would not go this route. If you really really want <1ms response
this should be explicit in the callsite. Disclaimer: i don't
know all the callsites and requirements.

> Equally as under 1), put a BUILD_BUG_ON in wait_for for <10us waits.
>

This is what I had in mind (wip/rfc):
https://cgit.freedesktop.org/~miku/drm-intel/log/?h=wait_until

Spiced with your patch and few build_bug_on, I think the
wait_for_atomic(_us) might become rare thing.

Thanks,
-Mika

> Regards,
>
> Tvrtko
> _______________________________________________
> 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