Re: [PATCH v2 04/34] media: bcm2835-unicam: Driver for CCP2/CSI2 camera interface

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

Hi Sakari,

(With a question for Dave below)

I'm replying to the two main points of your review. All the other
comments look fine at a glance, Jacopo is having a more detailed look
and will incorporate them in v3.

On Tue, Sep 15, 2020 at 10:03:26AM +0300, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thanks for the patch, and my apologies for the late review. Please do cc me
> for v3.
> 
> After a quick look I can already say this is the cleanest Unicam driver
> I've ever seen. But please also see my detailed comments below.
> 
> On Mon, May 04, 2020 at 12:25:41PM +0300, Laurent Pinchart wrote:
> > From: Naushir Patuck <naush@raspberrypi.com>
> > 
> > Add a driver for the Unicam camera receiver block on BCM283x processors.
> > Compared to the bcm2835-camera driver present in staging, this driver
> > handles the Unicam block only (CSI-2 receiver), and doesn't depend on
> > the VC4 firmware running on the VPU.
> > 
> > The commit is made up of a series of changes cherry-picked from the
> > rpi-5.4.y branch of https://github.com/raspberrypi/linux/ with
> > additional enhancements, forward-ported to the mainline kernel.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Re-fetch mbus code from subdev on a g_fmt call
> > - Group all ioctl disabling together
> > - Fix reference counting in unicam_open
> > - Add support for VIDIOC_[S|G]_SELECTION
> > ---
> >  MAINTAINERS                                   |    7 +
> >  drivers/media/platform/Kconfig                |    1 +
> >  drivers/media/platform/Makefile               |    2 +
> >  drivers/media/platform/bcm2835/Kconfig        |   15 +
> >  drivers/media/platform/bcm2835/Makefile       |    3 +
> >  .../media/platform/bcm2835/bcm2835-unicam.c   | 2825 +++++++++++++++++
> >  .../media/platform/bcm2835/vc4-regs-unicam.h  |  253 ++
> >  7 files changed, 3106 insertions(+)
> >  create mode 100644 drivers/media/platform/bcm2835/Kconfig
> >  create mode 100644 drivers/media/platform/bcm2835/Makefile
> >  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
> >  create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

[snip]

> > diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> > new file mode 100644
> > index 000000000000..2e9387cbc1e0
> > --- /dev/null
> > +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> > @@ -0,0 +1,2825 @@

[snip]

> > +static int unicam_enum_frameintervals(struct file *file, void *priv,
> > +				      struct v4l2_frmivalenum *fival)
> > +{
> > +	struct unicam_node *node = video_drvdata(file);
> > +	struct unicam_device *dev = node->dev;
> > +	const struct unicam_fmt *fmt;
> > +	struct v4l2_subdev_frame_interval_enum fie = {
> > +		.index = fival->index,
> > +		.width = fival->width,
> > +		.height = fival->height,
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	int ret;
> > +
> > +	fmt = find_format_by_pix(dev, fival->pixel_format);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	fie.code = fmt->code;
> > +	ret = v4l2_subdev_call(dev->sensor, pad, enum_frame_interval,
> > +			       NULL, &fie);
> 
> You're adding a new CSI-2 receiver driver but your driver appears to be
> video node centric and does not use MC / V4L2 subdev uAPIs for pipeline
> configuration.
> 
> This is effectively needed if you want to be able to capture embedded data.
> 
> I'd also recommend it since this way the driver will be compliant with all
> camera sensor drivers, not just those that expose a single sub-device.
> There are no good ways to change this once your driver is in upstream
> kernel.
> 
> This is also why e.g. ipu3-cio2 driver is MC-centric.

I've had lengthy discussions with Dave on this topic. While I agree with
you in principle, Dave had good arguments for keeping this
video-node-centric. We all agreed it wasn't a perfect solution, but it
could still be a pragmatic one.

If I remember correctly the discussion was in private e-mails though.
Dave, I'm pretty sure you're tired of repeating the same thing, but
Sakari can't be expected to know all we've talked about. I can try to
summarize your points, but I may not do a very good job at defending
your point of view given that I wish you would be wrong :-) Would you
like to summarize your position, or should I give it a go ?

> > +	if (ret)
> > +		return ret;
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > +	fival->discrete = fie.interval;
> > +
> > +	return 0;
> > +}

[snip]

> > +static int register_node(struct unicam_device *unicam, struct unicam_node *node,
> > +			 enum v4l2_buf_type type, int pad_id)
> > +{

[snip]

> > +	if (pad_id != METADATA_PAD || unicam->sensor_embedded_data) {
> > +		ret = media_create_pad_link(&unicam->sensor->entity, pad_id,
> > +					    &node->video_dev.entity, 0,
> > +					    MEDIA_LNK_FL_ENABLED |
> > +					    MEDIA_LNK_FL_IMMUTABLE);
> 
> This does create two links between the sensor and the CSI-2 receiver,
> doesn't it?
> 
> The links in Media controller represent physical links, not logical flows
> of data. At the time the API was added, it wasn't thought there would be a
> need to separate the two.
> 
> There is an effort to add the concept of data flow to V4L2, but it's been
> complicated and we haven't had patches for the CSI-2 receiver driver to
> support it. Perhaps Unicam could be the first one to do that?

I agree that this is the right approach. The V4L2 multiplexed streams
support seems to be one of these cursed series, where bad things happen
to anyone who touches it. I was about to actively start working on it
again back in June for a different project, which then got frozen at the
last minute :-S

Would you like to give it a try ? :-) I'd be more than happy to provide
you hardware as a present.

> Alternatively support for embedded data could be removed in the meantime.
> 
> The latest patchset is here I believe:
> 
> <URL:https://patchwork.kernel.org/project/linux-media/list/?series=98277>
> 
> > +		if (ret)
> > +			unicam_err(unicam, "Unable to create pad link for %s\n",
> > +				   vdev->name);
> > +	}
> > +
> > +	return ret;
> > +}

-- 
Regards,

Laurent Pinchart