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@xxxxxxxxxxxxx>
> > ---
> > 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
