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