diff mbox series

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

Message ID 20190225132037.31458-4-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers | expand

Commit Message

Hans de Goede Feb. 25, 2019, 1:20 p.m. UTC 
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(-)

Comments

Greg Kroah-Hartman Feb. 25, 2019, 2:06 p.m. UTC | #1 
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
Hans de Goede Feb. 25, 2019, 4:19 p.m. UTC | #2 
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
kernel test robot Feb. 26, 2019, 7:40 a.m. UTC | #3 
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
kernel test robot Feb. 26, 2019, 4:04 p.m. UTC | #4 
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
Heikki Krogerus Feb. 27, 2019, 9:44 a.m. UTC | #5 
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,
Hans de Goede Feb. 27, 2019, 3:51 p.m. UTC | #6 
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
diff mbox series

Patch

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[] = {