Janusz Krzysztofik (14): media: ov6650: Fix MODDULE_DESCRIPTION media: ov6650: Fix control handler not freed on init error media: ov6650: Fix unverified arguments used in .set_fmt() media: ov6650: Fix unverified arguments accepted by .get_fmt() media: ov6650: Fix unverified arguments accepted by .enum_mbus_code() media: ov6650: Fix unverified pad IDs accepted by .get/set_selectioon() media: ov6650: Fix unverified pad IDs accepted by .g/s_frame_interval() media: ov6650: Fix crop rectangle alignment not passed back media: ov6650: Fix incorrect use of JPEG colorspace media: ov6650: Fix some format attributes not under control media: ov6650: Fix .get_fmt() V4L2_SUBDEV_FORMAT_TRY support media: ov6650: Fix default format not applied on device probe media: ov6650: Fix stored frame format not in sync with hardware media: ov6650: Fix stored crop rectangle not in sync with hardware
drivers/media/i2c/ov6650.c | 183 ++++++++++++++++++++++++++----------- 1 file changed, 131 insertions(+), 52 deletions(-)
Commit 23a52386fabe ("media: ov6650: convert to standalone v4l2 subdevice") converted the driver from a soc_camera sensor to a standalone V4L subdevice driver. Unfortunately, module description was not updated to reflect the change. Fix it.
While being at it, update email address of the module author.
Fixes: 23a52386fabe ("media: ov6650: convert to standalone v4l2 subdevice") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 1b972e591b48..a3d00afcb0c8 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -1045,6 +1045,6 @@ static struct i2c_driver ov6650_i2c_driver = {
module_i2c_driver(ov6650_i2c_driver);
-MODULE_DESCRIPTION("SoC Camera driver for OmniVision OV6650"); -MODULE_AUTHOR("Janusz Krzysztofik jkrzyszt@tis.icnet.pl"); +MODULE_DESCRIPTION("V4L2 subdevice driver for OmniVision OV6650 camera sensor"); +MODULE_AUTHOR("Janusz Krzysztofik <jmkrzyszt@gmail.com"); MODULE_LICENSE("GPL v2");
Since commit afd9690c72c3 ("[media] ov6650: convert to the control framework"), if an error occurs during initialization of a control handler, resources possibly allocated to the handler are not freed before device initialiaton is aborted. Fix it.
Fixes: afd9690c72c3 ("[media] ov6650: convert to the control framework") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index a3d00afcb0c8..007f0ca24913 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -992,8 +992,10 @@ static int ov6650_probe(struct i2c_client *client, V4L2_CID_GAMMA, 0, 0xff, 1, 0x12);
priv->subdev.ctrl_handler = &priv->hdl; - if (priv->hdl.error) - return priv->hdl.error; + if (priv->hdl.error) { + ret = priv->hdl.error; + goto ectlhdlfree; + }
v4l2_ctrl_auto_cluster(2, &priv->autogain, 0, true); v4l2_ctrl_auto_cluster(3, &priv->autowb, 0, true); @@ -1012,8 +1014,10 @@ static int ov6650_probe(struct i2c_client *client, priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
ret = v4l2_async_register_subdev(&priv->subdev); - if (ret) - v4l2_ctrl_handler_free(&priv->hdl); + if (!ret) + return 0; +ectlhdlfree: + v4l2_ctrl_handler_free(&priv->hdl);
return ret; }
Commit 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") converted a former ov6650_try_fmt() video operation callback to an ov6650_set_fmt() pad operation callback. However, the function does not verify correctness of user provided format->which flag and pad config pointer arguments. Fix it.
Fixes: 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 007f0ca24913..3062c9a6c57b 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -679,6 +679,17 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL;
+ switch (format->which) { + case V4L2_SUBDEV_FORMAT_ACTIVE: + break; + case V4L2_SUBDEV_FORMAT_TRY: + if (cfg) + break; + /* fall through */ + default: + return -EINVAL; + } + if (is_unscaled_ok(mf->width, mf->height, &priv->rect)) v4l_bound_align_image(&mf->width, 2, W_CIF, 1, &mf->height, 2, H_CIF, 1, 0);
Hi Janusz,
On Mon, Apr 08, 2019 at 11:42:31PM +0200, Janusz Krzysztofik wrote:
Commit 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") converted a former ov6650_try_fmt() video operation callback to an ov6650_set_fmt() pad operation callback. However, the function does not verify correctness of user provided format->which flag and pad config pointer arguments. Fix it.
Fixes: 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org
drivers/media/i2c/ov6650.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 007f0ca24913..3062c9a6c57b 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -679,6 +679,17 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL;
- switch (format->which) {
- case V4L2_SUBDEV_FORMAT_ACTIVE:
break;- case V4L2_SUBDEV_FORMAT_TRY:
if (cfg)break;/* fall through */- default:
return -EINVAL;- }
For this to return an error, there would need to be a problem on the caller's side. In other words, this isn't supposed to happen.
Instead of adding such checks to all drivers, I think they instead should be added to the caller's side. The checks already exist for uAPI, but not for other drivers.
The same applies to patches until 7th (including).
- if (is_unscaled_ok(mf->width, mf->height, &priv->rect)) v4l_bound_align_image(&mf->width, 2, W_CIF, 1, &mf->height, 2, H_CIF, 1, 0);
Hi Sakari,
Sorry for late answer, I've just found your message in Gmail spam folder.
On Tuesday, April 30, 2019 3:58:09 PM CEST Sakari Ailus wrote:
Hi Janusz,
On Mon, Apr 08, 2019 at 11:42:31PM +0200, Janusz Krzysztofik wrote:
Commit 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") converted a former ov6650_try_fmt() video operation callback to an ov6650_set_fmt() pad operation callback. However, the function does not verify correctness of user provided format->which flag and pad config pointer arguments. Fix it.
Fixes: 717fd5b4907ad ("[media] v4l2: replace try_mbus_fmt by set_fmt") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org
drivers/media/i2c/ov6650.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 007f0ca24913..3062c9a6c57b 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -679,6 +679,17 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL;
- switch (format->which) {
- case V4L2_SUBDEV_FORMAT_ACTIVE:
break;- case V4L2_SUBDEV_FORMAT_TRY:
if (cfg)break;/* fall through */- default:
return -EINVAL;- }
For this to return an error, there would need to be a problem on the caller's side. In other words, this isn't supposed to happen.
How about raising a bug if that happens nevertheless?
@@ -677,10 +677,20 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, struct ov6650 *priv = to_ov6650(client);
if (format->pad) return -EINVAL;
+ switch (format->which) { + case V4L2_SUBDEV_FORMAT_TRY: + BUG_ON(!cfg); + /* fall through */ + case V4L2_SUBDEV_FORMAT_ACTIVE: + break; + default: + BUG(); + } + if (is_unscaled_ok(mf->width, mf->height, &priv->rect)) v4l_bound_align_image(&mf->width, 2, W_CIF, 1, &mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE;
Thanks, Janusz
Instead of adding such checks to all drivers, I think they instead should be added to the caller's side. The checks already exist for uAPI, but not for other drivers.
The same applies to patches until 7th (including).
- if (is_unscaled_ok(mf->width, mf->height, &priv->rect)) v4l_bound_align_image(&mf->width, 2, W_CIF, 1, &mf->height, 2, H_CIF, 1, 0);
Commit da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op get_fmt") converted a former ov6650_g_fmt() video operation callback to an ov6650_get_fmt() pad operation callback. However, the function does not verify correctness of user provided format->which flag and pad config pointer arguments. Fix it.
Even if the function never dereferences the pad config pointer argument, return -EINVAL if it is NULL on V4L2_SUBDEV_FORMAT_TRY.
Fixes: da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op get_fmt") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 3062c9a6c57b..5c1738c5a847 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -515,6 +515,17 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL;
+ switch (format->which) { + case V4L2_SUBDEV_FORMAT_ACTIVE: + break; + case V4L2_SUBDEV_FORMAT_TRY: + if (cfg) + break; + /* fall through */ + default: + return -EINVAL; + } + mf->width = priv->rect.width >> priv->half_scale; mf->height = priv->rect.height >> priv->half_scale; mf->code = priv->code;
Commit ebcff5fce6b1 ("[media] v4l2: replace enum_mbus_fmt by enum_mbus_code") converted a former ov6650_enum_fmt() video operation callback to an ov6650_enum_mbus_fmt() pad operation callback. However, the function dees not verify correctness of code->which flag and pad config pointer arguments. Fix it.
Even if the function has no need to dereference the pad config pointer argument, return -EINVAL if it is NULL on V4L2_SUBDEV_FORMAT_TRY.
Fixes: ebcff5fce6b1 ("[media] v4l2: replace enum_mbus_fmt by enum_mbus_code") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 5c1738c5a847..d72fcf56930a 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -737,7 +737,21 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { - if (code->pad || code->index >= ARRAY_SIZE(ov6650_codes)) + if (code->pad) + return -EINVAL; + + switch (code->which) { + case V4L2_SUBDEV_FORMAT_ACTIVE: + break; + case V4L2_SUBDEV_FORMAT_TRY: + if (cfg) + break; + /* fall through */ + default: + return -EINVAL; + } + + if (code->index >= ARRAY_SIZE(ov6650_codes)) return -EINVAL;
code->code = ov6650_codes[code->index];
Commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") converted former ov6650_g/s_crop() video operation callbacks to ov6650_get/set_selection() pad operation callbacks. However, the new functions don't verify correctness of pad IDs passed in user arguments. Fix it.
Even if pad ID arguments are not actually used in those functions, assumed to be 0, always return -EINVAL if an operation on an invalid (non-zero) pad is requested by a user.
Fixes: 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index d72fcf56930a..5df81dec06ae 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -444,6 +444,9 @@ static int ov6650_get_selection(struct v4l2_subdev *sd, struct i2c_client *client = v4l2_get_subdevdata(sd); struct ov6650 *priv = to_ov6650(client);
+ if (sel->pad) + return -EINVAL; + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) return -EINVAL;
@@ -471,6 +474,9 @@ static int ov6650_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect = sel->r; int ret;
+ if (sel->pad) + return -EINVAL; + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE || sel->target != V4L2_SEL_TGT_CROP) return -EINVAL;
Commit 4471109e3894 ("media: convert g/s_parm to g/s_frame_interval in subdevs") comverted former ov6650_g/s_parm() video_operation_callbacks to ov6650_g/s_frame_interval() pad operation callbacks. Howeveer, the new functions don't verify correcntess of pad IDs passed in user arguments. Fix it.
Even if pad ID arguments are not actually used in those functions, assumed to be 0, always return -EINVAL if an operation on an invalid (non-zero) pad is requested by a user.
Fixes: 4471109e3894 ("media: convert g/s_parm to g/s_frame_interval in subdevs") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 5df81dec06ae..1daef0c7ad91 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -770,6 +770,9 @@ static int ov6650_g_frame_interval(struct v4l2_subdev *sd, struct i2c_client *client = v4l2_get_subdevdata(sd); struct ov6650 *priv = to_ov6650(client);
+ if (ival->pad) + return -EINVAL; + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf, priv->pclk_limit, priv->pclk_max)); ival->interval.denominator = FRAME_RATE_MAX; @@ -789,6 +792,9 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd, int div, ret; u8 clkrc;
+ if (ival->pad) + return -EINVAL; + if (tpf->numerator == 0 || tpf->denominator == 0) div = 1; /* Reset to full rate */ else
Commit 4f996594ceaf ("[media] v4l2: make vidioc_s_crop const") introduced a writable copy of constified user requested crop rectangle in order to be able to perform hardware alignments on it. Later on, commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") replaced s_crop() video operaion using that const argument with set_selection() pad operation which had a corresponding argument not constified, however the original behavior of the driver was not restored. Since that time, any hardware alignment applied on a user requested crop rectangle is not passed back to the user calling .set_selection() as it should be.
Fix the issue by dropping the copy and replacing all references to it with references to the crop rectangle embedded in the user argument.
Fixes: 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: staable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 1daef0c7ad91..07d7a0708cca 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -471,7 +471,6 @@ static int ov6650_set_selection(struct v4l2_subdev *sd, { struct i2c_client *client = v4l2_get_subdevdata(sd); struct ov6650 *priv = to_ov6650(client); - struct v4l2_rect rect = sel->r; int ret;
if (sel->pad) @@ -481,31 +480,31 @@ static int ov6650_set_selection(struct v4l2_subdev *sd, sel->target != V4L2_SEL_TGT_CROP) return -EINVAL;
- v4l_bound_align_image(&rect.width, 2, W_CIF, 1, - &rect.height, 2, H_CIF, 1, 0); - v4l_bound_align_image(&rect.left, DEF_HSTRT << 1, - (DEF_HSTRT << 1) + W_CIF - (__s32)rect.width, 1, - &rect.top, DEF_VSTRT << 1, - (DEF_VSTRT << 1) + H_CIF - (__s32)rect.height, 1, - 0); + v4l_bound_align_image(&sel->r.width, 2, W_CIF, 1, + &sel->r.height, 2, H_CIF, 1, 0); + v4l_bound_align_image(&sel->r.left, DEF_HSTRT << 1, + (DEF_HSTRT << 1) + W_CIF - (__s32)sel->r.width, 1, + &sel->r.top, DEF_VSTRT << 1, + (DEF_VSTRT << 1) + H_CIF - (__s32)sel->r.height, + 1, 0);
- ret = ov6650_reg_write(client, REG_HSTRT, rect.left >> 1); + ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1); if (!ret) { - priv->rect.left = rect.left; + priv->rect.left = sel->r.left; ret = ov6650_reg_write(client, REG_HSTOP, - (rect.left + rect.width) >> 1); + (sel->r.left + sel->r.width) >> 1); } if (!ret) { - priv->rect.width = rect.width; - ret = ov6650_reg_write(client, REG_VSTRT, rect.top >> 1); + priv->rect.width = sel->r.width; + ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1); } if (!ret) { - priv->rect.top = rect.top; + priv->rect.top = sel->r.top; ret = ov6650_reg_write(client, REG_VSTOP, - (rect.top + rect.height) >> 1); + (sel->r.top + sel->r.height) >> 1); } if (!ret) - priv->rect.height = rect.height; + priv->rect.height = sel->r.height;
return ret; }
Since its initial submission, the driver selects V4L2_COLORSPACE_JPEG for supported formats other than V4L2_MBUS_FMT_SBGGR8_1X8. According to v4l2-compliance test program, V4L2_COLORSPACE_JPEG applies exclusively to V4L2_PIX_FMT_JPEG. Since the sensor does not support JPEG format, fix it to always select V4L2_COLORSPACE_SRGB.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 07d7a0708cca..dfee07ec976e 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -203,7 +203,6 @@ struct ov6650 { unsigned long pclk_max; /* from resolution and format */ struct v4l2_fract tpf; /* as requested with s_frame_interval */ u32 code; - enum v4l2_colorspace colorspace; };
@@ -534,7 +533,7 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd, mf->width = priv->rect.width >> priv->half_scale; mf->height = priv->rect.height >> priv->half_scale; mf->code = priv->code; - mf->colorspace = priv->colorspace; + mf->colorspace = V4L2_COLORSPACE_SRGB; mf->field = V4L2_FIELD_NONE;
return 0; @@ -642,11 +641,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) priv->pclk_max = 8000000; }
- if (code == MEDIA_BUS_FMT_SBGGR8_1X8) - priv->colorspace = V4L2_COLORSPACE_SRGB; - else if (code != 0) - priv->colorspace = V4L2_COLORSPACE_JPEG; - if (half_scale) { dev_dbg(&client->dev, "max resolution: QCIF\n"); coma_set |= COMA_QCIF; @@ -677,7 +671,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
if (!ret) { - mf->colorspace = priv->colorspace; mf->width = priv->rect.width >> half_scale; mf->height = priv->rect.height >> half_scale; } @@ -711,6 +704,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, &mf->height, 2, H_CIF, 1, 0);
mf->field = V4L2_FIELD_NONE; + mf->colorspace = V4L2_COLORSPACE_SRGB;
switch (mf->code) { case MEDIA_BUS_FMT_Y10_1X10: @@ -721,13 +715,11 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, case MEDIA_BUS_FMT_YUYV8_2X8: case MEDIA_BUS_FMT_VYUY8_2X8: case MEDIA_BUS_FMT_UYVY8_2X8: - mf->colorspace = V4L2_COLORSPACE_JPEG; break; default: mf->code = MEDIA_BUS_FMT_SBGGR8_1X8; /* fall through */ case MEDIA_BUS_FMT_SBGGR8_1X8: - mf->colorspace = V4L2_COLORSPACE_SRGB; break; }
@@ -1055,7 +1047,6 @@ static int ov6650_probe(struct i2c_client *client, priv->rect.height = H_CIF; priv->half_scale = false; priv->code = MEDIA_BUS_FMT_YUYV8_2X8; - priv->colorspace = V4L2_COLORSPACE_JPEG;
priv->subdev.internal_ops = &ov6650_internal_ops; priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
User arguments passed to .get/set_fmt() pad operation callbacks may contain unsupported values. The driver takes control over frame size and pixel code as well as colorspace and field attributes but has never cared for remainig format attributes, i.e., ycbcr_enc, quantization and xfer_func, introduced by commit 11ff030c7365 ("[media] v4l2-mediabus: improve colorspace support"). Fix it.
Set up a static v4l2_mbus_framefmt structure with attributes initialized to reasonable defaults and use it for updating content of user provided arguments. In case of V4L2_SUBDEV_FORMAT_ACTIVE, postpone frame size update, now performed from inside ov6650_s_fmt() helper, util the user argument is first updated in ov6650_set_fmt() with default frame format content. For V4L2_SUBDEV_FORMAT_TRY, don't copy all attributes to pad config, only those handled by the driver, then fill the response with the default frame format updated with resulting pad config format code and frame size.
Fixes: 11ff030c7365 ("[media] v4l2-mediabus: improve colorspace support") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 51 +++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index dfee07ec976e..f6b0b77c5abf 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -215,6 +215,17 @@ static u32 ov6650_codes[] = { MEDIA_BUS_FMT_Y8_1X8, };
+static const struct v4l2_mbus_framefmt ov6650_def_fmt = { + .width = W_CIF, + .height = H_CIF, + .code = MEDIA_BUS_FMT_SBGGR8_1X8, + .colorspace = V4L2_COLORSPACE_SRGB, + .field = V4L2_FIELD_NONE, + .ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT, + .quantization = V4L2_QUANTIZATION_DEFAULT, + .xfer_func = V4L2_XFER_FUNC_DEFAULT, +}; + /* read a register */ static int ov6650_reg_read(struct i2c_client *client, u8 reg, u8 *val) { @@ -530,11 +541,13 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd, return -EINVAL; }
+ /* initialize response with default media bus frame format */ + *mf = ov6650_def_fmt; + + /* update media bus format code and frame size */ mf->width = priv->rect.width >> priv->half_scale; mf->height = priv->rect.height >> priv->half_scale; mf->code = priv->code; - mf->colorspace = V4L2_COLORSPACE_SRGB; - mf->field = V4L2_FIELD_NONE;
return 0; } @@ -670,10 +683,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) if (!ret) ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask);
- if (!ret) { - mf->width = priv->rect.width >> half_scale; - mf->height = priv->rect.height >> half_scale; - } return ret; }
@@ -703,9 +712,6 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, v4l_bound_align_image(&mf->width, 2, W_CIF, 1, &mf->height, 2, H_CIF, 1, 0);
- mf->field = V4L2_FIELD_NONE; - mf->colorspace = V4L2_COLORSPACE_SRGB; - switch (mf->code) { case MEDIA_BUS_FMT_Y10_1X10: mf->code = MEDIA_BUS_FMT_Y8_1X8; @@ -723,10 +729,31 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd, break; }
- if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) - return ov6650_s_fmt(sd, mf); - cfg->try_fmt = *mf; + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { + /* store media bus format code and frame size in pad config */ + cfg->try_fmt.width = mf->width; + cfg->try_fmt.height = mf->height; + cfg->try_fmt.code = mf->code;
+ /* return default mbus frame format updated with pad config */ + *mf = ov6650_def_fmt; + mf->width = cfg->try_fmt.width; + mf->height = cfg->try_fmt.height; + mf->code = cfg->try_fmt.code; + + } else { + /* apply new media bus format code and frame size */ + int ret = ov6650_s_fmt(sd, mf); + + if (ret) + return ret; + + /* return default format updated with active size and code */ + *mf = ov6650_def_fmt; + mf->width = priv->rect.width >> priv->half_scale; + mf->height = priv->rect.height >> priv->half_scale; + mf->code = priv->code; + } return 0; }
Commit da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op get_fmt") converted a former ov6650_g_fmt() video operation callback to an ov6650_get_fmt() pad operation callback. However, the converted function disregards a format->which flag that pad operations should obey and always returns active frame format settings.
That can be fixed by always responding to V4L2_SUBDEV_FORMAT_TRY with -EINVAL, or providing the response from a pad config argument, likely updated by a former user call to V4L2_SUBDEV_FORMAT_TRY .set_fmt(). Since implementation of the latter is trivial, go for it.
Fixes: da298c6d98d5 ("[media] v4l2: replace video op g_mbus_fmt by pad op get_fmt") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index f6b0b77c5abf..e72210653f55 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -530,25 +530,27 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd, if (format->pad) return -EINVAL;
+ /* initialize response with default media bus frame format */ + *mf = ov6650_def_fmt; + + /* update media bus format code and frame size */ switch (format->which) { case V4L2_SUBDEV_FORMAT_ACTIVE: + mf->width = priv->rect.width >> priv->half_scale; + mf->height = priv->rect.height >> priv->half_scale; + mf->code = priv->code; break; case V4L2_SUBDEV_FORMAT_TRY: - if (cfg) + if (cfg) { + mf->width = cfg->try_fmt.width; + mf->height = cfg->try_fmt.height; + mf->code = cfg->try_fmt.code; break; + } /* fall through */ default: return -EINVAL; } - - /* initialize response with default media bus frame format */ - *mf = ov6650_def_fmt; - - /* update media bus format code and frame size */ - mf->width = priv->rect.width >> priv->half_scale; - mf->height = priv->rect.height >> priv->half_scale; - mf->code = priv->code; - return 0; }
It is not clear what pixel format is actually configured in hardware on reset. MEDIA_BUS_FMT_YUYV8_2X8, assumed on device probe since the driver was intiially submitted, is for sure not the one.
Fix it by explicitly applying a known, driver default frame format just after initial device reset.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index e72210653f55..8013afea0c02 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -920,6 +920,11 @@ static int ov6650_video_probe(struct v4l2_subdev *sd) ret = ov6650_reset(client); if (!ret) ret = ov6650_prog_dflt(client); + if (!ret) { + struct v4l2_mbus_framefmt mf = ov6650_def_fmt; + + ret = ov6650_s_fmt(sd, &mf); + } if (!ret) ret = v4l2_ctrl_handler_setup(&priv->hdl);
@@ -1074,8 +1079,6 @@ static int ov6650_probe(struct i2c_client *client, priv->rect.top = DEF_VSTRT << 1; priv->rect.width = W_CIF; priv->rect.height = H_CIF; - priv->half_scale = false; - priv->code = MEDIA_BUS_FMT_YUYV8_2X8;
priv->subdev.internal_ops = &ov6650_internal_ops; priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
The driver stores frame format settings supposed to be in line with hardware state in a device private structure. Since the driver initial submission, those settings are updated before they are actually applied on hardware. If an error occurs on device update, the stored settings my not reflect hardware state anymore and consecutive calls to .get_fmt() may return incorrect information. That in turn may affect ability of a host device to use correct DMA transfer settings if such incorrect informmation on active frame format returned by .get_fmt() is used.
Assuming a failed device update means its state hasn't changed, update frame format related settings stored in the device private structure only after they are successfully applied so the stored values always reflect hardware state as closely as possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 8013afea0c02..001457d39742 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -643,7 +643,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) dev_err(&client->dev, "Pixel format not handled: 0x%x\n", code); return -EINVAL; } - priv->code = code;
if (code == MEDIA_BUS_FMT_Y8_1X8 || code == MEDIA_BUS_FMT_SBGGR8_1X8) { @@ -664,7 +663,6 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) dev_dbg(&client->dev, "max resolution: CIF\n"); coma_mask |= COMA_QCIF; } - priv->half_scale = half_scale;
clkrc = CLKRC_12MHz; mclk = 12000000; @@ -682,8 +680,13 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask); if (!ret) ret = ov6650_reg_write(client, REG_CLKRC, clkrc); - if (!ret) + if (!ret) { + priv->half_scale = half_scale; + ret = ov6650_reg_rmw(client, REG_COML, coml_set, coml_mask); + } + if (!ret) + priv->code = code;
return ret; }
The driver stores crop rectangle settings supposed to be in line with hardware state in a device private structure. Since the driver initial submission, crop rectangle width and height settings are not updated correctly when rectangle offset settings are applied on hardware. If an error occurs while the device is updated, the stored settings my no longer reflect hardware state and consecutive calls to .get_selection() as well as .get_fmt() may return incorrect information. That in turn may affect ability of a host device to use correct DMA transfer settings if such incorrect informamtion on active frame format returned by .get_fmt() is used.
Assuming a failed update of the device means its actual settings haven't changed, update crop rectangle width and height settings stored in the device private structure correctly while the rectangle offset is successfully applied on hardware so the stored values always reflect actual hardware state to the extent possible.
Fixes: 2f6e2404799a ("[media] SoC Camera: add driver for OV6650 sensor") Signed-off-by: Janusz Krzysztofik jmkrzyszt@gmail.com Cc: stable@vger.kernel.org --- drivers/media/i2c/ov6650.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c index 001457d39742..cffe6aa906b2 100644 --- a/drivers/media/i2c/ov6650.c +++ b/drivers/media/i2c/ov6650.c @@ -500,6 +500,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1); if (!ret) { + priv->rect.width += priv->rect.left - sel->r.left; priv->rect.left = sel->r.left; ret = ov6650_reg_write(client, REG_HSTOP, (sel->r.left + sel->r.width) >> 1); @@ -509,6 +510,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd, ret = ov6650_reg_write(client, REG_VSTRT, sel->r.top >> 1); } if (!ret) { + priv->rect.height += priv->rect.top - sel->r.top; priv->rect.top = sel->r.top; ret = ov6650_reg_write(client, REG_VSTOP, (sel->r.top + sel->r.height) >> 1);
linux-stable-mirror@lists.linaro.org
-
Janusz Krzysztofik -
Sakari Ailus