Am 31.01.23 um 22:27 schrieb Doug Anderson:
Hi, On Thu, Jan 26, 2023 at 4:52 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:Hi, On Wed, Jan 18, 2023 at 1:34 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:Hi, On Thu, Jan 5, 2023 at 7:01 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:The unprepare sequence has started to fail after moving to panel bridge code in the msm drm driver (commit 007ac0262b0d ("drm/msm/dsi: switch to DRM_PANEL_BRIDGE")). You'll see messages like this in the kernel logs: panel-boe-tv101wum-nl6 ae94000.dsi.0: failed to set panel off: -22 This is because boe_panel_enter_sleep_mode() needs an operating DSI link to set the panel into sleep mode. Performing those writes in the unprepare phase of bridge ops is too late, because the link has already been torn down by the DSI controller in post_disable, i.e. the PHY has been disabled, etc. See dsi_mgr_bridge_post_disable() for more details on the DSI . Split the unprepare function into a disable part and an unprepare part. For now, just the DSI writes to enter sleep mode are put in the disable function. This fixes the panel off routine and keeps the panel happy. My Wormdingler has an integrated touchscreen that stops responding to touch if the panel is only half disabled too. This patch fixes it. And finally, this saves power when the screen is off because without this fix the regulators for the panel are left enabled when nothing is being displayed on the screen. Fixes: 007ac0262b0d ("drm/msm/dsi: switch to DRM_PANEL_BRIDGE") Fixes: a869b9db7adf ("drm/panel: support for boe tv101wum-nl6 wuxga dsi video mode panel") Cc: yangcong <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx> Cc: Jitao Shi <jitao.shi@xxxxxxxxxxxx> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx> Cc: Rob Clark <robdclark@xxxxxxxxxxxx> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> --- drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c index 857a2f0420d7..c924f1124ebc 100644 --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c @@ -1193,14 +1193,11 @@ static int boe_panel_enter_sleep_mode(struct boe_panel *boe) return 0; } -static int boe_panel_unprepare(struct drm_panel *panel) +static int boe_panel_disable(struct drm_panel *panel) { struct boe_panel *boe = to_boe_panel(panel); int ret; - if (!boe->prepared) - return 0; - ret = boe_panel_enter_sleep_mode(boe); if (ret < 0) { dev_err(panel->dev, "failed to set panel off: %d\n", ret); @@ -1209,6 +1206,16 @@ static int boe_panel_unprepare(struct drm_panel *panel) msleep(150); + return 0; +} + +static int boe_panel_unprepare(struct drm_panel *panel) +{ + struct boe_panel *boe = to_boe_panel(panel); + + if (!boe->prepared) + return 0; + if (boe->desc->discharge_on_disable) { regulator_disable(boe->avee); regulator_disable(boe->avdd); @@ -1528,6 +1535,7 @@ static enum drm_panel_orientation boe_panel_get_orientation(struct drm_panel *pa } static const struct drm_panel_funcs boe_panel_funcs = { + .disable = boe_panel_disable, .unprepare = boe_panel_unprepare, .prepare = boe_panel_prepare, .enable = boe_panel_enable,As mentioned by Stephen, my initial reaction was that this felt asymmetric. We were moving some stuff from unprepare() to disable() and it felt like that would mean we would also need to move something from prepare() to enable. Initially I thought maybe that "something" was all of boe_panel_init_dcs_cmd() but I guess that didn't work. I don't truly have a reason that this _has_ to be symmetric. I was initially worried that there might be some place where we call pre_enable(), then never call enable() / disable(), and then call post_disable(). That could have us in a bad state because we'd never enter sleep mode / turn the display off. However (as I think I've discovered before and just forgot), I don't think this is possible because we always call pre-enable() and enable() together. Also, as mentioned by Sam, we're about to fully shut the panel's power off so (unless it's on a shared rail) it probably doesn't really matter. Thus, I'd be OK with: Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> I'm also happy to land this (adding Cc: stable) to drm-misc-fixes if nobody has any objections (also happy if someone else wants to land it). I guess the one worry I have is that somehow this could break something for one of the other 8 panels that this driver supports (or it could have bad interactions with the display controller used on a board with one of these panels?). Maybe we should have "Cc: stable" off just to give it extra bake time? ...and maybe even push to drm-misc-next instead of -fixes again to give extra bake time?This thread has gone silent. I'll plan to land the patch in drm-misc-next early next week, maybe Monday, _without_ Ccing stable, so we have the longest possible bake time. If anyone has objections, please yell.Pushed to drm-misc-next:
I've seen this discussion too late. I have now cherry-picked the patch into drm-misc-fixes.
c913cd548993 drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
