On Tue, May 16, 2023 at 2:04 PM Marek Vasut <ma...@denx.de> wrote: > > On 5/16/23 10:25, Jagan Teki wrote: > > On Tue, May 16, 2023 at 1:47 PM Marek Vasut <ma...@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 <ma...@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 <ma...@denx.de> > >>>> --- > >>>> Cc: Andrzej Hajda <andrzej.ha...@intel.com> > >>>> Cc: Antonio Borneo <antonio.bor...@foss.st.com> > >>>> Cc: Daniel Vetter <dan...@ffwll.ch> > >>>> Cc: David Airlie <airl...@gmail.com> > >>>> Cc: Jernej Skrabec <jernej.skra...@gmail.com> > >>>> Cc: Jonas Karlman <jo...@kwiboo.se> > >>>> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > >>>> Cc: Marek Vasut <ma...@denx.de> > >>>> Cc: Neil Armstrong <neil.armstr...@linaro.org> > >>>> Cc: Philipp Zabel <p.za...@pengutronix.de> > >>>> Cc: Philippe Cornu <philippe.co...@foss.st.com> > >>>> Cc: Robert Foss <robert.f...@linaro.org> > >>>> Cc: Vincent Abriou <vincent.abr...@st.com> > >>>> Cc: Yannick Fertre <yannick.fer...@foss.st.com> > >>>> Cc: dri-devel@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.
