diff mbox

[RFC,v1,0/1] Support to use own workqueue for each LED

Message ID 9a0a70a8-0886-1115-6151-72d2cba842cf@sberdevices.ru
State New
Headers show

Commit Message

Arseniy Krasnov Oct. 30, 2022, 6:04 a.m. UTC 
This allows to use own workqueue for each LED. This could be useful when we
have multiple LEDs which must work mutually (from user's point of view), for
example when complex animation must be played. Problem is that default wq -
'system_wq' does not guarantee order of callbacks execution, which control
brightness of every LED. So when userspace or pattern logic wants to change
brightness in one order, kworkers may do it in random way, thus breaking
smoothness of animation.

Here is example how to use this patch:


E.g. special workqueue must be allocated and set for each LED during init of
'led_classdev'. Then later in 'led_init_core()', 'system_wq' won't be used for
such LEDs.

Arseniy Krasnov(1):
 drivers/leds/led-core.c | 8 ++++++--
 include/linux/leds.h    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Arseniy Krasnov Oct. 30, 2022, 2:01 p.m. UTC | #1 
On 30.10.2022 15:20, Pavel Machek wrote:
> On Sun 2022-10-30 06:07:05, Arseniy Krasnov wrote:
>> This allows to set own workqueue for each LED. This may be useful, because
>> default 'system_wq' does not guarantee execution order of each work_struct,
>> thus for several brightness update requests (for multiple leds), real
>> brightness switch could be in random order.
> 
> So.. what?
> 
> Even if execution order is switched, human eye will not be able to
> tell the difference.
Hello,

Problem arises on one of our boards where we have 14 triples of leds(each
triple contains R G B). Test case is to play complex animation on all leds:
smooth switch from on RGB state to another. Sometimes there are glitches in
this process - divergence from expectable RGB state. We fixed this by using
ordered workqueue.

Thanks, Arseniy

> 								Pavel
Pavel Machek Oct. 30, 2022, 8:15 p.m. UTC | #2 
Hi!

> >> This allows to set own workqueue for each LED. This may be useful, because
> >> default 'system_wq' does not guarantee execution order of each work_struct,
> >> thus for several brightness update requests (for multiple leds), real
> >> brightness switch could be in random order.
> > 
> > So.. what?
> > 
> > Even if execution order is switched, human eye will not be able to
> > tell the difference.
> Hello,
> 
> Problem arises on one of our boards where we have 14 triples of leds(each
> triple contains R G B). Test case is to play complex animation on all leds:
> smooth switch from on RGB state to another. Sometimes there are glitches in
> this process - divergence from expectable RGB state. We fixed this by using
> ordered workqueue.

Are there other solutions possible? Like batch and always apply _all_
the updates you have queued from your the worker code?

Best regards,
								Pavel
Arseniy Krasnov Oct. 31, 2022, 7:01 a.m. UTC | #3 
On 30.10.2022 23:15, Pavel Machek wrote:
> Hi!
> 
>>>> This allows to set own workqueue for each LED. This may be useful, because
>>>> default 'system_wq' does not guarantee execution order of each work_struct,
>>>> thus for several brightness update requests (for multiple leds), real
>>>> brightness switch could be in random order.
>>>
>>> So.. what?
>>>
>>> Even if execution order is switched, human eye will not be able to
>>> tell the difference.
>> Hello,
>>
>> Problem arises on one of our boards where we have 14 triples of leds(each
>> triple contains R G B). Test case is to play complex animation on all leds:
>> smooth switch from on RGB state to another. Sometimes there are glitches in
>> this process - divergence from expectable RGB state. We fixed this by using
>> ordered workqueue.
> 
> Are there other solutions possible? Like batch and always apply _all_
> the updates you have queued from your the worker code?

IIUC You, it is possible to do this if brightness update requests are performed using
write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
case) - I can't synchronize these requests as they are created internally in kernel on
timer tick.

Thanks, Arseniy
> 
> Best regards,
> 								Pavel
>
Dmitry Rokosov Oct. 31, 2022, 2:47 p.m. UTC | #4 
Hello Pavel and Arseniy,

Please find my thoughts below.

On Mon, Oct 31, 2022 at 10:01:28AM +0300, Arseniy Krasnov wrote:
> On 30.10.2022 23:15, Pavel Machek wrote:
> > Hi!
> > 
> >>>> This allows to set own workqueue for each LED. This may be useful, because
> >>>> default 'system_wq' does not guarantee execution order of each work_struct,
> >>>> thus for several brightness update requests (for multiple leds), real
> >>>> brightness switch could be in random order.
> >>>
> >>> So.. what?
> >>>
> >>> Even if execution order is switched, human eye will not be able to
> >>> tell the difference.
> >> Hello,
> >>
> >> Problem arises on one of our boards where we have 14 triples of leds(each
> >> triple contains R G B). Test case is to play complex animation on all leds:
> >> smooth switch from on RGB state to another. Sometimes there are glitches in
> >> this process - divergence from expectable RGB state. We fixed this by using
> >> ordered workqueue.
> > 
> > Are there other solutions possible? Like batch and always apply _all_
> > the updates you have queued from your the worker code?
> 
> IIUC You, it is possible to do this if brightness update requests are performed using
> write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
> case) - I can't synchronize these requests as they are created internally in kernel on
> timer tick.

Even more, system_wq is used when you push brightness changing requests
to sysfs node, and it could be re-ordered as well. In other words, from
queue perspective sysfs iface and trigger iface have the same behavior.

Also we can be faced with another big problem here: let's imagine you have
I2C based LED controller driver. Usually, in such drivers you're stuck
to the one driver owned mutex, which protects I2C transactions from each
other.

When you change brightness very often (let's say a hundred thousand times
per minute) you schedule many workers to system_wq. Due to system_wq is
multicore and unordered it creates many kworkers. Each kworker stucks on
the driver mutex and goes to TASK_UNINTERRUPTIBLE state. It affects Load
Average value so much. On the our device LA maximum could reach 30-35
units due to such idle kworkers.

I'm not sure custom workqueue initialization from specific HW driver is
a good solution... But it's much better than nothing.

Pavel, please share your thoughts about above problems? Maybe you have
more advanced and scalable solution idea, I would appreciate if you
could share it with us.
Dmitry Rokosov Nov. 9, 2022, 6:01 p.m. UTC | #5 
Hello Pavel,

Do you have any feedback/thoughts about this one?
The problem looks very bad, Load Average value should not grow up due to
any userspace triggered led animation. I suppose we stronlgy need to
fixup this behavior for sure.

Appreciate any help!

On Mon, Oct 31, 2022 at 05:47:04PM +0300, Dmitry Rokosov wrote:
> Hello Pavel and Arseniy,
> 
> Please find my thoughts below.
> 
> On Mon, Oct 31, 2022 at 10:01:28AM +0300, Arseniy Krasnov wrote:
> > On 30.10.2022 23:15, Pavel Machek wrote:
> > > Hi!
> > > 
> > >>>> This allows to set own workqueue for each LED. This may be useful, because
> > >>>> default 'system_wq' does not guarantee execution order of each work_struct,
> > >>>> thus for several brightness update requests (for multiple leds), real
> > >>>> brightness switch could be in random order.
> > >>>
> > >>> So.. what?
> > >>>
> > >>> Even if execution order is switched, human eye will not be able to
> > >>> tell the difference.
> > >> Hello,
> > >>
> > >> Problem arises on one of our boards where we have 14 triples of leds(each
> > >> triple contains R G B). Test case is to play complex animation on all leds:
> > >> smooth switch from on RGB state to another. Sometimes there are glitches in
> > >> this process - divergence from expectable RGB state. We fixed this by using
> > >> ordered workqueue.
> > > 
> > > Are there other solutions possible? Like batch and always apply _all_
> > > the updates you have queued from your the worker code?
> > 
> > IIUC You, it is possible to do this if brightness update requests are performed using
> > write to "brightness" file in /sys/class/led/. But if pattern trigger mode is used(in my
> > case) - I can't synchronize these requests as they are created internally in kernel on
> > timer tick.
> 
> Even more, system_wq is used when you push brightness changing requests
> to sysfs node, and it could be re-ordered as well. In other words, from
> queue perspective sysfs iface and trigger iface have the same behavior.
> 
> Also we can be faced with another big problem here: let's imagine you have
> I2C based LED controller driver. Usually, in such drivers you're stuck
> to the one driver owned mutex, which protects I2C transactions from each
> other.
> 
> When you change brightness very often (let's say a hundred thousand times
> per minute) you schedule many workers to system_wq. Due to system_wq is
> multicore and unordered it creates many kworkers. Each kworker stucks on
> the driver mutex and goes to TASK_UNINTERRUPTIBLE state. It affects Load
> Average value so much. On the our device LA maximum could reach 30-35
> units due to such idle kworkers.
> 
> I'm not sure custom workqueue initialization from specific HW driver is
> a good solution... But it's much better than nothing.
> 
> Pavel, please share your thoughts about above problems? Maybe you have
> more advanced and scalable solution idea, I would appreciate if you
> could share it with us.
diff mbox

Patch

--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -264,11 +264,17 @@  static int aw2013_probe_dt(struct aw2013 *chip)
 	struct device_node *np = dev_of_node(&chip->client->dev), *child;
 	int count, ret = 0, i = 0;
 	struct aw2013_led *led;
+	struct workqueue_struct *wq;

 	count = of_get_available_child_count(np);
 	if (!count || count > AW2013_MAX_LEDS)
 		return -EINVAL;

+	wq = alloc_ordered_workqueue("aw2013_wq", 0);
+
+	if (!wq)
+		return -ENOMEM;
+
 	regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);

 	for_each_available_child_of_node(np, child) {
@@ -299,6 +305,7 @@  static int aw2013_probe_dt(struct aw2013 *chip)

 		led->cdev.brightness_set_blocking = aw2013_brightness_set;
 		led->cdev.blink_set = aw2013_blink_set;
+		led->cdev.set_brightness_wq = wq;

 		ret = devm_led_classdev_register_ext(&chip->client->dev,
 						     &led->cdev, &init_data);