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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

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.

> 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.

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

-- 
Regards,

Laurent Pinchart