| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Mar 2022 15:59:50 +0100
From: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
linux-clk@...r.kernel.org, linux-staging@...ts.linux.dev,
Yong Deng <yong.deng@...ewell.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Maxime Ripard <mripard@...nel.org>,
Hans Verkuil <hans.verkuil@...co.com>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Helen Koike <helen.koike@...labora.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with
port management
Hi Sakari,
On Mon 14 Feb 22, 20:12, Sakari Ailus wrote:
> Hi Paul,
>
> On Sat, Feb 05, 2022 at 07:53:53PM +0100, Paul Kocialkowski wrote:
> > Introduce a bridge v4l2 subdev to prepare for separation between the
> > processing part (bridge) and the dma engine, which is required to
> > properly support ths isp workflow later on.
> >
> > Currently the bridge just manages fwnode mapping to media pads,
> > using an async notifier (which was previously in the main code).
> > The s_stream video op just forwards to the connected v4l2 subdev
> > (sensor or MIPI CSI-2 bridge).
> >
> > The video capture device is now registered after the bridge and
> > attaches to it with a media link.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > ---
> > .../media/platform/sunxi/sun6i-csi/Makefile | 2 +-
> > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 156 +-----
> > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 12 +-
> > .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 473 ++++++++++++++++++
> > .../sunxi/sun6i-csi/sun6i_csi_bridge.h | 44 ++
> > .../platform/sunxi/sun6i-csi/sun6i_video.c | 19 +
> > 6 files changed, 571 insertions(+), 135 deletions(-)
> > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> > create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
[...]
> > +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;
> > +
> > + sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > + *v4l2_subdev_get_try_format(subdev, state, format->pad) =
> > + *mbus_format;
> > + else
> > + csi_dev->bridge.mbus_format = *mbus_format;
>
> Note that the driver is responsible for serialising access to its data,
> i.e. you have to acquire the mutex here.
Thanks, will take care of that next time.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops sun6i_csi_bridge_pad_ops = {
> > + .init_cfg = sun6i_csi_bridge_init_cfg,
> > + .enum_mbus_code = sun6i_csi_bridge_enum_mbus_code,
> > + .get_fmt = sun6i_csi_bridge_get_fmt,
> > + .set_fmt = sun6i_csi_bridge_set_fmt,
> > +};
> > +
> > +const struct v4l2_subdev_ops sun6i_csi_bridge_subdev_ops = {
> > + .video = &sun6i_csi_bridge_video_ops,
> > + .pad = &sun6i_csi_bridge_pad_ops,
> > +};
> > +
> > +/* Media Entity */
> > +
> > +static int sun6i_csi_bridge_link_validate(struct media_link *link)
> > +{
> > + struct v4l2_subdev *subdev =
> > + media_entity_to_v4l2_subdev(link->sink->entity);
> > + struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> > + struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > + struct device *dev = csi_dev->dev;
> > + struct v4l2_subdev *source_subdev =
> > + media_entity_to_v4l2_subdev(link->source->entity);
> > + int ret;
> > +
> > + /* Only support one enabled source at a time. */
> > + if (bridge->source) {
> > + dev_err(dev, "multiple sources are connected to the bridge\n");
> > + return -EBUSY;
> > + }
> > +
> > + ret = v4l2_subdev_link_validate(link);
> > + if (ret)
> > + return ret;
> > +
> > + if (source_subdev == bridge->source_parallel.subdev)
> > + bridge->source = &bridge->source_parallel;
> > + else
>
> Useless use of else.
Fair enough.
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct media_entity_operations sun6i_csi_bridge_entity_ops = {
> > + .link_validate = sun6i_csi_bridge_link_validate,
> > +};
> > +
> > +/* V4L2 Async */
> > +
> > +static int sun6i_csi_bridge_link(struct sun6i_csi_device *csi_dev,
> > + int sink_pad_index,
> > + struct v4l2_subdev *remote_subdev,
> > + bool enabled)
> > +{
> > + struct device *dev = csi_dev->dev;
> > + struct v4l2_subdev *subdev = &csi_dev->bridge.subdev;
> > + struct media_entity *sink_entity = &subdev->entity;
> > + struct media_entity *source_entity = &remote_subdev->entity;
> > + int source_pad_index;
> > + int ret;
> > +
> > + /* Get the first remote source pad. */
> > + ret = media_entity_get_fwnode_pad(source_entity, remote_subdev->fwnode,
> > + MEDIA_PAD_FL_SOURCE);
> > + if (ret < 0) {
> > + dev_err(dev, "missing source pad in external entity %s\n",
> > + source_entity->name);
> > + return -EINVAL;
> > + }
> > +
> > + source_pad_index = ret;
> > +
> > + dev_dbg(dev, "creating %s:%u -> %s:%u link\n", source_entity->name,
> > + source_pad_index, sink_entity->name, sink_pad_index);
> > +
> > + ret = media_create_pad_link(source_entity, source_pad_index,
> > + sink_entity, sink_pad_index,
> > + enabled ? MEDIA_LNK_FL_ENABLED : 0);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to create %s:%u -> %s:%u link\n",
> > + source_entity->name, source_pad_index,
> > + sink_entity->name, sink_pad_index);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *remote_subdev,
> > + struct v4l2_async_subdev *async_subdev)
> > +{
> > + struct sun6i_csi_device *csi_dev =
> > + container_of(notifier, struct sun6i_csi_device,
> > + bridge.notifier);
> > + struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > + struct sun6i_csi_bridge_source *source = NULL;
> > + struct fwnode_handle *fwnode = dev_fwnode(csi_dev->dev);
> > + struct fwnode_handle *handle = NULL;
> > + bool enabled;
> > + int ret;
> > +
> > + while ((handle = fwnode_graph_get_next_endpoint(fwnode, handle))) {
>
> I'd instead store the information you need here in struct sun6i_csi_bridge.
> You could remove the loop here.
Is there a different method for matching a remote subdev to a local port?
The rationale here is that I need the handle for fwnode_graph_parse_endpoint
but cannot get that handle from the remote subdev's fwnode pointer directly.
> > + struct fwnode_endpoint endpoint = { 0 };
> > + struct fwnode_handle *remote_fwnode;
> > +
> > + remote_fwnode = fwnode_graph_get_remote_port_parent(handle);
> > + if (!remote_fwnode)
> > + continue;
> > +
> > + if (remote_fwnode != remote_subdev->fwnode)
> > + goto next;
> > +
> > + ret = fwnode_graph_parse_endpoint(handle, &endpoint);
> > + if (ret < 0)
> > + goto next;
> > +
> > + switch (endpoint.port) {
> > + case SUN6I_CSI_PORT_PARALLEL:
> > + source = &bridge->source_parallel;
> > + enabled = true;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > +next:
> > + fwnode_handle_put(remote_fwnode);
> > + }
> > +
> > + if (!source)
> > + return -EINVAL;
> > +
> > + source->subdev = remote_subdev;
> > +
> > + return sun6i_csi_bridge_link(csi_dev, SUN6I_CSI_BRIDGE_PAD_SINK,
> > + remote_subdev, enabled);
> > +}
> > +
> > +static int
> > +sun6i_csi_bridge_notifier_complete(struct v4l2_async_notifier *notifier)
> > +{
> > + struct sun6i_csi_device *csi_dev =
> > + container_of(notifier, struct sun6i_csi_device,
> > + bridge.notifier);
> > +
> > + return sun6i_csi_v4l2_complete(csi_dev);
>
> You could call v4l2_device_register_subdev_nodes() here.
That's definitely what sun6i_csi_v4l2_complete does (the diff is probably not
very clear). Note that the wrapper is extended later on to register the capture
video device for the no-isp path.
Maybe the capture registration could be kept in sun6i_csi_probe for the non-isp
path and then the wrapper wouldn't be needed. I don't mind either way.
> > +}
> > +
> > +static const struct v4l2_async_notifier_operations
> > +sun6i_csi_bridge_notifier_ops = {
> > + .bound = sun6i_csi_bridge_notifier_bound,
> > + .complete = sun6i_csi_bridge_notifier_complete,
> > +};
> > +
> > +/* Bridge */
> > +
> > +static int sun6i_csi_bridge_source_setup(struct sun6i_csi_device *csi_dev,
> > + struct sun6i_csi_bridge_source *source,
> > + u32 port,
> > + enum v4l2_mbus_type *bus_types)
> > +{
> > + struct device *dev = csi_dev->dev;
> > + struct v4l2_async_notifier *notifier = &csi_dev->bridge.notifier;
> > + struct v4l2_fwnode_endpoint *endpoint = &source->endpoint;
> > + struct v4l2_async_subdev *async_subdev;
> > + struct fwnode_handle *handle;
> > + int ret;
> > +
> > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), port, 0, 0);
> > + if (!handle)
> > + return -ENODEV;
> > +
> > + ret = v4l2_fwnode_endpoint_parse(handle, endpoint);
> > + if (ret)
> > + goto complete;
> > +
> > + if (bus_types) {
> > + bool valid = false;
> > + unsigned int i;
> > +
> > + for (i = 0; bus_types[i] != V4L2_MBUS_INVALID; i++) {
> > + if (endpoint->bus_type == bus_types[i]) {
> > + valid = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!valid) {
> > + dev_err(dev, "unsupported bus type for port %d\n",
> > + port);
> > + ret = -EINVAL;
> > + goto complete;
> > + }
> > + }
> > +
> > + async_subdev = v4l2_async_nf_add_fwnode_remote(notifier, handle,
> > + struct v4l2_async_subdev);
> > + if (IS_ERR(async_subdev)) {
> > + ret = PTR_ERR(async_subdev);
> > + goto complete;
> > + }
> > +
> > + source->expected = true;
> > +
> > +complete:
> > + fwnode_handle_put(handle);
> > +
> > + return ret;
> > +}
> > +
> > +int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> > +{
> > + struct device *dev = csi_dev->dev;
> > + struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > + struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_subdev *subdev = &bridge->subdev;
> > + struct v4l2_async_notifier *notifier = &bridge->notifier;
> > + struct media_pad *pads = bridge->pads;
> > + enum v4l2_mbus_type parallel_mbus_types[] = {
> > + V4L2_MBUS_PARALLEL,
> > + V4L2_MBUS_BT656,
> > + V4L2_MBUS_INVALID
> > + };
> > + int ret;
> > +
> > + /* V4L2 Subdev */
> > +
> > + v4l2_subdev_init(subdev, &sun6i_csi_bridge_subdev_ops);
> > + strscpy(subdev->name, SUN6I_CSI_BRIDGE_NAME, sizeof(subdev->name));
> > + subdev->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + subdev->owner = THIS_MODULE;
> > + subdev->dev = dev;
> > +
> > + v4l2_set_subdevdata(subdev, csi_dev);
> > +
> > + /* Media Entity */
> > +
> > + subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> > + subdev->entity.ops = &sun6i_csi_bridge_entity_ops;
> > +
> > + /* Media Pads */
> > +
> > + pads[SUN6I_CSI_BRIDGE_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > + pads[SUN6I_CSI_BRIDGE_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE |
> > + MEDIA_PAD_FL_MUST_CONNECT;
> > +
> > + ret = media_entity_pads_init(&subdev->entity,
> > + SUN6I_CSI_BRIDGE_PAD_COUNT, pads);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* V4L2 Subdev */
> > +
> > + ret = v4l2_device_register_subdev(v4l2_dev, subdev);
> > + if (ret) {
> > + dev_err(dev, "failed to register v4l2 subdev: %d\n", ret);
> > + goto error_media_entity;
> > + }
> > +
> > + /* V4L2 Async */
> > +
> > + v4l2_async_nf_init(notifier);
> > + notifier->ops = &sun6i_csi_bridge_notifier_ops;
> > +
> > + sun6i_csi_bridge_source_setup(csi_dev, &bridge->source_parallel,
> > + SUN6I_CSI_PORT_PARALLEL,
> > + parallel_mbus_types);
> > +
> > + ret = v4l2_async_nf_register(v4l2_dev, notifier);
> > + if (ret) {
> > + dev_err(dev, "failed to register v4l2 async notifier: %d\n",
> > + ret);
> > + goto error_v4l2_async_notifier;
> > + }
> > +
> > + return 0;
> > +
> > +error_v4l2_async_notifier:
> > + v4l2_async_nf_cleanup(notifier);
> > +
> > + v4l2_device_unregister_subdev(subdev);
> > +
> > +error_media_entity:
> > + media_entity_cleanup(&subdev->entity);
> > +
> > + return ret;
> > +}
> > +
> > +void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev)
> > +{
> > + struct v4l2_subdev *subdev = &csi_dev->bridge.subdev;
> > + struct v4l2_async_notifier *notifier = &csi_dev->bridge.notifier;
> > +
> > + v4l2_async_nf_unregister(notifier);
> > + v4l2_async_nf_cleanup(notifier);
> > +
> > + v4l2_device_unregister_subdev(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
> > new file mode 100644
> > index 000000000000..2ee7878102b6
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2021 Bootlin
>
> 2022?
Right, thanks!
> > + * Author: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > + */
> > +
> > +#ifndef _SUN6I_CSI_BRIDGE_H_
> > +#define _SUN6I_CSI_BRIDGE_H_
> > +
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#define SUN6I_CSI_BRIDGE_NAME "sun6i-csi-bridge"
> > +
> > +enum sun6i_csi_bridge_pad {
> > + SUN6I_CSI_BRIDGE_PAD_SINK = 0,
> > + SUN6I_CSI_BRIDGE_PAD_SOURCE = 1,
> > + SUN6I_CSI_BRIDGE_PAD_COUNT = 2,
> > +};
> > +
> > +struct sun6i_csi_device;
> > +
> > +struct sun6i_csi_bridge_source {
> > + struct v4l2_subdev *subdev;
> > + struct v4l2_fwnode_endpoint endpoint;
> > + bool expected;
> > +};
> > +
> > +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 sun6i_csi_bridge_source source_parallel;
> > + struct sun6i_csi_bridge_source *source;
> > +};
> > +
> > +/* Bridge */
> > +
> > +int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev);
> > +void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev);
> > +
> > +#endif
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > index d32ff6b81f8a..fa5bf3697ace 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > @@ -632,6 +632,7 @@ int sun6i_video_setup(struct sun6i_csi_device *csi_dev)
> > {
> > struct sun6i_video *video = &csi_dev->video;
> > struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_subdev *bridge_subdev = &csi_dev->bridge.subdev;
> > struct video_device *video_dev = &video->video_dev;
> > struct vb2_queue *queue = &video->queue;
> > struct media_pad *pad = &video->pad;
> > @@ -715,8 +716,26 @@ int sun6i_video_setup(struct sun6i_csi_device *csi_dev)
> > v4l2_info(v4l2_dev, "device %s registered as %s\n", video_dev->name,
> > video_device_node_name(video_dev));
> >
> > + /* Media Pad Link */
> > +
> > + ret = media_create_pad_link(&bridge_subdev->entity,
> > + SUN6I_CSI_BRIDGE_PAD_SOURCE,
> > + &video_dev->entity, 0,
> > + MEDIA_LNK_FL_ENABLED |
> > + MEDIA_LNK_FL_IMMUTABLE);
> > + if (ret < 0) {
> > + v4l2_err(v4l2_dev, "failed to create %s:%u -> %s:%u link\n",
> > + bridge_subdev->entity.name,
> > + SUN6I_CSI_BRIDGE_PAD_SOURCE,
> > + video_dev->entity.name, 0);
> > + goto error_video_device;
> > + }
> > +
> > return 0;
> >
> > +error_video_device:
> > + vb2_video_unregister_device(video_dev);
> > +
> > error_media_entity:
> > media_entity_cleanup(&video_dev->entity);
> >
>
> --
> Sakari Ailus
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists