On Wed, 24 Aug 2022 at 01:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > On 8/22/2022 11:33 AM, Dmitry Baryshkov wrote: > > On 22/08/2022 20:32, Abhinav Kumar wrote: > >> > >> > >> On 8/22/2022 9:49 AM, Dmitry Baryshkov wrote: > >>> On 22/08/2022 19:38, Abhinav Kumar wrote: > >>>> Hi Dmitry > >>>> > >>>> On 8/22/2022 9:18 AM, Dmitry Baryshkov wrote: > >>>>> On 17/08/2022 21:01, Kuogee Hsieh wrote: > >>>>>> DRM commit_tails() will disable downstream crtc/encoder/bridge if > >>>>>> both disable crtc is required and crtc->active is set before pushing > >>>>>> a new frame downstream. > >>>>>> > >>>>>> There is a rare case that user space display manager issue an extra > >>>>>> screen update immediately followed by close DRM device while down > >>>>>> stream display interface is disabled. This extra screen update will > >>>>>> timeout due to the downstream interface is disabled but will cause > >>>>>> crtc->active be set. Hence the followed commit_tails() called by > >>>>>> drm_release() will pass the disable downstream crtc/encoder/bridge > >>>>>> conditions checking even downstream interface is disabled. > >>>>>> This cause the crash to happen at dp_bridge_disable() due to it > >>>>>> trying > >>>>>> to access the main link register to push the idle pattern out > >>>>>> while main > >>>>>> link clocks is disabled. > >>>>>> > >>>>>> This patch adds atomic_check to prevent the extra frame will not > >>>>>> be pushed down if display interface is down so that crtc->active > >>>>>> will not be set neither. This will fail the conditions checking > >>>>>> of disabling down stream crtc/encoder/bridge which prevent > >>>>>> drm_release() from calling dp_bridge_disable() so that crash > >>>>>> at dp_bridge_disable() prevented. > >>>>> > >>>>> I must admit I had troubles parsing this description. However if I > >>>>> got you right, I think the check that the main link clock is > >>>>> running in the dp_bridge_disable() or dp_ctrl_push_idle() would be > >>>>> a better fix. > >>>> > >>>> Originally, thats what was posted > >>>> https://patchwork.freedesktop.org/patch/496984/. > >>> > >>> This patch is also not so correct from my POV. It checks for the hpd > >>> status, while in reality it should check for main link clocks being > >>> enabled. > >>> > >> > >> We can push another fix to check for the clk state instead of the hpd > >> status. But I must say we are again just masking something which the > >> fwk should have avoided isnt it? > >> > >> As per the doc in the include/drm/drm_bridge.h it says, > >> > >> "* > >> * The bridge can assume that the display pipe (i.e. clocks and timing > >> * signals) feeding it is still running when this callback is called. > >> *" > > > > Yes, that's what I meant about this chunk begging to go to the core. In > > my opinion, if we are talking about the disconnected sinks, it is the > > framework who should disallow submitting the frames to the disconnected > > sinks. > > > >> > >> By adding an extra layers of protection in the driver, we are just > >> avoiding another issue but the commit should not have been issued in > >> the first place. > >> > >> So shouldnt we do both then? That is add protection to check if clock > >> is ON and also, reject commits when display is disconnected. > >> > >>>> > >>>> Then it seemed like we were just protecting against an issue in the > >>>> framework which was allowing the frames to be pushed even after the > >>>> display was disconnected. The DP driver did send out the disconnect > >>>> event correctly and as per the logs, this frame came down after that > >>>> and the DRM fwk did allow it. > >>>> > >>>> So after discussing on IRC with Rob, we came up with this approach that > >>>> if the display is not connected, then atomic_check should fail. That > >>>> way the commit will not happen. > >>>> > >>>> Just seemed a bit cleaner instead of adding all our protections. > >>> > >>> The check to fail atomic_check if display is not connected seems out > >>> of place. In its current way it begs go to the upper layer, > >>> forbidding using disconnected sinks for all the drivers. There is > >>> nothing special in the MSM DP driver with respect to the HPD events > >>> processing and failing atomic_check() based on that. > >>> > >> > >> Why all the drivers? This is only for MSM DP bridge. > > > > Yes, we change the MSM DRM driver. But the check is generic enough. I'm > > not actually insisting on pushing the check to the core, just trying to > > understand the real cause here. > > > >> > > I actually wanted to push this to the core and thats what I had > originally asked on IRC because it does seem to be generic enough that > it should belong to the core but after discussion with Rob on freedreno, > he felt this was a better approach because for some of the legacy > connectors like VGA, this need not belong to the DRM core, hence we went > with this approach. It might be better to whitelist such connectors (S-VIDEO/composite comes to my mind rather than VGA). > >>>>>> SError Interrupt on CPU7, code 0x00000000be000411 -- SError > >>>>>> CPU: 7 PID: 3878 Comm: Xorg Not tainted 5.19.0-stb-cbq #19 > >>>>>> Hardware name: Google Lazor (rev3 - 8) (DT) > >>>>>> pstate: a04000c9 (NzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > >>>>>> pc : __cmpxchg_case_acq_32+0x14/0x2c > >>>>>> lr : do_raw_spin_lock+0xa4/0xdc > >>>>>> sp : ffffffc01092b6a0 > >>>>>> x29: ffffffc01092b6a0 x28: 0000000000000028 x27: 0000000000000038 > >>>>>> x26: 0000000000000004 x25: ffffffd2973dce48 x24: 0000000000000000 > >>>>>> x23: 00000000ffffffff x22: 00000000ffffffff x21: ffffffd2978d0008 > >>>>>> x20: ffffffd2978d0008 x19: ffffff80ff759fc0 x18: 0000000000000000 > >>>>>> x17: 004800a501260460 x16: 0441043b04600438 x15: 04380000089807d0 > >>>>>> x14: 07b0089807800780 x13: 0000000000000000 x12: 0000000000000000 > >>>>>> x11: 0000000000000438 x10: 00000000000007d0 x9 : ffffffd2973e09e4 > >>>>>> x8 : ffffff8092d53300 x7 : ffffff808902e8b8 x6 : 0000000000000001 > >>>>>> x5 : ffffff808902e880 x4 : 0000000000000000 x3 : ffffff80ff759fc0 > >>>>>> x2 : 0000000000000001 x1 : 0000000000000000 x0 : ffffff80ff759fc0 > >>>>>> Kernel panic - not syncing: Asynchronous SError Interrupt > >>>>>> CPU: 7 PID: 3878 Comm: Xorg Not tainted 5.19.0-stb-cbq #19 > >>>>>> Hardware name: Google Lazor (rev3 - 8) (DT) > >>>>>> Call trace: > >>>>>> dump_backtrace.part.0+0xbc/0xe4 > >>>>>> show_stack+0x24/0x70 > >>>>>> dump_stack_lvl+0x68/0x84 > >>>>>> dump_stack+0x18/0x34 > >>>>>> panic+0x14c/0x32c > >>>>>> nmi_panic+0x58/0x7c > >>>>>> arm64_serror_panic+0x78/0x84 > >>>>>> do_serror+0x40/0x64 > >>>>>> el1h_64_error_handler+0x30/0x48 > >>>>>> el1h_64_error+0x68/0x6c > >>>>>> __cmpxchg_case_acq_32+0x14/0x2c > >>>>>> _raw_spin_lock_irqsave+0x38/0x4c > > > > You know, after re-reading the trace, I could not help but notice that > > the issue seems to be related to completion/timer/spinlock memory > > becoming unavailable rather than disabling the main link clock. > > See, the SError comes in the spin_lock path, not during register read. > > > > Thus I think the commit message is a bit misleading. > > > > No, this issue is due to unclocked access. Please check this part of the > stack: Well, if it were for the unlocked access, we would see SError on the register access, wouldn't we? However in this case the SError comes from the raw spinlock code. > >>>>>> wait_for_completion_timeout+0x2c/0x54 > >>>>>> dp_ctrl_push_idle+0x40/0x88 > >>>>>> dp_bridge_disable+0x24/0x30 > >>>>>> drm_atomic_bridge_chain_disable+0x90/0xbc > >>>>>> drm_atomic_helper_commit_modeset_disables+0x198/0x444 > >>>>>> msm_atomic_commit_tail+0x1d0/0x374 > >>>>>> commit_tail+0x80/0x108 > >>>>>> drm_atomic_helper_commit+0x118/0x11c > >>>>>> drm_atomic_commit+0xb4/0xe0 > >>>>>> drm_client_modeset_commit_atomic+0x184/0x224 > >>>>>> drm_client_modeset_commit_locked+0x58/0x160 > >>>>>> drm_client_modeset_commit+0x3c/0x64 > > > Can we please get a trace checking which calls were actually made for > > the dp bridge and if the dp/dp->ctrl memory pointers are correct? > > > > I do not see the dp_display_disable() being called. Maybe I just missed > > the call. > > > > Yes it is called, please refer to the above part of the stack that I > have pasted. The stacktrace mentions dp_bridge_disable(), not dp_display_disable() (which I asked for). -- With best wishes Dmitry
