[Intel-gfx] [PATCH 18/19] drm/i915: save some time when waiting the eDP timings

[Paulo Zanoni]

2013/11/26 Chris Wilson <chris at chris-wilson.co.uk>:
> On Mon, Nov 25, 2013 at 06:38:53PM -0800, Ben Widawsky wrote:
>> On Mon, Nov 25, 2013 at 11:25:10PM +0000, Chris Wilson wrote:
>> > On Mon, Nov 25, 2013 at 02:17:55PM -0800, Ben Widawsky wrote:
>> > > On Thu, Nov 21, 2013 at 04:00:17PM +0000, Chris Wilson wrote:
>> > > > On Thu, Nov 21, 2013 at 01:47:32PM -0200, Paulo Zanoni wrote:
>> > > > > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > > > >
>> > > > > The eDP spec defines some points where after you do action A, you have
>> > > > > to wait some time before action B. The thing is that in our driver
>> > > > > action B does not happen exactly after action A, but we still use
>> > > > > msleep() calls directly. What this patch happens is that we record the
>> > > > > timestamp of when action A happened, then, just before action B, we
>> > > > > look at how much time has passed and only sleep the remaining amount
>> > > > > needed.
>> > > > >
>> > > > > With this change, I am able to save about 5-20ms (out of the total
>> > > > > 200ms) of the backlight_off delay and completely skip the 1ms
>> > > > > backlight_on delay. The 600ms vdd_off delay doesn't happen during
>> > > > > normal usage anymore due to a previous patch.
>> > > > >
>> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > > > > ---
>> > > > >  drivers/gpu/drm/i915/intel_dp.c  | 38 +++++++++++++++++++++++++++++++++++---
>> > > > >  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>> > > > >  2 files changed, 38 insertions(+), 3 deletions(-)
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > index b438e76..3a1ca80 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > @@ -1051,12 +1051,41 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
>> > > > >       ironlake_wait_panel_status(intel_dp, IDLE_OFF_MASK, IDLE_OFF_VALUE);
>> > > > >  }
>> > > > >
>> > > > > +static void ironlake_wait_jiffies_delay(unsigned long timestamp,
>> > > > > +                                     int to_wait_ms)
>> > > > This is not hw specific, so just
>> > > > intel_wait_until_after(timestamp_jiffies, to_wait_ms)
>> > >
>> > > Can't we do this with our existing wait_for, and get all the other junk
>> > > we've crammed in there?
>> > > wait_for(false, timestamp + to_wait_ms)
>> > >
>> > > Or do I have this all wrong?
>> >
>> > It would be
>> > wait_for(false, jiffies_to_ms(max(ms_to_jiffies(timestamp+to_wait_ms) - jiffies, 0))
>> > or something pretty similar. Definitely macro abuse as you would be
>> > hoping that the compiler turned
>> >   while (!time_after(jiffies, timeout)) msleep(1);
>> > into
>> >   msleep(to_ms(timeout-jiffies));
>> >
>> > So perhaps clearer would be:
>> > intel_wait_until_after(unsigned long timestamp_jiffies,
>> >                        int to_wait_ms)
>> >  {
>> >    timestamp_jiffies += msec_to_jiffes(to_wait_ms) + 1;
>> >    if (time_after(timestamp_jiffies, jiffies) {
>> >       timestamp_jiffies -= jiffies;
>> >       while (timestamp_jiffies)
>> >           timestamp_jiffies = schedule_timeout(timestamp_jiffies);
>> >    }
>> >  }
>>
>> Oops, I meant  "timestamp + to_wait_ms - jiffies_to_msecs(jiffies)"
>> I agree it does get a bit messy, but I think there is likely a cleaner
>> way that what you propose if we store things as jiffies.
>>
>> I hate dealing with jiffies, and I feel like even your function has a
>> race with jiffies though.
>
> Sure, I was contemplating using an extra variable to only access jiffies
> once, but I was lazy. Not mixing units here would make it less likely to
> misuse. However, I think the point stands that we can write a simple
> function that is clearer than abusing wait_for(). We will wait and see
> what Paulo creates. :)

I really don't think abusing wait_for is a good idea. I think wait_for
has a different purpose, and trying to invent clever ways to use it on
a non-standard use case will just make the code hard to read, without
any benefits. And with the risk that future changes to wait_for will
break our code.

Also, I think Chris' solution is very similar to my solution, but with
an open-coded msleep() instead of just the simple msleep() call.

So I'd really vote to apply the KISS design pattern and stick to the
original implementation + Chris' first suggestions (rename the
function, fix the msleep call), unless we can find another bug on it.
But since "applying whatever bikesheds we get" is always the fastest
(only?) way to gets patches merged, if you insist I will just do
whatever is requested.

Thanks,
Paulo

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni