On Fri, 15 Sept 2023 at 00:44, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Dmitry, > > On Mon, Sep 04, 2023 at 12:02:18AM +0300, Dmitry Baryshkov wrote: > > On Tue, 22 Aug 2023 at 17:17, Laurent Pinchart wrote: > > > On Thu, Aug 17, 2023 at 05:55:13PM +0300, Dmitry Baryshkov wrote: > > > > Supporting DP/USB-C can result in a chain of several transparent > > > > bridges (PHY, redrivers, mux, etc). This results in drivers having > > > > similar boilerplate code for such bridges. > > > > > > What do you mean by transparent bridge here ? Bridges are a DRM concept, > > > and as far as I can tell, a PHY isn't a bridge. Why does it need to be > > > handled as one, especially if it's completely transparent ? > > > > > > > Next, these drivers are susceptible to -EPROBE_DEFER loops: the next > > > > bridge can either be probed from the bridge->attach callback, when it is > > > > too late to return -EPROBE_DEFER, or from the probe() callback, when the > > > > next bridge might not yet be available, because it depends on the > > > > resources provided by the probing device. > > > > > > Can't device links help avoiding defer probing in those cases ? > > > > It looks like both Neil and I missed this question. > > And I missed this reply before replying to Neil and pointing to device > links again :-) > > > Two items wrt devlinks. First, I view them as a helper. So if one > > disables the devlinks enforcement, he'd still get a deferral loop. > > That may be true, but I don't think that's a compelling argument. If one > disables components meant to avoid probe deferral, they should expect > probe deferral :-) There is a difference between bare probe deferral and deferral boot loop. In this case this causes a loop, since deferred devices get reprobed after devices being created/removed (which happens during DP controller deferral). I'm fine with the occasional probe deferrals, especially if they are a result of the user's misdeed. But the deferral cycle shows that there is an issue in the device / driver structure. > > > Second, in this case we can not enforce devlinks (or return > > -EPROBE_DEFER from the probe() function) because the next bridge is > > not yet available when the main driver probes. Unfortunately bridges > > are allocated in the opposite order. So, using AUX devices helps us to > > break it. Because first typec mux/retimer/switch/etc devices probe (in > > the direction from the typec source to the typec port). Then DRM > > bridge devices are probed starting from the end of the chain > > (connector) to the DP source (root DP bridge/controller). > > I'm not too familiar with the drivers involved in the typec chain. Do > you mean that the sink driver needs to get hold of the source device at > probe time, deferring probe if the source is not available ? Does the > driver handler the USB connector need to do the same ? I'm looking at > the qcom_pmic_typec driver and I don't immediately see where it would > defer probing if its source isn't available, but I may well be missing > something. This is well hidden via the tcpm_register_port() -> typec_register_port() callchain. It checks (among other things) for the mux and retimer being present and probed. Same story applies to the retimers, which require 'previous' USB-C switch to be probed. So there is a dependency chain of qcom_pmic_typec -> nb7vpq904m -> qmp_combo_phy. For DRM bridge drivers I'd like to have the opposite dependency chain (so that the bridge knows that it can attach the next bridge): qmp_combo_phy -> nb7vpq904m -> qcom_pmic_typec. This patchset solves this by splitting drm bridges to separate aux drivers. So the resulting chain is: qmp_combo_phy.bridge -> nb7vpq904m.bridge. -> qcom_pmic_typec -> nb7vpq904m (main) -> qmp_combo_phy (main) > > > > > Last, but not least, this results in the the internal knowledge of DRM > > > > subsystem slowly diffusing into other subsystems, like PHY or USB/TYPEC. > > > > > > Why so ? The PHY subsystem should provide a PHY, without considering > > > what subsystem it will be used by. This patch series seems to me to > > > actually create this DRM dependency in other subsystems, which I don't > > > think is a very good idea. Resources should be registered in their own > > > subsystem with the appropriate API, not in a way that is tied to a > > > particular consumer. > > > > > > > To solve all these issues, define a separate DRM helper, which creates > > > > separate aux device just for the bridge. During probe such aux device > > > > doesn't result in the EPROBE_DEFER loops. Instead it allows the device > > > > drivers to probe properly, according to the actual resource > > > > dependencies. The bridge auxdevs are then probed when the next bridge > > > > becomes available, sparing drivers from drm_bridge_attach() returning > > > > -EPROBE_DEFER. > > > > > > I'm not thrilled :-( Let's discuss the questions above first. > > > > Laurent, please excuse me for the ping. Any further response from your side? > > I'd like to send the next iteration of this patchset. > > > > > > Proposed merge strategy: immutable branch with the drm commit, which is > > > > then merged into PHY and USB subsystems together with the corresponding > > > > patch. -- With best wishes Dmitry
