| Message ID | 20230627131830.54601-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand |
Hi Hans On 27/06/2023 15:18, Hans de Goede wrote: > ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY was getting > the try_fmt v4l2_mbus_framefmt struct from the passed in sd_state > and then storing the contents of that into the return by reference > format->format struct. > > While the right thing to do would be filling format->format based on > the just looked up mode and then store the results of that in > sd_state->pads[0].try_fmt . > > Before the previous change introducing ov2680_fill_format() this > resulted in ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY always > returning the zero-ed out sd_state->pads[0].try_fmt in format->format > breaking callers using this. > > After the introduction of ov2680_fill_format() which at least > initializes sd_state->pads[0].try_fmt properly, format->format > is now always being filled with the default 800x600 mode set by > ov2680_init_cfg() independent of the actual requested mode. > > Move the filling of format->format with ov2680_fill_format() to > before the if (which == V4L2_SUBDEV_FORMAT_TRY) and then store > the filled in format->format in sd_state->pads[0].try_fmt to > fix this. > > Note this removes the fmt local variable because IMHO having a local > variable which points to a sub-struct of one of the function arguments > just leads to confusion when reading the code. > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- For #5, #6 and this one: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > drivers/media/i2c/ov2680.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index c4a46c734d82..7fc4b39ebb37 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -603,7 +603,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_format *format) > { > struct ov2680_dev *sensor = to_ov2680_dev(sd); > - struct v4l2_mbus_framefmt *fmt = &format->format; > struct v4l2_mbus_framefmt *try_fmt; > const struct ov2680_mode_info *mode; > int ret = 0; > @@ -612,14 +611,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > return -EINVAL; > > mode = v4l2_find_nearest_size(ov2680_mode_data, > - ARRAY_SIZE(ov2680_mode_data), width, > - height, fmt->width, fmt->height); > + ARRAY_SIZE(ov2680_mode_data), > + width, height, > + format->format.width, > + format->format.height); > if (!mode) > return -EINVAL; > > + ov2680_fill_format(sensor, &format->format, mode->width, mode->height); > + > if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); > - format->format = *try_fmt; > + *try_fmt = format->format; > return 0; > } > > @@ -630,8 +633,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > goto unlock; > } > > - ov2680_fill_format(sensor, fmt, mode->width, mode->height); > - > sensor->current_mode = mode; > sensor->fmt = format->format; > sensor->mode_pending_changes = true;
Hi Hans On Tue, Jun 27, 2023 at 03:18:08PM +0200, Hans de Goede wrote: > ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY was getting > the try_fmt v4l2_mbus_framefmt struct from the passed in sd_state > and then storing the contents of that into the return by reference > format->format struct. > > While the right thing to do would be filling format->format based on > the just looked up mode and then store the results of that in > sd_state->pads[0].try_fmt . > > Before the previous change introducing ov2680_fill_format() this > resulted in ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY always > returning the zero-ed out sd_state->pads[0].try_fmt in format->format > breaking callers using this. > > After the introduction of ov2680_fill_format() which at least > initializes sd_state->pads[0].try_fmt properly, format->format > is now always being filled with the default 800x600 mode set by > ov2680_init_cfg() independent of the actual requested mode. > > Move the filling of format->format with ov2680_fill_format() to > before the if (which == V4L2_SUBDEV_FORMAT_TRY) and then store > the filled in format->format in sd_state->pads[0].try_fmt to > fix this. > > Note this removes the fmt local variable because IMHO having a local > variable which points to a sub-struct of one of the function arguments > just leads to confusion when reading the code. > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index c4a46c734d82..7fc4b39ebb37 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -603,7 +603,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_format *format) > { > struct ov2680_dev *sensor = to_ov2680_dev(sd); > - struct v4l2_mbus_framefmt *fmt = &format->format; > struct v4l2_mbus_framefmt *try_fmt; > const struct ov2680_mode_info *mode; > int ret = 0; > @@ -612,14 +611,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > return -EINVAL; > > mode = v4l2_find_nearest_size(ov2680_mode_data, > - ARRAY_SIZE(ov2680_mode_data), width, > - height, fmt->width, fmt->height); > + ARRAY_SIZE(ov2680_mode_data), > + width, height, > + format->format.width, > + format->format.height); > if (!mode) > return -EINVAL; Nit: only if you have to resend, could this be dropped? mode will be NULL only if ov2680_mode_data[] has no entries. > > + ov2680_fill_format(sensor, &format->format, mode->width, mode->height); > + > if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); > - format->format = *try_fmt; > + *try_fmt = format->format; > return 0; > } > > @@ -630,8 +633,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > goto unlock; > } > > - ov2680_fill_format(sensor, fmt, mode->width, mode->height); > - > sensor->current_mode = mode; > sensor->fmt = format->format; > sensor->mode_pending_changes = true; > -- > 2.41.0 >
On Tue, Jun 27, 2023 at 05:08:39PM +0200, Jacopo Mondi wrote: > On Tue, Jun 27, 2023 at 03:18:08PM +0200, Hans de Goede wrote: ... > > mode = v4l2_find_nearest_size(ov2680_mode_data, > > - ARRAY_SIZE(ov2680_mode_data), width, > > - height, fmt->width, fmt->height); > > + ARRAY_SIZE(ov2680_mode_data), > > + width, height, > > + format->format.width, > > + format->format.height); > > if (!mode) > > return -EINVAL; > > Nit: only if you have to resend, could this be dropped? mode will be NULL > only if ov2680_mode_data[] has no entries. We shouldn't rely on the implementation details of some API if it's not advertised that way. Even if it is, the robustness of the code is better with this check.
Hi, On 6/27/23 18:55, Andy Shevchenko wrote: > On Tue, Jun 27, 2023 at 05:08:39PM +0200, Jacopo Mondi wrote: >> On Tue, Jun 27, 2023 at 03:18:08PM +0200, Hans de Goede wrote: > > ... > >>> mode = v4l2_find_nearest_size(ov2680_mode_data, >>> - ARRAY_SIZE(ov2680_mode_data), width, >>> - height, fmt->width, fmt->height); >>> + ARRAY_SIZE(ov2680_mode_data), >>> + width, height, >>> + format->format.width, >>> + format->format.height); >>> if (!mode) >>> return -EINVAL; >> >> Nit: only if you have to resend, could this be dropped? mode will be NULL >> only if ov2680_mode_data[] has no entries. > > We shouldn't rely on the implementation details of some API if it's not > advertised that way. Even if it is, the robustness of the code is better with > this check. Note non of this is really important (as Jacopo already mentioned it is just a nitpick) since this code is completely removed in a later patch. Also note that the check was already there, so removing it would be out of scope for this patch. Regards, Hans
Hi Andy On Tue, Jun 27, 2023 at 07:55:48PM +0300, Andy Shevchenko wrote: > On Tue, Jun 27, 2023 at 05:08:39PM +0200, Jacopo Mondi wrote: > > On Tue, Jun 27, 2023 at 03:18:08PM +0200, Hans de Goede wrote: > > ... > > > > mode = v4l2_find_nearest_size(ov2680_mode_data, > > > - ARRAY_SIZE(ov2680_mode_data), width, > > > - height, fmt->width, fmt->height); > > > + ARRAY_SIZE(ov2680_mode_data), > > > + width, height, > > > + format->format.width, > > > + format->format.height); > > > if (!mode) > > > return -EINVAL; > > > > Nit: only if you have to resend, could this be dropped? mode will be NULL > > only if ov2680_mode_data[] has no entries. > > We shouldn't rely on the implementation details of some API if it's not > advertised that way. Even if it is, the robustness of the code is better with > this check. I don't 100% agree here. the s_fmt subdev operation is not supposed to return -EINVAL if the requested configuration cannot be found. So when I saw -EINVAL here it throw me off a little, until I didn't look into v4l2_find_nearest_size() implementation and realized this can't actually happen. But as Hans said, this is going away in the next patches, something I didn't notice when I wrote this comment, so no need to bother :) > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index c4a46c734d82..7fc4b39ebb37 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -603,7 +603,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_format *format) { struct ov2680_dev *sensor = to_ov2680_dev(sd); - struct v4l2_mbus_framefmt *fmt = &format->format; struct v4l2_mbus_framefmt *try_fmt; const struct ov2680_mode_info *mode; int ret = 0; @@ -612,14 +611,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, return -EINVAL; mode = v4l2_find_nearest_size(ov2680_mode_data, - ARRAY_SIZE(ov2680_mode_data), width, - height, fmt->width, fmt->height); + ARRAY_SIZE(ov2680_mode_data), + width, height, + format->format.width, + format->format.height); if (!mode) return -EINVAL; + ov2680_fill_format(sensor, &format->format, mode->width, mode->height); + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); - format->format = *try_fmt; + *try_fmt = format->format; return 0; } @@ -630,8 +633,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, goto unlock; } - ov2680_fill_format(sensor, fmt, mode->width, mode->height); - sensor->current_mode = mode; sensor->fmt = format->format; sensor->mode_pending_changes = true;
ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY was getting the try_fmt v4l2_mbus_framefmt struct from the passed in sd_state and then storing the contents of that into the return by reference format->format struct. While the right thing to do would be filling format->format based on the just looked up mode and then store the results of that in sd_state->pads[0].try_fmt . Before the previous change introducing ov2680_fill_format() this resulted in ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY always returning the zero-ed out sd_state->pads[0].try_fmt in format->format breaking callers using this. After the introduction of ov2680_fill_format() which at least initializes sd_state->pads[0].try_fmt properly, format->format is now always being filled with the default 800x600 mode set by ov2680_init_cfg() independent of the actual requested mode. Move the filling of format->format with ov2680_fill_format() to before the if (which == V4L2_SUBDEV_FORMAT_TRY) and then store the filled in format->format in sd_state->pads[0].try_fmt to fix this. Note this removes the fmt local variable because IMHO having a local variable which points to a sub-struct of one of the function arguments just leads to confusion when reading the code. Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2680.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)