Re: [PATCH v3 1/6] davinci vpbe: V4L2 display driver for DM644X SoC

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

 



A few minor comments:

On Thursday, December 02, 2010 13:38:05 Manjunath Hadli wrote:
> This is the display driver for Texas Instruments's DM644X family
> SoC.This patch contains the main implementation of the driver with the
> V4L2 interface.The driver is implements the streaming model with
> support for both kernel allocated buffers and user pointers. It also
> implements all of the necessary IOCTLs necessary and supported by the
> video display device
> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx>
> Signed-off-by: Muralidharan Karicheri <m-karicheri2@xxxxxx>
> ---
>  drivers/media/video/davinci/vpbe_display.c | 2103 ++++++++++++++++++++++++++++
>  include/media/davinci/vpbe_display.h       |  146 ++
>  include/media/davinci/vpbe_types.h         |   93 ++
>  3 files changed, 2342 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/davinci/vpbe_display.c
>  create mode 100644 include/media/davinci/vpbe_display.h
>  create mode 100644 include/media/davinci/vpbe_types.h
> 
> diff --git a/drivers/media/video/davinci/vpbe_display.c b/drivers/media/video/davinci/vpbe_display.c
> new file mode 100644
> index 0000000..7a2d447
> --- /dev/null
> +++ b/drivers/media/video/davinci/vpbe_display.c

<snip>

> +static void
> +vpbe_disp_calculate_scale_factor(struct vpbe_display *disp_dev,
> +			struct vpbe_display_obj *layer,
> +			int expected_xsize, int expected_ysize)
> +{
> +	struct display_layer_info *layer_info = &layer->layer_info;
> +	struct v4l2_pix_format *pixfmt = &layer->pix_fmt;
> +	struct osd_layer_config *cfg  = &layer->layer_info.config;
> +	int h_scale = 0, v_scale = 0, h_exp = 0, v_exp = 0, temp;
> +	v4l2_std_id standard_id = vpbe_dev->current_timings.timings.std_id;

Please add an empty line here for readability.

> +	/*
> +	 * Application initially set the image format. Current display
> +	 * size is obtained from the vpbe display controller. expected_xsize
> +	 * and expected_ysize are set through S_CROP ioctl. Based on this,
> +	 * driver will calculate the scale factors for vertical and
> +	 * horizontal direction so that the image is displayed scaled
> +	 * and expanded. Application uses expansion to display the image
> +	 * in a square pixel. Otherwise it is displayed using displays
> +	 * pixel aspect ratio.It is expected that application chooses
> +	 * the crop coordinates for cropped or scaled display. if crop
> +	 * size is less than the image size, it is displayed cropped or
> +	 * it is displayed scaled and/or expanded.
> +	 *
> +	 * to begin with, set the crop window same as expected. Later we
> +	 * will override with scaled window size
> +	 */
> +	cfg->xsize = pixfmt->width;
> +	cfg->ysize = pixfmt->height;
> +	layer_info->h_zoom = ZOOM_X1;	/* no horizontal zoom */
> +	layer_info->v_zoom = ZOOM_X1;	/* no horizontal zoom */
> +	layer_info->h_exp = H_EXP_OFF;	/* no horizontal zoom */
> +	layer_info->v_exp = V_EXP_OFF;	/* no horizontal zoom */
> +
> +	if (pixfmt->width < expected_xsize) {
> +		h_scale = vpbe_dev->current_timings.xres / pixfmt->width;
> +		if (h_scale < 2)
> +			h_scale = 1;
> +		else if (h_scale >= 4)
> +			h_scale = 4;
> +		else
> +			h_scale = 2;
> +		cfg->xsize *= h_scale;
> +		if (cfg->xsize < expected_xsize) {
> +			if ((standard_id == V4L2_STD_525_60) ||
> +			(standard_id == V4L2_STD_625_50)) {

Shouldn't this be '&' instead of '=='? Type v4l2_std_id is a bitmask, after all.

It's a good idea to check other occurences of this.

> +				temp = (cfg->xsize *
> +					VPBE_DISPLAY_H_EXP_RATIO_N) /
> +					VPBE_DISPLAY_H_EXP_RATIO_D;
> +				if (temp <= expected_xsize) {
> +					h_exp = 1;
> +					cfg->xsize = temp;
> +				}
> +			}
> +		}
> +		if (h_scale == 2)
> +			layer_info->h_zoom = ZOOM_X2;
> +		else if (h_scale == 4)
> +			layer_info->h_zoom = ZOOM_X4;
> +		if (h_exp)
> +			layer_info->h_exp = H_EXP_9_OVER_8;
> +	} else {
> +		/* no scaling, only cropping. Set display area to crop area */
> +		cfg->xsize = expected_xsize;
> +	}
> +
> +	if (pixfmt->height < expected_ysize) {
> +		v_scale = expected_ysize / pixfmt->height;
> +		if (v_scale < 2)
> +			v_scale = 1;
> +		else if (v_scale >= 4)
> +			v_scale = 4;
> +		else
> +			v_scale = 2;
> +		cfg->ysize *= v_scale;
> +		if (cfg->ysize < expected_ysize) {
> +			if ((standard_id == V4L2_STD_625_50)) {

Should be '&'.

> +				temp = (cfg->ysize *
> +					VPBE_DISPLAY_V_EXP_RATIO_N) /
> +					VPBE_DISPLAY_V_EXP_RATIO_D;
> +				if (temp <= expected_ysize) {
> +					v_exp = 1;
> +					cfg->ysize = temp;
> +				}
> +			}
> +		}
> +		if (v_scale == 2)
> +			layer_info->v_zoom = ZOOM_X2;
> +		else if (v_scale == 4)
> +			layer_info->v_zoom = ZOOM_X4;
> +		if (v_exp)
> +			layer_info->h_exp = V_EXP_6_OVER_5;
> +	} else {
> +		/* no scaling, only cropping. Set display area to crop area */
> +		cfg->ysize = expected_ysize;
> +	}
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> +		"crop display xsize = %d, ysize = %d\n",
> +		cfg->xsize, cfg->ysize);
> +}
> +
> +static void vpbe_disp_adj_position(struct vpbe_display *disp_dev,
> +			struct vpbe_display_obj *layer,
> +			int top, int left)
> +{
> +	struct osd_layer_config *cfg = &layer->layer_info.config;
> +	cfg->xpos = cfg->ypos = 0;
> +	if (left + cfg->xsize <= vpbe_dev->current_timings.xres)
> +		cfg->xpos = left;
> +	if (top + cfg->ysize <= vpbe_dev->current_timings.yres)
> +		cfg->ypos = top;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> +		"new xpos = %d, ypos = %d\n",
> +		cfg->xpos, cfg->ypos);
> +}

<snip>

> +static int vpbe_display_s_fmt(struct file *file, void *priv,
> +				struct v4l2_format *fmt)
> +{
> +	int ret = 0;
> +	struct vpbe_fh *fh = file->private_data;
> +	struct vpbe_display *disp_dev = video_drvdata(file);
> +	struct vpbe_display_obj *layer = fh->layer;
> +	struct osd_layer_config *cfg  = &layer->layer_info.config;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> +			"VIDIOC_S_FMT, layer id = %d\n",
> +			layer->device_id);
> +
> +	/* If streaming is started, return error */
> +	if (layer->started) {
> +		v4l2_err(&vpbe_dev->v4l2_dev, "Streaming is started\n");
> +		return -EBUSY;
> +	}
> +	if (V4L2_BUF_TYPE_VIDEO_OUTPUT == fmt->type) {
> +		struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
> +		/* Check for valid pixel format */
> +		ret = vpbe_try_format(disp_dev, pixfmt, 1);
> +		if (ret)
> +			return ret;
> +
> +	/* YUV420 is requested, check availability of the other video window */

The indentation of this line and the following lines seems to be off.

Instead of having one huge 'if' block followed by a small 'else' block it is
much better to handle the 'else' part first:

	if (V4L2_BUF_TYPE_VIDEO_OUTPUT != fmt->type) {
		v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "invalid type\n");
		return -EINVAL;
	}

Much more understandable.

> +
> +	layer->pix_fmt = *pixfmt;
> +
> +	/* Get osd layer config */
> +	osd_device->ops.get_layer_config(osd_device,
> +			layer->layer_info.id, cfg);
> +	/* Store the pixel format in the layer object */
> +	cfg->xsize = pixfmt->width;
> +	cfg->ysize = pixfmt->height;
> +	cfg->line_length = pixfmt->bytesperline;
> +	cfg->ypos = 0;
> +	cfg->xpos = 0;
> +	cfg->interlaced = vpbe_dev->current_timings.interlaced;
> +
> +	/* Change of the default pixel format for both video windows */
> +	if (V4L2_PIX_FMT_NV12 == pixfmt->pixelformat) {
> +		struct vpbe_display_obj *otherlayer;
> +		cfg->pixfmt = PIXFMT_NV12;
> +		otherlayer = _vpbe_display_get_other_win(disp_dev, layer);
> +		otherlayer->layer_info.config.pixfmt = PIXFMT_NV12;
> +	}
> +
> +	/* Set the layer config in the osd window */
> +	ret = osd_device->ops.set_layer_config(osd_device,
> +				layer->layer_info.id, cfg);
> +	if (ret < 0) {
> +		v4l2_err(&vpbe_dev->v4l2_dev, "Error in S_FMT params:\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Readback and fill the local copy of current pix format */
> +	osd_device->ops.get_layer_config(osd_device,
> +			layer->layer_info.id, cfg);
> +
> +	/* verify if readback values are as expected */
> +	if (layer->pix_fmt.width != cfg->xsize ||
> +		layer->pix_fmt.height != cfg->ysize ||
> +		layer->pix_fmt.bytesperline != layer->layer_info.
> +					config.line_length ||
> +		(cfg->interlaced
> +		&& layer->pix_fmt.field != V4L2_FIELD_INTERLACED) ||
> +		(!cfg->interlaced && layer->pix_fmt.field
> +					!= V4L2_FIELD_NONE)) {
> +
> +		v4l2_err(&vpbe_dev->v4l2_dev, "mismatch:layer conf params:\n");
> +		return -EINVAL;
> +	}
> +
> +	} else {
> +		v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "invalid type\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vpbe_display_try_fmt(struct file *file, void *priv,
> +				  struct v4l2_format *fmt)
> +{
> +	struct vpbe_display *disp_dev = video_drvdata(file);
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "VIDIOC_TRY_FMT\n");
> +
> +	if (V4L2_BUF_TYPE_VIDEO_OUTPUT == fmt->type) {
> +		struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
> +		/* Check for valid field format */
> +		return  vpbe_try_format(disp_dev, pixfmt, 0);
> +	} else {

'else' keyword is not needed since the 'if' will always return.

> +		v4l2_err(&vpbe_dev->v4l2_dev, "invalid type\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * vpbe_display_s_std - Set the given standard in the encoder
> + *
> + * Sets the standard if supported by the current encoder. Return the status.
> + * 0 - success & -EINVAL on error
> + */
> +static int vpbe_display_s_std(struct file *file, void *priv,
> +				v4l2_std_id *std_id)
> +{
> +	struct vpbe_fh *fh = priv;
> +	struct vpbe_display_obj *layer = fh->layer;
> +	int ret = 0;
> +
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "VIDIOC_S_STD\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.s_std) {
> +		ret = vpbe_dev->ops.s_std(vpbe_dev, std_id);
> +		if (ret) {
> +			v4l2_err(&vpbe_dev->v4l2_dev,
> +			"Failed to set standard for sub devices\n");
> +			return -EINVAL;

Shouldn't this be 'return ret;'? (or just do nothing actually).

> +		}
> +	}
> +	return ret;
> +}
> +
> +/**
> + * vpbe_display_g_std - Get the standard in the current encoder
> + *
> + * Get the standard in the current encoder. Return the status. 0 - success
> + * -EINVAL on error
> + */
> +static int vpbe_display_g_std(struct file *file, void *priv,
> +				v4l2_std_id *std_id)
> +{
> +	v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,	"VIDIOC_G_STD\n");
> +
> +	/* Get the standard from the current encoder */
> +	if (vpbe_dev->current_timings.timings_type & VPBE_ENC_STD) {
> +		*std_id = vpbe_dev->current_timings.timings.std_id;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +/**
> + * 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)
> +{
> +	int ret = 0;
> +
> +	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;

Ditto. I actually see it more often in this code.

> +		}
> +	}
> +	return ret;
> +}

<snip>


-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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