* [PATCH] radeon/PM Really wait for vblank before reclocking
@ 2010-02-15 15:09 Jaime Velasco Juan
2010-02-15 17:52 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Jaime Velasco Juan @ 2010-02-15 15:09 UTC (permalink / raw)
To: airlied; +Cc: Jaime Velasco Juan, dri-devel
The old code used a false condition so it always waited until
timeout
Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
---
drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index a8e151e..842952f 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
rdev->pm.req_vblank |= (1 << 1);
drm_vblank_get(rdev->ddev, 1);
}
- if (rdev->pm.active_crtcs)
- wait_event_interruptible_timeout(
- rdev->irq.vblank_queue, 0,
- msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
+ if (rdev->pm.active_crtcs) {
+ long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
+ __wait_event_interruptible_timeout(
+ rdev->irq.vblank_queue, 1,
+ timeout);
+ }
if (rdev->pm.req_vblank & (1 << 0)) {
rdev->pm.req_vblank &= ~(1 << 0);
drm_vblank_put(rdev->ddev, 0);
--
1.6.6.1
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] radeon/PM Really wait for vblank before reclocking
2010-02-15 15:09 [PATCH] radeon/PM Really wait for vblank before reclocking Jaime Velasco Juan
@ 2010-02-15 17:52 ` Rafał Miłecki
2010-02-16 13:41 ` Jaime Velasco Juan
2010-02-17 18:22 ` Rafał Miłecki
0 siblings, 2 replies; 6+ messages in thread
From: Rafał Miłecki @ 2010-02-15 17:52 UTC (permalink / raw)
To: Jaime Velasco Juan; +Cc: airlied, dri-devel
2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
> The old code used a false condition so it always waited until
> timeout
>
> Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
> ---
> drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index a8e151e..842952f 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> rdev->pm.req_vblank |= (1 << 1);
> drm_vblank_get(rdev->ddev, 1);
> }
> - if (rdev->pm.active_crtcs)
> - wait_event_interruptible_timeout(
> - rdev->irq.vblank_queue, 0,
> - msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
> + if (rdev->pm.active_crtcs) {
> + long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
> + __wait_event_interruptible_timeout(
> + rdev->irq.vblank_queue, 1,
> + timeout);
> + }
Yeah, it seems logic was wrong, thanks.
Two questions: is there some real reason for using "timeout" variable?
We don't re-use this anywhere. It seems useless, doesn't make code
shorter, forces braces usage and you defined this inside braces, not
on beginning of function, as kernel coding style says. Can you just
drop this redundant variable?
Second: could you say what is "better" in using
__wait_event_interruptible_timeout? Is this faster (doesn't check
condition) or something?
--
Rafał
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] radeon/PM Really wait for vblank before reclocking
2010-02-15 17:52 ` Rafał Miłecki
@ 2010-02-16 13:41 ` Jaime Velasco Juan
2010-02-16 15:54 ` Alex Deucher
2010-02-17 18:22 ` Rafał Miłecki
1 sibling, 1 reply; 6+ messages in thread
From: Jaime Velasco Juan @ 2010-02-16 13:41 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: airlied, dri-devel
El lun. 15 de feb. de 2010, a las 18:52:46 +0100, Rafał Miłecki escribió:
> 2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
> > The old code used a false condition so it always waited until
> > timeout
> >
> > Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
> > ---
> > drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
> > 1 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> > index a8e151e..842952f 100644
> > --- a/drivers/gpu/drm/radeon/radeon_pm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> > @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> > rdev->pm.req_vblank |= (1 << 1);
> > drm_vblank_get(rdev->ddev, 1);
> > }
> > - if (rdev->pm.active_crtcs)
> > - wait_event_interruptible_timeout(
> > - rdev->irq.vblank_queue, 0,
> > - msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
> > + if (rdev->pm.active_crtcs) {
> > + long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
> > + __wait_event_interruptible_timeout(
> > + rdev->irq.vblank_queue, 1,
> > + timeout);
> > + }
>
> Yeah, it seems logic was wrong, thanks.
>
> Two questions: is there some real reason for using "timeout" variable?
> We don't re-use this anywhere. It seems useless, doesn't make code
> shorter, forces braces usage and you defined this inside braces, not
> on beginning of function, as kernel coding style says. Can you just
> drop this redundant variable?
>
> Second: could you say what is "better" in using
> __wait_event_interruptible_timeout? Is this faster (doesn't check
> condition) or something?
>
I just took the code from the definition of wait_event_interruptible_timeout;
that macro checks the condition before calling
__wait_event_interruptible_timeout, where the task really goes to sleep,
but __wait_event_interruptible_timeout changes the timeout parameter, so
it needs to be a variable. I just tried to do the smallest change to the
code. It's true that almost nobody else use this macro directly.
Even with this, I still see some ocasional glitches when reclocking the
engine, but this does not stall rendering for 200ms as before (it was
very noticeable). This has nothing to do with this patch, but is it really
needed to take cp.mutex in radeon_pm_set_clocks? I've been doing some
testing without doing it and it doesn't seem to break anything.
Regards
> --
> Rafał
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] radeon/PM Really wait for vblank before reclocking
2010-02-16 13:41 ` Jaime Velasco Juan
@ 2010-02-16 15:54 ` Alex Deucher
0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2010-02-16 15:54 UTC (permalink / raw)
To: Jaime Velasco Juan; +Cc: airlied, dri-devel
2010/2/16 Jaime Velasco Juan <jsagarribay@gmail.com>:
> El lun. 15 de feb. de 2010, a las 18:52:46 +0100, Rafał Miłecki escribió:
>> 2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
>> > The old code used a false condition so it always waited until
>> > timeout
>> >
>> > Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
>> > ---
>> > drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
>> > 1 files changed, 6 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
>> > index a8e151e..842952f 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_pm.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>> > @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>> > rdev->pm.req_vblank |= (1 << 1);
>> > drm_vblank_get(rdev->ddev, 1);
>> > }
>> > - if (rdev->pm.active_crtcs)
>> > - wait_event_interruptible_timeout(
>> > - rdev->irq.vblank_queue, 0,
>> > - msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
>> > + if (rdev->pm.active_crtcs) {
>> > + long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
>> > + __wait_event_interruptible_timeout(
>> > + rdev->irq.vblank_queue, 1,
>> > + timeout);
>> > + }
>>
>> Yeah, it seems logic was wrong, thanks.
>>
>> Two questions: is there some real reason for using "timeout" variable?
>> We don't re-use this anywhere. It seems useless, doesn't make code
>> shorter, forces braces usage and you defined this inside braces, not
>> on beginning of function, as kernel coding style says. Can you just
>> drop this redundant variable?
>>
>> Second: could you say what is "better" in using
>> __wait_event_interruptible_timeout? Is this faster (doesn't check
>> condition) or something?
>>
> I just took the code from the definition of wait_event_interruptible_timeout;
> that macro checks the condition before calling
> __wait_event_interruptible_timeout, where the task really goes to sleep,
> but __wait_event_interruptible_timeout changes the timeout parameter, so
> it needs to be a variable. I just tried to do the smallest change to the
> code. It's true that almost nobody else use this macro directly.
>
> Even with this, I still see some ocasional glitches when reclocking the
> engine, but this does not stall rendering for 200ms as before (it was
> very noticeable). This has nothing to do with this patch, but is it really
> needed to take cp.mutex in radeon_pm_set_clocks? I've been doing some
> testing without doing it and it doesn't seem to break anything.
>
You generally don't want to change the clock while the engine using it
is active.
Alex
> Regards
>
>> --
>> Rafał
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon/PM Really wait for vblank before reclocking
2010-02-15 17:52 ` Rafał Miłecki
2010-02-16 13:41 ` Jaime Velasco Juan
@ 2010-02-17 18:22 ` Rafał Miłecki
2010-02-17 18:33 ` Rafał Miłecki
1 sibling, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2010-02-17 18:22 UTC (permalink / raw)
To: Jaime Velasco Juan; +Cc: airlied, dri-devel
W dniu 15 lutego 2010 18:52 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał:
> 2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
>> The old code used a false condition so it always waited until
>> timeout
>>
>> Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
>> ---
>> drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
>> 1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
>> index a8e151e..842952f 100644
>> --- a/drivers/gpu/drm/radeon/radeon_pm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>> @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>> rdev->pm.req_vblank |= (1 << 1);
>> drm_vblank_get(rdev->ddev, 1);
>> }
>> - if (rdev->pm.active_crtcs)
>> - wait_event_interruptible_timeout(
>> - rdev->irq.vblank_queue, 0,
>> - msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
>> + if (rdev->pm.active_crtcs) {
>> + long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
>> + __wait_event_interruptible_timeout(
>> + rdev->irq.vblank_queue, 1,
>> + timeout);
>> + }
>
> Yeah, it seems logic was wrong, thanks.
To make it clear (I didn't understand that correctly earlier):
include/linux/wait.h:
wait_event_interruptible_timeout - sleep until a condition gets true
or a timeout elapses
So using your __ prefix and "1" as condition argument is some hack
that could work indeed...
But maybe there is some "real" kernel function that simply waits for
wake_up with timeout?
--
Rafał
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] radeon/PM Really wait for vblank before reclocking
2010-02-17 18:22 ` Rafał Miłecki
@ 2010-02-17 18:33 ` Rafał Miłecki
0 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2010-02-17 18:33 UTC (permalink / raw)
To: Jaime Velasco Juan; +Cc: airlied, dri-devel
W dniu 17 lutego 2010 19:22 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał:
> W dniu 15 lutego 2010 18:52 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał:
>> 2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
>>> The old code used a false condition so it always waited until
>>> timeout
>>>
>>> Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
>>> ---
>>> drivers/gpu/drm/radeon/radeon_pm.c | 10 ++++++----
>>> 1 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
>>> index a8e151e..842952f 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_pm.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>>> @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
>>> rdev->pm.req_vblank |= (1 << 1);
>>> drm_vblank_get(rdev->ddev, 1);
>>> }
>>> - if (rdev->pm.active_crtcs)
>>> - wait_event_interruptible_timeout(
>>> - rdev->irq.vblank_queue, 0,
>>> - msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
>>> + if (rdev->pm.active_crtcs) {
>>> + long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
>>> + __wait_event_interruptible_timeout(
>>> + rdev->irq.vblank_queue, 1,
>>> + timeout);
>>> + }
>>
>> Yeah, it seems logic was wrong, thanks.
>
> To make it clear (I didn't understand that correctly earlier):
>
> include/linux/wait.h:
> wait_event_interruptible_timeout - sleep until a condition gets true
> or a timeout elapses
>
> So using your __ prefix and "1" as condition argument is some hack
> that could work indeed...
>
> But maybe there is some "real" kernel function that simply waits for
> wake_up with timeout?
Your solution doesn't work because __wait_event_interruptible checks
once more for condition before scheduling. If finds "1" and so it
breaks loop immediately.
--
Rafał
------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-17 18:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-15 15:09 [PATCH] radeon/PM Really wait for vblank before reclocking Jaime Velasco Juan
2010-02-15 17:52 ` Rafał Miłecki
2010-02-16 13:41 ` Jaime Velasco Juan
2010-02-16 15:54 ` Alex Deucher
2010-02-17 18:22 ` Rafał Miłecki
2010-02-17 18:33 ` Rafał Miłecki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.