Re: [PATCH v2 2/2] drm: bridge: cdns-mhdp8546: Add support for no-hpd

[Jayesh Choudhary <j-choudhary@ti.com>]

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@ti.com>
>>
>> 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@ti.com>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   .../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
>