[PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sebastian Reichel]

The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
afterwards it calls acx565akm_detect(), which sets the GPIO value to
HIGH. If the bootloader initialized the GPIO to HIGH before the probe
routine was called, there is only a very short time period of a few
instructions where the reset signal is LOW. Exact time depends on
compiler optimizations, kernel configuration and alignment of the stars,
but I expect it to be always way less than 10us. There are no public
datasheets for the panel, but acx565akm_power_on() has a comment with
timings and reset period should be at least 10us. So this potentially
brings the panel into a half-reset state.

The result is, that panel may not work after boot and can get into a
working state by re-enabling it (e.g. by blanking + unblanking), since
that does a clean reset cycle. This bug has recently been hit by Ivaylo
Dimitrov, but there are some older reports which are probably the same
bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
experienced it in 2017 describing the blank/unblank procedure as
possible workaround.

Note, that the bug really goes back in time. It has originally been
introduced in the predecessor of the omapfb driver in 3c45d05be382
("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
That driver eventually got replaced by a newer one, which had the bug
from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
driver") and still exists in fbdev world. That driver has later been
copied to omapdrm and then was used as a basis for this driver. Last
but not least the omapdrm specific driver has been removed in
45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").

Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reported-by: Tony Lindgren <tony@atomide.com>
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
index e95fdfb16b6c..ba0b3ead150f 100644
--- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
+++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
@@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
 	lcd->spi = spi;
 	mutex_init(&lcd->mutex);
 
-	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(lcd->reset_gpio)) {
 		dev_err(&spi->dev, "failed to get reset GPIO\n");
 		return PTR_ERR(lcd->reset_gpio);
-- 
2.29.2

[PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sebastian Reichel]

The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
afterwards it calls acx565akm_detect(), which sets the GPIO value to
HIGH. If the bootloader initialized the GPIO to HIGH before the probe
routine was called, there is only a very short time period of a few
instructions where the reset signal is LOW. Exact time depends on
compiler optimizations, kernel configuration and alignment of the stars,
but I expect it to be always way less than 10us. There are no public
datasheets for the panel, but acx565akm_power_on() has a comment with
timings and reset period should be at least 10us. So this potentially
brings the panel into a half-reset state.

The result is, that panel may not work after boot and can get into a
working state by re-enabling it (e.g. by blanking + unblanking), since
that does a clean reset cycle. This bug has recently been hit by Ivaylo
Dimitrov, but there are some older reports which are probably the same
bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
experienced it in 2017 describing the blank/unblank procedure as
possible workaround.

Note, that the bug really goes back in time. It has originally been
introduced in the predecessor of the omapfb driver in 3c45d05be382
("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
That driver eventually got replaced by a newer one, which had the bug
from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
driver") and still exists in fbdev world. That driver has later been
copied to omapdrm and then was used as a basis for this driver. Last
but not least the omapdrm specific driver has been removed in
45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").

Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reported-by: Tony Lindgren <tony@atomide.com>
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
index e95fdfb16b6c..ba0b3ead150f 100644
--- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
+++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
@@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
 	lcd->spi = spi;
 	mutex_init(&lcd->mutex);
 
-	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(lcd->reset_gpio)) {
 		dev_err(&spi->dev, "failed to get reset GPIO\n");
 		return PTR_ERR(lcd->reset_gpio);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Ivaylo Dimitrov]

You may add:

Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

On 27.11.20 г. 22:04 ч., Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>   	lcd->spi = spi;
>   	mutex_init(&lcd->mutex);
>   
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
>   	if (IS_ERR(lcd->reset_gpio)) {
>   		dev_err(&spi->dev, "failed to get reset GPIO\n");
>   		return PTR_ERR(lcd->reset_gpio);
> 

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Ivaylo Dimitrov]

You may add:

Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

On 27.11.20 г. 22:04 ч., Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>   	lcd->spi = spi;
>   	mutex_init(&lcd->mutex);
>   
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
>   	if (IS_ERR(lcd->reset_gpio)) {
>   		dev_err(&spi->dev, "failed to get reset GPIO\n");
>   		return PTR_ERR(lcd->reset_gpio);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Aaro Koskinen]

Hi,

On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

Thanks,

A.

> ---
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>  	lcd->spi = spi;
>  	mutex_init(&lcd->mutex);
>  
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(lcd->reset_gpio)) {
>  		dev_err(&spi->dev, "failed to get reset GPIO\n");
>  		return PTR_ERR(lcd->reset_gpio);
> -- 
> 2.29.2
> 

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Aaro Koskinen]

Hi,

On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

Thanks,

A.

> ---
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>  	lcd->spi = spi;
>  	mutex_init(&lcd->mutex);
>  
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(lcd->reset_gpio)) {
>  		dev_err(&spi->dev, "failed to get reset GPIO\n");
>  		return PTR_ERR(lcd->reset_gpio);
> -- 
> 2.29.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.

Good catch.

Looks like we got the reset polarity wrong in the driver though.
GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
low level. We can't fix that as it would require changing the device
tree :-(

> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>  	lcd->spi = spi;
>  	mutex_init(&lcd->mutex);
>  
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);

Wouldn't it be better to instead add a delay here (or in
acx565akm_detect()) ? If the panel is in a wrong state at boot time, a
real reset can help.

>  	if (IS_ERR(lcd->reset_gpio)) {
>  		dev_err(&spi->dev, "failed to get reset GPIO\n");
>  		return PTR_ERR(lcd->reset_gpio);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.

Good catch.

Looks like we got the reset polarity wrong in the driver though.
GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
low level. We can't fix that as it would require changing the device
tree :-(

> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index e95fdfb16b6c..ba0b3ead150f 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
>  	lcd->spi = spi;
>  	mutex_init(&lcd->mutex);
>  
> -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);

Wouldn't it be better to instead add a delay here (or in
acx565akm_detect()) ? If the panel is in a wrong state at boot time, a
real reset can help.

>  	if (IS_ERR(lcd->reset_gpio)) {
>  		dev_err(&spi->dev, "failed to get reset GPIO\n");
>  		return PTR_ERR(lcd->reset_gpio);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4476 bytes --]

Hi Laurent,

On Sun, Nov 29, 2020 at 12:08:47AM +0200, Laurent Pinchart wrote:
> On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> > The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> > afterwards it calls acx565akm_detect(), which sets the GPIO value to
> > HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> > routine was called, there is only a very short time period of a few
> > instructions where the reset signal is LOW. Exact time depends on
> > compiler optimizations, kernel configuration and alignment of the stars,
> > but I expect it to be always way less than 10us. There are no public
> > datasheets for the panel, but acx565akm_power_on() has a comment with
> > timings and reset period should be at least 10us. So this potentially
> > brings the panel into a half-reset state.
> 
> Good catch.
> 
> Looks like we got the reset polarity wrong in the driver though.
> GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
> low level. We can't fix that as it would require changing the device
> tree :-(

Yes, polarity is wrong unfortunately.

> > The result is, that panel may not work after boot and can get into a
> > working state by re-enabling it (e.g. by blanking + unblanking), since
> > that does a clean reset cycle. This bug has recently been hit by Ivaylo
> > Dimitrov, but there are some older reports which are probably the same
> > bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> > experienced it in 2017 describing the blank/unblank procedure as
> > possible workaround.
> > 
> > Note, that the bug really goes back in time. It has originally been
> > introduced in the predecessor of the omapfb driver in 3c45d05be382
> > ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> > That driver eventually got replaced by a newer one, which had the bug
> > from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> > driver") and still exists in fbdev world. That driver has later been
> > copied to omapdrm and then was used as a basis for this driver. Last
> > but not least the omapdrm specific driver has been removed in
> > 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> > 
> > Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Reported-by: Tony Lindgren <tony@atomide.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > index e95fdfb16b6c..ba0b3ead150f 100644
> > --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
> >  	lcd->spi = spi;
> >  	mutex_init(&lcd->mutex);
> >  
> > -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> > +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> 
> Wouldn't it be better to instead add a delay here (or in
> acx565akm_detect()) ? If the panel is in a wrong state at
> boot time, a real reset can help.

acx565akm_detect() reads some registers to detect a previously
enabled panel and then driver handles this case properly. If we
reset the panel before the detection code, any detection code
would be useless (panel is obviously not enabled after a reset).

I think this detection code is only needed to avoid flickering
when a bootsplash is shown. So by accepting a bit of flickering
we can simplify the driver by dropping that code and make it a
bit more robust by doing a reset. It's a tradeoff and I don't
have strong feelings for either option.

But I think, that this fix should be applied to fixes branch
(and backported to stable). Removing panel enable detection
should not be applied as fix IMHO.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 4476 bytes --]

Hi Laurent,

On Sun, Nov 29, 2020 at 12:08:47AM +0200, Laurent Pinchart wrote:
> On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> > The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> > afterwards it calls acx565akm_detect(), which sets the GPIO value to
> > HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> > routine was called, there is only a very short time period of a few
> > instructions where the reset signal is LOW. Exact time depends on
> > compiler optimizations, kernel configuration and alignment of the stars,
> > but I expect it to be always way less than 10us. There are no public
> > datasheets for the panel, but acx565akm_power_on() has a comment with
> > timings and reset period should be at least 10us. So this potentially
> > brings the panel into a half-reset state.
> 
> Good catch.
> 
> Looks like we got the reset polarity wrong in the driver though.
> GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
> low level. We can't fix that as it would require changing the device
> tree :-(

Yes, polarity is wrong unfortunately.

> > The result is, that panel may not work after boot and can get into a
> > working state by re-enabling it (e.g. by blanking + unblanking), since
> > that does a clean reset cycle. This bug has recently been hit by Ivaylo
> > Dimitrov, but there are some older reports which are probably the same
> > bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> > experienced it in 2017 describing the blank/unblank procedure as
> > possible workaround.
> > 
> > Note, that the bug really goes back in time. It has originally been
> > introduced in the predecessor of the omapfb driver in 3c45d05be382
> > ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> > That driver eventually got replaced by a newer one, which had the bug
> > from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> > driver") and still exists in fbdev world. That driver has later been
> > copied to omapdrm and then was used as a basis for this driver. Last
> > but not least the omapdrm specific driver has been removed in
> > 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> > 
> > Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > Reported-by: Tony Lindgren <tony@atomide.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > index e95fdfb16b6c..ba0b3ead150f 100644
> > --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
> >  	lcd->spi = spi;
> >  	mutex_init(&lcd->mutex);
> >  
> > -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> > +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> 
> Wouldn't it be better to instead add a delay here (or in
> acx565akm_detect()) ? If the panel is in a wrong state at
> boot time, a real reset can help.

acx565akm_detect() reads some registers to detect a previously
enabled panel and then driver handles this case properly. If we
reset the panel before the detection code, any detection code
would be useless (panel is obviously not enabled after a reset).

I think this detection code is only needed to avoid flickering
when a bootsplash is shown. So by accepting a bit of flickering
we can simplify the driver by dropping that code and make it a
bit more robust by doing a reset. It's a tradeoff and I don't
have strong feelings for either option.

But I think, that this fix should be applied to fixes branch
(and backported to stable). Removing panel enable detection
should not be applied as fix IMHO.

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Jarkko Nikula]

On 11/28/20 7:46 PM, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
>> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
>> afterwards it calls acx565akm_detect(), which sets the GPIO value to
>> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
>> routine was called, there is only a very short time period of a few
>> instructions where the reset signal is LOW. Exact time depends on
>> compiler optimizations, kernel configuration and alignment of the stars,
>> but I expect it to be always way less than 10us. There are no public
>> datasheets for the panel, but acx565akm_power_on() has a comment with
>> timings and reset period should be at least 10us. So this potentially
>> brings the panel into a half-reset state.
>>
>> The result is, that panel may not work after boot and can get into a
>> working state by re-enabling it (e.g. by blanking + unblanking), since
>> that does a clean reset cycle. This bug has recently been hit by Ivaylo
>> Dimitrov, but there are some older reports which are probably the same
>> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
>> experienced it in 2017 describing the blank/unblank procedure as
>> possible workaround.
>>
>> Note, that the bug really goes back in time. It has originally been
>> introduced in the predecessor of the omapfb driver in 3c45d05be382
>> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
>> That driver eventually got replaced by a newer one, which had the bug
>> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
>> driver") and still exists in fbdev world. That driver has later been
>> copied to omapdrm and then was used as a basis for this driver. Last
>> but not least the omapdrm specific driver has been removed in
>> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
>>
>> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
>> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Reported-by: Tony Lindgren <tony@atomide.com>
>> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
>> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> Cc: Merlijn Wajer <merlijn@wizzup.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> 
I had difficulties with recent kernels. Yesterday's vanilla head
c84e1efae022 and linux-next next-20201127 didn't boot, v5.9.11 had some
other DRM issues. I went back to v5.4.80 which didn't show this panel
issue and was actually working fine. Strange since obviously issue exist
before.

But v5.6.19 was testable, had the issue and this patch fixed it, so

Tested-by: Jarkko Nikula <jarkko.nikula@bitmer.com>

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Jarkko Nikula]

On 11/28/20 7:46 PM, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
>> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
>> afterwards it calls acx565akm_detect(), which sets the GPIO value to
>> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
>> routine was called, there is only a very short time period of a few
>> instructions where the reset signal is LOW. Exact time depends on
>> compiler optimizations, kernel configuration and alignment of the stars,
>> but I expect it to be always way less than 10us. There are no public
>> datasheets for the panel, but acx565akm_power_on() has a comment with
>> timings and reset period should be at least 10us. So this potentially
>> brings the panel into a half-reset state.
>>
>> The result is, that panel may not work after boot and can get into a
>> working state by re-enabling it (e.g. by blanking + unblanking), since
>> that does a clean reset cycle. This bug has recently been hit by Ivaylo
>> Dimitrov, but there are some older reports which are probably the same
>> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
>> experienced it in 2017 describing the blank/unblank procedure as
>> possible workaround.
>>
>> Note, that the bug really goes back in time. It has originally been
>> introduced in the predecessor of the omapfb driver in 3c45d05be382
>> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
>> That driver eventually got replaced by a newer one, which had the bug
>> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
>> driver") and still exists in fbdev world. That driver has later been
>> copied to omapdrm and then was used as a basis for this driver. Last
>> but not least the omapdrm specific driver has been removed in
>> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
>>
>> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
>> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Reported-by: Tony Lindgren <tony@atomide.com>
>> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
>> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> Cc: Merlijn Wajer <merlijn@wizzup.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> 
I had difficulties with recent kernels. Yesterday's vanilla head
c84e1efae022 and linux-next next-20201127 didn't boot, v5.9.11 had some
other DRM issues. I went back to v5.4.80 which didn't show this panel
issue and was actually working fine. Strange since obviously issue exist
before.

But v5.6.19 was testable, had the issue and this patch fixed it, so

Tested-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sam Ravnborg]

Hi Sebastian,
On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>


Fixed up the commit references, added Tested-by (impressive list) and
committed to drm-misc-fixes.

Commit references shall look like this:

    commit 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel driver")

The word commit is required according to dim (the tool we use for
drm-misc maintenance).

	Sam

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Sam Ravnborg]

Hi Sebastian,
On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> afterwards it calls acx565akm_detect(), which sets the GPIO value to
> HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> routine was called, there is only a very short time period of a few
> instructions where the reset signal is LOW. Exact time depends on
> compiler optimizations, kernel configuration and alignment of the stars,
> but I expect it to be always way less than 10us. There are no public
> datasheets for the panel, but acx565akm_power_on() has a comment with
> timings and reset period should be at least 10us. So this potentially
> brings the panel into a half-reset state.
> 
> The result is, that panel may not work after boot and can get into a
> working state by re-enabling it (e.g. by blanking + unblanking), since
> that does a clean reset cycle. This bug has recently been hit by Ivaylo
> Dimitrov, but there are some older reports which are probably the same
> bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> experienced it in 2017 describing the blank/unblank procedure as
> possible workaround.
> 
> Note, that the bug really goes back in time. It has originally been
> introduced in the predecessor of the omapfb driver in 3c45d05be382
> ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> That driver eventually got replaced by a newer one, which had the bug
> from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> driver") and still exists in fbdev world. That driver has later been
> copied to omapdrm and then was used as a basis for this driver. Last
> but not least the omapdrm specific driver has been removed in
> 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Reported-by: Tony Lindgren <tony@atomide.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>


Fixed up the commit references, added Tested-by (impressive list) and
committed to drm-misc-fixes.

Commit references shall look like this:

    commit 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel driver")

The word commit is required according to dim (the tool we use for
drm-misc maintenance).

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Laurent Pinchart]

Hi Sebastian,

On Sun, Nov 29, 2020 at 01:53:31AM +0100, Sebastian Reichel wrote:
> On Sun, Nov 29, 2020 at 12:08:47AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> > > The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> > > afterwards it calls acx565akm_detect(), which sets the GPIO value to
> > > HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> > > routine was called, there is only a very short time period of a few
> > > instructions where the reset signal is LOW. Exact time depends on
> > > compiler optimizations, kernel configuration and alignment of the stars,
> > > but I expect it to be always way less than 10us. There are no public
> > > datasheets for the panel, but acx565akm_power_on() has a comment with
> > > timings and reset period should be at least 10us. So this potentially
> > > brings the panel into a half-reset state.
> > 
> > Good catch.
> > 
> > Looks like we got the reset polarity wrong in the driver though.
> > GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
> > low level. We can't fix that as it would require changing the device
> > tree :-(
> 
> Yes, polarity is wrong unfortunately.
> 
> > > The result is, that panel may not work after boot and can get into a
> > > working state by re-enabling it (e.g. by blanking + unblanking), since
> > > that does a clean reset cycle. This bug has recently been hit by Ivaylo
> > > Dimitrov, but there are some older reports which are probably the same
> > > bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> > > experienced it in 2017 describing the blank/unblank procedure as
> > > possible workaround.
> > > 
> > > Note, that the bug really goes back in time. It has originally been
> > > introduced in the predecessor of the omapfb driver in 3c45d05be382
> > > ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> > > That driver eventually got replaced by a newer one, which had the bug
> > > from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> > > driver") and still exists in fbdev world. That driver has later been
> > > copied to omapdrm and then was used as a basis for this driver. Last
> > > but not least the omapdrm specific driver has been removed in
> > > 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> > > 
> > > Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> > > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > Reported-by: Tony Lindgren <tony@atomide.com>
> > > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > > Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > index e95fdfb16b6c..ba0b3ead150f 100644
> > > --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
> > >  	lcd->spi = spi;
> > >  	mutex_init(&lcd->mutex);
> > >  
> > > -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> > > +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> > 
> > Wouldn't it be better to instead add a delay here (or in
> > acx565akm_detect()) ? If the panel is in a wrong state at
> > boot time, a real reset can help.
> 
> acx565akm_detect() reads some registers to detect a previously
> enabled panel and then driver handles this case properly. If we
> reset the panel before the detection code, any detection code
> would be useless (panel is obviously not enabled after a reset).
> 
> I think this detection code is only needed to avoid flickering
> when a bootsplash is shown. So by accepting a bit of flickering
> we can simplify the driver by dropping that code and make it a
> bit more robust by doing a reset. It's a tradeoff and I don't
> have strong feelings for either option.
> 
> But I think, that this fix should be applied to fixes branch
> (and backported to stable). Removing panel enable detection
> should not be applied as fix IMHO.

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/panel: sony-acx565akm: Fix race condition in probe

[Laurent Pinchart]

Hi Sebastian,

On Sun, Nov 29, 2020 at 01:53:31AM +0100, Sebastian Reichel wrote:
> On Sun, Nov 29, 2020 at 12:08:47AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 27, 2020 at 09:04:29PM +0100, Sebastian Reichel wrote:
> > > The probe routine acquires the reset GPIO using GPIOD_OUT_LOW. Directly
> > > afterwards it calls acx565akm_detect(), which sets the GPIO value to
> > > HIGH. If the bootloader initialized the GPIO to HIGH before the probe
> > > routine was called, there is only a very short time period of a few
> > > instructions where the reset signal is LOW. Exact time depends on
> > > compiler optimizations, kernel configuration and alignment of the stars,
> > > but I expect it to be always way less than 10us. There are no public
> > > datasheets for the panel, but acx565akm_power_on() has a comment with
> > > timings and reset period should be at least 10us. So this potentially
> > > brings the panel into a half-reset state.
> > 
> > Good catch.
> > 
> > Looks like we got the reset polarity wrong in the driver though.
> > GPIOD_OUT_LOW should mean de-asserted, but the driver expects it to mean
> > low level. We can't fix that as it would require changing the device
> > tree :-(
> 
> Yes, polarity is wrong unfortunately.
> 
> > > The result is, that panel may not work after boot and can get into a
> > > working state by re-enabling it (e.g. by blanking + unblanking), since
> > > that does a clean reset cycle. This bug has recently been hit by Ivaylo
> > > Dimitrov, but there are some older reports which are probably the same
> > > bug. At least Tony Lindgren, Peter Ujfalusi and Jarkko Nikula have
> > > experienced it in 2017 describing the blank/unblank procedure as
> > > possible workaround.
> > > 
> > > Note, that the bug really goes back in time. It has originally been
> > > introduced in the predecessor of the omapfb driver in 3c45d05be382
> > > ("OMAPDSS: acx565akm panel: handle gpios in panel driver") in 2012.
> > > That driver eventually got replaced by a newer one, which had the bug
> > > from the beginning in 84192742d9c2 ("OMAPDSS: Add Sony ACX565AKM panel
> > > driver") and still exists in fbdev world. That driver has later been
> > > copied to omapdrm and then was used as a basis for this driver. Last
> > > but not least the omapdrm specific driver has been removed in
> > > 45f16c82db7e ("drm/omap: displays: Remove unused panel drivers").
> > > 
> > > Reported-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
> > > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > Reported-by: Tony Lindgren <tony@atomide.com>
> > > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > > Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > Fixes: 1c8fc3f0c5d2 ("drm/panel: Add driver for the Sony ACX565AKM panel")
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > index e95fdfb16b6c..ba0b3ead150f 100644
> > > --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> > > @@ -629,7 +629,7 @@ static int acx565akm_probe(struct spi_device *spi)
> > >  	lcd->spi = spi;
> > >  	mutex_init(&lcd->mutex);
> > >  
> > > -	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> > > +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> > 
> > Wouldn't it be better to instead add a delay here (or in
> > acx565akm_detect()) ? If the panel is in a wrong state at
> > boot time, a real reset can help.
> 
> acx565akm_detect() reads some registers to detect a previously
> enabled panel and then driver handles this case properly. If we
> reset the panel before the detection code, any detection code
> would be useless (panel is obviously not enabled after a reset).
> 
> I think this detection code is only needed to avoid flickering
> when a bootsplash is shown. So by accepting a bit of flickering
> we can simplify the driver by dropping that code and make it a
> bit more robust by doing a reset. It's a tradeoff and I don't
> have strong feelings for either option.
> 
> But I think, that this fix should be applied to fixes branch
> (and backported to stable). Removing panel enable detection
> should not be applied as fix IMHO.

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel