* [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-16 12:37 ` Ondrej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, Samuel Holland, dri-devel, linux-kernel When extending the driver for xbd599 panel support I tried to do minimal changes and keep the existing initialization timing. It turned out that it's not good enough and the existing init sequence is too aggressive and doesn't follow the specification. On PinePhone panel is being powered down/up during suspend/resume and with current timings this frequently leads to corrupted display. This patch series fixes the problems. The issue was reported by Samuel Holland. Relevant screenshots from the datasheet: Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf Please take a look. thank you and regards, Ondrej Jirman Ondrej Jirman (2): drm/panel: st7703: Make the sleep exit timing match the spec drm/panel: st7703: Fix the power up sequence of the panel drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-16 12:37 ` Ondrej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, linux-kernel, dri-devel, Samuel Holland When extending the driver for xbd599 panel support I tried to do minimal changes and keep the existing initialization timing. It turned out that it's not good enough and the existing init sequence is too aggressive and doesn't follow the specification. On PinePhone panel is being powered down/up during suspend/resume and with current timings this frequently leads to corrupted display. This patch series fixes the problems. The issue was reported by Samuel Holland. Relevant screenshots from the datasheet: Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf Please take a look. thank you and regards, Ondrej Jirman Ondrej Jirman (2): drm/panel: st7703: Make the sleep exit timing match the spec drm/panel: st7703: Fix the power up sequence of the panel drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] drm/panel: st7703: Make the sleep exit timing match the spec 2020-07-16 12:37 ` Ondrej Jirman @ 2020-07-16 12:37 ` Ondrej Jirman -1 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, Samuel Holland, dri-devel, linux-kernel The driver seemed to try to make the total cumulative time of all delays across the whole power up sequence to sum up to 120ms. The datasheet on the other hand specifies that there needs to be 120ms delay after the sleep out command, not after reset as the driver assumes. Lastly, the delay between init commands and the sleep exit command is not necessary. (not specified anywhere) Reported-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Ondrej Jirman <megous@megous.com> --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index 8996ced2b721..45833e6a0f4f 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -291,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx) dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP, 0x07, /* VREF_SEL = 4.2V */ 0x07 /* NVREF_SEL = 4.2V */); - msleep(20); dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM, 0x2C, /* VCOMDC_F = -0.67V */ @@ -369,16 +368,14 @@ static int st7703_enable(struct drm_panel *panel) return ret; } - msleep(20); - ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret < 0) { DRM_DEV_ERROR(ctx->dev, "Failed to exit sleep mode: %d\n", ret); return ret; } - /* Panel is operational 120 msec after reset */ - msleep(60); + /* Dsiplay on can be issued 120 msec after sleep out */ + msleep(120); ret = mipi_dsi_dcs_set_display_on(dsi); if (ret) -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/2] drm/panel: st7703: Make the sleep exit timing match the spec @ 2020-07-16 12:37 ` Ondrej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, linux-kernel, dri-devel, Samuel Holland The driver seemed to try to make the total cumulative time of all delays across the whole power up sequence to sum up to 120ms. The datasheet on the other hand specifies that there needs to be 120ms delay after the sleep out command, not after reset as the driver assumes. Lastly, the delay between init commands and the sleep exit command is not necessary. (not specified anywhere) Reported-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Ondrej Jirman <megous@megous.com> --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index 8996ced2b721..45833e6a0f4f 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -291,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx) dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP, 0x07, /* VREF_SEL = 4.2V */ 0x07 /* NVREF_SEL = 4.2V */); - msleep(20); dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM, 0x2C, /* VCOMDC_F = -0.67V */ @@ -369,16 +368,14 @@ static int st7703_enable(struct drm_panel *panel) return ret; } - msleep(20); - ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret < 0) { DRM_DEV_ERROR(ctx->dev, "Failed to exit sleep mode: %d\n", ret); return ret; } - /* Panel is operational 120 msec after reset */ - msleep(60); + /* Dsiplay on can be issued 120 msec after sleep out */ + msleep(120); ret = mipi_dsi_dcs_set_display_on(dsi); if (ret) -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] drm/panel: st7703: Fix the power up sequence of the panel 2020-07-16 12:37 ` Ondrej Jirman @ 2020-07-16 12:37 ` Ondrej Jirman -1 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, Samuel Holland, dri-devel, linux-kernel The datasheet specifies that it's better to keep reset asserted while powering up the supplies, and that IOVCC should be enabled first. There also needs to be a delay after enabling the supplies and before deasserting the reset. The datasheet specifies 1ms after the supplies reach the required voltage. Use 10-20ms to also give the power supplies some time to reach the required voltage, too. This fixes intermittent panel initialization failures and screen corruption during resume from sleep on PinePhone. Reported-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Ondrej Jirman <megous@megous.com> --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index 45833e6a0f4f..48569a8688f6 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -429,12 +429,8 @@ static int st7703_prepare(struct drm_panel *panel) return 0; DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n"); - ret = regulator_enable(ctx->vcc); - if (ret < 0) { - DRM_DEV_ERROR(ctx->dev, - "Failed to enable vcc supply: %d\n", ret); - return ret; - } + gpiod_set_value_cansleep(ctx->reset_gpio, 1); + ret = regulator_enable(ctx->iovcc); if (ret < 0) { DRM_DEV_ERROR(ctx->dev, @@ -442,10 +438,18 @@ static int st7703_prepare(struct drm_panel *panel) goto disable_vcc; } - gpiod_set_value_cansleep(ctx->reset_gpio, 1); - usleep_range(20, 40); + ret = regulator_enable(ctx->vcc); + if (ret < 0) { + DRM_DEV_ERROR(ctx->dev, + "Failed to enable vcc supply: %d\n", ret); + return ret; + } + + /* Give power supplies time to stabilize before deasserting reset. */ + usleep_range(10000, 20000); + gpiod_set_value_cansleep(ctx->reset_gpio, 0); - msleep(20); + usleep_range(15000, 20000); ctx->prepared = true; -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] drm/panel: st7703: Fix the power up sequence of the panel @ 2020-07-16 12:37 ` Ondrej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondrej Jirman @ 2020-07-16 12:37 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Guido Günther, Robert Chiras Cc: Ondrej Jirman, linux-kernel, dri-devel, Samuel Holland The datasheet specifies that it's better to keep reset asserted while powering up the supplies, and that IOVCC should be enabled first. There also needs to be a delay after enabling the supplies and before deasserting the reset. The datasheet specifies 1ms after the supplies reach the required voltage. Use 10-20ms to also give the power supplies some time to reach the required voltage, too. This fixes intermittent panel initialization failures and screen corruption during resume from sleep on PinePhone. Reported-by: Samuel Holland <samuel@sholland.org> Signed-off-by: Ondrej Jirman <megous@megous.com> --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index 45833e6a0f4f..48569a8688f6 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -429,12 +429,8 @@ static int st7703_prepare(struct drm_panel *panel) return 0; DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n"); - ret = regulator_enable(ctx->vcc); - if (ret < 0) { - DRM_DEV_ERROR(ctx->dev, - "Failed to enable vcc supply: %d\n", ret); - return ret; - } + gpiod_set_value_cansleep(ctx->reset_gpio, 1); + ret = regulator_enable(ctx->iovcc); if (ret < 0) { DRM_DEV_ERROR(ctx->dev, @@ -442,10 +438,18 @@ static int st7703_prepare(struct drm_panel *panel) goto disable_vcc; } - gpiod_set_value_cansleep(ctx->reset_gpio, 1); - usleep_range(20, 40); + ret = regulator_enable(ctx->vcc); + if (ret < 0) { + DRM_DEV_ERROR(ctx->dev, + "Failed to enable vcc supply: %d\n", ret); + return ret; + } + + /* Give power supplies time to stabilize before deasserting reset. */ + usleep_range(10000, 20000); + gpiod_set_value_cansleep(ctx->reset_gpio, 0); - msleep(20); + usleep_range(15000, 20000); ctx->prepared = true; -- 2.27.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-16 12:37 ` Ondrej Jirman @ 2020-07-16 14:08 ` Guido Günther -1 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-16 14:08 UTC (permalink / raw) To: Ondrej Jirman Cc: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi Ondrej, On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > When extending the driver for xbd599 panel support I tried to do minimal > changes and keep the existing initialization timing. > > It turned out that it's not good enough and the existing init sequence > is too aggressive and doesn't follow the specification. On PinePhone > panel is being powered down/up during suspend/resume and with current > timings this frequently leads to corrupted display. Given the amount of ST7703 look alikes i don't think you can go by the datasheet and hope not to break other panels. The current sleeps cater for the rocktech panel (which suffered from similar issues you describe when we took other parameters) so you need to make those panel specific. Cheers, -- Guido > > This patch series fixes the problems. > > The issue was reported by Samuel Holland. > > Relevant screenshots from the datasheet: > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > Please take a look. > > thank you and regards, > Ondrej Jirman > > Ondrej Jirman (2): > drm/panel: st7703: Make the sleep exit timing match the spec > drm/panel: st7703: Fix the power up sequence of the panel > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-16 14:08 ` Guido Günther 0 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-16 14:08 UTC (permalink / raw) To: Ondrej Jirman Cc: Samuel Holland, David Airlie, Sam Ravnborg, linux-kernel, dri-devel, Thierry Reding, Robert Chiras Hi Ondrej, On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > When extending the driver for xbd599 panel support I tried to do minimal > changes and keep the existing initialization timing. > > It turned out that it's not good enough and the existing init sequence > is too aggressive and doesn't follow the specification. On PinePhone > panel is being powered down/up during suspend/resume and with current > timings this frequently leads to corrupted display. Given the amount of ST7703 look alikes i don't think you can go by the datasheet and hope not to break other panels. The current sleeps cater for the rocktech panel (which suffered from similar issues you describe when we took other parameters) so you need to make those panel specific. Cheers, -- Guido > > This patch series fixes the problems. > > The issue was reported by Samuel Holland. > > Relevant screenshots from the datasheet: > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > Please take a look. > > thank you and regards, > Ondrej Jirman > > Ondrej Jirman (2): > drm/panel: st7703: Make the sleep exit timing match the spec > drm/panel: st7703: Fix the power up sequence of the panel > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > -- > 2.27.0 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-16 14:08 ` Guido Günther @ 2020-07-16 14:32 ` Ondřej Jirman -1 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-16 14:32 UTC (permalink / raw) To: Guido Günther Cc: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi Guido, On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > Hi Ondrej, > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > When extending the driver for xbd599 panel support I tried to do minimal > > changes and keep the existing initialization timing. > > > > It turned out that it's not good enough and the existing init sequence > > is too aggressive and doesn't follow the specification. On PinePhone > > panel is being powered down/up during suspend/resume and with current > > timings this frequently leads to corrupted display. > > Given the amount of ST7703 look alikes i don't think you can go by the > datasheet and hope not to break other panels. The current sleeps cater > for the rocktech panel (which suffered from similar issues you describe > when we took other parameters) so you need to make those panel specific. It should work on rocktech too. The patch mostly increases/reorders the delays slightly, to match the controller documentation. I don't see a reason to complicate the driver with per panel special delays, unless these patches don't work on your panel. The init sequence is still suboptimal, and doesn't follow the kernel docs completely, even after these patches. Controller spec also talks about adding some delay before enabling the backlight to avoid visual glitches. Which is what enable callback is documented to be for. Currently part of the initialization that belongs to prepare callback is also done in enable callback. I see the glitch (small vertical shift of the image on powerup), but personally don't care much to introduce even more delays to the driver, just for the cosmetic issue. regards, o. > Cheers, > -- Guido > > > > > This patch series fixes the problems. > > > > The issue was reported by Samuel Holland. > > > > Relevant screenshots from the datasheet: > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > Please take a look. > > > > thank you and regards, > > Ondrej Jirman > > > > Ondrej Jirman (2): > > drm/panel: st7703: Make the sleep exit timing match the spec > > drm/panel: st7703: Fix the power up sequence of the panel > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > -- > > 2.27.0 > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-16 14:32 ` Ondřej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-16 14:32 UTC (permalink / raw) To: Guido Günther Cc: Samuel Holland, David Airlie, Sam Ravnborg, linux-kernel, dri-devel, Thierry Reding, Robert Chiras Hi Guido, On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > Hi Ondrej, > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > When extending the driver for xbd599 panel support I tried to do minimal > > changes and keep the existing initialization timing. > > > > It turned out that it's not good enough and the existing init sequence > > is too aggressive and doesn't follow the specification. On PinePhone > > panel is being powered down/up during suspend/resume and with current > > timings this frequently leads to corrupted display. > > Given the amount of ST7703 look alikes i don't think you can go by the > datasheet and hope not to break other panels. The current sleeps cater > for the rocktech panel (which suffered from similar issues you describe > when we took other parameters) so you need to make those panel specific. It should work on rocktech too. The patch mostly increases/reorders the delays slightly, to match the controller documentation. I don't see a reason to complicate the driver with per panel special delays, unless these patches don't work on your panel. The init sequence is still suboptimal, and doesn't follow the kernel docs completely, even after these patches. Controller spec also talks about adding some delay before enabling the backlight to avoid visual glitches. Which is what enable callback is documented to be for. Currently part of the initialization that belongs to prepare callback is also done in enable callback. I see the glitch (small vertical shift of the image on powerup), but personally don't care much to introduce even more delays to the driver, just for the cosmetic issue. regards, o. > Cheers, > -- Guido > > > > > This patch series fixes the problems. > > > > The issue was reported by Samuel Holland. > > > > Relevant screenshots from the datasheet: > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > Please take a look. > > > > thank you and regards, > > Ondrej Jirman > > > > Ondrej Jirman (2): > > drm/panel: st7703: Make the sleep exit timing match the spec > > drm/panel: st7703: Fix the power up sequence of the panel > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > -- > > 2.27.0 > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-16 14:32 ` Ondřej Jirman @ 2020-07-18 17:31 ` Guido Günther -1 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-18 17:31 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi, On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > Hi Guido, > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > Hi Ondrej, > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > When extending the driver for xbd599 panel support I tried to do minimal > > > changes and keep the existing initialization timing. > > > > > > It turned out that it's not good enough and the existing init sequence > > > is too aggressive and doesn't follow the specification. On PinePhone > > > panel is being powered down/up during suspend/resume and with current > > > timings this frequently leads to corrupted display. > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > datasheet and hope not to break other panels. The current sleeps cater > > for the rocktech panel (which suffered from similar issues you describe > > when we took other parameters) so you need to make those panel specific. > > It should work on rocktech too. The patch mostly increases/reorders the delays > slightly, to match the controller documentation. I don't see a reason to > complicate the driver with per panel special delays, unless these patches don't > work on your panel. That's why i brought it up. It breaks the rocktech panel on blank/unblank loops where it just stays blank and then starts hitting DSI command timeouts. Cheers, -- Guido > > The init sequence is still suboptimal, and doesn't follow the kernel docs > completely, even after these patches. Controller spec also talks about adding > some delay before enabling the backlight to avoid visual glitches. > > Which is what enable callback is documented to be for. Currently part of the > initialization that belongs to prepare callback is also done in enable callback. > > I see the glitch (small vertical shift of the image on powerup), but personally > don't care much to introduce even more delays to the driver, just for the > cosmetic issue. > > regards, > o. > > > Cheers, > > -- Guido > > > > > > > > This patch series fixes the problems. > > > > > > The issue was reported by Samuel Holland. > > > > > > Relevant screenshots from the datasheet: > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > Please take a look. > > > > > > thank you and regards, > > > Ondrej Jirman > > > > > > Ondrej Jirman (2): > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > -- > > > 2.27.0 > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-18 17:31 ` Guido Günther 0 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-18 17:31 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi, On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > Hi Guido, > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > Hi Ondrej, > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > When extending the driver for xbd599 panel support I tried to do minimal > > > changes and keep the existing initialization timing. > > > > > > It turned out that it's not good enough and the existing init sequence > > > is too aggressive and doesn't follow the specification. On PinePhone > > > panel is being powered down/up during suspend/resume and with current > > > timings this frequently leads to corrupted display. > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > datasheet and hope not to break other panels. The current sleeps cater > > for the rocktech panel (which suffered from similar issues you describe > > when we took other parameters) so you need to make those panel specific. > > It should work on rocktech too. The patch mostly increases/reorders the delays > slightly, to match the controller documentation. I don't see a reason to > complicate the driver with per panel special delays, unless these patches don't > work on your panel. That's why i brought it up. It breaks the rocktech panel on blank/unblank loops where it just stays blank and then starts hitting DSI command timeouts. Cheers, -- Guido > > The init sequence is still suboptimal, and doesn't follow the kernel docs > completely, even after these patches. Controller spec also talks about adding > some delay before enabling the backlight to avoid visual glitches. > > Which is what enable callback is documented to be for. Currently part of the > initialization that belongs to prepare callback is also done in enable callback. > > I see the glitch (small vertical shift of the image on powerup), but personally > don't care much to introduce even more delays to the driver, just for the > cosmetic issue. > > regards, > o. > > > Cheers, > > -- Guido > > > > > > > > This patch series fixes the problems. > > > > > > The issue was reported by Samuel Holland. > > > > > > Relevant screenshots from the datasheet: > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > Please take a look. > > > > > > thank you and regards, > > > Ondrej Jirman > > > > > > Ondrej Jirman (2): > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > -- > > > 2.27.0 > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-18 17:31 ` Guido Günther @ 2020-07-18 17:42 ` Ondřej Jirman -1 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-18 17:42 UTC (permalink / raw) To: Guido Günther Cc: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hello, On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > Hi, > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > Hi Guido, > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > Hi Ondrej, > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > changes and keep the existing initialization timing. > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > panel is being powered down/up during suspend/resume and with current > > > > timings this frequently leads to corrupted display. > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > datasheet and hope not to break other panels. The current sleeps cater > > > for the rocktech panel (which suffered from similar issues you describe > > > when we took other parameters) so you need to make those panel specific. > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > slightly, to match the controller documentation. I don't see a reason to > > complicate the driver with per panel special delays, unless these patches don't > > work on your panel. > > That's why i brought it up. It breaks the rocktech panel on > blank/unblank loops where it just stays blank and then starts hitting > DSI command timeouts. Good to know. Does keeping the msleep(20); after init sequence and before sleep exit make it work? thank you and regards, o. > Cheers, > -- Guido > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > completely, even after these patches. Controller spec also talks about adding > > some delay before enabling the backlight to avoid visual glitches. > > > > Which is what enable callback is documented to be for. Currently part of the > > initialization that belongs to prepare callback is also done in enable callback. > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > don't care much to introduce even more delays to the driver, just for the > > cosmetic issue. > > > > regards, > > o. > > > > > Cheers, > > > -- Guido > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > Please take a look. > > > > > > > > thank you and regards, > > > > Ondrej Jirman > > > > > > > > Ondrej Jirman (2): > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > -- > > > > 2.27.0 > > > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-18 17:42 ` Ondřej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-18 17:42 UTC (permalink / raw) To: Guido Günther Cc: Samuel Holland, David Airlie, Sam Ravnborg, linux-kernel, dri-devel, Thierry Reding, Robert Chiras Hello, On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > Hi, > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > Hi Guido, > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > Hi Ondrej, > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > changes and keep the existing initialization timing. > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > panel is being powered down/up during suspend/resume and with current > > > > timings this frequently leads to corrupted display. > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > datasheet and hope not to break other panels. The current sleeps cater > > > for the rocktech panel (which suffered from similar issues you describe > > > when we took other parameters) so you need to make those panel specific. > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > slightly, to match the controller documentation. I don't see a reason to > > complicate the driver with per panel special delays, unless these patches don't > > work on your panel. > > That's why i brought it up. It breaks the rocktech panel on > blank/unblank loops where it just stays blank and then starts hitting > DSI command timeouts. Good to know. Does keeping the msleep(20); after init sequence and before sleep exit make it work? thank you and regards, o. > Cheers, > -- Guido > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > completely, even after these patches. Controller spec also talks about adding > > some delay before enabling the backlight to avoid visual glitches. > > > > Which is what enable callback is documented to be for. Currently part of the > > initialization that belongs to prepare callback is also done in enable callback. > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > don't care much to introduce even more delays to the driver, just for the > > cosmetic issue. > > > > regards, > > o. > > > > > Cheers, > > > -- Guido > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > Please take a look. > > > > > > > > thank you and regards, > > > > Ondrej Jirman > > > > > > > > Ondrej Jirman (2): > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > -- > > > > 2.27.0 > > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-18 17:42 ` Ondřej Jirman @ 2020-07-29 15:48 ` Guido Günther -1 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-29 15:48 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi, On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > Hello, > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > Hi, > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > Hi Guido, > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > Hi Ondrej, > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > changes and keep the existing initialization timing. > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > panel is being powered down/up during suspend/resume and with current > > > > > timings this frequently leads to corrupted display. > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > for the rocktech panel (which suffered from similar issues you describe > > > > when we took other parameters) so you need to make those panel specific. > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > slightly, to match the controller documentation. I don't see a reason to > > > complicate the driver with per panel special delays, unless these patches don't > > > work on your panel. > > > > That's why i brought it up. It breaks the rocktech panel on > > blank/unblank loops where it just stays blank and then starts hitting > > DSI command timeouts. > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > exit make it work? We need both sleeps to make this work reliably so basically reverting your 'drm/panel: st7703: Make the sleep exit timing match the spec' makes things stable again. We don't need to sleep 120ms after sleep out though since our panel only requires 15ms as per data sheet there so it really makes sense to make these configurable. Cheers, -- Guido > > thank you and regards, > o. > > > Cheers, > > -- Guido > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > completely, even after these patches. Controller spec also talks about adding > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > don't care much to introduce even more delays to the driver, just for the > > > cosmetic issue. > > > > > > regards, > > > o. > > > > > > > Cheers, > > > > -- Guido > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > Please take a look. > > > > > > > > > > thank you and regards, > > > > > Ondrej Jirman > > > > > > > > > > Ondrej Jirman (2): > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > -- > > > > > 2.27.0 > > > > > > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-29 15:48 ` Guido Günther 0 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-07-29 15:48 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi, On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > Hello, > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > Hi, > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > Hi Guido, > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > Hi Ondrej, > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > changes and keep the existing initialization timing. > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > panel is being powered down/up during suspend/resume and with current > > > > > timings this frequently leads to corrupted display. > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > for the rocktech panel (which suffered from similar issues you describe > > > > when we took other parameters) so you need to make those panel specific. > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > slightly, to match the controller documentation. I don't see a reason to > > > complicate the driver with per panel special delays, unless these patches don't > > > work on your panel. > > > > That's why i brought it up. It breaks the rocktech panel on > > blank/unblank loops where it just stays blank and then starts hitting > > DSI command timeouts. > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > exit make it work? We need both sleeps to make this work reliably so basically reverting your 'drm/panel: st7703: Make the sleep exit timing match the spec' makes things stable again. We don't need to sleep 120ms after sleep out though since our panel only requires 15ms as per data sheet there so it really makes sense to make these configurable. Cheers, -- Guido > > thank you and regards, > o. > > > Cheers, > > -- Guido > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > completely, even after these patches. Controller spec also talks about adding > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > don't care much to introduce even more delays to the driver, just for the > > > cosmetic issue. > > > > > > regards, > > > o. > > > > > > > Cheers, > > > > -- Guido > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > Please take a look. > > > > > > > > > > thank you and regards, > > > > > Ondrej Jirman > > > > > > > > > > Ondrej Jirman (2): > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > -- > > > > > 2.27.0 > > > > > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-29 15:48 ` Guido Günther @ 2020-07-30 13:41 ` Ondřej Jirman -1 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-30 13:41 UTC (permalink / raw) To: Guido Günther Cc: David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hello, On Wed, Jul 29, 2020 at 05:48:09PM +0200, Guido Günther wrote: > Hi, > On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > > Hello, > > > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > > Hi, > > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > > Hi Guido, > > > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > > Hi Ondrej, > > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > > changes and keep the existing initialization timing. > > > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > > panel is being powered down/up during suspend/resume and with current > > > > > > timings this frequently leads to corrupted display. > > > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > > for the rocktech panel (which suffered from similar issues you describe > > > > > when we took other parameters) so you need to make those panel specific. > > > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > > slightly, to match the controller documentation. I don't see a reason to > > > > complicate the driver with per panel special delays, unless these patches don't > > > > work on your panel. > > > > > > That's why i brought it up. It breaks the rocktech panel on > > > blank/unblank loops where it just stays blank and then starts hitting > > > DSI command timeouts. > > > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > > exit make it work? > > We need both sleeps to make this work reliably so basically > reverting your 'drm/panel: st7703: Make the sleep exit timing match the > spec' makes things stable again. > > We don't need to sleep 120ms after sleep out though since our panel only > requires 15ms as per data sheet there so it really makes sense to make > these configurable. Thank you for checking it. I'd be happy with just the other patch being applied. That would be enough to fix issues with xingbangda panel too. In my tests xbd panel doesn't need 120ms either, despite the datasheet. What breaks xbd panel is the lack of post-powerup delay before deasserting reset line. regards, o. > Cheers, > -- Guido > > > > > thank you and regards, > > o. > > > > > Cheers, > > > -- Guido > > > > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > > completely, even after these patches. Controller spec also talks about adding > > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > > don't care much to introduce even more delays to the driver, just for the > > > > cosmetic issue. > > > > > > > > regards, > > > > o. > > > > > > > > > Cheers, > > > > > -- Guido > > > > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > > > Please take a look. > > > > > > > > > > > > thank you and regards, > > > > > > Ondrej Jirman > > > > > > > > > > > > Ondrej Jirman (2): > > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > > > -- > > > > > > 2.27.0 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-07-30 13:41 ` Ondřej Jirman 0 siblings, 0 replies; 20+ messages in thread From: Ondřej Jirman @ 2020-07-30 13:41 UTC (permalink / raw) To: Guido Günther Cc: Samuel Holland, David Airlie, Sam Ravnborg, linux-kernel, dri-devel, Thierry Reding, Robert Chiras Hello, On Wed, Jul 29, 2020 at 05:48:09PM +0200, Guido Günther wrote: > Hi, > On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > > Hello, > > > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > > Hi, > > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > > Hi Guido, > > > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > > Hi Ondrej, > > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > > changes and keep the existing initialization timing. > > > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > > panel is being powered down/up during suspend/resume and with current > > > > > > timings this frequently leads to corrupted display. > > > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > > for the rocktech panel (which suffered from similar issues you describe > > > > > when we took other parameters) so you need to make those panel specific. > > > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > > slightly, to match the controller documentation. I don't see a reason to > > > > complicate the driver with per panel special delays, unless these patches don't > > > > work on your panel. > > > > > > That's why i brought it up. It breaks the rocktech panel on > > > blank/unblank loops where it just stays blank and then starts hitting > > > DSI command timeouts. > > > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > > exit make it work? > > We need both sleeps to make this work reliably so basically > reverting your 'drm/panel: st7703: Make the sleep exit timing match the > spec' makes things stable again. > > We don't need to sleep 120ms after sleep out though since our panel only > requires 15ms as per data sheet there so it really makes sense to make > these configurable. Thank you for checking it. I'd be happy with just the other patch being applied. That would be enough to fix issues with xingbangda panel too. In my tests xbd panel doesn't need 120ms either, despite the datasheet. What breaks xbd panel is the lack of post-powerup delay before deasserting reset line. regards, o. > Cheers, > -- Guido > > > > > thank you and regards, > > o. > > > > > Cheers, > > > -- Guido > > > > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > > completely, even after these patches. Controller spec also talks about adding > > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > > don't care much to introduce even more delays to the driver, just for the > > > > cosmetic issue. > > > > > > > > regards, > > > > o. > > > > > > > > > Cheers, > > > > > -- Guido > > > > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > > > Please take a look. > > > > > > > > > > > > thank you and regards, > > > > > > Ondrej Jirman > > > > > > > > > > > > Ondrej Jirman (2): > > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > > > -- > > > > > > 2.27.0 > > > > > > > > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures 2020-07-30 13:41 ` Ondřej Jirman @ 2020-08-01 11:04 ` Guido Günther -1 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-08-01 11:04 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi Ondřej, On Thu, Jul 30, 2020 at 03:41:11PM +0200, Ondřej Jirman wrote: > Hello, > > On Wed, Jul 29, 2020 at 05:48:09PM +0200, Guido Günther wrote: > > Hi, > > On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > > > Hello, > > > > > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > > > Hi, > > > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > > > Hi Guido, > > > > > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > > > Hi Ondrej, > > > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > > > changes and keep the existing initialization timing. > > > > > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > > > panel is being powered down/up during suspend/resume and with current > > > > > > > timings this frequently leads to corrupted display. > > > > > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > > > for the rocktech panel (which suffered from similar issues you describe > > > > > > when we took other parameters) so you need to make those panel specific. > > > > > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > > > slightly, to match the controller documentation. I don't see a reason to > > > > > complicate the driver with per panel special delays, unless these patches don't > > > > > work on your panel. > > > > > > > > That's why i brought it up. It breaks the rocktech panel on > > > > blank/unblank loops where it just stays blank and then starts hitting > > > > DSI command timeouts. > > > > > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > > > exit make it work? > > > > We need both sleeps to make this work reliably so basically > > reverting your 'drm/panel: st7703: Make the sleep exit timing match the > > spec' makes things stable again. > > > > We don't need to sleep 120ms after sleep out though since our panel only > > requires 15ms as per data sheet there so it really makes sense to make > > these configurable. > > Thank you for checking it. > > I'd be happy with just the other patch being applied. That would be enough > to fix issues with xingbangda panel too. That's fine since that case works here too. The commit message should be adjusted though since the ST7703 data sheet i'm looking at says there's no restriction about applying IOVCC and VCI (tRPW= +/- no limit). You're basically also switching to another reset pattern (the data sheet describes two). So instead of saying: "Fix the power up sequence of the panel" i'd rather say "Pick a reset sequence that works for ingbangda,xbd599 panel too" and then describe the changes since that helps us to identify later on why we made that change. With that and 1/2 dropped that looks good to me. Cheers, -- Guido > > In my tests xbd panel doesn't need 120ms either, despite the datasheet. > What breaks xbd panel is the lack of post-powerup delay before deasserting > reset line. > > regards, > o. > > > Cheers, > > -- Guido > > > > > > > > thank you and regards, > > > o. > > > > > > > Cheers, > > > > -- Guido > > > > > > > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > > > completely, even after these patches. Controller spec also talks about adding > > > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > > > don't care much to introduce even more delays to the driver, just for the > > > > > cosmetic issue. > > > > > > > > > > regards, > > > > > o. > > > > > > > > > > > Cheers, > > > > > > -- Guido > > > > > > > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > > > > > Please take a look. > > > > > > > > > > > > > > thank you and regards, > > > > > > > Ondrej Jirman > > > > > > > > > > > > > > Ondrej Jirman (2): > > > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > > > > > -- > > > > > > > 2.27.0 > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] Fix st7703 panel initialization failures @ 2020-08-01 11:04 ` Guido Günther 0 siblings, 0 replies; 20+ messages in thread From: Guido Günther @ 2020-08-01 11:04 UTC (permalink / raw) To: Ondřej Jirman, David Airlie, Daniel Vetter, Thierry Reding, Sam Ravnborg, Fabio Estevam, Robert Chiras, Samuel Holland, dri-devel, linux-kernel Hi Ondřej, On Thu, Jul 30, 2020 at 03:41:11PM +0200, Ondřej Jirman wrote: > Hello, > > On Wed, Jul 29, 2020 at 05:48:09PM +0200, Guido Günther wrote: > > Hi, > > On Sat, Jul 18, 2020 at 07:42:15PM +0200, Ondřej Jirman wrote: > > > Hello, > > > > > > On Sat, Jul 18, 2020 at 07:31:24PM +0200, Guido Günther wrote: > > > > Hi, > > > > On Thu, Jul 16, 2020 at 04:32:09PM +0200, Ondřej Jirman wrote: > > > > > Hi Guido, > > > > > > > > > > On Thu, Jul 16, 2020 at 04:08:43PM +0200, Guido Günther wrote: > > > > > > Hi Ondrej, > > > > > > On Thu, Jul 16, 2020 at 02:37:51PM +0200, Ondrej Jirman wrote: > > > > > > > When extending the driver for xbd599 panel support I tried to do minimal > > > > > > > changes and keep the existing initialization timing. > > > > > > > > > > > > > > It turned out that it's not good enough and the existing init sequence > > > > > > > is too aggressive and doesn't follow the specification. On PinePhone > > > > > > > panel is being powered down/up during suspend/resume and with current > > > > > > > timings this frequently leads to corrupted display. > > > > > > > > > > > > Given the amount of ST7703 look alikes i don't think you can go by the > > > > > > datasheet and hope not to break other panels. The current sleeps cater > > > > > > for the rocktech panel (which suffered from similar issues you describe > > > > > > when we took other parameters) so you need to make those panel specific. > > > > > > > > > > It should work on rocktech too. The patch mostly increases/reorders the delays > > > > > slightly, to match the controller documentation. I don't see a reason to > > > > > complicate the driver with per panel special delays, unless these patches don't > > > > > work on your panel. > > > > > > > > That's why i brought it up. It breaks the rocktech panel on > > > > blank/unblank loops where it just stays blank and then starts hitting > > > > DSI command timeouts. > > > > > > Good to know. Does keeping the msleep(20); after init sequence and before sleep > > > exit make it work? > > > > We need both sleeps to make this work reliably so basically > > reverting your 'drm/panel: st7703: Make the sleep exit timing match the > > spec' makes things stable again. > > > > We don't need to sleep 120ms after sleep out though since our panel only > > requires 15ms as per data sheet there so it really makes sense to make > > these configurable. > > Thank you for checking it. > > I'd be happy with just the other patch being applied. That would be enough > to fix issues with xingbangda panel too. That's fine since that case works here too. The commit message should be adjusted though since the ST7703 data sheet i'm looking at says there's no restriction about applying IOVCC and VCI (tRPW= +/- no limit). You're basically also switching to another reset pattern (the data sheet describes two). So instead of saying: "Fix the power up sequence of the panel" i'd rather say "Pick a reset sequence that works for ingbangda,xbd599 panel too" and then describe the changes since that helps us to identify later on why we made that change. With that and 1/2 dropped that looks good to me. Cheers, -- Guido > > In my tests xbd panel doesn't need 120ms either, despite the datasheet. > What breaks xbd panel is the lack of post-powerup delay before deasserting > reset line. > > regards, > o. > > > Cheers, > > -- Guido > > > > > > > > thank you and regards, > > > o. > > > > > > > Cheers, > > > > -- Guido > > > > > > > > > > > > > > The init sequence is still suboptimal, and doesn't follow the kernel docs > > > > > completely, even after these patches. Controller spec also talks about adding > > > > > some delay before enabling the backlight to avoid visual glitches. > > > > > > > > > > Which is what enable callback is documented to be for. Currently part of the > > > > > initialization that belongs to prepare callback is also done in enable callback. > > > > > > > > > > I see the glitch (small vertical shift of the image on powerup), but personally > > > > > don't care much to introduce even more delays to the driver, just for the > > > > > cosmetic issue. > > > > > > > > > > regards, > > > > > o. > > > > > > > > > > > Cheers, > > > > > > -- Guido > > > > > > > > > > > > > > > > > > > > This patch series fixes the problems. > > > > > > > > > > > > > > The issue was reported by Samuel Holland. > > > > > > > > > > > > > > Relevant screenshots from the datasheet: > > > > > > > > > > > > > > Power on timing: https://megous.com/dl/tmp/35b72e674ce0ca27.png > > > > > > > Power off timing: https://megous.com/dl/tmp/dea195517106ff17.png > > > > > > > More optimal reset on poweron: https://megous.com/dl/tmp/a9e5caf14e1b0dc6.png > > > > > > > Less optimal reset on poweron: https://megous.com/dl/tmp/246761039283c4cf.png > > > > > > > Datasheet: https://megous.com/dl/tmp/ST7703_DS_v01_20160128.pdf > > > > > > > > > > > > > > Please take a look. > > > > > > > > > > > > > > thank you and regards, > > > > > > > Ondrej Jirman > > > > > > > > > > > > > > Ondrej Jirman (2): > > > > > > > drm/panel: st7703: Make the sleep exit timing match the spec > > > > > > > drm/panel: st7703: Fix the power up sequence of the panel > > > > > > > > > > > > > > drivers/gpu/drm/panel/panel-sitronix-st7703.c | 29 ++++++++++--------- > > > > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > > > > > > > -- > > > > > > > 2.27.0 > > > > > > > > > > > > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-08-01 11:04 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-16 12:37 [PATCH 0/2] Fix st7703 panel initialization failures Ondrej Jirman 2020-07-16 12:37 ` Ondrej Jirman 2020-07-16 12:37 ` [PATCH 1/2] drm/panel: st7703: Make the sleep exit timing match the spec Ondrej Jirman 2020-07-16 12:37 ` Ondrej Jirman 2020-07-16 12:37 ` [PATCH 2/2] drm/panel: st7703: Fix the power up sequence of the panel Ondrej Jirman 2020-07-16 12:37 ` Ondrej Jirman 2020-07-16 14:08 ` [PATCH 0/2] Fix st7703 panel initialization failures Guido Günther 2020-07-16 14:08 ` Guido Günther 2020-07-16 14:32 ` Ondřej Jirman 2020-07-16 14:32 ` Ondřej Jirman 2020-07-18 17:31 ` Guido Günther 2020-07-18 17:31 ` Guido Günther 2020-07-18 17:42 ` Ondřej Jirman 2020-07-18 17:42 ` Ondřej Jirman 2020-07-29 15:48 ` Guido Günther 2020-07-29 15:48 ` Guido Günther 2020-07-30 13:41 ` Ondřej Jirman 2020-07-30 13:41 ` Ondřej Jirman 2020-08-01 11:04 ` Guido Günther 2020-08-01 11:04 ` Guido Günther
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.
