In J721s2 EVM, DP0 HPD is not connected to correct HPD pin on SOC
which results in HPD detect as always connnected, so when display
is not connected driver continuously retries to read EDID and DPCD
registers.
To handle such cases add support for no hpd configuration in
cdns-mhdp driver.
DT changes for display in j721s2 will be posted once we have no-hpd
support and multilink DP support[1] merged.
[1]:
<https://lore.kernel.org/all/[email protected]/>
Changelog v1->v2:
- edit the commit message for the bindings to clarify why we exactly need
no-hpd indication for mhdp.
v1 patch-link:
<https://lore.kernel.org/all/[email protected]/>
Rahul T R (2):
dt-bindings: drm/bridge: Add no-hpd property
drm: bridge: cdns-mhdp8546: Add support for no-hpd
.../display/bridge/cdns,mhdp8546.yaml | 6 +++
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 37 ++++++++++++++++---
.../drm/bridge/cadence/cdns-mhdp8546-core.h | 1 +
3 files changed, 39 insertions(+), 5 deletions(-)
--
2.25.1
From: Rahul T R <[email protected]>
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
Also change the log level for dpcd read failuers to
debug, since framework retries 32 times for each read
Signed-off-by: Rahul T R <[email protected]>
Signed-off-by: Jayesh Choudhary <[email protected]>
---
.../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);
@@ -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;
+ }
/* 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;
+ } else {
+ mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
+ }
if (!mhdp->plugged) {
cdns_mhdp_link_down(mhdp);
@@ -2451,6 +2475,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
mhdp->aux.dev = dev;
mhdp->aux.transfer = cdns_mhdp_transfer;
+ mhdp->no_hpd = of_property_read_bool(dev->of_node, "cdns,no-hpd");
+
mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(mhdp->regs)) {
dev_err(dev, "Failed to get memory resource\n");
@@ -2526,8 +2552,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
mhdp->bridge.of_node = pdev->dev.of_node;
mhdp->bridge.funcs = &cdns_mhdp_bridge_funcs;
- mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
- DRM_BRIDGE_OP_HPD;
+ mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+ if (!mhdp->no_hpd)
+ mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
if (mhdp->info)
mhdp->bridge.timings = mhdp->info->timings;
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index bedddd510d17..6786ccb51387 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -388,6 +388,7 @@ struct cdns_mhdp_device {
bool link_up;
bool plugged;
+ bool no_hpd;
/*
* "start_lock" protects the access to bridge_attached and
--
2.25.1
Hi Jayesh,
Thank you for the patch.
On Wed, Apr 05, 2023 at 07:54:40PM +0530, Jayesh Choudhary wrote:
> From: Rahul T R <[email protected]>
>
> 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.
> 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 ?
> Signed-off-by: Rahul T R <[email protected]>
> Signed-off-by: Jayesh Choudhary <[email protected]>
> ---
> .../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);
>
> @@ -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.
> /* 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.
> + } else {
> + mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
> + }
>
> if (!mhdp->plugged) {
> cdns_mhdp_link_down(mhdp);
> @@ -2451,6 +2475,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
> mhdp->aux.dev = dev;
> mhdp->aux.transfer = cdns_mhdp_transfer;
>
> + mhdp->no_hpd = of_property_read_bool(dev->of_node, "cdns,no-hpd");
> +
> mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(mhdp->regs)) {
> dev_err(dev, "Failed to get memory resource\n");
> @@ -2526,8 +2552,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
>
> mhdp->bridge.of_node = pdev->dev.of_node;
> mhdp->bridge.funcs = &cdns_mhdp_bridge_funcs;
> - mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> - DRM_BRIDGE_OP_HPD;
> + mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
> + if (!mhdp->no_hpd)
> + mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
> mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> if (mhdp->info)
> mhdp->bridge.timings = mhdp->info->timings;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bedddd510d17..6786ccb51387 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -388,6 +388,7 @@ struct cdns_mhdp_device {
>
> bool link_up;
> bool plugged;
> + bool no_hpd;
>
> /*
> * "start_lock" protects the access to bridge_attached and
--
Regards,
Laurent Pinchart
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jayesh Choudhary <[email protected]>
>> ---
>> .../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);
>>
>> @@ -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.
>
>> /* 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.
Thanks,
-Jayesh
>
>> + } else {
>> + mhdp->plugged = cdns_mhdp_detect_hpd(mhdp, &hpd_pulse);
>> + }
>>
>> if (!mhdp->plugged) {
>> cdns_mhdp_link_down(mhdp);
>> @@ -2451,6 +2475,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
>> mhdp->aux.dev = dev;
>> mhdp->aux.transfer = cdns_mhdp_transfer;
>>
>> + mhdp->no_hpd = of_property_read_bool(dev->of_node, "cdns,no-hpd");
>> +
>> mhdp->regs = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(mhdp->regs)) {
>> dev_err(dev, "Failed to get memory resource\n");
>> @@ -2526,8 +2552,9 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
>>
>> mhdp->bridge.of_node = pdev->dev.of_node;
>> mhdp->bridge.funcs = &cdns_mhdp_bridge_funcs;
>> - mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>> - DRM_BRIDGE_OP_HPD;
>> + mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
>> + if (!mhdp->no_hpd)
>> + mhdp->bridge.ops |= DRM_BRIDGE_OP_HPD;
>> mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>> if (mhdp->info)
>> mhdp->bridge.timings = mhdp->info->timings;
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> index bedddd510d17..6786ccb51387 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> @@ -388,6 +388,7 @@ struct cdns_mhdp_device {
>>
>> bool link_up;
>> bool plugged;
>> + bool no_hpd;
>>
>> /*
>> * "start_lock" protects the access to bridge_attached and
>
On 05/04/2023 17:24, Jayesh Choudhary wrote:
> In J721s2 EVM, DP0 HPD is not connected to correct HPD pin on SOC
> which results in HPD detect as always connnected, so when display
> is not connected driver continuously retries to read EDID and DPCD
> registers.
Where is the DP0 HPD connected to? Nowhere? If it's connected to a pin,
can that pin be used as a GPIO? In other words, do we have a HPD, just
not with mhdp, or is there just no HPD at all?
Tomi
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Jayesh Choudhary <[email protected]>
>>> ---
>>> .../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