Re: [PATCH v2 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function

[Marek Szyprowski]

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 Institute Poland

Re: [PATCH] dma-buf: add support for compat ioctl

[Sumit Semwal]

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

[Hans Verkuil]

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.

[Laurent Pinchart]

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

[Steve Longerbeam]

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

[Steve Longerbeam]

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

[Steve Longerbeam]

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

[Steve Longerbeam]

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

[Steve Longerbeam]

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

[Steve Longerbeam]

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

[Sean Young]

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, _type);
if (rc < 0)
goto out_table;
dev->enabled_protocols = rc_type;
}
 
+   if (dev->driver_type == RC_DRIVER_IR_RAW)
+   ir_raw_load_modules(_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

[Sean Young]

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()'

[Christophe JAILLET]

'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.

[Mauro Carvalho Chehab]

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

[Mats Randgaard (matrandg)]

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

[Thibault Saunier]

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 

Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided

[Nicolas Dufresne]

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 

Re: Looking more details and reasons for using orig_add_limit.

[Mauro Carvalho Chehab]

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));
>  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));
> >> 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) 

Re: Dead code in v4l2-mem2mem.c?

[Laurent Pinchart]

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.

[Laurent Pinchart]

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));
>  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));
> >> 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 

Re: v4l2: Adding support for multiple MIPI CSI-2 virtual channels

[Laurent Pinchart]

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

[Sergei Shtylyov]

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

[Daniel Vetter]

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

[Thibault Saunier]

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 

Re: [PATCH v2 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function

[Shuah Khan]

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 = _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 = _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,
> +   _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.
> +*/
> +   

Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided

[Hans Verkuil]

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 

Re: v4l2: Adding support for multiple MIPI CSI-2 virtual channels

[Guennadi Liakhovetski]

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

[Steve Longerbeam]

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

[Shuah Khan]

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(>mfc_mutex);
> init_waitqueue_head(>queue);
> @@ -1348,8 +1351,8 @@ static int s5p_mfc_remove(struct platform_device *pdev)
> v4l2_device_unregister(>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
> --- 

[PATCH 1/3] [media] soc-camera: ov5642: Add OF device ID table

[Javier Martinez Canillas]

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

[Javier Martinez Canillas]

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

[Javier Martinez Canillas]

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

[Nicolas Dufresne]

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 = >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

[Ramiro Oliveira]

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 

Re: [PATCH RESEND 1/1] media: platform: Renesas IMR driver

[Rob Herring]

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_MOD 823>;
> + };

Re: [PATCH v6 2/9] doc: DT: venus: binding document for Qualcomm video driver

[Rob Herring]

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_VENUS0_VCODEC0_CLK>,
>>> + < GCC_VENUS0_AHB_CLK>,
>>> + < GCC_VENUS0_AXI_CLK>;
>>> +clock-names = "core", "iface", "bus";
>>> +power-domains = < VENUS_GDSC>;
>>> +iommus = <_iommu 5>;
>>> +
>>> +video-decoder {
>>> +compatible = "venus-decoder";
>>> +clocks = < VIDEO_SUBCORE0_CLK>;
>>> +clock-names = "core";
>>> +power-domains = < VENUS_CORE0_GDSC>;
>>> +};
>>> +
>>> +video-encoder {
>>> +compatible = "venus-encoder";
>>> +clocks = < VIDEO_SUBCORE1_CLK>;
>>> +clock-names = "core";
>>> +power-domains = < VENUS_CORE1_GDSC>;
>>> +};
>>> +
>>> +video-firmware {
>>> +memory-region = <_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

[Bartosz Golaszewski]

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(>dev,
+ "enable", GPIOD_OUT_HIGH);
+   if (IS_ERR(descs))
+   dev_err(>dev, "Error requesting enable GPIOs\n");
+
vpif_dev = >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

[Thibault Saunier]

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 = >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

[Thibault Saunier]

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 

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Vladimir Zapolskiy]

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(>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(>lock);
> +
> + if (on && !ov5647->power_count) {
> + dev_dbg(>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
>>
>>   

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Vladimir Zapolskiy]

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 

Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Ramiro Oliveira]

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 != 

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Ramiro Oliveira]

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(>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(>lock);
 +
 +  if (on && !ov5647->power_count) {
 +  dev_dbg(>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 = _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

[Sean Young]

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

[Andrzej Hajda]

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 = >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

[Stanimir Varbanov]

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_VENUS0_VCODEC0_CLK>,
>> + < GCC_VENUS0_AHB_CLK>,
>> + < GCC_VENUS0_AXI_CLK>;
>> +clock-names = "core", "iface", "bus";
>> +power-domains = < VENUS_GDSC>;
>> +iommus = <_iommu 5>;
>> +
>> +video-decoder {
>> +compatible = "venus-decoder";
>> +clocks = < VIDEO_SUBCORE0_CLK>;
>> +clock-names = "core";
>> +power-domains = < VENUS_CORE0_GDSC>;
>> +};
>> +
>> +video-encoder {
>> +compatible = "venus-encoder";
>> +clocks = < VIDEO_SUBCORE1_CLK>;
>> +clock-names = "core";
>> +power-domains = < VENUS_CORE1_GDSC>;
>> +};
>> +
>> +video-firmware {
>> +memory-region = <_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