Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

Hi,


Am 02.10.20 um 00:48 schrieb Laurent Pinchart:
> Hi Dafna,
> 
> On Thu, Oct 01, 2020 at 09:36:07PM +0200, Dafna Hirschfeld wrote:
>> Am 01.10.20 um 04:03 schrieb Laurent Pinchart:
>>> On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
>>>> Hi Dafna,
>>>>
>>>> This commit is not just a simplification but a change of behavior.  The
>>>> change is for the better but it broke capture of NV12 and NV21 formats
>>>> in libcamera unexpectedly.
>>>>
>>>> The issue at hand is that libcamera when configuring the pipeline
>>>> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then
>>>> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad
>>>> (0) and then to the resizers source pad (1). Effectively propagating
>>>> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
>>>>
>>>> At this point if the video node (main or self) is configured with a
>>>> YUV420 format (NV12, NV21, etc) and with this change applied the link
>>>> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=
>>>> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is
>>>> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and
>>>> libcamera lockup the capture session.
>>>
>>> I would be very, very surprised is the hardware really used YUYV8_1_5X8.
>>> YUYV8_1X16 is a much more likely bus format between the resizer and the
>>> DMA engine, as well as between the ISP and the resizer.
>>
>> Format YUYV8_1X16 is for downsampling of 4:2:2, but the resizer has the ability
>> to downsample to 4:2:0.
>> I see there is also format YDYUYDYV8_1X16 for 4:2:0
>> maybe this is what I should set?
>>
>> Actually according to the TRM the resizer send the stream to the DMA
>> engine through two separated buses, one for luma and one for chroma.
> 
> In which document is this documented ? Is this two 8-bit buses side by
> side ?

here: https://rockchip.fr/RK3288%20TRM/rk3288-chapter-31-image-signal-processing-(isp).pdf

It is described that there is a splitter component before the
resizer. The diagram shows that those two separate streams are also
sent further to the DMA engine.

> 
> Looking at the registers, the output formats are controlled by the
> global MI_CTRL register, common to both the main and self paths, which
> should correspond to the DMA engine. I think it would make sense to
> model this at the video node level, and hardcode YUYV8_1X16 between the
> resizer and the video node.

Ok, I can change the mbus code. Is there a difference from userspace point of
view? If userspace wants to stream YUV 4:2:0, he should set the pixelformat on
the video node and should only care that the resizer and the video node agree
on the mbus code.

Thanks,
Dafna

> 
>>>> I will submit a fix for this to libcamera to bring it in sync with this
>>>> change.
>>>>
>>>> Would it be possible to ask that future changes to the rkisp1 driver be
>>>> tested with libcamera so we can track and update both the kernel and
>>>> user-space components of this driver at the same time and avoid nasty
>>>> surprises? :-)
>>>
>>> I strongly second this. Drivers that are supported in libcamera should
>>> be tested with libcamera to catch regressions, for any chance submitted
>>> to the kernel.
>>
>> I can run several 'cam' commands with different formats and dimensions to
>> find regressions. I currently have unit test only in v4l-utils.
> 
> That would be great :-) We will work on a test suite for higher-level
> tests (something similar to the Android CTS) at some point, which should
> also help catching regressions.
> 
>>>> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
>>>>> The capture has a mapping of the mbus code needed for each pixelformat.
>>>>> This can be used to simplify the link validation by comparing the mbus
>>>>> code in the capture with the code in the resizer.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>> ---
>>>>>    drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
>>>>>    1 file changed, 4 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> index 4dabd07a3da9..a5e2521577dd 100644
>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>    	struct v4l2_subdev *sd =
>>>>>    		media_entity_to_v4l2_subdev(link->source->entity);
>>>>>    	struct rkisp1_capture *cap = video_get_drvdata(vdev);
>>>>> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
>>>>> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
>>>>> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
>>>>> +	const struct rkisp1_capture_fmt_cfg *fmt =
>>>>> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
>>>>>    	struct v4l2_subdev_format sd_fmt;
>>>>>    	int ret;
>>>>>    
>>>>> -	if (cap_pix_enc != isp_pix_enc &&
>>>>> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
>>>>> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
>>>>> -		dev_err(cap->rkisp1->dev,
>>>>> -			"format type mismatch in link '%s:%d->%s:%d'\n",
>>>>> -			link->source->entity->name, link->source->index,
>>>>> -			link->sink->entity->name, link->sink->index);
>>>>> -		return -EPIPE;
>>>>> -	}
>>>>> -
>>>>>    	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>>>    	sd_fmt.pad = link->source->index;
>>>>>    	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
>>>>> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>    		return ret;
>>>>>    
>>>>>    	if (sd_fmt.format.height != cap->pix.fmt.height ||
>>>>> -	    sd_fmt.format.width != cap->pix.fmt.width)
>>>>> +	    sd_fmt.format.width != cap->pix.fmt.width ||
>>>>> +	    sd_fmt.format.code != fmt->mbus)
>>>>>    		return -EPIPE;
>>>>>    
>>>>>    	return 0;
>