| Message ID | 20210428215257.500088-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm + usb-type-c: Add support for out-of-band hotplug notification | expand |
Hi Hans, On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote: > +/** > + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data > + * > + * Contains data about out-of-band hotplug events, signalled through > + * drm_connector_oob_hotplug_event(). > + */ > +struct drm_connector_oob_hotplug_event_data { > + /** > + * @connected: New connected status for the connector. > + */ > + bool connected; > + /** > + * @dp_lanes: Number of available displayport lanes, 0 if unknown. > + */ > + int dp_lanes; > + /** > + * @orientation: Connector orientation. > + */ > + enum typec_orientation orientation; > +}; I don't think the orientation is relevant. It will always be "normal" from DP PoW after muxing, no? I'm also not sure those deatils are enough in the long run. Based on what I've understood from our graphics team guys, for example knowing if multi-function is preferred may be important in some cases. +Imre. All of that, and more, is already available in the Configuration VDO Status VDO that the we have negotiated with the DP partner. Both those VDOs are part of struct typec_displayport_data. I think we should simply supply that structure to the DRM code instead of picking those details out of it... > /** > * struct drm_tv_connector_state - TV connector related states > * @subconnector: selected subconnector > @@ -1110,6 +1132,15 @@ struct drm_connector_funcs { > */ > void (*atomic_print_state)(struct drm_printer *p, > const struct drm_connector_state *state); > + > + /** > + * @oob_hotplug_event: > + * > + * This will get called when a hotplug-event for a drm-connector > + * has been received from a source outside the display driver / device. > + */ > + void (*oob_hotplug_event)(struct drm_connector *connector, > + struct drm_connector_oob_hotplug_event_data *data); So I would not try to generalise this like that. This callback should be USB Type-C DP altmode specific: void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_displayport_data *data); Or like this if the orientation can really be reversed after muxing: void (*oob_hotplug_event)(struct drm_connector *connector, struct typec_altmode *altmode, struct typec_displayport_data *data); You can now check the orientation separately with typec_altmode_get_orientation() if necessary. thanks,
Hi, On 5/3/21 10:00 AM, Heikki Krogerus wrote: > Hi Hans, > > On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote: >> +/** >> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data >> + * >> + * Contains data about out-of-band hotplug events, signalled through >> + * drm_connector_oob_hotplug_event(). >> + */ >> +struct drm_connector_oob_hotplug_event_data { >> + /** >> + * @connected: New connected status for the connector. >> + */ >> + bool connected; >> + /** >> + * @dp_lanes: Number of available displayport lanes, 0 if unknown. >> + */ >> + int dp_lanes; >> + /** >> + * @orientation: Connector orientation. >> + */ >> + enum typec_orientation orientation; >> +}; > > I don't think the orientation is relevant. It will always be "normal" > from DP PoW after muxing, no? That is what I thought to, but during the discussion of my previous attempt at this one of the i915 devs mentioned that in some cases the muxes manage to swap the lane order when the connector upside-down and at least the Intel GPUs can correct for this on the GPU side, so they asked for this info to be included. > I'm also not sure those deatils are enough in the long run. Based on > what I've understood from our graphics team guys, for example knowing > if multi-function is preferred may be important in some cases. The current data being passed is just intended as a starting point, this is purely a kernel internal API so we can easily add more data to the struct. As I mentioned in the cover-letter the current oob_hotplug handler which the i915 patch adds to the i915 driver does not actually do anything with the data. ATM it is purely there to demonstrate that the ability to pass relevant data is there now (which was an issue with the previous attempt). I believe the current code is fine as a PoC of "pass event data" once GPU drivers actually start doing something with the data we can extend or outright replace it without issues. > +Imre. > > All of that, and more, is already available in the Configuration VDO > Status VDO that the we have negotiated with the DP partner. Both those > VDOs are part of struct typec_displayport_data. I think we should > simply supply that structure to the DRM code instead of picking those > details out of it... I'm not sure I like the idea of passing the raw VDO, but if the DRM folks think that would be useful we can certainly add it. Regards, Hans > >> /** >> * struct drm_tv_connector_state - TV connector related states >> * @subconnector: selected subconnector >> @@ -1110,6 +1132,15 @@ struct drm_connector_funcs { >> */ >> void (*atomic_print_state)(struct drm_printer *p, >> const struct drm_connector_state *state); >> + >> + /** >> + * @oob_hotplug_event: >> + * >> + * This will get called when a hotplug-event for a drm-connector >> + * has been received from a source outside the display driver / device. >> + */ >> + void (*oob_hotplug_event)(struct drm_connector *connector, >> + struct drm_connector_oob_hotplug_event_data *data); > > So I would not try to generalise this like that. This callback should > be USB Type-C DP altmode specific: > > void (*oob_hotplug_event)(struct drm_connector *connector, > struct typec_displayport_data *data); > > Or like this if the orientation can really be reversed after muxing: > > void (*oob_hotplug_event)(struct drm_connector *connector, > struct typec_altmode *altmode, > struct typec_displayport_data *data); > > You can now check the orientation separately with > typec_altmode_get_orientation() if necessary. > > > thanks, >
On Mon, May 03, 2021 at 11:00:20AM +0300, Heikki Krogerus wrote: > Hi Hans, > > On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote: > > +/** > > + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data > > + * > > + * Contains data about out-of-band hotplug events, signalled through > > + * drm_connector_oob_hotplug_event(). > > + */ > > +struct drm_connector_oob_hotplug_event_data { > > + /** > > + * @connected: New connected status for the connector. > > + */ > > + bool connected; > > + /** > > + * @dp_lanes: Number of available displayport lanes, 0 if unknown. > > + */ > > + int dp_lanes; > > + /** > > + * @orientation: Connector orientation. > > + */ > > + enum typec_orientation orientation; > > +}; > > I don't think the orientation is relevant. It will always be "normal" > from DP PoW after muxing, no? > > I'm also not sure those deatils are enough in the long run. Based on > what I've understood from our graphics team guys, for example knowing > if multi-function is preferred may be important in some cases. Combo PHY ports - which is what this patchset is adding the notification for - can only reverse the lane assignment. TypeC PHY ports (on ICL+) have a more C-type aware mux in the SoC (FIA) as well, so in theory we could have a system based on such platforms with an external mux only switching between the USB, DP, USB+DP (MFD) modes, but leaving the plug orientation specific muxing up to the FIA. The graphics driver is not involved in programming the FIA though, it's done by a firmware component, so I don't think this configuration needs to get passed. Yes, the driver needs to know if the PD controller configured the sink in the MFD mode (DP+USB) or in the DP-only mode. For that the number of lanes assigned to DP is enough. > +Imre. > > All of that, and more, is already available in the Configuration VDO > Status VDO that the we have negotiated with the DP partner. Both those > VDOs are part of struct typec_displayport_data. I think we should > simply supply that structure to the DRM code instead of picking those > details out of it... > > > /** > > * struct drm_tv_connector_state - TV connector related states > > * @subconnector: selected subconnector > > @@ -1110,6 +1132,15 @@ struct drm_connector_funcs { > > */ > > void (*atomic_print_state)(struct drm_printer *p, > > const struct drm_connector_state *state); > > + > > + /** > > + * @oob_hotplug_event: > > + * > > + * This will get called when a hotplug-event for a drm-connector > > + * has been received from a source outside the display driver / device. > > + */ > > + void (*oob_hotplug_event)(struct drm_connector *connector, > > + struct drm_connector_oob_hotplug_event_data *data); > > So I would not try to generalise this like that. This callback should > be USB Type-C DP altmode specific: > > void (*oob_hotplug_event)(struct drm_connector *connector, > struct typec_displayport_data *data); > > Or like this if the orientation can really be reversed after muxing: > > void (*oob_hotplug_event)(struct drm_connector *connector, > struct typec_altmode *altmode, > struct typec_displayport_data *data); > > You can now check the orientation separately with > typec_altmode_get_orientation() if necessary. > > > thanks, > > -- > heikki
On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote: > Hi, > > On 5/3/21 10:00 AM, Heikki Krogerus wrote: > > Hi Hans, > > > > On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote: > >> +/** > >> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data > >> + * > >> + * Contains data about out-of-band hotplug events, signalled through > >> + * drm_connector_oob_hotplug_event(). > >> + */ > >> +struct drm_connector_oob_hotplug_event_data { > >> + /** > >> + * @connected: New connected status for the connector. > >> + */ > >> + bool connected; > >> + /** > >> + * @dp_lanes: Number of available displayport lanes, 0 if unknown. > >> + */ > >> + int dp_lanes; > >> + /** > >> + * @orientation: Connector orientation. > >> + */ > >> + enum typec_orientation orientation; > >> +}; > > > > I don't think the orientation is relevant. It will always be "normal" > > from DP PoW after muxing, no? > > That is what I thought to, but during the discussion of my previous attempt > at this one of the i915 devs mentioned that in some cases the muxes manage > to swap the lane order when the connector upside-down and at least the > Intel GPUs can correct for this on the GPU side, so they asked for this > info to be included. > > > I'm also not sure those deatils are enough in the long run. Based on > > what I've understood from our graphics team guys, for example knowing > > if multi-function is preferred may be important in some cases. > > The current data being passed is just intended as a starting point, > this is purely a kernel internal API so we can easily add more > data to the struct. As I mentioned in the cover-letter the current > oob_hotplug handler which the i915 patch adds to the i915 driver does > not actually do anything with the data. ATM it is purely there to > demonstrate that the ability to pass relevant data is there now > (which was an issue with the previous attempt). I believe the current > code is fine as a PoC of "pass event data" once GPU drivers actually > start doing something with the data we can extend or outright replace > it without issues. Ah, if there is nothing using that information yet, then just don't pass it at all for now. As you said, it's kernel internal API, we can change it later if needed. > > All of that, and more, is already available in the Configuration VDO > > Status VDO that the we have negotiated with the DP partner. Both those > > VDOs are part of struct typec_displayport_data. I think we should > > simply supply that structure to the DRM code instead of picking those > > details out of it... > > I'm not sure I like the idea of passing the raw VDO, but if the > DRM folks think that would be useful we can certainly add it. Why are you against passing all the data that we have? What is the benefit in picking only certain details out of an object that has a standard format, and constructing a customised object for those details instead? thanks,
Hi, On 5/4/21 4:52 PM, Heikki Krogerus wrote: > On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote: >> Hi, >> >> On 5/3/21 10:00 AM, Heikki Krogerus wrote: >>> Hi Hans, >>> >>> On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote: >>>> +/** >>>> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data >>>> + * >>>> + * Contains data about out-of-band hotplug events, signalled through >>>> + * drm_connector_oob_hotplug_event(). >>>> + */ >>>> +struct drm_connector_oob_hotplug_event_data { >>>> + /** >>>> + * @connected: New connected status for the connector. >>>> + */ >>>> + bool connected; >>>> + /** >>>> + * @dp_lanes: Number of available displayport lanes, 0 if unknown. >>>> + */ >>>> + int dp_lanes; >>>> + /** >>>> + * @orientation: Connector orientation. >>>> + */ >>>> + enum typec_orientation orientation; >>>> +}; >>> >>> I don't think the orientation is relevant. It will always be "normal" >>> from DP PoW after muxing, no? >> >> That is what I thought to, but during the discussion of my previous attempt >> at this one of the i915 devs mentioned that in some cases the muxes manage >> to swap the lane order when the connector upside-down and at least the >> Intel GPUs can correct for this on the GPU side, so they asked for this >> info to be included. >> >>> I'm also not sure those deatils are enough in the long run. Based on >>> what I've understood from our graphics team guys, for example knowing >>> if multi-function is preferred may be important in some cases. >> >> The current data being passed is just intended as a starting point, >> this is purely a kernel internal API so we can easily add more >> data to the struct. As I mentioned in the cover-letter the current >> oob_hotplug handler which the i915 patch adds to the i915 driver does >> not actually do anything with the data. ATM it is purely there to >> demonstrate that the ability to pass relevant data is there now >> (which was an issue with the previous attempt). I believe the current >> code is fine as a PoC of "pass event data" once GPU drivers actually >> start doing something with the data we can extend or outright replace >> it without issues. > > Ah, if there is nothing using that information yet, then just don't > pass it at all for now. As you said, it's kernel internal API, we can > change it later if needed. > >>> All of that, and more, is already available in the Configuration VDO >>> Status VDO that the we have negotiated with the DP partner. Both those >>> VDOs are part of struct typec_displayport_data. I think we should >>> simply supply that structure to the DRM code instead of picking those >>> details out of it... >> >> I'm not sure I like the idea of passing the raw VDO, but if the >> DRM folks think that would be useful we can certainly add it. > > Why are you against passing all the data that we have? What is the > benefit in picking only certain details out of an object that has a > standard format, and constructing a customised object for those > details instead? The VDO is Type-C specific and the drm_connector_oob_hotplug_event() is intended to be a generic API. There are other OOB event sources, e.g. the drivers/apci/acpi_video.c code receives hotplug events for connectors on powered-down GPUs on dual/hybrid GPU laptops. ATM the GPU drivers register an ACPI notifier to catch these; and there are no immediate plans to change this, but this does illustrate how OOB hotplug notification is not just a Type-C thing, where as the VDO and its format very much are Type-C things. Regards, Hans
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 87c68563e6c3..7d3adc41c11a 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2676,6 +2676,26 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, return ret; } +/** + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to connector + * @connector: connector to report the event on + * @data: data related to the event + * + * On some hardware a hotplug event notification may come from outside the display + * driver / device. An example of this is some USB Type-C setups where the hardware + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD + * status bit to the GPU's DP HPD pin. + * + * This function can be used to report these out-of-band events after obtaining + * a drm_connector reference through calling drm_connector_find_by_fwnode(). + */ +void drm_connector_oob_hotplug_event(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data) +{ + if (connector->funcs->oob_hotplug_event) + connector->funcs->oob_hotplug_event(connector, data); +} +EXPORT_SYMBOL(drm_connector_oob_hotplug_event); /** * DOC: Tile group diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 1e5d154d1f4a..6a10f8890809 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -27,6 +27,7 @@ #include <linux/llist.h> #include <linux/ctype.h> #include <linux/hdmi.h> +#include <linux/usb/typec.h> /* For enum typec_orientation */ #include <drm/drm_mode_object.h> #include <drm/drm_util.h> @@ -649,6 +650,27 @@ struct drm_connector_tv_margins { unsigned int top; }; +/** + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data + * + * Contains data about out-of-band hotplug events, signalled through + * drm_connector_oob_hotplug_event(). + */ +struct drm_connector_oob_hotplug_event_data { + /** + * @connected: New connected status for the connector. + */ + bool connected; + /** + * @dp_lanes: Number of available displayport lanes, 0 if unknown. + */ + int dp_lanes; + /** + * @orientation: Connector orientation. + */ + enum typec_orientation orientation; +}; + /** * struct drm_tv_connector_state - TV connector related states * @subconnector: selected subconnector @@ -1110,6 +1132,15 @@ struct drm_connector_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_connector_state *state); + + /** + * @oob_hotplug_event: + * + * This will get called when a hotplug-event for a drm-connector + * has been received from a source outside the display driver / device. + */ + void (*oob_hotplug_event)(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data); }; /** @@ -1697,6 +1728,8 @@ drm_connector_is_unregistered(struct drm_connector *connector) } struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode); +void drm_connector_oob_hotplug_event(struct drm_connector *connector, + struct drm_connector_oob_hotplug_event_data *data); const char *drm_get_connector_type_name(unsigned int connector_type); const char *drm_get_connector_status_name(enum drm_connector_status status); const char *drm_get_subpixel_order_name(enum subpixel_order order);
Add a new drm_connector_oob_hotplug_event() function and oob_hotplug_event drm_connector_funcs member. On some hardware a hotplug event notification may come from outside the display driver / device. An example of this is some USB Type-C setups where the hardware muxes the DisplayPort data and aux-lines but does not pass the altmode HPD status bit to the GPU's DP HPD pin. In cases like this the new drm_connector_oob_hotplug_event() function can be used to report these out-of-band events after obtaining a drm_connector reference through calling drm_connector_find_by_fwnode(). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/drm_connector.c | 20 ++++++++++++++++++++ include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+)