| Message ID | 20250331061838.167781-1-tejasvipin76@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/panel: boe-bf060y8m-aj0: transition to mipi_dsi wrapped functions | expand |
Hi, On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@gmail.com> wrote: > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) > > ret = boe_bf060y8m_aj0_on(boe); > if (ret < 0) { > - dev_err(dev, "Failed to initialize panel: %d\n", ret); > gpiod_set_value_cansleep(boe->reset_gpio, 1); > return ret; It's not new, but the error handling here looks wrong to me. Instead of just returning after setting the GPIO, this should be turning off the regulators, shouldn't it? That would mean adding a new error label for turning off "BF060Y8M_VREG_VCI" and then jumping to that. Given that we're already squeezing an error handling fix into this patch, maybe it would make sense to add this one too (and, of course, mention it in the commit message). -Doug
On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote: > Hi, > > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@gmail.com> wrote: > > > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) > > > > ret = boe_bf060y8m_aj0_on(boe); > > if (ret < 0) { > > - dev_err(dev, "Failed to initialize panel: %d\n", ret); > > gpiod_set_value_cansleep(boe->reset_gpio, 1); > > return ret; > > It's not new, but the error handling here looks wrong to me. Instead > of just returning after setting the GPIO, this should be turning off > the regulators, shouldn't it? That would mean adding a new error label > for turning off "BF060Y8M_VREG_VCI" and then jumping to that. We should not be turning off the regulator in _prepare(), there will be an unmatched regulator disable call happening in _unprepare(). Of course it can be handled by adding a boolean, etc, but I think keeping them on is a saner thing. > > Given that we're already squeezing an error handling fix into this > patch, maybe it would make sense to add this one too (and, of course, > mention it in the commit message). > > -Doug
Hi, On Mon, Mar 31, 2025 at 1:28 PM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote: > > On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote: > > Hi, > > > > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@gmail.com> wrote: > > > > > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) > > > > > > ret = boe_bf060y8m_aj0_on(boe); > > > if (ret < 0) { > > > - dev_err(dev, "Failed to initialize panel: %d\n", ret); > > > gpiod_set_value_cansleep(boe->reset_gpio, 1); > > > return ret; > > > > It's not new, but the error handling here looks wrong to me. Instead > > of just returning after setting the GPIO, this should be turning off > > the regulators, shouldn't it? That would mean adding a new error label > > for turning off "BF060Y8M_VREG_VCI" and then jumping to that. > > We should not be turning off the regulator in _prepare(), there will be > an unmatched regulator disable call happening in _unprepare(). Of course > it can be handled by adding a boolean, etc, but I think keeping them on > is a saner thing. Hrmmmm. The issue is that if we're returning an error from a function the caller should expect that the function undid anything that it did partially. It _has_ to work that way, right? Otherwise we've lost the context of exactly how far we got through the function so we don't know which things to later undo and which things to later not undo. ...although I think you said that the DRM framework ignores errors from prepare() and still calls unprepare(). I guess this is in panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error code is ignored? This feels like a bug waiting to happen. Are you saying that boe_bf060y8m_aj0_unprepare() has to be written such that it doesn't hit regulator underflows no matter which fail path boe_bf060y8m_aj0_prepare() hit? That feels wrong. -Doug
On Mon, Mar 31, 2025 at 03:40:27PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Mar 31, 2025 at 1:28 PM Dmitry Baryshkov > <dmitry.baryshkov@oss.qualcomm.com> wrote: > > > > On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@gmail.com> wrote: > > > > > > > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) > > > > > > > > ret = boe_bf060y8m_aj0_on(boe); > > > > if (ret < 0) { > > > > - dev_err(dev, "Failed to initialize panel: %d\n", ret); > > > > gpiod_set_value_cansleep(boe->reset_gpio, 1); > > > > return ret; > > > > > > It's not new, but the error handling here looks wrong to me. Instead > > > of just returning after setting the GPIO, this should be turning off > > > the regulators, shouldn't it? That would mean adding a new error label > > > for turning off "BF060Y8M_VREG_VCI" and then jumping to that. > > > > We should not be turning off the regulator in _prepare(), there will be > > an unmatched regulator disable call happening in _unprepare(). Of course > > it can be handled by adding a boolean, etc, but I think keeping them on > > is a saner thing. > > Hrmmmm. > > The issue is that if we're returning an error from a function the > caller should expect that the function undid anything that it did > partially. It _has_ to work that way, right? Otherwise we've lost the > context of exactly how far we got through the function so we don't > know which things to later undo and which things to later not undo. Kind of yes. I'd rather make drm_panel functions return void here, as that matches panel bridge behaviour. The only driver that actually uses return values of those functions is analogix_dp, see analogix_dp_prepare_panel(). However most of invocations of that function can go away. I'll send a patchset. > > ...although I think you said that the DRM framework ignores errors > from prepare() and still calls unprepare(). I guess this is in > panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error > code is ignored? Yes. > This feels like a bug waiting to happen. Are you > saying that boe_bf060y8m_aj0_unprepare() has to be written such that > it doesn't hit regulator underflows no matter which fail path > boe_bf060y8m_aj0_prepare() hit? That feels wrong. Let me try to fix that.
On Tue, Apr 01, 2025 at 04:01:03AM +0300, Dmitry Baryshkov wrote: > On Mon, Mar 31, 2025 at 03:40:27PM -0700, Doug Anderson wrote: > > Hi, > > > > On Mon, Mar 31, 2025 at 1:28 PM Dmitry Baryshkov > > <dmitry.baryshkov@oss.qualcomm.com> wrote: > > > > > > On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@gmail.com> wrote: > > > > > > > > > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) > > > > > > > > > > ret = boe_bf060y8m_aj0_on(boe); > > > > > if (ret < 0) { > > > > > - dev_err(dev, "Failed to initialize panel: %d\n", ret); > > > > > gpiod_set_value_cansleep(boe->reset_gpio, 1); > > > > > return ret; > > > > > > > > It's not new, but the error handling here looks wrong to me. Instead > > > > of just returning after setting the GPIO, this should be turning off > > > > the regulators, shouldn't it? That would mean adding a new error label > > > > for turning off "BF060Y8M_VREG_VCI" and then jumping to that. > > > > > > We should not be turning off the regulator in _prepare(), there will be > > > an unmatched regulator disable call happening in _unprepare(). Of course > > > it can be handled by adding a boolean, etc, but I think keeping them on > > > is a saner thing. > > > > Hrmmmm. > > > > The issue is that if we're returning an error from a function the > > caller should expect that the function undid anything that it did > > partially. It _has_ to work that way, right? Otherwise we've lost the > > context of exactly how far we got through the function so we don't > > know which things to later undo and which things to later not undo. > > Kind of yes. I'd rather make drm_panel functions return void here, as > that matches panel bridge behaviour. The only driver that actually uses > return values of those functions is analogix_dp, see > analogix_dp_prepare_panel(). However most of invocations of that > function can go away. I'll send a patchset. > > > > > ...although I think you said that the DRM framework ignores errors > > from prepare() and still calls unprepare(). I guess this is in > > panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error > > code is ignored? > Hmm... Most of the drivers ignore the results of the drm_panel_prepare() / _unprepare() / _enable() / _disable(), but then the framework handles error values of the callbacks and skips calling the corresponding en/dis callback if the previous call has failed. Which means I was incorrect here. > > > This feels like a bug waiting to happen. Are you > > saying that boe_bf060y8m_aj0_unprepare() has to be written such that > > it doesn't hit regulator underflows no matter which fail path > > boe_bf060y8m_aj0_prepare() hit? That feels wrong. > > Let me try to fix that. > > -- > With best wishes > Dmitry
Hi, On Mon, Mar 31, 2025 at 6:52 PM Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote: > > > > ...although I think you said that the DRM framework ignores errors > > > from prepare() and still calls unprepare(). I guess this is in > > > panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error > > > code is ignored? > > > > Hmm... Most of the drivers ignore the results of the drm_panel_prepare() > / _unprepare() / _enable() / _disable(), but then the framework handles > error values of the callbacks and skips calling the corresponding > en/dis callback if the previous call has failed. Which means I was > incorrect here. Oh, right. LOL, that was even me adding that code in commit d2aacaf07395 ("drm/panel: Check for already prepared/enabled in drm_panel"). It wasn't my intention there to work around the fact that the panel_bridge ignores the error, but it's a happy accident. I guess that means that the warning: dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); ...would also happen any time a panel prepare returned an error code that was ignored by the bridge. That seems OK-ish to me even if the comment above the warning doesn't list that as one of the reasons the warning might be printed. I didn't test this myself, but assuming I got it right does anyone want to submit a patch to add this reason to the comment? ...or, maybe even better, we could fix the panel bridge code to not call the panel unprepare if the prepare failed... tl;dr for Tejas in this patch is that I still think he should spin his patch to fix it so the regulators get disabled in the error case. -Doug
diff --git a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c index 7e66db4a88bb..3b174b4a41b6 100644 --- a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c +++ b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c @@ -55,71 +55,51 @@ static void boe_bf060y8m_aj0_reset(struct boe_bf060y8m_aj0 *boe) static int boe_bf060y8m_aj0_on(struct boe_bf060y8m_aj0 *boe) { struct mipi_dsi_device *dsi = boe->dsi; - struct device *dev = &dsi->dev; - int ret; - - mipi_dsi_dcs_write_seq(dsi, 0xb0, 0xa5, 0x00); - mipi_dsi_dcs_write_seq(dsi, 0xb2, 0x00, 0x4c); - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_3D_CONTROL, 0x10); - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, DCS_ALLOW_HBM_RANGE); - mipi_dsi_dcs_write_seq(dsi, 0xf8, - 0x00, 0x08, 0x10, 0x00, 0x22, 0x00, 0x00, 0x2d); - - ret = mipi_dsi_dcs_exit_sleep_mode(dsi); - if (ret < 0) { - dev_err(dev, "Failed to exit sleep mode: %d\n", ret); - return ret; - } - msleep(30); - - mipi_dsi_dcs_write_seq(dsi, 0xb0, 0xa5, 0x00); - mipi_dsi_dcs_write_seq(dsi, 0xc0, - 0x08, 0x48, 0x65, 0x33, 0x33, 0x33, - 0x2a, 0x31, 0x39, 0x20, 0x09); - mipi_dsi_dcs_write_seq(dsi, 0xc1, 0x00, 0x00, 0x00, 0x1f, 0x1f, - 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, - 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f); - mipi_dsi_dcs_write_seq(dsi, 0xe2, 0x20, 0x04, 0x10, 0x12, 0x92, - 0x4f, 0x8f, 0x44, 0x84, 0x83, 0x83, 0x83, - 0x5c, 0x5c, 0x5c); - mipi_dsi_dcs_write_seq(dsi, 0xde, 0x01, 0x2c, 0x00, 0x77, 0x3e); - - msleep(30); - - ret = mipi_dsi_dcs_set_display_on(dsi); - if (ret < 0) { - dev_err(dev, "Failed to set display on: %d\n", ret); - return ret; - } - msleep(50); - - return 0; + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; + + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0xa5, 0x00); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2, 0x00, 0x4c); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_3D_CONTROL, 0x10); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_POWER_SAVE, DCS_ALLOW_HBM_RANGE); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf8, + 0x00, 0x08, 0x10, 0x00, 0x22, 0x00, 0x00, 0x2d); + + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + mipi_dsi_msleep(&dsi_ctx, 30); + + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0xa5, 0x00); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc0, + 0x08, 0x48, 0x65, 0x33, 0x33, 0x33, + 0x2a, 0x31, 0x39, 0x20, 0x09); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xc1, 0x00, 0x00, 0x00, 0x1f, 0x1f, + 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, + 0x1f, 0x1f, 0x1f, 0x1f, 0x1f, 0x1f); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe2, 0x20, 0x04, 0x10, 0x12, 0x92, + 0x4f, 0x8f, 0x44, 0x84, 0x83, 0x83, 0x83, + 0x5c, 0x5c, 0x5c); + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xde, 0x01, 0x2c, 0x00, 0x77, 0x3e); + + mipi_dsi_msleep(&dsi_ctx, 30); + + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + mipi_dsi_msleep(&dsi_ctx, 50); + + return dsi_ctx.accum_err; } -static int boe_bf060y8m_aj0_off(struct boe_bf060y8m_aj0 *boe) +static void boe_bf060y8m_aj0_off(struct boe_bf060y8m_aj0 *boe) { struct mipi_dsi_device *dsi = boe->dsi; - struct device *dev = &dsi->dev; - int ret; + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; /* OFF commands sent in HS mode */ dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; - ret = mipi_dsi_dcs_set_display_off(dsi); - if (ret < 0) { - dev_err(dev, "Failed to set display off: %d\n", ret); - return ret; - } - msleep(20); + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); + mipi_dsi_msleep(&dsi_ctx, 20); - ret = mipi_dsi_dcs_enter_sleep_mode(dsi); - if (ret < 0) { - dev_err(dev, "Failed to enter sleep mode: %d\n", ret); - return ret; - } - usleep_range(1000, 2000); + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); + mipi_dsi_usleep_range(&dsi_ctx, 1000, 2000); dsi->mode_flags |= MIPI_DSI_MODE_LPM; - - return 0; } static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) ret = boe_bf060y8m_aj0_on(boe); if (ret < 0) { - dev_err(dev, "Failed to initialize panel: %d\n", ret); gpiod_set_value_cansleep(boe->reset_gpio, 1); return ret; } @@ -178,15 +157,11 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel) static int boe_bf060y8m_aj0_unprepare(struct drm_panel *panel) { struct boe_bf060y8m_aj0 *boe = to_boe_bf060y8m_aj0(panel); - struct device *dev = &boe->dsi->dev; - int ret; - ret = boe_bf060y8m_aj0_off(boe); - if (ret < 0) - dev_err(dev, "Failed to un-initialize panel: %d\n", ret); + boe_bf060y8m_aj0_off(boe); gpiod_set_value_cansleep(boe->reset_gpio, 1); - ret = regulator_bulk_disable(ARRAY_SIZE(boe->vregs), boe->vregs); + regulator_bulk_disable(ARRAY_SIZE(boe->vregs), boe->vregs); return 0; } @@ -234,13 +209,11 @@ static int boe_bf060y8m_aj0_bl_update_status(struct backlight_device *bl) { struct mipi_dsi_device *dsi = bl_get_data(bl); u16 brightness = backlight_get_brightness(bl); - int ret; + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness); - if (ret < 0) - return ret; + mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, brightness); - return 0; + return dsi_ctx.accum_err; } static int boe_bf060y8m_aj0_bl_get_brightness(struct backlight_device *bl)