| Message ID | 20230607164712.63579-6-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 Wed, Jun 07, 2023 at 06:46:49PM +0200, Hans de Goede wrote: > On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY, > ov2680_set_fmt() does not talk to the sensor. > > So in this case there is no need to lock the sensor->lock mutex or > to check that the sensor is streaming. > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index d90bbca6e913..a26a6f18f4f1 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > if (format->pad != 0) > return -EINVAL; > > - mutex_lock(&sensor->lock); > - > - if (sensor->is_streaming) { > - ret = -EBUSY; > - goto unlock; > - } > - > mode = v4l2_find_nearest_size(ov2680_mode_data, > ARRAY_SIZE(ov2680_mode_data), width, > height, fmt->width, fmt->height); > - if (!mode) { > - ret = -EINVAL; > - goto unlock; > - } > + if (!mode) > + return -EINVAL; > > if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); > format->format = *try_fmt; Access to sd_state needs to be serialised, so mutex should be held here. > + return 0; > + } > + > + mutex_lock(&sensor->lock); > + > + if (sensor->is_streaming) { > + ret = -EBUSY; > goto unlock; > } >
Hi Sakari, On Thu, Jun 08, 2023 at 12:44:51PM +0000, Sakari Ailus wrote: > On Wed, Jun 07, 2023 at 06:46:49PM +0200, Hans de Goede wrote: > > On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY, > > ov2680_set_fmt() does not talk to the sensor. > > > > So in this case there is no need to lock the sensor->lock mutex or > > to check that the sensor is streaming. > > > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/media/i2c/ov2680.c | 20 +++++++++----------- > > 1 file changed, 9 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > > index d90bbca6e913..a26a6f18f4f1 100644 > > --- a/drivers/media/i2c/ov2680.c > > +++ b/drivers/media/i2c/ov2680.c > > @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > > if (format->pad != 0) > > return -EINVAL; > > > > - mutex_lock(&sensor->lock); > > - > > - if (sensor->is_streaming) { > > - ret = -EBUSY; > > - goto unlock; > > - } > > - > > mode = v4l2_find_nearest_size(ov2680_mode_data, > > ARRAY_SIZE(ov2680_mode_data), width, > > height, fmt->width, fmt->height); > > - if (!mode) { > > - ret = -EINVAL; > > - goto unlock; > > - } > > + if (!mode) > > + return -EINVAL; > > > > if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > > try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); > > format->format = *try_fmt; > > Access to sd_state needs to be serialised, so mutex should be held here. This operation should be called with the state lock held already, so I don't think any extra locking is needed. > > + return 0; > > + } > > + > > + mutex_lock(&sensor->lock); > > + > > + if (sensor->is_streaming) { > > + ret = -EBUSY; > > goto unlock; > > } > >
Hi Laurent, On Thu, Jun 08, 2023 at 03:48:30PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Jun 08, 2023 at 12:44:51PM +0000, Sakari Ailus wrote: > > On Wed, Jun 07, 2023 at 06:46:49PM +0200, Hans de Goede wrote: > > > On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY, > > > ov2680_set_fmt() does not talk to the sensor. > > > > > > So in this case there is no need to lock the sensor->lock mutex or > > > to check that the sensor is streaming. > > > > > > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/media/i2c/ov2680.c | 20 +++++++++----------- > > > 1 file changed, 9 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > > > index d90bbca6e913..a26a6f18f4f1 100644 > > > --- a/drivers/media/i2c/ov2680.c > > > +++ b/drivers/media/i2c/ov2680.c > > > @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, > > > if (format->pad != 0) > > > return -EINVAL; > > > > > > - mutex_lock(&sensor->lock); > > > - > > > - if (sensor->is_streaming) { > > > - ret = -EBUSY; > > > - goto unlock; > > > - } > > > - > > > mode = v4l2_find_nearest_size(ov2680_mode_data, > > > ARRAY_SIZE(ov2680_mode_data), width, > > > height, fmt->width, fmt->height); > > > - if (!mode) { > > > - ret = -EINVAL; > > > - goto unlock; > > > - } > > > + if (!mode) > > > + return -EINVAL; > > > > > > if (format->which == V4L2_SUBDEV_FORMAT_TRY) { > > > try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); > > > format->format = *try_fmt; > > > > Access to sd_state needs to be serialised, so mutex should be held here. > > This operation should be called with the state lock held already, so I > don't think any extra locking is needed. Ah, you're right indeed. I suppose lock here is redundant but controls need some care.
Hi, On 6/8/23 14:48, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Jun 08, 2023 at 12:44:51PM +0000, Sakari Ailus wrote: >> On Wed, Jun 07, 2023 at 06:46:49PM +0200, Hans de Goede wrote: >>> On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY, >>> ov2680_set_fmt() does not talk to the sensor. >>> >>> So in this case there is no need to lock the sensor->lock mutex or >>> to check that the sensor is streaming. >>> >>> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/media/i2c/ov2680.c | 20 +++++++++----------- >>> 1 file changed, 9 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >>> index d90bbca6e913..a26a6f18f4f1 100644 >>> --- a/drivers/media/i2c/ov2680.c >>> +++ b/drivers/media/i2c/ov2680.c >>> @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, >>> if (format->pad != 0) >>> return -EINVAL; >>> >>> - mutex_lock(&sensor->lock); >>> - >>> - if (sensor->is_streaming) { >>> - ret = -EBUSY; >>> - goto unlock; >>> - } >>> - >>> mode = v4l2_find_nearest_size(ov2680_mode_data, >>> ARRAY_SIZE(ov2680_mode_data), width, >>> height, fmt->width, fmt->height); >>> - if (!mode) { >>> - ret = -EINVAL; >>> - goto unlock; >>> - } >>> + if (!mode) >>> + return -EINVAL; >>> >>> if (format->which == V4L2_SUBDEV_FORMAT_TRY) { >>> try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); >>> format->format = *try_fmt; >> >> Access to sd_state needs to be serialised, so mutex should be held here. > > This operation should be called with the state lock held already, so I > don't think any extra locking is needed. Right sd_state is allocated per v4l2-subdev fd so I would expect the caller to take care of any necessary locking here. Regards, Hans > >>> + return 0; >>> + } >>> + >>> + mutex_lock(&sensor->lock); >>> + >>> + if (sensor->is_streaming) { >>> + ret = -EBUSY; >>> goto unlock; >>> } >>> >
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index d90bbca6e913..a26a6f18f4f1 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -592,24 +592,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd, if (format->pad != 0) return -EINVAL; - mutex_lock(&sensor->lock); - - if (sensor->is_streaming) { - ret = -EBUSY; - goto unlock; - } - mode = v4l2_find_nearest_size(ov2680_mode_data, ARRAY_SIZE(ov2680_mode_data), width, height, fmt->width, fmt->height); - if (!mode) { - ret = -EINVAL; - goto unlock; - } + if (!mode) + return -EINVAL; if (format->which == V4L2_SUBDEV_FORMAT_TRY) { try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); format->format = *try_fmt; + return 0; + } + + mutex_lock(&sensor->lock); + + if (sensor->is_streaming) { + ret = -EBUSY; goto unlock; }
On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY, ov2680_set_fmt() does not talk to the sensor. So in this case there is no need to lock the sensor->lock mutex or to check that the sensor is streaming. Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2680.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)