Signed-off-by: Rafał Miłecki <[email protected]>
---
We try to implement some PM in radeon KMS and we need to sync with VLBANK for
reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
timer handler just before reclocking. Then our IRQ handler calls wake_up and we
continue reclocking.
As you see our sleeping is condition-less, we just wait for waking up queue.
We hope this waking will happen from IRQ handler, but for less-happy case we
also use some timeout (this will probably cause some single corruption, but
we can live with it).
Following macro is soemthing that seems to work fine for us, but instead
introducing this to radeon KMS only, I'd like to propose adding this to whole
wait.h. Do you this it's something we should place there? Can someone take this
patch for me? Or maybe you find this rather useless and we should keep this
marco locally?
---
include/linux/wait.h | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index a48e16b..998475b 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -332,6 +332,31 @@ do { \
__ret; \
})
+/**
+ * wait_interruptible_timeout - sleep until a waitqueue is woken up
+ * @wq: the waitqueue to wait on
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_INTERRUPTIBLE) until the waitqueue
+ * @wq is woken up. It can be done manually with wake_up or will happen
+ * if timeout elapses.
+ *
+ * The function returns 0 if the @timeout elapsed, remaining jiffies
+ * if workqueue was waken up earlier.
+ */
+#define wait_interruptible_timeout(wq, timeout) \
+({ \
+ long __ret = timeout; \
+ \
+ DEFINE_WAIT(__wait); \
+ prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
+ if (!signal_pending(current)) \
+ __ret = schedule_timeout(__ret); \
+ finish_wait(&wq, &__wait); \
+ \
+ __ret; \
+})
+
#define __wait_event_interruptible_exclusive(wq, condition, ret) \
do { \
DEFINE_WAIT(__wait); \
--
1.6.4.2
Rafał Miłecki wrote:
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> We try to implement some PM in radeon KMS and we need to sync with VLBANK for
> reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
> timer handler just before reclocking. Then our IRQ handler calls wake_up and we
> continue reclocking.
>
> As you see our sleeping is condition-less, we just wait for waking up queue.
>
> We hope this waking will happen from IRQ handler, but for less-happy case we
> also use some timeout (this will probably cause some single corruption, but
> we can live with it).
>
> Following macro is soemthing that seems to work fine for us, but instead
> introducing this to radeon KMS only, I'd like to propose adding this to whole
> wait.h. Do you this it's something we should place there? Can someone take this
> patch for me? Or maybe you find this rather useless and we should keep this
> marco locally?
> ---
> include/linux/wait.h | 25 +++++++++++++++++++++++++
> 1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index a48e16b..998475b 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -332,6 +332,31 @@ do { \
> __ret; \
> })
>
> +/**
> + * wait_interruptible_timeout - sleep until a waitqueue is woken up
> + * @wq: the waitqueue to wait on
> + * @timeout: timeout, in jiffies
> + *
> + * The process is put to sleep (TASK_INTERRUPTIBLE) until the waitqueue
> + * @wq is woken up. It can be done manually with wake_up or will happen
> + * if timeout elapses.
> + *
> + * The function returns 0 if the @timeout elapsed, remaining jiffies
> + * if workqueue was waken up earlier.
> + */
> +#define wait_interruptible_timeout(wq, timeout) \
> +({ \
> + long __ret = timeout; \
> + \
> + DEFINE_WAIT(__wait); \
> + prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
> + if (!signal_pending(current)) \
> + __ret = schedule_timeout(__ret); \
> + finish_wait(&wq, &__wait); \
> + \
> + __ret; \
> +})
> +
> #define __wait_event_interruptible_exclusive(wq, condition, ret) \
> do { \
> DEFINE_WAIT(__wait); \
>
What about msleep_interruptible in <linux/delay.h> ?
/Thomas
W dniu 21 lutego 2010 16:01 użytkownik Thomas Hellstrom
<[email protected]> napisał:
> Rafał Miłecki wrote:
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> We try to implement some PM in radeon KMS and we need to sync with VLBANK
>> for
>> reclocking engine/memory. The easiest and cleanest way seems to be
>> sleeping in
>> timer handler just before reclocking. Then our IRQ handler calls wake_up
>> and we
>> continue reclocking.
>>
>> As you see our sleeping is condition-less, we just wait for waking up
>> queue.
>>
>> We hope this waking will happen from IRQ handler, but for less-happy case
>> we
>> also use some timeout (this will probably cause some single corruption,
>> but
>> we can live with it).
>>
>> Following macro is soemthing that seems to work fine for us, but instead
>> introducing this to radeon KMS only, I'd like to propose adding this to
>> whole
>> wait.h. Do you this it's something we should place there? Can someone take
>> this
>> patch for me? Or maybe you find this rather useless and we should keep
>> this
>> marco locally?
>
> What about msleep_interruptible in <linux/delay.h> ?
I guess this will wake up on every signal pending to driver's process.
I need to wake up using my own (VBLANK related) workqueue.
Is that right? Or maybe there is some hack/sth that will let me
achieve what I need?
--
Rafał
Ping?
Can I interpret lack of objections as permission for committing that?
If so, by which tree should we get this patch mainline?
Dave: this patch is needed for radeon driver. Can we get this through
drm-2.6 maybe?
--
Rafał
Forwarding to ppl I could often notice in git log time.h
---------- Wiadomość przekazana dalej ----------From: Rafał Miłecki <[email protected]>Date: 21 lutego 2010 15:10Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro tosleep (w. timeout) until wake_upTo: Linux Kernel Mailing List <[email protected]>,[email protected]: Rafał Miłecki <[email protected]>
Signed-off-by: Rafał Miłecki <[email protected]>---We try to implement some PM in radeon KMS and we need to sync with VLBANK forreclocking engine/memory. The easiest and cleanest way seems to be sleeping intimer handler just before reclocking. Then our IRQ handler calls wake_up and wecontinue reclocking.
As you see our sleeping is condition-less, we just wait for waking up queue.
We hope this waking will happen from IRQ handler, but for less-happy case wealso use some timeout (this will probably cause some single corruption, butwe can live with it).
Following macro is soemthing that seems to work fine for us, but insteadintroducing this to radeon KMS only, I'd like to propose adding this to wholewait.h. Do you this it's something we should place there? Can someone take thispatch for me? Or maybe you find this rather useless and we should keep thismarco locally?--- include/linux/wait.h | 25 +++++++++++++++++++++++++ 1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.hindex a48e16b..998475b 100644--- a/include/linux/wait.h+++ b/include/linux/wait.h@@ -332,6 +332,31 @@ do { \ __ret; \ })
+/**+ * wait_interruptible_timeout - sleep until a waitqueue is woken up+ * @wq: the waitqueue to wait on+ * @timeout: timeout, in jiffies+ *+ * The process is put to sleep (TASK_INTERRUPTIBLE) until the waitqueue+ * @wq is woken up. It can be done manually with wake_up or will happen+ * if timeout elapses.+ *+ * The function returns 0 if the @timeout elapsed, remaining jiffies+ * if workqueue was waken up earlier.+ */+#define wait_interruptible_timeout(wq, timeout) \+({ \+ long __ret = timeout; \+ \+ DEFINE_WAIT(__wait); \+ prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \+ if (!signal_pending(current)) \+ __ret = schedule_timeout(__ret); \+ finish_wait(&wq, &__wait); \+ \+ __ret; \+})+ #define __wait_event_interruptible_exclusive(wq, condition, ret) \ do { \ DEFINE_WAIT(__wait); \--1.6.4.2????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Fri, 26 Feb 2010, Rafał Miłecki wrote:
> Forwarding to ppl I could often notice in git log time.h
And how is this related to time.h ?
>
> ---------- Wiadomość przekazana dalej ----------
> From: Rafał Miłecki <[email protected]>
> Date: 21 lutego 2010 15:10
> Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro to
> sleep (w. timeout) until wake_up
> To: Linux Kernel Mailing List <[email protected]>,
> [email protected]
> CC: Rafał Miłecki <[email protected]>
>
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> We try to implement some PM in radeon KMS and we need to sync with VLBANK for
> reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
> timer handler just before reclocking. Then our IRQ handler calls wake_up and we
Sleeping in the timer handler ? In which context runs this timer handler ?
> continue reclocking.
>
> As you see our sleeping is condition-less, we just wait for waking up queue.
Your sleep is not condition-less at all. See below.
> We hope this waking will happen from IRQ handler, but for less-happy case we
> also use some timeout (this will probably cause some single corruption, but
> we can live with it).
>
> Following macro is soemthing that seems to work fine for us, but instead
> introducing this to radeon KMS only, I'd like to propose adding this to whole
> wait.h. Do you this it's something we should place there? Can someone take this
> patch for me? Or maybe you find this rather useless and we should keep this
> marco locally?
You better delete it right away. It's broken and racy.
If the interrupt happens right before you enqueue to the wake queue,
then you miss it. The old interruptible_sleep_on_timeout() was
replaced by the event wait functions which have a condition exaclty to
avoid that race.
And you have a condition: Did an interrupt happen?
Thanks,
tglx
W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
<[email protected]> napisał:
> On Fri, 26 Feb 2010, Rafał Miłecki wrote:
>
>> Forwarding to ppl I could often notice in git log time.h
>
> And how is this related to time.h ?
Ouch, time.h vs. wait.h. I'm sorry.
>> ---------- Wiadomość przekazana dalej ----------
>> From: Rafał Miłecki <[email protected]>
>> Date: 21 lutego 2010 15:10
>> Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro to
>> sleep (w. timeout) until wake_up
>> To: Linux Kernel Mailing List <[email protected]>,
>> [email protected]
>> CC: Rafał Miłecki <[email protected]>
>>
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> We try to implement some PM in radeon KMS and we need to sync with VLBANK for
>> reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
>> timer handler just before reclocking. Then our IRQ handler calls wake_up and we
>
> Sleeping in the timer handler ? In which context runs this timer handler ?
We have our struct delayed_work which we first init and then we use
"queue_delayed_work" to start this "timer". So it's not real-real
timer as struct timer_list.
So this is actually delayed_work handler. Sorry (again) for my bad naming.
Anyway in this handler we just take decision about (down|up)clocking,
we wait for VBLANK (to avoid display corrupted picture) and right
after it happens, we reclock engine (plus memory in future).
>> continue reclocking.
>>
>> As you see our sleeping is condition-less, we just wait for waking up queue.
>
> Your sleep is not condition-less at all. See below.
>
>> We hope this waking will happen from IRQ handler, but for less-happy case we
>> also use some timeout (this will probably cause some single corruption, but
>> we can live with it).
>>
>> Following macro is soemthing that seems to work fine for us, but instead
>> introducing this to radeon KMS only, I'd like to propose adding this to whole
>> wait.h. Do you this it's something we should place there? Can someone take this
>> patch for me? Or maybe you find this rather useless and we should keep this
>> marco locally?
>
> You better delete it right away. It's broken and racy.
>
> If the interrupt happens right before you enqueue to the wake queue,
> then you miss it. The old interruptible_sleep_on_timeout() was
> replaced by the event wait functions which have a condition exaclty to
> avoid that race.
Well, I'm completely fine with that. After taking decision about
reclocking I request hardware to start reporting VBLANK interrupts.
Then (without any hurry) I go into sleep and next VBLANK interrupt
wake me up. Right after waking up I reclock engine/memory and then
(without hurry) I tell hardware to stop reporting VBLANK interrupts.
I guess it can be AMD-GPU specific interrupts mechanism there, but
that's how it works. I can point responsible code in driver if you
wish.
> And you have a condition: Did an interrupt happen?
Yeah, I guess that's kind of condition. I meant that I don't use any
driver's variable as condition to stop sleeping.
Sorry again for my mistakes mentioned above.
--
Rafał
On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <[email protected]> wrote:
> +#define wait_interruptible_timeout(wq, timeout)
> \
> +({ \
> + long ret = timeout; \
> + \
> + DEFINE_WAIT(wait); \
> + prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE); \
> + if (!signal_pending(current)) \
> + ret = schedule_timeout(ret); \
> + finish_wait(&wq, &wait); \
> + \
> + ret; \
> +})
It's often a mistake to use signals in-kernel. Signals are more a
userspace thing and it's better to use the lower-level kernel-specific
messaging tools in-kernel. Bear in mind that userspace can
independently and asynchronously send, accept and block signals.
Can KMS use wait_event_interruptible_timeout()?
W dniu 26 lutego 2010 17:14 użytkownik Andrew Morton
<[email protected]> napisał:
> On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <[email protected]> wrote:
>
>> +#define wait_interruptible_timeout(wq, timeout)
>> \
>> +({ \
>> + long ret = timeout; \
>> + \
>> + DEFINE_WAIT(wait); \
>> + prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE); \
>> + if (!signal_pending(current)) \
>> + ret = schedule_timeout(ret); \
>> + finish_wait(&wq, &wait); \
>> + \
>> + ret; \
>> +})
>
> It's often a mistake to use signals in-kernel. Signals are more a
> userspace thing and it's better to use the lower-level kernel-specific
> messaging tools in-kernel. Bear in mind that userspace can
> independently and asynchronously send, accept and block signals.
Can you point me to something kernel-level please?
> Can KMS use wait_event_interruptible_timeout()?
No. Please check definition of this:
#define wait_event_interruptible_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
if (!(condition)) \
__wait_event_interruptible_timeout(wq, condition, __ret); \
__ret; \
})
It uses condition there, but that's not a big issue. We just need to
pass 0 (false) there and it will work so far.
But then check __wait_event_interruptible_timeout definition, please.
It goes into sleep and after waking up it checks for value returned by
schedule_timeout. That's what breaks our (needed by radeon) sleeping.
If timeout didn't expire it does into sleep again!
What we need is continue reclocking after waking up. If this has
happend before timeout expired, that means we was woken up by VBLANK
interrupt handler. We love that situation and we do not want to go
sleep again.
On the other hand we need to have some timeout in case VBLANK
interrupt won't come.
--
Rafał
On Fri, Feb 26, 2010 at 06:33:57PM +0100, Rafał Miłecki wrote:
> W dniu 26 lutego 2010 17:14 użytkownik Andrew Morton
> <[email protected]> napisał:
> > On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <[email protected]> wrote:
> >
> >> +#define wait_interruptible_timeout(wq, timeout)
> >> \
> >> +({ \
> >> + long ret = timeout; \
> >> + \
> >> + DEFINE_WAIT(wait); \
> >> + prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE); \
> >> + if (!signal_pending(current)) \
> >> + ret = schedule_timeout(ret); \
> >> + finish_wait(&wq, &wait); \
> >> + \
> >> + ret; \
> >> +})
> >
> > It's often a mistake to use signals in-kernel. Signals are more a
> > userspace thing and it's better to use the lower-level kernel-specific
> > messaging tools in-kernel. Bear in mind that userspace can
> > independently and asynchronously send, accept and block signals.
>
> Can you point me to something kernel-level please?
>
>
> > Can KMS use wait_event_interruptible_timeout()?
>
> No. Please check definition of this:
>
> #define wait_event_interruptible_timeout(wq, condition, timeout) \
> ({ \
> long __ret = timeout; \
> if (!(condition)) \
> __wait_event_interruptible_timeout(wq, condition, __ret); \
> __ret; \
> })
>
> It uses condition there, but that's not a big issue. We just need to
> pass 0 (false) there and it will work so far.
Disabling the condition check doesn't make sense.
You could use a completion.
init_completion(vbl_irq);
enable_vbl_irq();
wait_for_completion(vbl_irq);
disable_vbl_irq();
and call complete(vbl_irq) in the interrupt handler.
The same would of course work with just some flag or counter
and a wait queue. Isn't there already a vbl counter that you could
compare in the condition?
--
Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/
On Fri, 26 Feb 2010, Rafał Miłecki wrote:
>
> Following macro is soemthing that seems to work fine for us, but instead
> introducing this to radeon KMS only, I'd like to propose adding this to whole
> wait.h. Do you this it's something we should place there? Can someone take this
> patch for me? Or maybe you find this rather useless and we should keep this
> marco locally?
This does not smell generic to me. In fact, it makes me personally think
you're doing something wrong in the first place, but maybe it's ok. But in
case it really is ok, I'd still not put it in a generic header file unless
you can point to other cases where it really makes sense to do this.
Linus
W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <[email protected]> napisał:
> Disabling the condition check doesn't make sense.
>
> You could use a completion.
>
> init_completion(vbl_irq);
> enable_vbl_irq();
> wait_for_completion(vbl_irq);
> disable_vbl_irq();
> and call complete(vbl_irq) in the interrupt handler.
>
> The same would of course work with just some flag or counter
> and a wait queue.
Ouch, I can see it gone bad already.
Firstly I simply just wanted to avoid condition in wait_event_*. It
looked unnecessary as I got interrupts (signals). So I started playing
with other solutions (like my wait_interruptible_timeout where I had
not full understanding of waking up) and finally started analyzing
over-complex things like completions.
I'll just use some one more variable and some more basic solution.
Thanks for help and sorry for taking your time. I hope to provide at
least some of you dynamic radeon PM in return :)
--
Rafał