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.ha...@ti.com>
> Signed-off-by: Muralidharan Karicheri <m-kariche...@ti.com>
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
