[PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Douglas Anderson]

Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
chip to fail to turn the display back on after it turns off.

Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
handle the new power sequence. The Linux driver has almost nothing in
it and most of the logic for this bridge chip is in black-box firmware
that the bridge chip uses.

Also unfortunately, reverting the patch will break "tc358762".

The long term solution here is probably Dave Stevenson's series [1]
that would give more flexibility. However, that is likely not a quick
fix.

For the short term, let's introduce a module parameter that selects
between the two behaviors. This is a short term hack but at least can
keep both users working. We'll default the value of the module
parameter to the old behavior. Given that the old behavior has existed
for longer it's probably a safer default.

[1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com

Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- No longer a revert; now a module parameter.

Changes in v2:
- Remove the mud from my face.

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 50b987658b1f..2bf4123ef5df 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
 #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
 #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
 
+bool early_poweron;
+MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
+module_param(early_poweron, bool, 0600);
+
 static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
 {
 	return msm_dsim_glb.dsi[id];
@@ -389,6 +393,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
+	if (!early_poweron)
+		dsi_mgr_bridge_power_on(bridge);
+
 	/* Always call panel functions once, because even for dual panels,
 	 * there is only one drm_panel instance.
 	 */
@@ -570,7 +577,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
 	if (is_bonded_dsi && other_dsi)
 		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
 
-	dsi_mgr_bridge_power_on(bridge);
+	if (early_poweron)
+		dsi_mgr_bridge_power_on(bridge);
 }
 
 static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
-- 
2.36.0.550.gb090851708-goog

[PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Douglas Anderson]

Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
chip to fail to turn the display back on after it turns off.

Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
handle the new power sequence. The Linux driver has almost nothing in
it and most of the logic for this bridge chip is in black-box firmware
that the bridge chip uses.

Also unfortunately, reverting the patch will break "tc358762".

The long term solution here is probably Dave Stevenson's series [1]
that would give more flexibility. However, that is likely not a quick
fix.

For the short term, let's introduce a module parameter that selects
between the two behaviors. This is a short term hack but at least can
keep both users working. We'll default the value of the module
parameter to the old behavior. Given that the old behavior has existed
for longer it's probably a safer default.

[1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com

Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- No longer a revert; now a module parameter.

Changes in v2:
- Remove the mud from my face.

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 50b987658b1f..2bf4123ef5df 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
 #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
 #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
 
+bool early_poweron;
+MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
+module_param(early_poweron, bool, 0600);
+
 static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
 {
 	return msm_dsim_glb.dsi[id];
@@ -389,6 +393,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
+	if (!early_poweron)
+		dsi_mgr_bridge_power_on(bridge);
+
 	/* Always call panel functions once, because even for dual panels,
 	 * there is only one drm_panel instance.
 	 */
@@ -570,7 +577,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
 	if (is_bonded_dsi && other_dsi)
 		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
 
-	dsi_mgr_bridge_power_on(bridge);
+	if (early_poweron)
+		dsi_mgr_bridge_power_on(bridge);
 }
 
 static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Dmitry Baryshkov]

On 12/05/2022 23:52, Douglas Anderson wrote:
> Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
> chip to fail to turn the display back on after it turns off.
> 
> Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
> handle the new power sequence. The Linux driver has almost nothing in
> it and most of the logic for this bridge chip is in black-box firmware
> that the bridge chip uses.
> 
> Also unfortunately, reverting the patch will break "tc358762".
> 
> The long term solution here is probably Dave Stevenson's series [1]
> that would give more flexibility. However, that is likely not a quick
> fix.
> 
> For the short term, let's introduce a module parameter that selects
> between the two behaviors. This is a short term hack but at least can
> keep both users working. We'll default the value of the module
> parameter to the old behavior. Given that the old behavior has existed
> for longer it's probably a safer default.
> 
> [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com
> 
> Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Two minor issues below.

> ---
> 
> Changes in v3:
> - No longer a revert; now a module parameter.
> 
> Changes in v2:
> - Remove the mud from my face.
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 50b987658b1f..2bf4123ef5df 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
>   #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
>   #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
>   
> +bool early_poweron;
> +MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
> +module_param(early_poweron, bool, 0600);

Nit: dsi_early_poweron (to be clear that it related to DSI only).

I thought about suggesting 'dsi_no_early_poweron' instead to catch 
possible issues with other bridges. But... I think with Dave's series 
will have to enable bridges one by one, so it doesn't make real sense.

> +
>   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>   {
>   	return msm_dsim_glb.dsi[id];
> @@ -389,6 +393,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> +	if (!early_poweron)
> +		dsi_mgr_bridge_power_on(bridge);
> +
>   	/* Always call panel functions once, because even for dual panels,
>   	 * there is only one drm_panel instance.
>   	 */
> @@ -570,7 +577,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>   	if (is_bonded_dsi && other_dsi)
>   		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
>   
> -	dsi_mgr_bridge_power_on(bridge);
> +	if (early_poweron)
> +		dsi_mgr_bridge_power_on(bridge);
>   }
>   
>   static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,


-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Dmitry Baryshkov]

On 12/05/2022 23:52, Douglas Anderson wrote:
> Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
> chip to fail to turn the display back on after it turns off.
> 
> Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
> handle the new power sequence. The Linux driver has almost nothing in
> it and most of the logic for this bridge chip is in black-box firmware
> that the bridge chip uses.
> 
> Also unfortunately, reverting the patch will break "tc358762".
> 
> The long term solution here is probably Dave Stevenson's series [1]
> that would give more flexibility. However, that is likely not a quick
> fix.
> 
> For the short term, let's introduce a module parameter that selects
> between the two behaviors. This is a short term hack but at least can
> keep both users working. We'll default the value of the module
> parameter to the old behavior. Given that the old behavior has existed
> for longer it's probably a safer default.
> 
> [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com
> 
> Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Two minor issues below.

> ---
> 
> Changes in v3:
> - No longer a revert; now a module parameter.
> 
> Changes in v2:
> - Remove the mud from my face.
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 50b987658b1f..2bf4123ef5df 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
>   #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
>   #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
>   
> +bool early_poweron;
> +MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
> +module_param(early_poweron, bool, 0600);

Nit: dsi_early_poweron (to be clear that it related to DSI only).

I thought about suggesting 'dsi_no_early_poweron' instead to catch 
possible issues with other bridges. But... I think with Dave's series 
will have to enable bridges one by one, so it doesn't make real sense.

> +
>   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>   {
>   	return msm_dsim_glb.dsi[id];
> @@ -389,6 +393,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> +	if (!early_poweron)
> +		dsi_mgr_bridge_power_on(bridge);
> +
>   	/* Always call panel functions once, because even for dual panels,
>   	 * there is only one drm_panel instance.
>   	 */
> @@ -570,7 +577,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>   	if (is_bonded_dsi && other_dsi)
>   		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
>   
> -	dsi_mgr_bridge_power_on(bridge);
> +	if (early_poweron)
> +		dsi_mgr_bridge_power_on(bridge);
>   }
>   
>   static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,


-- 
With best wishes
Dmitry

Re: [PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Doug Anderson]

Hi,

On Thu, May 12, 2022 at 1:59 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/05/2022 23:52, Douglas Anderson wrote:
> > Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
> > chip to fail to turn the display back on after it turns off.
> >
> > Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
> > handle the new power sequence. The Linux driver has almost nothing in
> > it and most of the logic for this bridge chip is in black-box firmware
> > that the bridge chip uses.
> >
> > Also unfortunately, reverting the patch will break "tc358762".
> >
> > The long term solution here is probably Dave Stevenson's series [1]
> > that would give more flexibility. However, that is likely not a quick
> > fix.
> >
> > For the short term, let's introduce a module parameter that selects
> > between the two behaviors. This is a short term hack but at least can
> > keep both users working. We'll default the value of the module
> > parameter to the old behavior. Given that the old behavior has existed
> > for longer it's probably a safer default.
> >
> > [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com
> >
> > Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Two minor issues below.

I only saw one, so hopefully I didn't miss another request. ;-)


> > ---
> >
> > Changes in v3:
> > - No longer a revert; now a module parameter.
> >
> > Changes in v2:
> > - Remove the mud from my face.
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 50b987658b1f..2bf4123ef5df 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
> >   #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
> >   #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
> >
> > +bool early_poweron;
> > +MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
> > +module_param(early_poweron, bool, 0600);
>
> Nit: dsi_early_poweron (to be clear that it related to DSI only).
>
> I thought about suggesting 'dsi_no_early_poweron' instead to catch
> possible issues with other bridges. But... I think with Dave's series
> will have to enable bridges one by one, so it doesn't make real sense.

I made the change you requested and was about to send out a v4 when
offline Rob pointed out a better way that seems to work. I can use the
`compatible` of the next bridge. I've sent out v4 as per Rob's
suggestion:

https://lore.kernel.org/r/20220512145954.v4.1.Ia196e35ad985059e77b038a41662faae9e26f411@changeid

-Doug

Re: [PATCH v3] drm/msm/dsi: only powerup at modeset time if "early_poweron" modparam

[Doug Anderson]

Hi,

On Thu, May 12, 2022 at 1:59 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/05/2022 23:52, Douglas Anderson wrote:
> > Commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time") caused sc7180 Chromebooks that use the parade-ps8640 bridge
> > chip to fail to turn the display back on after it turns off.
> >
> > Unfortunately, it doesn't look easy to fix the parade-ps8640 driver to
> > handle the new power sequence. The Linux driver has almost nothing in
> > it and most of the logic for this bridge chip is in black-box firmware
> > that the bridge chip uses.
> >
> > Also unfortunately, reverting the patch will break "tc358762".
> >
> > The long term solution here is probably Dave Stevenson's series [1]
> > that would give more flexibility. However, that is likely not a quick
> > fix.
> >
> > For the short term, let's introduce a module parameter that selects
> > between the two behaviors. This is a short term hack but at least can
> > keep both users working. We'll default the value of the module
> > parameter to the old behavior. Given that the old behavior has existed
> > for longer it's probably a safer default.
> >
> > [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com
> >
> > Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Two minor issues below.

I only saw one, so hopefully I didn't miss another request. ;-)


> > ---
> >
> > Changes in v3:
> > - No longer a revert; now a module parameter.
> >
> > Changes in v2:
> > - Remove the mud from my face.
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 50b987658b1f..2bf4123ef5df 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,6 +34,10 @@ static struct msm_dsi_manager msm_dsim_glb;
> >   #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
> >   #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
> >
> > +bool early_poweron;
> > +MODULE_PARM_DESC(early_poweron, "Power DSI controller early");
> > +module_param(early_poweron, bool, 0600);
>
> Nit: dsi_early_poweron (to be clear that it related to DSI only).
>
> I thought about suggesting 'dsi_no_early_poweron' instead to catch
> possible issues with other bridges. But... I think with Dave's series
> will have to enable bridges one by one, so it doesn't make real sense.

I made the change you requested and was about to send out a v4 when
offline Rob pointed out a better way that seems to work. I can use the
`compatible` of the next bridge. I've sent out v4 as per Rob's
suggestion:

https://lore.kernel.org/r/20220512145954.v4.1.Ia196e35ad985059e77b038a41662faae9e26f411@changeid

-Doug