linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions
@ 2024-03-15 21:36 Douglas Anderson
  2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Douglas Anderson @ 2024-03-15 21:36 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov
  Cc: Douglas Anderson, Bjorn Andersson, Daniel Vetter, David Airlie,
	Guenter Roeck, Kuogee Hsieh, Marijn Suijten, Sean Paul,
	Stephen Boyd, Vara Reddy, dri-devel, freedreno, linux-arm-msm,
	linux-kernel


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 case for eDP in the
AUX transfer function.

NOTE: I managed to dig up some hardware to test the eDP case and my
basic testing shows that everything still works fine there after this
series.

Changes in v2:
- Don't look at the HPD line directly; have dp_display call us.
- ("Fix typo in static function (ststus => status)") new for v2.

Douglas Anderson (4):
  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
  drm/msm/dp: Fix typo in static function (ststus => status)

 drivers/gpu/drm/msm/dp/dp_aux.c     | 30 ++++++++++++++++-------------
 drivers/gpu/drm/msm/dp/dp_aux.h     |  1 +
 drivers/gpu/drm/msm/dp/dp_catalog.c |  7 ++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |  3 ++-
 drivers/gpu/drm/msm/dp/dp_display.c |  8 ++++++--
 5 files changed, 30 insertions(+), 19 deletions(-)

-- 
2.44.0.291.gc1ea87d7ee-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
  2024-03-15 21:36 [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions Douglas Anderson
@ 2024-03-15 21:36 ` Douglas Anderson
  2024-03-15 22:19   ` Guenter Roeck
  2024-03-18 18:16   ` Abhinav Kumar
  2024-03-15 21:36 ` [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback Douglas Anderson
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 17+ messages in thread
From: Douglas Anderson @ 2024-03-15 21:36 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov
  Cc: Douglas Anderson, Bjorn Andersson, Daniel Vetter, David Airlie,
	Guenter Roeck, Kuogee Hsieh, Marijn Suijten, Sean Paul,
	Stephen Boyd, Vara Reddy, dri-devel, freedreno, linux-arm-msm,
	linux-kernel

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
We want access to the drm_dp_auxN character device to fail faster than
8 seconds when no DP cable is plugged in.

Let's add a test to make transfers fail right away if a device isn't
plugged in. Rather than testing the HPD line directly, we have the
dp_display module tell us when AUX transfers should be enabled so we
can handle cases where HPD is signaled out of band like with Type C.

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Don't look at the HPD line directly; have dp_display call us.

 drivers/gpu/drm/msm/dp/dp_aux.c     | 20 ++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_aux.h     |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 03f4951c49f4..e67a80d56948 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -35,6 +35,7 @@ struct dp_aux_private {
 	bool no_send_stop;
 	bool initted;
 	bool is_edp;
+	bool enable_xfers;
 	u32 offset;
 	u32 segment;
 
@@ -301,6 +302,17 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		goto exit;
 	}
 
+	/*
+	 * If we're using DP and an external display isn't connected 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 && !aux->enable_xfers) {
+		ret = -ENXIO;
+		goto exit;
+	}
+
 	/*
 	 * 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_
@@ -433,6 +445,14 @@ irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux)
 	return IRQ_HANDLED;
 }
 
+void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled)
+{
+	struct dp_aux_private *aux;
+
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+	aux->enable_xfers = enabled;
+}
+
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
 {
 	struct dp_aux_private *aux;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 511305da4f66..f3052cb43306 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -12,6 +12,7 @@
 int dp_aux_register(struct drm_dp_aux *dp_aux);
 void dp_aux_unregister(struct drm_dp_aux *dp_aux);
 irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux);
+void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled);
 void dp_aux_init(struct drm_dp_aux *dp_aux);
 void dp_aux_deinit(struct drm_dp_aux *dp_aux);
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4c72124ffb5d..b0f3e2ef5a6b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -565,6 +565,8 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 	int ret;
 	struct platform_device *pdev = dp->dp_display.pdev;
 
+	dp_aux_enable_xfers(dp->aux, true);
+
 	mutex_lock(&dp->event_mutex);
 
 	state =  dp->hpd_state;
@@ -629,6 +631,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	u32 state;
 	struct platform_device *pdev = dp->dp_display.pdev;
 
+	dp_aux_enable_xfers(dp->aux, false);
+
 	mutex_lock(&dp->event_mutex);
 
 	state = dp->hpd_state;
-- 
2.44.0.291.gc1ea87d7ee-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback
  2024-03-15 21:36 [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions Douglas Anderson
  2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
@ 2024-03-15 21:36 ` Douglas Anderson
  2024-03-18 18:18   ` Abhinav Kumar
  2024-03-15 21:36 ` [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer Douglas Anderson
  2024-03-15 21:36 ` [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status) Douglas Anderson
  3 siblings, 1 reply; 17+ messages in thread
From: Douglas Anderson @ 2024-03-15 21:36 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov
  Cc: Douglas Anderson, Daniel Vetter, David Airlie, Kuogee Hsieh,
	Marijn Suijten, Sean Paul, dri-devel, freedreno, linux-arm-msm,
	linux-kernel

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 <dianders@chromium.org>
---

(no changes since v1)

 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 e67a80d56948..75c51f3ee106 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -322,7 +322,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	 * avoid ever doing the extra long wait for DP.
 	 */
 	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;
@@ -530,7 +531,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 5142aeb705a4..944ccb74f06c 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);
 }
 
 static void dump_regs(void __iomem *base, int len)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 38786e855b51..d116df1fc3ac 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);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
-- 
2.44.0.291.gc1ea87d7ee-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-15 21:36 [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions Douglas Anderson
  2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
  2024-03-15 21:36 ` [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback Douglas Anderson
@ 2024-03-15 21:36 ` Douglas Anderson
  2024-03-19  0:19   ` Abhinav Kumar
  2024-03-15 21:36 ` [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status) Douglas Anderson
  3 siblings, 1 reply; 17+ messages in thread
From: Douglas Anderson @ 2024-03-15 21:36 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov
  Cc: Douglas Anderson, Daniel Vetter, David Airlie, Kuogee Hsieh,
	Marijn Suijten, Sean Paul, dri-devel, freedreno, linux-arm-msm,
	linux-kernel

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 <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 75c51f3ee106..ecefd1922d6d 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		goto exit;
 	}
 
-	/*
-	 * 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.
-	 */
-	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;
-		}
-	}
-
 	dp_aux_update_offset_and_segment(aux, msg);
 	dp_aux_transfer_helper(aux, msg, true);
 
-- 
2.44.0.291.gc1ea87d7ee-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
  2024-03-15 21:36 [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions Douglas Anderson
                   ` (2 preceding siblings ...)
  2024-03-15 21:36 ` [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer Douglas Anderson
@ 2024-03-15 21:36 ` Douglas Anderson
  2024-03-18 19:26   ` Stephen Boyd
  3 siblings, 1 reply; 17+ messages in thread
From: Douglas Anderson @ 2024-03-15 21:36 UTC (permalink / raw)
  To: Rob Clark, Abhinav Kumar, Dmitry Baryshkov
  Cc: Douglas Anderson, Bjorn Andersson, Daniel Vetter, David Airlie,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, Stephen Boyd, dri-devel,
	freedreno, linux-arm-msm, linux-kernel

This is a no-op change to just fix a typo in the name of a static function.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("Fix typo in static function (ststus => status)") new for v2.

 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index b0f3e2ef5a6b..78e702f66ed2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -494,7 +494,7 @@ static void dp_display_handle_video_request(struct dp_display_private *dp)
 	}
 }
 
-static int dp_display_handle_port_ststus_changed(struct dp_display_private *dp)
+static int dp_display_handle_port_status_changed(struct dp_display_private *dp)
 {
 	int rc = 0;
 
@@ -551,7 +551,7 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 		drm_dbg_dp(dp->drm_dev, "hpd_state=%d sink_request=%d\n",
 					dp->hpd_state, sink_request);
 		if (sink_request & DS_PORT_STATUS_CHANGED)
-			rc = dp_display_handle_port_ststus_changed(dp);
+			rc = dp_display_handle_port_status_changed(dp);
 		else
 			rc = dp_display_handle_irq_hpd(dp);
 	}
-- 
2.44.0.291.gc1ea87d7ee-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
  2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
@ 2024-03-15 22:19   ` Guenter Roeck
  2024-03-18 18:16   ` Abhinav Kumar
  1 sibling, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2024-03-15 22:19 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Bjorn Andersson,
	Daniel Vetter, David Airlie, Guenter Roeck, Kuogee Hsieh,
	Marijn Suijten, Sean Paul, Stephen Boyd, Vara Reddy, dri-devel,
	freedreno, linux-arm-msm, linux-kernel

On Fri, Mar 15, 2024 at 2:37 PM Douglas Anderson <dianders@chromium.org> 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
> We want access to the drm_dp_auxN character device to fail faster than
> 8 seconds when no DP cable is plugged in.
>
> Let's add a test to make transfers fail right away if a device isn't
> plugged in. Rather than testing the HPD line directly, we have the
> dp_display module tell us when AUX transfers should be enabled so we
> can handle cases where HPD is signaled out of band like with Type C.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
> Changes in v2:
> - Don't look at the HPD line directly; have dp_display call us.
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 20 ++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_aux.h     |  1 +
>  drivers/gpu/drm/msm/dp/dp_display.c |  4 ++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 03f4951c49f4..e67a80d56948 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -35,6 +35,7 @@ struct dp_aux_private {
>         bool no_send_stop;
>         bool initted;
>         bool is_edp;
> +       bool enable_xfers;
>         u32 offset;
>         u32 segment;
>
> @@ -301,6 +302,17 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 goto exit;
>         }
>
> +       /*
> +        * If we're using DP and an external display isn't connected 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 && !aux->enable_xfers) {
> +               ret = -ENXIO;
> +               goto exit;
> +       }
> +
>         /*
>          * 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_
> @@ -433,6 +445,14 @@ irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux)
>         return IRQ_HANDLED;
>  }
>
> +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled)
> +{
> +       struct dp_aux_private *aux;
> +
> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +       aux->enable_xfers = enabled;
> +}
> +
>  void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
>  {
>         struct dp_aux_private *aux;
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
> index 511305da4f66..f3052cb43306 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -12,6 +12,7 @@
>  int dp_aux_register(struct drm_dp_aux *dp_aux);
>  void dp_aux_unregister(struct drm_dp_aux *dp_aux);
>  irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux);
> +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled);
>  void dp_aux_init(struct drm_dp_aux *dp_aux);
>  void dp_aux_deinit(struct drm_dp_aux *dp_aux);
>  void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 4c72124ffb5d..b0f3e2ef5a6b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -565,6 +565,8 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>         int ret;
>         struct platform_device *pdev = dp->dp_display.pdev;
>
> +       dp_aux_enable_xfers(dp->aux, true);
> +
>         mutex_lock(&dp->event_mutex);
>
>         state =  dp->hpd_state;
> @@ -629,6 +631,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>         u32 state;
>         struct platform_device *pdev = dp->dp_display.pdev;
>
> +       dp_aux_enable_xfers(dp->aux, false);
> +
>         mutex_lock(&dp->event_mutex);
>
>         state = dp->hpd_state;
> --
> 2.44.0.291.gc1ea87d7ee-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected
  2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
  2024-03-15 22:19   ` Guenter Roeck
@ 2024-03-18 18:16   ` Abhinav Kumar
  1 sibling, 0 replies; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-18 18:16 UTC (permalink / raw)
  To: Douglas Anderson, Rob Clark, Dmitry Baryshkov
  Cc: Bjorn Andersson, Daniel Vetter, David Airlie, Guenter Roeck,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, Stephen Boyd,
	Vara Reddy, dri-devel, freedreno, linux-arm-msm, linux-kernel



On 3/15/2024 2:36 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
>    real    0m8.200s
> We want access to the drm_dp_auxN character device to fail faster than
> 8 seconds when no DP cable is plugged in.
> 
> Let's add a test to make transfers fail right away if a device isn't
> plugged in. Rather than testing the HPD line directly, we have the
> dp_display module tell us when AUX transfers should be enabled so we
> can handle cases where HPD is signaled out of band like with Type C.
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Don't look at the HPD line directly; have dp_display call us.
> 
>   drivers/gpu/drm/msm/dp/dp_aux.c     | 20 ++++++++++++++++++++
>   drivers/gpu/drm/msm/dp/dp_aux.h     |  1 +
>   drivers/gpu/drm/msm/dp/dp_display.c |  4 ++++
>   3 files changed, 25 insertions(+)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback
  2024-03-15 21:36 ` [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback Douglas Anderson
@ 2024-03-18 18:18   ` Abhinav Kumar
  0 siblings, 0 replies; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-18 18:18 UTC (permalink / raw)
  To: Douglas Anderson, Rob Clark, Dmitry Baryshkov
  Cc: Daniel Vetter, David Airlie, Kuogee Hsieh, Marijn Suijten,
	Sean Paul, dri-devel, freedreno, linux-arm-msm, linux-kernel



On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> 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 <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>   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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
  2024-03-15 21:36 ` [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status) Douglas Anderson
@ 2024-03-18 19:26   ` Stephen Boyd
  2024-03-18 19:37     ` Doug Anderson
  0 siblings, 1 reply; 17+ messages in thread
From: Stephen Boyd @ 2024-03-18 19:26 UTC (permalink / raw)
  To: Abhinav Kumar, Dmitry Baryshkov, Douglas Anderson, Rob Clark
  Cc: Bjorn Andersson, Daniel Vetter, David Airlie, Kuogee Hsieh,
	Marijn Suijten, Sean Paul, dri-devel, freedreno, linux-arm-msm,
	linux-kernel

Quoting Douglas Anderson (2024-03-15 14:36:32)
> This is a no-op change to just fix a typo in the name of a static function.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - ("Fix typo in static function (ststus => status)") new for v2.

This was sent at
https://lore.kernel.org/r/20240306193515.455388-1-quic_abhinavk@quicinc.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
  2024-03-18 19:26   ` Stephen Boyd
@ 2024-03-18 19:37     ` Doug Anderson
  2024-03-19  0:22       ` Abhinav Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2024-03-18 19:37 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Abhinav Kumar, Dmitry Baryshkov, Rob Clark, Bjorn Andersson,
	Daniel Vetter, David Airlie, Kuogee Hsieh, Marijn Suijten,
	Sean Paul, dri-devel, freedreno, linux-arm-msm, linux-kernel

Hi,

On Mon, Mar 18, 2024 at 12:26 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2024-03-15 14:36:32)
> > This is a no-op change to just fix a typo in the name of a static function.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - ("Fix typo in static function (ststus => status)") new for v2.
>
> This was sent at
> https://lore.kernel.org/r/20240306193515.455388-1-quic_abhinavk@quicinc.com

Whoops! I guess we both noticed it at about the same time. My patch
should be dropped then. The rest of my series (patches #1 - #3) are
still relevant. I won't repost them since they can be applied just
fine even if this patch is dropped.

-Doug

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-15 21:36 ` [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer Douglas Anderson
@ 2024-03-19  0:19   ` Abhinav Kumar
  2024-03-19  0:55     ` Dmitry Baryshkov
  0 siblings, 1 reply; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-19  0:19 UTC (permalink / raw)
  To: Douglas Anderson, Rob Clark, Dmitry Baryshkov
  Cc: Daniel Vetter, David Airlie, Kuogee Hsieh, Marijn Suijten,
	Sean Paul, dri-devel, freedreno, linux-arm-msm, linux-kernel,
	Bjorn Andersson, Johan Hovold

+bjorn, johan as fyi for sc8280xp

On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> 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.
> 

How do we enforce that for any new edp panels to be used with MSM, the 
panels should atleast invoke wait_hpd_asserted()?

I agree that since MSM implements it, even though its listed as 
optional, we can drop this additional wait. So nothing wrong with this 
patch for current users including sc8280xp, sc7280 and sc7180.

But, does there need to be some documentation that the edp panels not 
using the panel-edp framework should still invoke wait_hpd_asserted()?

Since its marked as optional, what happens if the edp panel driver, 
skips calling wait_hpd_asserted()?

Now, since the wait from MSM is removed, it has a potential to fail.

> 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 <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
>   1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 75c51f3ee106..ecefd1922d6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   		goto exit;
>   	}
>   
> -	/*
> -	 * 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.
> -	 */
> -	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;
> -		}
> -	}
> -
>   	dp_aux_update_offset_and_segment(aux, msg);
>   	dp_aux_transfer_helper(aux, msg, true);
>   

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
  2024-03-18 19:37     ` Doug Anderson
@ 2024-03-19  0:22       ` Abhinav Kumar
  0 siblings, 0 replies; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-19  0:22 UTC (permalink / raw)
  To: Doug Anderson, Stephen Boyd
  Cc: Dmitry Baryshkov, Rob Clark, Bjorn Andersson, Daniel Vetter,
	David Airlie, Kuogee Hsieh, Marijn Suijten, Sean Paul, dri-devel,
	freedreno, linux-arm-msm, linux-kernel



On 3/18/2024 12:37 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Mar 18, 2024 at 12:26 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> Quoting Douglas Anderson (2024-03-15 14:36:32)
>>> This is a no-op change to just fix a typo in the name of a static function.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - ("Fix typo in static function (ststus => status)") new for v2.
>>
>> This was sent at
>> https://lore.kernel.org/r/20240306193515.455388-1-quic_abhinavk@quicinc.com
> 
> Whoops! I guess we both noticed it at about the same time. My patch
> should be dropped then. The rest of my series (patches #1 - #3) are
> still relevant. I won't repost them since they can be applied just
> fine even if this patch is dropped.
> 
> -Doug

Thanks for the patch.

I will pick up 
https://lore.kernel.org/r/20240306193515.455388-1-quic_abhinavk@quicinc.com 
for -fixes so you can drop this one.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-19  0:19   ` Abhinav Kumar
@ 2024-03-19  0:55     ` Dmitry Baryshkov
  2024-03-19 17:13       ` Abhinav Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19  0:55 UTC (permalink / raw)
  To: Abhinav Kumar
  Cc: Douglas Anderson, Rob Clark, Daniel Vetter, David Airlie,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, dri-devel, freedreno,
	linux-arm-msm, linux-kernel, Bjorn Andersson, Johan Hovold

On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> +bjorn, johan as fyi for sc8280xp
>
> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> > 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.
> >
>
> How do we enforce that for any new edp panels to be used with MSM, the
> panels should atleast invoke wait_hpd_asserted()?
>
> I agree that since MSM implements it, even though its listed as
> optional, we can drop this additional wait. So nothing wrong with this
> patch for current users including sc8280xp, sc7280 and sc7180.
>
> But, does there need to be some documentation that the edp panels not
> using the panel-edp framework should still invoke wait_hpd_asserted()?
>
> Since its marked as optional, what happens if the edp panel driver,
> skips calling wait_hpd_asserted()?

It is optional for the DP AUX implementations, not for the panel to be called.

>
> Now, since the wait from MSM is removed, it has a potential to fail.
>
> > 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 <dianders@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >   drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
> >   1 file changed, 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> > index 75c51f3ee106..ecefd1922d6d 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> >               goto exit;
> >       }
> >
> > -     /*
> > -      * 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.
> > -      */
> > -     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;
> > -             }
> > -     }
> > -
> >       dp_aux_update_offset_and_segment(aux, msg);
> >       dp_aux_transfer_helper(aux, msg, true);
> >



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-19  0:55     ` Dmitry Baryshkov
@ 2024-03-19 17:13       ` Abhinav Kumar
  2024-03-19 17:27         ` Dmitry Baryshkov
  0 siblings, 1 reply; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-19 17:13 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Douglas Anderson, Rob Clark, Daniel Vetter, David Airlie,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, dri-devel, freedreno,
	linux-arm-msm, linux-kernel, Bjorn Andersson, Johan Hovold



On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> +bjorn, johan as fyi for sc8280xp
>>
>> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
>>> 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.
>>>
>>
>> How do we enforce that for any new edp panels to be used with MSM, the
>> panels should atleast invoke wait_hpd_asserted()?
>>
>> I agree that since MSM implements it, even though its listed as
>> optional, we can drop this additional wait. So nothing wrong with this
>> patch for current users including sc8280xp, sc7280 and sc7180.
>>
>> But, does there need to be some documentation that the edp panels not
>> using the panel-edp framework should still invoke wait_hpd_asserted()?
>>
>> Since its marked as optional, what happens if the edp panel driver,
>> skips calling wait_hpd_asserted()?
> 
> It is optional for the DP AUX implementations, not for the panel to be called.
> 

Yes, I understood that part, but is there anything from the panel side 
which mandates calling wait_hpd_asserted()?

Is this documented somewhere for all edp panels to do:

if (aux->wait_hpd_asserted)
	aux->wait_hpd_asserted(aux, wait_us);

>>
>> Now, since the wait from MSM is removed, it has a potential to fail.
>>
>>> 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 <dianders@chromium.org>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>    drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
>>>    1 file changed, 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> index 75c51f3ee106..ecefd1922d6d 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>>>                goto exit;
>>>        }
>>>
>>> -     /*
>>> -      * 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.
>>> -      */
>>> -     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;
>>> -             }
>>> -     }
>>> -
>>>        dp_aux_update_offset_and_segment(aux, msg);
>>>        dp_aux_transfer_helper(aux, msg, true);
>>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-19 17:13       ` Abhinav Kumar
@ 2024-03-19 17:27         ` Dmitry Baryshkov
  2024-03-19 18:15           ` Doug Anderson
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 17:27 UTC (permalink / raw)
  To: Abhinav Kumar
  Cc: Douglas Anderson, Rob Clark, Daniel Vetter, David Airlie,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, dri-devel, freedreno,
	linux-arm-msm, linux-kernel, Bjorn Andersson, Johan Hovold

On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> > On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> +bjorn, johan as fyi for sc8280xp
> >>
> >> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> >>> 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.
> >>>
> >>
> >> How do we enforce that for any new edp panels to be used with MSM, the
> >> panels should atleast invoke wait_hpd_asserted()?
> >>
> >> I agree that since MSM implements it, even though its listed as
> >> optional, we can drop this additional wait. So nothing wrong with this
> >> patch for current users including sc8280xp, sc7280 and sc7180.
> >>
> >> But, does there need to be some documentation that the edp panels not
> >> using the panel-edp framework should still invoke wait_hpd_asserted()?
> >>
> >> Since its marked as optional, what happens if the edp panel driver,
> >> skips calling wait_hpd_asserted()?
> >
> > It is optional for the DP AUX implementations, not for the panel to be called.
> >
>
> Yes, I understood that part, but is there anything from the panel side
> which mandates calling wait_hpd_asserted()?
>
> Is this documented somewhere for all edp panels to do:
>
> if (aux->wait_hpd_asserted)
>         aux->wait_hpd_asserted(aux, wait_us);

That's obviously not true, e.g. if panel-edp.c handled HPD signal via
the GPIO pin.

But the documentation explicitly says that the host will be powered up
automatically, but the caller must ensure that the panel is powered
on. `It's up to the caller of this code to make sure that the panel is
powered on if getting an error back is not OK.'

>
> >>
> >> Now, since the wait from MSM is removed, it has a potential to fail.
> >>
> >>> 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 <dianders@chromium.org>
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>>    drivers/gpu/drm/msm/dp/dp_aux.c | 17 -----------------
> >>>    1 file changed, 17 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>> index 75c51f3ee106..ecefd1922d6d 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>> @@ -313,23 +313,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> >>>                goto exit;
> >>>        }
> >>>
> >>> -     /*
> >>> -      * 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.
> >>> -      */
> >>> -     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;
> >>> -             }
> >>> -     }
> >>> -
> >>>        dp_aux_update_offset_and_segment(aux, msg);
> >>>        dp_aux_transfer_helper(aux, msg, true);
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-19 17:27         ` Dmitry Baryshkov
@ 2024-03-19 18:15           ` Doug Anderson
  2024-03-19 18:27             ` Abhinav Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2024-03-19 18:15 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Abhinav Kumar, Rob Clark, Daniel Vetter, David Airlie,
	Kuogee Hsieh, Marijn Suijten, Sean Paul, dri-devel, freedreno,
	linux-arm-msm, linux-kernel, Bjorn Andersson, Johan Hovold

Hi,

On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> >
> >
> > On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> > > On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>
> > >> +bjorn, johan as fyi for sc8280xp
> > >>
> > >> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> > >>> 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.
> > >>>
> > >>
> > >> How do we enforce that for any new edp panels to be used with MSM, the
> > >> panels should atleast invoke wait_hpd_asserted()?
> > >>
> > >> I agree that since MSM implements it, even though its listed as
> > >> optional, we can drop this additional wait. So nothing wrong with this
> > >> patch for current users including sc8280xp, sc7280 and sc7180.
> > >>
> > >> But, does there need to be some documentation that the edp panels not
> > >> using the panel-edp framework should still invoke wait_hpd_asserted()?
> > >>
> > >> Since its marked as optional, what happens if the edp panel driver,
> > >> skips calling wait_hpd_asserted()?
> > >
> > > It is optional for the DP AUX implementations, not for the panel to be called.
> > >
> >
> > Yes, I understood that part, but is there anything from the panel side
> > which mandates calling wait_hpd_asserted()?
> >
> > Is this documented somewhere for all edp panels to do:
> >
> > if (aux->wait_hpd_asserted)
> >         aux->wait_hpd_asserted(aux, wait_us);
>
> That's obviously not true, e.g. if panel-edp.c handled HPD signal via
> the GPIO pin.
>
> But the documentation explicitly says that the host will be powered up
> automatically, but the caller must ensure that the panel is powered
> on. `It's up to the caller of this code to make sure that the panel is
> powered on if getting an error back is not OK.'

It wouldn't hurt to send out a documentation patch that makes this
more explicit. OK, I sent:

https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid

-Doug

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer
  2024-03-19 18:15           ` Doug Anderson
@ 2024-03-19 18:27             ` Abhinav Kumar
  0 siblings, 0 replies; 17+ messages in thread
From: Abhinav Kumar @ 2024-03-19 18:27 UTC (permalink / raw)
  To: Doug Anderson, Dmitry Baryshkov
  Cc: Rob Clark, Daniel Vetter, David Airlie, Kuogee Hsieh,
	Marijn Suijten, Sean Paul, dri-devel, freedreno, linux-arm-msm,
	linux-kernel, Bjorn Andersson, Johan Hovold



On 3/19/2024 11:15 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>> +bjorn, johan as fyi for sc8280xp
>>>>>
>>>>> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
>>>>>> 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.
>>>>>>
>>>>>
>>>>> How do we enforce that for any new edp panels to be used with MSM, the
>>>>> panels should atleast invoke wait_hpd_asserted()?
>>>>>
>>>>> I agree that since MSM implements it, even though its listed as
>>>>> optional, we can drop this additional wait. So nothing wrong with this
>>>>> patch for current users including sc8280xp, sc7280 and sc7180.
>>>>>
>>>>> But, does there need to be some documentation that the edp panels not
>>>>> using the panel-edp framework should still invoke wait_hpd_asserted()?
>>>>>
>>>>> Since its marked as optional, what happens if the edp panel driver,
>>>>> skips calling wait_hpd_asserted()?
>>>>
>>>> It is optional for the DP AUX implementations, not for the panel to be called.
>>>>
>>>
>>> Yes, I understood that part, but is there anything from the panel side
>>> which mandates calling wait_hpd_asserted()?
>>>
>>> Is this documented somewhere for all edp panels to do:
>>>
>>> if (aux->wait_hpd_asserted)
>>>          aux->wait_hpd_asserted(aux, wait_us);
>>
>> That's obviously not true, e.g. if panel-edp.c handled HPD signal via
>> the GPIO pin.
>>
>> But the documentation explicitly says that the host will be powered up
>> automatically, but the caller must ensure that the panel is powered
>> on. `It's up to the caller of this code to make sure that the panel is
>> powered on if getting an error back is not OK.'
> 
> It wouldn't hurt to send out a documentation patch that makes this
> more explicit. OK, I sent:
> 
> https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid
> 
> -Doug

Thanks, with that in place, this is

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2024-03-19 18:28 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-15 21:36 [PATCH v2 0/4] drm/msm/dp: Improve DP AUX transfer vs. HPD interactions Douglas Anderson
2024-03-15 21:36 ` [PATCH v2 1/4] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected Douglas Anderson
2024-03-15 22:19   ` Guenter Roeck
2024-03-18 18:16   ` Abhinav Kumar
2024-03-15 21:36 ` [PATCH v2 2/4] drm/msm/dp: Account for the timeout in wait_hpd_asserted() callback Douglas Anderson
2024-03-18 18:18   ` Abhinav Kumar
2024-03-15 21:36 ` [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer Douglas Anderson
2024-03-19  0:19   ` Abhinav Kumar
2024-03-19  0:55     ` Dmitry Baryshkov
2024-03-19 17:13       ` Abhinav Kumar
2024-03-19 17:27         ` Dmitry Baryshkov
2024-03-19 18:15           ` Doug Anderson
2024-03-19 18:27             ` Abhinav Kumar
2024-03-15 21:36 ` [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status) Douglas Anderson
2024-03-18 19:26   ` Stephen Boyd
2024-03-18 19:37     ` Doug Anderson
2024-03-19  0:22       ` Abhinav Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).