Hi,
On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
@@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = client->dev.of_node;
pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT)Checking for "no_hpd" here is not the right test IIUC. You want to check for eDP vs. DP (AKA whether a panel is downstream of you or a connector). Specifically if downstream of you is a panel then (I believe) HPD won't assert until you turn on the panel and you won't turn on the panel (which happens in pre_enable, right?) until HPD fires, so you've got a chicken-and-egg problem. If downstream of you is a connector, though, then by definition HPD has to just work without pre_enable running so then you're OK.
Agreed. It's even more true now that your rework has landed, as in the eDP case EDID is handled by the panel driver. I'll rework this.
Should I also condition setting HPD_DISABLE to the presence of a panel then ? I could drop of_property_read_bool() and set
pdata->no_hpd = !!panel;I guess then you'd need to figure out what to do if someone wants to use "HPD" on eDP. Do you need to put a polling loop in pre_enable then? Or you could just punt not support this case until someone needs it.
I think I'll stop short of saving the world this time, yes :-) We'll see what to do when this case arises.
How about as a compromise you still call of_property_read_bool() but add some type of warning in the logs if someone didn't set "no-hpd" but they have a panel?
| DRM_BRIDGE_OP_EDID;IMO somewhere in here if HPD is being used like this you should throw in a call to pm_runtime_get_sync(). I guess in your solution the regulators (for the bridge, not the panel) and enable pin are just left on all the time,
Correct, on my development board the SN65DSI86 is on all the time, I can't control that.
but plausibly someone might want to build a system to use HPD and also have the enable pin and/or regulators controlled by this driver, right?
True. DRM doesn't make this very easy, as, as far as I can tell, there's no standard infrastructure for userspace to register an interest in HPD that could be notified to bridges. I think it should be fixable, but it's out of scope for this series :-) Should I still add a pm_runtime_get_sync() at probe time, or leave this to be addressed by someone who will need to implement power control ?
IMO if you've detected you're running in DP mode you should add the pm_runtime_get_sync() in probe to keep it powered all the time and that seems the simplest. Technically I guess that's not really required since you're polling and you could power off between polls, but then you'd have to re-init a bunch of your state each time you polled too. If you ever transitioned to using an IRQ for HPD then you'd have to keep it always powered anyway.
-Doug