On Tue, 22 Aug 2023 at 17:19, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > On Tue, Aug 22, 2023 at 05:17:37PM +0300, Laurent Pinchart wrote: > > Hi Dmitry, > > > > Thank you for the patches. > > > > 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 ? During the design of the DT bindings for DisplayPort on these platforms we have evaluated different approaches. First approach was to add a special 'displayport' property to the USB-C connector, pointing to DP. This approach was declined by DT maintainers. Then we tried different approaches towards building connection graphs between different parties. Finally it was determined that the easiest way is to describe all USB-C signal paths properly. SS lines start at the PHY, then they pass through different muxes and retimers and then end up at the usb-c-connector. SS lines are muxed by the USB+DP PHY and switched between USB-3 (SuperSpeed) and DP. Then comes the question of binding everything together from the DRM point of view. The usb-c-connector is the logical place for the last drm_bridge, unfortunately. We need to send HPD events from the TypeC port driver (either directly or from the altmode driver). Then either we have to point the connector->fwnode to the DP port or build the full drm_bridge chain. Second path was selected, as it fits better into the overall framework. > > > 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
