[v2] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state
Commit Message
Use the V4L2 subdev active state API to store the active format.
This simplifies the driver not only by dropping the bridge mbus_format
field, but it also allows dropping the bridge lock, replaced with
the state lock.
The sun8i-a83t-mipi-csi2 hardware does not perform any format
conversion. Enforce identical formats on the sink and source pads in
the set_fmt() and init_state() callbacks.
Signed-off-by: Arash Golgol <arash.golgol@gmail.com>
---
Changes in v2:
- Initialize active state before calling v4l2_subdev_state_get_format()
- Fix line wrapping reported by checkpatch
- Link to media-ci report: https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/99865145/artifacts/report.htm
.../sun8i_a83t_mipi_csi2.c | 111 +++++++++---------
.../sun8i_a83t_mipi_csi2.h | 2 -
2 files changed, 55 insertions(+), 58 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The active state lock is leaked in the error path of `pm_runtime_resume_and_get()` within `sun8i_a83t_mipi_csi2_s_stream()`.
--
commit 1aff86a32a5202665d5639183ff47ad585d7b58e
Author: Arash Golgol <arash.golgol@gmail.com>
Subject: media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state
This commit updates the sun8i-a83t-mipi-csi2 driver to use the V4L2 subdev
active state API. This simplifies format storage and replaces the bridge lock
with the state lock.
> diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> index dbc51daa4fe37..743bc86bfeabe 100644
> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
[ ... ]
> @@ -215,8 +216,12 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> unsigned long pixel_rate;
> int ret;
>
> - if (!source_subdev)
> - return -ENODEV;
> + state = v4l2_subdev_lock_and_get_active_state(subdev);
> +
> + if (!source_subdev) {
> + ret = -ENODEV;
> + goto unlock;
> + }
>
> if (!on) {
> v4l2_subdev_call(source_subdev, video, s_stream, 0);
Does this code leak the active state lock if the subsequent runtime PM resume
fails?
Looking at the unmodified code that immediately follows this hunk in
sun8i_a83t_mipi_csi2_s_stream():
/* Runtime PM */
ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
return ret;
Since the state lock is now acquired at the beginning of the function,
if pm_runtime_resume_and_get() returns a negative value, it directly returns
to the caller.
Because it bypasses the "goto unlock" path, the state lock remains acquired,
which could cause a deadlock on any subsequent attempt to access the active
state or start the stream.
Should this error path be updated to "goto unlock" so the lock is properly
released?
@@ -144,12 +144,12 @@ sun8i_a83t_mipi_csi2_disable(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
}
static void
-sun8i_a83t_mipi_csi2_configure(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
+sun8i_a83t_mipi_csi2_configure(struct sun8i_a83t_mipi_csi2_device *csi2_dev,
+ const struct v4l2_mbus_framefmt *mbus_format)
{
struct regmap *regmap = csi2_dev->regmap;
unsigned int lanes_count =
csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes;
- struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
const struct sun8i_a83t_mipi_csi2_format *format;
struct device *dev = csi2_dev->dev;
u32 version = 0;
@@ -205,7 +205,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev;
union phy_configure_opts dphy_opts = { 0 };
struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
- struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
+ struct v4l2_subdev_state *state;
+ const struct v4l2_mbus_framefmt *mbus_format;
const struct sun8i_a83t_mipi_csi2_format *format;
struct phy *dphy = csi2_dev->dphy;
struct device *dev = csi2_dev->dev;
@@ -215,8 +216,12 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
unsigned long pixel_rate;
int ret;
- if (!source_subdev)
- return -ENODEV;
+ state = v4l2_subdev_lock_and_get_active_state(subdev);
+
+ if (!source_subdev) {
+ ret = -ENODEV;
+ goto unlock;
+ }
if (!on) {
v4l2_subdev_call(source_subdev, video, s_stream, 0);
@@ -254,6 +259,9 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
goto error_pm;
}
+ mbus_format =
+ v4l2_subdev_state_get_format(state,
+ SUN8I_A83T_MIPI_CSI2_PAD_SINK);
format = sun8i_a83t_mipi_csi2_format_find(mbus_format->code);
if (WARN_ON(!format)) {
ret = -ENODEV;
@@ -292,7 +300,7 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
/* Controller */
- sun8i_a83t_mipi_csi2_configure(csi2_dev);
+ sun8i_a83t_mipi_csi2_configure(csi2_dev, mbus_format);
sun8i_a83t_mipi_csi2_enable(csi2_dev);
/* D-PHY */
@@ -309,7 +317,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
if (ret && ret != -ENOIOCTLCMD)
goto disable;
- return 0;
+ ret = 0;
+ goto unlock;
disable:
phy_power_off(dphy);
@@ -318,6 +327,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
error_pm:
pm_runtime_put(dev);
+unlock:
+ v4l2_subdev_unlock_state(state);
return ret;
}
@@ -341,22 +352,24 @@ sun8i_a83t_mipi_csi2_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
static int sun8i_a83t_mipi_csi2_init_state(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *state)
{
- struct sun8i_a83t_mipi_csi2_device *csi2_dev =
- v4l2_get_subdevdata(subdev);
- unsigned int pad = SUN8I_A83T_MIPI_CSI2_PAD_SINK;
- struct v4l2_mbus_framefmt *mbus_format =
- v4l2_subdev_state_get_format(state, pad);
- struct mutex *lock = &csi2_dev->bridge.lock;
+ unsigned int pad;
+
+ /*
+ * This subdev does not perform format conversion,
+ * initialize both pads identically.
+ */
+ for (pad = 0; pad < subdev->entity.num_pads; pad++) {
+ struct v4l2_mbus_framefmt *mbus_format;
- mutex_lock(lock);
+ mbus_format = v4l2_subdev_state_get_format(state, pad);
- mbus_format->code = sun8i_a83t_mipi_csi2_formats[0].mbus_code;
- mbus_format->width = 640;
- mbus_format->height = 480;
+ mbus_format->code = sun8i_a83t_mipi_csi2_formats[0].mbus_code;
+ mbus_format->width = 640;
+ mbus_format->height = 480;
- sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
+ sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
+ }
- mutex_unlock(lock);
return 0;
}
@@ -375,55 +388,33 @@ sun8i_a83t_mipi_csi2_enum_mbus_code(struct v4l2_subdev *subdev,
return 0;
}
-static int sun8i_a83t_mipi_csi2_get_fmt(struct v4l2_subdev *subdev,
- struct v4l2_subdev_state *state,
- struct v4l2_subdev_format *format)
-{
- struct sun8i_a83t_mipi_csi2_device *csi2_dev =
- v4l2_get_subdevdata(subdev);
- struct v4l2_mbus_framefmt *mbus_format = &format->format;
- struct mutex *lock = &csi2_dev->bridge.lock;
-
- mutex_lock(lock);
-
- if (format->which == V4L2_SUBDEV_FORMAT_TRY)
- *mbus_format = *v4l2_subdev_state_get_format(state,
- format->pad);
- else
- *mbus_format = csi2_dev->bridge.mbus_format;
-
- mutex_unlock(lock);
-
- return 0;
-}
-
static int sun8i_a83t_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
struct v4l2_subdev_state *state,
struct v4l2_subdev_format *format)
{
- struct sun8i_a83t_mipi_csi2_device *csi2_dev =
- v4l2_get_subdevdata(subdev);
- struct v4l2_mbus_framefmt *mbus_format = &format->format;
- struct mutex *lock = &csi2_dev->bridge.lock;
+ struct v4l2_mbus_framefmt *fmt;
- mutex_lock(lock);
+ /* The format on the source pad always matches the sink pad. */
+ if (format->pad != SUN8I_A83T_MIPI_CSI2_PAD_SINK)
+ return v4l2_subdev_get_fmt(subdev, state, format);
- sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
+ sun8i_a83t_mipi_csi2_mbus_format_prepare(&format->format);
- if (format->which == V4L2_SUBDEV_FORMAT_TRY)
- *v4l2_subdev_state_get_format(state, format->pad) =
- *mbus_format;
- else
- csi2_dev->bridge.mbus_format = *mbus_format;
+ /* Set the format on the sink pad. */
+ fmt = v4l2_subdev_state_get_format(state, format->pad);
+ *fmt = format->format;
- mutex_unlock(lock);
+ /* Propagate the format to the source pad. */
+ fmt = v4l2_subdev_state_get_format(state,
+ SUN8I_A83T_MIPI_CSI2_PAD_SOURCE);
+ *fmt = format->format;
return 0;
}
static const struct v4l2_subdev_pad_ops sun8i_a83t_mipi_csi2_pad_ops = {
.enum_mbus_code = sun8i_a83t_mipi_csi2_enum_mbus_code,
- .get_fmt = sun8i_a83t_mipi_csi2_get_fmt,
+ .get_fmt = v4l2_subdev_get_fmt,
.set_fmt = sun8i_a83t_mipi_csi2_set_fmt,
};
@@ -540,8 +531,6 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
bool notifier_registered = false;
int ret;
- mutex_init(&bridge->lock);
-
/* V4L2 Subdev */
v4l2_subdev_init(subdev, &sun8i_a83t_mipi_csi2_subdev_ops);
@@ -570,6 +559,12 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
if (ret)
return ret;
+ /* V4L2 Subdev finalize */
+
+ ret = v4l2_subdev_init_finalize(subdev);
+ if (ret < 0)
+ goto error_media_entity_cleanup;
+
/* V4L2 Async */
v4l2_async_subdev_nf_init(notifier, subdev);
@@ -603,6 +598,9 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
error_v4l2_notifier_cleanup:
v4l2_async_nf_cleanup(notifier);
+ v4l2_subdev_cleanup(subdev);
+
+error_media_entity_cleanup:
media_entity_cleanup(&subdev->entity);
return ret;
@@ -617,6 +615,7 @@ sun8i_a83t_mipi_csi2_bridge_cleanup(struct sun8i_a83t_mipi_csi2_device *csi2_dev
v4l2_async_unregister_subdev(subdev);
v4l2_async_nf_unregister(notifier);
v4l2_async_nf_cleanup(notifier);
+ v4l2_subdev_cleanup(subdev);
media_entity_cleanup(&subdev->entity);
}
@@ -33,8 +33,6 @@ struct sun8i_a83t_mipi_csi2_bridge {
struct media_pad pads[SUN8I_A83T_MIPI_CSI2_PAD_COUNT];
struct v4l2_fwnode_endpoint endpoint;
struct v4l2_async_notifier notifier;
- struct v4l2_mbus_framefmt mbus_format;
- struct mutex lock; /* Mbus format lock. */
struct v4l2_subdev *source_subdev;
};