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

[Michael Riesch]

Hi Mehdi, Sakari,

On 3/2/24 12:51, Mehdi Djait wrote:
> 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@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.
>>
>> 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@xxxxxxxxxxxxxx/

The driver is indeed useful as is, therefore I was rather hoping that it
would be accepted quickly to facilitate further additions (such as the
aforementioned RK3568 support series).

However, I was not aware that the video node centric vs. media
controller centric approach has significant implications on user space
and hence on backwards compatibility. Now that Sakari has pointed out
that one, I am leaning towards converting the driver to MC before it is
integrated in mainline.

I fully understand, though, that Mehdi is not in the position to make
the required changes due to time constraints. Maybe I can fill in and
invest some time in that, provided that
 - it is OK for Mehdi and the Bootlin people that I take over the series
   at hand, leaving the authorship intact of course, but adding my
   Co-developed-by:
 - Sakari (or someone else from the linux-media community) can provide a
   brief overview of what exactly needs to be done to do the conversion
It should be noted that right now I have no clue what needs to be
changed, which implies that the conversion will not happen any time soon.

What do you think?

Best regards,
Michael