On 22/08/2023 16:19, Laurent Pinchart 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 ?
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.
Neil
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
