Re: [PATCH 7/9] V4L: soc-camera: add a Media Controller wrapper

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guennadi,

On Monday 03 October 2011 17:29:23 Guennadi Liakhovetski wrote:
> Hi Laurent
> 
> Thanks for the reviews!

You're welcome.

> On Mon, 3 Oct 2011, Laurent Pinchart wrote:
> > On Thursday 29 September 2011 18:18:55 Guennadi Liakhovetski wrote:
> > > This wrapper adds a Media Controller implementation to soc-camera
> > > drivers. To really benefit from it individual host drivers should
> > > implement support for values of enum soc_camera_target other than
> > > SOCAM_TARGET_PIPELINE in their .set_fmt() and .try_fmt() methods.
> > 
> > [snip]
> > 
> > > diff --git a/drivers/media/video/soc_entity.c
> > > b/drivers/media/video/soc_entity.c new file mode 100644
> > > index 0000000..3a04700
> > > --- /dev/null
> > > +++ b/drivers/media/video/soc_entity.c
> > > @@ -0,0 +1,284 @@
> > 
> > [snip]
> > 
> > > +static int bus_sd_pad_g_fmt(struct v4l2_subdev *sd, struct
> > > v4l2_subdev_fh *fh,
> > > +			    struct v4l2_subdev_format *sd_fmt)
> > > +{
> > > +	struct soc_camera_device *icd = v4l2_get_subdevdata(sd);
> > > +	struct v4l2_mbus_framefmt *f = &sd_fmt->format;
> > > +
> > > +	if (sd_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > +		sd_fmt->format = *v4l2_subdev_get_try_format(fh, sd_fmt->pad);
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (sd_fmt->pad == SOC_HOST_BUS_PAD_SINK) {
> > > +		f->width	= icd->host_input_width;
> > > +		f->height	= icd->host_input_height;
> > > +	} else {
> > > +		f->width	= icd->user_width;
> > > +		f->height	= icd->user_height;
> > > +	}
> > > +	f->field	= icd->field;
> > > +	f->code		= icd->current_fmt->code;
> > > +	f->colorspace	= icd->colorspace;
> > 
> > Can soc-camera hosts perform format conversion ? If so you will likely
> > need to store the mbus code for the input and output separately,
> > possibly in v4l2_mbus_format fields. You could then simplify the
> > [gs]_fmt functions by implementing similar to the __*_get_format
> > functions in the OMAP3 ISP driver.
> 
> They can, yes. But, under soc-camera conversions are performed between
> mediabus codes and fourcc formats. Upon pipeline construction (probing) a
> table of format conversions is built, where hosts generate one or more
> translation entries for all client formats, that they support. The only
> example of a more complex translations so far is MIPI CSI-2, but even
> there we have decided to identify CSI-2 formats using the same media-bus
> codes, as what you "get" "between" the CSI-2 block and the DMA engine. For
> the only CSI-2 capable soc-camera host so far - the CEU driver - this is
> also a very natural representation, because there the CSI-2 block is
> indeed an additional pipeline stage, uniquely translating CSI-2 to
> media-bus codes, that are then fed to the CEU parallel port.

How does that work with the MC API then ? If the bridge can, let's say, 
convert between raw bayer and YUV, shouldn't the format at the bridge input be 
raw bayer and at the bridge output YUV ?

> > > +	return 0;
> > > +}
> > > +
> > > +static int bus_sd_pad_s_fmt(struct v4l2_subdev *sd, struct
> > > v4l2_subdev_fh *fh,
> > > +			    struct v4l2_subdev_format *sd_fmt)
> > > +{
> > > +	struct soc_camera_device *icd = v4l2_get_subdevdata(sd);
> > > +	struct v4l2_mbus_framefmt *mf = &sd_fmt->format;
> > > +	struct v4l2_format vf = {
> > > +		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> > > +	};
> > > +	enum soc_camera_target tgt = sd_fmt->pad == SOC_HOST_BUS_PAD_SINK ?
> > > +		SOCAM_TARGET_HOST_IN : SOCAM_TARGET_HOST_OUT;
> > > +	int ret;
> > > +
> > > +	se_mbus_to_v4l2(icd, mf, &vf);
> > > +
> > > +	if (sd_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > +		struct v4l2_mbus_framefmt *try_fmt =
> > > +			v4l2_subdev_get_try_format(fh, sd_fmt->pad);
> > > +		ret = soc_camera_try_fmt(icd, &vf, tgt);
> > > +		if (!ret) {
> > > +			se_v4l2_to_mbus(icd, &vf, try_fmt);
> > > +			sd_fmt->format = *try_fmt;
> > > +		}
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = soc_camera_set_fmt(icd, &vf, tgt);
> > > +	if (!ret)
> > > +		se_v4l2_to_mbus(icd, &vf, &sd_fmt->format);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int bus_sd_pad_enum_mbus_code(struct v4l2_subdev *sd,
> > > +				     struct v4l2_subdev_fh *fh,
> > > +				     struct v4l2_subdev_mbus_code_enum *ce)
> > > +{
> > > +	struct soc_camera_device *icd = v4l2_get_subdevdata(sd);
> > > +
> > > +	if (ce->index >= icd->num_user_formats)
> > > +		return -EINVAL;
> > > +
> > > +	ce->code = icd->user_formats[ce->index].code;
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct v4l2_subdev_pad_ops se_bus_sd_pad_ops = {
> > > +	.get_fmt	= bus_sd_pad_g_fmt,
> > > +	.set_fmt	= bus_sd_pad_s_fmt,
> > > +	.enum_mbus_code	= bus_sd_pad_enum_mbus_code,
> > > +};
> > > +
> > > +static const struct v4l2_subdev_ops se_bus_sd_ops = {
> > > +	.pad		= &se_bus_sd_pad_ops,
> > > +};
> > > +
> > > +static const struct media_entity_operations se_bus_me_ops = {
> > > +};
> > > +
> > > +static const struct media_entity_operations se_vdev_me_ops = {
> > > +};
> > 
> > NULL operations are allowed, you don't have to use an empty structure.
> 
> Ok
> 
> > > +
> > > +int soc_camera_mc_streamon(struct soc_camera_device *icd)
> > > +{
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > +	struct v4l2_subdev *bus_sd = &ici->bus_sd;
> > > +	struct media_entity *bus_me = &bus_sd->entity;
> > > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > > +	struct v4l2_mbus_framefmt mf;
> > > +	int ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
> > > +	if (WARN_ON(ret < 0))
> > > +		return ret;
> > > +	if (icd->host_input_width != mf.width ||
> > > +	    icd->host_input_height != mf.height ||
> > > +	    icd->current_fmt->code != mf.code)
> > > +		return -EINVAL;
> > 
> > Shouldn't you also check that the source pad format matches the video
> > node format ?
> 
> I think, that's true by construction. It is already cheked in
> soc_camera_set_fmt():
> 
> 	} else if (!icd->current_fmt ||
> 		   icd->current_fmt->host_fmt->fourcc != pix->pixelformat) {
> 		dev_err(icd->pdev,
> 			"Host driver hasn't set up current format correctly!\n");
> 		return -EINVAL;

But aren't applications allowed to configure the format at the bridge output 
pad first ?

> > > +
> > > +	media_entity_pipeline_start(bus_me, &ici->pipe);
> > > +	return 0;
> > > +}
> > > +
> > > +void soc_camera_mc_streamoff(struct soc_camera_device *icd)
> > > +{
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > +	struct v4l2_subdev *bus_sd = &ici->bus_sd;
> > > +	struct media_entity *bus_me = &bus_sd->entity;
> > > +	media_entity_pipeline_stop(bus_me);
> > > +}
> > > +
> > > +int soc_camera_mc_install(struct soc_camera_device *icd)
> > > +{
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > +	struct v4l2_subdev *bus_sd = &ici->bus_sd;
> > > +	struct media_entity *bus_me = &bus_sd->entity;
> > > +	struct media_pad *bus_pads = ici->bus_pads;
> > > +	struct media_pad *vdev_pads = ici->vdev_pads;
> > > +	struct video_device *vdev = icd->vdev;
> > > +	struct media_entity *vdev_me = &vdev->entity;
> > > +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > > +	int ret;
> > > +
> > > +	if (!ici->v4l2_dev.mdev || soc_entity_native_mc(icd))
> > > +		return 0;
> > > +
> > > +	/* Configure the video bus subdevice, entity, and pads */
> > > +	v4l2_subdev_init(bus_sd, &se_bus_sd_ops);
> > > +	v4l2_set_subdevdata(bus_sd, icd);
> > > +	bus_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +	snprintf(bus_sd->name, sizeof(bus_sd->name), "%s input",
> > > ici->drv_name); +
> > > +	bus_pads[SOC_HOST_BUS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +	bus_pads[SOC_HOST_BUS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > > +	bus_me->ops = &se_bus_me_ops;
> > > +
> > > +	ret = media_entity_init(bus_me, 2, bus_pads, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* Configure the video-device entity */
> > > +	vdev_pads[SOC_HOST_VDEV_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +	vdev_me->ops = &se_vdev_me_ops;
> > > +
> > > +	ret = media_entity_init(vdev_me, 1, vdev_pads, 0);
> > > +	if (ret < 0)
> > > +		goto evmei;
> > > +
> > > +	/* Link the two entities */
> > > +	ret = media_entity_create_link(bus_me, SOC_HOST_BUS_PAD_SOURCE,
> > > +				vdev_me, SOC_HOST_VDEV_PAD_SINK,
> > > +				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +	if (ret < 0)
> > > +		goto elink;
> > > +
> > > +	ret = v4l2_device_register_subdev(&ici->v4l2_dev, bus_sd);
> > > +	if (ret < 0)
> > > +		goto eregsd;
> > > +
> > > +	ret = v4l2_device_register_subdev_nodes(&ici->v4l2_dev);
> > > +	if (ret < 0)
> > > +		goto eregsdn;
> > > +
> > > +	/*
> > > +	 * Link the client: make it immutable too for now, since there is no
> > > +	 * meaningful mapping for the .link_setup() method to the soc-camera
> > > +	 * API
> > > +	 */
> > > +	ret = media_entity_create_link(&sd->entity, 0,
> > > +				bus_me, SOC_HOST_BUS_PAD_SINK,
> > > +				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> > > +	if (ret < 0)
> > > +		goto eclink;
> > 
> > Qhat do you think about moving this above subdev registration ?
> 
> I don't yet:-) But yes, perhaps, this can be done.

-- 
Regards,

Laurent Pinchart