| Message ID | alpine.DEB.2.02.1108171551040.17540@ipanema (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bastian, Thanks for the patch. On Wednesday 17 August 2011 17:53:42 Bastian Hecht wrote: > This patch adds the ability to get arbitrary resolutions with a width > up to 2592 and a height up to 720 pixels instead of the standard 1280x720 > only. > > Signed-off-by: Bastian Hecht <hechtb@gmail.com> > --- > > diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c > index 6410bda..1b40d90 100644 > --- a/drivers/media/video/ov5642.c > +++ b/drivers/media/video/ov5642.c > @@ -14,8 +14,10 @@ > * published by the Free Software Foundation. > */ > > +#include <linux/bitops.h> > #include <linux/delay.h> > #include <linux/i2c.h> > +#include <linux/kernel.h> > #include <linux/slab.h> > #include <linux/videodev2.h> > > @@ -28,13 +30,25 @@ > #define REG_CHIP_ID_HIGH 0x300a > #define REG_CHIP_ID_LOW 0x300b > > +#define REG_RED_GAIN_HIGH 0x3400 > +#define REG_RED_GAIN_LOW 0x3401 > +#define REG_BLUE_GAIN_HIGH 0x3404 > +#define REG_BLUE_GAIN_LOW 0x3405 > +#define REG_AWB_MANUAL 0x3406 > +#define REG_EXP_HIGH 0x3500 > +#define REG_EXP_MIDDLE 0x3501 > +#define REG_EXP_LOW 0x3502 > +#define REG_EXP_GAIN_CTRL 0x3503 > +#define REG_GAIN 0x350b This belongs to the second patch. > +#define REG_EXTEND_FRAME_TIME_HIGH 0x350c > +#define REG_EXTEND_FRAME_TIME_LOW 0x350d > #define REG_WINDOW_START_X_HIGH 0x3800 > #define REG_WINDOW_START_X_LOW 0x3801 > #define REG_WINDOW_START_Y_HIGH 0x3802 > #define REG_WINDOW_START_Y_LOW 0x3803 > #define REG_WINDOW_WIDTH_HIGH 0x3804 > #define REG_WINDOW_WIDTH_LOW 0x3805 > -#define REG_WINDOW_HEIGHT_HIGH 0x3806 > +#define REG_WINDOW_HEIGHT_HIGH 0x3806 > #define REG_WINDOW_HEIGHT_LOW 0x3807 > #define REG_OUT_WIDTH_HIGH 0x3808 > #define REG_OUT_WIDTH_LOW 0x3809 > @@ -44,19 +58,56 @@ > #define REG_OUT_TOTAL_WIDTH_LOW 0x380d > #define REG_OUT_TOTAL_HEIGHT_HIGH 0x380e > #define REG_OUT_TOTAL_HEIGHT_LOW 0x380f > +#define REG_FLIP_SUBSAMPLE 0x3818 > +#define REG_OUTPUT_FORMAT 0x4300 > +#define REG_ISP_CTRL_01 0x5001 > +#define REG_DIGITAL_EFFECTS 0x5580 > +#define REG_HUE_COS 0x5581 > +#define REG_HUE_SIN 0x5582 > +#define REG_BLUE_SATURATION 0x5583 > +#define REG_RED_SATURATION 0x5584 > +#define REG_CONTRAST 0x5588 > +#define REG_BRIGHTNESS 0x5589 > +#define REG_D_E_AUXILLARY 0x558a > +#define REG_AVG_WINDOW_END_X_HIGH 0x5682 > +#define REG_AVG_WINDOW_END_X_LOW 0x5683 > +#define REG_AVG_WINDOW_END_Y_HIGH 0x5686 > +#define REG_AVG_WINDOW_END_Y_LOW 0x5687 So does this. > + > +/* active pixel array size */ > +#define OV5642_SENSOR_SIZE_X 2592 > +#define OV5642_SENSOR_SIZE_Y 1944 > + > +/* current maximum working size */ > +#define OV5642_MAX_WIDTH OV5642_SENSOR_SIZE_X > +#define OV5642_MAX_HEIGHT 720 > + > +/* default sizes */ > +#define OV5642_DEFAULT_WIDTH 1280 > +#define OV5642_DEFAULT_HEIGHT OV5642_MAX_HEIGHT > + > +/* minimum extra blanking */ > +#define BLANKING_EXTRA_WIDTH 500 > +#define BLANKING_EXTRA_HEIGHT 20 > > /* > - * define standard resolution. > - * Works currently only for up to 720 lines > - * eg. 320x240, 640x480, 800x600, 1280x720, 2048x720 > + * the sensor's autoexposure is buggy when setting total_height low. > + * It tries to expose longer than 1 frame period without taking care of it > + * and this leads to weird output. So we set 1000 lines as minimum. > */ > > -#define OV5642_WIDTH 1280 > -#define OV5642_HEIGHT 720 > -#define OV5642_TOTAL_WIDTH 3200 > -#define OV5642_TOTAL_HEIGHT 2000 > -#define OV5642_SENSOR_SIZE_X 2592 > -#define OV5642_SENSOR_SIZE_Y 1944 > +#define BLANKING_MIN_HEIGHT 1000 > + > +/* > + * About OV5642 resolution, cropping and binning: > + * This sensor supports it all, at least in the feature description. > + * Unfortunately, no combination of appropriate registers settings could > make + * the chip work the intended way. As it works with predefined > register lists, + * some undocumented registers are presumably changed > there to achieve their + * goals. > + * This driver currently only works for resolutions up to 720 lines with a > + * 1:1 scale. Hopefully these restrictions will be removed in the future. > + */ Can you please move this comment above the OV5642_MAX_WIDTH and OV5642_MAX_HEIGHT definitions ? > struct regval_list { > u16 reg_num; > @@ -105,10 +156,8 @@ static struct regval_list ov5642_default_regs_init[] = > { { 0x471d, 0x5 }, > { 0x4708, 0x6 }, > { 0x370c, 0xa0 }, > - { 0x5687, 0x94 }, Unless I'm mistaken, this register value is removed and isn't replaced by anything in this patch or the next one. Is that intentional ? > { 0x501f, 0x0 }, > { 0x5000, 0x4f }, > - { 0x5001, 0xcf }, This one is replaced by 0xff. I'm not sure how that's related to this patch. Could you please check all modifications to the ov5642_default_regs_init[] and ov5642_default_regs_finalise[] arrays ? Some probably need to move to the next patch, and others don't seem to be related to any of the two patches. > { 0x4300, 0x30 }, > { 0x4300, 0x30 }, > { 0x460b, 0x35 }, > @@ -121,11 +170,8 @@ static struct regval_list ov5642_default_regs_init[] = > { { 0x4402, 0x90 }, > { 0x460c, 0x22 }, > { 0x3815, 0x44 }, > - { 0x3503, 0x7 }, > { 0x3501, 0x73 }, > { 0x3502, 0x80 }, > - { 0x350b, 0x0 }, > - { 0x3818, 0xc8 }, > { 0x3824, 0x11 }, > { 0x3a00, 0x78 }, > { 0x3a1a, 0x4 }, > @@ -140,12 +186,6 @@ static struct regval_list ov5642_default_regs_init[] = > { { 0x350d, 0xd0 }, > { 0x3a0d, 0x8 }, > { 0x3a0e, 0x6 }, > - { 0x3500, 0x0 }, > - { 0x3501, 0x0 }, > - { 0x3502, 0x0 }, > - { 0x350a, 0x0 }, > - { 0x350b, 0x0 }, > - { 0x3503, 0x0 }, > { 0x3a0f, 0x3c }, > { 0x3a10, 0x32 }, > { 0x3a1b, 0x3c }, > @@ -298,7 +338,7 @@ static struct regval_list ov5642_default_regs_init[] = > { { 0x54b7, 0xdf }, > { 0x5402, 0x3f }, > { 0x5403, 0x0 }, > - { 0x3406, 0x0 }, > + { REG_AWB_MANUAL, 0x0 }, > { 0x5180, 0xff }, > { 0x5181, 0x52 }, > { 0x5182, 0x11 }, > @@ -515,7 +555,6 @@ static struct regval_list ov5642_default_regs_init[] = > { { 0x5088, 0x0 }, > { 0x5089, 0x0 }, > { 0x302b, 0x0 }, > - { 0x3503, 0x7 }, > { 0x3011, 0x8 }, > { 0x350c, 0x2 }, > { 0x350d, 0xe4 }, > @@ -526,7 +565,6 @@ static struct regval_list ov5642_default_regs_init[] = > { > > static struct regval_list ov5642_default_regs_finalise[] = { > { 0x3810, 0xc2 }, > - { 0x3818, 0xc9 }, > { 0x381c, 0x10 }, > { 0x381d, 0xa0 }, > { 0x381e, 0x5 }, > @@ -541,23 +579,20 @@ static struct regval_list > ov5642_default_regs_finalise[] = { { 0x3a0d, 0x2 }, > { 0x3a0e, 0x1 }, > { 0x401c, 0x4 }, > - { 0x5682, 0x5 }, > - { 0x5683, 0x0 }, > - { 0x5686, 0x2 }, > - { 0x5687, 0xcc }, > - { 0x5001, 0x4f }, > + { REG_ISP_CTRL_01, 0xff }, > + { REG_DIGITAL_EFFECTS, 0x6 }, > { 0x589b, 0x6 }, > { 0x589a, 0xc5 }, > - { 0x3503, 0x0 }, > + { REG_EXP_GAIN_CTRL, 0x0 }, > { 0x460c, 0x20 }, > { 0x460b, 0x37 }, > { 0x471c, 0xd0 }, > { 0x471d, 0x5 }, > { 0x3815, 0x1 }, > - { 0x3818, 0xc1 }, > + { REG_FLIP_SUBSAMPLE, 0xc1 }, > { 0x501f, 0x0 }, > { 0x5002, 0xe0 }, > - { 0x4300, 0x32 }, /* UYVY */ > + { REG_OUTPUT_FORMAT, 0x32 }, > { 0x3002, 0x1c }, > { 0x4800, 0x14 }, > { 0x4801, 0xf }, > @@ -578,9 +613,20 @@ struct ov5642_datafmt { > enum v4l2_colorspace colorspace; > }; > > +/* the output resolution and blanking information */ > +struct ov5642_out_size { > + int width; > + int height; > + int total_width; > + int total_height; > +}; > + > struct ov5642 { > struct v4l2_subdev subdev; > + > const struct ov5642_datafmt *fmt; > + struct v4l2_rect crop_rect; > + struct ov5642_out_size out_size; > }; > > static const struct ov5642_datafmt ov5642_colour_fmts[] = { > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client > *client) } > > /* Find a data format by a pixel code in an array */ > -static const struct ov5642_datafmt > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > v4l2_mbus_pixelcode code) { checkpatch.pl won't be happy. > int i; > > @@ -641,6 +686,26 @@ static int reg_write(struct i2c_client *client, u16 > reg, u8 val) > > return 0; > } > + > +/* > + * convenience function to write 16 bit register values that are split up > + * into two consecutive high and low parts > + */ > +static int reg_write16(struct i2c_client *client, u16 reg, u16 val16) > +{ > + int ret; > + u8 val8; > + > + val8 = val16 >> 8; > + ret = reg_write(client, reg, val8); You can use val16 >> 8 directly as a function argument and get rid of the val8 variable. > + if (ret) > + return ret; > + val8 = val16 & 0x00ff; > + ret = reg_write(client, reg + 1, val8); > + > + return ret; return reg_write(...) should be fine. > +} > + > #ifdef CONFIG_VIDEO_ADV_DEBUG > static int ov5642_get_register(struct v4l2_subdev *sd, struct > v4l2_dbg_register *reg) { > @@ -684,68 +749,72 @@ static int ov5642_write_array(struct i2c_client > *client, return 0; > } > > -static int ov5642_set_resolution(struct i2c_client *client) > +static int ov5642_set_resolution(struct v4l2_subdev *sd) > { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct ov5642 *priv = to_ov5642(client); > + int width = priv->out_size.width; > + int height = priv->out_size.height; > + int total_width = priv->out_size.total_width; > + int total_height = priv->out_size.total_height; > + int start_x = (OV5642_SENSOR_SIZE_X - width) / 2; > + int start_y = (OV5642_SENSOR_SIZE_Y - height) / 2; > int ret; > - u8 start_x_high = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) >> 8; > - u8 start_x_low = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) & 0xff; > - u8 start_y_high = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) >> 8; > - u8 start_y_low = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) & 0xff; > - > - u8 width_high = OV5642_WIDTH >> 8; > - u8 width_low = OV5642_WIDTH & 0xff; > - u8 height_high = OV5642_HEIGHT >> 8; > - u8 height_low = OV5642_HEIGHT & 0xff; > - > - u8 total_width_high = OV5642_TOTAL_WIDTH >> 8; > - u8 total_width_low = OV5642_TOTAL_WIDTH & 0xff; > - u8 total_height_high = OV5642_TOTAL_HEIGHT >> 8; > - u8 total_height_low = OV5642_TOTAL_HEIGHT & 0xff; > - > - ret = reg_write(client, REG_WINDOW_START_X_HIGH, start_x_high); > - if (!ret) > - ret = reg_write(client, REG_WINDOW_START_X_LOW, start_x_low); > - if (!ret) > - ret = reg_write(client, REG_WINDOW_START_Y_HIGH, start_y_high); > - if (!ret) > - ret = reg_write(client, REG_WINDOW_START_Y_LOW, start_y_low); > > + /* This should set the starting point for cropping. Doesn't work so far. > */ + ret = reg_write16(client, REG_WINDOW_START_X_HIGH, start_x); > if (!ret) > - ret = reg_write(client, REG_WINDOW_WIDTH_HIGH, width_high); > - if (!ret) > - ret = reg_write(client, REG_WINDOW_WIDTH_LOW , width_low); > + ret = reg_write16(client, REG_WINDOW_START_Y_HIGH, start_y); > + if (!ret) { > + priv->crop_rect.left = start_x; > + priv->crop_rect.top = start_y; > + } > + > if (!ret) > - ret = reg_write(client, REG_WINDOW_HEIGHT_HIGH, height_high); > + ret = reg_write16(client, REG_WINDOW_WIDTH_HIGH, width); > if (!ret) > - ret = reg_write(client, REG_WINDOW_HEIGHT_LOW, height_low); > + ret = reg_write16(client, REG_WINDOW_HEIGHT_HIGH, height); > + if (ret) > + return ret; > + priv->crop_rect.width = width; > + priv->crop_rect.height = height; > > + /* Set the output window size. Only 1:1 scale is supported so far. */ > + ret = reg_write16(client, REG_OUT_WIDTH_HIGH, width); > if (!ret) > - ret = reg_write(client, REG_OUT_WIDTH_HIGH, width_high); > - if (!ret) > - ret = reg_write(client, REG_OUT_WIDTH_LOW , width_low); > + ret = reg_write16(client, REG_OUT_HEIGHT_HIGH, height); > + > + /* Total width = output size + blanking */ > if (!ret) > - ret = reg_write(client, REG_OUT_HEIGHT_HIGH, height_high); > + ret = reg_write16(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width); > if (!ret) > - ret = reg_write(client, REG_OUT_HEIGHT_LOW, height_low); > + ret = reg_write16(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height); > > + /* set the maximum integration time */ > if (!ret) > - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width_high); > - if (!ret) > - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_LOW, total_width_low); > + ret = reg_write16(client, REG_EXTEND_FRAME_TIME_HIGH, > + total_height); > + > + /* Sets the window for AWB calculations */ > if (!ret) > - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height_high); > + ret = reg_write16(client, REG_AVG_WINDOW_END_X_HIGH, width); > if (!ret) > - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_LOW, total_height_low); > + ret = reg_write16(client, REG_AVG_WINDOW_END_Y_HIGH, height); > > return ret; > } > > -static int ov5642_try_fmt(struct v4l2_subdev *sd, > - struct v4l2_mbus_framefmt *mf) > +static int ov5642_try_fmt(struct v4l2_subdev *sd, struct > v4l2_mbus_framefmt *mf) { > - const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); > + const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); > > - dev_dbg(sd->v4l2_dev->dev, "%s(%u) width: %u heigth: %u\n", > + dev_dbg(sd->v4l2_dev->dev, "%s(%u) request width: %u heigth: %u\n", > + __func__, mf->code, mf->width, mf->height); > + > + v4l_bound_align_image(&mf->width, 48, OV5642_MAX_WIDTH, 1, > + &mf->height, 32, OV5642_MAX_HEIGHT, 1, 0); > + > + dev_dbg(sd->v4l2_dev->dev, "%s(%u) return width: %u heigth: %u\n", > __func__, mf->code, mf->width, mf->height); > > if (!fmt) { > @@ -753,20 +822,16 @@ static int ov5642_try_fmt(struct v4l2_subdev *sd, > mf->colorspace = ov5642_colour_fmts[0].colorspace; > } > > - mf->width = OV5642_WIDTH; > - mf->height = OV5642_HEIGHT; > mf->field = V4L2_FIELD_NONE; > > return 0; > } > > -static int ov5642_s_fmt(struct v4l2_subdev *sd, > - struct v4l2_mbus_framefmt *mf) > +static int ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt > *mf) { > struct i2c_client *client = v4l2_get_subdevdata(sd); > struct ov5642 *priv = to_ov5642(client); > - > - dev_dbg(sd->v4l2_dev->dev, "%s(%u)\n", __func__, mf->code); > + int ret; > > /* MIPI CSI could have changed the format, double-check */ > if (!ov5642_find_datafmt(mf->code)) > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, > > ov5642_try_fmt(sd, mf); > > + priv->out_size.width = mf->width; > + priv->out_size.height = mf->height; It looks like to me (but I may be wrong) that you achieve different resolutions using cropping, not scaling. If that's correct you should implement s_crop support and refuse changing the resolution through s_fmt. > + priv->out_size.total_width = mf->width + BLANKING_EXTRA_WIDTH; > + priv->out_size.total_height = max_t(int, mf->height + > + BLANKING_EXTRA_HEIGHT, > + BLANKING_MIN_HEIGHT); As you only use those two values once, maybe you can compute them in ov5642_set_resolution() instead of storing them in the device data structure. > + priv->crop_rect.width = mf->width; > + priv->crop_rect.height = mf->height; > + > priv->fmt = ov5642_find_datafmt(mf->code); > > - ov5642_write_array(client, ov5642_default_regs_init); > - ov5642_set_resolution(client); > - ov5642_write_array(client, ov5642_default_regs_finalise); > + ret = ov5642_write_array(client, ov5642_default_regs_init); > + if (!ret) > + ret = ov5642_set_resolution(sd); > + if (!ret) > + ret = ov5642_write_array(client, ov5642_default_regs_finalise); > > - return 0; > + return ret; > } > > -static int ov5642_g_fmt(struct v4l2_subdev *sd, > - struct v4l2_mbus_framefmt *mf) > +static int ov5642_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt > *mf) { > struct i2c_client *client = v4l2_get_subdevdata(sd); > struct ov5642 *priv = to_ov5642(client); > @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, > > mf->code = fmt->code; > mf->colorspace = fmt->colorspace; > - mf->width = OV5642_WIDTH; > - mf->height = OV5642_HEIGHT; > + mf->width = priv->out_size.width; > + mf->height = priv->out_size.height; > mf->field = V4L2_FIELD_NONE; > > + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", __func__, > + mf->width, mf->height); Isn't that a bit too verbose ? Printing the format in a debug message in the s_fmt handler is useful, but maybe doing it in g_fmt is a bit too much. > return 0; > } > > @@ -829,14 +906,17 @@ static int ov5642_g_chip_ident(struct v4l2_subdev > *sd, > > static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) > { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct ov5642 *priv = to_ov5642(client); > struct v4l2_rect *rect = &a->c; > - > a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > - rect->top = 0; > - rect->left = 0; > - rect->width = OV5642_WIDTH; > - rect->height = OV5642_HEIGHT; > + rect->top = priv->crop_rect.top; > + rect->left = priv->crop_rect.left; > + rect->width = priv->crop_rect.width; > + rect->height = priv->crop_rect.height; rect = priv->crop_rect; should do. > > + dev_dbg(sd->v4l2_dev->dev, "%s crop width: %u heigth: %u\n", __func__, > + rect->width, rect->height); Same comment as for g_fmt. > return 0; > } > > @@ -844,8 +924,8 @@ static int ov5642_cropcap(struct v4l2_subdev *sd, > struct v4l2_cropcap *a) { > a->bounds.left = 0; > a->bounds.top = 0; > - a->bounds.width = OV5642_WIDTH; > - a->bounds.height = OV5642_HEIGHT; > + a->bounds.width = OV5642_MAX_WIDTH; > + a->bounds.height = OV5642_MAX_HEIGHT; > a->defrect = a->bounds; > a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > a->pixelaspect.numerator = 1; > @@ -858,9 +938,8 @@ static int ov5642_g_mbus_config(struct v4l2_subdev *sd, > struct v4l2_mbus_config *cfg) > { > cfg->type = V4L2_MBUS_CSI2; > - cfg->flags = V4L2_MBUS_CSI2_2_LANE | > - V4L2_MBUS_CSI2_CHANNEL_0 | > - V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; > + cfg->flags = V4L2_MBUS_CSI2_2_LANE | V4L2_MBUS_CSI2_CHANNEL_0 | > + V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; > > return 0; > } > @@ -941,8 +1020,15 @@ static int ov5642_probe(struct i2c_client *client, > > v4l2_i2c_subdev_init(&priv->subdev, client, &ov5642_subdev_ops); > > - icd->ops = NULL; > - priv->fmt = &ov5642_colour_fmts[0]; > + icd->ops = NULL; > + priv->fmt = &ov5642_colour_fmts[0]; > + > + priv->crop_rect.width = OV5642_DEFAULT_WIDTH; > + priv->crop_rect.height = OV5642_DEFAULT_HEIGHT; > + priv->crop_rect.left = (OV5642_MAX_WIDTH - OV5642_DEFAULT_WIDTH) / 2; > + priv->crop_rect.top = (OV5642_MAX_HEIGHT - OV5642_DEFAULT_HEIGHT) / 2; > + priv->out_size.width = OV5642_DEFAULT_WIDTH; > + priv->out_size.height = OV5642_DEFAULT_HEIGHT; > > ret = ov5642_video_probe(icd, client); > if (ret < 0) > @@ -951,6 +1037,7 @@ static int ov5642_probe(struct i2c_client *client, > return 0; > > error: > + icd->ops = NULL; > kfree(priv); > return ret; > } > @@ -961,6 +1048,7 @@ static int ov5642_remove(struct i2c_client *client) > struct soc_camera_device *icd = client->dev.platform_data; > struct soc_camera_link *icl = to_soc_camera_link(icd); > > + icd->ops = NULL; > if (icl->free_bus) > icl->free_bus(icl); > kfree(priv);
Hi Laurent On Sun, 28 Aug 2011, Laurent Pinchart wrote: [snip] > > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client > > *client) } > > > > /* Find a data format by a pixel code in an array */ > > -static const struct ov5642_datafmt > > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > > v4l2_mbus_pixelcode code) { > > checkpatch.pl won't be happy. Since the lift of the hard 80-char limit, I often find lines of 86 characters more acceptable than their split versions. [snip] > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, > > > > ov5642_try_fmt(sd, mf); > > > > + priv->out_size.width = mf->width; > > + priv->out_size.height = mf->height; > > It looks like to me (but I may be wrong) that you achieve different > resolutions using cropping, not scaling. If that's correct you should > implement s_crop support and refuse changing the resolution through s_fmt. As the patch explains (I think) on several occasions, currently only the 1:1 scale is supported, and it was our deliberate choice to implement this using the scaling API > > @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, > > > > mf->code = fmt->code; > > mf->colorspace = fmt->colorspace; > > - mf->width = OV5642_WIDTH; > > - mf->height = OV5642_HEIGHT; > > + mf->width = priv->out_size.width; > > + mf->height = priv->out_size.height; > > mf->field = V4L2_FIELD_NONE; > > > > + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", __func__, > > + mf->width, mf->height); > > Isn't that a bit too verbose ? Printing the format in a debug message in the > s_fmt handler is useful, but maybe doing it in g_fmt is a bit too much. This is a dev_dbg()... Personally, as long as they don't clutter the source code needlessly, compile without warnings and have their typos fixed (;-)) I don't have problems with an odd instance, even if I don't really perceive its output as particularly useful:-) [snip] > > + dev_dbg(sd->v4l2_dev->dev, "%s crop width: %u heigth: %u\n", __func__, > > + rect->width, rect->height); > > Same comment as for g_fmt. same reply:-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > [snip] > > > > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct > > > i2c_client *client) } > > > > > > /* Find a data format by a pixel code in an array */ > > > > > > -static const struct ov5642_datafmt > > > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > > > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > > > v4l2_mbus_pixelcode code) { > > > > checkpatch.pl won't be happy. > > Since the lift of the hard 80-char limit, I often find lines of 86 > characters more acceptable than their split versions. When has that been lifted ? > [snip] > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, > > > > > > ov5642_try_fmt(sd, mf); > > > > > > + priv->out_size.width = mf->width; > > > + priv->out_size.height = mf->height; > > > > It looks like to me (but I may be wrong) that you achieve different > > resolutions using cropping, not scaling. If that's correct you should > > implement s_crop support and refuse changing the resolution through > > s_fmt. > > As the patch explains (I think) on several occasions, currently only the > 1:1 scale is supported, and it was our deliberate choice to implement this > using the scaling API If you implement cropping, you should use the crop API, not the scaling API :-) > > > @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, > > > > > > mf->code = fmt->code; > > > mf->colorspace = fmt->colorspace; > > > > > > - mf->width = OV5642_WIDTH; > > > - mf->height = OV5642_HEIGHT; > > > + mf->width = priv->out_size.width; > > > + mf->height = priv->out_size.height; > > > > > > mf->field = V4L2_FIELD_NONE; > > > > > > + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", > > > __func__, + mf->width, mf->height); > > > > Isn't that a bit too verbose ? Printing the format in a debug message in > > the s_fmt handler is useful, but maybe doing it in g_fmt is a bit too > > much. > > This is a dev_dbg()... Personally, as long as they don't clutter the source > code needlessly, compile without warnings and have their typos fixed (;-)) Removing it is a good way to fix the typo :-) > I don't have problems with an odd instance, even if I don't really perceive > its output as particularly useful:-)
On Mon, 29 Aug 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > [snip] > > > > > > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct > > > > i2c_client *client) } > > > > > > > > /* Find a data format by a pixel code in an array */ > > > > > > > > -static const struct ov5642_datafmt > > > > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > > > > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > > > > v4l2_mbus_pixelcode code) { > > > > > > checkpatch.pl won't be happy. > > > > Since the lift of the hard 80-char limit, I often find lines of 86 > > characters more acceptable than their split versions. > > When has that been lifted ? Sorry, don't have a link at hand, a few months ago, either beginning of this or end of last year, perhaps. There has been a lengthy discussion on multiple lists, mainly lkml, the common mood was, that 80 chars wasn't meaningful anymore, but I'm not sure what exact actions have been executed to document this. > > > [snip] > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > + priv->out_size.width = mf->width; > > > > + priv->out_size.height = mf->height; > > > > > > It looks like to me (but I may be wrong) that you achieve different > > > resolutions using cropping, not scaling. If that's correct you should > > > implement s_crop support and refuse changing the resolution through > > > s_fmt. > > > > As the patch explains (I think) on several occasions, currently only the > > 1:1 scale is supported, and it was our deliberate choice to implement this > > using the scaling API > > If you implement cropping, you should use the crop API, not the scaling API > :-) It's changing both - input and output sizes. > > > > > @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, > > > > > > > > mf->code = fmt->code; > > > > mf->colorspace = fmt->colorspace; > > > > > > > > - mf->width = OV5642_WIDTH; > > > > - mf->height = OV5642_HEIGHT; > > > > + mf->width = priv->out_size.width; > > > > + mf->height = priv->out_size.height; > > > > > > > > mf->field = V4L2_FIELD_NONE; > > > > > > > > + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", > > > > __func__, + mf->width, mf->height); > > > > > > Isn't that a bit too verbose ? Printing the format in a debug message in > > > the s_fmt handler is useful, but maybe doing it in g_fmt is a bit too > > > much. > > > > This is a dev_dbg()... Personally, as long as they don't clutter the source > > code needlessly, compile without warnings and have their typos fixed (;-)) > > Removing it is a good way to fix the typo :-) > > > I don't have problems with an odd instance, even if I don't really perceive > > its output as particularly useful:-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > [snip] > > > > > > > > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct > > > > > i2c_client *client) } > > > > > > > > > > /* Find a data format by a pixel code in an array */ > > > > > > > > > > -static const struct ov5642_datafmt > > > > > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > > > > > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > > > > > v4l2_mbus_pixelcode code) { > > > > > > > > checkpatch.pl won't be happy. > > > > > > Since the lift of the hard 80-char limit, I often find lines of 86 > > > characters more acceptable than their split versions. > > > > When has that been lifted ? > > Sorry, don't have a link at hand, a few months ago, either beginning of > this or end of last year, perhaps. There has been a lengthy discussion on > multiple lists, mainly lkml, the common mood was, that 80 chars wasn't > meaningful anymore, but I'm not sure what exact actions have been executed > to document this. I remember the discussion but I wasn't sure if anything had been decided and set in stone. > > > [snip] > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev > > > > > *sd, > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > + priv->out_size.height = mf->height; > > > > > > > > It looks like to me (but I may be wrong) that you achieve different > > > > resolutions using cropping, not scaling. If that's correct you should > > > > implement s_crop support and refuse changing the resolution through > > > > s_fmt. > > > > > > As the patch explains (I think) on several occasions, currently only > > > the 1:1 scale is supported, and it was our deliberate choice to > > > implement this using the scaling API > > > > If you implement cropping, you should use the crop API, not the scaling > > API > > > > :-) > > It's changing both - input and output sizes. Sure, but it's still cropping.
On Mon, 29 Aug 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > [snip] > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev > > > > > > *sd, > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve different > > > > > resolutions using cropping, not scaling. If that's correct you should > > > > > implement s_crop support and refuse changing the resolution through > > > > > s_fmt. > > > > > > > > As the patch explains (I think) on several occasions, currently only > > > > the 1:1 scale is supported, and it was our deliberate choice to > > > > implement this using the scaling API > > > > > > If you implement cropping, you should use the crop API, not the scaling > > > API > > > > > > :-) > > > > It's changing both - input and output sizes. > > Sure, but it's still cropping. Why? Isn't it a matter of the PoV? It changes the output window, i.e., implements S_FMT. And S_FMT is by far more important / widely used than S_CROP. Refusing to change the output window and always just returning the == crop size wouldn't be polite, IMHO. Don't think many users would guess to use S_CROP. Standard applications a la mplayer don't use S_CROP at all. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 29, 2011 at 02:18:50PM +0200, Guennadi Liakhovetski wrote: > Hi Laurent > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > [snip] > > > > @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client > > > *client) } > > > > > > /* Find a data format by a pixel code in an array */ > > > -static const struct ov5642_datafmt > > > - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) > > > +static const struct ov5642_datafmt *ov5642_find_datafmt(enum > > > v4l2_mbus_pixelcode code) { > > > > checkpatch.pl won't be happy. > > Since the lift of the hard 80-char limit, I often find lines of 86 characters > more acceptable than their split versions. It's not lifted, and it's still a rule. It's only that exceptions are nowadays allowed to that rule where they make sense. > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, > > > > > > ov5642_try_fmt(sd, mf); > > > > > > + priv->out_size.width = mf->width; > > > + priv->out_size.height = mf->height; > > > > It looks like to me (but I may be wrong) that you achieve different > > resolutions using cropping, not scaling. If that's correct you should > > implement s_crop support and refuse changing the resolution through s_fmt. > > As the patch explains (I think) on several occasions, currently only the > 1:1 scale is supported, and it was our deliberate choice to implement this > using the scaling API As there is a subdev op to implement crop (s_crop) it should be used instead. Otherwise any application thinks that the hardware can actually do scaling but instead it crops. The above looks like gross misuse of the API to me. Is there any particular reason why you think it should be implemented this way?
Hi Guennadi, On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > [snip] > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > correct you should implement s_crop support and refuse changing > > > > > > the resolution through s_fmt. > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > only the 1:1 scale is supported, and it was our deliberate choice > > > > > to implement this using the scaling API > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > scaling API > > > > > > > > :-) > > > > > > It's changing both - input and output sizes. > > > > Sure, but it's still cropping. > > Why? Isn't it a matter of the PoV? No it isn't. Cropping is cropping, regardless of how you look at it. > It changes the output window, i.e., implements S_FMT. And S_FMT is by far > more important / widely used than S_CROP. Refusing to change the output > window and always just returning the == crop size wouldn't be polite, IMHO. If your sensor has no scaler the output size is equal to the crop rectangle. There's no way around that, and there's no reason to have the driver behave otherwise. > Don't think many users would guess to use S_CROP. Users who want to crop use S_CROP. > Standard applications a la mplayer don't use S_CROP at all. That's because they don't want to crop. mplayer expects the driver to perform scaling when it calls S_FMT, and users won't be happy if you crop instead.
(also replying to a similar comment by Sakari) On Tue, 30 Aug 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > correct you should implement s_crop support and refuse changing > > > > > > > the resolution through s_fmt. > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > only the 1:1 scale is supported, and it was our deliberate choice > > > > > > to implement this using the scaling API > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > scaling API > > > > > > > > > > :-) > > > > > > > > It's changing both - input and output sizes. > > > > > > Sure, but it's still cropping. > > > > Why? Isn't it a matter of the PoV? > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by far > > more important / widely used than S_CROP. Refusing to change the output > > window and always just returning the == crop size wouldn't be polite, IMHO. > > If your sensor has no scaler the output size is equal to the crop rectangle. > There's no way around that, and there's no reason to have the driver behave > otherwise. > > > Don't think many users would guess to use S_CROP. > > Users who want to crop use S_CROP. > > > Standard applications a la mplayer don't use S_CROP at all. > > That's because they don't want to crop. mplayer expects the driver to perform > scaling when it calls S_FMT, and users won't be happy if you crop instead. So, here's my opinion, based on the V4L2 spec. I'm going to base on this and pull this patch into my tree and let Mauro decide, unless he expresses his negative opinion before that. The spec defines S_FMT as an operation to set the output (in case of a capture device) frame format. Which this driver clearly does. The output format should be set, using scaling, however, if the driver or the hardware are unable to preserve the exact same input rectangle to satisfy the request, the driver is also allowed to change the cropping rectangle _as much as necessary_ - S_FMT takes precedence. This has been discussed before, and the conclusion was - of the two geometry calls (S_FMT and S_CROP) the last call overrides previous setting of the opposite geometry. It also defines S_CROP as an operation to set the cropping rectangle. The driver is also allowed to change the output window, if it cannot be preserved. Similarly, the last call wins. Ideally in this situation I would implement both S_CROP and S_FMT and let both change the opposite window as needed, which in this case means set it equal to the one, being configured. Since most applications are primarily interested in the S_FMT call to configure their user interface, I find it a wrong approach to refuse S_FMT and always return the current cropping rectangle. In such a case the application will possibly be stuck with some default output rectangle, because it certainly will _not_ guess to use S_CROP to configure it. Whereas if we implement S_FMT with a constant 1:1 scale the application will get the required UI size. I agree, that changing the view area, while changing the output window, is not exactly what the user expects, but it's better than presenting all applications with a fixed, possibly completely unsuitable, UI window. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mauro, Could you please comment on this ? In a nutshell (and from my biased point of view), the question is "can cropping be configured using S_FMT instead of S_CROP ?". The answer is of course no :-) On Tuesday 30 August 2011 15:13:25 Guennadi Liakhovetski wrote: > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > correct you should implement s_crop support and refuse > > > > > > > > changing the resolution through s_fmt. > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > only the 1:1 scale is supported, and it was our deliberate > > > > > > > choice to implement this using the scaling API > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > scaling API > > > > > > > > > > > > :-) > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > Sure, but it's still cropping. > > > > > > Why? Isn't it a matter of the PoV? > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by > > > far more important / widely used than S_CROP. Refusing to change the > > > output window and always just returning the == crop size wouldn't be > > > polite, IMHO. > > > > If your sensor has no scaler the output size is equal to the crop > > rectangle. There's no way around that, and there's no reason to have the > > driver behave otherwise. > > > > > Don't think many users would guess to use S_CROP. > > > > Users who want to crop use S_CROP. > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > That's because they don't want to crop. mplayer expects the driver to > > perform scaling when it calls S_FMT, and users won't be happy if you > > crop instead. > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > and pull this patch into my tree and let Mauro decide, unless he expresses > his negative opinion before that. > > The spec defines S_FMT as an operation to set the output (in case of a > capture device) frame format. Which this driver clearly does. The output > format should be set, using scaling, however, if the driver or the > hardware are unable to preserve the exact same input rectangle to satisfy > the request, the driver is also allowed to change the cropping rectangle > _as much as necessary_ - S_FMT takes precedence. This has been discussed > before, and the conclusion was - of the two geometry calls (S_FMT and > S_CROP) the last call overrides previous setting of the opposite geometry. > > It also defines S_CROP as an operation to set the cropping rectangle. The > driver is also allowed to change the output window, if it cannot be > preserved. Similarly, the last call wins. > > Ideally in this situation I would implement both S_CROP and S_FMT and let > both change the opposite window as needed, which in this case means set it > equal to the one, being configured. Since most applications are primarily > interested in the S_FMT call to configure their user interface, I find it > a wrong approach to refuse S_FMT and always return the current cropping > rectangle. In such a case the application will possibly be stuck with some > default output rectangle, because it certainly will _not_ guess to use > S_CROP to configure it. Whereas if we implement S_FMT with a constant 1:1 > scale the application will get the required UI size. I agree, that > changing the view area, while changing the output window, is not exactly > what the user expects, but it's better than presenting all applications > with a fixed, possibly completely unsuitable, UI window.
Hi Guennadi, On Tuesday 30 August 2011 15:13:25 Guennadi Liakhovetski wrote: > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > correct you should implement s_crop support and refuse > > > > > > > > changing the resolution through s_fmt. > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > only the 1:1 scale is supported, and it was our deliberate > > > > > > > choice to implement this using the scaling API > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > scaling API > > > > > > > > > > > > :-) > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > Sure, but it's still cropping. > > > > > > Why? Isn't it a matter of the PoV? > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by > > > far more important / widely used than S_CROP. Refusing to change the > > > output window and always just returning the == crop size wouldn't be > > > polite, IMHO. > > > > If your sensor has no scaler the output size is equal to the crop > > rectangle. There's no way around that, and there's no reason to have the > > driver behave otherwise. > > > > > Don't think many users would guess to use S_CROP. > > > > Users who want to crop use S_CROP. > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > That's because they don't want to crop. mplayer expects the driver to > > perform scaling when it calls S_FMT, and users won't be happy if you > > crop instead. > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > and pull this patch into my tree and let Mauro decide, unless he expresses > his negative opinion before that. I've also made other comments. I expect at least a v2 that addresses them.
On Tue, 30 Aug 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Tuesday 30 August 2011 15:13:25 Guennadi Liakhovetski wrote: > > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > > correct you should implement s_crop support and refuse > > > > > > > > > changing the resolution through s_fmt. > > > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > > only the 1:1 scale is supported, and it was our deliberate > > > > > > > > choice to implement this using the scaling API > > > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > > scaling API > > > > > > > > > > > > > > :-) > > > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > > > Sure, but it's still cropping. > > > > > > > > Why? Isn't it a matter of the PoV? > > > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by > > > > far more important / widely used than S_CROP. Refusing to change the > > > > output window and always just returning the == crop size wouldn't be > > > > polite, IMHO. > > > > > > If your sensor has no scaler the output size is equal to the crop > > > rectangle. There's no way around that, and there's no reason to have the > > > driver behave otherwise. > > > > > > > Don't think many users would guess to use S_CROP. > > > > > > Users who want to crop use S_CROP. > > > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > > > That's because they don't want to crop. mplayer expects the driver to > > > perform scaling when it calls S_FMT, and users won't be happy if you > > > crop instead. > > > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > > and pull this patch into my tree and let Mauro decide, unless he expresses > > his negative opinion before that. > > I've also made other comments. I expect at least a v2 that addresses them. Of course, (most of) your other comments are valid and they will be addressed. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Laurent and others, 2011/8/30 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Guennadi, > > On Tuesday 30 August 2011 15:13:25 Guennadi Liakhovetski wrote: >> On Tue, 30 Aug 2011, Laurent Pinchart wrote: >> > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: >> > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: >> > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: >> > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: >> > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: >> > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: >> > > > > > > >> > > > > > > [snip] >> > > > > > > >> > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct >> > > > > > > > > v4l2_subdev *sd, >> > > > > > > > > >> > > > > > > > > ov5642_try_fmt(sd, mf); >> > > > > > > > > >> > > > > > > > > + priv->out_size.width = mf->width; >> > > > > > > > > + priv->out_size.height = mf->height; >> > > > > > > > >> > > > > > > > It looks like to me (but I may be wrong) that you achieve >> > > > > > > > different resolutions using cropping, not scaling. If that's >> > > > > > > > correct you should implement s_crop support and refuse >> > > > > > > > changing the resolution through s_fmt. >> > > > > > > >> > > > > > > As the patch explains (I think) on several occasions, currently >> > > > > > > only the 1:1 scale is supported, and it was our deliberate >> > > > > > > choice to implement this using the scaling API >> > > > > > >> > > > > > If you implement cropping, you should use the crop API, not the >> > > > > > scaling API >> > > > > > >> > > > > > :-) >> > > > > >> > > > > It's changing both - input and output sizes. >> > > > >> > > > Sure, but it's still cropping. >> > > >> > > Why? Isn't it a matter of the PoV? >> > >> > No it isn't. Cropping is cropping, regardless of how you look at it. >> > >> > > It changes the output window, i.e., implements S_FMT. And S_FMT is by >> > > far more important / widely used than S_CROP. Refusing to change the >> > > output window and always just returning the == crop size wouldn't be >> > > polite, IMHO. >> > >> > If your sensor has no scaler the output size is equal to the crop >> > rectangle. There's no way around that, and there's no reason to have the >> > driver behave otherwise. >> > >> > > Don't think many users would guess to use S_CROP. >> > >> > Users who want to crop use S_CROP. >> > >> > > Standard applications a la mplayer don't use S_CROP at all. >> > >> > That's because they don't want to crop. mplayer expects the driver to >> > perform scaling when it calls S_FMT, and users won't be happy if you >> > crop instead. >> >> So, here's my opinion, based on the V4L2 spec. I'm going to base on this >> and pull this patch into my tree and let Mauro decide, unless he expresses >> his negative opinion before that. > > I've also made other comments. I expect at least a v2 that addresses them. Being the author I should drop a note too, I guess. Currently I'm working on all the comments and preparing a new set of patches. I will of course either implement the suggestion or reply to it in 1 or 2 days. As far as the cropping/scaling discussion is going on - I wait until you agreed on something (what is probably not happening though ;) before adressing it directly in the code. I don't have a real own opinion here as I am too unexperienced. If the arguments are balanced I would prefer leaving the code as it is :-) but I'm open to change it if necessary. best regards, Bastian > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 30, 2011 15:13:25 Guennadi Liakhovetski wrote: > (also replying to a similar comment by Sakari) > > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > > Hi Guennadi, > > > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > correct you should implement s_crop support and refuse changing > > > > > > > > the resolution through s_fmt. > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > only the 1:1 scale is supported, and it was our deliberate choice > > > > > > > to implement this using the scaling API > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > scaling API > > > > > > > > > > > > :-) > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > Sure, but it's still cropping. > > > > > > Why? Isn't it a matter of the PoV? > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by far > > > more important / widely used than S_CROP. Refusing to change the output > > > window and always just returning the == crop size wouldn't be polite, IMHO. > > > > If your sensor has no scaler the output size is equal to the crop rectangle. > > There's no way around that, and there's no reason to have the driver behave > > otherwise. > > > > > Don't think many users would guess to use S_CROP. > > > > Users who want to crop use S_CROP. > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > That's because they don't want to crop. mplayer expects the driver to perform > > scaling when it calls S_FMT, and users won't be happy if you crop instead. > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > and pull this patch into my tree and let Mauro decide, unless he expresses > his negative opinion before that. > > The spec defines S_FMT as an operation to set the output (in case of a > capture device) frame format. Which this driver clearly does. The output > format should be set, using scaling, however, if the driver or the > hardware are unable to preserve the exact same input rectangle to satisfy > the request, the driver is also allowed to change the cropping rectangle > _as much as necessary_ - S_FMT takes precedence. This has been discussed > before, and the conclusion was - of the two geometry calls (S_FMT and > S_CROP) the last call overrides previous setting of the opposite geometry. > > It also defines S_CROP as an operation to set the cropping rectangle. The > driver is also allowed to change the output window, if it cannot be > preserved. Similarly, the last call wins. > > Ideally in this situation I would implement both S_CROP and S_FMT and let > both change the opposite window as needed, which in this case means set it > equal to the one, being configured. Since most applications are primarily > interested in the S_FMT call to configure their user interface, I find it > a wrong approach to refuse S_FMT and always return the current cropping > rectangle. In such a case the application will possibly be stuck with some > default output rectangle, because it certainly will _not_ guess to use > S_CROP to configure it. Whereas if we implement S_FMT with a constant 1:1 > scale the application will get the required UI size. I agree, that > changing the view area, while changing the output window, is not exactly > what the user expects, but it's better than presenting all applications > with a fixed, possibly completely unsuitable, UI window. The problem with S_FMT changing the crop rectangle (and I assume we are not talking about small pixel tweaks to make the hardware happy) is that the crop operation actually removes part of the frame. That's not something you would expect S_FMT to do, ever. Such an operation has to be explicitly requested by the user. It's also why properly written applications (e.g. capture-example.c) has code like this to reset the crop rectangle before starting streaming: if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; crop.c = cropcap.defrect; /* reset to default */ if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { switch (errno) { case EINVAL: /* Cropping not supported. */ break; default: /* Errors ignored. */ break; } } } (Hmm, capture-example.c should also test for ENOTTY since we changed the error code). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans On Tue, 30 Aug 2011, Hans Verkuil wrote: > On Tuesday, August 30, 2011 15:13:25 Guennadi Liakhovetski wrote: > > (also replying to a similar comment by Sakari) > > > > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > > > > Hi Guennadi, > > > > > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > > correct you should implement s_crop support and refuse > changing > > > > > > > > > the resolution through s_fmt. > > > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > > only the 1:1 scale is supported, and it was our deliberate > choice > > > > > > > > to implement this using the scaling API > > > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > > scaling API > > > > > > > > > > > > > > :-) > > > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > > > Sure, but it's still cropping. > > > > > > > > Why? Isn't it a matter of the PoV? > > > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by > far > > > > more important / widely used than S_CROP. Refusing to change the output > > > > window and always just returning the == crop size wouldn't be polite, > IMHO. > > > > > > If your sensor has no scaler the output size is equal to the crop > rectangle. > > > There's no way around that, and there's no reason to have the driver > behave > > > otherwise. > > > > > > > Don't think many users would guess to use S_CROP. > > > > > > Users who want to crop use S_CROP. > > > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > > > That's because they don't want to crop. mplayer expects the driver to > perform > > > scaling when it calls S_FMT, and users won't be happy if you crop instead. > > > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > > and pull this patch into my tree and let Mauro decide, unless he expresses > > his negative opinion before that. > > > > The spec defines S_FMT as an operation to set the output (in case of a > > capture device) frame format. Which this driver clearly does. The output > > format should be set, using scaling, however, if the driver or the > > hardware are unable to preserve the exact same input rectangle to satisfy > > the request, the driver is also allowed to change the cropping rectangle > > _as much as necessary_ - S_FMT takes precedence. This has been discussed > > before, and the conclusion was - of the two geometry calls (S_FMT and > > S_CROP) the last call overrides previous setting of the opposite geometry. > > > > It also defines S_CROP as an operation to set the cropping rectangle. The > > driver is also allowed to change the output window, if it cannot be > > preserved. Similarly, the last call wins. > > > > Ideally in this situation I would implement both S_CROP and S_FMT and let > > both change the opposite window as needed, which in this case means set it > > equal to the one, being configured. Since most applications are primarily > > interested in the S_FMT call to configure their user interface, I find it > > a wrong approach to refuse S_FMT and always return the current cropping > > rectangle. In such a case the application will possibly be stuck with some > > default output rectangle, because it certainly will _not_ guess to use > > S_CROP to configure it. Whereas if we implement S_FMT with a constant 1:1 > > scale the application will get the required UI size. I agree, that > > changing the view area, while changing the output window, is not exactly > > what the user expects, but it's better than presenting all applications > > with a fixed, possibly completely unsuitable, UI window. > > The problem with S_FMT changing the crop rectangle (and I assume we are not > talking about small pixel tweaks to make the hardware happy) is that the > crop operation actually removes part of the frame. That's not something you > would expect S_FMT to do, ever. Such an operation has to be explicitly > requested by the user. > > It's also why properly written applications (e.g. capture-example.c) has > code like this to reset the crop rectangle before starting streaming: > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > crop.c = cropcap.defrect; /* reset to default */ > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { > switch (errno) { > case EINVAL: > /* Cropping not supported. */ > break; > default: > /* Errors ignored. */ > break; > } > } > } > > (Hmm, capture-example.c should also test for ENOTTY since we changed the > error code). I agree, that preserving input rectangle == output rectangle in reply to S_FMT is not nice, and should be avoided, wherever possible. Still, I prefer this to sticking with just one fixed output geometry, especially since (1) the spec doesn't prohibit this behaviour, (2) there are already precedents in the mainline. Maybe, a bit of hardware background would help: the sensor is actually supposed to be able to both crop and scale, and we did try to implement scales other than 1:1, but the chip just refused to produce anything meaningful. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote: > Hi Hans > > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > > On Tuesday, August 30, 2011 15:13:25 Guennadi Liakhovetski wrote: > > > (also replying to a similar comment by Sakari) > > > > > > On Tue, 30 Aug 2011, Laurent Pinchart wrote: > > > > > > > Hi Guennadi, > > > > > > > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote: > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote: > > > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote: > > > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote: > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct > > > > > > > > > > > v4l2_subdev *sd, > > > > > > > > > > > > > > > > > > > > > > ov5642_try_fmt(sd, mf); > > > > > > > > > > > > > > > > > > > > > > + priv->out_size.width = mf->width; > > > > > > > > > > > + priv->out_size.height = mf->height; > > > > > > > > > > > > > > > > > > > > It looks like to me (but I may be wrong) that you achieve > > > > > > > > > > different resolutions using cropping, not scaling. If that's > > > > > > > > > > correct you should implement s_crop support and refuse > > changing > > > > > > > > > > the resolution through s_fmt. > > > > > > > > > > > > > > > > > > As the patch explains (I think) on several occasions, currently > > > > > > > > > only the 1:1 scale is supported, and it was our deliberate > > choice > > > > > > > > > to implement this using the scaling API > > > > > > > > > > > > > > > > If you implement cropping, you should use the crop API, not the > > > > > > > > scaling API > > > > > > > > > > > > > > > > :-) > > > > > > > > > > > > > > It's changing both - input and output sizes. > > > > > > > > > > > > Sure, but it's still cropping. > > > > > > > > > > Why? Isn't it a matter of the PoV? > > > > > > > > No it isn't. Cropping is cropping, regardless of how you look at it. > > > > > > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is by > > far > > > > > more important / widely used than S_CROP. Refusing to change the output > > > > > window and always just returning the == crop size wouldn't be polite, > > IMHO. > > > > > > > > If your sensor has no scaler the output size is equal to the crop > > rectangle. > > > > There's no way around that, and there's no reason to have the driver > > behave > > > > otherwise. > > > > > > > > > Don't think many users would guess to use S_CROP. > > > > > > > > Users who want to crop use S_CROP. > > > > > > > > > Standard applications a la mplayer don't use S_CROP at all. > > > > > > > > That's because they don't want to crop. mplayer expects the driver to > > perform > > > > scaling when it calls S_FMT, and users won't be happy if you crop instead. > > > > > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this > > > and pull this patch into my tree and let Mauro decide, unless he expresses > > > his negative opinion before that. > > > > > > The spec defines S_FMT as an operation to set the output (in case of a > > > capture device) frame format. Which this driver clearly does. The output > > > format should be set, using scaling, however, if the driver or the > > > hardware are unable to preserve the exact same input rectangle to satisfy > > > the request, the driver is also allowed to change the cropping rectangle > > > _as much as necessary_ - S_FMT takes precedence. This has been discussed > > > before, and the conclusion was - of the two geometry calls (S_FMT and > > > S_CROP) the last call overrides previous setting of the opposite geometry. > > > > > > It also defines S_CROP as an operation to set the cropping rectangle. The > > > driver is also allowed to change the output window, if it cannot be > > > preserved. Similarly, the last call wins. > > > > > > Ideally in this situation I would implement both S_CROP and S_FMT and let > > > both change the opposite window as needed, which in this case means set it > > > equal to the one, being configured. Since most applications are primarily > > > interested in the S_FMT call to configure their user interface, I find it > > > a wrong approach to refuse S_FMT and always return the current cropping > > > rectangle. In such a case the application will possibly be stuck with some > > > default output rectangle, because it certainly will _not_ guess to use > > > S_CROP to configure it. Whereas if we implement S_FMT with a constant 1:1 > > > scale the application will get the required UI size. I agree, that > > > changing the view area, while changing the output window, is not exactly > > > what the user expects, but it's better than presenting all applications > > > with a fixed, possibly completely unsuitable, UI window. > > > > The problem with S_FMT changing the crop rectangle (and I assume we are not > > talking about small pixel tweaks to make the hardware happy) is that the > > crop operation actually removes part of the frame. That's not something you > > would expect S_FMT to do, ever. Such an operation has to be explicitly > > requested by the user. > > > > It's also why properly written applications (e.g. capture-example.c) has > > code like this to reset the crop rectangle before starting streaming: > > > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > crop.c = cropcap.defrect; /* reset to default */ > > > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { > > switch (errno) { > > case EINVAL: > > /* Cropping not supported. */ > > break; > > default: > > /* Errors ignored. */ > > break; > > } > > } > > } > > > > (Hmm, capture-example.c should also test for ENOTTY since we changed the > > error code). > > I agree, that preserving input rectangle == output rectangle in reply to > S_FMT is not nice, and should be avoided, wherever possible. Still, I > prefer this to sticking with just one fixed output geometry, especially > since (1) the spec doesn't prohibit this behaviour, Hmm, I think it should be prohibited. Few drivers actually implement crop, and fewer applications use it. So I'm not surprised the spec doesn't go into much detail. > (2) there are already > precedents in the mainline. Which precedents? My guess is that any driver that does this was either not (or poorly) reviewed, or everyone just missed it. > Maybe, a bit of hardware background would help: the sensor is actually > supposed to be able to both crop and scale, and we did try to implement > scales other than 1:1, but the chip just refused to produce anything > meaningful. I still don't see any reason why S_FMT would suddenly crop on such a sensor. It's completely unexpected and the user does not get what he expects. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 30 Aug 2011, Hans Verkuil wrote: > On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote: > > Hi Hans > > > > On Tue, 30 Aug 2011, Hans Verkuil wrote: [snip] > > > The problem with S_FMT changing the crop rectangle (and I assume we are not > > > talking about small pixel tweaks to make the hardware happy) is that the > > > crop operation actually removes part of the frame. That's not something you > > > would expect S_FMT to do, ever. Such an operation has to be explicitly > > > requested by the user. > > > > > > It's also why properly written applications (e.g. capture-example.c) has > > > code like this to reset the crop rectangle before starting streaming: > > > > > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { > > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > > crop.c = cropcap.defrect; /* reset to default */ > > > > > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { > > > switch (errno) { > > > case EINVAL: > > > /* Cropping not supported. */ > > > break; > > > default: > > > /* Errors ignored. */ > > > break; > > > } > > > } > > > } > > > > > > (Hmm, capture-example.c should also test for ENOTTY since we changed the > > > error code). > > > > I agree, that preserving input rectangle == output rectangle in reply to > > S_FMT is not nice, and should be avoided, wherever possible. Still, I > > prefer this to sticking with just one fixed output geometry, especially > > since (1) the spec doesn't prohibit this behaviour, > > Hmm, I think it should be prohibited. Few drivers actually implement crop, > and fewer applications use it. So I'm not surprised the spec doesn't go into > much detail. > > > (2) there are already > > precedents in the mainline. > > Which precedents? My guess is that any driver that does this was either not > (or poorly) reviewed, or everyone just missed it. My first two sensor drivers mt9m001 and mt9v022 do this, but, I suspect, I didn't invent it at that time, I think, I copied it from somewhere, cannot say for sure though anymore. > > Maybe, a bit of hardware background would help: the sensor is actually > > supposed to be able to both crop and scale, and we did try to implement > > scales other than 1:1, but the chip just refused to produce anything > > meaningful. > > I still don't see any reason why S_FMT would suddenly crop on such a sensor. > It's completely unexpected and the user does not get what he expects. Good, let's make it simple for all (except Bastian) then: Bastian, sorry for having misguided you, please, switch to .s_crop(). Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/30 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > On Tue, 30 Aug 2011, Hans Verkuil wrote: > >> On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote: >> > Hi Hans >> > >> > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > [snip] > >> > > The problem with S_FMT changing the crop rectangle (and I assume we are not >> > > talking about small pixel tweaks to make the hardware happy) is that the >> > > crop operation actually removes part of the frame. That's not something you >> > > would expect S_FMT to do, ever. Such an operation has to be explicitly >> > > requested by the user. >> > > >> > > It's also why properly written applications (e.g. capture-example.c) has >> > > code like this to reset the crop rectangle before starting streaming: >> > > >> > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { >> > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> > > crop.c = cropcap.defrect; /* reset to default */ >> > > >> > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { >> > > switch (errno) { >> > > case EINVAL: >> > > /* Cropping not supported. */ >> > > break; >> > > default: >> > > /* Errors ignored. */ >> > > break; >> > > } >> > > } >> > > } >> > > >> > > (Hmm, capture-example.c should also test for ENOTTY since we changed the >> > > error code). >> > >> > I agree, that preserving input rectangle == output rectangle in reply to >> > S_FMT is not nice, and should be avoided, wherever possible. Still, I >> > prefer this to sticking with just one fixed output geometry, especially >> > since (1) the spec doesn't prohibit this behaviour, >> >> Hmm, I think it should be prohibited. Few drivers actually implement crop, >> and fewer applications use it. So I'm not surprised the spec doesn't go into >> much detail. >> >> > (2) there are already >> > precedents in the mainline. >> >> Which precedents? My guess is that any driver that does this was either not >> (or poorly) reviewed, or everyone just missed it. > > My first two sensor drivers mt9m001 and mt9v022 do this, but, I suspect, I > didn't invent it at that time, I think, I copied it from somewhere, cannot > say for sure though anymore. > >> > Maybe, a bit of hardware background would help: the sensor is actually >> > supposed to be able to both crop and scale, and we did try to implement >> > scales other than 1:1, but the chip just refused to produce anything >> > meaningful. >> >> I still don't see any reason why S_FMT would suddenly crop on such a sensor. >> It's completely unexpected and the user does not get what he expects. > > Good, let's make it simple for all (except Bastian) then: Bastian, sorry > for having misguided you, please, switch to .s_crop(). Sure, no problem. So s_fmt() shall be not available at all then, right? Instead the cropping rectangle can be changed and the output rectangle is adjusted accordingly. best regards, Bastian > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 30 Aug 2011, Bastian Hecht wrote: > 2011/8/30 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > > >> On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote: > >> > Hi Hans > >> > > >> > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > > > [snip] > > > >> > > The problem with S_FMT changing the crop rectangle (and I assume we are not > >> > > talking about small pixel tweaks to make the hardware happy) is that the > >> > > crop operation actually removes part of the frame. That's not something you > >> > > would expect S_FMT to do, ever. Such an operation has to be explicitly > >> > > requested by the user. > >> > > > >> > > It's also why properly written applications (e.g. capture-example.c) has > >> > > code like this to reset the crop rectangle before starting streaming: > >> > > > >> > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { > >> > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > >> > > crop.c = cropcap.defrect; /* reset to default */ > >> > > > >> > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { > >> > > switch (errno) { > >> > > case EINVAL: > >> > > /* Cropping not supported. */ > >> > > break; > >> > > default: > >> > > /* Errors ignored. */ > >> > > break; > >> > > } > >> > > } > >> > > } > >> > > > >> > > (Hmm, capture-example.c should also test for ENOTTY since we changed the > >> > > error code). > >> > > >> > I agree, that preserving input rectangle == output rectangle in reply to > >> > S_FMT is not nice, and should be avoided, wherever possible. Still, I > >> > prefer this to sticking with just one fixed output geometry, especially > >> > since (1) the spec doesn't prohibit this behaviour, > >> > >> Hmm, I think it should be prohibited. Few drivers actually implement crop, > >> and fewer applications use it. So I'm not surprised the spec doesn't go into > >> much detail. > >> > >> > (2) there are already > >> > precedents in the mainline. > >> > >> Which precedents? My guess is that any driver that does this was either not > >> (or poorly) reviewed, or everyone just missed it. > > > > My first two sensor drivers mt9m001 and mt9v022 do this, but, I suspect, I > > didn't invent it at that time, I think, I copied it from somewhere, cannot > > say for sure though anymore. > > > >> > Maybe, a bit of hardware background would help: the sensor is actually > >> > supposed to be able to both crop and scale, and we did try to implement > >> > scales other than 1:1, but the chip just refused to produce anything > >> > meaningful. > >> > >> I still don't see any reason why S_FMT would suddenly crop on such a sensor. > >> It's completely unexpected and the user does not get what he expects. > > > > Good, let's make it simple for all (except Bastian) then: Bastian, sorry > > for having misguided you, please, switch to .s_crop(). > > Sure, no problem. So s_fmt() shall be not available at all then, > right? Instead the cropping rectangle can be changed and the output > rectangle is adjusted accordingly. I would keep .s_fmt() and just return the current configuration from it. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Tuesday 30 August 2011 17:34:05 Guennadi Liakhovetski wrote: > On Tue, 30 Aug 2011, Bastian Hecht wrote: > > 2011/8/30 Guennadi Liakhovetski <g.liakhovetski@gmx.de>: > > > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > >> On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote: > > >> > Hi Hans > > > > > >> > On Tue, 30 Aug 2011, Hans Verkuil wrote: > > > [snip] > > > > > >> > > The problem with S_FMT changing the crop rectangle (and I assume > > >> > > we are not talking about small pixel tweaks to make the hardware > > >> > > happy) is that the crop operation actually removes part of the > > >> > > frame. That's not something you would expect S_FMT to do, ever. > > >> > > Such an operation has to be explicitly requested by the user. > > >> > > > > >> > > It's also why properly written applications (e.g. > > >> > > capture-example.c) has code like this to reset the crop rectangle > > >> > > before starting streaming: > > >> > > > > >> > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) { > > >> > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > >> > > crop.c = cropcap.defrect; /* reset to default */ > > >> > > > > >> > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) { > > >> > > switch (errno) { > > >> > > case EINVAL: > > >> > > /* Cropping not supported. */ > > >> > > break; > > >> > > default: > > >> > > /* Errors ignored. */ > > >> > > break; > > >> > > } > > >> > > } > > >> > > } > > >> > > > > >> > > (Hmm, capture-example.c should also test for ENOTTY since we > > >> > > changed the error code). > > >> > > > >> > I agree, that preserving input rectangle == output rectangle in > > >> > reply to S_FMT is not nice, and should be avoided, wherever > > >> > possible. Still, I prefer this to sticking with just one fixed > > >> > output geometry, especially since (1) the spec doesn't prohibit > > >> > this behaviour, > > >> > > >> Hmm, I think it should be prohibited. Few drivers actually implement > > >> crop, and fewer applications use it. So I'm not surprised the spec > > >> doesn't go into much detail. > > >> > > >> > (2) there are already > > >> > precedents in the mainline. > > >> > > >> Which precedents? My guess is that any driver that does this was > > >> either not (or poorly) reviewed, or everyone just missed it. > > > > > > My first two sensor drivers mt9m001 and mt9v022 do this, but, I > > > suspect, I didn't invent it at that time, I think, I copied it from > > > somewhere, cannot say for sure though anymore. > > > > > >> > Maybe, a bit of hardware background would help: the sensor is > > >> > actually supposed to be able to both crop and scale, and we did try > > >> > to implement scales other than 1:1, but the chip just refused to > > >> > produce anything meaningful. > > >> > > >> I still don't see any reason why S_FMT would suddenly crop on such a > > >> sensor. It's completely unexpected and the user does not get what he > > >> expects. > > > > > > Good, let's make it simple for all (except Bastian) then: Bastian, > > > sorry for having misguided you, please, switch to .s_crop(). > > > > Sure, no problem. So s_fmt() shall be not available at all then, > > right? Instead the cropping rectangle can be changed and the output > > rectangle is adjusted accordingly. > > I would keep .s_fmt() and just return the current configuration from it. That's what I would do as well. Having no .s_fmt() would be very confusing for applications and/or bridges.
Hello Laurent, 2011/8/28 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Bastian, > > Thanks for the patch. > [snip] >> >> +#define REG_RED_GAIN_HIGH 0x3400 >> +#define REG_RED_GAIN_LOW 0x3401 >> +#define REG_BLUE_GAIN_HIGH 0x3404 >> +#define REG_BLUE_GAIN_LOW 0x3405 >> +#define REG_AWB_MANUAL 0x3406 >> +#define REG_EXP_HIGH 0x3500 >> +#define REG_EXP_MIDDLE 0x3501 >> +#define REG_EXP_LOW 0x3502 >> +#define REG_EXP_GAIN_CTRL 0x3503 >> +#define REG_GAIN 0x350b > > This belongs to the second patch. Yes, I've changed it. >> +#define REG_EXTEND_FRAME_TIME_HIGH 0x350c >> +#define REG_EXTEND_FRAME_TIME_LOW 0x350d >> #define REG_WINDOW_START_X_HIGH 0x3800 >> #define REG_WINDOW_START_X_LOW 0x3801 >> #define REG_WINDOW_START_Y_HIGH 0x3802 >> #define REG_WINDOW_START_Y_LOW 0x3803 >> #define REG_WINDOW_WIDTH_HIGH 0x3804 >> #define REG_WINDOW_WIDTH_LOW 0x3805 >> -#define REG_WINDOW_HEIGHT_HIGH 0x3806 >> +#define REG_WINDOW_HEIGHT_HIGH 0x3806 >> #define REG_WINDOW_HEIGHT_LOW 0x3807 >> #define REG_OUT_WIDTH_HIGH 0x3808 >> #define REG_OUT_WIDTH_LOW 0x3809 >> @@ -44,19 +58,56 @@ >> #define REG_OUT_TOTAL_WIDTH_LOW 0x380d >> #define REG_OUT_TOTAL_HEIGHT_HIGH 0x380e >> #define REG_OUT_TOTAL_HEIGHT_LOW 0x380f >> +#define REG_FLIP_SUBSAMPLE 0x3818 >> +#define REG_OUTPUT_FORMAT 0x4300 >> +#define REG_ISP_CTRL_01 0x5001 >> +#define REG_DIGITAL_EFFECTS 0x5580 >> +#define REG_HUE_COS 0x5581 >> +#define REG_HUE_SIN 0x5582 >> +#define REG_BLUE_SATURATION 0x5583 >> +#define REG_RED_SATURATION 0x5584 >> +#define REG_CONTRAST 0x5588 >> +#define REG_BRIGHTNESS 0x5589 >> +#define REG_D_E_AUXILLARY 0x558a >> +#define REG_AVG_WINDOW_END_X_HIGH 0x5682 >> +#define REG_AVG_WINDOW_END_X_LOW 0x5683 >> +#define REG_AVG_WINDOW_END_Y_HIGH 0x5686 >> +#define REG_AVG_WINDOW_END_Y_LOW 0x5687 > > So does this. Changed as well. >> + >> +/* active pixel array size */ >> +#define OV5642_SENSOR_SIZE_X 2592 >> +#define OV5642_SENSOR_SIZE_Y 1944 >> + >> +/* current maximum working size */ >> +#define OV5642_MAX_WIDTH OV5642_SENSOR_SIZE_X >> +#define OV5642_MAX_HEIGHT 720 >> + >> +/* default sizes */ >> +#define OV5642_DEFAULT_WIDTH 1280 >> +#define OV5642_DEFAULT_HEIGHT OV5642_MAX_HEIGHT >> + >> +/* minimum extra blanking */ >> +#define BLANKING_EXTRA_WIDTH 500 >> +#define BLANKING_EXTRA_HEIGHT 20 >> >> /* >> - * define standard resolution. >> - * Works currently only for up to 720 lines >> - * eg. 320x240, 640x480, 800x600, 1280x720, 2048x720 >> + * the sensor's autoexposure is buggy when setting total_height low. >> + * It tries to expose longer than 1 frame period without taking care of it >> + * and this leads to weird output. So we set 1000 lines as minimum. >> */ >> >> -#define OV5642_WIDTH 1280 >> -#define OV5642_HEIGHT 720 >> -#define OV5642_TOTAL_WIDTH 3200 >> -#define OV5642_TOTAL_HEIGHT 2000 >> -#define OV5642_SENSOR_SIZE_X 2592 >> -#define OV5642_SENSOR_SIZE_Y 1944 >> +#define BLANKING_MIN_HEIGHT 1000 >> + >> +/* >> + * About OV5642 resolution, cropping and binning: >> + * This sensor supports it all, at least in the feature description. >> + * Unfortunately, no combination of appropriate registers settings could >> make + * the chip work the intended way. As it works with predefined >> register lists, + * some undocumented registers are presumably changed >> there to achieve their + * goals. >> + * This driver currently only works for resolutions up to 720 lines with a >> + * 1:1 scale. Hopefully these restrictions will be removed in the future. >> + */ > > Can you please move this comment above the OV5642_MAX_WIDTH and > OV5642_MAX_HEIGHT definitions ? Ok, done. >> struct regval_list { >> u16 reg_num; >> @@ -105,10 +156,8 @@ static struct regval_list ov5642_default_regs_init[] = >> { { 0x471d, 0x5 }, >> { 0x4708, 0x6 }, >> { 0x370c, 0xa0 }, >> - { 0x5687, 0x94 }, > > Unless I'm mistaken, this register value is removed and isn't replaced by > anything in this patch or the next one. Is that intentional ? This was a cropping offset. It is done now dynamically. >> { 0x501f, 0x0 }, >> { 0x5000, 0x4f }, >> - { 0x5001, 0xcf }, > > This one is replaced by 0xff. I'm not sure how that's related to this patch. > Could you please check all modifications to the ov5642_default_regs_init[] and > ov5642_default_regs_finalise[] arrays ? Some probably need to move to the next > patch, and others don't seem to be related to any of the two patches. There are 2 reasons why the registers are changed: 1st) values are calculated dynamically now. 2nd) the register list contains duplicates. It changes some registers multiple times with different values each time. I tried to clean this up. I've done it this way now: I left the original register list untouched for this patch, as the modifications belong belong almost all to the controls. >> { 0x4300, 0x30 }, >> { 0x4300, 0x30 }, >> { 0x460b, 0x35 }, >> @@ -121,11 +170,8 @@ static struct regval_list ov5642_default_regs_init[] = >> { { 0x4402, 0x90 }, >> { 0x460c, 0x22 }, >> { 0x3815, 0x44 }, >> - { 0x3503, 0x7 }, >> { 0x3501, 0x73 }, >> { 0x3502, 0x80 }, >> - { 0x350b, 0x0 }, >> - { 0x3818, 0xc8 }, >> { 0x3824, 0x11 }, >> { 0x3a00, 0x78 }, >> { 0x3a1a, 0x4 }, >> @@ -140,12 +186,6 @@ static struct regval_list ov5642_default_regs_init[] = >> { { 0x350d, 0xd0 }, >> { 0x3a0d, 0x8 }, >> { 0x3a0e, 0x6 }, >> - { 0x3500, 0x0 }, >> - { 0x3501, 0x0 }, >> - { 0x3502, 0x0 }, >> - { 0x350a, 0x0 }, >> - { 0x350b, 0x0 }, >> - { 0x3503, 0x0 }, >> { 0x3a0f, 0x3c }, >> { 0x3a10, 0x32 }, >> { 0x3a1b, 0x3c }, >> @@ -298,7 +338,7 @@ static struct regval_list ov5642_default_regs_init[] = >> { { 0x54b7, 0xdf }, >> { 0x5402, 0x3f }, >> { 0x5403, 0x0 }, >> - { 0x3406, 0x0 }, >> + { REG_AWB_MANUAL, 0x0 }, >> { 0x5180, 0xff }, >> { 0x5181, 0x52 }, >> { 0x5182, 0x11 }, >> @@ -515,7 +555,6 @@ static struct regval_list ov5642_default_regs_init[] = >> { { 0x5088, 0x0 }, >> { 0x5089, 0x0 }, >> { 0x302b, 0x0 }, >> - { 0x3503, 0x7 }, >> { 0x3011, 0x8 }, >> { 0x350c, 0x2 }, >> { 0x350d, 0xe4 }, >> @@ -526,7 +565,6 @@ static struct regval_list ov5642_default_regs_init[] = >> { >> >> static struct regval_list ov5642_default_regs_finalise[] = { >> { 0x3810, 0xc2 }, >> - { 0x3818, 0xc9 }, >> { 0x381c, 0x10 }, >> { 0x381d, 0xa0 }, >> { 0x381e, 0x5 }, >> @@ -541,23 +579,20 @@ static struct regval_list >> ov5642_default_regs_finalise[] = { { 0x3a0d, 0x2 }, >> { 0x3a0e, 0x1 }, >> { 0x401c, 0x4 }, >> - { 0x5682, 0x5 }, >> - { 0x5683, 0x0 }, >> - { 0x5686, 0x2 }, >> - { 0x5687, 0xcc }, >> - { 0x5001, 0x4f }, >> + { REG_ISP_CTRL_01, 0xff }, >> + { REG_DIGITAL_EFFECTS, 0x6 }, >> { 0x589b, 0x6 }, >> { 0x589a, 0xc5 }, >> - { 0x3503, 0x0 }, >> + { REG_EXP_GAIN_CTRL, 0x0 }, >> { 0x460c, 0x20 }, >> { 0x460b, 0x37 }, >> { 0x471c, 0xd0 }, >> { 0x471d, 0x5 }, >> { 0x3815, 0x1 }, >> - { 0x3818, 0xc1 }, >> + { REG_FLIP_SUBSAMPLE, 0xc1 }, >> { 0x501f, 0x0 }, >> { 0x5002, 0xe0 }, >> - { 0x4300, 0x32 }, /* UYVY */ >> + { REG_OUTPUT_FORMAT, 0x32 }, >> { 0x3002, 0x1c }, >> { 0x4800, 0x14 }, >> { 0x4801, 0xf }, >> @@ -578,9 +613,20 @@ struct ov5642_datafmt { >> enum v4l2_colorspace colorspace; >> }; >> >> +/* the output resolution and blanking information */ >> +struct ov5642_out_size { >> + int width; >> + int height; >> + int total_width; >> + int total_height; >> +}; >> + >> struct ov5642 { >> struct v4l2_subdev subdev; >> + >> const struct ov5642_datafmt *fmt; >> + struct v4l2_rect crop_rect; >> + struct ov5642_out_size out_size; >> }; >> >> static const struct ov5642_datafmt ov5642_colour_fmts[] = { >> @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client >> *client) } >> >> /* Find a data format by a pixel code in an array */ >> -static const struct ov5642_datafmt >> - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) >> +static const struct ov5642_datafmt *ov5642_find_datafmt(enum >> v4l2_mbus_pixelcode code) { > > checkpatch.pl won't be happy. Well, I broke it. >> int i; >> >> @@ -641,6 +686,26 @@ static int reg_write(struct i2c_client *client, u16 >> reg, u8 val) >> >> return 0; >> } >> + >> +/* >> + * convenience function to write 16 bit register values that are split up >> + * into two consecutive high and low parts >> + */ >> +static int reg_write16(struct i2c_client *client, u16 reg, u16 val16) >> +{ >> + int ret; >> + u8 val8; >> + >> + val8 = val16 >> 8; >> + ret = reg_write(client, reg, val8); > > You can use val16 >> 8 directly as a function argument and get rid of the val8 > variable. Ok, changed. >> + if (ret) >> + return ret; >> + val8 = val16 & 0x00ff; >> + ret = reg_write(client, reg + 1, val8); >> + >> + return ret; > > return reg_write(...) should be fine. Changed. >> +} >> + >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> static int ov5642_get_register(struct v4l2_subdev *sd, struct >> v4l2_dbg_register *reg) { >> @@ -684,68 +749,72 @@ static int ov5642_write_array(struct i2c_client >> *client, return 0; >> } >> >> -static int ov5642_set_resolution(struct i2c_client *client) >> +static int ov5642_set_resolution(struct v4l2_subdev *sd) >> { >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> + struct ov5642 *priv = to_ov5642(client); >> + int width = priv->out_size.width; >> + int height = priv->out_size.height; >> + int total_width = priv->out_size.total_width; >> + int total_height = priv->out_size.total_height; >> + int start_x = (OV5642_SENSOR_SIZE_X - width) / 2; >> + int start_y = (OV5642_SENSOR_SIZE_Y - height) / 2; >> int ret; >> - u8 start_x_high = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) >> 8; >> - u8 start_x_low = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) & 0xff; >> - u8 start_y_high = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) >> 8; >> - u8 start_y_low = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) & 0xff; >> - >> - u8 width_high = OV5642_WIDTH >> 8; >> - u8 width_low = OV5642_WIDTH & 0xff; >> - u8 height_high = OV5642_HEIGHT >> 8; >> - u8 height_low = OV5642_HEIGHT & 0xff; >> - >> - u8 total_width_high = OV5642_TOTAL_WIDTH >> 8; >> - u8 total_width_low = OV5642_TOTAL_WIDTH & 0xff; >> - u8 total_height_high = OV5642_TOTAL_HEIGHT >> 8; >> - u8 total_height_low = OV5642_TOTAL_HEIGHT & 0xff; >> - >> - ret = reg_write(client, REG_WINDOW_START_X_HIGH, start_x_high); >> - if (!ret) >> - ret = reg_write(client, REG_WINDOW_START_X_LOW, start_x_low); >> - if (!ret) >> - ret = reg_write(client, REG_WINDOW_START_Y_HIGH, start_y_high); >> - if (!ret) >> - ret = reg_write(client, REG_WINDOW_START_Y_LOW, start_y_low); >> >> + /* This should set the starting point for cropping. Doesn't work so far. >> */ + ret = reg_write16(client, REG_WINDOW_START_X_HIGH, start_x); >> if (!ret) >> - ret = reg_write(client, REG_WINDOW_WIDTH_HIGH, width_high); >> - if (!ret) >> - ret = reg_write(client, REG_WINDOW_WIDTH_LOW , width_low); >> + ret = reg_write16(client, REG_WINDOW_START_Y_HIGH, start_y); >> + if (!ret) { >> + priv->crop_rect.left = start_x; >> + priv->crop_rect.top = start_y; >> + } >> + >> if (!ret) >> - ret = reg_write(client, REG_WINDOW_HEIGHT_HIGH, height_high); >> + ret = reg_write16(client, REG_WINDOW_WIDTH_HIGH, width); >> if (!ret) >> - ret = reg_write(client, REG_WINDOW_HEIGHT_LOW, height_low); >> + ret = reg_write16(client, REG_WINDOW_HEIGHT_HIGH, height); >> + if (ret) >> + return ret; >> + priv->crop_rect.width = width; >> + priv->crop_rect.height = height; >> >> + /* Set the output window size. Only 1:1 scale is supported so far. */ >> + ret = reg_write16(client, REG_OUT_WIDTH_HIGH, width); >> if (!ret) >> - ret = reg_write(client, REG_OUT_WIDTH_HIGH, width_high); >> - if (!ret) >> - ret = reg_write(client, REG_OUT_WIDTH_LOW , width_low); >> + ret = reg_write16(client, REG_OUT_HEIGHT_HIGH, height); >> + >> + /* Total width = output size + blanking */ >> if (!ret) >> - ret = reg_write(client, REG_OUT_HEIGHT_HIGH, height_high); >> + ret = reg_write16(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width); >> if (!ret) >> - ret = reg_write(client, REG_OUT_HEIGHT_LOW, height_low); >> + ret = reg_write16(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height); >> >> + /* set the maximum integration time */ >> if (!ret) >> - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width_high); >> - if (!ret) >> - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_LOW, total_width_low); >> + ret = reg_write16(client, REG_EXTEND_FRAME_TIME_HIGH, >> + total_height); >> + >> + /* Sets the window for AWB calculations */ >> if (!ret) >> - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height_high); >> + ret = reg_write16(client, REG_AVG_WINDOW_END_X_HIGH, width); >> if (!ret) >> - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_LOW, total_height_low); >> + ret = reg_write16(client, REG_AVG_WINDOW_END_Y_HIGH, height); >> >> return ret; >> } >> >> -static int ov5642_try_fmt(struct v4l2_subdev *sd, >> - struct v4l2_mbus_framefmt *mf) >> +static int ov5642_try_fmt(struct v4l2_subdev *sd, struct >> v4l2_mbus_framefmt *mf) { >> - const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); >> + const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); >> >> - dev_dbg(sd->v4l2_dev->dev, "%s(%u) width: %u heigth: %u\n", >> + dev_dbg(sd->v4l2_dev->dev, "%s(%u) request width: %u heigth: %u\n", >> + __func__, mf->code, mf->width, mf->height); >> + >> + v4l_bound_align_image(&mf->width, 48, OV5642_MAX_WIDTH, 1, >> + &mf->height, 32, OV5642_MAX_HEIGHT, 1, 0); >> + >> + dev_dbg(sd->v4l2_dev->dev, "%s(%u) return width: %u heigth: %u\n", >> __func__, mf->code, mf->width, mf->height); >> >> if (!fmt) { >> @@ -753,20 +822,16 @@ static int ov5642_try_fmt(struct v4l2_subdev *sd, >> mf->colorspace = ov5642_colour_fmts[0].colorspace; >> } >> >> - mf->width = OV5642_WIDTH; >> - mf->height = OV5642_HEIGHT; >> mf->field = V4L2_FIELD_NONE; >> >> return 0; >> } >> >> -static int ov5642_s_fmt(struct v4l2_subdev *sd, >> - struct v4l2_mbus_framefmt *mf) >> +static int ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt >> *mf) { >> struct i2c_client *client = v4l2_get_subdevdata(sd); >> struct ov5642 *priv = to_ov5642(client); >> - >> - dev_dbg(sd->v4l2_dev->dev, "%s(%u)\n", __func__, mf->code); >> + int ret; >> >> /* MIPI CSI could have changed the format, double-check */ >> if (!ov5642_find_datafmt(mf->code)) >> @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, >> >> ov5642_try_fmt(sd, mf); >> >> + priv->out_size.width = mf->width; >> + priv->out_size.height = mf->height; > > It looks like to me (but I may be wrong) that you achieve different > resolutions using cropping, not scaling. If that's correct you should > implement s_crop support and refuse changing the resolution through s_fmt. Well we had that discussion :-) Now it is handled by s_crop and s_fmt just returns the current settings. >> + priv->out_size.total_width = mf->width + BLANKING_EXTRA_WIDTH; >> + priv->out_size.total_height = max_t(int, mf->height + >> + BLANKING_EXTRA_HEIGHT, >> + BLANKING_MIN_HEIGHT); > > As you only use those two values once, maybe you can compute them in > ov5642_set_resolution() instead of storing them in the device data structure. I agree in general. As I use it with the next patch, I wonder if this cosmetic inaccuracy is tolerable. >> + priv->crop_rect.width = mf->width; >> + priv->crop_rect.height = mf->height; >> + >> priv->fmt = ov5642_find_datafmt(mf->code); >> >> - ov5642_write_array(client, ov5642_default_regs_init); >> - ov5642_set_resolution(client); >> - ov5642_write_array(client, ov5642_default_regs_finalise); >> + ret = ov5642_write_array(client, ov5642_default_regs_init); >> + if (!ret) >> + ret = ov5642_set_resolution(sd); >> + if (!ret) >> + ret = ov5642_write_array(client, ov5642_default_regs_finalise); >> >> - return 0; >> + return ret; >> } >> >> -static int ov5642_g_fmt(struct v4l2_subdev *sd, >> - struct v4l2_mbus_framefmt *mf) >> +static int ov5642_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt >> *mf) { >> struct i2c_client *client = v4l2_get_subdevdata(sd); >> struct ov5642 *priv = to_ov5642(client); >> @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, >> >> mf->code = fmt->code; >> mf->colorspace = fmt->colorspace; >> - mf->width = OV5642_WIDTH; >> - mf->height = OV5642_HEIGHT; >> + mf->width = priv->out_size.width; >> + mf->height = priv->out_size.height; >> mf->field = V4L2_FIELD_NONE; >> >> + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", __func__, >> + mf->width, mf->height); > > Isn't that a bit too verbose ? Printing the format in a debug message in the > s_fmt handler is useful, but maybe doing it in g_fmt is a bit too much. Removed. >> return 0; >> } >> >> @@ -829,14 +906,17 @@ static int ov5642_g_chip_ident(struct v4l2_subdev >> *sd, >> >> static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) >> { >> + struct i2c_client *client = v4l2_get_subdevdata(sd); >> + struct ov5642 *priv = to_ov5642(client); >> struct v4l2_rect *rect = &a->c; >> - >> a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> - rect->top = 0; >> - rect->left = 0; >> - rect->width = OV5642_WIDTH; >> - rect->height = OV5642_HEIGHT; >> + rect->top = priv->crop_rect.top; >> + rect->left = priv->crop_rect.left; >> + rect->width = priv->crop_rect.width; >> + rect->height = priv->crop_rect.height; > > rect = priv->crop_rect; > > should do. Changed. >> >> + dev_dbg(sd->v4l2_dev->dev, "%s crop width: %u heigth: %u\n", __func__, >> + rect->width, rect->height); > > Same comment as for g_fmt. Removed as well. >> return 0; >> } >> >> @@ -844,8 +924,8 @@ static int ov5642_cropcap(struct v4l2_subdev *sd, >> struct v4l2_cropcap *a) { >> a->bounds.left = 0; >> a->bounds.top = 0; >> - a->bounds.width = OV5642_WIDTH; >> - a->bounds.height = OV5642_HEIGHT; >> + a->bounds.width = OV5642_MAX_WIDTH; >> + a->bounds.height = OV5642_MAX_HEIGHT; >> a->defrect = a->bounds; >> a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> a->pixelaspect.numerator = 1; >> @@ -858,9 +938,8 @@ static int ov5642_g_mbus_config(struct v4l2_subdev *sd, >> struct v4l2_mbus_config *cfg) >> { >> cfg->type = V4L2_MBUS_CSI2; >> - cfg->flags = V4L2_MBUS_CSI2_2_LANE | >> - V4L2_MBUS_CSI2_CHANNEL_0 | >> - V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; >> + cfg->flags = V4L2_MBUS_CSI2_2_LANE | V4L2_MBUS_CSI2_CHANNEL_0 | >> + V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; >> >> return 0; >> } >> @@ -941,8 +1020,15 @@ static int ov5642_probe(struct i2c_client *client, >> >> v4l2_i2c_subdev_init(&priv->subdev, client, &ov5642_subdev_ops); >> >> - icd->ops = NULL; >> - priv->fmt = &ov5642_colour_fmts[0]; >> + icd->ops = NULL; >> + priv->fmt = &ov5642_colour_fmts[0]; >> + >> + priv->crop_rect.width = OV5642_DEFAULT_WIDTH; >> + priv->crop_rect.height = OV5642_DEFAULT_HEIGHT; >> + priv->crop_rect.left = (OV5642_MAX_WIDTH - OV5642_DEFAULT_WIDTH) / 2; >> + priv->crop_rect.top = (OV5642_MAX_HEIGHT - OV5642_DEFAULT_HEIGHT) / 2; >> + priv->out_size.width = OV5642_DEFAULT_WIDTH; >> + priv->out_size.height = OV5642_DEFAULT_HEIGHT; >> >> ret = ov5642_video_probe(icd, client); >> if (ret < 0) >> @@ -951,6 +1037,7 @@ static int ov5642_probe(struct i2c_client *client, >> return 0; >> >> error: >> + icd->ops = NULL; >> kfree(priv); >> return ret; >> } >> @@ -961,6 +1048,7 @@ static int ov5642_remove(struct i2c_client *client) >> struct soc_camera_device *icd = client->dev.platform_data; >> struct soc_camera_link *icl = to_soc_camera_link(icd); >> >> + icd->ops = NULL; >> if (icl->free_bus) >> icl->free_bus(icl); >> kfree(priv); > > -- > Regards, > > Laurent Pinchart > Thanks for the review, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c index 6410bda..1b40d90 100644 --- a/drivers/media/video/ov5642.c +++ b/drivers/media/video/ov5642.c @@ -14,8 +14,10 @@ * published by the Free Software Foundation. */ +#include <linux/bitops.h> #include <linux/delay.h> #include <linux/i2c.h> +#include <linux/kernel.h> #include <linux/slab.h> #include <linux/videodev2.h> @@ -28,13 +30,25 @@ #define REG_CHIP_ID_HIGH 0x300a #define REG_CHIP_ID_LOW 0x300b +#define REG_RED_GAIN_HIGH 0x3400 +#define REG_RED_GAIN_LOW 0x3401 +#define REG_BLUE_GAIN_HIGH 0x3404 +#define REG_BLUE_GAIN_LOW 0x3405 +#define REG_AWB_MANUAL 0x3406 +#define REG_EXP_HIGH 0x3500 +#define REG_EXP_MIDDLE 0x3501 +#define REG_EXP_LOW 0x3502 +#define REG_EXP_GAIN_CTRL 0x3503 +#define REG_GAIN 0x350b +#define REG_EXTEND_FRAME_TIME_HIGH 0x350c +#define REG_EXTEND_FRAME_TIME_LOW 0x350d #define REG_WINDOW_START_X_HIGH 0x3800 #define REG_WINDOW_START_X_LOW 0x3801 #define REG_WINDOW_START_Y_HIGH 0x3802 #define REG_WINDOW_START_Y_LOW 0x3803 #define REG_WINDOW_WIDTH_HIGH 0x3804 #define REG_WINDOW_WIDTH_LOW 0x3805 -#define REG_WINDOW_HEIGHT_HIGH 0x3806 +#define REG_WINDOW_HEIGHT_HIGH 0x3806 #define REG_WINDOW_HEIGHT_LOW 0x3807 #define REG_OUT_WIDTH_HIGH 0x3808 #define REG_OUT_WIDTH_LOW 0x3809 @@ -44,19 +58,56 @@ #define REG_OUT_TOTAL_WIDTH_LOW 0x380d #define REG_OUT_TOTAL_HEIGHT_HIGH 0x380e #define REG_OUT_TOTAL_HEIGHT_LOW 0x380f +#define REG_FLIP_SUBSAMPLE 0x3818 +#define REG_OUTPUT_FORMAT 0x4300 +#define REG_ISP_CTRL_01 0x5001 +#define REG_DIGITAL_EFFECTS 0x5580 +#define REG_HUE_COS 0x5581 +#define REG_HUE_SIN 0x5582 +#define REG_BLUE_SATURATION 0x5583 +#define REG_RED_SATURATION 0x5584 +#define REG_CONTRAST 0x5588 +#define REG_BRIGHTNESS 0x5589 +#define REG_D_E_AUXILLARY 0x558a +#define REG_AVG_WINDOW_END_X_HIGH 0x5682 +#define REG_AVG_WINDOW_END_X_LOW 0x5683 +#define REG_AVG_WINDOW_END_Y_HIGH 0x5686 +#define REG_AVG_WINDOW_END_Y_LOW 0x5687 + +/* active pixel array size */ +#define OV5642_SENSOR_SIZE_X 2592 +#define OV5642_SENSOR_SIZE_Y 1944 + +/* current maximum working size */ +#define OV5642_MAX_WIDTH OV5642_SENSOR_SIZE_X +#define OV5642_MAX_HEIGHT 720 + +/* default sizes */ +#define OV5642_DEFAULT_WIDTH 1280 +#define OV5642_DEFAULT_HEIGHT OV5642_MAX_HEIGHT + +/* minimum extra blanking */ +#define BLANKING_EXTRA_WIDTH 500 +#define BLANKING_EXTRA_HEIGHT 20 /* - * define standard resolution. - * Works currently only for up to 720 lines - * eg. 320x240, 640x480, 800x600, 1280x720, 2048x720 + * the sensor's autoexposure is buggy when setting total_height low. + * It tries to expose longer than 1 frame period without taking care of it + * and this leads to weird output. So we set 1000 lines as minimum. */ -#define OV5642_WIDTH 1280 -#define OV5642_HEIGHT 720 -#define OV5642_TOTAL_WIDTH 3200 -#define OV5642_TOTAL_HEIGHT 2000 -#define OV5642_SENSOR_SIZE_X 2592 -#define OV5642_SENSOR_SIZE_Y 1944 +#define BLANKING_MIN_HEIGHT 1000 + +/* + * About OV5642 resolution, cropping and binning: + * This sensor supports it all, at least in the feature description. + * Unfortunately, no combination of appropriate registers settings could make + * the chip work the intended way. As it works with predefined register lists, + * some undocumented registers are presumably changed there to achieve their + * goals. + * This driver currently only works for resolutions up to 720 lines with a + * 1:1 scale. Hopefully these restrictions will be removed in the future. + */ struct regval_list { u16 reg_num; @@ -105,10 +156,8 @@ static struct regval_list ov5642_default_regs_init[] = { { 0x471d, 0x5 }, { 0x4708, 0x6 }, { 0x370c, 0xa0 }, - { 0x5687, 0x94 }, { 0x501f, 0x0 }, { 0x5000, 0x4f }, - { 0x5001, 0xcf }, { 0x4300, 0x30 }, { 0x4300, 0x30 }, { 0x460b, 0x35 }, @@ -121,11 +170,8 @@ static struct regval_list ov5642_default_regs_init[] = { { 0x4402, 0x90 }, { 0x460c, 0x22 }, { 0x3815, 0x44 }, - { 0x3503, 0x7 }, { 0x3501, 0x73 }, { 0x3502, 0x80 }, - { 0x350b, 0x0 }, - { 0x3818, 0xc8 }, { 0x3824, 0x11 }, { 0x3a00, 0x78 }, { 0x3a1a, 0x4 }, @@ -140,12 +186,6 @@ static struct regval_list ov5642_default_regs_init[] = { { 0x350d, 0xd0 }, { 0x3a0d, 0x8 }, { 0x3a0e, 0x6 }, - { 0x3500, 0x0 }, - { 0x3501, 0x0 }, - { 0x3502, 0x0 }, - { 0x350a, 0x0 }, - { 0x350b, 0x0 }, - { 0x3503, 0x0 }, { 0x3a0f, 0x3c }, { 0x3a10, 0x32 }, { 0x3a1b, 0x3c }, @@ -298,7 +338,7 @@ static struct regval_list ov5642_default_regs_init[] = { { 0x54b7, 0xdf }, { 0x5402, 0x3f }, { 0x5403, 0x0 }, - { 0x3406, 0x0 }, + { REG_AWB_MANUAL, 0x0 }, { 0x5180, 0xff }, { 0x5181, 0x52 }, { 0x5182, 0x11 }, @@ -515,7 +555,6 @@ static struct regval_list ov5642_default_regs_init[] = { { 0x5088, 0x0 }, { 0x5089, 0x0 }, { 0x302b, 0x0 }, - { 0x3503, 0x7 }, { 0x3011, 0x8 }, { 0x350c, 0x2 }, { 0x350d, 0xe4 }, @@ -526,7 +565,6 @@ static struct regval_list ov5642_default_regs_init[] = { static struct regval_list ov5642_default_regs_finalise[] = { { 0x3810, 0xc2 }, - { 0x3818, 0xc9 }, { 0x381c, 0x10 }, { 0x381d, 0xa0 }, { 0x381e, 0x5 }, @@ -541,23 +579,20 @@ static struct regval_list ov5642_default_regs_finalise[] = { { 0x3a0d, 0x2 }, { 0x3a0e, 0x1 }, { 0x401c, 0x4 }, - { 0x5682, 0x5 }, - { 0x5683, 0x0 }, - { 0x5686, 0x2 }, - { 0x5687, 0xcc }, - { 0x5001, 0x4f }, + { REG_ISP_CTRL_01, 0xff }, + { REG_DIGITAL_EFFECTS, 0x6 }, { 0x589b, 0x6 }, { 0x589a, 0xc5 }, - { 0x3503, 0x0 }, + { REG_EXP_GAIN_CTRL, 0x0 }, { 0x460c, 0x20 }, { 0x460b, 0x37 }, { 0x471c, 0xd0 }, { 0x471d, 0x5 }, { 0x3815, 0x1 }, - { 0x3818, 0xc1 }, + { REG_FLIP_SUBSAMPLE, 0xc1 }, { 0x501f, 0x0 }, { 0x5002, 0xe0 }, - { 0x4300, 0x32 }, /* UYVY */ + { REG_OUTPUT_FORMAT, 0x32 }, { 0x3002, 0x1c }, { 0x4800, 0x14 }, { 0x4801, 0xf }, @@ -578,9 +613,20 @@ struct ov5642_datafmt { enum v4l2_colorspace colorspace; }; +/* the output resolution and blanking information */ +struct ov5642_out_size { + int width; + int height; + int total_width; + int total_height; +}; + struct ov5642 { struct v4l2_subdev subdev; + const struct ov5642_datafmt *fmt; + struct v4l2_rect crop_rect; + struct ov5642_out_size out_size; }; static const struct ov5642_datafmt ov5642_colour_fmts[] = { @@ -593,8 +639,7 @@ static struct ov5642 *to_ov5642(const struct i2c_client *client) } /* Find a data format by a pixel code in an array */ -static const struct ov5642_datafmt - *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) +static const struct ov5642_datafmt *ov5642_find_datafmt(enum v4l2_mbus_pixelcode code) { int i; @@ -641,6 +686,26 @@ static int reg_write(struct i2c_client *client, u16 reg, u8 val) return 0; } + +/* + * convenience function to write 16 bit register values that are split up + * into two consecutive high and low parts + */ +static int reg_write16(struct i2c_client *client, u16 reg, u16 val16) +{ + int ret; + u8 val8; + + val8 = val16 >> 8; + ret = reg_write(client, reg, val8); + if (ret) + return ret; + val8 = val16 & 0x00ff; + ret = reg_write(client, reg + 1, val8); + + return ret; +} + #ifdef CONFIG_VIDEO_ADV_DEBUG static int ov5642_get_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg) { @@ -684,68 +749,72 @@ static int ov5642_write_array(struct i2c_client *client, return 0; } -static int ov5642_set_resolution(struct i2c_client *client) +static int ov5642_set_resolution(struct v4l2_subdev *sd) { + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov5642 *priv = to_ov5642(client); + int width = priv->out_size.width; + int height = priv->out_size.height; + int total_width = priv->out_size.total_width; + int total_height = priv->out_size.total_height; + int start_x = (OV5642_SENSOR_SIZE_X - width) / 2; + int start_y = (OV5642_SENSOR_SIZE_Y - height) / 2; int ret; - u8 start_x_high = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) >> 8; - u8 start_x_low = ((OV5642_SENSOR_SIZE_X - OV5642_WIDTH) / 2) & 0xff; - u8 start_y_high = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) >> 8; - u8 start_y_low = ((OV5642_SENSOR_SIZE_Y - OV5642_HEIGHT) / 2) & 0xff; - - u8 width_high = OV5642_WIDTH >> 8; - u8 width_low = OV5642_WIDTH & 0xff; - u8 height_high = OV5642_HEIGHT >> 8; - u8 height_low = OV5642_HEIGHT & 0xff; - - u8 total_width_high = OV5642_TOTAL_WIDTH >> 8; - u8 total_width_low = OV5642_TOTAL_WIDTH & 0xff; - u8 total_height_high = OV5642_TOTAL_HEIGHT >> 8; - u8 total_height_low = OV5642_TOTAL_HEIGHT & 0xff; - - ret = reg_write(client, REG_WINDOW_START_X_HIGH, start_x_high); - if (!ret) - ret = reg_write(client, REG_WINDOW_START_X_LOW, start_x_low); - if (!ret) - ret = reg_write(client, REG_WINDOW_START_Y_HIGH, start_y_high); - if (!ret) - ret = reg_write(client, REG_WINDOW_START_Y_LOW, start_y_low); + /* This should set the starting point for cropping. Doesn't work so far. */ + ret = reg_write16(client, REG_WINDOW_START_X_HIGH, start_x); if (!ret) - ret = reg_write(client, REG_WINDOW_WIDTH_HIGH, width_high); - if (!ret) - ret = reg_write(client, REG_WINDOW_WIDTH_LOW , width_low); + ret = reg_write16(client, REG_WINDOW_START_Y_HIGH, start_y); + if (!ret) { + priv->crop_rect.left = start_x; + priv->crop_rect.top = start_y; + } + if (!ret) - ret = reg_write(client, REG_WINDOW_HEIGHT_HIGH, height_high); + ret = reg_write16(client, REG_WINDOW_WIDTH_HIGH, width); if (!ret) - ret = reg_write(client, REG_WINDOW_HEIGHT_LOW, height_low); + ret = reg_write16(client, REG_WINDOW_HEIGHT_HIGH, height); + if (ret) + return ret; + priv->crop_rect.width = width; + priv->crop_rect.height = height; + /* Set the output window size. Only 1:1 scale is supported so far. */ + ret = reg_write16(client, REG_OUT_WIDTH_HIGH, width); if (!ret) - ret = reg_write(client, REG_OUT_WIDTH_HIGH, width_high); - if (!ret) - ret = reg_write(client, REG_OUT_WIDTH_LOW , width_low); + ret = reg_write16(client, REG_OUT_HEIGHT_HIGH, height); + + /* Total width = output size + blanking */ if (!ret) - ret = reg_write(client, REG_OUT_HEIGHT_HIGH, height_high); + ret = reg_write16(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width); if (!ret) - ret = reg_write(client, REG_OUT_HEIGHT_LOW, height_low); + ret = reg_write16(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height); + /* set the maximum integration time */ if (!ret) - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_HIGH, total_width_high); - if (!ret) - ret = reg_write(client, REG_OUT_TOTAL_WIDTH_LOW, total_width_low); + ret = reg_write16(client, REG_EXTEND_FRAME_TIME_HIGH, + total_height); + + /* Sets the window for AWB calculations */ if (!ret) - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_HIGH, total_height_high); + ret = reg_write16(client, REG_AVG_WINDOW_END_X_HIGH, width); if (!ret) - ret = reg_write(client, REG_OUT_TOTAL_HEIGHT_LOW, total_height_low); + ret = reg_write16(client, REG_AVG_WINDOW_END_Y_HIGH, height); return ret; } -static int ov5642_try_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *mf) +static int ov5642_try_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) { - const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); + const struct ov5642_datafmt *fmt = ov5642_find_datafmt(mf->code); - dev_dbg(sd->v4l2_dev->dev, "%s(%u) width: %u heigth: %u\n", + dev_dbg(sd->v4l2_dev->dev, "%s(%u) request width: %u heigth: %u\n", + __func__, mf->code, mf->width, mf->height); + + v4l_bound_align_image(&mf->width, 48, OV5642_MAX_WIDTH, 1, + &mf->height, 32, OV5642_MAX_HEIGHT, 1, 0); + + dev_dbg(sd->v4l2_dev->dev, "%s(%u) return width: %u heigth: %u\n", __func__, mf->code, mf->width, mf->height); if (!fmt) { @@ -753,20 +822,16 @@ static int ov5642_try_fmt(struct v4l2_subdev *sd, mf->colorspace = ov5642_colour_fmts[0].colorspace; } - mf->width = OV5642_WIDTH; - mf->height = OV5642_HEIGHT; mf->field = V4L2_FIELD_NONE; return 0; } -static int ov5642_s_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *mf) +static int ov5642_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) { struct i2c_client *client = v4l2_get_subdevdata(sd); struct ov5642 *priv = to_ov5642(client); - - dev_dbg(sd->v4l2_dev->dev, "%s(%u)\n", __func__, mf->code); + int ret; /* MIPI CSI could have changed the format, double-check */ if (!ov5642_find_datafmt(mf->code)) @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct v4l2_subdev *sd, ov5642_try_fmt(sd, mf); + priv->out_size.width = mf->width; + priv->out_size.height = mf->height; + priv->out_size.total_width = mf->width + BLANKING_EXTRA_WIDTH; + priv->out_size.total_height = max_t(int, mf->height + + BLANKING_EXTRA_HEIGHT, + BLANKING_MIN_HEIGHT); + priv->crop_rect.width = mf->width; + priv->crop_rect.height = mf->height; + priv->fmt = ov5642_find_datafmt(mf->code); - ov5642_write_array(client, ov5642_default_regs_init); - ov5642_set_resolution(client); - ov5642_write_array(client, ov5642_default_regs_finalise); + ret = ov5642_write_array(client, ov5642_default_regs_init); + if (!ret) + ret = ov5642_set_resolution(sd); + if (!ret) + ret = ov5642_write_array(client, ov5642_default_regs_finalise); - return 0; + return ret; } -static int ov5642_g_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *mf) +static int ov5642_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) { struct i2c_client *client = v4l2_get_subdevdata(sd); struct ov5642 *priv = to_ov5642(client); @@ -793,10 +868,12 @@ static int ov5642_g_fmt(struct v4l2_subdev *sd, mf->code = fmt->code; mf->colorspace = fmt->colorspace; - mf->width = OV5642_WIDTH; - mf->height = OV5642_HEIGHT; + mf->width = priv->out_size.width; + mf->height = priv->out_size.height; mf->field = V4L2_FIELD_NONE; + dev_dbg(sd->v4l2_dev->dev, "%s return width: %u heigth: %u\n", __func__, + mf->width, mf->height); return 0; } @@ -829,14 +906,17 @@ static int ov5642_g_chip_ident(struct v4l2_subdev *sd, static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) { + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov5642 *priv = to_ov5642(client); struct v4l2_rect *rect = &a->c; - a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - rect->top = 0; - rect->left = 0; - rect->width = OV5642_WIDTH; - rect->height = OV5642_HEIGHT; + rect->top = priv->crop_rect.top; + rect->left = priv->crop_rect.left; + rect->width = priv->crop_rect.width; + rect->height = priv->crop_rect.height; + dev_dbg(sd->v4l2_dev->dev, "%s crop width: %u heigth: %u\n", __func__, + rect->width, rect->height); return 0; } @@ -844,8 +924,8 @@ static int ov5642_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a) { a->bounds.left = 0; a->bounds.top = 0; - a->bounds.width = OV5642_WIDTH; - a->bounds.height = OV5642_HEIGHT; + a->bounds.width = OV5642_MAX_WIDTH; + a->bounds.height = OV5642_MAX_HEIGHT; a->defrect = a->bounds; a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; a->pixelaspect.numerator = 1; @@ -858,9 +938,8 @@ static int ov5642_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg) { cfg->type = V4L2_MBUS_CSI2; - cfg->flags = V4L2_MBUS_CSI2_2_LANE | - V4L2_MBUS_CSI2_CHANNEL_0 | - V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; + cfg->flags = V4L2_MBUS_CSI2_2_LANE | V4L2_MBUS_CSI2_CHANNEL_0 | + V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; return 0; } @@ -941,8 +1020,15 @@ static int ov5642_probe(struct i2c_client *client, v4l2_i2c_subdev_init(&priv->subdev, client, &ov5642_subdev_ops); - icd->ops = NULL; - priv->fmt = &ov5642_colour_fmts[0]; + icd->ops = NULL; + priv->fmt = &ov5642_colour_fmts[0]; + + priv->crop_rect.width = OV5642_DEFAULT_WIDTH; + priv->crop_rect.height = OV5642_DEFAULT_HEIGHT; + priv->crop_rect.left = (OV5642_MAX_WIDTH - OV5642_DEFAULT_WIDTH) / 2; + priv->crop_rect.top = (OV5642_MAX_HEIGHT - OV5642_DEFAULT_HEIGHT) / 2; + priv->out_size.width = OV5642_DEFAULT_WIDTH; + priv->out_size.height = OV5642_DEFAULT_HEIGHT; ret = ov5642_video_probe(icd, client); if (ret < 0) @@ -951,6 +1037,7 @@ static int ov5642_probe(struct i2c_client *client, return 0; error: + icd->ops = NULL; kfree(priv); return ret; } @@ -961,6 +1048,7 @@ static int ov5642_remove(struct i2c_client *client) struct soc_camera_device *icd = client->dev.platform_data; struct soc_camera_link *icl = to_soc_camera_link(icd); + icd->ops = NULL; if (icl->free_bus) icl->free_bus(icl); kfree(priv);
This patch adds the ability to get arbitrary resolutions with a width up to 2592 and a height up to 720 pixels instead of the standard 1280x720 only. Signed-off-by: Bastian Hecht <hechtb@gmail.com> --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html