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

[PATCH v4] 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.

As per kernel document "Documentation/timers/timers-howto.txt" sleeping
for 10us to 20ms its recomended to use usleep_range.

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

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

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

[Chris Wilson]

On Tue, Mar 25, 2014 at 02:28:22PM +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.
> 
> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
> for 10us to 20ms its recomended to use usleep_range.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
tweaking in future.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jani Nikula]

On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Mar 25, 2014 at 02:28:22PM +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.
>> 
>> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
>> for 10us to 20ms its recomended to use usleep_range.
>> 
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>
> Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
> tweaking in future.

With the current code, this is essentially the same as the original
patch. We never have W > 20, and thus we always take the usleep_range()
path. So W is definitely worth tweaking if we go with this now.

Nitpick, the macro params should be parenthesized. This will now break
for _wait_for(cond, 10, 2 + 1) and such.

Arun, please don't immediately reply with updated patches if there's
discussion still going on. See what the conclusion is first. Thanks.

BR,
Jani.




>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Murthy, Arun R]

On Tuesday 25 March 2014 03:02 PM, Jani Nikula wrote:
> On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Tue, Mar 25, 2014 at 02:28:22PM +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.
>>>
>>> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
>>> for 10us to 20ms its recomended to use usleep_range.
>>>
>>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
>> tweaking in future.
> With the current code, this is essentially the same as the original
> patch. We never have W > 20, and thus we always take the usleep_range()
> path. So W is definitely worth tweaking if we go with this now.
>
> Nitpick, the macro params should be parenthesized. This will now break
> for _wait_for(cond, 10, 2 + 1) and such.

wait_for(COND, TIMEOUT, ATOMIC, MS)
and remove all wait_for_X

function will look like
_wait_for(COND< TIMEOUT, ATOMIC, MS)
{
     /* loop */
         /* check condition */
         if (atomic)
             cpu_relax()
         else
             if (ms > 20)
                 msleep
             else
                 usleep_range
}

caller for wait_for will be setting all the parameters and hence no tweaks.

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

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

[Murthy, Arun R]

On Tuesday 25 March 2014 03:16 PM, Murthy, Arun R wrote:
> On Tuesday 25 March 2014 03:02 PM, Jani Nikula wrote:
>> On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>> On Tue, Mar 25, 2014 at 02:28:22PM +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.
>>>>
>>>> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
>>>> for 10us to 20ms its recomended to use usleep_range.
>>>>
>>>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>>> Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
>>> tweaking in future.
>> With the current code, this is essentially the same as the original
>> patch. We never have W > 20, and thus we always take the usleep_range()
>> path. So W is definitely worth tweaking if we go with this now.
>>
>> Nitpick, the macro params should be parenthesized. This will now break
>> for _wait_for(cond, 10, 2 + 1) and such.
> wait_for(COND, TIMEOUT, ATOMIC, MS)
> and remove all wait_for_X
>
> function will look like
> _wait_for(COND< TIMEOUT, ATOMIC, MS)
> {
>       /* loop */
>           /* check condition */
>           if (atomic)
>               cpu_relax()
>           else
>               if (ms > 20)
>                   msleep
>               else
>                   usleep_range
> }
>
> caller for wait_for will be setting all the parameters and hence no tweaks.

Any comments on this?

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

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

[Murthy, Arun R]

On Thursday 27 March 2014 10:18 AM, Murthy, Arun R wrote:
> On Tuesday 25 March 2014 03:16 PM, Murthy, Arun R wrote:
>> On Tuesday 25 March 2014 03:02 PM, Jani Nikula wrote:
>>> On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>> On Tue, Mar 25, 2014 at 02:28:22PM +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.
>>>>>
>>>>> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
>>>>> for 10us to 20ms its recomended to use usleep_range.
>>>>>
>>>>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>>>> Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
>>>> tweaking in future.
>>> With the current code, this is essentially the same as the original
>>> patch. We never have W > 20, and thus we always take the usleep_range()
>>> path. So W is definitely worth tweaking if we go with this now.
>>>
>>> Nitpick, the macro params should be parenthesized. This will now break
>>> for _wait_for(cond, 10, 2 + 1) and such.
>> wait_for(COND, TIMEOUT, ATOMIC, MS)
>> and remove all wait_for_X
>>
>> function will look like
>> _wait_for(COND< TIMEOUT, ATOMIC, MS)
>> {
>>        /* loop */
>>            /* check condition */
>>            if (atomic)
>>                cpu_relax()
>>            else
>>                if (ms > 20)
>>                    msleep
>>                else
>>                    usleep_range
>> }
>>
>> caller for wait_for will be setting all the parameters and hence no tweaks.
> Any comments on this?
Gentle reminder!

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

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

[Daniel Vetter]

On Tue, Apr 01, 2014 at 10:14:12AM +0530, Murthy, Arun R wrote:
> On Thursday 27 March 2014 10:18 AM, Murthy, Arun R wrote:
> >On Tuesday 25 March 2014 03:16 PM, Murthy, Arun R wrote:
> >>On Tuesday 25 March 2014 03:02 PM, Jani Nikula wrote:
> >>>On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >>>>On Tue, Mar 25, 2014 at 02:28:22PM +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.
> >>>>>
> >>>>>As per kernel document "Documentation/timers/timers-howto.txt" sleeping
> >>>>>for 10us to 20ms its recomended to use usleep_range.
> >>>>>
> >>>>>Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> >>>>Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
> >>>>tweaking in future.
> >>>With the current code, this is essentially the same as the original
> >>>patch. We never have W > 20, and thus we always take the usleep_range()
> >>>path. So W is definitely worth tweaking if we go with this now.
> >>>
> >>>Nitpick, the macro params should be parenthesized. This will now break
> >>>for _wait_for(cond, 10, 2 + 1) and such.
> >>wait_for(COND, TIMEOUT, ATOMIC, MS)
> >>and remove all wait_for_X
> >>
> >>function will look like
> >>_wait_for(COND< TIMEOUT, ATOMIC, MS)
> >>{
> >>       /* loop */
> >>           /* check condition */
> >>           if (atomic)
> >>               cpu_relax()
> >>           else
> >>               if (ms > 20)
> >>                   msleep
> >>               else
> >>                   usleep_range
> >>}
> >>
> >>caller for wait_for will be setting all the parameters and hence no tweaks.
> >Any comments on this?
> Gentle reminder!

See my other mail somewhere in one your patch resends:

http://www.spinics.net/lists/intel-gfx/msg42439.html

If this is really just to optimize vblank waits we can do much better.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Tue, Mar 25, 2014 at 11:32:23AM +0200, Jani Nikula wrote:
> On Tue, 25 Mar 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Mar 25, 2014 at 02:28:22PM +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.
> >> 
> >> As per kernel document "Documentation/timers/timers-howto.txt" sleeping
> >> for 10us to 20ms its recomended to use usleep_range.
> >> 
> >> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> >
> > Lgtm, I still feel that our use of W=1 is fairly arbitrary and worth
> > tweaking in future.
> 
> With the current code, this is essentially the same as the original
> patch. We never have W > 20, and thus we always take the usleep_range()
> path. So W is definitely worth tweaking if we go with this now.
> 
> Nitpick, the macro params should be parenthesized. This will now break
> for _wait_for(cond, 10, 2 + 1) and such.
> 
> Arun, please don't immediately reply with updated patches if there's
> discussion still going on. See what the conclusion is first. Thanks.

Also when quickly replying a single patch please use in-reply-to to the
correct thread. This way the discussion is still kept tighlty grouped
together even when you resend quickly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch