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 <dianders@xxxxxxxxxxxx>
---
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 */
