Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface

[Michael Riesch <michael.riesch@wolfvision.net>]

Hi Mehdi,

On 11/13/23 12:06, Mehdi Djait wrote:
> Hi Michael,
> 
> On Fri, Nov 10, 2023 at 01:50:01PM +0100, Michael Riesch wrote:
>> Hi Mehdi,
>>
>> Thanks for your patches.
>>
>> The good news first: with some hacks applied I was able to capture the
>> video stream from a HDMI receiver chip via BT.1120 on a Rockchip RK3568.
> 
> this is really cool!
> 
>>
>> As a next step, I'll clean up the hacky RK3568 support and submit them
>> for review.
>>
>> Still, there are some issues that needs to be addressed. Please find my
>> comments inline.
>>
>> On 11/8/23 17:38, Mehdi Djait wrote:
>>> 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.
>>>
>>> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
>>> +static const struct cif_input_fmt in_fmts[] = {
>>> +	{
>>> +		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
>>> +		.dvp_fmt_val	= CIF_FORMAT_YUV_INPUT_422 |
>>> +				  CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
>>> +		.csi_fmt_val	= CIF_CSI_WRDDR_TYPE_YUV422,
>>
>> What is the point of this csi_fmt_val field? If the strategy is to kick
>> everything out that is not explicitly required then this should be
>> removed (and added at a later stage, if needed).
>>
> 
> I can remove this but I don't really see the harm of keeping this.
> It can even save some time for the person adding the support for CSI
> later.

IMHO removing it would be more consistent with your approach to kick
everything out that is not explicitly needed. I also think that this
approach is common practice.

>>> +		.fmt_type	= CIF_FMT_TYPE_YUV,
>>> +		.field		= V4L2_FIELD_NONE,
>>> +	}, {
>>> +		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
>>> +		.dvp_fmt_val	= CIF_FORMAT_YUV_INPUT_422 |
>>> +				  CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
>>> +		.csi_fmt_val	= CIF_CSI_WRDDR_TYPE_YUV422,
>>> +		.fmt_type	= CIF_FMT_TYPE_YUV,
>>> +		.field		= V4L2_FIELD_INTERLACED,
>>> +	}, {
>>> +		.mbus_code	= MEDIA_BUS_FMT_YVYU8_2X8,
>>> +		.dvp_fmt_val	= CIF_FORMAT_YUV_INPUT_422 |
>>> +				  CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
>>> +		.csi_fmt_val	= CIF_CSI_WRDDR_TYPE_YUV422,
>>> +		.fmt_type	= CIF_FMT_TYPE_YUV,
>>> +		.field		= V4L2_FIELD_NONE,
>>
>> What is the difference between this entry (in_fmts[2]) and in_fmts[0]?
>> Similarly, between in_fmts[1] and in_fmts[3]?
>>
> 
> Between in_fmts[0] and in_fmts[2] is the order of Y U V components:
> 0 -> YUYV
> 2 -> YVYU
> 
> between in_fmts[1] and in_fmts[3]: the same thing:
> 1 -> YUYV
> 3 -> YVYU

Of course. Apparently I was staring at the code for too long. Sorry for
the noise.

> [...]
>>> +	input_mode = (remote_info->std == V4L2_STD_NTSC) ?
>>> +		      CIF_FORMAT_INPUT_MODE_NTSC :
>>> +		      CIF_FORMAT_INPUT_MODE_PAL;
>>
>> This seems to be an oversimplification. How can one use BT.656 here?
> 
> I don't quite understand the question. This is used to configure the
> hardware, i.e., the INPUT_MODE of the format VIP_FOR
> 
> bits 4:2
> 
> INPUT_MODE Input mode:
> 
> 3'b000 : YUV
> 3'b010 : PAL
> 3'b011 : NTSC
> 3'b100 : RAW
> 3'b101 : JPEG
> 3'b110 : MIPI
> Other : invalid

OK, for the RK3568 VICAP the table reads

3'b000: BT601 YUV422
3'b010: BT656 YUV422
3'b100: BT601 RAW
3'b101: SONY RAW
3'b111: BT1120 YUV422
others: Reserved

The PX30 TRM states "Support CCIR656(PAL/NTSC) input" and I think
CCIR-656 equals ITU-R BT.656.

I think in the end the mbus format needs to be evaluated first. If it is
BT.656 AND if the subdevice supports TV standards, the decision between
PAL and NTSC can be made.

>> (Aren't you using BT.656 as mbus format between your video decoder and
>> the PX30 VIP?)
> 
> I look into this. I will probably need to add this.

I think it would help if your device tree example in the YAML
documentation contained the complete setup (vip + video decoder chip).

>> You should not assume that the remote is capable of any TV standards
>> (this statement holds for the driver in general).
>>
> 
> But this is the support I am adding right now, for cif with a SDTV
> decoder capable of TV standards. This statement will need to be 
> changed when support for sensors is added. 
> 
>>> +	mbus_flags = remote_info->mbus.bus.parallel.flags;
>>> +	href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
>>> +			0 : CIF_FORMAT_HSY_LOW_ACTIVE;
>>> +	vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
>>> +			CIF_FORMAT_VSY_HIGH_ACTIVE : 0;
>>> +
>>> +	val = vsync_pol | href_pol | input_mode | stream->cif_fmt_out->fmt_val |
>>> +	      stream->cif_fmt_in->dvp_fmt_val | xfer_mode | yc_swap;
>>> +void cif_set_default_format(struct cif_device *cif_dev)
>>> +{
>>> +	struct cif_stream *stream = &cif_dev->stream;
>>> +	struct v4l2_pix_format pix;
>>> +
>>> +	cif_dev->remote.std = V4L2_STD_NTSC;
>>
>> Not every subdevice supports TV standards. Is this really reasonable?
>>
> 
> For the support I am adding right now it is reasonable but for future
> support it needs to be changed.

OK then I guess I'll need to change that in my first batch of changes :-)

>>> +
>>> +	pix.pixelformat = V4L2_PIX_FMT_NV12;
>>> +	pix.width = CIF_DEFAULT_WIDTH;
>>> +	pix.height = CIF_DEFAULT_HEIGHT;
>>> +
>>> +	cif_set_fmt(stream, &pix);
>>> +}
>>> +
>>> +static int cif_enum_input(struct file *file, void *priv,
>>> +			  struct v4l2_input *input)
>>> +{
>>> +	struct cif_stream *stream = video_drvdata(file);
>>> +	struct v4l2_subdev *sd = stream->cifdev->remote.sd;
>>> +	int ret;
>>> +
>>> +	if (input->index > 0)
>>> +		return -EINVAL;
>>> +
>>> +	ret = v4l2_subdev_call(sd, video, g_input_status, &input->status);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	strscpy(input->name, "Camera", sizeof(input->name));
>>> +	input->type = V4L2_INPUT_TYPE_CAMERA;
>>
>> Wait, we are a camera in any case? How does this fit together with your
>> video decoder setup?
>>
> 
> Yes the video decoder is attached to a camera.
> 
> From the kernel documentation:
> https://docs.kernel.org/userspace-api/media/v4l/vidioc-enuminput.html?highlight=v4l2_input_type_camera
> --------------------------------------------------------------------------------
> V4L2_INPUT_TYPE_CAMERA
> Any non-tuner video input, for example Composite Video, S-Video, HDMI, camera
> sensor. The naming as _TYPE_CAMERA is historical, today we would have called it
> _TYPE_VIDEO.
> --------------------------------------------------------------------------------

Huh, learned something. Thanks for the pointer!

>>> +	input->std = stream->vdev.tvnorms;
>>> +	input->capabilities = V4L2_IN_CAP_STD;
>>
>> Not every subdevice supports TV standards. Is this really reasonable?
>>
> 
> see above answer.

OK. I think I can submit a series that makes the driver more general
based on your v11. Looking forward to that ;-)

Thanks and best regards,
Michael