Re: [PATCH v3 6/7] arm64: dts: qcom: sm8650: add GPU nodes
On 12/03/2024 01:20, Konrad Dybcio wrote:
On 2/16/24 12:03, Neil Armstrong wrote:
Add GPU nodes for the SM8650 platform.
Signed-off-by: Neil Armstrong
---
arch/arm64/boot/dts/qcom/sm8650.dtsi | 166 +++
1 file changed, 166 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi
b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 62e6ae93a9a8..27dcef27b6ad 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2589,6 +2589,128 @@ tcsr: clock-controller@1fc {
#reset-cells = <1>;
};
+ gpu: gpu@3d0 {
+ compatible = "qcom,adreno-43051401", "qcom,adreno";
+ reg = <0x0 0x03d0 0x0 0x4>,
+ <0x0 0x03d9e000 0x0 0x1000>,
+ <0x0 0x03d61000 0x0 0x800>;
+ reg-names = "kgsl_3d0_reg_memory",
+ "cx_mem",
+ "cx_dbgc";
+
+ interrupts = ;
+
+ iommus = <&adreno_smmu 0 0x0>,
+ <&adreno_smmu 1 0x0>;
+
+ operating-points-v2 = <&gpu_opp_table>;
+
+ qcom,gmu = <&gmu>;
+
+ status = "disabled";
+
+ zap-shader {
+ memory-region = <&gpu_micro_code_mem>;
+ };
+
+ /* Speedbin needs more work on A740+, keep only lower freqs */
+ gpu_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-68000 {
+ opp-hz = /bits/ 64 <68000>;
+ opp-level = ;
+ };
I got a memo from krzk that we should be sorting OPPs low-to-high,
could you please reorder these (and under gmu)?
Ack, I also add 3 more OPPs that works with all speedbins.
Neil
Otherwise lgtm
Konrad
Re: drm/msm: DisplayPort hard-reset on hotplug regression in 6.8-rc1
On Mon, Mar 11, 2024 at 09:51:29AM -0700, Abhinav Kumar wrote:
> On 3/11/2024 6:43 AM, Johan Hovold wrote:
> > On Sat, Mar 09, 2024 at 05:30:17PM +0100, Johan Hovold wrote:
> >> On Fri, Mar 08, 2024 at 09:50:17AM -0800, Abhinav Kumar wrote:
> >>> I have posted my analysis with the patch here as a RFC y'day:
> >>>
> >>> https://patchwork.freedesktop.org/patch/581758/
> > I was able to reproduce the reset with some of my debug printks in place
> > after reapplying the reverted hpd notify change so I have an explanation
> > for (one of) the ways we can up in this state now.
> >
> > This does not match description of the problem in the fix linked to
> > above and I don't think that patch solves the underlying issue even if
> > it may make the race window somewhat smaller. More details below.
> Its the same condition you described below that enable does not go
> through and we bail out as we are in ST_DISCONNECTED.
It's closely related but clearly not the same as user space is not
involved in the reset I see.
> > Turns out there are paths like that and we hit those more often before
> > reverting e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify().
> >
> > Specifically, when a previous connect attempt did not enable the bridge
> > fully so that it is still in the ST_MAINLINK_READY when we receive a
> > disconnect event, dp_hpd_unplug_handle() will turn of the link clock.
> >
> > [ 204.527625] msm-dp-display ae98000.displayport-controller:
> > dp_bridge_hpd_notify - link_ready = 1, status = 2
> > [ 204.531553] msm-dp-display ae98000.displayport-controller:
> > dp_hpd_unplug_handle
> > [ 204.533261] msm-dp-display ae98000.displayport-controller:
> > dp_ctrl_off_link
> >
> > A racing connect event, such as the one I described earlier, can then
> > try to enable the bridge again but dp_bridge_atomic_enable() just bails
> > out early (and leaks a rpm reference) because we're now in
> > ST_DISCONNECTED:
> >
> > [ 204.535773] msm-dp-display ae98000.displayport-controller:
> > dp_bridge_hpd_notify - link_ready = 1, status = 1
> > [ 204.536187] [CONNECTOR:35:DP-2] status updated from disconnected to
> > connected
> > [ 204.536905] msm-dp-display ae98000.displayport-controller:
> > dp_display_notify_disconnect - would clear link ready (1), state = 0
> > [ 204.537821] msm-dp-display ae98000.displayport-controller:
> > dp_bridge_atomic_check - link_ready = 1
> > [ 204.538063] msm-dp-display ae98000.displayport-controller:
> > dp_display_send_hpd_notification - hpd = 0, link_ready = 1
> > [ 204.542778] msm-dp-display ae98000.displayport-controller:
> > dp_bridge_atomic_enable
> > [ 204.586547] msm-dp-display ae98000.displayport-controller:
> > dp_bridge_atomic_enable - state = 0 (rpm leak?)
> >
> > Clearing link_ready already in dp_display_notify_disconnect() would make
> > the race window slightly smaller, but it would essentially just paper
> > over the bug as the events are still not serialised. Notably, there is
> > no user space interaction involved here and it's the spurious connect
> > event that triggers the bridge enable.
> Yes, it only narrows down the race condition window. The issue can still
> happen if the commit / modeset was issued before we marked link_ready as
> false.
>
> And yes, I was only targetting a short term fix till we rework the HPD.
> That will happen only incrementally as its a delicate piece of code.
Ok, thanks for confirming. Please also make that clear in the commit
message of any patch.
I am however not sure that your patch (RFC) is needed at this point as
the HPD revert fixes the 6.8-rc1 regression, and moving the clearing of
link_ready can actually make things worse as it makes any spurious
hotplug event always be processed (not just if they happen after
dp_display_send_hpd_notification()).
I'll reply to you patch as well.
> > So, while it may still be theoretically possible to hit the resets after
> > the revert, the HPD notify revert effectively "fixed" the regression in
> > 6.8-rc1 by removing the preconditions that now made us hit it (i.e. the
> > half-initialised bridge).
> Not entirely. In the bug which was reported before the patch
> e467e0bde881 ("drm/msm/dp: use drm_bridge_hpd_notify() got landed, its a
> classic example of how this issue can happen with userspace involvement
> and not just fbdev client which was your case.
Sure, but you already added some kind of band-aid for that issue, right?
https://lore.kernel.org/all/1664408211-25314-1-git-send-email-quic_khs...@quicinc.com/
How likely is it that you'd still hit that? Have you had an reports of
anyone actually hitting that issue since the above workaround was
merged?
Note that I, and other users of the X13s, only started hitting the
resets with 6.8-rc1, which broke hotplug notification and resulted in
the half-initialised bridges.
I'm not saying it's impossible to hit the unclocked access still, just
that that does not seem to be relevant for the regression.
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote:
> There are cases where the userspace might still send another
> frame after the HPD disconnect causing a modeset cycle after
> a disconnect. This messes the internal state machine of MSM DP driver
> and can lead to a crash as there can be an imbalance between
> bridge_disable() and bridge_enable().
Can you be more specific here? What steps would lead to this issue and
how exactly does is mess with the state machine? Is there an easy way
to reproduce it (e.g. by instrumenting the code with some sleep)?
The hotplug code is really convoluted and having a clear description of
the problem is needed to evaluate the patch (including when revisiting
it some time from now when I've forgotten about how this state machine
works).
As you know, we ran into a related issue on sc8280xp (X13s) since
6.8-rc1, but that did not involve any user space interaction at all.
For reference, there are some more details in this thread:
https://lore.kernel.org/all/ze8ke_m2xhypy...@hovoldconsulting.com/
> This was also previously reported on [1] for which [2] was posted
> and helped resolve the issue by rejecting commits if the DP is not
> in connected state.
>
> The change resolved the bug but there can also be another race condition.
> If hpd_event_thread does not pick up the EV_USER_NOTIFICATION and process it
> link_ready will also not be set to false allowing the frame to sneak in.
How could the event thread fail to pick up the notification event? Or do
you mean there's a race window before it has been processed?
> Lets move setting link_ready outside of hpd_event_thread() processing to
> eliminate a window of race condition.
As we discussed in thread above, this patch does not eliminate the race,
even if it may reduce the race window.
> [1] : https://gitlab.freedesktop.org/drm/msm/-/issues/17
> [2] :
> https://lore.kernel.org/all/1664408211-25314-1-git-send-email-quic_khs...@quicinc.com/
>
> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable
> and disable")
> Signed-off-by: Abhinav Kumar
> @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device
> *dev)
> {
> struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
> + dp->dp_display.link_ready = false;
As I also pointed out in the other thread, setting link_ready to false
here means that any spurious connect event (during physical disconnect)
will always be processed, something which can currently lead to a leaked
runtime pm reference.
Wasting some power is of course preferred over crashing the machine, but
please take it into consideration anyway.
Especially if your intention with this patch was to address the resets
we saw with sc8280xp which are gone since the HPD notify revert (which
fixed the hotplug detect issue that left the bridge in a
half-initialised state).
> +
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>
> return 0;
Johan
Re: [PATCH v2 2/7] clk: qcom: clk-alpha-pll: Add HUAYRA_2290 support
On 2/23/24 23:48, Trilok Soni wrote: On 2/23/2024 1:21 PM, Konrad Dybcio wrote: + /* Wait 50us for PLL_LOCK_DET bit to go high */ + usleep_range(50, 55); + + /* Enable PLL output */ + regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, PLL_OUTCTRL); +} +EXPORT_SYMBOL(clk_huayra_2290_pll_configure); Please use EXPORT_SYMBOL_GPL. Sure, I glanced over this! I've also noticed that it's a very common oversight.. would you be interested in extending scripts/checkpatch.pl to suggest the _GPL variant? Konrad
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote:
> On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote:
> > @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device
> > *dev)
> > {
> > struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >
> > + dp->dp_display.link_ready = false;
>
> As I also pointed out in the other thread, setting link_ready to false
> here means that any spurious connect event (during physical disconnect)
> will always be processed, something which can currently lead to a leaked
> runtime pm reference.
>
> Wasting some power is of course preferred over crashing the machine, but
> please take it into consideration anyway.
>
> Especially if your intention with this patch was to address the resets
> we saw with sc8280xp which are gone since the HPD notify revert (which
> fixed the hotplug detect issue that left the bridge in a
> half-initialised state).
Heh. This is getting ridiculous. I just tried running with this patch
and it again breaks hotplug detect in a VT console and in X (where I
could enable a reconnected external display by running xrandr twice
before).
So, please, do not apply this one.
Johan
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On Tue, Mar 12, 2024 at 05:41:23PM +0100, Johan Hovold wrote:
> On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote:
> > On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote:
>
> > > @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device
> > > *dev)
> > > {
> > > struct dp_display_private *dp = dev_get_dp_display_private(dev);
> > >
> > > + dp->dp_display.link_ready = false;
> >
> > As I also pointed out in the other thread, setting link_ready to false
> > here means that any spurious connect event (during physical disconnect)
> > will always be processed, something which can currently lead to a leaked
> > runtime pm reference.
> >
> > Wasting some power is of course preferred over crashing the machine, but
> > please take it into consideration anyway.
> >
> > Especially if your intention with this patch was to address the resets
> > we saw with sc8280xp which are gone since the HPD notify revert (which
> > fixed the hotplug detect issue that left the bridge in a
> > half-initialised state).
>
> Heh. This is getting ridiculous. I just tried running with this patch
> and it again breaks hotplug detect in a VT console and in X (where I
> could enable a reconnected external display by running xrandr twice
> before).
>
> So, please, do not apply this one.
To make things worse, I indeed also hit the reset when disconnecting
after such a failed hotplug.
Johan
Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread
On 3/12/2024 9:59 AM, Johan Hovold wrote:
On Tue, Mar 12, 2024 at 05:41:23PM +0100, Johan Hovold wrote:
On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote:
On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote:
@@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device *dev)
{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
+ dp->dp_display.link_ready = false;
As I also pointed out in the other thread, setting link_ready to false
here means that any spurious connect event (during physical disconnect)
will always be processed, something which can currently lead to a leaked
runtime pm reference.
Wasting some power is of course preferred over crashing the machine, but
please take it into consideration anyway.
Especially if your intention with this patch was to address the resets
we saw with sc8280xp which are gone since the HPD notify revert (which
fixed the hotplug detect issue that left the bridge in a
half-initialised state).
Heh. This is getting ridiculous. I just tried running with this patch
and it again breaks hotplug detect in a VT console and in X (where I
could enable a reconnected external display by running xrandr twice
before).
So, please, do not apply this one.
To make things worse, I indeed also hit the reset when disconnecting
after such a failed hotplug.
Johan
Ack, I will hold off till I analyze your issues more which you have
listed in separate replies. Especially about the spurious connect, I
believe you are trying to mention that, by adding logs, you are able to
delay the processing of a connect event to *make* it like a spurious
one? In case, I got this part wrong, can you pls explain the spurious
connect scenario again?
A short response on why this change was made is that commit can be
issued by userspace or the fbdev client. So userspace involvement only
makes commit happen from a different path. It would be incorrect to
assume the issues from the earlier bug and the current one are different
only because there was userspace involvement in that one and not this.
Because in the end, it manifests itself in the same way that
atomic_enable() did not go through after an atomic_disable() and the
next atomic_disable() crashes.
Reverting the hpd_notify patch only eliminated some paths but I think
both you and me agree the issue can still happen and in the very same
way. So till someone else reports this issue, till HPD is reworked, I
wanted to do whats possible to avoid this situation. If users are fine
with what we have, I have no inclination to push this.
[PATCH 0/3] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions
The main goal of this patch series is to avoid problems running "fwupd" on Qualcomm devices. Right now several of the plugins used with fwupd try talking over all DP AUX busses and this results in a very long timeout on Qualcomm devices. As part of fixing this, I noticed a case where the MSM DP code wasn't respecing the timeout properly when asked to wait for HPD. I also noticed that, now that we've implemented wait_hpd_asserted(), we no longer need the long hardcoded timeout / special cse for eDP in the AUX transfer function. NOTE: I no longer have any hardware setup that uses this driver for eDP so I've only tested the DP case. The eDP changes are straightforward so hopefully there are no problems there. Douglas Anderson (3): drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer drivers/gpu/drm/msm/dp/dp_aux.c | 21 - drivers/gpu/drm/msm/dp/dp_catalog.c | 17 ++--- drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +++- 3 files changed, 25 insertions(+), 17 deletions(-) -- 2.44.0.278.ge034bb2e1d-goog
[PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
As documented in the description of the transfer() function of
"struct drm_dp_aux", the transfer() function can be called at any time
regardless of the state of the DP port. Specifically if the kernel has
the DP AUX character device enabled and userspace accesses
"/dev/drm_dp_auxN" directly then the AUX transfer function will be
called regardless of whether a DP device is connected.
For eDP panels we have a special rule where we wait (with a 5 second
timeout) for HPD to go high. This rule was important before all panels
drivers were converted to call wait_hpd_asserted() and actually can be
removed in a future commit.
For external DP devices we never checked for HPD. That means that
trying to access the DP AUX character device (AKA `hexdump -C
/dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
system:
$ time hexdump -C /dev/drm_dp_aux0
hexdump: /dev/drm_dp_aux0: Connection timed out
real0m8.200s
Let's add a check for HPD to avoid the slow timeout. This matches
what, for instance, the intel_dp_aux_xfer() function does when it
calls intel_tc_port_connected_locked(). That call has a document by it
explaining that it's important to avoid the long timeouts.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Douglas Anderson
---
drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 03f4951c49f4..de0b0eabced9 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
* turned on the panel and then tried to do an AUX transfer. The panel
* driver has no way of knowing when the panel is ready, so it's up
* to us to wait. For DP we never get into this situation so let's
-* avoid ever doing the extra long wait for DP.
+* avoid ever doing the extra long wait for DP and just query HPD
+* directly.
*/
if (aux->is_edp) {
ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
@@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
DRM_DEBUG_DP("Panel not ready for aux transactions\n");
goto exit;
}
+ } else {
+ if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
+ ret = -ENXIO;
+ goto exit;
+ }
}
dp_aux_update_offset_and_segment(aux, msg);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 5142aeb705a4..93e2d413a1e7 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct
dp_catalog *dp_catalog)
2000, 50);
}
+bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ /* poll for hpd connected status every 2ms and timeout after 500ms */
+ return readl(catalog->io->dp_controller.aux.base +
REG_DP_DP_HPD_INT_STATUS) &
+ DP_DP_HPD_STATE_STATUS_CONNECTED;
+}
+
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 38786e855b51..1694040c530f 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -86,6 +86,7 @@ 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);
+bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog);
u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
/* DP Controller APIs */
--
2.44.0.278.ge034bb2e1d-goog
[PATCH 3/3] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
Before the introduction of the wait_hpd_asserted() callback in commit
841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
that it was up to the AUX bus driver to wait for HPD in the transfer()
function.
Now wait_hpd_asserted() has been added. The two panel drivers that are
DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
3b5765df375c ("drm/panel: atna33xc20: Take advantage of
wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
no longer any reason for long wait in the AUX transfer() function.
Remove it.
NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
optional for the DP AUX bus to implement. In the case of the MSM DP
driver we implement it so we can assume it will be called.
ALSO NOTE: the wait wasn't actually _hurting_ anything and wasn't even
causing long timeouts, but it's still nice to get rid of unneeded
code. Specificaly it's not truly needed because to handle other DP
drivers that can't power on as quickly (specifically parade-ps8640) we
already avoid DP AUX transfers for eDP panels that aren't powered
on. See commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX transfers when
eDP panels are not powered").
Signed-off-by: Douglas Anderson
---
drivers/gpu/drm/msm/dp/dp_aux.c | 26 +++---
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index fc398e8a69a7..dd62ad6007a6 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -302,26 +302,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
}
/*
-* For eDP it's important to give a reasonably long wait here for HPD
-* to be asserted. This is because the panel driver may have _just_
-* turned on the panel and then tried to do an AUX transfer. The panel
-* driver has no way of knowing when the panel is ready, so it's up
-* to us to wait. For DP we never get into this situation so let's
-* avoid ever doing the extra long wait for DP and just query HPD
-* directly.
+* If HPD isn't asserted then the transfer won't succeed. Return
+* right away. If we don't do this we can end up with long timeouts
+* if someone tries to access the DP AUX character device when no
+* DP device is connected.
*/
- if (aux->is_edp) {
- ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog,
- 50);
- if (ret) {
- DRM_DEBUG_DP("Panel not ready for aux transactions\n");
- goto exit;
- }
- } else {
- if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
- ret = -ENXIO;
- goto exit;
- }
+ if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
+ ret = -ENXIO;
+ goto exit;
}
dp_aux_update_offset_and_segment(aux, msg);
--
2.44.0.278.ge034bb2e1d-goog
[PATCH 2/3] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback
The DP wait_hpd_asserted() callback is passed a timeout which
indicates how long we should wait for HPD. This timeout was being
ignored in the MSM DP implementation and instead a hardcoded 500 ms
timeout was used. Fix it to use the proper timeout.
As part of this we move the hardcoded 500 ms number into the AUX
transfer function, which isn't given a timeout. The wait in the AUX
transfer function will be removed in a future commit.
Fixes: e2969ee30252 ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()")
Signed-off-by: Douglas Anderson
---
drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++--
drivers/gpu/drm/msm/dp/dp_catalog.c | 7 ---
drivers/gpu/drm/msm/dp/dp_catalog.h | 3 ++-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index de0b0eabced9..fc398e8a69a7 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -311,7 +311,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
* directly.
*/
if (aux->is_edp) {
- ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog,
+ 50);
if (ret) {
DRM_DEBUG_DP("Panel not ready for aux transactions\n");
goto exit;
@@ -516,7 +517,7 @@ static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux,
aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
pm_runtime_get_sync(aux->dev);
- ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog, wait_us);
pm_runtime_put_sync(aux->dev);
return ret;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 93e2d413a1e7..b45cf3174aa0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -253,17 +253,18 @@ 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)
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog,
+ unsigned long wait_us)
{
u32 state;
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- /* poll for hpd connected status every 2ms and timeout after 500ms */
+ /* poll for hpd connected status every 2ms and timeout after wait_us */
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, 50);
+ min(wait_us, 2000), wait_us);
}
bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 1694040c530f..4248c8de5cf7 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -85,7 +85,8 @@ 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);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog,
+ unsigned long wait_us);
bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog);
u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
--
2.44.0.278.ge034bb2e1d-goog
Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson wrote:
>
> As documented in the description of the transfer() function of
> "struct drm_dp_aux", the transfer() function can be called at any time
> regardless of the state of the DP port. Specifically if the kernel has
> the DP AUX character device enabled and userspace accesses
> "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> called regardless of whether a DP device is connected.
>
> For eDP panels we have a special rule where we wait (with a 5 second
> timeout) for HPD to go high. This rule was important before all panels
> drivers were converted to call wait_hpd_asserted() and actually can be
> removed in a future commit.
>
> For external DP devices we never checked for HPD. That means that
> trying to access the DP AUX character device (AKA `hexdump -C
> /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> system:
> $ time hexdump -C /dev/drm_dp_aux0
> hexdump: /dev/drm_dp_aux0: Connection timed out
>
> real0m8.200s
>
> Let's add a check for HPD to avoid the slow timeout. This matches
> what, for instance, the intel_dp_aux_xfer() function does when it
> calls intel_tc_port_connected_locked(). That call has a document by it
> explaining that it's important to avoid the long timeouts.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Douglas Anderson
> ---
>
> drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++-
> drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++
> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 03f4951c49f4..de0b0eabced9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> * turned on the panel and then tried to do an AUX transfer. The panel
> * driver has no way of knowing when the panel is ready, so it's up
> * to us to wait. For DP we never get into this situation so let's
> -* avoid ever doing the extra long wait for DP.
> +* avoid ever doing the extra long wait for DP and just query HPD
> +* directly.
> */
> if (aux->is_edp) {
> ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> DRM_DEBUG_DP("Panel not ready for aux
> transactions\n");
> goto exit;
> }
> + } else {
> + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
> + ret = -ENXIO;
> + goto exit;
> + }
> }
>
> dp_aux_update_offset_and_segment(aux, msg);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 5142aeb705a4..93e2d413a1e7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct
> dp_catalog *dp_catalog)
> 2000, 50);
> }
>
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
> +{
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + /* poll for hpd connected status every 2ms and timeout after 500ms */
Maybe I am missing something, but the comment doesn't seem to match
the code below.
Guenter
> + return readl(catalog->io->dp_controller.aux.base +
> REG_DP_DP_HPD_INT_STATUS) &
> + DP_DP_HPD_STATE_STATUS_CONNECTED;
> +}
> +
> 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 38786e855b51..1694040c530f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -86,6 +86,7 @@ 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);
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog);
> u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>
> /* DP Controller APIs */
> --
> 2.44.0.278.ge034bb2e1d-goog
>
Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
Hi,
On Tue, Mar 12, 2024 at 5:47 PM Guenter Roeck wrote:
>
> On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson
> wrote:
> >
> > As documented in the description of the transfer() function of
> > "struct drm_dp_aux", the transfer() function can be called at any time
> > regardless of the state of the DP port. Specifically if the kernel has
> > the DP AUX character device enabled and userspace accesses
> > "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> > called regardless of whether a DP device is connected.
> >
> > For eDP panels we have a special rule where we wait (with a 5 second
> > timeout) for HPD to go high. This rule was important before all panels
> > drivers were converted to call wait_hpd_asserted() and actually can be
> > removed in a future commit.
> >
> > For external DP devices we never checked for HPD. That means that
> > trying to access the DP AUX character device (AKA `hexdump -C
> > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> > system:
> > $ time hexdump -C /dev/drm_dp_aux0
> > hexdump: /dev/drm_dp_aux0: Connection timed out
> >
> > real0m8.200s
> >
> > Let's add a check for HPD to avoid the slow timeout. This matches
> > what, for instance, the intel_dp_aux_xfer() function does when it
> > calls intel_tc_port_connected_locked(). That call has a document by it
> > explaining that it's important to avoid the long timeouts.
> >
> > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> > Signed-off-by: Douglas Anderson
> > ---
> >
> > drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++-
> > drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++
> > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> > 3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index 03f4951c49f4..de0b0eabced9 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> > *dp_aux,
> > * turned on the panel and then tried to do an AUX transfer. The
> > panel
> > * driver has no way of knowing when the panel is ready, so it's up
> > * to us to wait. For DP we never get into this situation so let's
> > -* avoid ever doing the extra long wait for DP.
> > +* avoid ever doing the extra long wait for DP and just query HPD
> > +* directly.
> > */
> > if (aux->is_edp) {
> > ret =
> > dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> > *dp_aux,
> > DRM_DEBUG_DP("Panel not ready for aux
> > transactions\n");
> > goto exit;
> > }
> > + } else {
> > + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
> > + ret = -ENXIO;
> > + goto exit;
> > + }
> > }
> >
> > dp_aux_update_offset_and_segment(aux, msg);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 5142aeb705a4..93e2d413a1e7 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct
> > dp_catalog *dp_catalog)
> > 2000, 50);
> > }
> >
> > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
> > +{
> > + struct dp_catalog_private *catalog = container_of(dp_catalog,
> > + struct dp_catalog_private, dp_catalog);
> > +
> > + /* poll for hpd connected status every 2ms and timeout after 500ms
> > */
>
> Maybe I am missing something, but the comment doesn't seem to match
> the code below.
>
> Guenter
>
> > + return readl(catalog->io->dp_controller.aux.base +
> > REG_DP_DP_HPD_INT_STATUS) &
> > + DP_DP_HPD_STATE_STATUS_CONNECTED;
LOL. I guess I overlooked that. Thanks for catching! The comment got
copied from the dp_catalog_aux_wait_for_hpd_connect_state(). I'll
remove the comment and send a v2, but I'll wait a little bit to see if
there is additional feedback.
-Doug
