Hi, On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote: > > Hi Douglas, > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge: > > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if > > someone manually tries to do an AUX transfer (like via `i2cdump ${bus} > > 0x50 i`) while the panel is off we don't just get a simple transfer > > error. Instead, the whole ps8640 gets thrown for a loop and goes into > > a bad state. > > > > Let's put the function to wait for the HPD (and the magical 50 ms > > after first reset) back in when we're doing an AUX transfer. This > > shouldn't actually make things much slower (assuming the panel is on) > > because we should immediately poll and see the HPD high. Mostly this > > is just an extra i2c transfer to the bridge. > > > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux") > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > --- > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 541e4f5afc4c..fb5e9ae9ad81 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > > int ret; > > > > pm_runtime_get_sync(dev); > > + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); > > + if (ret) { > > + pm_runtime_put_sync_suspend(dev); > > + return ret; > > + } > > ret = ps8640_aux_transfer_msg(aux, msg); > > pm_runtime_mark_last_busy(dev); > > pm_runtime_put_autosuspend(dev); > > -- > > 2.43.0.472.g3155946c3a-goog > > > > I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device > to ensure suspend/resume order") is trying to address the same > problem, but we see this issue here because the device link is missing > DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we > don't need to add a _ps8640_wait_hpd_asserted() after every > pm_runtime_get_*() call. I disagree. We've had several discussions on the lists about this topic before, though since I'm technically on vacation right now I'm not going to go look them up. In general "pm_runtime" is not sufficient for powering up DRM components. While DRM components can use pm_runtime themselves (as we are doing here), powering up another DRM component by grabbing a pm_runtime reference isn't right. There is a reason for the complexity of the DRM prepare/enable and all the current debates about the right order to call components in prepare() just demonstrates further that a simple pm_runtime reference isn't enough. It can be noted that, with ${SUBJECT} patch we _aren't_ powering up the panel. I actually tested two cases on sc7180-lazor. In one case I just closed the lid, which powered off the panel, but the touchscreen kept the panel power rail on. In this case with my patch I could still read the panel EDID. I then hacked the touchscreen off. Now when I closed the lid I'd get a timeout. This is different than if we tried to get a pm_runtime reference to the panel. > As a side note, I've verified both this patch and DL_FLAG_PM_RUNTIME > in our downstream v5.15 kernel and panel-edp driver. Both of them > successfully wait for HPD asserted when the timeout used to happen, > but the panel is black in that situation. That being said, this patch > still brings us to a better state. Originally, panel_edp_resume() > would return an error when the timeout occurs, so the panel-edp driver > is stuck at an unexpected state. With this patch or > DL_FLAG_PM_RUNTIME, the runtime PM callbacks won't fail and a system > suspend/resume brings the panel back. OK. I'm going to shut off email for real this time while I enjoy some time off. Hopefully the above convinces you. Otherwise I guess we can continue to debate in mid-January. -Doug