On 1/27/2023 2:33 PM, Doug Anderson wrote:
Hi, On Fri, Jan 27, 2023 at 10:54 AM Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:On 1/13/2023 3:56 PM, Douglas Anderson wrote:In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time"), we moved powering up DSI hosts to modeset time. This wasn't because it was an elegant design, but there were no better options. That commit actually ended up breaking ps8640, and thus was born commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") as a temporary hack to un-break ps8640 by moving it to the old way of doing things. It turns out that ps8640 _really_ doesn't like its pre_enable() function to be called after dsi_mgr_bridge_power_on(). Specifically (from experimentation, not because I have any inside knowledge), it looks like the assertion of "RST#" in the ps8640 runtime resume handler seems like it's not allowed to happen after dsi_mgr_bridge_power_on() Recently, Dave Stevenson's series landed allowing bridges some control over pre_enable ordering. The meaty commit for our purposes is commit 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order"). As documented by that series, if a bridge doesn't set "pre_enable_prev_first" then we should use the old ordering. Now that we have the commit ("drm/bridge: tc358762: Set pre_enable_prev_first") we can go back to the old ordering, which also allows us to remove the ps8640 special case. One last note is that even without reverting commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ revert the ps8640 special case and try it out then it doesn't seem to fail anymore. I spent time bisecting / debugging this and it turns out to be mostly luck, so we still want this patch to make sure it's solid. Specifically the reason it sorta works these days is because we implemented wait_hpd_asserted() in ps8640 now, plus the magic of "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() implemented means that we actually power the bridge chip up just a wee bit earlier and then the bridge happens to stay on because of autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). Cc: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>Why is the patch title showing 2/2? I am not seeing any 1/2 here.Is it a problem with your mail filters? You can see it at: https://lore.kernel.org/r/20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid/ You are listed on the "To:" line. ;-)
Ah, I see what happened. The first patch did not have freedreno CCed but the second one did.
So freedreno PW got confused thinking , hey where is the first patch? :) https://patchwork.freedesktop.org/series/112824/ And so did I :) Perhaps freedreno should be CCed on both patches because its a series.
No sure what I am missing here but I certainly dont see msm_dsi_host_disable_irq() being part of any error handling labels which made me think you fixed that.@@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) host1_en_fail: msm_dsi_host_disable(host); host_en_fail: - + msm_dsi_host_disable_irq(host); + if (is_bonded_dsi && msm_dsi1) { + msm_dsi_host_disable_irq(msm_dsi1->host); + msm_dsi_host_power_off(msm_dsi1->host); + }In addition to Dmitry's comment of keeping the bridge_power_on() name, this part of the change seems independent of the patch. This was missing cleanup for DSI1 (esp the disable_irq part). So can we break it up into two parts. 1) Add missing cleanup for DSI1 2) Just get rid of dsi_mgr_power_on_early() and keep the call dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally.I didn't intentionally fix any bug in my patch--I just reverted it all back to how it was before. ;-)
So looking more closely, it looks like overall the current code (AKA
what's landed today and without ${SUBJECT} patch) doesn't really
handle errors with dsi_mgr_bridge_power_on() very well. The normal
case of calling dsi_mgr_bridge_power_on() from modeset is totally
ignored because modeset returns no error. Then the special workaround
for ps8640 just followed the same pattern and assumed that
dsi_mgr_bridge_power_on() succeeded. It also assumed that if the rest
of dsi_mgr_bridge_pre_enable() failed that it didn't need to undo
dsi_mgr_bridge_power_on() because it wouldn't have undone it in the
modeset case.
Yes thats right.
While the current code isn't the best, it's not like the pre_enable() call could have returned errors anyway. It probably wasn't truly the end of the world to behave the way it did. With all that, I guess my plan would be to do as Dmitry says and just always call dsi_mgr_bridge_power_on() from dsi_mgr_bridge_pre_enable(). In the first patch I'll just do that and remove the ps8640 workaround. Then I can add a 2nd patch that improves the error handling by having dsi_mgr_bridge_power_on() return an error code and then adding a matching dsi_mgr_bridge_power_off() that will undo it and include the extra cleanup.
Sounds good to me.
-Doug
