diff mbox series

[4/9] drm/connector: Add support for out-of-band hotplug notification (v2)

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

Commit Message

Hans de Goede May 3, 2021, 3:46 p.m. UTC
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(+)

Comments

Heikki Krogerus May 4, 2021, 3:10 p.m. UTC | #1
> +/**

> + * 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
Hans de Goede May 4, 2021, 3:35 p.m. UTC | #2
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
Heikki Krogerus May 5, 2021, 9:50 a.m. UTC | #3
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
Hans de Goede May 5, 2021, 10:42 a.m. UTC | #4
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 mbox series

Patch

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);