Message-ID: <334f8ea8-581e-8711-5f1b-55e596484d9e@suse.de>
Date:   Wed, 1 Feb 2023 12:02:31 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Doug Anderson <dianders@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>
Cc:     Rob Clark <robdclark@...omium.org>,
        Jitao Shi <jitao.shi@...iatek.com>,
        yangcong <yangcong5@...qin.corp-partner.google.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        patches@...ts.linux.dev, Thierry Reding <thierry.reding@...il.com>,
        linux-mediatek@...ts.infradead.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sam Ravnborg <sam@...nborg.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed
 during disable



Am 31.01.23 um 22:27 schrieb Doug Anderson:
> Hi,
> 
> On Thu, Jan 26, 2023 at 4:52 PM Doug Anderson <dianders@...omium.org> wrote:
>>
>> Hi,
>>
>> On Wed, Jan 18, 2023 at 1:34 PM Doug Anderson <dianders@...omium.org> wrote:
>>>
>>> Hi,
>>>
>>> On Thu, Jan 5, 2023 at 7:01 PM Stephen Boyd <swboyd@...omium.org> 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@...qin.corp-partner.google.com>
>>>> Cc: Douglas Anderson <dianders@...omium.org>
>>>> Cc: Jitao Shi <jitao.shi@...iatek.com>
>>>> Cc: Sam Ravnborg <sam@...nborg.org>
>>>> Cc: Rob Clark <robdclark@...omium.org>
>>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
>>>> ---
>>>>   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@...omium.org>
>>>
>>> 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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists