diff mbox series

[v3] backlight: ktd253: Stabilize backlight

Message ID 20210604063201.3235245-1-linus.walleij@linaro.org (mailing list archive)
State New, archived
Headers show
Series [v3] backlight: ktd253: Stabilize backlight | expand

Commit Message

Linus Walleij June 4, 2021, 6:32 a.m. UTC
Remove interrupt disablement during backlight setting. It is
way to dangerous and makes platforms instable by having it
miss vblank IRQs leading to the graphics derailing.

The code is using ndelay() which is not available on
platforms such as ARM and will result in 32 * udelay(1)
which is substantial.

Add some code to detect if an interrupt occurs during the
tight loop and in that case just redo it from the top.

Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
Cc: Stephan Gerhold <stephan@gerhold.net>
Reported-by: newbyte@disroot.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Read my own patch and realized a bug: when we get a timeout
  we bounce back to max period, but still count down the pwm
  with one leading to an off-by-one error. Fix it by extending
  some else clauses.
ChangeLog v1->v2:
- Alter the dimming code to check for how many ns the pulse
  is low, and if it gets to ~100 us then redo from start.
  This is to account for the advent that an IRQ arrives while
  setting backlight and hits the low pulse making it way
  too long.
---
 drivers/video/backlight/ktd253-backlight.c | 75 ++++++++++++++++------
 1 file changed, 55 insertions(+), 20 deletions(-)

Comments

Daniel Thompson June 7, 2021, 11:11 a.m. UTC | #1
On Fri, Jun 04, 2021 at 08:32:01AM +0200, Linus Walleij wrote:
> Remove interrupt disablement during backlight setting. It is
> way to dangerous and makes platforms instable by having it
> miss vblank IRQs leading to the graphics derailing.
> 
> The code is using ndelay() which is not available on
> platforms such as ARM and will result in 32 * udelay(1)
> which is substantial.
> 
> Add some code to detect if an interrupt occurs during the
> tight loop and in that case just redo it from the top.
> 
> Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Reported-by: newbyte@disroot.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.


> ---
> ChangeLog v2->v3:
> - Read my own patch and realized a bug: when we get a timeout
>   we bounce back to max period, but still count down the pwm
>   with one leading to an off-by-one error. Fix it by extending
>   some else clauses.
> ChangeLog v1->v2:
> - Alter the dimming code to check for how many ns the pulse
>   is low, and if it gets to ~100 us then redo from start.
>   This is to account for the advent that an IRQ arrives while
>   setting backlight and hits the low pulse making it way
>   too long.
> ---
>  drivers/video/backlight/ktd253-backlight.c | 75 ++++++++++++++++------
>  1 file changed, 55 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c
> index a7df5bcca9da..37aa5a669530 100644
> --- a/drivers/video/backlight/ktd253-backlight.c
> +++ b/drivers/video/backlight/ktd253-backlight.c
> @@ -25,6 +25,7 @@
>  
>  #define KTD253_T_LOW_NS (200 + 10) /* Additional 10ns as safety factor */
>  #define KTD253_T_HIGH_NS (200 + 10) /* Additional 10ns as safety factor */
> +#define KTD253_T_OFF_CRIT_NS 100000 /* 100 us, now it doesn't look good */
>  #define KTD253_T_OFF_MS 3
>  
>  struct ktd253_backlight {
> @@ -34,13 +35,50 @@ struct ktd253_backlight {
>  	u16 ratio;
>  };
>  
> +static void ktd253_backlight_set_max_ratio(struct ktd253_backlight *ktd253)
> +{
> +	gpiod_set_value_cansleep(ktd253->gpiod, 1);
> +	ndelay(KTD253_T_HIGH_NS);
> +	/* We always fall back to this when we power on */
> +}
> +
> +static int ktd253_backlight_stepdown(struct ktd253_backlight *ktd253)
> +{
> +	/*
> +	 * These GPIO operations absolutely can NOT sleep so no _cansleep
> +	 * suffixes, and no using GPIO expanders on slow buses for this!
> +	 *
> +	 * The maximum number of cycles of the loop is 32  so the time taken
> +	 * should nominally be:
> +	 * (T_LOW_NS + T_HIGH_NS + loop_time) * 32
> +	 *
> +	 * Architectures do not always support ndelay() and we will get a few us
> +	 * instead. If we get to a critical time limit an interrupt has likely
> +	 * occured in the low part of the loop and we need to restart from the
> +	 * top so we have the backlight in a known state.
> +	 */
> +	u64 ns;
> +
> +	ns = ktime_get_ns();
> +	gpiod_set_value(ktd253->gpiod, 0);
> +	ndelay(KTD253_T_LOW_NS);
> +	gpiod_set_value(ktd253->gpiod, 1);
> +	ns = ktime_get_ns() - ns;
> +	if (ns >= KTD253_T_OFF_CRIT_NS) {
> +		dev_err(ktd253->dev, "PCM on backlight took too long (%llu ns)\n", ns);
> +		return -EAGAIN;
> +	}
> +	ndelay(KTD253_T_HIGH_NS);
> +	return 0;
> +}
> +
>  static int ktd253_backlight_update_status(struct backlight_device *bl)
>  {
>  	struct ktd253_backlight *ktd253 = bl_get_data(bl);
>  	int brightness = backlight_get_brightness(bl);
>  	u16 target_ratio;
>  	u16 current_ratio = ktd253->ratio;
> -	unsigned long flags;
> +	int ret;
>  
>  	dev_dbg(ktd253->dev, "new brightness/ratio: %d/32\n", brightness);
>  
> @@ -62,37 +100,34 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
>  	}
>  
>  	if (current_ratio == 0) {
> -		gpiod_set_value_cansleep(ktd253->gpiod, 1);
> -		ndelay(KTD253_T_HIGH_NS);
> -		/* We always fall back to this when we power on */
> +		ktd253_backlight_set_max_ratio(ktd253);
>  		current_ratio = KTD253_MAX_RATIO;
>  	}
>  
> -	/*
> -	 * WARNING:
> -	 * The loop to set the correct current level is performed
> -	 * with interrupts disabled as it is timing critical.
> -	 * The maximum number of cycles of the loop is 32
> -	 * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32,
> -	 */
> -	local_irq_save(flags);
>  	while (current_ratio != target_ratio) {
>  		/*
>  		 * These GPIO operations absolutely can NOT sleep so no
>  		 * _cansleep suffixes, and no using GPIO expanders on
>  		 * slow buses for this!
>  		 */
> -		gpiod_set_value(ktd253->gpiod, 0);
> -		ndelay(KTD253_T_LOW_NS);
> -		gpiod_set_value(ktd253->gpiod, 1);
> -		ndelay(KTD253_T_HIGH_NS);
> -		/* After 1/32 we loop back to 32/32 */
> -		if (current_ratio == KTD253_MIN_RATIO)
> +		ret = ktd253_backlight_stepdown(ktd253);
> +		if (ret == -EAGAIN) {
> +			/*
> +			 * Something disturbed the backlight setting code when
> +			 * running so we need to bring the PWM back to a known
> +			 * state. This shouldn't happen too much.
> +			 */
> +			gpiod_set_value_cansleep(ktd253->gpiod, 0);
> +			msleep(KTD253_T_OFF_MS);
> +			ktd253_backlight_set_max_ratio(ktd253);
> +			current_ratio = KTD253_MAX_RATIO;
> +		} else if (current_ratio == KTD253_MIN_RATIO) {
> +			/* After 1/32 we loop back to 32/32 */
>  			current_ratio = KTD253_MAX_RATIO;
> -		else
> +		} else {
>  			current_ratio--;
> +		}
>  	}
> -	local_irq_restore(flags);
>  	ktd253->ratio = current_ratio;
>  
>  	dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio);
> -- 
> 2.31.1
>
Linus Walleij July 13, 2021, 3:37 p.m. UTC | #2
On Fri, Jun 4, 2021 at 8:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> Remove interrupt disablement during backlight setting. It is
> way to dangerous and makes platforms instable by having it
> miss vblank IRQs leading to the graphics derailing.
>
> The code is using ndelay() which is not available on
> platforms such as ARM and will result in 32 * udelay(1)
> which is substantial.
>
> Add some code to detect if an interrupt occurs during the
> tight loop and in that case just redo it from the top.
>
> Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Reported-by: newbyte@disroot.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Hm it seems this patch did not make it into v5.14-rc1, could it be applied
as a fix for the -rc:s?

Shall I resend it with Daniel's ACK?

Yours,
Linus Walleij
Lee Jones July 15, 2021, 11:20 a.m. UTC | #3
On Tue, 13 Jul 2021, Linus Walleij wrote:

> On Fri, Jun 4, 2021 at 8:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > Remove interrupt disablement during backlight setting. It is
> > way to dangerous and makes platforms instable by having it
> > miss vblank IRQs leading to the graphics derailing.
> >
> > The code is using ndelay() which is not available on
> > platforms such as ARM and will result in 32 * udelay(1)
> > which is substantial.
> >
> > Add some code to detect if an interrupt occurs during the
> > tight loop and in that case just redo it from the top.
> >
> > Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
> > Cc: Stephan Gerhold <stephan@gerhold.net>
> > Reported-by: newbyte@disroot.org
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Hm it seems this patch did not make it into v5.14-rc1, could it be applied
> as a fix for the -rc:s?

Ah, it was sent late in the cycle, so I postponed it.

> Shall I resend it with Daniel's ACK?

Yes please.
diff mbox series

Patch

diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c
index a7df5bcca9da..37aa5a669530 100644
--- a/drivers/video/backlight/ktd253-backlight.c
+++ b/drivers/video/backlight/ktd253-backlight.c
@@ -25,6 +25,7 @@ 
 
 #define KTD253_T_LOW_NS (200 + 10) /* Additional 10ns as safety factor */
 #define KTD253_T_HIGH_NS (200 + 10) /* Additional 10ns as safety factor */
+#define KTD253_T_OFF_CRIT_NS 100000 /* 100 us, now it doesn't look good */
 #define KTD253_T_OFF_MS 3
 
 struct ktd253_backlight {
@@ -34,13 +35,50 @@  struct ktd253_backlight {
 	u16 ratio;
 };
 
+static void ktd253_backlight_set_max_ratio(struct ktd253_backlight *ktd253)
+{
+	gpiod_set_value_cansleep(ktd253->gpiod, 1);
+	ndelay(KTD253_T_HIGH_NS);
+	/* We always fall back to this when we power on */
+}
+
+static int ktd253_backlight_stepdown(struct ktd253_backlight *ktd253)
+{
+	/*
+	 * These GPIO operations absolutely can NOT sleep so no _cansleep
+	 * suffixes, and no using GPIO expanders on slow buses for this!
+	 *
+	 * The maximum number of cycles of the loop is 32  so the time taken
+	 * should nominally be:
+	 * (T_LOW_NS + T_HIGH_NS + loop_time) * 32
+	 *
+	 * Architectures do not always support ndelay() and we will get a few us
+	 * instead. If we get to a critical time limit an interrupt has likely
+	 * occured in the low part of the loop and we need to restart from the
+	 * top so we have the backlight in a known state.
+	 */
+	u64 ns;
+
+	ns = ktime_get_ns();
+	gpiod_set_value(ktd253->gpiod, 0);
+	ndelay(KTD253_T_LOW_NS);
+	gpiod_set_value(ktd253->gpiod, 1);
+	ns = ktime_get_ns() - ns;
+	if (ns >= KTD253_T_OFF_CRIT_NS) {
+		dev_err(ktd253->dev, "PCM on backlight took too long (%llu ns)\n", ns);
+		return -EAGAIN;
+	}
+	ndelay(KTD253_T_HIGH_NS);
+	return 0;
+}
+
 static int ktd253_backlight_update_status(struct backlight_device *bl)
 {
 	struct ktd253_backlight *ktd253 = bl_get_data(bl);
 	int brightness = backlight_get_brightness(bl);
 	u16 target_ratio;
 	u16 current_ratio = ktd253->ratio;
-	unsigned long flags;
+	int ret;
 
 	dev_dbg(ktd253->dev, "new brightness/ratio: %d/32\n", brightness);
 
@@ -62,37 +100,34 @@  static int ktd253_backlight_update_status(struct backlight_device *bl)
 	}
 
 	if (current_ratio == 0) {
-		gpiod_set_value_cansleep(ktd253->gpiod, 1);
-		ndelay(KTD253_T_HIGH_NS);
-		/* We always fall back to this when we power on */
+		ktd253_backlight_set_max_ratio(ktd253);
 		current_ratio = KTD253_MAX_RATIO;
 	}
 
-	/*
-	 * WARNING:
-	 * The loop to set the correct current level is performed
-	 * with interrupts disabled as it is timing critical.
-	 * The maximum number of cycles of the loop is 32
-	 * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32,
-	 */
-	local_irq_save(flags);
 	while (current_ratio != target_ratio) {
 		/*
 		 * These GPIO operations absolutely can NOT sleep so no
 		 * _cansleep suffixes, and no using GPIO expanders on
 		 * slow buses for this!
 		 */
-		gpiod_set_value(ktd253->gpiod, 0);
-		ndelay(KTD253_T_LOW_NS);
-		gpiod_set_value(ktd253->gpiod, 1);
-		ndelay(KTD253_T_HIGH_NS);
-		/* After 1/32 we loop back to 32/32 */
-		if (current_ratio == KTD253_MIN_RATIO)
+		ret = ktd253_backlight_stepdown(ktd253);
+		if (ret == -EAGAIN) {
+			/*
+			 * Something disturbed the backlight setting code when
+			 * running so we need to bring the PWM back to a known
+			 * state. This shouldn't happen too much.
+			 */
+			gpiod_set_value_cansleep(ktd253->gpiod, 0);
+			msleep(KTD253_T_OFF_MS);
+			ktd253_backlight_set_max_ratio(ktd253);
+			current_ratio = KTD253_MAX_RATIO;
+		} else if (current_ratio == KTD253_MIN_RATIO) {
+			/* After 1/32 we loop back to 32/32 */
 			current_ratio = KTD253_MAX_RATIO;
-		else
+		} else {
 			current_ratio--;
+		}
 	}
-	local_irq_restore(flags);
 	ktd253->ratio = current_ratio;
 
 	dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio);