Re: [PATCH 00/15] Intel IPU6 and IPU6 input system drivers

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

On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/4/23 05:13, Cao, Bingbu wrote:
> > Hans,
> > 
> > Thanks for your test and report.
> > 
> > I have made some changes locally which will support the latest
> > v4l2-async APIs, I will also add the fix for this issue and merge
> > them in v3.
> 
> I have been trying to make rawbayer capture with the mainline isys code
> work in libcamera and I have hit several short comings in the ipu6-isys
> code. I have attached 3 patches to fix various issues please integrate
> these into the next version of this series.

They look good to me.

> Talking about the next version of this series, I think it would be
> good to post a new version soon ?
> 

> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 16:37:11 +0200
> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>  /dev/video enum_fmt
> 
> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index dc1605491352..20fd03f6f204 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>  			      struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
> -		return -EINVAL;
> +	unsigned int i, found = 0;
>  
> -	f->flags = 0;
> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
> +	if (!f->mbus_code) {
> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
> +			return -EINVAL;
>  
> -	return 0;
> +		f->flags = 0;
> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
> +			continue;
> +
> +		if (f->index == found) {
> +			f->flags = 0;
> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
> +			return 0;
> +		}
> +
> +		found++;
> +	}
> +
> +	return -EINVAL;
>  }
>  
>  static int vidioc_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> -- 
> 2.41.0
> 

> From 8250d2c3fd1c2ab83debcca80b4947d3b9d410f7 Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 17:02:06 +0200
> Subject: [PATCH 2/3] media: intel/ipu6: Implement g_enum_framesizes for isys
>  /dev/video# nodes
> 
> Implement g_enum_framesizes for isys /dev/video# nodes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index 20fd03f6f204..ad74a19527b7 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -157,6 +157,23 @@ int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>  	return -EINVAL;
>  }
>  
> +static int ipu6_isys_vidioc_g_enum_framesizes(struct file *file, void *fh,
> +					      struct v4l2_frmsizeenum *fsize)
> +{
> +	if (fsize->index > 0)
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> +	fsize->stepwise.min_width = IPU6_ISYS_MIN_WIDTH;
> +	fsize->stepwise.max_width = IPU6_ISYS_MAX_WIDTH;
> +	fsize->stepwise.min_height = IPU6_ISYS_MIN_HEIGHT;
> +	fsize->stepwise.max_height = IPU6_ISYS_MAX_HEIGHT;
> +	fsize->stepwise.step_width = 1;
> +	fsize->stepwise.step_height = 1;
> +
> +	return 0;
> +}
> +
>  static int vidioc_g_fmt_vid_cap_mplane(struct file *file, void *fh,
>  				       struct v4l2_format *fmt)
>  {
> @@ -987,6 +1004,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
>  static const struct v4l2_ioctl_ops ioctl_ops_mplane = {
>  	.vidioc_querycap = ipu6_isys_vidioc_querycap,
>  	.vidioc_enum_fmt_vid_cap = ipu6_isys_vidioc_enum_fmt,
> +	.vidioc_enum_framesizes = ipu6_isys_vidioc_g_enum_framesizes,
>  	.vidioc_g_fmt_vid_cap_mplane = vidioc_g_fmt_vid_cap_mplane,
>  	.vidioc_s_fmt_vid_cap_mplane = vidioc_s_fmt_vid_cap_mplane,
>  	.vidioc_try_fmt_vid_cap_mplane = vidioc_try_fmt_vid_cap_mplane,
> -- 
> 2.41.0
> 

> From b5db94bbd147711885986c1f6e46f59fdca10d3c Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 16:05:35 +0200
> Subject: [PATCH 3/3] media: intel/ipu6: Set V4L2_CAP_IO_MC flag for isys
>  /dev/video# nodes
> 
> The IPU6 isys is a media-controller centric device which needs
> the pipeline to be configured using the media controller API before use.
> 
> Set the V4L2_CAP_IO_MC flag to reflect this.
> 
> This also allows dropping of the enum_input() g_input() and s_input()
> implementations, with V4L2_CAP_IO_MC set the v4l2-core will take care
> of those.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 ++-----------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index ad74a19527b7..e6fc32603c3f 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -262,29 +262,6 @@ static int vidioc_try_fmt_vid_cap_mplane(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static int vidioc_enum_input(struct file *file, void *fh,
> -			     struct v4l2_input *input)
> -{
> -	if (input->index > 0)
> -		return -EINVAL;
> -	strscpy(input->name, "camera", sizeof(input->name));
> -	input->type = V4L2_INPUT_TYPE_CAMERA;
> -
> -	return 0;
> -}
> -
> -static int vidioc_g_input(struct file *file, void *fh, unsigned int *input)
> -{
> -	*input = 0;
> -
> -	return 0;
> -}
> -
> -static int vidioc_s_input(struct file *file, void *fh, unsigned int input)
> -{
> -	return input == 0 ? 0 : -EINVAL;
> -}
> -
>  static int link_validate(struct media_link *link)
>  {
>  	struct ipu6_isys_video *av =
> @@ -1017,9 +994,6 @@ static const struct v4l2_ioctl_ops ioctl_ops_mplane = {
>  	.vidioc_streamon = vb2_ioctl_streamon,
>  	.vidioc_streamoff = vb2_ioctl_streamoff,
>  	.vidioc_expbuf = vb2_ioctl_expbuf,
> -	.vidioc_enum_input = vidioc_enum_input,
> -	.vidioc_g_input = vidioc_g_input,
> -	.vidioc_s_input = vidioc_s_input,
>  };
>  
>  static const struct media_entity_operations entity_ops = {
> @@ -1217,7 +1191,8 @@ int ipu6_isys_video_init(struct ipu6_isys_video *av)
>  
>  	mutex_init(&av->mutex);
>  	av->vdev.device_caps = V4L2_CAP_STREAMING |
> -			       V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +			       V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> +			       V4L2_CAP_IO_MC;
>  	av->vdev.vfl_dir = VFL_DIR_RX;
>  
>  	ret = ipu6_isys_queue_init(&av->aq);
> -- 
> 2.41.0
> 


-- 
Regards,

Laurent Pinchart