diff mbox series

drm/panel: move some dsi commands from unprepare to disable

Message ID 20230613-panel-dsi-disable-v1-1-5e454e409fa8@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/panel: move some dsi commands from unprepare to disable | expand

Commit Message

Caleb Connolly June 12, 2023, 11:36 p.m. UTC
The commit 007ac0262b0d ("drm/msm/dsi: switch to DRM_PANEL_BRIDGE")
breaks panels which send DSI commands in their .unprepare callbacks.
Migrate to using .disable for that for some SDM845 panels.

Co-developed-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
base-commit: 858fd168a95c5b9669aac8db6c14a9aeab446375

// Caleb (they/them)
---
 drivers/gpu/drm/panel/panel-ebbg-ft8719.c      | 18 +++++++-----------
 drivers/gpu/drm/panel/panel-novatek-nt36672a.c | 11 ++++++++++-
 drivers/gpu/drm/panel/panel-visionox-rm69299.c | 11 ++++++++++-
 3 files changed, 27 insertions(+), 13 deletions(-)

Comments

Stephan Gerhold June 13, 2023, 9:08 p.m. UTC | #1
[added Dmitry to Cc, since he suggested doing this in [1]]

On Tue, Jun 13, 2023 at 12:36:52AM +0100, Caleb Connolly wrote:
> The commit 007ac0262b0d ("drm/msm/dsi: switch to DRM_PANEL_BRIDGE")
> breaks panels which send DSI commands in their .unprepare callbacks.
> Migrate to using .disable for that for some SDM845 panels.
> 
> Co-developed-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/gpu/drm/panel/panel-ebbg-ft8719.c      | 18 +++++++-----------
>  drivers/gpu/drm/panel/panel-novatek-nt36672a.c | 11 ++++++++++-
>  drivers/gpu/drm/panel/panel-visionox-rm69299.c | 11 ++++++++++-
>  3 files changed, 27 insertions(+), 13 deletions(-)

After Dmitry's description in [1] it's still not quite clear to me if
the MSM DSI driver behaves wrong here (perhaps just "differently"?) or
if most of the panel drivers have this set up wrong.

This patch suggests that panel drivers shouldn't send any DSI commands
in .unprepare(). If this is really the case I think this change should
be applied to all panel drivers, not just the ones that happen to be
used with SDM845 devices. There are several others that also send DSI
commands in .unprepare().

I'm still quite confused about what exactly is supposed to be in
(un)prepare and what in enable/disable. I've seen some related
discussion every now and then but it's still quite inconsistent across
different panel drivers... Can someone clarify this?

Thanks!
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/CAA8EJpq_9iC1rkiZVom28Kv_B3QLd4pBgFObxBfSpJ+Xh=Mp1g@mail.gmail.com/
Linus Walleij June 14, 2023, 8:58 p.m. UTC | #2
On Tue, Jun 13, 2023 at 11:08 PM Stephan Gerhold <stephan@gerhold.net> wrote:

> I'm still quite confused about what exactly is supposed to be in
> (un)prepare and what in enable/disable. I've seen some related
> discussion every now and then but it's still quite inconsistent across
> different panel drivers... Can someone clarify this?

It is somewhat clarified in commit 45527d435c5e39b6eec4aa0065a562e7cf05d503
that added the callbacks:

Author: Ajay Kumar <ajaykumar.rs@samsung.com>
Date:   Fri Jul 18 02:13:48 2014 +0530

    drm/panel: add .prepare() and .unprepare() functions

    Panels often require an initialization sequence that consists of three
    steps: a) powering up the panel, b) starting transmission of video data
    and c) enabling the panel (e.g. turn on backlight). This is usually
    necessary to avoid visual glitches at the beginning of video data
    transmission.

    Similarly, the shutdown sequence is typically done in three steps as
    well: a) disable the panel (e.g. turn off backlight), b) cease video
    data transmission and c) power down the panel.

    Currently drivers can only implement .enable() and .disable() functions,
    which is not enough to implement the above sequences. This commit adds a
    second pair of functions, .prepare() and .unprepare() to allow more
    fine-grained control over when the above steps are performed.

    Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
    [treding: rewrite changelog, add kerneldoc]
    Signed-off-by: Thierry Reding <treding@nvidia.com>

My interpretation is that .enable/.disable is for enabling/disabling
backlight and well, make things show up on the display, and that
happens quickly.

prepare/unprepare is for everything else setting up/tearing down
the data transmission pipeline.

In the clock subsystem the enable/disable could be called in fastpath
and prepare/unprepare only from slowpath so e.g an IRQ handler
can gate a simple gated clock. This semantic seems to have nothing
to do with the display semantic. :/

Yours,
Linus Walleij
Neil Armstrong June 15, 2023, 7:49 a.m. UTC | #3
On 14/06/2023 22:58, Linus Walleij wrote:
> On Tue, Jun 13, 2023 at 11:08 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> 
>> I'm still quite confused about what exactly is supposed to be in
>> (un)prepare and what in enable/disable. I've seen some related
>> discussion every now and then but it's still quite inconsistent across
>> different panel drivers... Can someone clarify this?
> 
> It is somewhat clarified in commit 45527d435c5e39b6eec4aa0065a562e7cf05d503
> that added the callbacks:
> 
> Author: Ajay Kumar <ajaykumar.rs@samsung.com>
> Date:   Fri Jul 18 02:13:48 2014 +0530
> 
>      drm/panel: add .prepare() and .unprepare() functions
> 
>      Panels often require an initialization sequence that consists of three
>      steps: a) powering up the panel, b) starting transmission of video data
>      and c) enabling the panel (e.g. turn on backlight). This is usually
>      necessary to avoid visual glitches at the beginning of video data
>      transmission.
> 
>      Similarly, the shutdown sequence is typically done in three steps as
>      well: a) disable the panel (e.g. turn off backlight), b) cease video
>      data transmission and c) power down the panel.
> 
>      Currently drivers can only implement .enable() and .disable() functions,
>      which is not enough to implement the above sequences. This commit adds a
>      second pair of functions, .prepare() and .unprepare() to allow more
>      fine-grained control over when the above steps are performed.
> 
>      Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>      [treding: rewrite changelog, add kerneldoc]
>      Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> My interpretation is that .enable/.disable is for enabling/disabling
> backlight and well, make things show up on the display, and that
> happens quickly.
> 
> prepare/unprepare is for everything else setting up/tearing down
> the data transmission pipeline.
> 
> In the clock subsystem the enable/disable could be called in fastpath
> and prepare/unprepare only from slowpath so e.g an IRQ handler
> can gate a simple gated clock. This semantic seems to have nothing
> to do with the display semantic. :/

It had to do, .prepare is called when the DSI link is at LP11 state
before it has switched to the VIDEO mode, and .unprepare is the
reverse when VIDEO mode has been disabled and before the DSI link
is totally disabled.

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L938

then

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L855

but Doug has started changing this starting with MSM DSI driver, leading to
current panel drivers not working anymore with the current DSI init mode
and requires setting pre_enable_prev_first for only some DSI hosts
who switched out of set_mode().

The DSI init model doesn't fit at all with the atomic bridge model and
some DSI controllers doesn't support the same features like the allwinner
DSI controller not support sending LP commands when in HS video mode
for example.

Neil

> 
> Yours,
> Linus Walleij
Stephan Gerhold June 15, 2023, 8:09 a.m. UTC | #4
On Thu, Jun 15, 2023 at 09:49:27AM +0200, Neil Armstrong wrote:
> On 14/06/2023 22:58, Linus Walleij wrote:
> > On Tue, Jun 13, 2023 at 11:08 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> > 
> > > I'm still quite confused about what exactly is supposed to be in
> > > (un)prepare and what in enable/disable. I've seen some related
> > > discussion every now and then but it's still quite inconsistent across
> > > different panel drivers... Can someone clarify this?
> > 
> > It is somewhat clarified in commit 45527d435c5e39b6eec4aa0065a562e7cf05d503
> > that added the callbacks:
> > 
> > Author: Ajay Kumar <ajaykumar.rs@samsung.com>
> > Date:   Fri Jul 18 02:13:48 2014 +0530
> > 
> >      drm/panel: add .prepare() and .unprepare() functions
> > 
> >      Panels often require an initialization sequence that consists of three
> >      steps: a) powering up the panel, b) starting transmission of video data
> >      and c) enabling the panel (e.g. turn on backlight). This is usually
> >      necessary to avoid visual glitches at the beginning of video data
> >      transmission.
> > 
> >      Similarly, the shutdown sequence is typically done in three steps as
> >      well: a) disable the panel (e.g. turn off backlight), b) cease video
> >      data transmission and c) power down the panel.
> > 
> >      Currently drivers can only implement .enable() and .disable() functions,
> >      which is not enough to implement the above sequences. This commit adds a
> >      second pair of functions, .prepare() and .unprepare() to allow more
> >      fine-grained control over when the above steps are performed.
> > 
> >      Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >      [treding: rewrite changelog, add kerneldoc]
> >      Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > My interpretation is that .enable/.disable is for enabling/disabling
> > backlight and well, make things show up on the display, and that
> > happens quickly.
> > 
> > prepare/unprepare is for everything else setting up/tearing down
> > the data transmission pipeline.
> > 
> > In the clock subsystem the enable/disable could be called in fastpath
> > and prepare/unprepare only from slowpath so e.g an IRQ handler
> > can gate a simple gated clock. This semantic seems to have nothing
> > to do with the display semantic. :/
> 
> It had to do, .prepare is called when the DSI link is at LP11 state
> before it has switched to the VIDEO mode, and .unprepare is the
> reverse when VIDEO mode has been disabled and before the DSI link
> is totally disabled.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L938
> 
> then
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L855
> 
> but Doug has started changing this starting with MSM DSI driver, leading to
> current panel drivers not working anymore with the current DSI init mode
> and requires setting pre_enable_prev_first for only some DSI hosts
> who switched out of set_mode().
> 

Hm, do I understand you correctly that setting
bridge->pre_enable_prev_first / panel->prepare_prev_first should work as
an alternative to $subject patch, at least for the MSM DSI driver? With
it DSI commands should be possible to be sent in .unprepare()?

Thanks,
Stephan
Doug Anderson June 15, 2023, 11:36 p.m. UTC | #5
Hi,

On Thu, Jun 15, 2023 at 12:49 AM Neil Armstrong
<neil.armstrong@linaro.org> wrote:
>
> On 14/06/2023 22:58, Linus Walleij wrote:
> > On Tue, Jun 13, 2023 at 11:08 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> >> I'm still quite confused about what exactly is supposed to be in
> >> (un)prepare and what in enable/disable. I've seen some related
> >> discussion every now and then but it's still quite inconsistent across
> >> different panel drivers... Can someone clarify this?
> >
> > It is somewhat clarified in commit 45527d435c5e39b6eec4aa0065a562e7cf05d503
> > that added the callbacks:
> >
> > Author: Ajay Kumar <ajaykumar.rs@samsung.com>
> > Date:   Fri Jul 18 02:13:48 2014 +0530
> >
> >      drm/panel: add .prepare() and .unprepare() functions
> >
> >      Panels often require an initialization sequence that consists of three
> >      steps: a) powering up the panel, b) starting transmission of video data
> >      and c) enabling the panel (e.g. turn on backlight). This is usually
> >      necessary to avoid visual glitches at the beginning of video data
> >      transmission.
> >
> >      Similarly, the shutdown sequence is typically done in three steps as
> >      well: a) disable the panel (e.g. turn off backlight), b) cease video
> >      data transmission and c) power down the panel.
> >
> >      Currently drivers can only implement .enable() and .disable() functions,
> >      which is not enough to implement the above sequences. This commit adds a
> >      second pair of functions, .prepare() and .unprepare() to allow more
> >      fine-grained control over when the above steps are performed.
> >
> >      Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >      [treding: rewrite changelog, add kerneldoc]
> >      Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> > My interpretation is that .enable/.disable is for enabling/disabling
> > backlight and well, make things show up on the display, and that
> > happens quickly.
> >
> > prepare/unprepare is for everything else setting up/tearing down
> > the data transmission pipeline.
> >
> > In the clock subsystem the enable/disable could be called in fastpath
> > and prepare/unprepare only from slowpath so e.g an IRQ handler
> > can gate a simple gated clock. This semantic seems to have nothing
> > to do with the display semantic. :/
>
> It had to do, .prepare is called when the DSI link is at LP11 state
> before it has switched to the VIDEO mode, and .unprepare is the
> reverse when VIDEO mode has been disabled and before the DSI link
> is totally disabled.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L938
>
> then
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L855
>
> but Doug has started changing this starting with MSM DSI driver, leading to
> current panel drivers not working anymore with the current DSI init mode
> and requires setting pre_enable_prev_first for only some DSI hosts
> who switched out of set_mode().
>
> The DSI init model doesn't fit at all with the atomic bridge model and
> some DSI controllers doesn't support the same features like the allwinner
> DSI controller not support sending LP commands when in HS video mode
> for example.

Summary of the history here as I understand it:

1. Before the switch to DRM_PANEL_BRIDGE, things worked OK.

2. After the switch to DRM_PANEL_BRIDGE, things broke for tc358762.
That led to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup
to modeset time"), which was a little ugly but sorta OK, except ...

3. Moving the DSI host powerup to modeset time broke ps8640. That led
to commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
for parade-ps8640"), which was a hack.

4. We fixed tc358762 using the new "pre_enable_prev_first" in commit
55cac10739d5 ("drm/bridge: tc358762: Set pre_enable_prev_first") and
thus were able to undo moving DSI host powerup to modeset time and
then undo the ps8640 hack. I talk about this a bit more in the message
for commit 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering
up DSI hosts at modeset").

If there are other things that need "pre_enable_prev_first" we could
do that. If I understand Dave Stevenson [1], though, this doesn't hurt
but technically shouldn't be required. He says that "It is documented
that the mipi_dsi_host_ops transfer function should be called with the
host in any state [1], so the host driver is failing there." Even if
it shouldn't be required, though, "pre_enable_prev_first" can still
have a benefit as Dave says [2] because it would mean that the DSI
controller doesn't have to power itself up and down for each
transfer...

If I understand, if the MSM DSI driver did what Dave said (proactive
turn on if someone sends commands) then we'd actually be OK even with
ps8640, since we don't send any commands in the ps8640 pre_enable()
function.

I guess one other point of reference is commit a3ee9e0b57f8
("drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during
disable"). I think Stephen made that change before
"pre_enable_prev_first" was widely available...

Hopefully I summarized all that correctly. If I messed up, please yell.

I guess the tl;dr (summary of my summary) is:

a) Moving panels like this to "pre_enable_prev_first" seems like a
reasonable idea anyway and (presumably) works around the issue.

b) Moving some commands between disable() / post_diable() or
pre_enable() / enable() can also make sense and isn't terrible. As
people have pointed out, there is some vagueness on exactly what
belongs in each.

c) Ideally someone could fix the MSM DSI driver to behave as Dave documented.


[1] https://lore.kernel.org/r/CAPY8ntA=Dq6GFQ3gEOm9PzPyOa9bHTr8JrpXLibnai7xKqRbpQ@mail.gmail.com
[2] https://lore.kernel.org/r/CAPY8ntAUhVB6UtQTeHAcxNW950Ou+NcEoGwk3JnVWLay89_0Nw@mail.gmail.com
Jakob Hauser June 18, 2023, 1:47 p.m. UTC | #6
Hi all,

On 16.06.23 01:36, Doug Anderson wrote:
...
> I guess the tl;dr (summary of my summary) is:
> 
> a) Moving panels like this to "pre_enable_prev_first" seems like a
> reasonable idea anyway and (presumably) works around the issue.
> 
> b) Moving some commands between disable() / post_diable() or
> pre_enable() / enable() can also make sense and isn't terrible. As
> people have pointed out, there is some vagueness on exactly what
> belongs in each.
> 
> c) Ideally someone could fix the MSM DSI driver to behave as Dave documented.
...
Actually I don't want to disturb the discussion here. Still I would like 
to point out that option a) "pre_enable_prev_first" doesn't seem to work 
well yet. On my device samsung-serranove with panel 
samsung-s6e88a0-ams427ap24 (not in mainline, [1]), when applying 
"pre_enable_prev_first" I notice two issues.

1) According to commig 4fb912e5e190 ("drm/bridge: Introduce 
pre_enable_prev_first to alter bridge init order") [2] it's supposed to 
reverse the order in "post_disable" as well. That doesn't work on my 
device, the "post_disable" order doesn't get changed by 
"pre_enable_prev_first".

2) When enabling the panel, for each mipi_dsi_dcs_... command there is 
an error in dmesg "msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] 
*ERROR* wait for video done timed out". The panel works well 
nonetheless. However, before commit 007ac0262b0d ("drm/msm/dsi: switch 
to DRM_PANEL_BRIDGE") these errors didn't show up.

I tried to get more insight in the order of issue 1). I added additional 
debug markers in drivers/gpu/drm/drm_bridge.c (original state linked in 
[3]) and got the following behavior. To me it's rather hard to 
understand the decision-making, to be honest. Both in "pre_enable" as 
well as in "post_disable" first the host and then the panel are 
processed. At "post_disable" it should be the other way around.

I'm currently on kernel 6.4-rc4 with cherry-picked commits from 
linux-next 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up 
DSI hosts at modeset") and d8dd416cb420 ("drm/msm/dsi: More properly 
handle errors in regards to dsi_mgr_bridge_power_on()") and added 
"pre_enable_prev_first" to the panel driver. Device is samsung-serranove 
in X11 environment.

At boot:
--------
chain_pre_enable: bridge at beginning: 'host'
chain_pre_enable: iter in the list loop at the beginning: 'panel'
chain_pre_enable: next if iter prev_first: 'panel'
chain_pre_enable: limit if iter prev_first: 'host'
chain_pre_enable: next in list loop from_reverse: 'panel'
chain_pre_enable: next in list loop from_reverse: 'host'
chain_pre_enable: break because 'next == bridge' 'host'
chain_pre_enable: marker at end of first list loop block
chain_pre_enable: iter at end of first list loop block: 'panel'
chain_pre_enable: next at end of first list loop block: 'host'
chain_pre_enable: limit at end of first list loop block: 'host'
chain_pre_enable: next in 2nd list loop block: 'host'
chain_pre_enable: next is not iter, call pre_enable within 2nd list loop
                   block: 'host'
call_pre_enable: pre_enable bridge 'host'
call_pre_enable: inside 'else if', do pre_enable
chain_pre_enable: next in 2nd list loop block: 'panel'
chain_pre_enable: break because 'next == iter': 'panel'
chain_pre_enable: marker at end of 2nd list loop block
chain_pre_enable: iter after 2nd list loop block, call pre_enable:
                  'panel'
call_pre_enable: pre_enable bridge 'panel'
call_pre_enable: inside 'if'
call_pre_enable: passed second 'if', do atomic_pre_enable
msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] *ERROR* wait for video
                      done timed out
msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] *ERROR* wait for video
                      done timed out
...
chain_pre_enable: if iter is prev_first...: 'panel'
chain_pre_enable: ... change iter to 'limit': 'host'
chain_pre_enable: iter at the end of function: 'host'
chain_pre_enable: bridge at the end of function: 'host'
chain_pre_enable: break if iter is bridge at the end of function: 'host'

Then the panel get's turned off:
--------------------------------
chain_post_disable: bridge at beginning: 'host'
chain_post_disable: bridge of the list loop at the beginning: 'host'
chain_post_disable: if entry is not last, set 'next' to next entry:
                     'panel'
chain_post_disable: if 'next' is prev_first, set 'limit' to next:
                     'panel'
chain_post_disable: loop through the list of 'next': 'panel'
chain_post_disable: if 'next' is prev_first, change 'next' to: 'host'
chain_post_disable: ... and 'limit' to the same: 'host'
chain_post_disable: new loop through list of 'next' in reverse order:
                     'host'
chain_post_disable: break because 'next == bridge' 'host'
chain_post_disable: after !list_is_last block, call post_disable for
                     bridge: 'host'
call_post_disable: post_disable bridge: 'host'
call_post_disable: inside 'else if', do post_disable
chain_post_disable: if limit available, set 'bridge = limit': 'host'
chain_post_disable: bridge of the list loop at the beginning: 'panel'
chain_post_disable: after !list_is_last block, call post_disable for
                     bridge: 'panel'
call_post_disable: post_disable bridge: 'panel'
call_post_disable: inside 'if'
call_post_disable: passed second 'if', do atomic_post_disable
panel-s6e88a0-ams427ap24 1a98000.dsi.0: Failed to set display off: -22
panel-s6e88a0-ams427ap24 1a98000.dsi.0: Failed to un-initialize panel:
                                         -22

It's not my idea to go into detailed debugging. I just wanted to say 
that option a) "pre_enable_prev_first" currently doesn't work well, at 
least on my device. I would assume it affects other devices too. Also I 
didn't want to delay Caleb's patch review. However, it might be related 
if the "pre_enable_prev_first" approach doesn't work on disable.

[1] 
https://github.com/msm8916-mainline/linux/blob/msm8916/6.4-rc4/drivers/gpu/drm/panel/msm8916-generated/panel-samsung-s6e88a0-ams427ap24.c
[2] 
https://github.com/torvalds/linux/commit/4fb912e5e19075874379cfcf074d90bd51ebf8ea
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_bridge.c?h=v6.4-rc4

Kind regards,
Jakob
Jakob Hauser June 18, 2023, 1:52 p.m. UTC | #7
A typo in my previous e-mail:

On 18.06.23 15:47, Jakob Hauser wrote:
...
> Then the panel get's turned off:
> --------------------------------

... should be "When..." instead of "Then...".
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
index e85d63a176d0..b40a27558e7c 100644
--- a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
+++ b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
@@ -87,22 +87,22 @@  static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
 	return 0;
 }
 
-static int ebbg_ft8719_off(struct ebbg_ft8719 *ctx)
+static int ebbg_ft8719_disable(struct drm_panel *panel)
 {
-	struct mipi_dsi_device *dsi = ctx->dsi;
-	struct device *dev = &dsi->dev;
+	struct ebbg_ft8719 *ctx = to_ebbg_ft8719(panel);
+	struct device *dev = &ctx->dsi->dev;
 	int ret;
 
-	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
+	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
 	if (ret < 0) {
 		dev_err(dev, "Failed to set display off: %d\n", ret);
 		return ret;
 	}
 	usleep_range(10000, 11000);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
 	if (ret < 0) {
 		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
 		return ret;
@@ -137,13 +137,8 @@  static int ebbg_ft8719_prepare(struct drm_panel *panel)
 static int ebbg_ft8719_unprepare(struct drm_panel *panel)
 {
 	struct ebbg_ft8719 *ctx = to_ebbg_ft8719(panel);
-	struct device *dev = &ctx->dsi->dev;
 	int ret;
 
-	ret = ebbg_ft8719_off(ctx);
-	if (ret < 0)
-		dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
-
 	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
@@ -188,6 +183,7 @@  static int ebbg_ft8719_get_modes(struct drm_panel *panel,
 
 static const struct drm_panel_funcs ebbg_ft8719_panel_funcs = {
 	.prepare = ebbg_ft8719_prepare,
+	.disable = ebbg_ft8719_disable,
 	.unprepare = ebbg_ft8719_unprepare,
 	.get_modes = ebbg_ft8719_get_modes,
 };
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
index 73bcffa1e0c1..2e1a87001479 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
@@ -115,7 +115,7 @@  static int nt36672a_panel_power_off(struct drm_panel *panel)
 	return ret;
 }
 
-static int nt36672a_panel_unprepare(struct drm_panel *panel)
+static int nt36672a_panel_disable(struct drm_panel *panel)
 {
 	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
 	int ret;
@@ -144,6 +144,14 @@  static int nt36672a_panel_unprepare(struct drm_panel *panel)
 	/* 0x3C = 60ms delay */
 	msleep(60);
 
+	return ret;
+}
+
+static int nt36672a_panel_unprepare(struct drm_panel *panel)
+{
+	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
+	int ret;
+
 	ret = nt36672a_panel_power_off(panel);
 	if (ret < 0)
 		dev_err(panel->dev, "power_off failed ret = %d\n", ret);
@@ -255,6 +263,7 @@  static int nt36672a_panel_get_modes(struct drm_panel *panel,
 }
 
 static const struct drm_panel_funcs panel_funcs = {
+	.disable = nt36672a_panel_disable,
 	.unprepare = nt36672a_panel_unprepare,
 	.prepare = nt36672a_panel_prepare,
 	.get_modes = nt36672a_panel_get_modes,
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
index ec228c269146..9dc17afbe540 100644
--- a/drivers/gpu/drm/panel/panel-visionox-rm69299.c
+++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
@@ -59,7 +59,7 @@  static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
 	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
 }
 
-static int visionox_rm69299_unprepare(struct drm_panel *panel)
+static int visionox_rm69299_disable(struct drm_panel *panel)
 {
 	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
 	int ret;
@@ -78,6 +78,14 @@  static int visionox_rm69299_unprepare(struct drm_panel *panel)
 		dev_err(ctx->panel.dev, "enter_sleep cmd failed ret = %d\n", ret);
 	}
 
+	return ret;
+}
+
+static int visionox_rm69299_unprepare(struct drm_panel *panel)
+{
+	struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+	int ret;
+
 	ret = visionox_rm69299_power_off(ctx);
 
 	ctx->prepared = false;
@@ -184,6 +192,7 @@  static int visionox_rm69299_get_modes(struct drm_panel *panel,
 }
 
 static const struct drm_panel_funcs visionox_rm69299_drm_funcs = {
+	.disable = visionox_rm69299_disable,
 	.unprepare = visionox_rm69299_unprepare,
 	.prepare = visionox_rm69299_prepare,
 	.get_modes = visionox_rm69299_get_modes,