[PATCH] drm/i915: use hrtimer in wait for vblank

[PATCH] drm/i915: use hrtimer in wait for vblank

[Arun R Murthy]

In wait for vblank use usleep_range, which will use hrtimers instead of
msleep. Using msleep(1~20) there are more chances of sleeping for 20ms.
Using usleep_range uses hrtimers and hence are precise, worst case will
trigger an interrupt at the higher/max timeout.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 44067bc..079280a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -52,7 +52,7 @@
 			break;						\
 		}							\
 		if (W && drm_can_sleep())  {				\
-			msleep(W);					\
+			usleep_range(W * 1000, W * 2 * 1000);		\
 		} else {						\
 			cpu_relax();					\
 		}							\
-- 
1.7.9.5

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Chris Wilson]

On Mon, Mar 24, 2014 at 01:43:38PM +0530, Arun R Murthy wrote:
> In wait for vblank use usleep_range, which will use hrtimers instead of
> msleep. Using msleep(1~20) there are more chances of sleeping for 20ms.
> Using usleep_range uses hrtimers and hence are precise, worst case will
> trigger an interrupt at the higher/max timeout.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 44067bc..079280a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -52,7 +52,7 @@
>  			break;						\
>  		}							\
>  		if (W && drm_can_sleep())  {				\
> -			msleep(W);					\
> +			usleep_range(W * 1000, W * 2 * 1000);		\
>  		} else {						\
>  			cpu_relax();					\
>  		}							\

Ok. But W is still just a random value we picked for being the mininum
legal value for msleep(). So just usleep_range(500, 2000) or somesuch
will be fine. We can rename W to CAN_SLEEP it that helps.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Murthy, Arun R]

> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 44067bc..079280a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -52,7 +52,7 @@
>  			break;						\
>  		}							\
>  		if (W && drm_can_sleep())  {				\
> -			msleep(W);					\
> +			usleep_range(W * 1000, W * 2 * 1000);		\
>  		} else {						\
>  			cpu_relax();					\
>  		}							\

Ok. But W is still just a random value we picked for being the mininum legal value for msleep(). So just usleep_range(500, 2000) or somesuch will be fine. We can rename W to CAN_SLEEP it that helps.

Ok, will take care of this in my next version of the patch.

Thanks and Regards,
Arun R Murthy
------------------

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Jani Nikula]

On Mon, 24 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Mar 24, 2014 at 01:43:38PM +0530, Arun R Murthy wrote:
>> In wait for vblank use usleep_range, which will use hrtimers instead of
>> msleep. Using msleep(1~20) there are more chances of sleeping for 20ms.
>> Using usleep_range uses hrtimers and hence are precise, worst case will
>> trigger an interrupt at the higher/max timeout.
>> 
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 44067bc..079280a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -52,7 +52,7 @@
>>  			break;						\
>>  		}							\
>>  		if (W && drm_can_sleep())  {				\
>> -			msleep(W);					\
>> +			usleep_range(W * 1000, W * 2 * 1000);		\
>>  		} else {						\
>>  			cpu_relax();					\
>>  		}							\
>
> Ok. But W is still just a random value we picked for being the mininum
> legal value for msleep(). So just usleep_range(500, 2000) or somesuch
> will be fine. We can rename W to CAN_SLEEP it that helps.

We do use _wait_for directly from intel_dp.c with W == 10 to not retry
so many times on what's expected to be a long wait.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Murthy, Arun R]

>> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 44067bc..079280a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -52,7 +52,7 @@
>>  			break;						\
>>  		}							\
>>  		if (W && drm_can_sleep())  {				\
>> -			msleep(W);					\
>> +			usleep_range(W * 1000, W * 2 * 1000);		\
>>  		} else {						\
>>  			cpu_relax();					\
>>  		}							\
>
> Ok. But W is still just a random value we picked for being the mininum 
> legal value for msleep(). So just usleep_range(500, 2000) or somesuch 
> will be fine. We can rename W to CAN_SLEEP it that helps.

We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait.

Its not expected to be too long, we tend to get vblank every 16ms.  With using usleep_range
with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.
Having 10ms sleep timer, we might tend to be blocking for 6ms in some cases.(from the above data)
Moreover 10ms usleep doesn't guarantee that we get a chance to execute after 10ms, it might
get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.

Thanks and Regards,
Arun R Murthy
------------------ 

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Chris Wilson]

On Mon, Mar 24, 2014 at 09:34:35AM +0000, Murthy, Arun R wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 44067bc..079280a 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -52,7 +52,7 @@
> >>  			break;						\
> >>  		}							\
> >>  		if (W && drm_can_sleep())  {				\
> >> -			msleep(W);					\
> >> +			usleep_range(W * 1000, W * 2 * 1000);		\
> >>  		} else {						\
> >>  			cpu_relax();					\
> >>  		}							\
> >
> > Ok. But W is still just a random value we picked for being the mininum 
> > legal value for msleep(). So just usleep_range(500, 2000) or somesuch 
> > will be fine. We can rename W to CAN_SLEEP it that helps.
> 
> We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait.

Hmm, missed that. We can fallback to the fallback plan of switching
based on W between msleep and usleep_range. Using -1, would be best.

 if (W && drm_can_sleep()) {
  if (__builtin_constant_p(W) && W < 0)
     usleep_range(500, 2000);
  else
     msleep(W);
 }
 
> Its not expected to be too long, we tend to get vblank every 16ms.  With using usleep_range
> with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.

vs msleep()? That would be nice information to capture in the changelog.
We expect the average wait here to be 8ms, ofc.

> Having 10ms sleep timer, we might tend to be blocking for 6ms in some cases.(from the above data)
> Moreover 10ms usleep doesn't guarantee that we get a chance to execute after 10ms, it might
> get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.

Ideal for what? We only use the sleeping path where we do not care too
great about the latency, otherwise we use the busy-wait path. Improving
the latency of the sleep without adversely affecting overall system
performance/power is ideal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Daniel Vetter]

On Mon, Mar 24, 2014 at 09:48:09AM +0000, Chris Wilson wrote:
> On Mon, Mar 24, 2014 at 09:34:35AM +0000, Murthy, Arun R wrote:
> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > >> b/drivers/gpu/drm/i915/intel_drv.h
> > >> index 44067bc..079280a 100644
> > >> --- a/drivers/gpu/drm/i915/intel_drv.h
> > >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> > >> @@ -52,7 +52,7 @@
> > >>  			break;						\
> > >>  		}							\
> > >>  		if (W && drm_can_sleep())  {				\
> > >> -			msleep(W);					\
> > >> +			usleep_range(W * 1000, W * 2 * 1000);		\
> > >>  		} else {						\
> > >>  			cpu_relax();					\
> > >>  		}							\
> > >
> > > Ok. But W is still just a random value we picked for being the mininum 
> > > legal value for msleep(). So just usleep_range(500, 2000) or somesuch 
> > > will be fine. We can rename W to CAN_SLEEP it that helps.
> > 
> > We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait.
> 
> Hmm, missed that. We can fallback to the fallback plan of switching
> based on W between msleep and usleep_range. Using -1, would be best.
> 
>  if (W && drm_can_sleep()) {
>   if (__builtin_constant_p(W) && W < 0)
>      usleep_range(500, 2000);
>   else
>      msleep(W);
>  }
>  
> > Its not expected to be too long, we tend to get vblank every 16ms.  With using usleep_range
> > with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.
> 
> vs msleep()? That would be nice information to capture in the changelog.
> We expect the average wait here to be 8ms, ofc.
> 
> > Having 10ms sleep timer, we might tend to be blocking for 6ms in some cases.(from the above data)
> > Moreover 10ms usleep doesn't guarantee that we get a chance to execute after 10ms, it might
> > get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.
> 
> Ideal for what? We only use the sleeping path where we do not care too
> great about the latency, otherwise we use the busy-wait path. Improving
> the latency of the sleep without adversely affecting overall system
> performance/power is ideal.

We've used it in a few places where we'd care a bit more about latency
(like edid reads) but on most platforms those switched to interrupts now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Murthy, Arun R]

> > > Ok. But W is still just a random value we picked for being the 
> > > mininum legal value for msleep(). So just usleep_range(500, 2000) 
> > > or somesuch will be fine. We can rename W to CAN_SLEEP it that helps.
> > 
> > We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait.
> 
> Hmm, missed that. We can fallback to the fallback plan of switching 
> based on W between msleep and usleep_range. Using -1, would be best.
> 
>  if (W && drm_can_sleep()) {
>   if (__builtin_constant_p(W) && W < 0)
>      usleep_range(500, 2000);
>   else
>      msleep(W);
>  }
>  

Seems to be a good approach, will take care of this in the next version of the patch.

> > Its not expected to be too long, we tend to get vblank every 16ms.  
> > With using usleep_range with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.
> 
> vs msleep()? That would be nice information to capture in the changelog.
> We expect the average wait here to be 8ms, ofc.
> 

Sure, will add this in the changelog.

> > Having 10ms sleep timer, we might tend to be blocking for 6ms in 
> > some cases.(from the above data) Moreover 10ms usleep doesn't 
> > guarantee that we get a chance to execute after 10ms, it might get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.
> 
> Ideal for what? We only use the sleeping path where we do not care too 
> great about the latency, otherwise we use the busy-wait path. 
> Improving the latency of the sleep without adversely affecting overall 
> system performance/power is ideal.

We've used it in a few places where we'd care a bit more about latency (like edid reads) but on most platforms those switched to interrupts now.
-Daniel

Thanks and Regards,
Arun R Murthy
------------------

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Daniel Vetter]

On Mon, Mar 24, 2014 at 09:34:35AM +0000, Murthy, Arun R wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 44067bc..079280a 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -52,7 +52,7 @@
> >>  			break;						\
> >>  		}							\
> >>  		if (W && drm_can_sleep())  {				\
> >> -			msleep(W);					\
> >> +			usleep_range(W * 1000, W * 2 * 1000);		\
> >>  		} else {						\
> >>  			cpu_relax();					\
> >>  		}							\
> >
> > Ok. But W is still just a random value we picked for being the mininum 
> > legal value for msleep(). So just usleep_range(500, 2000) or somesuch 
> > will be fine. We can rename W to CAN_SLEEP it that helps.
> 
> We do use _wait_for directly from intel_dp.c with W == 10 to not retry so many times on what's expected to be a long wait.
> 
> Its not expected to be too long, we tend to get vblank every 16ms.  With using usleep_range
> with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.
> Having 10ms sleep timer, we might tend to be blocking for 6ms in some cases.(from the above data)
> Moreover 10ms usleep doesn't guarantee that we get a chance to execute after 10ms, it might
> get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.

Can you please do proper quoting when replying? It makes it fairly hard to
figure out what's your response when you reply in-line, especially in more
complicated threads. And top-posting isn't an option either, especially
for more in-depth discussions where the different issues need to be
treated as separate points.

You need a real mail client for this, Outlook won't cut it. Jesse is
floating slides internal with all the details and tons of links to
resources.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: use hrtimer in wait for vblank

[Murthy, Arun R]

> Its not expected to be too long, we tend to get vblank every 16ms.  
> With using usleep_range with min and max as 1-2ms, we have observed that this function consuming 4ms to 17ms.
> Having 10ms sleep timer, we might tend to be blocking for 6ms in some 
> cases.(from the above data) Moreover 10ms usleep doesn't guarantee 
> that we get a chance to execute after 10ms, it might get increased due to scheduling. So ideal solution would be to use useep_range which uses hrtimers.

Can you please do proper quoting when replying? It makes it fairly hard to figure out what's your response when you reply in-line, especially in more complicated threads. And top-posting isn't an option either, especially for more in-depth discussions where the different issues need to be treated as separate points.

You need a real mail client for this, Outlook won't cut it. Jesse is floating slides internal with all the details and tons of links to resources.

Sure will take care of this in future.

Thanks and Regards,
Arun R Murthy
------------------