Hi Sakari,
Thank you for the review!
On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
> Hi Mahdi,
>
> On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> > From: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> >
> > This introduces a V4L2 driver for the Rockchip CIF video capture controller.
> >
> > This controller supports multiple parallel interfaces, but for now only the
> > BT.656 interface could be tested, hence it's the only one that's supported
> > in the first version of this driver.
> >
> > This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
> > but for now it's only been tested on the PX30.
> >
> > CIF is implemented as a video node-centric driver.
> >
> > Most of this driver was written following the BSP driver from Rockchip,
> > removing the parts that either didn't fit correctly the guidelines, or that
> > couldn't be tested.
> >
> > This basic version doesn't support cropping nor scaling and is only
> > designed with one SDTV video decoder being attached to it at any time.
> >
> > This version uses the "pingpong" mode of the controller, which is a
> > double-buffering mechanism.
> >
> > Reviewed-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx>
> > Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@xxxxxxxxx>
> > ---
> > MAINTAINERS | 7 +
> > drivers/media/platform/rockchip/Kconfig | 1 +
> > drivers/media/platform/rockchip/Makefile | 1 +
> > drivers/media/platform/rockchip/cif/Kconfig | 14 +
> > drivers/media/platform/rockchip/cif/Makefile | 3 +
> > .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> > .../media/platform/rockchip/cif/cif-capture.h | 20 +
> > .../media/platform/rockchip/cif/cif-common.h | 128 ++
> > drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> > .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> > 10 files changed, 1720 insertions(+)
> > create mode 100644 drivers/media/platform/rockchip/cif/Kconfig
> > create mode 100644 drivers/media/platform/rockchip/cif/Makefile
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.c
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
> >
> > +static int cif_start_streaming(struct vb2_queue *queue, unsigned int count)
> > +{
> > + struct cif_stream *stream = queue->drv_priv;
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct v4l2_device *v4l2_dev = &cif_dev->v4l2_dev;
> > + struct v4l2_subdev *sd;
> > + int ret;
> > +
> > + if (!cif_dev->remote.sd) {
> > + ret = -ENODEV;
> > + v4l2_err(v4l2_dev, "No remote subdev detected\n");
> > + goto destroy_buf;
> > + }
> > +
> > + ret = pm_runtime_resume_and_get(cif_dev->dev);
> > + if (ret < 0) {
> > + v4l2_err(v4l2_dev, "Failed to get runtime pm, %d\n", ret);
> > + goto destroy_buf;
> > + }
> > +
> > + sd = cif_dev->remote.sd;
> > +
> > + stream->cif_fmt_in = get_input_fmt(cif_dev->remote.sd);
>
> You should use the format on the local pad, not get it from a remote
> sub-device.
>
> Link validation ensures they're the same (or at least compatible).
>
> Speaking of which---you don't have link_validate callbacks set for the
> sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
> example.
>
...
> > + if (!stream->cif_fmt_in)
> > + goto runtime_put;
> > +
> > + ret = cif_stream_start(stream);
> > + if (ret < 0)
> > + goto stop_stream;
> > +
> > + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > + if (ret < 0)
> > + goto stop_stream;
> > +
> > + return 0;
> > +
> > +stop_stream:
> > + cif_stream_stop(stream);
> > +runtime_put:
> > + pm_runtime_put(cif_dev->dev);
> > +destroy_buf:
> > + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> > +
> > + return ret;
> > +}
> > +
> > +static int cif_set_fmt(struct cif_stream *stream,
> > + struct v4l2_pix_format *pix)
> > +{
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct v4l2_subdev_format sd_fmt;
> > + struct cif_output_fmt *fmt;
> > + int ret;
> > +
> > + if (vb2_is_streaming(&stream->buf_queue))
> > + return -EBUSY;
> > +
> > + fmt = find_output_fmt(stream, pix->pixelformat);
> > + if (!fmt)
> > + fmt = &out_fmts[0];
> > +
> > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + sd_fmt.pad = 0;
> > + sd_fmt.format.width = pix->width;
> > + sd_fmt.format.height = pix->height;
> > +
> > + ret = v4l2_subdev_call(cif_dev->remote.sd, pad, set_fmt, NULL, &sd_fmt);
>
> The user space is responsible for controlling the sensor i.e. you shouldn't
> call set_fmt sub-device op from this driver.
>
> As the driver is MC-enabled, generally the sub-devices act as a control
> interface and the V4L2 video nodes are a data interface.
>
While this is true for MC-centric (Media Controller) drivers, this driver is
video-node-centric (I mentioned this in the commit msg)
>From the Kernel Documentation: https://docs.kernel.org/userspace-api/media/v4l/open.html
1 - The devices that are fully controlled via V4L2 device nodes are called video-node-centric.
2- Note:
A video-node-centric may still provide media-controller and sub-device interfaces as well.
However, in that case the media-controller and the sub-device interfaces are read-only and just
provide information about the device. The actual configuration is done via the video nodes.
> > + if (ret)
> > + return ret;
> > +
> > + cif_update_pix(stream, fmt, pix);
> > + stream->pix = *pix;
> > + stream->cif_fmt_out = fmt;
> > +
> > + return 0;
> > +}
--
Kind Regards
Mehdi Djait
