Re: [PATCH v16 01/13] davinci vpbe: V4L2 display driver for DM644X SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Manjunath,

On Tuesday 26 April 2011 16:47:45 Hadli, Manjunath wrote:
> Laurent,
>   Can you please review the patches with your suggestions from :
> http://git.linuxtv.org/mhadli/v4l-dvb-davinci_devices.git?a=shortlog;h=refs
> /heads/forkhilman2 and let me know if you think all your suggestions are
> taken care of?
> 
> The patch you reviewed was :
> 
> http://git.linuxtv.org/mhadli/v4l-dvb-davinci_devices.git?a=commitdiff;h=69
> f60ed7577ab9184ceabd7efbe5bb3453bf7ef1;hp=a400604f47c339831880c50eda6f6b032
> 21579e3

I've reviewed the same patch, here are my comments.

> +/*
> + * vpbe_display_isr()
> + * ISR function. It changes status of the displayed buffer, takes next 
buffer
> + * from the queue and sets its address in VPBE registers
> + */
> +static void vpbe_display_isr(unsigned int event, struct vpbe_display 
*disp_obj)
> +{
> +	struct osd_state *osd_device = disp_obj->osd_device;
> +	struct timespec timevalue;
> +	struct vpbe_layer *layer;
> +	unsigned long addr;
> +	int fid;
> +	int i;
> +
> +	ktime_get_ts(&timevalue);
> +
> +	for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> +		layer = disp_obj->dev[i];
> +		/* If streaming is started in this layer */
> +		if (!layer->started)
> +			continue;

What about moving everything above to venc_isr(), and having this function
handle a single layer only ? It will lower the max indentation level. I also
wonder whether you couldn't share some code between the non-interlaced and
the interlaced cases by reorganizing the function body (the fid == 1 code
looks quite similar to the non-interlaced code).

[snip]

> +/**
> + * vpbe_try_format()
> + * If user application provides width and height, and have bytesperline set
> + * to zero, driver calculates bytesperline and sizeimage based on hardware
> + * limits. If application likes to add pads at the end of each line and
> + * end of the buffer , it can set bytesperline to line size and sizeimage 
to
> + * bytesperline * height of the buffer. If driver fills zero for active
> + * video width and height, and has requested user bytesperline and 
sizeimage,
> + * width and height is adjusted to maximum display limit or buffer width
> + * height which ever is lower

This still sounds a bit cryptic to me.

vpbe_try_format() should return a format closest to what the user requested:

- If the pixel format is invalid, select a default value (done)
- If the field is invalid or not specified, select a default value (partly
  done, you don't check for default values)
- If width and/or height are invalid (including being set to 0), select
  default values (partly done, you compute width/height based on bytesperline
  and sizeimage when they're set to 0, and I don't understand why)
- If bytesperline is invalid (smaller than the minimum value according to the
  selected width, or larger than the maximum allowable value), fix it
- If sizeimage is invalid (smaller than the minimum value according the the
  selected height and bytesperline), fix it

Is there a need to allow sizeimage values different than
height * bytesperline ?

> + */

[snip]

> +static int vpbe_display_querycap(struct file *file, void  *priv,
> +			       struct v4l2_capability *cap)
> +{
> +	struct vpbe_fh *fh = file->private_data;
> +	struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +
> +	cap->version = VPBE_DISPLAY_VERSION_CODE;
> +	cap->capabilities = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> +	strlcpy(cap->driver, VPBE_DISPLAY_DRIVER, sizeof(cap->driver));
> +	strlcpy(cap->bus_info, "platform", sizeof(cap->bus_info));
> +	/* check the name of davinci device */
> +	if (vpbe_dev->cfg->module_name != NULL)

module_name can't be NULL, as it's declared as a char[32].

> +		strlcpy(cap->card, vpbe_dev->cfg->module_name,
> +			sizeof(cap->card));
> +
> +	return 0;
> +}

[snip]

> +static int vpbe_display_g_fmt(struct file *file, void *priv,
> +				struct v4l2_format *fmt)
> +{
> +	struct vpbe_fh *fh = file->private_data;
> +	struct vpbe_layer *layer = fh->layer;
> +	struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> +			"VIDIOC_G_FMT, layer id = %d\n",
> +			layer->device_id);
> +
> +	/* If buffer type is video output */
> +	if (V4L2_BUF_TYPE_VIDEO_OUTPUT == fmt->type) {
> +		/* Fill in the information about format */
> +		fmt->fmt.pix = layer->pix_fmt;
> +	} else {
> +		v4l2_err(&vpbe_dev->v4l2_dev, "invalid type\n");
> +		return -EINVAL;
> +	}

You should do it the other way around. Return -EINVAL when the type isn't
OUTPUT, and remove the else. This will increase code readability by decreasing
the max indentation level in the common case.

> +
> +	return 0;
> +}

[snip]

> +/**
> + * vpbe_display_enum_output - enumerate outputs
> + *
> + * Enumerates the outputs available at the vpbe display
> + * returns the status, -EINVAL if end of output list
> + */
> +static int vpbe_display_enum_output(struct file *file, void *priv,
> +				    struct v4l2_output *output)
> +{
> +	struct vpbe_fh *fh = priv;
> +	struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +	int ret;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,	"VIDIOC_ENUM_OUTPUT\n");
> +
> +	/* Enumerate outputs */
> +
> +	if (NULL != vpbe_dev->ops.enum_outputs) {
> +		ret = vpbe_dev->ops.enum_outputs(vpbe_dev, output);
> +		if (ret) {
> +			v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> +				"Failed to enumerate outputs\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}

Other way around here too please.

> +
> +	return 0;
> +}
> +
> +/**
> + * vpbe_display_s_output - Set output to
> + * the output specified by the index
> + */
> +static int vpbe_display_s_output(struct file *file, void *priv,
> +				unsigned int i)
> +{
> +	struct vpbe_fh *fh = priv;
> +	struct vpbe_layer *layer = fh->layer;
> +	struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +	int ret;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,	"VIDIOC_S_OUTPUT\n");
> +	/* If streaming is started, return error */
> +	if (layer->started) {
> +		v4l2_err(&vpbe_dev->v4l2_dev, "Streaming is started\n");
> +		return -EBUSY;
> +	}
> +	if (NULL != vpbe_dev->ops.set_output) {
> +		ret = vpbe_dev->ops.set_output(vpbe_dev, i);
> +		if (ret) {
> +			v4l2_err(&vpbe_dev->v4l2_dev,
> +				"Failed to set output for sub devices\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}

And here too.

> +
> +	return 0;
> +}

[snip]

> +static __devinit int init_vpbe_layer(int i, struct vpbe_display *disp_dev,
> +				     struct platform_device *pdev)
> +{
> +	struct vpbe_layer *vpbe_display_layer = NULL;
> +	struct video_device *vbd = NULL;
> +	int k;
> +	int err;
> +
> +	/* Allocate memory for four plane display objects */
> +
> +	disp_dev->dev[i] =
> +		kmalloc(sizeof(struct vpbe_layer), GFP_KERNEL);

You can use kzalloc() and avoid several initializations to 0 below.

> +
> +	/* If memory allocation fails, return error */
> +	if (!disp_dev->dev[i]) {
> +		printk(KERN_ERR "ran out of memory\n");
> +		err = -ENOMEM;
> +		goto free_mem;
> +	}
> +	spin_lock_init(&disp_dev->dev[i]->irqlock);
> +	mutex_init(&disp_dev->dev[i]->opslock);
> +
> +	/* Get the pointer to the layer object */
> +	vpbe_display_layer = disp_dev->dev[i];
> +	/* Allocate memory for video device */
> +	vbd = video_device_alloc();

There's no need to allocate the device dynamically, you can embed struct
video_device into struct vpbe_layer (i.e. replace struct video_device
*video_dev with struct video_device video_dev inside struct vpbe_layer)

(and feel free to rename video_dev to something shorter if needed)

> +	if (vbd == NULL) {
> +		v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> +				"ran out of memory\n");
> +		err = -ENOMEM;
> +		goto free_mem;
> +	}
> +	/* Initialize field of video device */
> +	vbd->release	= video_device_release;

You should then use video_device_release_empty instead of
video_device_release.

> +	vbd->fops	= &vpbe_fops;
> +	vbd->ioctl_ops	= &vpbe_ioctl_ops;
> +	vbd->minor	= -1;
> +	vbd->v4l2_dev   = &disp_dev->vpbe_dev->v4l2_dev;
> +	vbd->lock	= &vpbe_display_layer->opslock;
> +
> +	if (disp_dev->vpbe_dev->current_timings.timings_type &
> +			VPBE_ENC_STD) {
> +		vbd->tvnorms = (V4L2_STD_525_60 | V4L2_STD_625_50);
> +		vbd->current_norm =
> +			disp_dev->vpbe_dev->
> +			current_timings.timings.std_id;
> +	} else
> +		vbd->current_norm = 0;
> +
> +	snprintf(vbd->name, sizeof(vbd->name),
> +			"DaVinci_VPBE Display_DRIVER_V%d.%d.%d",
> +			(VPBE_DISPLAY_VERSION_CODE >> 16) & 0xff,
> +			(VPBE_DISPLAY_VERSION_CODE >> 8) & 0xff,
> +			(VPBE_DISPLAY_VERSION_CODE) & 0xff);
> +
> +	/* Set video_dev to the video device */
> +	vpbe_display_layer->video_dev = vbd;
> +	vpbe_display_layer->device_id = i;
> +
> +	vpbe_display_layer->layer_info.id =
> +		((i == VPBE_DISPLAY_DEVICE_0) ? WIN_VID0 : WIN_VID1);
> +
> +	/* Initialize field of the display layer objects */
> +	vpbe_display_layer->usrs = 0;
> +	vpbe_display_layer->io_usrs = 0;
> +	vpbe_display_layer->started = 0;
> +
> +	/* Initialize prio member of layer object */
> +	v4l2_prio_init(&vpbe_display_layer->prio);
> +
> +	return 0;
> +
> +free_mem:
> +	for (k = 0; k < i-1; k++) {
> +		/* Get the pointer to the layer object */
> +		vpbe_display_layer = disp_dev->dev[k];
> +		/* Release video device */
> +		video_device_release(vpbe_display_layer->video_dev);
> +		vpbe_display_layer->video_dev = NULL;
> +		/* free layer memory */
> +		kfree(disp_dev->dev[k]);
> +	}

This should be moved to the error cleanup part of vpbe_display_probe(). A
function that registers a single device shouldn't clean other devices up in
case of error.

> +
> +	return -ENODEV;
> +}
> +
> +static __devinit int register_devices(int i, struct vpbe_display *disp_dev,
> +				      struct platform_device *pdev) {

This registers a single device, so I would call it vpbe_register_device.

> +	struct vpbe_layer *vpbe_display_layer = NULL;
> +	int err;
> +	int k;
> +
> +	vpbe_display_layer = disp_dev->dev[i];

Please pass disp_dev->dev[i] to the function instead of i.

> +	v4l2_info(&disp_dev->vpbe_dev->v4l2_dev,
> +		  "Trying to register VPBE display device.\n");
> +	v4l2_info(&disp_dev->vpbe_dev->v4l2_dev,
> +		  "layer=%x,layer->video_dev=%x\n",
> +		  (int)vpbe_display_layer,
> +		  (int)&vpbe_display_layer->video_dev);
> +
> +	err = video_register_device(vpbe_display_layer->video_dev,
> +				    VFL_TYPE_GRABBER,
> +				    -1);
> +	if (err)
> +		goto video_register_failed;
> +
> +	vpbe_display_layer->disp_dev = disp_dev;
> +	/* set the driver data in platform device */
> +	platform_set_drvdata(pdev, disp_dev);
> +	video_set_drvdata(vpbe_display_layer->video_dev,
> +			  vpbe_display_layer);
> +
> +	return 0;
> +
> +video_register_failed:
> +	for (k = 0; k < i-1; k++)
> +		video_unregister_device(vpbe_display_layer->video_dev);
> +
> +	for (k = 0; k < VPBE_DISPLAY_MAX_DEVICES; k++) {
> +		/* Get the pointer to the layer object */
> +		vpbe_display_layer = disp_dev->dev[k];
> +		/* Release video device */
> +		video_device_release(vpbe_display_layer->video_dev);
> +		/* Unregister video device */
> +		video_unregister_device(vpbe_display_layer->video_dev);
> +		vpbe_display_layer->video_dev = NULL;
> +		/* free layer memory */
> +		kfree(disp_dev->dev[k]);
> +	}

This should be moved to the error cleanup part of vpbe_display_probe() as
well.

> +	return -ENODEV;
> +}
> +
> +
> +
> +/*
> + * vpbe_display_probe()
> + * This function creates device entries by register itself to the V4L2 
driver
> + * and initializes fields of each layer objects
> + */
> +static __devinit int vpbe_display_probe(struct platform_device *pdev)
> +{
> +	struct vpbe_display *disp_dev;
> +	struct resource *res;
> +	int i;
> +	int err;
> +	int irq;
> +
> +	printk(KERN_DEBUG "vpbe_display_probe\n");
> +	/* Allocate memory for vpbe_display */
> +	disp_dev = kzalloc(sizeof(struct vpbe_display), GFP_KERNEL);
> +	if (!disp_dev) {
> +		printk(KERN_ERR "ran out of memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&disp_dev->dma_queue_lock);
> +	/*
> +	 * Scan all the platform devices to find the vpbe
> +	 * controller device and get the vpbe_dev object
> +	 */
> +	err = bus_for_each_dev(&platform_bus_type, NULL, disp_dev,
> +			vpbe_device_get);
> +	if (err < 0)
> +		return err;
> +	/* Initialize the vpbe display controller */
> +	if (NULL != disp_dev->vpbe_dev->ops.initialize) {
> +		err = disp_dev->vpbe_dev->ops.initialize(&pdev->dev,
> +							 disp_dev->vpbe_dev);
> +		if (err) {
> +			v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> +					"Error initing vpbe\n");
> +			err = -ENOMEM;
> +			goto probe_out;
> +		}
> +	}
> +
> +	for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> +		if (init_vpbe_layer(i, disp_dev, pdev)) {
> +			err = -ENODEV;
> +			goto probe_out;
> +		}
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res) {
> +		v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> +			 "Unable to get VENC interrupt resource\n");
> +		err = -ENODEV;
> +		goto probe_out;
> +	}
> +
> +	irq = res->start;
> +	if (request_irq(irq, venc_isr,  IRQF_DISABLED, VPBE_DISPLAY_DRIVER,
> +		disp_dev)) {
> +		v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> +				"Unable to request interrupt\n");
> +		err = -ENODEV;
> +		goto probe_out;
> +	}
> +
> +	for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> +		if (register_devices(i, disp_dev, pdev)) {
> +			err = -ENODEV;
> +			goto probe_out;
> +		}
> +	}
> +
> +	printk(KERN_DEBUG "Successfully completed the probing of vpbe v4l2 
device\n");
> +	return 0;
> +
> +probe_out:

You need to unregister the IRQ handler (and move the cleanup code from the
two previous functions here).

> +	kfree(disp_dev);
> +	return err;
> +}

[snip]

> +/* Function for module initialization and cleanup */
> +module_init(vpbe_display_init);
> +module_exit(vpbe_display_cleanup);
> +
> +MODULE_DESCRIPTION("TI DMXXX VPBE Display controller");

What about "TI DM644x/DM355/DM365" then ? DMXXX makes it look like it supports 
all DaVinci chips.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Texas Instruments");

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux