[PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Hans de Goede]

Hi All,

On some Cherry Trail devices, DisplayPort over Type-C is supported through
a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed
datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this
case does the PD/alt-mode negotiation itself, rather then everything being
handled in firmware.

So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch
to DP mode and sets the mux accordingly. In this setup the HPD pin is not
connected, so the i915 driver needs to respond to a software event and scan
the DP port for changes manually.

Thanks to Heikki's great work on the DisplayPort altmode support in the
typec subsys, we now correctly tell the dongle to switch to DP altmode
and we correctly set the mux and orientation switches to connect the
DP lines to the Type-C connector.

This just leaves sending an out-of-band hotplug event from the Type-C
subsystem to the i915 driver and then we've fully working DP over Type-C
on these devices.

This series implements this. The first patch adds a generic mechanism
for oob hotplug events to be send to the drm subsys, the second patch
adds support for this mechanism to the i915 driver and the third patch
makes the typec displayport_altmode driver send these events.

The commit message of the first patch explains why I've chosen to things
the way these patches do them.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm: Add support for out-of-band hotplug notification

[Hans de Goede]

On some hardware a hotplug event notification may come from outside the
display driver / device.

One example of this is laptops with hybrid graphics where one or more
outputs are connected to the discrete GPU only. In this case if the
discrete GPU is fully powered down to save power, a hotplug to one of
these outputs will not be noticed through normal means. These laptops
have another mechanism to detect the hotplug which will typically raise
an event on the ACPI video bus.

Another 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 GPUs DP HPD pin.

This commit adds a loose coupling to the drm subsys allowing event-sources
to notify drm-drivers of such events without needing a reference to a
drm-device or a drm-connector.

This loose coupling is implemented through a blocking notifier. drm-drivers
interested in oob hotplug events can register themselves to receive
notifations and event-sources call drm_kms_call_oob_hotplug_notifier_chain
to let any listeners know about events.

An earlier attempt has been done to implement this functionality with a
tight coupling, where the event-source would somehow figure out the right
drm-connector to deliver the event to and then send it directly to that
drm-connector. Such a tight coupling approach has several issues with
lifetime management of the coupled objects as well as introducing several
probe-ordering issues. Therefor the tight coupling approach has been
abandoned.

Note for now drm_kms_call_oob_hotplug_notifier_chain's event parameter can
only have 1 value: DRM_OOB_HOTPLUG_TYPE_C_DP.  The ACPI videobus hotplug
event case is currently already supported in the nouveau driver by
registering a generic acpi event notifier.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/gpu/drm-kms-helpers.rst |  1 +
 drivers/gpu/drm/drm_probe_helper.c    | 67 +++++++++++++++++++++++++++
 include/drm/drm_probe_helper.h        | 12 +++++
 3 files changed, 80 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index b422eb8edf16..f144d68f8e7a 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -249,6 +249,7 @@ Output Probing Helper Functions Reference
 
 .. kernel-doc:: drivers/gpu/drm/drm_probe_helper.c
    :doc: output probing helper overview
+   :doc: out-of-band hotplug event helper overview
 
 .. kernel-doc:: drivers/gpu/drm/drm_probe_helper.c
    :export:
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6fd08e04b323..4f0b421514ef 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -31,6 +31,7 @@
 
 #include <linux/export.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_client.h>
@@ -792,3 +793,69 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	return changed;
 }
 EXPORT_SYMBOL(drm_helper_hpd_irq_event);
+
+/**
+ * DOC: out-of-band hotplug event helper overview
+ *
+ * On some hardware a hotplug event notification may come from outside
+ * the display driver / device.
+ *
+ * One example of this is laptops with hybrid graphics where one or more
+ * outputs are connected to the discrete GPU only. In this case if the discrete
+ * GPU is fully powered down to save power, a hotplug to one of these outputs
+ * will not be noticed through normal means. These laptops have another
+ * mechanism to detect the hotplug which will typically raise an event on the
+ * ACPI video bus.
+ *
+ * Another 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 GPUs DP HPD pin.
+ *
+ * The oob hotplug helper functions allow a drm display driver to listen
+ * for such oob events and allow other subsystems to notify listeners of
+ * these events.
+ */
+
+static BLOCKING_NOTIFIER_HEAD(drm_kms_oob_hotplug_notifier_head);
+
+/**
+ * drm_kms_register_oob_hotplug_notifier - register an oob hotplug notifier
+ * @nb: notifier_block to register
+ *
+ * Drivers can use this helper function to register a notifier for
+ * out-of-band hotplug events.
+ */
+int drm_kms_register_oob_hotplug_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(
+				&drm_kms_oob_hotplug_notifier_head, nb);
+}
+EXPORT_SYMBOL(drm_kms_register_oob_hotplug_notifier);
+
+/**
+ * drm_kms_unregister_oob_hotplug_notifier - unregister an oob hotplug notifier
+ * @nb: notifier_block to register
+ *
+ * Drivers can use this helper function to unregister a notifier for
+ * out-of-band hotplug events.
+ */
+int drm_kms_unregister_oob_hotplug_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(
+				&drm_kms_oob_hotplug_notifier_head, nb);
+}
+EXPORT_SYMBOL(drm_kms_unregister_oob_hotplug_notifier);
+
+/**
+ * drm_kms_call_oob_hotplug_notifier_chain - notify about an oob hotplug event
+ * @event: enum drm_kms_oob_hotplug_event value describing the event
+ *
+ * Out-of-band hotplug event sources can call this helper function to notify
+ * kms-drivers about an oob hotplug event.
+ */
+int drm_kms_call_oob_hotplug_notifier_chain(unsigned long event)
+{
+	return blocking_notifier_call_chain(
+			&drm_kms_oob_hotplug_notifier_head, event, NULL);
+}
+EXPORT_SYMBOL(drm_kms_call_oob_hotplug_notifier_chain);
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 8d3ed2834d34..68cfce47d35f 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -24,4 +24,16 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
+/**
+ * enum drm_kms_oob_hotplug_event - out-of-band hotplug events
+ * @DRM_OOB_HOTPLUG_TYPE_C_DP: DisplayPort over Type-C hotplug event
+ */
+enum drm_kms_oob_hotplug_event {
+	DRM_OOB_HOTPLUG_TYPE_C_DP = 0,
+};
+
+int drm_kms_register_oob_hotplug_notifier(struct notifier_block *nb);
+int drm_kms_unregister_oob_hotplug_notifier(struct notifier_block *nb);
+int drm_kms_call_oob_hotplug_notifier_chain(unsigned long event);
+
 #endif
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] i915: Add support for out-of-bound hotplug events

[Hans de Goede]

On some Cherry Trail devices, DisplayPort over Type-C is supported through
a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed
datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this
case does the PD/alt-mode negotiation itself, rather then everything being
handled in firmware.

So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch
to DP mode and sets the mux accordingly. In this setup the HPD pin is not
connected, so the i915 driver needs to respond to a software event and scan
the DP port for changes manually.

This commit adds support for this. Together with the recent addition of
DP alt-mode support to the Type-C subsystem this makes DP over Type-C
work on these devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_irq.c      |  2 ++
 drivers/gpu/drm/i915/intel_hotplug.c | 38 ++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b1c31967194b..5d8c585ddbf7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -153,6 +153,7 @@ enum hpd_pin {
 
 struct i915_hotplug {
 	struct work_struct hotplug_work;
+	struct notifier_block oob_notifier;
 
 	struct {
 		unsigned long last_jiffies;
@@ -2632,6 +2633,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 			   u32 pin_mask, u32 long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
 void intel_hpd_init_work(struct drm_i915_private *dev_priv);
+void intel_hpd_fini_work(struct drm_i915_private *dev_priv);
 void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
 enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
 				   enum port port);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8211045a981b..14f3323b721b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4965,6 +4965,8 @@ void intel_irq_fini(struct drm_i915_private *i915)
 
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		kfree(i915->l3_parity.remap_info[i]);
+
+	intel_hpd_fini_work(i915);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index e24174d08fed..221878fa26af 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -518,6 +518,36 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 		schedule_work(&dev_priv->hotplug.hotplug_work);
 }
 
+static int intel_hpd_oob_notifier(struct notifier_block *nb,
+				  unsigned long event, void *data)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(nb, struct drm_i915_private, hotplug.oob_notifier);
+	struct intel_encoder *encoder;
+	u32 bits = 0;
+
+	/* We only support DP over Type-C notifications */
+	if (event != DRM_OOB_HOTPLUG_TYPE_C_DP)
+		return NOTIFY_DONE;
+
+	/* Schedule a hotplug check for each DP encoder, except for EDP ones */
+	for_each_intel_dp(&dev_priv->drm, encoder) {
+		if (encoder->type == INTEL_OUTPUT_EDP)
+			continue;
+
+		bits |= BIT(encoder->hpd_pin);
+	}
+
+	if (bits) {
+		spin_lock_irq(&dev_priv->irq_lock);
+		dev_priv->hotplug.event_bits |= bits;
+		spin_unlock_irq(&dev_priv->irq_lock);
+		schedule_work(&dev_priv->hotplug.hotplug_work);
+	}
+
+	return NOTIFY_DONE;
+}
+
 /**
  * intel_hpd_init - initializes and enables hpd support
  * @dev_priv: i915 device instance
@@ -640,6 +670,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 	INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
 	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
 			  intel_hpd_irq_storm_reenable_work);
+	dev_priv->hotplug.oob_notifier.notifier_call = intel_hpd_oob_notifier;
+	drm_kms_register_oob_hotplug_notifier(&dev_priv->hotplug.oob_notifier);
+}
+
+void intel_hpd_fini_work(struct drm_i915_private *dev_priv)
+{
+	drm_kms_unregister_oob_hotplug_notifier(
+					&dev_priv->hotplug.oob_notifier);
 }
 
 void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Hans de Goede]

Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load
drm/kms drivers know about DisplayPort over Type-C hotplug events.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/altmodes/displayport.c | 34 ++++++++++++++++--------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 35161594e368..87760ea252d6 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/usb/pd_vdo.h>
 #include <linux/usb/typec_dp.h>
+#include <drm/drm_probe_helper.h>
 
 #define DP_HEADER(cmd)			(VDO(USB_TYPEC_DP_SID, 1, cmd) | \
 					 VDO_OPOS(USB_TYPEC_DP_MODE))
@@ -67,12 +68,23 @@ struct dp_altmode {
 	const struct typec_altmode *port;
 };
 
-static int dp_altmode_notify(struct dp_altmode *dp)
+static int dp_altmode_notify(struct dp_altmode *dp, unsigned long conf)
+{
+	int ret;
+
+	ret = typec_altmode_notify(dp->alt, conf, &dp->data);
+	if (ret)
+		return ret;
+
+	drm_kms_call_oob_hotplug_notifier_chain(DRM_OOB_HOTPLUG_TYPE_C_DP);
+	return 0;
+}
+
+static int dp_altmode_notify_dp(struct dp_altmode *dp)
 {
 	u8 state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
 
-	return typec_altmode_notify(dp->alt, TYPEC_MODAL_STATE(state),
-				   &dp->data);
+	return dp_altmode_notify(dp, TYPEC_MODAL_STATE(state));
 }
 
 static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
@@ -145,10 +157,9 @@ static int dp_altmode_configured(struct dp_altmode *dp)
 	sysfs_notify(&dp->alt->dev.kobj, "displayport", "configuration");
 
 	if (!dp->data.conf)
-		return typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
-					    &dp->data);
+		return dp_altmode_notify(dp, TYPEC_STATE_USB);
 
-	ret = dp_altmode_notify(dp);
+	ret = dp_altmode_notify_dp(dp);
 	if (ret)
 		return ret;
 
@@ -162,7 +173,7 @@ static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
 	u32 header = DP_HEADER(DP_CMD_CONFIGURE);
 	int ret;
 
-	ret = typec_altmode_notify(dp->alt, TYPEC_STATE_SAFE, &dp->data);
+	ret = dp_altmode_notify(dp, TYPEC_STATE_SAFE);
 	if (ret) {
 		dev_err(&dp->alt->dev,
 			"unable to put to connector to safe mode\n");
@@ -172,10 +183,9 @@ static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
 	ret = typec_altmode_vdm(dp->alt, header, &conf, 2);
 	if (ret) {
 		if (DP_CONF_GET_PIN_ASSIGN(dp->data.conf))
-			dp_altmode_notify(dp);
+			dp_altmode_notify_dp(dp);
 		else
-			typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
-					     &dp->data);
+			dp_altmode_notify(dp, TYPEC_STATE_USB);
 	}
 
 	return ret;
@@ -241,7 +251,7 @@ static void dp_altmode_attention(struct typec_altmode *alt, const u32 vdo)
 	if (dp_altmode_status_update(dp))
 		dev_warn(&alt->dev, "%s: status update failed\n", __func__);
 
-	if (dp_altmode_notify(dp))
+	if (dp_altmode_notify_dp(dp))
 		dev_err(&alt->dev, "%s: notification failed\n", __func__);
 
 	if (old_state == DP_STATE_IDLE && dp->state != DP_STATE_IDLE)
@@ -556,6 +566,8 @@ static void dp_altmode_remove(struct typec_altmode *alt)
 
 	sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group);
 	cancel_work_sync(&dp->work);
+
+	drm_kms_call_oob_hotplug_notifier_chain(DRM_OOB_HOTPLUG_TYPE_C_DP);
 }
 
 static const struct typec_device_id dp_typec_id[] = {
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Greg Kroah-Hartman]

On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote:
> Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load
> drm/kms drivers know about DisplayPort over Type-C hotplug events.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 34 ++++++++++++++++--------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 35161594e368..87760ea252d6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/usb/pd_vdo.h>
>  #include <linux/usb/typec_dp.h>
> +#include <drm/drm_probe_helper.h>
>  
>  #define DP_HEADER(cmd)			(VDO(USB_TYPEC_DP_SID, 1, cmd) | \
>  					 VDO_OPOS(USB_TYPEC_DP_MODE))
> @@ -67,12 +68,23 @@ struct dp_altmode {
>  	const struct typec_altmode *port;
>  };
>  
> -static int dp_altmode_notify(struct dp_altmode *dp)
> +static int dp_altmode_notify(struct dp_altmode *dp, unsigned long conf)
> +{
> +	int ret;
> +
> +	ret = typec_altmode_notify(dp->alt, conf, &dp->data);
> +	if (ret)
> +		return ret;
> +
> +	drm_kms_call_oob_hotplug_notifier_chain(DRM_OOB_HOTPLUG_TYPE_C_DP);

Is this causing a build/run-time dependancy of the USB code on DRM now?
What about typec systems without DRM, is that a thing?

I have no objection to this if the DRM people like this type of api
(personally I hate notifier chains), just curious about the dependancy
issues involved.

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Hans de Goede]

Hi,

On 25-02-19 15:06, Greg Kroah-Hartman wrote:
> On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote:
>> Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load

s/load/let/ fixed in my tree.

>> drm/kms drivers know about DisplayPort over Type-C hotplug events.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/altmodes/displayport.c | 34 ++++++++++++++++--------
>>   1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
>> index 35161594e368..87760ea252d6 100644
>> --- a/drivers/usb/typec/altmodes/displayport.c
>> +++ b/drivers/usb/typec/altmodes/displayport.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/module.h>
>>   #include <linux/usb/pd_vdo.h>
>>   #include <linux/usb/typec_dp.h>
>> +#include <drm/drm_probe_helper.h>
>>   
>>   #define DP_HEADER(cmd)			(VDO(USB_TYPEC_DP_SID, 1, cmd) | \
>>   					 VDO_OPOS(USB_TYPEC_DP_MODE))
>> @@ -67,12 +68,23 @@ struct dp_altmode {
>>   	const struct typec_altmode *port;
>>   };
>>   
>> -static int dp_altmode_notify(struct dp_altmode *dp)
>> +static int dp_altmode_notify(struct dp_altmode *dp, unsigned long conf)
>> +{
>> +	int ret;
>> +
>> +	ret = typec_altmode_notify(dp->alt, conf, &dp->data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_kms_call_oob_hotplug_notifier_chain(DRM_OOB_HOTPLUG_TYPE_C_DP);
> 
> Is this causing a build/run-time dependancy of the USB code on DRM now?
> What about typec systems without DRM, is that a thing?

Good point, yes this adds a build/run-time dependancy on the drm-core
to the Type-C DisplayPort altmode driver (typec_displayport.ko). But
only to that driver, which can be enabled / disabled separately through
CONFIG_TYPEC_DP_ALTMODE and that specific Type-C altmode makes little
sense without having drm/kms support.

Your remark does make me realize that I have forgotten to add a Kconfig
dependency for this to the TYPEC_DP_ALTMODE Kconfig symbol, I will fix
this for v2.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20190225]
[cannot apply to v5.0-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Propagate-DP-over-Type-C-hotplug-events-from-Type-C-subsys-to-drm-drivers/20190226-005334
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-m1-201908 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-5) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "drm_kms_call_oob_hotplug_notifier_chain" [drivers/usb/typec/altmodes/typec_displayport.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27926 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20190226]
[cannot apply to v5.0-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Propagate-DP-over-Type-C-hotplug-events-from-Type-C-subsys-to-drm-drivers/20190226-005334
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s1-02261802 (attached as .config)
compiler: gcc-6 (Debian 6.5.0-2) 6.5.0 20181026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   ld: drivers/usb/typec/altmodes/displayport.o: in function `dp_altmode_remove':
>> drivers/usb/typec/altmodes/displayport.c:570: undefined reference to `drm_kms_call_oob_hotplug_notifier_chain'
   ld: drivers/usb/typec/altmodes/displayport.o: in function `dp_altmode_notify':
   drivers/usb/typec/altmodes/displayport.c:79: undefined reference to `drm_kms_call_oob_hotplug_notifier_chain'

vim +570 drivers/usb/typec/altmodes/displayport.c

   562	
   563	static void dp_altmode_remove(struct typec_altmode *alt)
   564	{
   565		struct dp_altmode *dp = typec_altmode_get_drvdata(alt);
   566	
   567		sysfs_remove_group(&alt->dev.kobj, &dp_altmode_group);
   568		cancel_work_sync(&dp->work);
   569	
 > 570		drm_kms_call_oob_hotplug_notifier_chain(DRM_OOB_HOTPLUG_TYPE_C_DP);
   571	}
   572	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32485 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Heikki Krogerus]

On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote:
> Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load
> drm/kms drivers know about DisplayPort over Type-C hotplug events.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I'm OK with this. I'll wait for the v2 and see if I can test these.

thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events

[Hans de Goede]

Hi,

On 27-02-19 10:44, Heikki Krogerus wrote:
> On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote:
>> Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load
>> drm/kms drivers know about DisplayPort over Type-C hotplug events.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I'm OK with this. I'll wait for the v2 and see if I can test these.

The only change I've queued for v2 is adding a "depends on DRM" to
the TYPEC_DP_ALTMODE Kconfig.

And given the discussion about passing lane-info, it might be a while
before we get a v2, so if you want to give this a test run, it is
probably best to just test v1 for now (if you've time).

Note to test this on the GPD win (which you have AFAIK) you will also
need the fusb302 + pi3usb30532 patches I've send out recently, as well as:

https://github.com/jwrdegoede/linux-sunxi/commit/945c6fe0a18957357b42e04ed34bf33667003030

I've one Type-C to VGA dongle (without any other functions) where the Type-C
mode negotiation fails. This one does work on a XPS 15 so I need to borrow
some hardware from a friend so that I can capture the USB-PD signals and
see what the Alpine Ridge controller does different compared to the in kernel
stack and fix this. My other 4 dongles work fine, including this "standard" model:
http://media.redgamingtech.com/rgt-website/2015/03/Apple-HDMI-Usb-Type-C-dongle.jpg

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus]

Hi Hans,

On Mon, Feb 25, 2019 at 02:20:34PM +0100, Hans de Goede wrote:
> Hi All,
> 
> On some Cherry Trail devices, DisplayPort over Type-C is supported through
> a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed
> datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this
> case does the PD/alt-mode negotiation itself, rather then everything being
> handled in firmware.
> 
> So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch
> to DP mode and sets the mux accordingly. In this setup the HPD pin is not
> connected, so the i915 driver needs to respond to a software event and scan
> the DP port for changes manually.
> 
> Thanks to Heikki's great work on the DisplayPort altmode support in the
> typec subsys, we now correctly tell the dongle to switch to DP altmode
> and we correctly set the mux and orientation switches to connect the
> DP lines to the Type-C connector.
> 
> This just leaves sending an out-of-band hotplug event from the Type-C
> subsystem to the i915 driver and then we've fully working DP over Type-C
> on these devices.
> 
> This series implements this. The first patch adds a generic mechanism
> for oob hotplug events to be send to the drm subsys, the second patch
> adds support for this mechanism to the i915 driver and the third patch
> makes the typec displayport_altmode driver send these events.
> 
> The commit message of the first patch explains why I've chosen to things
> the way these patches do them.

One thing that this series does not consider is the DP lane count
problem. The GPU drivers (i915 in this case) does not know is four,
two or one DP lanes in use.

I guess that is not a critical issue since there is a workaround (I
think) where the driver basically does trial and error, but ideally we
should be able to tell i915 also the pin assignment that was
negotiated with the partner device so it knows the DP lane count.

My original idea was that we pass that struct typec_displayport_data
as payload in the event. From the Configuration VDO the GPU drivers
can then see the negotiated DP pin assignment and determine the lane
count.


thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Jani Nikula]

On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> One thing that this series does not consider is the DP lane count
> problem. The GPU drivers (i915 in this case) does not know is four,
> two or one DP lanes in use.

Also, orientation.

> I guess that is not a critical issue since there is a workaround (I
> think) where the driver basically does trial and error, but ideally we
> should be able to tell i915 also the pin assignment that was
> negotiated with the partner device so it knows the DP lane count.

Yeah, if the information is there, we'd like to know. With the
orientation, there's a worst case of sixth attempt of finding out
there's just one lane in a certain orientation. Couple that with link
rate selection (did it not work because too high link rate or because
the lanes are just not there?) we get pretty confused about what we
should try.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus]

On Wed, Feb 27, 2019 at 01:16:27PM +0200, Jani Nikula wrote:
> On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > One thing that this series does not consider is the DP lane count
> > problem. The GPU drivers (i915 in this case) does not know is four,
> > two or one DP lanes in use.
> 
> Also, orientation.
> 
> > I guess that is not a critical issue since there is a workaround (I
> > think) where the driver basically does trial and error, but ideally we
> > should be able to tell i915 also the pin assignment that was
> > negotiated with the partner device so it knows the DP lane count.
> 
> Yeah, if the information is there, we'd like to know. With the
> orientation, there's a worst case of sixth attempt of finding out
> there's just one lane in a certain orientation. Couple that with link
> rate selection (did it not work because too high link rate or because
> the lanes are just not there?) we get pretty confused about what we
> should try.

The orientation is also considered in the alt mode API. We have a
function for that typec_altmode_get_orientation(), but it of course
requires that the caller has a handle to the alt mode object. So
basically we would again need tight coupling between the DP connector
and USB Type-C connector.

Hans, I'm not so sure we should, or can, rule out the "tight coupling"
option after all.


thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Hans de Goede]

Hi,

On 27-02-19 12:16, Jani Nikula wrote:
> On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>> One thing that this series does not consider is the DP lane count
>> problem. The GPU drivers (i915 in this case) does not know is four,
>> two or one DP lanes in use.
> 
> Also, orientation.

The orientation should simply be correct with Type-C over DP. The mux /
orientation-switch used will take care of (physically) swapping the
lanes if the connector is inserted upside-down.

>> I guess that is not a critical issue since there is a workaround (I
>> think) where the driver basically does trial and error, but ideally we
>> should be able to tell i915 also the pin assignment that was
>> negotiated with the partner device so it knows the DP lane count.
> 
> Yeah, if the information is there, we'd like to know.

So far machines actually using a setup where the kernel does the
USB-PD / Type-C negotiation rather then this being handled in firmware
in say the Alpine Ridge controller, are very rare.

AFAIK in the Alpine Ridge controller case, there is no communication
with the i915 driver, the only thing the Alpine Ridge controller does
which the everything done in the kernel approach does not, is that
it actually has a pin connected to the HDP pin of the displayport in
question.  But that just lets the i915 driver know about hotplug-events,
not how many lanes are used.

Currently I'm aware of only 2 x86 models which actually need the
hotplug event propagation from the Type-C subsystem to the i915 driver.
Do we really want to come up with a much more complex solution to
optimize for this corner case, while the much more common case
(Alpine Ridge controller) does not provide this info to the i915 driver?

To give some idea of the complexity, first of all we need some
mechanism to let the kernel know which drm_connector is connected
to which Type-C port. For the 2 models I know if which need this, this
info is not available and we would need to hardcode it in the kernel
based on e.g. DMI matching.

Then once we have this link, we need to start thinking about probe
ordering. Likely we need the typec framework to find the drm_connector,
since the typec-partner device is only created when there actually is
a DP capable "dongle" connected, making doing it the other way around
tricky. Then the typec-partner device needs to get a reference or some
such to make sure the drm_connector does not fgo away during the lifetime
of the typec-partner device.

I would really like to avoid this, so if we want to send more info to
the i915 driver, I suggest we create some event struct for this which
gets passed to the notifier, this could include a string to
describe the DP connector which the Type-C connector is connected to
when the mux is set to DP mode, e.g. "i915/DP-1" should be unique
or probably better, use the PCI device name, so: "0000:00:02.0/DP-1"

Then we still have a loose coupling avoiding lifetime issues, while
the receiving drm driver can check which connector the event is for
and we can then also include other info such as lane-count and orientation
in the event struct.

> With the orientation

As said, the orientation *should* be corrected by the mux switch,
it is corrected by the mux switch on the hardware I know about and
actually getting it wrong breaks the display output (we had a bug there),
so I guess that getting it wrong leads to the lines being swizzled in a way
which the i915 driver does not check for ...

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus]

On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 27-02-19 12:16, Jani Nikula wrote:
> > On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > One thing that this series does not consider is the DP lane count
> > > problem. The GPU drivers (i915 in this case) does not know is four,
> > > two or one DP lanes in use.
> > 
> > Also, orientation.
> 
> The orientation should simply be correct with Type-C over DP. The mux /
> orientation-switch used will take care of (physically) swapping the
> lanes if the connector is inserted upside-down.
> 
> > > I guess that is not a critical issue since there is a workaround (I
> > > think) where the driver basically does trial and error, but ideally we
> > > should be able to tell i915 also the pin assignment that was
> > > negotiated with the partner device so it knows the DP lane count.
> > 
> > Yeah, if the information is there, we'd like to know.
> 
> So far machines actually using a setup where the kernel does the
> USB-PD / Type-C negotiation rather then this being handled in firmware
> in say the Alpine Ridge controller, are very rare.
> 
> AFAIK in the Alpine Ridge controller case, there is no communication
> with the i915 driver, the only thing the Alpine Ridge controller does
> which the everything done in the kernel approach does not, is that
> it actually has a pin connected to the HDP pin of the displayport in
> question.  But that just lets the i915 driver know about hotplug-events,
> not how many lanes are used.
> 
> Currently I'm aware of only 2 x86 models which actually need the
> hotplug event propagation from the Type-C subsystem to the i915 driver.
> Do we really want to come up with a much more complex solution to
> optimize for this corner case, while the much more common case
> (Alpine Ridge controller) does not provide this info to the i915 driver?

The HPD signal is often handled with a GPIO on Intel Platforms. Either
the PD controller or EC controller, after receiving the Attention
message, triggers the GPIO. On some systems though that GPIO method
could not be used, so instead a side channel communication is used:
the GFX device (so i915 driver) receives a specific custom interrupt.

Unfortunately it now appears that there may be products coming where
using the GPIO is not going to be possible, and also side channel
communication is going to be out of the question. I've started an
internal discussion inside Intel about the possibility to extend the
UCSI specification to be able to support that kind of products.

Alpine Ridge uses TI's Power Delivery controllers. The platforms that
have Alpine Ridge TBT controller(s) often expose the USB Type-C
connectors on them to the OS via UCSI, however, it appears the Alpine
Ridge actually allows direct access to the PD controllers from the OS.
That would mean we can supply the same information to the GPU drivers
that we will deliver on CHT also on every platform that uses Alpine
Ridge.

> To give some idea of the complexity, first of all we need some
> mechanism to let the kernel know which drm_connector is connected
> to which Type-C port. For the 2 models I know if which need this, this
> info is not available and we would need to hardcode it in the kernel
> based on e.g. DMI matching.

I've been thinking about this... Do we actually need to link the
correct drm_connector to the Type-C connector? Perhaps we can make
this work by just linking the GFX device to the Type-C connector.

> Then once we have this link, we need to start thinking about probe
> ordering. Likely we need the typec framework to find the drm_connector,
> since the typec-partner device is only created when there actually is
> a DP capable "dongle" connected, making doing it the other way around
> tricky. Then the typec-partner device needs to get a reference or some
> such to make sure the drm_connector does not fgo away during the lifetime
> of the typec-partner device.

No! We must not link the partner device with anything other than the
Type-C connector. We link the USB Type-C connector to the DisplayPort,
and we link the USB Type-C connector to the partner. The Type-C
connector is the proxy here.

> I would really like to avoid this, so if we want to send more info to
> the i915 driver, I suggest we create some event struct for this which
> gets passed to the notifier, this could include a string to
> describe the DP connector which the Type-C connector is connected to
> when the mux is set to DP mode, e.g. "i915/DP-1" should be unique
> or probably better, use the PCI device name, so: "0000:00:02.0/DP-1"
> 
> Then we still have a loose coupling avoiding lifetime issues, while
> the receiving drm driver can check which connector the event is for
> and we can then also include other info such as lane-count and orientation
> in the event struct.

Well, I don't think we can deny the GPU driver (in this case) the
information that we have and that is relevant to it, just because it
seems difficult to deliver that information to the right location.

I'm not sure we have checked all the options we have yet. Perhaps
there is a simpler way of doing this.


thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Hans de Goede]

Hi,

On 28-02-19 10:15, Heikki Krogerus wrote:
> On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 27-02-19 12:16, Jani Nikula wrote:
>>> On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>>>> One thing that this series does not consider is the DP lane count
>>>> problem. The GPU drivers (i915 in this case) does not know is four,
>>>> two or one DP lanes in use.
>>>
>>> Also, orientation.
>>
>> The orientation should simply be correct with Type-C over DP. The mux /
>> orientation-switch used will take care of (physically) swapping the
>> lanes if the connector is inserted upside-down.
>>
>>>> I guess that is not a critical issue since there is a workaround (I
>>>> think) where the driver basically does trial and error, but ideally we
>>>> should be able to tell i915 also the pin assignment that was
>>>> negotiated with the partner device so it knows the DP lane count.
>>>
>>> Yeah, if the information is there, we'd like to know.
>>
>> So far machines actually using a setup where the kernel does the
>> USB-PD / Type-C negotiation rather then this being handled in firmware
>> in say the Alpine Ridge controller, are very rare.
>>
>> AFAIK in the Alpine Ridge controller case, there is no communication
>> with the i915 driver, the only thing the Alpine Ridge controller does
>> which the everything done in the kernel approach does not, is that
>> it actually has a pin connected to the HDP pin of the displayport in
>> question.  But that just lets the i915 driver know about hotplug-events,
>> not how many lanes are used.
>>
>> Currently I'm aware of only 2 x86 models which actually need the
>> hotplug event propagation from the Type-C subsystem to the i915 driver.
>> Do we really want to come up with a much more complex solution to
>> optimize for this corner case, while the much more common case
>> (Alpine Ridge controller) does not provide this info to the i915 driver?
> 
> The HPD signal is often handled with a GPIO on Intel Platforms. Either
> the PD controller or EC controller, after receiving the Attention
> message, triggers the GPIO. On some systems though that GPIO method
> could not be used, so instead a side channel communication is used:
> the GFX device (so i915 driver) receives a specific custom interrupt.
> 
> Unfortunately it now appears that there may be products coming where
> using the GPIO is not going to be possible, and also side channel
> communication is going to be out of the question. I've started an
> internal discussion inside Intel about the possibility to extend the
> UCSI specification to be able to support that kind of products.
> 
> Alpine Ridge uses TI's Power Delivery controllers. The platforms that
> have Alpine Ridge TBT controller(s) often expose the USB Type-C
> connectors on them to the OS via UCSI, however, it appears the Alpine
> Ridge actually allows direct access to the PD controllers from the OS.
> That would mean we can supply the same information to the GPU drivers
> that we will deliver on CHT also on every platform that uses Alpine
> Ridge.

Ok.

>> To give some idea of the complexity, first of all we need some
>> mechanism to let the kernel know which drm_connector is connected
>> to which Type-C port. For the 2 models I know if which need this, this
>> info is not available and we would need to hardcode it in the kernel
>> based on e.g. DMI matching.
> 
> I've been thinking about this... Do we actually need to link the
> correct drm_connector to the Type-C connector? Perhaps we can make
> this work by just linking the GFX device to the Type-C connector.

What use is it to the kms driver if it gets an event there is a DP
hotplug with x lanes and orientation foo, if we are not telling it
on which DP port it is ? kms devices already have multiple DP ports
and more then one could be hooked-up to type-c connectors.

>> Then once we have this link, we need to start thinking about probe
>> ordering. Likely we need the typec framework to find the drm_connector,
>> since the typec-partner device is only created when there actually is
>> a DP capable "dongle" connected, making doing it the other way around
>> tricky. Then the typec-partner device needs to get a reference or some
>> such to make sure the drm_connector does not fgo away during the lifetime
>> of the typec-partner device.
> 
> No! We must not link the partner device with anything other than the
> Type-C connector. We link the USB Type-C connector to the DisplayPort,
> and we link the USB Type-C connector to the partner. The Type-C
> connector is the proxy here.

Maybe, but even then we still need one side of the link to have a
reference on the other, having a proxy in between does not change
anything.

>> I would really like to avoid this, so if we want to send more info to
>> the i915 driver, I suggest we create some event struct for this which
>> gets passed to the notifier, this could include a string to
>> describe the DP connector which the Type-C connector is connected to
>> when the mux is set to DP mode, e.g. "i915/DP-1" should be unique
>> or probably better, use the PCI device name, so: "0000:00:02.0/DP-1"
>>
>> Then we still have a loose coupling avoiding lifetime issues, while
>> the receiving drm driver can check which connector the event is for
>> and we can then also include other info such as lane-count and orientation
>> in the event struct.
> 
> Well, I don't think we can deny the GPU driver (in this case) the
> information that we have and that is relevant to it, just because it
> seems difficult to deliver that information to the right location.

Right, but this does not require a tight-coupling. My original
proposal can do this if we pass a data struct with an identifier
for the DP port for which the event is to the notifier. I suggest using
a string for identifier, something like: "0000:00:02.0/DP-1" this
event struct could then also contain all the info we want to pass.

> I'm not sure we have checked all the options we have yet. Perhaps
> there is a simpler way of doing this.

As a result of writing this patches I've been thinking that we really
have a need for some sort of in kernel event mechanism, think something
pub/sub ish, a bit like mqtt.

I'm thinking a global event-queue with an API like this:

struct kernel_event {
	enum kernel_event_type type;
	char source_id[32];
	char dest_id[32];
	union data {
		kernel_event_type_foo foo;
		kernel_event_type_bar bar;
	};
}

Where drivers interested in events can then specify that they
only want events of a certain type and optionally also filter
on source / dest id.

Note that setting dest_id would be optional for event generators,
since not all event generators will know this.

Looking at all the extcon and power_supply notifications we
already have going on with the Type-C PD support, all using their
own private notifier solutions, I think something generic like this,
which does not depend on one device getting some sort of reference
on another device, might in the end be better.

This would also avoid a lot of PROBE_DEFER handling in various places,
which in some cases gets rather tricky wrt ordering.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus]

Hi Hans,

On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 28-02-19 10:15, Heikki Krogerus wrote:
> > On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 27-02-19 12:16, Jani Nikula wrote:
> > > > On Wed, 27 Feb 2019, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > > > One thing that this series does not consider is the DP lane count
> > > > > problem. The GPU drivers (i915 in this case) does not know is four,
> > > > > two or one DP lanes in use.
> > > > 
> > > > Also, orientation.
> > > 
> > > The orientation should simply be correct with Type-C over DP. The mux /
> > > orientation-switch used will take care of (physically) swapping the
> > > lanes if the connector is inserted upside-down.
> > > 
> > > > > I guess that is not a critical issue since there is a workaround (I
> > > > > think) where the driver basically does trial and error, but ideally we
> > > > > should be able to tell i915 also the pin assignment that was
> > > > > negotiated with the partner device so it knows the DP lane count.
> > > > 
> > > > Yeah, if the information is there, we'd like to know.
> > > 
> > > So far machines actually using a setup where the kernel does the
> > > USB-PD / Type-C negotiation rather then this being handled in firmware
> > > in say the Alpine Ridge controller, are very rare.
> > > 
> > > AFAIK in the Alpine Ridge controller case, there is no communication
> > > with the i915 driver, the only thing the Alpine Ridge controller does
> > > which the everything done in the kernel approach does not, is that
> > > it actually has a pin connected to the HDP pin of the displayport in
> > > question.  But that just lets the i915 driver know about hotplug-events,
> > > not how many lanes are used.
> > > 
> > > Currently I'm aware of only 2 x86 models which actually need the
> > > hotplug event propagation from the Type-C subsystem to the i915 driver.
> > > Do we really want to come up with a much more complex solution to
> > > optimize for this corner case, while the much more common case
> > > (Alpine Ridge controller) does not provide this info to the i915 driver?
> > 
> > The HPD signal is often handled with a GPIO on Intel Platforms. Either
> > the PD controller or EC controller, after receiving the Attention
> > message, triggers the GPIO. On some systems though that GPIO method
> > could not be used, so instead a side channel communication is used:
> > the GFX device (so i915 driver) receives a specific custom interrupt.
> > 
> > Unfortunately it now appears that there may be products coming where
> > using the GPIO is not going to be possible, and also side channel
> > communication is going to be out of the question. I've started an
> > internal discussion inside Intel about the possibility to extend the
> > UCSI specification to be able to support that kind of products.
> > 
> > Alpine Ridge uses TI's Power Delivery controllers. The platforms that
> > have Alpine Ridge TBT controller(s) often expose the USB Type-C
> > connectors on them to the OS via UCSI, however, it appears the Alpine
> > Ridge actually allows direct access to the PD controllers from the OS.
> > That would mean we can supply the same information to the GPU drivers
> > that we will deliver on CHT also on every platform that uses Alpine
> > Ridge.
> 
> Ok.
> 
> > > To give some idea of the complexity, first of all we need some
> > > mechanism to let the kernel know which drm_connector is connected
> > > to which Type-C port. For the 2 models I know if which need this, this
> > > info is not available and we would need to hardcode it in the kernel
> > > based on e.g. DMI matching.
> > 
> > I've been thinking about this... Do we actually need to link the
> > correct drm_connector to the Type-C connector? Perhaps we can make
> > this work by just linking the GFX device to the Type-C connector.
> 
> What use is it to the kms driver if it gets an event there is a DP
> hotplug with x lanes and orientation foo, if we are not telling it
> on which DP port it is ? kms devices already have multiple DP ports
> and more then one could be hooked-up to type-c connectors.

I was looking at this series. You walk trough every DP port in the
system when the DP alt mode driver broadcasts the event, but maybe
that's different. Never mind.

> > > Then once we have this link, we need to start thinking about probe
> > > ordering. Likely we need the typec framework to find the drm_connector,
> > > since the typec-partner device is only created when there actually is
> > > a DP capable "dongle" connected, making doing it the other way around
> > > tricky. Then the typec-partner device needs to get a reference or some
> > > such to make sure the drm_connector does not fgo away during the lifetime
> > > of the typec-partner device.
> > 
> > No! We must not link the partner device with anything other than the
> > Type-C connector. We link the USB Type-C connector to the DisplayPort,
> > and we link the USB Type-C connector to the partner. The Type-C
> > connector is the proxy here.
> 
> Maybe, but even then we still need one side of the link to have a
> reference on the other, having a proxy in between does not change
> anything.
> 
> > > I would really like to avoid this, so if we want to send more info to
> > > the i915 driver, I suggest we create some event struct for this which
> > > gets passed to the notifier, this could include a string to
> > > describe the DP connector which the Type-C connector is connected to
> > > when the mux is set to DP mode, e.g. "i915/DP-1" should be unique
> > > or probably better, use the PCI device name, so: "0000:00:02.0/DP-1"
> > > 
> > > Then we still have a loose coupling avoiding lifetime issues, while
> > > the receiving drm driver can check which connector the event is for
> > > and we can then also include other info such as lane-count and orientation
> > > in the event struct.
> > 
> > Well, I don't think we can deny the GPU driver (in this case) the
> > information that we have and that is relevant to it, just because it
> > seems difficult to deliver that information to the right location.
> 
> Right, but this does not require a tight-coupling. My original
> proposal can do this if we pass a data struct with an identifier
> for the DP port for which the event is to the notifier. I suggest using
> a string for identifier, something like: "0000:00:02.0/DP-1" this
> event struct could then also contain all the info we want to pass.

I do agree that we should not tie the objects (device entries)
representing these components in kernel together, but I assume that we
agree now that the connection between the two - the USB Type-C
connector and the DisplayPort - must be described somewhere, either in
firmware or build-in? So I guess we are talking implementation details
here, right?

If that is the case...

That string identifier you proposed would basically provide the
details about the connection, so if we know those details, we might as
well use "normal" ways to describe the connection and just extract
them in runtime in the function that our DP alt mode driver calls. I
think the connection has to be defined in i915 on CHT in any case.

The DP alt mode driver should definitely not need to pass anything
else to the notifier other than handle to itself (actually, handle
straight to the port device would be better) as an identifier. The
notifier function needs to be the one that determines the actual
connection using that handle. Even if the target DP is described using
a string like you propose, then that string has to come from
somewhere, most likely from a device property. The notifier function
can just as well extract it, we don't need to pass it separately.

Here's my suggestion for function prototype:

int drm_typec_dp_notification(struct device *typec_port_dev,
                              struct typec_displayport_data *data);

So that function finds the connection between typec_port_dev and the
correct DP in runtime, possibly by letting i915 (or what ever GPU
driver) to do that. Once the function is done, it decrements any ref
counts that it incremented before returning.

That struct typec_displayport_data has all the information we have -
the current pin assignment from the Configure VDO, HPD IRQ from the
last Status Update, etc. - so it needs to be passed as payload to the
notifier.

> > I'm not sure we have checked all the options we have yet. Perhaps
> > there is a simpler way of doing this.
> 
> As a result of writing this patches I've been thinking that we really
> have a need for some sort of in kernel event mechanism, think something
> pub/sub ish, a bit like mqtt.
> 
> I'm thinking a global event-queue with an API like this:
> 
> struct kernel_event {
> 	enum kernel_event_type type;
> 	char source_id[32];
> 	char dest_id[32];
> 	union data {
> 		kernel_event_type_foo foo;
> 		kernel_event_type_bar bar;
> 	};
> }
> 
> Where drivers interested in events can then specify that they
> only want events of a certain type and optionally also filter
> on source / dest id.
> 
> Note that setting dest_id would be optional for event generators,
> since not all event generators will know this.
> 
> Looking at all the extcon and power_supply notifications we
> already have going on with the Type-C PD support, all using their
> own private notifier solutions, I think something generic like this,
> which does not depend on one device getting some sort of reference
> on another device, might in the end be better.
> 
> This would also avoid a lot of PROBE_DEFER handling in various places,
> which in some cases gets rather tricky wrt ordering.

Interesting proposal. Something like that could be useful.


thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Hans de Goede]

Hi Heikki,

On 28-02-19 15:47, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 28-02-19 10:15, Heikki Krogerus wrote:

<snip>

>>> I've been thinking about this... Do we actually need to link the
>>> correct drm_connector to the Type-C connector? Perhaps we can make
>>> this work by just linking the GFX device to the Type-C connector.
>>
>> What use is it to the kms driver if it gets an event there is a DP
>> hotplug with x lanes and orientation foo, if we are not telling it
>> on which DP port it is ? kms devices already have multiple DP ports
>> and more then one could be hooked-up to type-c connectors.
> 
> I was looking at this series. You walk trough every DP port in the
> system when the DP alt mode driver broadcasts the event, but maybe
> that's different. Never mind.

Right, my "simple / naive" solution simply tells the kms driver to
check all DP ports for connection state changes, similar to how
running "xrandr" under Xorg causes the kms driver to re-check the
connection status of all ports. Actually running xrandr under Xorg
after plugging in the cable, is how I did my initial DP over Type-C
testing on the GPD win.

But once we start passing extra-info, I believe the kms driver needs
to know to which connector that info belongs.

<snip>

>>> Well, I don't think we can deny the GPU driver (in this case) the
>>> information that we have and that is relevant to it, just because it
>>> seems difficult to deliver that information to the right location.
>>
>> Right, but this does not require a tight-coupling. My original
>> proposal can do this if we pass a data struct with an identifier
>> for the DP port for which the event is to the notifier. I suggest using
>> a string for identifier, something like: "0000:00:02.0/DP-1" this
>> event struct could then also contain all the info we want to pass.
> 
> I do agree that we should not tie the objects (device entries)
> representing these components in kernel together, but I assume that we
> agree now that the connection between the two - the USB Type-C
> connector and the DisplayPort - must be described somewhere, either in
> firmware or build-in? So I guess we are talking implementation details
> here, right?

Right.

> If that is the case...
> 
> That string identifier you proposed would basically provide the
> details about the connection, so if we know those details, we might as
> well use "normal" ways to describe the connection and just extract
> them in runtime in the function that our DP alt mode driver calls. I
> think the connection has to be defined in i915 on CHT in any case.

Interesting, I think the connection should be described in the fwnode
for the fusb302 device for the CHT/GPD win case. Specifically I think
this fits well as a property of the dp altmode.

> The DP alt mode driver should definitely not need to pass anything
> else to the notifier other than handle to itself (actually, handle
> straight to the port device would be better) as an identifier. The
> notifier function needs to be the one that determines the actual
> connection using that handle. Even if the target DP is described using
> a string like you propose, then that string has to come from
> somewhere, most likely from a device property. The notifier function
> can just as well extract it, we don't need to pass it separately.
> 
> Here's my suggestion for function prototype:
> 
> int drm_typec_dp_notification(struct device *typec_port_dev,
>                                struct typec_displayport_data *data);

How about instead of the port_dev we pass in the altmode object and
we have a method to get the fwnode for the altmode? Then the
drm_typec_dp_notification() function can get info from that fwnode
to implement the connection finding you describe below:

> So that function finds the connection between typec_port_dev and the
> correct DP in runtime, possibly by letting i915 (or what ever GPU
> driver) to do that. Once the function is done, it decrements any ref
> counts that it incremented before returning.
> 
> That struct typec_displayport_data has all the information we have -
> the current pin assignment from the Configure VDO, HPD IRQ from the
> last Status Update, etc. - so it needs to be passed as payload to the
> notifier.

Ack.

So I believe that this discussion ties into the discussion from the:
"[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes"

Mail thread. As discussed there I agree that adding a usb_connector
child fwnode to the fwnode for the fusb302 to describe things like
sink- and source-pdos is a good idea.

Our last few mails were discussing describing supported alt-modes on
the connector by adding altmode child-nodes to the usb_connector node.

I think it is best to continue that discussion here, as the 2 discussions
tie into one another.

So my last proposal in that thread was adding the following to:

Documentation/devicetree/bindings/connector/usb-connector.txt:

"""
Optionally an "usb-c-connector" can have child nodes, describing
supported alt-modes.

Required properties for usb-c-connector altmode child-nodes:
compatible:        "usb-type-c-altmode"
svid:              integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32
vdo:               integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific
"""

Since we now want to have the kernel know which DP connector belongs to
the usb_connector being in DP altmode I suggest additionally adding
the following:

"""
Optional properties for DisplayPort (svid==0xff01) altmode child-nodes.

linux,dp-connector String in the form of "device-name/connector-name" describing the
                    DisplayPort connector on the GPU which is used when the usb-c-connector
                    is in DisplayPort altmode, e.g. "0000:00:02.0/DP-1"
"""

This to me feels like it is the most logical place to store the connection info,
at least for the CHT/GPD win case.  For other cases we may very-well need something
different. Since on the CHT/GPD win case both the producer and consumer of this
property will be in kernel, I think it is best to just go with this for now.
If we then later get a different solution for other cases and that solution turns
out to be generic enough that it will also work on the GPD win we can always move
the GPD win (and pocket) over to the new solution. Just like we are moving it
over to the usb_connector fwnode now.

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Heikki Krogerus]

Hi Hans,

On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote:
> Hi Heikki,
> 
> On 28-02-19 15:47, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 28-02-19 10:15, Heikki Krogerus wrote:
> 
> <snip>
> 
> > > > I've been thinking about this... Do we actually need to link the
> > > > correct drm_connector to the Type-C connector? Perhaps we can make
> > > > this work by just linking the GFX device to the Type-C connector.
> > > 
> > > What use is it to the kms driver if it gets an event there is a DP
> > > hotplug with x lanes and orientation foo, if we are not telling it
> > > on which DP port it is ? kms devices already have multiple DP ports
> > > and more then one could be hooked-up to type-c connectors.
> > 
> > I was looking at this series. You walk trough every DP port in the
> > system when the DP alt mode driver broadcasts the event, but maybe
> > that's different. Never mind.
> 
> Right, my "simple / naive" solution simply tells the kms driver to
> check all DP ports for connection state changes, similar to how
> running "xrandr" under Xorg causes the kms driver to re-check the
> connection status of all ports. Actually running xrandr under Xorg
> after plugging in the cable, is how I did my initial DP over Type-C
> testing on the GPD win.
> 
> But once we start passing extra-info, I believe the kms driver needs
> to know to which connector that info belongs.
> 
> <snip>
> 
> > > > Well, I don't think we can deny the GPU driver (in this case) the
> > > > information that we have and that is relevant to it, just because it
> > > > seems difficult to deliver that information to the right location.
> > > 
> > > Right, but this does not require a tight-coupling. My original
> > > proposal can do this if we pass a data struct with an identifier
> > > for the DP port for which the event is to the notifier. I suggest using
> > > a string for identifier, something like: "0000:00:02.0/DP-1" this
> > > event struct could then also contain all the info we want to pass.
> > 
> > I do agree that we should not tie the objects (device entries)
> > representing these components in kernel together, but I assume that we
> > agree now that the connection between the two - the USB Type-C
> > connector and the DisplayPort - must be described somewhere, either in
> > firmware or build-in? So I guess we are talking implementation details
> > here, right?
> 
> Right.
> 
> > If that is the case...
> > 
> > That string identifier you proposed would basically provide the
> > details about the connection, so if we know those details, we might as
> > well use "normal" ways to describe the connection and just extract
> > them in runtime in the function that our DP alt mode driver calls. I
> > think the connection has to be defined in i915 on CHT in any case.
> 
> Interesting, I think the connection should be described in the fwnode
> for the fusb302 device for the CHT/GPD win case. Specifically I think
> this fits well as a property of the dp altmode.

OK, you are correct. I was stupidly still thinking about the driver
loading order, but the order does not matter.

> > The DP alt mode driver should definitely not need to pass anything
> > else to the notifier other than handle to itself (actually, handle
> > straight to the port device would be better) as an identifier. The
> > notifier function needs to be the one that determines the actual
> > connection using that handle. Even if the target DP is described using
> > a string like you propose, then that string has to come from
> > somewhere, most likely from a device property. The notifier function
> > can just as well extract it, we don't need to pass it separately.
> > 
> > Here's my suggestion for function prototype:
> > 
> > int drm_typec_dp_notification(struct device *typec_port_dev,
> >                                struct typec_displayport_data *data);
> 
> How about instead of the port_dev we pass in the altmode object and
> we have a method to get the fwnode for the altmode? Then the
> drm_typec_dp_notification() function can get info from that fwnode
> to implement the connection finding you describe below:

We can pass the altmode object, np, but let's not decide which fwnode
we'll ultimately use. I'm still leaning towards the connector node.

> > So that function finds the connection between typec_port_dev and the
> > correct DP in runtime, possibly by letting i915 (or what ever GPU
> > driver) to do that. Once the function is done, it decrements any ref
> > counts that it incremented before returning.
> > 
> > That struct typec_displayport_data has all the information we have -
> > the current pin assignment from the Configure VDO, HPD IRQ from the
> > last Status Update, etc. - so it needs to be passed as payload to the
> > notifier.
> 
> Ack.
> 
> So I believe that this discussion ties into the discussion from the:
> "[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes"
> 
> Mail thread. As discussed there I agree that adding a usb_connector
> child fwnode to the fwnode for the fusb302 to describe things like
> sink- and source-pdos is a good idea.
> 
> Our last few mails were discussing describing supported alt-modes on
> the connector by adding altmode child-nodes to the usb_connector node.
> 
> I think it is best to continue that discussion here, as the 2 discussions
> tie into one another.
> 
> So my last proposal in that thread was adding the following to:
> 
> Documentation/devicetree/bindings/connector/usb-connector.txt:
> 
> """
> Optionally an "usb-c-connector" can have child nodes, describing
> supported alt-modes.
> 
> Required properties for usb-c-connector altmode child-nodes:
> compatible:        "usb-type-c-altmode"
> svid:              integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32
> vdo:               integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific
> """
> 
> Since we now want to have the kernel know which DP connector belongs to
> the usb_connector being in DP altmode I suggest additionally adding
> the following:
> 
> """
> Optional properties for DisplayPort (svid==0xff01) altmode child-nodes.
> 
> linux,dp-connector String in the form of "device-name/connector-name" describing the
>                    DisplayPort connector on the GPU which is used when the usb-c-connector
>                    is in DisplayPort altmode, e.g. "0000:00:02.0/DP-1"
> """
> 
> This to me feels like it is the most logical place to store the connection info,
> at least for the CHT/GPD win case.  For other cases we may very-well need something
> different. Since on the CHT/GPD win case both the producer and consumer of this
> property will be in kernel, I think it is best to just go with this for now.
> If we then later get a different solution for other cases and that solution turns
> out to be generic enough that it will also work on the GPD win we can always move
> the GPD win (and pocket) over to the new solution. Just like we are moving it
> over to the usb_connector fwnode now.

I don't have a problem with your proposal of using a string like that
at this point, but don't document it. I want to at least see if it's
possible to use real reference instead of a string. I'm also still
not sure should that be placed under the altmode node or should go
under the connector node.

So please don't add it to the usb-connector.txt at this point, even as
an optional property.


thanks,

-- 
heikki
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers

[Hans de Goede]

Hi,

On 04-03-19 16:17, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote:
>> Hi Heikki,
>>
>> On 28-02-19 15:47, Heikki Krogerus wrote:
>>> Hi Hans,
>>>
>>> On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 28-02-19 10:15, Heikki Krogerus wrote:
>>
>> <snip>
>>
>>>>> I've been thinking about this... Do we actually need to link the
>>>>> correct drm_connector to the Type-C connector? Perhaps we can make
>>>>> this work by just linking the GFX device to the Type-C connector.
>>>>
>>>> What use is it to the kms driver if it gets an event there is a DP
>>>> hotplug with x lanes and orientation foo, if we are not telling it
>>>> on which DP port it is ? kms devices already have multiple DP ports
>>>> and more then one could be hooked-up to type-c connectors.
>>>
>>> I was looking at this series. You walk trough every DP port in the
>>> system when the DP alt mode driver broadcasts the event, but maybe
>>> that's different. Never mind.
>>
>> Right, my "simple / naive" solution simply tells the kms driver to
>> check all DP ports for connection state changes, similar to how
>> running "xrandr" under Xorg causes the kms driver to re-check the
>> connection status of all ports. Actually running xrandr under Xorg
>> after plugging in the cable, is how I did my initial DP over Type-C
>> testing on the GPD win.
>>
>> But once we start passing extra-info, I believe the kms driver needs
>> to know to which connector that info belongs.
>>
>> <snip>
>>
>>>>> Well, I don't think we can deny the GPU driver (in this case) the
>>>>> information that we have and that is relevant to it, just because it
>>>>> seems difficult to deliver that information to the right location.
>>>>
>>>> Right, but this does not require a tight-coupling. My original
>>>> proposal can do this if we pass a data struct with an identifier
>>>> for the DP port for which the event is to the notifier. I suggest using
>>>> a string for identifier, something like: "0000:00:02.0/DP-1" this
>>>> event struct could then also contain all the info we want to pass.
>>>
>>> I do agree that we should not tie the objects (device entries)
>>> representing these components in kernel together, but I assume that we
>>> agree now that the connection between the two - the USB Type-C
>>> connector and the DisplayPort - must be described somewhere, either in
>>> firmware or build-in? So I guess we are talking implementation details
>>> here, right?
>>
>> Right.
>>
>>> If that is the case...
>>>
>>> That string identifier you proposed would basically provide the
>>> details about the connection, so if we know those details, we might as
>>> well use "normal" ways to describe the connection and just extract
>>> them in runtime in the function that our DP alt mode driver calls. I
>>> think the connection has to be defined in i915 on CHT in any case.
>>
>> Interesting, I think the connection should be described in the fwnode
>> for the fusb302 device for the CHT/GPD win case. Specifically I think
>> this fits well as a property of the dp altmode.
> 
> OK, you are correct. I was stupidly still thinking about the driver
> loading order, but the order does not matter.
> 
>>> The DP alt mode driver should definitely not need to pass anything
>>> else to the notifier other than handle to itself (actually, handle
>>> straight to the port device would be better) as an identifier. The
>>> notifier function needs to be the one that determines the actual
>>> connection using that handle. Even if the target DP is described using
>>> a string like you propose, then that string has to come from
>>> somewhere, most likely from a device property. The notifier function
>>> can just as well extract it, we don't need to pass it separately.
>>>
>>> Here's my suggestion for function prototype:
>>>
>>> int drm_typec_dp_notification(struct device *typec_port_dev,
>>>                                 struct typec_displayport_data *data);
>>
>> How about instead of the port_dev we pass in the altmode object and
>> we have a method to get the fwnode for the altmode? Then the
>> drm_typec_dp_notification() function can get info from that fwnode
>> to implement the connection finding you describe below:
> 
> We can pass the altmode object, np, but let's not decide which fwnode
> we'll ultimately use. I'm still leaning towards the connector node.
> 
>>> So that function finds the connection between typec_port_dev and the
>>> correct DP in runtime, possibly by letting i915 (or what ever GPU
>>> driver) to do that. Once the function is done, it decrements any ref
>>> counts that it incremented before returning.
>>>
>>> That struct typec_displayport_data has all the information we have -
>>> the current pin assignment from the Configure VDO, HPD IRQ from the
>>> last Status Update, etc. - so it needs to be passed as payload to the
>>> notifier.
>>
>> Ack.
>>
>> So I believe that this discussion ties into the discussion from the:
>> "[PATCH 0/2] platform/x86: intel_cht_int33fe: Start using software nodes"
>>
>> Mail thread. As discussed there I agree that adding a usb_connector
>> child fwnode to the fwnode for the fusb302 to describe things like
>> sink- and source-pdos is a good idea.
>>
>> Our last few mails were discussing describing supported alt-modes on
>> the connector by adding altmode child-nodes to the usb_connector node.
>>
>> I think it is best to continue that discussion here, as the 2 discussions
>> tie into one another.
>>
>> So my last proposal in that thread was adding the following to:
>>
>> Documentation/devicetree/bindings/connector/usb-connector.txt:
>>
>> """
>> Optionally an "usb-c-connector" can have child nodes, describing
>> supported alt-modes.
>>
>> Required properties for usb-c-connector altmode child-nodes:
>> compatible:        "usb-type-c-altmode"
>> svid:              integer, Standard or Vendor ID for the altmode (u16 stored in an u32) property and an u32
>> vdo:               integer, Vendor Data Object, VDO describing the altmode capabilies, SVID specific
>> """
>>
>> Since we now want to have the kernel know which DP connector belongs to
>> the usb_connector being in DP altmode I suggest additionally adding
>> the following:
>>
>> """
>> Optional properties for DisplayPort (svid==0xff01) altmode child-nodes.
>>
>> linux,dp-connector String in the form of "device-name/connector-name" describing the
>>                     DisplayPort connector on the GPU which is used when the usb-c-connector
>>                     is in DisplayPort altmode, e.g. "0000:00:02.0/DP-1"
>> """
>>
>> This to me feels like it is the most logical place to store the connection info,
>> at least for the CHT/GPD win case.  For other cases we may very-well need something
>> different. Since on the CHT/GPD win case both the producer and consumer of this
>> property will be in kernel, I think it is best to just go with this for now.
>> If we then later get a different solution for other cases and that solution turns
>> out to be generic enough that it will also work on the GPD win we can always move
>> the GPD win (and pocket) over to the new solution. Just like we are moving it
>> over to the usb_connector fwnode now.
> 
> I don't have a problem with your proposal of using a string like that
> at this point, but don't document it. I want to at least see if it's
> possible to use real reference instead of a string. I'm also still
> not sure should that be placed under the altmode node or should go
> under the connector node.
> 
> So please don't add it to the usb-connector.txt at this point, even as
> an optional property.

Ok, I will give the discussed approach a try and then post a new version of
the patchset. I hope to get around to this this weekend (as you know this is a side /
spare-time project for me).

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel