[Freedreno] [PATCH v4 0/3] drm: simplify support for transparent DRM bridges

[Neil Armstrong]

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
>