On Fri, 15 Sept 2023 at 00:23, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Neil, > > Sorry about the delay, the series got burried in my inbox. > > On Tue, Aug 22, 2023 at 04:27:37PM +0200, Neil Armstrong wrote: > > On 22/08/2023 16:19, Laurent Pinchart wrote: > > > On Tue, Aug 22, 2023 at 05:17:37PM +0300, 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 ? > > >> > > >>> 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, > > > > > > I was wrong on this one, there are indeed existing drm_bridge instances > > > in drivers/usb/ and drivers/phy/. That's certainly not nice. Why do we > > > even need drm_bridge there, why can't the PHYs be acquired by their > > > consumers in DRM (and anywhere else) using the PHY API ? > > > > Because with USB-C Altmode/USB4/Thunderbolt, DisplayPort is one of the > > data streams handled by PHYs, USB-C PD manager, re-timers, SBU muxes... > > and all this must be coordinated with the display controller and can > > be considered as bridges between the DP controller and the USB-C connector. > > > > As of today, it has been handled by OOB events on Intel & AMD, but the entirety > > of USB-C chain is handled in firmare, so this scales. > > When we need to describe the entire USB-C data stream chain as port/endpoint > > in DT, OOB handling doesn't work anymore since we need to sync the entire > > USB-C chain (muxes, switches, retimers, phys...) handled by Linux before > > starting the DP stream. > > No disagreement here. Handling the component as part of the bridges > chain certainly helps. Ideally, this should be done without spreading > usage of drm_bridge outside of the DRM subsystem. For instance, we > handle (some) D-PHYs in DRM and V4L2 by exposing them as PHYs, and > acquiring them in DSI or CSI-2 controller drivers. This is true. We tried doing that. This quickly results in DT not describing the actual hardware. Consider the SS lanes of the USB-C controller. They should go to some kind of mux that switches them between DP and USB-SS controllers. In our case such a mux is the USB+DP PHY. So it becomes used both via tha phys = <> property and via the OF graph. And as we do not want to circumvent the drm_bridge OF-related code, this OF graph link results in an extra drm_bridge being created on the path to the final drm_bridge in TCPM, which actually implements HPD ops. > Do I understand correctly that, in this case, the video stream is fully > handled by the PHY (& related) component, without any other device (in > the OF sense) wrapping the PHY like the DSI and CSI-2 controllers do ? > If so that would indeed make it difficult to create the drm_bridge in a > DRM driver that would acquire the PHY. We could come up with a different > mechanism, but that's likely overkill to solve this particular issue (at > least until other similar use cases create a critical mass that will > call for a major refactoring). > > In this specific case, however, I'm a bit puzzled. What coordination is > required between the PHYs and the display controller ? The two drivers > modified in patches 2/3 and 3/3 indeed create bridges, but those bridges > don't implement any operation other than attach. Is this needed only > because the PHY has an OF node that sits between the display controller > and the connector, requiring a drm_bridge to exist to bridge the gap and > create a complete chain of bridges up to the connector ? This would > simplify the use case, but probably still call for creating a > drm_bridge in the PHY driver, as other solutions are likely still too > complex. Yes, these bridges just fill gaps in the bridge chain. HPD events are generated in the TCPM / altmode driver, so there should be a bridge there. > > It seems to me that this series tries to address two issues. One of them > is minimizing the DRM-specific amount of code needed in the PHY drivers. > The second one is to avoid probe deferrals. For the first issue, I agree > that a helper is currently a good option. For the second issue, however, > couldn't device links help avoiding probe deferral ? If so, the helper > could be simplified, avoiding the need to create an auxiliary device. This is largely discussed in the other subthread. > > > >> 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. > > >> > > >>> Proposed merge strategy: immutable branch with the drm commit, which is > > >>> then merged into PHY and USB subsystems together with the corresponding > > >>> patch. > > >>> > > >>> Changes since v3: > > >>> - Moved bridge driver to gpu/drm/bridge (Neil Armstrong) > > >>> - Renamed it to aux-bridge (since there is already a simple_bridge driver) > > >>> - Made CONFIG_OF mandatory for this driver (Neil Armstrong) > > >>> - Added missing kfree and ida_free (Dan Carpenter) > > >>> > > >>> Changes since v2: > > >>> - ifdef'ed bridge->of_node access (LKP) > > >>> > > >>> Changes since v1: > > >>> - Added EXPORT_SYMBOL_GPL / MODULE_LICENSE / etc. to drm_simple_bridge > > >>> > > >>> Dmitry Baryshkov (3): > > >>> drm/bridge: add transparent bridge helper > > >>> phy: qcom: qmp-combo: switch to DRM_AUX_BRIDGE > > >>> usb: typec: nb7vpq904m: switch to DRM_AUX_BRIDGE > > >>> > > >>> drivers/gpu/drm/bridge/Kconfig | 9 ++ > > >>> drivers/gpu/drm/bridge/Makefile | 1 + > > >>> drivers/gpu/drm/bridge/aux-bridge.c | 132 ++++++++++++++++++++++ > > >>> drivers/phy/qualcomm/Kconfig | 2 +- > > >>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 44 +------- > > >>> drivers/usb/typec/mux/Kconfig | 2 +- > > >>> drivers/usb/typec/mux/nb7vpq904m.c | 44 +------- > > >>> include/drm/bridge/aux-bridge.h | 19 ++++ > > >>> 8 files changed, 167 insertions(+), 86 deletions(-) > > >>> create mode 100644 drivers/gpu/drm/bridge/aux-bridge.c > > >>> create mode 100644 include/drm/bridge/aux-bridge.h > > -- > Regards, > > Laurent Pinchart -- With best wishes Dmitry
