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
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
real 0m8.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 <[email protected]>
---
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, 500000);
}
+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
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 <[email protected]>
---
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,
+ 500000);
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, 500000);
+ 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
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 <[email protected]>
---
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,
- 500000);
- 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
Hi,
On Tue, Mar 12, 2024 at 5:47 PM Guenter Roeck <[email protected]> wrote:
>
> On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson <[email protected]> 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
> >
> > real 0m8.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 <[email protected]>
> > ---
> >
> > 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, 500000);
> > }
> >
> > +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
On 3/12/2024 5:13 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.
>
I do see
"
* Also note that this callback can be called no matter the
* state @dev is in and also no matter what state the panel is
* in. It's expected:
"
I understand about the host state that we need to allow the transfers by
powering on if the host was off.
But I wonder why we should allow the transfer if the sink is not
connected because it will anyway timeout.
Does it make sense to have get_hpd_status() from the aux dev and not
issue the transfers if the sink was not connected?
This is more of questioning the intent of drm_dp_helpers to allow
transfers without checking the sink status.
> 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
>
> real 0m8.200s
>
IIUC, we want to timeout faster by not bailing out if not connected right?
> 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 <[email protected]>
> ---
>
> 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, 500000);
> }
>
> +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;
> +}
This method of checking HPD status works for devices which use internal
HPD block to control the HPD (like sc7180/sc7280) but not for devices
where HPD is controlled outside the MSM DP controller like sc8280xp,
sc835-/sm8450 etc etc which use pmic_glink and DP driver only receives
the hpd status using the dp_bridge_hpd_notify() callback.
If we want to make this generic, we have to do something like:
dp_hpd_unplug_handle() notifies the dp_aux.c module that status is
disconncted and we should bail out
dp_hpd_plug_handle() notifies dp_aux.c module that status is connected
again and we allow the aux transfers.
> +
> 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 */
Hi,
On Wed, Mar 13, 2024 at 1:41 PM Abhinav Kumar <[email protected]> wrote:
>
>
>
> On 3/12/2024 5:13 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.
> >
>
> I do see
>
> "
> * Also note that this callback can be called no matter the
> * state @dev is in and also no matter what state the panel is
> * in. It's expected:
> "
>
> I understand about the host state that we need to allow the transfers by
> powering on if the host was off.
>
> But I wonder why we should allow the transfer if the sink is not
> connected because it will anyway timeout.
We shouldn't! That's what this patch is about. ;-)
> Does it make sense to have get_hpd_status() from the aux dev and not
> issue the transfers if the sink was not connected?
>
> This is more of questioning the intent of drm_dp_helpers to allow
> transfers without checking the sink status.
It's a good question. I guess some of this just comes from the
abstraction that we currently have.
Thinking about this, the ideal would be to somehow query back to the
"drm_connector" since it already has a "->detect" function. ...but we
can't really do this since the AUX bus needs to be able to do
transfers early before all the DRM components aren't initialized. This
is, for instance, how the eDP panel code queries the EDID while being
probed.
We could consider adding a new callback to "struct drm_dp_aux" that
would allow checking the HPD status, but it feels to me like this adds
unneeded complexity. We'd be adding a callback that people need to
think about just to avoid them adding an "if" statement to their AUX
transfer routine. I'm not totally convinced.
Interestingly, we actually _could_ use the infrastructure I just
introduced in commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX
transfers when eDP panels are not powered") here, at least if we're in
the DP case and not the eDP case. When we're in the DP case there is
no panel involved so the DP driver itself knows when things are
"powered". For now I'm _not_ going to do this since it feels to me
like the "if" test makes it clearer what's happening, but yell if you
want me to change it.
> > 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
> >
> > real 0m8.200s
> >
>
> IIUC, we want to timeout faster by not bailing out if not connected right?
Correct. I can try to clarify the commit message for v2 to make this
more obvious.
> > 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 <[email protected]>
> > ---
> >
> > 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, 500000);
> > }
> >
> > +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;
> > +}
>
> This method of checking HPD status works for devices which use internal
> HPD block to control the HPD (like sc7180/sc7280) but not for devices
> where HPD is controlled outside the MSM DP controller like sc8280xp,
> sc835-/sm8450 etc etc which use pmic_glink and DP driver only receives
> the hpd status using the dp_bridge_hpd_notify() callback.
>
> If we want to make this generic, we have to do something like:
>
> dp_hpd_unplug_handle() notifies the dp_aux.c module that status is
> disconncted and we should bail out
>
> dp_hpd_plug_handle() notifies dp_aux.c module that status is connected
> again and we allow the aux transfers.
Ah, good point about devices where HPD comes from elsewhere. OK, using
dp_hpd_plug_handle() and dp_hpd_unplug_handle() and having it notify
"dp_aux.c" seems to work OK for the external DP case (at least the one
on trogdor). It didn't work when I tested it on eDP but I can make
this rule just for non-eDP since they don't have the same issues.
-Doug
On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson <dianders@chromiumorg> 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
>
> real 0m8.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 <[email protected]>
> ---
>
> 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, 500000);
> }
>
> +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
>