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.
-Doug
