[PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
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

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Thomas Hellstrom]

Rafał Miłecki wrote:
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> 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

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 21 lutego 2010 16:01 użytkownik Thomas Hellstrom
<thomas@shipmail.org> napisał:
> Rafał Miłecki wrote:
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

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ł

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3626 bytes --]

Forwarding to ppl I could often notice in git log time.h

---------- Wiadomość przekazana dalej ----------From: Rafał Miłecki <zajec5@gmail.com>Date: 21 lutego 2010 15:10Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro tosleep (w. timeout) until wake_upTo: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,dri-devel@lists.sourceforge.netCC: Rafał Miłecki <zajec5@gmail.com>

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>---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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

Forwarding to ppl I could often notice in git log time.h


---------- Wiadomość przekazana dalej ----------
From: Rafał Miłecki <zajec5@gmail.com>
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 <linux-kernel@vger.kernel.org>,
dri-devel@lists.sourceforge.net
CC: Rafał Miłecki <zajec5@gmail.com>


Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
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

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1921 bytes --]

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 <zajec5@gmail.com>
> 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 <linux-kernel@vger.kernel.org>,
> dri-devel@lists.sourceforge.net
> CC: Rafał Miłecki <zajec5@gmail.com>
> 
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> 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

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
<tglx@linutronix.de> 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 <zajec5@gmail.com>
>> 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 <linux-kernel@vger.kernel.org>,
>> dri-devel@lists.sourceforge.net
>> CC: Rafał Miłecki <zajec5@gmail.com>
>>
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
<tglx@linutronix.de> 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 <zajec5@gmail.com>
>> 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 <linux-kernel@vger.kernel.org>,
>> dri-devel@lists.sourceforge.net
>> CC: Rafał Miłecki <zajec5@gmail.com>
>>
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Andrew Morton]

On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zajec5@gmail.com> 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()?

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 17:14 użytkownik Andrew Morton
<akpm@linux-foundation.org> napisał:
> On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zajec5@gmail.com> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 17:14 użytkownik Andrew Morton
<akpm@linux-foundation.org> napisał:
> On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zajec5@gmail.com> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Ville Syrjälä]

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
> <akpm@linux-foundation.org> napisał:
> > On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zajec5@gmail.com> 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ä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Ville Syrjälä]

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
> <akpm@linux-foundation.org> napisał:
> > On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zajec5@gmail.com> 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ä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Linus Torvalds]

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

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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ł

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Michel Dänzer]

On Sat, 2010-02-27 at 10:33 +0100, Rafał Miłecki wrote: 
> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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 this code runs in user process context? If so, it should return to
userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
movement may suffer.

If that's a problem, then maybe the code should run in a different
context, e.g. a tasklet or some kind of worker kernel thread.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Michel Dänzer]

On Sat, 2010-02-27 at 10:33 +0100, Rafał Miłecki wrote: 
> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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 this code runs in user process context? If so, it should return to
userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
movement may suffer.

If that's a problem, then maybe the code should run in a different
context, e.g. a tasklet or some kind of worker kernel thread.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 1 marca 2010 17:37 użytkownik Michel Dänzer <michel@daenzer.net> napisał:
> On Sat, 2010-02-27 at 10:33 +0100, Rafał Miłecki wrote:
>> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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 this code runs in user process context? If so, it should return to
> userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
> movement may suffer.
>
> If that's a problem, then maybe the code should run in a different
> context, e.g. a tasklet or some kind of worker kernel thread.

It has nothing to do with userspace. Please see my previous description:



W dniu 26 lutego 2010 13:16 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał:
> W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
>> 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.



It's delayed_work.

-- 
Rafał

Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up

[Rafał Miłecki]

W dniu 1 marca 2010 17:37 użytkownik Michel Dänzer <michel@daenzer.net> napisał:
> On Sat, 2010-02-27 at 10:33 +0100, Rafał Miłecki wrote:
>> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syrjala@sci.fi> 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 this code runs in user process context? If so, it should return to
> userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
> movement may suffer.
>
> If that's a problem, then maybe the code should run in a different
> context, e.g. a tasklet or some kind of worker kernel thread.

It has nothing to do with userspace. Please see my previous description:



W dniu 26 lutego 2010 13:16 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał:
> W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
>> 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.



It's delayed_work.

-- 
Rafał

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel