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@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;
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
