Re: [PATCH v2 2/2] drm: bridge: cdns-mhdp8546: Add support for no-hpd
From: Tomi Valkeinen
Date: Mon Apr 17 2023 - 05:31:59 EST
On 14/04/2023 18:10, Jayesh Choudhary wrote:
On 06/04/23 07:22, Laurent Pinchart wrote:
Hi Jayesh,
Thank you for the patch.
On Wed, Apr 05, 2023 at 07:54:40PM +0530, Jayesh Choudhary wrote:
From: Rahul T R <r-ravikumar@xxxxxx>
In J721S2 EVMs DP0 hpd is not connected to correct
hpd pin on SOC, to handle such cases, Add support for
"no-hpd" property in the device tree node to disable
hpd
s/hpd/hpd./
You can also reflow the commit message to 72 columns.
Okay. Thanks for the suggestion. Will do.
Also change the log level for dpcd read failuers to
debug, since framework retries 32 times for each read
s/read/read./
Doesn't this apply to writes as well ?
Based on message request, we went into the conditional that uses
read. So just changing the log-level for dpcd read was enough to
get rid of the debug logs.
Signed-off-by: Rahul T R <r-ravikumar@xxxxxx>
Signed-off-by: Jayesh Choudhary <j-choudhary@xxxxxx>
---
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 37 ++++++++++++++++---
.../drm/bridge/cadence/cdns-mhdp8546-core.h | 1 +
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index f6822dfa3805..e177794b069d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -54,6 +54,8 @@
#include "cdns-mhdp8546-hdcp.h"
#include "cdns-mhdp8546-j721e.h"
+static int cdns_mhdp_update_link_status(struct cdns_mhdp_device *mhdp);
+
static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
{
int ret, empty;
@@ -749,7 +751,7 @@ static int cdns_mhdp_fw_activate(const struct
firmware *fw,
* MHDP_HW_STOPPED happens only due to driver removal when
* bridge should already be detached.
*/
- if (mhdp->bridge_attached)
+ if (mhdp->bridge_attached && !mhdp->no_hpd)
writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
mhdp->regs + CDNS_APB_INT_MASK);
@@ -845,7 +847,7 @@ static ssize_t cdns_mhdp_transfer(struct
drm_dp_aux *aux,
ret = cdns_mhdp_dpcd_read(mhdp, msg->address,
msg->buffer, msg->size);
if (ret) {
- dev_err(mhdp->dev,
+ dev_dbg(mhdp->dev,
"Failed to read DPCD addr %u\n",
msg->address);
I don't much like this change. Now the write and read paths have
different log level. This also affects the with-hpd case. And it "hides"
error prints for "real" errors.
Now, I think it's a valid question whether this function should use
dev_dbg rather than dev_err (both write and read cases), as dev_err may
be a bit spammy, and this might perhaps be user triggerable.
If the DP transfer is used for polling I think there should be specific
handling for that case here. Before the driver polls with DP transfer,
set a flag, and then in this function skip logging altogether. I don't
think you want to spam the debug log with DPCD errors either.
@@ -1738,6 +1740,19 @@ static int cdns_mhdp_attach(struct drm_bridge
*bridge,
spin_unlock(&mhdp->start_lock);
+ if (mhdp->no_hpd) {
+ ret = wait_event_timeout(mhdp->fw_load_wq,
+ mhdp->hw_state == MHDP_HW_READY,
+ msecs_to_jiffies(100));
+ if (ret == 0) {
+ dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n",
+ __func__);
+ return -ETIMEDOUT;
+ }
+
+ cdns_mhdp_update_link_status(mhdp);
+ return 0;
+ }
Missing blank line.
It's not clear to me while you need to wait for the state to change to
MHDP_HW_READY in the no_hpd case. This should be explained in the commit
message.
I presume it's because both the firmware loading and bridge attach
happens asynchronously. With HPD the driver acts only on the interrupt,
which is enabled only after both have happened. Here the driver waits in
the attach until everything is ready, and then probes the DP.
But it does feel hacky.
/* Enable SW event interrupts */
if (hw_ready)
writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
@@ -2256,7 +2271,16 @@ static int cdns_mhdp_update_link_status(struct
cdns_mhdp_device *mhdp)
mutex_lock(&mhdp->link_mutex);
- mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+ if (mhdp->no_hpd) {
+ ret = drm_dp_dpcd_read_link_status(&mhdp->aux, status);
+ hpd_pulse = false;
+ if (ret < 0)
+ mhdp->plugged = false;
+ else
+ mhdp->plugged = true;
I think there's an issue with how the driver uses mhdp->plugged. In the
no_hpd case, you try to detect if a display is connected by reading the
link status at attach time, and then never update mhdp->plugged. This
means that if no display is connected at that point, functions like
cdns_mhdp_get_edid() will always fail, even if a display gets plugged
later. As the goal of this series is (as far as I understand) support
systems where the HPD signal could be connected to a SoC GPIO instead of
the bridge, I don't think this is good enough.
In the driver, I see that this is the only call which changes
mhdp->plugged. Do you have any suggestions on how to work on this?
Polling the value of drm_dp_dpdc_read_link_status does not seem like a
clean way.
Here by doing this, we are settling for few functionalities of display.
I think it depends a bit on what you want to support. The HW sounds
quite broken, as HPD is rather important feature of DP. Did you check
the DP spec if it has any guidelines on how to handle no HPD case?
You could perhaps say that you only support eDP without HPD on this HW.
Then the broken HPD doesn't matter.
If you want to support normal DP, I think you have to do some kind of
polling to make the DP somewhat usable, or a manual detect in certain
strategic places (probe time, get_edid, get_modes, perhaps?).
Is there anything in the PHY that could inform that something has been
connected? I remember some HDMI PHYs can tell if something is connected
to the pins. It's not HPD, but could be used to filter away other,
slower, polling. Or maybe the mdhp has something to help here.
Also, if you do polling, I don't think you need to go via the
drm_dp_dpcd_read_link_status. You could first just use
cdns_mhdp_dpcd_read directly, and if that succeeds, then proceed getting
more info from the monitor.
Tomi