This patch fixes flickering after resume from sleep on panel
xingbangda,xbd599 (e.g. used in PinePhone).
It was originally submitted by Ondrej Jirman in July 2020:
https://lore.kernel.org/all/[email protected]/
The original patchset contained two patches. This submission fixes the
patch that broke handling of the JH057N panel of the Purism Librem 5.
In essence, it does not change any behavior towards the JH057N panel,
but only affects the XDB599.
The patch is just a refactoring of Ondřej's original patch, that is
already used today by PinePhone distributions like PostmarketOS.
I was torn between using function pointers and just calling msleep()
with device specific delays. I decided to go with function pointers,
because my understanding is that calling msleep(0), which would be
required for waiting for the JH057N display to discharge, still results
in a delay. The empty function I used has no side effect on that panel.
The patch is based on drm-next.
Ondřej, since this is just a refactoring, I would gladly add your SoB,
if you wish so.
Thanks,
Frank
Frank Oltmanns (1):
drm/panel: st7703: Fix resume of XBD599 panel
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)
--
2.39.1
In contrast to the JH057N panel, the XBD599 panel does not require a 20
msec delay after initialization and exiting sleep mode. Therefore, move
the delay into the already existing device specific initialization
function.
Also, the timing contraints after entering and exiting sleep mode differ
between the two panels:
- The JH057N requires a shorter delay than the XDB599 after waking up
from sleep mode and before enabling the display.
- The XDB599 requires a delay in order to drain the display of charge,
which is not required on the JH057N.
Therefore, introduce panel specific functions for the delays.
The XDB599 does not require a 20 msec delay between the SETBGP and
SETVCOM commands. Therefore, remove the delay from the device specific
initialization function.
Signed-off-by: Frank Oltmanns <[email protected]>
Cc: Ondrej Jirman <[email protected]>
Reported-by: Samuel Holland <[email protected]>
---
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
index 6747ca237ced..a149341c4a8b 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
@@ -66,6 +66,8 @@ struct st7703_panel_desc {
unsigned long mode_flags;
enum mipi_dsi_pixel_format format;
int (*init_sequence)(struct st7703 *ctx);
+ void (*wait_after_sleep_out)(void);
+ void (*drain_charge)(void);
};
static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
@@ -126,10 +128,24 @@ static int jh057n_init_sequence(struct st7703 *ctx)
0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
0x11, 0x18);
+ msleep(20);
return 0;
}
+static void jh057n_wait_after_sleep_out(void)
+{
+ /*
+ * Panel is operational 120 msec after reset, i.e. 60 msec after
+ * sleep out.
+ */
+ msleep(60);
+}
+
+static void jh057n_drain_charge(void)
+{
+}
+
static const struct drm_display_mode jh057n00900_mode = {
.hdisplay = 720,
.hsync_start = 720 + 90,
@@ -152,6 +168,8 @@ static const struct st7703_panel_desc jh057n00900_panel_desc = {
MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
.format = MIPI_DSI_FMT_RGB888,
.init_sequence = jh057n_init_sequence,
+ .wait_after_sleep_out = jh057n_wait_after_sleep_out,
+ .drain_charge = jh057n_drain_charge,
};
static int xbd599_init_sequence(struct st7703 *ctx)
@@ -273,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx)
mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP,
0x07, /* VREF_SEL = 4.2V */
0x07 /* NVREF_SEL = 4.2V */);
- msleep(20);
mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM,
0x2C, /* VCOMDC_F = -0.67V */
@@ -315,6 +332,18 @@ static int xbd599_init_sequence(struct st7703 *ctx)
return 0;
}
+static void xbd599_wait_after_sleep_out(void)
+{
+ msleep(120);
+}
+
+static void xbd599_drain_charge(void)
+{
+ /* Drain diplay of charge, to work correctly on next power on. */
+ msleep(120);
+}
+
+
static const struct drm_display_mode xbd599_mode = {
.hdisplay = 720,
.hsync_start = 720 + 40,
@@ -336,6 +365,8 @@ static const struct st7703_panel_desc xbd599_desc = {
.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
.format = MIPI_DSI_FMT_RGB888,
.init_sequence = xbd599_init_sequence,
+ .wait_after_sleep_out = xbd599_wait_after_sleep_out,
+ .drain_charge = xbd599_drain_charge,
};
static int st7703_enable(struct drm_panel *panel)
@@ -350,16 +381,13 @@ static int st7703_enable(struct drm_panel *panel)
return ret;
}
- msleep(20);
-
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
if (ret < 0) {
dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
return ret;
}
- /* Panel is operational 120 msec after reset */
- msleep(60);
+ ctx->desc->wait_after_sleep_out();
ret = mipi_dsi_dcs_set_display_on(dsi);
if (ret)
@@ -384,6 +412,8 @@ static int st7703_disable(struct drm_panel *panel)
if (ret < 0)
dev_err(ctx->dev, "Failed to enter sleep mode: %d\n", ret);
+ ctx->desc->drain_charge();
+
return 0;
}
--
2.39.1
On Sun, Feb 12, 2023 at 01:08:29PM +0100, Frank Oltmanns wrote:
> In contrast to the JH057N panel, the XBD599 panel does not require a 20
> msec delay after initialization and exiting sleep mode. Therefore, move
> the delay into the already existing device specific initialization
> function.
>
> Also, the timing contraints after entering and exiting sleep mode differ
> between the two panels:
> - The JH057N requires a shorter delay than the XDB599 after waking up
> from sleep mode and before enabling the display.
> - The XDB599 requires a delay in order to drain the display of charge,
> which is not required on the JH057N.
There's no difference between the panels here. It's a controller specified
requirement.
https://megous.com/dl/tmp/1ef533ed8a7ce841.png
60ms used in the driver between sleep out and display on is just
incorrect from the datasheet perspective.
You also have to wait 120ms after sleep in (or HW reset) and before shutting
down the panel. If you don't, after a bunch of cycles of this incorrect
power up/down sequence the panel will start blinking weirdly, and the incorrect
power up/down sequence without delays will not be able to recover it. You'll
have to let the panel sit for 5-10 minutes powered off before it starts to
behave itself again.
The documentation for sleep in specifies what's happening during sleep in,
and why this delay is necessary after sleep in:
https://megous.com/dl/tmp/2284b9d0f506b9b8.png
So there needs to be 120ms delay after sleep in and after sleep out,
regardless of which panel is driven by this controller, to ensure the panel
stays operational even when the user is quickly switching it on/off repeatedly.
So I don't think you should be doing panel specific quirks here.
regards,
o.
> Therefore, introduce panel specific functions for the delays.
>
> The XDB599 does not require a 20 msec delay between the SETBGP and
> SETVCOM commands. Therefore, remove the delay from the device specific
> initialization function.
>
> Signed-off-by: Frank Oltmanns <[email protected]>
> Cc: Ondrej Jirman <[email protected]>
> Reported-by: Samuel Holland <[email protected]>
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++---
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> index 6747ca237ced..a149341c4a8b 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> @@ -66,6 +66,8 @@ struct st7703_panel_desc {
> unsigned long mode_flags;
> enum mipi_dsi_pixel_format format;
> int (*init_sequence)(struct st7703 *ctx);
> + void (*wait_after_sleep_out)(void);
> + void (*drain_charge)(void);
> };
>
> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
> @@ -126,10 +128,24 @@ static int jh057n_init_sequence(struct st7703 *ctx)
> 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> 0x11, 0x18);
> + msleep(20);
>
> return 0;
> }
>
> +static void jh057n_wait_after_sleep_out(void)
> +{
> + /*
> + * Panel is operational 120 msec after reset, i.e. 60 msec after
> + * sleep out.
> + */
> + msleep(60);
> +}
> +
> +static void jh057n_drain_charge(void)
> +{
> +}
> +
> static const struct drm_display_mode jh057n00900_mode = {
> .hdisplay = 720,
> .hsync_start = 720 + 90,
> @@ -152,6 +168,8 @@ static const struct st7703_panel_desc jh057n00900_panel_desc = {
> MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
> .format = MIPI_DSI_FMT_RGB888,
> .init_sequence = jh057n_init_sequence,
> + .wait_after_sleep_out = jh057n_wait_after_sleep_out,
> + .drain_charge = jh057n_drain_charge,
> };
>
> static int xbd599_init_sequence(struct st7703 *ctx)
> @@ -273,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx)
> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP,
> 0x07, /* VREF_SEL = 4.2V */
> 0x07 /* NVREF_SEL = 4.2V */);
> - msleep(20);
>
> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM,
> 0x2C, /* VCOMDC_F = -0.67V */
> @@ -315,6 +332,18 @@ static int xbd599_init_sequence(struct st7703 *ctx)
> return 0;
> }
>
> +static void xbd599_wait_after_sleep_out(void)
> +{
> + msleep(120);
> +}
> +
> +static void xbd599_drain_charge(void)
> +{
> + /* Drain diplay of charge, to work correctly on next power on. */
> + msleep(120);
> +}
> +
> +
> static const struct drm_display_mode xbd599_mode = {
> .hdisplay = 720,
> .hsync_start = 720 + 40,
> @@ -336,6 +365,8 @@ static const struct st7703_panel_desc xbd599_desc = {
> .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
> .format = MIPI_DSI_FMT_RGB888,
> .init_sequence = xbd599_init_sequence,
> + .wait_after_sleep_out = xbd599_wait_after_sleep_out,
> + .drain_charge = xbd599_drain_charge,
> };
>
> static int st7703_enable(struct drm_panel *panel)
> @@ -350,16 +381,13 @@ static int st7703_enable(struct drm_panel *panel)
> return ret;
> }
>
> - msleep(20);
> -
> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> if (ret < 0) {
> dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> return ret;
> }
>
> - /* Panel is operational 120 msec after reset */
> - msleep(60);
> + ctx->desc->wait_after_sleep_out();
>
> ret = mipi_dsi_dcs_set_display_on(dsi);
> if (ret)
> @@ -384,6 +412,8 @@ static int st7703_disable(struct drm_panel *panel)
> if (ret < 0)
> dev_err(ctx->dev, "Failed to enter sleep mode: %d\n", ret);
>
> + ctx->desc->drain_charge();
> +
> return 0;
> }
>
> --
> 2.39.1
>
Hi Ondřej,
hi Guido,
Ondřej, thank you very much for your feedback!
I have a couple of questions.
Ondřej Jirman <[email protected]> writes:
> On Sun, Feb 12, 2023 at 01:08:29PM +0100, Frank Oltmanns wrote:
>> In contrast to the JH057N panel, the XBD599 panel does not require a 20
>> msec delay after initialization and exiting sleep mode. Therefore, move
>> the delay into the already existing device specific initialization
>> function.
>>
>> Also, the timing contraints after entering and exiting sleep mode differ
>> between the two panels:
>> - The JH057N requires a shorter delay than the XDB599 after waking up
>> from sleep mode and before enabling the display.
>> - The XDB599 requires a delay in order to drain the display of charge,
>> which is not required on the JH057N.
>
> There’s no difference between the panels here. It’s a controller specified
> requirement.
>
> <https://megous.com/dl/tmp/1ef533ed8a7ce841.png>
>
> 60ms used in the driver between sleep out and display on is just
> incorrect from the datasheet perspective.
Please let me point you to the discussion you and Guido had ~2.5 years ago:
<https://lore.kernel.org/all/[email protected]/>
What resonates most with me is the following statement from Guido:
> > > > 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.
My takeaway is, that neither panel needed the actual 120 msec wait time. But Guido was reluctant to change the timing for the Librem 5 devkit panel. That’s why I went for the panel specific implementation.
Of course, we can revisit that decision. Since I don’t have the Librem 5 devkit, I have to kindly ask Guido for advise.
> You also have to wait 120ms after sleep in (or HW reset) and before shutting
> down the panel. If you don’t, after a bunch of cycles of this incorrect
> power up/down sequence the panel will start blinking weirdly, and the incorrect
> power up/down sequence without delays will not be able to recover it. You’ll
> have to let the panel sit for 5-10 minutes powered off before it starts to
> behave itself again.
>
> The documentation for sleep in specifies what’s happening during sleep in,
> and why this delay is necessary after sleep in:
>
> <https://megous.com/dl/tmp/2284b9d0f506b9b8.png>
I read that screenshot, that we need a 120 msec wait after sleep OUT before we can send another sleep in (see the “Restriction” row). I can’t seem to find the reference to the 120 msec delay after the sleep IN command. I read the flow chart at the bottom as informational about the duration of the whole procedure that happens after issuing the sleep in command. The only restriction is that we can’t issue any command for 5 msec after sleep in was issued.
> So there needs to be 120ms delay after sleep in and after sleep out,
> regardless of which panel is driven by this controller, to ensure the panel
> stays operational even when the user is quickly switching it on/off repeatedly.
>
> So I don’t think you should be doing panel specific quirks here.
Maybe. I can only say that without the timings in this patch (i.e. the ones from your kernel branch) the display on my pinephone is flickering after the first (and every subsequent) time the display is turned off. With your new timing everything works great on the pinephone. Guido states that the timings in your original patch (i.e. the XDB599 specific timings in this patch) the Librem 5 devkit panel doesn’t work.
Do you have a proposal how to proceed without implementing panel specific timings?
Thanks,
Frank
>
> regards,
> o.
>
>> Therefore, introduce panel specific functions for the delays.
>>
>> The XDB599 does not require a 20 msec delay between the SETBGP and
>> SETVCOM commands. Therefore, remove the delay from the device specific
>> initialization function.
>>
>> Signed-off-by: Frank Oltmanns <[email protected]>
>> Cc: Ondrej Jirman <[email protected]>
>> Reported-by: Samuel Holland <[email protected]>
>> —
>> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++—
>> 1 file changed, 35 insertions(+), 5 deletions(-)
>>
>> diff –git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> index 6747ca237ced..a149341c4a8b 100644
>> — a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> @@ -66,6 +66,8 @@ struct st7703_panel_desc {
>> unsigned long mode_flags;
>> enum mipi_dsi_pixel_format format;
>> int (*init_sequence)(struct st7703 *ctx);
>> + void (*wait_after_sleep_out)(void);
>> + void (*drain_charge)(void);
>> };
>>
>> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
>> @@ -126,10 +128,24 @@ static int jh057n_init_sequence(struct st7703 *ctx)
>> 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
>> 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
>> 0x11, 0x18);
>> + msleep(20);
>>
>> return 0;
>> }
>>
>> +static void jh057n_wait_after_sleep_out(void)
>> +{
>> + /*
>> + * Panel is operational 120 msec after reset, i.e. 60 msec after
>> + * sleep out.
>> + */
>> + msleep(60);
>> +}
>> +
>> +static void jh057n_drain_charge(void)
>> +{
>> +}
>> +
>> static const struct drm_display_mode jh057n00900_mode = {
>> .hdisplay = 720,
>> .hsync_start = 720 + 90,
>> @@ -152,6 +168,8 @@ static const struct st7703_panel_desc jh057n00900_panel_desc = {
>> MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
>> .format = MIPI_DSI_FMT_RGB888,
>> .init_sequence = jh057n_init_sequence,
>> + .wait_after_sleep_out = jh057n_wait_after_sleep_out,
>> + .drain_charge = jh057n_drain_charge,
>> };
>>
>> static int xbd599_init_sequence(struct st7703 *ctx)
>> @@ -273,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP,
>> 0x07, /* VREF_SEL = 4.2V */
>> 0x07 /* NVREF_SEL = 4.2V */);
>> - msleep(20);
>>
>> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM,
>> 0x2C, /* VCOMDC_F = -0.67V */
>> @@ -315,6 +332,18 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>> return 0;
>> }
>>
>> +static void xbd599_wait_after_sleep_out(void)
>> +{
>> + msleep(120);
>> +}
>> +
>> +static void xbd599_drain_charge(void)
>> +{
>> + /* Drain diplay of charge, to work correctly on next power on. */
>> + msleep(120);
>> +}
>> +
>> +
>> static const struct drm_display_mode xbd599_mode = {
>> .hdisplay = 720,
>> .hsync_start = 720 + 40,
>> @@ -336,6 +365,8 @@ static const struct st7703_panel_desc xbd599_desc = {
>> .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
>> .format = MIPI_DSI_FMT_RGB888,
>> .init_sequence = xbd599_init_sequence,
>> + .wait_after_sleep_out = xbd599_wait_after_sleep_out,
>> + .drain_charge = xbd599_drain_charge,
>> };
>>
>> static int st7703_enable(struct drm_panel *panel)
>> @@ -350,16 +381,13 @@ static int st7703_enable(struct drm_panel *panel)
>> return ret;
>> }
>>
>> - msleep(20);
>> -
>> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>> if (ret < 0) {
>> dev_err(ctx->dev, “Failed to exit sleep mode: %d\n”, ret);
>> return ret;
>> }
>>
>> - /* Panel is operational 120 msec after reset */
>> - msleep(60);
>> + ctx->desc->wait_after_sleep_out();
>>
>> ret = mipi_dsi_dcs_set_display_on(dsi);
>> if (ret)
>> @@ -384,6 +412,8 @@ static int st7703_disable(struct drm_panel *panel)
>> if (ret < 0)
>> dev_err(ctx->dev, “Failed to enter sleep mode: %d\n”, ret);
>>
>> + ctx->desc->drain_charge();
>> +
>> return 0;
>> }
>>
>> –
>> 2.39.1
>>
Hello,
On Sun, Feb 12, 2023 at 06:52:05PM +0100, Frank Oltmanns wrote:
> Hi Ondřej,
> hi Guido,
>
> Ondřej, thank you very much for your feedback!
>
> I have a couple of questions.
>
> Ondřej Jirman <[email protected]> writes:
>
> > On Sun, Feb 12, 2023 at 01:08:29PM +0100, Frank Oltmanns wrote:
> >> In contrast to the JH057N panel, the XBD599 panel does not require a 20
> >> msec delay after initialization and exiting sleep mode. Therefore, move
> >> the delay into the already existing device specific initialization
> >> function.
> >>
> >> Also, the timing contraints after entering and exiting sleep mode differ
> >> between the two panels:
> >> - The JH057N requires a shorter delay than the XDB599 after waking up
> >> from sleep mode and before enabling the display.
> >> - The XDB599 requires a delay in order to drain the display of charge,
> >> which is not required on the JH057N.
> >
> > There’s no difference between the panels here. It’s a controller specified
> > requirement.
> >
> > <https://megous.com/dl/tmp/1ef533ed8a7ce841.png>
> >
> > 60ms used in the driver between sleep out and display on is just
> > incorrect from the datasheet perspective.
>
> Please let me point you to the discussion you and Guido had ~2.5 years ago:
> <https://lore.kernel.org/all/[email protected]/>
Guido references some unspecified datasheet. We only have a st7703 datasheet
to go by, and that requires 120ms and is relevant for both panels.
Also the patch that Guido tested removed a few 20ms delays from the init
sequence for the librem panel. Maybe that broke the init for librem panel and
not the extra few ms after sleep out that the patch added.
> What resonates most with me is the following statement from Guido:
> > > > > 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.
>
> My takeaway is, that neither panel needed the actual 120 msec wait time. But
> Guido was reluctant to change the timing for the Librem 5 devkit panel. That’s
> why I went for the panel specific implementation.
>
> Of course, we can revisit that decision. Since I don’t have the Librem
> 5 devkit, I have to kindly ask Guido for advise.
>
> > You also have to wait 120ms after sleep in (or HW reset) and before shutting
> > down the panel. If you don’t, after a bunch of cycles of this incorrect
> > power up/down sequence the panel will start blinking weirdly, and the incorrect
> > power up/down sequence without delays will not be able to recover it. You’ll
> > have to let the panel sit for 5-10 minutes powered off before it starts to
> > behave itself again.
> >
> > The documentation for sleep in specifies what’s happening during sleep in,
> > and why this delay is necessary after sleep in:
> >
> > <https://megous.com/dl/tmp/2284b9d0f506b9b8.png>
>
> I read that screenshot, that we need a 120 msec wait after sleep OUT before we
> can send another sleep in (see the “Restriction” row). I can’t seem to find
> the reference to the 120 msec delay after the sleep IN command. I read the
> flow chart at the bottom as informational about the duration of the whole
> procedure that happens after issuing the sleep in command. The only
> restriction is that we can’t issue any command for 5 msec after sleep in was
> issued.
It's at the bottom, sleep in takes 120ms to execute. Part of the execution is
draining the charge from the panel. You can't shutdown power supplies before
sleep in completes, so you need the delay after sleep in and before regulator
powerdown, otherwise the flow chart will not have the time to execute properly,
and the panel will be left in a bad state.
> > So there needs to be 120ms delay after sleep in and after sleep out,
> > regardless of which panel is driven by this controller, to ensure the panel
> > stays operational even when the user is quickly switching it on/off repeatedly.
> >
> > So I don’t think you should be doing panel specific quirks here.
>
> Maybe. I can only say that without the timings in this patch (i.e. the ones
> from your kernel branch) the display on my pinephone is flickering after the
> first (and every subsequent) time the display is turned off. With your new
> timing everything works great on the pinephone. Guido states that the timings
> in your original patch (i.e. the XDB599 specific timings in this patch) the
> Librem 5 devkit panel doesn’t work.
Adding extra delays after sleep in/before sleep out should not break Librem
panel. Previous patches also changed the powerup sequence to hold the reset
before powerup of the power supplies, and rearranged other delays.
They were made before the problem with the panel discharge was properly
understood.
I suggest just going a bit more conservatively than what I did with my original
patch series, and just make sure there are 120ms delays after sleep out and
before sleep in + extra delay after regulator powerup and don't do anything that
may decrease delays for librem devkit panel. Just adding delays should not break
anything, sine the only timings that have maximums specified in the datasheet
are for power rail voltage rampup times.
(Also some timings during powerup/down may appear to have different needs on
Librem devkit simply because panel driver is not really affecting the
regulators, because they are already powered up due to being referenced by other
drivers/devices, because they are shared. This is a bit tricky to test
properly. It's necessary to test both power sequence cases, where the regulators
are known to be off, and when they are already referenced by other drivers.
On librem devkit, one of them seems to be shared by audio codec and touchscreen,
on pinephone there's less sharing. Easiest way to do that is to unload the
relevant drivers for other devices that share the regulators and check with
debugfs/regulator/regulator_summary that the refcount of lcd regulators)
regards,
o.
> Do you have a proposal how to proceed without implementing panel specific
> timings?
>
> Thanks,
> Frank
>
> >
> > regards,
> > o.
> >
> >> Therefore, introduce panel specific functions for the delays.
> >>
> >> The XDB599 does not require a 20 msec delay between the SETBGP and
> >> SETVCOM commands. Therefore, remove the delay from the device specific
> >> initialization function.
> >>
> >> Signed-off-by: Frank Oltmanns <[email protected]>
> >> Cc: Ondrej Jirman <[email protected]>
> >> Reported-by: Samuel Holland <[email protected]>
> >> —
> >> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++—
> >> 1 file changed, 35 insertions(+), 5 deletions(-)
> >>
> >> diff –git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> index 6747ca237ced..a149341c4a8b 100644
> >> — a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> @@ -66,6 +66,8 @@ struct st7703_panel_desc {
> >> unsigned long mode_flags;
> >> enum mipi_dsi_pixel_format format;
> >> int (*init_sequence)(struct st7703 *ctx);
> >> + void (*wait_after_sleep_out)(void);
> >> + void (*drain_charge)(void);
> >> };
> >>
> >> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
> >> @@ -126,10 +128,24 @@ static int jh057n_init_sequence(struct st7703 *ctx)
> >> 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> >> 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> >> 0x11, 0x18);
> >> + msleep(20);
> >>
> >> return 0;
> >> }
> >>
> >> +static void jh057n_wait_after_sleep_out(void)
> >> +{
> >> + /*
> >> + * Panel is operational 120 msec after reset, i.e. 60 msec after
> >> + * sleep out.
> >> + */
> >> + msleep(60);
> >> +}
> >> +
> >> +static void jh057n_drain_charge(void)
> >> +{
> >> +}
> >> +
> >> static const struct drm_display_mode jh057n00900_mode = {
> >> .hdisplay = 720,
> >> .hsync_start = 720 + 90,
> >> @@ -152,6 +168,8 @@ static const struct st7703_panel_desc jh057n00900_panel_desc = {
> >> MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
> >> .format = MIPI_DSI_FMT_RGB888,
> >> .init_sequence = jh057n_init_sequence,
> >> + .wait_after_sleep_out = jh057n_wait_after_sleep_out,
> >> + .drain_charge = jh057n_drain_charge,
> >> };
> >>
> >> static int xbd599_init_sequence(struct st7703 *ctx)
> >> @@ -273,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx)
> >> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP,
> >> 0x07, /* VREF_SEL = 4.2V */
> >> 0x07 /* NVREF_SEL = 4.2V */);
> >> - msleep(20);
> >>
> >> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM,
> >> 0x2C, /* VCOMDC_F = -0.67V */
> >> @@ -315,6 +332,18 @@ static int xbd599_init_sequence(struct st7703 *ctx)
> >> return 0;
> >> }
> >>
> >> +static void xbd599_wait_after_sleep_out(void)
> >> +{
> >> + msleep(120);
> >> +}
> >> +
> >> +static void xbd599_drain_charge(void)
> >> +{
> >> + /* Drain diplay of charge, to work correctly on next power on. */
> >> + msleep(120);
> >> +}
> >> +
> >> +
> >> static const struct drm_display_mode xbd599_mode = {
> >> .hdisplay = 720,
> >> .hsync_start = 720 + 40,
> >> @@ -336,6 +365,8 @@ static const struct st7703_panel_desc xbd599_desc = {
> >> .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
> >> .format = MIPI_DSI_FMT_RGB888,
> >> .init_sequence = xbd599_init_sequence,
> >> + .wait_after_sleep_out = xbd599_wait_after_sleep_out,
> >> + .drain_charge = xbd599_drain_charge,
> >> };
> >>
> >> static int st7703_enable(struct drm_panel *panel)
> >> @@ -350,16 +381,13 @@ static int st7703_enable(struct drm_panel *panel)
> >> return ret;
> >> }
> >>
> >> - msleep(20);
> >> -
> >> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >> if (ret < 0) {
> >> dev_err(ctx->dev, “Failed to exit sleep mode: %d\n”, ret);
> >> return ret;
> >> }
> >>
> >> - /* Panel is operational 120 msec after reset */
> >> - msleep(60);
> >> + ctx->desc->wait_after_sleep_out();
> >>
> >> ret = mipi_dsi_dcs_set_display_on(dsi);
> >> if (ret)
> >> @@ -384,6 +412,8 @@ static int st7703_disable(struct drm_panel *panel)
> >> if (ret < 0)
> >> dev_err(ctx->dev, “Failed to enter sleep mode: %d\n”, ret);
> >>
> >> + ctx->desc->drain_charge();
> >> +
> >> return 0;
> >> }
> >>
> >> –
> >> 2.39.1
> >>
Hi Ondřej,
ok, now I get it. Thank you very much for your thorough explanation. It really appreciate it!
Ondřej Jirman <[email protected]> writes:
> On Sun, Feb 12, 2023 at 06:52:05PM +0100, Frank Oltmanns wrote:
>> Ondřej Jirman <[email protected]> writes:
>>
>> > On Sun, Feb 12, 2023 at 01:08:29PM +0100, Frank Oltmanns wrote:
>> Please let me point you to the discussion you and Guido had ~2.5 years ago:
>> <https://lore.kernel.org/all/[email protected]/>
>
> Guido references some unspecified datasheet. We only have a st7703 datasheet
> to go by, and that requires 120ms and is relevant for both panels.
>
> Also the patch that Guido tested removed a few 20ms delays from the init
> sequence for the librem panel. Maybe that broke the init for librem panel and
> not the extra few ms after sleep out that the patch added.
Ok, I see. You removed the 20 msec delay after the panel specific
initialization sequence and before issuing SLPOUT in your original
patch.
>> I read that screenshot, that we need a 120 msec wait after sleep OUT before we
>> can send another sleep in (see the “Restriction” row). I can’t seem to find
>> the reference to the 120 msec delay after the sleep IN command. I read the
>> flow chart at the bottom as informational about the duration of the whole
>> procedure that happens after issuing the sleep in command. The only
>> restriction is that we can’t issue any command for 5 msec after sleep in was
>> issued.
>
> It’s at the bottom, sleep in takes 120ms to execute. Part of the execution is
> draining the charge from the panel. You can’t shutdown power supplies before
> sleep in completes, so you need the delay after sleep in and before regulator
> powerdown, otherwise the flow chart will not have the time to execute properly,
> and the panel will be left in a bad state.
Ok, that makes sense.
>> > So there needs to be 120ms delay after sleep in and after sleep out,
>> > regardless of which panel is driven by this controller, to ensure the panel
>> > stays operational even when the user is quickly switching it on/off repeatedly.
>> >
>> > So I don’t think you should be doing panel specific quirks here.
>>
>> Maybe. I can only say that without the timings in this patch (i.e. the ones
>> from your kernel branch) the display on my pinephone is flickering after the
>> first (and every subsequent) time the display is turned off. With your new
>> timing everything works great on the pinephone. Guido states that the timings
>> in your original patch (i.e. the XDB599 specific timings in this patch) the
>> Librem 5 devkit panel doesn’t work.
>
> Adding extra delays after sleep in/before sleep out should not break Librem
> panel. Previous patches also changed the powerup sequence to hold the reset
> before powerup of the power supplies, and rearranged other delays.
>
> They were made before the problem with the panel discharge was properly
> understood.
>
> I suggest just going a bit more conservatively than what I did with my original
> patch series, and just make sure there are 120ms delays after sleep out and
> before sleep in + extra delay after regulator powerup and don’t do anything that
> may decrease delays for librem devkit panel. Just adding delays should not break
> anything, sine the only timings that have maximums specified in the datasheet
> are for power rail voltage rampup times.
I agree, that the one 20 msec delay that you removed, is probably what
broke the JH057N panel and not the extra delays. Unfortunately, there is
no way for me to verify that statement, because I don’t have access to
Librem’s devkit.
So, I can only offer to prepare a V2 and kindly ask Guido to test it. V2
will be much simpler:
• Increase the delay after issuing sleep out from 60 to 120 msec for all
panels.
• Add a delay of 120 msec after issuing sleep in for all panels.
• Move the 20 msec delay between initialization and sleep out into the
initialization function for the JH057N panel (like in this version of
the patch).
• Remove a 20 msec delay from the initialization function of the XBD599
panel (like in this version of the patch).
> (Also some timings during powerup/down may appear to have different needs on
> Librem devkit simply because panel driver is not really affecting the
> regulators, because they are already powered up due to being referenced by other
> drivers/devices, because they are shared. This is a bit tricky to test
> properly. It’s necessary to test both power sequence cases, where the regulators
> are known to be off, and when they are already referenced by other drivers.
> On librem devkit, one of them seems to be shared by audio codec and touchscreen,
> on pinephone there’s less sharing. Easiest way to do that is to unload the
> relevant drivers for other devices that share the regulators and check with
> debugfs/regulator/regulator_summary that the refcount of lcd regulators)
Well, that was interesting! Thank you! I checked on my pinephone and I
only have refcounts of 1 on the relevant child nodes as you expeceted. :)
Thanks again,
Frank
> regards,
> o.
>
>> Do you have a proposal how to proceed without implementing panel specific
>> timings?
>>
>> Thanks,
>> Frank
>>
>> >
>> > regards,
>> > o.
>> >
>> >> Therefore, introduce panel specific functions for the delays.
>> >>
>> >> The XDB599 does not require a 20 msec delay between the SETBGP and
>> >> SETVCOM commands. Therefore, remove the delay from the device specific
>> >> initialization function.
>> >>
>> >> Signed-off-by: Frank Oltmanns <[email protected]>
>> >> Cc: Ondrej Jirman <[email protected]>
>> >> Reported-by: Samuel Holland <[email protected]>
>> >> —
>> >> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 40 ++++++++++++++++—
>> >> 1 file changed, 35 insertions(+), 5 deletions(-)
>> >>
>> >> diff –git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> index 6747ca237ced..a149341c4a8b 100644
>> >> — a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> @@ -66,6 +66,8 @@ struct st7703_panel_desc {
>> >> unsigned long mode_flags;
>> >> enum mipi_dsi_pixel_format format;
>> >> int (*init_sequence)(struct st7703 *ctx);
>> >> + void (*wait_after_sleep_out)(void);
>> >> + void (*drain_charge)(void);
>> >> };
>> >>
>> >> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
>> >> @@ -126,10 +128,24 @@ static int jh057n_init_sequence(struct st7703 *ctx)
>> >> 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
>> >> 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
>> >> 0x11, 0x18);
>> >> + msleep(20);
>> >>
>> >> return 0;
>> >> }
>> >>
>> >> +static void jh057n_wait_after_sleep_out(void)
>> >> +{
>> >> + /*
>> >> + * Panel is operational 120 msec after reset, i.e. 60 msec after
>> >> + * sleep out.
>> >> + */
>> >> + msleep(60);
>> >> +}
>> >> +
>> >> +static void jh057n_drain_charge(void)
>> >> +{
>> >> +}
>> >> +
>> >> static const struct drm_display_mode jh057n00900_mode = {
>> >> .hdisplay = 720,
>> >> .hsync_start = 720 + 90,
>> >> @@ -152,6 +168,8 @@ static const struct st7703_panel_desc jh057n00900_panel_desc = {
>> >> MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
>> >> .format = MIPI_DSI_FMT_RGB888,
>> >> .init_sequence = jh057n_init_sequence,
>> >> + .wait_after_sleep_out = jh057n_wait_after_sleep_out,
>> >> + .drain_charge = jh057n_drain_charge,
>> >> };
>> >>
>> >> static int xbd599_init_sequence(struct st7703 *ctx)
>> >> @@ -273,7 +291,6 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>> >> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP,
>> >> 0x07, /* VREF_SEL = 4.2V */
>> >> 0x07 /* NVREF_SEL = 4.2V */);
>> >> - msleep(20);
>> >>
>> >> mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM,
>> >> 0x2C, /* VCOMDC_F = -0.67V */
>> >> @@ -315,6 +332,18 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>> >> return 0;
>> >> }
>> >>
>> >> +static void xbd599_wait_after_sleep_out(void)
>> >> +{
>> >> + msleep(120);
>> >> +}
>> >> +
>> >> +static void xbd599_drain_charge(void)
>> >> +{
>> >> + /* Drain diplay of charge, to work correctly on next power on. */
>> >> + msleep(120);
>> >> +}
>> >> +
>> >> +
>> >> static const struct drm_display_mode xbd599_mode = {
>> >> .hdisplay = 720,
>> >> .hsync_start = 720 + 40,
>> >> @@ -336,6 +365,8 @@ static const struct st7703_panel_desc xbd599_desc = {
>> >> .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
>> >> .format = MIPI_DSI_FMT_RGB888,
>> >> .init_sequence = xbd599_init_sequence,
>> >> + .wait_after_sleep_out = xbd599_wait_after_sleep_out,
>> >> + .drain_charge = xbd599_drain_charge,
>> >> };
>> >>
>> >> static int st7703_enable(struct drm_panel *panel)
>> >> @@ -350,16 +381,13 @@ static int st7703_enable(struct drm_panel *panel)
>> >> return ret;
>> >> }
>> >>
>> >> - msleep(20);
>> >> -
>> >> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>> >> if (ret < 0) {
>> >> dev_err(ctx->dev, “Failed to exit sleep mode: %d\n”, ret);
>> >> return ret;
>> >> }
>> >>
>> >> - /* Panel is operational 120 msec after reset */
>> >> - msleep(60);
>> >> + ctx->desc->wait_after_sleep_out();
>> >>
>> >> ret = mipi_dsi_dcs_set_display_on(dsi);
>> >> if (ret)
>> >> @@ -384,6 +412,8 @@ static int st7703_disable(struct drm_panel *panel)
>> >> if (ret < 0)
>> >> dev_err(ctx->dev, “Failed to enter sleep mode: %d\n”, ret);
>> >>
>> >> + ctx->desc->drain_charge();
>> >> +
>> >> return 0;
>> >> }
>> >>
>> >> –
>> >> 2.39.1
>> >>
