* [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state
@ 2023-01-26 21:34 Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix Laurent Pinchart
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
Hello,
This small series moves the imx-mipi-csis driver to use the subdev
active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
the main change in 4/5. Patch 5/5 is further cleanup that could have
been included in 4/5, but that should be easier to review as a separate
patch.
The series has been tested on the i.MX8MP with the ISI, and IMX296 and
IMX327 camera sensors.
Laurent Pinchart (5):
media: imx-mipi-csis: Rename error labels with 'err_' prefix
media: imx-mipi-csis: Don't take lock in runtime PM handlers
media: imx-mipi-csis: Pass format explicitly to internal functions
media: imx-mipi-csis: Use V4L2 subdev active state
media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
1 file changed, 103 insertions(+), 146 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
@ 2023-01-26 21:34 ` Laurent Pinchart
2023-01-27 3:34 ` Adam Ford
2023-01-26 21:34 ` [PATCH v1 2/5] media: imx-mipi-csis: Don't take lock in runtime PM handlers Laurent Pinchart
` (5 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
It is customary to prefix error labels with 'err_' to make their purpose
clearer. Do so in the probe function.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 905072871ed2..d949b2de8e74 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
dev_name(dev), csis);
if (ret) {
dev_err(dev, "Interrupt request failed\n");
- goto disable_clock;
+ goto err_disable_clock;
}
/* Initialize and register the subdev. */
ret = mipi_csis_subdev_init(csis);
if (ret < 0)
- goto disable_clock;
+ goto err_disable_clock;
platform_set_drvdata(pdev, &csis->sd);
ret = mipi_csis_async_register(csis);
if (ret < 0) {
dev_err(dev, "async register failed: %d\n", ret);
- goto cleanup;
+ goto err_cleanup;
}
/* Initialize debugfs. */
@@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
if (!pm_runtime_enabled(dev)) {
ret = mipi_csis_runtime_resume(dev);
if (ret < 0)
- goto unregister_all;
+ goto err_unregister_all;
}
dev_info(dev, "lanes: %d, freq: %u\n",
@@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
return 0;
-unregister_all:
+err_unregister_all:
mipi_csis_debugfs_exit(csis);
-cleanup:
+err_cleanup:
media_entity_cleanup(&csis->sd.entity);
v4l2_async_nf_unregister(&csis->notifier);
v4l2_async_nf_cleanup(&csis->notifier);
v4l2_async_unregister_subdev(&csis->sd);
-disable_clock:
+err_disable_clock:
mipi_csis_clk_disable(csis);
fwnode_handle_put(csis->sd.fwnode);
mutex_destroy(&csis->lock);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 2/5] media: imx-mipi-csis: Don't take lock in runtime PM handlers
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix Laurent Pinchart
@ 2023-01-26 21:34 ` Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 3/5] media: imx-mipi-csis: Pass format explicitly to internal functions Laurent Pinchart
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
The runtime PM handlers don't need manual locking as
- they are serialized by the runtime PM core
- they can't race with other functions taking the same lock, as they
don't access any data protect by that lock
Drop the locking.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx-mipi-csis.c | 28 +++++++++-------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index d949b2de8e74..4e1363ff5646 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1348,40 +1348,34 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
- int ret = 0;
-
- mutex_lock(&csis->lock);
+ int ret;
ret = mipi_csis_phy_disable(csis);
if (ret)
- goto unlock;
+ return -EAGAIN;
mipi_csis_clk_disable(csis);
-unlock:
- mutex_unlock(&csis->lock);
-
- return ret ? -EAGAIN : 0;
+ return 0;
}
static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
- int ret = 0;
-
- mutex_lock(&csis->lock);
+ int ret;
ret = mipi_csis_phy_enable(csis);
if (ret)
- goto unlock;
+ return -EAGAIN;
- mipi_csis_clk_enable(csis);
+ ret = mipi_csis_clk_enable(csis);
+ if (ret) {
+ mipi_csis_phy_disable(csis);
+ return ret;
+ }
-unlock:
- mutex_unlock(&csis->lock);
-
- return ret ? -EAGAIN : 0;
+ return 0;
}
static const struct dev_pm_ops mipi_csis_pm_ops = {
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 3/5] media: imx-mipi-csis: Pass format explicitly to internal functions
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 2/5] media: imx-mipi-csis: Don't take lock in runtime PM handlers Laurent Pinchart
@ 2023-01-26 21:34 ` Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 4/5] media: imx-mipi-csis: Use V4L2 subdev active state Laurent Pinchart
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
To prepare for usage of the subdev active state that will replace the
csis_fmt and format_mbus fields stored in the mipi_csis_device
structure, pass the format explicitly to the functions called when
starting streaming to avoid accessing those two fields. Not functional
change intended.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++---------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 4e1363ff5646..38ed88646632 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -560,9 +560,10 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
}
/* Called with the csis.lock mutex held */
-static void __mipi_csis_set_format(struct mipi_csis_device *csis)
+static void __mipi_csis_set_format(struct mipi_csis_device *csis,
+ const struct v4l2_mbus_framefmt *format,
+ const struct csis_pix_format *csis_fmt)
{
- struct v4l2_mbus_framefmt *mf = &csis->format_mbus[CSIS_PAD_SINK];
u32 val;
/* Color format */
@@ -583,25 +584,26 @@ static void __mipi_csis_set_format(struct mipi_csis_device *csis)
*
* TODO: Verify which other formats require DUAL (or QUAD) modes.
*/
- if (csis->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
+ if (csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8)
val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
- val |= MIPI_CSIS_ISPCFG_FMT(csis->csis_fmt->data_type);
+ val |= MIPI_CSIS_ISPCFG_FMT(csis_fmt->data_type);
mipi_csis_write(csis, MIPI_CSIS_ISP_CONFIG_CH(0), val);
/* Pixel resolution */
- val = mf->width | (mf->height << 16);
+ val = format->width | (format->height << 16);
mipi_csis_write(csis, MIPI_CSIS_ISP_RESOL_CH(0), val);
}
-static int mipi_csis_calculate_params(struct mipi_csis_device *csis)
+static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
+ const struct csis_pix_format *csis_fmt)
{
s64 link_freq;
u32 lane_rate;
/* Calculate the line rate from the pixel rate. */
link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
- csis->csis_fmt->width,
+ csis_fmt->width,
csis->bus.num_data_lanes * 2);
if (link_freq < 0) {
dev_err(csis->dev, "Unable to obtain link frequency: %d\n",
@@ -643,7 +645,9 @@ static int mipi_csis_calculate_params(struct mipi_csis_device *csis)
return 0;
}
-static void mipi_csis_set_params(struct mipi_csis_device *csis)
+static void mipi_csis_set_params(struct mipi_csis_device *csis,
+ const struct v4l2_mbus_framefmt *format,
+ const struct csis_pix_format *csis_fmt)
{
int lanes = csis->bus.num_data_lanes;
u32 val;
@@ -655,7 +659,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis)
val |= MIPI_CSIS_CMN_CTRL_INTER_MODE;
mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL, val);
- __mipi_csis_set_format(csis);
+ __mipi_csis_set_format(csis, format, csis_fmt);
mipi_csis_write(csis, MIPI_CSIS_DPHY_CMN_CTRL,
MIPI_CSIS_DPHY_CMN_CTRL_HSSETTLE(csis->hs_settle) |
@@ -728,10 +732,12 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
return ret;
}
-static void mipi_csis_start_stream(struct mipi_csis_device *csis)
+static void mipi_csis_start_stream(struct mipi_csis_device *csis,
+ const struct v4l2_mbus_framefmt *format,
+ const struct csis_pix_format *csis_fmt)
{
mipi_csis_sw_reset(csis);
- mipi_csis_set_params(csis);
+ mipi_csis_set_params(csis, format, csis_fmt);
mipi_csis_system_enable(csis, true);
mipi_csis_enable_interrupts(csis, true);
}
@@ -935,6 +941,8 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
{
struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
+ const struct v4l2_mbus_framefmt *format = &csis->format_mbus[CSIS_PAD_SINK];
+ const struct csis_pix_format *csis_fmt = csis->csis_fmt;
int ret;
if (!enable) {
@@ -953,7 +961,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
return 0;
}
- ret = mipi_csis_calculate_params(csis);
+ ret = mipi_csis_calculate_params(csis, csis_fmt);
if (ret < 0)
return ret;
@@ -965,7 +973,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
mutex_lock(&csis->lock);
- mipi_csis_start_stream(csis);
+ mipi_csis_start_stream(csis, format, csis_fmt);
ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
if (ret < 0)
goto error;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 4/5] media: imx-mipi-csis: Use V4L2 subdev active state
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
` (2 preceding siblings ...)
2023-01-26 21:34 ` [PATCH v1 3/5] media: imx-mipi-csis: Pass format explicitly to internal functions Laurent Pinchart
@ 2023-01-26 21:34 ` Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 5/5] media: imx-mipi-csis: Implement .init_cfg() using .set_fmt() Laurent Pinchart
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
Use the V4L2 subdev active state API to store the active format. This
simplifies the driver not only by dropping the mipi_csis_device csis_fmt
and format_mbus fields, but it also allows dropping the device lock,
replaced with the state lock.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx-mipi-csis.c | 135 +++++++--------------
1 file changed, 47 insertions(+), 88 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 38ed88646632..9e424cb1c4b1 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -327,10 +327,6 @@ struct mipi_csis_device {
u32 hs_settle;
u32 clk_settle;
- struct mutex lock; /* Protect csis_fmt and format_mbus */
- const struct csis_pix_format *csis_fmt;
- struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM];
-
spinlock_t slock; /* Protect events */
struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
struct dentry *debugfs_root;
@@ -559,7 +555,6 @@ static void mipi_csis_system_enable(struct mipi_csis_device *csis, int on)
mipi_csis_write(csis, MIPI_CSIS_DPHY_CMN_CTRL, val);
}
-/* Called with the csis.lock mutex held */
static void __mipi_csis_set_format(struct mipi_csis_device *csis,
const struct v4l2_mbus_framefmt *format,
const struct csis_pix_format *csis_fmt)
@@ -941,79 +936,67 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
{
struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
- const struct v4l2_mbus_framefmt *format = &csis->format_mbus[CSIS_PAD_SINK];
- const struct csis_pix_format *csis_fmt = csis->csis_fmt;
+ const struct v4l2_mbus_framefmt *format;
+ const struct csis_pix_format *csis_fmt;
+ struct v4l2_subdev_state *state;
int ret;
if (!enable) {
- mutex_lock(&csis->lock);
-
v4l2_subdev_call(csis->src_sd, video, s_stream, 0);
mipi_csis_stop_stream(csis);
if (csis->debug.enable)
mipi_csis_log_counters(csis, true);
- mutex_unlock(&csis->lock);
-
pm_runtime_put(csis->dev);
return 0;
}
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+
+ format = v4l2_subdev_get_pad_format(sd, state, CSIS_PAD_SINK);
+ csis_fmt = find_csis_format(format->code);
+
ret = mipi_csis_calculate_params(csis, csis_fmt);
if (ret < 0)
- return ret;
+ goto err_unlock;
mipi_csis_clear_counters(csis);
ret = pm_runtime_resume_and_get(csis->dev);
if (ret < 0)
- return ret;
-
- mutex_lock(&csis->lock);
+ goto err_unlock;
mipi_csis_start_stream(csis, format, csis_fmt);
+
ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1);
if (ret < 0)
- goto error;
+ goto err_stop;
mipi_csis_log_counters(csis, true);
- mutex_unlock(&csis->lock);
+ v4l2_subdev_unlock_state(state);
return 0;
-error:
+err_stop:
mipi_csis_stop_stream(csis);
- mutex_unlock(&csis->lock);
pm_runtime_put(csis->dev);
+err_unlock:
+ v4l2_subdev_unlock_state(state);
return ret;
}
-static struct v4l2_mbus_framefmt *
-mipi_csis_get_format(struct mipi_csis_device *csis,
- struct v4l2_subdev_state *sd_state,
- enum v4l2_subdev_format_whence which,
- unsigned int pad)
-{
- if (which == V4L2_SUBDEV_FORMAT_TRY)
- return v4l2_subdev_get_try_format(&csis->sd, sd_state, pad);
-
- return &csis->format_mbus[pad];
-}
-
static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state)
{
- struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
struct v4l2_mbus_framefmt *fmt_sink;
struct v4l2_mbus_framefmt *fmt_source;
- enum v4l2_subdev_format_whence which;
- which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
- fmt_sink = mipi_csis_get_format(csis, sd_state, which, CSIS_PAD_SINK);
+ fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SINK);
+ fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
fmt_sink->code = MEDIA_BUS_FMT_UYVY8_1X16;
fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
@@ -1027,36 +1010,15 @@ static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
fmt_sink->ycbcr_enc);
- fmt_source = mipi_csis_get_format(csis, sd_state, which,
- CSIS_PAD_SOURCE);
*fmt_source = *fmt_sink;
return 0;
}
-static int mipi_csis_get_fmt(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *sdformat)
-{
- struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
- struct v4l2_mbus_framefmt *fmt;
-
- fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
- sdformat->pad);
-
- mutex_lock(&csis->lock);
- sdformat->format = *fmt;
- mutex_unlock(&csis->lock);
-
- return 0;
-}
-
static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
{
- struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
-
/*
* The CSIS can't transcode in any way, the source format is identical
* to the sink format.
@@ -1067,8 +1029,7 @@ static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
if (code->index > 0)
return -EINVAL;
- fmt = mipi_csis_get_format(csis, sd_state, code->which,
- code->pad);
+ fmt = v4l2_subdev_get_pad_format(sd, sd_state, code->pad);
code->code = fmt->code;
return 0;
}
@@ -1088,7 +1049,6 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_format *sdformat)
{
- struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
struct csis_pix_format const *csis_fmt;
struct v4l2_mbus_framefmt *fmt;
unsigned int align;
@@ -1098,7 +1058,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
* modified.
*/
if (sdformat->pad == CSIS_PAD_SOURCE)
- return mipi_csis_get_fmt(sd, sd_state, sdformat);
+ return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
if (sdformat->pad != CSIS_PAD_SINK)
return -EINVAL;
@@ -1136,10 +1096,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
&sdformat->format.height, 1,
CSIS_MAX_PIX_HEIGHT, 0, 0);
- fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
- sdformat->pad);
-
- mutex_lock(&csis->lock);
+ fmt = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
fmt->code = csis_fmt->code;
fmt->width = sdformat->format.width;
@@ -1152,44 +1109,40 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
sdformat->format = *fmt;
/* Propagate the format from sink to source. */
- fmt = mipi_csis_get_format(csis, sd_state, sdformat->which,
- CSIS_PAD_SOURCE);
+ fmt = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
*fmt = sdformat->format;
/* The format on the source pad might change due to unpacking. */
fmt->code = csis_fmt->output;
- /* Store the CSIS format descriptor for active formats. */
- if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
- csis->csis_fmt = csis_fmt;
-
- mutex_unlock(&csis->lock);
-
return 0;
}
static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
struct v4l2_mbus_frame_desc *fd)
{
- struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[0];
+ const struct csis_pix_format *csis_fmt;
+ const struct v4l2_mbus_framefmt *fmt;
+ struct v4l2_subdev_state *state;
if (pad != CSIS_PAD_SOURCE)
return -EINVAL;
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+ fmt = v4l2_subdev_get_pad_format(sd, state, CSIS_PAD_SOURCE);
+ csis_fmt = find_csis_format(fmt->code);
+ v4l2_subdev_unlock_state(state);
+
fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
fd->num_entries = 1;
memset(entry, 0, sizeof(*entry));
- mutex_lock(&csis->lock);
-
entry->flags = 0;
- entry->pixelcode = csis->csis_fmt->code;
+ entry->pixelcode = csis_fmt->code;
entry->bus.csi2.vc = 0;
- entry->bus.csi2.dt = csis->csis_fmt->data_type;
-
- mutex_unlock(&csis->lock);
+ entry->bus.csi2.dt = csis_fmt->data_type;
return 0;
}
@@ -1216,7 +1169,7 @@ static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
.init_cfg = mipi_csis_init_cfg,
.enum_mbus_code = mipi_csis_enum_mbus_code,
- .get_fmt = mipi_csis_get_fmt,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = mipi_csis_set_fmt,
.get_frame_desc = mipi_csis_get_frame_desc,
};
@@ -1398,6 +1351,7 @@ static const struct dev_pm_ops mipi_csis_pm_ops = {
static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
{
struct v4l2_subdev *sd = &csis->sd;
+ int ret;
v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
sd->owner = THIS_MODULE;
@@ -1419,15 +1373,21 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
return -ENOENT;
}
- csis->csis_fmt = &mipi_csis_formats[0];
- mipi_csis_init_cfg(sd, NULL);
-
csis->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
| MEDIA_PAD_FL_MUST_CONNECT;
csis->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
| MEDIA_PAD_FL_MUST_CONNECT;
- return media_entity_pads_init(&sd->entity, CSIS_PADS_NUM,
- csis->pads);
+ ret = media_entity_pads_init(&sd->entity, CSIS_PADS_NUM, csis->pads);
+ if (ret)
+ return ret;
+
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret) {
+ media_entity_cleanup(&sd->entity);
+ return ret;
+ }
+
+ return 0;
}
static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
@@ -1452,7 +1412,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
if (!csis)
return -ENOMEM;
- mutex_init(&csis->lock);
spin_lock_init(&csis->slock);
csis->dev = dev;
@@ -1533,6 +1492,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
err_unregister_all:
mipi_csis_debugfs_exit(csis);
err_cleanup:
+ v4l2_subdev_cleanup(&csis->sd);
media_entity_cleanup(&csis->sd.entity);
v4l2_async_nf_unregister(&csis->notifier);
v4l2_async_nf_cleanup(&csis->notifier);
@@ -1540,7 +1500,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
err_disable_clock:
mipi_csis_clk_disable(csis);
fwnode_handle_put(csis->sd.fwnode);
- mutex_destroy(&csis->lock);
return ret;
}
@@ -1558,9 +1517,9 @@ static int mipi_csis_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
mipi_csis_runtime_suspend(&pdev->dev);
mipi_csis_clk_disable(csis);
+ v4l2_subdev_cleanup(&csis->sd);
media_entity_cleanup(&csis->sd.entity);
fwnode_handle_put(csis->sd.fwnode);
- mutex_destroy(&csis->lock);
pm_runtime_set_suspended(&pdev->dev);
return 0;
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 5/5] media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
` (3 preceding siblings ...)
2023-01-26 21:34 ` [PATCH v1 4/5] media: imx-mipi-csis: Use V4L2 subdev active state Laurent Pinchart
@ 2023-01-26 21:34 ` Laurent Pinchart
2023-01-27 1:54 ` [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Adam Ford
2023-01-31 9:28 ` Rui Miguel Silva
6 siblings, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-26 21:34 UTC (permalink / raw)
To: linux-media; +Cc: Rui Miguel Silva, Adam Ford, kernel, linux-imx
The .set_fmt() handler is responsible for adjusting the requested format
based on the device limitations. Implement .init_cfg() as a wrapper of
.set_fmt(), to ensure that the initial configuration always matches the
rules implemented in .set_fmt(), should they ever change.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx-mipi-csis.c | 48 ++++++++++------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 9e424cb1c4b1..e99633565463 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -989,32 +989,6 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
return ret;
}
-static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state)
-{
- struct v4l2_mbus_framefmt *fmt_sink;
- struct v4l2_mbus_framefmt *fmt_source;
-
- fmt_sink = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SINK);
- fmt_source = v4l2_subdev_get_pad_format(sd, sd_state, CSIS_PAD_SOURCE);
-
- fmt_sink->code = MEDIA_BUS_FMT_UYVY8_1X16;
- fmt_sink->width = MIPI_CSIS_DEF_PIX_WIDTH;
- fmt_sink->height = MIPI_CSIS_DEF_PIX_HEIGHT;
- fmt_sink->field = V4L2_FIELD_NONE;
-
- fmt_sink->colorspace = V4L2_COLORSPACE_SMPTE170M;
- fmt_sink->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt_sink->colorspace);
- fmt_sink->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt_sink->colorspace);
- fmt_sink->quantization =
- V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt_sink->colorspace,
- fmt_sink->ycbcr_enc);
-
- *fmt_source = *fmt_sink;
-
- return 0;
-}
-
static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
@@ -1101,6 +1075,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
fmt->code = csis_fmt->code;
fmt->width = sdformat->format.width;
fmt->height = sdformat->format.height;
+ fmt->field = V4L2_FIELD_NONE;
fmt->colorspace = sdformat->format.colorspace;
fmt->quantization = sdformat->format.quantization;
fmt->xfer_func = sdformat->format.xfer_func;
@@ -1147,6 +1122,27 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
return 0;
}
+static int mipi_csis_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
+{
+ struct v4l2_subdev_format fmt = {
+ .pad = CSIS_PAD_SINK,
+ };
+
+ fmt.format.code = mipi_csis_formats[0].code;
+ fmt.format.width = MIPI_CSIS_DEF_PIX_WIDTH;
+ fmt.format.height = MIPI_CSIS_DEF_PIX_HEIGHT;
+
+ fmt.format.colorspace = V4L2_COLORSPACE_SMPTE170M;
+ fmt.format.xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt.format.colorspace);
+ fmt.format.ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt.format.colorspace);
+ fmt.format.quantization =
+ V4L2_MAP_QUANTIZATION_DEFAULT(false, fmt.format.colorspace,
+ fmt.format.ycbcr_enc);
+
+ return mipi_csis_set_fmt(sd, sd_state, &fmt);
+}
+
static int mipi_csis_log_status(struct v4l2_subdev *sd)
{
struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
` (4 preceding siblings ...)
2023-01-26 21:34 ` [PATCH v1 5/5] media: imx-mipi-csis: Implement .init_cfg() using .set_fmt() Laurent Pinchart
@ 2023-01-27 1:54 ` Adam Ford
2023-01-27 2:29 ` Laurent Pinchart
2023-01-31 9:28 ` Rui Miguel Silva
6 siblings, 1 reply; 12+ messages in thread
From: Adam Ford @ 2023-01-27 1:54 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, Rui Miguel Silva, kernel, linux-imx
On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This small series moves the imx-mipi-csis driver to use the subdev
> active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> the main change in 4/5. Patch 5/5 is further cleanup that could have
> been included in 4/5, but that should be easier to review as a separate
> patch.
>
> The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> IMX327 camera sensors.
I didn't notice any overall change in the CSIS capture on the imx8mn.
I can test the Mini if you want.
For the series:
Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
adam
>
> Laurent Pinchart (5):
> media: imx-mipi-csis: Rename error labels with 'err_' prefix
> media: imx-mipi-csis: Don't take lock in runtime PM handlers
> media: imx-mipi-csis: Pass format explicitly to internal functions
> media: imx-mipi-csis: Use V4L2 subdev active state
> media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
>
> drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> 1 file changed, 103 insertions(+), 146 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state
2023-01-27 1:54 ` [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Adam Ford
@ 2023-01-27 2:29 ` Laurent Pinchart
2023-01-27 3:05 ` Adam Ford
0 siblings, 1 reply; 12+ messages in thread
From: Laurent Pinchart @ 2023-01-27 2:29 UTC (permalink / raw)
To: Adam Ford; +Cc: linux-media, Rui Miguel Silva, kernel, linux-imx
Hi Adam,
On Thu, Jan 26, 2023 at 07:54:10PM -0600, Adam Ford wrote:
> On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart wrote:
> >
> > Hello,
> >
> > This small series moves the imx-mipi-csis driver to use the subdev
> > active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> > the main change in 4/5. Patch 5/5 is further cleanup that could have
> > been included in 4/5, but that should be easier to review as a separate
> > patch.
> >
> > The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> > IMX327 camera sensors.
>
> I didn't notice any overall change in the CSIS capture on the imx8mn.
> I can test the Mini if you want.
That would be great. Would you be able to test it in conjunction with
the similar imx7-media-csi series I've just sent ([1]) ? I expect a
possible lockdep warning if this series is applied with the
corresponding change in imx7-media-csi, but together they should be
fine.
[1] https://lore.kernel.org/linux-media/20230127022715.27234-1-laurent.pinchart@ideasonboard.com
> For the series:
> Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
>
> > Laurent Pinchart (5):
> > media: imx-mipi-csis: Rename error labels with 'err_' prefix
> > media: imx-mipi-csis: Don't take lock in runtime PM handlers
> > media: imx-mipi-csis: Pass format explicitly to internal functions
> > media: imx-mipi-csis: Use V4L2 subdev active state
> > media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
> >
> > drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> > 1 file changed, 103 insertions(+), 146 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state
2023-01-27 2:29 ` Laurent Pinchart
@ 2023-01-27 3:05 ` Adam Ford
0 siblings, 0 replies; 12+ messages in thread
From: Adam Ford @ 2023-01-27 3:05 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, Rui Miguel Silva, kernel, linux-imx
On Thu, Jan 26, 2023 at 8:29 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Adam,
>
> On Thu, Jan 26, 2023 at 07:54:10PM -0600, Adam Ford wrote:
> > On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart wrote:
> > >
> > > Hello,
> > >
> > > This small series moves the imx-mipi-csis driver to use the subdev
> > > active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> > > the main change in 4/5. Patch 5/5 is further cleanup that could have
> > > been included in 4/5, but that should be easier to review as a separate
> > > patch.
> > >
> > > The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> > > IMX327 camera sensors.
> >
> > I didn't notice any overall change in the CSIS capture on the imx8mn.
> > I can test the Mini if you want.
>
> That would be great. Would you be able to test it in conjunction with
> the similar imx7-media-csi series I've just sent ([1]) ? I expect a
> possible lockdep warning if this series is applied with the
> corresponding change in imx7-media-csi, but together they should be
> fine.
I am working on figuring out why my mini doesn't boot, but I've
already taken the CSIS series and the imx7-media-csi series and
applied them to my working branch. I'll report my findings as soon as
I can get it booting.
adam
>
> [1] https://lore.kernel.org/linux-media/20230127022715.27234-1-laurent.pinchart@ideasonboard.com
>
> > For the series:
> > Tested-by: Adam Ford <aford173@gmail.com> #imx8mn-beacon
> >
> > > Laurent Pinchart (5):
> > > media: imx-mipi-csis: Rename error labels with 'err_' prefix
> > > media: imx-mipi-csis: Don't take lock in runtime PM handlers
> > > media: imx-mipi-csis: Pass format explicitly to internal functions
> > > media: imx-mipi-csis: Use V4L2 subdev active state
> > > media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
> > >
> > > drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> > > 1 file changed, 103 insertions(+), 146 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix
2023-01-26 21:34 ` [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix Laurent Pinchart
@ 2023-01-27 3:34 ` Adam Ford
2023-01-27 4:30 ` Adam Ford
0 siblings, 1 reply; 12+ messages in thread
From: Adam Ford @ 2023-01-27 3:34 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, Rui Miguel Silva, kernel, linux-imx
On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> It is customary to prefix error labels with 'err_' to make their purpose
> clearer. Do so in the probe function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
I tested this series on the imx8mm, but I get an error stating the
capture format is invalid.
My media info looks like:
Media device information
------------------------
driver imx7-csi
model imx-media
serial
bus info platform:32e20000.csi
hw revision 0x0
driver version 6.2.0
Device topology
- entity 1: csi (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
<- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
-> "csi capture":0 [ENABLED,IMMUTABLE]
- entity 4: csi capture (1 pad, 1 link)
type Node subtype V4L flags 0
device node name /dev/video0
pad0: Sink
<- "csi":1 [ENABLED,IMMUTABLE]
- entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev1
pad0: Sink
[fmt:UYVY8_1X16/640x480 field:none]
<- "ov5640 1-003c":0 [ENABLED]
pad1: Source
[fmt:UYVY8_1X16/640x480 field:none]
-> "csi":0 [ENABLED,IMMUTABLE]
- entity 15: ov5640 1-003c (1 pad, 1 link)
type V4L2 subdev subtype Sensor flags 0
device node name /dev/v4l-subdev2
pad0: Source
[fmt:UYVY8_1X16/640x480@1/30 field:none colorspace:srgb xfer:srgb
ycbcr:601 quantization:full-range
crop.bounds:(0,0)/2624x1964
crop:(16,14)/2592x1944]
-> "csis-32e30000.mipi-csi":0 [ENABLED]
From what I can see, each node is configured for UYVY8_1X16/640x480
Yet, the following occurs:
gst-launch-1.0 v4l2src ! video/x-raw,format=UYVY,width=640,height=480 ! fakesink
Setting pipeline to PAUSED ...
Pipeline is live and does not[ 335.986728] imx7-csi 32e20000.csi:
capture format not valid
need PREROLL ...
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Failed
to allocate required memory.
Additional debug info:
../git/sys/v4l2/gstv4l2src.c(659): gst_v4l2src_decide_allocation ():
/GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
Buffer pool activation failed
Execution ended after 0:00:00.009848500
Setting pipeline to NULL ...
ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
Internal data stream error.
Additional debug info:
../git/libs/gst/base/gstbasesrc.c(3127): gst_base_src_loop ():
/GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
streaming stopped, reason not-negotiated (-4)
Freeing pipeline ...
root@beacon-imx8mm-kit:~#
I'm going to unroll this series to see if the mini can capture, and
I'll report back my findings.
adam
> drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 905072871ed2..d949b2de8e74 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
> dev_name(dev), csis);
> if (ret) {
> dev_err(dev, "Interrupt request failed\n");
> - goto disable_clock;
> + goto err_disable_clock;
> }
>
> /* Initialize and register the subdev. */
> ret = mipi_csis_subdev_init(csis);
> if (ret < 0)
> - goto disable_clock;
> + goto err_disable_clock;
>
> platform_set_drvdata(pdev, &csis->sd);
>
> ret = mipi_csis_async_register(csis);
> if (ret < 0) {
> dev_err(dev, "async register failed: %d\n", ret);
> - goto cleanup;
> + goto err_cleanup;
> }
>
> /* Initialize debugfs. */
> @@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> if (!pm_runtime_enabled(dev)) {
> ret = mipi_csis_runtime_resume(dev);
> if (ret < 0)
> - goto unregister_all;
> + goto err_unregister_all;
> }
>
> dev_info(dev, "lanes: %d, freq: %u\n",
> @@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
>
> return 0;
>
> -unregister_all:
> +err_unregister_all:
> mipi_csis_debugfs_exit(csis);
> -cleanup:
> +err_cleanup:
> media_entity_cleanup(&csis->sd.entity);
> v4l2_async_nf_unregister(&csis->notifier);
> v4l2_async_nf_cleanup(&csis->notifier);
> v4l2_async_unregister_subdev(&csis->sd);
> -disable_clock:
> +err_disable_clock:
> mipi_csis_clk_disable(csis);
> fwnode_handle_put(csis->sd.fwnode);
> mutex_destroy(&csis->lock);
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix
2023-01-27 3:34 ` Adam Ford
@ 2023-01-27 4:30 ` Adam Ford
0 siblings, 0 replies; 12+ messages in thread
From: Adam Ford @ 2023-01-27 4:30 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, Rui Miguel Silva, kernel, linux-imx
On Thu, Jan 26, 2023 at 9:34 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Thu, Jan 26, 2023 at 3:34 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > It is customary to prefix error labels with 'err_' to make their purpose
> > clearer. Do so in the probe function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
>
> I tested this series on the imx8mm, but I get an error stating the
> capture format is invalid.
>
> My media info looks like:
>
> Media device information
> ------------------------
> driver imx7-csi
> model imx-media
> serial
> bus info platform:32e20000.csi
> hw revision 0x0
> driver version 6.2.0
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev0
> pad0: Sink
> [fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
> quantization:lim-range]
> <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:UYVY8_1X16/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601
> quantization:lim-range]
> -> "csi capture":0 [ENABLED,IMMUTABLE]
>
> - entity 4: csi capture (1 pad, 1 link)
> type Node subtype V4L flags 0
> device node name /dev/video0
> pad0: Sink
> <- "csi":1 [ENABLED,IMMUTABLE]
>
> - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev1
> pad0: Sink
> [fmt:UYVY8_1X16/640x480 field:none]
> <- "ov5640 1-003c":0 [ENABLED]
> pad1: Source
> [fmt:UYVY8_1X16/640x480 field:none]
> -> "csi":0 [ENABLED,IMMUTABLE]
>
> - entity 15: ov5640 1-003c (1 pad, 1 link)
> type V4L2 subdev subtype Sensor flags 0
> device node name /dev/v4l-subdev2
> pad0: Source
> [fmt:UYVY8_1X16/640x480@1/30 field:none colorspace:srgb xfer:srgb
> ycbcr:601 quantization:full-range
> crop.bounds:(0,0)/2624x1964
> crop:(16,14)/2592x1944]
> -> "csis-32e30000.mipi-csi":0 [ENABLED]
>
> From what I can see, each node is configured for UYVY8_1X16/640x480
>
> Yet, the following occurs:
>
> gst-launch-1.0 v4l2src ! video/x-raw,format=UYVY,width=640,height=480 ! fakesink
> Setting pipeline to PAUSED ...
> Pipeline is live and does not[ 335.986728] imx7-csi 32e20000.csi:
> capture format not valid
> need PREROLL ...
> Pipeline is PREROLLED ...
> Setting pipeline to PLAYING ...
> New clock: GstSystemClock
> ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0: Failed
> to allocate required memory.
> Additional debug info:
> ../git/sys/v4l2/gstv4l2src.c(659): gst_v4l2src_decide_allocation ():
> /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> Buffer pool activation failed
> Execution ended after 0:00:00.009848500
> Setting pipeline to NULL ...
> ERROR: from element /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> Internal data stream error.
> Additional debug info:
> ../git/libs/gst/base/gstbasesrc.c(3127): gst_base_src_loop ():
> /GstPipeline:pipeline0/GstV4l2Src:v4l2src0:
> streaming stopped, reason not-negotiated (-4)
> Freeing pipeline ...
> root@beacon-imx8mm-kit:~#
>
>
> I'm going to unroll this series to see if the mini can capture, and
> I'll report back my findings.
I unrolled both the imx7 capture stuff and the CSIS stuff, but I still
get the same error about the capture format not valid. I rolled back
to 6.2-rc5 and it captures, but the colors seem off. I need to get
some sleep, and I'll be traveling Friday-Sunday, but I'll try to git
bisect to see where stuff stopped working on Mini when I get back.
adam
>
> adam
>
>
> > drivers/media/platform/nxp/imx-mipi-csis.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 905072871ed2..d949b2de8e74 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -1496,20 +1496,20 @@ static int mipi_csis_probe(struct platform_device *pdev)
> > dev_name(dev), csis);
> > if (ret) {
> > dev_err(dev, "Interrupt request failed\n");
> > - goto disable_clock;
> > + goto err_disable_clock;
> > }
> >
> > /* Initialize and register the subdev. */
> > ret = mipi_csis_subdev_init(csis);
> > if (ret < 0)
> > - goto disable_clock;
> > + goto err_disable_clock;
> >
> > platform_set_drvdata(pdev, &csis->sd);
> >
> > ret = mipi_csis_async_register(csis);
> > if (ret < 0) {
> > dev_err(dev, "async register failed: %d\n", ret);
> > - goto cleanup;
> > + goto err_cleanup;
> > }
> >
> > /* Initialize debugfs. */
> > @@ -1520,7 +1520,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
> > if (!pm_runtime_enabled(dev)) {
> > ret = mipi_csis_runtime_resume(dev);
> > if (ret < 0)
> > - goto unregister_all;
> > + goto err_unregister_all;
> > }
> >
> > dev_info(dev, "lanes: %d, freq: %u\n",
> > @@ -1528,14 +1528,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
> >
> > return 0;
> >
> > -unregister_all:
> > +err_unregister_all:
> > mipi_csis_debugfs_exit(csis);
> > -cleanup:
> > +err_cleanup:
> > media_entity_cleanup(&csis->sd.entity);
> > v4l2_async_nf_unregister(&csis->notifier);
> > v4l2_async_nf_cleanup(&csis->notifier);
> > v4l2_async_unregister_subdev(&csis->sd);
> > -disable_clock:
> > +err_disable_clock:
> > mipi_csis_clk_disable(csis);
> > fwnode_handle_put(csis->sd.fwnode);
> > mutex_destroy(&csis->lock);
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
` (5 preceding siblings ...)
2023-01-27 1:54 ` [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Adam Ford
@ 2023-01-31 9:28 ` Rui Miguel Silva
6 siblings, 0 replies; 12+ messages in thread
From: Rui Miguel Silva @ 2023-01-31 9:28 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: Adam Ford, kernel, linux-imx
Hey Laurent,
Thanks for all the cleanups and updates.
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> Hello,
>
> This small series moves the imx-mipi-csis driver to use the subdev
> active state. Patches 1/5 to 3/5 are small preparatory cleanups, with
> the main change in 4/5. Patch 5/5 is further cleanup that could have
> been included in 4/5, but that should be easier to review as a separate
> patch.
>
> The series has been tested on the i.MX8MP with the ISI, and IMX296 and
> IMX327 camera sensors.
>
> Laurent Pinchart (5):
> media: imx-mipi-csis: Rename error labels with 'err_' prefix
> media: imx-mipi-csis: Don't take lock in runtime PM handlers
> media: imx-mipi-csis: Pass format explicitly to internal functions
> media: imx-mipi-csis: Use V4L2 subdev active state
> media: imx-mipi-csis: Implement .init_cfg() using .set_fmt()
LGTM, for the all series.
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Cheers,
Rui
>
> drivers/media/platform/nxp/imx-mipi-csis.c | 249 +++++++++------------
> 1 file changed, 103 insertions(+), 146 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-31 9:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-26 21:34 [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 1/5] media: imx-mipi-csis: Rename error labels with 'err_' prefix Laurent Pinchart
2023-01-27 3:34 ` Adam Ford
2023-01-27 4:30 ` Adam Ford
2023-01-26 21:34 ` [PATCH v1 2/5] media: imx-mipi-csis: Don't take lock in runtime PM handlers Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 3/5] media: imx-mipi-csis: Pass format explicitly to internal functions Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 4/5] media: imx-mipi-csis: Use V4L2 subdev active state Laurent Pinchart
2023-01-26 21:34 ` [PATCH v1 5/5] media: imx-mipi-csis: Implement .init_cfg() using .set_fmt() Laurent Pinchart
2023-01-27 1:54 ` [PATCH v1 0/5] media: imx-mipi-csis: Move to subdev active state Adam Ford
2023-01-27 2:29 ` Laurent Pinchart
2023-01-27 3:05 ` Adam Ford
2023-01-31 9:28 ` Rui Miguel Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.