| Message ID | 1647452154-16361-7-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the eDP panel on sc7280 CRD | expand |
Quoting Sankeerth Billakanti (2022-03-16 10:35:51) > The source device should ensure the sink is ready before > proceeding to read the sink capability or performing any aux transactions. > The sink will indicate its readiness by asserting the HPD line. > > The eDP sink requires power from the source and its HPD line will > be asserted only after the panel is powered on. The panel power will be > enabled from the panel-edp driver. > > The controller driver needs to wait for the hpd line to be asserted > by the sink. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ > drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- > 4 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 6d36f63..2ddc303 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > goto exit; > } > > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); Why are we making aux transactions when hpd isn't asserted? Can we only register the aux device once we know that state is "connected"? I'm concerned that we're going to be possibly polling the connected bit up to some amount of time (0x0003FFFF usecs?) for each aux transfer when that doesn't make any sense to keep checking. We should be able to check it once, register aux, and then when disconnect happens we can unregister aux. > + if (ret) { > + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); > + goto exit; > + } > + > dp_aux_update_offset_and_segment(aux, msg); > dp_aux_transfer_helper(aux, msg, true); > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index fac815f..2c3b0f7 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) > phy_calibrate(phy); > } > > +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > +{ > + u32 state, hpd_en, timeout; > + struct dp_catalog_private *catalog = container_of(dp_catalog, > + struct dp_catalog_private, dp_catalog); > + > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & > + DP_DP_HPD_CTRL_HPD_EN; Use two lines hpd_en = dp_read_aux(); hpd_en &= DP_DP_HPD_CTRL_HPD_EN; > + > + /* no-hpd case */ > + if (!hpd_en) > + return 0; > + > + /* Poll for HPD connected status */ > + timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) & > + DP_HPD_CONNECT_TIME_MASK; > + > + return readl_poll_timeout(catalog->io->dp_controller.aux.base + > + REG_DP_DP_HPD_INT_STATUS, > + state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, > + 2000, timeout); > +} > + > static void dump_regs(void __iomem *base, int len) > { > int i;
Hi, On Thu, Mar 17, 2022 at 6:19 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Sankeerth Billakanti (2022-03-16 10:35:51) > > The source device should ensure the sink is ready before > > proceeding to read the sink capability or performing any aux transactions. > > The sink will indicate its readiness by asserting the HPD line. > > > > The eDP sink requires power from the source and its HPD line will > > be asserted only after the panel is powered on. The panel power will be > > enabled from the panel-edp driver. > > > > The controller driver needs to wait for the hpd line to be asserted > > by the sink. > > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > --- > > drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ > > drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ > > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > > drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- > > 4 files changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > > index 6d36f63..2ddc303 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > > goto exit; > > } > > > > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > > Why are we making aux transactions when hpd isn't asserted? Can we only > register the aux device once we know that state is "connected"? I'm > concerned that we're going to be possibly polling the connected bit up > to some amount of time (0x0003FFFF usecs?) for each aux transfer when > that doesn't make any sense to keep checking. We should be able to check > it once, register aux, and then when disconnect happens we can > unregister aux. This is for eDP and, unless someone wants to redesign it again, is just how it works. Specifically: 1. On eDP you _always_ report "connected". This is because when an eDP panel is turned off (but still there) you actually have no way to detect it--you just have to assume it's there. And thus you _always_ register the AUX bus. 2. When we are asked to read the EDID that happens _before_ the normal prepare/enable steps. The way that this should work is that the request travels down to the panel. The panel turns itself on (waiting for any hardcoded delays it knows about) and then initiates an AUX transaction. The AUX transaction is in charge of waiting for HPD. For the DP case this should not cause any significant overhead, right? HPD should always be asserted so this is basically just one extra IO read confirming that HPD is asserted which should be almost nothing... You're just about to do a whole bunch of IO reads/writes in order to program the AUX transaction anyway. > > + if (ret) { > > + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); > > + goto exit; > > + } > > + > > dp_aux_update_offset_and_segment(aux, msg); > > dp_aux_transfer_helper(aux, msg, true); > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > > index fac815f..2c3b0f7 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) > > phy_calibrate(phy); > > } > > > > +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > > +{ > > + u32 state, hpd_en, timeout; > > + struct dp_catalog_private *catalog = container_of(dp_catalog, > > + struct dp_catalog_private, dp_catalog); > > + > > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & > > + DP_DP_HPD_CTRL_HPD_EN; > > Use two lines > > hpd_en = dp_read_aux(); > hpd_en &= DP_DP_HPD_CTRL_HPD_EN; > > > + > > + /* no-hpd case */ > > + if (!hpd_en) > > + return 0; I guess reading from hardware is fine, but I would have expected the driver to simply know whether HPD is used or not. Don't need to read it from hardware, do we? It's not like it's changing from minute to minute--this is something known at probe time. > > + /* Poll for HPD connected status */ > > + timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) & > > + DP_HPD_CONNECT_TIME_MASK; > > + > > + return readl_poll_timeout(catalog->io->dp_controller.aux.base + > > + REG_DP_DP_HPD_INT_STATUS, > > + state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, > > + 2000, timeout); The timeout that comes from hardware is really stored in microseconds? That's the units of the value passed to readl_poll_timeout(), right? Looking at your #defines, that means that your max value here is 0x3fff which is 16383 microseconds or ~16 ms. That doesn't seem like nearly a long enough timeout to wait for a panel to power itself up. Also: I'm not sure why exactly you're using the timeout in the register here. Isn't the time you need to wait more about how long an eDP panel might conceivably need to power itself on? Most eDP panels I've seen have max delays of ~200 ms. I'd probably just wait 500 ms to be on the safe side. -Doug
Quoting Doug Anderson (2022-03-18 09:24:17) > Hi, > > On Thu, Mar 17, 2022 at 6:19 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Sankeerth Billakanti (2022-03-16 10:35:51) > > > The source device should ensure the sink is ready before > > > proceeding to read the sink capability or performing any aux transactions. > > > The sink will indicate its readiness by asserting the HPD line. > > > > > > The eDP sink requires power from the source and its HPD line will > > > be asserted only after the panel is powered on. The panel power will be > > > enabled from the panel-edp driver. > > > > > > The controller driver needs to wait for the hpd line to be asserted > > > by the sink. > > > > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > > --- > > > drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ > > > drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ > > > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > > > drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- > > > 4 files changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > > > index 6d36f63..2ddc303 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > > @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > > > goto exit; > > > } > > > > > > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > > > > Why are we making aux transactions when hpd isn't asserted? Can we only > > register the aux device once we know that state is "connected"? I'm > > concerned that we're going to be possibly polling the connected bit up > > to some amount of time (0x0003FFFF usecs?) for each aux transfer when > > that doesn't make any sense to keep checking. We should be able to check > > it once, register aux, and then when disconnect happens we can > > unregister aux. > > This is for eDP and, unless someone wants to redesign it again, is > just how it works. > > Specifically: > > 1. On eDP you _always_ report "connected". This is because when an eDP > panel is turned off (but still there) you actually have no way to > detect it--you just have to assume it's there. And thus you _always_ > register the AUX bus. Is reporting "connected" the same as HPD being asserted in the case of eDP? I can understand wanting to report "connected", because as you say, the panel is always connected; there aren't dongles or cables involved. But the state of the HPD pin is changing at runtime, and eDP supports irq_hpd pulses from what I recall, for "link management". I think this device requires the status bit in the hardware to say it is "connected" before aux transactions are guaranteed to work. Presumably the HPD pin could go be asserted at the SoC's pad and there could be some time still where the hardware status bit hasn't flipped over to "connected" yet and thus aux transactions are going to fail. Can qcom confirm this? > > 2. When we are asked to read the EDID that happens _before_ the normal > prepare/enable steps. The way that this should work is that the > request travels down to the panel. The panel turns itself on (waiting > for any hardcoded delays it knows about) and then initiates an AUX > transaction. The AUX transaction is in charge of waiting for HPD. Are we talking about generic_edp_panel_probe()? Why doesn't that poll hpd gpio like panel_edp_prepare_once() does? Are there any links to discussions about this I can read? Pushing hpd state checking into aux transactions looks like the wrong direction. Also, as I said up above I am concerned that even checking the GPIO won't work and we need some way to ask the bridge if HPD is asserted or not and then fallback to the GPIO method if the display phy/controller doesn't have support to check HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > > For the DP case this should not cause any significant overhead, right? > HPD should always be asserted so this is basically just one extra IO > read confirming that HPD is asserted which should be almost nothing... > You're just about to do a whole bunch of IO reads/writes in order to > program the AUX transaction anyway. In the DP case the dongle/cable can be disconnected in the middle of aux transactions. If that happens we could be waiting a while in this transfer function to timeout looking for the status bit. The driver already gets an "unplug" irq when the cable is disconnected though so it would be better to figure out a way to stop the aux transactions quickly when that happens without having to read the hardware and poll the bit that we already know is doomed to timeout. I think apple dongles throw this logic for a loop though because the HDMI cable can be disconnected from the dongle and then we don't see an "unplug" irq, just the number of sinks becomes 0. Maybe there's an irq_hpd event, not sure. > > > > > + if (ret) { > > > + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); > > > + goto exit; > > > + } > > > + > > > dp_aux_update_offset_and_segment(aux, msg); > > > dp_aux_transfer_helper(aux, msg, true); > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > index fac815f..2c3b0f7 100644 > > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) > > > phy_calibrate(phy); > > > } > > > > > > +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > > > +{ > > > + u32 state, hpd_en, timeout; > > > + struct dp_catalog_private *catalog = container_of(dp_catalog, > > > + struct dp_catalog_private, dp_catalog); > > > + > > > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & > > > + DP_DP_HPD_CTRL_HPD_EN; > > > > Use two lines > > > > hpd_en = dp_read_aux(); > > hpd_en &= DP_DP_HPD_CTRL_HPD_EN; > > > > > + > > > + /* no-hpd case */ > > > + if (!hpd_en) > > > + return 0; > > I guess reading from hardware is fine, but I would have expected the > driver to simply know whether HPD is used or not. Don't need to read > it from hardware, do we? It's not like it's changing from minute to > minute--this is something known at probe time. Are you saying that HPD is always asserted? That doesn't sound right. My understanding is that HPD will be asserted after the panel is powered up. Before that HPD is deasserted. Similarly, when we power down the panel, HPD will be deasserted. I guess DRM wants to assume that an eDP panel is always connected? That sounds like it might be OK as long as userspace doesn't use "connected" to know that it's OK to do things like read/write aux or push pixels to the panel when HPD is deasserted.
Hi, On Fri, Mar 18, 2022 at 1:17 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > > > > > > Why are we making aux transactions when hpd isn't asserted? Can we only > > > register the aux device once we know that state is "connected"? I'm > > > concerned that we're going to be possibly polling the connected bit up > > > to some amount of time (0x0003FFFF usecs?) for each aux transfer when > > > that doesn't make any sense to keep checking. We should be able to check > > > it once, register aux, and then when disconnect happens we can > > > unregister aux. > > > > This is for eDP and, unless someone wants to redesign it again, is > > just how it works. > > > > Specifically: > > > > 1. On eDP you _always_ report "connected". This is because when an eDP > > panel is turned off (but still there) you actually have no way to > > detect it--you just have to assume it's there. And thus you _always_ > > register the AUX bus. > > Is reporting "connected" the same as HPD being asserted in the case of > eDP? I can understand wanting to report "connected", because as you say, > the panel is always connected; there aren't dongles or cables involved. No. What I mean by connected is that when DRM asks "hey, do you have a panel" connected then for eDP we always say "yes" regardless of any hardware state. HPD is a _huge_ misnomer for eDP and IMO the name causes lots of confusion. It's not "hot plug detect". You don't hot plug eDP. It's really "panel ready / panel IRQ" > But the state of the HPD pin is changing at runtime, and eDP supports > irq_hpd pulses from what I recall, for "link management". > > I think this device requires the status bit in the hardware to say it is > "connected" before aux transactions are guaranteed to work. Presumably > the HPD pin could go be asserted at the SoC's pad and there could be > some time still where the hardware status bit hasn't flipped over to > "connected" yet and thus aux transactions are going to fail. Can qcom > confirm this? > > > > > 2. When we are asked to read the EDID that happens _before_ the normal > > prepare/enable steps. The way that this should work is that the > > request travels down to the panel. The panel turns itself on (waiting > > for any hardcoded delays it knows about) and then initiates an AUX > > transaction. The AUX transaction is in charge of waiting for HPD. > > Are we talking about generic_edp_panel_probe()? Why doesn't that poll > hpd gpio like panel_edp_prepare_once() does? There's no HPD GPIO in this case, right? In the trogdor case we ended up not using the HPD that was part of the ti-sn65dsi86 controller because it was fairly useless (it debounced for far too long), so we ended up hooking it up as a GPIO and I guess gave up on getting the extra notifications from the panel. Maybe a good thing, in hindsight, that we didn't do PSR because that might have been a pain. In any case, originally I had the GPIO being handled by the ti-sn65dsi86 controller driver and that seemed like it made sense to me (after all, the ti-sn65dsi86 driver would have to handle HPD if this went to the dedicated HPD pin) but got told "no, put it in the panel" by both you and Laurent [1]. [1] https://lore.kernel.org/r/20200415203256.GP4758@pendragon.ideasonboard.com/ > Are there any links to > discussions about this I can read? I'm not sure if there's any more than the conversation I pointed at above where we talked about hpd-gpios. Atop that, I believe I just realized that this was the only way it could work without re-designing again. To some extent the status quo is documented in commit a64ad9c3e4a5 ("drm/panel-edp: Fix "prepare_to_enable" if panel doesn't handle HPD"). I wrote that commit when I thought about how HPD would need to be handled if it was a dedicated pin on the controller and the panel didn't have knowledge about it. > Pushing hpd state checking into aux > transactions looks like the wrong direction. Also, as I said up above I > am concerned that even checking the GPIO won't work and we need some way > to ask the bridge if HPD is asserted or not and then fallback to the > GPIO method if the display phy/controller doesn't have support to check > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? If we could somehow get the HPD status from the bridge in the panel driver it definitely would be convenient. It does feel like that's an improvement that could be done later, though. We've already landed a few instances of doing what's done here, like for parade-ps8640 and analogix_dp. I suspect designing a new mechanism might not be the most trivial. I haven't actually tried it, but I suspect that to get something like what you're talking about we'd have to get the rest of drm to know that for eDP ports that it should assume something is connected _regardless_ of what the "detect" / "HPD" options say. Then we'd have to extend the edp-panel code to be able to be able to query the next bridge in the chain if a GPIO wasn't provided. > > For the DP case this should not cause any significant overhead, right? > > HPD should always be asserted so this is basically just one extra IO > > read confirming that HPD is asserted which should be almost nothing... > > You're just about to do a whole bunch of IO reads/writes in order to > > program the AUX transaction anyway. > > In the DP case the dongle/cable can be disconnected in the middle of aux > transactions. If that happens we could be waiting a while in this > transfer function to timeout looking for the status bit. The driver > already gets an "unplug" irq when the cable is disconnected though so it > would be better to figure out a way to stop the aux transactions quickly > when that happens without having to read the hardware and poll the bit > that we already know is doomed to timeout. I think apple dongles throw > this logic for a loop though because the HDMI cable can be disconnected > from the dongle and then we don't see an "unplug" irq, just the number > of sinks becomes 0. Maybe there's an irq_hpd event, not sure. Ah, interesting. Having a DP cable unplugged in the middle of an aux transaction does seem like it could be a problem. What if we just wait in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"? That should be easy, right? > > > > + if (ret) { > > > > + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); > > > > + goto exit; > > > > + } > > > > + > > > > dp_aux_update_offset_and_segment(aux, msg); > > > > dp_aux_transfer_helper(aux, msg, true); > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > index fac815f..2c3b0f7 100644 > > > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) > > > > phy_calibrate(phy); > > > > } > > > > > > > > +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > > > > +{ > > > > + u32 state, hpd_en, timeout; > > > > + struct dp_catalog_private *catalog = container_of(dp_catalog, > > > > + struct dp_catalog_private, dp_catalog); > > > > + > > > > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & > > > > + DP_DP_HPD_CTRL_HPD_EN; > > > > > > Use two lines > > > > > > hpd_en = dp_read_aux(); > > > hpd_en &= DP_DP_HPD_CTRL_HPD_EN; > > > > > > > + > > > > + /* no-hpd case */ > > > > + if (!hpd_en) > > > > + return 0; > > > > I guess reading from hardware is fine, but I would have expected the > > driver to simply know whether HPD is used or not. Don't need to read > > it from hardware, do we? It's not like it's changing from minute to > > minute--this is something known at probe time. > > Are you saying that HPD is always asserted? I don't think this is looking for HPD assertion, is it? This is looking for whether the HPD interrupt is enabled, isn't it? This is to support the case of eDP panels where we didn't hook the HPD line up, right? It should be known at probe time whether we've hooked HPD up or not. ...or am I misreading? > That doesn't sound right. > My understanding is that HPD will be asserted after the panel is powered > up. Before that HPD is deasserted. Similarly, when we power down the > panel, HPD will be deasserted. I guess DRM wants to assume that an eDP > panel is always connected? That sounds like it might be OK as long as > userspace doesn't use "connected" to know that it's OK to do things like > read/write aux or push pixels to the panel when HPD is deasserted. IMO having userspace reading / writing aux directly and expecting it to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP case, but it's really not good in the eDP case. To me it's sorta like expecting things to be amazing and foolproof when you go behind the kernel's back and write to an i2c device using `i2cset -f`. Sure, it might work, but it can also confuse the heck out of things. It also turns out to be a huge problem when you get to PSR because userspace will get errors if it tries to write to the AUX channel and the panel is in PSR mode. This came up in the context of Brian's analogix dp patches [1]. The right answer, in my mind, is to treat userspace accessing the AUX channel directly as more of a debug feature, at least for eDP panels. In terms of userspace pushing pixels to the panel, I don't think that's quite the same, is it? Generally userspace is in charge of whether the eDP panel is powered on or powered off, isn't it? So generally I think that for eDP a panel is always "connected" in all senses of the word. It might not be "powered" at some given point of time, but it's always connected. [1] https://lore.kernel.org/r/CAD=FV=VYe1rLKANQ8eom7g8x1v6_s_OYnX819Ax4m7O3UwDHmg@mail.gmail.com/ -Doug
Quoting Doug Anderson (2022-03-18 14:58:55) > Hi, > > On Fri, Mar 18, 2022 at 1:17 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > > > > > > > > Why are we making aux transactions when hpd isn't asserted? Can we only > > > > register the aux device once we know that state is "connected"? I'm > > > > concerned that we're going to be possibly polling the connected bit up > > > > to some amount of time (0x0003FFFF usecs?) for each aux transfer when > > > > that doesn't make any sense to keep checking. We should be able to check > > > > it once, register aux, and then when disconnect happens we can > > > > unregister aux. > > > > > > This is for eDP and, unless someone wants to redesign it again, is > > > just how it works. > > > > > > Specifically: > > > > > > 1. On eDP you _always_ report "connected". This is because when an eDP > > > panel is turned off (but still there) you actually have no way to > > > detect it--you just have to assume it's there. And thus you _always_ > > > register the AUX bus. > > > > Is reporting "connected" the same as HPD being asserted in the case of > > eDP? I can understand wanting to report "connected", because as you say, > > the panel is always connected; there aren't dongles or cables involved. > > No. What I mean by connected is that when DRM asks "hey, do you have a > panel" connected then for eDP we always say "yes" regardless of any > hardware state. > > HPD is a _huge_ misnomer for eDP and IMO the name causes lots of > confusion. It's not "hot plug detect". You don't hot plug eDP. It's > really "panel ready / panel IRQ" > > > > But the state of the HPD pin is changing at runtime, and eDP supports > > irq_hpd pulses from what I recall, for "link management". > > > > I think this device requires the status bit in the hardware to say it is > > "connected" before aux transactions are guaranteed to work. Presumably > > the HPD pin could go be asserted at the SoC's pad and there could be > > some time still where the hardware status bit hasn't flipped over to > > "connected" yet and thus aux transactions are going to fail. Can qcom > > confirm this? > > > > > > > > 2. When we are asked to read the EDID that happens _before_ the normal > > > prepare/enable steps. The way that this should work is that the > > > request travels down to the panel. The panel turns itself on (waiting > > > for any hardcoded delays it knows about) and then initiates an AUX > > > transaction. The AUX transaction is in charge of waiting for HPD. > > > > Are we talking about generic_edp_panel_probe()? Why doesn't that poll > > hpd gpio like panel_edp_prepare_once() does? > > There's no HPD GPIO in this case, right? Right. The hardware supports HPD here, so polling the pin as a gpio is incorrect. > > In the trogdor case we ended up not using the HPD that was part of the > ti-sn65dsi86 controller because it was fairly useless (it debounced > for far too long), so we ended up hooking it up as a GPIO and I guess > gave up on getting the extra notifications from the panel. Maybe a > good thing, in hindsight, that we didn't do PSR because that might > have been a pain. > > In any case, originally I had the GPIO being handled by the > ti-sn65dsi86 controller driver and that seemed like it made sense to > me (after all, the ti-sn65dsi86 driver would have to handle HPD if > this went to the dedicated HPD pin) but got told "no, put it in the > panel" by both you and Laurent [1]. > > [1] https://lore.kernel.org/r/20200415203256.GP4758@pendragon.ideasonboard.com/ > > > > Are there any links to > > discussions about this I can read? > > I'm not sure if there's any more than the conversation I pointed at > above where we talked about hpd-gpios. Atop that, I believe I just > realized that this was the only way it could work without re-designing > again. > > To some extent the status quo is documented in commit a64ad9c3e4a5 > ("drm/panel-edp: Fix "prepare_to_enable" if panel doesn't handle > HPD"). I wrote that commit when I thought about how HPD would need to > be handled if it was a dedicated pin on the controller and the panel > didn't have knowledge about it. > > > > Pushing hpd state checking into aux > > transactions looks like the wrong direction. Also, as I said up above I > > am concerned that even checking the GPIO won't work and we need some way > > to ask the bridge if HPD is asserted or not and then fallback to the > > GPIO method if the display phy/controller doesn't have support to check > > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > If we could somehow get the HPD status from the bridge in the panel > driver it definitely would be convenient. It does feel like that's an > improvement that could be done later, though. We've already landed a > few instances of doing what's done here, like for parade-ps8640 and > analogix_dp. I suspect designing a new mechanism might not be the most > trivial. What is done in the bridge drivers is to wait for a fixed timeout and assume aux is ready? Or is it something else? If there's just a fixed timeout for the eDP case it sounds OK to do that for now and we can fine tune it later to actually check HPD status register before the panel tries to read EDID. > > I haven't actually tried it, but I suspect that to get something like > what you're talking about we'd have to get the rest of drm to know > that for eDP ports that it should assume something is connected > _regardless_ of what the "detect" / "HPD" options say. Then we'd have > to extend the edp-panel code to be able to be able to query the next > bridge in the chain if a GPIO wasn't provided. Can the panel interrogate the bridge chain somehow? It feels like either something in the chain should know the status of HPD (the case here where display controller in the SoC knows) or it should be a gpio to the panel (trogdor case). The bridge ops can implement DRM_BRIDGE_OP_HPD and the first bridge from the encoder that supports HPD can implement some sort of "wait for hpd asserted" function that the panel then uses once it powers up the panel during probe. If the panel has a gpio and nothing else in the chain can detect hpd then the panel polls the gpio, or it waits for the amount of time delay after powering on the panel if the panel's hpd function is called. > > > > > For the DP case this should not cause any significant overhead, right? > > > HPD should always be asserted so this is basically just one extra IO > > > read confirming that HPD is asserted which should be almost nothing... > > > You're just about to do a whole bunch of IO reads/writes in order to > > > program the AUX transaction anyway. > > > > In the DP case the dongle/cable can be disconnected in the middle of aux > > transactions. If that happens we could be waiting a while in this > > transfer function to timeout looking for the status bit. The driver > > already gets an "unplug" irq when the cable is disconnected though so it > > would be better to figure out a way to stop the aux transactions quickly > > when that happens without having to read the hardware and poll the bit > > that we already know is doomed to timeout. I think apple dongles throw > > this logic for a loop though because the HDMI cable can be disconnected > > from the dongle and then we don't see an "unplug" irq, just the number > > of sinks becomes 0. Maybe there's an irq_hpd event, not sure. > > Ah, interesting. Having a DP cable unplugged in the middle of an aux > transaction does seem like it could be a problem. What if we just wait > in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"? That should > be easy, right? Sounds like it would work. Is this supposed to fix some DP case as well though? There were some patches to speed up aux failures when the dongle was unplugged but I haven't checked after that. I guess this waiting is only important for eDP because the edp-panel code is trying to read EDID and it isn't waiting for HPD to be asserted before doing that. > > > > > > > + if (ret) { > > > > > + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); > > > > > + goto exit; > > > > > + } > > > > > + > > > > > dp_aux_update_offset_and_segment(aux, msg); > > > > > dp_aux_transfer_helper(aux, msg, true); > > > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > > index fac815f..2c3b0f7 100644 > > > > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > > > @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) > > > > > phy_calibrate(phy); > > > > > } > > > > > > > > > > +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > > > > > +{ > > > > > + u32 state, hpd_en, timeout; > > > > > + struct dp_catalog_private *catalog = container_of(dp_catalog, > > > > > + struct dp_catalog_private, dp_catalog); > > > > > + > > > > > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & > > > > > + DP_DP_HPD_CTRL_HPD_EN; > > > > > > > > Use two lines > > > > > > > > hpd_en = dp_read_aux(); > > > > hpd_en &= DP_DP_HPD_CTRL_HPD_EN; > > > > > > > > > + > > > > > + /* no-hpd case */ > > > > > + if (!hpd_en) > > > > > + return 0; > > > > > > I guess reading from hardware is fine, but I would have expected the > > > driver to simply know whether HPD is used or not. Don't need to read > > > it from hardware, do we? It's not like it's changing from minute to > > > minute--this is something known at probe time. > > > > Are you saying that HPD is always asserted? > > I don't think this is looking for HPD assertion, is it? This is > looking for whether the HPD interrupt is enabled, isn't it? This is to > support the case of eDP panels where we didn't hook the HPD line up, > right? It should be known at probe time whether we've hooked HPD up or > not. ...or am I misreading? Ah right. This is basically a proxy for "is no-hpd present in DT?" per the last patch in this series. > > > > That doesn't sound right. > > My understanding is that HPD will be asserted after the panel is powered > > up. Before that HPD is deasserted. Similarly, when we power down the > > panel, HPD will be deasserted. I guess DRM wants to assume that an eDP > > panel is always connected? That sounds like it might be OK as long as > > userspace doesn't use "connected" to know that it's OK to do things like > > read/write aux or push pixels to the panel when HPD is deasserted. > > IMO having userspace reading / writing aux directly and expecting it > to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP > case, but it's really not good in the eDP case. To me it's sorta like > expecting things to be amazing and foolproof when you go behind the > kernel's back and write to an i2c device using `i2cset -f`. Sure, it > might work, but it can also confuse the heck out of things. It also > turns out to be a huge problem when you get to PSR because userspace > will get errors if it tries to write to the AUX channel and the panel > is in PSR mode. This came up in the context of Brian's analogix dp > patches [1]. The right answer, in my mind, is to treat userspace > accessing the AUX channel directly as more of a debug feature, at > least for eDP panels. If it's a debug feature then it should be removed from the system. The flow of data is passing through the kernel so if the kernel is getting confused about backdoor access over aux it should snoop the transactions and block things it doesn't like. I don't know the backstory on aux being exposed to userspace, but leaving it in a broken state isn't good. > > In terms of userspace pushing pixels to the panel, I don't think > that's quite the same, is it? Generally userspace is in charge of > whether the eDP panel is powered on or powered off, isn't it? I'm not sure what it's the same as, but I meant drawing on the screen when the display is powering on but not visible yet is concerning. drm_helper_hpd_irq_event() is used to tell userspace that it can "start drawing now" because it calls drm_kms_helper_hotplug_event() when the connector status changes. This is my understanding of how the DP path works in this driver. I don't know how it works for eDP bridge drivers. > > So generally I think that for eDP a panel is always "connected" in all > senses of the word. It might not be "powered" at some given point of > time, but it's always connected. > > [1] https://lore.kernel.org/r/CAD=FV=VYe1rLKANQ8eom7g8x1v6_s_OYnX819Ax4m7O3UwDHmg@mail.gmail.com/ >
Hi, On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Pushing hpd state checking into aux > > > transactions looks like the wrong direction. Also, as I said up above I > > > am concerned that even checking the GPIO won't work and we need some way > > > to ask the bridge if HPD is asserted or not and then fallback to the > > > GPIO method if the display phy/controller doesn't have support to check > > > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > > > If we could somehow get the HPD status from the bridge in the panel > > driver it definitely would be convenient. It does feel like that's an > > improvement that could be done later, though. We've already landed a > > few instances of doing what's done here, like for parade-ps8640 and > > analogix_dp. I suspect designing a new mechanism might not be the most > > trivial. > > What is done in the bridge drivers is to wait for a fixed timeout and > assume aux is ready? Or is it something else? If there's just a fixed > timeout for the eDP case it sounds OK to do that for now and we can fine > tune it later to actually check HPD status register before the panel > tries to read EDID. Right. For the parade chip (which is only used for eDP as far as I know--never DP) waits for up to 200 ms. See ps8640_ensure_hpd(). So I guess tl;dr to Sankeerth that it's OK for his patch to have the wait in the aux transfer function, but only for eDP. Other discussions here are about how we could make it better in future patches. > > I haven't actually tried it, but I suspect that to get something like > > what you're talking about we'd have to get the rest of drm to know > > that for eDP ports that it should assume something is connected > > _regardless_ of what the "detect" / "HPD" options say. Then we'd have > > to extend the edp-panel code to be able to be able to query the next > > bridge in the chain if a GPIO wasn't provided. > > Can the panel interrogate the bridge chain somehow? It feels like either > something in the chain should know the status of HPD (the case here > where display controller in the SoC knows) or it should be a gpio to the > panel (trogdor case). The bridge ops can implement DRM_BRIDGE_OP_HPD and > the first bridge from the encoder that supports HPD can implement some > sort of "wait for hpd asserted" function that the panel then uses once > it powers up the panel during probe. If the panel has a gpio and nothing > else in the chain can detect hpd then the panel polls the gpio, or it > waits for the amount of time delay after powering on the panel if the > panel's hpd function is called. Yeah, there ought to be some way to make something like that work. I don't think it's just DRM_BRIDGE_OP_HPD, though, for a few reasons: 1. That operation actually specifically means that HPD can cause an interrupt and that the bridge promises to call drm_bridge_hpd_notify() when the interrupt occurs. It seems to work hand-in-hand with "DRM_BRIDGE_OP_DETECT". I know it's legit to advertise "detect" without "HPD" (you have an HPD line that can be polled but not cause interrupts) but I'd have to research whether it's legal to advertise "HPD" without "detect". 2. If it were up to me, I'd rather avoid conflating what we need with the existing "HPD" and "DETECT" ops. While the line is called "HPD" in the eDP spec, what we're looking for here is really a different concept. eDP panels are never hot plugged and are always present, so I'd personally rather it be a new OP like "OP_PANEL_READY". Of course, in whatever future patch we could always debate this. 3. The main reason I'd prefer a different op is that I think using the existing ops will confuse DRM (not just because I'm being pedantic). If DRM sees that the eDP controller driver advertises that it can "detect" and support "hpd" then it will use these functions to decide whether it should start turning on the panel. ...and it won't even try using the panel until one is detected, right? ...but that means that it won't be powered on and will never be detected. ;-) This is what I'm trying to get at: it's a different concept. The panel is always there and never hotplugged. The existing DRM ops (IMO) are for knowing whether a panel is physically present. For eDP the answer is always a resounding "yes", even if we have no actual evidence (because we can't gather evidence for an "off" panel). On eDP the HPD line simply means something different than on DP. > > > > For the DP case this should not cause any significant overhead, right? > > > > HPD should always be asserted so this is basically just one extra IO > > > > read confirming that HPD is asserted which should be almost nothing... > > > > You're just about to do a whole bunch of IO reads/writes in order to > > > > program the AUX transaction anyway. > > > > > > In the DP case the dongle/cable can be disconnected in the middle of aux > > > transactions. If that happens we could be waiting a while in this > > > transfer function to timeout looking for the status bit. The driver > > > already gets an "unplug" irq when the cable is disconnected though so it > > > would be better to figure out a way to stop the aux transactions quickly > > > when that happens without having to read the hardware and poll the bit > > > that we already know is doomed to timeout. I think apple dongles throw > > > this logic for a loop though because the HDMI cable can be disconnected > > > from the dongle and then we don't see an "unplug" irq, just the number > > > of sinks becomes 0. Maybe there's an irq_hpd event, not sure. > > > > Ah, interesting. Having a DP cable unplugged in the middle of an aux > > transaction does seem like it could be a problem. What if we just wait > > in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"? That should > > be easy, right? > > Sounds like it would work. Is this supposed to fix some DP case as well > though? There were some patches to speed up aux failures when the dongle > was unplugged but I haven't checked after that. I guess this waiting is > only important for eDP because the edp-panel code is trying to read EDID > and it isn't waiting for HPD to be asserted before doing that. Right, I think this is only important for eDP. > > > That doesn't sound right. > > > My understanding is that HPD will be asserted after the panel is powered > > > up. Before that HPD is deasserted. Similarly, when we power down the > > > panel, HPD will be deasserted. I guess DRM wants to assume that an eDP > > > panel is always connected? That sounds like it might be OK as long as > > > userspace doesn't use "connected" to know that it's OK to do things like > > > read/write aux or push pixels to the panel when HPD is deasserted. > > > > IMO having userspace reading / writing aux directly and expecting it > > to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP > > case, but it's really not good in the eDP case. To me it's sorta like > > expecting things to be amazing and foolproof when you go behind the > > kernel's back and write to an i2c device using `i2cset -f`. Sure, it > > might work, but it can also confuse the heck out of things. It also > > turns out to be a huge problem when you get to PSR because userspace > > will get errors if it tries to write to the AUX channel and the panel > > is in PSR mode. This came up in the context of Brian's analogix dp > > patches [1]. The right answer, in my mind, is to treat userspace > > accessing the AUX channel directly as more of a debug feature, at > > least for eDP panels. > > If it's a debug feature then it should be removed from the system. The > flow of data is passing through the kernel so if the kernel is getting > confused about backdoor access over aux it should snoop the transactions > and block things it doesn't like. I don't know the backstory on aux > being exposed to userspace, but leaving it in a broken state isn't good. Agreed, it's not a great situation. :( -Doug
> -----Original Message----- > From: Doug Anderson <dianders@chromium.org> > Sent: Saturday, March 19, 2022 5:26 AM > To: Stephen Boyd <swboyd@chromium.org> > Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; dri-devel <dri-devel@lists.freedesktop.org>; > freedreno <freedreno@lists.freedesktop.org>; linux-arm-msm <linux-arm- > msm@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Clark > <robdclark@gmail.com>; Sean Paul <seanpaul@chromium.org>; > quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) > <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) > <quic_khsieh@quicinc.com>; Andy Gross <agross@kernel.org>; > bjorn.andersson@linaro.org; Rob Herring <robh+dt@kernel.org>; > krzk+dt@kernel.org; Sean Paul <sean@poorly.run>; David Airlie > <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Thierry Reding > <thierry.reding@gmail.com>; Sam Ravnborg <sam@ravnborg.org>; > dmitry.baryshkov@linaro.org; quic_vproddut <quic_vproddut@quicinc.com> > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink > interaction > > Hi, > > On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd <swboyd@chromium.org> > wrote: > > > > > > Pushing hpd state checking into aux transactions looks like the > > > > wrong direction. Also, as I said up above I am concerned that even > > > > checking the GPIO won't work and we need some way to ask the > > > > bridge if HPD is asserted or not and then fallback to the GPIO > > > > method if the display phy/controller doesn't have support to check > > > > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > > > > > If we could somehow get the HPD status from the bridge in the panel > > > driver it definitely would be convenient. It does feel like that's > > > an improvement that could be done later, though. We've already > > > landed a few instances of doing what's done here, like for > > > parade-ps8640 and analogix_dp. I suspect designing a new mechanism > > > might not be the most trivial. > > > > What is done in the bridge drivers is to wait for a fixed timeout and > > assume aux is ready? Or is it something else? If there's just a fixed > > timeout for the eDP case it sounds OK to do that for now and we can > > fine tune it later to actually check HPD status register before the > > panel tries to read EDID. > > Right. For the parade chip (which is only used for eDP as far as I know--never > DP) waits for up to 200 ms. See ps8640_ensure_hpd(). > > So I guess tl;dr to Sankeerth that it's OK for his patch to have the wait in the > aux transfer function, but only for eDP. Other discussions here are about > how we could make it better in future patches. > > The aux transactions for external DP are initiated by the dp_display driver only after the display is hot plugged to the connector. The phy_init is necessary for the aux transactions to take place. So, for the DP case, like Doug mentioned below, this patch is introducing an overhead of three register reads to detect hpd_high before performing aux transactions. So, we felt this was okay to do for DP. On the other hand, for eDP, it is necessary to wait for panel ready through this hpd connect status. Currently there is no way to know which type of connector it is in the dp_aux sub-module. However, as the discussion suggested, to have the wait only for eDP, I am thinking to pass the connector_type information to aux sub-module and register different aux_transfer functions for eDP and DP. The eDP transfer function will wait for hpd_high and the DP transfer function will be same as the one before this patch. What do you think? > > > I haven't actually tried it, but I suspect that to get something > > > like what you're talking about we'd have to get the rest of drm to > > > know that for eDP ports that it should assume something is connected > > > _regardless_ of what the "detect" / "HPD" options say. Then we'd > > > have to extend the edp-panel code to be able to be able to query the > > > next bridge in the chain if a GPIO wasn't provided. > > > > Can the panel interrogate the bridge chain somehow? It feels like > > either something in the chain should know the status of HPD (the case > > here where display controller in the SoC knows) or it should be a gpio > > to the panel (trogdor case). The bridge ops can implement > > DRM_BRIDGE_OP_HPD and the first bridge from the encoder that supports > > HPD can implement some sort of "wait for hpd asserted" function that > > the panel then uses once it powers up the panel during probe. If the > > panel has a gpio and nothing else in the chain can detect hpd then the > > panel polls the gpio, or it waits for the amount of time delay after > > powering on the panel if the panel's hpd function is called. > > Yeah, there ought to be some way to make something like that work. I don't > think it's just DRM_BRIDGE_OP_HPD, though, for a few reasons: > > 1. That operation actually specifically means that HPD can cause an interrupt > and that the bridge promises to call drm_bridge_hpd_notify() when the > interrupt occurs. It seems to work hand-in-hand with > "DRM_BRIDGE_OP_DETECT". I know it's legit to advertise "detect" > without "HPD" (you have an HPD line that can be polled but not cause > interrupts) but I'd have to research whether it's legal to advertise "HPD" > without "detect". > > 2. If it were up to me, I'd rather avoid conflating what we need with the > existing "HPD" and "DETECT" ops. While the line is called "HPD" in the eDP > spec, what we're looking for here is really a different concept. eDP panels are > never hot plugged and are always present, so I'd personally rather it be a > new OP like "OP_PANEL_READY". Of course, in whatever future patch we > could always debate this. > > 3. The main reason I'd prefer a different op is that I think using the existing > ops will confuse DRM (not just because I'm being pedantic). > If DRM sees that the eDP controller driver advertises that it can "detect" and > support "hpd" then it will use these functions to decide whether it should > start turning on the panel. ...and it won't even try using the panel until one is > detected, right? ...but that means that it won't be powered on and will never > be detected. ;-) This is what I'm trying to get at: it's a different concept. The > panel is always there and never hotplugged. The existing DRM ops (IMO) are > for knowing whether a panel is physically present. For eDP the answer is > always a resounding "yes", even if we have no actual evidence (because we > can't gather evidence for an "off" panel). On eDP the HPD line simply means > something different than on DP. > > > > > > > For the DP case this should not cause any significant overhead, right? > > > > > HPD should always be asserted so this is basically just one > > > > > extra IO read confirming that HPD is asserted which should be almost > nothing... > > > > > You're just about to do a whole bunch of IO reads/writes in > > > > > order to program the AUX transaction anyway. > > > > > > > > In the DP case the dongle/cable can be disconnected in the middle > > > > of aux transactions. If that happens we could be waiting a while > > > > in this transfer function to timeout looking for the status bit. > > > > The driver already gets an "unplug" irq when the cable is > > > > disconnected though so it would be better to figure out a way to > > > > stop the aux transactions quickly when that happens without having > > > > to read the hardware and poll the bit that we already know is > > > > doomed to timeout. I think apple dongles throw this logic for a > > > > loop though because the HDMI cable can be disconnected from the > > > > dongle and then we don't see an "unplug" irq, just the number of sinks > becomes 0. Maybe there's an irq_hpd event, not sure. > > > > > > Ah, interesting. Having a DP cable unplugged in the middle of an aux > > > transaction does seem like it could be a problem. What if we just > > > wait in the case our bridge.type is "DRM_MODE_CONNECTOR_eDP"? > That > > > should be easy, right? > > > > Sounds like it would work. Is this supposed to fix some DP case as > > well though? There were some patches to speed up aux failures when the > > dongle was unplugged but I haven't checked after that. I guess this > > waiting is only important for eDP because the edp-panel code is trying > > to read EDID and it isn't waiting for HPD to be asserted before doing that. > > Right, I think this is only important for eDP. > > > > > > That doesn't sound right. > > > > My understanding is that HPD will be asserted after the panel is > > > > powered up. Before that HPD is deasserted. Similarly, when we > > > > power down the panel, HPD will be deasserted. I guess DRM wants to > > > > assume that an eDP panel is always connected? That sounds like it > > > > might be OK as long as userspace doesn't use "connected" to know > > > > that it's OK to do things like read/write aux or push pixels to the panel > when HPD is deasserted. > > > > > > IMO having userspace reading / writing aux directly and expecting it > > > to work is a terrible idea anyway. It's _maybe_ sorta OK in the DP > > > case, but it's really not good in the eDP case. To me it's sorta > > > like expecting things to be amazing and foolproof when you go behind > > > the kernel's back and write to an i2c device using `i2cset -f`. > > > Sure, it might work, but it can also confuse the heck out of things. > > > It also turns out to be a huge problem when you get to PSR because > > > userspace will get errors if it tries to write to the AUX channel > > > and the panel is in PSR mode. This came up in the context of Brian's > > > analogix dp patches [1]. The right answer, in my mind, is to treat > > > userspace accessing the AUX channel directly as more of a debug > > > feature, at least for eDP panels. > > > > If it's a debug feature then it should be removed from the system. The > > flow of data is passing through the kernel so if the kernel is getting > > confused about backdoor access over aux it should snoop the > > transactions and block things it doesn't like. I don't know the > > backstory on aux being exposed to userspace, but leaving it in a broken > state isn't good. > > Agreed, it's not a great situation. :( > > -Doug
Hi, On Fri, Mar 25, 2022 at 8:54 AM Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > > -----Original Message----- > > From: Doug Anderson <dianders@chromium.org> > > Sent: Saturday, March 19, 2022 5:26 AM > > To: Stephen Boyd <swboyd@chromium.org> > > Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; open list:OPEN > > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > > <devicetree@vger.kernel.org>; dri-devel <dri-devel@lists.freedesktop.org>; > > freedreno <freedreno@lists.freedesktop.org>; linux-arm-msm <linux-arm- > > msm@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob Clark > > <robdclark@gmail.com>; Sean Paul <seanpaul@chromium.org>; > > quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) > > <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) > > <quic_khsieh@quicinc.com>; Andy Gross <agross@kernel.org>; > > bjorn.andersson@linaro.org; Rob Herring <robh+dt@kernel.org>; > > krzk+dt@kernel.org; Sean Paul <sean@poorly.run>; David Airlie > > <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Thierry Reding > > <thierry.reding@gmail.com>; Sam Ravnborg <sam@ravnborg.org>; > > dmitry.baryshkov@linaro.org; quic_vproddut <quic_vproddut@quicinc.com> > > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink > > interaction > > > > Hi, > > > > On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd <swboyd@chromium.org> > > wrote: > > > > > > > > Pushing hpd state checking into aux transactions looks like the > > > > > wrong direction. Also, as I said up above I am concerned that even > > > > > checking the GPIO won't work and we need some way to ask the > > > > > bridge if HPD is asserted or not and then fallback to the GPIO > > > > > method if the display phy/controller doesn't have support to check > > > > > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > > > > > > > If we could somehow get the HPD status from the bridge in the panel > > > > driver it definitely would be convenient. It does feel like that's > > > > an improvement that could be done later, though. We've already > > > > landed a few instances of doing what's done here, like for > > > > parade-ps8640 and analogix_dp. I suspect designing a new mechanism > > > > might not be the most trivial. > > > > > > What is done in the bridge drivers is to wait for a fixed timeout and > > > assume aux is ready? Or is it something else? If there's just a fixed > > > timeout for the eDP case it sounds OK to do that for now and we can > > > fine tune it later to actually check HPD status register before the > > > panel tries to read EDID. > > > > Right. For the parade chip (which is only used for eDP as far as I know--never > > DP) waits for up to 200 ms. See ps8640_ensure_hpd(). > > > > So I guess tl;dr to Sankeerth that it's OK for his patch to have the wait in the > > aux transfer function, but only for eDP. Other discussions here are about > > how we could make it better in future patches. > > > > > > The aux transactions for external DP are initiated by the dp_display driver only after the > display is hot plugged to the connector. The phy_init is necessary for the aux transactions > to take place. So, for the DP case, like Doug mentioned below, this patch is introducing > an overhead of three register reads to detect hpd_high before performing aux transactions. > So, we felt this was okay to do for DP. Personally I'm not that upset about the 3 register reads. The problem Stephen pointed out is bigger. It's possible that a DP cable is unplugged _just_ as we started an AUX transaction. In that case we'll have a big delay here when we don't actually need one. > On the other hand, for eDP, it is necessary to wait for panel ready through this hpd connect status. > Currently there is no way to know which type of connector it is in the dp_aux sub-module. > > However, as the discussion suggested, to have the wait only for eDP, I am thinking to pass the > connector_type information to aux sub-module and register different aux_transfer functions > for eDP and DP. The eDP transfer function will wait for hpd_high and the DP transfer function > will be same as the one before this patch. Personally I wouldn't register two separate functions. You could just store a boolean in your structure and only wait for HPD if this is eDP. One extra "if" test doesn't seem like it justifies splitting off into two functions... -Doug
> -----Original Message----- > From: Doug Anderson <dianders@chromium.org> > Sent: Friday, March 25, 2022 9:36 PM > To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> > Cc: Stephen Boyd <swboyd@chromium.org>; David Airlie <airlied@linux.ie>; > dri-devel <dri-devel@lists.freedesktop.org>; bjorn.andersson@linaro.org; > Thierry Reding <thierry.reding@gmail.com>; Sam Ravnborg > <sam@ravnborg.org>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; > Andy Gross <agross@kernel.org>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > quic_vproddut <quic_vproddut@quicinc.com>; linux-arm-msm <linux-arm- > msm@vger.kernel.org>; Abhinav Kumar (QUIC) > <quic_abhinavk@quicinc.com>; Rob Herring <robh+dt@kernel.org>; Sean > Paul <seanpaul@chromium.org>; Sean Paul <sean@poorly.run>; > quic_kalyant <quic_kalyant@quicinc.com>; LKML <linux- > kernel@vger.kernel.org>; dmitry.baryshkov@linaro.org; > krzk+dt@kernel.org; freedreno <freedreno@lists.freedesktop.org> > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink > interaction > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > Hi, > > On Fri, Mar 25, 2022 at 8:54 AM Sankeerth Billakanti (QUIC) > <quic_sbillaka@quicinc.com> wrote: > > > > > -----Original Message----- > > > From: Doug Anderson <dianders@chromium.org> > > > Sent: Saturday, March 19, 2022 5:26 AM > > > To: Stephen Boyd <swboyd@chromium.org> > > > Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; open > > > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > > > <devicetree@vger.kernel.org>; dri-devel > > > <dri-devel@lists.freedesktop.org>; > > > freedreno <freedreno@lists.freedesktop.org>; linux-arm-msm > > > <linux-arm- msm@vger.kernel.org>; LKML > > > <linux-kernel@vger.kernel.org>; Rob Clark <robdclark@gmail.com>; > > > Sean Paul <seanpaul@chromium.org>; quic_kalyant > > > <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) > > > <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) > > > <quic_khsieh@quicinc.com>; Andy Gross <agross@kernel.org>; > > > bjorn.andersson@linaro.org; Rob Herring <robh+dt@kernel.org>; > > > krzk+dt@kernel.org; Sean Paul <sean@poorly.run>; David Airlie > > > <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Thierry Reding > > > <thierry.reding@gmail.com>; Sam Ravnborg <sam@ravnborg.org>; > > > dmitry.baryshkov@linaro.org; quic_vproddut > > > <quic_vproddut@quicinc.com> > > > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any > > > sink interaction > > > > > > Hi, > > > > > > On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd <swboyd@chromium.org> > > > wrote: > > > > > > > > > > Pushing hpd state checking into aux transactions looks like > > > > > > the wrong direction. Also, as I said up above I am concerned > > > > > > that even checking the GPIO won't work and we need some way to > > > > > > ask the bridge if HPD is asserted or not and then fallback to > > > > > > the GPIO method if the display phy/controller doesn't have > > > > > > support to check HPD internally. Something on top of > DRM_BRIDGE_OP_HPD? > > > > > > > > > > If we could somehow get the HPD status from the bridge in the > > > > > panel driver it definitely would be convenient. It does feel > > > > > like that's an improvement that could be done later, though. > > > > > We've already landed a few instances of doing what's done here, > > > > > like for > > > > > parade-ps8640 and analogix_dp. I suspect designing a new > > > > > mechanism might not be the most trivial. > > > > > > > > What is done in the bridge drivers is to wait for a fixed timeout > > > > and assume aux is ready? Or is it something else? If there's just > > > > a fixed timeout for the eDP case it sounds OK to do that for now > > > > and we can fine tune it later to actually check HPD status > > > > register before the panel tries to read EDID. > > > > > > Right. For the parade chip (which is only used for eDP as far as I > > > know--never > > > DP) waits for up to 200 ms. See ps8640_ensure_hpd(). > > > > > > So I guess tl;dr to Sankeerth that it's OK for his patch to have the > > > wait in the aux transfer function, but only for eDP. Other > > > discussions here are about how we could make it better in future > patches. > > > > > > > > > > The aux transactions for external DP are initiated by the dp_display > > driver only after the display is hot plugged to the connector. The > > phy_init is necessary for the aux transactions to take place. So, for > > the DP case, like Doug mentioned below, this patch is introducing an > overhead of three register reads to detect hpd_high before performing aux > transactions. > > So, we felt this was okay to do for DP. > > Personally I'm not that upset about the 3 register reads. The problem > Stephen pointed out is bigger. It's possible that a DP cable is unplugged > _just_ as we started an AUX transaction. In that case we'll have a big delay > here when we don't actually need one. > > Okay. Got it > > On the other hand, for eDP, it is necessary to wait for panel ready through > this hpd connect status. > > Currently there is no way to know which type of connector it is in the > dp_aux sub-module. > > > > However, as the discussion suggested, to have the wait only for eDP, I > > am thinking to pass the connector_type information to aux sub-module > > and register different aux_transfer functions for eDP and DP. The eDP > > transfer function will wait for hpd_high and the DP transfer function will be > same as the one before this patch. > > Personally I wouldn't register two separate functions. You could just store a > boolean in your structure and only wait for HPD if this is eDP. One extra "if" > test doesn't seem like it justifies splitting off into two functions... > > -Doug Okay. I will make that change. Thank you.
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..2ddc303 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; } + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); + if (ret) { + DRM_DEBUG_DP("DP sink not ready for aux transactions\n"); + goto exit; + } + dp_aux_update_offset_and_segment(aux, msg); dp_aux_transfer_helper(aux, msg, true); diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..2c3b0f7 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); } +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +{ + u32 state, hpd_en, timeout; + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL) & + DP_DP_HPD_CTRL_HPD_EN; + + /* no-hpd case */ + if (!hpd_en) + return 0; + + /* Poll for HPD connected status */ + timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) & + DP_HPD_CONNECT_TIME_MASK; + + return readl_poll_timeout(catalog->io->dp_controller.aux.base + + REG_DP_DP_HPD_INT_STATUS, + state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, + 2000, timeout); +} + static void dump_regs(void __iomem *base, int len) { int i; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 7dea101..45140a3 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog); void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog); /* DP Controller APIs */ diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h index 2686028..d68c71b 100644 --- a/drivers/gpu/drm/msm/dp/dp_reg.h +++ b/drivers/gpu/drm/msm/dp/dp_reg.h @@ -53,9 +53,14 @@ #define DP_DP_HPD_REFTIMER_ENABLE (1 << 16) #define REG_DP_DP_HPD_EVENT_TIME_0 (0x0000001C) -#define REG_DP_DP_HPD_EVENT_TIME_1 (0x00000020) #define DP_DP_HPD_EVENT_TIME_0_VAL (0x3E800FA) +#define DP_HPD_GLITCH_TIME_MASK (0xFFFC0000) +#define DP_HPD_CONNECT_TIME_MASK (0x0003FFFF) + +#define REG_DP_DP_HPD_EVENT_TIME_1 (0x00000020) #define DP_DP_HPD_EVENT_TIME_1_VAL (0x1F407D0) +#define DP_HPD_DISCONNECT_TIME_MASK (0xFFFFC000) +#define DP_IRQ_HPD_MAX_TIME_MASK (0x00003FFF) #define REG_DP_AUX_CTRL (0x00000030) #define DP_AUX_CTRL_ENABLE (0x00000001)
The source device should ensure the sink is ready before proceeding to read the sink capability or performing any aux transactions. The sink will indicate its readiness by asserting the HPD line. The eDP sink requires power from the source and its HPD line will be asserted only after the panel is powered on. The panel power will be enabled from the panel-edp driver. The controller driver needs to wait for the hpd line to be asserted by the sink. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_aux.c | 6 ++++++ drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_reg.h | 7 ++++++- 4 files changed, 36 insertions(+), 1 deletion(-)