Re: [PATCH 1/2] Driver for Freescale 8610 and 5121 DIU

[Jiri Slaby <jirislaby@gmail.com>]

On 03/12/2008 10:43 PM, York Sun wrote:
> +static int fsl_diu_open(struct fb_info *info, int user)
> +{
> +	struct mfb_info *mfbi = info->par;
> +	int res = 0;
> +
> +	spin_lock(&diu_lock);
> +	mfbi->count++;
> +	if (mfbi->count == 1) {
> +		DPRINTK("open plane index %d\n", mfbi->index);
> +		fsl_diu_check_var(&info->var, info);
> +		fsl_diu_set_par(info);

Please retest your code (at least) with sleep-inside spinlock debug option. If I 
see correctly you call GFP_KERNEL allocation somewhere deeper in this function, 
which might sleep.

> +		res = fsl_diu_enable_panel(info);
> +		if (res < 0)
> +			mfbi->count--;
> +	}
> +
> +	spin_unlock(&diu_lock);
> +	return res;
> +}

> +static void __exit uninstall_fb(struct fb_info *info)
> +{
> +	struct mfb_info *mfbi = info->par;
> +
> +	DPRINTK("Entered: uninstall_fb\n");

You don't need this stuff everywhere (kprobes).

> +	if (!mfbi->registered)
> +		return;
> +
> +	unregister_framebuffer(info);
> +	unmap_video_memory(info);
> +	if (&info->cmap)
> +		fb_dealloc_cmap(&info->cmap);
> +
> +	mfbi->registered = 0;
> +}
[...]
> +static int fsl_diu_probe(struct platform_device *pdev)
> +{
> +	struct mfb_info *mfbi;
> +	unsigned long dummy_ad_addr;
> +	int ret, i, error = 0;
> +
> +	DPRINTK("Entered: fsl_diu_probe\n");
> +
> +	/* Area descriptor memory pool aligns to 64-bit boundary */
> +	allocate_buf(&pool.ad, sizeof(struct diu_ad) * FSL_AOI_NUM, 8);
> +
> +	/* Get memory for Gamma Table  - 32-byte aligned memory */
> +	allocate_buf(&pool.gamma, 768, 32);
> +
> +	/* For performance, cursor bitmap buffer aligns to 32-byte boundary */
> +	allocate_buf(&pool.cursor, MAX_CURS * MAX_CURS * 2, 32);
> +
> +	i = sizeof(fsl_diu_info) / sizeof(struct fb_info);
> +	dummy_ad = (struct diu_ad *)((u32)pool.ad.vaddr + pool.ad.offset) + i;
> +	dummy_ad->paddr = pool.ad.paddr + i * sizeof(struct diu_ad);
> +	dummy_aoi_virt = fsl_diu_alloc(64, &dummy_ad_addr);
> +	dummy_ad->addr = cpu_to_le32(dummy_ad_addr);
> +	dummy_ad->pix_fmt = 0x88882317;
> +	dummy_ad->src_size_g_alpha = cpu_to_le32((4 << 12) | 4);
> +	dummy_ad->aoi_size = cpu_to_le32((4 << 16) |  2);
> +	dummy_ad->offset_xyi = 0;
> +	dummy_ad->offset_xyd = 0;
> +	dummy_ad->next_ad = 0;
> +	memset(dummy_aoi_virt, 0x00, 64);
> +	write_reg(&(dr.diu_reg->desc[0]), dummy_ad->paddr);
> +	write_reg(&(dr.diu_reg->desc[1]), dummy_ad->paddr);
> +	write_reg(&(dr.diu_reg->desc[2]), dummy_ad->paddr);
> +
> +	for (i = 0; i < sizeof(fsl_diu_info) / sizeof(struct fb_info); i++) {
> +		fsl_diu_info[i].fix.smem_start = 0;
> +		mfbi = fsl_diu_info[i].par;
> +		mfbi->ad = (struct diu_ad *)((u32)pool.ad.vaddr
> +					+ pool.ad.offset) + i;
> +		mfbi->ad->paddr = pool.ad.paddr + i * sizeof(struct diu_ad);
> +		ret = install_fb(&fsl_diu_info[i], pdev);
> +		if (ret) {
> +			printk(KERN_ERR "Failed to register framebuffer %d\n",
> +					i);
> +			return ret;

some kind of free here

> +		}
> +	}
[...]
> +int __init fsl_diu_init(void)
> +{
> +	struct device_node *np;
> +	struct resource r;
> +	int error;
> +	DPRINTK("Entered: fsl_diu_init\n");
> +	np = of_find_compatible_node(NULL, NULL, "fsl-diu");
> +	if (!np) {
> +		printk(KERN_ERR "Err: can't find device node 'fsl-diu'\n");
> +		return -ENODEV;
> +	}
> +
> +	of_address_to_resource(np, 0, &r);
> +	of_node_put(np);
> +
> +	DPRINTK("%s, r.start: 0x%08x\n", __func__, r.start);
> +
> +	dr.diu_reg = ioremap(r.start, sizeof(struct diu));

The arch never fails with remapping?

> +
> +	write_reg(&(dr.diu_reg->diu_mode), 0);		/* disable DIU anyway*/
> +
> +	spin_lock_init(&dr.reg_lock);
> +
> +
> +	/*
> +	 * For kernel boot options (in 'video=xxxfb:<options>' format)
> +	 */
> +#ifndef MODULE
> +	{
> +		char *option;
> +
> +		if (fb_get_options("fslfb", &option))
> +			return -ENODEV;
> +		fsl_diu_setup(option);
> +	}
> +#endif
> +	error = platform_driver_register(&fsl_diu_driver);
> +
> +	if (!error) {
> +		error = platform_device_register(&fsl_diu_device);
> +		if (error) {
> +			printk(KERN_ERR "Err: "
> +					"can't register FB device driver!\n");
> +			platform_driver_unregister(&fsl_diu_driver);

iounmap

> +		}
> +		printk(KERN_INFO "FSL_DIU_FB: registed FB device driver!\n");
> +	}

else iounmap

> +
> +	return error;
> +}
[...]
> diff --git a/drivers/video/fsl-diu-fb.h b/drivers/video/fsl-diu-fb.h
> new file mode 100644
> index 0000000..294d0bf
> --- /dev/null
> +++ b/drivers/video/fsl-diu-fb.h
> @@ -0,0 +1,388 @@
[...]
> +#ifndef __FSL_DIU_FB_H__
> +#define __FSL_DIU_FB_H__
> +
> +/* FIXME: This should be changed to dev_dbg this will be done as soon as
> + * we can obtain dev through the dts setup

then use at least pr_debug() here:

> + */
> +#ifdef DEBUG
> +#define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __func__, ## args)
> +#else
> +#define DPRINTK(fmt, args...)
> +#endif

regards,
-- 
Jiri Slaby
Faculty of Informatics, Masaryk University
Suse Labs