| Message ID | 20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer | expand |
Hi Douglas, On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > --- > > 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. 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. Regards, Pin-yen
Hi, On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi Douglas, > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > > --- > > > > 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
Hi, On Fri, Dec 22, 2023 at 11:34 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Hi Douglas, > > > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > > > --- > > > > > > 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. > Okay, thanks for the detailed explanation. Then, let's go with the approach in this patch. So, Tested-by: Pin-yen Lin <treapking@chromium.org> Reviewed-by: Pin-yen Lin <treapking@chromium.org> > > > 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 Happy holiday! Pin-yen
Hi, On Mon, Dec 25, 2023 at 1:08 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi, > > On Fri, Dec 22, 2023 at 11:34 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > Hi Douglas, > > > > > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> 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@chromium.org> > > > > --- > > > > > > > > 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. > > > Okay, thanks for the detailed explanation. Then, let's go with the > approach in this patch. So, > > Tested-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Pin-yen Lin <treapking@chromium.org> Thanks for the tags. I've pushed this to drm-misc-fixes: 024b32db43a3 drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer
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);
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@chromium.org> --- drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ 1 file changed, 5 insertions(+)