Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
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
Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
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
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 > > > --- > > > 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.ai...@linux.intel.com
Re: [RFC PATCH 4/5] media: ov6650: Fix frame scaling not reset on crop
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
> > ---
> > 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 4/5] media: ov6650: Fix frame scaling not reset on crop
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
> ---
> 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
