[RFC PATCH 0/5] media: ov6650: V4L2 subdev compliance fixes

[RFC PATCH 0/5] media: ov6650: V4L2 subdev compliance fixes

[Janusz Krzysztofik]

An attepmt to make the driver more compliant with V4L2 subdevice
interface specification.

Non-compliant frame half scaling is kept in .set_fmt() until composing
is implemented.

Janusz Krzysztofik (5):
  media: ov6650: Fix V4L2_SEL_FLAG_KEEP_CONFIG handling
  media: ov6650: Refactor ov6650_s_fmt() helper
  media: ov6650: Fix active crop rectangle affected by .set_fmt()
  media: ov6650: Fix frame scaling not reset on crop
  media: ov6650: Add .init_cfg() pad operation callback

 drivers/media/i2c/ov6650.c | 71 +++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 28 deletions(-)

-- 
2.21.0

[RFC PATCH 1/5] media: ov6650: Fix V4L2_SEL_FLAG_KEEP_CONFIG handling

[Janusz Krzysztofik]

This flag is now ignored - output frame size is affected by new crop
settings regardless of the flag value.  Fix it.

Since keeping output frame size untouched while applying new crop
settings is not supported, simply return results of .get_selection() if
V4L2_SEL_FLAG_KEEP_CONFIG is passed to .set_selection().

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index c728f718716b..1b02479b616f 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -484,6 +484,10 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
 	    sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
+	/* No support for changing crop rectangle with frame size preserved */
+	if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG)
+		return ov6650_get_selection(sd, cfg, sel);
+
 	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,
-- 
2.21.0

[RFC PATCH 2/5] media: ov6650: Refactor ov6650_s_fmt() helper

[Janusz Krzysztofik]

The driver suffers from a few compliance and implementation issues:
- crop rectangle is affected by .set_fmt() pad operation callback,
- frame scaling is not reset on modification of crop rectangle,
- V4L2_SUBDEV_FORMAT_TRY support in .set_fmt() uses active crop
  rectangle, not the one from a pad config, as a reference.

For easy resolution of those issues, ov6650_s_fmt() will first be
refactored.

The ov6650_s_fmt() helper function, mostly called form .set_fmt() pad
operation callback, now takes a decision if half scaling is applicable
for current crop rectangle and requested frame size, then possibly
adjusts hardware crop settings, frame scaling and media bus frame
format.  It accepts a struct v4l2_mbus_framefmt argument passed by a
user to .set_fmt().

Move the decision on applicability of half scaling up to .set_fmt(),
then modify the ov6650_s_fmt() API so it accepts a half scaling flag.
In order to avoid passing full struct v4l2_mbus_framefmt argument to it
when called from functions other than .set_fmt(), also accept a target
pixel code as an argument and make the struct v4l2_mbus_framefmt used
for crop rectangle modification optional.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 1b02479b616f..8cb30f3e4851 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -570,25 +570,18 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
 }
 
 /* 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, struct v4l2_mbus_framefmt *mf,
+			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;
 	unsigned long mclk, pclk;
 	u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
-	int ret;
+	int ret = 0;
 
 	/* select color matrix configuration for given color encoding */
 	switch (code) {
@@ -668,7 +661,16 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 	dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
 			mclk / pclk, 10 * mclk % pclk / pclk);
 
-	ret = ov6650_set_selection(sd, NULL, &sel);
+	if (mf) {
+		sel.r.left = priv->rect.left + (priv->rect.width >> 1) -
+			(mf->width >> (1 - half_scale)),
+		sel.r.top = priv->rect.top + (priv->rect.height >> 1) -
+			(mf->height >> (1 - half_scale)),
+		sel.r.width = mf->width << half_scale,
+		sel.r.height = mf->height << half_scale,
+
+		ret = ov6650_set_selection(sd, NULL, &sel);
+	}
 	if (!ret)
 		ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
 	if (!ret)
@@ -691,11 +693,14 @@ 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);
+	bool half_scale;
 
 	if (format->pad)
 		return -EINVAL;
 
-	if (is_unscaled_ok(mf->width, mf->height, &priv->rect))
+	half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
+
+	if (!half_scale)
 		v4l_bound_align_image(&mf->width, 2, W_CIF, 1,
 				&mf->height, 2, H_CIF, 1, 0);
 
@@ -730,7 +735,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
 	} else {
 		/* apply new media bus format code and frame size */
-		int ret = ov6650_s_fmt(sd, mf);
+		int ret = ov6650_s_fmt(sd, mf, mf->code, half_scale);
 
 		if (ret)
 			return ret;
@@ -885,11 +890,8 @@ 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 = ov6650_s_fmt(sd, NULL, ov6650_def_fmt.code, false);
 	if (!ret)
 		ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
-- 
2.21.0

[RFC PATCH 3/5] media: ov6650: Fix active crop rectangle affected by .set_fmt()

[Janusz Krzysztofik]

According to subdevice interface specification found in V4L2 API
documentation, .set_fmt() pad operation callback should not affect
image geometry set in preceding image processing steps. Unfortunately,
ov6650_s_fmt() helper, when called from .set_fmt() with new user
requested frame size passed to it, does not respect that requirement
as that was not the case before, when the helper was still called from
.mbus_set_fmt() video operation callback.

Remmove a call to .set_selection() from ov6650_s_fmt() helper so it no
longer modifies the active crop rectangle and simplify its API by
removing no longer needed struct v4l2_mbus_framefmt argument.  For
consistency of its results with active format processing, also update
try format path inside .set_fmt() (still using active crop rectangle as
a reference instead of the try one from pad config which is not yet
maintained by the driver).

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 8cb30f3e4851..47590cd51190 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -570,15 +570,10 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
 }
 
 /* set the format we will capture in */
-static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf,
-			u32 code, bool half_scale)
+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);
-	struct v4l2_subdev_selection sel = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.target = V4L2_SEL_TGT_CROP,
-	};
 	unsigned long mclk, pclk;
 	u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
 	int ret = 0;
@@ -661,18 +656,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf,
 	dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
 			mclk / pclk, 10 * mclk % pclk / pclk);
 
-	if (mf) {
-		sel.r.left = priv->rect.left + (priv->rect.width >> 1) -
-			(mf->width >> (1 - half_scale)),
-		sel.r.top = priv->rect.top + (priv->rect.height >> 1) -
-			(mf->height >> (1 - half_scale)),
-		sel.r.width = mf->width << half_scale,
-		sel.r.height = mf->height << half_scale,
-
-		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)
 		ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
 	if (!ret) {
@@ -700,10 +684,6 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
 	half_scale = !is_unscaled_ok(mf->width, mf->height, &priv->rect);
 
-	if (!half_scale)
-		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;
@@ -723,8 +703,8 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
 	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.width = priv->rect.width >> half_scale;
+		cfg->try_fmt.height = priv->rect.height >> half_scale;
 		cfg->try_fmt.code = mf->code;
 
 		/* return default mbus frame format updated with pad config */
@@ -735,7 +715,7 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
 	} else {
 		/* apply new media bus format code and frame size */
-		int ret = ov6650_s_fmt(sd, mf, mf->code, half_scale);
+		int ret = ov6650_s_fmt(sd, mf->code, half_scale);
 
 		if (ret)
 			return ret;
@@ -891,7 +871,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 	if (!ret)
 		ret = ov6650_prog_dflt(client);
 	if (!ret)
-		ret = ov6650_s_fmt(sd, NULL, ov6650_def_fmt.code, false);
+		ret = ov6650_s_fmt(sd, ov6650_def_fmt.code, false);
 	if (!ret)
 		ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
-- 
2.21.0

[RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Janusz Krzysztofik]

According to V4L2 subdevice interface specification, frame scaling
factors should be reset to default values on modification of input frame
format.  Assuming that requirement also applies in case of crop
rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
the driver now does not respect it.

The KEEP_CONFIG case is already implemented, fix the other path.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 47590cd51190..cc70d8952999 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
 	}
 }
 
+static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool half_scale);
+
 static int ov6650_set_selection(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_selection *sel)
@@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
 	}
 	if (!ret)
 		priv->rect.height = sel->r.height;
+	else
+		return ret;
 
+	if (priv->half_scale) {
+		/* turn off half scaling, preserve media bus format */
+		ret = ov6650_s_fmt(sd, priv->code, false);
+	}
 	return ret;
 }
 
-- 
2.21.0

[RFC PATCH 5/5] media: ov6650: Add .init_cfg() pad operation callback

[Janusz Krzysztofik]

The driver now supports V4L2_SUBDEV_FORMAT_TRY operation mode only in
.get/set_fmt() pad operation callbacks.  That means only .try_format
member of pad config is maintained.  As a consequence, active crop
rectangle is used as a referece while V4L2_SUBDEV_FORMAT_TRY requests
are processed.  In order to fix that, a method for initialization of
.try_crop pad config member is needed.

Implement .init_cfg() pad operation callback which initializes the pad
config from current active format and selection settings.  From now on,
and before the driver V4L2_SUBDEV_FORMAT_TRY support is further
modified, host interface drivers should call .init_cfg() on a pad
config before passing it to V4L2_SUBDEV_FORMAT_TRY operations.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index cc70d8952999..c3d4c1f598b2 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -447,6 +447,26 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+static int ov6650_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov6650 *priv = to_ov6650(client);
+	struct v4l2_mbus_framefmt *mf;
+	struct v4l2_rect *rect;
+
+	mf = &cfg->try_fmt;
+	*mf = ov6650_def_fmt;
+	mf->width = priv->rect.width >> priv->half_scale;
+	mf->height = priv->rect.height >> priv->half_scale;
+	mf->code = priv->code;
+
+	rect = &cfg->try_crop;
+	*rect = priv->rect;
+
+	return 0;
+}
+
 static int ov6650_get_selection(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_selection *sel)
@@ -959,6 +979,7 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
+	.init_cfg	= ov6650_init_cfg,
 	.enum_mbus_code = ov6650_enum_mbus_code,
 	.get_selection	= ov6650_get_selection,
 	.set_selection	= ov6650_set_selection,
-- 
2.21.0

Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Sakari Ailus]

Hi Janusz,

On Sun, May 26, 2019 at 10:47:57PM +0200, Janusz Krzysztofik wrote:
> According to V4L2 subdevice interface specification, frame scaling
> factors should be reset to default values on modification of input frame
> format.  Assuming that requirement also applies in case of crop
> rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
> the driver now does not respect it.
> 
> The KEEP_CONFIG case is already implemented, fix the other path.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/media/i2c/ov6650.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index 47590cd51190..cc70d8952999 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev *sd,
>  	}
>  }
>  
> +static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool half_scale);
> +

Would it be possible to rearrange the functions in the file so you wouldn't
need extra prototypes? Preferrably that'd be a new patch.

>  static int ov6650_set_selection(struct v4l2_subdev *sd,
>  		struct v4l2_subdev_pad_config *cfg,
>  		struct v4l2_subdev_selection *sel)
> @@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev *sd,
>  	}
>  	if (!ret)
>  		priv->rect.height = sel->r.height;
> +	else
> +		return ret;

if (ret)
	return ret;

...

>  
> +	if (priv->half_scale) {
> +		/* turn off half scaling, preserve media bus format */
> +		ret = ov6650_s_fmt(sd, priv->code, false);
> +	}
>  	return ret;
>  }
>  

-- 
Regards,

Sakari Ailus

Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Janusz Krzysztofik]

Hi Sakari,

On Friday, May 31, 2019 1:42:58 PM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Sun, May 26, 2019 at 10:47:57PM +0200, Janusz Krzysztofik wrote:
> > According to V4L2 subdevice interface specification, frame scaling
> > factors should be reset to default values on modification of input frame
> > format.  Assuming that requirement also applies in case of crop
> > rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
> > the driver now does not respect it.
> > 
> > The KEEP_CONFIG case is already implemented, fix the other path.
> > 
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > ---
> >  drivers/media/i2c/ov6650.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index 47590cd51190..cc70d8952999 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev 
*sd,
> >  	}
> >  }
> >  
> > +static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool 
half_scale);
> > +
> 
> Would it be possible to rearrange the functions in the file so you wouldn't
> need extra prototypes? Preferrably that'd be a new patch.

Sure. I've intentionally done it like that for better readability of the 
patches, but I have a task in my queue for rearrangement of functions order as 
soon as other modifications are in place.

> >  static int ov6650_set_selection(struct v4l2_subdev *sd,
> >  		struct v4l2_subdev_pad_config *cfg,
> >  		struct v4l2_subdev_selection *sel)
> > @@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev 
*sd,
> >  	}
> >  	if (!ret)
> >  		priv->rect.height = sel->r.height;
> > +	else
> > +		return ret;
> 
> if (ret)
> 	return ret;

OK

Perhaps you will have more comments on other patches so I'll wait a bit and 
then resubmit the series as v2.

Thanks,
Janusz

> ...
> 
> >  
> > +	if (priv->half_scale) {
> > +		/* turn off half scaling, preserve media bus format */
> > +		ret = ov6650_s_fmt(sd, priv->code, false);
> > +	}
> >  	return ret;
> >  }
> >  
> 
> 

Re: [RFC PATCH 5/5] media: ov6650: Add .init_cfg() pad operation callback

[Sakari Ailus]

Hi Janusz,

On Sun, May 26, 2019 at 10:47:58PM +0200, Janusz Krzysztofik wrote:
> The driver now supports V4L2_SUBDEV_FORMAT_TRY operation mode only in
> .get/set_fmt() pad operation callbacks.  That means only .try_format
> member of pad config is maintained.  As a consequence, active crop
> rectangle is used as a referece while V4L2_SUBDEV_FORMAT_TRY requests
> are processed.  In order to fix that, a method for initialization of
> .try_crop pad config member is needed.
> 
> Implement .init_cfg() pad operation callback which initializes the pad
> config from current active format and selection settings.  From now on,

The values set by init_cfg should be the defaults and not reflect the
current configuration. Apart from that the patch seems fine.

> and before the driver V4L2_SUBDEV_FORMAT_TRY support is further
> modified, host interface drivers should call .init_cfg() on a pad
> config before passing it to V4L2_SUBDEV_FORMAT_TRY operations.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/media/i2c/ov6650.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index cc70d8952999..c3d4c1f598b2 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -447,6 +447,26 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
>  	return ret;
>  }
>  
> +static int ov6650_init_cfg(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov6650 *priv = to_ov6650(client);
> +	struct v4l2_mbus_framefmt *mf;
> +	struct v4l2_rect *rect;
> +
> +	mf = &cfg->try_fmt;
> +	*mf = ov6650_def_fmt;
> +	mf->width = priv->rect.width >> priv->half_scale;
> +	mf->height = priv->rect.height >> priv->half_scale;
> +	mf->code = priv->code;
> +
> +	rect = &cfg->try_crop;
> +	*rect = priv->rect;
> +
> +	return 0;
> +}
> +
>  static int ov6650_get_selection(struct v4l2_subdev *sd,
>  		struct v4l2_subdev_pad_config *cfg,
>  		struct v4l2_subdev_selection *sel)
> @@ -959,6 +979,7 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = {
>  };
>  
>  static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
> +	.init_cfg	= ov6650_init_cfg,
>  	.enum_mbus_code = ov6650_enum_mbus_code,
>  	.get_selection	= ov6650_get_selection,
>  	.set_selection	= ov6650_set_selection,
> -- 
> 2.21.0
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Sakari Ailus]

Hi Janusz,

On Fri, May 31, 2019 at 07:56:33PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Friday, May 31, 2019 1:42:58 PM CEST Sakari Ailus wrote:
> > Hi Janusz,
> > 
> > On Sun, May 26, 2019 at 10:47:57PM +0200, Janusz Krzysztofik wrote:
> > > According to V4L2 subdevice interface specification, frame scaling
> > > factors should be reset to default values on modification of input frame
> > > format.  Assuming that requirement also applies in case of crop
> > > rectangle modification unless V4L2_SEL_FLAG_KEEP_CONFIG is requested,
> > > the driver now does not respect it.
> > > 
> > > The KEEP_CONFIG case is already implemented, fix the other path.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > ---
> > >  drivers/media/i2c/ov6650.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > index 47590cd51190..cc70d8952999 100644
> > > --- a/drivers/media/i2c/ov6650.c
> > > +++ b/drivers/media/i2c/ov6650.c
> > > @@ -472,6 +472,8 @@ static int ov6650_get_selection(struct v4l2_subdev 
> *sd,
> > >  	}
> > >  }
> > >  
> > > +static int ov6650_s_fmt(struct v4l2_subdev *sd, u32 code, bool 
> half_scale);
> > > +
> > 
> > Would it be possible to rearrange the functions in the file so you wouldn't
> > need extra prototypes? Preferrably that'd be a new patch.
> 
> Sure. I've intentionally done it like that for better readability of the 
> patches, but I have a task in my queue for rearrangement of functions order as 
> soon as other modifications are in place.

I'm totally fine with doing that after this set on as well. Up to you.

> 
> > >  static int ov6650_set_selection(struct v4l2_subdev *sd,
> > >  		struct v4l2_subdev_pad_config *cfg,
> > >  		struct v4l2_subdev_selection *sel)
> > > @@ -515,7 +517,13 @@ static int ov6650_set_selection(struct v4l2_subdev 
> *sd,
> > >  	}
> > >  	if (!ret)
> > >  		priv->rect.height = sel->r.height;
> > > +	else
> > > +		return ret;
> > 
> > if (ret)
> > 	return ret;
> 
> OK
> 
> Perhaps you will have more comments on other patches so I'll wait a bit and 
> then resubmit the series as v2.

Not so much on this set BUT I realised that the subtle effect of "media:
ov6650: Register with asynchronous subdevice framework" is that the driver
is now responsible for serialising the access to its own data structures
now. And it doesn't do that. Could you submit a fix, please? It'd be good to
get that to 5.2 through the fixes branch.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Janusz Krzysztofik]

Hi Sakari,

On Sunday, June 2, 2019 12:37:55 AM CEST Sakari Ailus wrote:
> 
> ... I realised that the subtle effect of "media:
> ov6650: Register with asynchronous subdevice framework" is that the driver
> is now responsible for serialising the access to its own data structures
> now. 

Indeed, I must have been not thinking much while preparing it, only following 
patterns from other implementations blindly, sorry.

> And it doesn't do that. Could you submit a fix, please? It'd be good to
> get that to 5.2 through the fixes branch.

How about dropping that V4L2_SUBDEV_FL_HAS_DEVNODE flag for now?  I think that 
will be the most safe approach for a quick fix.  I'd then re-add it together 
with proper locking in a separate patch later.  What do yo think?

Thanks,
Janusz

Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop

[Sakari Ailus]

Hi Janusz,

On Sun, Jun 02, 2019 at 11:58:23AM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Sunday, June 2, 2019 12:37:55 AM CEST Sakari Ailus wrote:
> > 
> > ... I realised that the subtle effect of "media:
> > ov6650: Register with asynchronous subdevice framework" is that the driver
> > is now responsible for serialising the access to its own data structures
> > now. 
> 
> Indeed, I must have been not thinking much while preparing it, only following 
> patterns from other implementations blindly, sorry.

No worries. I missed it at the time, too...

> 
> > And it doesn't do that. Could you submit a fix, please? It'd be good to
> > get that to 5.2 through the fixes branch.
> 
> How about dropping that V4L2_SUBDEV_FL_HAS_DEVNODE flag for now?  I think that 
> will be the most safe approach for a quick fix.  I'd then re-add it together 
> with proper locking in a separate patch later.  What do yo think?

Sure. Then we just re-introduce the flag when the driver is ready for that.

-- 
Regards,

Sakari Ailus