mbox series

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

Message ID 20230817145516.5924-1-dmitry.baryshkov@linaro.org
Headers show
Series drm: simplify support for transparent DRM bridges | expand

Message

Dmitry Baryshkov Aug. 17, 2023, 2:55 p.m. UTC
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.

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.

Last, but not least, this results in the the internal knowledge of DRM
subsystem slowly diffusing into other subsystems, like PHY or USB/TYPEC.

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.

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

Comments

Laurent Pinchart Aug. 22, 2023, 2:17 p.m. UTC | #1
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
Laurent Pinchart Aug. 22, 2023, 2:19 p.m. UTC | #2
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
Neil Armstrong Aug. 22, 2023, 2:27 p.m. UTC | #3
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
>
Dmitry Baryshkov Aug. 28, 2023, 10:28 p.m. UTC | #4
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
Dmitry Baryshkov Sept. 3, 2023, 9:02 p.m. UTC | #5
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.
Laurent Pinchart Sept. 14, 2023, 9:23 p.m. UTC | #6
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
Laurent Pinchart Sept. 14, 2023, 9:44 p.m. UTC | #7
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.
Dmitry Baryshkov Sept. 14, 2023, 10:01 p.m. UTC | #8
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.
Dmitry Baryshkov Sept. 14, 2023, 10:10 p.m. UTC | #9
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