Re: [PATCH v5 3/9] v4l: platform: Add Renesas CEU driver

[jacopo mondi]

Hi Hans,

On Tue, Jan 16, 2018 at 10:46:42AM +0100, Hans Verkuil wrote:
> Hi Jacopo,
>
> Sorry for the late review, but here is finally is.
>
> BTW, can you provide the v4l2-compliance output (ideally with the -f option)
> in the cover letter for v6?

Sure, it was attacched to v3 I guess, since then it has not changed.
I will include that in v6 cover letter.

> On 01/12/2018 03:04 PM, Jacopo Mondi wrote:
> > Add driver for Renesas Capture Engine Unit (CEU).
> >
> > The CEU interface supports capturing 'data' (YUV422) and 'images'
> > (NV[12|21|16|61]).
> >
> > This driver aims to replace the soc_camera-based sh_mobile_ceu one.
> >
> > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> > platform GR-Peach.
> >
> > Tested with ov7725 camera sensor on SH4 platform Migo-R.
> >
> > Signed-off-by: Jacopo Mondi 
> > Reviewed-by: Laurent Pinchart 
> > ---
> >  drivers/media/platform/Kconfig   |9 +
> >  drivers/media/platform/Makefile  |1 +
> >  drivers/media/platform/renesas-ceu.c | 1648 
> > ++
> >  3 files changed, 1658 insertions(+)
> >  create mode 100644 drivers/media/platform/renesas-ceu.c
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index fd0c998..fe7bd26 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI
> >   To compile this driver as a module, choose M here: the module
> >   will be called stm32-dcmi.
> >
> > +config VIDEO_RENESAS_CEU
> > +   tristate "Renesas Capture Engine Unit (CEU) driver"
> > +   depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
> > +   depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST
> > +   select VIDEOBUF2_DMA_CONTIG
> > +   select V4L2_FWNODE
> > +   ---help---
> > + This is a v4l2 driver for the Renesas CEU Interface
> > +
> >  source "drivers/media/platform/soc_camera/Kconfig"
> >  source "drivers/media/platform/exynos4-is/Kconfig"
> >  source "drivers/media/platform/am437x/Kconfig"
> > diff --git a/drivers/media/platform/Makefile 
> > b/drivers/media/platform/Makefile
> > index 003b0bb..6580a6b 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_SH_VOU)+= sh_vou.o
> >  obj-$(CONFIG_SOC_CAMERA)   += soc_camera/
> >
> >  obj-$(CONFIG_VIDEO_RCAR_DRIF)  += rcar_drif.o
> > +obj-$(CONFIG_VIDEO_RENESAS_CEU)+= renesas-ceu.o
> >  obj-$(CONFIG_VIDEO_RENESAS_FCP)+= rcar-fcp.o
> >  obj-$(CONFIG_VIDEO_RENESAS_FDP1)   += rcar_fdp1.o
> >  obj-$(CONFIG_VIDEO_RENESAS_JPU)+= rcar_jpu.o
> > diff --git a/drivers/media/platform/renesas-ceu.c 
> > b/drivers/media/platform/renesas-ceu.c
> > new file mode 100644
> > index 000..ccca838
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas-ceu.c
>
> 
>
> > +/*
> > + * ceu_vb2_setup() - is called to check whether the driver can accept the
> > + *  requested number of buffers and to fill in plane sizes
> > + *  for the current frame format, if required.
> > + */
> > +static int ceu_vb2_setup(struct vb2_queue *vq, unsigned int *count,
> > +unsigned int *num_planes, unsigned int sizes[],
> > +struct device *alloc_devs[])
> > +{
> > +   struct ceu_device *ceudev = vb2_get_drv_priv(vq);
> > +   struct v4l2_pix_format_mplane *pix = >v4l2_pix;
> > +   unsigned int i;
> > +
> > +   if (!*count)
> > +   *count = 2;
>
> Don't do this. Instead set the min_buffers_needed field to 2 in the vb2_queue
> struct.
>

I guess setting min_buffers_needed makes this check not required. Will
drop.

> > +
> > +   /* num_planes is set: just check plane sizes. */
> > +   if (*num_planes) {
> > +   for (i = 0; i < pix->num_planes; i++)
> > +   if (sizes[i] < pix->plane_fmt[i].sizeimage)
> > +   return -EINVAL;
> > +
> > +   return 0;
> > +   }
> > +
> > +   /* num_planes not set: called from REQBUFS, just set plane sizes. */
> > +   *num_planes = pix->num_planes;
> > +   for (i = 0; i < pix->num_planes; i++)
> > +   sizes[i] = pix->plane_fmt[i].sizeimage;
> > +
> > +   return 0;
> > +}
> > +
> > +static void ceu_vb2_queue(struct vb2_buffer *vb)
> > +{
> > +   struct ceu_device *ceudev = vb2_get_drv_priv(vb->vb2_queue);
> > +   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +   struct v4l2_pix_format_mplane *pix = >v4l2_pix;
> > +   struct ceu_buffer *buf = vb2_to_ceu(vbuf);
> > +   unsigned long irqflags;
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < pix->num_planes; i++) {
> > +   if (vb2_plane_size(vb, i) < pix->plane_fmt[i].sizeimage) {
> > +   vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> > +   return;
> > +   }
> > +
> > +   

Re: [PATCH v5 3/9] v4l: platform: Add Renesas CEU driver

[Laurent Pinchart]

Hi Hans,

On Tuesday, 16 January 2018 11:46:42 EET Hans Verkuil wrote:
> Hi Jacopo,
> 
> Sorry for the late review, but here is finally is.
> 
> BTW, can you provide the v4l2-compliance output (ideally with the -f option)
> in the cover letter for v6?
> 
> On 01/12/2018 03:04 PM, Jacopo Mondi wrote:
> > Add driver for Renesas Capture Engine Unit (CEU).
> > 
> > The CEU interface supports capturing 'data' (YUV422) and 'images'
> > (NV[12|21|16|61]).
> > 
> > This driver aims to replace the soc_camera-based sh_mobile_ceu one.
> > 
> > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> > platform GR-Peach.
> > 
> > Tested with ov7725 camera sensor on SH4 platform Migo-R.
> > 
> > Signed-off-by: Jacopo Mondi 
> > Reviewed-by: Laurent Pinchart 
> > ---
> > 
> >  drivers/media/platform/Kconfig   |9 +
> >  drivers/media/platform/Makefile  |1 +
> >  drivers/media/platform/renesas-ceu.c | 1648 +
> >  3 files changed, 1658 insertions(+)
> >  create mode 100644 drivers/media/platform/renesas-ceu.c

[snip]

> > diff --git a/drivers/media/platform/renesas-ceu.c
> > b/drivers/media/platform/renesas-ceu.c new file mode 100644
> > index 000..ccca838
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas-ceu.c

[snip]

> > +static int ceu_s_input(struct file *file, void *priv, unsigned int i)
> > +{
> > +   struct ceu_device *ceudev = video_drvdata(file);
> > +   struct ceu_subdev *ceu_sd_old;
> > +   int ret;
> > +
> 
> Add a check:
> 
>   if (i == ceudev->sd_index)
>   return 0;
> 
> I.e. if the new input == the old input, then that's fine regardless of the
> streaming state.

On a side note this is the kind of checks that the core should handle, but 
that's out of scope for this patch.

> > +   if (vb2_is_streaming(>vb2_vq))
> > +   return -EBUSY;
> > +
> > +   if (i >= ceudev->num_sd)
> > +   return -EINVAL;
> 
> Move this up as the first test.
> 
> > +
> > +   ceu_sd_old = ceudev->sd;
> > +   ceudev->sd = >subdevs[i];
> > +
> > +   /* Make sure we can generate output image formats. */
> > +   ret = ceu_init_formats(ceudev);
> > +   if (ret) {
> > +   ceudev->sd = ceu_sd_old;
> > +   return -EINVAL;
> > +   }
> > +
> > +   /* now that we're sure we can use the sensor, power off the old one. */
> > +   v4l2_subdev_call(ceu_sd_old->v4l2_sd, core, s_power, 0);
> > +   v4l2_subdev_call(ceudev->sd->v4l2_sd, core, s_power, 1);
> > +
> > +   ceudev->sd_index = i;
> > +
> > +   return 0;
> > +}

[snip]

> > +
> > +static int ceu_notify_complete(struct v4l2_async_notifier *notifier)
> > +{
> > +   struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > +   struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > +   struct video_device *vdev = >vdev;
> > +   struct vb2_queue *q = >vb2_vq;
> > +   struct v4l2_subdev *v4l2_sd;
> > +   int ret;
> > +
> > +   /* Initialize vb2 queue. */
> > +   q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > +   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> 
> Don't include VB2_USERPTR. It shouldn't be used with dma_contig.
> 
> You also added a read() fop (vb2_fop_read), so either add VB2_READ here
> or remove the read fop.

Agreed. I'd drop both VB2_USERPTR and vb2_fop_read().

> > +   q->drv_priv = ceudev;
> > +   q->ops  = _vb2_ops;
> > +   q->mem_ops  = _dma_contig_memops;
> > +   q->buf_struct_size  = sizeof(struct ceu_buffer);
> > +   q->timestamp_flags  = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +   q->lock = >mlock;
> > +   q->dev  = ceudev->v4l2_dev.dev;
> > +
> > +   ret = vb2_queue_init(q);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /*
> > +* Make sure at least one sensor is primary and use it to initialize
> > +* ceu formats.
> > +*/
> > +   if (!ceudev->sd) {
> > +   ceudev->sd = >subdevs[0];
> > +   ceudev->sd_index = 0;
> > +   }
> > +
> > +   v4l2_sd = ceudev->sd->v4l2_sd;
> > +
> > +   ret = ceu_init_formats(ceudev);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = ceu_set_default_fmt(ceudev);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Register the video device. */
> > +   strncpy(vdev->name, DRIVER_NAME, strlen(DRIVER_NAME));
> > +   vdev->v4l2_dev  = v4l2_dev;
> > +   vdev->lock  = >mlock;
> > +   vdev->queue = >vb2_vq;
> > +   vdev->ctrl_handler  = v4l2_sd->ctrl_handler;
> > +   vdev->fops  = _fops;
> > +   vdev->ioctl_ops = _ioctl_ops;
> > +   vdev->release   = ceu_vdev_release;
> > +   vdev->device_caps   = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> > + V4L2_CAP_STREAMING;
> > +   video_set_drvdata(vdev, ceudev);
> > +
> > +   ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > +   if (ret < 0) {
> > +   

Re: [PATCH v5 3/9] v4l: platform: Add Renesas CEU driver

[Hans Verkuil]

Hi Jacopo,

Sorry for the late review, but here is finally is.

BTW, can you provide the v4l2-compliance output (ideally with the -f option)
in the cover letter for v6?

On 01/12/2018 03:04 PM, Jacopo Mondi wrote:
> Add driver for Renesas Capture Engine Unit (CEU).
> 
> The CEU interface supports capturing 'data' (YUV422) and 'images'
> (NV[12|21|16|61]).
> 
> This driver aims to replace the soc_camera-based sh_mobile_ceu one.
> 
> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> platform GR-Peach.
> 
> Tested with ov7725 camera sensor on SH4 platform Migo-R.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Laurent Pinchart 
> ---
>  drivers/media/platform/Kconfig   |9 +
>  drivers/media/platform/Makefile  |1 +
>  drivers/media/platform/renesas-ceu.c | 1648 
> ++
>  3 files changed, 1658 insertions(+)
>  create mode 100644 drivers/media/platform/renesas-ceu.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index fd0c998..fe7bd26 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI
> To compile this driver as a module, choose M here: the module
> will be called stm32-dcmi.
>  
> +config VIDEO_RENESAS_CEU
> + tristate "Renesas Capture Engine Unit (CEU) driver"
> + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
> + depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + ---help---
> +   This is a v4l2 driver for the Renesas CEU Interface
> +
>  source "drivers/media/platform/soc_camera/Kconfig"
>  source "drivers/media/platform/exynos4-is/Kconfig"
>  source "drivers/media/platform/am437x/Kconfig"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 003b0bb..6580a6b 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_VIDEO_SH_VOU)  += sh_vou.o
>  obj-$(CONFIG_SOC_CAMERA) += soc_camera/
>  
>  obj-$(CONFIG_VIDEO_RCAR_DRIF)+= rcar_drif.o
> +obj-$(CONFIG_VIDEO_RENESAS_CEU)  += renesas-ceu.o
>  obj-$(CONFIG_VIDEO_RENESAS_FCP)  += rcar-fcp.o
>  obj-$(CONFIG_VIDEO_RENESAS_FDP1) += rcar_fdp1.o
>  obj-$(CONFIG_VIDEO_RENESAS_JPU)  += rcar_jpu.o
> diff --git a/drivers/media/platform/renesas-ceu.c 
> b/drivers/media/platform/renesas-ceu.c
> new file mode 100644
> index 000..ccca838
> --- /dev/null
> +++ b/drivers/media/platform/renesas-ceu.c



> +/*
> + * ceu_vb2_setup() - is called to check whether the driver can accept the
> + *requested number of buffers and to fill in plane sizes
> + *for the current frame format, if required.
> + */
> +static int ceu_vb2_setup(struct vb2_queue *vq, unsigned int *count,
> +  unsigned int *num_planes, unsigned int sizes[],
> +  struct device *alloc_devs[])
> +{
> + struct ceu_device *ceudev = vb2_get_drv_priv(vq);
> + struct v4l2_pix_format_mplane *pix = >v4l2_pix;
> + unsigned int i;
> +
> + if (!*count)
> + *count = 2;

Don't do this. Instead set the min_buffers_needed field to 2 in the vb2_queue
struct.

> +
> + /* num_planes is set: just check plane sizes. */
> + if (*num_planes) {
> + for (i = 0; i < pix->num_planes; i++)
> + if (sizes[i] < pix->plane_fmt[i].sizeimage)
> + return -EINVAL;
> +
> + return 0;
> + }
> +
> + /* num_planes not set: called from REQBUFS, just set plane sizes. */
> + *num_planes = pix->num_planes;
> + for (i = 0; i < pix->num_planes; i++)
> + sizes[i] = pix->plane_fmt[i].sizeimage;
> +
> + return 0;
> +}
> +
> +static void ceu_vb2_queue(struct vb2_buffer *vb)
> +{
> + struct ceu_device *ceudev = vb2_get_drv_priv(vb->vb2_queue);
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct v4l2_pix_format_mplane *pix = >v4l2_pix;
> + struct ceu_buffer *buf = vb2_to_ceu(vbuf);
> + unsigned long irqflags;
> + unsigned int i;
> +
> + for (i = 0; i < pix->num_planes; i++) {
> + if (vb2_plane_size(vb, i) < pix->plane_fmt[i].sizeimage) {
> + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> + return;
> + }
> +
> + vb2_set_plane_payload(vb, i, pix->plane_fmt[i].sizeimage);

This is not the right vb2 op for this test, this belongs in the buf_prepare
op. There you can just return an error and you don't need to call buffer_done.

> + }
> +
> + spin_lock_irqsave(>lock, irqflags);
> + list_add_tail(>queue, >capture);
> + spin_unlock_irqrestore(>lock, irqflags);
> +}
> +
> +static int ceu_start_streaming(struct vb2_queue *vq,