| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Apr 2023 20:40:18 +0530
From: Jayesh Choudhary <j-choudhary@...com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: <dri-devel@...ts.freedesktop.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <krzysztof.kozlowski@...aro.org>,
<andrzej.hajda@...el.com>, <neil.armstrong@...aro.org>,
<rfoss@...nel.org>, <jonas@...boo.se>, <jernej.skrabec@...il.com>,
<airlied@...il.com>, <daniel@...ll.ch>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <sam@...nborg.org>,
<jani.nikula@...el.com>, <tzimmermann@...e.de>,
<javierm@...hat.com>, <ville.syrjala@...ux.intel.com>,
<r-ravikumar@...com>, <lyude@...hat.com>,
<alexander.deucher@....com>, <sjakhade@...ence.com>,
<yamonkar@...ence.com>, <a-bhatia1@...com>,
<tomi.valkeinen@...asonboard.com>
Subject: Re: [PATCH v2 2/2] drm: bridge: cdns-mhdp8546: Add support for no-hpd
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@...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@...com>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@...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
>
Powered by blists - more mailing lists