[PATCH] drm: bridge: dw-mipi-dsi: Drop panel_bridge post_disable call

[Jagan Teki]

On Tue, May 16, 2023 at 2:04 PM Marek Vasut <marex at denx.de> wrote:
>
> On 5/16/23 10:25, Jagan Teki wrote:
> > On Tue, May 16, 2023 at 1:47 PM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 5/16/23 10:12, Jagan Teki wrote:
> >>> Hi Marek and Neil,
> >>>
> >>> On Sun, May 14, 2023 at 1:40 AM Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> This panel_bridge post_disable callback is called from the bridge chain now,
> >>>> so drop the explicit call here. This fixes call imbalance, where this driver
> >>>> does not call ->pre_enable, but does call ->post_disable . In case either of
> >>>> the two callbacks implemented in one of the panel or bridge drivers contains
> >>>> any operation with refcounted object, like regulator, this would make kernel
> >>>> complain about the imbalance.
> >>>>
> >>>> This can be triggered e.g. with ST7701 driver, which operates on regulators
> >>>> in its prepare/unprepare callbacks, which are called from pre_enable/post_disable
> >>>> callbacks respectively. The former is called once, the later twice, during
> >>>> entry to suspend.
> >>>>
> >>>> Drop the post_disable call to fix the imbalance.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>> ---
> >>>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
> >>>> Cc: Antonio Borneo <antonio.borneo at foss.st.com>
> >>>> Cc: Daniel Vetter <daniel at ffwll.ch>
> >>>> Cc: David Airlie <airlied at gmail.com>
> >>>> Cc: Jernej Skrabec <jernej.skrabec at gmail.com>
> >>>> Cc: Jonas Karlman <jonas at kwiboo.se>
> >>>> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> >>>> Cc: Marek Vasut <marex at denx.de>
> >>>> Cc: Neil Armstrong <neil.armstrong at linaro.org>
> >>>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> >>>> Cc: Philippe Cornu <philippe.cornu at foss.st.com>
> >>>> Cc: Robert Foss <robert.foss at linaro.org>
> >>>> Cc: Vincent Abriou <vincent.abriou at st.com>
> >>>> Cc: Yannick Fertre <yannick.fertre at foss.st.com>
> >>>> Cc: dri-devel at lists.freedesktop.org
> >>>> ---
> >>>>    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 9 ---------
> >>>>    1 file changed, 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>>> index b2efecf7d1603..63ac972547361 100644
> >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>>> @@ -859,15 +859,6 @@ static void dw_mipi_dsi_bridge_post_atomic_disable(struct drm_bridge *bridge,
> >>>>            */
> >>>>           dw_mipi_dsi_set_mode(dsi, 0);
> >>>>
> >>>> -       /*
> >>>> -        * TODO Only way found to call panel-bridge post_disable &
> >>>> -        * panel unprepare before the dsi "final" disable...
> >>>> -        * This needs to be fixed in the drm_bridge framework and the API
> >>>> -        * needs to be updated to manage our own call chains...
> >>>> -        */
> >>>> -       if (dsi->panel_bridge->funcs->post_disable)
> >>>> -               dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
> >>>> -
> >>>
> >>> If my understanding was correct, the controller set the low-speed DCS
> >>> in pre_enable and high-speed DCS in enable. So I'm thinking this
> >>> explicit post_disable still needs to revert the operation within the
> >>> bridge chain. I didn't test this but trying to understand how the
> >>> existing behaviour is satisfied if we drop this.
> >>
> >> Did I miss a panel_bridge pre_enable call somewhere in the driver ?
> >> Where is it ?
> >
> > Haa, sorry the next bridge pre_enable.  driver setting the
> > command-mode (low-speed) in mode_set so when the next bridge
> > pre_enable is called, low-speed DCS can be sent, then the bridge
> > enable() sets video mode (high-speed). This is where an explicit
> > post_disable would be required, this is what I understood so far.
>
> So, in the end, all is good with this patch or is there anything missing ?

It is not good, explicit post_disable would required for graceful
speed change as it is done in mode_set and enable.

Jagan.