Re: [PATCH v4] drm/msm/dsi: don't powerup at modeset time for parade-ps8640
Hi,
On 5/12/2022 3:44 PM, Doug Anderson wrote:
Hi,
On Thu, May 12, 2022 at 3:34 PM Abhinav Kumar wrote:
On 5/12/2022 3:16 PM, Dmitry Baryshkov wrote:
On 13/05/2022 01:00, 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, we'll look at the compatible of the next bridge in
the chain and go back to the old way for the Parade PS8640 bridge
chip. If it's found that other bridge chips also need this workaround
then we can add them to the list or consider inverting the condition.
[1]
https://lore.kernel.org/r/cover.1646406653.git.dave.steven...@raspberrypi.com
Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time")
Suggested-by: Rob Clark
Signed-off-by: Douglas Anderson
Reviewed-by: Dmitry Baryshkov
Yes, I think this is a better solution than a full revert
Reviewed-by: Abhinav Kumar
I am curious to know why this doesnt work for parade but will not hold
this patch back for that. We are initializing and turning on DSI PHY now
before turning on the bridge chip which is actually better as we are
putting PHY in a good state.
So this should have been better, but somehow doesn't work.
I can't really explain it, but mostly because the Parade chip is just
a big black box. There have been several times when an OEM using this
bridge chip had one problem or another with getting the display to
turn on, then the parade FAE would make some magic tweak to the
firmware and it would be fixed. The current way that the Linux driver
is working is with pretty much zero configuration so I think this chip
bakes in a bunch of assumptions about the timings / signal coming from
the MIPI DSI side. It doesn't surprise me that changing the order like
this would confuse it.
In theory I believe the Parade chip can run in a less "automatic" mode
where everything is configured and controlled by Linux. I'd really
have preferred if we could have gotten that done, but it didn't end up
happening. :(
-Doug
Again this is not to block this change (you already have my ack) but
perhaps to help you debug this for future reference as we wont know what
change can break parade even in the future if this happens again and the
compatible check for parade will keep growing.
One suggestion I can give is can we read any status bits out of the
parade chip in the power_on() method as its right after the new method
to check why its not in good status or what error bits it throws and
perhaps share those error bits with the FAE to see when this error can
come to decode this black box a bit :)
From those bits, we can narrow down why this timing or sequence change
is affecting them. Like some things could be somehow this is delaying
the video pixels from transmitting, it expects PHY to be in some state
etc and we can kind-of reverse engineer why this change broke it.
Like-wise, it is highly possible at the moment we have identified only
parade not to work but if there is something wrong with this change we
can atleast know what and address it.
Thanks
Abhinav
Re: [PATCH v4] drm/msm/dsi: don't powerup at modeset time for parade-ps8640
Hi,
On Thu, May 12, 2022 at 3:34 PM Abhinav Kumar wrote:
>
> On 5/12/2022 3:16 PM, Dmitry Baryshkov wrote:
> > On 13/05/2022 01:00, 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, we'll look at the compatible of the next bridge in
> >> the chain and go back to the old way for the Parade PS8640 bridge
> >> chip. If it's found that other bridge chips also need this workaround
> >> then we can add them to the list or consider inverting the condition.
> >>
> >> [1]
> >> https://lore.kernel.org/r/cover.1646406653.git.dave.steven...@raspberrypi.com
> >>
> >>
> >> Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >> time")
> >> Suggested-by: Rob Clark
> >> Signed-off-by: Douglas Anderson
> >
> > Reviewed-by: Dmitry Baryshkov
> >
> Yes, I think this is a better solution than a full revert
>
> Reviewed-by: Abhinav Kumar
>
> I am curious to know why this doesnt work for parade but will not hold
> this patch back for that. We are initializing and turning on DSI PHY now
> before turning on the bridge chip which is actually better as we are
> putting PHY in a good state.
>
> So this should have been better, but somehow doesn't work.
I can't really explain it, but mostly because the Parade chip is just
a big black box. There have been several times when an OEM using this
bridge chip had one problem or another with getting the display to
turn on, then the parade FAE would make some magic tweak to the
firmware and it would be fixed. The current way that the Linux driver
is working is with pretty much zero configuration so I think this chip
bakes in a bunch of assumptions about the timings / signal coming from
the MIPI DSI side. It doesn't surprise me that changing the order like
this would confuse it.
In theory I believe the Parade chip can run in a less "automatic" mode
where everything is configured and controlled by Linux. I'd really
have preferred if we could have gotten that done, but it didn't end up
happening. :(
-Doug
Re: [PATCH v4] drm/msm/dsi: don't powerup at modeset time for parade-ps8640
On 5/12/2022 3:16 PM, Dmitry Baryshkov wrote:
On 13/05/2022 01:00, 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, we'll look at the compatible of the next bridge in
the chain and go back to the old way for the Parade PS8640 bridge
chip. If it's found that other bridge chips also need this workaround
then we can add them to the list or consider inverting the condition.
[1]
https://lore.kernel.org/r/cover.1646406653.git.dave.steven...@raspberrypi.com
Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time")
Suggested-by: Rob Clark
Signed-off-by: Douglas Anderson
Reviewed-by: Dmitry Baryshkov
Yes, I think this is a better solution than a full revert
Reviewed-by: Abhinav Kumar
I am curious to know why this doesnt work for parade but will not hold
this patch back for that. We are initializing and turning on DSI PHY now
before turning on the bridge chip which is actually better as we are
putting PHY in a good state.
So this should have been better, but somehow doesnt work.
---
Note that, unlike `struct device`, `struct drm_bridge` still has a
`#ifdef` around the `of_node`. The extra stub function in this patch
is to make sure that we can pass COMPILE_TEST, not because I expect
that we'll actually run into real users who are running this driver
without device tree.
Changes in v4:
- Use the compatible string of the next bridge as per Rob.
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 | 26 +-
1 file changed, 25 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..2cabba65a8f1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,6 +34,26 @@ 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)
+#ifdef CONFIG_OF
+static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
+
+ /*
+ * If the next bridge in the chain is the Parade ps8640 bridge chip
+ * then don't power on early since it seems to violate the
expectations
+ * of the firmware that the bridge chip is running.
+ */
+ return !(next_bridge && next_bridge->of_node &&
+ of_device_is_compatible(next_bridge->of_node,
"parade,ps8640"));
+}
+#else
+static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ return true;
+}
+#endif
+
static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
{
return msm_dsim_glb.dsi[id];
@@ -389,6 +409,9 @@ static void dsi_mgr_bridge_pre_enable(struct
drm_bridge *bridge)
if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
return;
+ if (!dsi_mgr_power_on_early(bridge))
+ 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 +593,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 (dsi_mgr_power_on_early(bridge))
+ dsi_mgr_bridge_power_on(bridge);
}
static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct
drm_bridge *bridge,
Re: [PATCH v4] drm/msm/dsi: don't powerup at modeset time for parade-ps8640
On 13/05/2022 01:00, 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, we'll look at the compatible of the next bridge in
the chain and go back to the old way for the Parade PS8640 bridge
chip. If it's found that other bridge chips also need this workaround
then we can add them to the list or consider inverting the condition.
[1]
https://lore.kernel.org/r/cover.1646406653.git.dave.steven...@raspberrypi.com
Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
Suggested-by: Rob Clark
Signed-off-by: Douglas Anderson
Reviewed-by: Dmitry Baryshkov
---
Note that, unlike `struct device`, `struct drm_bridge` still has a
`#ifdef` around the `of_node`. The extra stub function in this patch
is to make sure that we can pass COMPILE_TEST, not because I expect
that we'll actually run into real users who are running this driver
without device tree.
Changes in v4:
- Use the compatible string of the next bridge as per Rob.
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 | 26 +-
1 file changed, 25 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..2cabba65a8f1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,6 +34,26 @@ 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)
+#ifdef CONFIG_OF
+static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
+
+ /*
+* If the next bridge in the chain is the Parade ps8640 bridge chip
+* then don't power on early since it seems to violate the expectations
+* of the firmware that the bridge chip is running.
+*/
+ return !(next_bridge && next_bridge->of_node &&
+of_device_is_compatible(next_bridge->of_node,
"parade,ps8640"));
+}
+#else
+static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ return true;
+}
+#endif
+
static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
{
return msm_dsim_glb.dsi[id];
@@ -389,6 +409,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge
*bridge)
if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
return;
+ if (!dsi_mgr_power_on_early(bridge))
+ 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 +593,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 (dsi_mgr_power_on_early(bridge))
+ dsi_mgr_bridge_power_on(bridge);
}
static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
--
With best wishes
Dmitry
[PATCH v4] drm/msm/dsi: don't powerup at modeset time for parade-ps8640
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, we'll look at the compatible of the next bridge in
the chain and go back to the old way for the Parade PS8640 bridge
chip. If it's found that other bridge chips also need this workaround
then we can add them to the list or consider inverting the condition.
[1]
https://lore.kernel.org/r/cover.1646406653.git.dave.steven...@raspberrypi.com
Fixes: 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time")
Suggested-by: Rob Clark
Signed-off-by: Douglas Anderson
---
Note that, unlike `struct device`, `struct drm_bridge` still has a
`#ifdef` around the `of_node`. The extra stub function in this patch
is to make sure that we can pass COMPILE_TEST, not because I expect
that we'll actually run into real users who are running this driver
without device tree.
Changes in v4:
- Use the compatible string of the next bridge as per Rob.
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 | 26 +-
1 file changed, 25 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..2cabba65a8f1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,6 +34,26 @@ 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)
+#ifdef CONFIG_OF
+static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
+
+ /*
+* If the next bridge in the chain is the Parade ps8640 bridge chip
+* then don't power on early since it seems to violate the expectations
+* of the firmware that the bridge chip is running.
+*/
+ return !(next_bridge && next_bridge->of_node &&
+of_device_is_compatible(next_bridge->of_node,
"parade,ps8640"));
+}
+#else
+static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
+{
+ return true;
+}
+#endif
+
static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
{
return msm_dsim_glb.dsi[id];
@@ -389,6 +409,9 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge
*bridge)
if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
return;
+ if (!dsi_mgr_power_on_early(bridge))
+ 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 +593,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 (dsi_mgr_power_on_early(bridge))
+ 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
