From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Arash Golgol <arash.golgol@gmail.com>
Cc: linux-media@vger.kernel.org, yong.deng@magewell.com,
paulk@sys-base.io, mchehab@kernel.org, wens@kernel.org,
jernej.skrabec@gmail.com, samuel@sholland.org,
linux-sunxi@lists.linux.dev
Subject: Re: [PATCH v1 1/3] media: sun6i-csi: bridge: Use V4L2 subdev active state
Date: Thu, 7 May 2026 16:55:20 +0300 [thread overview]
Message-ID: <20260507135520.GF1938994@killaraus.ideasonboard.com> (raw)
In-Reply-To: <20260217064050.18388-2-arash.golgol@gmail.com>
Hi Arash,
Thank you for the patch.
On Tue, Feb 17, 2026 at 10:10:48AM +0330, Arash Golgol wrote:
> 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.
>
> Previously, capture accessed bridge private state directly. After
> moving to framework-managed state, resolve the format through the
> subdev pad API.
>
> The sun6i-csi-bridge 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>
> ---
> .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 154 ++++++++----------
> .../sunxi/sun6i-csi/sun6i_csi_bridge.h | 9 -
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 27 ++-
> 3 files changed, 85 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> index d006d9dd0170..4406b0f8c839 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> @@ -13,26 +13,6 @@
> #include "sun6i_csi_bridge.h"
> #include "sun6i_csi_reg.h"
>
> -/* Helpers */
> -
> -void sun6i_csi_bridge_dimensions(struct sun6i_csi_device *csi_dev,
> - unsigned int *width, unsigned int *height)
> -{
> - if (width)
> - *width = csi_dev->bridge.mbus_format.width;
> - if (height)
> - *height = csi_dev->bridge.mbus_format.height;
> -}
> -
> -void sun6i_csi_bridge_format(struct sun6i_csi_device *csi_dev,
> - u32 *mbus_code, u32 *field)
> -{
> - if (mbus_code)
> - *mbus_code = csi_dev->bridge.mbus_format.code;
> - if (field)
> - *field = csi_dev->bridge.mbus_format.field;
> -}
> -
> /* Format */
>
> static const struct sun6i_csi_bridge_format sun6i_csi_bridge_formats[] = {
> @@ -226,7 +206,8 @@ static void sun6i_csi_bridge_disable(struct sun6i_csi_device *csi_dev)
> }
>
> static void
> -sun6i_csi_bridge_configure_parallel(struct sun6i_csi_device *csi_dev)
> +sun6i_csi_bridge_configure_parallel(struct sun6i_csi_device *csi_dev,
> + const struct v4l2_mbus_framefmt *mbus_format)
> {
> struct device *dev = csi_dev->dev;
> struct regmap *regmap = csi_dev->regmap;
> @@ -234,11 +215,9 @@ sun6i_csi_bridge_configure_parallel(struct sun6i_csi_device *csi_dev)
> &csi_dev->bridge.source_parallel.endpoint;
> unsigned char bus_width = endpoint->bus.parallel.bus_width;
> unsigned int flags = endpoint->bus.parallel.flags;
> - u32 field;
> + u32 field = mbus_format->field;
> u32 value = SUN6I_CSI_IF_CFG_IF_CSI;
>
> - sun6i_csi_bridge_format(csi_dev, NULL, &field);
> -
> if (field == V4L2_FIELD_INTERLACED ||
> field == V4L2_FIELD_INTERLACED_TB ||
> field == V4L2_FIELD_INTERLACED_BT)
> @@ -317,13 +296,12 @@ sun6i_csi_bridge_configure_parallel(struct sun6i_csi_device *csi_dev)
> }
>
> static void
> -sun6i_csi_bridge_configure_mipi_csi2(struct sun6i_csi_device *csi_dev)
> +sun6i_csi_bridge_configure_mipi_csi2(struct sun6i_csi_device *csi_dev,
> + const struct v4l2_mbus_framefmt *mbus_format)
> {
> struct regmap *regmap = csi_dev->regmap;
> u32 value = SUN6I_CSI_IF_CFG_IF_MIPI;
> - u32 field;
> -
> - sun6i_csi_bridge_format(csi_dev, NULL, &field);
> + u32 field = mbus_format->field;
>
> if (field == V4L2_FIELD_INTERLACED ||
> field == V4L2_FIELD_INTERLACED_TB ||
> @@ -335,19 +313,19 @@ sun6i_csi_bridge_configure_mipi_csi2(struct sun6i_csi_device *csi_dev)
> regmap_write(regmap, SUN6I_CSI_IF_CFG_REG, value);
> }
>
> -static void sun6i_csi_bridge_configure_format(struct sun6i_csi_device *csi_dev)
> +static void sun6i_csi_bridge_configure_format(struct sun6i_csi_device *csi_dev,
> + const struct v4l2_mbus_framefmt *mbus_format)
> {
> struct regmap *regmap = csi_dev->regmap;
> bool capture_streaming = csi_dev->capture.state.streaming;
> const struct sun6i_csi_bridge_format *bridge_format;
> const struct sun6i_csi_capture_format *capture_format;
> - u32 mbus_code, field, pixelformat;
> + u32 pixelformat;
> + u32 field = mbus_format->field;
> u8 input_format, input_yuv_seq, output_format;
> u32 value = 0;
>
> - sun6i_csi_bridge_format(csi_dev, &mbus_code, &field);
> -
> - bridge_format = sun6i_csi_bridge_format_find(mbus_code);
> + bridge_format = sun6i_csi_bridge_format_find(mbus_format->code);
> if (WARN_ON(!bridge_format))
> return;
>
> @@ -391,16 +369,17 @@ static void sun6i_csi_bridge_configure_format(struct sun6i_csi_device *csi_dev)
> }
>
> static void sun6i_csi_bridge_configure(struct sun6i_csi_device *csi_dev,
> - struct sun6i_csi_bridge_source *source)
> + struct sun6i_csi_bridge_source *source,
> + const struct v4l2_mbus_framefmt *mbus_format)
> {
> struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
>
> if (source == &bridge->source_parallel)
> - sun6i_csi_bridge_configure_parallel(csi_dev);
> + sun6i_csi_bridge_configure_parallel(csi_dev, mbus_format);
> else
> - sun6i_csi_bridge_configure_mipi_csi2(csi_dev);
> + sun6i_csi_bridge_configure_mipi_csi2(csi_dev, mbus_format);
>
> - sun6i_csi_bridge_configure_format(csi_dev);
> + sun6i_csi_bridge_configure_format(csi_dev, mbus_format);
> }
>
> /* V4L2 Subdev */
> @@ -415,6 +394,8 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
> struct sun6i_csi_bridge_source *source;
> struct v4l2_subdev *source_subdev;
> struct media_pad *remote_pad;
> + struct v4l2_subdev_state *state;
> + const struct v4l2_mbus_framefmt *mbus_format;
> int ret;
>
> /* Source */
> @@ -433,6 +414,10 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
> else
> source = &bridge->source_mipi_csi2;
>
> + /* Active State */
> +
> + state = v4l2_subdev_lock_and_get_active_state(subdev);
> +
> if (!on) {
> v4l2_subdev_call(source_subdev, video, s_stream, 0);
> ret = 0;
> @@ -443,7 +428,7 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
>
> ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> - return ret;
> + goto unlock;
>
> /* Clear */
>
> @@ -451,7 +436,9 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
>
> /* Configure */
>
> - sun6i_csi_bridge_configure(csi_dev, source);
> + mbus_format = v4l2_subdev_state_get_format(state,
> + SUN6I_CSI_BRIDGE_PAD_SINK);
> + sun6i_csi_bridge_configure(csi_dev, source, mbus_format);
>
> if (capture_streaming)
> sun6i_csi_capture_configure(csi_dev);
> @@ -472,7 +459,8 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
> if (ret && ret != -ENOIOCTLCMD)
> goto disable;
>
> - return 0;
> + ret = 0;
> + goto unlock;
>
> disable:
> if (capture_streaming)
> @@ -482,6 +470,8 @@ static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
>
> pm_runtime_put(dev);
>
> +unlock:
> + v4l2_subdev_unlock_state(state);
> return ret;
> }
>
> @@ -504,21 +494,23 @@ sun6i_csi_bridge_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
> static int sun6i_csi_bridge_init_state(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *state)
> {
> - struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> - unsigned int pad = SUN6I_CSI_BRIDGE_PAD_SINK;
> - struct v4l2_mbus_framefmt *mbus_format =
> - v4l2_subdev_state_get_format(state, pad);
> - struct mutex *lock = &csi_dev->bridge.lock;
> + unsigned int pad;
>
> - mutex_lock(lock);
> + /*
> + * 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;
>
> - mbus_format->code = sun6i_csi_bridge_formats[0].mbus_code;
> - mbus_format->width = 1280;
> - mbus_format->height = 720;
> + mbus_format = v4l2_subdev_state_get_format(state, pad);
>
> - sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> + mbus_format->code = sun6i_csi_bridge_formats[0].mbus_code;
> + mbus_format->width = 1280;
> + mbus_format->height = 720;
>
> - mutex_unlock(lock);
> + sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> + }
>
> return 0;
> }
> @@ -536,53 +528,32 @@ sun6i_csi_bridge_enum_mbus_code(struct v4l2_subdev *subdev,
> return 0;
> }
>
> -static int sun6i_csi_bridge_get_fmt(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_state *state,
> - struct v4l2_subdev_format *format)
> -{
> - struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> - struct v4l2_mbus_framefmt *mbus_format = &format->format;
> - struct mutex *lock = &csi_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 = csi_dev->bridge.mbus_format;
> -
> - mutex_unlock(lock);
> -
> - return 0;
> -}
> -
> static int sun6i_csi_bridge_set_fmt(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format)
> {
> - struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> - struct v4l2_mbus_framefmt *mbus_format = &format->format;
> - struct mutex *lock = &csi_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 != SUN6I_CSI_BRIDGE_PAD_SINK)
> + return v4l2_subdev_get_fmt(subdev, state, format);
>
> - sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> + sun6i_csi_bridge_mbus_format_prepare(&format->format);
>
> - if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> - *v4l2_subdev_state_get_format(state, format->pad) =
> - *mbus_format;
> - else
> - csi_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, SUN6I_CSI_BRIDGE_PAD_SOURCE);
> + *fmt = format->format;
>
> return 0;
> }
>
> static const struct v4l2_subdev_pad_ops sun6i_csi_bridge_pad_ops = {
> .enum_mbus_code = sun6i_csi_bridge_enum_mbus_code,
> - .get_fmt = sun6i_csi_bridge_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = sun6i_csi_bridge_set_fmt,
> };
>
> @@ -780,8 +751,6 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> };
> int ret;
>
> - mutex_init(&bridge->lock);
> -
> /* V4L2 Subdev */
>
> v4l2_subdev_init(subdev, &sun6i_csi_bridge_subdev_ops);
> @@ -809,6 +778,12 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> if (ret < 0)
> return ret;
>
> + /* V4L2 Subdev finalize */
> +
> + ret = v4l2_subdev_init_finalize(subdev);
> + if (ret < 0)
> + goto error_media_entity;
> +
> /* V4L2 Subdev */
>
> if (csi_dev->isp_available)
> @@ -818,7 +793,7 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
>
> if (ret) {
> dev_err(dev, "failed to register v4l2 subdev: %d\n", ret);
> - goto error_media_entity;
> + goto error_subdev_finalize;
> }
>
> /* V4L2 Async */
> @@ -852,6 +827,9 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> else
> v4l2_device_unregister_subdev(subdev);
>
> +error_subdev_finalize:
> + v4l2_subdev_cleanup(subdev);
> +
> error_media_entity:
> media_entity_cleanup(&subdev->entity);
>
> @@ -868,5 +846,7 @@ void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev)
>
> v4l2_device_unregister_subdev(subdev);
>
> + v4l2_subdev_cleanup(subdev);
> +
> media_entity_cleanup(&subdev->entity);
> }
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> index 44653b38f722..a5b0a6f064dd 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> @@ -42,20 +42,11 @@ struct sun6i_csi_bridge {
> struct v4l2_subdev subdev;
> struct v4l2_async_notifier notifier;
> struct media_pad pads[2];
> - struct v4l2_mbus_framefmt mbus_format;
> - struct mutex lock; /* Mbus format lock. */
>
> struct sun6i_csi_bridge_source source_parallel;
> struct sun6i_csi_bridge_source source_mipi_csi2;
> };
>
> -/* Helpers */
> -
> -void sun6i_csi_bridge_dimensions(struct sun6i_csi_device *csi_dev,
> - unsigned int *width, unsigned int *height);
> -void sun6i_csi_bridge_format(struct sun6i_csi_device *csi_dev,
> - u32 *mbus_code, u32 *field);
> -
> /* Format */
>
> const struct sun6i_csi_bridge_format *
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> index 65879f4802c0..a21a146fb02a 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -888,14 +888,19 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> media_entity_to_video_device(link->sink->entity);
> struct sun6i_csi_device *csi_dev = video_get_drvdata(video_dev);
> struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> + struct v4l2_subdev *src_subdev =
> + media_entity_to_v4l2_subdev(link->source->entity);
> const struct sun6i_csi_capture_format *capture_format;
> const struct sun6i_csi_bridge_format *bridge_format;
> unsigned int capture_width, capture_height;
> - unsigned int bridge_width, bridge_height;
> const struct v4l2_format_info *format_info;
> + struct v4l2_subdev_format src_fmt = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = link->source->index
> + };
> u32 pixelformat, capture_field;
> - u32 mbus_code, bridge_field;
> bool match;
> + int ret;
>
> sun6i_csi_capture_dimensions(csi_dev, &capture_width, &capture_height);
>
> @@ -904,19 +909,22 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> if (WARN_ON(!capture_format))
> return -EINVAL;
>
> - sun6i_csi_bridge_dimensions(csi_dev, &bridge_width, &bridge_height);
> + /* Resolve csi bridge format. */
> + ret = v4l2_subdev_call(src_subdev, pad, get_fmt, NULL, &src_fmt);
> + if (ret)
> + return ret;
>
> - sun6i_csi_bridge_format(csi_dev, &mbus_code, &bridge_field);
> - bridge_format = sun6i_csi_bridge_format_find(mbus_code);
> + bridge_format = sun6i_csi_bridge_format_find(src_fmt.format.code);
> if (WARN_ON(!bridge_format))
> return -EINVAL;
>
> /* No cropping/scaling is supported. */
> - if (capture_width != bridge_width || capture_height != bridge_height) {
> + if (capture_width != src_fmt.format.width ||
> + capture_height != src_fmt.format.height) {
if (capture_width != src_fmt.format.width ||
capture_height != src_fmt.format.height) {
With that,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> v4l2_err(v4l2_dev,
> "invalid input/output dimensions: %ux%u/%ux%u\n",
> - bridge_width, bridge_height, capture_width,
> - capture_height);
> + src_fmt.format.width, src_fmt.format.height,
> + capture_width, capture_height);
> return -EINVAL;
> }
>
> @@ -947,7 +955,8 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> /* With raw input mode, we need a 1:1 match between input and output. */
> if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> capture_format->input_format_raw) {
> - match = sun6i_csi_capture_format_match(pixelformat, mbus_code);
> + match = sun6i_csi_capture_format_match(pixelformat,
> + src_fmt.format.code);
> if (!match)
> goto invalid;
> }
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2026-05-07 13:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-17 6:40 [PATCH v1 0/3] media: sun6i-csi: Convert to active state and improve MC support Arash Golgol
2026-02-17 6:40 ` [PATCH v1 1/3] media: sun6i-csi: bridge: Use V4L2 subdev active state Arash Golgol
2026-05-07 11:55 ` Paul Kocialkowski
2026-05-07 13:55 ` Laurent Pinchart [this message]
2026-02-17 6:40 ` [PATCH v1 2/3] media: sun6i-csi: capture: Implement vidioc_enum_framesizes Arash Golgol
2026-05-07 11:56 ` Paul Kocialkowski
2026-05-07 13:55 ` Laurent Pinchart
2026-02-17 6:40 ` [PATCH v1 3/3] media: sun6i-csi: capture: Support MC-centric format enumeration Arash Golgol
2026-05-07 11:58 ` Paul Kocialkowski
2026-05-07 13:57 ` Laurent Pinchart
2026-04-24 13:55 ` [PATCH v1 0/3] media: sun6i-csi: Convert to active state and improve MC support arash golgol
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260507135520.GF1938994@killaraus.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=arash.golgol@gmail.com \
--cc=jernej.skrabec@gmail.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=paulk@sys-base.io \
--cc=samuel@sholland.org \
--cc=wens@kernel.org \
--cc=yong.deng@magewell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox