* [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
@ 2023-01-29 2:34 Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 1/8] media: imx: imx7-media-csi: Drop imx7_csi.cc field Laurent Pinchart
` (11 more replies)
0 siblings, 12 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
Hello,
This small series moves the imx7-mipi-csi driver to use the subdev
active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
the main change in 8/8.
Compared to v1, I've now successfully tested the series on an i.MX8MM.
The issues reported by Adam have been resolved by adding patch 7/8 and
fixing a problem in 8/8.
Laurent Pinchart (8):
media: imx: imx7-media-csi: Drop imx7_csi.cc field
media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
media: imx: imx7-media-csi: Drop unneeded check when starting
streaming
media: imx: imx7-media-csi: Drop unneeded src_sd check
media: imx: imx7-media-csi: Drop unneeded pad checks
media: imx: imx7-media-csi: Cleanup errors in
imx7_csi_async_register()
media: imx: imx7-media-csi: Zero format struct before calling
.get_fmt()
media: imx: imx7-media-csi: Use V4L2 subdev active state
drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++--------------
1 file changed, 70 insertions(+), 165 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/8] media: imx: imx7-media-csi: Drop imx7_csi.cc field
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 2/8] media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format() Laurent Pinchart
` (10 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The imx7_csi.cc field is set but never used. Drop it.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 1ef92c8c0098..f1d7da7763d5 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -228,7 +228,6 @@ struct imx7_csi {
struct media_pad pad[IMX7_CSI_PADS_NUM];
struct v4l2_mbus_framefmt format_mbus[IMX7_CSI_PADS_NUM];
- const struct imx7_csi_pixfmt *cc[IMX7_CSI_PADS_NUM];
/* Video device */
struct video_device *vdev; /* Video device */
@@ -1807,8 +1806,6 @@ static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
mf->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mf->colorspace);
mf->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(!cc->yuv,
mf->colorspace, mf->ycbcr_enc);
-
- csi->cc[i] = cc;
}
return 0;
@@ -2016,14 +2013,8 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
outfmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SRC,
sdformat->which);
*outfmt = format.format;
-
- if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- csi->cc[IMX7_CSI_PAD_SRC] = outcc;
}
- if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- csi->cc[sdformat->pad] = cc;
-
out_unlock:
mutex_unlock(&csi->lock);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/8] media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 1/8] media: imx: imx7-media-csi: Drop imx7_csi.cc field Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 3/8] media: imx: imx7-media-csi: Drop unneeded check when starting streaming Laurent Pinchart
` (9 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The imx7_csi_video_init_format() function instantiates a
v4l2_subdev_format on the stack, to only use the .format field of that
structure. Replace it with a v4l2_mbus_framefmt instance.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index f1d7da7763d5..112af7279ccf 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1600,17 +1600,15 @@ static struct imx7_csi_vb2_buffer *imx7_csi_video_next_buf(struct imx7_csi *csi)
static int imx7_csi_video_init_format(struct imx7_csi *csi)
{
- struct v4l2_subdev_format fmt_src = {
- .pad = IMX7_CSI_PAD_SRC,
- .which = V4L2_SUBDEV_FORMAT_ACTIVE,
- };
- fmt_src.format.code = IMX7_CSI_DEF_MBUS_CODE;
- fmt_src.format.width = IMX7_CSI_DEF_PIX_WIDTH;
- fmt_src.format.height = IMX7_CSI_DEF_PIX_HEIGHT;
+ struct v4l2_mbus_framefmt format = { };
- imx7_csi_mbus_fmt_to_pix_fmt(&csi->vdev_fmt, &fmt_src.format, NULL);
- csi->vdev_compose.width = fmt_src.format.width;
- csi->vdev_compose.height = fmt_src.format.height;
+ format.code = IMX7_CSI_DEF_MBUS_CODE;
+ format.width = IMX7_CSI_DEF_PIX_WIDTH;
+ format.height = IMX7_CSI_DEF_PIX_HEIGHT;
+
+ imx7_csi_mbus_fmt_to_pix_fmt(&csi->vdev_fmt, &format, NULL);
+ csi->vdev_compose.width = format.width;
+ csi->vdev_compose.height = format.height;
csi->vdev_cc = imx7_csi_find_pixel_format(csi->vdev_fmt.pixelformat);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/8] media: imx: imx7-media-csi: Drop unneeded check when starting streaming
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 1/8] media: imx: imx7-media-csi: Drop imx7_csi.cc field Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 2/8] media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format() Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 4/8] media: imx: imx7-media-csi: Drop unneeded src_sd check Laurent Pinchart
` (8 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The .s_stream() operation is guaranteed not to be called to start/stop
an already started/stopped subdev. Remove the unneeded check.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 112af7279ccf..45b29d312276 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1736,9 +1736,6 @@ static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
goto out_unlock;
}
- if (csi->is_streaming == !!enable)
- goto out_unlock;
-
if (enable) {
ret = imx7_csi_init(csi);
if (ret < 0)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/8] media: imx: imx7-media-csi: Drop unneeded src_sd check
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (2 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 3/8] media: imx: imx7-media-csi: Drop unneeded check when starting streaming Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 5/8] media: imx: imx7-media-csi: Drop unneeded pad checks Laurent Pinchart
` (7 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The .s_stream() and .link_validate() operations can't be called with a
NULL src_sd, as subdev nodes are not registered before the async
notifier completes. Remove the unneeded checks.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 45b29d312276..62232cd6775f 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1731,11 +1731,6 @@ static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
mutex_lock(&csi->lock);
- if (!csi->src_sd) {
- ret = -EPIPE;
- goto out_unlock;
- }
-
if (enable) {
ret = imx7_csi_init(csi);
if (ret < 0)
@@ -2026,9 +2021,6 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
unsigned int i;
int ret;
- if (!csi->src_sd)
- return -EPIPE;
-
/*
* Validate the source link, and record whether the source uses the
* parallel input or the CSI-2 receiver.
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 5/8] media: imx: imx7-media-csi: Drop unneeded pad checks
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (3 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 4/8] media: imx: imx7-media-csi: Drop unneeded src_sd check Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 6/8] media: imx: imx7-media-csi: Cleanup errors in imx7_csi_async_register() Laurent Pinchart
` (6 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The subdev core guarantees that the .set_fmt() operation is always
called with a valid pad. Drop the unneeded pad checks.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 62232cd6775f..1adf5c3392d9 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1936,6 +1936,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
sdformat->format.quantization = in_fmt->quantization;
sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
break;
+
case IMX7_CSI_PAD_SINK:
*cc = imx7_csi_find_mbus_format(sdformat->format.code);
if (!*cc) {
@@ -1947,8 +1948,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
if (sdformat->format.field != V4L2_FIELD_INTERLACED)
sdformat->format.field = V4L2_FIELD_NONE;
break;
- default:
- return -EINVAL;
}
imx7_csi_try_colorimetry(&sdformat->format);
@@ -1968,9 +1967,6 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_format format;
int ret = 0;
- if (sdformat->pad >= IMX7_CSI_PADS_NUM)
- return -EINVAL;
-
mutex_lock(&csi->lock);
if (csi->is_streaming) {
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 6/8] media: imx: imx7-media-csi: Cleanup errors in imx7_csi_async_register()
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (4 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 5/8] media: imx: imx7-media-csi: Drop unneeded pad checks Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 7/8] media: imx: imx7-media-csi: Zero format struct before calling .get_fmt() Laurent Pinchart
` (5 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
It's good practice for functions to perform error cleanup internally
when they fail, in order to not leave the device in a half-initialized
state. Move the async notifier cleanup from the probe error path to the
imx7_csi_async_register(), and drop the v4l2_async_nf_unregister() call
as there is no error path after the async notifier gets registered.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 1adf5c3392d9..733e44700ff9 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -2177,7 +2177,7 @@ static int imx7_csi_async_register(struct imx7_csi *csi)
ret = PTR_ERR(asd);
/* OK if asd already exists */
if (ret != -EEXIST)
- return ret;
+ goto error;
}
}
@@ -2185,9 +2185,13 @@ static int imx7_csi_async_register(struct imx7_csi *csi)
ret = v4l2_async_nf_register(&csi->v4l2_dev, &csi->notifier);
if (ret)
- return ret;
+ goto error;
return 0;
+
+error:
+ v4l2_async_nf_cleanup(&csi->notifier);
+ return ret;
}
static void imx7_csi_media_cleanup(struct imx7_csi *csi)
@@ -2329,13 +2333,10 @@ static int imx7_csi_probe(struct platform_device *pdev)
ret = imx7_csi_async_register(csi);
if (ret)
- goto subdev_notifier_cleanup;
+ goto media_cleanup;
return 0;
-subdev_notifier_cleanup:
- v4l2_async_nf_unregister(&csi->notifier);
- v4l2_async_nf_cleanup(&csi->notifier);
media_cleanup:
imx7_csi_media_cleanup(csi);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 7/8] media: imx: imx7-media-csi: Zero format struct before calling .get_fmt()
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (5 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 6/8] media: imx: imx7-media-csi: Cleanup errors in imx7_csi_async_register() Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state Laurent Pinchart
` (4 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
The v4l2_subdev_format structure passed to the .get_fmt() subdev
operation in imx7_csi_video_validate_fmt() isn't zeroed, which can cause
undefined behaviour. Fix it.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx7-media-csi.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 733e44700ff9..943e541768bd 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -1412,13 +1412,14 @@ static void imx7_csi_video_buf_queue(struct vb2_buffer *vb)
static int imx7_csi_video_validate_fmt(struct imx7_csi *csi)
{
- struct v4l2_subdev_format fmt_src;
+ struct v4l2_subdev_format fmt_src = {
+ .pad = IMX7_CSI_PAD_SRC,
+ .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+ };
const struct imx7_csi_pixfmt *cc;
int ret;
/* Retrieve the media bus format on the source subdev. */
- fmt_src.pad = IMX7_CSI_PAD_SRC;
- fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
ret = v4l2_subdev_call(&csi->sd, pad, get_fmt, NULL, &fmt_src);
if (ret)
return ret;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (6 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 7/8] media: imx: imx7-media-csi: Zero format struct before calling .get_fmt() Laurent Pinchart
@ 2023-01-29 2:34 ` Laurent Pinchart
2023-01-30 19:26 ` Jacopo Mondi
2023-01-30 23:45 ` [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to " Adam Ford
` (3 subsequent siblings)
11 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-29 2:34 UTC (permalink / raw)
To: linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
Use the V4L2 subdev active state API to store the active format. This
simplifies the driver not only by dropping the state stored in the
imx7_csi structure, but also by replacing the manual lock with the state
lock.
The is_streaming field is now protected by the active state lock, either
explicitly in .s_stream(), where the active state is locked manually, or
implicitly in .set_fmt(), which is called with the state locked.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Drop manual init_cfg call from probe
---
drivers/media/platform/nxp/imx7-media-csi.c | 173 ++++++--------------
1 file changed, 51 insertions(+), 122 deletions(-)
diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
index 943e541768bd..c22bf5c827e7 100644
--- a/drivers/media/platform/nxp/imx7-media-csi.c
+++ b/drivers/media/platform/nxp/imx7-media-csi.c
@@ -211,7 +211,6 @@ struct imx7_csi {
int irq;
struct clk *mclk;
- struct mutex lock; /* Protects is_streaming, format_mbus, cc */
spinlock_t irqlock; /* Protects last_eof */
/* Media and V4L2 device */
@@ -227,8 +226,6 @@ struct imx7_csi {
struct v4l2_subdev sd;
struct media_pad pad[IMX7_CSI_PADS_NUM];
- struct v4l2_mbus_framefmt format_mbus[IMX7_CSI_PADS_NUM];
-
/* Video device */
struct video_device *vdev; /* Video device */
struct media_pad vdev_pad; /* Video device pad */
@@ -509,7 +506,8 @@ static void imx7_csi_dma_stop(struct imx7_csi *csi)
imx7_csi_hw_disable_irq(csi);
}
-static void imx7_csi_configure(struct imx7_csi *csi)
+static void imx7_csi_configure(struct imx7_csi *csi,
+ struct v4l2_subdev_state *sd_state)
{
struct v4l2_pix_format *out_pix = &csi->vdev_fmt;
int width = out_pix->width;
@@ -540,12 +538,17 @@ static void imx7_csi_configure(struct imx7_csi *csi)
out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
width *= 2;
} else {
+ const struct v4l2_mbus_framefmt *sink_fmt;
+
+ sink_fmt = v4l2_subdev_get_pad_format(&csi->sd, sd_state,
+ IMX7_CSI_PAD_SINK);
+
cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC
| BIT_MCLKDIV(1) | BIT_MCLKEN;
cr18 |= BIT_DATA_FROM_MIPI;
- switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) {
+ switch (sink_fmt->code) {
case MEDIA_BUS_FMT_Y8_1X8:
case MEDIA_BUS_FMT_SBGGR8_1X8:
case MEDIA_BUS_FMT_SGBRG8_1X8:
@@ -626,7 +629,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
imx7_csi_reg_write(csi, stride, CSI_CSIFBUF_PARA);
}
-static int imx7_csi_init(struct imx7_csi *csi)
+static int imx7_csi_init(struct imx7_csi *csi,
+ struct v4l2_subdev_state *sd_state)
{
int ret;
@@ -634,7 +638,7 @@ static int imx7_csi_init(struct imx7_csi *csi)
if (ret < 0)
return ret;
- imx7_csi_configure(csi);
+ imx7_csi_configure(csi, sd_state);
ret = imx7_csi_dma_setup(csi);
if (ret < 0) {
@@ -1420,7 +1424,7 @@ static int imx7_csi_video_validate_fmt(struct imx7_csi *csi)
int ret;
/* Retrieve the media bus format on the source subdev. */
- ret = v4l2_subdev_call(&csi->sd, pad, get_fmt, NULL, &fmt_src);
+ ret = v4l2_subdev_call_state_active(&csi->sd, pad, get_fmt, &fmt_src);
if (ret)
return ret;
@@ -1728,12 +1732,13 @@ static int imx7_csi_video_init(struct imx7_csi *csi)
static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
{
struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+ struct v4l2_subdev_state *sd_state;
int ret = 0;
- mutex_lock(&csi->lock);
+ sd_state = v4l2_subdev_lock_and_get_active_state(sd);
if (enable) {
- ret = imx7_csi_init(csi);
+ ret = imx7_csi_init(csi, sd_state);
if (ret < 0)
goto out_unlock;
@@ -1755,29 +1760,14 @@ static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
csi->is_streaming = !!enable;
out_unlock:
- mutex_unlock(&csi->lock);
+ v4l2_subdev_unlock_state(sd_state);
return ret;
}
-static struct v4l2_mbus_framefmt *
-imx7_csi_get_format(struct imx7_csi *csi,
- struct v4l2_subdev_state *sd_state,
- unsigned int pad,
- enum v4l2_subdev_format_whence which)
-{
- if (which == V4L2_SUBDEV_FORMAT_TRY)
- return v4l2_subdev_get_try_format(&csi->sd, sd_state, pad);
-
- return &csi->format_mbus[pad];
-}
-
static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state)
{
- const enum v4l2_subdev_format_whence which =
- sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
- struct imx7_csi *csi = v4l2_get_subdevdata(sd);
const struct imx7_csi_pixfmt *cc;
int i;
@@ -1785,7 +1775,7 @@ static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
struct v4l2_mbus_framefmt *mf =
- imx7_csi_get_format(csi, sd_state, i, which);
+ v4l2_subdev_get_pad_format(sd, sd_state, i);
mf->code = IMX7_CSI_DEF_MBUS_CODE;
mf->width = IMX7_CSI_DEF_PIX_WIDTH;
@@ -1806,59 +1796,30 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
{
- struct imx7_csi *csi = v4l2_get_subdevdata(sd);
struct v4l2_mbus_framefmt *in_fmt;
int ret = 0;
- mutex_lock(&csi->lock);
-
- in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
- code->which);
+ in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
switch (code->pad) {
case IMX7_CSI_PAD_SINK:
ret = imx7_csi_enum_mbus_formats(&code->code, code->index);
break;
+
case IMX7_CSI_PAD_SRC:
if (code->index != 0) {
ret = -EINVAL;
- goto out_unlock;
+ break;
}
code->code = in_fmt->code;
break;
+
default:
ret = -EINVAL;
+ break;
}
-out_unlock:
- mutex_unlock(&csi->lock);
-
- return ret;
-}
-
-static int imx7_csi_get_fmt(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *sdformat)
-{
- struct imx7_csi *csi = v4l2_get_subdevdata(sd);
- struct v4l2_mbus_framefmt *fmt;
- int ret = 0;
-
- mutex_lock(&csi->lock);
-
- fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
- sdformat->which);
- if (!fmt) {
- ret = -EINVAL;
- goto out_unlock;
- }
-
- sdformat->format = *fmt;
-
-out_unlock:
- mutex_unlock(&csi->lock);
-
return ret;
}
@@ -1908,19 +1869,16 @@ static void imx7_csi_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt)
tryfmt->ycbcr_enc);
}
-static int imx7_csi_try_fmt(struct imx7_csi *csi,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *sdformat,
- const struct imx7_csi_pixfmt **cc)
+static void imx7_csi_try_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *sdformat,
+ const struct imx7_csi_pixfmt **cc)
{
const struct imx7_csi_pixfmt *in_cc;
struct v4l2_mbus_framefmt *in_fmt;
u32 code;
- in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
- sdformat->which);
- if (!in_fmt)
- return -EINVAL;
+ in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
switch (sdformat->pad) {
case IMX7_CSI_PAD_SRC:
@@ -1952,8 +1910,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
}
imx7_csi_try_colorimetry(&sdformat->format);
-
- return 0;
}
static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
@@ -1966,25 +1922,13 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
const struct imx7_csi_pixfmt *cc;
struct v4l2_mbus_framefmt *fmt;
struct v4l2_subdev_format format;
- int ret = 0;
- mutex_lock(&csi->lock);
+ if (csi->is_streaming)
+ return -EBUSY;
- if (csi->is_streaming) {
- ret = -EBUSY;
- goto out_unlock;
- }
+ imx7_csi_try_fmt(sd, sd_state, sdformat, &cc);
- ret = imx7_csi_try_fmt(csi, sd_state, sdformat, &cc);
- if (ret < 0)
- goto out_unlock;
-
- fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
- sdformat->which);
- if (!fmt) {
- ret = -EINVAL;
- goto out_unlock;
- }
+ fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
*fmt = sdformat->format;
@@ -1993,19 +1937,14 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
format.pad = IMX7_CSI_PAD_SRC;
format.which = sdformat->which;
format.format = sdformat->format;
- if (imx7_csi_try_fmt(csi, sd_state, &format, &outcc)) {
- ret = -EINVAL;
- goto out_unlock;
- }
- outfmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SRC,
- sdformat->which);
+ imx7_csi_try_fmt(sd, sd_state, &format, &outcc);
+
+ outfmt = v4l2_subdev_get_pad_format(sd, sd_state,
+ IMX7_CSI_PAD_SRC);
*outfmt = format.format;
}
-out_unlock:
- mutex_unlock(&csi->lock);
-
- return ret;
+ return 0;
}
static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
@@ -2105,7 +2044,7 @@ static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
.init_cfg = imx7_csi_init_cfg,
.enum_mbus_code = imx7_csi_enum_mbus_code,
- .get_fmt = imx7_csi_get_fmt,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = imx7_csi_set_fmt,
.link_validate = imx7_csi_pad_link_validate,
};
@@ -2199,6 +2138,7 @@ static void imx7_csi_media_cleanup(struct imx7_csi *csi)
{
v4l2_device_unregister(&csi->v4l2_dev);
media_device_unregister(&csi->mdev);
+ v4l2_subdev_cleanup(&csi->sd);
media_device_cleanup(&csi->mdev);
}
@@ -2266,6 +2206,10 @@ static int imx7_csi_media_init(struct imx7_csi *csi)
if (ret)
goto error;
+ ret = v4l2_subdev_init_finalize(&csi->sd);
+ if (ret)
+ goto error;
+
ret = v4l2_device_register_subdev(&csi->v4l2_dev, &csi->sd);
if (ret)
goto error;
@@ -2291,27 +2235,22 @@ static int imx7_csi_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, csi);
spin_lock_init(&csi->irqlock);
- mutex_init(&csi->lock);
/* Acquire resources and install interrupt handler. */
csi->mclk = devm_clk_get(&pdev->dev, "mclk");
if (IS_ERR(csi->mclk)) {
ret = PTR_ERR(csi->mclk);
dev_err(dev, "Failed to get mclk: %d", ret);
- goto destroy_mutex;
+ return ret;
}
csi->irq = platform_get_irq(pdev, 0);
- if (csi->irq < 0) {
- ret = csi->irq;
- goto destroy_mutex;
- }
+ if (csi->irq < 0)
+ return csi->irq;
csi->regbase = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(csi->regbase)) {
- ret = PTR_ERR(csi->regbase);
- goto destroy_mutex;
- }
+ if (IS_ERR(csi->regbase))
+ return PTR_ERR(csi->regbase);
csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
@@ -2319,31 +2258,23 @@ static int imx7_csi_probe(struct platform_device *pdev)
(void *)csi);
if (ret < 0) {
dev_err(dev, "Request CSI IRQ failed.\n");
- goto destroy_mutex;
+ return ret;
}
/* Initialize all the media device infrastructure. */
ret = imx7_csi_media_init(csi);
if (ret)
- goto destroy_mutex;
-
- /* Set the default mbus formats. */
- ret = imx7_csi_init_cfg(&csi->sd, NULL);
- if (ret)
- goto media_cleanup;
+ return ret;
ret = imx7_csi_async_register(csi);
if (ret)
- goto media_cleanup;
+ goto err_media_cleanup;
return 0;
-media_cleanup:
+err_media_cleanup:
imx7_csi_media_cleanup(csi);
-destroy_mutex:
- mutex_destroy(&csi->lock);
-
return ret;
}
@@ -2357,8 +2288,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
v4l2_async_nf_cleanup(&csi->notifier);
v4l2_async_unregister_subdev(&csi->sd);
- mutex_destroy(&csi->lock);
-
return 0;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state
2023-01-29 2:34 ` [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state Laurent Pinchart
@ 2023-01-30 19:26 ` Jacopo Mondi
2023-01-30 20:14 ` Laurent Pinchart
0 siblings, 1 reply; 16+ messages in thread
From: Jacopo Mondi @ 2023-01-30 19:26 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Adam Ford, Martin Kepplinger, Paul Elder,
Rui Miguel Silva, kernel, linux-imx
Hi Laurent
On Sun, Jan 29, 2023 at 04:34:29AM +0200, Laurent Pinchart wrote:
> Use the V4L2 subdev active state API to store the active format. This
> simplifies the driver not only by dropping the state stored in the
> imx7_csi structure, but also by replacing the manual lock with the state
> lock.
>
> The is_streaming field is now protected by the active state lock, either
> explicitly in .s_stream(), where the active state is locked manually, or
> implicitly in .set_fmt(), which is called with the state locked.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Drop manual init_cfg call from probe
> ---
> drivers/media/platform/nxp/imx7-media-csi.c | 173 ++++++--------------
> 1 file changed, 51 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> index 943e541768bd..c22bf5c827e7 100644
> --- a/drivers/media/platform/nxp/imx7-media-csi.c
> +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> @@ -211,7 +211,6 @@ struct imx7_csi {
> int irq;
> struct clk *mclk;
>
> - struct mutex lock; /* Protects is_streaming, format_mbus, cc */
> spinlock_t irqlock; /* Protects last_eof */
>
> /* Media and V4L2 device */
> @@ -227,8 +226,6 @@ struct imx7_csi {
> struct v4l2_subdev sd;
> struct media_pad pad[IMX7_CSI_PADS_NUM];
>
> - struct v4l2_mbus_framefmt format_mbus[IMX7_CSI_PADS_NUM];
> -
> /* Video device */
> struct video_device *vdev; /* Video device */
> struct media_pad vdev_pad; /* Video device pad */
> @@ -509,7 +506,8 @@ static void imx7_csi_dma_stop(struct imx7_csi *csi)
> imx7_csi_hw_disable_irq(csi);
> }
>
> -static void imx7_csi_configure(struct imx7_csi *csi)
> +static void imx7_csi_configure(struct imx7_csi *csi,
> + struct v4l2_subdev_state *sd_state)
> {
> struct v4l2_pix_format *out_pix = &csi->vdev_fmt;
> int width = out_pix->width;
> @@ -540,12 +538,17 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
> width *= 2;
> } else {
> + const struct v4l2_mbus_framefmt *sink_fmt;
> +
> + sink_fmt = v4l2_subdev_get_pad_format(&csi->sd, sd_state,
> + IMX7_CSI_PAD_SINK);
> +
> cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC
> | BIT_MCLKDIV(1) | BIT_MCLKEN;
>
> cr18 |= BIT_DATA_FROM_MIPI;
>
> - switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) {
> + switch (sink_fmt->code) {
> case MEDIA_BUS_FMT_Y8_1X8:
> case MEDIA_BUS_FMT_SBGGR8_1X8:
> case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -626,7 +629,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> imx7_csi_reg_write(csi, stride, CSI_CSIFBUF_PARA);
> }
>
> -static int imx7_csi_init(struct imx7_csi *csi)
> +static int imx7_csi_init(struct imx7_csi *csi,
> + struct v4l2_subdev_state *sd_state)
> {
> int ret;
>
> @@ -634,7 +638,7 @@ static int imx7_csi_init(struct imx7_csi *csi)
> if (ret < 0)
> return ret;
>
> - imx7_csi_configure(csi);
> + imx7_csi_configure(csi, sd_state);
>
> ret = imx7_csi_dma_setup(csi);
> if (ret < 0) {
> @@ -1420,7 +1424,7 @@ static int imx7_csi_video_validate_fmt(struct imx7_csi *csi)
> int ret;
>
> /* Retrieve the media bus format on the source subdev. */
> - ret = v4l2_subdev_call(&csi->sd, pad, get_fmt, NULL, &fmt_src);
> + ret = v4l2_subdev_call_state_active(&csi->sd, pad, get_fmt, &fmt_src);
> if (ret)
> return ret;
>
> @@ -1728,12 +1732,13 @@ static int imx7_csi_video_init(struct imx7_csi *csi)
> static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct v4l2_subdev_state *sd_state;
> int ret = 0;
>
> - mutex_lock(&csi->lock);
> + sd_state = v4l2_subdev_lock_and_get_active_state(sd);
>
> if (enable) {
> - ret = imx7_csi_init(csi);
> + ret = imx7_csi_init(csi, sd_state);
> if (ret < 0)
> goto out_unlock;
>
> @@ -1755,29 +1760,14 @@ static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
> csi->is_streaming = !!enable;
>
> out_unlock:
> - mutex_unlock(&csi->lock);
> + v4l2_subdev_unlock_state(sd_state);
>
> return ret;
> }
>
> -static struct v4l2_mbus_framefmt *
> -imx7_csi_get_format(struct imx7_csi *csi,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad,
> - enum v4l2_subdev_format_whence which)
> -{
> - if (which == V4L2_SUBDEV_FORMAT_TRY)
> - return v4l2_subdev_get_try_format(&csi->sd, sd_state, pad);
> -
> - return &csi->format_mbus[pad];
> -}
> -
> static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state)
> {
> - const enum v4l2_subdev_format_whence which =
> - sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> const struct imx7_csi_pixfmt *cc;
> int i;
>
> @@ -1785,7 +1775,7 @@ static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
>
> for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
> struct v4l2_mbus_framefmt *mf =
> - imx7_csi_get_format(csi, sd_state, i, which);
> + v4l2_subdev_get_pad_format(sd, sd_state, i);
>
> mf->code = IMX7_CSI_DEF_MBUS_CODE;
> mf->width = IMX7_CSI_DEF_PIX_WIDTH;
> @@ -1806,59 +1796,30 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *in_fmt;
> int ret = 0;
>
> - mutex_lock(&csi->lock);
> -
> - in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
> - code->which);
> + in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
>
> switch (code->pad) {
> case IMX7_CSI_PAD_SINK:
> ret = imx7_csi_enum_mbus_formats(&code->code, code->index);
> break;
> +
> case IMX7_CSI_PAD_SRC:
> if (code->index != 0) {
> ret = -EINVAL;
> - goto out_unlock;
> + break;
> }
>
> code->code = in_fmt->code;
> break;
> +
> default:
> ret = -EINVAL;
> + break;
> }
>
> -out_unlock:
> - mutex_unlock(&csi->lock);
> -
> - return ret;
> -}
> -
> -static int imx7_csi_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *sdformat)
> -{
> - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> - struct v4l2_mbus_framefmt *fmt;
> - int ret = 0;
> -
> - mutex_lock(&csi->lock);
> -
> - fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
> - sdformat->which);
> - if (!fmt) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> -
> - sdformat->format = *fmt;
> -
> -out_unlock:
> - mutex_unlock(&csi->lock);
> -
> return ret;
> }
>
> @@ -1908,19 +1869,16 @@ static void imx7_csi_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt)
> tryfmt->ycbcr_enc);
> }
>
> -static int imx7_csi_try_fmt(struct imx7_csi *csi,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *sdformat,
> - const struct imx7_csi_pixfmt **cc)
> +static void imx7_csi_try_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *sdformat,
> + const struct imx7_csi_pixfmt **cc)
> {
> const struct imx7_csi_pixfmt *in_cc;
> struct v4l2_mbus_framefmt *in_fmt;
> u32 code;
>
> - in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
> - sdformat->which);
> - if (!in_fmt)
> - return -EINVAL;
> + in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
>
> switch (sdformat->pad) {
> case IMX7_CSI_PAD_SRC:
> @@ -1952,8 +1910,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> }
>
> imx7_csi_try_colorimetry(&sdformat->format);
> -
> - return 0;
> }
>
> static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> @@ -1966,25 +1922,13 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> const struct imx7_csi_pixfmt *cc;
> struct v4l2_mbus_framefmt *fmt;
> struct v4l2_subdev_format format;
> - int ret = 0;
>
> - mutex_lock(&csi->lock);
> + if (csi->is_streaming)
> + return -EBUSY;
Do you still need this or the state is locked by s_stream() and if I'm
not mistaken the core locks the state before calling the op ?
>
> - if (csi->is_streaming) {
> - ret = -EBUSY;
> - goto out_unlock;
> - }
> + imx7_csi_try_fmt(sd, sd_state, sdformat, &cc);
>
> - ret = imx7_csi_try_fmt(csi, sd_state, sdformat, &cc);
> - if (ret < 0)
> - goto out_unlock;
> -
> - fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
> - sdformat->which);
> - if (!fmt) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> + fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
>
> *fmt = sdformat->format;
>
> @@ -1993,19 +1937,14 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> format.pad = IMX7_CSI_PAD_SRC;
> format.which = sdformat->which;
> format.format = sdformat->format;
> - if (imx7_csi_try_fmt(csi, sd_state, &format, &outcc)) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> - outfmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SRC,
> - sdformat->which);
> + imx7_csi_try_fmt(sd, sd_state, &format, &outcc);
> +
> + outfmt = v4l2_subdev_get_pad_format(sd, sd_state,
> + IMX7_CSI_PAD_SRC);
> *outfmt = format.format;
> }
>
> -out_unlock:
> - mutex_unlock(&csi->lock);
> -
> - return ret;
> + return 0;
> }
>
> static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
> @@ -2105,7 +2044,7 @@ static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
> static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
> .init_cfg = imx7_csi_init_cfg,
> .enum_mbus_code = imx7_csi_enum_mbus_code,
> - .get_fmt = imx7_csi_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = imx7_csi_set_fmt,
> .link_validate = imx7_csi_pad_link_validate,
> };
> @@ -2199,6 +2138,7 @@ static void imx7_csi_media_cleanup(struct imx7_csi *csi)
> {
> v4l2_device_unregister(&csi->v4l2_dev);
> media_device_unregister(&csi->mdev);
> + v4l2_subdev_cleanup(&csi->sd);
> media_device_cleanup(&csi->mdev);
> }
>
> @@ -2266,6 +2206,10 @@ static int imx7_csi_media_init(struct imx7_csi *csi)
> if (ret)
> goto error;
>
> + ret = v4l2_subdev_init_finalize(&csi->sd);
> + if (ret)
> + goto error;
> +
> ret = v4l2_device_register_subdev(&csi->v4l2_dev, &csi->sd);
> if (ret)
> goto error;
> @@ -2291,27 +2235,22 @@ static int imx7_csi_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, csi);
>
> spin_lock_init(&csi->irqlock);
> - mutex_init(&csi->lock);
>
> /* Acquire resources and install interrupt handler. */
> csi->mclk = devm_clk_get(&pdev->dev, "mclk");
> if (IS_ERR(csi->mclk)) {
> ret = PTR_ERR(csi->mclk);
> dev_err(dev, "Failed to get mclk: %d", ret);
> - goto destroy_mutex;
> + return ret;
> }
>
> csi->irq = platform_get_irq(pdev, 0);
> - if (csi->irq < 0) {
> - ret = csi->irq;
> - goto destroy_mutex;
> - }
> + if (csi->irq < 0)
> + return csi->irq;
>
> csi->regbase = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(csi->regbase)) {
> - ret = PTR_ERR(csi->regbase);
> - goto destroy_mutex;
> - }
> + if (IS_ERR(csi->regbase))
> + return PTR_ERR(csi->regbase);
>
> csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
>
> @@ -2319,31 +2258,23 @@ static int imx7_csi_probe(struct platform_device *pdev)
> (void *)csi);
> if (ret < 0) {
> dev_err(dev, "Request CSI IRQ failed.\n");
> - goto destroy_mutex;
> + return ret;
> }
>
> /* Initialize all the media device infrastructure. */
> ret = imx7_csi_media_init(csi);
> if (ret)
> - goto destroy_mutex;
> -
> - /* Set the default mbus formats. */
> - ret = imx7_csi_init_cfg(&csi->sd, NULL);
> - if (ret)
> - goto media_cleanup;
> + return ret;
>
> ret = imx7_csi_async_register(csi);
> if (ret)
> - goto media_cleanup;
> + goto err_media_cleanup;
>
> return 0;
>
> -media_cleanup:
> +err_media_cleanup:
> imx7_csi_media_cleanup(csi);
>
> -destroy_mutex:
> - mutex_destroy(&csi->lock);
> -
> return ret;
> }
>
> @@ -2357,8 +2288,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
> v4l2_async_nf_cleanup(&csi->notifier);
> v4l2_async_unregister_subdev(&csi->sd);
>
> - mutex_destroy(&csi->lock);
> -
> return 0;
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state
2023-01-30 19:26 ` Jacopo Mondi
@ 2023-01-30 20:14 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-30 20:14 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Adam Ford, Martin Kepplinger, Paul Elder,
Rui Miguel Silva, kernel, linux-imx
Hi Jacopo,
On Mon, Jan 30, 2023 at 08:26:10PM +0100, Jacopo Mondi wrote:
> On Sun, Jan 29, 2023 at 04:34:29AM +0200, Laurent Pinchart wrote:
> > Use the V4L2 subdev active state API to store the active format. This
> > simplifies the driver not only by dropping the state stored in the
> > imx7_csi structure, but also by replacing the manual lock with the state
> > lock.
> >
> > The is_streaming field is now protected by the active state lock, either
> > explicitly in .s_stream(), where the active state is locked manually, or
> > implicitly in .set_fmt(), which is called with the state locked.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Drop manual init_cfg call from probe
> > ---
> > drivers/media/platform/nxp/imx7-media-csi.c | 173 ++++++--------------
> > 1 file changed, 51 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c b/drivers/media/platform/nxp/imx7-media-csi.c
> > index 943e541768bd..c22bf5c827e7 100644
> > --- a/drivers/media/platform/nxp/imx7-media-csi.c
> > +++ b/drivers/media/platform/nxp/imx7-media-csi.c
> > @@ -211,7 +211,6 @@ struct imx7_csi {
> > int irq;
> > struct clk *mclk;
> >
> > - struct mutex lock; /* Protects is_streaming, format_mbus, cc */
> > spinlock_t irqlock; /* Protects last_eof */
> >
> > /* Media and V4L2 device */
> > @@ -227,8 +226,6 @@ struct imx7_csi {
> > struct v4l2_subdev sd;
> > struct media_pad pad[IMX7_CSI_PADS_NUM];
> >
> > - struct v4l2_mbus_framefmt format_mbus[IMX7_CSI_PADS_NUM];
> > -
> > /* Video device */
> > struct video_device *vdev; /* Video device */
> > struct media_pad vdev_pad; /* Video device pad */
> > @@ -509,7 +506,8 @@ static void imx7_csi_dma_stop(struct imx7_csi *csi)
> > imx7_csi_hw_disable_irq(csi);
> > }
> >
> > -static void imx7_csi_configure(struct imx7_csi *csi)
> > +static void imx7_csi_configure(struct imx7_csi *csi,
> > + struct v4l2_subdev_state *sd_state)
> > {
> > struct v4l2_pix_format *out_pix = &csi->vdev_fmt;
> > int width = out_pix->width;
> > @@ -540,12 +538,17 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
> > width *= 2;
> > } else {
> > + const struct v4l2_mbus_framefmt *sink_fmt;
> > +
> > + sink_fmt = v4l2_subdev_get_pad_format(&csi->sd, sd_state,
> > + IMX7_CSI_PAD_SINK);
> > +
> > cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC
> > | BIT_MCLKDIV(1) | BIT_MCLKEN;
> >
> > cr18 |= BIT_DATA_FROM_MIPI;
> >
> > - switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) {
> > + switch (sink_fmt->code) {
> > case MEDIA_BUS_FMT_Y8_1X8:
> > case MEDIA_BUS_FMT_SBGGR8_1X8:
> > case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -626,7 +629,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > imx7_csi_reg_write(csi, stride, CSI_CSIFBUF_PARA);
> > }
> >
> > -static int imx7_csi_init(struct imx7_csi *csi)
> > +static int imx7_csi_init(struct imx7_csi *csi,
> > + struct v4l2_subdev_state *sd_state)
> > {
> > int ret;
> >
> > @@ -634,7 +638,7 @@ static int imx7_csi_init(struct imx7_csi *csi)
> > if (ret < 0)
> > return ret;
> >
> > - imx7_csi_configure(csi);
> > + imx7_csi_configure(csi, sd_state);
> >
> > ret = imx7_csi_dma_setup(csi);
> > if (ret < 0) {
> > @@ -1420,7 +1424,7 @@ static int imx7_csi_video_validate_fmt(struct imx7_csi *csi)
> > int ret;
> >
> > /* Retrieve the media bus format on the source subdev. */
> > - ret = v4l2_subdev_call(&csi->sd, pad, get_fmt, NULL, &fmt_src);
> > + ret = v4l2_subdev_call_state_active(&csi->sd, pad, get_fmt, &fmt_src);
> > if (ret)
> > return ret;
> >
> > @@ -1728,12 +1732,13 @@ static int imx7_csi_video_init(struct imx7_csi *csi)
> > static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> > + struct v4l2_subdev_state *sd_state;
> > int ret = 0;
> >
> > - mutex_lock(&csi->lock);
> > + sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> >
> > if (enable) {
> > - ret = imx7_csi_init(csi);
> > + ret = imx7_csi_init(csi, sd_state);
> > if (ret < 0)
> > goto out_unlock;
> >
> > @@ -1755,29 +1760,14 @@ static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
> > csi->is_streaming = !!enable;
> >
> > out_unlock:
> > - mutex_unlock(&csi->lock);
> > + v4l2_subdev_unlock_state(sd_state);
> >
> > return ret;
> > }
> >
> > -static struct v4l2_mbus_framefmt *
> > -imx7_csi_get_format(struct imx7_csi *csi,
> > - struct v4l2_subdev_state *sd_state,
> > - unsigned int pad,
> > - enum v4l2_subdev_format_whence which)
> > -{
> > - if (which == V4L2_SUBDEV_FORMAT_TRY)
> > - return v4l2_subdev_get_try_format(&csi->sd, sd_state, pad);
> > -
> > - return &csi->format_mbus[pad];
> > -}
> > -
> > static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state)
> > {
> > - const enum v4l2_subdev_format_whence which =
> > - sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> > const struct imx7_csi_pixfmt *cc;
> > int i;
> >
> > @@ -1785,7 +1775,7 @@ static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
> >
> > for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
> > struct v4l2_mbus_framefmt *mf =
> > - imx7_csi_get_format(csi, sd_state, i, which);
> > + v4l2_subdev_get_pad_format(sd, sd_state, i);
> >
> > mf->code = IMX7_CSI_DEF_MBUS_CODE;
> > mf->width = IMX7_CSI_DEF_PIX_WIDTH;
> > @@ -1806,59 +1796,30 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> > struct v4l2_mbus_framefmt *in_fmt;
> > int ret = 0;
> >
> > - mutex_lock(&csi->lock);
> > -
> > - in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
> > - code->which);
> > + in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
> >
> > switch (code->pad) {
> > case IMX7_CSI_PAD_SINK:
> > ret = imx7_csi_enum_mbus_formats(&code->code, code->index);
> > break;
> > +
> > case IMX7_CSI_PAD_SRC:
> > if (code->index != 0) {
> > ret = -EINVAL;
> > - goto out_unlock;
> > + break;
> > }
> >
> > code->code = in_fmt->code;
> > break;
> > +
> > default:
> > ret = -EINVAL;
> > + break;
> > }
> >
> > -out_unlock:
> > - mutex_unlock(&csi->lock);
> > -
> > - return ret;
> > -}
> > -
> > -static int imx7_csi_get_fmt(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *sdformat)
> > -{
> > - struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> > - struct v4l2_mbus_framefmt *fmt;
> > - int ret = 0;
> > -
> > - mutex_lock(&csi->lock);
> > -
> > - fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
> > - sdformat->which);
> > - if (!fmt) {
> > - ret = -EINVAL;
> > - goto out_unlock;
> > - }
> > -
> > - sdformat->format = *fmt;
> > -
> > -out_unlock:
> > - mutex_unlock(&csi->lock);
> > -
> > return ret;
> > }
> >
> > @@ -1908,19 +1869,16 @@ static void imx7_csi_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt)
> > tryfmt->ycbcr_enc);
> > }
> >
> > -static int imx7_csi_try_fmt(struct imx7_csi *csi,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *sdformat,
> > - const struct imx7_csi_pixfmt **cc)
> > +static void imx7_csi_try_fmt(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_format *sdformat,
> > + const struct imx7_csi_pixfmt **cc)
> > {
> > const struct imx7_csi_pixfmt *in_cc;
> > struct v4l2_mbus_framefmt *in_fmt;
> > u32 code;
> >
> > - in_fmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SINK,
> > - sdformat->which);
> > - if (!in_fmt)
> > - return -EINVAL;
> > + in_fmt = v4l2_subdev_get_pad_format(sd, sd_state, IMX7_CSI_PAD_SINK);
> >
> > switch (sdformat->pad) {
> > case IMX7_CSI_PAD_SRC:
> > @@ -1952,8 +1910,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> > }
> >
> > imx7_csi_try_colorimetry(&sdformat->format);
> > -
> > - return 0;
> > }
> >
> > static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> > @@ -1966,25 +1922,13 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> > const struct imx7_csi_pixfmt *cc;
> > struct v4l2_mbus_framefmt *fmt;
> > struct v4l2_subdev_format format;
> > - int ret = 0;
> >
> > - mutex_lock(&csi->lock);
> > + if (csi->is_streaming)
> > + return -EBUSY;
>
> Do you still need this or the state is locked by s_stream() and if I'm
> not mistaken the core locks the state before calling the op ?
The lock protects .set_fmt() racing with .s_stream(), but it doesn't
prevent .set_fmt() from being called while the stream is active, after
.s_stream() has returned. I think this should be handled by the subdev
core, but at the moment it isn't.
> > - if (csi->is_streaming) {
> > - ret = -EBUSY;
> > - goto out_unlock;
> > - }
> > + imx7_csi_try_fmt(sd, sd_state, sdformat, &cc);
> >
> > - ret = imx7_csi_try_fmt(csi, sd_state, sdformat, &cc);
> > - if (ret < 0)
> > - goto out_unlock;
> > -
> > - fmt = imx7_csi_get_format(csi, sd_state, sdformat->pad,
> > - sdformat->which);
> > - if (!fmt) {
> > - ret = -EINVAL;
> > - goto out_unlock;
> > - }
> > + fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
> >
> > *fmt = sdformat->format;
> >
> > @@ -1993,19 +1937,14 @@ static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
> > format.pad = IMX7_CSI_PAD_SRC;
> > format.which = sdformat->which;
> > format.format = sdformat->format;
> > - if (imx7_csi_try_fmt(csi, sd_state, &format, &outcc)) {
> > - ret = -EINVAL;
> > - goto out_unlock;
> > - }
> > - outfmt = imx7_csi_get_format(csi, sd_state, IMX7_CSI_PAD_SRC,
> > - sdformat->which);
> > + imx7_csi_try_fmt(sd, sd_state, &format, &outcc);
> > +
> > + outfmt = v4l2_subdev_get_pad_format(sd, sd_state,
> > + IMX7_CSI_PAD_SRC);
> > *outfmt = format.format;
> > }
> >
> > -out_unlock:
> > - mutex_unlock(&csi->lock);
> > -
> > - return ret;
> > + return 0;
> > }
> >
> > static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
> > @@ -2105,7 +2044,7 @@ static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
> > static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
> > .init_cfg = imx7_csi_init_cfg,
> > .enum_mbus_code = imx7_csi_enum_mbus_code,
> > - .get_fmt = imx7_csi_get_fmt,
> > + .get_fmt = v4l2_subdev_get_fmt,
> > .set_fmt = imx7_csi_set_fmt,
> > .link_validate = imx7_csi_pad_link_validate,
> > };
> > @@ -2199,6 +2138,7 @@ static void imx7_csi_media_cleanup(struct imx7_csi *csi)
> > {
> > v4l2_device_unregister(&csi->v4l2_dev);
> > media_device_unregister(&csi->mdev);
> > + v4l2_subdev_cleanup(&csi->sd);
> > media_device_cleanup(&csi->mdev);
> > }
> >
> > @@ -2266,6 +2206,10 @@ static int imx7_csi_media_init(struct imx7_csi *csi)
> > if (ret)
> > goto error;
> >
> > + ret = v4l2_subdev_init_finalize(&csi->sd);
> > + if (ret)
> > + goto error;
> > +
> > ret = v4l2_device_register_subdev(&csi->v4l2_dev, &csi->sd);
> > if (ret)
> > goto error;
> > @@ -2291,27 +2235,22 @@ static int imx7_csi_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, csi);
> >
> > spin_lock_init(&csi->irqlock);
> > - mutex_init(&csi->lock);
> >
> > /* Acquire resources and install interrupt handler. */
> > csi->mclk = devm_clk_get(&pdev->dev, "mclk");
> > if (IS_ERR(csi->mclk)) {
> > ret = PTR_ERR(csi->mclk);
> > dev_err(dev, "Failed to get mclk: %d", ret);
> > - goto destroy_mutex;
> > + return ret;
> > }
> >
> > csi->irq = platform_get_irq(pdev, 0);
> > - if (csi->irq < 0) {
> > - ret = csi->irq;
> > - goto destroy_mutex;
> > - }
> > + if (csi->irq < 0)
> > + return csi->irq;
> >
> > csi->regbase = devm_platform_ioremap_resource(pdev, 0);
> > - if (IS_ERR(csi->regbase)) {
> > - ret = PTR_ERR(csi->regbase);
> > - goto destroy_mutex;
> > - }
> > + if (IS_ERR(csi->regbase))
> > + return PTR_ERR(csi->regbase);
> >
> > csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev);
> >
> > @@ -2319,31 +2258,23 @@ static int imx7_csi_probe(struct platform_device *pdev)
> > (void *)csi);
> > if (ret < 0) {
> > dev_err(dev, "Request CSI IRQ failed.\n");
> > - goto destroy_mutex;
> > + return ret;
> > }
> >
> > /* Initialize all the media device infrastructure. */
> > ret = imx7_csi_media_init(csi);
> > if (ret)
> > - goto destroy_mutex;
> > -
> > - /* Set the default mbus formats. */
> > - ret = imx7_csi_init_cfg(&csi->sd, NULL);
> > - if (ret)
> > - goto media_cleanup;
> > + return ret;
> >
> > ret = imx7_csi_async_register(csi);
> > if (ret)
> > - goto media_cleanup;
> > + goto err_media_cleanup;
> >
> > return 0;
> >
> > -media_cleanup:
> > +err_media_cleanup:
> > imx7_csi_media_cleanup(csi);
> >
> > -destroy_mutex:
> > - mutex_destroy(&csi->lock);
> > -
> > return ret;
> > }
> >
> > @@ -2357,8 +2288,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
> > v4l2_async_nf_cleanup(&csi->notifier);
> > v4l2_async_unregister_subdev(&csi->sd);
> >
> > - mutex_destroy(&csi->lock);
> > -
> > return 0;
> > }
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (7 preceding siblings ...)
2023-01-29 2:34 ` [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state Laurent Pinchart
@ 2023-01-30 23:45 ` Adam Ford
2023-01-31 9:30 ` Rui Miguel Silva
` (2 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Adam Ford @ 2023-01-30 23:45 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Martin Kepplinger, Paul Elder, Rui Miguel Silva,
kernel, linux-imx
On Sat, Jan 28, 2023 at 8:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This small series moves the imx7-mipi-csi driver to use the subdev
> active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
> the main change in 8/8.
>
> Compared to v1, I've now successfully tested the series on an i.MX8MM.
> The issues reported by Adam have been resolved by adding patch 7/8 and
> fixing a problem in 8/8.
>
> Laurent Pinchart (8):
> media: imx: imx7-media-csi: Drop imx7_csi.cc field
> media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
> media: imx: imx7-media-csi: Drop unneeded check when starting
> streaming
> media: imx: imx7-media-csi: Drop unneeded src_sd check
> media: imx: imx7-media-csi: Drop unneeded pad checks
> media: imx: imx7-media-csi: Cleanup errors in
> imx7_csi_async_register()
> media: imx: imx7-media-csi: Zero format struct before calling
> .get_fmt()
> media: imx: imx7-media-csi: Use V4L2 subdev active state
>
Thanks for the V2! It works nicely along with the CSIS updates you
posted before.
For the series,
Tested-by: Adam Ford <aford173@gmail.com> #imx8mm-beacon-kit
> drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++--------------
> 1 file changed, 70 insertions(+), 165 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (8 preceding siblings ...)
2023-01-30 23:45 ` [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to " Adam Ford
@ 2023-01-31 9:30 ` Rui Miguel Silva
2023-01-31 10:17 ` Martin Kepplinger
2023-01-31 11:18 ` Martin Kepplinger
11 siblings, 0 replies; 16+ messages in thread
From: Rui Miguel Silva @ 2023-01-31 9:30 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Adam Ford, Martin Kepplinger, Paul Elder, kernel, linux-imx
Hi Laurent,
Also thanks for the work on updating this one.
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> Hello,
>
> This small series moves the imx7-mipi-csi driver to use the subdev
> active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
> the main change in 8/8.
>
> Compared to v1, I've now successfully tested the series on an i.MX8MM.
> The issues reported by Adam have been resolved by adding patch 7/8 and
> fixing a problem in 8/8.
>
> Laurent Pinchart (8):
> media: imx: imx7-media-csi: Drop imx7_csi.cc field
> media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
> media: imx: imx7-media-csi: Drop unneeded check when starting
> streaming
> media: imx: imx7-media-csi: Drop unneeded src_sd check
> media: imx: imx7-media-csi: Drop unneeded pad checks
> media: imx: imx7-media-csi: Cleanup errors in
> imx7_csi_async_register()
> media: imx: imx7-media-csi: Zero format struct before calling
> .get_fmt()
> media: imx: imx7-media-csi: Use V4L2 subdev active state
LGTM, for the all series.
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Cheers,
Rui
>
> drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++--------------
> 1 file changed, 70 insertions(+), 165 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (9 preceding siblings ...)
2023-01-31 9:30 ` Rui Miguel Silva
@ 2023-01-31 10:17 ` Martin Kepplinger
2023-01-31 10:24 ` Laurent Pinchart
2023-01-31 11:18 ` Martin Kepplinger
11 siblings, 1 reply; 16+ messages in thread
From: Martin Kepplinger @ 2023-01-31 10:17 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Adam Ford, Paul Elder, Rui Miguel Silva, kernel, linux-imx
Am Sonntag, dem 29.01.2023 um 04:34 +0200 schrieb Laurent Pinchart:
> Hello,
>
> This small series moves the imx7-mipi-csi driver to use the subdev
> active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
> the main change in 8/8.
>
> Compared to v1, I've now successfully tested the series on an
> i.MX8MM.
> The issues reported by Adam have been resolved by adding patch 7/8
> and
> fixing a problem in 8/8.
>
> Laurent Pinchart (8):
> media: imx: imx7-media-csi: Drop imx7_csi.cc field
> media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
> media: imx: imx7-media-csi: Drop unneeded check when starting
> streaming
> media: imx: imx7-media-csi: Drop unneeded src_sd check
> media: imx: imx7-media-csi: Drop unneeded pad checks
> media: imx: imx7-media-csi: Cleanup errors in
> imx7_csi_async_register()
> media: imx: imx7-media-csi: Zero format struct before calling
> .get_fmt()
> media: imx: imx7-media-csi: Use V4L2 subdev active state
>
> drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++------------
> --
> 1 file changed, 70 insertions(+), 165 deletions(-)
>
hi Laurent, what tree did you base this on? onto v6.2-rc6 I have a
change missing: in imx7_csi_video_validate_fmt(), before calling
v4l2_subdev_call_state_active(), there's fmt_src.pad and fmt_src.which
assigned. I don't have that.
thanks
martin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
2023-01-31 10:17 ` Martin Kepplinger
@ 2023-01-31 10:24 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2023-01-31 10:24 UTC (permalink / raw)
To: Martin Kepplinger
Cc: linux-media, Adam Ford, Paul Elder, Rui Miguel Silva, kernel, linux-imx
Hi Martin,
On Tue, Jan 31, 2023 at 11:17:46AM +0100, Martin Kepplinger wrote:
> Am Sonntag, dem 29.01.2023 um 04:34 +0200 schrieb Laurent Pinchart:
> > Hello,
> >
> > This small series moves the imx7-mipi-csi driver to use the subdev
> > active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
> > the main change in 8/8.
> >
> > Compared to v1, I've now successfully tested the series on an
> > i.MX8MM.
> > The issues reported by Adam have been resolved by adding patch 7/8
> > and
> > fixing a problem in 8/8.
> >
> > Laurent Pinchart (8):
> > media: imx: imx7-media-csi: Drop imx7_csi.cc field
> > media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
> > media: imx: imx7-media-csi: Drop unneeded check when starting
> > streaming
> > media: imx: imx7-media-csi: Drop unneeded src_sd check
> > media: imx: imx7-media-csi: Drop unneeded pad checks
> > media: imx: imx7-media-csi: Cleanup errors in
> > imx7_csi_async_register()
> > media: imx: imx7-media-csi: Zero format struct before calling
> > .get_fmt()
> > media: imx: imx7-media-csi: Use V4L2 subdev active state
> >
> > drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++------------
> > --
> > 1 file changed, 70 insertions(+), 165 deletions(-)
> >
>
> hi Laurent, what tree did you base this on? onto v6.2-rc6 I have a
> change missing: in imx7_csi_video_validate_fmt(), before calling
> v4l2_subdev_call_state_active(), there's fmt_src.pad and fmt_src.which
> assigned. I don't have that.
The patches are based on top of the master branch fo
git://linuxtv.org/media_stage.git. I've pushed my branches to
git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git (named
next/media/imx-csis and next/media/imx-csi).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
` (10 preceding siblings ...)
2023-01-31 10:17 ` Martin Kepplinger
@ 2023-01-31 11:18 ` Martin Kepplinger
11 siblings, 0 replies; 16+ messages in thread
From: Martin Kepplinger @ 2023-01-31 11:18 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Adam Ford, Paul Elder, Rui Miguel Silva, kernel, linux-imx
Am Sonntag, dem 29.01.2023 um 04:34 +0200 schrieb Laurent Pinchart:
> Hello,
>
> This small series moves the imx7-mipi-csi driver to use the subdev
> active state. Patches 1/8 to 7/8 are small preparatory cleanups, with
> the main change in 8/8.
>
> Compared to v1, I've now successfully tested the series on an
> i.MX8MM.
> The issues reported by Adam have been resolved by adding patch 7/8
> and
> fixing a problem in 8/8.
>
> Laurent Pinchart (8):
> media: imx: imx7-media-csi: Drop imx7_csi.cc field
> media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format()
> media: imx: imx7-media-csi: Drop unneeded check when starting
> streaming
> media: imx: imx7-media-csi: Drop unneeded src_sd check
> media: imx: imx7-media-csi: Drop unneeded pad checks
> media: imx: imx7-media-csi: Cleanup errors in
> imx7_csi_async_register()
> media: imx: imx7-media-csi: Zero format struct before calling
> .get_fmt()
> media: imx: imx7-media-csi: Use V4L2 subdev active state
>
> drivers/media/platform/nxp/imx7-media-csi.c | 235 ++++++------------
> --
> 1 file changed, 70 insertions(+), 165 deletions(-)
>
thanks Laurent,
Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>
martin
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-01-31 11:18 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-29 2:34 [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to subdev active state Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 1/8] media: imx: imx7-media-csi: Drop imx7_csi.cc field Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 2/8] media: imx: imx7-media-csi: Simplify imx7_csi_video_init_format() Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 3/8] media: imx: imx7-media-csi: Drop unneeded check when starting streaming Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 4/8] media: imx: imx7-media-csi: Drop unneeded src_sd check Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 5/8] media: imx: imx7-media-csi: Drop unneeded pad checks Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 6/8] media: imx: imx7-media-csi: Cleanup errors in imx7_csi_async_register() Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 7/8] media: imx: imx7-media-csi: Zero format struct before calling .get_fmt() Laurent Pinchart
2023-01-29 2:34 ` [PATCH v2 8/8] media: imx: imx7-media-csi: Use V4L2 subdev active state Laurent Pinchart
2023-01-30 19:26 ` Jacopo Mondi
2023-01-30 20:14 ` Laurent Pinchart
2023-01-30 23:45 ` [PATCH v2 0/8] media: nxp: imx7-media-csi: Move to " Adam Ford
2023-01-31 9:30 ` Rui Miguel Silva
2023-01-31 10:17 ` Martin Kepplinger
2023-01-31 10:24 ` Laurent Pinchart
2023-01-31 11:18 ` Martin Kepplinger
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).