Received-SPF: pass (google.com: domain of linux-kernel+bounces-75235-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 21 Feb 2024 18:55:54 +0100
From: Mehdi Djait <mehdi.djait.k@gmail.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
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: <ZdY5KrTfss4lTjPO@mehdi-archlinux>
References: <cover.1707677804.git.mehdi.djait.k@gmail.com>
 <715d89214d1ed6a8bb16cbb6268718a737485560.1707677804.git.mehdi.djait.k@gmail.com>
 <Zctwo3s9hso6mQvT@valkosipuli.retiisi.eu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zctwo3s9hso6mQvT@valkosipuli.retiisi.eu>

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.

> > +	if (ret)
> > +		return ret;
> > +
> > +	cif_update_pix(stream, fmt, pix);
> > +	stream->pix = *pix;
> > +	stream->cif_fmt_out = fmt;
> > +
> > +	return 0;
> > +}

--
Kind Regards
Mehdi Djait