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

[Chris Wilson]

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. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre