Re: [PATCH v2 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function
Hi Shuah, On 2017-02-22 19:07, Shuah Khan wrote: On Mon, Feb 20, 2017 at 6:38 AM, Marek Szyprowski wrote: To complete DMA memory configuration for MFC device, allocation of the firmware buffer is needed, because some parameters are dependant on its base address. Till now, this has been handled in the s5p_mfc_alloc_firmware() function. This patch moves that logic to s5p_mfc_configure_dma_memory() to keep DMA memory related operations in a single place. This way s5p_mfc_alloc_firmware() is simplified and does what it name says. The other consequence of this change is moving s5p_mfc_alloc_firmware() call from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory(). Overall looks good. This patch makes subtle change in the dma and firwmare initialization sequence. Might be okay, but wanted to call out just in case, Before this change: vb2_dma_contig_set_max_seg_size() is done for both iommu and non-iommu case before s5p_mfc_alloc_firmware(). With this change setting dma_contig max size happens after s5p_mfc_alloc_firmware(). From what I can tell this might not be an issue. vb2_dma_contig_clear_max_seg_size() still happens after s5p_mfc_release_firmware(), so that part hasn't changed. Do any of the dma_* calls made from s5p_mfc_alloc_firmware() and later during the dma congiguration sequence depend on dmap_parms being allocated? Doesn't looks like it from what I can tell, but safe to ask. Firmware allocation doesn't depend on dma max segment size at all. The only calls which might depend on it are dma_map_sg(), which are performed much later as a part of video buffer allocation/preparation in videobuf2, when dma-buf or user pointer v4l2 modes are selected. > [...] Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
Re: [PATCH] dma-buf: add support for compat ioctl
Hi Marek, On 23 February 2017 at 00:37, Daniel Vetter wrote: > On Tue, Feb 21, 2017 at 4:08 PM, Christian König > wrote: >> Am 21.02.2017 um 15:55 schrieb Marek Szyprowski: >>> >>> Dear All, >>> >>> On 2017-02-21 15:37, Marek Szyprowski wrote: Hi Christian, On 2017-02-21 14:59, Christian König wrote: > > Am 21.02.2017 um 14:21 schrieb Marek Szyprowski: >> >> Add compat ioctl support to dma-buf. This lets one to use >> DMA_BUF_IOCTL_SYNC >> ioctl from 32bit application on 64bit kernel. Data structures for both >> 32 >> and 64bit modes are same, so there is no need for additional >> translation >> layer. > > > Well I might be wrong, but IIRC compat_ioctl was just optional and if > not specified unlocked_ioctl was called instead. > > If that is true your patch wouldn't have any effect at all. Well, then why I got -ENOTTY in the 32bit test app for this ioctl on 64bit ARM64 kernel without this patch? >>> >>> I've checked in fs/compat_ioctl.c, I see no fallback in >>> COMPAT_SYSCALL_DEFINE3, >>> so one has to provide compat_ioctl callback to have ioctl working with >>> 32bit >>> apps. >> >> >> Then my memory cheated on me. >> >> In this case the patch is Reviewed-by: Christian König >> . > Thanks much for spotting this! > Since you have commit rights for drm-misc, care to push this to > drm-misc-next-fixes pls? Also I think this warrants a cc: stable, > clearly an obvious screw-up in creating this api on our side :( So > feel free to smash my ack on the patch. > Daniel, Christian, I saw this just now, so if Christian hasn't already pulled it into drm-misc-next-fixes, I'll give it a stab. > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel Best, Sumit. -- Thanks and regards, Sumit Semwal Linaro Mobile Group - Kernel Team Lead Linaro.org │ Open source software for ARM SoCs
cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Thu Feb 23 05:00:35 CET 2017 media-tree git hash:e6b377dbbb944d5e3ceef4e5d429fc5c841e3692 media_build git hash: 785cdf7f0798964681b33aad44fc2ff4d734733d v4l-utils git hash: 27b8492c4b9e33f2080a69364ebe9a1a4e92601d gcc version:i686-linux-gcc (GCC) 6.2.0 sparse version: v0.5.0-3553-g78b2ea6 smatch version: v0.5.0-3553-g78b2ea6 host hardware: x86_64 host os:4.9.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: ERRORS linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: WARNINGS linux-2.6.37.6-i686: WARNINGS linux-2.6.38.8-i686: WARNINGS linux-2.6.39.4-i686: WARNINGS linux-3.0.60-i686: WARNINGS linux-3.1.10-i686: WARNINGS linux-3.2.37-i686: WARNINGS linux-3.3.8-i686: WARNINGS linux-3.4.27-i686: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.7.4-i686: WARNINGS linux-3.8-i686: WARNINGS linux-3.9.2-i686: WARNINGS linux-3.10.1-i686: WARNINGS linux-3.11.1-i686: WARNINGS linux-3.12.67-i686: WARNINGS linux-3.13.11-i686: WARNINGS linux-3.14.9-i686: WARNINGS linux-3.15.2-i686: WARNINGS linux-3.16.7-i686: WARNINGS linux-3.17.8-i686: WARNINGS linux-3.18.7-i686: WARNINGS linux-3.19-i686: WARNINGS linux-4.0.9-i686: WARNINGS linux-4.1.33-i686: WARNINGS linux-4.2.8-i686: WARNINGS linux-4.3.6-i686: WARNINGS linux-4.4.22-i686: WARNINGS linux-4.5.7-i686: WARNINGS linux-4.6.7-i686: WARNINGS linux-4.7.5-i686: WARNINGS linux-4.8-i686: OK linux-4.9-i686: OK linux-4.10-rc3-i686: OK linux-2.6.36.4-x86_64: WARNINGS linux-2.6.37.6-x86_64: WARNINGS linux-2.6.38.8-x86_64: WARNINGS linux-2.6.39.4-x86_64: WARNINGS linux-3.0.60-x86_64: WARNINGS linux-3.1.10-x86_64: WARNINGS linux-3.2.37-x86_64: WARNINGS linux-3.3.8-x86_64: WARNINGS linux-3.4.27-x86_64: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.4-x86_64: WARNINGS linux-3.8-x86_64: WARNINGS linux-3.9.2-x86_64: WARNINGS linux-3.10.1-x86_64: WARNINGS linux-3.11.1-x86_64: WARNINGS linux-3.12.67-x86_64: WARNINGS linux-3.13.11-x86_64: WARNINGS linux-3.14.9-x86_64: WARNINGS linux-3.15.2-x86_64: WARNINGS linux-3.16.7-x86_64: WARNINGS linux-3.17.8-x86_64: WARNINGS linux-3.18.7-x86_64: WARNINGS linux-3.19-x86_64: WARNINGS linux-4.0.9-x86_64: WARNINGS linux-4.1.33-x86_64: WARNINGS linux-4.2.8-x86_64: WARNINGS linux-4.3.6-x86_64: WARNINGS linux-4.4.22-x86_64: WARNINGS linux-4.5.7-x86_64: WARNINGS linux-4.6.7-x86_64: WARNINGS linux-4.7.5-x86_64: WARNINGS linux-4.8-x86_64: OK linux-4.9-x86_64: OK linux-4.10-rc3-x86_64: OK apps: WARNINGS spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: Looking more details and reasons for using orig_add_limit.
Hi Mauro, On Wednesday 22 Feb 2017 17:25:41 Mauro Carvalho Chehab wrote: > Em Wed, 22 Feb 2017 21:53:08 +0200 Laurent Pinchart escreveu: > > On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote: > >> Hi mchehab/linux-media, > >> > >> It is not clear why KERNEL_DS was set explicitly here. In this path > >> video_usercopy() gets called and it > >> copies the “struct v4l2_buffer” struct to user space stack memory. > >> > >> Can you please share reasons for setting to KERNEL_DS here? > > > > It's a bit of historical hack. To implement compat ioctl handling, we copy > > the ioctl 32-bit argument from userspace, turn it into a native 64-bit > > ioctl argument, and call the native ioctl code. That code expects the > > argument to be stored in userspace memory and uses get_user() and > > put_user() to access it. As the 64-bit argument now lives in kernel > > memory, my understanding is that we fake things up with KERNEL_DS. > > Precisely. Actually, if I remember well, this was needed to pass pointer > arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of > V4L2 ioctls that pass structures with pointers on it. Setting DS cause > those pointers to do the right thing, but yeah, it is hackish. We should restructure the core ioctl code to decouple copy from/to user and ioctl execution (this might just be a matter of exporting a currently static function), and change the compat code to perform the copy/from to user directly when converting between 32-bit and 64-bit structures (dropping all the alloc in userspace hacks) and call the ioctl execution handler. That will fix the problem. Any volunteer ? :-) > This used to work fine on x86_64 (when such code was written e. g. Kernel > 2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we > received some patches fixing support for some ioctl compat code due to > x86_64/arm64 differences in the past. > > On what Kernel version it started to cause troubles? 4.9? If so, then > maybe the breakage is a side effect of VM stack changes. > > > The ioctl code should be refactored to get rid of this hack. > > Agreed. -- Regards, Laurent Pinchart
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On 02/22/2017 04:06 PM, Steve Longerbeam wrote: On 02/17/2017 03:06 AM, Russell King - ARM Linux wrote: On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: +static void csi2_dphy_init(struct csi2_dev *csi2) +{ +/* + * FIXME: 0x14 is derived from a fixed D-PHY reference + * clock from the HSI_TX PLL, and a fixed target lane max + * bandwidth of 300 Mbps. This value should be derived If the table in https://community.nxp.com/docs/DOC-94312 is correct, this should be 850 Mbps. Where does this 300 Mbps value come from? I thought you had some code to compute the correct value, although I guess we've lost the ability to know how fast the sensor is going to drive the link. Note that the IMX219 currently drives the data lanes at 912Mbps almost exclusively, as I've yet to finish working out how to derive the PLL parameters. (I have something that works, but it currently takes on the order of 100k iterations to derive the parameters. gcd() doesn't help you in this instance.) Hi Russell, As I mentioned, I've added code to imx6-mipi-csi2 to determine the sources link frequency via V4L2_CID_LINK_FREQ. If you were to implement this control and return 912 Mbps-per-lane, argh, I mean return 912 / 2. Steve the D-PHY will be programmed correctly for the IMX219 (at least, that is the theory anyway). Alternatively, I could up the default in imx6-mipi-csi2 to 950 Mbps. I will have to test that to make sure it still works with OV5640 and tc358743. Steve
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On 02/17/2017 03:06 AM, Russell King - ARM Linux wrote: On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: +static void csi2_dphy_init(struct csi2_dev *csi2) +{ + /* +* FIXME: 0x14 is derived from a fixed D-PHY reference +* clock from the HSI_TX PLL, and a fixed target lane max +* bandwidth of 300 Mbps. This value should be derived If the table in https://community.nxp.com/docs/DOC-94312 is correct, this should be 850 Mbps. Where does this 300 Mbps value come from? I thought you had some code to compute the correct value, although I guess we've lost the ability to know how fast the sensor is going to drive the link. Note that the IMX219 currently drives the data lanes at 912Mbps almost exclusively, as I've yet to finish working out how to derive the PLL parameters. (I have something that works, but it currently takes on the order of 100k iterations to derive the parameters. gcd() doesn't help you in this instance.) Hi Russell, As I mentioned, I've added code to imx6-mipi-csi2 to determine the sources link frequency via V4L2_CID_LINK_FREQ. If you were to implement this control and return 912 Mbps-per-lane, the D-PHY will be programmed correctly for the IMX219 (at least, that is the theory anyway). Alternatively, I could up the default in imx6-mipi-csi2 to 950 Mbps. I will have to test that to make sure it still works with OV5640 and tc358743. Steve
Re: [PATCH v4 17/36] media: Add userspace header file for i.MX
On 02/16/2017 03:33 AM, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: +/* + * events from the subdevs + */ +#define V4L2_EVENT_IMX_CLASS V4L2_EVENT_PRIVATE_START +#define V4L2_EVENT_IMX_NFB4EOF(V4L2_EVENT_IMX_CLASS + 1) +#define V4L2_EVENT_IMX_FRAME_INTERVAL (V4L2_EVENT_IMX_CLASS + 2) These events are still i.MX specific. I think they shouldn't be. Done, I've exported them to V4L2_EVENT_FRAME_INTERVAL_ERROR V4L2_EVENT_NEW_FRAME_BEFORE_EOF Steve
Re: [PATCH v4 33/36] media: imx: redo pixel format enumeration and negotiation
Hi Philipp, On 02/16/2017 03:32 AM, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: The previous API and negotiation of mbus codes and pixel formats was broken, and has been completely redone. The negotiation of media bus codes should be as follows: CSI: sink pad direct src pad IDMAC src pad - RGB (any)IPU RGB RGB (any) YUV (any)IPU YUV YUV (any) Bayer N/A must be same bayer code as sink The IDMAC src pad should also use the internal 32-bit RGB / YUV format, except if bayer/raw mode is selected, in which case the attached capture video device should only allow a single mode corresponding to the output pad media bus format. The IDMAC source pad is going to memory, so it has left the IPU. Are you sure it should be an internal IPU format? I realize it is linked to a capture device node, and the IPU format could then be translated to a v4l2 fourcc by the capture device, but IMHO this pad is external to the IPU. Steve
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On 02/22/2017 03:38 PM, Steve Longerbeam wrote: On 02/17/2017 03:38 AM, Philipp Zabel wrote: On Fri, 2017-02-17 at 11:06 +, Russell King - ARM Linux wrote: On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: +static void csi2_dphy_init(struct csi2_dev *csi2) +{ +/* + * FIXME: 0x14 is derived from a fixed D-PHY reference + * clock from the HSI_TX PLL, and a fixed target lane max + * bandwidth of 300 Mbps. This value should be derived If the table in https://community.nxp.com/docs/DOC-94312 is correct, this should be 850 Mbps. Where does this 300 Mbps value come from? I thought you had some code to compute the correct value, although I guess we've lost the ability to know how fast the sensor is going to drive the link. I had code to calculate the number of needed lanes from the bit rate and link frequency. I did not actually change the D-PHY register value. And as you pointed out, calculating the number of lanes is not useful without input from the sensor driver, as some lane configurations might not be supported. Note that the IMX219 currently drives the data lanes at 912Mbps almost exclusively, as I've yet to finish working out how to derive the PLL parameters. (I have something that works, but it currently takes on the order of 100k iterations to derive the parameters. gcd() doesn't help you in this instance.) The tc358743 also currently only implements a fixed rate (of 594 Mbps). I've analyzed the OV5640 video modes, and they generate the following "sysclk"'s in Mhz: 210 280 420 560 840 But I don't know whether this is equivalent to bit rate. Is it the same as Mbps-per-lane? If so, this could be indicated to the sink by implementing V4L2_CID_LINK_FREQ in the ov5640.c sensor driver. er, rather, if I knew what this "sysclk" value was, I could use it to convert to a link frequency and return it in V4L2_CID_LINK_FREQ. Steve The Mbps-per-lane value would then be link_freq * 2, and then Mbps-per-lane could be used to lookup the correct "hsfreqsel" value to program the D-PHY. I've added this to imx6-mipi-csi2.c. If the source didn't implement V4L2_CID_LINK_FREQ then it uses a default 849 Mbps-per-lane. Steve
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On 02/17/2017 03:38 AM, Philipp Zabel wrote: On Fri, 2017-02-17 at 11:06 +, Russell King - ARM Linux wrote: On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote: On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: +static void csi2_dphy_init(struct csi2_dev *csi2) +{ + /* +* FIXME: 0x14 is derived from a fixed D-PHY reference +* clock from the HSI_TX PLL, and a fixed target lane max +* bandwidth of 300 Mbps. This value should be derived If the table in https://community.nxp.com/docs/DOC-94312 is correct, this should be 850 Mbps. Where does this 300 Mbps value come from? I thought you had some code to compute the correct value, although I guess we've lost the ability to know how fast the sensor is going to drive the link. I had code to calculate the number of needed lanes from the bit rate and link frequency. I did not actually change the D-PHY register value. And as you pointed out, calculating the number of lanes is not useful without input from the sensor driver, as some lane configurations might not be supported. Note that the IMX219 currently drives the data lanes at 912Mbps almost exclusively, as I've yet to finish working out how to derive the PLL parameters. (I have something that works, but it currently takes on the order of 100k iterations to derive the parameters. gcd() doesn't help you in this instance.) The tc358743 also currently only implements a fixed rate (of 594 Mbps). I've analyzed the OV5640 video modes, and they generate the following "sysclk"'s in Mhz: 210 280 420 560 840 But I don't know whether this is equivalent to bit rate. Is it the same as Mbps-per-lane? If so, this could be indicated to the sink by implementing V4L2_CID_LINK_FREQ in the ov5640.c sensor driver. The Mbps-per-lane value would then be link_freq * 2, and then Mbps-per-lane could be used to lookup the correct "hsfreqsel" value to program the D-PHY. I've added this to imx6-mipi-csi2.c. If the source didn't implement V4L2_CID_LINK_FREQ then it uses a default 849 Mbps-per-lane. Steve
[PATCH] [media] rc: raw decoder for keymap protocol is not loaded on register
When the protocol is set via the sysfs protocols attribute, the decoder is loaded. However, when it is not when a device is first plugged in or registered. Fixes: acc1c3c ("[media] media: rc: load decoder modules on-demand") Signed-off-by: Sean Young Cc: # v4.5+ --- drivers/media/rc/rc-main.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 2424946..a158b32 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -1663,6 +1663,7 @@ static int rc_setup_rx_device(struct rc_dev *dev) { int rc; struct rc_map *rc_map; + u64 rc_type; if (!dev->map_name) return -EINVAL; @@ -1677,15 +1678,18 @@ static int rc_setup_rx_device(struct rc_dev *dev) if (rc) return rc; - if (dev->change_protocol) { - u64 rc_type = (1ll << rc_map->rc_type); + rc_type = BIT_ULL(rc_map->rc_type); + if (dev->change_protocol) { rc = dev->change_protocol(dev, &rc_type); if (rc < 0) goto out_table; dev->enabled_protocols = rc_type; } + if (dev->driver_type == RC_DRIVER_IR_RAW) + ir_raw_load_modules(&rc_type); + set_bit(EV_KEY, dev->input_dev->evbit); set_bit(EV_REP, dev->input_dev->evbit); set_bit(EV_MSC, dev->input_dev->evbit); -- 2.9.3
Re: Bug: decoders referenced in kernel rc-keymaps not loaded on boot
On Tue, Feb 21, 2017 at 11:52:24PM +0100, Matthias Reichl wrote: > On Tue, Feb 21, 2017 at 07:34:39PM +, Sean Young wrote: > > On Tue, Feb 21, 2017 at 07:49:29PM +0100, Matthias Reichl wrote: > > > There seems to be a bug in on-demand loading of IR protocol decoders. > > > > > > After bootup the protocol referenced in the in-kernel rc keymap shows > > > up as enabled (in sysfs and ir-keytable) but the protocol decoder > > > is not loaded and thus no rc input events will be generated. > > > > > > For example, RPi3 with kernel 4.10 and gpio-ir-recv configured to use > > > the rc-hauppauge keymap in devicetree: > > > > > > # lsmod | grep '^\(ir\|rc_\)' > > > ir_lirc_codec 5590 0 > > > rc_hauppauge2422 0 > > > rc_core24320 5 > > > rc_hauppauge,ir_lirc_codec,lirc_dev,gpio_ir_recv > > > > > > # cat /sys/class/rc/rc0/protocols > > > other unknown [rc-5] nec rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp > > > cec [lirc] > > > > > > # dmesg | grep "IR " > > > [4.506728] Registered IR keymap rc-hauppauge > > > [4.554651] lirc_dev: IR Remote Control driver registered, major 242 > > > [4.576490] IR LIRC bridge handler initialized > > > > > > The same happens with other IR receivers, eg the streamzap receiver, > > > which uses the rc-5-sz protocol / ir_rc5_decoder, on x86. > > > > > > Reverting the on-demand-loading patches > > > > > > [media] media: rc: remove unneeded code > > > commit c1500ba0b61e9abf95e0e7ecd3c4ad877f019abe > > > > > > [media] media: rc: move check whether a protocol is enabled to the core > > > commit d80ca8bd71f0b01b2b12459189927cb3299cfab9 > > > > > > [media] media: rc: load decoder modules on-demand > > > commit acc1c3c688ed8cc862ddc007eab0dcef839f4ec8 > > > > > > restores the previous behaviour, all decoders are enabled and IR > > > events can be generated immediately after boot without having to > > > manually trigger loading of a protocol decoder. > > > > Hmm this seems to be working fine for me. If you write to the protocols > > file, eg. "echo +nec > /sys/class/rc/rc0/protocols", is ir-nec-decoder > > loaded and do you get any messages in dmesg (you should). > > > > What's your config? > > When I do an "echo +nec > /sys/class/rc/rc0/protocols" it triggers > the load of both rc5 and nec decoder modules: > > root@rpi3:~# cat /sys/class/rc/rc0/protocols > other unknown [rc-5] nec rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec > [lirc] > root@rpi3:~# echo +nec > /sys/class/rc/rc0/protocols > root@rpi3:~# cat /sys/class/rc/rc0/protocols > other unknown [rc-5] [nec] rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec > [lirc] > root@rpi3:~# dmesg | grep "IR " > [3.565061] Registered IR keymap rc-hauppauge > [3.613031] lirc_dev: IR Remote Control driver registered, major 242 > [3.641423] IR LIRC bridge handler initialized > [ 41.877263] IR RC5(x/sz) protocol handler initialized > [ 41.931575] IR NEC protocol handler initialized > > I'm currently testing with downstream RPi kernel 4.9 on Raspbian Jessie > (a Debian derivate). > > Kernel config is here: > https://github.com/raspberrypi/linux/blob/rpi-4.9.y/arch/arm/configs/bcm2709_defconfig > > To reproduce the issue it's important to disable the udev rule that > runs ir-keytable -a as that can trigger a load of the kernel keytable > via the userspace keymap/protocol. Ah. Yes, this is broken. In commit "9f0bf36 [media] media: rc: preparation for on-demand decoder module loading", code was added so that only the required decoder module was loaded. This added it to the sysfs protocols attribute handling, but not to the rc_register_device(), so no decoder will be loaded when a device is first registered. As you say the udev rule papers over the problem by executing ir-keytable, so I never noticed the problem either. I'll send a patch as a reply to this email. Sean
[PATCH] [media] tm6000: Fix resource freeing in 'tm6000_prepare_isoc()'
'usb_free_urb(urb)' is a no-op, because urb is known to be NULL. It is likelly that releasing resources allocated by 'tm6000_alloc_urb_buffers()' just a few lines above is expected here. This has been spotted by the following coccinelle script: @@ expression ret, x, e; identifier f; @@ * if (x == NULL) { ... when != x = e; ( *f(<+...x...+>); | *ret = f(<+...x...+>); ) ... } Signed-off-by: Christophe JAILLET --- drivers/media/usb/tm6000/tm6000-video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c index c4fdc1fa32ef..7e960d0a5b92 100644 --- a/drivers/media/usb/tm6000/tm6000-video.c +++ b/drivers/media/usb/tm6000/tm6000-video.c @@ -631,7 +631,7 @@ static int tm6000_prepare_isoc(struct tm6000_core *dev) urb = usb_alloc_urb(max_packets, GFP_KERNEL); if (!urb) { tm6000_uninit_isoc(dev); - usb_free_urb(urb); + tm6000_free_urb_buffers(dev); return -ENOMEM; } dev->isoc_ctl.urb[i] = urb; -- 2.9.3
Re: Looking more details and reasons for using orig_add_limit.
Em Wed, 22 Feb 2017 21:53:08 +0200 Laurent Pinchart escreveu: > Hi Prasad, > > On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote: > > Hi mchehab/linux-media, > > > > It is not clear why KERNEL_DS was set explicitly here. In this path > > video_usercopy() gets called and it > > copies the “struct v4l2_buffer” struct to user space stack memory. > > > > Can you please share reasons for setting to KERNEL_DS here? > > It's a bit of historical hack. To implement compat ioctl handling, we copy > the > ioctl 32-bit argument from userspace, turn it into a native 64-bit ioctl > argument, and call the native ioctl code. That code expects the argument to > be > stored in userspace memory and uses get_user() and put_user() to access it. > As > the 64-bit argument now lives in kernel memory, my understanding is that we > fake things up with KERNEL_DS. Precisely. Actually, if I remember well, this was needed to pass pointer arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of V4L2 ioctls that pass structures with pointers on it. Setting DS cause those pointers to do the right thing, but yeah, it is hackish. This used to work fine on x86_64 (when such code was written e. g. Kernel 2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we received some patches fixing support for some ioctl compat code due to x86_64/arm64 differences in the past. On what Kernel version it started to cause troubles? 4.9? If so, then maybe the breakage is a side effect of VM stack changes. > The ioctl code should be refactored to get rid of this hack. Agreed. Thanks, Mauro
Re: [PATCH 2/3] [media] tc358743: Add OF device ID table
CC: Philipp Zabel who added device tree support to this driver Regards, Mats Randgaard On 02/22/2017 05:11 PM, Javier Martinez Canillas wrote: The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. Signed-off-by: Javier Martinez Canillas --- drivers/media/i2c/tc358743.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c index f569a05fe105..76baf7a7bd57 100644 --- a/drivers/media/i2c/tc358743.c +++ b/drivers/media/i2c/tc358743.c @@ -1951,9 +1951,18 @@ static struct i2c_device_id tc358743_id[] = { MODULE_DEVICE_TABLE(i2c, tc358743_id); +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id tc358743_of_match[] = { + { .compatible = "toshiba,tc358743" }, + {}, +}; +MODULE_DEVICE_TABLE(of, tc358743_of_match); +#endif + static struct i2c_driver tc358743_driver = { .driver = { .name = "tc358743", + .of_match_table = of_match_ptr(tc358743_of_match), }, .probe = tc358743_probe, .remove = tc358743_remove,
Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided
On 02/22/2017 05:03 PM, Nicolas Dufresne wrote: Le mercredi 22 février 2017 à 15:57 -0300, Thibault Saunier a écrit : On 02/22/2017 03:06 PM, Hans Verkuil wrote: On 02/22/2017 05:05 AM, Thibault Saunier wrote: Hello, On 02/21/2017 11:19 PM, Hans Verkuil wrote: On 02/21/2017 11:20 AM, Thibault Saunier wrote: Use colorspace provided by the user as we are only doing scaling and color encoding conversion, we won't be able to transform the colorspace itself and the colorspace won't mater in that operation. Also always use output colorspace on the capture side. Start using 576p as a threashold to compute the colorspace. The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers don't agree on the display resolution that should be used as a threshold. From EIA CEA 861B about colorimetry for various resolutions: - 5.1 480p, 480i, 576p, 576i, 240p, and 288p The color space used by the 480-line, 576-line, 240- line, and 288-line formats will likely be based on SMPTE 170M [1]. - 5.2 1080i, 1080p, and 720p The color space used by the high definition formats will likely be based on ITU-R BT.709-4 This indicates that in the case that userspace does not specify what colorspace should be used, we should use 576p as a threshold to set V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M. Even if it is only 'likely' and not a requirement it is the best guess we can make. The stream should have been encoded with the information and userspace has to pass it to the driver if it is not the case, otherwise we won't be able to handle it properly anyhow. Also, check for the resolution in G_FMT instead unconditionally setting the V4L2_COLORSPACE_REC709 colorspace. Signed-off-by: Javier Martinez Canillas Signed-off-by: Thibault Saunier Reviewed-by: Andrzej Hajda --- Changes in v5: - Squash commit to always use output colorspace on the capture side inside this one - Fix typo in commit message Changes in v4: - Reword commit message to better back our assumptions on specifications Changes in v3: - Do not check values in the g_fmt functions as Andrzej explained in previous review - Added 'Reviewed-by: Andrzej Hajda ' Changes in v2: None drivers/media/platform/exynos-gsc/gsc-core.c | 20 +++- drivers/media/platform/exynos-gsc/gsc-core.h | 1 + 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c index 59a634201830..772599de8c13 100644 --- a/drivers/media/platform/exynos-gsc/gsc-core.c +++ b/drivers/media/platform/exynos-gsc/gsc-core.c @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) } else { min_w = variant->pix_min->target_rot_dis_w; min_h = variant->pix_min->target_rot_dis_h; +pix_mp->colorspace = ctx->out_colorspace; } pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d", @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) pix_mp->num_planes = fmt->num_planes; -if (pix_mp->width >= 1280) /* HD */ -pix_mp->colorspace = V4L2_COLORSPACE_REC709; -else /* SD */ -pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; +if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) { +if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */ I'd use || instead of && here. Ditto for the next patch. +pix_mp->colorspace = V4L2_COLORSPACE_REC709; +else /* SD */ +pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; +} Are you sure this is in fact how it is used? If the source of the video is the sensor (camera), then the colorspace is typically SRGB. I'm not really sure you should guess here. All a mem2mem driver should do is to pass the colorspace etc. information from the output to the capture side, and not invent things. That simply makes no sense. This is not a camera device but a colorspace conversion device, in many Not really, this is a color encoding conversion device. I.e. it only affects the Y'CbCr encoding and quantization range. The colorspace (aka chromaticities) and transfer function remain unchanged. Well, right, sorry I am talking in GStreamer terminlogy where what you call colorspace is called colorimetry, and colorspace is what I am talking about here. In fact, I suspect (correct me if I am wrong) that it only converts between RGB, Y'CbCr 601 and Y'CbCr 709 encodings, where RGB is full range and the Y'CbCr formats are limited range. That sounds correct. If you pass in limited range RGB it will probably do the wrong thing as I don't seen any quantization checks in the code. So the colorspace and xfer_func fields remain unchanged by this driver. If you want to do this really correctly, then you need to do more. I don't have time
Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided
Le mercredi 22 février 2017 à 15:57 -0300, Thibault Saunier a écrit : > On 02/22/2017 03:06 PM, Hans Verkuil wrote: > > > > On 02/22/2017 05:05 AM, Thibault Saunier wrote: > > > Hello, > > > > > > > > > On 02/21/2017 11:19 PM, Hans Verkuil wrote: > > > > On 02/21/2017 11:20 AM, Thibault Saunier wrote: > > > > > Use colorspace provided by the user as we are only doing > > > > > scaling and > > > > > color encoding conversion, we won't be able to transform the > > > > > colorspace > > > > > itself and the colorspace won't mater in that operation. > > > > > > > > > > Also always use output colorspace on the capture side. > > > > > > > > > > Start using 576p as a threashold to compute the colorspace. > > > > > The media documentation says that the > > > > > V4L2_COLORSPACE_SMPTE170M colorspace > > > > > should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. > > > > > But drivers > > > > > don't agree on the display resolution that should be used as > > > > > a threshold. > > > > > > > > > > From EIA CEA 861B about colorimetry for various > > > > > resolutions: > > > > > > > > > > - 5.1 480p, 480i, 576p, 576i, 240p, and 288p > > > > > The color space used by the 480-line, 576-line, 240- > > > > > line, and 288-line > > > > > formats will likely be based on SMPTE 170M [1]. > > > > > - 5.2 1080i, 1080p, and 720p > > > > > The color space used by the high definition formats > > > > > will likely be > > > > > based on ITU-R BT.709-4 > > > > > > > > > > This indicates that in the case that userspace does not > > > > > specify what > > > > > colorspace should be used, we should use 576p as a threshold > > > > > to set > > > > > V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M. > > > > > Even if it is > > > > > only 'likely' and not a requirement it is the best guess we > > > > > can make. > > > > > > > > > > The stream should have been encoded with the information and > > > > > userspace > > > > > has to pass it to the driver if it is not the case, otherwise > > > > > we won't be > > > > > able to handle it properly anyhow. > > > > > > > > > > Also, check for the resolution in G_FMT instead > > > > > unconditionally setting > > > > > the V4L2_COLORSPACE_REC709 colorspace. > > > > > > > > > > Signed-off-by: Javier Martinez Canillas > > > > om> > > > > > Signed-off-by: Thibault Saunier > > > > .com> > > > > > Reviewed-by: Andrzej Hajda > > > > > > > > > > --- > > > > > > > > > > Changes in v5: > > > > > - Squash commit to always use output colorspace on the > > > > > capture side > > > > > inside this one > > > > > - Fix typo in commit message > > > > > > > > > > Changes in v4: > > > > > - Reword commit message to better back our assumptions on > > > > > specifications > > > > > > > > > > Changes in v3: > > > > > - Do not check values in the g_fmt functions as Andrzej > > > > > explained in previous review > > > > > - Added 'Reviewed-by: Andrzej Hajda ' > > > > > > > > > > Changes in v2: None > > > > > > > > > > drivers/media/platform/exynos-gsc/gsc-core.c | 20 > > > > > +++- > > > > > drivers/media/platform/exynos-gsc/gsc-core.h | 1 + > > > > > 2 files changed, 16 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c > > > > > b/drivers/media/platform/exynos-gsc/gsc-core.c > > > > > index 59a634201830..772599de8c13 100644 > > > > > --- a/drivers/media/platform/exynos-gsc/gsc-core.c > > > > > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c > > > > > @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx > > > > > *ctx, struct v4l2_format *f) > > > > > } else { > > > > > min_w = variant->pix_min->target_rot_dis_w; > > > > > min_h = variant->pix_min->target_rot_dis_h; > > > > > +pix_mp->colorspace = ctx->out_colorspace; > > > > > } > > > > > pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = > > > > > %d", > > > > > @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx > > > > > *ctx, struct v4l2_format *f) > > > > > pix_mp->num_planes = fmt->num_planes; > > > > > -if (pix_mp->width >= 1280) /* HD */ > > > > > -pix_mp->colorspace = V4L2_COLORSPACE_REC709; > > > > > -else /* SD */ > > > > > -pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; > > > > > +if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) { > > > > > +if (pix_mp->width > 720 && pix_mp->height > 576) /* > > > > > HD */ > > > > > > > > I'd use || instead of && here. Ditto for the next patch. > > > > > > > > > +pix_mp->colorspace = V4L2_COLORSPACE_REC709; > > > > > +else /* SD */ > > > > > +pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; > > > > > +} > > > > > > > > Are you sure this is in fact how it is used? If the source of > > > > the video > > > > is the sensor (camera), then the colorspace is typically SRGB. > > > > I'm not > > > > really sure you should
Re: Looking more details and reasons for using orig_add_limit.
HI Sodagudi, Em Tue, 21 Feb 2017 06:20:58 -0800 Sodagudi Prasad escreveu: > Hi mchehab/linux-media, > > It is not clear why KERNEL_DS was set explicitly here. In this path > video_usercopy() gets called and it > copies the “struct v4l2_buffer” struct to user space stack memory. > > Can you please share reasons for setting to KERNEL_DS here? I'm not sure. If I remember well, such code came from the patch that moved the V4L2 compat32 code from FS core to V4L2 in the old days. As it worked fine since 2.6.x, it was assumed to be the right thing to do, and we never had any reason to change it to something else :-) As it is now causing troubles on newer Kernels, it needs to be converted to the modern practices. Patches are welcomed! > > static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned > long arg) > { > … > … > > if (compatible_arg) > err = native_ioctl(file, cmd, (unsigned long)up); > else { > mm_segment_t old_fs = get_fs(); > > set_fs(KERNEL_DS); > err = native_ioctl(file, cmd, (unsigned long)&karg); > set_fs(old_fs); > } > … > } > > On 2017-02-16 02:39, James Morse wrote: > > Hi Prasad, > > > > On 15/02/17 21:12, Sodagudi Prasad wrote: > >> On 2017-02-15 04:09, James Morse wrote: > >>> On 15/02/17 05:52, Sodagudi Prasad wrote: > that driver is calling set_fs(KERNEL_DS) and then copy_to_user() to > user space > memory. > >>> > >>> Don't do this, its exactly the case PAN+UAO and the code you pointed > >>> to are > >>> designed to catch. Accessing userspace needs doing carefully, setting > >>> USER_DS > >>> and using the put_user()/copy_to_user() accessors are the required > >>> steps. > >>> > >>> Which driver is doing this? Is it in mainline? > >> > >> Yes. It is mainline driver - > >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > > >> In some v4l2 use-case kernel panic is observed. Below part > >> of the code has set_fs to KERNEL_DS before calling native_ioctl(). > >> > >> static long do_video_ioctl(struct file *file, unsigned int cmd, > >> unsigned long arg) > >> { > >> … > >> … > >> if (compatible_arg) > >> err = native_ioctl(file, cmd, (unsigned long)up); > >> else { > >> mm_segment_t old_fs = get_fs(); > >> > >> set_fs(KERNEL_DS); > KERNEL_DS. > >> err = native_ioctl(file, cmd, (unsigned long)&karg); > >> set_fs(old_fs); > >> } > >> > >> Here is the call stack which is resulting crash, because user space > >> memory has > >> read only permissions. > >> [27249.920041] [] __arch_copy_to_user+0x110/0x180 > >> [27249.920047] [] video_ioctl2+0x38/0x44 > >> [27249.920054] [] v4l2_ioctl+0x78/0xb4 > >> [27249.920059] [] do_video_ioctl+0x91c/0x1160 > >> [27249.920064] [] v4l2_compat_ioctl32+0x60/0xcc > >> [27249.920071] [] compat_SyS_ioctl+0x124/0xd88 > >> [27249.920077] [] el0_svc_naked+0x24/0x2 > > > > It's not totally clear to me what is going on here, but some > > observations: > > the ioctl is trying to copy_to_user() to some read-only memory. This > > would > > normally fail gracefully with -EFAULT, but because KERNEL_DS has been > > set, the > > kernel checks this before calling the fault handler and calls die() on > > your ioctl(). > > > > The ioctl code is doing this deliberately as a compat mechanism, but > > the code > > behind file->f_op->unlocked_ioctl() expects fs==USER_DS when it does > > its work. > > That code needs to be made aware of this compat translation, or a > > compat_ioctl > > call provided. > > > > > Which v4l driver is this? Which ioctl is being called? Does the driver > > using the > > v4l framework have a compat_ioctl() call? > Yes. Same kernel crash is seen with both video and camera use cases. > Yes. Driver have compact_ioctl(). > > > What path does this call take through v4l2_compat_ioctl32()? It looks > > like > > compat_ioctl will be skipped in certain cases, v4l2_compat_ioctl32() > > has: > >>if (_IOC_TYPE(cmd) == 'V' && _IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) > >>ret = do_video_ioctl(file, cmd, arg); > >>else if (vdev->fops->compat_ioctl32) > >>ret = vdev->fops->compat_ioctl32(file, cmd, arg); > > > > Is your ioctl matched by that top if()? > Yes. Top if condition in true and do_video_ioctl() getting called. > > > > If there is permission fault for user space address the above > condition > is leading to kernel crash. Because orig_add_limit is having > KERNEL_DS as set_fs > called before copy_to_user(). > > 1)So I would like to understand that, is that user space > pointer leading to > permission fault not correct(condition_1) in this scenario? > >>> > >>> The correct thing has happened here. To access user space > >>> set_fs(USER_DS) first. > >>> (and se
Re: Dead code in v4l2-mem2mem.c?
Hi Shaobo, On Monday 20 Feb 2017 12:49:18 Shaobo wrote: > Hi Laurent, > > I'd like to. It sounds interesting and useful to me. Could you give me some > pointers about how to audit drivers? It's pretty simple, you need to check all functions that call get_queue_ctx() and follow the call stacks up to drivers to see if the context can be NULL. It's a bit of work though :-) -- Regards, Laurent Pinchart
Re: Looking more details and reasons for using orig_add_limit.
Hi Prasad, On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote: > Hi mchehab/linux-media, > > It is not clear why KERNEL_DS was set explicitly here. In this path > video_usercopy() gets called and it > copies the “struct v4l2_buffer” struct to user space stack memory. > > Can you please share reasons for setting to KERNEL_DS here? It's a bit of historical hack. To implement compat ioctl handling, we copy the ioctl 32-bit argument from userspace, turn it into a native 64-bit ioctl argument, and call the native ioctl code. That code expects the argument to be stored in userspace memory and uses get_user() and put_user() to access it. As the 64-bit argument now lives in kernel memory, my understanding is that we fake things up with KERNEL_DS. The ioctl code should be refactored to get rid of this hack. > static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned > long arg) > { > … > … > > if (compatible_arg) > err = native_ioctl(file, cmd, (unsigned long)up); > else { > mm_segment_t old_fs = get_fs(); > > set_fs(KERNEL_DS); > err = native_ioctl(file, cmd, (unsigned long)&karg); > set_fs(old_fs); > } > … > } > > On 2017-02-16 02:39, James Morse wrote: > > Hi Prasad, > > > > On 15/02/17 21:12, Sodagudi Prasad wrote: > >> On 2017-02-15 04:09, James Morse wrote: > >>> On 15/02/17 05:52, Sodagudi Prasad wrote: > that driver is calling set_fs(KERNEL_DS) and then copy_to_user() to > user space > memory. > >>> > >>> Don't do this, its exactly the case PAN+UAO and the code you pointed > >>> to are > >>> designed to catch. Accessing userspace needs doing carefully, setting > >>> USER_DS > >>> and using the put_user()/copy_to_user() accessors are the required > >>> steps. > >>> > >>> Which driver is doing this? Is it in mainline? > >> > >> Yes. It is mainline driver - > >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c > >> > >> In some v4l2 use-case kernel panic is observed. Below part > >> of the code has set_fs to KERNEL_DS before calling native_ioctl(). > >> > >> static long do_video_ioctl(struct file *file, unsigned int cmd, > >> unsigned long arg) > >> { > >> … > >> … > >> > >> if (compatible_arg) > >> > >> err = native_ioctl(file, cmd, (unsigned long)up); > >> > >> else { > >> > >> mm_segment_t old_fs = get_fs(); > >> > >> set_fs(KERNEL_DS); > KERNEL_DS. > >> err = native_ioctl(file, cmd, (unsigned long)&karg); > >> set_fs(old_fs); > >> > >> } > >> > >> Here is the call stack which is resulting crash, because user space > >> memory has > >> read only permissions. > >> [27249.920041] [] __arch_copy_to_user+0x110/0x180 > >> [27249.920047] [] video_ioctl2+0x38/0x44 > >> [27249.920054] [] v4l2_ioctl+0x78/0xb4 > >> [27249.920059] [] do_video_ioctl+0x91c/0x1160 > >> [27249.920064] [] v4l2_compat_ioctl32+0x60/0xcc > >> [27249.920071] [] compat_SyS_ioctl+0x124/0xd88 > >> [27249.920077] [] el0_svc_naked+0x24/0x2 > > > > It's not totally clear to me what is going on here, but some > > observations: > > the ioctl is trying to copy_to_user() to some read-only memory. This > > would > > normally fail gracefully with -EFAULT, but because KERNEL_DS has been > > set, the > > kernel checks this before calling the fault handler and calls die() on > > your ioctl(). > > > > The ioctl code is doing this deliberately as a compat mechanism, but > > the code > > behind file->f_op->unlocked_ioctl() expects fs==USER_DS when it does > > its work. > > That code needs to be made aware of this compat translation, or a > > compat_ioctl > > call provided. > > > > > > Which v4l driver is this? Which ioctl is being called? Does the driver > > using the > > v4l framework have a compat_ioctl() call? > > Yes. Same kernel crash is seen with both video and camera use cases. > Yes. Driver have compact_ioctl(). > > > What path does this call take through v4l2_compat_ioctl32()? It looks > > like > > compat_ioctl will be skipped in certain cases, v4l2_compat_ioctl32() > > > > has: > >>if (_IOC_TYPE(cmd) == 'V' && _IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) > >> > >>ret = do_video_ioctl(file, cmd, arg); > >> > >>else if (vdev->fops->compat_ioctl32) > >> > >>ret = vdev->fops->compat_ioctl32(file, cmd, arg); > > > > Is your ioctl matched by that top if()? > > Yes. Top if condition in true and do_video_ioctl() getting called. > > If there is permission fault for user space address the above > condition > is leading to kernel crash. Because orig_add_limit is having > KERNEL_DS as set_fs > called before copy_to_user(). > > 1)So I would like to understand that, is that user space > pointer leading to > permission fault not correct(condition
Re: v4l2: Adding support for multiple MIPI CSI-2 virtual channels
Hi Guennadi, On Wednesday 22 Feb 2017 18:54:20 Guennadi Liakhovetski wrote: > On Tue, 21 Feb 2017, Ajay kumar wrote: > > On Fri, Feb 17, 2017 at 7:27 PM, Thomas Axelsson wrote: > >> Hi, > >> > >> I have a v4l2_subdev that provides multiple MIPI CSI-2 Virtual > >> Channels. I want to configure each virtual channel individually (e.g. > >> set_fmt), but the v4l2 interface does not seem to have a clear way to > >> access configuration on a virtual channel level, but only the > >> v4l2_subdev as a whole. Using one v4l2_subdev for multiple virtual > >> channels by extending the "reg" tag to be an array looks like the > >> correct way to do it, based on the mipi-dsi-bus.txt document and > >> current device tree endpoint structure. > >> > >> However, I cannot figure out how to extend e.g. set_fmt/get_fmt subdev > >> ioctls to specify which virtual channel the call applies to. Does > >> anyone have any advice on how to handle this case? > > > > This would be helpful for my project as well since even I need to > > support multiple streams using Virtual Channels. > > Can anyone point out to some V4L2 driver, if this kind of support is > > already implemented? > > My understanding is, that MIPI CSI virtual channel handling requires > extensions to the V4L2 subdev API. These extensions have been discussed at > a media mini-summit almost a year ago, slides are available at [1], but as > my priorities shifted away from this work, don't think those extensions > ever got implemented. We've also discussed the topic last week in a face to face meeting with Niklas (CC'ed) and Sakari. Niklas will start working on upstreaming the necessary V4L2 API extensions for CSI-2 virtual channel support. The current plan is to start the work at the beginning of April. > [1] > https://linuxtv.org/downloads/presentations/media_summit_2016_san_diego/v4l > 2-multistream.pdf > > >> Previous thread: "Device Tree formatting for multiple virtual channels > >> in ti-vpe/cal driver?" > >> > >> Best Regards, > >> Thomas Axelsson > >> > >> PS. First e-mail seems to have gotten caught in the spam filter. I > >> apologize if this is a duplicate. -- Regards, Laurent Pinchart
Re: [PATCH RESEND 1/1] media: platform: Renesas IMR driver
Hello! On 02/22/2017 05:25 PM, Rob Herring wrote: From: Konstantin Kozhevnikov The image renderer light extended 4 (IMR-LX4) or the distortion correction engine is a drawing processor with a simple instruction system capable of referencing data on an external memory as 2D texture data and performing texture mapping and drawing with respect to any shape that is split into triangular objects. This V4L2 memory-to-memory device driver only supports image renderer found in the R-Car gen3 SoCs; the R-Car gen2 support can be added later... [Sergei: merged 2 original patches, added the patch description, removed unrelated parts, added the binding document, ported the driver to the modern kernel, renamed the UAPI header file and the guard macros to match the driver name, extended the copyrights, fixed up Kconfig prompt/depends/ help, made use of the BIT()/GENMASK() macros, sorted #include's, removed leading dots and fixed grammar in the comments, fixed up indentation to use tabs where possible, renamed IMR_DLSR to IMR_DLPR to match the manual, separated the register offset/bit #define's, removed *inline* from .c file, fixed lines over 80 columns, removed useless parens, operators, casts, braces, variables, #include's, (commented out) statements, and even function, inserted empty line after desclaration, removed extra empty lines, reordered some local variable desclarations, removed calls to 4l2_err() on kmalloc() failure, fixed the error returned by imr_default(), avoided code duplication in the IRQ handler, used '__packed' for the UAPI structures, enclosed the macro parameters in parens, exchanged the values of IMR_MAP_AUTO[SD]G macros.] Signed-off-by: Konstantin Kozhevnikov Signed-off-by: Sergei Shtylyov --- This patch is against the 'media_tree.git' repo's 'master' branch. Documentation/devicetree/bindings/media/rcar_imr.txt | 23 drivers/media/platform/Kconfig | 13 drivers/media/platform/Makefile |1 drivers/media/platform/rcar_imr.c| 1923 +++ include/uapi/linux/rcar_imr.h| 94 5 files changed, 2054 insertions(+) Index: media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt === --- /dev/null +++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt @@ -0,0 +1,23 @@ +Renesas R-Car Image Renderer (Distortion Correction Engine) +--- + +The image renderer or the distortion correction engine is a drawing processor +with a simple instruction system capable of referencing data in external memory +as 2D texture data and performing texture mapping and drawing with respect to +any shape that is split into triangular objects. Please fix extra spaces in here. OK. Seems to be your pet peeve? :-) + +Required properties: +- compatible: must be "renesas,imr-lx4" for the image renderer light extended 4 + (IMR-LX4) found in the R-Car gen3 SoCs; Needs an SoC specific compatible string too. Strings, to be precise -- there are several SoCs but the IMR-LX4 core seems the same among them. Well, if you say so... The description is above, so you just need to list the compatible strings. There's (most probably) gonna be other versions of the IMR core supported, (this core can be forund in gen2 SoCs too)... +- reg: offset and length of the register block; +- interrupts: interrupt specifier; How many interrupts? I thought it was clear from using singular. +- clocks: clock phandle and specifier pair. How many clocks? Two, perhaps. I meant a single clock by using singular again. [...] MBR, Sergei
Re: [PATCH] dma-buf: add support for compat ioctl
On Tue, Feb 21, 2017 at 4:08 PM, Christian König wrote: > Am 21.02.2017 um 15:55 schrieb Marek Szyprowski: >> >> Dear All, >> >> On 2017-02-21 15:37, Marek Szyprowski wrote: >>> >>> Hi Christian, >>> >>> On 2017-02-21 14:59, Christian König wrote: Am 21.02.2017 um 14:21 schrieb Marek Szyprowski: > > Add compat ioctl support to dma-buf. This lets one to use > DMA_BUF_IOCTL_SYNC > ioctl from 32bit application on 64bit kernel. Data structures for both > 32 > and 64bit modes are same, so there is no need for additional > translation > layer. Well I might be wrong, but IIRC compat_ioctl was just optional and if not specified unlocked_ioctl was called instead. If that is true your patch wouldn't have any effect at all. >>> >>> >>> Well, then why I got -ENOTTY in the 32bit test app for this ioctl on >>> 64bit ARM64 kernel without this patch? >>> >> >> I've checked in fs/compat_ioctl.c, I see no fallback in >> COMPAT_SYSCALL_DEFINE3, >> so one has to provide compat_ioctl callback to have ioctl working with >> 32bit >> apps. > > > Then my memory cheated on me. > > In this case the patch is Reviewed-by: Christian König > . Since you have commit rights for drm-misc, care to push this to drm-misc-next-fixes pls? Also I think this warrants a cc: stable, clearly an obvious screw-up in creating this api on our side :( So feel free to smash my ack on the patch. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided
On 02/22/2017 03:06 PM, Hans Verkuil wrote: On 02/22/2017 05:05 AM, Thibault Saunier wrote: Hello, On 02/21/2017 11:19 PM, Hans Verkuil wrote: On 02/21/2017 11:20 AM, Thibault Saunier wrote: Use colorspace provided by the user as we are only doing scaling and color encoding conversion, we won't be able to transform the colorspace itself and the colorspace won't mater in that operation. Also always use output colorspace on the capture side. Start using 576p as a threashold to compute the colorspace. The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers don't agree on the display resolution that should be used as a threshold. From EIA CEA 861B about colorimetry for various resolutions: - 5.1 480p, 480i, 576p, 576i, 240p, and 288p The color space used by the 480-line, 576-line, 240-line, and 288-line formats will likely be based on SMPTE 170M [1]. - 5.2 1080i, 1080p, and 720p The color space used by the high definition formats will likely be based on ITU-R BT.709-4 This indicates that in the case that userspace does not specify what colorspace should be used, we should use 576p as a threshold to set V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M. Even if it is only 'likely' and not a requirement it is the best guess we can make. The stream should have been encoded with the information and userspace has to pass it to the driver if it is not the case, otherwise we won't be able to handle it properly anyhow. Also, check for the resolution in G_FMT instead unconditionally setting the V4L2_COLORSPACE_REC709 colorspace. Signed-off-by: Javier Martinez Canillas Signed-off-by: Thibault Saunier Reviewed-by: Andrzej Hajda --- Changes in v5: - Squash commit to always use output colorspace on the capture side inside this one - Fix typo in commit message Changes in v4: - Reword commit message to better back our assumptions on specifications Changes in v3: - Do not check values in the g_fmt functions as Andrzej explained in previous review - Added 'Reviewed-by: Andrzej Hajda ' Changes in v2: None drivers/media/platform/exynos-gsc/gsc-core.c | 20 +++- drivers/media/platform/exynos-gsc/gsc-core.h | 1 + 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c index 59a634201830..772599de8c13 100644 --- a/drivers/media/platform/exynos-gsc/gsc-core.c +++ b/drivers/media/platform/exynos-gsc/gsc-core.c @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) } else { min_w = variant->pix_min->target_rot_dis_w; min_h = variant->pix_min->target_rot_dis_h; +pix_mp->colorspace = ctx->out_colorspace; } pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d", @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) pix_mp->num_planes = fmt->num_planes; -if (pix_mp->width >= 1280) /* HD */ -pix_mp->colorspace = V4L2_COLORSPACE_REC709; -else /* SD */ -pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; +if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) { +if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */ I'd use || instead of && here. Ditto for the next patch. +pix_mp->colorspace = V4L2_COLORSPACE_REC709; +else /* SD */ +pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; +} Are you sure this is in fact how it is used? If the source of the video is the sensor (camera), then the colorspace is typically SRGB. I'm not really sure you should guess here. All a mem2mem driver should do is to pass the colorspace etc. information from the output to the capture side, and not invent things. That simply makes no sense. This is not a camera device but a colorspace conversion device, in many Not really, this is a color encoding conversion device. I.e. it only affects the Y'CbCr encoding and quantization range. The colorspace (aka chromaticities) and transfer function remain unchanged. Well, right, sorry I am talking in GStreamer terminlogy where what you call colorspace is called colorimetry, and colorspace is what I am talking about here. In fact, I suspect (correct me if I am wrong) that it only converts between RGB, Y'CbCr 601 and Y'CbCr 709 encodings, where RGB is full range and the Y'CbCr formats are limited range. That sounds correct. If you pass in limited range RGB it will probably do the wrong thing as I don't seen any quantization checks in the code. So the colorspace and xfer_func fields remain unchanged by this driver. If you want to do this really correctly, then you need to do more. I don't have time right now to go into this in-depth, but I will try to get back to this on Monday. I am thinking of documenting this as part of the V
Re: [PATCH v2 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function
On Mon, Feb 20, 2017 at 6:38 AM, Marek Szyprowski wrote: > To complete DMA memory configuration for MFC device, allocation of the > firmware buffer is needed, because some parameters are dependant on its base > address. Till now, this has been handled in the s5p_mfc_alloc_firmware() > function. This patch moves that logic to s5p_mfc_configure_dma_memory() to > keep DMA memory related operations in a single place. This way > s5p_mfc_alloc_firmware() is simplified and does what it name says. The > other consequence of this change is moving s5p_mfc_alloc_firmware() call > from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory(). Overall looks good. This patch makes subtle change in the dma and firwmare initialization sequence. Might be okay, but wanted to call out just in case, Before this change: vb2_dma_contig_set_max_seg_size() is done for both iommu and non-iommu case before s5p_mfc_alloc_firmware(). With this change setting dma_contig max size happens after s5p_mfc_alloc_firmware(). From what I can tell this might not be an issue. vb2_dma_contig_clear_max_seg_size() still happens after s5p_mfc_release_firmware(), so that part hasn't changed. Do any of the dma_* calls made from s5p_mfc_alloc_firmware() and later during the dma congiguration sequence depend on dmap_parms being allocated? Doesn't looks like it from what I can tell, but safe to ask. thanks, -- Shuah > > Signed-off-by: Marek Szyprowski > --- > drivers/media/platform/s5p-mfc/s5p_mfc.c | 62 > +-- > drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 31 -- > 2 files changed, 49 insertions(+), 44 deletions(-) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c > b/drivers/media/platform/s5p-mfc/s5p_mfc.c > index bc1aeb25ebeb..4403487a494a 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c > @@ -1110,6 +1110,11 @@ static struct device *s5p_mfc_alloc_memdev(struct > device *dev, > static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev) > { > struct device *dev = &mfc_dev->plat_dev->dev; > + void *bank2_virt; > + dma_addr_t bank2_dma_addr; > + unsigned long align_size = 1 << MFC_BASE_ALIGN_ORDER; > + struct s5p_mfc_priv_buf *fw_buf = &mfc_dev->fw_buf; > + int ret; > > /* > * When IOMMU is available, we cannot use the default configuration, > @@ -1122,14 +1127,21 @@ static int s5p_mfc_configure_dma_memory(struct > s5p_mfc_dev *mfc_dev) > if (exynos_is_iommu_available(dev)) { > int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE, > S5P_MFC_IOMMU_DMA_SIZE); > - if (ret == 0) { > - mfc_dev->mem_dev[BANK1_CTX] = > - mfc_dev->mem_dev[BANK2_CTX] = dev; > - vb2_dma_contig_set_max_seg_size(dev, > - DMA_BIT_MASK(32)); > + if (ret) > + return ret; > + > + mfc_dev->mem_dev[BANK1_CTX] = mfc_dev->mem_dev[BANK2_CTX] = > dev; > + ret = s5p_mfc_alloc_firmware(mfc_dev); > + if (ret) { > + exynos_unconfigure_iommu(dev); > + return ret; > } > > - return ret; > + mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma; > + mfc_dev->dma_base[BANK2_CTX] = fw_buf->dma; > + vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + > + return 0; > } > > /* > @@ -1147,6 +1159,35 @@ static int s5p_mfc_configure_dma_memory(struct > s5p_mfc_dev *mfc_dev) > return -ENODEV; > } > > + /* Allocate memory for firmware and initialize both banks addresses */ > + ret = s5p_mfc_alloc_firmware(mfc_dev); > + if (ret) { > + device_unregister(mfc_dev->mem_dev[BANK2_CTX]); > + device_unregister(mfc_dev->mem_dev[BANK1_CTX]); > + return ret; > + } > + > + mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma; > + > + bank2_virt = dma_alloc_coherent(mfc_dev->mem_dev[BANK2_CTX], > align_size, > + &bank2_dma_addr, GFP_KERNEL); > + if (!bank2_virt) { > + mfc_err("Allocating bank2 base failed\n"); > + s5p_mfc_release_firmware(mfc_dev); > + device_unregister(mfc_dev->mem_dev[BANK2_CTX]); > + device_unregister(mfc_dev->mem_dev[BANK1_CTX]); > + return -ENOMEM; > + } > + > + /* Valid buffers passed to MFC encoder with LAST_FRAME command > +* should not have address of bank2 - MFC will treat it as a null > frame. > +* To avoid such situation we set bank2 address below the pool > address. > +*/ > + mfc_dev->dma_base[BANK2_CTX] = bank2_dm
Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided
On 02/22/2017 05:05 AM, Thibault Saunier wrote: > Hello, > > > On 02/21/2017 11:19 PM, Hans Verkuil wrote: >> >> On 02/21/2017 11:20 AM, Thibault Saunier wrote: >>> Use colorspace provided by the user as we are only doing scaling and >>> color encoding conversion, we won't be able to transform the colorspace >>> itself and the colorspace won't mater in that operation. >>> >>> Also always use output colorspace on the capture side. >>> >>> Start using 576p as a threashold to compute the colorspace. >>> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace >>> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers >>> don't agree on the display resolution that should be used as a threshold. >>> >>> From EIA CEA 861B about colorimetry for various resolutions: >>> >>>- 5.1 480p, 480i, 576p, 576i, 240p, and 288p >>> The color space used by the 480-line, 576-line, 240-line, and 288-line >>> formats will likely be based on SMPTE 170M [1]. >>>- 5.2 1080i, 1080p, and 720p >>> The color space used by the high definition formats will likely be >>> based on ITU-R BT.709-4 >>> >>> This indicates that in the case that userspace does not specify what >>> colorspace should be used, we should use 576p as a threshold to set >>> V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M. Even if it is >>> only 'likely' and not a requirement it is the best guess we can make. >>> >>> The stream should have been encoded with the information and userspace >>> has to pass it to the driver if it is not the case, otherwise we won't be >>> able to handle it properly anyhow. >>> >>> Also, check for the resolution in G_FMT instead unconditionally setting >>> the V4L2_COLORSPACE_REC709 colorspace. >>> >>> Signed-off-by: Javier Martinez Canillas >>> Signed-off-by: Thibault Saunier >>> Reviewed-by: Andrzej Hajda >>> >>> --- >>> >>> Changes in v5: >>> - Squash commit to always use output colorspace on the capture side >>>inside this one >>> - Fix typo in commit message >>> >>> Changes in v4: >>> - Reword commit message to better back our assumptions on specifications >>> >>> Changes in v3: >>> - Do not check values in the g_fmt functions as Andrzej explained in >>> previous review >>> - Added 'Reviewed-by: Andrzej Hajda ' >>> >>> Changes in v2: None >>> >>> drivers/media/platform/exynos-gsc/gsc-core.c | 20 +++- >>> drivers/media/platform/exynos-gsc/gsc-core.h | 1 + >>> 2 files changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c >>> b/drivers/media/platform/exynos-gsc/gsc-core.c >>> index 59a634201830..772599de8c13 100644 >>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c >>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c >>> @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct >>> v4l2_format *f) >>> } else { >>> min_w = variant->pix_min->target_rot_dis_w; >>> min_h = variant->pix_min->target_rot_dis_h; >>> +pix_mp->colorspace = ctx->out_colorspace; >>> } >>> pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d", >>> @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct >>> v4l2_format *f) >>> pix_mp->num_planes = fmt->num_planes; >>> -if (pix_mp->width >= 1280) /* HD */ >>> -pix_mp->colorspace = V4L2_COLORSPACE_REC709; >>> -else /* SD */ >>> -pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; >>> +if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) { >>> +if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */ >> I'd use || instead of && here. Ditto for the next patch. >> >>> +pix_mp->colorspace = V4L2_COLORSPACE_REC709; >>> +else /* SD */ >>> +pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; >>> +} >> Are you sure this is in fact how it is used? If the source of the video >> is the sensor (camera), then the colorspace is typically SRGB. I'm not >> really sure you should guess here. All a mem2mem driver should do is to >> pass the colorspace etc. information from the output to the capture side, >> and not invent things. That simply makes no sense. > > This is not a camera device but a colorspace conversion device, in many Not really, this is a color encoding conversion device. I.e. it only affects the Y'CbCr encoding and quantization range. The colorspace (aka chromaticities) and transfer function remain unchanged. In fact, I suspect (correct me if I am wrong) that it only converts between RGB, Y'CbCr 601 and Y'CbCr 709 encodings, where RGB is full range and the Y'CbCr formats are limited range. If you pass in limited range RGB it will probably do the wrong thing as I don't seen any quantization checks in the code. So the colorspace and xfer_func fields remain unchanged by this driver. If you want to do this really correctly, then you need to do more. I don't have time right now to go into this in-dept
Re: v4l2: Adding support for multiple MIPI CSI-2 virtual channels
Hi, On Tue, 21 Feb 2017, Ajay kumar wrote: > Hi Everyone, > > On Fri, Feb 17, 2017 at 7:27 PM, Thomas Axelsson > wrote: > > Hi, > > > > I have a v4l2_subdev that provides multiple MIPI CSI-2 Virtual > > Channels. I want to configure each virtual channel individually (e.g. > > set_fmt), but the v4l2 interface does not seem to have a clear way to > > access configuration on a virtual channel level, but only the > > v4l2_subdev as a whole. Using one v4l2_subdev for multiple virtual > > channels by extending the "reg" tag to be an array looks like the > > correct way to do it, based on the mipi-dsi-bus.txt document and > > current device tree endpoint structure. > > > > However, I cannot figure out how to extend e.g. set_fmt/get_fmt subdev > > ioctls to specify which virtual channel the call applies to. Does > > anyone have any advice on how to handle this case? > This would be helpful for my project as well since even I need to > support multiple streams using Virtual Channels. > Can anyone point out to some V4L2 driver, if this kind of support is > already implemented? My understanding is, that MIPI CSI virtual channel handling requires extensions to the V4L2 subdev API. These extensions have been discussed at a media mini-summit almost a year ago, slides are available at [1], but as my priorities shifted away from this work, don't think those extensions ever got implemented. Thanks Guennadi [1] https://linuxtv.org/downloads/presentations/media_summit_2016_san_diego/v4l2-multistream.pdf > > Thanks. > > > > Previous thread: "Device Tree formatting for multiple virtual channels in > > ti-vpe/cal driver?" > > > > > > Best Regards, > > Thomas Axelsson > > > > PS. First e-mail seems to have gotten caught in the spam filter. I > > apologize if this is a duplicate. >
Re: [PATCH v4 12/36] add mux and video interface bridge entity functions
On 02/19/2017 01:28 PM, Pavel Machek wrote: On Wed 2017-02-15 18:19:14, Steve Longerbeam wrote: From: Philipp Zabel Signed-off-by: Philipp Zabel - renamed MEDIA_ENT_F_MUX to MEDIA_ENT_F_VID_MUX Signed-off-by: Steve Longerbeam This is slightly "interesting" format of changelog. Normally signoffs go below. diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst index 3e03dc2..023be29 100644 --- a/Documentation/media/uapi/mediactl/media-types.rst +++ b/Documentation/media/uapi/mediactl/media-types.rst @@ -298,6 +298,28 @@ Types and flags used to represent the media graph elements received on its sink pad and outputs the statistics data on its source pad. +- .. row 29 + + .. _MEDIA-ENT-F-MUX: + + - ``MEDIA_ENT_F_MUX`` And you probably want to rename it here, too. Done, thanks. Steve With that fixed: Reviewed-by: Pavel Machek
Re: [PATCH v2 03/15] media: s5p-mfc: Replace mem_dev_* entries with an array
On Mon, Feb 20, 2017 at 6:38 AM, Marek Szyprowski wrote: > Internal MFC driver device structure contains two pointers to devices used > for DMA memory allocation: mem_dev_l and mem_dev_r. Replace them with the > mem_dev[] array and use defines for accessing particular banks. This will > help to simplify code in the next patches. Hi Marek, The change looks good to me. One comment thought that it would be good to keep the left and right banks in the driver code to be in sync with the DT nomenclature. BANK_L_CTX instead of BANK1_CTX BANK_R_CTX instead of BANK2_CTX > > Signed-off-by: Marek Szyprowski > Reviewed-by: Javier Martinez Canillas > Tested-by: Javier Martinez Canillas > --- > drivers/media/platform/s5p-mfc/s5p_mfc.c| 31 +--- > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 11 - > drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 23 +- > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 8 +++ > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 10 > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c | 32 > ++--- > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 15 ++-- > 7 files changed, 69 insertions(+), 61 deletions(-) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c > b/drivers/media/platform/s5p-mfc/s5p_mfc.c > index ad3d7377f40d..f7664910f12c 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c > @@ -1123,7 +1123,8 @@ static int s5p_mfc_configure_dma_memory(struct > s5p_mfc_dev *mfc_dev) > int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE, > S5P_MFC_IOMMU_DMA_SIZE); > if (ret == 0) > - mfc_dev->mem_dev_l = mfc_dev->mem_dev_r = dev; > + mfc_dev->mem_dev[BANK1_CTX] = > + mfc_dev->mem_dev[BANK2_CTX] = dev; > return ret; > } > > @@ -1131,14 +1132,14 @@ static int s5p_mfc_configure_dma_memory(struct > s5p_mfc_dev *mfc_dev) > * Create and initialize virtual devices for accessing > * reserved memory regions. > */ > - mfc_dev->mem_dev_l = s5p_mfc_alloc_memdev(dev, "left", > - MFC_BANK1_ALLOC_CTX); > - if (!mfc_dev->mem_dev_l) > + mfc_dev->mem_dev[BANK1_CTX] = s5p_mfc_alloc_memdev(dev, "left", > + BANK1_CTX); > + if (!mfc_dev->mem_dev[BANK1_CTX]) > return -ENODEV; > - mfc_dev->mem_dev_r = s5p_mfc_alloc_memdev(dev, "right", > - MFC_BANK2_ALLOC_CTX); > - if (!mfc_dev->mem_dev_r) { > - device_unregister(mfc_dev->mem_dev_l); > + mfc_dev->mem_dev[BANK2_CTX] = s5p_mfc_alloc_memdev(dev, "right", > + BANK2_CTX); > + if (!mfc_dev->mem_dev[BANK2_CTX]) { > + device_unregister(mfc_dev->mem_dev[BANK1_CTX]); > return -ENODEV; > } > > @@ -1154,8 +1155,8 @@ static void s5p_mfc_unconfigure_dma_memory(struct > s5p_mfc_dev *mfc_dev) > return; > } > > - device_unregister(mfc_dev->mem_dev_l); > - device_unregister(mfc_dev->mem_dev_r); > + device_unregister(mfc_dev->mem_dev[BANK1_CTX]); > + device_unregister(mfc_dev->mem_dev[BANK2_CTX]); > } > > /* MFC probe function */ > @@ -1213,8 +1214,10 @@ static int s5p_mfc_probe(struct platform_device *pdev) > goto err_dma; > } > > - vb2_dma_contig_set_max_seg_size(dev->mem_dev_l, DMA_BIT_MASK(32)); > - vb2_dma_contig_set_max_seg_size(dev->mem_dev_r, DMA_BIT_MASK(32)); > + vb2_dma_contig_set_max_seg_size(dev->mem_dev[BANK1_CTX], > + DMA_BIT_MASK(32)); > + vb2_dma_contig_set_max_seg_size(dev->mem_dev[BANK2_CTX], > + DMA_BIT_MASK(32)); > > mutex_init(&dev->mfc_mutex); > init_waitqueue_head(&dev->queue); > @@ -1348,8 +1351,8 @@ static int s5p_mfc_remove(struct platform_device *pdev) > v4l2_device_unregister(&dev->v4l2_dev); > s5p_mfc_release_firmware(dev); > s5p_mfc_unconfigure_dma_memory(dev); > - vb2_dma_contig_clear_max_seg_size(dev->mem_dev_l); > - vb2_dma_contig_clear_max_seg_size(dev->mem_dev_r); > + vb2_dma_contig_clear_max_seg_size(dev->mem_dev[BANK1_CTX]); > + vb2_dma_contig_clear_max_seg_size(dev->mem_dev[BANK2_CTX]); > > s5p_mfc_final_pm(dev); > return 0; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > index 2f1387a4c386..27d4c864e06e 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > @@
[PATCH 1/3] [media] soc-camera: ov5642: Add OF device ID table
The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. Signed-off-by: Javier Martinez Canillas --- drivers/media/i2c/soc_camera/ov5642.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/media/i2c/soc_camera/ov5642.c b/drivers/media/i2c/soc_camera/ov5642.c index 3d185bd622a3..1926f382dfce 100644 --- a/drivers/media/i2c/soc_camera/ov5642.c +++ b/drivers/media/i2c/soc_camera/ov5642.c @@ -1063,9 +1063,18 @@ static const struct i2c_device_id ov5642_id[] = { }; MODULE_DEVICE_TABLE(i2c, ov5642_id); +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id ov5642_of_match[] = { + { .compatible = "ovti,ov5642" }, + { }, +}; +MODULE_DEVICE_TABLE(of, ov5642_of_match); +#endif + static struct i2c_driver ov5642_i2c_driver = { .driver = { .name = "ov5642", + .of_match_table = of_match_ptr(ov5642_of_match), }, .probe = ov5642_probe, .remove = ov5642_remove, -- 2.9.3
[PATCH 2/3] [media] tc358743: Add OF device ID table
The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. Signed-off-by: Javier Martinez Canillas --- drivers/media/i2c/tc358743.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c index f569a05fe105..76baf7a7bd57 100644 --- a/drivers/media/i2c/tc358743.c +++ b/drivers/media/i2c/tc358743.c @@ -1951,9 +1951,18 @@ static struct i2c_device_id tc358743_id[] = { MODULE_DEVICE_TABLE(i2c, tc358743_id); +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id tc358743_of_match[] = { + { .compatible = "toshiba,tc358743" }, + {}, +}; +MODULE_DEVICE_TABLE(of, tc358743_of_match); +#endif + static struct i2c_driver tc358743_driver = { .driver = { .name = "tc358743", + .of_match_table = of_match_ptr(tc358743_of_match), }, .probe = tc358743_probe, .remove = tc358743_remove, -- 2.9.3
[PATCH 3/3] [media] si4713: Add OF device ID table
The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. Signed-off-by: Javier Martinez Canillas --- drivers/media/radio/si4713/si4713.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c index 60f026a58076..f4a53f1e856e 100644 --- a/drivers/media/radio/si4713/si4713.c +++ b/drivers/media/radio/si4713/si4713.c @@ -1656,9 +1656,18 @@ static const struct i2c_device_id si4713_id[] = { }; MODULE_DEVICE_TABLE(i2c, si4713_id); +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id si4713_of_match[] = { + { .compatible = "silabs,si4713" }, + { }, +}; +MODULE_DEVICE_TABLE(of, si4713_of_match); +#endif + static struct i2c_driver si4713_i2c_driver = { .driver = { .name = "si4713", + .of_match_table = of_match_ptr(si4713_of_match), }, .probe = si4713_probe, .remove = si4713_remove, -- 2.9.3
Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt
Le mercredi 22 février 2017 à 10:10 -0300, Thibault Saunier a écrit : > Hello, > > On 02/22/2017 06:29 AM, Andrzej Hajda wrote: > > On 21.02.2017 20:20, Thibault Saunier wrote: > > > It is required by the standard that the field order is set by the > > > driver. > > > > > > Signed-off-by: Thibault Saunier > > > > > > > > > --- > > > > > > Changes in v5: > > > - Just adapt the field and never error out. > > > > > > Changes in v4: None > > > Changes in v3: > > > - Do not check values in the g_fmt functions as Andrzej explained > > > in previous review > > > > > > Changes in v2: > > > - Fix a silly build error that slipped in while rebasing the > > > patches > > > > > > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > index 0976c3e0a5ce..44ed2afe0780 100644 > > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > > > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, > > > void *priv, struct v4l2_format *f) > > > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > > > struct s5p_mfc_fmt *fmt; > > > > > > + if (f->fmt.pix.field == V4L2_FIELD_ANY) > > > + f->fmt.pix.field = V4L2_FIELD_NONE; > > > + > > > > In previous version the only supported field type was NONE, here > > you > > support everything. > > If the only supported format is none you should set 'field' > > unconditionally to NONE, nothing more. > > Afaict we support 2 things: > > 1. NONE > 2. INTERLACE > > Until now we were not checking what was supported or not and > basically > ignoring that info, this patch > keeps the old behaviour making sure to be compliant. > > I had a doubt and was pondering doing: > > ``` diff > > + if (f->fmt.pix.field != V4L2_FIELD_INTERLACED) > + f->fmt.pix.field = V4L2_FIELD_NONE; > + > This looks better to me. > ``` > > instead, it is probably more correct, do you think it is what should > be > done here? > > Regards, > > Thibault > > > > > Regards > > Andrzej > > > > > mfc_debug(2, "Type is %d\n", f->type); > > > if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > > fmt = find_format(f, MFC_FMT_DEC); > > signature.asc Description: This is a digitally signed message part
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Vladimir On 2/22/2017 11:39 AM, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/22/2017 12:57 PM, Ramiro Oliveira wrote: >> Hi Vladimir >> >> On 2/21/2017 10:37 PM, Vladimir Zapolskiy wrote: >>> Hi Sakari, >>> >>> On 02/21/2017 11:48 PM, Sakari Ailus wrote: Hi, Vladimir! How do you do? :-) >>> >>> deferring execution of boring tasks by doing code review :) >>> On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/21/2017 10:13 PM, Ramiro Oliveira wrote: >> Hi Vladimir, >> >> Thank you for your feedback >> >> On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote: >>> Hi Ramiro, >>> >>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: Create device tree bindings documentation. Signed-off-by: Ramiro Oliveira Acked-by: Rob Herring --- .../devicetree/bindings/media/i2c/ov5647.txt | 35 ++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt new file mode 100644 index ..31956426d3b9 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt @@ -0,0 +1,35 @@ +Omnivision OV5647 raw image sensor +- + +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces +and CCI (I2C compatible) control bus. + +Required properties: + +- compatible : "ovti,ov5647". +- reg : I2C slave address of the sensor. +- clocks : Reference to the xclk clock. >>> >>> Is "xclk" clock a pixel clock or something else? >>> >> >> It's an external oscillator. > > hmm, I suppose a clock of any type could serve as a clock for the sensor. > It can be an external oscillator on a particular board, or it can be > something else on another board. Any clock source could be used I presume. >>> >>> That's exactly my point, and it is a reason to rename "xclk" to something >>> more generic. >>> >> >> xclk it's the name being used in the camera datasheet, but I can change it to >> something more generic >> > > Ah, if the name comes from the sensor datasheet, then it should be okay > to keep it. > > > Can you please describe what for does ov5647 sensor need this clock, what > is its function? Camera modules (sensors) quite commonly require an external clock as they do not have an oscillator on their own. A lot of devices under Documentation/devicetree/bindings/media/i2c/ have similar properties. >>> >>> So, what should be a better replacement of "xclk" in the description above? >>> >>> E.g. >>> >>> - clocks: Phandle to a device supply clock. >>> > >> +- clock-names : Should be "xclk". >>> >>> We got an agreement that "clock-names" property is removed, nevertheless >>> if it is added back, is should not be "xclk". >>> >>> >>> You can remove this property, because there is only one source clock. >>> >> >> Ok. >> +- clock-frequency : Frequency of the xclk clock. >>> >>> And after the last updates in the driver this property can be removed >>> as well. >>> >> >> But I'm still using clk_get_rate in the driver, if I remove the >> frequency here >> the probing will fail. >> > > I doubt it, there should be no connection between a custom > "clock-frequency" > device tree property in a clock consumer device node and clk_get_rate() > function > from the CCF, which takes a clock provider as its argument. The purpose is to make sure the clock frequency is really usable for the device, in this particular case the driver can work with one particular frequency. That said, the driver does not appear to use the property at the moment. It should. It'd be good to verify that the rate matches: clk_set_rate() is not guaranteed to produce the requested clock rate, and the driver could conceivably be updated with support for more frequencies. There are typically a few frequencies that a SoC such a sensor is connected can support, and 25 MHz is not one of the common frequencies. With this property, the frequency would be always there explicitly. >>> >>> I can provide my arguments given at v8 review time again, since I don't >>> see a contradiction with my older comments. >>> >>> Briefly "clock-frequency" as a device tree property on a consumer side >>> can be considered as redundant, because there
Re: [PATCH RESEND 1/1] media: platform: Renesas IMR driver
On Sat, Feb 11, 2017 at 11:02:01PM +0300, Sergei Shtylyov wrote: > From: Konstantin Kozhevnikov > > The image renderer light extended 4 (IMR-LX4) or the distortion correction > engine is a drawing processor with a simple instruction system capable of > referencing data on an external memory as 2D texture data and performing > texture mapping and drawing with respect to any shape that is split into > triangular objects. > > This V4L2 memory-to-memory device driver only supports image renderer found > in the R-Car gen3 SoCs; the R-Car gen2 support can be added later... > > [Sergei: merged 2 original patches, added the patch description, removed > unrelated parts, added the binding document, ported the driver to the > modern kernel, renamed the UAPI header file and the guard macros to match > the driver name, extended the copyrights, fixed up Kconfig prompt/depends/ > help, made use of the BIT()/GENMASK() macros, sorted #include's, removed > leading dots and fixed grammar in the comments, fixed up indentation to > use tabs where possible, renamed IMR_DLSR to IMR_DLPR to match the manual, > separated the register offset/bit #define's, removed *inline* from .c file, > fixed lines over 80 columns, removed useless parens, operators, casts, > braces, variables, #include's, (commented out) statements, and even > function, inserted empty line after desclaration, removed extra empty > lines, reordered some local variable desclarations, removed calls to > 4l2_err() on kmalloc() failure, fixed the error returned by imr_default(), > avoided code duplication in the IRQ handler, used '__packed' for the UAPI > structures, enclosed the macro parameters in parens, exchanged the values > of IMR_MAP_AUTO[SD]G macros.] > > Signed-off-by: Konstantin Kozhevnikov > > Signed-off-by: Sergei Shtylyov > > --- > This patch is against the 'media_tree.git' repo's 'master' branch. > > Documentation/devicetree/bindings/media/rcar_imr.txt | 23 > drivers/media/platform/Kconfig | 13 > drivers/media/platform/Makefile |1 > drivers/media/platform/rcar_imr.c| 1923 > +++ > include/uapi/linux/rcar_imr.h| 94 > 5 files changed, 2054 insertions(+) > > Index: media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt > === > --- /dev/null > +++ media_tree/Documentation/devicetree/bindings/media/rcar_imr.txt > @@ -0,0 +1,23 @@ > +Renesas R-Car Image Renderer (Distortion Correction Engine) > +--- > + > +The image renderer or the distortion correction engine is a drawing > processor > +with a simple instruction system capable of referencing data in external > memory > +as 2D texture data and performing texture mapping and drawing with respect > to > +any shape that is split into triangular objects. Please fix extra spaces in here. > + > +Required properties: > +- compatible: must be "renesas,imr-lx4" for the image renderer light > extended 4 > + (IMR-LX4) found in the R-Car gen3 SoCs; Needs an SoC specific compatible string too. The description is above, so you just need to list the compatible strings. > +- reg: offset and length of the register block; > +- interrupts: interrupt specifier; How many interrupts? > +- clocks: clock phandle and specifier pair. How many clocks? > + > +Example: > + > + imr-lx4@fe86 { > + compatible = "renesas,imr-lx4"; > + reg = <0 0xfe86 0 0x2000>; > + interrupts = ; > + clocks = <&cpg CPG_MOD 823>; > + };
Re: [PATCH v6 2/9] doc: DT: venus: binding document for Qualcomm video driver
On Wed, Feb 22, 2017 at 3:25 AM, Stanimir Varbanov wrote: > Hi Rob, > > On 02/22/2017 02:09 AM, Rob Herring wrote: >> On Tue, Feb 07, 2017 at 03:10:17PM +0200, Stanimir Varbanov wrote: >>> Add binding document for Venus video encoder/decoder driver >>> >>> Cc: Rob Herring >>> Cc: devicet...@vger.kernel.org >>> Signed-off-by: Stanimir Varbanov >>> --- >>> Changes since previous v5: >>> * dropped rproc phandle (remoteproc is not used anymore) >>> * added subnodes paragraph with descrition of three subnodes: >>> - video-decoder and video-encoder - describes decoder (core0) and >>> encoder (core1) power-domains and clocks (applicable for msm8996 >>> Venus core). >>> - video-firmware - needed to get reserved memory region where the >>> firmware is stored. >>> >>> .../devicetree/bindings/media/qcom,venus.txt | 112 >>> + >>> 1 file changed, 112 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >>> >>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt >>> b/Documentation/devicetree/bindings/media/qcom,venus.txt >>> new file mode 100644 >>> index ..4427af3ca5a5 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >>> @@ -0,0 +1,112 @@ >> >> [...] >> >>> +* Subnodes >>> +The Venus node must contain three subnodes representing video-decoder, >>> +video-encoder and video-firmware. >> >> [...] >> >>> +The video-firmware subnode should contain: >>> + >>> +- memory-region: >>> +Usage: required >>> +Value type: >>> +Definition: reference to the reserved-memory for the memory region >>> + >>> +* An Example >>> +video-codec@1d0 { >>> +compatible = "qcom,msm8916-venus"; >>> +reg = <0x01d0 0xff000>; >>> +interrupts = ; >>> +clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, >>> + <&gcc GCC_VENUS0_AHB_CLK>, >>> + <&gcc GCC_VENUS0_AXI_CLK>; >>> +clock-names = "core", "iface", "bus"; >>> +power-domains = <&gcc VENUS_GDSC>; >>> +iommus = <&apps_iommu 5>; >>> + >>> +video-decoder { >>> +compatible = "venus-decoder"; >>> +clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >>> +clock-names = "core"; >>> +power-domains = <&mmcc VENUS_CORE0_GDSC>; >>> +}; >>> + >>> +video-encoder { >>> +compatible = "venus-encoder"; >>> +clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >>> +clock-names = "core"; >>> +power-domains = <&mmcc VENUS_CORE1_GDSC>; >>> +}; >>> + >>> +video-firmware { >>> +memory-region = <&venus_mem>; >> >> Why does this need to be a sub node? > > Because firmware reserved memory region must have separate struct > device, otherwise allocating video buffers (and map them through iommu) > for video-codec will fail because dma_alloc_coherent trying to allocate > from per-device coherent area. Why can't the struct device be the video-codec device? Looking at the code, I don't see why you need the 2nd struct device. In any case, this is letting the driver design the binding which is wrong. From a binding perspective, there's no reason to have this node. Rob
[PATCH] media: vpif: request enable-gpios
This change is needed to make vpif capture work on the da850-evm board where the capture function must be selected on the UI expander. Signed-off-by: Bartosz Golaszewski --- drivers/media/platform/davinci/vpif_capture.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c index b62a399..7dea358 100644 --- a/drivers/media/platform/davinci/vpif_capture.c +++ b/drivers/media/platform/davinci/vpif_capture.c @@ -24,6 +24,7 @@ #include #include #include +#include #include @@ -1433,6 +1434,7 @@ static __init int vpif_probe(struct platform_device *pdev) { struct vpif_subdev_info *subdevdata; struct i2c_adapter *i2c_adap; + struct gpio_descs *descs; struct resource *res; int subdev_count; int res_idx = 0; @@ -1443,6 +1445,11 @@ static __init int vpif_probe(struct platform_device *pdev) return -EINVAL; } + descs = devm_gpiod_get_array_optional(&pdev->dev, + "enable", GPIOD_OUT_HIGH); + if (IS_ERR(descs)) + dev_err(&pdev->dev, "Error requesting enable GPIOs\n"); + vpif_dev = &pdev->dev; err = initialize_vpif(); -- 2.9.3
Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt
Hello, On 02/22/2017 06:29 AM, Andrzej Hajda wrote: On 21.02.2017 20:20, Thibault Saunier wrote: It is required by the standard that the field order is set by the driver. Signed-off-by: Thibault Saunier --- Changes in v5: - Just adapt the field and never error out. Changes in v4: None Changes in v3: - Do not check values in the g_fmt functions as Andrzej explained in previous review Changes in v2: - Fix a silly build error that slipped in while rebasing the patches drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c index 0976c3e0a5ce..44ed2afe0780 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f) struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; struct s5p_mfc_fmt *fmt; + if (f->fmt.pix.field == V4L2_FIELD_ANY) + f->fmt.pix.field = V4L2_FIELD_NONE; + In previous version the only supported field type was NONE, here you support everything. If the only supported format is none you should set 'field' unconditionally to NONE, nothing more. Afaict we support 2 things: 1. NONE 2. INTERLACE Until now we were not checking what was supported or not and basically ignoring that info, this patch keeps the old behaviour making sure to be compliant. I had a doubt and was pondering doing: ``` diff + if (f->fmt.pix.field != V4L2_FIELD_INTERLACED) + f->fmt.pix.field = V4L2_FIELD_NONE; + ``` instead, it is probably more correct, do you think it is what should be done here? Regards, Thibault Regards Andrzej mfc_debug(2, "Type is %d\n", f->type); if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { fmt = find_format(f, MFC_FMT_DEC);
Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided
Hello, On 02/21/2017 11:19 PM, Hans Verkuil wrote: On 02/21/2017 11:20 AM, Thibault Saunier wrote: Use colorspace provided by the user as we are only doing scaling and color encoding conversion, we won't be able to transform the colorspace itself and the colorspace won't mater in that operation. Also always use output colorspace on the capture side. Start using 576p as a threashold to compute the colorspace. The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers don't agree on the display resolution that should be used as a threshold. From EIA CEA 861B about colorimetry for various resolutions: - 5.1 480p, 480i, 576p, 576i, 240p, and 288p The color space used by the 480-line, 576-line, 240-line, and 288-line formats will likely be based on SMPTE 170M [1]. - 5.2 1080i, 1080p, and 720p The color space used by the high definition formats will likely be based on ITU-R BT.709-4 This indicates that in the case that userspace does not specify what colorspace should be used, we should use 576p as a threshold to set V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M. Even if it is only 'likely' and not a requirement it is the best guess we can make. The stream should have been encoded with the information and userspace has to pass it to the driver if it is not the case, otherwise we won't be able to handle it properly anyhow. Also, check for the resolution in G_FMT instead unconditionally setting the V4L2_COLORSPACE_REC709 colorspace. Signed-off-by: Javier Martinez Canillas Signed-off-by: Thibault Saunier Reviewed-by: Andrzej Hajda --- Changes in v5: - Squash commit to always use output colorspace on the capture side inside this one - Fix typo in commit message Changes in v4: - Reword commit message to better back our assumptions on specifications Changes in v3: - Do not check values in the g_fmt functions as Andrzej explained in previous review - Added 'Reviewed-by: Andrzej Hajda ' Changes in v2: None drivers/media/platform/exynos-gsc/gsc-core.c | 20 +++- drivers/media/platform/exynos-gsc/gsc-core.h | 1 + 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c index 59a634201830..772599de8c13 100644 --- a/drivers/media/platform/exynos-gsc/gsc-core.c +++ b/drivers/media/platform/exynos-gsc/gsc-core.c @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) } else { min_w = variant->pix_min->target_rot_dis_w; min_h = variant->pix_min->target_rot_dis_h; + pix_mp->colorspace = ctx->out_colorspace; } pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h = %d", @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f) pix_mp->num_planes = fmt->num_planes; - if (pix_mp->width >= 1280) /* HD */ - pix_mp->colorspace = V4L2_COLORSPACE_REC709; - else /* SD */ - pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; + if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) { + if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */ I'd use || instead of && here. Ditto for the next patch. + pix_mp->colorspace = V4L2_COLORSPACE_REC709; + else /* SD */ + pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M; + } Are you sure this is in fact how it is used? If the source of the video is the sensor (camera), then the colorspace is typically SRGB. I'm not really sure you should guess here. All a mem2mem driver should do is to pass the colorspace etc. information from the output to the capture side, and not invent things. That simply makes no sense. This is not a camera device but a colorspace conversion device, in many cases the info will not be passed by the userspace, basically because the info is not encoded in the media stream (this often happens). I am not inventing here, just making sure we use the most likely value in case none was provided (and if none was provided it should always be correct given that the encoded stream was not broken). In the case the source is a camera and then we use the colorspace converter then the info should copied from the camera to the transcoding node (by userspace) so there should be no problem. What I am doing here is what is done in other drivers. Regards, Thibault We may have to update the documentation or v4l2-compliance test if this is an issue. The more I think about this, the more I think we shouldn't do this guessing game. Regards, Hans + + if (V4L2_TYPE_IS_OUTPUT(f->type)) + ctx->out_colorspace = pix_mp->colorspace; for (i = 0; i < pix_mp->num_planes; ++i) { struct v4l2_plane_pix_format *plane_fmt = &pi
Re: [PATCH v9 2/2] Add support for OV5647 sensor.
On 02/22/2017 12:51 PM, Ramiro Oliveira wrote: > Hi Zakari, > > On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote: >> Hi Ramiro, >> >> On 02/21/2017 06:42 PM, Ramiro Oliveira wrote: >>> Hi Vladimir, >>> >>> Thank you for your feedback >>> >>> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote: Hi Ramiro, please find some review comments below. On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: > The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8 > and RAW 10 output formats, and MIPI CSI-2 interface. > > The driver adds support for 640x480 RAW 8. > > Signed-off-by: Ramiro Oliveira > --- [snip] > + > +struct ov5647 { > + struct v4l2_subdev sd; > + struct media_padpad; > + struct mutexlock; > + struct v4l2_mbus_framefmt format; > + unsigned intwidth; > + unsigned intheight; > + int power_count; > + struct clk *xclk; > + /* External clock frequency currently supported is 30MHz */ > + u32 xclk_freq; See a comment about 25MHz vs 30MHz below. Also I assume you can remove 'xclk_freq' from the struct fields, it can be replaced by a local variable. >>> >>> I'll do that. >>> > +}; [snip] > + > +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > +{ > + int ret; > + unsigned char data_w[2] = { reg >> 8, reg & 0xff }; > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + > + ret = i2c_master_send(client, data_w, 2); > + if (ret < 0) { > + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", s/i2c read error/i2c write error/ >>> >>> I'm not sure I understand what you mean. >> >> That's a sed expression for string substitution. Here you do >> i2c_master_send() >> but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo >> to fix. >> > > Ohh... now I see. I'll change it. > > + __func__, reg); > + return ret; > + } > + >> >> [snip] >> > + > +static int sensor_power(struct v4l2_subdev *sd, int on) >> >> On the caller's side (functions ov5647_open() and ov5647_close()) the second >> argument of the function is of 'bool' type, however .s_power callback from >> struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as >> 'int'. >> >> It's just a nitpicking, please feel free to ignore the comment above or >> please consider to change the arguments on callers' side to integers. >> >> Also you may consider to add 'ov5647_' prefix to the function name to >> distinguish it from a potentially added in future sensor_power() function, >> the original name sounds too generic. >> > > OK. I'll add the prefix and change the variable type from int to bool. > Just to eliminate any potential misunderstanding, if you consider to reuse the current sensor_power() function, please change variables from bool to int on a caller's side, the signature of the function shall not be changed to match .s_power type. > +{ > + int ret; > + struct ov5647 *ov5647 = to_state(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + > + ret = 0; > + mutex_lock(&ov5647->lock); > + > + if (on && !ov5647->power_count) { > + dev_dbg(&client->dev, "OV5647 power on\n"); > + > + clk_set_rate(ov5647->xclk, ov5647->xclk_freq); Now clk_set_rate() is redundant, please remove it. If once it is needed again, please move it to the .probe function, so it is called only once in the runtime. >>> >>> Ok. I'll remove it for now. >>> > + > + ret = clk_prepare_enable(ov5647->xclk); I wonder would it be possible to unload the driver or to unbind the device and leave the clock unintentionally enabled? If yes, then this is a bug. >>> >>> You're saying that if the driver was unloaded and the clock was left enabled >>> when the driver was loaded again this line would cause an error? >> >> Not exactly, here I saw a potential resource leak, namely a potentially left >> prepared/enabled clock. >> >>> >>> Should I disable the clock when the driver is removed? >>> >> >> The driver (and framework) shall guarantee that when it is detached from >> device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 >> device), >> all acquired resources are released. >> >> But in this particular case most probably I've been overly alert, I believe >> that V4L2 framework correcly handles device power states, so please ignore my >> comment. >> >> To add something valuable to the review, could you please confirm that >> ov5647_subdev_internal_ops data is in use by the driver? >> >> E.g. shouldn't it be registered by >> >> sd->internal_
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Ramiro, On 02/22/2017 12:57 PM, Ramiro Oliveira wrote: > Hi Vladimir > > On 2/21/2017 10:37 PM, Vladimir Zapolskiy wrote: >> Hi Sakari, >> >> On 02/21/2017 11:48 PM, Sakari Ailus wrote: >>> Hi, Vladimir! >>> >>> How do you do? :-) >> >> deferring execution of boring tasks by doing code review :) >> >>> On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote: Hi Ramiro, On 02/21/2017 10:13 PM, Ramiro Oliveira wrote: > Hi Vladimir, > > Thank you for your feedback > > On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote: >> Hi Ramiro, >> >> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: >>> Create device tree bindings documentation. >>> >>> Signed-off-by: Ramiro Oliveira >>> Acked-by: Rob Herring >>> --- >>> .../devicetree/bindings/media/i2c/ov5647.txt | 35 >>> ++ >>> 1 file changed, 35 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/media/i2c/ov5647.txt >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt >>> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt >>> new file mode 100644 >>> index ..31956426d3b9 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt >>> @@ -0,0 +1,35 @@ >>> +Omnivision OV5647 raw image sensor >>> +- >>> + >>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data >>> interfaces >>> +and CCI (I2C compatible) control bus. >>> + >>> +Required properties: >>> + >>> +- compatible : "ovti,ov5647". >>> +- reg : I2C slave address of the sensor. >>> +- clocks : Reference to the xclk clock. >> >> Is "xclk" clock a pixel clock or something else? >> > > It's an external oscillator. hmm, I suppose a clock of any type could serve as a clock for the sensor. It can be an external oscillator on a particular board, or it can be something else on another board. >>> >>> Any clock source could be used I presume. >>> >> >> That's exactly my point, and it is a reason to rename "xclk" to something >> more generic. >> > > xclk it's the name being used in the camera datasheet, but I can change it to > something more generic > Ah, if the name comes from the sensor datasheet, then it should be okay to keep it. Can you please describe what for does ov5647 sensor need this clock, what is its function? >>> >>> Camera modules (sensors) quite commonly require an external clock as they do >>> not have an oscillator on their own. A lot of devices under >>> Documentation/devicetree/bindings/media/i2c/ have similar properties. >>> >> >> So, what should be a better replacement of "xclk" in the description above? >> >> E.g. >> >> - clocks : Phandle to a device supply clock. >> > >>> +- clock-names : Should be "xclk". >> >> We got an agreement that "clock-names" property is removed, nevertheless >> if it is added back, is should not be "xclk". >> >> >> You can remove this property, because there is only one source clock. >> > > Ok. > >>> +- clock-frequency : Frequency of the xclk clock. >> >> And after the last updates in the driver this property can be removed as >> well. >> > > But I'm still using clk_get_rate in the driver, if I remove the frequency > here > the probing will fail. > I doubt it, there should be no connection between a custom "clock-frequency" device tree property in a clock consumer device node and clk_get_rate() function from the CCF, which takes a clock provider as its argument. >>> >>> The purpose is to make sure the clock frequency is really usable for the >>> device, in this particular case the driver can work with one particular >>> frequency. >>> >>> That said, the driver does not appear to use the property at the moment. It >>> should. >>> >>> It'd be good to verify that the rate matches: clk_set_rate() is not >>> guaranteed to produce the requested clock rate, and the driver could >>> conceivably be updated with support for more frequencies. There are >>> typically a few frequencies that a SoC such a sensor is connected can >>> support, and 25 MHz is not one of the common frequencies. With this >>> property, the frequency would be always there explicitly. >>> >> >> I can provide my arguments given at v8 review time again, since I don't >> see a contradiction with my older comments. >> >> Briefly "clock-frequency" as a device tree property on a consumer side >> can be considered as redundant, because there is a mechanism to specify >> a wanted clock frequency on a clock provider side right in a board DTB. >> >> So, the clock frequency set up is delegated to CCF, and when any other >> than 25 MHz frequencies are got
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Vladimir On 2/21/2017 10:37 PM, Vladimir Zapolskiy wrote: > Hi Sakari, > > On 02/21/2017 11:48 PM, Sakari Ailus wrote: >> Hi, Vladimir! >> >> How do you do? :-) > > deferring execution of boring tasks by doing code review :) > >> On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote: >>> Hi Ramiro, >>> >>> On 02/21/2017 10:13 PM, Ramiro Oliveira wrote: Hi Vladimir, Thank you for your feedback On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: >> Create device tree bindings documentation. >> >> Signed-off-by: Ramiro Oliveira >> Acked-by: Rob Herring >> --- >> .../devicetree/bindings/media/i2c/ov5647.txt | 35 >> ++ >> 1 file changed, 35 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/media/i2c/ov5647.txt >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt >> b/Documentation/devicetree/bindings/media/i2c/ov5647.txt >> new file mode 100644 >> index ..31956426d3b9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt >> @@ -0,0 +1,35 @@ >> +Omnivision OV5647 raw image sensor >> +- >> + >> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data >> interfaces >> +and CCI (I2C compatible) control bus. >> + >> +Required properties: >> + >> +- compatible: "ovti,ov5647". >> +- reg : I2C slave address of the sensor. >> +- clocks: Reference to the xclk clock. > > Is "xclk" clock a pixel clock or something else? > It's an external oscillator. >>> >>> hmm, I suppose a clock of any type could serve as a clock for the sensor. >>> It can be an external oscillator on a particular board, or it can be >>> something else on another board. >> >> Any clock source could be used I presume. >> > > That's exactly my point, and it is a reason to rename "xclk" to something > more generic. > xclk it's the name being used in the camera datasheet, but I can change it to something more generic >>> >>> Can you please describe what for does ov5647 sensor need this clock, what >>> is its function? >> >> Camera modules (sensors) quite commonly require an external clock as they do >> not have an oscillator on their own. A lot of devices under >> Documentation/devicetree/bindings/media/i2c/ have similar properties. >> > > So, what should be a better replacement of "xclk" in the description above? > > E.g. > > - clocks : Phandle to a device supply clock. > >>> >> +- clock-names : Should be "xclk". > > We got an agreement that "clock-names" property is removed, nevertheless > if it is added back, is should not be "xclk". > > > You can remove this property, because there is only one source clock. > Ok. >> +- clock-frequency : Frequency of the xclk clock. > > And after the last updates in the driver this property can be removed as > well. > But I'm still using clk_get_rate in the driver, if I remove the frequency here the probing will fail. >>> >>> I doubt it, there should be no connection between a custom "clock-frequency" >>> device tree property in a clock consumer device node and clk_get_rate() >>> function >>> from the CCF, which takes a clock provider as its argument. >> >> The purpose is to make sure the clock frequency is really usable for the >> device, in this particular case the driver can work with one particular >> frequency. >> >> That said, the driver does not appear to use the property at the moment. It >> should. >> >> It'd be good to verify that the rate matches: clk_set_rate() is not >> guaranteed to produce the requested clock rate, and the driver could >> conceivably be updated with support for more frequencies. There are >> typically a few frequencies that a SoC such a sensor is connected can >> support, and 25 MHz is not one of the common frequencies. With this >> property, the frequency would be always there explicitly. >> > > I can provide my arguments given at v8 review time again, since I don't > see a contradiction with my older comments. > > Briefly "clock-frequency" as a device tree property on a consumer side > can be considered as redundant, because there is a mechanism to specify > a wanted clock frequency on a clock provider side right in a board DTB. > > So, the clock frequency set up is delegated to CCF, and when any other > than 25 MHz frequencies are got supported, that's only the matter of > driver updates, DTBs won't be touched. > In the driver, I'm using this piece of code to check that the frequency is 25Mhz xclk_freq = clk_get_rate(sensor->xclk); if (xclk_freq != 2500) { dev_err(dev, "
Re: [PATCH v9 2/2] Add support for OV5647 sensor.
Hi Zakari, On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/21/2017 06:42 PM, Ramiro Oliveira wrote: >> Hi Vladimir, >> >> Thank you for your feedback >> >> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote: >>> Hi Ramiro, >>> >>> please find some review comments below. >>> >>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8 and RAW 10 output formats, and MIPI CSI-2 interface. The driver adds support for 640x480 RAW 8. Signed-off-by: Ramiro Oliveira --- >>> >>> [snip] >>> + +struct ov5647 { + struct v4l2_subdev sd; + struct media_padpad; + struct mutexlock; + struct v4l2_mbus_framefmt format; + unsigned intwidth; + unsigned intheight; + int power_count; + struct clk *xclk; + /* External clock frequency currently supported is 30MHz */ + u32 xclk_freq; >>> >>> See a comment about 25MHz vs 30MHz below. >>> >>> Also I assume you can remove 'xclk_freq' from the struct fields, >>> it can be replaced by a local variable. >>> >> >> I'll do that. >> +}; >>> >>> [snip] >>> + +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) +{ + int ret; + unsigned char data_w[2] = { reg >> 8, reg & 0xff }; + struct i2c_client *client = v4l2_get_subdevdata(sd); + + ret = i2c_master_send(client, data_w, 2); + if (ret < 0) { + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", >>> >>> s/i2c read error/i2c write error/ >>> >> >> I'm not sure I understand what you mean. > > That's a sed expression for string substitution. Here you do i2c_master_send() > but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo to > fix. > Ohh... now I see. I'll change it. + __func__, reg); + return ret; + } + > > [snip] > + +static int sensor_power(struct v4l2_subdev *sd, int on) > > On the caller's side (functions ov5647_open() and ov5647_close()) the second > argument of the function is of 'bool' type, however .s_power callback from > struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as > 'int'. > > It's just a nitpicking, please feel free to ignore the comment above or > please consider to change the arguments on callers' side to integers. > > Also you may consider to add 'ov5647_' prefix to the function name to > distinguish it from a potentially added in future sensor_power() function, > the original name sounds too generic. > OK. I'll add the prefix and change the variable type from int to bool. +{ + int ret; + struct ov5647 *ov5647 = to_state(sd); + struct i2c_client *client = v4l2_get_subdevdata(sd); + + ret = 0; + mutex_lock(&ov5647->lock); + + if (on && !ov5647->power_count) { + dev_dbg(&client->dev, "OV5647 power on\n"); + + clk_set_rate(ov5647->xclk, ov5647->xclk_freq); >>> >>> Now clk_set_rate() is redundant, please remove it. >>> >>> If once it is needed again, please move it to the .probe function, so >>> it is called only once in the runtime. >>> >> >> Ok. I'll remove it for now. >> + + ret = clk_prepare_enable(ov5647->xclk); >>> >>> I wonder would it be possible to unload the driver or to unbind the device >>> and leave the clock unintentionally enabled? If yes, then this is a bug. >>> >> >> You're saying that if the driver was unloaded and the clock was left enabled >> when the driver was loaded again this line would cause an error? > > Not exactly, here I saw a potential resource leak, namely a potentially left > prepared/enabled clock. > >> >> Should I disable the clock when the driver is removed? >> > > The driver (and framework) shall guarantee that when it is detached from > device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 > device), > all acquired resources are released. > > But in this particular case most probably I've been overly alert, I believe > that V4L2 framework correcly handles device power states, so please ignore my > comment. > > To add something valuable to the review, could you please confirm that > ov5647_subdev_internal_ops data is in use by the driver? > > E.g. shouldn't it be registered by > > sd->internal_ops = &ov5647_subdev_internal_ops; > > before calling v4l2_async_register_subdev(sd) ? > You're right, it's not being registered. I think I'll remove them since they aren't being used. > -- > With best wishes, > Vladimir > -- Best Regards Ramiro Oliveira ramiro.olive...@synopsys.com
Re: [PATCH v2 17/19] [media] lirc: implement reading scancode
On Wed, Feb 22, 2017 at 08:14:50AM +0800, kbuild test robot wrote: > Hi Sean, > > [auto build test ERROR on linuxtv-media/master] > [also build test ERROR on next-20170221] > [cannot apply to v4.10] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Sean-Young/Teach-lirc-how-to-send-and-receive-scancodes/20170222-073718 > base: git://linuxtv.org/media_tree.git master > config: i386-randconfig-x015-201708 (attached as .config) > compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > Note: the > linux-review/Sean-Young/Teach-lirc-how-to-send-and-receive-scancodes/20170222-073718 > HEAD 9a4d3444d507190ad7996731c8c7e4ef80979de4 builds fine. > It only hurts bisectibility. > > All error/warnings (new ones prefixed by >>): > >drivers/media/rc/ir-lirc-codec.c: In function 'ir_lirc_poll': > >> drivers/media/rc/ir-lirc-codec.c:393:7: error: 'drv' undeclared (first use > >> in this function) > if (!drv->attached) { Oops, too much rebasing without testing. :( The final commit compiles fine, I'll have to do some better testing of each commit and resend. Sean
Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt
On 21.02.2017 20:20, Thibault Saunier wrote: > It is required by the standard that the field order is set by the > driver. > > Signed-off-by: Thibault Saunier > > --- > > Changes in v5: > - Just adapt the field and never error out. > > Changes in v4: None > Changes in v3: > - Do not check values in the g_fmt functions as Andrzej explained in previous > review > > Changes in v2: > - Fix a silly build error that slipped in while rebasing the patches > > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > index 0976c3e0a5ce..44ed2afe0780 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file, void *priv, > struct v4l2_format *f) > struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp; > struct s5p_mfc_fmt *fmt; > > + if (f->fmt.pix.field == V4L2_FIELD_ANY) > + f->fmt.pix.field = V4L2_FIELD_NONE; > + In previous version the only supported field type was NONE, here you support everything. If the only supported format is none you should set 'field' unconditionally to NONE, nothing more. Regards Andrzej > mfc_debug(2, "Type is %d\n", f->type); > if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > fmt = find_format(f, MFC_FMT_DEC);
Re: [PATCH v6 2/9] doc: DT: venus: binding document for Qualcomm video driver
Hi Rob, On 02/22/2017 02:09 AM, Rob Herring wrote: > On Tue, Feb 07, 2017 at 03:10:17PM +0200, Stanimir Varbanov wrote: >> Add binding document for Venus video encoder/decoder driver >> >> Cc: Rob Herring >> Cc: devicet...@vger.kernel.org >> Signed-off-by: Stanimir Varbanov >> --- >> Changes since previous v5: >> * dropped rproc phandle (remoteproc is not used anymore) >> * added subnodes paragraph with descrition of three subnodes: >> - video-decoder and video-encoder - describes decoder (core0) and >> encoder (core1) power-domains and clocks (applicable for msm8996 >> Venus core). >> - video-firmware - needed to get reserved memory region where the >> firmware is stored. >> >> .../devicetree/bindings/media/qcom,venus.txt | 112 >> + >> 1 file changed, 112 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >> >> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt >> b/Documentation/devicetree/bindings/media/qcom,venus.txt >> new file mode 100644 >> index ..4427af3ca5a5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >> @@ -0,0 +1,112 @@ > > [...] > >> +* Subnodes >> +The Venus node must contain three subnodes representing video-decoder, >> +video-encoder and video-firmware. > > [...] > >> +The video-firmware subnode should contain: >> + >> +- memory-region: >> +Usage: required >> +Value type: >> +Definition: reference to the reserved-memory for the memory region >> + >> +* An Example >> +video-codec@1d0 { >> +compatible = "qcom,msm8916-venus"; >> +reg = <0x01d0 0xff000>; >> +interrupts = ; >> +clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, >> + <&gcc GCC_VENUS0_AHB_CLK>, >> + <&gcc GCC_VENUS0_AXI_CLK>; >> +clock-names = "core", "iface", "bus"; >> +power-domains = <&gcc VENUS_GDSC>; >> +iommus = <&apps_iommu 5>; >> + >> +video-decoder { >> +compatible = "venus-decoder"; >> +clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >> +clock-names = "core"; >> +power-domains = <&mmcc VENUS_CORE0_GDSC>; >> +}; >> + >> +video-encoder { >> +compatible = "venus-encoder"; >> +clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >> +clock-names = "core"; >> +power-domains = <&mmcc VENUS_CORE1_GDSC>; >> +}; >> + >> +video-firmware { >> +memory-region = <&venus_mem>; > > Why does this need to be a sub node? Because firmware reserved memory region must have separate struct device, otherwise allocating video buffers (and map them through iommu) for video-codec will fail because dma_alloc_coherent trying to allocate from per-device coherent area. -- regards, Stan