Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

[Mehdi Djait <mehdi.djait.k@gmail.com>]

Hi Sakari,

On Tue, Feb 27, 2024 at 10:23:46AM +0000, Sakari Ailus wrote:
> Hi Mehdi,
> 
> On Wed, Feb 21, 2024 at 06:55:54PM +0100, Mehdi Djait wrote:
> > 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@bootlin.com>
> > > > 
> > > > 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@wolfvision.net>
> > > > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> > > > ---
> > > >  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.
> 
> Are you sure you even want to do this?
> 
> It'll limit what kind of sensors you can attach to the device and even more
> so in the future as we're reworking the sensor APIs to allow better control
> of the sensors, using internal pads (that require MC).
> 
> There have been some such drivers in the past but many have been already
> converted, or in some cases the newer hardware generation uses MC. Keeping
> API compatibility is a requirement so you can't just "add support" later
> on.

I totally agree that using the MC approach is better but this has nothing to
do with me wanting this but due to constraints I unfortunately cannot control
it is impossible to convert it now.

I would say the px30 driver is still very useful and people are going to use it: a follow-up patch series to
add support for the Rockchip RK3568 Video Capture has already been sent:
https://lore.kernel.org/linux-media/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/

--
Kind Regards
Mehdi Djait