Re: [PATCH] media: include/video/omapfb_dss.h: use IS_ENABLED()

[Randy Dunlap]

On 05/08/2018 03:56 PM, Randy Dunlap wrote:
> On 05/05/2018 02:14 PM, Mauro Carvalho Chehab wrote:
>> Em Sat, 5 May 2018 10:59:23 -0700
>> Randy Dunlap  escreveu:
>>
>>> On 05/04/2018 01:49 PM, Mauro Carvalho Chehab wrote:
 Just checking for ifdefs cause build issues as reported by
 kernel test:

 config: openrisc-allmodconfig (attached as .config)
 compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)

 All errors (new ones prefixed by >>):

drivers/video/fbdev/omap2/omapfb/omapfb-main.c: In function 
 'omapfb_init_connections':  
>> drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2396:8: error: implicit 
>> declaration of function 'omapdss_find_mgr_from_display' 
>> [-Werror=implicit-function-declaration]  
  mgr = omapdss_find_mgr_from_display(def_dssdev);
^
drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2396:6: warning: 
 assignment makes pointer from integer without a cast [-Wint-conversion]
  mgr = omapdss_find_mgr_from_display(def_dssdev);
  ^
drivers/video/fbdev/omap2/omapfb/omapfb-main.c: In function 
 'omapfb_find_default_display':  
>> drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2430:13: error: implicit 
>> declaration of function 'omapdss_get_default_display_name' 
>> [-Werror=implicit-function-declaration]  
  def_name = omapdss_get_default_display_name();
 ^~~~
drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2430:11: warning: 
 assignment makes pointer from integer without a cast [-Wint-conversion]
  def_name = omapdss_get_default_display_name();
   ^

 So, use IS_ENABLED() instead.  
>>>
>>> Hi,
>>>
>>> I would like to test this (the change doesn't make much sense to me),
>>> but I cannot find the kernel config file nor the kernel test robot's
>>> email of this report.
>>>
>>> Please include an lkml.kernel.org/r/ reference to such emails
>>> so that interested parties can join the party.
>>
>> The message was not c/c to lkml. You can see the original here:
>>
>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg130809.html
>>
>>
>>>
>>> Does this patch apply only to your media tree?  so hopefully I can see it in
>>> linux-next on Monday.
>>
>> Yes, as it is over another two patches applied there.
>>
>> If you want to test it earlier, it is in the top of the master branch:
>>  https://git.linuxtv.org/media_tree.git
>>
>>>
>>> Thanks.
>>>
 Cc: Bartlomiej Zolnierkiewicz 
 Cc: Randy Dunlap 
 Cc: tomi.valkei...@ti.com
 Cc: linux-o...@vger.kernel.org
 Cc: linux-fb...@vger.kernel.org
 Fixes: 771f7be87ff9 ("media: omapfb: omapfb_dss.h: add stubs to build with 
 COMPILE_TEST && DRM_OMAP")
 Signed-off-by: Mauro Carvalho Chehab 
 ---
  include/video/omapfb_dss.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
 index e9775144ff3b..12755d8d9b4f 100644
 --- a/include/video/omapfb_dss.h
 +++ b/include/video/omapfb_dss.h
 @@ -778,7 +778,7 @@ struct omap_dss_driver {
  
  typedef void (*omap_dispc_isr_t) (void *arg, u32 mask);
  
 -#ifdef CONFIG_FB_OMAP2
 +#if IS_ENABLED(CONFIG_FB_OMAP2)
  
  enum omapdss_version omapdss_get_version(void);
  bool omapdss_is_initialized(void);
> 
> The patch doesn't make any sense to me.  I would like to see an
> explanation of why this is needed, other than "it fixes the build." ;)

I get it now.  Using
#if IS_ENABLED(CONFIG_FB_OMAP2)

is just the "modern" way of saying
#if defined(CONFIG_FB_OMAP2) || defined(CONFIG_FB_OMAP2_MODULE)

which also builds without errors.



> But it does fix the build, so:
> Tested-by: Randy Dunlap 
Acked-by: Randy Dunlap 

-- 
~Randy

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

On 05/08/2018 03:28 AM, Sakari Ailus wrote:

Hi Steve,

Again, sorry about the delay. This thread got buried in my inbox. :-(
Please see my reply below.

On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:


On 04/23/2018 12:14 AM, Sakari Ailus wrote:

Hi Steve,

On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:

Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
for parsing a sub-device's fwnode port endpoints for connected remote
sub-devices, registering a sub-device notifier, and then registering
the sub-device itself.

Signed-off-by: Steve Longerbeam 
---
Changes since v2:
- fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
to put device.
Changes since v1:
- add #include  to v4l2-fwnode.h for
'struct v4l2_subdev' declaration.
---
   drivers/media/v4l2-core/v4l2-fwnode.c | 101 
++
   include/media/v4l2-fwnode.h   |  43 +++
   2 files changed, 144 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
b/drivers/media/v4l2-core/v4l2-fwnode.c
index 99198b9..d42024d 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct 
v4l2_subdev *sd)
   }
   EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
+int v4l2_async_register_fwnode_subdev(
+   struct v4l2_subdev *sd, size_t asd_struct_size,
+   unsigned int *ports, unsigned int num_ports,
+   int (*parse_endpoint)(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd))
+{
+   struct v4l2_async_notifier *notifier;
+   struct device *dev = sd->dev;
+   struct fwnode_handle *fwnode;
+   unsigned int subdev_port;
+   bool is_port;
+   int ret;
+
+   if (WARN_ON(!dev))
+   return -ENODEV;
+
+   fwnode = dev_fwnode(dev);
+   if (!fwnode_device_is_available(fwnode))
+   return -ENODEV;
+
+   is_port = (is_of_node(fwnode) &&
+  of_node_cmp(to_of_node(fwnode)->name, "port") == 0);

What's the intent of this and the code below? You may not parse the graph
data structure here, it should be done in the actual firmware
implementation instead.

The i.MX6 CSI sub-device registers itself from a port fwnode, so
the intent of the is_port code below is to support the i.MX6 CSI.

I can remove the is_port checks, but it means
v4l2_async_register_fwnode_subdev() won't be usable by the CSI
sub-device.

This won't scale.


The vast majority of sub-devices register themselves as
port parent nodes. So for now at least, I think
v4l2_async_register_fwnode_subdev() could be useful to many
platforms.


  Instead, I think we'd need to separate registering
sub-devices (through async sub-devices) and binding them with the driver
that registered the notifier. Or at least change how that process works: a
single sub-device can well be bound to multiple notifiers,


Ok, that is certainly not the case now, a sub-device can only
be bound to a single notifier.


  or multiple
times to the same notifier while it may be registered only once.


Anyway, this is a future generalization if I understand you
correctly. Not something to deal with here.




Also, sub-devices generally do not match ports.

Yes that's generally true, sub-devices generally match to port parent
nodes. But I do know of one other sub-device that buck that trend.
The ADV748x CSI-2 output sub-devices match against endpoint nodes.

Endpoints, yes, but not ports.


Well, the imx CSI registers from a port node.




   How sub-devices generally
correspond to fwnodes is up to the device.

What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
and a v4l2_async_register_endpoint_fwnode_subdev() to support such
sub-devices?

The endpoint is more specific than a port, so why the port and not the
endpoint?


Do you mean there should be a
v4l2_async_register_endpoint_fwnode_subdev() but not
v4l2_async_register_endpoint_port_subdev()?

Steve

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:   Wed May  9 05:00:12 CEST 2018
media-tree git hash:f10379aad39e9da8bc7d1822e251b5f0673067ef
media_build git hash:   f4c610423a165a6fc0409ea5c06f8a83948f910f
v4l-utils git hash: 03e763fd4b361b2082019032fc315b7606669335
gcc version:i686-linux-gcc (GCC) 7.3.0
sparse version: 0.5.2-RC1
smatch version: 0.5.1
host hardware:  x86_64
host os:4.15.0-3-amd64

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-arm64: OK
linux-git-i686: OK
linux-git-mips: WARNINGS
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
Check COMPILE_TEST: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.101-i686: OK
linux-3.0.101-x86_64: OK
linux-3.1.10-i686: OK
linux-3.1.10-x86_64: OK
linux-3.2.101-i686: OK
linux-3.2.101-x86_64: OK
linux-3.3.8-i686: ERRORS
linux-3.3.8-x86_64: ERRORS
linux-3.4.113-i686: ERRORS
linux-3.4.113-x86_64: ERRORS
linux-3.5.7-i686: ERRORS
linux-3.5.7-x86_64: ERRORS
linux-3.6.11-i686: ERRORS
linux-3.6.11-x86_64: ERRORS
linux-3.7.10-i686: ERRORS
linux-3.7.10-x86_64: ERRORS
linux-3.8.13-i686: ERRORS
linux-3.8.13-x86_64: ERRORS
linux-3.9.11-i686: ERRORS
linux-3.9.11-x86_64: ERRORS
linux-3.10.108-i686: ERRORS
linux-3.10.108-x86_64: ERRORS
linux-3.11.10-i686: ERRORS
linux-3.11.10-x86_64: ERRORS
linux-3.12.74-i686: ERRORS
linux-3.12.74-x86_64: ERRORS
linux-3.13.11-i686: ERRORS
linux-3.13.11-x86_64: ERRORS
linux-3.14.79-i686: ERRORS
linux-3.14.79-x86_64: ERRORS
linux-3.15.10-i686: ERRORS
linux-3.15.10-x86_64: ERRORS
linux-3.16.56-i686: ERRORS
linux-3.16.56-x86_64: ERRORS
linux-3.17.8-i686: ERRORS
linux-3.17.8-x86_64: ERRORS
linux-3.18.102-i686: ERRORS
linux-3.18.102-x86_64: ERRORS
linux-3.19.8-i686: ERRORS
linux-3.19.8-x86_64: ERRORS
linux-4.0.9-i686: ERRORS
linux-4.0.9-x86_64: ERRORS
linux-4.1.51-i686: ERRORS
linux-4.1.51-x86_64: ERRORS
linux-4.2.8-i686: ERRORS
linux-4.2.8-x86_64: ERRORS
linux-4.3.6-i686: ERRORS
linux-4.3.6-x86_64: ERRORS
linux-4.4.109-i686: ERRORS
linux-4.4.109-x86_64: ERRORS
linux-4.5.7-i686: ERRORS
linux-4.5.7-x86_64: ERRORS
linux-4.6.7-i686: ERRORS
linux-4.6.7-x86_64: ERRORS
linux-4.7.10-i686: ERRORS
linux-4.7.10-x86_64: ERRORS
linux-4.8.17-i686: ERRORS
linux-4.8.17-x86_64: ERRORS
linux-4.9.91-i686: ERRORS
linux-4.9.91-x86_64: ERRORS
linux-4.10.17-i686: ERRORS
linux-4.10.17-x86_64: ERRORS
linux-4.11.12-i686: ERRORS
linux-4.11.12-x86_64: ERRORS
linux-4.12.14-i686: ERRORS
linux-4.12.14-x86_64: ERRORS
linux-4.13.16-i686: ERRORS
linux-4.13.16-x86_64: ERRORS
linux-4.14.31-i686: ERRORS
linux-4.14.31-x86_64: ERRORS
linux-4.15.14-i686: ERRORS
linux-4.15.14-x86_64: ERRORS
linux-4.16-i686: ERRORS
linux-4.16-x86_64: ERRORS
apps: OK
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Wednesday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

[PATCH] media: uvcvideo: Support realtek's UVC 1.5 device

[ming_qian]

From: ming_qian 

The length of UVC 1.5 video control is 48, and it id 34 for UVC 1.1.
Change it to 48 for UVC 1.5 device,
and the UVC 1.5 device can be recognized.

More changes to the driver are needed for full UVC 1.5 compatibility.
However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have
been reported to work well.

Signed-off-by: ming_qian 
---
 drivers/media/usb/uvc/uvc_video.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c 
b/drivers/media/usb/uvc/uvc_video.c
index aa0082f..32dfb32 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -171,6 +171,8 @@ static int uvc_get_video_ctrl(struct uvc_streaming *stream,
int ret;
 
size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
+   if (stream->dev->uvc_version >= 0x0150)
+   size = 48;
if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
query == UVC_GET_DEF)
return -EIO;
@@ -259,6 +261,8 @@ static int uvc_set_video_ctrl(struct uvc_streaming *stream,
int ret;
 
size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
+   if (stream->dev->uvc_version >= 0x0150)
+   size = 48;
data = kzalloc(size, GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
-- 
2.7.4

Re: [PATCH v2 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Steve Longerbeam]

Hi Philipp, Jan,


On 05/08/2018 07:25 AM, Philipp Zabel wrote:

On Tue, 2018-05-08 at 16:14 +0200, Jan Luebbe wrote:

The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
To handle this, we extend imx_media_pixfmt with a cycles per pixel
field, which is used for generic formats on the parallel bus.

Before actually adding RGB565_2X8 support for the parallel bus, this
series simplifies handing of the the different configurations for RGB565
between parallel and MIPI CSI-2 in imx-media-capture. This avoids having
to explicitly pass on the format in the second patch.

Changes since v1:
   - fixed problems reported the kbuild test robot
   - added helper functions as suggested by Steve Longerbeam
 (is_parallel_bus and requires_passthrough)
   - removed passthough format check in csi_link_validate() (suggested by
 Philipp Zabel during internal review)

The theory is that IC only supports AYUV8_1X32 and RGB888_1X24 input,
and any passthrough format on the CSI sink will differ from those.
Mismatching formats are already caught by v4l2_subdev_link_validate
called on the ipu?_vdic or ipu?_ic_prp entities' sink pads.


Right, the CSI will pass parallel-bus RGB565_2X8 through to the source
pad. If the CSI is then linked to ->IC_PRP or ->VDIC, 
v4l2_subdev_link_validate

will catch the mbus format mismatch. So the check in csi_link_validate
is not really necessary, thanks for catching.

Steve

Re: [PATCH v9 11/15] vb2: add in-fence support to QBUF

[Gustavo Padovan]

Hi Hans,

On Mon, 2018-05-07 at 14:07 +0200, Hans Verkuil wrote:
> On 04/05/18 22:06, Ezequiel Garcia wrote:
> > From: Gustavo Padovan 
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers can't be queued
> > to the
> > driver before its fences signal. And a buffer can't be queued to
> > the driver
> > out of the order they were queued from userspace. That means that
> > even if
> > its fence signals it must wait for all other buffers, ahead of it
> > in the queue,
> > to signal first.
> > 
> > If the fence for some buffer fails we do not queue it to the
> > driver,
> > instead we mark it as error and wait until the previous buffer is
> > done
> > to notify userspace of the error. We wait here to deliver the
> > buffers back
> > to userspace in order.
> > 
> > v12: fixed dvb_vb2.c usage of vb2_core_qbuf.
> > 
> > v11: - minor doc/comments fixes (Hans Verkuil)
> >  - reviewed the in-fence path at __fill_v4l2_buffer()
> > 
> > v10: - rename fence to in_fence in many places
> >  - handle fences signalling with error better (Hans Verkuil)
> > 
> > v9: - improve comments and docs (Hans Verkuil)
> > - fix unlocking of vb->fence_cb_lock on vb2_core_qbuf (Hans
> > Verkuil)
> > - move in-fences code that was in the out-fences patch here
> > (Alex)
> > 
> > v8: - improve comments about fences with errors
> > 
> > v7: - get rid of the fence array stuff for ordering and just use
> >   get_num_buffers_ready() (Hans)
> > - fix issue of queuing the buffer twice (Hans)
> > - avoid the dma_fence_wait() in core_qbuf() (Alex)
> > - merge preparation commit in
> > 
> > v6: - With fences always keep the order userspace queues the
> > buffers.
> > - Protect in_fence manipulation with a lock (Brian Starkey)
> > - check if fences have the same context before adding a fence
> > array
> > - Fix last_fence ref unbalance in __set_in_fence() (Brian
> > Starkey)
> > - Clean up fence if __set_in_fence() fails (Brian Starkey)
> > - treat -EINVAL from dma_fence_add_callback() (Brian Starkey)
> > 
> > v5: - use fence_array to keep buffers ordered in vb2 core when
> >   needed (Brian Starkey)
> > - keep backward compat on the reserved2 field (Brian Starkey)
> > - protect fence callback removal with lock (Brian Starkey)
> > 
> > v4: - Add a comment about dma_fence_add_callback() not returning a
> >   error (Hans)
> > - Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
> > - select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
> > - Move dma_fence_is_signaled() check to __enqueue_in_driver()
> > (Hans)
> > - Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
> > - Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
> >   vb2_start_streaming() (Hans)
> > - set IN_FENCE flags on __fill_v4l2_buffer (Hans)
> > - Queue buffers to the driver as soon as they are ready (Hans)
> > - call fill_user_buffer() after queuing the buffer (Hans)
> > - add err: label to clean up fence
> > - add dma_fence_wait() before calling vb2_start_streaming()
> > 
> > v3: - document fence parameter
> > - remove ternary if at vb2_qbuf() return (Mauro)
> > - do not change if conditions behaviour (Mauro)
> > 
> > v2: - fix vb2_queue_or_prepare_buf() ret check
> > - remove check for VB2_MEMORY_DMABUF only (Javier)
> > - check num of ready buffers to start streaming
> > - when queueing, start from the first ready buffer
> > - handle queue cancel
> > 
> > Signed-off-by: Gustavo Padovan 
> > Signed-off-by: Ezequiel Garcia 
> > ---
> >  drivers/media/common/videobuf2/videobuf2-core.c | 197
> > 
> >  drivers/media/common/videobuf2/videobuf2-v4l2.c |  34 +++-
> >  drivers/media/dvb-core/dvb_vb2.c|   2 +-
> >  drivers/media/v4l2-core/Kconfig |  33 
> >  include/media/videobuf2-core.h  |  14 +-
> >  5 files changed, 249 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> > b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 6b8e083893ad..996b99497a98 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -352,6 +352,7 @@ static int __vb2_queue_alloc(struct vb2_queue
> > *q, enum vb2_memory memory,
> > vb->index = q->num_buffers + buffer;
> > vb->type = q->type;
> > vb->memory = memory;
> > +   spin_lock_init(>fence_cb_lock);
> > for (plane = 0; plane < num_planes; ++plane) {
> > vb->planes[plane].length =
> > plane_sizes[plane];
> > vb->planes[plane].min_length =
> > plane_sizes[plane];
> > @@ -905,20 +906,12 @@ void *vb2_plane_cookie(struct vb2_buffer *vb,
> > unsigned int 

Re: [PATCH] media: include/video/omapfb_dss.h: use IS_ENABLED()

[Randy Dunlap]

On 05/05/2018 02:14 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 5 May 2018 10:59:23 -0700
> Randy Dunlap  escreveu:
> 
>> On 05/04/2018 01:49 PM, Mauro Carvalho Chehab wrote:
>>> Just checking for ifdefs cause build issues as reported by
>>> kernel test:
>>>
>>> config: openrisc-allmodconfig (attached as .config)
>>> compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>drivers/video/fbdev/omap2/omapfb/omapfb-main.c: In function 
>>> 'omapfb_init_connections':  
> drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2396:8: error: implicit 
> declaration of function 'omapdss_find_mgr_from_display' 
> [-Werror=implicit-function-declaration]  
>>>  mgr = omapdss_find_mgr_from_display(def_dssdev);
>>>^
>>>drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2396:6: warning: 
>>> assignment makes pointer from integer without a cast [-Wint-conversion]
>>>  mgr = omapdss_find_mgr_from_display(def_dssdev);
>>>  ^
>>>drivers/video/fbdev/omap2/omapfb/omapfb-main.c: In function 
>>> 'omapfb_find_default_display':  
> drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2430:13: error: implicit 
> declaration of function 'omapdss_get_default_display_name' 
> [-Werror=implicit-function-declaration]  
>>>  def_name = omapdss_get_default_display_name();
>>> ^~~~
>>>drivers/video/fbdev/omap2/omapfb/omapfb-main.c:2430:11: warning: 
>>> assignment makes pointer from integer without a cast [-Wint-conversion]
>>>  def_name = omapdss_get_default_display_name();
>>>   ^
>>>
>>> So, use IS_ENABLED() instead.  
>>
>> Hi,
>>
>> I would like to test this (the change doesn't make much sense to me),
>> but I cannot find the kernel config file nor the kernel test robot's
>> email of this report.
>>
>> Please include an lkml.kernel.org/r/ reference to such emails
>> so that interested parties can join the party.
> 
> The message was not c/c to lkml. You can see the original here:
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg130809.html
> 
> 
>>
>> Does this patch apply only to your media tree?  so hopefully I can see it in
>> linux-next on Monday.
> 
> Yes, as it is over another two patches applied there.
> 
> If you want to test it earlier, it is in the top of the master branch:
>   https://git.linuxtv.org/media_tree.git
> 
>>
>> Thanks.
>>
>>> Cc: Bartlomiej Zolnierkiewicz 
>>> Cc: Randy Dunlap 
>>> Cc: tomi.valkei...@ti.com
>>> Cc: linux-o...@vger.kernel.org
>>> Cc: linux-fb...@vger.kernel.org
>>> Fixes: 771f7be87ff9 ("media: omapfb: omapfb_dss.h: add stubs to build with 
>>> COMPILE_TEST && DRM_OMAP")
>>> Signed-off-by: Mauro Carvalho Chehab 
>>> ---
>>>  include/video/omapfb_dss.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
>>> index e9775144ff3b..12755d8d9b4f 100644
>>> --- a/include/video/omapfb_dss.h
>>> +++ b/include/video/omapfb_dss.h
>>> @@ -778,7 +778,7 @@ struct omap_dss_driver {
>>>  
>>>  typedef void (*omap_dispc_isr_t) (void *arg, u32 mask);
>>>  
>>> -#ifdef CONFIG_FB_OMAP2
>>> +#if IS_ENABLED(CONFIG_FB_OMAP2)
>>>  
>>>  enum omapdss_version omapdss_get_version(void);
>>>  bool omapdss_is_initialized(void);

The patch doesn't make any sense to me.  I would like to see an
explanation of why this is needed, other than "it fixes the build." ;)

But it does fix the build, so:
Tested-by: Randy Dunlap 


thanks,
-- 
~Randy

[PATCH 3/5] cx23885: Ryzen DMA related RiSC engine stall fixes

[Brad Love]

This bug affects all of Hauppauge QuadHD boards when used on all Ryzen
platforms and some XEON platforms. On these platforms it is possible to
error out the RiSC engine and cause it to stall, whereafter the only
way to reset the board to a working state is to reboot.

This is the fatal condition with current driver:

[  255.663598] cx23885: cx23885[0]: mpeg risc op code error
[  255.663607] cx23885: cx23885[0]: TS1 B - dma channel status dump
[  255.663612] cx23885: cx23885[0]:   cmds: init risc lo   : 0xffe54000
[  255.663615] cx23885: cx23885[0]:   cmds: init risc hi   : 0x
[  255.663619] cx23885: cx23885[0]:   cmds: cdt base   : 0x00010870
[  255.663622] cx23885: cx23885[0]:   cmds: cdt size   : 0x000a
[  255.663625] cx23885: cx23885[0]:   cmds: iq base: 0x00010630
[  255.663629] cx23885: cx23885[0]:   cmds: iq size: 0x0010
[  255.663632] cx23885: cx23885[0]:   cmds: risc pc lo : 0xffe54018
[  255.663636] cx23885: cx23885[0]:   cmds: risc pc hi : 0x
[  255.663639] cx23885: cx23885[0]:   cmds: iq wr ptr  : 0x4192
[  255.663642] cx23885: cx23885[0]:   cmds: iq rd ptr  : 0x418c
[  255.663645] cx23885: cx23885[0]:   cmds: cdt current: 0x00010898
[  255.663649] cx23885: cx23885[0]:   cmds: pci target lo  : 0xf85ca340
[  255.663652] cx23885: cx23885[0]:   cmds: pci target hi  : 0x
[  255.663655] cx23885: cx23885[0]:   cmds: line / byte: 0x000c
[  255.663659] cx23885: cx23885[0]:   risc0:
[  255.663661] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663666] cx23885: cx23885[0]:   risc1:
[  255.663667] 0xf85ca050 [ INVALID sol 22 20 19 18 resync 13 count=80 ]
[  255.663674] cx23885: cx23885[0]:   risc2:
[  255.663674] 0x [ INVALID count=0 ]
[  255.663678] cx23885: cx23885[0]:   risc3:
[  255.663679] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663684] cx23885: cx23885[0]:   (0x00010630) iq 0:
[  255.663685] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663690] cx23885: cx23885[0]:   iq 1: 0xf85ca630 [ arg #1 ]
[  255.663693] cx23885: cx23885[0]:   iq 2: 0x [ arg #2 ]
[  255.663696] cx23885: cx23885[0]:   (0x0001063c) iq 3:
[  255.663697] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663702] cx23885: cx23885[0]:   iq 4: 0xf85ca920 [ arg #1 ]
[  255.663705] cx23885: cx23885[0]:   iq 5: 0x [ arg #2 ]
[  255.663709] cx23885: cx23885[0]:   (0x00010648) iq 6:
[  255.663709] 0xf85ca340 [ INVALID sol 22 20 19 18 resync 13 count=832 ]
[  255.663716] cx23885: cx23885[0]:   (0x0001064c) iq 7:
[  255.663717] 0x [ INVALID count=0 ]
[  255.663721] cx23885: cx23885[0]:   (0x00010650) iq 8:
[  255.663721] 0x [ INVALID count=0 ]
[  255.663725] cx23885: cx23885[0]:   (0x00010654) iq 9:
[  255.663726] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663731] cx23885: cx23885[0]:   iq a: 0xf85c9780 [ arg #1 ]
[  255.663734] cx23885: cx23885[0]:   iq b: 0x [ arg #2 ]
[  255.663737] cx23885: cx23885[0]:   (0x00010660) iq c:
[  255.663738] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663743] cx23885: cx23885[0]:   iq d: 0xf85c9a70 [ arg #1 ]
[  255.663746] cx23885: cx23885[0]:   iq e: 0x [ arg #2 ]
[  255.663749] cx23885: cx23885[0]:   (0x0001066c) iq f:
[  255.663750] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663755] cx23885: cx23885[0]:   iq 10: 0xf4fa2920 [ arg #1 ]
[  255.663758] cx23885: cx23885[0]:   iq 11: 0x [ arg #2 ]
[  255.663759] cx23885: cx23885[0]: fifo: 0x5000 -> 0x6000
[  255.663760] cx23885: cx23885[0]: ctrl: 0x00010630 -> 0x10690
[  255.663764] cx23885: cx23885[0]:   ptr1_reg: 0x5980
[  255.663767] cx23885: cx23885[0]:   ptr2_reg: 0x000108a8
[  255.663770] cx23885: cx23885[0]:   cnt1_reg: 0x000b
[  255.663773] cx23885: cx23885[0]:   cnt2_reg: 0x0003

Included is checks of the TC_REQ and TC_REQ_SET registers during states
of board initialization, reset, DMA start, and DMA stop. If both registers
are set, this indicates a stall in the RiSC engine, at which point the
bridge error is cleared.

A small delay is introduced in stop_dma as well, to allow transfers in
progress to finish.

After application all models work on Ryzen, occasionally yielding:

cx23885_clear_bridge_error: dma in progress detected 0x0001 0x0001, 
clearing

Signed-off-by: Brad Love 
---
 drivers/media/pci/cx23885/cx23885-core.c | 63 +++-
 drivers/media/pci/cx23885/cx23885-reg.h  | 14 +++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 3f55319..20a1fd2 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -601,6 +601,25 @@ static void cx23885_risc_disasm(struct cx23885_tsport 
*port,
}
 }
 
+static void cx23885_clear_bridge_error(struct cx23885_dev *dev)
+{
+   uint32_t reg1_val = cx_read(TC_REQ); /* read-only */
+   uint32_t reg2_val = 

[PATCH 1/5] cx23885: Handle additional bufs on interrupt

[Brad Love]

On Ryzen systems interrupts are occasionally missed:

cx23885: cx23885_wakeup: [99b384b83c00/28] wakeup reg=5406 buf=5405
cx23885: cx23885_wakeup: [99b40bf79400/31] wakeup reg=9537 buf=9536

This patch loops up to five times on wakeup, marking any buffers
found done.

Since the count register is u16, but the vb2 counter is u32, some modulo
arithmetic is used to accommodate wraparound and ensure current active
buffer is the buffer expected.

Signed-off-by: Brad Love 
---
 drivers/media/pci/cx23885/cx23885-core.c | 37 +---
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 019fac4..b279758 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -422,19 +422,30 @@ static void cx23885_wakeup(struct cx23885_tsport *port,
   struct cx23885_dmaqueue *q, u32 count)
 {
struct cx23885_buffer *buf;
-
-   if (list_empty(>active))
-   return;
-   buf = list_entry(q->active.next,
-struct cx23885_buffer, queue);
-
-   buf->vb.vb2_buf.timestamp = ktime_get_ns();
-   buf->vb.sequence = q->count++;
-   dprintk(1, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
-   buf->vb.vb2_buf.index,
-   count, q->count);
-   list_del(>queue);
-   vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_DONE);
+   int count_delta;
+   int max_buf_done = 5; /* service maximum five buffers */
+
+   do {
+   if (list_empty(>active))
+   return;
+   buf = list_entry(q->active.next,
+struct cx23885_buffer, queue);
+
+   buf->vb.vb2_buf.timestamp = ktime_get_ns();
+   buf->vb.sequence = q->count++;
+   if (count != (q->count % 65536)) {
+   dprintk(1, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
+   buf->vb.vb2_buf.index, count, q->count);
+   } else {
+   dprintk(7, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
+   buf->vb.vb2_buf.index, count, q->count);
+   }
+   list_del(>queue);
+   vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_DONE);
+   max_buf_done--;
+   /* count register is 16 bits so apply modulo appropriately */
+   count_delta = ((int)count - (int)(q->count % 65536));
+   } while ((count_delta > 0) && (max_buf_done > 0));
 }
 
 int cx23885_sram_channel_setup(struct cx23885_dev *dev,
-- 
2.7.4

[PATCH 2/5] cx23885: Use PCI and TS masks in irq functions

[Brad Love]

Currently mask is read for pci_status/ts1_status/ts2_status, but
otherwise ignored. The masks are now used to determine whether
action is warranted.

Signed-off-by: Brad Love 
---
 drivers/media/pci/cx23885/cx23885-core.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index b279758..3f55319 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1704,6 +1704,12 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
 
pci_status = cx_read(PCI_INT_STAT);
pci_mask = cx23885_irq_get_mask(dev);
+   if ((pci_status & pci_mask) == 0) {
+   dprintk(7, "pci_status: 0x%08x  pci_mask: 0x%08x\n",
+   pci_status, pci_mask);
+   goto out;
+   }
+
vida_status = cx_read(VID_A_INT_STAT);
vida_mask = cx_read(VID_A_INT_MSK);
audint_status = cx_read(AUDIO_INT_INT_STAT);
@@ -1713,7 +1719,9 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
ts2_status = cx_read(VID_C_INT_STAT);
ts2_mask = cx_read(VID_C_INT_MSK);
 
-   if ((pci_status == 0) && (ts2_status == 0) && (ts1_status == 0))
+   if (((pci_status & pci_mask) == 0) &&
+   ((ts2_status & ts2_mask) == 0) &&
+   ((ts1_status & ts1_mask) == 0))
goto out;
 
vida_count = cx_read(VID_A_GPCNT);
@@ -1840,7 +1848,7 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
}
 
if (handled)
-   cx_write(PCI_INT_STAT, pci_status);
+   cx_write(PCI_INT_STAT, pci_status & pci_mask);
 out:
return IRQ_RETVAL(handled);
 }
-- 
2.7.4

[PATCH 0/5] cx23885: Ryzen/Xeon DMA/interrupt fixes

[Brad Love]

Various problems have been reported to Hauppauge related
to usage of QuadHD products on Ryzen platforms and some
Xeon platforms. The most serious issue causes adapters on
the card to stop working. When the four tuners are in use it
is possible to experience RiSc engine OP CODE errors, at which
point the DMA engine for that port stalls and becomes inoperable
until a reboot. Unloading and reloading the driver does not
fix the condition. This DMA stall is handled by checking
TC_REQ and TC_REQ_SET registers in a lot of various spots.
If both of these registers are found to have the same bits
set, then their contents must be written back to 'un-freeze'
the DMA engine and let operation continue. The problem can
be quite hard to reproduce depending on motherboard and cpu.
The attached patch has been reported to fix all various parties
reporting issues with an assortment of Ryzen motherboards and
CPU combinations. It has been reported that some manufacturers
have released BIOS updates to address the PCIe instability.
This patch has been confirmed to have no adverse affets on
the reporting systems.


In the investigation of the issue a couple other problems
were found:
- On Ryzen/Xeon platforms it is possible to skip interrupts
- PCI and TS masks were ignored during irq


Brad Love (5):
  cx23885: Handle additional bufs on interrupt
  cx23885: Use PCI and TS masks in irq functions
  cx23885: Ryzen DMA related RiSC engine stall fixes
  cx23885: Expand registers printed during dma tsport reg dump
  cx23885: Add some missing register documentation

 drivers/media/pci/cx23885/cx23885-core.c | 144 +++
 drivers/media/pci/cx23885/cx23885-reg.h  |  14 +++
 2 files changed, 139 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 5/5] cx23885: Add some missing register documentation

[Brad Love]

Document what these two register calls are doing.

Signed-off-by: Brad Love 
---
 drivers/media/pci/cx23885/cx23885-core.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 1150160..ca19e0d 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1465,8 +1465,15 @@ int cx23885_start_dma(struct cx23885_tsport *port,
reg = reg | 0xa;
cx_write(PAD_CTRL, reg);
 
-   /* FIXME and these two registers should be documented. */
+   /* Sets MOE_CLK_DIS to disable MoE clock */
+   /* sets MCLK_DLY_SEL/BCLK_DLY_SEL to 1 buffer delay each */
cx_write(CLK_DELAY, cx_read(CLK_DELAY) | 0x8011);
+
+   /* ALT_GPIO_ALT_SET: GPIO[0]
+* IR_ALT_TX_SEL: GPIO[1]
+* GPIO1_ALT_SEL: VIP_656_DATA[0]
+* GPIO0_ALT_SEL: VIP_656_CLK
+*/
cx_write(ALT_PIN_OUT_SEL, 0x10100045);
}
 
-- 
2.7.4

[PATCH 4/5] cx23885: Expand registers in dma tsport reg dump

[Brad Love]

Include some additional useful registers in the output.

Signed-off-by: Brad Love 
---
 drivers/media/pci/cx23885/cx23885-core.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 20a1fd2..1150160 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1369,6 +1369,18 @@ static void cx23885_tsport_reg_dump(struct 
cx23885_tsport *port)
port->reg_ts_clk_en, cx_read(port->reg_ts_clk_en));
dprintk(1, "%s() ts_int_msk(0x%08X) 0x%08x\n", __func__,
port->reg_ts_int_msk, cx_read(port->reg_ts_int_msk));
+   dprintk(1, "%s() ts_int_status(0x%08X)  0x%08x\n", __func__,
+   port->reg_ts_int_stat, cx_read(port->reg_ts_int_stat));
+   dprintk(1, "%s() PCI_INT_STAT   0x%08X\n", __func__,
+   cx_read(PCI_INT_STAT));
+   dprintk(1, "%s() VID_B_INT_MSTAT0x%08X\n", __func__,
+   cx_read(VID_B_INT_MSTAT));
+   dprintk(1, "%s() VID_B_INT_SSTAT0x%08X\n", __func__,
+   cx_read(VID_B_INT_SSTAT));
+   dprintk(1, "%s() VID_C_INT_MSTAT0x%08X\n", __func__,
+   cx_read(VID_C_INT_MSTAT));
+   dprintk(1, "%s() VID_C_INT_SSTAT0x%08X\n", __func__,
+   cx_read(VID_C_INT_SSTAT));
 }
 
 int cx23885_start_dma(struct cx23885_tsport *port,
-- 
2.7.4

Re: [PATCH v5 0/8] Add support for multi-planar formats and 10 bit formats

[Hyun Kwon]

Hi Hans,

On Tue, 2018-05-08 at 00:57:25 -0700, Hans Verkuil wrote:
> On 05/07/2018 07:45 PM, Hyun Kwon wrote:
> > Hi Hans,
> > 
> > Thanks for the comment.
> > 
> > On Mon, 2018-05-07 at 05:59:39 -0700, Hans Verkuil wrote:
> >> Hi Satish,
> >>
> >> On 03/05/18 04:42, Satish Kumar Nagireddy wrote:
> >>>  The patches are for xilinx v4l. The patcheset enable support to handle 
> >>> multiplanar
> >>>  formats and 10 bit formats. Single planar implementation is removed as 
> >>> mplane can
> >>>  handle both.
> >>
> >> If I understand the format correctly, then the planes are contiguous in 
> >> memory,
> >> i.e. it is a single buffer.
> >>
> >> You do not need to switch to the _MPLANE API for that: that API is meant 
> >> for the
> >> case where the planes are not contiguous in memory but each plane has its 
> >> own
> >> buffer. And yes, we should have called it the _MBUFFER API or something :-(
> >>
> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-nv12.html
> >>
> >> Switching to the _MPLANE API will actually break userspace, so that's 
> >> another
> >> reason why you shouldn't do this. But from what I can tell, it really isn't
> >> needed at all.
> >>
> > 
> > Sharing some background to get your further input. :-)
> > 
> > The Xilinx V4L driver is currently only for the soft IPs, which are
> > programmable on FPGA, and those IPs are constantly updated. Initially, IPs
> > didn't support _MPLANE formats, so it started with single buffer type 
> > format.
> > Now, the IPs support _MPLANE formats, even though those formats are not 
> > part of
> > this patch. Those formats are in downstream vendor tree and will be 
> > upstreamed
> > at some point[1]. While implementing the multi-buffer formats, we had 
> > similar
> > concern regarding UAPI and ended up having the module param[2]. It was there
> > for a couple of Xilinx release cycles to migrate internal applications to
> > _MPLANE formats and to get report if that breaks any external applications. 
> > Now
> > we thought it's good time to hard-switch the driver to _MPLANE completely
> > rather than keeping single buffer code, especially because it seems legal to
> > support single buffer formats with _MPLANE type. If this is not the case and
> > the applications with single buffer formats but without mplane formats 
> > should
> > be supported, we can revive the single buffer code in one way or another.
> 
> In that case you need to split off the _MPLANE API change into a separate 
> patch.
> Switching to the MPLANE API has nothing to do with supporting this format, it 
> is
> done for a different reason (future support for real multiplane formats).
> 
> Since this also breaks userspace applications you will need to clearly state 
> in
> the commit log why this is a reasonable thing to do.
> 

Sure. I agree, and we will re-organize patches.

Thanks,
-hyun

> Regards,
> 
>   Hans
> 
> > 
> > Thanks,
> > -hyun
> > 
> > [1] 
> > https://github.com/Xilinx/linux-xlnx/blob/xilinx-v2018.1/drivers/media/platform/xilinx/xilinx-vip.c#L33
> > [2] 
> > https://github.com/Xilinx/linux-xlnx/blob/xilinx-v2018.1/drivers/media/platform/xilinx/xilinx-vipp.c#L40
> > 
> >> Regards,
> >>
> >>Hans
> >>
> >>>
> >>>  Patch-set has downstream changes and bug fixes. Added new media bus 
> >>> format
> >>>  MEDIA_BUS_FMT_VYYUYY8_1X24, new pixel format V4L2_PIX_FMT_XV15 and rst
> >>>  documentation.
> >>>
> >>> Jeffrey Mouroux (1):
> >>>   uapi: media: New fourcc code and rst for 10 bit format
> >>>
> >>> Radhey Shyam Pandey (1):
> >>>   v4l: xilinx: dma: Remove colorspace check in xvip_dma_verify_format
> >>>
> >>> Rohit Athavale (1):
> >>>   xilinx: v4l: dma: Update driver to allow for probe defer
> >>>
> >>> Satish Kumar Nagireddy (4):
> >>>   media-bus: uapi: Add YCrCb 420 media bus format and rst
> >>>   v4l: xilinx: dma: Update video format descriptor
> >>>   v4l: xilinx: dma: Add multi-planar support
> >>>   v4l: xilinx: dma: Add support for 10 bit formats
> >>>
> >>> Vishal Sagar (1):
> >>>   xilinx: v4l: dma: Terminate DMA when media pipeline fail to start
> >>>
> >>>  Documentation/media/uapi/v4l/pixfmt-xv15.rst| 134 +++
> >>>  Documentation/media/uapi/v4l/subdev-formats.rst |  38 +-
> >>>  Documentation/media/uapi/v4l/yuv-formats.rst|   1 +
> >>>  drivers/media/platform/xilinx/xilinx-dma.c  | 170 
> >>> +++-
> >>>  drivers/media/platform/xilinx/xilinx-dma.h  |   4 +-
> >>>  drivers/media/platform/xilinx/xilinx-vip.c  |  37 --
> >>>  drivers/media/platform/xilinx/xilinx-vip.h  |  15 ++-
> >>>  drivers/media/platform/xilinx/xilinx-vipp.c |  16 +--
> >>>  include/uapi/linux/media-bus-format.h   |   3 +-
> >>>  include/uapi/linux/videodev2.h  |   1 +
> >>>  10 files changed, 333 insertions(+), 86 deletions(-)
> >>>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-xv15.rst
> >>>
> >>
> 

Re: [PATCH 0/4] media: ov2680: follow up from initial version

[Sakari Ailus]

Hi,

On Mon, May 07, 2018 at 04:56:51PM +0100, Rui Miguel Silva wrote:
> Sorry I have Out-of-Office some part of last week, I had v6 of the original
> series ready but since I have received the notification from patchwork that 
> the
> v5 was accepted, I am sending this as a follow up tha address Fabio comments.

I think I somehow managed to get the patchwork state wrong; the original
ov2680 set hasn't been applied yet. The latest patches I have applied are
here:



Could you merge this to the original set and resend, please?

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

wir bieten 2% Kredite

[Ronald Bernstein]

Sehr geehrte Damen und Herren,

Sie brauchen Geld? Sie sind auf der suche nach einem Darlehen? Seriös und
unkompliziert?
Dann sind Sie hier bei uns genau richtig.
Durch unsere jahrelange Erfahrung und kompetente Beratung sind wir
Europaweit tätig.

Wir bieten jedem ein GÜNSTIGES Darlehen zu TOP Konditionen an.
Darlehnen zwischen 5000 CHF/Euro bis zu 20 Millionen CHF/Euro möglich.
Wir erheben dazu 2% Zinssatz.

Lassen Sie sich von unserem kompetenten Team beraten.

Zögern Sie nicht und kontaktieren Sie mich unter für weitere Infos &
Anfragen unter der eingeblendeten Email Adresse.


Ich freue mich von Ihnen zu hören.

Re: [PATCH v9 11/15] vb2: add in-fence support to QBUF

[Ezequiel Garcia]

On Mon, 2018-05-07 at 14:07 +0200, Hans Verkuil wrote:
> On 04/05/18 22:06, Ezequiel Garcia wrote:
> > From: Gustavo Padovan 
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers can't be queued to the
> > driver before its fences signal. And a buffer can't be queued to the driver
> > out of the order they were queued from userspace. That means that even if
> > its fence signals it must wait for all other buffers, ahead of it in the 
> > queue,
> > to signal first.
> > 
> > If the fence for some buffer fails we do not queue it to the driver,
> > instead we mark it as error and wait until the previous buffer is done
> > to notify userspace of the error. We wait here to deliver the buffers back
> > to userspace in order.
> > 
> > v12: fixed dvb_vb2.c usage of vb2_core_qbuf.
> > 
> > v11: - minor doc/comments fixes (Hans Verkuil)
> >  - reviewed the in-fence path at __fill_v4l2_buffer()
> > 
> > v10: - rename fence to in_fence in many places
> >  - handle fences signalling with error better (Hans Verkuil)
> > 
> > v9: - improve comments and docs (Hans Verkuil)
> > - fix unlocking of vb->fence_cb_lock on vb2_core_qbuf (Hans Verkuil)
> > - move in-fences code that was in the out-fences patch here (Alex)
> > 
> > v8: - improve comments about fences with errors
> > 
> > v7: - get rid of the fence array stuff for ordering and just use
> >   get_num_buffers_ready() (Hans)
> > - fix issue of queuing the buffer twice (Hans)
> > - avoid the dma_fence_wait() in core_qbuf() (Alex)
> > - merge preparation commit in
> > 
> > v6: - With fences always keep the order userspace queues the buffers.
> > - Protect in_fence manipulation with a lock (Brian Starkey)
> > - check if fences have the same context before adding a fence array
> > - Fix last_fence ref unbalance in __set_in_fence() (Brian Starkey)
> > - Clean up fence if __set_in_fence() fails (Brian Starkey)
> > - treat -EINVAL from dma_fence_add_callback() (Brian Starkey)
> > 
> > v5: - use fence_array to keep buffers ordered in vb2 core when
> >   needed (Brian Starkey)
> > - keep backward compat on the reserved2 field (Brian Starkey)
> > - protect fence callback removal with lock (Brian Starkey)
> > 
> > v4: - Add a comment about dma_fence_add_callback() not returning a
> >   error (Hans)
> > - Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
> > - select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
> > - Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans)
> > - Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
> > - Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
> >   vb2_start_streaming() (Hans)
> > - set IN_FENCE flags on __fill_v4l2_buffer (Hans)
> > - Queue buffers to the driver as soon as they are ready (Hans)
> > - call fill_user_buffer() after queuing the buffer (Hans)
> > - add err: label to clean up fence
> > - add dma_fence_wait() before calling vb2_start_streaming()
> > 
> > v3: - document fence parameter
> > - remove ternary if at vb2_qbuf() return (Mauro)
> > - do not change if conditions behaviour (Mauro)
> > 
> > v2: - fix vb2_queue_or_prepare_buf() ret check
> > - remove check for VB2_MEMORY_DMABUF only (Javier)
> > - check num of ready buffers to start streaming
> > - when queueing, start from the first ready buffer
> > - handle queue cancel
> > 
> > Signed-off-by: Gustavo Padovan 
> > Signed-off-by: Ezequiel Garcia 
> > ---
> >  drivers/media/common/videobuf2/videobuf2-core.c | 197 
> > 
> >  drivers/media/common/videobuf2/videobuf2-v4l2.c |  34 +++-
> >  drivers/media/dvb-core/dvb_vb2.c|   2 +-
> >  drivers/media/v4l2-core/Kconfig |  33 
> >  include/media/videobuf2-core.h  |  14 +-
> >  5 files changed, 249 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> > b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 6b8e083893ad..996b99497a98 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -352,6 +352,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> > vb2_memory memory,
> > vb->index = q->num_buffers + buffer;
> > vb->type = q->type;
> > vb->memory = memory;
> > +   spin_lock_init(>fence_cb_lock);
> > for (plane = 0; plane < num_planes; ++plane) {
> > vb->planes[plane].length = plane_sizes[plane];
> > vb->planes[plane].min_length = plane_sizes[plane];
> > @@ -905,20 +906,12 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, 
> > unsigned int plane_no)
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_plane_cookie);
> >  

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Steve Longerbeam]

Hi Jan,


On 05/08/2018 07:13 AM, Jan Lübbe wrote:

Hi,

On Mon, 2018-05-07 at 11:21 -0700, Steve Longerbeam wrote:

In other words, if the sensor bus is parallel, only 8-bit bus UYVY_2x8
and YUYV_2x8 can be routed to the IC pad or component packed/unpacked
by the IDMAC pad. All other sensor formats on a parallel bus (8 or 16
bit) must be sent to IDMAC pad as pass-through.

I think the code can be simplified/made more readable because of this,
something like:

static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
{
  return ep->bus_type != V4L2_MBUS_CSI2;
}

static inline bool requires_pass_through(
  struct v4l2_fwnode_endpoint *ep,
  struct v4l2_mbus_framefmt *infmt,
  const struct imx_media_pixfmt *incc) {
  return incc->bayer || (is_parallel_bus(ep) && infmt->code !=
UYVY_2x8 && infmt->code != YUYV_2x8);
}


Then requires_pass_through() can be used everywhere we need to
determine the pass-though requirement.

OK, i've added these helper functions. In csi_link_validate() we don't
have the infmt handy, but as the downstream elements check if they have
a native format anyway, this check is redundant and so i've dropped it.


Makes sense.




Also, there's something wrong with the 'switch (image.pix.pixelformat)
{...}' block in csi_idmac_setup_channel(). Pass-though, burst size, pass-though
bits, should be determined by input media-bus code, not final capture V4L2 pix
format.

I just followed the existing code there, which already configures all
of these.


Sorry never mind, I forgot that there is a need to check for planar formats
here.


Assuming that above does not work (and indeed parallel RGB565
must be handled as pass-through), then I think support for capturing
parallel RGB555 as pass-through should be added to this series as
well.

I don't have a sensor which produces RGB555, so it wouldn't be able to
test it.

Understood, but for code readability and consistency I think the code
can be cleaned up as above.

Yes, i've changed that for v2.




The new macros can be used in more places. I will respond to v2 patch.

Steve

[PATCH v4] media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions

[Sami Tolvanen]

This change removes IOCTL_INFO_STD and adds stub functions where
needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call
mismatches with Control-Flow Integrity, caused by calling standard
ioctls using a function pointer that doesn't match the function type.

Signed-off-by: Sami Tolvanen 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
 1 file changed, 130 insertions(+), 115 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96dbe69e0..a40dbec271f1d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
unsigned int ioctl;
u32 flags;
const char * const name;
-   union {
-   u32 offset;
-   int (*func)(const struct v4l2_ioctl_ops *ops,
-   struct file *file, void *fh, void *p);
-   } u;
+   int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
+   void *fh, void *p);
void (*debug)(const void *arg, bool write_only);
 };
 
@@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
 #define INFO_FL_PRIO   (1 << 0)
 /* This control can be valid if the filehandle passes a control handler. */
 #define INFO_FL_CTRL   (1 << 1)
-/* This is a standard ioctl, no need for special code */
-#define INFO_FL_STD(1 << 2)
-/* This is ioctl has its own function */
-#define INFO_FL_FUNC   (1 << 3)
 /* Queuing ioctl */
-#define INFO_FL_QUEUE  (1 << 4)
+#define INFO_FL_QUEUE  (1 << 2)
 /* Always copy back result, even on error */
-#define INFO_FL_ALWAYS_COPY(1 << 5)
+#define INFO_FL_ALWAYS_COPY(1 << 3)
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
  sizeof(((struct v4l2_struct *)0)->field)) << 16)
 #define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
 
-#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)
\
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_STD,  \
-   .name = #_ioctl,\
-   .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
-   .debug = _debug,\
+#define DEFINE_V4L_STUB_FUNC(_vidioc)  \
+   static int v4l_stub_ ## _vidioc(\
+   const struct v4l2_ioctl_ops *ops,   \
+   struct file *file, void *fh, void *p)   \
+   {   \
+   return ops->vidioc_ ## _vidioc(file, fh, p);\
}
 
-#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)  \
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_FUNC, \
-   .name = #_ioctl,\
-   .u.func = _func,\
-   .debug = _debug,\
+#define IOCTL_INFO(_ioctl, _func, _debug, _flags)  \
+   [_IOC_NR(_ioctl)] = {   \
+   .ioctl = _ioctl,\
+   .flags = _flags,\
+   .name = #_ioctl,\
+   .func = _func,  \
+   .debug = _debug,\
}
 
+DEFINE_V4L_STUB_FUNC(g_fbuf)
+DEFINE_V4L_STUB_FUNC(s_fbuf)
+DEFINE_V4L_STUB_FUNC(expbuf)
+DEFINE_V4L_STUB_FUNC(g_std)
+DEFINE_V4L_STUB_FUNC(g_audio)
+DEFINE_V4L_STUB_FUNC(s_audio)
+DEFINE_V4L_STUB_FUNC(g_input)
+DEFINE_V4L_STUB_FUNC(g_edid)
+DEFINE_V4L_STUB_FUNC(s_edid)
+DEFINE_V4L_STUB_FUNC(g_output)
+DEFINE_V4L_STUB_FUNC(g_audout)
+DEFINE_V4L_STUB_FUNC(s_audout)
+DEFINE_V4L_STUB_FUNC(g_jpegcomp)
+DEFINE_V4L_STUB_FUNC(s_jpegcomp)
+DEFINE_V4L_STUB_FUNC(enumaudio)
+DEFINE_V4L_STUB_FUNC(enumaudout)
+DEFINE_V4L_STUB_FUNC(enum_framesizes)
+DEFINE_V4L_STUB_FUNC(enum_frameintervals)
+DEFINE_V4L_STUB_FUNC(g_enc_index)
+DEFINE_V4L_STUB_FUNC(encoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_encoder_cmd)
+DEFINE_V4L_STUB_FUNC(decoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_decoder_cmd)
+DEFINE_V4L_STUB_FUNC(s_dv_timings)
+DEFINE_V4L_STUB_FUNC(g_dv_timings)
+DEFINE_V4L_STUB_FUNC(enum_dv_timings)
+DEFINE_V4L_STUB_FUNC(query_dv_timings)
+DEFINE_V4L_STUB_FUNC(dv_timings_cap)
+
 static struct 

Re: [PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Hans Verkuil]

On 05/08/2018 07:35 PM, Sami Tolvanen wrote:
> This change fixes indirect call mismatches with Control-Flow Integrity
> checking, which are caused by calling standard ioctls using a function
> pointer that doesn't match the type of the actual function.

I really like the patch, but we've drifted away quite a bit from the original
patch, and I think the commit message needs to be updated a bit.

Just mention that IOCTL_INFO_STD has been removed, we now always call a function
and that DEFINE_V4L_STUB_FUNC is used to create stub functions where needed.

Basically the current message describes the problem (good), but it doesn't
describe the chosen solution (bad).

Sorry for asking for a v4, but this is core v4l2 code, so we're more careful
about this.

Regards,

Hans

> 
> Signed-off-by: Sami Tolvanen 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
>  1 file changed, 130 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index de5d96dbe69e0..a40dbec271f1d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
>   unsigned int ioctl;
>   u32 flags;
>   const char * const name;
> - union {
> - u32 offset;
> - int (*func)(const struct v4l2_ioctl_ops *ops,
> - struct file *file, void *fh, void *p);
> - } u;
> + int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
> + void *fh, void *p);
>   void (*debug)(const void *arg, bool write_only);
>  };
>  
> @@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
>  #define INFO_FL_PRIO (1 << 0)
>  /* This control can be valid if the filehandle passes a control handler. */
>  #define INFO_FL_CTRL (1 << 1)
> -/* This is a standard ioctl, no need for special code */
> -#define INFO_FL_STD  (1 << 2)
> -/* This is ioctl has its own function */
> -#define INFO_FL_FUNC (1 << 3)
>  /* Queuing ioctl */
> -#define INFO_FL_QUEUE(1 << 4)
> +#define INFO_FL_QUEUE(1 << 2)
>  /* Always copy back result, even on error */
> -#define INFO_FL_ALWAYS_COPY  (1 << 5)
> +#define INFO_FL_ALWAYS_COPY  (1 << 3)
>  /* Zero struct from after the field to the end */
>  #define INFO_FL_CLEAR(v4l2_struct, field)\
>   ((offsetof(struct v4l2_struct, field) + \
> sizeof(((struct v4l2_struct *)0)->field)) << 16)
>  #define INFO_FL_CLEAR_MASK   (_IOC_SIZEMASK << 16)
>  
> -#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)  
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_STD,  \
> - .name = #_ioctl,\
> - .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
> - .debug = _debug,\
> +#define DEFINE_V4L_STUB_FUNC(_vidioc)\
> + static int v4l_stub_ ## _vidioc(\
> + const struct v4l2_ioctl_ops *ops,   \
> + struct file *file, void *fh, void *p)   \
> + {   \
> + return ops->vidioc_ ## _vidioc(file, fh, p);\
>   }
>  
> -#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_FUNC, \
> - .name = #_ioctl,\
> - .u.func = _func,\
> - .debug = _debug,\
> +#define IOCTL_INFO(_ioctl, _func, _debug, _flags)\
> + [_IOC_NR(_ioctl)] = {   \
> + .ioctl = _ioctl,\
> + .flags = _flags,\
> + .name = #_ioctl,\
> + .func = _func,  \
> + .debug = _debug,\
>   }
>  
> +DEFINE_V4L_STUB_FUNC(g_fbuf)
> +DEFINE_V4L_STUB_FUNC(s_fbuf)
> +DEFINE_V4L_STUB_FUNC(expbuf)
> +DEFINE_V4L_STUB_FUNC(g_std)
> +DEFINE_V4L_STUB_FUNC(g_audio)
> +DEFINE_V4L_STUB_FUNC(s_audio)
> +DEFINE_V4L_STUB_FUNC(g_input)
> +DEFINE_V4L_STUB_FUNC(g_edid)
> +DEFINE_V4L_STUB_FUNC(s_edid)
> +DEFINE_V4L_STUB_FUNC(g_output)
> +DEFINE_V4L_STUB_FUNC(g_audout)
> 

[PATCH v3] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Sami Tolvanen]

This change fixes indirect call mismatches with Control-Flow Integrity
checking, which are caused by calling standard ioctls using a function
pointer that doesn't match the type of the actual function.

Signed-off-by: Sami Tolvanen 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
 1 file changed, 130 insertions(+), 115 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96dbe69e0..a40dbec271f1d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
unsigned int ioctl;
u32 flags;
const char * const name;
-   union {
-   u32 offset;
-   int (*func)(const struct v4l2_ioctl_ops *ops,
-   struct file *file, void *fh, void *p);
-   } u;
+   int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
+   void *fh, void *p);
void (*debug)(const void *arg, bool write_only);
 };
 
@@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
 #define INFO_FL_PRIO   (1 << 0)
 /* This control can be valid if the filehandle passes a control handler. */
 #define INFO_FL_CTRL   (1 << 1)
-/* This is a standard ioctl, no need for special code */
-#define INFO_FL_STD(1 << 2)
-/* This is ioctl has its own function */
-#define INFO_FL_FUNC   (1 << 3)
 /* Queuing ioctl */
-#define INFO_FL_QUEUE  (1 << 4)
+#define INFO_FL_QUEUE  (1 << 2)
 /* Always copy back result, even on error */
-#define INFO_FL_ALWAYS_COPY(1 << 5)
+#define INFO_FL_ALWAYS_COPY(1 << 3)
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
  sizeof(((struct v4l2_struct *)0)->field)) << 16)
 #define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
 
-#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)
\
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_STD,  \
-   .name = #_ioctl,\
-   .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
-   .debug = _debug,\
+#define DEFINE_V4L_STUB_FUNC(_vidioc)  \
+   static int v4l_stub_ ## _vidioc(\
+   const struct v4l2_ioctl_ops *ops,   \
+   struct file *file, void *fh, void *p)   \
+   {   \
+   return ops->vidioc_ ## _vidioc(file, fh, p);\
}
 
-#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)  \
-   [_IOC_NR(_ioctl)] = {   \
-   .ioctl = _ioctl,\
-   .flags = _flags | INFO_FL_FUNC, \
-   .name = #_ioctl,\
-   .u.func = _func,\
-   .debug = _debug,\
+#define IOCTL_INFO(_ioctl, _func, _debug, _flags)  \
+   [_IOC_NR(_ioctl)] = {   \
+   .ioctl = _ioctl,\
+   .flags = _flags,\
+   .name = #_ioctl,\
+   .func = _func,  \
+   .debug = _debug,\
}
 
+DEFINE_V4L_STUB_FUNC(g_fbuf)
+DEFINE_V4L_STUB_FUNC(s_fbuf)
+DEFINE_V4L_STUB_FUNC(expbuf)
+DEFINE_V4L_STUB_FUNC(g_std)
+DEFINE_V4L_STUB_FUNC(g_audio)
+DEFINE_V4L_STUB_FUNC(s_audio)
+DEFINE_V4L_STUB_FUNC(g_input)
+DEFINE_V4L_STUB_FUNC(g_edid)
+DEFINE_V4L_STUB_FUNC(s_edid)
+DEFINE_V4L_STUB_FUNC(g_output)
+DEFINE_V4L_STUB_FUNC(g_audout)
+DEFINE_V4L_STUB_FUNC(s_audout)
+DEFINE_V4L_STUB_FUNC(g_jpegcomp)
+DEFINE_V4L_STUB_FUNC(s_jpegcomp)
+DEFINE_V4L_STUB_FUNC(enumaudio)
+DEFINE_V4L_STUB_FUNC(enumaudout)
+DEFINE_V4L_STUB_FUNC(enum_framesizes)
+DEFINE_V4L_STUB_FUNC(enum_frameintervals)
+DEFINE_V4L_STUB_FUNC(g_enc_index)
+DEFINE_V4L_STUB_FUNC(encoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_encoder_cmd)
+DEFINE_V4L_STUB_FUNC(decoder_cmd)
+DEFINE_V4L_STUB_FUNC(try_decoder_cmd)
+DEFINE_V4L_STUB_FUNC(s_dv_timings)
+DEFINE_V4L_STUB_FUNC(g_dv_timings)
+DEFINE_V4L_STUB_FUNC(enum_dv_timings)
+DEFINE_V4L_STUB_FUNC(query_dv_timings)
+DEFINE_V4L_STUB_FUNC(dv_timings_cap)
+
 static struct v4l2_ioctl_info v4l2_ioctls[] = {
-   IOCTL_INFO_FNC(VIDIOC_QUERYCAP, 

Re: [PATCH v2] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Sami Tolvanen]

On Tue, May 08, 2018 at 07:22:21PM +0200, Hans Verkuil wrote:
> This looks good, I would just rename DEFINE_IOCTL_FNC to DEFINE_V4L_STUB_FUNC.
> This makes it clear that it defines a v4l stub function.

Sure, sounds good. I'll send v3 shortly. Thanks for the reviews!

Sami

Re: [PATCH v3 13/14] media: imx7.rst: add documentation for i.MX7 media driver

[Randy Dunlap]

Hi,

I have a few editing suggestions below...

On 05/07/2018 09:21 AM, Rui Miguel Silva wrote:
> Add rst document to describe the i.MX7 media driver and also a working example
> from the Warp7 board usage with a OV2680 sensor.
> 
> Signed-off-by: Rui Miguel Silva 
> ---
>  Documentation/media/v4l-drivers/imx7.rst  | 157 ++
>  Documentation/media/v4l-drivers/index.rst |   1 +
>  2 files changed, 158 insertions(+)
>  create mode 100644 Documentation/media/v4l-drivers/imx7.rst
> 
> diff --git a/Documentation/media/v4l-drivers/imx7.rst 
> b/Documentation/media/v4l-drivers/imx7.rst
> new file mode 100644
> index ..64b97b442277
> --- /dev/null
> +++ b/Documentation/media/v4l-drivers/imx7.rst
> @@ -0,0 +1,157 @@
> +i.MX7 Video Capture Driver
> +==
> +
> +Introduction
> +
> +
> +The i.MX7 contrary to the i.MX5/6 family does not contain an Image Processing
> +Unit (IPU), because of that the capabilities to perform operations or

s/,/;/

> +manipulation of the capture frames is less feature rich.

  are less

> +
> +For image capture the i.MX7 have three units:

   has

> +- CMOS Sensor Interface (CSI)
> +- Video Multiplexer
> +- MIPI CSI-2 Receiver
> +
> +::
> +   |\
> +   MIPI Camera Input ---> MIPI CSI-2 --- > | \
> +   |  \
> +   | M |
> +   | U | -->  CSI ---> Capture
> +   | X |
> +   |  /
> +   Parallel Camera Input > | /
> +   |/
> +
> +For additional information, please refer to the latest versions of the i.MX7
> +reference manual [#f1]_.
> +
> +Entities
> +
> +
> +imx7-mipi-csi2
> +--
> +
> +This is the MIPI CSI-2 recevier entity. It has one sink pad to receive the 
> pixel

  receiver

> +data from MIPI CSI-2 camera sensor. It has one source pad, corresponding to 
> the
> +virtual channel 0. This module is compliant to previous version of Samsung
> +D-phy, and support two D-PHY Rx Data lanes.

  supports

> +
> +csi_mux
> +---
> +
> +This is the video multiplexer. It has two sink pads to select from either 
> camera
> +sensors with a parallel interface or from MIPI CSI-2 virtual channel 0.  It 
> has

   sensor

> +a single source pad that routes to the CSI.
> +
> +csi
> +---
> +
> +The CSI enables the chip to connect directly to external CMOS image sensor. 
> CSI
> +can interfaces directly with Parallel and MIPI CSI-2 buses. It has 256 x 64 
> FIFO

   interface

> +to store received image pixel data and embedded DMA controllers to transfer 
> data
> +from the FIFO through AHB bus.
> +
> +This entity has one sink pad that receive from the csi_mux entity and a 
> single

 receives

> +source pad that route video frames directly to memory buffers, this pad is

   routesbuffers. This pad is

> +routed to a capture device node.
> +
> +Usage Notes
> +---
> +
> +To aid in configuration and for backward compatibility with V4L2 applications
> +that access controls only from video device nodes, the capture device 
> interfaces
> +inherit controls from the active entities in the current pipeline, so 
> controls
> +can be accessed either directly from the subdev or from the active capture
> +device interface. For example, the sensor controls are available either from 
> the
> +sensor subdevs or from the active capture device.
> +
> +Warp7 with OV2680
> +-
> +
> +On this platform an OV2680 MIPI CSI-2 module is connected to the internal 
> MIPI
> +CSI-2 receiver. The following example configures a video capture pipeline 
> with
> +an output of 800x600, and BGGR 10 bit bayer format:
> +
> +.. code-block:: none
> +   # Setup links
> +   media-ctl -l "'ov2680 1-0036':0 -> 'imx7-mipi-csis.0':0[1]"
> +   media-ctl -l "'imx7-mipi-csis.0':1 -> 'csi_mux':1[1]"
> +   media-ctl -l "'csi_mux':2 -> 'csi':0[1]"
> +   media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> +
> +   # Configure pads for pipeline
> +   media-ctl -V "'ov2680 1-0036':0 [fmt:SBGGR10_1X10/800x600 field:none]"
> +   media-ctl -V "'csi_mux':1 [fmt:SBGGR10_1X10/800x600 field:none]"
> +   media-ctl -V "'csi_mux':2 [fmt:SBGGR10_1X10/800x600 field:none]"
> +   media-ctl -V "'imx7-mipi-csis.0':0 [fmt:SBGGR10_1X10/800x600 field:none]"
> +   media-ctl -V "'csi':0 [fmt:SBGGR10_1X10/800x600 field:none]"
> +
> +After this streaming can start, the v4l2-ctl tool can be used to select any 
> of

can start. The

> +the resolutions supported by the sensor.
> +
> +.. code-block:: none
> +root@imx7s-warp:~# media-ctl -p
> +Media 

Re: [PATCH v2] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Hans Verkuil]

On 05/08/2018 07:17 PM, Sami Tolvanen wrote:
> On Tue, May 08, 2018 at 10:18:24AM +0200, Hans Verkuil wrote:
>> Just call this v4l_stub_g_fbuf, conform the naming of the other functions.
>>
>> So just replace vidioc_ by v4l_stub_ in all these DEFINE_IOCTL_FNC macros.
>>
>> This way the function name in the big array matches the name in this macro,
>> and the 'stub' part indicates that it is just a stub function.
> 
> vidioc_ is actually part of the function name in struct v4l2_ioctl_ops,

I'm stupid, I should have seen that.

> which the stub needs to call. I can change the stub name to start with
> v4l_stub_, but if you prefer to drop vidioc_ entirely from the name,
> the macro still wouldn't end up matching the array. It would have to be
> something like this:
> 
>   #define DEFINE_IOCTL_FNC(_vidioc) \
>   static int v4l_stub_ ## _vidioc( \
>   ...
>   return ops->vidioc_ ## _vidioc(file, fh, p); \
>   ...
>   DEFINE_IOCTL_FNC(g_fbuf)
>   ...
>   static struct v4l2_ioctl_info v4l2_ioctls[] = {
>   ...
>   IOCTL_INFO(VIDIOC_G_FBUF, v4l_stub_g_fbuf, ...),

This looks good, I would just rename DEFINE_IOCTL_FNC to DEFINE_V4L_STUB_FUNC.
This makes it clear that it defines a v4l stub function.

Regards,

Hans

Re: [PATCH v2] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Sami Tolvanen]

On Tue, May 08, 2018 at 10:18:24AM +0200, Hans Verkuil wrote:
> Just call this v4l_stub_g_fbuf, conform the naming of the other functions.
> 
> So just replace vidioc_ by v4l_stub_ in all these DEFINE_IOCTL_FNC macros.
> 
> This way the function name in the big array matches the name in this macro,
> and the 'stub' part indicates that it is just a stub function.

vidioc_ is actually part of the function name in struct v4l2_ioctl_ops,
which the stub needs to call. I can change the stub name to start with
v4l_stub_, but if you prefer to drop vidioc_ entirely from the name,
the macro still wouldn't end up matching the array. It would have to be
something like this:

  #define DEFINE_IOCTL_FNC(_vidioc) \
static int v4l_stub_ ## _vidioc( \
...
return ops->vidioc_ ## _vidioc(file, fh, p); \
  ...
  DEFINE_IOCTL_FNC(g_fbuf)
  ...
  static struct v4l2_ioctl_info v4l2_ioctls[] = {
...
IOCTL_INFO(VIDIOC_G_FBUF, v4l_stub_g_fbuf, ...),

Any thoughts?

Sami

Re: [PATCH 1/2] media: dt-bindings: add binding for TI SCAN921226H video deserializer

[Rob Herring]

On Fri, May 04, 2018 at 02:49:02PM +0200, Jan Luebbe wrote:
> This deserializer can be used with sensors that directly produce a
> 10-bit LVDS stream and converts it to a parallel bus.
> 
> Controlling it via the optional GPIOs is mainly useful for avoiding
> conflicts when another parallel sensor is connected to the same data bus
> as the deserializer.
> 
> Signed-off-by: Jan Luebbe 
> ---
>  .../bindings/media/ti,scan921226h.txt | 59 +++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,scan921226h.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,scan921226h.txt 
> b/Documentation/devicetree/bindings/media/ti,scan921226h.txt
> new file mode 100644
> index ..4e475672d7bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,scan921226h.txt
> @@ -0,0 +1,59 @@
> +TI SCAN921226H Video Deserializer
> +-
> +
> +The SCAN921226H receives a LVDS serial data stream with embedded clock and
> +converts it to a 10-bit wide parallel data bus and recovers parallel clock.
> +Some CMOS sensors such as the ON Semiconductor MT9V024 produce a LVDS signal
> +compatible with this deserializer.
> +
> +Required properties:
> +- compatible : should be "ti,scan921226h"
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@0: serial (LVDS) input
> +- port@1: parallel output
> +
> +The device node should contain two 'port' child nodes (one each for input and
> +output), in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Optional Properties:
> +- enable-gpios: reference to the GPIO connected to the REN (output enable) 
> pin,
> +  if any.
> +- npwrdn-gpios: reference to the GPIO connected to the nPWRDN pin, if any.

Use the standard powerdown-gpios here.

Both should state the active level.

> +
> +Optionally, #address-cells, #size-cells, and port nodes can be grouped under 
> a
> +ports node as described in Documentation/devicetree/bindings/graph.txt.
> +
> +Example:
> +
> +  csi0_deserializer: csi0_deserializer {

Don't use '_' in node names.

> +  compatible = "ti,scan921226h";
> +
> +  enable-gpios = < 20 GPIO_ACTIVE_HIGH>;
> +  npwrdn-gpios = < 24 GPIO_ACTIVE_HIGH>;
> +
> +  #address-cells = <1>;
> +  #size-cells = <0>;
> +
> +  /* serial sink interface */
> +  port@0 {
> +  reg = <0>;
> +
> +  des0_in: endpoint {
> +  remote-endpoint = <_0_out>;
> +  };
> +  };
> +
> +  /* parallel source interface */
> +  port@1 {
> +  reg = <1>;
> +
> +  des0_out: endpoint {
> +  remote-endpoint = 
> <_csi0_mux_from_parallel_sensor>;
> +  bus-width = <8>;
> +  hsync-active = <1>;
> +  vsync-active = <1>;
> +  };
> +  };
> +  };
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

UVC defines a method of handling asynchronous controls, which sends a
USB packet over the interrupt pipe. This patch implements support for
such packets by sending a control event to the user. Since this can
involve USB traffic and, therefore, scheduling, this has to be done
in a work queue.

Signed-off-by: Guennadi Liakhovetski 
---

v8:

* avoid losing events by delaying the status URB resubmission until
  after completion of the current event
* extract control value calculation into __uvc_ctrl_get_value()
* do not proactively return EBUSY if the previous control hasn't
  completed yet, let the camera handle such cases
* multiple cosmetic changes

 drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++---
 drivers/media/usb/uvc/uvc_status.c | 112 ++---
 drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
 drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
 include/uapi/linux/uvcvideo.h  |   2 +
 5 files changed, 255 insertions(+), 44 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 2a213c8..796f86a 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain 
*chain,
return 0;
 }
 
+static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
+   const u8 *data)
+{
+   s32 value = mapping->get(mapping, UVC_GET_CUR, data);
+
+   if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
+   struct uvc_menu_info *menu = mapping->menu_info;
+   unsigned int i;
+
+   for (i = 0; i < mapping->menu_count; ++i, ++menu) {
+   if (menu->value == value) {
+   value = i;
+   break;
+   }
+   }
+   }
+
+   return value;
+}
+
 static int __uvc_ctrl_get(struct uvc_video_chain *chain,
struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
s32 *value)
 {
-   struct uvc_menu_info *menu;
-   unsigned int i;
int ret;
 
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
@@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
ctrl->loaded = 1;
}
 
-   *value = mapping->get(mapping, UVC_GET_CUR,
-   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
-
-   if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
-   menu = mapping->menu_info;
-   for (i = 0; i < mapping->menu_count; ++i, ++menu) {
-   if (menu->value == *value) {
-   *value = i;
-   break;
-   }
-   }
-   }
+   *value = __uvc_ctrl_get_value(mapping,
+   uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
 
return 0;
 }
@@ -1222,30 +1231,121 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
 {
struct v4l2_subscribed_event *sev;
struct v4l2_event ev;
+   bool autoupdate;
 
if (list_empty(>ev_subs))
return;
 
+   if (!handle) {
+   autoupdate = true;
+   sev = list_first_entry(>ev_subs,
+  struct v4l2_subscribed_event, node);
+   handle = container_of(sev->fh, struct uvc_fh, vfh);
+   } else {
+   autoupdate = false;
+   }
+
uvc_ctrl_fill_event(handle->chain, , ctrl, mapping, value, changes);
 
list_for_each_entry(sev, >ev_subs, node) {
if (sev->fh != >vfh ||
(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
-   (changes & V4L2_EVENT_CTRL_CH_FLAGS))
+   (changes & V4L2_EVENT_CTRL_CH_FLAGS) || autoupdate)
v4l2_event_queue_fh(sev->fh, );
}
 }
 
-static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
-   struct uvc_control *master, u32 slave_id,
-   const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
+   struct uvc_video_chain *chain, struct uvc_control *master, u32 slave_id)
 {
struct uvc_control_mapping *mapping = NULL;
struct uvc_control *ctrl = NULL;
u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
-   unsigned int i;
s32 val = 0;
 
+   __uvc_find_control(master->entity, slave_id, , , 0);
+   if (ctrl == NULL)
+   return;
+
+   if (__uvc_ctrl_get(handle ? handle->chain : chain, ctrl, mapping,
+  ) == 0)
+   changes |= V4L2_EVENT_CTRL_CH_VALUE;
+
+   uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+}
+
+static void uvc_ctrl_status_event_work(struct 

[PATCH v8 3/3] uvcvideo: handle control pipe protocol STALLs

[Guennadi Liakhovetski]

When a command ends up in a STALL on the control pipe, use the Request
Error Code control to provide a more precise error information to the
user. For example, if a camera is still busy processing a control,
when the same or an interrelated control set request arrives, the
camera can react with a STALL and then return the "Not ready" status
in response to a UVC_VC_REQUEST_ERROR_CODE_CONTROL command. With this
patch the user would then get an EBUSY error code instead of a
generic EPIPE.

Signed-off-by: Guennadi Liakhovetski 
---

v8:

* restrict UVC_VC_REQUEST_ERROR_CODE_CONTROL to the control interface
* fix the wIndex value
* improve error codes
* cosmetic changes

 drivers/media/usb/uvc/uvc_video.c | 52 ++-
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c 
b/drivers/media/usb/uvc/uvc_video.c
index aa0082f..ce65cd6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -73,17 +73,57 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 
unit,
u8 intfnum, u8 cs, void *data, u16 size)
 {
int ret;
+   u8 error;
+   u8 tmp;
 
ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
UVC_CTRL_CONTROL_TIMEOUT);
-   if (ret != size) {
-   uvc_printk(KERN_ERR, "Failed to query (%s) UVC control %u on "
-   "unit %u: %d (exp. %u).\n", uvc_query_name(query), cs,
-   unit, ret, size);
-   return -EIO;
+   if (likely(ret == size))
+   return 0;
+
+   uvc_printk(KERN_ERR,
+  "Failed to query (%s) UVC control %u on unit %u: %d (exp. 
%u).\n",
+  uvc_query_name(query), cs, unit, ret, size);
+
+   if (ret != -EPIPE)
+   return ret;
+
+   tmp = *(u8 *)data;
+
+   ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
+  UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
+  UVC_CTRL_CONTROL_TIMEOUT);
+
+   error = *(u8 *)data;
+   *(u8 *)data = tmp;
+
+   if (ret != 1)
+   return ret < 0 ? ret : -EPIPE;
+
+   uvc_trace(UVC_TRACE_CONTROL, "Control error %u\n", error);
+
+   switch (error) {
+   case 0:
+   /* Cannot happen - we received a STALL */
+   return -EPIPE;
+   case 1: /* Not ready */
+   return -EBUSY;
+   case 2: /* Wrong state */
+   return -EILSEQ;
+   case 3: /* Power */
+   return -EREMOTE;
+   case 4: /* Out of range */
+   return -ERANGE;
+   case 5: /* Invalid unit */
+   case 6: /* Invalid control */
+   case 7: /* Invalid Request */
+   case 8: /* Invalid value within range */
+   return -EINVAL;
+   default: /* reserved or unknown */
+   break;
}
 
-   return 0;
+   return -EPIPE;
 }
 
 static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
-- 
1.9.3

[PATCH v8 1/3] uvcvideo: remove a redundant check

[Guennadi Liakhovetski]

Event subscribers cannot have a NULL file handle. They are only added
at a single location in the code, and the .fh pointer is used without
checking there.

Signed-off-by: Guennadi Liakhovetski 
---
 drivers/media/usb/uvc/uvc_ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a36b4fb..2a213c8 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1229,9 +1229,9 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
uvc_ctrl_fill_event(handle->chain, , ctrl, mapping, value, changes);
 
list_for_each_entry(sev, >ev_subs, node) {
-   if (sev->fh && (sev->fh != >vfh ||
+   if (sev->fh != >vfh ||
(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
-   (changes & V4L2_EVENT_CTRL_CH_FLAGS)))
+   (changes & V4L2_EVENT_CTRL_CH_FLAGS))
v4l2_event_queue_fh(sev->fh, );
}
 }
-- 
1.9.3

[PATCH v8 0/3] uvcvideo: asynchronous controls

[Guennadi Liakhovetski]

Added a patch to remove a redundant check, addressed Laurent's
comments.

Guennadi Liakhovetski (3):
  uvcvideo: remove a redundant check
  uvcvideo: send a control event when a Control Change interrupt arrives
  uvcvideo: handle control pipe protocol STALLs

 drivers/media/usb/uvc/uvc_ctrl.c   | 168 ++---
 drivers/media/usb/uvc/uvc_status.c | 112 ++---
 drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
 drivers/media/usb/uvc/uvc_video.c  |  52 ++--
 drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
 include/uapi/linux/uvcvideo.h  |   2 +
 6 files changed, 302 insertions(+), 51 deletions(-)

-- 
1.9.3

Thanks
Guennadi

Re: [PATCH v3 07/14] media: dt-bindings: add bindings for i.MX7 media driver

[Philipp Zabel]

On Mon, 2018-05-07 at 17:21 +0100, Rui Miguel Silva wrote:
> Add bindings documentation for i.MX7 media drivers.
> 
> Signed-off-by: Rui Miguel Silva 
> ---
>  .../devicetree/bindings/media/imx7.txt| 152 ++
>  1 file changed, 152 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/imx7.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/imx7.txt 
> b/Documentation/devicetree/bindings/media/imx7.txt
> new file mode 100644
> index ..06d723d6354d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx7.txt
> @@ -0,0 +1,152 @@
> +Freescale i.MX7 Media Video Device
> +==
> +
> +Video Media Controller node
> +---
> +
> +This is the media controller node for video capture support. It is a
> +virtual device that lists the camera serial interface nodes that the
> +media device will control.
> +
> +Required properties:
> +- compatible : "fsl,imx7-capture-subsystem";
> +- ports  : Should contain a list of phandles pointing to camera
> + sensor interface port of CSI
> +
> +example:
> +
> +capture-subsystem {
> + compatible = "fsl,imx7-capture-subsystem";
> + ports = <>;
> +};

Purely from a device tree perspective, I think this node is unnecessary
on i.MX7. The reason we have it for i.MX6 is that there are two
identical IPUs on some of them, which makes it impossible to reasonably
bind the subsystem driver to one or the other of the ipu nodes. On i.MX7
the csi node is unique.

The relevant imx-media-dev.c code in imx_media_probe could be refactored
into a utility function that could be called from the probe function of
the csi driver as well.

[...]
> +csi node
> +
> +
> +This is device node for the CMOS Sensor Interface (CSI) which enables the 
> chip
> +to connect directly to external CMOS image sensors.
> +
> +Required properties:
> +
> +- compatible: "fsl,imx7-csi";
> +- reg   : base address and length of the register set for the device;
> +- interrupts: should contain CSI interrupt;
> +- clocks: list of clock specifiers, see
> +Documentation/devicetree/bindings/clock/clock-bindings.txt for 
> details;
> +- clock-names   : must contain "axi", "mclk" and "dcic" entries, matching
> + entries in the clock property;
> +
> +port node
> +-
> +
> +- reg  : (required) should be 0 for the sink port;

Not necessary, see below.

> +
> +example:
> +
> +csi: csi@3071 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +compatible = "fsl,imx7-csi";
> +reg = <0x3071 0x1>;
> +interrupts = ;
> +clocks = < IMX7D_CLK_DUMMY>,
> +< IMX7D_CSI_MCLK_ROOT_CLK>,
> +< IMX7D_CLK_DUMMY>;
> +clock-names = "axi", "mclk", "dcic";
> +
> +port@0 {
> +reg = <0>;

Since there is only one port, it does not need to be numbered.

> +
> +csi_from_csi_mux: endpoint {
> +remote-endpoint = <_mux_to_csi>;
> +};
> +};
> +};

regards
Philipp

Re: [PATCH v2 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Philipp Zabel]

On Tue, 2018-05-08 at 16:14 +0200, Jan Luebbe wrote:
> The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
> mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
> To handle this, we extend imx_media_pixfmt with a cycles per pixel
> field, which is used for generic formats on the parallel bus.
> 
> Before actually adding RGB565_2X8 support for the parallel bus, this
> series simplifies handing of the the different configurations for RGB565
> between parallel and MIPI CSI-2 in imx-media-capture. This avoids having
> to explicitly pass on the format in the second patch.
> 
> Changes since v1:
>   - fixed problems reported the kbuild test robot
>   - added helper functions as suggested by Steve Longerbeam
> (is_parallel_bus and requires_passthrough)
>   - removed passthough format check in csi_link_validate() (suggested by
> Philipp Zabel during internal review)

The theory is that IC only supports AYUV8_1X32 and RGB888_1X24 input,
and any passthrough format on the CSI sink will differ from those.
Mismatching formats are already caught by v4l2_subdev_link_validate
called on the ipu?_vdic or ipu?_ic_prp entities' sink pads.

regards
Philipp

[PATCH v2 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[Jan Luebbe]

The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
To handle this, we extend imx_media_pixfmt with a cycles per pixel
field, which is used for generic formats on the parallel bus.

Based on the selected format and bus, we then update the width to
account for the multiple cycles per pixel.

The passthrough check in csi_link_validate() can be dropped because the
downstream elements already verifiy their input formats.

Signed-off-by: Jan Luebbe 
---
 drivers/staging/media/imx/imx-media-csi.c   | 68 -
 drivers/staging/media/imx/imx-media-utils.c |  1 +
 drivers/staging/media/imx/imx-media.h   |  2 +
 3 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 08b636084286..af5f52f62a9c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -127,6 +127,20 @@ static inline bool is_parallel_16bit_bus(struct 
v4l2_fwnode_endpoint *ep)
ep->bus.parallel.bus_width >= 16;
 }
 
+static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
+{
+   return ep->bus_type != V4L2_MBUS_CSI2;
+}
+
+static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
+   struct v4l2_mbus_framefmt *infmt,
+   const struct imx_media_pixfmt *incc)
+{
+   return incc->bayer || (is_parallel_bus(ep) &&
+   infmt->code != MEDIA_BUS_FMT_UYVY8_2X8 &&
+   infmt->code != MEDIA_BUS_FMT_YUYV8_2X8);
+}
+
 /*
  * Parses the fwnode endpoint from the source pad of the entity
  * connected to this CSI. This will either be the entity directly
@@ -370,6 +384,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
struct v4l2_mbus_framefmt *infmt;
struct ipu_image image;
u32 passthrough_bits;
+   u32 passthrough_cycles;
dma_addr_t phys[2];
bool passthrough;
u32 burst_size;
@@ -395,6 +410,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 * - raw bayer formats
 * - the CSI is receiving from a 16-bit parallel bus
 */
+   passthrough_cycles = 1;
switch (image.pix.pixelformat) {
case V4L2_PIX_FMT_SBGGR8:
case V4L2_PIX_FMT_SGBRG8:
@@ -431,6 +447,16 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough = is_parallel_16bit_bus(>upstream_ep);
passthrough_bits = 16;
break;
+   case V4L2_PIX_FMT_RGB565:
+   /* without CSI2 we can only use passthrough mode */
+   if (priv->upstream_ep.bus_type != V4L2_MBUS_CSI2) {
+   burst_size = 16;
+   passthrough = true;
+   passthrough_bits = 8;
+   passthrough_cycles = 2;
+   break;
+   }
+   /* fallthrough */
default:
burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough = is_parallel_16bit_bus(>upstream_ep);
@@ -439,7 +465,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
}
 
if (passthrough) {
-   ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+   ipu_cpmem_set_resolution(priv->idmac_ch,
+image.rect.width * passthrough_cycles,
 image.rect.height);
ipu_cpmem_set_stride(priv->idmac_ch, image.pix.bytesperline);
ipu_cpmem_set_buffer(priv->idmac_ch, 0, image.phys0);
@@ -630,11 +657,14 @@ static void csi_idmac_stop(struct csi_priv *priv)
 static int csi_setup(struct csi_priv *priv)
 {
struct v4l2_mbus_framefmt *infmt, *outfmt;
+   const struct imx_media_pixfmt *outcc;
struct v4l2_mbus_config mbus_cfg;
struct v4l2_mbus_framefmt if_fmt;
+   struct v4l2_rect crop;
 
infmt = >format_mbus[CSI_SINK_PAD];
outfmt = >format_mbus[priv->active_output_pad];
+   outcc = priv->cc[priv->active_output_pad];
 
/* compose mbus_config from the upstream endpoint */
mbus_cfg.type = priv->upstream_ep.bus_type;
@@ -648,8 +678,18 @@ static int csi_setup(struct csi_priv *priv)
 */
if_fmt = *infmt;
if_fmt.field = outfmt->field;
+   crop = priv->crop;
 
-   ipu_csi_set_window(priv->csi, >crop);
+   /*
+* if cycles is set, we need to handle this over multiple cycles as
+* generic/bayer data
+*/
+   if (is_parallel_bus(>upstream_ep) && outcc->cycles) {
+   if_fmt.width *= outcc->cycles;
+   crop.width *= outcc->cycles;
+   }
+
+   ipu_csi_set_window(priv->csi, );
 
ipu_csi_set_downsize(priv->csi,
  

[PATCH v2 1/2] media: imx: capture: refactor enum_/try_fmt

[Jan Luebbe]

By checking and handling the internal IPU formats (ARGB or AYUV) first,
we don't need to check whether it's a bayer format, as we can default to
passing the input format on in all other cases.

This simplifies handling the different configurations for RGB565 between
parallel and MIPI CSI-2, as we don't need to check the details of the
format anymore.

Signed-off-by: Jan Luebbe 
---
 drivers/staging/media/imx/imx-media-capture.c | 38 +--
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c 
b/drivers/staging/media/imx/imx-media-capture.c
index 0ccabe04b0e1..64c23ef92931 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -170,23 +170,22 @@ static int capture_enum_fmt_vid_cap(struct file *file, 
void *fh,
}
 
cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
-   if (!cc_src)
-   cc_src = imx_media_find_mbus_format(fmt_src.format.code,
-   CS_SEL_ANY, true);
-   if (!cc_src)
-   return -EINVAL;
-
-   if (cc_src->bayer) {
-   if (f->index != 0)
-   return -EINVAL;
-   fourcc = cc_src->fourcc;
-   } else {
+   if (cc_src) {
u32 cs_sel = (cc_src->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB;
 
ret = imx_media_enum_format(, f->index, cs_sel);
if (ret)
return ret;
+   } else {
+   cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+   CS_SEL_ANY, true);
+   if (WARN_ON(!cc_src))
+   return -EINVAL;
+
+   if (f->index != 0)
+   return -EINVAL;
+   fourcc = cc_src->fourcc;
}
 
f->pixelformat = fourcc;
@@ -219,15 +218,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void 
*fh,
return ret;
 
cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
-   if (!cc_src)
-   cc_src = imx_media_find_mbus_format(fmt_src.format.code,
-   CS_SEL_ANY, true);
-   if (!cc_src)
-   return -EINVAL;
-
-   if (cc_src->bayer) {
-   cc = cc_src;
-   } else {
+   if (cc_src) {
u32 fourcc, cs_sel;
 
cs_sel = (cc_src->cs == IPUV3_COLORSPACE_YUV) ?
@@ -239,6 +230,13 @@ static int capture_try_fmt_vid_cap(struct file *file, void 
*fh,
imx_media_enum_format(, 0, cs_sel);
cc = imx_media_find_format(fourcc, cs_sel, false);
}
+   } else {
+   cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+   CS_SEL_ANY, true);
+   if (WARN_ON(!cc_src))
+   return -EINVAL;
+
+   cc = cc_src;
}
 
imx_media_mbus_fmt_to_pix_fmt(>fmt.pix, _src.format, cc);
-- 
2.17.0

[PATCH v2 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Jan Luebbe]

The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
To handle this, we extend imx_media_pixfmt with a cycles per pixel
field, which is used for generic formats on the parallel bus.

Before actually adding RGB565_2X8 support for the parallel bus, this
series simplifies handing of the the different configurations for RGB565
between parallel and MIPI CSI-2 in imx-media-capture. This avoids having
to explicitly pass on the format in the second patch.

Changes since v1:
  - fixed problems reported the kbuild test robot
  - added helper functions as suggested by Steve Longerbeam
(is_parallel_bus and requires_passthrough)
  - removed passthough format check in csi_link_validate() (suggested by
Philipp Zabel during internal review)

Jan Luebbe (2):
  media: imx: capture: refactor enum_/try_fmt
  media: imx: add support for RGB565_2X8 on parallel bus

 drivers/staging/media/imx/imx-media-capture.c | 38 +--
 drivers/staging/media/imx/imx-media-csi.c | 68 ++-
 drivers/staging/media/imx/imx-media-utils.c   |  1 +
 drivers/staging/media/imx/imx-media.h |  2 +
 4 files changed, 73 insertions(+), 36 deletions(-)

-- 
2.17.0

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Jan Lübbe]

Hi,

On Mon, 2018-05-07 at 11:21 -0700, Steve Longerbeam wrote:
> On 05/07/2018 07:23 AM, Jan Lübbe wrote:
> > On Sat, 2018-05-05 at 15:22 -0700, Steve Longerbeam wrote:
> > > I reviewed this patch series, and while I don't have any
> > > objections to the code-level changes, but my question
> > > is more specifically about how the IPU/CSI deals with
> > > receiving RGB565 over a parallel bus.
> > > 
> > > My understanding was that if the CSI receives RGB565
> > > over a parallel 8-bit sensor bus, the CSI_SENS_DATA_FORMAT
> > > register field is programmed to RGB565, and the CSI outputs
> > > ARGB. Then IDMAC component packing can be setup to
> > > write pixels to memory as RGB565. Does that not work?
> > 
> > This was our first thought too. As far as we can see in our
> > experiments, that mode doesn't actually work for the parallel bus.
> > Philipp's interpretation is that this mode is only intended for use
> > with the MIPI-CSI2 input.
> 
> Ok, that was my suspicion on reading this as well. And it's likely
> that the other sensor formats on parallel busses require pass-through,
> e.g. anything other than UYVY_2x8 and YUYV_2x8 requires
> pass-through.

OK.

> In other words, if the sensor bus is parallel, only 8-bit bus UYVY_2x8
> and YUYV_2x8 can be routed to the IC pad or component packed/unpacked
> by the IDMAC pad. All other sensor formats on a parallel bus (8 or 16 
> bit) must be sent to IDMAC pad as pass-through.
> 
> I think the code can be simplified/made more readable because of this,
> something like:
> 
> static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
> {
>  return ep->bus_type != V4L2_MBUS_CSI2;
> }
> 
> static inline bool requires_pass_through(
>  struct v4l2_fwnode_endpoint *ep,
>  struct v4l2_mbus_framefmt *infmt,
>  const struct imx_media_pixfmt *incc) {
>  return incc->bayer || (is_parallel_bus(ep) && infmt->code != 
> UYVY_2x8 && infmt->code != YUYV_2x8);
> }
> 
> 
> Then requires_pass_through() can be used everywhere we need to
> determine the pass-though requirement.

OK, i've added these helper functions. In csi_link_validate() we don't
have the infmt handy, but as the downstream elements check if they have
a native format anyway, this check is redundant and so i've dropped it.

> Also, there's something wrong with the 'switch (image.pix.pixelformat) 
> {...}' block in csi_idmac_setup_channel(). Pass-though, burst size, 
> pass-though 
> bits, should be determined by input media-bus code, not final capture V4L2 pix
> format.

I just followed the existing code there, which already configures all
of these.

> > > Assuming that above does not work (and indeed parallel RGB565
> > > must be handled as pass-through), then I think support for capturing
> > > parallel RGB555 as pass-through should be added to this series as
> > > well.
> > 
> > I don't have a sensor which produces RGB555, so it wouldn't be able to
> > test it.
> 
> Understood, but for code readability and consistency I think the code
> can be cleaned up as above.

Yes, i've changed that for v2.

> > > Also what about RGB565 over a 16-bit parallel sensor bus? The
> > > reference manual hints that perhaps this could be treated as
> > > non-passthrough ("on the fly processing"), e.g. could be passed
> > > on to the IC pre-processor:
> > > 
> > >   16 bit RGB565
> > >   This is the only mode that allows on the fly processing of 16 
> > > bit data. In this mode the
> > >   CSI is programmed to receive 16 bit generic data. In this mode 
> > > the interface is
> > >   restricted to be in "non-gated mode" and the CSI#_DATA_SOURCE 
> > > bit has to be set
> > >   If the external device is 24bit - the user can connect a 16 bit 
> > > sample of it (RGB565
> > >   format). The IPU has to be configured in the same way as the 
> > > case of
> > >   CSI#_SENS_DATA_FORMAT=RGB565
> > 
> > I've not looked at this case, as I don't have a sensor with that format
> > either. :/
> 
> Ok, I was just curious if you knew anything more about this. Let's
> ignore it :)

OK. :)

Regards,
Jan
-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [PATCH 0/4] media: ov2680: follow up from initial version

[Fabio Estevam]

Hi Rui,

On Tue, May 8, 2018 at 10:52 AM, Rui Miguel Silva  wrote:

> So, my understand is that the patches will be applied or are already
> applied to someone tree (strange patchwork does not send who or which
> tree), but since I do not want to break someone workflow I will wait for
> some maintainer word on this... If it is better to send the original series
> or the follow up patches.

Ah, didn't know it has been applied. It seems it will soon appear in
linux-next then.

In this case, the follow up series makes sense.

Thanks

Re: [PATCH 0/4] media: ov2680: follow up from initial version

[Rui Miguel Silva]

Ola Fabio,
On Tue 08 May 2018 at 13:29, Fabio Estevam wrote:

Hi Rui,

On Mon, May 7, 2018 at 12:56 PM, Rui Miguel Silva 
 wrote:
Sorry I have Out-of-Office some part of last week, I had v6 of 
the original
series ready but since I have received the notification from 
patchwork that the
v5 was accepted, I am sending this as a follow up tha address 
Fabio comments.


- this adds the power supplies to this sensor
- fix gpio polarity and naming.

Cheers,
   Rui


Rui Miguel Silva (4):
  media: ov2680: dt: add voltage supplies as required
  media: ov2680: dt: rename gpio to reset and fix polarity
  media: ov2680: rename powerdown gpio and fix polarity
  media: ov2680: add regulators to supply control


As the initial ov2680 series has not been applied, I think it 
would be

better if you send a new version with all these fixes.


What I've got was this from patchwork:
Hello,

The following patches (submitted by you) have been updated in 
patchwork:


* linux-media: [v5,1/2] media: ov2680: dt: Add bindings for 
OV2680

- http://patchwork.linuxtv.org/patch/48819/
- for: Linux Media kernel patches
   was: New
   now: Accepted

* linux-media: [v4,1/2] media: ov2680: dt: Add bindings for 
OV2680

- http://patchwork.linuxtv.org/patch/48357/
- for: Linux Media kernel patches
   was: New
   now: Accepted

This email is a notification only - you do not need to respond.

-

Patches submitted to linux-media@vger.kernel.org have the 
following

possible states:



Accepted: when some driver maintainer says that the patch will be 
applied
	  via his tree, or when everything is ok and it got 
	  applied
	  either at the main tree or via some other tree (fixes 
	  tree;
	  some other maintainer's tree - when it belongs to other 
	  subsystems,

  etc);

So, my understand is that the patches will be applied or are 
already
applied to someone tree (strange patchwork does not send who or 
which
tree), but since I do not want to break someone workflow I will 
wait for
some maintainer word on this... If it is better to send the 
original series

or the follow up patches.

Thanks for your feedback.
---
Cheers,
Rui

Re: [PATCH v2 2/2] ARM: dts: r8a7740: Add CEU0

[Simon Horman]

On Thu, Apr 26, 2018 at 08:24:43PM +0200, Jacopo Mondi wrote:
> Describe CEU0 peripheral for Renesas R-Mobile A1 R8A7740 Soc.
> 
> Reported-by: Geert Uytterhoeven 
> Signed-off-by: Jacopo Mondi 

Thanks, applied.

Re: [PATCH 0/4] media: ov2680: follow up from initial version

[Fabio Estevam]

Hi Rui,

On Mon, May 7, 2018 at 12:56 PM, Rui Miguel Silva  wrote:
> Sorry I have Out-of-Office some part of last week, I had v6 of the original
> series ready but since I have received the notification from patchwork that 
> the
> v5 was accepted, I am sending this as a follow up tha address Fabio comments.
>
> - this adds the power supplies to this sensor
> - fix gpio polarity and naming.
>
> Cheers,
>Rui
>
>
> Rui Miguel Silva (4):
>   media: ov2680: dt: add voltage supplies as required
>   media: ov2680: dt: rename gpio to reset and fix polarity
>   media: ov2680: rename powerdown gpio and fix polarity
>   media: ov2680: add regulators to supply control

As the initial ov2680 series has not been applied, I think it would be
better if you send a new version with all these fixes.

Re: [PATCH v3 12/14] ARM: dts: imx7s-warp: add ov2680 sensor node

[Fabio Estevam]

Hi Rui,

On Mon, May 7, 2018 at 1:21 PM, Rui Miguel Silva  wrote:

> +   reg_peri_3p15v: regulator-peri-3p15v {
> +   compatible = "regulator-fixed";
> +   regulator-name = "peri_3p15v_reg";
> +   regulator-min-microvolt = <315>;
> +   regulator-max-microvolt = <315>;
> +   regulator-always-on;

You can remove the 'regulator-always-on' property as this regulator
will be controlled by AVDD-supply.

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Sakari Ailus]

On Tue, May 08, 2018 at 08:04:54AM -0500, Wenwen Wang wrote:
> On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter  
> wrote:
> > On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
> >> At the end of atomisp_subdev_set_selection(), the function
> >> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
> >> this function may return a NULL pointer, it is firstly invoked to check
> >> the returned pointer. If the returned pointer is not NULL, then the
> >> function is invoked again to obtain the pointer and the memory content
> >> at the location of the returned pointer is copied to the memory location of
> >> r. In most cases, the pointers returned by the two invocations are same.
> >> However, given that the pointer returned by the function
> >> atomisp_subdev_get_rect() is not a constant, it is possible that the two
> >> invocations return two different pointers. For example, another thread may
> >> race to modify the related pointers during the two invocations.
> >
> > You're assuming a very serious race condition exists.
> >
> >
> >> In that
> >> case, even if the first returned pointer is not null, the second returned
> >> pointer might be null, which will cause issues such as null pointer
> >> dereference.
> >
> > And then complaining that if a really serious bug exists then this very
> > minor bug would exist too...  If there were really a race condition like
> > that then we'd want to fix it instead.  In other words, this is not a
> > real life bug fix.
> >
> > But it would be fine as a readability or static checker fix so that's
> > fine.
> 
> Thanks for your response. From the performance perspective, this bug
> should also be fixed, as the second invocation is redundant if it is
> expected to return a same pointer as the first one.

The arguments are unchanged so the function returns the same pointer.

Btw. this driver is being removed; please see discussion here:



-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCHv13 03/28] media-request: implement media requests

[Sakari Ailus]

Hi Mauro,

On Tue, May 08, 2018 at 09:41:03AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 8 May 2018 13:52:33 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Mauro, Hans,
> > 
> > On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Fri, 4 May 2018 15:27:50 +0300
> > > Sakari Ailus  escreveu:
> > >   
> > > > Hi Hans,
> > > > 
> > > > I've read this patch a large number of times and I think also the 
> > > > details
> > > > begin to seem sound. A few comments below.  
> > > 
> > > I'm sending this after analyzing the other patches in this series,
> > > as this is the core of the changes. So, although I wrote the comments
> > > early, I wanted to read first all other patches before sending it.
> > >   
> > > > 
> > > > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:  
> > > > > From: Hans Verkuil 
> > > > > 
> > > > > Add initial media request support:
> > > > > 
> > > > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > > > 2) Add struct media_request to store request objects.
> > > > > 3) Add struct media_request_object to represent a request object.
> > > > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > > > 
> > > > > Basic lifecycle: the application allocates a request, adds
> > > > > objects to it, queues the request, polls until it is completed
> > > > > and can then read the final values of the objects at the time
> > > > > of completion. When it closes the file descriptor the request
> > > > > memory will be freed (actually, when the last user of that request
> > > > > releases the request).
> > > > > 
> > > > > Drivers will bind an object to a request (the 'adds objects to it'
> > > > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > > > validated (req_validate op), then queued (the req_queue op).
> > > > > 
> > > > > When done with an object it can either be unbound from the request
> > > > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > > > completed (e.g. for controls associated with a buffer). When all
> > > > > objects in the request are completed (or unbound), then the request
> > > > > fd will signal an exception (poll).
> > > > > 
> > > > > Signed-off-by: Hans Verkuil   
> > > 
> > > Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> > > indicates that part of the stuff you're adding here were authored by
> > > others. So, you should use Co-developed-by: tag here, and get the SOBs
> > > from the other developers that did part of the work[1].
> > > 
> > > [1] except if your work was sponsored by Cisco, Intel and Google, but
> > > I think this is not the case.  
> > 
> > I think this could be appropriate:
> > 
> > Co-developed-by: Laurent Pinchart 
> > 
> > Co-developed-by: Sakari Ailus 
> > Co-developed-by: Alexandre Courbot 
> > 
> > ...
> > 
> > > > > +static long media_request_ioctl_queue(struct media_request *req)
> > > > > +{
> > > > > + struct media_device *mdev = req->mdev;
> > > > > + enum media_request_state state;
> > > > > + unsigned long flags;
> > > > > + int ret = 0;
> > > > 
> > > > ret is unconditionally assigned below, no need to initialise here.
> > > >   
> > > > > +
> > > > > + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > > > +
> > > > > + /*
> > > > > +  * Ensure the request that is validated will be the one that 
> > > > > gets queued
> > > > > +  * next by serialising the queueing process. This mutex is also 
> > > > > used
> > > > > +  * to serialize with canceling a vb2 queue and with setting 
> > > > > values such
> > > > > +  * as controls in a request.
> > > > > +  */
> > > > > + mutex_lock(>req_queue_mutex);
> > > > > +
> > > > > + spin_lock_irqsave(>lock, flags);
> > > > > + state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > > > +MEDIA_REQUEST_STATE_VALIDATING);
> > > > > + spin_unlock_irqrestore(>lock, flags);  
> > > 
> > > It looks weird to serialize access to it with a mutex, a spin lock and 
> > > an atomic call.  
> > 
> > req->lock is needed for state changes from idle to other states as also
> > other struct members need to be serialised with that, with the request
> > state. Which is kind of in line with my earlier point: there's little
> > if any benefit in making this field atomic.
> 
> In this case, only req->state change is serialized - although atomic
> also serializes it.

Correct. But other fields also need to be serialised --- together with the
state change. Therefore relying on atomic only is not an option.

> 
> > The mutex is there to ensure only a single request remains in validating
> > state, i.e. we will be changing the state of the device request tip one
> > request at a time.

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter  wrote:
> On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
>> At the end of atomisp_subdev_set_selection(), the function
>> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
>> this function may return a NULL pointer, it is firstly invoked to check
>> the returned pointer. If the returned pointer is not NULL, then the
>> function is invoked again to obtain the pointer and the memory content
>> at the location of the returned pointer is copied to the memory location of
>> r. In most cases, the pointers returned by the two invocations are same.
>> However, given that the pointer returned by the function
>> atomisp_subdev_get_rect() is not a constant, it is possible that the two
>> invocations return two different pointers. For example, another thread may
>> race to modify the related pointers during the two invocations.
>
> You're assuming a very serious race condition exists.
>
>
>> In that
>> case, even if the first returned pointer is not null, the second returned
>> pointer might be null, which will cause issues such as null pointer
>> dereference.
>
> And then complaining that if a really serious bug exists then this very
> minor bug would exist too...  If there were really a race condition like
> that then we'd want to fix it instead.  In other words, this is not a
> real life bug fix.
>
> But it would be fine as a readability or static checker fix so that's
> fine.

Thanks for your response. From the performance perspective, this bug
should also be fixed, as the second invocation is redundant if it is
expected to return a same pointer as the first one.

Wenwen

Re: [PATCHv13 03/28] media-request: implement media requests

[Mauro Carvalho Chehab]

Em Tue, 8 May 2018 13:52:33 +0300
Sakari Ailus  escreveu:

> Hi Mauro, Hans,
> 
> On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> > Em Fri, 4 May 2018 15:27:50 +0300
> > Sakari Ailus  escreveu:
> >   
> > > Hi Hans,
> > > 
> > > I've read this patch a large number of times and I think also the details
> > > begin to seem sound. A few comments below.  
> > 
> > I'm sending this after analyzing the other patches in this series,
> > as this is the core of the changes. So, although I wrote the comments
> > early, I wanted to read first all other patches before sending it.
> >   
> > > 
> > > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:  
> > > > From: Hans Verkuil 
> > > > 
> > > > Add initial media request support:
> > > > 
> > > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > > 2) Add struct media_request to store request objects.
> > > > 3) Add struct media_request_object to represent a request object.
> > > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > > 
> > > > Basic lifecycle: the application allocates a request, adds
> > > > objects to it, queues the request, polls until it is completed
> > > > and can then read the final values of the objects at the time
> > > > of completion. When it closes the file descriptor the request
> > > > memory will be freed (actually, when the last user of that request
> > > > releases the request).
> > > > 
> > > > Drivers will bind an object to a request (the 'adds objects to it'
> > > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > > validated (req_validate op), then queued (the req_queue op).
> > > > 
> > > > When done with an object it can either be unbound from the request
> > > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > > completed (e.g. for controls associated with a buffer). When all
> > > > objects in the request are completed (or unbound), then the request
> > > > fd will signal an exception (poll).
> > > > 
> > > > Signed-off-by: Hans Verkuil   
> > 
> > Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> > indicates that part of the stuff you're adding here were authored by
> > others. So, you should use Co-developed-by: tag here, and get the SOBs
> > from the other developers that did part of the work[1].
> > 
> > [1] except if your work was sponsored by Cisco, Intel and Google, but
> > I think this is not the case.  
> 
> I think this could be appropriate:
> 
> Co-developed-by: Laurent Pinchart 
> 
> Co-developed-by: Sakari Ailus 
> Co-developed-by: Alexandre Courbot 
> 
> ...
> 
> > > > +static long media_request_ioctl_queue(struct media_request *req)
> > > > +{
> > > > +   struct media_device *mdev = req->mdev;
> > > > +   enum media_request_state state;
> > > > +   unsigned long flags;
> > > > +   int ret = 0;
> > > 
> > > ret is unconditionally assigned below, no need to initialise here.
> > >   
> > > > +
> > > > +   dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > > +
> > > > +   /*
> > > > +* Ensure the request that is validated will be the one that 
> > > > gets queued
> > > > +* next by serialising the queueing process. This mutex is also 
> > > > used
> > > > +* to serialize with canceling a vb2 queue and with setting 
> > > > values such
> > > > +* as controls in a request.
> > > > +*/
> > > > +   mutex_lock(>req_queue_mutex);
> > > > +
> > > > +   spin_lock_irqsave(>lock, flags);
> > > > +   state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > > +  MEDIA_REQUEST_STATE_VALIDATING);
> > > > +   spin_unlock_irqrestore(>lock, flags);  
> > 
> > It looks weird to serialize access to it with a mutex, a spin lock and 
> > an atomic call.  
> 
> req->lock is needed for state changes from idle to other states as also
> other struct members need to be serialised with that, with the request
> state. Which is kind of in line with my earlier point: there's little
> if any benefit in making this field atomic.

In this case, only req->state change is serialized - although atomic
also serializes it.

> The mutex is there to ensure only a single request remains in validating
> state, i.e. we will be changing the state of the device request tip one
> request at a time.

Better to add a comment explaining it.

> > 
> > IMHO, locking is still an issue here. I would love to test the 
> > locks with some tool that would randomize syscalls, issuing close(),
> > poll() and read() at wrong times and inverting the order of some calls, 
> > in order to do some empiric test that all locks are at the right places.
> > 
> > Complex locking schemas like that usually tend to cause a lot of

[PATCH] media: usb: cx231xx-417: include linux/slab.h header

[Anders Roxell]

cx231-417 uses kmalloc/kfree functions, so slab header needs to be
included in order to fix the following build errors:
drivers/media/usb/cx231xx/cx231xx-417.c: In function ‘cx231xx_bulk_copy’:
  CC  drivers/media/platform/qcom/venus/firmware.o
drivers/media/usb/cx231xx/cx231xx-417.c:1389:11: error: implicit declaration of 
function ‘kmalloc’; did you mean ‘vmalloc’? 
[-Werror=implicit-function-declaration]
  buffer = kmalloc(buffer_size, GFP_ATOMIC);
   ^~~
   vmalloc
drivers/media/usb/cx231xx/cx231xx-417.c:1389:9: warning: assignment makes 
pointer from integer without a cast [-Wint-conversion]
  buffer = kmalloc(buffer_size, GFP_ATOMIC);
 ^
drivers/media/usb/cx231xx/cx231xx-417.c:1400:2: error: implicit declaration of 
function ‘kfree’; did you mean ‘vfree’? [-Werror=implicit-function-declaration]
  kfree(buffer);
  ^
  vfree
drivers/media/usb/cx231xx/cx231xx-417.c: In function ‘mpeg_open’:
drivers/media/usb/cx231xx/cx231xx-417.c:1713:7: error: implicit declaration of 
function ‘kzalloc’; did you mean ‘vzalloc’? 
[-Werror=implicit-function-declaration]
  fh = kzalloc(sizeof(*fh), GFP_KERNEL);
   ^~~
   vzalloc
drivers/media/usb/cx231xx/cx231xx-417.c:1713:5: warning: assignment makes 
pointer from integer without a cast [-Wint-conversion]
  fh = kzalloc(sizeof(*fh), GFP_KERNEL);
 ^

Signed-off-by: Anders Roxell 
---
 drivers/media/usb/cx231xx/cx231xx-417.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c 
b/drivers/media/usb/cx231xx/cx231xx-417.c
index b80e6857e2eb..2f3b0564d676 100644
--- a/drivers/media/usb/cx231xx/cx231xx-417.c
+++ b/drivers/media/usb/cx231xx/cx231xx-417.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
-- 
2.17.0

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Dan Carpenter]

On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
> At the end of atomisp_subdev_set_selection(), the function
> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
> this function may return a NULL pointer, it is firstly invoked to check
> the returned pointer. If the returned pointer is not NULL, then the
> function is invoked again to obtain the pointer and the memory content
> at the location of the returned pointer is copied to the memory location of
> r. In most cases, the pointers returned by the two invocations are same.
> However, given that the pointer returned by the function
> atomisp_subdev_get_rect() is not a constant, it is possible that the two
> invocations return two different pointers. For example, another thread may
> race to modify the related pointers during the two invocations.

You're assuming a very serious race condition exists.


> In that
> case, even if the first returned pointer is not null, the second returned
> pointer might be null, which will cause issues such as null pointer
> dereference.

And then complaining that if a really serious bug exists then this very
minor bug would exist too...  If there were really a race condition like
that then we'd want to fix it instead.  In other words, this is not a
real life bug fix.

But it would be fine as a readability or static checker fix so that's
fine.

regards,
dan carpenter

Re: [PATCH v7 1/2] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

One more comment-to-comment:

On Mon, 7 May 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 10 April 2018 14:31:35 EEST Guennadi Liakhovetski wrote:
> > On Fri, 23 Mar 2018, Laurent Pinchart wrote:
> > > On Friday, 23 March 2018 11:24:00 EET Laurent Pinchart wrote:
> > >> From: Guennadi Liakhovetski 
> > >> 
> > >> UVC defines a method of handling asynchronous controls, which sends a
> > >> USB packet over the interrupt pipe. This patch implements support for
> > >> such packets by sending a control event to the user. Since this can
> > >> involve USB traffic and, therefore, scheduling, this has to be done
> > >> in a work queue.
> > >> 
> > >> Signed-off-by: Guennadi Liakhovetski 
> > >> ---
> > >> 
> > >>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++---
> > >>  drivers/media/usb/uvc/uvc_status.c | 111 ++---
> > >>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>  include/uapi/linux/uvcvideo.h  |   2 +
> > >>  5 files changed, 269 insertions(+), 29 deletions(-)

[snip]

> > >> diff --git a/drivers/media/usb/uvc/uvc_status.c
> > >> b/drivers/media/usb/uvc/uvc_status.c index 7b710410584a..d1d83aed6a1d
> > >> 100644
> > >> --- a/drivers/media/usb/uvc/uvc_status.c
> > >> +++ b/drivers/media/usb/uvc/uvc_status.c

[snip]

> > >> +ctrl = uvc_event_entity_ctrl(entity,
> > >> + 
> > >> status->bSelector);
> > >> +/*
> > >> + * Some buggy cameras send asynchronous 
> > >> Control
> > >> + * Change events for control, other 
> > >> than the
> > >> + * ones, that had been changed, even 
> > >> though the
> > >> + * AutoUpdate flag isn't set for the 
> > >> control.
> > >> + */
> > > 
> > > That's lots of commas, I'm not sure what you mean here. Are there cameras
> > > that send event for controls that haven't changed ? Or cameras that send
> > > events for controls that don't have the auto-update flag set ? Do you
> > > know what cameras are affected ?
> > 
> > I meant a case like
> > 
> > set_control(x=X)
> > interrupt(x=X)
> > interrupt(y=Y)
> > 
> > where y is a different control and it doesn't have an auto-update flag
> > set. I think those were some early versions of our cameras, but as far as
> > I can see, we need the check anyway for autoupdate controls, so, I can
> > just remove the comment.
> 
> OK. But now that I read the comment again, how is it related to the check ? 
> Why do you need ctrl->handle->chain == *chain ? Isn't that only a partial 
> guard for the case above, as both x and y could be part of the same chain ?

The uvc_event_entity_ctrl() function checks the selector, so, a selector + 
chain should be unique?

Thanks
Guennadi

Re: [PATCHv13 05/28] media-request: add media_request_object_find

[Sakari Ailus]

On Fri, May 04, 2018 at 03:43:07PM +0300, Sakari Ailus wrote:
> > diff --git a/include/media/media-request.h b/include/media/media-request.h
> > index 997e096d7128..5367b4a2f91c 100644
> > --- a/include/media/media-request.h
> > +++ b/include/media/media-request.h
> > @@ -196,6 +196,22 @@ static inline void media_request_object_get(struct 
> > media_request_object *obj)
> >   */
> >  void media_request_object_put(struct media_request_object *obj);
> >  
> > +/**
> > + * media_request_object_find - Find an object in a request
> > + *
> > + * @ops: Find an object with this ops value
> > + * @priv: Find an object with this priv value
> > + *
> > + * Both @ops and @priv must be non-NULL.
> > + *
> > + * Returns NULL if not found or the object pointer. The caller must
> 
> I'd describe the successful case first. I.e. "Returns the object pointer or
> NULL it not found".

Oops... "Returns the object pointer or NULL if not found".

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCHv13 03/28] media-request: implement media requests

[Sakari Ailus]

Hi Mauro, Hans,

On Tue, May 08, 2018 at 07:21:16AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 4 May 2018 15:27:50 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Hans,
> > 
> > I've read this patch a large number of times and I think also the details
> > begin to seem sound. A few comments below.
> 
> I'm sending this after analyzing the other patches in this series,
> as this is the core of the changes. So, although I wrote the comments
> early, I wanted to read first all other patches before sending it.
> 
> > 
> > On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil 
> > > 
> > > Add initial media request support:
> > > 
> > > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > > 2) Add struct media_request to store request objects.
> > > 3) Add struct media_request_object to represent a request object.
> > > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > > 
> > > Basic lifecycle: the application allocates a request, adds
> > > objects to it, queues the request, polls until it is completed
> > > and can then read the final values of the objects at the time
> > > of completion. When it closes the file descriptor the request
> > > memory will be freed (actually, when the last user of that request
> > > releases the request).
> > > 
> > > Drivers will bind an object to a request (the 'adds objects to it'
> > > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > > validated (req_validate op), then queued (the req_queue op).
> > > 
> > > When done with an object it can either be unbound from the request
> > > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > > completed (e.g. for controls associated with a buffer). When all
> > > objects in the request are completed (or unbound), then the request
> > > fd will signal an exception (poll).
> > > 
> > > Signed-off-by: Hans Verkuil 
> 
> Hmm... As you're adding Copyrights from Intel/Google in this patch, that
> indicates that part of the stuff you're adding here were authored by
> others. So, you should use Co-developed-by: tag here, and get the SOBs
> from the other developers that did part of the work[1].
> 
> [1] except if your work was sponsored by Cisco, Intel and Google, but
> I think this is not the case.

I think this could be appropriate:

Co-developed-by: Laurent Pinchart 

Co-developed-by: Sakari Ailus 
Co-developed-by: Alexandre Courbot 

...

> > > +static long media_request_ioctl_queue(struct media_request *req)
> > > +{
> > > + struct media_device *mdev = req->mdev;
> > > + enum media_request_state state;
> > > + unsigned long flags;
> > > + int ret = 0;  
> > 
> > ret is unconditionally assigned below, no need to initialise here.
> > 
> > > +
> > > + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
> > > +
> > > + /*
> > > +  * Ensure the request that is validated will be the one that gets queued
> > > +  * next by serialising the queueing process. This mutex is also used
> > > +  * to serialize with canceling a vb2 queue and with setting values such
> > > +  * as controls in a request.
> > > +  */
> > > + mutex_lock(>req_queue_mutex);
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + state = atomic_cmpxchg(>state, MEDIA_REQUEST_STATE_IDLE,
> > > +MEDIA_REQUEST_STATE_VALIDATING);
> > > + spin_unlock_irqrestore(>lock, flags);
> 
> It looks weird to serialize access to it with a mutex, a spin lock and 
> an atomic call.

req->lock is needed for state changes from idle to other states as also
other struct members need to be serialised with that, with the request
state. Which is kind of in line with my earlier point: there's little
if any benefit in making this field atomic.

The mutex is there to ensure only a single request remains in validating
state, i.e. we will be changing the state of the device request tip one
request at a time.

> 
> IMHO, locking is still an issue here. I would love to test the 
> locks with some tool that would randomize syscalls, issuing close(),
> poll() and read() at wrong times and inverting the order of some calls, 
> in order to do some empiric test that all locks are at the right places.
> 
> Complex locking schemas like that usually tend to cause a lot of
> troubles.
> 
> > > + if (state != MEDIA_REQUEST_STATE_IDLE) {
> > > + dev_dbg(mdev->dev,
> > > + "request: unable to queue %s, request in state %s\n",
> > > + req->debug_str, media_request_state_str(state));
> > > + mutex_unlock(>req_queue_mutex);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + ret = mdev->ops->req_validate(req);
> > > +
> > > + /*
> > > +  * If the req_validate was successful, then we mark the state as QUEUED
> > > +  * and call req_queue. The reason we set the state first is 

Re: [PATCHv13 12/28] v4l2-ctrls: add core request support

[Mauro Carvalho Chehab]

Em Tue, 8 May 2018 10:07:22 +0200
Hans Verkuil  escreveu:

> >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >> index 76352eb59f14..a0f7c38d1a90 100644
> >> --- a/include/media/v4l2-ctrls.h
> >> +++ b/include/media/v4l2-ctrls.h
> >> @@ -250,6 +250,10 @@ struct v4l2_ctrl {
> >>   *``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
> >>   * @from_other_dev: If true, then @ctrl was defined in another
> >>   *device than the  v4l2_ctrl_handler.
> >> + * @done: If true, then this control reference is part of a
> >> + *control cluster that was already set while applying
> >> + *the controls in this media request object.  
> > 
> > Hmm... I would rename it for request_done or something similar, as it
> > seems that this applies only for requests.  
> 
> No, the variable name is correct (it serves the same purpose as the 'done'
> field in struct v4l2_ctrl), but the description should be improved.
> 
> I also want to take another look at this: I wonder if it isn't possible to
> use the v4l2_ctrl 'done' field for this instead of having to add a new field
> here.

If it can use v4l2_ctrl done, it would indeed be better. Otherwise, as
this new "done" is used only by requests, I really think that it shold
be renamed, to let it clearer that this is the "done" that should be used
when requests are used.

Thanks,
Mauro

[GIT PULL FOR v4.18] v2: Various fixes/improvements

[Hans Verkuil]

Fixes/improvements all over the place.

Changes since v1:

Replaced "media: media-device: fix ioctl function types" with the v2 version
of that patch. My fault, I missed Sakari's request for a change of v1.

Regards,

Hans

The following changes since commit f10379aad39e9da8bc7d1822e251b5f0673067ef:

  media: include/video/omapfb_dss.h: use IS_ENABLED() (2018-05-05 11:45:51 
-0400)

are available in the Git repository at:

  git://linuxtv.org/hverkuil/media_tree.git for-v4.18b

for you to fetch changes up to 171d364998d1e2373c12b93924fe63cc71586101:

  media: media-device: fix ioctl function types (2018-05-08 12:40:23 +0200)


Arvind Yadav (2):
  platform: Use gpio_is_valid()
  sta2x11: Use gpio_is_valid() and remove unnecessary check

Brad Love (4):
  em28xx: Fix DualHD broken second tuner
  intel-ipu3: Kconfig coding style issue
  cec: Kconfig coding style issue
  saa7164: Fix driver name in debug output

Colin Ian King (1):
  media/usbvision: fix spelling mistake: "compresion" -> "compression"

Dan Carpenter (1):
  media: vpbe_venc: potential uninitialized variable in ven_sub_dev_init()

Fengguang Wu (1):
  media: vcodec: fix ptr_ret.cocci warnings

Hans Verkuil (2):
  cec-gpio: use GPIOD_OUT_HIGH_OPEN_DRAIN
  v4l2-dev.h: fix doc warning

Jacopo Mondi (1):
  media: renesas-ceu: Set mbus_fmt on subdev operations

Jan Luebbe (1):
  media: imx-csi: fix burst size for 16 bit

Jasmin Jessich (2):
  media: Use ktime_set() in pt1.c
  media: Revert cleanup ktime_set() usage

Julia Lawall (1):
  pvrusb2: delete unneeded include

Niklas Söderlund (1):
  media: entity: fix spelling for media_entity_get_fwnode_pad()

Philipp Zabel (4):
  media: coda: reuse coda_s_fmt_vid_cap to propagate format in 
coda_s_fmt_vid_out
  media: coda: do not try to propagate format if capture queue busy
  media: coda: set colorimetry on coded queue
  media: imx: add 16-bit grayscale support

Robin Murphy (1):
  media: videobuf-dma-sg: Fix dma_{sync,unmap}_sg() calls

Sami Tolvanen (1):
  media: media-device: fix ioctl function types

Simon Que (1):
  v4l2-core: Rename array 'video_driver' to 'video_drivers'

Souptick Joarder (1):
  videobuf: Change return type to vm_fault_t

Wolfram Sang (1):
  media: platform: am437x: simplify getting .drvdata

 drivers/media/Kconfig   | 12 ++--
 drivers/media/dvb-core/dmxdev.c |  2 +-
 drivers/media/media-device.c| 21 +++--
 drivers/media/pci/cx88/cx88-input.c |  6 --
 drivers/media/pci/intel/ipu3/Kconfig| 12 ++--
 drivers/media/pci/pt1/pt1.c |  2 +-
 drivers/media/pci/pt3/pt3.c |  2 +-
 drivers/media/pci/saa7164/saa7164-fw.c  |  3 ++-
 drivers/media/pci/sta2x11/sta2x11_vip.c | 31 
+++
 drivers/media/platform/am437x/am437x-vpfe.c |  6 ++
 drivers/media/platform/cec-gpio/cec-gpio.c  |  2 +-
 drivers/media/platform/coda/coda-common.c   | 45 
+++--
 drivers/media/platform/davinci/vpbe_venc.c  |  2 +-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c |  5 +
 drivers/media/platform/renesas-ceu.c| 20 +++-
 drivers/media/platform/via-camera.c |  2 +-
 drivers/media/usb/em28xx/em28xx-dvb.c   |  2 +-
 drivers/media/usb/pvrusb2/pvrusb2-cx2584x-v4l.c |  1 -
 drivers/media/usb/usbvision/usbvision-core.c|  2 +-
 drivers/media/v4l2-core/v4l2-dev.c  | 22 +++---
 drivers/media/v4l2-core/videobuf-dma-sg.c   |  6 +++---
 drivers/staging/media/imx/imx-media-csi.c   |  3 ++-
 drivers/staging/media/imx/imx-media-utils.c |  9 +
 include/media/media-entity.h|  2 +-
 include/media/v4l2-dev.h|  1 +
 25 files changed, 128 insertions(+), 93 deletions(-)

Re: [PATCHv13 06/28] v4l2-dev: lock req_queue_mutex

[Mauro Carvalho Chehab]

Em Tue, 8 May 2018 09:45:27 +0200
Hans Verkuil  escreveu:

> On 05/07/2018 07:20 PM, Mauro Carvalho Chehab wrote:
> > Em Thu,  3 May 2018 16:52:56 +0200
> > Hans Verkuil  escreveu:
> >   
> >> From: Hans Verkuil 
> >>
> >> We need to serialize streamon/off with queueing new requests.
> >> These ioctls may trigger the cancellation of a streaming
> >> operation, and that should not be mixed with queuing a new
> >> request at the same time.
> >>
> >> Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
> >> with MEDIA_REQUEST_IOC_QUEUE.
> >>
> >> Finally close() needs this lock since that too can trigger the
> >> cancellation of a streaming operation.
> >>
> >> We take the req_queue_mutex here before any other locks since
> >> it is a very high-level lock.
> >>
> >> Signed-off-by: Hans Verkuil 
> >> ---
> >>  drivers/media/v4l2-core/v4l2-dev.c | 37 +-
> >>  1 file changed, 36 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> >> b/drivers/media/v4l2-core/v4l2-dev.c
> >> index 1d0b2208e8fb..b1c9efc0ecc4 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -353,13 +353,36 @@ static long v4l2_ioctl(struct file *filp, unsigned 
> >> int cmd, unsigned long arg)
> >>  
> >>if (vdev->fops->unlocked_ioctl) {
> >>struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> >> +  struct mutex *queue_lock = NULL;
> >>  
> >> -  if (lock && mutex_lock_interruptible(lock))
> >> +  /*
> >> +   * We need to serialize streamon/off with queueing new requests.
> >> +   * These ioctls may trigger the cancellation of a streaming
> >> +   * operation, and that should not be mixed with queueing a new
> >> +   * request at the same time.
> >> +   *
> >> +   * Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
> >> +   * with MEDIA_REQUEST_IOC_QUEUE.
> >> +   */
> >> +  if (vdev->v4l2_dev->mdev &&
> >> +  (cmd == VIDIOC_STREAMON || cmd == VIDIOC_STREAMOFF ||
> >> +   cmd == VIDIOC_S_EXT_CTRLS || cmd == VIDIOC_TRY_EXT_CTRLS))
> >> +  queue_lock = >v4l2_dev->mdev->req_queue_mutex;
> >> +
> >> +  if (queue_lock && mutex_lock_interruptible(queue_lock))
> >> +  return -ERESTARTSYS;  
> > 
> > Taking both locks seems risky. Here you're taking first v4l2 lock, returned
> > by v4l2_ioctl_get_lock(vdev, cmd), and then you're taking the req_queue 
> > lock.  
> 
> No,  v4l2_ioctl_get_lock() only returns a pointer to a mutex, it doesn't lock
> anything. I think you got confused there. I'll reorganize the code a bit so
> the call to  v4l2_ioctl_get_lock() happens after the queue_lock has been 
> taken.

Yeah, I didn't actually look at the implementation of v4l2_ioctl_get_lock().

As we're using "_get" along this patch series to increment krefs (with is
a sort of locking), the name here confused me. IMHO, we should rename it
to v4l2_ioctl_return_lock() (or similar) on some future, in order to avoid
confusion.

> I'll also rename queue_lock to req_queue_lock (it's a bit more descriptive).

Agreed.

> 
> So we first take the high-level media_device req_queue_mutex if needed, and
> then the ioctl serialization lock. Doing it the other way around will indeed
> promptly deadlock (as I very quickly discovered after my initial 
> implementation!).
> 
> So the order is:
> 
>   req_queue_mutex (serialize request state changes from/to IDLE)
>   ioctl lock (serialize ioctls)
>   request->lock (spinlock)
> 
> The last is only held for short periods when updating the media_request 
> struct.
> 
> > 
> > It is possible to call parts of the code that only handles req_queue
> > or v4l2 lock (for example, by mixing request API calls with non-requests
> > one). Worse than that, there are parts of the code where the request API
> > patches get both a mutex and a spin lock.
> > 
> > I didn't look too closely (nor ran any test), but I'm almost sure that
> > there are paths where it will end by leading into dead locks.  
> 
> I've done extensive testing with this and actually been very careful about
> the lock handling. It's also been tested with the cedrus driver.

I don't doubt it works using your apps, but real life can be messier:
people could be issuing ioctls at different orders, programs can abort
any time, closing file descriptors at random times, threads can be
used to paralelize ioctls, etc.

That not discarding the possibility of someone coming with some
ingenious code meant to cause machine hangups or to exposure some
other security flaws.



Thanks,
Mauro

Re: [PATCH][media-next] media: ddbridge: avoid out-of-bounds write on array demod_in_use

[Colin Ian King]

On 08/05/18 11:38, Daniel Scheller wrote:
> Hi Colin,
> 
> Am Tue,  8 May 2018 00:08:42 +0100
> schrieb Colin King :
> 
>> From: Colin Ian King 
>>
>> In function stop there is a check to see if state->demod is a stopped
>> value of 0xff, however, later on, array demod_in_use is indexed with
>> this value causing an out-of-bounds write error.  Avoid this by only
>> writing to array demod_in_use if state->demod is not set to the stopped
>> sentinal value for this specific corner case.  Also, replace the magic
>> value 0xff with DEMOD_STOPPED to make code more readable.
>>
>> Detected by CoverityScan, CID#1468550 ("Out-of-bounds write")
>>
>> Fixes: daeeb1319e6f ("media: ddbridge: initial support for MCI-based MaxSX8 
>> cards")
>> Signed-off-by: Colin Ian King 
>> ---
>>  drivers/media/pci/ddbridge/ddbridge-mci.c | 11 +++
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/pci/ddbridge/ddbridge-mci.c 
>> b/drivers/media/pci/ddbridge/ddbridge-mci.c
>> index a85ff3e6b919..1f5ed53c8d35 100644
>> --- a/drivers/media/pci/ddbridge/ddbridge-mci.c
>> +++ b/drivers/media/pci/ddbridge/ddbridge-mci.c
>> @@ -20,6 +20,8 @@
>>  #include "ddbridge-io.h"
>>  #include "ddbridge-mci.h"
>>  
>> +#define DEMOD_STOPPED   (0xff)
>> +
>>  static LIST_HEAD(mci_list);
>>  
>>  static const u32 MCLK = (155000 / 12);
>> @@ -193,7 +195,7 @@ static int stop(struct dvb_frontend *fe)
>>  u32 input = state->tuner;
>>  
>>  memset(, 0, sizeof(cmd));
>> -if (state->demod != 0xff) {
>> +if (state->demod != DEMOD_STOPPED) {
>>  cmd.command = MCI_CMD_STOP;
>>  cmd.demod = state->demod;
>>  mci_cmd(state, , NULL);
>> @@ -209,10 +211,11 @@ static int stop(struct dvb_frontend *fe)
>>  state->base->tuner_use_count[input]--;
>>  if (!state->base->tuner_use_count[input])
>>  mci_set_tuner(fe, input, 0);
>> -state->base->demod_in_use[state->demod] = 0;
>> +if (state->demod != DEMOD_STOPPED)
>> +state->base->demod_in_use[state->demod] = 0;
>>  state->base->used_ldpc_bitrate[state->nr] = 0;
>> -state->demod = 0xff;
>> -state->base->assigned_demod[state->nr] = 0xff;
>> +state->demod = DEMOD_STOPPED;
>> +state->base->assigned_demod[state->nr] = DEMOD_STOPPED;
>>  state->base->iq_mode = 0;
>>  mutex_unlock(>base->tuner_lock);
>>  state->started = 0;
> 
> Thanks for the patch, or - better - pointing this out. While it's
> unlikely this will ever be an issue, I'm fine with changing the code
> like that, but I'd prefer to change it a bit differently (ie.
> DEMOD_STOPPED should be DEMOD_UNUSED, and I'd add defines for max.
> tuners and use/compare against them).

Sounds like a good idea.

> 
> I'll send out a different patch that will cover the potential
> coverityscan problem throughout the end of the week.

Great. Thanks!

> 
> Best regards,
> Daniel Scheller
> 

Re: [PATCH][media-next] media: ddbridge: avoid out-of-bounds write on array demod_in_use

[Daniel Scheller]

Hi Colin,

Am Tue,  8 May 2018 00:08:42 +0100
schrieb Colin King :

> From: Colin Ian King 
> 
> In function stop there is a check to see if state->demod is a stopped
> value of 0xff, however, later on, array demod_in_use is indexed with
> this value causing an out-of-bounds write error.  Avoid this by only
> writing to array demod_in_use if state->demod is not set to the stopped
> sentinal value for this specific corner case.  Also, replace the magic
> value 0xff with DEMOD_STOPPED to make code more readable.
> 
> Detected by CoverityScan, CID#1468550 ("Out-of-bounds write")
> 
> Fixes: daeeb1319e6f ("media: ddbridge: initial support for MCI-based MaxSX8 
> cards")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/media/pci/ddbridge/ddbridge-mci.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/pci/ddbridge/ddbridge-mci.c 
> b/drivers/media/pci/ddbridge/ddbridge-mci.c
> index a85ff3e6b919..1f5ed53c8d35 100644
> --- a/drivers/media/pci/ddbridge/ddbridge-mci.c
> +++ b/drivers/media/pci/ddbridge/ddbridge-mci.c
> @@ -20,6 +20,8 @@
>  #include "ddbridge-io.h"
>  #include "ddbridge-mci.h"
>  
> +#define DEMOD_STOPPED(0xff)
> +
>  static LIST_HEAD(mci_list);
>  
>  static const u32 MCLK = (155000 / 12);
> @@ -193,7 +195,7 @@ static int stop(struct dvb_frontend *fe)
>   u32 input = state->tuner;
>  
>   memset(, 0, sizeof(cmd));
> - if (state->demod != 0xff) {
> + if (state->demod != DEMOD_STOPPED) {
>   cmd.command = MCI_CMD_STOP;
>   cmd.demod = state->demod;
>   mci_cmd(state, , NULL);
> @@ -209,10 +211,11 @@ static int stop(struct dvb_frontend *fe)
>   state->base->tuner_use_count[input]--;
>   if (!state->base->tuner_use_count[input])
>   mci_set_tuner(fe, input, 0);
> - state->base->demod_in_use[state->demod] = 0;
> + if (state->demod != DEMOD_STOPPED)
> + state->base->demod_in_use[state->demod] = 0;
>   state->base->used_ldpc_bitrate[state->nr] = 0;
> - state->demod = 0xff;
> - state->base->assigned_demod[state->nr] = 0xff;
> + state->demod = DEMOD_STOPPED;
> + state->base->assigned_demod[state->nr] = DEMOD_STOPPED;
>   state->base->iq_mode = 0;
>   mutex_unlock(>base->tuner_lock);
>   state->started = 0;

Thanks for the patch, or - better - pointing this out. While it's
unlikely this will ever be an issue, I'm fine with changing the code
like that, but I'd prefer to change it a bit differently (ie.
DEMOD_STOPPED should be DEMOD_UNUSED, and I'd add defines for max.
tuners and use/compare against them).

I'll send out a different patch that will cover the potential
coverityscan problem throughout the end of the week.

Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd

[Mauro Carvalho Chehab]

Em Tue, 8 May 2018 09:34:11 +0200
Hans Verkuil  escreveu:

> On 05/07/2018 07:01 PM, Mauro Carvalho Chehab wrote:
> > Em Thu,  3 May 2018 16:52:54 +0200
> > Hans Verkuil  escreveu:
> >   
> >> From: Hans Verkuil 
> >>
> >> Add media_request_get_by_fd() to find a request based on the file
> >> descriptor.
> >>
> >> The caller has to call media_request_put() for the returned
> >> request since this function increments the refcount.
> >>
> >> Signed-off-by: Hans Verkuil 
> >> ---
> >>  drivers/media/media-request.c | 32 
> >>  include/media/media-request.h | 24 
> >>  2 files changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> >> index c216c4ab628b..edc1c3af1959 100644
> >> --- a/drivers/media/media-request.c
> >> +++ b/drivers/media/media-request.c
> >> @@ -218,6 +218,38 @@ static const struct file_operations request_fops = {
> >>.release = media_request_close,
> >>  };
> >>  
> >> +struct media_request *
> >> +media_request_get_by_fd(struct media_device *mdev, int request_fd)
> >> +{
> >> +  struct file *filp;
> >> +  struct media_request *req;
> >> +
> >> +  if (!mdev || !mdev->ops ||
> >> +  !mdev->ops->req_validate || !mdev->ops->req_queue)
> >> +  return ERR_PTR(-EPERM);
> >> +
> >> +  filp = fget(request_fd);
> >> +  if (!filp)
> >> +  return ERR_PTR(-ENOENT);
> >> +
> >> +  if (filp->f_op != _fops)
> >> +  goto err_fput;
> >> +  req = filp->private_data;
> >> +  if (req->mdev != mdev)
> >> +  goto err_fput;
> >> +
> >> +  media_request_get(req);  
> > 
> > Hmm... this function changes the req struct (by calling kref_get) without
> > holding neither the mutex or the spin lock...  
> 
> kref_get is atomic, so why would you need to add additional locking?
> 
> Neither can the request be 'put' by another process before media_request_get()
> is called due to the fget() above: while the fd has a refcount > 0 the
> request refcount is also > 0. I'll add some comments to clarify this.

I see. Yeah, please add a comment here.

Thanks,
Mauro





> 
> >   
> >> +  fput(filp);
> >> +
> >> +  return req;
> >> +
> >> +err_fput:
> >> +  fput(filp);
> >> +
> >> +  return ERR_PTR(-ENOENT);
> >> +}
> >> +EXPORT_SYMBOL_GPL(media_request_get_by_fd);
> >> +
> >>  int media_request_alloc(struct media_device *mdev,
> >>struct media_request_alloc *alloc)
> >>  {
> >> diff --git a/include/media/media-request.h b/include/media/media-request.h
> >> index e39122dfd717..997e096d7128 100644
> >> --- a/include/media/media-request.h
> >> +++ b/include/media/media-request.h
> >> @@ -86,6 +86,24 @@ static inline void media_request_get(struct 
> >> media_request *req)
> >>   */
> >>  void media_request_put(struct media_request *req);
> >>  
> >> +/**
> >> + * media_request_get_by_fd - Get a media request by fd
> >> + *
> >> + * @mdev: Media device this request belongs to
> >> + * @request_fd: The file descriptor of the request
> >> + *
> >> + * Get the request represented by @request_fd that is owned
> >> + * by the media device.
> >> + *
> >> + * Return a -EPERM error pointer if requests are not supported
> >> + * by this driver. Return -ENOENT if the request was not found.
> >> + * Return the pointer to the request if found: the caller will
> >> + * have to call @media_request_put when it finished using the
> >> + * request.  
> > 
> > ... so, it should be said here how this should be serialized, in order
> > to avoid it to be destroyed by a task while some other task might be
> > trying to instantiate it.  
> 
> This doesn't need any serialization. If the request_fd is found, then
> it will return the request with the request refcount increased.
> 
> I also do not understand what you mean with "some other task might be
> trying to instantiate it".
> 
> I think there is some misunderstanding here.
> 
> Regards,
> 
>   Hans
> 
> 
> >   
> >> + */
> >> +struct media_request *
> >> +media_request_get_by_fd(struct media_device *mdev, int request_fd);
> >> +
> >>  /**
> >>   * media_request_alloc - Allocate the media request
> >>   *
> >> @@ -107,6 +125,12 @@ static inline void media_request_put(struct 
> >> media_request *req)
> >>  {
> >>  }
> >>  
> >> +static inline struct media_request *
> >> +media_request_get_by_fd(struct media_device *mdev, int request_fd)
> >> +{
> >> +  return ERR_PTR(-EPERM);
> >> +}
> >> +
> >>  #endif
> >>  
> >>  /**  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: 4.17-rc3 regression in UVC driver

[Kieran Bingham]

On 05/05/18 11:32, Mauro Carvalho Chehab wrote:
> Hi Kieran,



>> I manually pull from  git://linuxtv.org/pinchartl/media.git uvc/fixes
>> and picked the patch.
> 
>> It should be at media_tree.git at the fixes branch.

Thank you Mauro - that's great!

--
Kieran

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

Hi Steve,

Again, sorry about the delay. This thread got buried in my inbox. :-(
Please see my reply below.

On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> > > for parsing a sub-device's fwnode port endpoints for connected remote
> > > sub-devices, registering a sub-device notifier, and then registering
> > > the sub-device itself.
> > > 
> > > Signed-off-by: Steve Longerbeam 
> > > ---
> > > Changes since v2:
> > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
> > >to put device.
> > > Changes since v1:
> > > - add #include  to v4l2-fwnode.h for
> > >'struct v4l2_subdev' declaration.
> > > ---
> > >   drivers/media/v4l2-core/v4l2-fwnode.c | 101 
> > > ++
> > >   include/media/v4l2-fwnode.h   |  43 +++
> > >   2 files changed, 144 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 99198b9..d42024d 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct 
> > > v4l2_subdev *sd)
> > >   }
> > >   EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> > > +int v4l2_async_register_fwnode_subdev(
> > > + struct v4l2_subdev *sd, size_t asd_struct_size,
> > > + unsigned int *ports, unsigned int num_ports,
> > > + int (*parse_endpoint)(struct device *dev,
> > > +   struct v4l2_fwnode_endpoint *vep,
> > > +   struct v4l2_async_subdev *asd))
> > > +{
> > > + struct v4l2_async_notifier *notifier;
> > > + struct device *dev = sd->dev;
> > > + struct fwnode_handle *fwnode;
> > > + unsigned int subdev_port;
> > > + bool is_port;
> > > + int ret;
> > > +
> > > + if (WARN_ON(!dev))
> > > + return -ENODEV;
> > > +
> > > + fwnode = dev_fwnode(dev);
> > > + if (!fwnode_device_is_available(fwnode))
> > > + return -ENODEV;
> > > +
> > > + is_port = (is_of_node(fwnode) &&
> > > +of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> > What's the intent of this and the code below? You may not parse the graph
> > data structure here, it should be done in the actual firmware
> > implementation instead.
> 
> The i.MX6 CSI sub-device registers itself from a port fwnode, so
> the intent of the is_port code below is to support the i.MX6 CSI.
> 
> I can remove the is_port checks, but it means
> v4l2_async_register_fwnode_subdev() won't be usable by the CSI
> sub-device.

This won't scale. Instead, I think we'd need to separate registering
sub-devices (through async sub-devices) and binding them with the driver
that registered the notifier. Or at least change how that process works: a
single sub-device can well be bound to multiple notifiers, or multiple
times to the same notifier while it may be registered only once.

> 
> > 
> > Also, sub-devices generally do not match ports.
> 
> Yes that's generally true, sub-devices generally match to port parent
> nodes. But I do know of one other sub-device that buck that trend.
> The ADV748x CSI-2 output sub-devices match against endpoint nodes.

Endpoints, yes, but not ports.

> 
> >   How sub-devices generally
> > correspond to fwnodes is up to the device.
> 
> What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
> and a v4l2_async_register_endpoint_fwnode_subdev() to support such
> sub-devices?

The endpoint is more specific than a port, so why the port and not the
endpoint?

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCHv13 00/28] Request API

[Mauro Carvalho Chehab]

Em Thu,  3 May 2018 16:52:50 +0200
Hans Verkuil  escreveu:

> From: Hans Verkuil 

> Regarding locking:
> 
> There are two request locks used: req_queue_mutex (part of media_device)
> ensures high-level serialization of queueing/reiniting and canceling
> requests. It serializes STREAMON/OFF, TRY/S_EXT_CTRLS, close() and
> MEDIA_REQUEST_IOC_QUEUE. This is the top-level lock and should be taken
> before any others.

Why it doesn't serialize poll()? 

> The lock spin_lock in struct media_request protects that structure and
> should be held for a short time only.
> 
> Note that VIDIOC_QBUF does not take this mutex: when
> a buffer is queued for a request it will add it to the
> request by calling media_request_object_bind(), and that returns an
> error if the request is in the wrong state. It is serialized via the
> spin_lock which in this specific case is sufficient.

It looks weird that some syscalls are not serialized. By not having
poll() and VIDIOC_QBUF serialized, I'm wandering if playing with
them and close() on separate threads won't cause bad locking. 

Thanks,
Mauro

Re: [PATCHv13 03/28] media-request: implement media requests

[Mauro Carvalho Chehab]

Em Fri, 4 May 2018 15:27:50 +0300
Sakari Ailus  escreveu:

> Hi Hans,
> 
> I've read this patch a large number of times and I think also the details
> begin to seem sound. A few comments below.

I'm sending this after analyzing the other patches in this series,
as this is the core of the changes. So, although I wrote the comments
early, I wanted to read first all other patches before sending it.

> 
> On Thu, May 03, 2018 at 04:52:53PM +0200, Hans Verkuil wrote:
> > From: Hans Verkuil 
> > 
> > Add initial media request support:
> > 
> > 1) Add MEDIA_IOC_REQUEST_ALLOC ioctl support to media-device.c
> > 2) Add struct media_request to store request objects.
> > 3) Add struct media_request_object to represent a request object.
> > 4) Add MEDIA_REQUEST_IOC_QUEUE/REINIT ioctl support.
> > 
> > Basic lifecycle: the application allocates a request, adds
> > objects to it, queues the request, polls until it is completed
> > and can then read the final values of the objects at the time
> > of completion. When it closes the file descriptor the request
> > memory will be freed (actually, when the last user of that request
> > releases the request).
> > 
> > Drivers will bind an object to a request (the 'adds objects to it'
> > phase), when MEDIA_REQUEST_IOC_QUEUE is called the request is
> > validated (req_validate op), then queued (the req_queue op).
> > 
> > When done with an object it can either be unbound from the request
> > (e.g. when the driver has finished with a vb2 buffer) or marked as
> > completed (e.g. for controls associated with a buffer). When all
> > objects in the request are completed (or unbound), then the request
> > fd will signal an exception (poll).
> > 
> > Signed-off-by: Hans Verkuil 

Hmm... As you're adding Copyrights from Intel/Google in this patch, that
indicates that part of the stuff you're adding here were authored by
others. So, you should use Co-developed-by: tag here, and get the SOBs
from the other developers that did part of the work[1].

[1] except if your work was sponsored by Cisco, Intel and Google, but
I think this is not the case.

> > ---
> >  drivers/media/Makefile|   3 +-
> >  drivers/media/media-device.c  |  14 ++
> >  drivers/media/media-request.c | 407 ++
> >  include/media/media-device.h  |  16 ++
> >  include/media/media-request.h | 244 
> >  5 files changed, 683 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/media/media-request.c
> >  create mode 100644 include/media/media-request.h
> > 
> > diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> > index 594b462ddf0e..985d35ec6b29 100644
> > --- a/drivers/media/Makefile
> > +++ b/drivers/media/Makefile
> > @@ -3,7 +3,8 @@
> >  # Makefile for the kernel multimedia device drivers.
> >  #
> >  
> > -media-objs := media-device.o media-devnode.o media-entity.o
> > +media-objs := media-device.o media-devnode.o media-entity.o \
> > +  media-request.o
> >  
> >  #
> >  # I2C drivers should come before other drivers, otherwise they'll fail
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 35e81f7c0d2f..bb6a64acd3f0 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -32,6 +32,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >  
> > @@ -366,6 +367,15 @@ static long media_device_get_topology(struct 
> > media_device *mdev,
> > return ret;
> >  }
> >  
> > +static long media_device_request_alloc(struct media_device *mdev,
> > +  struct media_request_alloc *alloc)
> > +{
> > +   if (!mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue)
> > +   return -ENOTTY;
> > +
> > +   return media_request_alloc(mdev, alloc);
> > +}
> > +
> >  static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int 
> > cmd)
> >  {
> > /* All media IOCTLs are _IOWR() */
> > @@ -414,6 +424,7 @@ static const struct media_ioctl_info ioctl_info[] = {
> > MEDIA_IOC(ENUM_LINKS, media_device_enum_links, 
> > MEDIA_IOC_FL_GRAPH_MUTEX),
> > MEDIA_IOC(SETUP_LINK, media_device_setup_link, 
> > MEDIA_IOC_FL_GRAPH_MUTEX),
> > MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, 
> > MEDIA_IOC_FL_GRAPH_MUTEX),
> > +   MEDIA_IOC(REQUEST_ALLOC, media_device_request_alloc, 0),
> >  };
> >  
> >  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> > @@ -686,6 +697,8 @@ void media_device_init(struct media_device *mdev)
> > INIT_LIST_HEAD(>pads);
> > INIT_LIST_HEAD(>links);
> > INIT_LIST_HEAD(>entity_notify);
> > +
> > +   mutex_init(>req_queue_mutex);
> > mutex_init(>graph_mutex);
> > ida_init(>entity_internal_idx);
> >  
> > @@ -699,6 +712,7 @@ void media_device_cleanup(struct media_device *mdev)
> > mdev->entity_internal_idx_max = 0;
> > 

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> Hi Sakari,
> 
> 
> On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Thanks for the patchset.
> > 
> > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> > > that the asd's match_type is valid and that no other equivalent asd's
> > > have already been added to this notifier's asd list, or to other
> > > registered notifier's waiting or done lists, and increments num_subdevs.
> > > 
> > > v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> > > array, otherwise it would have to re-allocate the array every time the
> > > function was called. In place of the subdevs array, the function adds
> > > the asd to a new master asd_list. The function will return error with a
> > > WARN() if it is ever called with the subdevs array allocated.
> > > 
> > > In v4l2_async_notifier_has_async_subdev(), 
> > > __v4l2_async_notifier_register(),
> > > and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> > > array or a non-empty notifier->asd_list.
> > I do agree with the approach, but this patch leaves the remaining users of
> > the subdevs array in the notifier intact. Could you rework them to use the
> > v4l2_async_notifier_add_subdev() as well?
> > 
> > There seem to be just a few of them --- exynos4-is and rcar_drif.
> 
> I count more than a few :)
> 
> % cd drivers/media && grep -l -r --include "*.[ch]"
> 'notifier[\.\-]>*subdevs[   ]*='
> v4l2-core/v4l2-async.c
> platform/pxa_camera.c
> platform/ti-vpe/cal.c
> platform/exynos4-is/media-dev.c
> platform/qcom/camss-8x16/camss.c
> platform/soc_camera/soc_camera.c
> platform/atmel/atmel-isi.c
> platform/atmel/atmel-isc.c
> platform/stm32/stm32-dcmi.c
> platform/davinci/vpif_capture.c
> platform/davinci/vpif_display.c
> platform/renesas-ceu.c
> platform/am437x/am437x-vpfe.c
> platform/xilinx/xilinx-vipp.c
> platform/rcar_drif.c
> 
> 
> So not including v4l2-core, the list is:
> 
> pxa_camera
> ti-vpe
> exynos4-is
> qcom
> soc_camera
> atmel
> stm32
> davinci
> renesas-ceu
> am437x
> xilinx
> rcar_drif
> 
> 
> Given such a large list of the users of the notifier->subdevs array,
> I think this should be done is two steps: submit this patchset first,
> that keeps the backward compatibility, and then a subsequent patchset
> that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> which point the subdevs array can be removed from struct
> v4l2_async_notifier.
> 
> Or, do you still think this should be done all at once? I could add a
> large patch to this patchset that does the conversion and removes
> the subdevs array.

Sorry for the delay. I grepped for this, too, but I guess I ended up using
an expression that missed most of the users. :-(

I think it'd be very good to perform that conversion --- the code in the
framework would be quite a bit cleaner and easier to maintain whereas the
per-driver conversions seem pretty simple, such as this on in
drivers/media/platform/atmel/atmel-isi.c :

/* Register the subdevices notifier. */
subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
if (!subdevs) {
of_node_put(isi->entity.node);
return -ENOMEM;
}

subdevs[0] = >entity.asd;

isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
isi->notifier.ops = _graph_notify_ops;

> 
> Steve
> 
> 
> > 
> > > Signed-off-by: Steve Longerbeam 
> > > ---
> > > Changes since v2:
> > > - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> > > Changes since v1:
> > > - none
> > > ---
> > >   drivers/media/v4l2-core/v4l2-async.c | 206 
> > > +++
> > >   include/media/v4l2-async.h   |  22 
> > >   2 files changed, 184 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > > b/drivers/media/v4l2-core/v4l2-async.c
> > > index b59bbac..7b7f7e2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
> > >   struct v4l2_async_notifier *notifier, struct v4l2_async_subdev 
> > > *asd,
> > >   unsigned int this_index)
> > >   {
> > > + struct v4l2_async_subdev *asd_y;
> > >   unsigned int j;
> > >   lockdep_assert_held(_lock);
> > >   /* Check that an asd is not being added more than once. */
> > > - for (j = 0; j < this_index; j++) {
> > > - struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
> > > -
> > > - if (asd_equal(asd, asd_y))
> > > - return true;
> > > + if (notifier->subdevs) {
> > > + for (j = 0; j < this_index; j++) {
> > > + asd_y = notifier->subdevs[j];
> > > + if 

Re: [PATCHv13 16/28] videobuf2-core: embed media_request_object

[Mauro Carvalho Chehab]

Em Thu,  3 May 2018 16:53:06 +0200
Hans Verkuil  escreveu:

> From: Hans Verkuil 
> 
> Make vb2_buffer a request object.
> 
> Signed-off-by: Hans Verkuil 
> ---
>  include/media/videobuf2-core.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 224c4820a044..3d54654c3cd4 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define VB2_MAX_FRAME(32)
>  #define VB2_MAX_PLANES   (8)
> @@ -238,6 +239,7 @@ struct vb2_queue;
>   * @num_planes:  number of planes in the buffer
>   *   on an internal driver queue.
>   * @timestamp:   frame timestamp in ns.
> + * @req_obj: used to bind this buffer to a request
>   */
>  struct vb2_buffer {
>   struct vb2_queue*vb2_queue;
> @@ -246,6 +248,7 @@ struct vb2_buffer {
>   unsigned intmemory;
>   unsigned intnum_planes;
>   u64 timestamp;
> + struct media_request_object req_obj;
>  
>   /* private: internal use only
>*

Hmm... this has a side effect of embedding a kref at struct vb2_buffer.
One struct can have just one kref.

I guess this is likely ok, but this is a big struct. I don't like
the idea of having a hidden kref indirectly embedded there, as the
lifetime of this struct will now be controlled outside vb2, with
looks weird.

Thanks,
Mauro

Re: [PATCH v2] media: media-device: fix ioctl function types

[Sakari Ailus]

On Tue, May 08, 2018 at 10:08:41AM +0200, Hans Verkuil wrote:
> Hi Sami,
> 
> This is unchanged from the previous version, right? I've already added that 
> to a
> pull request.

Casting has been removed from the void pointers as I suggested. That's the
difference.

> 
> If this v2 has changes, then let me know asap.
> 
> Regards,
> 
>   Hans
> 
> On 05/07/2018 08:09 PM, Sami Tolvanen wrote:
> > This change fixes function types for media device ioctls to avoid
> > indirect call mismatches with Control-Flow Integrity checking.
> > 
> > Signed-off-by: Sami Tolvanen 
> > ---
> >  drivers/media/media-device.c | 21 +++--
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 35e81f7c0d2f1..ae59c31775557 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -54,9 +54,10 @@ static int media_device_close(struct file *filp)
> > return 0;
> >  }
> >  
> > -static int media_device_get_info(struct media_device *dev,
> > -struct media_device_info *info)
> > +static long media_device_get_info(struct media_device *dev, void *arg)
> >  {
> > +   struct media_device_info *info = arg;
> > +
> > memset(info, 0, sizeof(*info));
> >  
> > if (dev->driver_name[0])
> > @@ -93,9 +94,9 @@ static struct media_entity *find_entity(struct 
> > media_device *mdev, u32 id)
> > return NULL;
> >  }
> >  
> > -static long media_device_enum_entities(struct media_device *mdev,
> > -  struct media_entity_desc *entd)
> > +static long media_device_enum_entities(struct media_device *mdev, void 
> > *arg)
> >  {
> > +   struct media_entity_desc *entd = arg;
> > struct media_entity *ent;
> >  
> > ent = find_entity(mdev, entd->id);
> > @@ -146,9 +147,9 @@ static void media_device_kpad_to_upad(const struct 
> > media_pad *kpad,
> > upad->flags = kpad->flags;
> >  }
> >  
> > -static long media_device_enum_links(struct media_device *mdev,
> > -   struct media_links_enum *links)
> > +static long media_device_enum_links(struct media_device *mdev, void *arg)
> >  {
> > +   struct media_links_enum *links = arg;
> > struct media_entity *entity;
> >  
> > entity = find_entity(mdev, links->entity);
> > @@ -195,9 +196,9 @@ static long media_device_enum_links(struct media_device 
> > *mdev,
> > return 0;
> >  }
> >  
> > -static long media_device_setup_link(struct media_device *mdev,
> > -   struct media_link_desc *linkd)
> > +static long media_device_setup_link(struct media_device *mdev, void *arg)
> >  {
> > +   struct media_link_desc *linkd = arg;
> > struct media_link *link = NULL;
> > struct media_entity *source;
> > struct media_entity *sink;
> > @@ -225,9 +226,9 @@ static long media_device_setup_link(struct media_device 
> > *mdev,
> > return __media_entity_setup_link(link, linkd->flags);
> >  }
> >  
> > -static long media_device_get_topology(struct media_device *mdev,
> > - struct media_v2_topology *topo)
> > +static long media_device_get_topology(struct media_device *mdev, void *arg)
> >  {
> > +   struct media_v2_topology *topo = arg;
> > struct media_entity *entity;
> > struct media_interface *intf;
> > struct media_pad *pad;
> > 
> 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v2] media: v4l: Add new 10-bit packed grayscale format

[Sakari Ailus]

On Tue, May 08, 2018 at 11:46:17AM +0300, Todor Tomov wrote:
> The new format will be called V4L2_PIX_FMT_Y10P.
> It is similar to the V4L2_PIX_FMT_SBGGR10P family formats
> but V4L2_PIX_FMT_Y10P is a grayscale format.
> 
> Signed-off-by: Todor Tomov 

Thanks!

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 1/2] media: v4l: Add new 2X8 10-bit grayscale media bus code

[Sakari Ailus]

On Fri, Apr 27, 2018 at 02:40:38PM +0300, Todor Tomov wrote:
> The code will be called MEDIA_BUS_FMT_Y10_2X8_PADHI_LE.
> It is similar to MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE
> but MEDIA_BUS_FMT_Y10_2X8_PADHI_LE describes grayscale
> data.
> 
> Signed-off-by: Todor Tomov 

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

[GIT PULL for 4.18] Omap3isp fixes

[Sakari Ailus]

Hi Mauro,

Here are some fixes for the omap3isp driver.

Please pull.


The following changes since commit f10379aad39e9da8bc7d1822e251b5f0673067ef:

  media: include/video/omapfb_dss.h: use IS_ENABLED() (2018-05-05 11:45:51 
-0400)

are available in the git repository at:

  ssh://linuxtv.org/git/sailus/media_tree.git omap3isp

for you to fetch changes up to 68578cecbfd0909a6c2547578446da95537498c9:

  omap3isp: Don't use GFP_DMA (2018-05-07 16:25:03 +0300)


Arnd Bergmann (1):
  omap3isp: support 64-bit version of omap3isp_stat_data

Sakari Ailus (2):
  omap3isp: Remove useless NULL check in omap3isp_stat_config
  omap3isp: Don't use GFP_DMA

 drivers/media/platform/omap3isp/isph3a_aewb.c |  2 ++
 drivers/media/platform/omap3isp/isph3a_af.c   |  2 ++
 drivers/media/platform/omap3isp/isphist.c |  2 ++
 drivers/media/platform/omap3isp/ispstat.c | 29 ++-
 drivers/media/platform/omap3isp/ispstat.h |  4 +++-
 include/uapi/linux/omap3isp.h | 22 
 6 files changed, 51 insertions(+), 10 deletions(-)

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [RESEND PATCH v9 1/2] media: dt-bindings: Add bindings for Dongwoon DW9807 voice coil

[Sakari Ailus]

On Tue, May 08, 2018 at 05:36:48AM +, Yeh, Andy wrote:
> Dear reviewers,
> 
> As the dt-binding patch has been accepted, would any help set the driver 
> patch be accepted too? Any missing action from my side blocked the process. 
> Thanks in advance. 
> 
> media: dw9807: Add dw9807 vcm driver
> https://patchwork.linuxtv.org/patch/49159/

The patchwork status was wrong and I've fixed it now; the patches
themselves are contained in a pull request to Mauro here:



-- 
Sakari Ailus
sakari.ai...@linux.intel.com

[PATCH v2] media: v4l: Add new 10-bit packed grayscale format

[Todor Tomov]

The new format will be called V4L2_PIX_FMT_Y10P.
It is similar to the V4L2_PIX_FMT_SBGGR10P family formats
but V4L2_PIX_FMT_Y10P is a grayscale format.

Signed-off-by: Todor Tomov 
---

v2:
- doc: improved bit-packed representation: added bit positions for LSB bits;
- doc: improved bit-packed representation: added table column widths.

 Documentation/media/uapi/v4l/pixfmt-y10p.rst | 33 
 Documentation/media/uapi/v4l/yuv-formats.rst |  1 +
 drivers/media/v4l2-core/v4l2-ioctl.c |  1 +
 include/uapi/linux/videodev2.h   |  1 +
 4 files changed, 36 insertions(+)
 create mode 100644 Documentation/media/uapi/v4l/pixfmt-y10p.rst

diff --git a/Documentation/media/uapi/v4l/pixfmt-y10p.rst 
b/Documentation/media/uapi/v4l/pixfmt-y10p.rst
new file mode 100644
index 000..13b5713
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-y10p.rst
@@ -0,0 +1,33 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _V4L2-PIX-FMT-Y10P:
+
+**
+V4L2_PIX_FMT_Y10P ('Y10P')
+**
+
+Grey-scale image as a MIPI RAW10 packed array
+
+
+Description
+===
+
+This is a packed grey-scale image format with a depth of 10 bits per
+pixel. Every four consecutive pixels are packed into 5 bytes. Each of
+the first 4 bytes contain the 8 high order bits of the pixels, and
+the 5th byte contains the 2 least significants bits of each pixel,
+in the same order.
+
+**Bit-packed representation.**
+
+.. flat-table::
+:header-rows:  0
+:stub-columns: 0
+:widths: 8 8 8 8 64
+
+* - Y'\ :sub:`00[9:2]`
+  - Y'\ :sub:`01[9:2]`
+  - Y'\ :sub:`02[9:2]`
+  - Y'\ :sub:`03[9:2]`
+  - Y'\ :sub:`03[1:0]`\ (bits 7--6) Y'\ :sub:`02[1:0]`\ (bits 5--4)
+   Y'\ :sub:`01[1:0]`\ (bits 3--2) Y'\ :sub:`00[1:0]`\ (bits 1--0)
diff --git a/Documentation/media/uapi/v4l/yuv-formats.rst 
b/Documentation/media/uapi/v4l/yuv-formats.rst
index 3334ea4..9ab0592 100644
--- a/Documentation/media/uapi/v4l/yuv-formats.rst
+++ b/Documentation/media/uapi/v4l/yuv-formats.rst
@@ -29,6 +29,7 @@ to brightness information.
 pixfmt-y10
 pixfmt-y12
 pixfmt-y10b
+pixfmt-y10p
 pixfmt-y16
 pixfmt-y16-be
 pixfmt-y8i
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96d..dececea 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1147,6 +1147,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_PIX_FMT_Y16:  descr = "16-bit Greyscale"; break;
case V4L2_PIX_FMT_Y16_BE:   descr = "16-bit Greyscale BE"; break;
case V4L2_PIX_FMT_Y10BPACK: descr = "10-bit Greyscale (Packed)"; 
break;
+   case V4L2_PIX_FMT_Y10P: descr = "10-bit Greyscale (MIPI 
Packed)"; break;
case V4L2_PIX_FMT_Y8I:  descr = "Interleaved 8-bit Greyscale"; 
break;
case V4L2_PIX_FMT_Y12I: descr = "Interleaved 12-bit Greyscale"; 
break;
case V4L2_PIX_FMT_Z16:  descr = "16-bit Depth"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 600877b..b24ab720 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -522,6 +522,7 @@ struct v4l2_pix_format {
 
 /* Grey bit-packed formats */
 #define V4L2_PIX_FMT_Y10BPACKv4l2_fourcc('Y', '1', '0', 'B') /* 10  
Greyscale bit-packed */
+#define V4L2_PIX_FMT_Y10Pv4l2_fourcc('Y', '1', '0', 'P') /* 10  Greyscale, 
MIPI RAW10 packed */
 
 /* Palette formats */
 #define V4L2_PIX_FMT_PAL8v4l2_fourcc('P', 'A', 'L', '8') /*  8  8-bit 
palette */
-- 
2.7.4

Re: [bug report] [media] exynos4-is: Add the FIMC-IS ISP capture DMA driver

[Sylwester Nawrocki]

On 05/03/2018 01:43 PM, Dan Carpenter wrote:
> [ This code is five years old now.  It's so weird to me that the warning
>   is showing up in my new warnings pile.  Perhaps this wasn't included
>   in my allmodconfig before?  - dan ]

Might be, the bug found is real. The module is normally disabled anyway 
for other reasons.

> Hello Sylwester Nawrocki,
> 
> The patch 34947b8aebe3: "[media] exynos4-is: Add the FIMC-IS ISP
> capture DMA driver" from Dec 20, 2013, leads to the following static
> checker warning:
> 
>   drivers/media/platform/exynos4-is/fimc-isp-video.c:408 
> isp_video_try_fmt_mplane()
>   error: NULL dereference inside function '__isp_video_try_fmt(isp, 
> >fmt.pix_mp, (0))()'.
> 
> drivers/media/platform/exynos4-is/fimc-isp-video.c
>383  static void __isp_video_try_fmt(struct fimc_isp *isp,
>384  struct v4l2_pix_format_mplane *pixm,
>385  const struct fimc_fmt **fmt)
>386  {
>387  *fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
> 
> Unchecked dereference.  We're not allowed to pass a NULL "fmt".
> 
>388  
>389  pixm->colorspace = V4L2_COLORSPACE_SRGB;
>390  pixm->field = V4L2_FIELD_NONE;
>391  pixm->num_planes = (*fmt)->memplanes;
>392  pixm->pixelformat = (*fmt)->fourcc;
>393  /*
>394   * TODO: double check with the docmentation these width/height
>395   * constraints are correct.
>396   */
>397  v4l_bound_align_image(>width, FIMC_ISP_SOURCE_WIDTH_MIN,
>398FIMC_ISP_SOURCE_WIDTH_MAX, 3,
>399>height, 
> FIMC_ISP_SOURCE_HEIGHT_MIN,
>400FIMC_ISP_SOURCE_HEIGHT_MAX, 0, 0);
>401  }
>402  
>403  static int isp_video_try_fmt_mplane(struct file *file, void *fh,
>404  struct v4l2_format *f)
>405  {
>406  struct fimc_isp *isp = video_drvdata(file);
>407  
>408  __isp_video_try_fmt(isp, >fmt.pix_mp, NULL);
>  
> And yet here we are.
> 
>409  return 0;
>410  }

We may need something like:

8<
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c 
b/drivers/media/platform/exynos4-is/fimc-isp-video.c
index 55ba696b8cf4..a920164f53f1 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
@@ -384,12 +384,17 @@ static void __isp_video_try_fmt(struct fimc_isp *isp,
struct v4l2_pix_format_mplane *pixm,
const struct fimc_fmt **fmt)
 {
-   *fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+   const struct fimc_fmt *__fmt;
+
+   __fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+
+   if (fmt)
+   *fmt = __fmt;
 
pixm->colorspace = V4L2_COLORSPACE_SRGB;
pixm->field = V4L2_FIELD_NONE;
-   pixm->num_planes = (*fmt)->memplanes;
-   pixm->pixelformat = (*fmt)->fourcc;
+   pixm->num_planes = __fmt->memplanes;
+   pixm->pixelformat = __fmt->fourcc;
/*
 * TODO: double check with the docmentation these width/height
 * constraints are correct.
8<

I will post the patch to clear the warning.

-- 
Thanks,
Sylwester

Re: [PATCHv13 01/28] v4l2-device.h: always expose mdev

[Hans Verkuil]

On 05/07/2018 05:46 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 4 May 2018 13:51:28 +0300
> Sakari Ailus  escreveu:
> 
>> On Thu, May 03, 2018 at 04:52:51PM +0200, Hans Verkuil wrote:
>>> From: Hans Verkuil 
>>>
>>> The mdev field is only present if CONFIG_MEDIA_CONTROLLER is set.
>>> But since we will need to pass the media_device to vb2 and the
>>> control framework it is very convenient to just make this field
>>> available all the time. If CONFIG_MEDIA_CONTROLLER is not set,
>>> then it will just be NULL.
>>>
>>> Signed-off-by: Hans Verkuil   
>>
>> Acked-by: Sakari Ailus 
>>
> 
> This patch is no-brainer. It could be sent no matter what. However,
> the patch is too simple :-)
> 
> There are a number of places where if CONFIG_MEDIA_CONTROLLER
> (and for CONFIG_MEDIA_CONTROLLER_DVB - with is also optionally 
> added at DVB core) is tested just because the field may or may
> not be there.
> 
> If we're willing to always have it at the struct, then we should look
> on all #ifs for CONFIG_MEDIA_CONTROLLER and get rid of most (or all)
> of them, ensuring that function stubs will be enough for the code
> itself to do the right thing if !CONFIG_MEDIA_CONTROLLER.

I looked at this, and in all cases where v4l2_dev->mdev is checked the
following code always uses something that requires CONFIG_MEDIA_CONTROLLER
anyway (usually the entity).

Regards,

Hans

> 
> Thanks,
> Mauro
> 

Re: [PATCH v11] media: imx258: Add imx258 camera sensor driver

[Tomasz Figa]

Hi Andy,

Sorry, missed one problem in previous review. Would you be able to send a
fixup patch?

On Thu, May 3, 2018 at 12:38 AM Andy Yeh  wrote:

> From: Jason Chen 

> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.

> Signed-off-by: Andy Yeh 
> Signed-off-by: Alan Chiang 
> Reviewed-by: Sakari Ailus 
> Reviewed-by: Tomasz Figa 

> ---
> since v2:
> -- Update the streaming function to remove SW_STANDBY in the beginning.
> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
> since v3:
> -- fix the sd.entity to make code be compiled on the mainline kernel.
> since v4:
> -- Enabled AG, DG, and Exposure time control correctly.
> since v5:
> -- Sensor vendor provided a new setting to fix different CLK issue
> -- Add one more resolution for 1048x780, used for VGA streaming
> since v6:
> -- improved i2c read/write function to support writing 2 registers
> -- modified i2c reg read/write function with a more portable way
> -- utilized v4l2_find_nearest_size instead of the local find_best_fit
function
> -- defined an enum for the link freq entries for explicit indexing
> since v7:
> -- Removed usleep due to sufficient delay implemented in coreboot
> -- Added handling for VBLANK control that auto frame-line-control is
enabled
> since v8:
> -- Fix some error return and intents
> since v9:
> -- Fix a typo (fmr -> fmt)
> since v9.1:
> -- Add code for test pattern control
> -- set vblank and read only since auto-FLL is enabled
> since v10:
> -- Implement test pattern feature:
> -- Output order of test pattern is always BGGR, so it needs a flip to
rotate bayer pattern to required one (GRBG)


>   MAINTAINERS|7 +
>   drivers/media/i2c/Kconfig  |   11 +
>   drivers/media/i2c/Makefile |1 +
>   drivers/media/i2c/imx258.c | 1321

>   4 files changed, 1340 insertions(+)
>   create mode 100644 drivers/media/i2c/imx258.c

> diff --git a/MAINTAINERS b/MAINTAINERS
> index a339bb5..9f75510 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12646,6 +12646,13 @@ S: Maintained
>   F: drivers/ssb/
>   F: include/linux/ssb/

> +SONY IMX258 SENSOR DRIVER
> +M: Sakari Ailus 
> +L: linux-media@vger.kernel.org
> +T: git git://linuxtv.org/media_tree.git
> +S: Maintained
> +F: drivers/media/i2c/imx258.c
> +
>   SONY IMX274 SENSOR DRIVER
>   M: Leon Luo 
>   L: linux-media@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd01842..bcd4bf1 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL
>   config VIDEO_SMIAPP_PLL
>  tristate

> +config VIDEO_IMX258
> +   tristate "Sony IMX258 sensor support"
> +   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +   depends on MEDIA_CAMERA_SUPPORT
> +   ---help---
> + This is a Video4Linux2 sensor-level driver for the Sony
> + IMX258 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx258.
> +
>   config VIDEO_IMX274
>  tristate "Sony IMX274 sensor support"
>  depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 1b62639..4bf7d00 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>   obj-$(CONFIG_VIDEO_ML86V7667)  += ml86v7667.o
>   obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
>   obj-$(CONFIG_VIDEO_TC358743)   += tc358743.o
> +obj-$(CONFIG_VIDEO_IMX258) += imx258.o
>   obj-$(CONFIG_VIDEO_IMX274) += imx274.o

>   obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> new file mode 100644
> index 000..b2e6d06
> --- /dev/null
> +++ b/drivers/media/i2c/imx258.c
> @@ -0,0 +1,1321 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Intel Corporation
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IMX258_REG_VALUE_08BIT 1
> +#define IMX258_REG_VALUE_16BIT 2
> +
> +#define IMX258_REG_MODE_SELECT 0x0100
> +#define IMX258_MODE_STANDBY0x00
> +#define IMX258_MODE_STREAMING  0x01
> +
> +/* Chip ID */
> +#define IMX258_REG_CHIP_ID 0x0016
> +#define IMX258_CHIP_ID 0x0258
> +
> +/* V_TIMING internal */
> +#define IMX258_VTS_30FPS   0x0c98
> +#define IMX258_VTS_30FPS_2K0x0638
> +#define IMX258_VTS_30FPS_VGA   0x034c
> +#define IMX258_VTS_MAX  

Re: [PATCH v2] media: v4l2-ioctl: fix function types for IOCTL_INFO_STD

[Hans Verkuil]

Hi Sami,

This looks much better. There is one thing that can be improved further though:

On 05/07/2018 10:51 PM, Sami Tolvanen wrote:
> This change fixes indirect call mismatches with Control-Flow Integrity
> checking, which are caused by calling standard ioctls using a function
> pointer that doesn't match the type of the actual function.
> 
> Signed-off-by: Sami Tolvanen 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 245 ++-
>  1 file changed, 130 insertions(+), 115 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index de5d96dbe69e0..406b93bd47b47 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2489,11 +2489,8 @@ struct v4l2_ioctl_info {
>   unsigned int ioctl;
>   u32 flags;
>   const char * const name;
> - union {
> - u32 offset;
> - int (*func)(const struct v4l2_ioctl_ops *ops,
> - struct file *file, void *fh, void *p);
> - } u;
> + int (*func)(const struct v4l2_ioctl_ops *ops, struct file *file,
> + void *fh, void *p);
>   void (*debug)(const void *arg, bool write_only);
>  };
>  
> @@ -2501,121 +2498,145 @@ struct v4l2_ioctl_info {
>  #define INFO_FL_PRIO (1 << 0)
>  /* This control can be valid if the filehandle passes a control handler. */
>  #define INFO_FL_CTRL (1 << 1)
> -/* This is a standard ioctl, no need for special code */
> -#define INFO_FL_STD  (1 << 2)
> -/* This is ioctl has its own function */
> -#define INFO_FL_FUNC (1 << 3)
>  /* Queuing ioctl */
> -#define INFO_FL_QUEUE(1 << 4)
> +#define INFO_FL_QUEUE(1 << 2)
>  /* Always copy back result, even on error */
> -#define INFO_FL_ALWAYS_COPY  (1 << 5)
> +#define INFO_FL_ALWAYS_COPY  (1 << 3)
>  /* Zero struct from after the field to the end */
>  #define INFO_FL_CLEAR(v4l2_struct, field)\
>   ((offsetof(struct v4l2_struct, field) + \
> sizeof(((struct v4l2_struct *)0)->field)) << 16)
>  #define INFO_FL_CLEAR_MASK   (_IOC_SIZEMASK << 16)
>  
> -#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags)  
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_STD,  \
> - .name = #_ioctl,\
> - .u.offset = offsetof(struct v4l2_ioctl_ops, _vidioc),   \
> - .debug = _debug,\
> +#define DEFINE_IOCTL_FNC(_vidioc)\
> + static int v4l_ ## _vidioc( \

Don't add the v4l_ prefix here, just use:

static int _vidioc(

> + const struct v4l2_ioctl_ops *ops,   \
> + struct file *file, void *fh, void *p)   \
> + {   \
> + return ops->_vidioc(file, fh, p);   \
>   }
>  
> -#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags)
> \
> - [_IOC_NR(_ioctl)] = {   \
> - .ioctl = _ioctl,\
> - .flags = _flags | INFO_FL_FUNC, \
> - .name = #_ioctl,\
> - .u.func = _func,\
> - .debug = _debug,\
> +#define IOCTL_INFO(_ioctl, _func, _debug, _flags)\
> + [_IOC_NR(_ioctl)] = {   \
> + .ioctl = _ioctl,\
> + .flags = _flags,\
> + .name = #_ioctl,\
> + .func = _func,  \
> + .debug = _debug,\
>   }
>  
> +DEFINE_IOCTL_FNC(vidioc_g_fbuf)

Just call this v4l_stub_g_fbuf, conform the naming of the other functions.

So just replace vidioc_ by v4l_stub_ in all these DEFINE_IOCTL_FNC macros.

This way the function name in the big array matches the name in this macro,
and the 'stub' part indicates that it is just a stub function.

Regards,

Hans

> +DEFINE_IOCTL_FNC(vidioc_s_fbuf)
> +DEFINE_IOCTL_FNC(vidioc_expbuf)
> +DEFINE_IOCTL_FNC(vidioc_g_std)
> +DEFINE_IOCTL_FNC(vidioc_g_audio)
> +DEFINE_IOCTL_FNC(vidioc_s_audio)
> +DEFINE_IOCTL_FNC(vidioc_g_input)
> +DEFINE_IOCTL_FNC(vidioc_g_edid)
> +DEFINE_IOCTL_FNC(vidioc_s_edid)
> +DEFINE_IOCTL_FNC(vidioc_g_output)
> +DEFINE_IOCTL_FNC(vidioc_g_audout)
> +DEFINE_IOCTL_FNC(vidioc_s_audout)
> 

Re: [PATCH v2] media: media-device: fix ioctl function types

[Hans Verkuil]

Hi Sami,

This is unchanged from the previous version, right? I've already added that to a
pull request.

If this v2 has changes, then let me know asap.

Regards,

Hans

On 05/07/2018 08:09 PM, Sami Tolvanen wrote:
> This change fixes function types for media device ioctls to avoid
> indirect call mismatches with Control-Flow Integrity checking.
> 
> Signed-off-by: Sami Tolvanen 
> ---
>  drivers/media/media-device.c | 21 +++--
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7c0d2f1..ae59c31775557 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -54,9 +54,10 @@ static int media_device_close(struct file *filp)
>   return 0;
>  }
>  
> -static int media_device_get_info(struct media_device *dev,
> -  struct media_device_info *info)
> +static long media_device_get_info(struct media_device *dev, void *arg)
>  {
> + struct media_device_info *info = arg;
> +
>   memset(info, 0, sizeof(*info));
>  
>   if (dev->driver_name[0])
> @@ -93,9 +94,9 @@ static struct media_entity *find_entity(struct media_device 
> *mdev, u32 id)
>   return NULL;
>  }
>  
> -static long media_device_enum_entities(struct media_device *mdev,
> -struct media_entity_desc *entd)
> +static long media_device_enum_entities(struct media_device *mdev, void *arg)
>  {
> + struct media_entity_desc *entd = arg;
>   struct media_entity *ent;
>  
>   ent = find_entity(mdev, entd->id);
> @@ -146,9 +147,9 @@ static void media_device_kpad_to_upad(const struct 
> media_pad *kpad,
>   upad->flags = kpad->flags;
>  }
>  
> -static long media_device_enum_links(struct media_device *mdev,
> - struct media_links_enum *links)
> +static long media_device_enum_links(struct media_device *mdev, void *arg)
>  {
> + struct media_links_enum *links = arg;
>   struct media_entity *entity;
>  
>   entity = find_entity(mdev, links->entity);
> @@ -195,9 +196,9 @@ static long media_device_enum_links(struct media_device 
> *mdev,
>   return 0;
>  }
>  
> -static long media_device_setup_link(struct media_device *mdev,
> - struct media_link_desc *linkd)
> +static long media_device_setup_link(struct media_device *mdev, void *arg)
>  {
> + struct media_link_desc *linkd = arg;
>   struct media_link *link = NULL;
>   struct media_entity *source;
>   struct media_entity *sink;
> @@ -225,9 +226,9 @@ static long media_device_setup_link(struct media_device 
> *mdev,
>   return __media_entity_setup_link(link, linkd->flags);
>  }
>  
> -static long media_device_get_topology(struct media_device *mdev,
> -   struct media_v2_topology *topo)
> +static long media_device_get_topology(struct media_device *mdev, void *arg)
>  {
> + struct media_v2_topology *topo = arg;
>   struct media_entity *entity;
>   struct media_interface *intf;
>   struct media_pad *pad;
> 

Re: [PATCHv13 12/28] v4l2-ctrls: add core request support

[Hans Verkuil]

On 05/07/2018 08:06 PM, Mauro Carvalho Chehab wrote:
> Em Thu,  3 May 2018 16:53:02 +0200
> Hans Verkuil  escreveu:
> 
>> From: Hans Verkuil 
>>
>> Integrate the request support. This adds the v4l2_ctrl_request_complete
>> and v4l2_ctrl_request_setup functions to complete a request and (as a
>> helper function) to apply a request to the hardware.
>>
>> It takes care of queuing requests and correctly chaining control values
>> in the request queue.
>>
>> Note that when a request is marked completed it will copy control values
>> to the internal request state. This can be optimized in the future since
>> this is sub-optimal when dealing with large compound and/or array controls.
>>
>> For the initial 'stateless codec' use-case the current implementation is
>> sufficient.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 331 ++-
>>  include/media/v4l2-ctrls.h   |  23 ++
>>  2 files changed, 348 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index da4cc1485dc4..56b986185463 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control *c,
>>  return ptr_to_user(c, ctrl, ctrl->p_new);
>>  }
>>  
>> +/* Helper function: copy the request value back to the caller */
>> +static int req_to_user(struct v4l2_ext_control *c,
>> +   struct v4l2_ctrl_ref *ref)
>> +{
>> +return ptr_to_user(c, ref->ctrl, ref->p_req);
>> +}
>> +
>>  /* Helper function: copy the initial control value back to the caller */
>>  static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>>  {
>> @@ -1766,6 +1773,26 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
>>  ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
>>  }
>>  
>> +/* Copy the new value to the request value */
>> +static void new_to_req(struct v4l2_ctrl_ref *ref)
>> +{
>> +if (!ref)
>> +return;
>> +ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
>> +ref->req = ref;
>> +}
>> +
>> +/* Copy the request value to the new value */
>> +static void req_to_new(struct v4l2_ctrl_ref *ref)
>> +{
>> +if (!ref)
>> +return;
>> +if (ref->req)
>> +ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl->p_new);
>> +else
>> +ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl->p_new);
>> +}
>> +
>>  /* Return non-zero if one or more of the controls in the cluster has a new
>> value that differs from the current value. */
>>  static int cluster_changed(struct v4l2_ctrl *master)
>> @@ -1875,6 +1902,9 @@ int v4l2_ctrl_handler_init_class(struct 
>> v4l2_ctrl_handler *hdl,
>>  lockdep_set_class_and_name(hdl->lock, key, name);
>>  INIT_LIST_HEAD(>ctrls);
>>  INIT_LIST_HEAD(>ctrl_refs);
>> +INIT_LIST_HEAD(>requests);
>> +INIT_LIST_HEAD(>requests_queued);
>> +hdl->request_is_queued = false;
>>  hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
>>  hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
>>sizeof(hdl->buckets[0]),
>> @@ -1895,6 +1925,14 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler 
>> *hdl)
>>  if (hdl == NULL || hdl->buckets == NULL)
>>  return;
>>  
>> +if (!hdl->req_obj.req && !list_empty(>requests)) {
>> +struct v4l2_ctrl_handler *req, *next_req;
>> +
>> +list_for_each_entry_safe(req, next_req, >requests, 
>> requests) {
>> +media_request_object_unbind(>req_obj);
>> +media_request_object_put(>req_obj);
>> +}
>> +}
>>  mutex_lock(hdl->lock);
>>  /* Free all nodes */
>>  list_for_each_entry_safe(ref, next_ref, >ctrl_refs, node) {
>> @@ -2816,6 +2854,128 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, 
>> struct v4l2_querymenu *qm)
>>  }
>>  EXPORT_SYMBOL(v4l2_querymenu);
>>  
>> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>> +   const struct v4l2_ctrl_handler *from)
>> +{
>> +struct v4l2_ctrl_ref *ref;
>> +int err;
>> +
>> +if (WARN_ON(!hdl || hdl == from))
>> +return -EINVAL;
>> +
>> +if (hdl->error)
>> +return hdl->error;
>> +
>> +WARN_ON(hdl->lock != >_lock);
>> +
>> +mutex_lock(from->lock);
>> +list_for_each_entry(ref, >ctrl_refs, node) {
>> +struct v4l2_ctrl *ctrl = ref->ctrl;
>> +struct v4l2_ctrl_ref *new_ref;
>> +
>> +/* Skip refs inherited from other devices */
>> +if (ref->from_other_dev)
>> +continue;
>> +/* And buttons */
>> +if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
>> +continue;
>> +err = handler_new_ref(hdl, ctrl, _ref, false, 

Re: [PATCH v5 0/8] Add support for multi-planar formats and 10 bit formats

[Hans Verkuil]

On 05/07/2018 07:45 PM, Hyun Kwon wrote:
> Hi Hans,
> 
> Thanks for the comment.
> 
> On Mon, 2018-05-07 at 05:59:39 -0700, Hans Verkuil wrote:
>> Hi Satish,
>>
>> On 03/05/18 04:42, Satish Kumar Nagireddy wrote:
>>>  The patches are for xilinx v4l. The patcheset enable support to handle 
>>> multiplanar
>>>  formats and 10 bit formats. Single planar implementation is removed as 
>>> mplane can
>>>  handle both.
>>
>> If I understand the format correctly, then the planes are contiguous in 
>> memory,
>> i.e. it is a single buffer.
>>
>> You do not need to switch to the _MPLANE API for that: that API is meant for 
>> the
>> case where the planes are not contiguous in memory but each plane has its own
>> buffer. And yes, we should have called it the _MBUFFER API or something :-(
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-nv12.html
>>
>> Switching to the _MPLANE API will actually break userspace, so that's another
>> reason why you shouldn't do this. But from what I can tell, it really isn't
>> needed at all.
>>
> 
> Sharing some background to get your further input. :-)
> 
> The Xilinx V4L driver is currently only for the soft IPs, which are
> programmable on FPGA, and those IPs are constantly updated. Initially, IPs
> didn't support _MPLANE formats, so it started with single buffer type format.
> Now, the IPs support _MPLANE formats, even though those formats are not part 
> of
> this patch. Those formats are in downstream vendor tree and will be upstreamed
> at some point[1]. While implementing the multi-buffer formats, we had similar
> concern regarding UAPI and ended up having the module param[2]. It was there
> for a couple of Xilinx release cycles to migrate internal applications to
> _MPLANE formats and to get report if that breaks any external applications. 
> Now
> we thought it's good time to hard-switch the driver to _MPLANE completely
> rather than keeping single buffer code, especially because it seems legal to
> support single buffer formats with _MPLANE type. If this is not the case and
> the applications with single buffer formats but without mplane formats should
> be supported, we can revive the single buffer code in one way or another.

In that case you need to split off the _MPLANE API change into a separate patch.
Switching to the MPLANE API has nothing to do with supporting this format, it is
done for a different reason (future support for real multiplane formats).

Since this also breaks userspace applications you will need to clearly state in
the commit log why this is a reasonable thing to do.

Regards,

Hans

> 
> Thanks,
> -hyun
> 
> [1] 
> https://github.com/Xilinx/linux-xlnx/blob/xilinx-v2018.1/drivers/media/platform/xilinx/xilinx-vip.c#L33
> [2] 
> https://github.com/Xilinx/linux-xlnx/blob/xilinx-v2018.1/drivers/media/platform/xilinx/xilinx-vipp.c#L40
> 
>> Regards,
>>
>>  Hans
>>
>>>
>>>  Patch-set has downstream changes and bug fixes. Added new media bus format
>>>  MEDIA_BUS_FMT_VYYUYY8_1X24, new pixel format V4L2_PIX_FMT_XV15 and rst
>>>  documentation.
>>>
>>> Jeffrey Mouroux (1):
>>>   uapi: media: New fourcc code and rst for 10 bit format
>>>
>>> Radhey Shyam Pandey (1):
>>>   v4l: xilinx: dma: Remove colorspace check in xvip_dma_verify_format
>>>
>>> Rohit Athavale (1):
>>>   xilinx: v4l: dma: Update driver to allow for probe defer
>>>
>>> Satish Kumar Nagireddy (4):
>>>   media-bus: uapi: Add YCrCb 420 media bus format and rst
>>>   v4l: xilinx: dma: Update video format descriptor
>>>   v4l: xilinx: dma: Add multi-planar support
>>>   v4l: xilinx: dma: Add support for 10 bit formats
>>>
>>> Vishal Sagar (1):
>>>   xilinx: v4l: dma: Terminate DMA when media pipeline fail to start
>>>
>>>  Documentation/media/uapi/v4l/pixfmt-xv15.rst| 134 +++
>>>  Documentation/media/uapi/v4l/subdev-formats.rst |  38 +-
>>>  Documentation/media/uapi/v4l/yuv-formats.rst|   1 +
>>>  drivers/media/platform/xilinx/xilinx-dma.c  | 170 
>>> +++-
>>>  drivers/media/platform/xilinx/xilinx-dma.h  |   4 +-
>>>  drivers/media/platform/xilinx/xilinx-vip.c  |  37 --
>>>  drivers/media/platform/xilinx/xilinx-vip.h  |  15 ++-
>>>  drivers/media/platform/xilinx/xilinx-vipp.c |  16 +--
>>>  include/uapi/linux/media-bus-format.h   |   3 +-
>>>  include/uapi/linux/videodev2.h  |   1 +
>>>  10 files changed, 333 insertions(+), 86 deletions(-)
>>>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-xv15.rst
>>>
>>

Re: [PATCHv13 09/28] v4l2-ctrls: prepare internal structs for request API

[Hans Verkuil]

On 05/07/2018 07:35 PM, Mauro Carvalho Chehab wrote:
> Em Thu,  3 May 2018 16:52:59 +0200
> Hans Verkuil  escreveu:
> 
>> From: Hans Verkuil 
>>
>> Embed and initialize a media_request_object in struct v4l2_ctrl_handler.
>>
>> Add a p_req field to struct v4l2_ctrl_ref that will store the
>> request value.
>>
>> Signed-off-by: Hans Verkuil 
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 1 +
>>  include/media/v4l2-ctrls.h   | 7 +++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index aa1dd2015e84..d09f49530d9e 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1880,6 +1880,7 @@ int v4l2_ctrl_handler_init_class(struct 
>> v4l2_ctrl_handler *hdl,
>>sizeof(hdl->buckets[0]),
>>GFP_KERNEL | __GFP_ZERO);
>>  hdl->error = hdl->buckets ? 0 : -ENOMEM;
>> +media_request_object_init(>req_obj);
>>  return hdl->error;
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index d26b8ddebb56..76352eb59f14 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  /* forward references */
>>  struct file;
>> @@ -249,6 +250,8 @@ struct v4l2_ctrl {
>>   *  ``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
>>   * @from_other_dev: If true, then @ctrl was defined in another
>>   *  device than the  v4l2_ctrl_handler.
>> + * @p_req:  The request value. Only used if the control handler
>> + *  is bound to a media request.
> 
> Could you please better elaborate the description of this field?
> 
> I read this patch dozen times to understand what you meant by
> "request value", as I would be expecting here a "media_request_"
> object or something similar. Instead, what you meant to say is that
> @p_req will be used to cache the data passed via a request API call,
> while the request is not handled yet, right?

The control handler has the request object. If the control handler is
part of a request, then the values of the controls that are owned by that
control handler are stored in p_req. When the request has been completed
p_req will contain the value at completion time.

I'll improve the documentation.

> 
> 
>>   *
>>   * Each control handler has a list of these refs. The list_head is used to
>>   * keep a sorted-by-control-ID list of all controls, while the next pointer
>> @@ -260,6 +263,7 @@ struct v4l2_ctrl_ref {
>>  struct v4l2_ctrl *ctrl;
>>  struct v4l2_ctrl_helper *helper;
>>  bool from_other_dev;
>> +union v4l2_ctrl_ptr p_req;
>>  };
>>  
>>  /**
>> @@ -283,6 +287,8 @@ struct v4l2_ctrl_ref {
>>   * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
>>   * @nr_of_buckets: Total number of buckets in the array.
>>   * @error:  The error code of the first failed control addition.
>> + * @req_obj:The  media_request_object, used to link into a
>> + *   media_request.
> 
> I would document that there is kref is inside @req_obj, as we can't
> add another kref here later.

OK.

Regards,

Hans

> 
>>   */
>>  struct v4l2_ctrl_handler {
>>  struct mutex _lock;
>> @@ -295,6 +301,7 @@ struct v4l2_ctrl_handler {
>>  void *notify_priv;
>>  u16 nr_of_buckets;
>>  int error;
>> +struct media_request_object req_obj;
>>  };
>>  
>>  /**
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv13 06/28] v4l2-dev: lock req_queue_mutex

[Hans Verkuil]

On 05/07/2018 07:20 PM, Mauro Carvalho Chehab wrote:
> Em Thu,  3 May 2018 16:52:56 +0200
> Hans Verkuil  escreveu:
> 
>> From: Hans Verkuil 
>>
>> We need to serialize streamon/off with queueing new requests.
>> These ioctls may trigger the cancellation of a streaming
>> operation, and that should not be mixed with queuing a new
>> request at the same time.
>>
>> Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
>> with MEDIA_REQUEST_IOC_QUEUE.
>>
>> Finally close() needs this lock since that too can trigger the
>> cancellation of a streaming operation.
>>
>> We take the req_queue_mutex here before any other locks since
>> it is a very high-level lock.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 37 +-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
>> b/drivers/media/v4l2-core/v4l2-dev.c
>> index 1d0b2208e8fb..b1c9efc0ecc4 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -353,13 +353,36 @@ static long v4l2_ioctl(struct file *filp, unsigned int 
>> cmd, unsigned long arg)
>>  
>>  if (vdev->fops->unlocked_ioctl) {
>>  struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
>> +struct mutex *queue_lock = NULL;
>>  
>> -if (lock && mutex_lock_interruptible(lock))
>> +/*
>> + * We need to serialize streamon/off with queueing new requests.
>> + * These ioctls may trigger the cancellation of a streaming
>> + * operation, and that should not be mixed with queueing a new
>> + * request at the same time.
>> + *
>> + * Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
>> + * with MEDIA_REQUEST_IOC_QUEUE.
>> + */
>> +if (vdev->v4l2_dev->mdev &&
>> +(cmd == VIDIOC_STREAMON || cmd == VIDIOC_STREAMOFF ||
>> + cmd == VIDIOC_S_EXT_CTRLS || cmd == VIDIOC_TRY_EXT_CTRLS))
>> +queue_lock = >v4l2_dev->mdev->req_queue_mutex;
>> +
>> +if (queue_lock && mutex_lock_interruptible(queue_lock))
>> +return -ERESTARTSYS;
> 
> Taking both locks seems risky. Here you're taking first v4l2 lock, returned
> by v4l2_ioctl_get_lock(vdev, cmd), and then you're taking the req_queue lock.

No,  v4l2_ioctl_get_lock() only returns a pointer to a mutex, it doesn't lock
anything. I think you got confused there. I'll reorganize the code a bit so
the call to  v4l2_ioctl_get_lock() happens after the queue_lock has been taken.

I'll also rename queue_lock to req_queue_lock (it's a bit more descriptive).

So we first take the high-level media_device req_queue_mutex if needed, and
then the ioctl serialization lock. Doing it the other way around will indeed
promptly deadlock (as I very quickly discovered after my initial 
implementation!).

So the order is:

req_queue_mutex (serialize request state changes from/to IDLE)
ioctl lock (serialize ioctls)
request->lock (spinlock)

The last is only held for short periods when updating the media_request struct.

> 
> It is possible to call parts of the code that only handles req_queue
> or v4l2 lock (for example, by mixing request API calls with non-requests
> one). Worse than that, there are parts of the code where the request API
> patches get both a mutex and a spin lock.
> 
> I didn't look too closely (nor ran any test), but I'm almost sure that
> there are paths where it will end by leading into dead locks.

I've done extensive testing with this and actually been very careful about
the lock handling. It's also been tested with the cedrus driver.

Regards,

Hans

> 
>> +
>> +if (lock && mutex_lock_interruptible(lock)) {
>> +if (queue_lock)
>> +mutex_unlock(queue_lock);
>>  return -ERESTARTSYS;
>> +}
>>  if (video_is_registered(vdev))
>>  ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
>>  if (lock)
>>  mutex_unlock(lock);
>> +if (queue_lock)
>> +mutex_unlock(queue_lock);
>>  } else
>>  ret = -ENOTTY;
>>  
>> @@ -442,8 +465,20 @@ static int v4l2_release(struct inode *inode, struct 
>> file *filp)
>>  struct video_device *vdev = video_devdata(filp);
>>  int ret = 0;
>>  
>> +/*
>> + * We need to serialize the release() with queueing new requests.
>> + * The release() may trigger the cancellation of a streaming
>> + * operation, and that should not be mixed with queueing a new
>> + * request at the same time.
>> + */
>> +if (vdev->v4l2_dev->mdev)
>> +mutex_lock(>v4l2_dev->mdev->req_queue_mutex);
>> +
>>  if (vdev->fops->release)

Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd

[Hans Verkuil]

On 05/07/2018 07:01 PM, Mauro Carvalho Chehab wrote:
> Em Thu,  3 May 2018 16:52:54 +0200
> Hans Verkuil  escreveu:
> 
>> From: Hans Verkuil 
>>
>> Add media_request_get_by_fd() to find a request based on the file
>> descriptor.
>>
>> The caller has to call media_request_put() for the returned
>> request since this function increments the refcount.
>>
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/media-request.c | 32 
>>  include/media/media-request.h | 24 
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> index c216c4ab628b..edc1c3af1959 100644
>> --- a/drivers/media/media-request.c
>> +++ b/drivers/media/media-request.c
>> @@ -218,6 +218,38 @@ static const struct file_operations request_fops = {
>>  .release = media_request_close,
>>  };
>>  
>> +struct media_request *
>> +media_request_get_by_fd(struct media_device *mdev, int request_fd)
>> +{
>> +struct file *filp;
>> +struct media_request *req;
>> +
>> +if (!mdev || !mdev->ops ||
>> +!mdev->ops->req_validate || !mdev->ops->req_queue)
>> +return ERR_PTR(-EPERM);
>> +
>> +filp = fget(request_fd);
>> +if (!filp)
>> +return ERR_PTR(-ENOENT);
>> +
>> +if (filp->f_op != _fops)
>> +goto err_fput;
>> +req = filp->private_data;
>> +if (req->mdev != mdev)
>> +goto err_fput;
>> +
>> +media_request_get(req);
> 
> Hmm... this function changes the req struct (by calling kref_get) without
> holding neither the mutex or the spin lock...

kref_get is atomic, so why would you need to add additional locking?

Neither can the request be 'put' by another process before media_request_get()
is called due to the fget() above: while the fd has a refcount > 0 the
request refcount is also > 0. I'll add some comments to clarify this.

> 
>> +fput(filp);
>> +
>> +return req;
>> +
>> +err_fput:
>> +fput(filp);
>> +
>> +return ERR_PTR(-ENOENT);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_get_by_fd);
>> +
>>  int media_request_alloc(struct media_device *mdev,
>>  struct media_request_alloc *alloc)
>>  {
>> diff --git a/include/media/media-request.h b/include/media/media-request.h
>> index e39122dfd717..997e096d7128 100644
>> --- a/include/media/media-request.h
>> +++ b/include/media/media-request.h
>> @@ -86,6 +86,24 @@ static inline void media_request_get(struct media_request 
>> *req)
>>   */
>>  void media_request_put(struct media_request *req);
>>  
>> +/**
>> + * media_request_get_by_fd - Get a media request by fd
>> + *
>> + * @mdev: Media device this request belongs to
>> + * @request_fd: The file descriptor of the request
>> + *
>> + * Get the request represented by @request_fd that is owned
>> + * by the media device.
>> + *
>> + * Return a -EPERM error pointer if requests are not supported
>> + * by this driver. Return -ENOENT if the request was not found.
>> + * Return the pointer to the request if found: the caller will
>> + * have to call @media_request_put when it finished using the
>> + * request.
> 
> ... so, it should be said here how this should be serialized, in order
> to avoid it to be destroyed by a task while some other task might be
> trying to instantiate it.

This doesn't need any serialization. If the request_fd is found, then
it will return the request with the request refcount increased.

I also do not understand what you mean with "some other task might be
trying to instantiate it".

I think there is some misunderstanding here.

Regards,

Hans


> 
>> + */
>> +struct media_request *
>> +media_request_get_by_fd(struct media_device *mdev, int request_fd);
>> +
>>  /**
>>   * media_request_alloc - Allocate the media request
>>   *
>> @@ -107,6 +125,12 @@ static inline void media_request_put(struct 
>> media_request *req)
>>  {
>>  }
>>  
>> +static inline struct media_request *
>> +media_request_get_by_fd(struct media_device *mdev, int request_fd)
>> +{
>> +return ERR_PTR(-EPERM);
>> +}
>> +
>>  #endif
>>  
>>  /**
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH] media: atomisp: fix spelling mistake: "diregard" -> "disregard"

[Yunliang Ding]

On 2018-04-29 at 13:06:47 +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in ia_css_print message text

Hi Colin,

The atomisp drivers will soon EOL accorinding to the ML discussion.

> 
> Signed-off-by: Colin Ian King 
> ---
>  .../css2400/css_2401_csi2p_system/host/csi_rx_private.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/host/csi_rx_private.h
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/host/csi_rx_private.h
> index 9c0cb4a63862..4fa74e7a96e6 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/host/csi_rx_private.h
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/host/csi_rx_private.h
> @@ -202,7 +202,7 @@ static inline void csi_rx_be_ctrl_dump_state(
>   ia_css_print("CSI RX BE STATE Controller %d PEC ID %d custom 
> pec 0x%x \n", ID, i, state->pec[i]);
>   }
>  #endif
> - ia_css_print("CSI RX BE STATE Controller %d Global LUT diregard reg 
> 0x%x \n", ID, state->global_lut_disregard_reg);
> + ia_css_print("CSI RX BE STATE Controller %d Global LUT disregard reg 
> 0x%x \n", ID, state->global_lut_disregard_reg);
>   ia_css_print("CSI RX BE STATE Controller %d packet stall reg 0x%x \n", 
> ID, state->packet_status_stall);
>   /*
>* Get the values of the register-set per
> -- 
> 2.17.0
> 

Re: [PATCH 2/2] media: v4l: Add new 10-bit packed grayscale format

[Todor Tomov]

Hi Sakari,

On  7.05.2018 13:59, Sakari Ailus wrote:
> Hi Todor,
> 
> On Fri, Apr 27, 2018 at 02:40:39PM +0300, Todor Tomov wrote:
>> The new format will be called V4L2_PIX_FMT_Y10P.
>> It is similar to the V4L2_PIX_FMT_SBGGR10P family formats
>> but V4L2_PIX_FMT_Y10P is a grayscale format.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  Documentation/media/uapi/v4l/pixfmt-y10p.rst | 31 
>> 
>>  Documentation/media/uapi/v4l/yuv-formats.rst |  1 +
>>  drivers/media/v4l2-core/v4l2-ioctl.c |  1 +
>>  include/uapi/linux/videodev2.h   |  1 +
>>  4 files changed, 34 insertions(+)
>>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-y10p.rst
>>
>> diff --git a/Documentation/media/uapi/v4l/pixfmt-y10p.rst 
>> b/Documentation/media/uapi/v4l/pixfmt-y10p.rst
>> new file mode 100644
>> index 000..0018fe7
>> --- /dev/null
>> +++ b/Documentation/media/uapi/v4l/pixfmt-y10p.rst
>> @@ -0,0 +1,31 @@
>> +.. -*- coding: utf-8; mode: rst -*-
>> +
>> +.. _V4L2-PIX-FMT-Y10P:
>> +
>> +**
>> +V4L2_PIX_FMT_Y10P ('Y10P')
>> +**
>> +
>> +Grey-scale image as a MIPI RAW10 packed array
>> +
>> +
>> +Description
>> +===
>> +
>> +This is a packed grey-scale image format with a depth of 10 bits per
>> +pixel. Every four consecutive pixels are packed into 5 bytes. Each of
>> +the first 4 bytes contain the 8 high order bits of the pixels, and
>> +the 5th byte contains the 2 least significants bits of each pixel,
>> +in the same order.
>> +
>> +**Bit-packed representation.**
>> +
>> +.. flat-table::
>> +:header-rows:  0
>> +:stub-columns: 0
>> +
>> +* - Y'\ :sub:`00[9:2]`
>> +  - Y'\ :sub:`01[9:2]`
>> +  - Y'\ :sub:`02[9:2]`
>> +  - Y'\ :sub:`03[9:2]`
>> +  - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[1:0]`\ Y'\ :sub:`01[1:0]`\ Y'\ 
>> :sub:`00[1:0]`
> 
> Could you add which exact bits the two LSBs of each pixel go to in the last
> byte, as in the 10-bit packed Bayer format documentation?

Thank you for review. I'll add and send v2.

Best regards,
Todor

> 
>> diff --git a/Documentation/media/uapi/v4l/yuv-formats.rst 
>> b/Documentation/media/uapi/v4l/yuv-formats.rst
>> index 3334ea4..9ab0592 100644
>> --- a/Documentation/media/uapi/v4l/yuv-formats.rst
>> +++ b/Documentation/media/uapi/v4l/yuv-formats.rst
>> @@ -29,6 +29,7 @@ to brightness information.
>>  pixfmt-y10
>>  pixfmt-y12
>>  pixfmt-y10b
>> +pixfmt-y10p
>>  pixfmt-y16
>>  pixfmt-y16-be
>>  pixfmt-y8i
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index f48c505..bdf2399 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1147,6 +1147,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>  case V4L2_PIX_FMT_Y16:  descr = "16-bit Greyscale"; break;
>>  case V4L2_PIX_FMT_Y16_BE:   descr = "16-bit Greyscale BE"; break;
>>  case V4L2_PIX_FMT_Y10BPACK: descr = "10-bit Greyscale (Packed)"; 
>> break;
>> +case V4L2_PIX_FMT_Y10P: descr = "10-bit Greyscale (MIPI 
>> Packed)"; break;
>>  case V4L2_PIX_FMT_Y8I:  descr = "Interleaved 8-bit Greyscale"; 
>> break;
>>  case V4L2_PIX_FMT_Y12I: descr = "Interleaved 12-bit Greyscale"; 
>> break;
>>  case V4L2_PIX_FMT_Z16:  descr = "16-bit Depth"; break;
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 600877b..b24ab720 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -522,6 +522,7 @@ struct v4l2_pix_format {
>>  
>>  /* Grey bit-packed formats */
>>  #define V4L2_PIX_FMT_Y10BPACKv4l2_fourcc('Y', '1', '0', 'B') /* 10  
>> Greyscale bit-packed */
>> +#define V4L2_PIX_FMT_Y10Pv4l2_fourcc('Y', '1', '0', 'P') /* 10  
>> Greyscale, MIPI RAW10 packed */
>>  
>>  /* Palette formats */
>>  #define V4L2_PIX_FMT_PAL8v4l2_fourcc('P', 'A', 'L', '8') /*  8  8-bit 
>> palette */
>> -- 
>> 2.7.4
>>
> 

[PATCH] media: i2c: tda1997: Fix an error handling path 'tda1997x_probe()'

[Christophe JAILLET]

If 'media_entity_pads_init()' fails, we must free the resources allocated
by 'v4l2_ctrl_handler_init()', as already done in the previous error
handling path.

'goto' the right label to fix it.

Fixes: 9ac0038db9a7 ("media: i2c: Add TDA1997x HDMI receiver driver")
Signed-off-by: Christophe JAILLET 
---
 drivers/media/i2c/tda1997x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
index 31bdab96f380..039a92c3294a 100644
--- a/drivers/media/i2c/tda1997x.c
+++ b/drivers/media/i2c/tda1997x.c
@@ -2723,7 +2723,7 @@ static int tda1997x_probe(struct i2c_client *client,
state->pads);
if (ret) {
v4l_err(client, "failed entity_init: %d", ret);
-   goto err_free_mutex;
+   goto err_free_handler;
}
 
ret = v4l2_async_register_subdev(sd);
-- 
2.17.0