[PATCH 0/3] media: ov6650: Fix crop rectangle affected by set format

[PATCH 0/3] media: ov6650: Fix crop rectangle affected by set format

[Janusz Krzysztofik]

According to subdevice interface specification found in V4L2 API
documentation, set format pad operations should not affect image
geometry set in preceding image processing steps. Unfortunately, that
requirement is not respected by the driver implementation of set format
as it was not the case when that code was still implementing a pair of
now obsolete .s_mbus_fmt() / .try_mbus_fmt() video operations before
they have been merged and reused as an implementation of .set_fmt() pad
operation by commit 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt
by set_fmt").

In case of set format active processing path the issue can be fixed
easily by excluding a call to set active selection from that path. That
will effectively limit frame size processing to optimal frame scaling
against active crop rectangle without touching it.  Users can just call
set active selection themselves to obtain desired frame size.  However,
set format try processing path needs more work.

First of all, the driver should be extended with set try selection
support.  Lack of it constraints video device drivers to not use
subdevice cropping at all while processing user requested active frame
size, otherwise their set try format results might differ from active.

Next, set format try processing path should use pad config crop
rectangle as a reference, not the active one as it does now.  That
issue can be resolved easily as soon as set try selection support is
added to the driver so pad config crop rectangle can be maintained by
users via selection API.

Last, set format try processing path should give the same results as
active in respect to active vs. pad config crop rectangle geometry.
Both rectangles should be either not touched by set format (that's what
we are going to achieve) or modified the same way, otherwise users
won't be able to obtain equal results from both paths while iterating
through set format and set selection operations in order to obtain
desired frame size.

We can't begin with modifying set format pad operation as not to touch
crop rectangle since that depends on availability of set try selection
for symmetry.  Neither can we begin with adding set try selection since
that in turn depends on equal handling of active and pad config crop
rectangles by set format.  We can either implement all required
modifications in a single patch, or begin with fixing current set
format try processing path to appropriately handle pad config crop
rectangle.

This series implements the latter approach as believed to be more
readable.  However, the patches can be squashed if so decided by
subsystem maintainers.

Janusz Krzysztofik (3):
  media: ov6650: Fix set format try processing path
  media: ov6650: Add try support to selection API operations
  media: ov6650: Fix crop rectangle affected by set format

 drivers/media/i2c/ov6650.c | 115 ++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 45 deletions(-)

-- 
2.24.1

[PATCH 1/3] media: ov6650: Fix set format try processing path

[Janusz Krzysztofik]

According to subdevice interface specification found in V4L2 API
documentation, set format pad operations should not affect image
geometry set in preceding image processing steps. Unfortunately, that
requirement is not respected by the driver implementation of set format
as it was not the case when that code was still implementing a pair of
now obsolete .s_mbus_fmt() / .try_mbus_fmt() video operations before
they have been merged and reused as an implementation of .set_fmt() pad
operation by commit 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt
by set_fmt").

In case of set format active processing path the issue can be fixed
easily by excluding a call to set active selection from that path. That
will effectively limit frame size processing to optimal frame scaling
against active crop rectangle without touching it.  Users can just call
set active selection themselves to obtain desired frame size.  However,
set format try processing path needs more work.

First of all, the driver should be extended with set try selection
support.  Lack of it constraints video device drivers to not use
subdevice cropping at all while processing user requested active frame
size, otherwise their set try format results might differ from active.

Next, set format try processing path should use pad config crop
rectangle as a reference, not the active one as it does now.  That
issue can be resolved easily as soon as set try selection support is
added to the driver so pad config crop rectangle can be maintained by
users via selection API.

Last, set format try processing path should give the same results as
active in respect to active vs. pad config crop rectangle geometry.
Both rectangles should be either not touched by set format (that's what
we are going to achieve) or modified the same way, otherwise users
won't be able to obtain equal results from both paths while iterating
through set format and set selection operations in order to obtain
desired frame size.

We can't begin with modifying set format pad operation as not to touch
crop rectangle since that depends on availability of set try selection
for symmetry.  Neither can we begin with adding set try selection since
that in turn depends on equal handling of active and pad config crop
rectangles by set format.  We can either implement all required
modifications in a single patch, or begin with fixing current set
format try processing path to appropriately handle pad config crop
rectangle.  This patch implements the latter approach as believed to
be more readable.

Move crop rectangle adjustments code from a helper (the former
implementation of .s_fmt(), now called from set format active
processing path) to the body of set format pad operation function
where it can be also used for processing try requests for symmetry with
active ones.  As the helper no longer processes frame geometry, only
frame format and half scaling, simplify its API accordingly and update
its users.

Moreover, extract code that applies crop rectangle hardware limits
(now a part of .set_selection() operation which is called from set
format active processing path) to a new helper and call that helper
from set format try processing path as well for symmetry with active.

Fixes: 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt by set_fmt")
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 83 ++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 91906b94f978..c0ac3d0ae167 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -491,6 +491,17 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
 	}
 }
 
+static void ov6650_bind_align_crop_rectangle(struct v4l2_rect *rect)
+{
+	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);
+}
+
 static int ov6650_set_selection(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_selection *sel)
@@ -503,13 +514,7 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
 	    sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	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);
+	ov6650_bind_align_crop_rectangle(&sel->r);
 
 	ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
 	if (!ret) {
@@ -570,22 +575,10 @@ static bool is_unscaled_ok(int width, int height, struct v4l2_rect *rect)
 #define to_clkrc(div)	((div) - 1)
 
 /* set the format we will capture in */
-static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool half_scale)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
-	bool half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
-	struct v4l2_subdev_selection sel = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.target = V4L2_SEL_TGT_CROP,
-		.r.left = priv->rect.left + (priv->rect.width >> 1) -
-			(mf->width >> (1 - half_scale)),
-		.r.top = priv->rect.top + (priv->rect.height >> 1) -
-			(mf->height >> (1 - half_scale)),
-		.r.width = mf->width << half_scale,
-		.r.height = mf->height << half_scale,
-	};
-	u32 code = mf->code;
 	u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask;
 	int ret;
 
@@ -653,9 +646,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 		coma_mask |= COMA_QCIF;
 	}
 
-	ret = ov6650_set_selection(sd, NULL, &sel);
-	if (!ret)
-		ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
+	ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
 	if (!ret) {
 		priv->half_scale = half_scale;
 
@@ -674,14 +665,16 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
+	struct v4l2_subdev_selection sel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.target = V4L2_SEL_TGT_CROP,
+	};
+	struct v4l2_rect *crop = &sel.r;
+	bool half_scale;
 
 	if (format->pad)
 		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);
-
 	switch (mf->code) {
 	case MEDIA_BUS_FMT_Y10_1X10:
 		mf->code = MEDIA_BUS_FMT_Y8_1X8;
@@ -699,10 +692,24 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 		break;
 	}
 
+	*crop = priv->rect;
+	half_scale = !is_unscaled_ok(mf->width, mf->height, crop);
+
+	/* adjust new crop rectangle position against its current center */
+	crop->left += (crop->width - (mf->width << half_scale)) / 2;
+	crop->top += (crop->height - (mf->height << half_scale)) / 2;
+	/* adjust new crop rectangle size */
+	crop->width = mf->width << half_scale;
+	crop->height = mf->height << half_scale;
+
 	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;
+		/* store new crop rectangle, hadware bound, in pad config */
+		ov6650_bind_align_crop_rectangle(crop);
+		cfg->try_crop = *crop;
+
+		/* store new mbus frame format code and size in pad config */
+		cfg->try_fmt.width = crop->width >> half_scale;
+		cfg->try_fmt.height = crop->height >> half_scale;
 		cfg->try_fmt.code = mf->code;
 
 		/* return default mbus frame format updated with pad config */
@@ -712,9 +719,16 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 		mf->code = cfg->try_fmt.code;
 
 	} else {
-		/* apply new media bus format code and frame size */
-		int ret = ov6650_s_fmt(sd, mf);
+		int ret;
 
+		/* apply new crop rectangle */
+		ret = ov6650_set_selection(sd, NULL, &sel);
+		if (ret)
+			return ret;
+
+		/* apply new media bus frame format and scaling if changed */
+		if (mf->code != priv->code || half_scale != priv->half_scale)
+			ret = ov6650_s_fmt(sd, mf->code, half_scale);
 		if (ret)
 			return ret;
 
@@ -890,9 +904,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 	if (!ret)
 		ret = ov6650_prog_dflt(client, xclk->clkrc);
 	if (!ret) {
-		struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
-
-		ret = ov6650_s_fmt(sd, &mf);
+		/* driver default frame format, no scaling */
+		ret = ov6650_s_fmt(sd, ov6650_def_fmt.code, false);
 	}
 	if (!ret)
 		ret = v4l2_ctrl_handler_setup(&priv->hdl);
-- 
2.24.1

[PATCH 2/3] media: ov6650: Add try support to selection API operations

[Janusz Krzysztofik]

Try requests are now only supported by format processing pad operations
implemented by the driver.  The driver selection API operations
currently respond to them with -EINVAL.  While that is correct, it
constraints video device drivers to not use subdevice cropping at all
while processing user requested active frame size, otherwise their set
try format results might differ from active.  As a consequence, we
can't fix set format pad operation as not to touch crop rectangle since
that would affect users not being able to set arbitrary frame sizes.
Moreover, without a working set try selection support we are not able
to use pad config crop rectangle as a reference while processing set
try format requests.

Implement missing try selection support.  Moreover, as it will be now
possible to maintain the pad config crop rectangle via selection API,
start using it instead of the active one as a reference while
processing set try format requests.

is_unscaled_ok() helper, now also called from set selection operation,
has been just moved up in the source file to avoid a prototype, with no
functional changes.

Fixes: 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt by set_fmt")
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 54 ++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index c0ac3d0ae167..65701f2c7c7c 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -472,9 +472,16 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
+	struct v4l2_rect *rect;
 
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		/* pre-select try crop rectangle */
+		rect = &cfg->try_crop;
+
+	} else {
+		/* pre-select active crop rectangle */
+		rect = &priv->rect;
+	}
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
@@ -483,14 +490,22 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
 		sel->r.width = W_CIF;
 		sel->r.height = H_CIF;
 		return 0;
+
 	case V4L2_SEL_TGT_CROP:
-		sel->r = priv->rect;
+		/* use selected crop rectangle */
+		sel->r = *rect;
 		return 0;
+
 	default:
 		return -EINVAL;
 	}
 }
 
+static bool is_unscaled_ok(int width, int height, struct v4l2_rect *rect)
+{
+	return width > rect->width >> 1 || height > rect->height >> 1;
+}
+
 static void ov6650_bind_align_crop_rectangle(struct v4l2_rect *rect)
 {
 	v4l_bound_align_image(&rect->width, 2, W_CIF, 1,
@@ -510,12 +525,30 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
 	struct ov6650 *priv = to_ov6650(client);
 	int ret;
 
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
-	    sel->target != V4L2_SEL_TGT_CROP)
+	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
 	ov6650_bind_align_crop_rectangle(&sel->r);
 
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		struct v4l2_rect *crop = &cfg->try_crop;
+		struct v4l2_mbus_framefmt *mf = &cfg->try_fmt;
+		/* detect current pad config scaling factor */
+		bool half_scale = !is_unscaled_ok(mf->width, mf->height, crop);
+
+		/* store new crop rectangle */
+		*crop = sel->r;
+
+		/* adjust frame size */
+		mf->width = crop->width >> half_scale;
+		mf->height = crop->height >> half_scale;
+
+		return 0;
+	}
+
+	/* V4L2_SUBDEV_FORMAT_ACTIVE */
+
+	/* apply new crop rectangle */
 	ret = ov6650_reg_write(client, REG_HSTRT, sel->r.left >> 1);
 	if (!ret) {
 		priv->rect.width += priv->rect.left - sel->r.left;
@@ -567,11 +600,6 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static bool is_unscaled_ok(int width, int height, struct v4l2_rect *rect)
-{
-	return width > rect->width >> 1 || height > rect->height >> 1;
-}
-
 #define to_clkrc(div)	((div) - 1)
 
 /* set the format we will capture in */
@@ -692,7 +720,11 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 		break;
 	}
 
-	*crop = priv->rect;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
+		*crop = cfg->try_crop;
+	else
+		*crop = priv->rect;
+
 	half_scale = !is_unscaled_ok(mf->width, mf->height, crop);
 
 	/* adjust new crop rectangle position against its current center */
-- 
2.24.1

[PATCH 3/3] media: ov6650: Fix crop rectangle affected by set format

[Janusz Krzysztofik]

According to subdevice interface specification found in V4L2 API
documentation, set format pad operations should not affect image
geometry set in preceding image processing steps. Unfortunately, that
requirement is not respected by the driver implementation of set format
as it was not the case when that code was still implementing a pair of
now obsolete .s_mbus_fmt() / .try_mbus_fmt() video operations before
they have been merged and reused as an implementation of .set_fmt() pad
operation by commit 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt
by set_fmt").

Exclude non-compliant crop rectangle adjustments from set format try,
as well as a call to .set_selection() from set format active processing
path, so only frame scaling is applied as needed and crop rectangle is
no longer modified.

Fixes: 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt by set_fmt")
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 65701f2c7c7c..4439e2aca076 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -693,11 +693,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
-	struct v4l2_subdev_selection sel = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.target = V4L2_SEL_TGT_CROP,
-	};
-	struct v4l2_rect *crop = &sel.r;
+	struct v4l2_rect *crop;
 	bool half_scale;
 
 	if (format->pad)
@@ -721,24 +717,13 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
-		*crop = cfg->try_crop;
+		crop = &cfg->try_crop;
 	else
-		*crop = priv->rect;
+		crop = &priv->rect;
 
 	half_scale = !is_unscaled_ok(mf->width, mf->height, crop);
 
-	/* adjust new crop rectangle position against its current center */
-	crop->left += (crop->width - (mf->width << half_scale)) / 2;
-	crop->top += (crop->height - (mf->height << half_scale)) / 2;
-	/* adjust new crop rectangle size */
-	crop->width = mf->width << half_scale;
-	crop->height = mf->height << half_scale;
-
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		/* store new crop rectangle, hadware bound, in pad config */
-		ov6650_bind_align_crop_rectangle(crop);
-		cfg->try_crop = *crop;
-
 		/* store new mbus frame format code and size in pad config */
 		cfg->try_fmt.width = crop->width >> half_scale;
 		cfg->try_fmt.height = crop->height >> half_scale;
@@ -751,12 +736,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 		mf->code = cfg->try_fmt.code;
 
 	} else {
-		int ret;
-
-		/* apply new crop rectangle */
-		ret = ov6650_set_selection(sd, NULL, &sel);
-		if (ret)
-			return ret;
+		int ret = 0;
 
 		/* apply new media bus frame format and scaling if changed */
 		if (mf->code != priv->code || half_scale != priv->half_scale)
-- 
2.24.1

Re: [PATCH 0/3] media: ov6650: Fix crop rectangle affected by set format

[Janusz Krzysztofik]

Hi Sakari,

I've seen this series, as well as my two other patches sent shortly after, 
assigned to you on V4L2 patchwork.  Have you already had a chance to look at 
them?  Have you any comments?  I'm assuming there is no need to resend them.

I'm asking because I still have a few ov650 patches in my queue which depend 
no those already submitted.  I'd like to be sure I'm not going into wrong 
direction before submitting more.

Thanks,
Janusz

On Monday, May 4, 2020 12:06:15 A.M. CEST Janusz Krzysztofik wrote:
> According to subdevice interface specification found in V4L2 API
> documentation, set format pad operations should not affect image
> geometry set in preceding image processing steps. Unfortunately, that
> requirement is not respected by the driver implementation of set format
> as it was not the case when that code was still implementing a pair of
> now obsolete .s_mbus_fmt() / .try_mbus_fmt() video operations before
> they have been merged and reused as an implementation of .set_fmt() pad
> operation by commit 717fd5b4907a ("[media] v4l2: replace try_mbus_fmt
> by set_fmt").
> 
> In case of set format active processing path the issue can be fixed
> easily by excluding a call to set active selection from that path. That
> will effectively limit frame size processing to optimal frame scaling
> against active crop rectangle without touching it.  Users can just call
> set active selection themselves to obtain desired frame size.  However,
> set format try processing path needs more work.
> 
> First of all, the driver should be extended with set try selection
> support.  Lack of it constraints video device drivers to not use
> subdevice cropping at all while processing user requested active frame
> size, otherwise their set try format results might differ from active.
> 
> Next, set format try processing path should use pad config crop
> rectangle as a reference, not the active one as it does now.  That
> issue can be resolved easily as soon as set try selection support is
> added to the driver so pad config crop rectangle can be maintained by
> users via selection API.
> 
> Last, set format try processing path should give the same results as
> active in respect to active vs. pad config crop rectangle geometry.
> Both rectangles should be either not touched by set format (that's what
> we are going to achieve) or modified the same way, otherwise users
> won't be able to obtain equal results from both paths while iterating
> through set format and set selection operations in order to obtain
> desired frame size.
> 
> We can't begin with modifying set format pad operation as not to touch
> crop rectangle since that depends on availability of set try selection
> for symmetry.  Neither can we begin with adding set try selection since
> that in turn depends on equal handling of active and pad config crop
> rectangles by set format.  We can either implement all required
> modifications in a single patch, or begin with fixing current set
> format try processing path to appropriately handle pad config crop
> rectangle.
> 
> This series implements the latter approach as believed to be more
> readable.  However, the patches can be squashed if so decided by
> subsystem maintainers.
> 
> Janusz Krzysztofik (3):
>   media: ov6650: Fix set format try processing path
>   media: ov6650: Add try support to selection API operations
>   media: ov6650: Fix crop rectangle affected by set format
> 
>  drivers/media/i2c/ov6650.c | 115 ++++++++++++++++++++++---------------
>  1 file changed, 70 insertions(+), 45 deletions(-)
> 
>