Re: [PATCH v5 4/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 29/08/2022 20:38, Doug Anderson wrote:
Hi,

On Wed, Aug 24, 2022 at 6:00 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:

From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>

Implement the bridge connector-related .get_edid() and .detect()
operations, and report the related bridge capabilities and type.

These ops are only added for DP mode. They should also be used for eDP
mode, but the driver seems to be mostly used for eDP and, according to
the comments, they've had issues with eDP panels and HPD. So better be
safe and only enable them for DP for now.

Just to be clear: the "They should also be used for eDP" is not correct.

* The detect() function should be returning whether the display is
physically there. For eDP it is _always_ physically there. Thus for

Really? I thought detect() is the polling counter-part of HPD interrupt. What is the point of returning true from detect() if the display is there, but cannot be used?

eDP the _correct_ implementation for detect is to always return true.
Yes, there is a line called HPD for eDP and yes that line is used for
full DisplayPort for detecting a display. For eDP, though, HPD does
not detect the presence of a display. A display is always there.

But for eDP it still signals the actual availability of the display, similarly to DP, doesn't it? You can't communicate with the monitor or read the EDID until you get the HPD.

* For eDP implementing get_edid() is done in the panel so that power
sequencing can be done properly. While it could have been designed
other ways, that's how we ended up in the end. Thus eDP controllers
don't implement get_edid().

Ok. I guess eDP panels do what they want and the drivers cannot rely on the HPD.

Or is the whole point here that because eDP panel drivers deal with the panel quirks, the get_edid() and also detect (if any) is handled by the eDP panel driver, and thus the bridge should not implement get_edid() nor detect() for eDP?

@@ -1163,10 +1165,33 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
         pm_runtime_put_sync(pdata->dev);
  }

+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;
+
+       pm_runtime_get_sync(pdata->dev);
+       regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
+       pm_runtime_put_autosuspend(pdata->dev);
+
+       return val & HPD_DEBOUNCED_STATE ? connector_status_connected
+                                        : connector_status_disconnected;
+}

I thought in the end we decided that you _could_ get a hot plug detect
interrupt if you just did a pm_runtime_get_sync() sometime earlier in
the case of DP. Basically you're just saying that if you're DP that
you always powered up. Doing some searches makes me find some
discussion at:

https://lore.kernel.org/r/20220310152227.2122960-4-kieran.bingham+renesas@xxxxxxxxxxxxxxxx

Specifically, the right answer is: "In general the pm_runtime_get
reference need to go with the IRQ enabling"

In any case, if we want to start with just implementing "detect"
that's OK with me...

Yes, I have the HPD interrupt working in my branch, kind of. The problem is that with the HPD interrupt I encountered issues (even if the monitor was always connected): every now and then the dsi86 does not display anything and I get a spam of LOSS_OF_DP_SYNC_LOCK_ERR errors, and I couldn't figure out the problem. All the registers on the DSI source and DSI sink side looked identical, so it hints to some kind of race issue, which might well be there even with polling, but just doesn't seem to trigger.

To make things worse, the board in question is a remote board and I can't actually test the HPD, i.e. plugging in and out the cable, changing the monitors, powering up/down the monitors, etc.

On top of that, a few years back I had a lot of problems working on Cadence DP controller, dealing with all kinds of corner case race issues with DP HPD and trying to comply with the DP spec, which made me realize that DP is just really complex.

So, I thought it's better if I just try to get a minimum version working so that we can have a picture on a monitor, without even trying to claim real HPD support.

 Tomi



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux