Received-SPF: pass (google.com: domain of linux-kernel+bounces-83026-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 27 Feb 2024 10:23:46 +0000
From: Sakari Ailus <sakari.ailus@iki.fi>
To: Mehdi Djait <mehdi.djait.k@gmail.com>
Cc: mchehab@kernel.org, heiko@sntech.de, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, robh+dt@kernel.org,
	conor+dt@kernel.org, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	thomas.petazzoni@bootlin.com, alexandre.belloni@bootlin.com,
	maxime.chevallier@bootlin.com, paul.kocialkowski@bootlin.com,
	michael.riesch@wolfvision.net, laurent.pinchart@ideasonboard.com,
	Mehdi Djait <mehdi.djait@bootlin.com>
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for
 Rockchip's camera interface
Message-ID: <Zd24MhLYJlSTRysr@valkosipuli.retiisi.eu>
References: <cover.1707677804.git.mehdi.djait.k@gmail.com>
 <715d89214d1ed6a8bb16cbb6268718a737485560.1707677804.git.mehdi.djait.k@gmail.com>
 <Zctwo3s9hso6mQvT@valkosipuli.retiisi.eu>
 <ZdY5KrTfss4lTjPO@mehdi-archlinux>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZdY5KrTfss4lTjPO@mehdi-archlinux>

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.

-- 
Regards,

Sakari Ailus