[Libdlo] [PATCH 2.6.32-rc7 1/1] udlfb: add dynamic modeset support

[Greg KH]

On Sun, Nov 15, 2009 at 07:48:01PM -0800, Bernie Thompson wrote:
> This is the udlfb 0.4.0 full kernel patch, 
> with the equivalent functionality to the standalone release.
> 
> There are some additional changes vs 0.4.0 to conform to kernel style (checkpatch.pl),
> and the copy of drm_edid.h is not included, as this targets only the latest kernel.
> Looked at separating out the ref counting/synchronization into its own patch, but it's 
> been tested for several months in this form, and is due for other rework soon (which
> of course would then be done as its own patch from the start).
> 
> The patch below is also available at http://git.plugable.com/dynamicmode.patch
> 
> Feedback welcome if there's anything amiss.
> 
> --
> 
> Add dynamic modeset support
> 
> udlfb uses EDID to find the monitor’s preferred mode
> udlfb no longer has fixed mode tables – it’s able to set any mode 
> dynamically, from the standard VESA timing characteristics of the monitor.
> Probe and disconnect also now have better synchronization.
> 
> Code ported from displaylink-mod 0.3 branch of Roberto De Ioris, with
> changes to minimize diffs and clean for checkpatch.pl style
> 
> Signed-off-by: Bernie Thompson <bernie at plugable.com>

Hm, which changelog comment above should I use?

>  
> diff -uprN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7/drivers/staging/udlfb/udlfb.c linux-2.6/drivers/staging/udlfb/udlfb.c
> --- linux-2.6.32-rc7/drivers/staging/udlfb/udlfb.c	2009-11-12 16:46:07.000000000 -0800
> +++ linux-2.6/drivers/staging/udlfb/udlfb.c	2009-11-15 12:02:36.000000000 -0800
> @@ -1,12 +1,14 @@
>  /*****************************************************************************
>   *                          DLFB Kernel Driver                               *
> - *                            Version 0.2 (udlfb)                            *
> + *                            Version 0.4 (udlfb)                            *
>   *             (C) 2009 Roberto De Ioris <roberto at unbit.it>                  *
>   *                                                                           *
>   *     This file is licensed under the GPLv2. See COPYING in the package.    *
>   * Based on the amazing work of Florian Echtler and libdlo 0.1               *
>   *                                                                           *
>   *                                                                           *
> + * 11.11.09 release 0.4 merge 0.3 work back into udlfb for kernel tree       *
> + * 24.06.09 release 0.3 as displylink-mod (resolution manager, new ioctls)   *

Don't add this to the top of the driver, that makes no sense for an
in-kernel driver.

>   * 10.06.09 release 0.2.3 (edid ioctl, fallback for unsupported modes)       *
>   * 05.06.09 release 0.2.2 (real screen blanking, rle compression, double buffer) *
>   * 31.05.09 release 0.2                                                      *
> @@ -22,10 +24,19 @@
>  #include <linux/fb.h>
>  #include <linux/mutex.h>
>  #include <linux/vmalloc.h>
> +#include <linux/version.h>
> +
> +/* Many users compile this as a loadable module on older kernels.Keep for now */
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> +#include <drm/drm_edid.h>
> +#else
> +#include "drm_edid.h"
> +#endif

Same with this, please remove this.

>  /* ioctl structure */
> +
> +struct dlores {
> +	int w, h;
> +	int freq;
> +};

No, you're kidding me, right?  An 'int' for an ioctl structure?  Are you
_trying_ to make things hard?  :)

Seriously, please fix this up now before you start to have more problems
with different architectures.  Use the __u32 and friend types for values
that pass across the kernel/user boundry in an ioctl to ensure that
everything works properly.

> +
>  struct dloarea {
>  	int x, y;
>  	int w, h;
> @@ -164,6 +181,9 @@ image_blit(struct dlfb_data *dev_info, i
>  
>  	char *bufptr;
>  
> +	if (dev_info->udev == NULL)
> +		return 0;

How can this happen?  On disconnect?  If so, then you need to have a
lock for it so you don't race.  If not, it doesn't make much sense.

And shouldn't you return -ENODEV?

> @@ -568,23 +483,42 @@ static void dlfb_copyarea(struct fb_info
>  
>  	struct dlfb_data *dev = info->par;
>  
> +	mutex_lock(&dev->fb_mutex);
> +
> +	dev = info->par;
> +
> +	if (dev->udev == NULL)
> +		return;
> +

Your lock is still held.  And again, why check this?

>  	copyarea(dev, area->dx, area->dy, area->sx, area->sy, area->width,
>  		 area->height);
>  
> +	mutex_unlock(&dev->fb_mutex);
> +
>  	/* printk("COPY AREA %d %d %d %d %d %d !!!\n", area->dx, area->dy, area->sx, area->sy, area->width, area->height); */
>  
>  }
>  
>  static void dlfb_imageblit(struct fb_info *info, const struct fb_image *image)
>  {
> -
> -	int ret;
>  	struct dlfb_data *dev = info->par;
> +
> +	mutex_lock(&dev->fb_mutex);
> +
> +	dev = info->par;
> +
> +	if (dev->udev == NULL)
> +		return;

Lock is still held.

> +
>  	/* printk("IMAGE BLIT (1) %d %d %d %d DEPTH %d {%p}!!!\n", image->dx, image->dy, image->width, image->height, image->depth, dev->udev); */
> +
>  	cfb_imageblit(info, image);
> -	ret =
> -	    image_blit(dev, image->dx, image->dy, image->width, image->height,
> +
> +	image_blit(dev, image->dx, image->dy, image->width, image->height,
>  		       info->screen_base);

this function checks for dev->udev, so why check it above as well?

And, you aren't checking the return value of the function, why?

> @@ -595,11 +529,21 @@ static void dlfb_fillrect(struct fb_info
>  	unsigned char red, green, blue;
>  	struct dlfb_data *dev = info->par;
>  
> +	mutex_lock(&dev->fb_mutex);
> +
> +	dev = info->par;
> +
> +	if (dev->udev == NULL)
> +		return;

We can continue on with this same line of review, but I think you should
fix these first :)

thanks,

greg k-h