Messages in this thread | | | Date | Fri, 27 Jan 2023 14:51:46 -0800 | Subject | Re: [Freedreno] [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset | From | Abhinav Kumar <> |
| |
On 1/27/2023 2:33 PM, Doug Anderson wrote: > Hi, > > On Fri, Jan 27, 2023 at 10:54 AM Abhinav Kumar > <quic_abhinavk@quicinc.com> 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@raspberrypi.com> >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> 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.
> > >>> @@ -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. ;-) > 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.
> 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
| |