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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > 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
