Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1462282 > unrolled thread

[PATCH] drm/udl: Ensure channel is selected before using the device.

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm/udl: Ensure channel is selected before using the device. Jamie Lentin <jm@lentin.co.uk> - 2016-08-14 20:20 +0200
    Re: [PATCH] drm/udl: Ensure channel is selected before using the  device. Daniel Vetter <daniel@ffwll.ch> - 2016-08-15 08:50 +0200
      [PATCH v2] drm/udl: Ensure channel is selected before using the device. Jamie Lentin <jm@lentin.co.uk> - 2016-08-23 00:20 +0200
        Re: [PATCH v2] drm/udl: Ensure channel is selected before using the  device. Daniel Vetter <daniel@ffwll.ch> - 2016-08-23 08:00 +0200

#1462282 — [PATCH] drm/udl: Ensure channel is selected before using the device.

Lift configuration command from udlfb. If this command is not sent,
then the device never outputs a signal, it's status LED is continually
flashing and occasional "udlfb: wait for urb interrupted" messages are
produced.

Tested with a Rextron VCUD-60 attached to a Thinkpad X201s on Linux 4.7.0

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
This ended up in udl_connector_init() since the name suggests it has
something to do with configuring which output to use, although a quick
search through other displaylink drivers didn't shed any light on what
the bytes in set_def_chn actually mean.

I'm not sure which displaylink chipset the VCUD-60 has, but it's USB ID
is 17e9:0136 if that helps. The vendor descriptor is all 00.

Cheers,
---
 drivers/gpu/drm/udl/udl_connector.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index 4709b54..2ee8b38 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -16,6 +16,8 @@
 #include <drm/drm_crtc_helper.h>
 #include "udl_drv.h"
 
+#define NR_USB_REQUEST_CHANNEL 0x12
+
 /* dummy connector to just get EDID,
    all UDL appear to have a DVI-D */
 
@@ -54,6 +56,26 @@ error:
 	return NULL;
 }
 
+/*
+ * This is necessary before we can communicate with the display controller.
+ */
+static int udl_select_std_channel(struct udl_device *udl)
+{
+	int ret;
+	u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
+			    0x1C, 0x88, 0x5E, 0x15,
+			    0x60, 0xFE, 0xC6, 0x97,
+			    0x16, 0x3D, 0x47, 0xF2};
+
+	ret = usb_control_msg(udl->udev,
+			      usb_sndctrlpipe(udl->udev, 0),
+			      NR_USB_REQUEST_CHANNEL,
+			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
+			      set_def_chn, sizeof(set_def_chn),
+			      USB_CTRL_SET_TIMEOUT);
+	return ret < 0 ? ret : 0;
+}
+
 static int udl_get_modes(struct drm_connector *connector)
 {
 	struct udl_device *udl = connector->dev->dev_private;
@@ -139,6 +161,7 @@ static const struct drm_connector_funcs udl_connector_funcs = {
 int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
+	int ret;
 
 	connector = kzalloc(sizeof(struct drm_connector), GFP_KERNEL);
 	if (!connector)
@@ -147,6 +170,10 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
 	drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_DVII);
 	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
 
+	ret = udl_select_std_channel(connector->dev->dev_private);
+	if (ret)
+		DRM_ERROR("Selecting channel failed err %x\n", ret);
+
 	drm_connector_register(connector);
 	drm_mode_connector_attach_encoder(connector, encoder);
 
-- 
2.8.1

[toc] | [next] | [standalone]

#1462569 — Re: [PATCH] drm/udl: Ensure channel is selected before using the device.

On Sun, Aug 14, 2016 at 07:19:35PM +0100, Jamie Lentin wrote:
> Lift configuration command from udlfb. If this command is not sent,
> then the device never outputs a signal, it's status LED is continually
> flashing and occasional "udlfb: wait for urb interrupted" messages are
> produced.
> 
> Tested with a Rextron VCUD-60 attached to a Thinkpad X201s on Linux 4.7.0
> 
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> ---
> This ended up in udl_connector_init() since the name suggests it has
> something to do with configuring which output to use, although a quick
> search through other displaylink drivers didn't shed any light on what
> the bytes in set_def_chn actually mean.
> 
> I'm not sure which displaylink chipset the VCUD-60 has, but it's USB ID
> is 17e9:0136 if that helps. The vendor descriptor is all 00.
> 
> Cheers,

Connector init feels like the wrong place. I'd either put it into
udl_driver_load (if it's really just one-time init work). Or into
udl_encoder_commit if we need to set this every time we do a modeset, e.g.
also after resume.
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_connector.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index 4709b54..2ee8b38 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -16,6 +16,8 @@
>  #include <drm/drm_crtc_helper.h>
>  #include "udl_drv.h"
>  
> +#define NR_USB_REQUEST_CHANNEL 0x12
> +
>  /* dummy connector to just get EDID,
>     all UDL appear to have a DVI-D */
>  
> @@ -54,6 +56,26 @@ error:
>  	return NULL;
>  }
>  
> +/*
> + * This is necessary before we can communicate with the display controller.
> + */
> +static int udl_select_std_channel(struct udl_device *udl)
> +{
> +	int ret;
> +	u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
> +			    0x1C, 0x88, 0x5E, 0x15,
> +			    0x60, 0xFE, 0xC6, 0x97,
> +			    0x16, 0x3D, 0x47, 0xF2};
> +
> +	ret = usb_control_msg(udl->udev,
> +			      usb_sndctrlpipe(udl->udev, 0),
> +			      NR_USB_REQUEST_CHANNEL,
> +			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
> +			      set_def_chn, sizeof(set_def_chn),
> +			      USB_CTRL_SET_TIMEOUT);
> +	return ret < 0 ? ret : 0;
> +}
> +
>  static int udl_get_modes(struct drm_connector *connector)
>  {
>  	struct udl_device *udl = connector->dev->dev_private;
> @@ -139,6 +161,7 @@ static const struct drm_connector_funcs udl_connector_funcs = {
>  int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
>  {
>  	struct drm_connector *connector;
> +	int ret;
>  
>  	connector = kzalloc(sizeof(struct drm_connector), GFP_KERNEL);
>  	if (!connector)
> @@ -147,6 +170,10 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
>  	drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_DVII);
>  	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
>  
> +	ret = udl_select_std_channel(connector->dev->dev_private);
> +	if (ret)
> +		DRM_ERROR("Selecting channel failed err %x\n", ret);
> +
>  	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
> -- 
> 2.8.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1468105 — [PATCH v2] drm/udl: Ensure channel is selected before using the device.

Lift configuration command from udlfb. This appears to be essential for
at least a Rextron VCUD-60, without which no URB communication occurs.

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
udl_encoder_commit() is too late to do this set up in it seems. This
setup doesn't need to be performed again after a suspend, although this
is somewhat academic until I send the patch adding suspend and resume
functions.

Tested with a Rextron VCUD-60 attached to a Thinkpad X201s on Linux 4.7.0

Cheers,
---
 drivers/gpu/drm/udl/udl_main.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 33dbfb2..29f0207 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -16,6 +16,8 @@
 /* -BULK_SIZE as per usb-skeleton. Can we get full page and avoid overhead? */
 #define BULK_SIZE 512
 
+#define NR_USB_REQUEST_CHANNEL 0x12
+
 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
 #define WRITES_IN_FLIGHT (4)
 #define MAX_VENDOR_DESCRIPTOR_SIZE 256
@@ -90,6 +92,26 @@ success:
 	return true;
 }
 
+/*
+ * Need to ensure a channel is selected before submitting URBs
+ */
+static int udl_select_std_channel(struct udl_device *udl)
+{
+	int ret;
+	u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
+			    0x1C, 0x88, 0x5E, 0x15,
+			    0x60, 0xFE, 0xC6, 0x97,
+			    0x16, 0x3D, 0x47, 0xF2};
+
+	ret = usb_control_msg(udl->udev,
+			      usb_sndctrlpipe(udl->udev, 0),
+			      NR_USB_REQUEST_CHANNEL,
+			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
+			      set_def_chn, sizeof(set_def_chn),
+			      USB_CTRL_SET_TIMEOUT);
+	return ret < 0 ? ret : 0;
+}
+
 static void udl_release_urb_work(struct work_struct *work)
 {
 	struct urb_node *unode = container_of(work, struct urb_node,
@@ -301,6 +323,9 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags)
 		goto err;
 	}
 
+	if (udl_select_std_channel(udl))
+		DRM_ERROR("Selecting channel failed\n");
+
 	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
 		DRM_ERROR("udl_alloc_urb_list failed\n");
 		goto err;
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1468299 — Re: [PATCH v2] drm/udl: Ensure channel is selected before using the device.

On Mon, Aug 22, 2016 at 11:17:34PM +0100, Jamie Lentin wrote:
> Lift configuration command from udlfb. This appears to be essential for
> at least a Rextron VCUD-60, without which no URB communication occurs.
> 
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> ---
> udl_encoder_commit() is too late to do this set up in it seems. This
> setup doesn't need to be performed again after a suspend, although this
> is somewhat academic until I send the patch adding suspend and resume
> functions.
> 
> Tested with a Rextron VCUD-60 attached to a Thinkpad X201s on Linux 4.7.0

Applied to drm-misc, thanks.
-Daniel

> 
> Cheers,
> ---
>  drivers/gpu/drm/udl/udl_main.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 33dbfb2..29f0207 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -16,6 +16,8 @@
>  /* -BULK_SIZE as per usb-skeleton. Can we get full page and avoid overhead? */
>  #define BULK_SIZE 512
>  
> +#define NR_USB_REQUEST_CHANNEL 0x12
> +
>  #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
>  #define WRITES_IN_FLIGHT (4)
>  #define MAX_VENDOR_DESCRIPTOR_SIZE 256
> @@ -90,6 +92,26 @@ success:
>  	return true;
>  }
>  
> +/*
> + * Need to ensure a channel is selected before submitting URBs
> + */
> +static int udl_select_std_channel(struct udl_device *udl)
> +{
> +	int ret;
> +	u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
> +			    0x1C, 0x88, 0x5E, 0x15,
> +			    0x60, 0xFE, 0xC6, 0x97,
> +			    0x16, 0x3D, 0x47, 0xF2};
> +
> +	ret = usb_control_msg(udl->udev,
> +			      usb_sndctrlpipe(udl->udev, 0),
> +			      NR_USB_REQUEST_CHANNEL,
> +			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
> +			      set_def_chn, sizeof(set_def_chn),
> +			      USB_CTRL_SET_TIMEOUT);
> +	return ret < 0 ? ret : 0;
> +}
> +
>  static void udl_release_urb_work(struct work_struct *work)
>  {
>  	struct urb_node *unode = container_of(work, struct urb_node,
> @@ -301,6 +323,9 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto err;
>  	}
>  
> +	if (udl_select_std_channel(udl))
> +		DRM_ERROR("Selecting channel failed\n");
> +
>  	if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
>  		DRM_ERROR("udl_alloc_urb_list failed\n");
>  		goto err;
> -- 
> 2.8.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web