| Message ID | 20230817145516.5924-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | drm: simplify support for transparent DRM bridges | expand |
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, 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
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 ? > 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
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 >
On Tue, 22 Aug 2023 at 17:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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
On Tue, 22 Aug 2023 at 17:17, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 ? It looks like both Neil and I missed this question. 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. 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). > > > 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.
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. 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. 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. > >> 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
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 :-) > 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. > > > 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.
On Fri, 15 Sept 2023 at 00:44, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.
On Fri, 15 Sept 2023 at 00:23, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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