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@bootlin.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@bootlin.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