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

Commit Message

Arun R Murthy March 25, 2014, 8:58 a.m. UTC

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 <[e-mail]>
---
 drivers/gpu/drm/i915/intel_drv.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Chris Wilson March 25, 2014, 9:07 a.m. UTC | #1

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 <[e-mail]>

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

Reviewed-by: Chris Wilson <[e-mail]>
-Chris

Jani Nikula March 25, 2014, 9:32 a.m. UTC | #2

On Tue, 25 Mar 2014, Chris Wilson <[e-mail]> 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 <[e-mail]>
>
> 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 <[e-mail]>
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Murthy, Arun R March 25, 2014, 9:46 a.m. UTC | #3

On Tuesday 25 March 2014 03:02 PM, Jani Nikula wrote:
> On Tue, 25 Mar 2014, Chris Wilson <[e-mail]> 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 <[e-mail]>
>> 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
-------------------

Daniel Vetter March 25, 2014, 10 a.m. UTC | #4

On Tue, Mar 25, 2014 at 11:32:23AM +0200, Jani Nikula wrote:
> On Tue, 25 Mar 2014, Chris Wilson <[e-mail]> 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 <[e-mail]>
> >
> > 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

Murthy, Arun R March 27, 2014, 4:48 a.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]>
>>> 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
-------------------

Murthy, Arun R April 1, 2014, 4:44 a.m. UTC | #6

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 <[e-mail]> 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 <[e-mail]>
>>>> 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
-------------------

Daniel Vetter April 1, 2014, 7:29 a.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]>
> >>>>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

Patch

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();					\
 		}							\