From: Hans Verkuil <hverkuil@xs4all.nl> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>, magnus.damm@gmail.com, geert@glider.be, mchehab@kernel.org, festevam@gmail.com, sakari.ailus@iki.fi, robh+dt@kernel.org, mark.rutland@arm.com, pombredanne@nexb.com, linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, linux-sh@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 3/9] v4l: platform: Add Renesas CEU driver Date: Fri, 19 Jan 2018 12:25:39 +0000 [thread overview] Message-ID: <4195ec6a-b99d-2fad-3898-9ce9c02fce00@xs4all.nl> (raw) In-Reply-To: <2529707.jzkV7c2yGz@avalon> On 01/19/18 13:20, Laurent Pinchart wrote: > Hi Hans, > > On Friday, 19 January 2018 13:20:19 EET Hans Verkuil wrote: >> On 01/16/18 22:44, 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 <jacopo+renesas@jmondi.org> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> >>> drivers/media/platform/Kconfig | 9 + >>> drivers/media/platform/Makefile | 1 + >>> drivers/media/platform/renesas-ceu.c | 1659 +++++++++++++++++++++++++++++ >>> 3 files changed, 1669 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 0000000..1b8f0ef >>> --- /dev/null >>> +++ b/drivers/media/platform/renesas-ceu.c > > [snip] > >>> +static void ceu_update_plane_sizes(struct v4l2_plane_pix_format *plane, >>> + unsigned int bpl, unsigned int szimage) >>> +{ >>> + if (plane->bytesperline < bpl) >>> + plane->bytesperline = bpl; >>> + if (plane->sizeimage < szimage) >>> + plane->sizeimage = szimage; >> >> As mentioned in your cover letter, you do need to check for invalid >> bytesperline values. The v4l2-compliance test is to see what happens >> when userspace gives insane values, so yes, drivers need to be able >> to handle that. > > What limit would you set, what is an acceptable large value versus an invalid > large value ? I think we should have rules for this at the API level (or at > least, if not part of the API, rules that are consistent across drivers). I would expect this to be the max of what the hardware can support. If that's really high, then this can be, say, 4 times the width. Note that there are very few drivers that can handle a user-specified stride. >> plane->sizeimage is set by the driver, so drop the 'if' before the >> assignment. > > I don't think that's correct. Userspace should be able to control padding > lines at the end of the image, the same way it controls padding pixels at the > end of lines. If userspace wants larger buffers, then it should use VIDIOC_CREATE_BUFS. sizeimage is exclusively set by the driver, applications rely on that. > >>> +} > > [snip] > >>> +static const struct v4l2_ioctl_ops ceu_ioctl_ops = { >>> + .vidioc_querycap = ceu_querycap, >>> + >>> + .vidioc_enum_fmt_vid_cap_mplane = ceu_enum_fmt_vid_cap, >>> + .vidioc_try_fmt_vid_cap_mplane = ceu_try_fmt_vid_cap, >>> + .vidioc_s_fmt_vid_cap_mplane = ceu_s_fmt_vid_cap, >>> + .vidioc_g_fmt_vid_cap_mplane = ceu_g_fmt_vid_cap, >>> + >>> + .vidioc_enum_input = ceu_enum_input, >>> + .vidioc_g_input = ceu_g_input, >>> + .vidioc_s_input = ceu_s_input, >>> + >>> + .vidioc_reqbufs = vb2_ioctl_reqbufs, >>> + .vidioc_querybuf = vb2_ioctl_querybuf, >>> + .vidioc_qbuf = vb2_ioctl_qbuf, >>> + .vidioc_expbuf = vb2_ioctl_expbuf, >>> + .vidioc_dqbuf = vb2_ioctl_dqbuf, >>> + .vidioc_create_bufs = vb2_ioctl_create_bufs, >>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, >>> + .vidioc_streamon = vb2_ioctl_streamon, >>> + .vidioc_streamoff = vb2_ioctl_streamoff, >>> + >>> + .vidioc_g_parm = ceu_g_parm, >>> + .vidioc_s_parm = ceu_s_parm, >>> + .vidioc_enum_framesizes = ceu_enum_framesizes, >>> + .vidioc_enum_frameintervals = ceu_enum_frameintervals, >> >> You're missing these ioctls: >> >> .vidioc_log_status = v4l2_ctrl_log_status, > > Is log status mandatory ? No, but it doesn't hurt to add this one. It comes for free. > >> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >> .vidioc_unsubscribe_event = v4l2_event_unsubscribe, >> >> These missing _event ops are the reason for this compliance failure: >> >> fail: v4l2-test-controls.cpp(782): subscribe event for control 'User >> Controls' failed >>> +}; > > [snip] > Regards, Hans
WARNING: multiple messages have this Message-ID (diff)
From: Hans Verkuil <hverkuil@xs4all.nl> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>, magnus.damm@gmail.com, geert@glider.be, mchehab@kernel.org, festevam@gmail.com, sakari.ailus@iki.fi, robh+dt@kernel.org, mark.rutland@arm.com, pombredanne@nexb.com, linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, linux-sh@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 3/9] v4l: platform: Add Renesas CEU driver Date: Fri, 19 Jan 2018 13:25:39 +0100 [thread overview] Message-ID: <4195ec6a-b99d-2fad-3898-9ce9c02fce00@xs4all.nl> (raw) In-Reply-To: <2529707.jzkV7c2yGz@avalon> On 01/19/18 13:20, Laurent Pinchart wrote: > Hi Hans, > > On Friday, 19 January 2018 13:20:19 EET Hans Verkuil wrote: >> On 01/16/18 22:44, 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 <jacopo+renesas@jmondi.org> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> >>> drivers/media/platform/Kconfig | 9 + >>> drivers/media/platform/Makefile | 1 + >>> drivers/media/platform/renesas-ceu.c | 1659 +++++++++++++++++++++++++++++ >>> 3 files changed, 1669 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 0000000..1b8f0ef >>> --- /dev/null >>> +++ b/drivers/media/platform/renesas-ceu.c > > [snip] > >>> +static void ceu_update_plane_sizes(struct v4l2_plane_pix_format *plane, >>> + unsigned int bpl, unsigned int szimage) >>> +{ >>> + if (plane->bytesperline < bpl) >>> + plane->bytesperline = bpl; >>> + if (plane->sizeimage < szimage) >>> + plane->sizeimage = szimage; >> >> As mentioned in your cover letter, you do need to check for invalid >> bytesperline values. The v4l2-compliance test is to see what happens >> when userspace gives insane values, so yes, drivers need to be able >> to handle that. > > What limit would you set, what is an acceptable large value versus an invalid > large value ? I think we should have rules for this at the API level (or at > least, if not part of the API, rules that are consistent across drivers). I would expect this to be the max of what the hardware can support. If that's really high, then this can be, say, 4 times the width. Note that there are very few drivers that can handle a user-specified stride. >> plane->sizeimage is set by the driver, so drop the 'if' before the >> assignment. > > I don't think that's correct. Userspace should be able to control padding > lines at the end of the image, the same way it controls padding pixels at the > end of lines. If userspace wants larger buffers, then it should use VIDIOC_CREATE_BUFS. sizeimage is exclusively set by the driver, applications rely on that. > >>> +} > > [snip] > >>> +static const struct v4l2_ioctl_ops ceu_ioctl_ops = { >>> + .vidioc_querycap = ceu_querycap, >>> + >>> + .vidioc_enum_fmt_vid_cap_mplane = ceu_enum_fmt_vid_cap, >>> + .vidioc_try_fmt_vid_cap_mplane = ceu_try_fmt_vid_cap, >>> + .vidioc_s_fmt_vid_cap_mplane = ceu_s_fmt_vid_cap, >>> + .vidioc_g_fmt_vid_cap_mplane = ceu_g_fmt_vid_cap, >>> + >>> + .vidioc_enum_input = ceu_enum_input, >>> + .vidioc_g_input = ceu_g_input, >>> + .vidioc_s_input = ceu_s_input, >>> + >>> + .vidioc_reqbufs = vb2_ioctl_reqbufs, >>> + .vidioc_querybuf = vb2_ioctl_querybuf, >>> + .vidioc_qbuf = vb2_ioctl_qbuf, >>> + .vidioc_expbuf = vb2_ioctl_expbuf, >>> + .vidioc_dqbuf = vb2_ioctl_dqbuf, >>> + .vidioc_create_bufs = vb2_ioctl_create_bufs, >>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, >>> + .vidioc_streamon = vb2_ioctl_streamon, >>> + .vidioc_streamoff = vb2_ioctl_streamoff, >>> + >>> + .vidioc_g_parm = ceu_g_parm, >>> + .vidioc_s_parm = ceu_s_parm, >>> + .vidioc_enum_framesizes = ceu_enum_framesizes, >>> + .vidioc_enum_frameintervals = ceu_enum_frameintervals, >> >> You're missing these ioctls: >> >> .vidioc_log_status = v4l2_ctrl_log_status, > > Is log status mandatory ? No, but it doesn't hurt to add this one. It comes for free. > >> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, >> .vidioc_unsubscribe_event = v4l2_event_unsubscribe, >> >> These missing _event ops are the reason for this compliance failure: >> >> fail: v4l2-test-controls.cpp(782): subscribe event for control 'User >> Controls' failed >>> +}; > > [snip] > Regards, Hans
next prev parent reply other threads:[~2018-01-19 12:25 UTC|newest] Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-01-16 21:44 [PATCH v6 0/9] Renesas Capture Engine Unit (CEU) V4L2 driver Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:44 ` [PATCH v6 1/9] dt-bindings: media: Add Renesas CEU bindings Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-17 7:59 ` Sakari Ailus 2018-01-17 7:59 ` Sakari Ailus 2018-01-17 8:35 ` jacopo mondi 2018-01-17 8:35 ` jacopo mondi 2018-01-17 8:55 ` Sakari Ailus 2018-01-17 8:55 ` Sakari Ailus 2018-01-16 21:44 ` [PATCH v6 2/9] include: media: Add Renesas CEU driver interface Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:44 ` [PATCH v6 3/9] v4l: platform: Add Renesas CEU driver Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-19 11:20 ` Hans Verkuil 2018-01-19 11:20 ` Hans Verkuil 2018-01-19 12:20 ` Laurent Pinchart 2018-01-19 12:20 ` Laurent Pinchart 2018-01-19 12:25 ` Hans Verkuil [this message] 2018-01-19 12:25 ` Hans Verkuil [not found] ` <4195ec6a-b99d-2fad-3898-9ce9c02fce00-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> 2018-01-22 0:52 ` Laurent Pinchart 2018-01-22 0:52 ` Laurent Pinchart 2018-01-22 0:52 ` Laurent Pinchart 2018-01-22 9:07 ` Hans Verkuil 2018-01-22 9:07 ` Hans Verkuil 2018-01-21 9:53 ` jacopo mondi 2018-01-21 9:53 ` jacopo mondi 2018-01-21 10:21 ` Hans Verkuil 2018-01-21 10:21 ` Hans Verkuil 2018-01-21 10:23 ` Hans Verkuil 2018-01-21 10:23 ` Hans Verkuil 2018-01-21 17:29 ` jacopo mondi 2018-01-21 17:29 ` jacopo mondi 2018-01-22 9:56 ` Hans Verkuil 2018-01-22 9:56 ` Hans Verkuil 2018-01-19 22:35 ` kbuild test robot 2018-01-19 22:35 ` kbuild test robot 2018-01-19 22:35 ` kbuild test robot 2018-01-20 2:11 ` kbuild test robot 2018-01-20 2:11 ` kbuild test robot 2018-01-20 2:11 ` kbuild test robot 2018-01-16 21:44 ` [PATCH v6 4/9] ARM: dts: r7s72100: Add Capture Engine Unit (CEU) Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:44 ` [PATCH v6 5/9] v4l: i2c: Copy ov772x soc_camera sensor driver Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:44 ` [PATCH v6 6/9] media: i2c: ov772x: Remove soc_camera dependencies Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-19 10:24 ` Hans Verkuil 2018-01-19 10:24 ` Hans Verkuil [not found] ` <d67c21e5-2488-977b-39d8-561048409209-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> 2018-01-19 10:47 ` Hans Verkuil 2018-01-19 10:47 ` Hans Verkuil 2018-01-19 10:47 ` Hans Verkuil 2018-01-19 11:19 ` Sakari Ailus 2018-01-19 11:19 ` Sakari Ailus 2018-01-19 11:44 ` Hans Verkuil 2018-01-19 11:44 ` Hans Verkuil [not found] ` <20180119111917.76wosrokgracbdrz-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org> 2018-01-19 12:23 ` Laurent Pinchart 2018-01-19 12:23 ` Laurent Pinchart 2018-01-19 12:23 ` Laurent Pinchart 2018-01-21 9:31 ` jacopo mondi 2018-01-21 9:31 ` jacopo mondi 2018-01-21 10:18 ` Hans Verkuil 2018-01-21 10:18 ` Hans Verkuil 2018-01-16 21:44 ` [PATCH v6 7/9] v4l: i2c: Copy tw9910 soc_camera sensor driver Jacopo Mondi 2018-01-16 21:44 ` Jacopo Mondi 2018-01-16 21:45 ` [PATCH v6 8/9] media: i2c: tw9910: Remove soc_camera dependencies Jacopo Mondi 2018-01-16 21:45 ` Jacopo Mondi 2018-01-16 21:45 ` [PATCH v6 9/9] arch: sh: migor: Use new renesas-ceu camera driver Jacopo Mondi 2018-01-16 21:45 ` Jacopo Mondi 2018-01-19 11:26 ` [PATCH v6 0/9] Renesas Capture Engine Unit (CEU) V4L2 driver Hans Verkuil 2018-01-19 11:26 ` Hans Verkuil [not found] ` <6fcd22c1-19a5-e0b7-2b00-961e1cd1ebaa-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> 2018-01-21 17:49 ` jacopo mondi 2018-01-21 17:49 ` jacopo mondi 2018-01-21 17:49 ` jacopo mondi 2018-01-22 10:00 ` Hans Verkuil 2018-01-22 10:00 ` Hans Verkuil 2018-01-22 10:00 ` Hans Verkuil 2018-01-19 22:12 ` Randy Dunlap 2018-01-19 22:12 ` Randy Dunlap 2018-01-21 17:54 ` jacopo mondi 2018-01-21 17:54 ` jacopo mondi 2018-01-21 18:04 ` Randy Dunlap 2018-01-21 18:04 ` Randy Dunlap
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4195ec6a-b99d-2fad-3898-9ce9c02fce00@xs4all.nl \ --to=hverkuil@xs4all.nl \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=geert@glider.be \ --cc=jacopo+renesas@jmondi.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ --cc=magnus.damm@gmail.com \ --cc=mark.rutland@arm.com \ --cc=mchehab@kernel.org \ --cc=pombredanne@nexb.com \ --cc=robh+dt@kernel.org \ --cc=sakari.ailus@iki.fi \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.