Message-ID: <20160815064328.GZ6232@phenom.ffwll.local>
Date:	Mon, 15 Aug 2016 08:43:28 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Jamie Lentin <jm@...tin.co.uk>
Cc:	David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: 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@...tin.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@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Powered by blists - more mailing lists