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

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

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 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? :-)

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;
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund