[PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

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@huaqin.corp-partner.google.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Jitao Shi <jitao.shi@mediatek.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.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,

base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
https://chromeos.dev

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Sam Ravnborg]

Hi Stephen.

On Thu, Jan 05, 2023 at 07:01:08PM -0800, Stephen Boyd 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.
The pattern we use in several (but not all) panel drivers are that
errors in unprepare/disable are logged but the function do not skip
the remainder of the function. This is to avoid that we do not disable
power supplies etc.

For this case we could ask ourself if the display needs to enter sleep
mode right before we disable the regulator. But if the regulator is
fixed, so the disable has no effect, this seems OK.

Please fix the unprepare to not jump out early, on top of or together
with the other fix.

	Sam

> 
> 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@huaqin.corp-partner.google.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Jitao Shi <jitao.shi@mediatek.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.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,
> 
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> -- 
> https://chromeos.dev

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

Quoting Sam Ravnborg (2023-01-07 12:28:41)
> On Thu, Jan 05, 2023 at 07:01:08PM -0800, Stephen Boyd 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.
>
> The pattern we use in several (but not all) panel drivers are that
> errors in unprepare/disable are logged but the function do not skip
> the remainder of the function. This is to avoid that we do not disable
> power supplies etc.

Ah that would have made it so I didn't see a problem. But I wonder if
the panel gets borked if you don't disable it via DSI writes before
yanking the power?

>
> For this case we could ask ourself if the display needs to enter sleep
> mode right before we disable the regulator. But if the regulator is
> fixed, so the disable has no effect, this seems OK.

What do you mean by fixed?

>
> Please fix the unprepare to not jump out early, on top of or together
> with the other fix.

After this patch the unprepare only bails out early if the bool
'prepared' flag isn't set. Are you suggesting to get rid of that flag
and unconditionally disable the regulators? Is it possible for the
unprepare to be called multiple times without a balanced call to
prepare?

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Sam Ravnborg]

Hi Stephen,
On Tue, Jan 10, 2023 at 11:29:41AM -0800, Stephen Boyd wrote:
> Quoting Sam Ravnborg (2023-01-07 12:28:41)
> 
> >
> > For this case we could ask ourself if the display needs to enter sleep
> > mode right before we disable the regulator. But if the regulator is
> > fixed, so the disable has no effect, this seems OK.
> 
> What do you mean by fixed?
What I tried to say here is if we have a fixed regulator - or in others
words a supply voltage we cannot turn off, then entering sleep mode is
important to reduce power consumption.
But any sane design where power consumption is a concern will have the
possibility to turn off the power anyway.

> 
> >
> > Please fix the unprepare to not jump out early, on top of or together
> > with the other fix.
> 
> After this patch the unprepare only bails out early if the bool
> 'prepared' flag isn't set.
OK, then everything is fine.

	Sam

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Dave Stevenson]

Hi Stephen

On Fri, 6 Jan 2023 at 03:01, Stephen Boyd <swboyd@chromium.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.

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.

This sounds like the same issue I was addressing in adding the
prepare_prev_first flag to drm_panel, and pre_enable_prev_first to
drm_bridge via [2].
Defining prepare_prev_first for your panel would result in the host
pre_enable being called before the panel prepare, and therefore the
transfer calls from boe_panel_init_dcs_cmd boe_panel_prepare won't be
relying on the DSI host powering up early. It will also call the panel
unprepare before the DSI host bridge post_disable is called, and
therefore the DSI host will still be powered up and the transfer won't
fail.

Actually I've just noted the comment at [3]. [2] is that framework fix
that means that the magic workaround isn't required, but it will
require this panel to opt in to the ordering change.

Cheers.
  Dave

[1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops
[2] https://patchwork.freedesktop.org/series/100252/
[3] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/dsi/dsi_manager.c#L47

> 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@huaqin.corp-partner.google.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Jitao Shi <jitao.shi@mediatek.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.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,
>
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> --
> https://chromeos.dev
>

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

Quoting Sam Ravnborg (2023-01-13 06:52:09)
> Hi Stephen,
> On Tue, Jan 10, 2023 at 11:29:41AM -0800, Stephen Boyd wrote:
> > Quoting Sam Ravnborg (2023-01-07 12:28:41)
> >
> > >
> > > For this case we could ask ourself if the display needs to enter sleep
> > > mode right before we disable the regulator. But if the regulator is
> > > fixed, so the disable has no effect, this seems OK.
> >
> > What do you mean by fixed?
> What I tried to say here is if we have a fixed regulator - or in others
> words a supply voltage we cannot turn off, then entering sleep mode is
> important to reduce power consumption.
> But any sane design where power consumption is a concern will have the
> possibility to turn off the power anyway.

Ok got it!

>
> >
> > >
> > > Please fix the unprepare to not jump out early, on top of or together
> > > with the other fix.
> >
> > After this patch the unprepare only bails out early if the bool
> > 'prepared' flag isn't set.
> OK, then everything is fine.
>

Doug pointed out that enable isn't symmetric because it doesn't do the
DSI writes. I've updated the patch and I'll send a v2.

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

Quoting Dave Stevenson (2023-01-13 08:27:29)
> Hi Stephen
>
> On Fri, 6 Jan 2023 at 03:01, Stephen Boyd <swboyd@chromium.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.
>
> 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.

Thanks for the info! It says "Drivers that need the underlying device to
be powered to perform these operations will first need to make sure it’s
been properly enabled." Does that mean the panel driver needs to make
sure the underlying dsi host device is powered? The sentence is too
ambiguous for me to understand what 'drivers' and 'underlying device'
are.

>
> This sounds like the same issue I was addressing in adding the
> prepare_prev_first flag to drm_panel, and pre_enable_prev_first to
> drm_bridge via [2].
> Defining prepare_prev_first for your panel would result in the host
> pre_enable being called before the panel prepare, and therefore the
> transfer calls from boe_panel_init_dcs_cmd boe_panel_prepare won't be
> relying on the DSI host powering up early. It will also call the panel
> unprepare before the DSI host bridge post_disable is called, and
> therefore the DSI host will still be powered up and the transfer won't
> fail.
>
> Actually I've just noted the comment at [3]. [2] is that framework fix
> that means that the magic workaround isn't required, but it will
> require this panel to opt in to the ordering change.

Cool. Glad that we can clean that up with your series.

Is it wrong to split unprepare to a disable and unprepare phase? I'm not
super keen on fixing 6.1 stable kernels by opting this driver into the
ordering change. Splitting the phase into two is small and simple and
works. I suspect changing the ordering may uncover more bugs, or be a
larger task. I'd be glad to test that series[2] from you to get rid of
[3].

>
>
> [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops
> [2] https://patchwork.freedesktop.org/series/100252/
> [3] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/dsi/dsi_manager.c#L47
>

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Dave Stevenson]

Hi Stephen

On Fri, 13 Jan 2023 at 21:12, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dave Stevenson (2023-01-13 08:27:29)
> > Hi Stephen
> >
> > On Fri, 6 Jan 2023 at 03:01, Stephen Boyd <swboyd@chromium.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.
> >
> > 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.
>
> Thanks for the info! It says "Drivers that need the underlying device to
> be powered to perform these operations will first need to make sure it’s
> been properly enabled." Does that mean the panel driver needs to make
> sure the underlying dsi host device is powered? The sentence is too
> ambiguous for me to understand what 'drivers' and 'underlying device'
> are.

The DSI host driver (ie in your case something under
drivers/gpu/drm/msm/dsi) should ensure that a transfer can be made,
regardless of state.

I must say that this has been documented as the case, but doesn't
necessarily reflect reality in a number of drivers.

> >
> > This sounds like the same issue I was addressing in adding the
> > prepare_prev_first flag to drm_panel, and pre_enable_prev_first to
> > drm_bridge via [2].
> > Defining prepare_prev_first for your panel would result in the host
> > pre_enable being called before the panel prepare, and therefore the
> > transfer calls from boe_panel_init_dcs_cmd boe_panel_prepare won't be
> > relying on the DSI host powering up early. It will also call the panel
> > unprepare before the DSI host bridge post_disable is called, and
> > therefore the DSI host will still be powered up and the transfer won't
> > fail.
> >
> > Actually I've just noted the comment at [3]. [2] is that framework fix
> > that means that the magic workaround isn't required, but it will
> > require this panel to opt in to the ordering change.
>
> Cool. Glad that we can clean that up with your series.
>
> Is it wrong to split unprepare to a disable and unprepare phase? I'm not
> super keen on fixing 6.1 stable kernels by opting this driver into the
> ordering change. Splitting the phase into two is small and simple and
> works. I suspect changing the ordering may uncover more bugs, or be a
> larger task. I'd be glad to test that series[2] from you to get rid of
> [3].

Ah, I hadn't realised it was a regression in a released kernel :(

Splitting into a disable and unprepare is totally fine. Normally
disable would normally disable the panel and backlight (probably by
drm_panel before the panel disable call), with unprepare disabling
power and clocks

Do note that AIUI you will be telling the panel to enter sleep mode
whilst video is still being transmitted. That should be safe as the
panel has to still be partially active to handle an exit sleep mode
command, but actually powering hardware down at that point could cause
DSI bus arbitration errors as clock or data lanes could be pulled down
when the host is trying to adopt HS or LP11.

  Dave

> >
> >
> > [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops
> > [2] https://patchwork.freedesktop.org/series/100252/
> > [3] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/dsi/dsi_manager.c#L47
> >

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

Quoting Dave Stevenson (2023-01-16 06:11:02)
> Hi Stephen
>
> On Fri, 13 Jan 2023 at 21:12, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> >
> > Thanks for the info! It says "Drivers that need the underlying device to
> > be powered to perform these operations will first need to make sure it’s
> > been properly enabled." Does that mean the panel driver needs to make
> > sure the underlying dsi host device is powered? The sentence is too
> > ambiguous for me to understand what 'drivers' and 'underlying device'
> > are.
>
> The DSI host driver (ie in your case something under
> drivers/gpu/drm/msm/dsi) should ensure that a transfer can be made,
> regardless of state.
>
> I must say that this has been documented as the case, but doesn't
> necessarily reflect reality in a number of drivers.

Alright, thanks for the clarification.

> >
> > Cool. Glad that we can clean that up with your series.
> >
> > Is it wrong to split unprepare to a disable and unprepare phase? I'm not
> > super keen on fixing 6.1 stable kernels by opting this driver into the
> > ordering change. Splitting the phase into two is small and simple and
> > works. I suspect changing the ordering may uncover more bugs, or be a
> > larger task. I'd be glad to test that series[2] from you to get rid of
> > [3].
>
> Ah, I hadn't realised it was a regression in a released kernel :(
>
> Splitting into a disable and unprepare is totally fine. Normally
> disable would normally disable the panel and backlight (probably by
> drm_panel before the panel disable call), with unprepare disabling
> power and clocks
>
> Do note that AIUI you will be telling the panel to enter sleep mode
> whilst video is still being transmitted. That should be safe as the
> panel has to still be partially active to handle an exit sleep mode
> command, but actually powering hardware down at that point could cause
> DSI bus arbitration errors as clock or data lanes could be pulled down
> when the host is trying to adopt HS or LP11.
>

Ok. I don't think I'm running into that issue, but I have run into a
different issue. I tried to split the prepare phase into enable and
prepare with the DSI writes in the enable to make things symmetric but
that totally failed. Now I get lots of timeouts when enabling the panel.

This patch is still the best fix I have. Maybe with your series we can
combine the unprepare and disable ops together again (i.e. revert this)
so that power is removed immediately after sending the DSI commands?  Or
is that not enough to avoid the DSI bus arbitration problems you're
talking about? When is the host adopting HS or LP11 with regards to the
bridge ops?

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Stephen Boyd]

Quoting Stephen Boyd (2023-01-13 12:49:43)
> Quoting Sam Ravnborg (2023-01-13 06:52:09)
> > Hi Stephen,
> > On Tue, Jan 10, 2023 at 11:29:41AM -0800, Stephen Boyd wrote:
> > >
> > > After this patch the unprepare only bails out early if the bool
> > > 'prepared' flag isn't set.
> > OK, then everything is fine.
> >
>
> Doug pointed out that enable isn't symmetric because it doesn't do the
> DSI writes. I've updated the patch and I'll send a v2.

Turns out that splitting prepare into prepare and enable breaks the
display even more for me. For now this is the best patch I have and it
fixes the regression I see in v6.1 kernels. Can I get your Reviewed-by
on this patch?

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Doug Anderson]

Hi,

On Thu, Jan 5, 2023 at 7:01 PM Stephen Boyd <swboyd@chromium.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@huaqin.corp-partner.google.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Jitao Shi <jitao.shi@mediatek.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.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@chromium.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?


In any case, I still wanted to look closer. I'll repeat my constant
refrain that I'm no expert here, so call me out if I say anything too
stupid.

As far as I can tell, boe_panel_enter_sleep_mode() does a bunch of
things that have no true opposite in the driver. Let me paste it here
for reference since Stephen's patch didn't touch it:

> static int boe_panel_enter_sleep_mode(struct boe_panel *boe)
> {
>     struct mipi_dsi_device *dsi = boe->dsi;
>     int ret;
>
>     dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

The above line is particularly concerning. Since there's no opposite
anywhere, I'm going to assume that the panels in this file that use
"LPM" end up not using LPM after the first suspend/resume cycle.
Almost all other panel drivers that clear this flag only do so
temporarily. Seems like maybe this was an oversight in the initial
commit a869b9db7adf ("drm/panel: support for boe tv101wum-nl6 wuxga
dsi video mode panel")? Nothing new, but maybe we should fix it?


>     ret = mipi_dsi_dcs_set_display_off(dsi);
>     if (ret < 0)
>         return ret;
>
>     ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>     if (ret < 0)
>         return ret;

The first of these two (set_display_off) seems quite safe and matches
well with the concept of "disable". We're basically blacking the
screen, I imagine. I then wondered: where do we turn the display on?
It seems like there should be a call to mipi_dsi_dcs_set_display_on(),
right?

Digging a little, there actually is, at least for 3 of the 9 panels
that this driver supports. It's hidden in the giant blob of "DCS"
commands. Specifically, this magic sequence:

_INIT_DELAY_CMD(...),
_INIT_DCS_CMD(0x11),
_INIT_DELAY_CMD(...),
_INIT_DCS_CMD(0x29),
_INIT_DELAY_CMD(...),

The 0x11 there is mipi_dsi_dcs_exit_sleep_mode() and the 0x29 there is
mipi_dsi_dcs_set_display_on()

Now, I'd have to ask why the other 6 of 9 panels _don't_ have those
commands and how the display gets turned on / pulled out of sleep
mode. Maybe they just come up that way? It does feel likely that we
could probably:

a) Parameterize the delays

b) Remove the hardcoded 0x11 and 0x29 from the dcs command blob and
add calls to mipi_dsi_dcs_exit_sleep_mode() /
mipi_dsi_dcs_set_display_on()

c) At least add the mipi_dsi_dcs_set_display_on() to the "enable" call
and then see if mipi_dsi_dcs_exit_sleep_mode() worked in enable() or
if that was important to keep in prepare().

Even if we eventually are able to revert Stephen's patch once we have
the power sequence ordering series, it still might be nice to make the
enable/exit of sleep mode explicit instead of hidden in the DCS
command blob.

-Doug

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Doug Anderson]

Hi,

On Wed, Jan 18, 2023 at 1:34 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jan 5, 2023 at 7:01 PM Stephen Boyd <swboyd@chromium.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@huaqin.corp-partner.google.com>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Jitao Shi <jitao.shi@mediatek.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Rob Clark <robdclark@chromium.org>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.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@chromium.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.

-Doug

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Doug Anderson]

Hi,

On Thu, Jan 26, 2023 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Jan 18, 2023 at 1:34 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Jan 5, 2023 at 7:01 PM Stephen Boyd <swboyd@chromium.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@huaqin.corp-partner.google.com>
> > > Cc: Douglas Anderson <dianders@chromium.org>
> > > Cc: Jitao Shi <jitao.shi@mediatek.com>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Rob Clark <robdclark@chromium.org>
> > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.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@chromium.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:

c913cd548993 drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed
during disable

Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6451 bytes --]



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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]