* [PATCH 0/5] media: i2c: imx219: Use subdev active state
@ 2023-07-04 10:36 Jacopo Mondi
2023-07-04 10:40 ` Jacopo Mondi
0 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:36 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
Hello,
this series ports the imx219 to use the subdev active state with great
simplification of format handling, locking and device initialization.
It resumes the work started by Jean-Michel but had to be ported to the new API
and applied on the most recent version of the sensor driver.
Tested with libcamera, by streaming in 1080p, VGA applying V/H flips to make
sure the mbus code permutation handling is still correct.
Thanks
j
Jacopo Mondi (3):
media: i2c: imx219: Use subdev active state
media: i2c: imx219: Simplify format assignment
media: i2c: imx219: Simplify code handling in s_fmt
Jean-Michel Hautbois (2):
media: i2c: imx219: Rename mbus codes array
media: i2c: imx219: Switch from open to init_cfg
drivers/media/i2c/imx219.c | 289 ++++++++++++-------------------------
1 file changed, 91 insertions(+), 198 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/5] media: i2c: imx219: Use subdev active state
2023-07-04 10:36 [PATCH 0/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 10:40 ` [PATCH 1/5] media: i2c: imx219: Rename mbus codes array Jacopo Mondi
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
Hello,
this series ports the imx219 to use the subdev active state with great
simplification of format handling, locking and device initialization.
It resumes the work started by Jean-Michel but had to be ported to the new API
and applied on the most recent version of the sensor driver.
Tested with libcamera, by streaming in 1080p, VGA applying V/H flips to make
sure the mbus code permutation handling is still correct.
Thanks
j
Jacopo Mondi (3):
media: i2c: imx219: Use subdev active state
media: i2c: imx219: Simplify format assignment
media: i2c: imx219: Simplify code handling in s_fmt
Jean-Michel Hautbois (2):
media: i2c: imx219: Rename mbus codes array
media: i2c: imx219: Switch from open to init_cfg
drivers/media/i2c/imx219.c | 289 ++++++++++++-------------------------
1 file changed, 91 insertions(+), 198 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/5] media: i2c: imx219: Rename mbus codes array
2023-07-04 10:40 ` Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-07 16:30 ` Tommaso Merciai
2023-07-04 10:40 ` [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg Jacopo Mondi
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois,
Jean-Michel Hautbois
From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
The imx219 is using the name codes[] for the mbus format which is not
easy to read and know what it means. Change it to imx219_mbus_formats.
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f9471c9e3a74..998a673a4290 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -345,7 +345,7 @@ static const char * const imx219_supply_name[] = {
* - v flip
* - h&v flips
*/
-static const u32 codes[] = {
+static const u32 imx219_mbus_formats[] = {
MEDIA_BUS_FMT_SRGGB10_1X10,
MEDIA_BUS_FMT_SGRBG10_1X10,
MEDIA_BUS_FMT_SGBRG10_1X10,
@@ -578,17 +578,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
lockdep_assert_held(&imx219->mutex);
- for (i = 0; i < ARRAY_SIZE(codes); i++)
- if (codes[i] == code)
+ for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
+ if (imx219_mbus_formats[i] == code)
break;
- if (i >= ARRAY_SIZE(codes))
+ if (i >= ARRAY_SIZE(imx219_mbus_formats))
i = 0;
i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
(imx219->hflip->val ? 1 : 0);
- return codes[i];
+ return imx219_mbus_formats[i];
}
static void imx219_set_default_format(struct imx219 *imx219)
@@ -731,11 +731,11 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
{
struct imx219 *imx219 = to_imx219(sd);
- if (code->index >= (ARRAY_SIZE(codes) / 4))
+ if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
return -EINVAL;
mutex_lock(&imx219->mutex);
- code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
+ code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
mutex_unlock(&imx219->mutex);
return 0;
@@ -831,14 +831,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
mutex_lock(&imx219->mutex);
- for (i = 0; i < ARRAY_SIZE(codes); i++)
- if (codes[i] == fmt->format.code)
+ for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
+ if (imx219_mbus_formats[i] == fmt->format.code)
break;
- if (i >= ARRAY_SIZE(codes))
+ if (i >= ARRAY_SIZE(imx219_mbus_formats))
i = 0;
/* Bayer order varies with flips */
- fmt->format.code = imx219_get_format_code(imx219, codes[i]);
+ fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
mode = v4l2_find_nearest_size(supported_modes,
ARRAY_SIZE(supported_modes),
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg
2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 10:40 ` [PATCH 1/5] media: i2c: imx219: Rename mbus codes array Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 11:11 ` Dave Stevenson
2023-07-07 15:20 ` Laurent Pinchart
2023-07-04 10:40 ` [PATCH 3/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois,
Jean-Michel Hautbois
From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Use the init_cfg pad level operation instead of the internal subdev
open operation to set default formats on the pads.
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 58 +++++++++++++++++---------------------
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 998a673a4290..191cb4a427cc 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
fmt->field = V4L2_FIELD_NONE;
}
-static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
- struct imx219 *imx219 = to_imx219(sd);
- struct v4l2_mbus_framefmt *try_fmt =
- v4l2_subdev_get_try_format(sd, fh->state, 0);
- struct v4l2_rect *try_crop;
-
- mutex_lock(&imx219->mutex);
-
- /* Initialize try_fmt */
- try_fmt->width = supported_modes[0].width;
- try_fmt->height = supported_modes[0].height;
- try_fmt->code = imx219_get_format_code(imx219,
- MEDIA_BUS_FMT_SRGGB10_1X10);
- try_fmt->field = V4L2_FIELD_NONE;
-
- /* Initialize try_crop rectangle. */
- try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
- try_crop->top = IMX219_PIXEL_ARRAY_TOP;
- try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
- try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
- try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
-
- mutex_unlock(&imx219->mutex);
-
- return 0;
-}
-
static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx219 *imx219 =
@@ -725,6 +697,31 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
.s_ctrl = imx219_set_ctrl,
};
+static int imx219_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state)
+{
+ struct imx219 *imx219 = to_imx219(sd);
+ struct v4l2_mbus_framefmt *format;
+ struct v4l2_rect *try_crop;
+
+ /* Initialize try_fmt */
+ format = v4l2_subdev_get_try_format(sd, state, 0);
+ format->width = supported_modes[0].width;
+ format->height = supported_modes[0].height;
+ format->code = imx219_get_format_code(imx219,
+ MEDIA_BUS_FMT_SRGGB10_1X10);
+ format->field = V4L2_FIELD_NONE;
+
+ /* Initialize try_crop rectangle. */
+ try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
+ try_crop->top = IMX219_PIXEL_ARRAY_TOP;
+ try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
+ try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
+ try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
+
+ return 0;
+}
+
static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
@@ -1235,6 +1232,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
};
static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
+ .init_cfg = imx219_init_cfg,
.enum_mbus_code = imx219_enum_mbus_code,
.get_fmt = imx219_get_pad_format,
.set_fmt = imx219_set_pad_format,
@@ -1248,9 +1246,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
.pad = &imx219_pad_ops,
};
-static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
- .open = imx219_open,
-};
static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
{
@@ -1509,7 +1504,6 @@ static int imx219_probe(struct i2c_client *client)
goto error_power_off;
/* Initialize subdev */
- imx219->sd.internal_ops = &imx219_internal_ops;
imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
V4L2_SUBDEV_FL_HAS_EVENTS;
imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] media: i2c: imx219: Use subdev active state
2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 10:40 ` [PATCH 1/5] media: i2c: imx219: Rename mbus codes array Jacopo Mondi
2023-07-04 10:40 ` [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-07 15:45 ` Laurent Pinchart
2023-07-07 16:46 ` Tommaso Merciai
2023-07-04 10:40 ` [PATCH 4/5] media: i2c: imx219: Simplify format assignment Jacopo Mondi
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
4 siblings, 2 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
Port the imx219 sensor driver to use the subdev active state.
Move all the format configuration to the subdevice state and simplify
the format handling, locking and initialization.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 182 +++++++++++--------------------------
1 file changed, 51 insertions(+), 131 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 191cb4a427cc..127ecee3643d 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -460,8 +460,6 @@ struct imx219 {
struct v4l2_subdev sd;
struct media_pad pad;
- struct v4l2_mbus_framefmt fmt;
-
struct clk *xclk; /* system clock to IMX219 */
u32 xclk_freq;
@@ -481,12 +479,6 @@ struct imx219 {
/* Current mode */
const struct imx219_mode *mode;
- /*
- * Mutex for serialized access:
- * Protect sensor module set pad format and start/stop streaming safely.
- */
- struct mutex mutex;
-
/* Streaming on/off */
bool streaming;
@@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
{
unsigned int i;
- lockdep_assert_held(&imx219->mutex);
-
for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
if (imx219_mbus_formats[i] == code)
break;
@@ -591,23 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
return imx219_mbus_formats[i];
}
-static void imx219_set_default_format(struct imx219 *imx219)
-{
- struct v4l2_mbus_framefmt *fmt;
-
- fmt = &imx219->fmt;
- fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
- fmt->colorspace = V4L2_COLORSPACE_SRGB;
- fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
- fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
- fmt->colorspace,
- fmt->ycbcr_enc);
- fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
- fmt->width = supported_modes[0].width;
- fmt->height = supported_modes[0].height;
- fmt->field = V4L2_FIELD_NONE;
-}
-
static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
{
struct imx219 *imx219 =
@@ -705,15 +678,21 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
struct v4l2_rect *try_crop;
/* Initialize try_fmt */
- format = v4l2_subdev_get_try_format(sd, state, 0);
+ format = v4l2_subdev_get_pad_format(sd, state, 0);
format->width = supported_modes[0].width;
format->height = supported_modes[0].height;
format->code = imx219_get_format_code(imx219,
MEDIA_BUS_FMT_SRGGB10_1X10);
format->field = V4L2_FIELD_NONE;
+ format->colorspace = V4L2_COLORSPACE_SRGB;
+ format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
+ format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+ format->colorspace,
+ format->ycbcr_enc);
+ format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
/* Initialize try_crop rectangle. */
- try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
+ try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
try_crop->top = IMX219_PIXEL_ARRAY_TOP;
try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
@@ -731,9 +710,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
return -EINVAL;
- mutex_lock(&imx219->mutex);
code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
- mutex_unlock(&imx219->mutex);
return 0;
}
@@ -748,9 +725,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
if (fse->index >= ARRAY_SIZE(supported_modes))
return -EINVAL;
- mutex_lock(&imx219->mutex);
code = imx219_get_format_code(imx219, fse->code);
- mutex_unlock(&imx219->mutex);
if (fse->code != code)
return -EINVAL;
@@ -782,52 +757,15 @@ static void imx219_update_pad_format(struct imx219 *imx219,
imx219_reset_colorspace(&fmt->format);
}
-static int __imx219_get_pad_format(struct imx219 *imx219,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
-{
- if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
- struct v4l2_mbus_framefmt *try_fmt =
- v4l2_subdev_get_try_format(&imx219->sd, sd_state,
- fmt->pad);
- /* update the code which could change due to vflip or hflip: */
- try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
- fmt->format = *try_fmt;
- } else {
- imx219_update_pad_format(imx219, imx219->mode, fmt);
- fmt->format.code = imx219_get_format_code(imx219,
- imx219->fmt.code);
- }
-
- return 0;
-}
-
-static int imx219_get_pad_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
-{
- struct imx219 *imx219 = to_imx219(sd);
- int ret;
-
- mutex_lock(&imx219->mutex);
- ret = __imx219_get_pad_format(imx219, sd_state, fmt);
- mutex_unlock(&imx219->mutex);
-
- return ret;
-}
-
static int imx219_set_pad_format(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *fmt)
{
struct imx219 *imx219 = to_imx219(sd);
const struct imx219_mode *mode;
- struct v4l2_mbus_framefmt *framefmt;
int exposure_max, exposure_def, hblank;
unsigned int i;
- mutex_lock(&imx219->mutex);
-
for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
if (imx219_mbus_formats[i] == fmt->format.code)
break;
@@ -841,13 +779,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
ARRAY_SIZE(supported_modes),
width, height,
fmt->format.width, fmt->format.height);
+
imx219_update_pad_format(imx219, mode, fmt);
- if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
- framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
- *framefmt = fmt->format;
- } else if (imx219->mode != mode ||
- imx219->fmt.code != fmt->format.code) {
- imx219->fmt = fmt->format;
+
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
imx219->mode = mode;
/* Update limits and set FPS to default */
__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -873,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
hblank);
}
- mutex_unlock(&imx219->mutex);
+ *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
return 0;
}
-static int imx219_set_framefmt(struct imx219 *imx219)
+static int imx219_set_framefmt(struct imx219 *imx219,
+ const struct v4l2_mbus_framefmt *format)
{
- switch (imx219->fmt.code) {
+ switch (format->code) {
case MEDIA_BUS_FMT_SRGGB8_1X8:
case MEDIA_BUS_FMT_SGRBG8_1X8:
case MEDIA_BUS_FMT_SGBRG8_1X8:
@@ -899,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
return -EINVAL;
}
-static int imx219_set_binning(struct imx219 *imx219)
+static int imx219_set_binning(struct imx219 *imx219,
+ const struct v4l2_mbus_framefmt *format)
{
if (!imx219->mode->binning) {
return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
@@ -907,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
IMX219_BINNING_NONE);
}
- switch (imx219->fmt.code) {
+ switch (format->code) {
case MEDIA_BUS_FMT_SRGGB8_1X8:
case MEDIA_BUS_FMT_SGRBG8_1X8:
case MEDIA_BUS_FMT_SGBRG8_1X8:
@@ -928,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
return -EINVAL;
}
-static const struct v4l2_rect *
-__imx219_get_pad_crop(struct imx219 *imx219,
- struct v4l2_subdev_state *sd_state,
- unsigned int pad, enum v4l2_subdev_format_whence which)
-{
- switch (which) {
- case V4L2_SUBDEV_FORMAT_TRY:
- return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
- case V4L2_SUBDEV_FORMAT_ACTIVE:
- return &imx219->mode->crop;
- }
-
- return NULL;
-}
-
static int imx219_get_selection(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_selection *sel)
{
switch (sel->target) {
case V4L2_SEL_TGT_CROP: {
- struct imx219 *imx219 = to_imx219(sd);
-
- mutex_lock(&imx219->mutex);
- sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
- sel->which);
- mutex_unlock(&imx219->mutex);
-
+ sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
return 0;
}
@@ -987,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
};
-static int imx219_start_streaming(struct imx219 *imx219)
+static int imx219_start_streaming(struct imx219 *imx219,
+ struct v4l2_subdev_state *state)
{
struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+ const struct v4l2_mbus_framefmt *format;
const struct imx219_reg_list *reg_list;
int ret;
@@ -1019,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
goto err_rpm_put;
}
- ret = imx219_set_framefmt(imx219);
+ format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
+ ret = imx219_set_framefmt(imx219, format);
if (ret) {
dev_err(&client->dev, "%s failed to set frame format: %d\n",
__func__, ret);
goto err_rpm_put;
}
- ret = imx219_set_binning(imx219);
+ ret = imx219_set_binning(imx219, format);
if (ret) {
dev_err(&client->dev, "%s failed to set binning: %d\n",
__func__, ret);
@@ -1075,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
{
struct imx219 *imx219 = to_imx219(sd);
+ struct v4l2_subdev_state *state;
int ret = 0;
- mutex_lock(&imx219->mutex);
- if (imx219->streaming == enable) {
- mutex_unlock(&imx219->mutex);
- return 0;
- }
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+
+ if (imx219->streaming == enable)
+ goto unlock;
if (enable) {
/*
* Apply default & customized values
* and then start streaming.
*/
- ret = imx219_start_streaming(imx219);
+ ret = imx219_start_streaming(imx219, state);
if (ret)
- goto err_unlock;
+ goto unlock;
} else {
imx219_stop_streaming(imx219);
}
imx219->streaming = enable;
- mutex_unlock(&imx219->mutex);
-
- return ret;
-
-err_unlock:
- mutex_unlock(&imx219->mutex);
-
+unlock:
+ v4l2_subdev_unlock_state(state);
return ret;
}
@@ -1168,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct imx219 *imx219 = to_imx219(sd);
+ struct v4l2_subdev_state *state;
int ret;
if (imx219->streaming) {
- ret = imx219_start_streaming(imx219);
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+ ret = imx219_start_streaming(imx219, state);
+ v4l2_subdev_unlock_state(state);
if (ret)
goto error;
}
@@ -1234,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
.init_cfg = imx219_init_cfg,
.enum_mbus_code = imx219_enum_mbus_code,
- .get_fmt = imx219_get_pad_format,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = imx219_set_pad_format,
.get_selection = imx219_get_selection,
.enum_frame_size = imx219_enum_frame_size,
@@ -1267,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
if (ret)
return ret;
- mutex_init(&imx219->mutex);
- ctrl_hdlr->lock = &imx219->mutex;
-
/* By default, PIXEL_RATE is read only */
imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
V4L2_CID_PIXEL_RATE,
@@ -1366,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
error:
v4l2_ctrl_handler_free(ctrl_hdlr);
- mutex_destroy(&imx219->mutex);
return ret;
}
@@ -1374,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
static void imx219_free_controls(struct imx219 *imx219)
{
v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
- mutex_destroy(&imx219->mutex);
}
static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
@@ -1511,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
/* Initialize source pad */
imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
- /* Initialize default format */
- imx219_set_default_format(imx219);
-
ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
if (ret) {
dev_err(dev, "failed to init entity pads: %d\n", ret);
goto error_handler_free;
}
+ imx219->sd.state_lock = imx219->ctrl_handler.lock;
+ ret = v4l2_subdev_init_finalize(&imx219->sd);
+ if (ret < 0) {
+ dev_err(dev, "subdev init error: %d\n", ret);
+ goto error_media_entity;
+ }
+
ret = v4l2_async_register_subdev_sensor(&imx219->sd);
if (ret < 0) {
dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
- goto error_media_entity;
+ goto error_subdev_cleanup;
}
/* Enable runtime PM and turn off the device */
@@ -1533,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
return 0;
+error_subdev_cleanup:
+ v4l2_subdev_cleanup(&imx219->sd);
+
error_media_entity:
media_entity_cleanup(&imx219->sd.entity);
@@ -1551,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
struct imx219 *imx219 = to_imx219(sd);
v4l2_async_unregister_subdev(sd);
+ v4l2_subdev_cleanup(sd);
media_entity_cleanup(&sd->entity);
imx219_free_controls(imx219);
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/5] media: i2c: imx219: Simplify format assignment
2023-07-04 10:40 ` Jacopo Mondi
` (2 preceding siblings ...)
2023-07-04 10:40 ` [PATCH 3/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-07 15:52 ` Laurent Pinchart
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
4 siblings, 1 reply; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
The set_fmt and init_cfg functions both fills a v4l2_mbus_framefmt
instance, passing in the mode and the media bus code. While set_fmt
uses function helpers, init_cfg open-codes the assignments.
Simplify the format initialization by moving it to a common helper.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 56 ++++++++++++++------------------------
1 file changed, 21 insertions(+), 35 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 127ecee3643d..c1246bd23b0d 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -670,6 +670,23 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
.s_ctrl = imx219_set_ctrl,
};
+static void imx219_update_pad_format(struct imx219 *imx219,
+ const struct imx219_mode *mode,
+ struct v4l2_mbus_framefmt *fmt, u32 code)
+{
+ /* Bayer order varies with flips */
+ fmt->code = imx219_get_format_code(imx219, code);
+ fmt->width = mode->width;
+ fmt->height = mode->height;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = V4L2_COLORSPACE_SRGB;
+ fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+ fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+ fmt->colorspace,
+ fmt->ycbcr_enc);
+ fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+}
+
static int imx219_init_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_state *state)
{
@@ -679,17 +696,8 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
/* Initialize try_fmt */
format = v4l2_subdev_get_pad_format(sd, state, 0);
- format->width = supported_modes[0].width;
- format->height = supported_modes[0].height;
- format->code = imx219_get_format_code(imx219,
- MEDIA_BUS_FMT_SRGGB10_1X10);
- format->field = V4L2_FIELD_NONE;
- format->colorspace = V4L2_COLORSPACE_SRGB;
- format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
- format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
- format->colorspace,
- format->ycbcr_enc);
- format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
+ imx219_update_pad_format(imx219, &supported_modes[0], format,
+ MEDIA_BUS_FMT_SRGGB10_1X10);
/* Initialize try_crop rectangle. */
try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
@@ -737,26 +745,6 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
return 0;
}
-static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
-{
- fmt->colorspace = V4L2_COLORSPACE_SRGB;
- fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
- fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
- fmt->colorspace,
- fmt->ycbcr_enc);
- fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
-}
-
-static void imx219_update_pad_format(struct imx219 *imx219,
- const struct imx219_mode *mode,
- struct v4l2_subdev_format *fmt)
-{
- fmt->format.width = mode->width;
- fmt->format.height = mode->height;
- fmt->format.field = V4L2_FIELD_NONE;
- imx219_reset_colorspace(&fmt->format);
-}
-
static int imx219_set_pad_format(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *fmt)
@@ -772,15 +760,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
if (i >= ARRAY_SIZE(imx219_mbus_formats))
i = 0;
- /* Bayer order varies with flips */
- fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
-
mode = v4l2_find_nearest_size(supported_modes,
ARRAY_SIZE(supported_modes),
width, height,
fmt->format.width, fmt->format.height);
- imx219_update_pad_format(imx219, mode, fmt);
+ imx219_update_pad_format(imx219, mode, &fmt->format,
+ imx219_mbus_formats[i]);
if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
imx219->mode = mode;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
2023-07-04 10:40 ` Jacopo Mondi
` (3 preceding siblings ...)
2023-07-04 10:40 ` [PATCH 4/5] media: i2c: imx219: Simplify format assignment Jacopo Mondi
@ 2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 14:39 ` kernel test robot
` (2 more replies)
4 siblings, 3 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-04 10:40 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
The imx219_set_pad_format() function adjusts the media bus code provided
through the v4l2_subdev_format parameter to a media bus code known
to be supported by the sensor.
The same exact operation is performed by the imx219_get_format_code()
function which called by imx219_update_pad_format(), which is in the
imx219_set_pad_format() call path.
Remove the duplicated operation and simplify imx219_set_pad_format().
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index c1246bd23b0d..37630e173040 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -754,19 +754,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
int exposure_max, exposure_def, hblank;
unsigned int i;
- for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
- if (imx219_mbus_formats[i] == fmt->format.code)
- break;
- if (i >= ARRAY_SIZE(imx219_mbus_formats))
- i = 0;
-
mode = v4l2_find_nearest_size(supported_modes,
ARRAY_SIZE(supported_modes),
width, height,
fmt->format.width, fmt->format.height);
- imx219_update_pad_format(imx219, mode, &fmt->format,
- imx219_mbus_formats[i]);
+ imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
imx219->mode = mode;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg
2023-07-04 10:40 ` [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg Jacopo Mondi
@ 2023-07-04 11:11 ` Dave Stevenson
2023-07-07 15:20 ` Laurent Pinchart
1 sibling, 0 replies; 18+ messages in thread
From: Dave Stevenson @ 2023-07-04 11:11 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Hans Verkuil, Sakari Ailus, Laurent Pinchart,
Tomi Valkeinen, Jean-Michel Hautbois, Jean-Michel Hautbois
Hi Jacopo
Thanks for the patch.
On Tue, 4 Jul 2023 at 11:41, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> Use the init_cfg pad level operation instead of the internal subdev
> open operation to set default formats on the pads.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
> drivers/media/i2c/imx219.c | 58 +++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 998a673a4290..191cb4a427cc 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
> fmt->field = V4L2_FIELD_NONE;
> }
>
> -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> -{
> - struct imx219 *imx219 = to_imx219(sd);
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(sd, fh->state, 0);
> - struct v4l2_rect *try_crop;
> -
> - mutex_lock(&imx219->mutex);
> -
> - /* Initialize try_fmt */
> - try_fmt->width = supported_modes[0].width;
> - try_fmt->height = supported_modes[0].height;
> - try_fmt->code = imx219_get_format_code(imx219,
> - MEDIA_BUS_FMT_SRGGB10_1X10);
> - try_fmt->field = V4L2_FIELD_NONE;
> -
> - /* Initialize try_crop rectangle. */
> - try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
> - try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> - try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> - try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> - try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> -
> - mutex_unlock(&imx219->mutex);
> -
> - return 0;
> -}
> -
> static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx219 *imx219 =
> @@ -725,6 +697,31 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> .s_ctrl = imx219_set_ctrl,
> };
>
> +static int imx219_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *try_crop;
> +
> + /* Initialize try_fmt */
> + format = v4l2_subdev_get_try_format(sd, state, 0);
> + format->width = supported_modes[0].width;
> + format->height = supported_modes[0].height;
> + format->code = imx219_get_format_code(imx219,
> + MEDIA_BUS_FMT_SRGGB10_1X10);
> + format->field = V4L2_FIELD_NONE;
> +
> + /* Initialize try_crop rectangle. */
> + try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
> + try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> + try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> + try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> + try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> +
> + return 0;
> +}
> +
> static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> @@ -1235,6 +1232,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> };
>
> static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> + .init_cfg = imx219_init_cfg,
> .enum_mbus_code = imx219_enum_mbus_code,
> .get_fmt = imx219_get_pad_format,
> .set_fmt = imx219_set_pad_format,
> @@ -1248,9 +1246,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
> .pad = &imx219_pad_ops,
> };
>
> -static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> - .open = imx219_open,
> -};
>
> static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> {
> @@ -1509,7 +1504,6 @@ static int imx219_probe(struct i2c_client *client)
> goto error_power_off;
>
> /* Initialize subdev */
> - imx219->sd.internal_ops = &imx219_internal_ops;
> imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> V4L2_SUBDEV_FL_HAS_EVENTS;
> imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
@ 2023-07-04 14:39 ` kernel test robot
2023-07-04 15:41 ` kernel test robot
2023-07-07 15:54 ` Laurent Pinchart
2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-07-04 14:39 UTC (permalink / raw)
To: Jacopo Mondi, linux-media
Cc: llvm, oe-kbuild-all, Jacopo Mondi, Dave Stevenson, Hans Verkuil,
Sakari Ailus, Laurent Pinchart, Tomi Valkeinen,
Jean-Michel Hautbois
Hi Jacopo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v6.4 next-20230704]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-i2c-imx219-Rename-mbus-codes-array/20230704-184252
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230704104057.149837-6-jacopo.mondi%40ideasonboard.com
patch subject: [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
config: i386-randconfig-r005-20230704 (https://download.01.org/0day-ci/archive/20230704/202307042233.nrNvmP4V-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230704/202307042233.nrNvmP4V-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307042233.nrNvmP4V-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/media/i2c/imx219.c:755:15: warning: unused variable 'i' [-Wunused-variable]
unsigned int i;
^
1 warning generated.
vim +/i +755 drivers/media/i2c/imx219.c
1283b3b8f82b90 Dave Stevenson 2020-01-20 747
1283b3b8f82b90 Dave Stevenson 2020-01-20 748 static int imx219_set_pad_format(struct v4l2_subdev *sd,
0d346d2a6f54f0 Tomi Valkeinen 2021-06-10 749 struct v4l2_subdev_state *sd_state,
1283b3b8f82b90 Dave Stevenson 2020-01-20 750 struct v4l2_subdev_format *fmt)
1283b3b8f82b90 Dave Stevenson 2020-01-20 751 {
1283b3b8f82b90 Dave Stevenson 2020-01-20 752 struct imx219 *imx219 = to_imx219(sd);
1283b3b8f82b90 Dave Stevenson 2020-01-20 753 const struct imx219_mode *mode;
1283b3b8f82b90 Dave Stevenson 2020-01-20 754 int exposure_max, exposure_def, hblank;
22da1d56e98215 Lad Prabhakar 2020-03-10 @755 unsigned int i;
1283b3b8f82b90 Dave Stevenson 2020-01-20 756
1283b3b8f82b90 Dave Stevenson 2020-01-20 757 mode = v4l2_find_nearest_size(supported_modes,
1283b3b8f82b90 Dave Stevenson 2020-01-20 758 ARRAY_SIZE(supported_modes),
1283b3b8f82b90 Dave Stevenson 2020-01-20 759 width, height,
1283b3b8f82b90 Dave Stevenson 2020-01-20 760 fmt->format.width, fmt->format.height);
563219f153d5f7 Jacopo Mondi 2023-07-04 761
7471d0495584f7 Jacopo Mondi 2023-07-04 762 imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
563219f153d5f7 Jacopo Mondi 2023-07-04 763
563219f153d5f7 Jacopo Mondi 2023-07-04 764 if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
1283b3b8f82b90 Dave Stevenson 2020-01-20 765 imx219->mode = mode;
1283b3b8f82b90 Dave Stevenson 2020-01-20 766 /* Update limits and set FPS to default */
1283b3b8f82b90 Dave Stevenson 2020-01-20 767 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
1283b3b8f82b90 Dave Stevenson 2020-01-20 768 IMX219_VTS_MAX - mode->height, 1,
1283b3b8f82b90 Dave Stevenson 2020-01-20 769 mode->vts_def - mode->height);
1283b3b8f82b90 Dave Stevenson 2020-01-20 770 __v4l2_ctrl_s_ctrl(imx219->vblank,
1283b3b8f82b90 Dave Stevenson 2020-01-20 771 mode->vts_def - mode->height);
1283b3b8f82b90 Dave Stevenson 2020-01-20 772 /* Update max exposure while meeting expected vblanking */
1283b3b8f82b90 Dave Stevenson 2020-01-20 773 exposure_max = mode->vts_def - 4;
1283b3b8f82b90 Dave Stevenson 2020-01-20 774 exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
1283b3b8f82b90 Dave Stevenson 2020-01-20 775 exposure_max : IMX219_EXPOSURE_DEFAULT;
1283b3b8f82b90 Dave Stevenson 2020-01-20 776 __v4l2_ctrl_modify_range(imx219->exposure,
1283b3b8f82b90 Dave Stevenson 2020-01-20 777 imx219->exposure->minimum,
1283b3b8f82b90 Dave Stevenson 2020-01-20 778 exposure_max, imx219->exposure->step,
1283b3b8f82b90 Dave Stevenson 2020-01-20 779 exposure_def);
1283b3b8f82b90 Dave Stevenson 2020-01-20 780 /*
1283b3b8f82b90 Dave Stevenson 2020-01-20 781 * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
1283b3b8f82b90 Dave Stevenson 2020-01-20 782 * depends on mode->width only, and is not changeble in any
1283b3b8f82b90 Dave Stevenson 2020-01-20 783 * way other than changing the mode.
1283b3b8f82b90 Dave Stevenson 2020-01-20 784 */
1283b3b8f82b90 Dave Stevenson 2020-01-20 785 hblank = IMX219_PPL_DEFAULT - mode->width;
1283b3b8f82b90 Dave Stevenson 2020-01-20 786 __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
1283b3b8f82b90 Dave Stevenson 2020-01-20 787 hblank);
1283b3b8f82b90 Dave Stevenson 2020-01-20 788 }
1283b3b8f82b90 Dave Stevenson 2020-01-20 789
563219f153d5f7 Jacopo Mondi 2023-07-04 790 *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
1283b3b8f82b90 Dave Stevenson 2020-01-20 791
1283b3b8f82b90 Dave Stevenson 2020-01-20 792 return 0;
1283b3b8f82b90 Dave Stevenson 2020-01-20 793 }
1283b3b8f82b90 Dave Stevenson 2020-01-20 794
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
2023-07-04 14:39 ` kernel test robot
@ 2023-07-04 15:41 ` kernel test robot
2023-07-07 15:54 ` Laurent Pinchart
2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-07-04 15:41 UTC (permalink / raw)
To: Jacopo Mondi, linux-media
Cc: oe-kbuild-all, Jacopo Mondi, Dave Stevenson, Hans Verkuil,
Sakari Ailus, Laurent Pinchart, Tomi Valkeinen,
Jean-Michel Hautbois
Hi Jacopo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v6.4 next-20230704]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-i2c-imx219-Rename-mbus-codes-array/20230704-184252
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230704104057.149837-6-jacopo.mondi%40ideasonboard.com
patch subject: [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230704/202307042337.lqiNqeJW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230704/202307042337.lqiNqeJW-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307042337.lqiNqeJW-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/media/i2c/imx219.c: In function 'imx219_set_pad_format':
>> drivers/media/i2c/imx219.c:755:22: warning: unused variable 'i' [-Wunused-variable]
755 | unsigned int i;
| ^
vim +/i +755 drivers/media/i2c/imx219.c
1283b3b8f82b90 Dave Stevenson 2020-01-20 747
1283b3b8f82b90 Dave Stevenson 2020-01-20 748 static int imx219_set_pad_format(struct v4l2_subdev *sd,
0d346d2a6f54f0 Tomi Valkeinen 2021-06-10 749 struct v4l2_subdev_state *sd_state,
1283b3b8f82b90 Dave Stevenson 2020-01-20 750 struct v4l2_subdev_format *fmt)
1283b3b8f82b90 Dave Stevenson 2020-01-20 751 {
1283b3b8f82b90 Dave Stevenson 2020-01-20 752 struct imx219 *imx219 = to_imx219(sd);
1283b3b8f82b90 Dave Stevenson 2020-01-20 753 const struct imx219_mode *mode;
1283b3b8f82b90 Dave Stevenson 2020-01-20 754 int exposure_max, exposure_def, hblank;
22da1d56e98215 Lad Prabhakar 2020-03-10 @755 unsigned int i;
1283b3b8f82b90 Dave Stevenson 2020-01-20 756
1283b3b8f82b90 Dave Stevenson 2020-01-20 757 mode = v4l2_find_nearest_size(supported_modes,
1283b3b8f82b90 Dave Stevenson 2020-01-20 758 ARRAY_SIZE(supported_modes),
1283b3b8f82b90 Dave Stevenson 2020-01-20 759 width, height,
1283b3b8f82b90 Dave Stevenson 2020-01-20 760 fmt->format.width, fmt->format.height);
563219f153d5f7 Jacopo Mondi 2023-07-04 761
7471d0495584f7 Jacopo Mondi 2023-07-04 762 imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
563219f153d5f7 Jacopo Mondi 2023-07-04 763
563219f153d5f7 Jacopo Mondi 2023-07-04 764 if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
1283b3b8f82b90 Dave Stevenson 2020-01-20 765 imx219->mode = mode;
1283b3b8f82b90 Dave Stevenson 2020-01-20 766 /* Update limits and set FPS to default */
1283b3b8f82b90 Dave Stevenson 2020-01-20 767 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
1283b3b8f82b90 Dave Stevenson 2020-01-20 768 IMX219_VTS_MAX - mode->height, 1,
1283b3b8f82b90 Dave Stevenson 2020-01-20 769 mode->vts_def - mode->height);
1283b3b8f82b90 Dave Stevenson 2020-01-20 770 __v4l2_ctrl_s_ctrl(imx219->vblank,
1283b3b8f82b90 Dave Stevenson 2020-01-20 771 mode->vts_def - mode->height);
1283b3b8f82b90 Dave Stevenson 2020-01-20 772 /* Update max exposure while meeting expected vblanking */
1283b3b8f82b90 Dave Stevenson 2020-01-20 773 exposure_max = mode->vts_def - 4;
1283b3b8f82b90 Dave Stevenson 2020-01-20 774 exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
1283b3b8f82b90 Dave Stevenson 2020-01-20 775 exposure_max : IMX219_EXPOSURE_DEFAULT;
1283b3b8f82b90 Dave Stevenson 2020-01-20 776 __v4l2_ctrl_modify_range(imx219->exposure,
1283b3b8f82b90 Dave Stevenson 2020-01-20 777 imx219->exposure->minimum,
1283b3b8f82b90 Dave Stevenson 2020-01-20 778 exposure_max, imx219->exposure->step,
1283b3b8f82b90 Dave Stevenson 2020-01-20 779 exposure_def);
1283b3b8f82b90 Dave Stevenson 2020-01-20 780 /*
1283b3b8f82b90 Dave Stevenson 2020-01-20 781 * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
1283b3b8f82b90 Dave Stevenson 2020-01-20 782 * depends on mode->width only, and is not changeble in any
1283b3b8f82b90 Dave Stevenson 2020-01-20 783 * way other than changing the mode.
1283b3b8f82b90 Dave Stevenson 2020-01-20 784 */
1283b3b8f82b90 Dave Stevenson 2020-01-20 785 hblank = IMX219_PPL_DEFAULT - mode->width;
1283b3b8f82b90 Dave Stevenson 2020-01-20 786 __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
1283b3b8f82b90 Dave Stevenson 2020-01-20 787 hblank);
1283b3b8f82b90 Dave Stevenson 2020-01-20 788 }
1283b3b8f82b90 Dave Stevenson 2020-01-20 789
563219f153d5f7 Jacopo Mondi 2023-07-04 790 *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
1283b3b8f82b90 Dave Stevenson 2020-01-20 791
1283b3b8f82b90 Dave Stevenson 2020-01-20 792 return 0;
1283b3b8f82b90 Dave Stevenson 2020-01-20 793 }
1283b3b8f82b90 Dave Stevenson 2020-01-20 794
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg
2023-07-04 10:40 ` [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg Jacopo Mondi
2023-07-04 11:11 ` Dave Stevenson
@ 2023-07-07 15:20 ` Laurent Pinchart
1 sibling, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-07-07 15:20 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Tomi Valkeinen, Jean-Michel Hautbois, Jean-Michel Hautbois
Hi Jacopo,
Thank you for the patch.
On Tue, Jul 04, 2023 at 12:40:54PM +0200, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> Use the init_cfg pad level operation instead of the internal subdev
> open operation to set default formats on the pads.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 58 +++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 998a673a4290..191cb4a427cc 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
> fmt->field = V4L2_FIELD_NONE;
> }
>
> -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> -{
> - struct imx219 *imx219 = to_imx219(sd);
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(sd, fh->state, 0);
> - struct v4l2_rect *try_crop;
> -
> - mutex_lock(&imx219->mutex);
> -
> - /* Initialize try_fmt */
> - try_fmt->width = supported_modes[0].width;
> - try_fmt->height = supported_modes[0].height;
> - try_fmt->code = imx219_get_format_code(imx219,
> - MEDIA_BUS_FMT_SRGGB10_1X10);
> - try_fmt->field = V4L2_FIELD_NONE;
> -
> - /* Initialize try_crop rectangle. */
> - try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
> - try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> - try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> - try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> - try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> -
> - mutex_unlock(&imx219->mutex);
> -
> - return 0;
> -}
> -
> static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx219 *imx219 =
> @@ -725,6 +697,31 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> .s_ctrl = imx219_set_ctrl,
> };
>
> +static int imx219_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *try_crop;
> +
> + /* Initialize try_fmt */
> + format = v4l2_subdev_get_try_format(sd, state, 0);
This should be v4l2_subdev_get_pad_format(). The two functions are
identical, but v4l2_subdev_get_try_format() implies it access formats in
the try state only, while .init_cfg() will be used for the active state
too later in this series. The comment should also be updated, or just
dropped.
> + format->width = supported_modes[0].width;
> + format->height = supported_modes[0].height;
> + format->code = imx219_get_format_code(imx219,
> + MEDIA_BUS_FMT_SRGGB10_1X10);
> + format->field = V4L2_FIELD_NONE;
> +
> + /* Initialize try_crop rectangle. */
> + try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
Same here, an the variable should be renamed to crop.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> + try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> + try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> + try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> +
> + return 0;
> +}
> +
> static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> @@ -1235,6 +1232,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> };
>
> static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> + .init_cfg = imx219_init_cfg,
> .enum_mbus_code = imx219_enum_mbus_code,
> .get_fmt = imx219_get_pad_format,
> .set_fmt = imx219_set_pad_format,
> @@ -1248,9 +1246,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
> .pad = &imx219_pad_ops,
> };
>
> -static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> - .open = imx219_open,
> -};
>
> static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> {
> @@ -1509,7 +1504,6 @@ static int imx219_probe(struct i2c_client *client)
> goto error_power_off;
>
> /* Initialize subdev */
> - imx219->sd.internal_ops = &imx219_internal_ops;
> imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> V4L2_SUBDEV_FL_HAS_EVENTS;
> imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] media: i2c: imx219: Use subdev active state
2023-07-04 10:40 ` [PATCH 3/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
@ 2023-07-07 15:45 ` Laurent Pinchart
2023-07-10 14:13 ` Jacopo Mondi
2023-07-07 16:46 ` Tommaso Merciai
1 sibling, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-07-07 15:45 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Tomi Valkeinen, Jean-Michel Hautbois
Hi Jacopo,
Thank you for the patch.
On Tue, Jul 04, 2023 at 12:40:55PM +0200, Jacopo Mondi wrote:
> Port the imx219 sensor driver to use the subdev active state.
>
> Move all the format configuration to the subdevice state and simplify
> the format handling, locking and initialization.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 182 +++++++++++--------------------------
> 1 file changed, 51 insertions(+), 131 deletions(-)
I like this :-)
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 191cb4a427cc..127ecee3643d 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -460,8 +460,6 @@ struct imx219 {
> struct v4l2_subdev sd;
> struct media_pad pad;
>
> - struct v4l2_mbus_framefmt fmt;
> -
> struct clk *xclk; /* system clock to IMX219 */
> u32 xclk_freq;
>
> @@ -481,12 +479,6 @@ struct imx219 {
> /* Current mode */
> const struct imx219_mode *mode;
>
> - /*
> - * Mutex for serialized access:
> - * Protect sensor module set pad format and start/stop streaming safely.
> - */
> - struct mutex mutex;
> -
> /* Streaming on/off */
> bool streaming;
>
> @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> {
> unsigned int i;
>
> - lockdep_assert_held(&imx219->mutex);
> -
> for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> if (imx219_mbus_formats[i] == code)
> break;
> @@ -591,23 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> return imx219_mbus_formats[i];
> }
>
> -static void imx219_set_default_format(struct imx219 *imx219)
> -{
> - struct v4l2_mbus_framefmt *fmt;
> -
> - fmt = &imx219->fmt;
> - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - fmt->colorspace,
> - fmt->ycbcr_enc);
> - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> - fmt->width = supported_modes[0].width;
> - fmt->height = supported_modes[0].height;
> - fmt->field = V4L2_FIELD_NONE;
> -}
> -
> static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx219 *imx219 =
> @@ -705,15 +678,21 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_rect *try_crop;
>
> /* Initialize try_fmt */
> - format = v4l2_subdev_get_try_format(sd, state, 0);
> + format = v4l2_subdev_get_pad_format(sd, state, 0);
> format->width = supported_modes[0].width;
> format->height = supported_modes[0].height;
> format->code = imx219_get_format_code(imx219,
> MEDIA_BUS_FMT_SRGGB10_1X10);
> format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_SRGB;
> + format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> + format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> + format->colorspace,
> + format->ycbcr_enc);
> + format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
I would have moved this to the previous patch.
>
> /* Initialize try_crop rectangle. */
> - try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
> + try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> @@ -731,9 +710,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> return -EINVAL;
>
> - mutex_lock(&imx219->mutex);
> code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> - mutex_unlock(&imx219->mutex);
>
> return 0;
> }
> @@ -748,9 +725,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> if (fse->index >= ARRAY_SIZE(supported_modes))
> return -EINVAL;
>
> - mutex_lock(&imx219->mutex);
> code = imx219_get_format_code(imx219, fse->code);
> - mutex_unlock(&imx219->mutex);
> if (fse->code != code)
> return -EINVAL;
>
> @@ -782,52 +757,15 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> imx219_reset_colorspace(&fmt->format);
> }
>
> -static int __imx219_get_pad_format(struct imx219 *imx219,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> - fmt->pad);
> - /* update the code which could change due to vflip or hflip: */
> - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> - fmt->format = *try_fmt;
> - } else {
> - imx219_update_pad_format(imx219, imx219->mode, fmt);
> - fmt->format.code = imx219_get_format_code(imx219,
> - imx219->fmt.code);
> - }
> -
> - return 0;
> -}
> -
> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - struct imx219 *imx219 = to_imx219(sd);
> - int ret;
> -
> - mutex_lock(&imx219->mutex);
> - ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> - mutex_unlock(&imx219->mutex);
> -
> - return ret;
> -}
> -
> static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> {
> struct imx219 *imx219 = to_imx219(sd);
> const struct imx219_mode *mode;
> - struct v4l2_mbus_framefmt *framefmt;
> int exposure_max, exposure_def, hblank;
> unsigned int i;
>
> - mutex_lock(&imx219->mutex);
> -
> for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> if (imx219_mbus_formats[i] == fmt->format.code)
> break;
> @@ -841,13 +779,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> ARRAY_SIZE(supported_modes),
> width, height,
> fmt->format.width, fmt->format.height);
> +
> imx219_update_pad_format(imx219, mode, fmt);
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> - *framefmt = fmt->format;
> - } else if (imx219->mode != mode ||
> - imx219->fmt.code != fmt->format.code) {
> - imx219->fmt = fmt->format;
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
Shouldn't you keep the mode change or mbus code change check ?
> imx219->mode = mode;
> /* Update limits and set FPS to default */
> __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -873,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> hblank);
> }
>
> - mutex_unlock(&imx219->mutex);
> + *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
>
> return 0;
> }
>
> -static int imx219_set_framefmt(struct imx219 *imx219)
> +static int imx219_set_framefmt(struct imx219 *imx219,
> + const struct v4l2_mbus_framefmt *format)
> {
> - switch (imx219->fmt.code) {
> + switch (format->code) {
> case MEDIA_BUS_FMT_SRGGB8_1X8:
> case MEDIA_BUS_FMT_SGRBG8_1X8:
> case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -899,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
> return -EINVAL;
> }
>
> -static int imx219_set_binning(struct imx219 *imx219)
> +static int imx219_set_binning(struct imx219 *imx219,
> + const struct v4l2_mbus_framefmt *format)
> {
> if (!imx219->mode->binning) {
> return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> @@ -907,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
> IMX219_BINNING_NONE);
> }
>
> - switch (imx219->fmt.code) {
> + switch (format->code) {
> case MEDIA_BUS_FMT_SRGGB8_1X8:
> case MEDIA_BUS_FMT_SGRBG8_1X8:
> case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -928,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
> return -EINVAL;
> }
>
> -static const struct v4l2_rect *
> -__imx219_get_pad_crop(struct imx219 *imx219,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, enum v4l2_subdev_format_whence which)
> -{
> - switch (which) {
> - case V4L2_SUBDEV_FORMAT_TRY:
> - return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> - case V4L2_SUBDEV_FORMAT_ACTIVE:
> - return &imx219->mode->crop;
> - }
> -
> - return NULL;
> -}
> -
> static int imx219_get_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> {
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP: {
> - struct imx219 *imx219 = to_imx219(sd);
> -
> - mutex_lock(&imx219->mutex);
> - sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> - sel->which);
> - mutex_unlock(&imx219->mutex);
> -
> + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> return 0;
> }
>
> @@ -987,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
> };
>
> -static int imx219_start_streaming(struct imx219 *imx219)
> +static int imx219_start_streaming(struct imx219 *imx219,
> + struct v4l2_subdev_state *state)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> + const struct v4l2_mbus_framefmt *format;
> const struct imx219_reg_list *reg_list;
> int ret;
>
> @@ -1019,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> - ret = imx219_set_framefmt(imx219);
> + format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> + ret = imx219_set_framefmt(imx219, format);
> if (ret) {
> dev_err(&client->dev, "%s failed to set frame format: %d\n",
> __func__, ret);
> goto err_rpm_put;
> }
>
> - ret = imx219_set_binning(imx219);
> + ret = imx219_set_binning(imx219, format);
> if (ret) {
> dev_err(&client->dev, "%s failed to set binning: %d\n",
> __func__, ret);
> @@ -1075,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
> static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_subdev_state *state;
> int ret = 0;
>
> - mutex_lock(&imx219->mutex);
> - if (imx219->streaming == enable) {
> - mutex_unlock(&imx219->mutex);
> - return 0;
> - }
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> + if (imx219->streaming == enable)
> + goto unlock;
This can't happen, I'd drop the check (possibly in a separate patch if
you prefer).
>
> if (enable) {
> /*
> * Apply default & customized values
> * and then start streaming.
> */
> - ret = imx219_start_streaming(imx219);
> + ret = imx219_start_streaming(imx219, state);
> if (ret)
> - goto err_unlock;
> + goto unlock;
> } else {
> imx219_stop_streaming(imx219);
> }
>
> imx219->streaming = enable;
>
> - mutex_unlock(&imx219->mutex);
> -
> - return ret;
> -
> -err_unlock:
> - mutex_unlock(&imx219->mutex);
> -
> +unlock:
> + v4l2_subdev_unlock_state(state);
> return ret;
> }
>
> @@ -1168,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
> {
> struct v4l2_subdev *sd = dev_get_drvdata(dev);
> struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_subdev_state *state;
> int ret;
>
> if (imx219->streaming) {
> - ret = imx219_start_streaming(imx219);
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + ret = imx219_start_streaming(imx219, state);
> + v4l2_subdev_unlock_state(state);
This shouldn't be needed, as the CSI-2 RX should always be suspended
before and resumed after the sensor. You could fix this issue before
this patch, or on top.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> if (ret)
> goto error;
> }
> @@ -1234,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> .init_cfg = imx219_init_cfg,
> .enum_mbus_code = imx219_enum_mbus_code,
> - .get_fmt = imx219_get_pad_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = imx219_set_pad_format,
> .get_selection = imx219_get_selection,
> .enum_frame_size = imx219_enum_frame_size,
> @@ -1267,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> if (ret)
> return ret;
>
> - mutex_init(&imx219->mutex);
> - ctrl_hdlr->lock = &imx219->mutex;
> -
> /* By default, PIXEL_RATE is read only */
> imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> @@ -1366,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>
> error:
> v4l2_ctrl_handler_free(ctrl_hdlr);
> - mutex_destroy(&imx219->mutex);
>
> return ret;
> }
> @@ -1374,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> static void imx219_free_controls(struct imx219 *imx219)
> {
> v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> - mutex_destroy(&imx219->mutex);
> }
>
> static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> @@ -1511,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
> /* Initialize source pad */
> imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>
> - /* Initialize default format */
> - imx219_set_default_format(imx219);
> -
> ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> if (ret) {
> dev_err(dev, "failed to init entity pads: %d\n", ret);
> goto error_handler_free;
> }
>
> + imx219->sd.state_lock = imx219->ctrl_handler.lock;
> + ret = v4l2_subdev_init_finalize(&imx219->sd);
> + if (ret < 0) {
> + dev_err(dev, "subdev init error: %d\n", ret);
> + goto error_media_entity;
> + }
> +
> ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> if (ret < 0) {
> dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> - goto error_media_entity;
> + goto error_subdev_cleanup;
> }
>
> /* Enable runtime PM and turn off the device */
> @@ -1533,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
>
> return 0;
>
> +error_subdev_cleanup:
> + v4l2_subdev_cleanup(&imx219->sd);
> +
> error_media_entity:
> media_entity_cleanup(&imx219->sd.entity);
>
> @@ -1551,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
> struct imx219 *imx219 = to_imx219(sd);
>
> v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> media_entity_cleanup(&sd->entity);
> imx219_free_controls(imx219);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] media: i2c: imx219: Simplify format assignment
2023-07-04 10:40 ` [PATCH 4/5] media: i2c: imx219: Simplify format assignment Jacopo Mondi
@ 2023-07-07 15:52 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-07-07 15:52 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Tomi Valkeinen, Jean-Michel Hautbois
Hi Jacopo,
Thank you for the patch.
On Tue, Jul 04, 2023 at 12:40:56PM +0200, Jacopo Mondi wrote:
> The set_fmt and init_cfg functions both fills a v4l2_mbus_framefmt
> instance, passing in the mode and the media bus code. While set_fmt
> uses function helpers, init_cfg open-codes the assignments.
>
> Simplify the format initialization by moving it to a common helper.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 56 ++++++++++++++------------------------
> 1 file changed, 21 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 127ecee3643d..c1246bd23b0d 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -670,6 +670,23 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> .s_ctrl = imx219_set_ctrl,
> };
>
> +static void imx219_update_pad_format(struct imx219 *imx219,
> + const struct imx219_mode *mode,
> + struct v4l2_mbus_framefmt *fmt, u32 code)
> +{
> + /* Bayer order varies with flips */
> + fmt->code = imx219_get_format_code(imx219, code);
> + fmt->width = mode->width;
> + fmt->height = mode->height;
> + fmt->field = V4L2_FIELD_NONE;
> + fmt->colorspace = V4L2_COLORSPACE_SRGB;
Candidate for a separate fix: use V4L2_COLORSPACE_RAW.
> + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> + fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> + fmt->colorspace,
> + fmt->ycbcr_enc);
And this can then be V4L2_QUANTIZATION_FULL_RANGE.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +}
> +
> static int imx219_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state)
> {
> @@ -679,17 +696,8 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>
> /* Initialize try_fmt */
> format = v4l2_subdev_get_pad_format(sd, state, 0);
> - format->width = supported_modes[0].width;
> - format->height = supported_modes[0].height;
> - format->code = imx219_get_format_code(imx219,
> - MEDIA_BUS_FMT_SRGGB10_1X10);
> - format->field = V4L2_FIELD_NONE;
> - format->colorspace = V4L2_COLORSPACE_SRGB;
> - format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> - format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - format->colorspace,
> - format->ycbcr_enc);
> - format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> + imx219_update_pad_format(imx219, &supported_modes[0], format,
> + MEDIA_BUS_FMT_SRGGB10_1X10);
>
> /* Initialize try_crop rectangle. */
> try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -737,26 +745,6 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> -{
> - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - fmt->colorspace,
> - fmt->ycbcr_enc);
> - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> -}
> -
> -static void imx219_update_pad_format(struct imx219 *imx219,
> - const struct imx219_mode *mode,
> - struct v4l2_subdev_format *fmt)
> -{
> - fmt->format.width = mode->width;
> - fmt->format.height = mode->height;
> - fmt->format.field = V4L2_FIELD_NONE;
> - imx219_reset_colorspace(&fmt->format);
> -}
> -
> static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> @@ -772,15 +760,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> if (i >= ARRAY_SIZE(imx219_mbus_formats))
> i = 0;
>
> - /* Bayer order varies with flips */
> - fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
> -
> mode = v4l2_find_nearest_size(supported_modes,
> ARRAY_SIZE(supported_modes),
> width, height,
> fmt->format.width, fmt->format.height);
>
> - imx219_update_pad_format(imx219, mode, fmt);
> + imx219_update_pad_format(imx219, mode, &fmt->format,
> + imx219_mbus_formats[i]);
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> imx219->mode = mode;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
2023-07-04 14:39 ` kernel test robot
2023-07-04 15:41 ` kernel test robot
@ 2023-07-07 15:54 ` Laurent Pinchart
2 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-07-07 15:54 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Tomi Valkeinen, Jean-Michel Hautbois
Hi Jacopo,
Thank you for the patch.
On Tue, Jul 04, 2023 at 12:40:57PM +0200, Jacopo Mondi wrote:
> The imx219_set_pad_format() function adjusts the media bus code provided
> through the v4l2_subdev_format parameter to a media bus code known
> to be supported by the sensor.
>
> The same exact operation is performed by the imx219_get_format_code()
> function which called by imx219_update_pad_format(), which is in the
> imx219_set_pad_format() call path.
>
> Remove the duplicated operation and simplify imx219_set_pad_format().
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index c1246bd23b0d..37630e173040 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -754,19 +754,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> int exposure_max, exposure_def, hblank;
> unsigned int i;
This variable should be dropped too.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> - for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> - if (imx219_mbus_formats[i] == fmt->format.code)
> - break;
> - if (i >= ARRAY_SIZE(imx219_mbus_formats))
> - i = 0;
> -
> mode = v4l2_find_nearest_size(supported_modes,
> ARRAY_SIZE(supported_modes),
> width, height,
> fmt->format.width, fmt->format.height);
>
> - imx219_update_pad_format(imx219, mode, &fmt->format,
> - imx219_mbus_formats[i]);
> + imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> imx219->mode = mode;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] media: i2c: imx219: Rename mbus codes array
2023-07-04 10:40 ` [PATCH 1/5] media: i2c: imx219: Rename mbus codes array Jacopo Mondi
@ 2023-07-07 16:30 ` Tommaso Merciai
0 siblings, 0 replies; 18+ messages in thread
From: Tommaso Merciai @ 2023-07-07 16:30 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois,
Jean-Michel Hautbois
Hi Jacopo,
On Tue, Jul 04, 2023 at 12:40:53PM +0200, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> The imx219 is using the name codes[] for the mbus format which is not
> easy to read and know what it means. Change it to imx219_mbus_formats.
What about:
The imx219 is using the name "codes" for the mbus formats array.
This is not easy to read and know what it means. Change it to
"imx219_mbus_formats"
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f9471c9e3a74..998a673a4290 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -345,7 +345,7 @@ static const char * const imx219_supply_name[] = {
> * - v flip
> * - h&v flips
> */
> -static const u32 codes[] = {
> +static const u32 imx219_mbus_formats[] = {
> MEDIA_BUS_FMT_SRGGB10_1X10,
> MEDIA_BUS_FMT_SGRBG10_1X10,
> MEDIA_BUS_FMT_SGBRG10_1X10,
> @@ -578,17 +578,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>
> lockdep_assert_held(&imx219->mutex);
>
> - for (i = 0; i < ARRAY_SIZE(codes); i++)
> - if (codes[i] == code)
> + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> + if (imx219_mbus_formats[i] == code)
> break;
>
> - if (i >= ARRAY_SIZE(codes))
> + if (i >= ARRAY_SIZE(imx219_mbus_formats))
> i = 0;
>
> i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> (imx219->hflip->val ? 1 : 0);
>
> - return codes[i];
> + return imx219_mbus_formats[i];
> }
>
> static void imx219_set_default_format(struct imx219 *imx219)
> @@ -731,11 +731,11 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> {
> struct imx219 *imx219 = to_imx219(sd);
>
> - if (code->index >= (ARRAY_SIZE(codes) / 4))
> + if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> return -EINVAL;
>
> mutex_lock(&imx219->mutex);
> - code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
> + code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> mutex_unlock(&imx219->mutex);
>
> return 0;
> @@ -831,14 +831,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>
> mutex_lock(&imx219->mutex);
>
> - for (i = 0; i < ARRAY_SIZE(codes); i++)
> - if (codes[i] == fmt->format.code)
> + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> + if (imx219_mbus_formats[i] == fmt->format.code)
> break;
> - if (i >= ARRAY_SIZE(codes))
> + if (i >= ARRAY_SIZE(imx219_mbus_formats))
> i = 0;
>
> /* Bayer order varies with flips */
> - fmt->format.code = imx219_get_format_code(imx219, codes[i]);
> + fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
>
> mode = v4l2_find_nearest_size(supported_modes,
> ARRAY_SIZE(supported_modes),
> --
> 2.40.1
Make sense for use "imx219_mbus_formats" instead of generic name "codes".
Looks good to me. Thanks for the patch.
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Regards,
Tommaso
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] media: i2c: imx219: Use subdev active state
2023-07-04 10:40 ` [PATCH 3/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
2023-07-07 15:45 ` Laurent Pinchart
@ 2023-07-07 16:46 ` Tommaso Merciai
2023-07-10 13:34 ` Jacopo Mondi
1 sibling, 1 reply; 18+ messages in thread
From: Tommaso Merciai @ 2023-07-07 16:46 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Dave Stevenson, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Tomi Valkeinen, Jean-Michel Hautbois
Hi Jacopo,
On Tue, Jul 04, 2023 at 12:40:55PM +0200, Jacopo Mondi wrote:
> Port the imx219 sensor driver to use the subdev active state.
>
> Move all the format configuration to the subdevice state and simplify
> the format handling, locking and initialization.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 182 +++++++++++--------------------------
> 1 file changed, 51 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 191cb4a427cc..127ecee3643d 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -460,8 +460,6 @@ struct imx219 {
> struct v4l2_subdev sd;
> struct media_pad pad;
>
> - struct v4l2_mbus_framefmt fmt;
> -
> struct clk *xclk; /* system clock to IMX219 */
> u32 xclk_freq;
>
> @@ -481,12 +479,6 @@ struct imx219 {
> /* Current mode */
> const struct imx219_mode *mode;
>
> - /*
> - * Mutex for serialized access:
> - * Protect sensor module set pad format and start/stop streaming safely.
> - */
> - struct mutex mutex;
> -
> /* Streaming on/off */
> bool streaming;
>
> @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> {
> unsigned int i;
>
> - lockdep_assert_held(&imx219->mutex);
> -
> for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> if (imx219_mbus_formats[i] == code)
> break;
> @@ -591,23 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> return imx219_mbus_formats[i];
> }
>
> -static void imx219_set_default_format(struct imx219 *imx219)
> -{
> - struct v4l2_mbus_framefmt *fmt;
> -
> - fmt = &imx219->fmt;
> - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - fmt->colorspace,
> - fmt->ycbcr_enc);
> - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> - fmt->width = supported_modes[0].width;
> - fmt->height = supported_modes[0].height;
> - fmt->field = V4L2_FIELD_NONE;
> -}
> -
> static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct imx219 *imx219 =
> @@ -705,15 +678,21 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_rect *try_crop;
>
> /* Initialize try_fmt */
> - format = v4l2_subdev_get_try_format(sd, state, 0);
> + format = v4l2_subdev_get_pad_format(sd, state, 0);
> format->width = supported_modes[0].width;
> format->height = supported_modes[0].height;
> format->code = imx219_get_format_code(imx219,
> MEDIA_BUS_FMT_SRGGB10_1X10);
> format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_SRGB;
> + format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> + format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> + format->colorspace,
> + format->ycbcr_enc);
I get warning here from checkpatch.pl
CHECK: Alignment should match open parenthesis
#87: FILE: drivers/media/i2c/imx219.c:690:
+ format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+
Thanks & Regards,
Tommaso
> + format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
>
> /* Initialize try_crop rectangle. */
> - try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
> + try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> @@ -731,9 +710,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> return -EINVAL;
>
> - mutex_lock(&imx219->mutex);
> code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> - mutex_unlock(&imx219->mutex);
>
> return 0;
> }
> @@ -748,9 +725,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> if (fse->index >= ARRAY_SIZE(supported_modes))
> return -EINVAL;
>
> - mutex_lock(&imx219->mutex);
> code = imx219_get_format_code(imx219, fse->code);
> - mutex_unlock(&imx219->mutex);
> if (fse->code != code)
> return -EINVAL;
>
> @@ -782,52 +757,15 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> imx219_reset_colorspace(&fmt->format);
> }
>
> -static int __imx219_get_pad_format(struct imx219 *imx219,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> - fmt->pad);
> - /* update the code which could change due to vflip or hflip: */
> - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> - fmt->format = *try_fmt;
> - } else {
> - imx219_update_pad_format(imx219, imx219->mode, fmt);
> - fmt->format.code = imx219_get_format_code(imx219,
> - imx219->fmt.code);
> - }
> -
> - return 0;
> -}
> -
> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - struct imx219 *imx219 = to_imx219(sd);
> - int ret;
> -
> - mutex_lock(&imx219->mutex);
> - ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> - mutex_unlock(&imx219->mutex);
> -
> - return ret;
> -}
> -
> static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> {
> struct imx219 *imx219 = to_imx219(sd);
> const struct imx219_mode *mode;
> - struct v4l2_mbus_framefmt *framefmt;
> int exposure_max, exposure_def, hblank;
> unsigned int i;
>
> - mutex_lock(&imx219->mutex);
> -
> for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> if (imx219_mbus_formats[i] == fmt->format.code)
> break;
> @@ -841,13 +779,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> ARRAY_SIZE(supported_modes),
> width, height,
> fmt->format.width, fmt->format.height);
> +
> imx219_update_pad_format(imx219, mode, fmt);
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> - *framefmt = fmt->format;
> - } else if (imx219->mode != mode ||
> - imx219->fmt.code != fmt->format.code) {
> - imx219->fmt = fmt->format;
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> imx219->mode = mode;
> /* Update limits and set FPS to default */
> __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -873,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> hblank);
> }
>
> - mutex_unlock(&imx219->mutex);
> + *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
>
> return 0;
> }
>
> -static int imx219_set_framefmt(struct imx219 *imx219)
> +static int imx219_set_framefmt(struct imx219 *imx219,
> + const struct v4l2_mbus_framefmt *format)
> {
> - switch (imx219->fmt.code) {
> + switch (format->code) {
> case MEDIA_BUS_FMT_SRGGB8_1X8:
> case MEDIA_BUS_FMT_SGRBG8_1X8:
> case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -899,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
> return -EINVAL;
> }
>
> -static int imx219_set_binning(struct imx219 *imx219)
> +static int imx219_set_binning(struct imx219 *imx219,
> + const struct v4l2_mbus_framefmt *format)
> {
> if (!imx219->mode->binning) {
> return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> @@ -907,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
> IMX219_BINNING_NONE);
> }
>
> - switch (imx219->fmt.code) {
> + switch (format->code) {
> case MEDIA_BUS_FMT_SRGGB8_1X8:
> case MEDIA_BUS_FMT_SGRBG8_1X8:
> case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -928,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
> return -EINVAL;
> }
>
> -static const struct v4l2_rect *
> -__imx219_get_pad_crop(struct imx219 *imx219,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, enum v4l2_subdev_format_whence which)
> -{
> - switch (which) {
> - case V4L2_SUBDEV_FORMAT_TRY:
> - return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> - case V4L2_SUBDEV_FORMAT_ACTIVE:
> - return &imx219->mode->crop;
> - }
> -
> - return NULL;
> -}
> -
> static int imx219_get_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> {
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP: {
> - struct imx219 *imx219 = to_imx219(sd);
> -
> - mutex_lock(&imx219->mutex);
> - sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> - sel->which);
> - mutex_unlock(&imx219->mutex);
> -
> + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> return 0;
> }
>
> @@ -987,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
> };
>
> -static int imx219_start_streaming(struct imx219 *imx219)
> +static int imx219_start_streaming(struct imx219 *imx219,
> + struct v4l2_subdev_state *state)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> + const struct v4l2_mbus_framefmt *format;
> const struct imx219_reg_list *reg_list;
> int ret;
>
> @@ -1019,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> - ret = imx219_set_framefmt(imx219);
> + format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> + ret = imx219_set_framefmt(imx219, format);
> if (ret) {
> dev_err(&client->dev, "%s failed to set frame format: %d\n",
> __func__, ret);
> goto err_rpm_put;
> }
>
> - ret = imx219_set_binning(imx219);
> + ret = imx219_set_binning(imx219, format);
> if (ret) {
> dev_err(&client->dev, "%s failed to set binning: %d\n",
> __func__, ret);
> @@ -1075,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
> static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_subdev_state *state;
> int ret = 0;
>
> - mutex_lock(&imx219->mutex);
> - if (imx219->streaming == enable) {
> - mutex_unlock(&imx219->mutex);
> - return 0;
> - }
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> + if (imx219->streaming == enable)
> + goto unlock;
>
> if (enable) {
> /*
> * Apply default & customized values
> * and then start streaming.
> */
> - ret = imx219_start_streaming(imx219);
> + ret = imx219_start_streaming(imx219, state);
> if (ret)
> - goto err_unlock;
> + goto unlock;
> } else {
> imx219_stop_streaming(imx219);
> }
>
> imx219->streaming = enable;
>
> - mutex_unlock(&imx219->mutex);
> -
> - return ret;
> -
> -err_unlock:
> - mutex_unlock(&imx219->mutex);
> -
> +unlock:
> + v4l2_subdev_unlock_state(state);
> return ret;
> }
>
> @@ -1168,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
> {
> struct v4l2_subdev *sd = dev_get_drvdata(dev);
> struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_subdev_state *state;
> int ret;
>
> if (imx219->streaming) {
> - ret = imx219_start_streaming(imx219);
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + ret = imx219_start_streaming(imx219, state);
> + v4l2_subdev_unlock_state(state);
> if (ret)
> goto error;
> }
> @@ -1234,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> .init_cfg = imx219_init_cfg,
> .enum_mbus_code = imx219_enum_mbus_code,
> - .get_fmt = imx219_get_pad_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = imx219_set_pad_format,
> .get_selection = imx219_get_selection,
> .enum_frame_size = imx219_enum_frame_size,
> @@ -1267,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> if (ret)
> return ret;
>
> - mutex_init(&imx219->mutex);
> - ctrl_hdlr->lock = &imx219->mutex;
> -
> /* By default, PIXEL_RATE is read only */
> imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> @@ -1366,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>
> error:
> v4l2_ctrl_handler_free(ctrl_hdlr);
> - mutex_destroy(&imx219->mutex);
>
> return ret;
> }
> @@ -1374,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> static void imx219_free_controls(struct imx219 *imx219)
> {
> v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> - mutex_destroy(&imx219->mutex);
> }
>
> static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> @@ -1511,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
> /* Initialize source pad */
> imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>
> - /* Initialize default format */
> - imx219_set_default_format(imx219);
> -
> ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> if (ret) {
> dev_err(dev, "failed to init entity pads: %d\n", ret);
> goto error_handler_free;
> }
>
> + imx219->sd.state_lock = imx219->ctrl_handler.lock;
> + ret = v4l2_subdev_init_finalize(&imx219->sd);
> + if (ret < 0) {
> + dev_err(dev, "subdev init error: %d\n", ret);
> + goto error_media_entity;
> + }
> +
> ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> if (ret < 0) {
> dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> - goto error_media_entity;
> + goto error_subdev_cleanup;
> }
>
> /* Enable runtime PM and turn off the device */
> @@ -1533,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
>
> return 0;
>
> +error_subdev_cleanup:
> + v4l2_subdev_cleanup(&imx219->sd);
> +
> error_media_entity:
> media_entity_cleanup(&imx219->sd.entity);
>
> @@ -1551,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
> struct imx219 *imx219 = to_imx219(sd);
>
> v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> media_entity_cleanup(&sd->entity);
> imx219_free_controls(imx219);
>
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] media: i2c: imx219: Use subdev active state
2023-07-07 16:46 ` Tommaso Merciai
@ 2023-07-10 13:34 ` Jacopo Mondi
0 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-10 13:34 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Jacopo Mondi, linux-media, Dave Stevenson, Hans Verkuil,
Sakari Ailus, Laurent Pinchart, Tomi Valkeinen,
Jean-Michel Hautbois
Hi Tommaso
On Fri, Jul 07, 2023 at 06:46:09PM +0200, Tommaso Merciai wrote:
> Hi Jacopo,
>
> On Tue, Jul 04, 2023 at 12:40:55PM +0200, Jacopo Mondi wrote:
> > Port the imx219 sensor driver to use the subdev active state.
> >
> > Move all the format configuration to the subdevice state and simplify
> > the format handling, locking and initialization.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > drivers/media/i2c/imx219.c | 182 +++++++++++--------------------------
> > 1 file changed, 51 insertions(+), 131 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 191cb4a427cc..127ecee3643d 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -460,8 +460,6 @@ struct imx219 {
> > struct v4l2_subdev sd;
> > struct media_pad pad;
> >
> > - struct v4l2_mbus_framefmt fmt;
> > -
> > struct clk *xclk; /* system clock to IMX219 */
> > u32 xclk_freq;
> >
> > @@ -481,12 +479,6 @@ struct imx219 {
> > /* Current mode */
> > const struct imx219_mode *mode;
> >
> > - /*
> > - * Mutex for serialized access:
> > - * Protect sensor module set pad format and start/stop streaming safely.
> > - */
> > - struct mutex mutex;
> > -
> > /* Streaming on/off */
> > bool streaming;
> >
> > @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > {
> > unsigned int i;
> >
> > - lockdep_assert_held(&imx219->mutex);
> > -
> > for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> > if (imx219_mbus_formats[i] == code)
> > break;
> > @@ -591,23 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > return imx219_mbus_formats[i];
> > }
> >
> > -static void imx219_set_default_format(struct imx219 *imx219)
> > -{
> > - struct v4l2_mbus_framefmt *fmt;
> > -
> > - fmt = &imx219->fmt;
> > - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > - fmt->colorspace,
> > - fmt->ycbcr_enc);
> > - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > - fmt->width = supported_modes[0].width;
> > - fmt->height = supported_modes[0].height;
> > - fmt->field = V4L2_FIELD_NONE;
> > -}
> > -
> > static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct imx219 *imx219 =
> > @@ -705,15 +678,21 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> > struct v4l2_rect *try_crop;
> >
> > /* Initialize try_fmt */
> > - format = v4l2_subdev_get_try_format(sd, state, 0);
> > + format = v4l2_subdev_get_pad_format(sd, state, 0);
> > format->width = supported_modes[0].width;
> > format->height = supported_modes[0].height;
> > format->code = imx219_get_format_code(imx219,
> > MEDIA_BUS_FMT_SRGGB10_1X10);
> > format->field = V4L2_FIELD_NONE;
> > + format->colorspace = V4L2_COLORSPACE_SRGB;
> > + format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> > + format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > + format->colorspace,
> > + format->ycbcr_enc);
>
> I get warning here from checkpatch.pl
>
> CHECK: Alignment should match open parenthesis
> #87: FILE: drivers/media/i2c/imx219.c:690:
> + format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +
Intentional, the line would go over 80 cols otherwise
I know we could go over, but as long as the subsystem enforces 80 cols
(see --max-line-length=80 in
Documentation/driver-api/media/maintainer-entry-profile.rst)
I would prefer to keep it this way
>
> Thanks & Regards,
> Tommaso
>
> > + format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> >
> > /* Initialize try_crop rectangle. */
> > - try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
> > + try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> > try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> > try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> > @@ -731,9 +710,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> > if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> > return -EINVAL;
> >
> > - mutex_lock(&imx219->mutex);
> > code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> > - mutex_unlock(&imx219->mutex);
> >
> > return 0;
> > }
> > @@ -748,9 +725,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > if (fse->index >= ARRAY_SIZE(supported_modes))
> > return -EINVAL;
> >
> > - mutex_lock(&imx219->mutex);
> > code = imx219_get_format_code(imx219, fse->code);
> > - mutex_unlock(&imx219->mutex);
> > if (fse->code != code)
> > return -EINVAL;
> >
> > @@ -782,52 +757,15 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> > imx219_reset_colorspace(&fmt->format);
> > }
> >
> > -static int __imx219_get_pad_format(struct imx219 *imx219,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *fmt)
> > -{
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > - struct v4l2_mbus_framefmt *try_fmt =
> > - v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> > - fmt->pad);
> > - /* update the code which could change due to vflip or hflip: */
> > - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> > - fmt->format = *try_fmt;
> > - } else {
> > - imx219_update_pad_format(imx219, imx219->mode, fmt);
> > - fmt->format.code = imx219_get_format_code(imx219,
> > - imx219->fmt.code);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *fmt)
> > -{
> > - struct imx219 *imx219 = to_imx219(sd);
> > - int ret;
> > -
> > - mutex_lock(&imx219->mutex);
> > - ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> > - mutex_unlock(&imx219->mutex);
> > -
> > - return ret;
> > -}
> > -
> > static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_format *fmt)
> > {
> > struct imx219 *imx219 = to_imx219(sd);
> > const struct imx219_mode *mode;
> > - struct v4l2_mbus_framefmt *framefmt;
> > int exposure_max, exposure_def, hblank;
> > unsigned int i;
> >
> > - mutex_lock(&imx219->mutex);
> > -
> > for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> > if (imx219_mbus_formats[i] == fmt->format.code)
> > break;
> > @@ -841,13 +779,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > ARRAY_SIZE(supported_modes),
> > width, height,
> > fmt->format.width, fmt->format.height);
> > +
> > imx219_update_pad_format(imx219, mode, fmt);
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > - *framefmt = fmt->format;
> > - } else if (imx219->mode != mode ||
> > - imx219->fmt.code != fmt->format.code) {
> > - imx219->fmt = fmt->format;
> > +
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > imx219->mode = mode;
> > /* Update limits and set FPS to default */
> > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -873,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > hblank);
> > }
> >
> > - mutex_unlock(&imx219->mutex);
> > + *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
> >
> > return 0;
> > }
> >
> > -static int imx219_set_framefmt(struct imx219 *imx219)
> > +static int imx219_set_framefmt(struct imx219 *imx219,
> > + const struct v4l2_mbus_framefmt *format)
> > {
> > - switch (imx219->fmt.code) {
> > + switch (format->code) {
> > case MEDIA_BUS_FMT_SRGGB8_1X8:
> > case MEDIA_BUS_FMT_SGRBG8_1X8:
> > case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -899,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
> > return -EINVAL;
> > }
> >
> > -static int imx219_set_binning(struct imx219 *imx219)
> > +static int imx219_set_binning(struct imx219 *imx219,
> > + const struct v4l2_mbus_framefmt *format)
> > {
> > if (!imx219->mode->binning) {
> > return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> > @@ -907,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
> > IMX219_BINNING_NONE);
> > }
> >
> > - switch (imx219->fmt.code) {
> > + switch (format->code) {
> > case MEDIA_BUS_FMT_SRGGB8_1X8:
> > case MEDIA_BUS_FMT_SGRBG8_1X8:
> > case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -928,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
> > return -EINVAL;
> > }
> >
> > -static const struct v4l2_rect *
> > -__imx219_get_pad_crop(struct imx219 *imx219,
> > - struct v4l2_subdev_state *sd_state,
> > - unsigned int pad, enum v4l2_subdev_format_whence which)
> > -{
> > - switch (which) {
> > - case V4L2_SUBDEV_FORMAT_TRY:
> > - return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> > - case V4L2_SUBDEV_FORMAT_ACTIVE:
> > - return &imx219->mode->crop;
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > static int imx219_get_selection(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_selection *sel)
> > {
> > switch (sel->target) {
> > case V4L2_SEL_TGT_CROP: {
> > - struct imx219 *imx219 = to_imx219(sd);
> > -
> > - mutex_lock(&imx219->mutex);
> > - sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> > - sel->which);
> > - mutex_unlock(&imx219->mutex);
> > -
> > + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > return 0;
> > }
> >
> > @@ -987,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
> > };
> >
> > -static int imx219_start_streaming(struct imx219 *imx219)
> > +static int imx219_start_streaming(struct imx219 *imx219,
> > + struct v4l2_subdev_state *state)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > + const struct v4l2_mbus_framefmt *format;
> > const struct imx219_reg_list *reg_list;
> > int ret;
> >
> > @@ -1019,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > goto err_rpm_put;
> > }
> >
> > - ret = imx219_set_framefmt(imx219);
> > + format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> > + ret = imx219_set_framefmt(imx219, format);
> > if (ret) {
> > dev_err(&client->dev, "%s failed to set frame format: %d\n",
> > __func__, ret);
> > goto err_rpm_put;
> > }
> >
> > - ret = imx219_set_binning(imx219);
> > + ret = imx219_set_binning(imx219, format);
> > if (ret) {
> > dev_err(&client->dev, "%s failed to set binning: %d\n",
> > __func__, ret);
> > @@ -1075,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
> > static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct imx219 *imx219 = to_imx219(sd);
> > + struct v4l2_subdev_state *state;
> > int ret = 0;
> >
> > - mutex_lock(&imx219->mutex);
> > - if (imx219->streaming == enable) {
> > - mutex_unlock(&imx219->mutex);
> > - return 0;
> > - }
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > +
> > + if (imx219->streaming == enable)
> > + goto unlock;
> >
> > if (enable) {
> > /*
> > * Apply default & customized values
> > * and then start streaming.
> > */
> > - ret = imx219_start_streaming(imx219);
> > + ret = imx219_start_streaming(imx219, state);
> > if (ret)
> > - goto err_unlock;
> > + goto unlock;
> > } else {
> > imx219_stop_streaming(imx219);
> > }
> >
> > imx219->streaming = enable;
> >
> > - mutex_unlock(&imx219->mutex);
> > -
> > - return ret;
> > -
> > -err_unlock:
> > - mutex_unlock(&imx219->mutex);
> > -
> > +unlock:
> > + v4l2_subdev_unlock_state(state);
> > return ret;
> > }
> >
> > @@ -1168,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
> > {
> > struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > struct imx219 *imx219 = to_imx219(sd);
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > if (imx219->streaming) {
> > - ret = imx219_start_streaming(imx219);
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + ret = imx219_start_streaming(imx219, state);
> > + v4l2_subdev_unlock_state(state);
> > if (ret)
> > goto error;
> > }
> > @@ -1234,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> > static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> > .init_cfg = imx219_init_cfg,
> > .enum_mbus_code = imx219_enum_mbus_code,
> > - .get_fmt = imx219_get_pad_format,
> > + .get_fmt = v4l2_subdev_get_fmt,
> > .set_fmt = imx219_set_pad_format,
> > .get_selection = imx219_get_selection,
> > .enum_frame_size = imx219_enum_frame_size,
> > @@ -1267,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> > if (ret)
> > return ret;
> >
> > - mutex_init(&imx219->mutex);
> > - ctrl_hdlr->lock = &imx219->mutex;
> > -
> > /* By default, PIXEL_RATE is read only */
> > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > V4L2_CID_PIXEL_RATE,
> > @@ -1366,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> >
> > error:
> > v4l2_ctrl_handler_free(ctrl_hdlr);
> > - mutex_destroy(&imx219->mutex);
> >
> > return ret;
> > }
> > @@ -1374,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> > static void imx219_free_controls(struct imx219 *imx219)
> > {
> > v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> > - mutex_destroy(&imx219->mutex);
> > }
> >
> > static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > @@ -1511,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
> > /* Initialize source pad */
> > imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> >
> > - /* Initialize default format */
> > - imx219_set_default_format(imx219);
> > -
> > ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> > if (ret) {
> > dev_err(dev, "failed to init entity pads: %d\n", ret);
> > goto error_handler_free;
> > }
> >
> > + imx219->sd.state_lock = imx219->ctrl_handler.lock;
> > + ret = v4l2_subdev_init_finalize(&imx219->sd);
> > + if (ret < 0) {
> > + dev_err(dev, "subdev init error: %d\n", ret);
> > + goto error_media_entity;
> > + }
> > +
> > ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> > if (ret < 0) {
> > dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> > - goto error_media_entity;
> > + goto error_subdev_cleanup;
> > }
> >
> > /* Enable runtime PM and turn off the device */
> > @@ -1533,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
> >
> > return 0;
> >
> > +error_subdev_cleanup:
> > + v4l2_subdev_cleanup(&imx219->sd);
> > +
> > error_media_entity:
> > media_entity_cleanup(&imx219->sd.entity);
> >
> > @@ -1551,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
> > struct imx219 *imx219 = to_imx219(sd);
> >
> > v4l2_async_unregister_subdev(sd);
> > + v4l2_subdev_cleanup(sd);
> > media_entity_cleanup(&sd->entity);
> > imx219_free_controls(imx219);
> >
> > --
> > 2.40.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] media: i2c: imx219: Use subdev active state
2023-07-07 15:45 ` Laurent Pinchart
@ 2023-07-10 14:13 ` Jacopo Mondi
0 siblings, 0 replies; 18+ messages in thread
From: Jacopo Mondi @ 2023-07-10 14:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, linux-media, Dave Stevenson, Hans Verkuil,
Sakari Ailus, Tomi Valkeinen, Jean-Michel Hautbois
Hi Laurent
On Fri, Jul 07, 2023 at 06:45:50PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jul 04, 2023 at 12:40:55PM +0200, Jacopo Mondi wrote:
> > Port the imx219 sensor driver to use the subdev active state.
> >
> > Move all the format configuration to the subdevice state and simplify
> > the format handling, locking and initialization.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > drivers/media/i2c/imx219.c | 182 +++++++++++--------------------------
> > 1 file changed, 51 insertions(+), 131 deletions(-)
>
> I like this :-)
>
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 191cb4a427cc..127ecee3643d 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -460,8 +460,6 @@ struct imx219 {
> > struct v4l2_subdev sd;
> > struct media_pad pad;
> >
> > - struct v4l2_mbus_framefmt fmt;
> > -
> > struct clk *xclk; /* system clock to IMX219 */
> > u32 xclk_freq;
> >
> > @@ -481,12 +479,6 @@ struct imx219 {
> > /* Current mode */
> > const struct imx219_mode *mode;
> >
> > - /*
> > - * Mutex for serialized access:
> > - * Protect sensor module set pad format and start/stop streaming safely.
> > - */
> > - struct mutex mutex;
> > -
> > /* Streaming on/off */
> > bool streaming;
> >
> > @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > {
> > unsigned int i;
> >
> > - lockdep_assert_held(&imx219->mutex);
> > -
> > for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> > if (imx219_mbus_formats[i] == code)
> > break;
> > @@ -591,23 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> > return imx219_mbus_formats[i];
> > }
> >
> > -static void imx219_set_default_format(struct imx219 *imx219)
> > -{
> > - struct v4l2_mbus_framefmt *fmt;
> > -
> > - fmt = &imx219->fmt;
> > - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > - fmt->colorspace,
> > - fmt->ycbcr_enc);
> > - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > - fmt->width = supported_modes[0].width;
> > - fmt->height = supported_modes[0].height;
> > - fmt->field = V4L2_FIELD_NONE;
> > -}
> > -
> > static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > struct imx219 *imx219 =
> > @@ -705,15 +678,21 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> > struct v4l2_rect *try_crop;
> >
> > /* Initialize try_fmt */
> > - format = v4l2_subdev_get_try_format(sd, state, 0);
> > + format = v4l2_subdev_get_pad_format(sd, state, 0);
> > format->width = supported_modes[0].width;
> > format->height = supported_modes[0].height;
> > format->code = imx219_get_format_code(imx219,
> > MEDIA_BUS_FMT_SRGGB10_1X10);
> > format->field = V4L2_FIELD_NONE;
> > + format->colorspace = V4L2_COLORSPACE_SRGB;
> > + format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> > + format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > + format->colorspace,
> > + format->ycbcr_enc);
> > + format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
>
> I would have moved this to the previous patch.
>
> >
> > /* Initialize try_crop rectangle. */
> > - try_crop = v4l2_subdev_get_try_crop(sd, state, 0);
> > + try_crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> > try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> > try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> > @@ -731,9 +710,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> > if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> > return -EINVAL;
> >
> > - mutex_lock(&imx219->mutex);
> > code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> > - mutex_unlock(&imx219->mutex);
> >
> > return 0;
> > }
> > @@ -748,9 +725,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > if (fse->index >= ARRAY_SIZE(supported_modes))
> > return -EINVAL;
> >
> > - mutex_lock(&imx219->mutex);
> > code = imx219_get_format_code(imx219, fse->code);
> > - mutex_unlock(&imx219->mutex);
> > if (fse->code != code)
> > return -EINVAL;
> >
> > @@ -782,52 +757,15 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> > imx219_reset_colorspace(&fmt->format);
> > }
> >
> > -static int __imx219_get_pad_format(struct imx219 *imx219,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *fmt)
> > -{
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > - struct v4l2_mbus_framefmt *try_fmt =
> > - v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> > - fmt->pad);
> > - /* update the code which could change due to vflip or hflip: */
> > - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> > - fmt->format = *try_fmt;
> > - } else {
> > - imx219_update_pad_format(imx219, imx219->mode, fmt);
> > - fmt->format.code = imx219_get_format_code(imx219,
> > - imx219->fmt.code);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *fmt)
> > -{
> > - struct imx219 *imx219 = to_imx219(sd);
> > - int ret;
> > -
> > - mutex_lock(&imx219->mutex);
> > - ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> > - mutex_unlock(&imx219->mutex);
> > -
> > - return ret;
> > -}
> > -
> > static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_format *fmt)
> > {
> > struct imx219 *imx219 = to_imx219(sd);
> > const struct imx219_mode *mode;
> > - struct v4l2_mbus_framefmt *framefmt;
> > int exposure_max, exposure_def, hblank;
> > unsigned int i;
> >
> > - mutex_lock(&imx219->mutex);
> > -
> > for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> > if (imx219_mbus_formats[i] == fmt->format.code)
> > break;
> > @@ -841,13 +779,10 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > ARRAY_SIZE(supported_modes),
> > width, height,
> > fmt->format.width, fmt->format.height);
> > +
> > imx219_update_pad_format(imx219, mode, fmt);
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > - *framefmt = fmt->format;
> > - } else if (imx219->mode != mode ||
> > - imx219->fmt.code != fmt->format.code) {
> > - imx219->fmt = fmt->format;
> > +
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>
> Shouldn't you keep the mode change or mbus code change check ?
>
Ah yes, let's skip over-writing the same mode over and over
> > imx219->mode = mode;
> > /* Update limits and set FPS to default */
> > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -873,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > hblank);
> > }
> >
> > - mutex_unlock(&imx219->mutex);
> > + *v4l2_subdev_get_pad_format(sd, sd_state, 0) = fmt->format;
> >
> > return 0;
> > }
> >
> > -static int imx219_set_framefmt(struct imx219 *imx219)
> > +static int imx219_set_framefmt(struct imx219 *imx219,
> > + const struct v4l2_mbus_framefmt *format)
> > {
> > - switch (imx219->fmt.code) {
> > + switch (format->code) {
> > case MEDIA_BUS_FMT_SRGGB8_1X8:
> > case MEDIA_BUS_FMT_SGRBG8_1X8:
> > case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -899,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
> > return -EINVAL;
> > }
> >
> > -static int imx219_set_binning(struct imx219 *imx219)
> > +static int imx219_set_binning(struct imx219 *imx219,
> > + const struct v4l2_mbus_framefmt *format)
> > {
> > if (!imx219->mode->binning) {
> > return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> > @@ -907,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
> > IMX219_BINNING_NONE);
> > }
> >
> > - switch (imx219->fmt.code) {
> > + switch (format->code) {
> > case MEDIA_BUS_FMT_SRGGB8_1X8:
> > case MEDIA_BUS_FMT_SGRBG8_1X8:
> > case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -928,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
> > return -EINVAL;
> > }
> >
> > -static const struct v4l2_rect *
> > -__imx219_get_pad_crop(struct imx219 *imx219,
> > - struct v4l2_subdev_state *sd_state,
> > - unsigned int pad, enum v4l2_subdev_format_whence which)
> > -{
> > - switch (which) {
> > - case V4L2_SUBDEV_FORMAT_TRY:
> > - return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> > - case V4L2_SUBDEV_FORMAT_ACTIVE:
> > - return &imx219->mode->crop;
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > static int imx219_get_selection(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_selection *sel)
> > {
> > switch (sel->target) {
> > case V4L2_SEL_TGT_CROP: {
> > - struct imx219 *imx219 = to_imx219(sd);
> > -
> > - mutex_lock(&imx219->mutex);
> > - sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> > - sel->which);
> > - mutex_unlock(&imx219->mutex);
> > -
> > + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > return 0;
> > }
> >
> > @@ -987,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
> > };
> >
> > -static int imx219_start_streaming(struct imx219 *imx219)
> > +static int imx219_start_streaming(struct imx219 *imx219,
> > + struct v4l2_subdev_state *state)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > + const struct v4l2_mbus_framefmt *format;
> > const struct imx219_reg_list *reg_list;
> > int ret;
> >
> > @@ -1019,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > goto err_rpm_put;
> > }
> >
> > - ret = imx219_set_framefmt(imx219);
> > + format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> > + ret = imx219_set_framefmt(imx219, format);
> > if (ret) {
> > dev_err(&client->dev, "%s failed to set frame format: %d\n",
> > __func__, ret);
> > goto err_rpm_put;
> > }
> >
> > - ret = imx219_set_binning(imx219);
> > + ret = imx219_set_binning(imx219, format);
> > if (ret) {
> > dev_err(&client->dev, "%s failed to set binning: %d\n",
> > __func__, ret);
> > @@ -1075,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
> > static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct imx219 *imx219 = to_imx219(sd);
> > + struct v4l2_subdev_state *state;
> > int ret = 0;
> >
> > - mutex_lock(&imx219->mutex);
> > - if (imx219->streaming == enable) {
> > - mutex_unlock(&imx219->mutex);
> > - return 0;
> > - }
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > +
> > + if (imx219->streaming == enable)
> > + goto unlock;
>
> This can't happen, I'd drop the check (possibly in a separate patch if
> you prefer).
>
I'm always doubtful when I see this in drivers, however as far as I
recall the core doesn't keep track of the subdev streaming state, does
it ?
> >
> > if (enable) {
> > /*
> > * Apply default & customized values
> > * and then start streaming.
> > */
> > - ret = imx219_start_streaming(imx219);
> > + ret = imx219_start_streaming(imx219, state);
> > if (ret)
> > - goto err_unlock;
> > + goto unlock;
> > } else {
> > imx219_stop_streaming(imx219);
> > }
> >
> > imx219->streaming = enable;
> >
> > - mutex_unlock(&imx219->mutex);
> > -
> > - return ret;
> > -
> > -err_unlock:
> > - mutex_unlock(&imx219->mutex);
> > -
> > +unlock:
> > + v4l2_subdev_unlock_state(state);
> > return ret;
> > }
> >
> > @@ -1168,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
> > {
> > struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > struct imx219 *imx219 = to_imx219(sd);
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > if (imx219->streaming) {
> > - ret = imx219_start_streaming(imx219);
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + ret = imx219_start_streaming(imx219, state);
> > + v4l2_subdev_unlock_state(state);
>
> This shouldn't be needed, as the CSI-2 RX should always be suspended
> before and resumed after the sensor. You could fix this issue before
> this patch, or on top.
Also this part puzzled me a bit, but as I get it it serves to resume
streaming after a suspend, why is the suspend order relevant ? Is the
CSI-2 presumed to call s_stream() on the sensor subdev after a pm
resume ?
Thanks
j
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > if (ret)
> > goto error;
> > }
> > @@ -1234,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> > static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> > .init_cfg = imx219_init_cfg,
> > .enum_mbus_code = imx219_enum_mbus_code,
> > - .get_fmt = imx219_get_pad_format,
> > + .get_fmt = v4l2_subdev_get_fmt,
> > .set_fmt = imx219_set_pad_format,
> > .get_selection = imx219_get_selection,
> > .enum_frame_size = imx219_enum_frame_size,
> > @@ -1267,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> > if (ret)
> > return ret;
> >
> > - mutex_init(&imx219->mutex);
> > - ctrl_hdlr->lock = &imx219->mutex;
> > -
> > /* By default, PIXEL_RATE is read only */
> > imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> > V4L2_CID_PIXEL_RATE,
> > @@ -1366,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> >
> > error:
> > v4l2_ctrl_handler_free(ctrl_hdlr);
> > - mutex_destroy(&imx219->mutex);
> >
> > return ret;
> > }
> > @@ -1374,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> > static void imx219_free_controls(struct imx219 *imx219)
> > {
> > v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> > - mutex_destroy(&imx219->mutex);
> > }
> >
> > static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > @@ -1511,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
> > /* Initialize source pad */
> > imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> >
> > - /* Initialize default format */
> > - imx219_set_default_format(imx219);
> > -
> > ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> > if (ret) {
> > dev_err(dev, "failed to init entity pads: %d\n", ret);
> > goto error_handler_free;
> > }
> >
> > + imx219->sd.state_lock = imx219->ctrl_handler.lock;
> > + ret = v4l2_subdev_init_finalize(&imx219->sd);
> > + if (ret < 0) {
> > + dev_err(dev, "subdev init error: %d\n", ret);
> > + goto error_media_entity;
> > + }
> > +
> > ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> > if (ret < 0) {
> > dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> > - goto error_media_entity;
> > + goto error_subdev_cleanup;
> > }
> >
> > /* Enable runtime PM and turn off the device */
> > @@ -1533,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
> >
> > return 0;
> >
> > +error_subdev_cleanup:
> > + v4l2_subdev_cleanup(&imx219->sd);
> > +
> > error_media_entity:
> > media_entity_cleanup(&imx219->sd.entity);
> >
> > @@ -1551,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
> > struct imx219 *imx219 = to_imx219(sd);
> >
> > v4l2_async_unregister_subdev(sd);
> > + v4l2_subdev_cleanup(sd);
> > media_entity_cleanup(&sd->entity);
> > imx219_free_controls(imx219);
> >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-07-10 14:13 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-04 10:36 [PATCH 0/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
2023-07-04 10:40 ` Jacopo Mondi
2023-07-04 10:40 ` [PATCH 1/5] media: i2c: imx219: Rename mbus codes array Jacopo Mondi
2023-07-07 16:30 ` Tommaso Merciai
2023-07-04 10:40 ` [PATCH 2/5] media: i2c: imx219: Switch from open to init_cfg Jacopo Mondi
2023-07-04 11:11 ` Dave Stevenson
2023-07-07 15:20 ` Laurent Pinchart
2023-07-04 10:40 ` [PATCH 3/5] media: i2c: imx219: Use subdev active state Jacopo Mondi
2023-07-07 15:45 ` Laurent Pinchart
2023-07-10 14:13 ` Jacopo Mondi
2023-07-07 16:46 ` Tommaso Merciai
2023-07-10 13:34 ` Jacopo Mondi
2023-07-04 10:40 ` [PATCH 4/5] media: i2c: imx219: Simplify format assignment Jacopo Mondi
2023-07-07 15:52 ` Laurent Pinchart
2023-07-04 10:40 ` [PATCH 5/5] media: i2c: imx219: Simplify code handling in s_fmt Jacopo Mondi
2023-07-04 14:39 ` kernel test robot
2023-07-04 15:41 ` kernel test robot
2023-07-07 15:54 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).