| Message ID | 20210817215201.795062-9-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm + usb-type-c: Add support for out-of-band hotplug notification (v4 resend) | expand |
Quoting Hans de Goede (2021-08-17 14:52:01) > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c > index aa669b9cf70e..c1d8c23baa39 100644 > --- a/drivers/usb/typec/altmodes/displayport.c > +++ b/drivers/usb/typec/altmodes/displayport.c > @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con) > static int dp_altmode_status_update(struct dp_altmode *dp) > { > bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf); > + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE); > u8 con = DP_STATUS_CONNECTION(dp->data.status); > int ret = 0; > > @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp) > ret = dp_altmode_configure(dp, con); > if (!ret) > dp->state = DP_STATE_CONFIGURE; > + } else { > + if (dp->hpd != hpd) { > + drm_connector_oob_hotplug_event(dp->connector_fwnode); > + dp->hpd = hpd; > + } > } > > return ret; > @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = { > int dp_altmode_probe(struct typec_altmode *alt) > { > const struct typec_altmode *port = typec_altmode_get_partner(alt); > + struct fwnode_handle *fwnode; > struct dp_altmode *dp; > int ret; > > @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt) > alt->desc = "DisplayPort"; > alt->ops = &dp_altmode_ops; > > + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */ > + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0); I'm trying to figure out how to translate this over to DT bindings. Is there a binding document for this fwnode reference? If not, can you please update Documentation/devicetree/bindings/connector/usb-connector.yaml with this property? I think this means that the type-c node would have a 'displayport = <&some_phandle>' property in it that points to the display port hardware device that's pumping out the DisplayPort data? > + if (IS_ERR(dp->connector_fwnode)) > + dp->connector_fwnode = NULL; > + > typec_altmode_set_drvdata(alt, dp); > > dp->state = DP_STATE_ENTER; > @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt) > > sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group); > cancel_work_sync(&dp->work); > + > + if (dp->connector_fwnode) { > + if (dp->hpd) > + drm_connector_oob_hotplug_event(dp->connector_fwnode); I was hoping that we could make a type-c connector into a drm_bridge. I'm thinking that it would be a DP-to-panel bridge. Then a panel could be created as well on the end of the type-c connector and the bridge would report hpd whenever the type-c logic figures out the cable has been connected and hpd is asserted. The actual DisplayPort hardware that's encoding data would then find the bridge through the graph binding connected to the output node. I'm not sure how MST is handled though. In that scenario maybe there's more than one panel? If you're interested the dts file that I'm trying to make this work for is sc7180-trogdor.dtsi and I need to hook up mdss_dp's output port to the two type-c connectors, usb_c0 and usb_c1, somehow. The two ports are actually muxed by the EC (parent node) so only one type-c port can be connected to the DP hardware at a time. > + > + fwnode_handle_put(dp->connector_fwnode); > + }
Hi, On 9/16/21 5:20 AM, Stephen Boyd wrote: > Quoting Hans de Goede (2021-08-17 14:52:01) >> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c >> index aa669b9cf70e..c1d8c23baa39 100644 >> --- a/drivers/usb/typec/altmodes/displayport.c >> +++ b/drivers/usb/typec/altmodes/displayport.c >> @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con) >> static int dp_altmode_status_update(struct dp_altmode *dp) >> { >> bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf); >> + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE); >> u8 con = DP_STATUS_CONNECTION(dp->data.status); >> int ret = 0; >> >> @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp) >> ret = dp_altmode_configure(dp, con); >> if (!ret) >> dp->state = DP_STATE_CONFIGURE; >> + } else { >> + if (dp->hpd != hpd) { >> + drm_connector_oob_hotplug_event(dp->connector_fwnode); >> + dp->hpd = hpd; >> + } >> } >> >> return ret; >> @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = { >> int dp_altmode_probe(struct typec_altmode *alt) >> { >> const struct typec_altmode *port = typec_altmode_get_partner(alt); >> + struct fwnode_handle *fwnode; >> struct dp_altmode *dp; >> int ret; >> >> @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt) >> alt->desc = "DisplayPort"; >> alt->ops = &dp_altmode_ops; >> >> + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */ >> + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0); > > I'm trying to figure out how to translate this over to DT bindings. First of all let me say that it is great that people are looking into using this functionality outside of the niche application for which I wrote it. > Is > there a binding document for this fwnode reference? If not, can you > please update > Documentation/devicetree/bindings/connector/usb-connector.yaml with this > property? My use case is some Intel Cherry Trail based mini-laptops which use a Intel Whiskey Cove PMIC with a FUSB302 Type-C controller + a PI3USB30532 USB switch, specifically the GPD Win and GPD Pocket (first generation of each). These are ACPI/X86 devices so devicetree is not used there, the connector_fwnode checked here is actually sw_node, this sw_node reference gets setup here: drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c The setup/use of a sw_node reference for this was actually designed by Heikki (in the Cc already), so he might be a better person to answer your questions. With that said, as to your question if I can document this in usb-connector.yaml, no I cannot (sorry). Since this is a sw_node setup by X86 code and not a normal devicetree fwnode reference, atm this is purely a kernel internal API/binding really and the DT-binding maintainers have repeatedly told me that I should NOT submit DT-binding updates for these. Only once a real devicetree user for this shows up will the accept DT-bindings patches for this. The good news here though is, that if this turns out to be non-ideal for the devicetree case we can still change things as long as drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c also gets updated so as to not break things. > I think this means that the type-c node would have a 'displayport = > <&some_phandle>' property in it that points to the display port hardware > device that's pumping out the DisplayPort data? It points to a fwnode belonging to the drm-connector for the DisplayPort connector/pins to which the Type-C data-lines mux/switch is connected, see: https://cgit.freedesktop.org/drm-misc/commit/?id=48c429c6d18db115c277b75000152d8fa4cd35d0 https://cgit.freedesktop.org/drm-misc/commit/?id=3d3f7c1e68691574c1d87cd0f9f2348323bc0199 https://cgit.freedesktop.org/drm-intel/commit/?id=a481d0e80eabbc3fed666103744aeaf47f63e708 So this get working with devicetree you would need something like this: 1. Have devicetree-node-s describing video-output connectors, similar to how we have usb-connector nodes to describe usb-connectors 2. Have your drm/kms driver lookup these video-output-connector of_node-s and assign them to drm_connector.fwnode, at least for the DP video-output used. 3. Have the usb-connector node for your Type-C connector (the same of_node as which has the source-pdos, sink-pdos, etc props) have a 'displayport = <&some_phandle>' property pointing to the of_node for the DP video-output used. I hope this helps. > >> + if (IS_ERR(dp->connector_fwnode)) >> + dp->connector_fwnode = NULL; >> + >> typec_altmode_set_drvdata(alt, dp); >> >> dp->state = DP_STATE_ENTER; >> @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt) >> >> sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group); >> cancel_work_sync(&dp->work); >> + >> + if (dp->connector_fwnode) { >> + if (dp->hpd) >> + drm_connector_oob_hotplug_event(dp->connector_fwnode); > > I was hoping that we could make a type-c connector into a drm_bridge. > I'm thinking that it would be a DP-to-panel bridge. Then a panel could > be created as well on the end of the type-c connector and the bridge > would report hpd whenever the type-c logic figures out the cable has > been connected and hpd is asserted. The actual DisplayPort hardware > that's encoding data would then find the bridge through the graph > binding connected to the output node. > > I'm not sure how MST is handled though. In that scenario maybe there's > more than one panel? Yeah, given that MST over DP over Type-C is very much possible my first instinct is that this drm_bridge + bridge-to-panel idea is not going to work very well. Also the output could be anything, it could be a projector, or the DP input of a video-grabber, or ... so modelling this as a panel feels wrong. Regards, Hans
(Sorry for the slow reply) Quoting Hans de Goede (2021-09-16 06:17:56) > Hi, > > On 9/16/21 5:20 AM, Stephen Boyd wrote: > > Quoting Hans de Goede (2021-08-17 14:52:01) > >> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c > >> index aa669b9cf70e..c1d8c23baa39 100644 > >> --- a/drivers/usb/typec/altmodes/displayport.c > >> +++ b/drivers/usb/typec/altmodes/displayport.c > >> @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con) > >> static int dp_altmode_status_update(struct dp_altmode *dp) > >> { > >> bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf); > >> + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE); > >> u8 con = DP_STATUS_CONNECTION(dp->data.status); > >> int ret = 0; > >> > >> @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp) > >> ret = dp_altmode_configure(dp, con); > >> if (!ret) > >> dp->state = DP_STATE_CONFIGURE; > >> + } else { > >> + if (dp->hpd != hpd) { > >> + drm_connector_oob_hotplug_event(dp->connector_fwnode); > >> + dp->hpd = hpd; > >> + } > >> } > >> > >> return ret; > >> @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = { > >> int dp_altmode_probe(struct typec_altmode *alt) > >> { > >> const struct typec_altmode *port = typec_altmode_get_partner(alt); > >> + struct fwnode_handle *fwnode; > >> struct dp_altmode *dp; > >> int ret; > >> > >> @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt) > >> alt->desc = "DisplayPort"; > >> alt->ops = &dp_altmode_ops; > >> > >> + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */ > >> + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0); > > > > I'm trying to figure out how to translate this over to DT bindings. > > First of all let me say that it is great that people are looking > into using this functionality outside of the niche application > for which I wrote it. Glad to hear it! This sort of thing is the norm on ARM based SoCs. I rarely ever see the HPD pin actually connected to the GPU/DPU hardware. It almost always goes through some other chip and then software has to deal with it by looking at the gpio or external hardware block, etc. > > > Is > > there a binding document for this fwnode reference? If not, can you > > please update > > Documentation/devicetree/bindings/connector/usb-connector.yaml with this > > property? > > My use case is some Intel Cherry Trail based mini-laptops which use a > Intel Whiskey Cove PMIC with a FUSB302 Type-C controller + a > PI3USB30532 USB switch, specifically the GPD Win and GPD Pocket > (first generation of each). > > These are ACPI/X86 devices so devicetree is not used there, the > connector_fwnode checked here is actually sw_node, this sw_node > reference gets setup here: > > drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c > > The setup/use of a sw_node reference for this was actually > designed by Heikki (in the Cc already), so he might be a better > person to answer your questions. Ok, thanks for the background. > > With that said, as to your question if I can document this in > usb-connector.yaml, no I cannot (sorry). Since this is a sw_node > setup by X86 code and not a normal devicetree fwnode reference, > atm this is purely a kernel internal API/binding really and the > DT-binding maintainers have repeatedly told me that I should NOT > submit DT-binding updates for these. Only once a real devicetree > user for this shows up will the accept DT-bindings patches for this. > > The good news here though is, that if this turns out to be > non-ideal for the devicetree case we can still change things > as long as drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c > also gets updated so as to not break things. I see. At least there's an escape hatch. > > > I think this means that the type-c node would have a 'displayport = > > <&some_phandle>' property in it that points to the display port hardware > > device that's pumping out the DisplayPort data? > > It points to a fwnode belonging to the drm-connector for the DisplayPort > connector/pins to which the Type-C data-lines mux/switch is connected, see: > > https://cgit.freedesktop.org/drm-misc/commit/?id=48c429c6d18db115c277b75000152d8fa4cd35d0 > https://cgit.freedesktop.org/drm-misc/commit/?id=3d3f7c1e68691574c1d87cd0f9f2348323bc0199 > https://cgit.freedesktop.org/drm-intel/commit/?id=a481d0e80eabbc3fed666103744aeaf47f63e708 > > So this get working with devicetree you would need something like this: > > 1. Have devicetree-node-s describing video-output connectors, similar > to how we have usb-connector nodes to describe usb-connectors > > 2. Have your drm/kms driver lookup these video-output-connector of_node-s > and assign them to drm_connector.fwnode, at least for the DP video-output > used. > > 3. Have the usb-connector node for your Type-C connector (the same of_node > as which has the source-pdos, sink-pdos, etc props) have a > 'displayport = <&some_phandle>' property pointing to the of_node for > the DP video-output used. > > I hope this helps. Yep. For DT we already have a graph binding to show the output of the display hardware (DPU). Right now it's not connected to anything though. I'm currently thinking that a clean solution is to connect that graph endpoint to some mux node and then have the mux node connect to each type-c node. Then a driver that registers the mux and type-c connectors can generate a drm_bridge instance (or instances) and drm can chain them all together by walking the graph and connecting the DPU to the mux bridge and then each type-c connector's bridge. I don't know how a mux would work in the drm bridge design, so either that becomes a first class type of operation or we register two bridges in the mux driver and mux in the backend. This feels similar to how we handle DSI-to-eDP bridges, except that the end of the bridge chain is a type-c connector that could have MST to deal with. I'm not super familiar with MST but maybe MST could also become bridge aware and then we wouldn't have to do anything special. I'll cross that bridge when I come to it. I image the DPU would search the graph for a panel and then if there isn't a panel downstream I think it would make one up, or do nothing? I'm not very sure about this part either. Anyway, I'm going to try to prototype out some code to do this unless someone wants to say it's a terrible idea. Let me know. > > > > >> + if (IS_ERR(dp->connector_fwnode)) > >> + dp->connector_fwnode = NULL; > >> + > >> typec_altmode_set_drvdata(alt, dp); > >> > >> dp->state = DP_STATE_ENTER; > >> @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt) > >> > >> sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group); > >> cancel_work_sync(&dp->work); > >> + > >> + if (dp->connector_fwnode) { > >> + if (dp->hpd) > >> + drm_connector_oob_hotplug_event(dp->connector_fwnode); > > > > I was hoping that we could make a type-c connector into a drm_bridge. > > I'm thinking that it would be a DP-to-panel bridge. Then a panel could > > be created as well on the end of the type-c connector and the bridge > > would report hpd whenever the type-c logic figures out the cable has > > been connected and hpd is asserted. The actual DisplayPort hardware > > that's encoding data would then find the bridge through the graph > > binding connected to the output node. > > > > I'm not sure how MST is handled though. In that scenario maybe there's > > more than one panel? > > Yeah, given that MST over DP over Type-C is very much possible my > first instinct is that this drm_bridge + bridge-to-panel idea is > not going to work very well. Also the output could be anything, it > could be a projector, or the DP input of a video-grabber, or ... > so modelling this as a panel feels wrong. Agreed. I suppose making a panel in the connector is the wrong direction, but making a bridge there should be OK.
diff --git a/drivers/usb/typec/altmodes/Kconfig b/drivers/usb/typec/altmodes/Kconfig index 60d375e9c3c7..1a6b5e872b0d 100644 --- a/drivers/usb/typec/altmodes/Kconfig +++ b/drivers/usb/typec/altmodes/Kconfig @@ -4,6 +4,7 @@ menu "USB Type-C Alternate Mode drivers" config TYPEC_DP_ALTMODE tristate "DisplayPort Alternate Mode driver" + depends on DRM help DisplayPort USB Type-C Alternate Mode allows DisplayPort displays and adapters to be attached to the USB Type-C diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c index aa669b9cf70e..c1d8c23baa39 100644 --- a/drivers/usb/typec/altmodes/displayport.c +++ b/drivers/usb/typec/altmodes/displayport.c @@ -11,8 +11,10 @@ #include <linux/delay.h> #include <linux/mutex.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/usb/pd_vdo.h> #include <linux/usb/typec_dp.h> +#include <drm/drm_connector.h> #include "displayport.h" #define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd) \ @@ -57,11 +59,13 @@ struct dp_altmode { struct typec_displayport_data data; enum dp_state state; + bool hpd; struct mutex lock; /* device lock */ struct work_struct work; struct typec_altmode *alt; const struct typec_altmode *port; + struct fwnode_handle *connector_fwnode; }; static int dp_altmode_notify(struct dp_altmode *dp) @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con) static int dp_altmode_status_update(struct dp_altmode *dp) { bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf); + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE); u8 con = DP_STATUS_CONNECTION(dp->data.status); int ret = 0; @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode *dp) ret = dp_altmode_configure(dp, con); if (!ret) dp->state = DP_STATE_CONFIGURE; + } else { + if (dp->hpd != hpd) { + drm_connector_oob_hotplug_event(dp->connector_fwnode); + dp->hpd = hpd; + } } return ret; @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = { int dp_altmode_probe(struct typec_altmode *alt) { const struct typec_altmode *port = typec_altmode_get_partner(alt); + struct fwnode_handle *fwnode; struct dp_altmode *dp; int ret; @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt) alt->desc = "DisplayPort"; alt->ops = &dp_altmode_ops; + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */ + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0); + if (IS_ERR(dp->connector_fwnode)) + dp->connector_fwnode = NULL; + typec_altmode_set_drvdata(alt, dp); dp->state = DP_STATE_ENTER; @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt) sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group); cancel_work_sync(&dp->work); + + if (dp->connector_fwnode) { + if (dp->hpd) + drm_connector_oob_hotplug_event(dp->connector_fwnode); + + fwnode_handle_put(dp->connector_fwnode); + } } EXPORT_SYMBOL_GPL(dp_altmode_remove);