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:

quoted

+/**
+ * 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...

quoted

 /**
  * 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