Skip to content

drm/panel: waveshare-dsi: Add defensive initialization for I2C/DSI timing #7182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
foonerd wants to merge 2 commits into
base: rpi-6.12.y
Choose a base branch
from
Open

drm/panel: waveshare-dsi: Add defensive initialization for I2C/DSI timing #7182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ba76a9
drm/panel: waveshare-dsi: Add I2C retry logic for cold boot reliability
foonerd Dec 18, 2025
55f8bc2
drm/panel: waveshare-dsi-v2: Add DSI command retry logic for cold boo…
foonerd Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions drivers/gpu/drm/panel/panel-waveshare-dsi-v2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@

#include <video/mipi_display.h>

#define WS_DSI_RETRIES 3 /* Number of retry attempts for DSI operations */
#define WS_DSI_RETRY_DELAY_MS 50 /* Delay between retries in milliseconds */
Copy link
Contributor

@6by9 6by9 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 retries with 50ms between would be 150ms per command. I make it nearly 200 commands per table. Worst case situation is you've just added a 30second delay.

Why do you believe there needs to be a delay between retries? DSI is an uncontended point to point interface, so it's not as if you need to wait for the bus to clear.

Copy link
Contributor Author

@foonerd foonerd Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. DSI is point-to-point with no bus contention - if a command fails, waiting and retrying does not address the underlying cause.
The reported cold boot failures were on I2C-based panels (7.9", 11.9") where retry logic makes sense for transient bus issues. I do not have evidence of the same problem on DSI-based panels.


struct ws_panel_desc {
const struct panel_init_cmd *init;
const struct drm_display_mode *mode;
Expand Down Expand Up @@ -1707,6 +1710,9 @@ static int ws_panel_init_dcs_cmd(struct ws_panel *ts)
struct drm_panel *panel = &ts->panel;
int i, err = 0;

/* Allow panel controller to stabilize after power-up/reset */
msleep(WS_DSI_RETRY_DELAY_MS);
Copy link
Contributor

@6by9 6by9 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already had a msleep(60) in ws_panel_prepare after setting reset high, or 20ms after having enabled avdd. Extend that delay if you view that to be too short as that is what they are explicitly there to do.

Or if it is one particular panel type that is slow, add an _INIT_DELAY_CMD entry at the start of that table.

Copy link
Contributor Author

@foonerd foonerd Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - the delay infrastructure already exists in ws_panel_prepare. Adding another msleep here is redundant.
If specific panels need longer stabilization, DELAY_CMD in their init tables is the approach.


if (ts->desc->init) {
const struct panel_init_cmd *init_cmds = ts->desc->init;

Expand All @@ -1720,11 +1726,20 @@ static int ws_panel_init_dcs_cmd(struct ws_panel *ts)
break;

case INIT_DCS_CMD:
err = mipi_dsi_dcs_write(
dsi, cmd->data[0],
cmd->len <= 1 ? NULL : &cmd->data[1],
cmd->len - 1);
{
int retries = WS_DSI_RETRIES;

do {
err = mipi_dsi_dcs_write(
dsi, cmd->data[0],
cmd->len <= 1 ? NULL : &cmd->data[1],
cmd->len - 1);
if (err >= 0)
break;
msleep(WS_DSI_RETRY_DELAY_MS);
} while (--retries > 0);
break;
}

default:
err = -EINVAL;
Expand Down
23 changes: 18 additions & 5 deletions drivers/gpu/drm/panel/panel-waveshare-dsi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@

#define WS_DSI_DRIVER_NAME "ws-ts-dsi"

#define WS_I2C_RETRIES 3 /* Number of retry attempts for I2C operations */
#define WS_I2C_RETRY_DELAY_MS 50 /* Delay between retries in milliseconds */
Copy link
Contributor

@6by9 6by9 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here as to why delay between retries. Multiple I2C masters on the same bus aren't supported by the Pi, so what do you believe is happening during the 50ms you're delaying for?

Copy link
Contributor Author

@foonerd foonerd Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The observed failure mode is cold boot - driver probes before the panel controller IC completes its internal power-on initialization. First I2C write returns -5 (EIO).
The delay between retries gives the IC additional time since power-on. Not waiting for bus clearance - waiting for the IC to become responsive.
However, if the IC is not ready at probe time, would returning -EPROBE_DEFER be more appropriate than retry loops? Let the kernel reschedule the probe rather than busy-waiting in the driver.

Copy link
Contributor

@6by9 6by9 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question for @waveshare then.

The microcontroller that is on the other end of the I2C interface will have been powered up with the Pi. Getting to the point where this driver will poke the controller is going to be multiple seconds down the line. What on earth is the microcontroller doing to take that long to be ready to process I2C requests, and how can adding 100ms be enough to make it happy?!

#define WS_INIT_DELAY_MS 100 /* Initial stabilization delay for panel controller */

struct ws_panel {
struct drm_panel base;
struct mipi_dsi_device *dsi;
Expand Down Expand Up @@ -329,13 +333,19 @@ static struct ws_panel *panel_to_ts(struct drm_panel *panel)
return container_of(panel, struct ws_panel, base);
}

static void ws_panel_i2c_write(struct ws_panel *ts, u8 reg, u8 val)
static int ws_panel_i2c_write(struct ws_panel *ts, u8 reg, u8 val)
{
int ret;
int ret, retries = WS_I2C_RETRIES;

ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
if (ret)
dev_err(&ts->i2c->dev, "I2C write failed: %d\n", ret);
while (retries--) {
ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
if (!ret)
return 0;
msleep(WS_I2C_RETRY_DELAY_MS);
}
dev_err(&ts->i2c->dev, "I2C write failed after %d retries: %d\n",
WS_I2C_RETRIES, ret);
return ret;
}

static int ws_panel_disable(struct drm_panel *panel)
Expand Down Expand Up @@ -479,6 +489,9 @@ static int ws_panel_probe(struct i2c_client *i2c)

ts->i2c = i2c;

/* Allow panel controller to stabilize after power-up */
msleep(WS_INIT_DELAY_MS);
Copy link
Contributor

@6by9 6by9 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you waiting on for this? No commands have been issued to the display at all by the driver at this stage, nor control of any power rails (there is no control). The display will have got power when the Pi was powered on, so adding 100ms here means nothing.

ts->i2c = i2c; does not obtain an I2C handle, it is just storing a handle given to it by the probing framework within the state structure.

Copy link
Contributor Author

@foonerd foonerd Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent was to handle cases where probe occurs before the panel controller IC is ready to accept I2C commands after power-on. The actual fix is the retry logic in ws_panel_i2c_write - the initial delay was belt-and-suspenders.
Options:

Remove the msleep entirely and rely on retry logic alone
Move the stabilization delay into the first iteration of the retry loop

Perhaps the retry logic alone may be sufficient given the 50ms inter-retry delays.


ws_panel_i2c_write(ts, 0xc0, 0x01);
ws_panel_i2c_write(ts, 0xc2, 0x01);
ws_panel_i2c_write(ts, 0xac, 0x01);
Expand Down
Loading