From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fllvem-ot04.ext.ti.com (fllvem-ot04.ext.ti.com [198.47.19.246]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EB53F16A956 for ; Mon, 26 May 2025 07:44:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.47.19.246 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748245464; cv=none; b=J930Gqp/E2szbEY8GmHS1PKJFBJf262gzgW5BK3yEZ888PoKRBZQT+Y9CiyFFfawsH+MxLaC/Cb6ln/q4wCq6M5dQpPKUDSCLW8G1TV0HfKAwBBJ+pt5HJTAbtIscJlNLAtd+BzphPABRspCqEspoBeI3NrbZ1GfnTMRy617Blw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748245464; c=relaxed/simple; bh=f+ESKyFJ+mdFlEs1iGJbPVKlmFTChKxh4lvJirCZ+GQ=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=DAyBaDG3UTQMwdLNWr/IvsmPCNs6qrJPxZy5c4rVVT2BgdkQAyHY3RgP8bgwONJAUMg/wdKkU4G6vop/+06bmsy9dowBZ/FzY2NVRm9tFwRQO6JOTpEnoq1SXVlM2Ar94qKCajT9KoMsoRDSX2BK/XMJLGLoaWutkL6ZuB+LZjU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com; spf=pass smtp.mailfrom=ti.com; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b=Y4ykRYuv; arc=none smtp.client-ip=198.47.19.246 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ti.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="Y4ykRYuv" Received: from fllvem-sh03.itg.ti.com ([10.64.41.86]) by fllvem-ot04.ext.ti.com (8.15.2/8.15.2) with ESMTP id 54Q7hhVl1446791; Mon, 26 May 2025 02:43:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1748245423; bh=pfNxNBtxFbE6b5isOFlmebHM/gu4Na0OpYCEd6fKOLo=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=Y4ykRYuvtep7ANL8NgvCo/Gd1Y3XuSfhlsTLDmys8WVn/3/WPFKJc56n2AO2XiXqQ EfKp72trsCjM7LJrsoa/CEwth68Hi3Phw16tkKb0t5pBetaz6a2knfpNWSKmjTKfoD ikT5yK4uUGdP0tgRywalustnZFIfLFb7D4vZv9Hs= Received: from DLEE108.ent.ti.com (dlee108.ent.ti.com [157.170.170.38]) by fllvem-sh03.itg.ti.com (8.18.1/8.18.1) with ESMTPS id 54Q7hgSn1960879 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA256 bits=128 verify=FAIL); Mon, 26 May 2025 02:43:42 -0500 Received: from DLEE113.ent.ti.com (157.170.170.24) by DLEE108.ent.ti.com (157.170.170.38) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Mon, 26 May 2025 02:43:42 -0500 Received: from lelvem-mr06.itg.ti.com (10.180.75.8) by DLEE113.ent.ti.com (157.170.170.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Mon, 26 May 2025 02:43:42 -0500 Received: from [10.24.72.182] (jayesh-hp-z2-tower-g5-workstation.dhcp.ti.com [10.24.72.182]) by lelvem-mr06.itg.ti.com (8.18.1/8.18.1) with ESMTP id 54Q7hZCE2635573; Mon, 26 May 2025 02:43:36 -0500 Message-ID: <97764129-e78a-47d2-8f2c-e219b3686f53@ti.com> Date: Mon, 26 May 2025 13:13:34 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort connector type To: Dmitry Baryshkov , Doug Anderson CC: , , , , , , , , , , , , , , , , , References: <20250508115433.449102-1-j-choudhary@ti.com> Content-Language: en-US From: Jayesh Choudhary In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-C2ProcessedOrg: 333ef613-75bf-4e12-a4b1-8e3623f5dcea Hello Dmitry, Doug, Thanks a lot for the review. On 22/05/25 18:44, Dmitry Baryshkov wrote: > On Wed, May 21, 2025 at 06:10:59PM -0700, Doug Anderson wrote: >> Hi, >> >> On Thu, May 8, 2025 at 4:54 AM Jayesh Choudhary wrote: >>> >>> By default, HPD was disabled on SN65DSI86 bridge. When the driver was >>> added (commit "a095f15c00e27"), the HPD_DISABLE bit was set in pre-enable >>> call which was moved to other function calls subsequently. >>> Later on, commit "c312b0df3b13" added detect utility for DP mode. But with >>> HPD_DISABLE bit set, all the HPD events are disabled[0] and the debounced >>> state always return 1 (always connected state) >>> >>> Also, with the suspend and resume calls before every register access, the >>> bridge starts with disconnected state and the HPD state is reflected >>> correctly only after debounce time (400ms). However, adding this delay >>> in the detect function causes frame drop and visible glitch in display. >>> >>> So to get the detect utility working properly for DP mode without any >>> performance issues in display, instead of reading HPD state from the >>> register, rely on aux communication. Use 'drm_dp_dpcd_read_link_status' >>> to find if we have something connected at the sink. >>> >>> [0]: (Pg. 32) >>> >>> Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP") >>> Cc: Max Krummenacher >>> Signed-off-by: Jayesh Choudhary >>> --- >>> >>> v1 patch link which was sent as RFC: >>> >>> >>> Changelog v1->v2: >>> - Drop additional property in bindings and use conditional. >>> - Instead of register read for HPD state, use dpcd read which returns 0 >>> for success and error codes for no connection >>> - Add relevant history for the required change in commit message >>> - Drop RFC subject-prefix in v2 as v2 is in better state after discussion >>> in v1 and Max's mail thread >>> - Add "Cc:" tag >>> >>> This approach does not make suspend/resume no-op and no additional >>> delay needs to be added in the detect hook which causes frame drops. >>> >>> Here, I am adding conditional to HPD_DISABLE bit even when we are >>> not using the register read to get HPD state. This is to prevent >>> unnecessary register updates in every resume call. >>> (It was adding to latency and leading to ~2-3 frame drop every 10 sec) >>> >>> Tested and verified on TI's J784S4-EVM platform: >>> - Display comes up >>> - Detect utility works with a couple of seconds latency. >>> But I guess without interrupt support, this is acceptable. >>> - No frame-drop observed >>> >>> Discussion thread for Max's patch: >>> >>> >>> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 ++++++++++------- >>> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> Sorry for the delay in responding. Things got a little crazy over the >> last few weeks. >> >> >>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c >>> index 60224f476e1d..9489e78b6da3 100644 >>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c >>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c >>> @@ -352,8 +352,10 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata, >>> * change this to be conditional on someone specifying that HPD should >>> * be used. >>> */ >>> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, >>> - HPD_DISABLE); >>> + >>> + if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP) >>> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, >>> + HPD_DISABLE); >> >> Given your an Max's testing, I'm totally on-board with the above. >> >>> >>> pdata->comms_enabled = true; >>> >>> @@ -1194,13 +1196,14 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) >>> { >>> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); >>> int val = 0; >>> + u8 link_status[DP_LINK_STATUS_SIZE]; >>> >>> - pm_runtime_get_sync(pdata->dev); >>> - regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val); >>> - pm_runtime_put_autosuspend(pdata->dev); >>> + val = drm_dp_dpcd_read_link_status(&pdata->aux, link_status); >>> >>> - return val & HPD_DEBOUNCED_STATE ? connector_status_connected >>> - : connector_status_disconnected; >>> + if (val < 0) >>> + return connector_status_disconnected; >>> + else >>> + return connector_status_connected; >> >> I'd really rather not do this. It took me a little while to realize >> why this was working and also not being slow like your 400ms delay >> was. I believe that each time you do the AUX transfer it grabs a >> pm_runtime reference and then puts it with "autosuspend". Then you're >> relying on the fact that detect is called often enough so that the >> autosuspend doesn't actually hit so the next time your function runs >> then it's fast. Is that accurate? >> >> I'd rather see something like this in the bridge's probe (untested) >> >> if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) { >> pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT; >> >> /* >> * In order for DRM_BRIDGE_OP_DETECT to work in a reasonable >> * way we need to keep the bridge powered on all the time. >> * The bridge takes hundreds of milliseconds to debounce HPD >> * and we simply can't wait that amount of time in every call >> * to detect. >> */ >> pm_runtime_get_sync(pdata->dev); >> } >> >> ...obviously you'd also need to find the right times to undo this in >> error handling and in remove. > > What about: > - keeping pm_runtime_get()/put_autosuspend() in detect, but.. > - also adding .hpd_enable() / .hpd_disable() callbacks which would also > get and put the runtime PM, making sure that there is no additional > delay in .detect()? > Keeping a reference alive via hpd_enable() fixes the issue. Things works with the previous detect logic and I do not need to rely on reading link status. In hpd_enable()/disable(), I do not need to add anything else: +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge) +{ + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + pm_runtime_get_sync(pdata->dev); +} + +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge) +{ + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + pm_runtime_put_sync(pdata->dev); +} + Posting v3 with these changes. Warm Regards, Jayesh >> >> Nicely, this would be the same type of solution needed for if we ever >> enabled interrupts. >