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 <j-choudh...@ti.com> 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]: <https://www.ti.com/lit/gpn/SN65DSI86> (Pg. 32) > > > > Fixes: c312b0df3b13 ("drm/bridge: ti-sn65dsi86: Implement bridge connector > > operations for DP") > > Cc: Max Krummenacher <max.krummenac...@toradex.com> > > Signed-off-by: Jayesh Choudhary <j-choudh...@ti.com> > > --- > > > > v1 patch link which was sent as RFC: > > <https://patchwork.kernel.org/project/dri-devel/patch/20250424105432.255309-1-j-choudh...@ti.com/> > > > > 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: > > <https://patchwork.kernel.org/project/dri-devel/patch/20250501074805.3069311-1-max.oss...@gmail.com/> > > > > 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()? > > Nicely, this would be the same type of solution needed for if we ever > enabled interrupts. -- With best wishes Dmitry
