| Message ID | 20210503154647.142551-5-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm + usb-type-c: Add support for out-of-band hotplug notification (v2) | expand |
> +/** > + * 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 fwnode_handle *connector_fwnode, > + struct drm_connector_oob_hotplug_event_data *data) > +{ > + struct drm_connector *connector; > + > + connector = drm_connector_find_by_fwnode(connector_fwnode); > + if (IS_ERR(connector)) > + return; > + > + if (connector->funcs->oob_hotplug_event) > + connector->funcs->oob_hotplug_event(connector, data); > + > + drm_connector_put(connector); > +} > +EXPORT_SYMBOL(drm_connector_oob_hotplug_event); So it does looks like the "data" parameter is not needed at all: void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode) { struct drm_connector *connector; connector = drm_connector_find_by_fwnode(connector_fwnode); if (IS_ERR(connector)) return; if (connector->funcs->oob_hotplug_event) connector->funcs->oob_hotplug_event(connector); drm_connector_put(connector); } thanks, -- heikki
Hi, On 5/4/21 5:10 PM, Heikki Krogerus wrote: >> +/** >> + * 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 fwnode_handle *connector_fwnode, >> + struct drm_connector_oob_hotplug_event_data *data) >> +{ >> + struct drm_connector *connector; >> + >> + connector = drm_connector_find_by_fwnode(connector_fwnode); >> + if (IS_ERR(connector)) >> + return; >> + >> + if (connector->funcs->oob_hotplug_event) >> + connector->funcs->oob_hotplug_event(connector, data); >> + >> + drm_connector_put(connector); >> +} >> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event); > > So it does looks like the "data" parameter is not needed at all: Well Imre did indicate that having the number of lanes is useful, so for the next version I'll drop the orientation but I plan to keep the number of lanes if that is ok with you. Not having passing along this info was one of the reasons why my previous attempt at this was nacked, so dropping it all together feels wrong. Regards, Hans
On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote: > Hi, > > On 5/4/21 5:10 PM, Heikki Krogerus wrote: > >> +/** > >> + * 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 fwnode_handle *connector_fwnode, > >> + struct drm_connector_oob_hotplug_event_data *data) > >> +{ > >> + struct drm_connector *connector; > >> + > >> + connector = drm_connector_find_by_fwnode(connector_fwnode); > >> + if (IS_ERR(connector)) > >> + return; > >> + > >> + if (connector->funcs->oob_hotplug_event) > >> + connector->funcs->oob_hotplug_event(connector, data); > >> + > >> + drm_connector_put(connector); > >> +} > >> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event); > > > > So it does looks like the "data" parameter is not needed at all: > > Well Imre did indicate that having the number of lanes is useful, so > for the next version I'll drop the orientation but I plan to keep > the number of lanes if that is ok with you. > > Not having passing along this info was one of the reasons why my > previous attempt at this was nacked, so dropping it all together > feels wrong. If you need to pass any information to the function, then you need to pass all the information that we have. Don't start with abstraction. First create a dedicated API, and then, only if we really have another user for it, we can add an abstraction layer that both users can use. All cases are going to be different. We don't know how the abstraction / generalisation should look like before we have at least two real users (ideally more than two, actually). Right now we can not even say for sure that generalising the API is even possible. I would not make a huge deal out of this unless I wasn't myself being told by guys like Greg KH in the past to drop my attempts to "generalize" things from the beginning when I only had a single user. By doing so you'll not only force all ends, the source of the data (the typec drivers in this case) as well as the consumer (i915), to be always changed together, it will also confuse things. We are not always going to be able to tell the lane count for example like we can with USB Type-C, so i915 can't really rely on that information. Right now we also don't know what exact details i915 (or what ever GPU driver) needs. We can only say for sure that some details are going to be needed. Trying to guess and cherry-pick the details now does not makes sense because of that reason too. So just make this API USB Type-C DP Alt Mode specific API first, and supply it everything we have. thanks, -- heikki
Hi, On 5/5/21 11:50 AM, Heikki Krogerus wrote: > On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote: >> Hi, >> >> On 5/4/21 5:10 PM, Heikki Krogerus wrote: >>>> +/** >>>> + * 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 fwnode_handle *connector_fwnode, >>>> + struct drm_connector_oob_hotplug_event_data *data) >>>> +{ >>>> + struct drm_connector *connector; >>>> + >>>> + connector = drm_connector_find_by_fwnode(connector_fwnode); >>>> + if (IS_ERR(connector)) >>>> + return; >>>> + >>>> + if (connector->funcs->oob_hotplug_event) >>>> + connector->funcs->oob_hotplug_event(connector, data); >>>> + >>>> + drm_connector_put(connector); >>>> +} >>>> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event); >>> >>> So it does looks like the "data" parameter is not needed at all: >> >> Well Imre did indicate that having the number of lanes is useful, so >> for the next version I'll drop the orientation but I plan to keep >> the number of lanes if that is ok with you. >> >> Not having passing along this info was one of the reasons why my >> previous attempt at this was nacked, so dropping it all together >> feels wrong. > > If you need to pass any information to the function, then you need to > pass all the information that we have. Don't start with abstraction. > First create a dedicated API, and then, only if we really have another > user for it, we can add an abstraction layer that both users can use. > All cases are going to be different. We don't know how the abstraction > / generalisation should look like before we have at least two real > users (ideally more than two, actually). Right now we can not even say > for sure that generalising the API is even possible. > > I would not make a huge deal out of this unless I wasn't myself being > told by guys like Greg KH in the past to drop my attempts to > "generalize" things from the beginning when I only had a single user. > By doing so you'll not only force all ends, the source of the data > (the typec drivers in this case) as well as the consumer (i915), to be > always changed together, it will also confuse things. We are not > always going to be able to tell the lane count for example like we can > with USB Type-C, so i915 can't really rely on that information. > > Right now we also don't know what exact details i915 (or what ever GPU > driver) needs. We can only say for sure that some details are going to > be needed. Trying to guess and cherry-pick the details now does not > makes sense because of that reason too. > > So just make this API USB Type-C DP Alt Mode specific API first, and > supply it everything we have. Hmm, ok I'll just drop the data argument all together for now (as you already suggested); and then we can see what is best once an actual user for the info shows up. Regards, Hans
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index ef759d6add81..b5e09d751694 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2726,6 +2726,35 @@ struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode) return found; } +/** + * 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 fwnode_handle *connector_fwnode, + struct drm_connector_oob_hotplug_event_data *data) +{ + struct drm_connector *connector; + + connector = drm_connector_find_by_fwnode(connector_fwnode); + if (IS_ERR(connector)) + return; + + if (connector->funcs->oob_hotplug_event) + connector->funcs->oob_hotplug_event(connector, data); + + drm_connector_put(connector); +} +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 ae377354e48e..bb61aeb9ba2d 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); }; /** @@ -1704,6 +1735,8 @@ drm_connector_is_unregistered(struct drm_connector *connector) DRM_CONNECTOR_UNREGISTERED; } +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode, + 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. Changes in v2: - Make drm_connector_oob_hotplug_event() take a fwnode as argument and have it call drm_connector_find_by_fwnode() internally. This allows making drm_connector_find_by_fwnode() a drm-internal function and avoids code outside the drm subsystem potentially holding on the a drm_connector reference for a longer period. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/drm_connector.c | 29 +++++++++++++++++++++++++++++ include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+)