Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil :

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > 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 
> > ---
> >  drivers/media/Kconfig|  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 
> > +---
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 -
> >  include/media/videobuf2-core.h   |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> 
> 
> 
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
> > b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 110fb45..e6ad77f 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -23,6 +23,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> >  
> >  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  {
> > +   struct dma_fence *fence = NULL;
> > int ret;
> >  
> > if (vb2_fileio_is_active(q)) {
> > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer 
> > *b)
> > }
> >  
> > ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > -   return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > +   fence = sync_file_get_fence(b->fence_fd);
> > +   if (!fence) {
> > +   dprintk(1, "failed to get in-fence from fd\n");
> > +   return -EINVAL;
> > +   }
> > +   }
> > +
> > +   return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 
> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> if there is a fence pending. It should also fill in fence_fd.

It was userspace who sent the fence_fd in the first place. Why do we
need to send it back? The initial plan was - from a userspace point of view
- to send the in-fence in the fence_fd field and receive the out-fence
 in the same field.

As per setting the IN_FENCE flag, that is racy, as the fence can signal
just after we called __fill_v4l2_buffer. Why is it important to set it?

Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil :

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > 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 
> > ---
> >  drivers/media/Kconfig|  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 
> > +---
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 -
> >  include/media/videobuf2-core.h   |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > index 55d9c2b..3cd1d3d 100644
> > --- a/drivers/media/Kconfig
> > +++ b/drivers/media/Kconfig
> > @@ -11,6 +11,7 @@ config CEC_NOTIFIER
> >  menuconfig MEDIA_SUPPORT
> > tristate "Multimedia support"
> > depends on HAS_IOMEM
> > +   select SYNC_FILE
> 
> Is this the right place for this? Shouldn't this be selected in
> 'config VIDEOBUF2_CORE'?
> 
> Fences are specific to vb2 after all.

Definitely.

> 
> > help
> >   If you want to use Webcams, Video grabber devices and/or TV devices
> >   enable this option and other options below.
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index ea83126..29aa9d4 100644

> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, 
> > const void *pb)
> > return 0;
> >  }
> >  
> > +static int __get_num_ready_buffers(struct vb2_queue *q)
> > +{
> > +   struct vb2_buffer *vb;
> > +   int ready_count = 0;
> > +
> > +   /* count num of buffers ready in front of the queued_list */
> > +   list_for_each_entry(vb, >queued_list, queued_entry) {
> > +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +   break;
> 
> Obviously the break is wrong as Mauro mentioned.

I replied this in the other email to Mauro, if a fence is not signaled
it is not ready te be queued by the driver nor is all buffers following
it. Hence the break. They need all to be in order and in front of the
queue.

In any case I'll check this again as now there is two people saying I'm
wrong! :)

> 
> > +
> > +   ready_count++;
> > +   }
> > +
> > +   return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> > struct vb2_buffer *vb;
> > @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  * If any buffers were queued before streamon,
> >  * we can now pass them to driver for processing.
> >  */
> > -   list_for_each_entry(vb, >queued_list, queued_entry)
> > +   list_for_each_entry(vb, >queued_list, queued_entry) {
> > +   if (vb->state != VB2_BUF_STATE_QUEUED)
> > +   continue;
> 
> I think this test is unnecessary.

It might be indeed. It is necessary in __vb2_core_qbuf() so I think I
just copied it here without thinking.

> 
> > +
> > +   if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +   break;
> > +
> > __enqueue_in_driver(vb);
> 
> I would move the above test (after fixing it as Mauro said) to 
> __enqueue_in_driver.
> I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

Yes, having this check inside __enqueue_in_driver() makes it looks much
nicer.

> 
> > +   }
> >  
> > /* Tell the driver to start streaming */
> > q->start_streaming_called = 1;
> > @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  
> >  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> >  {
> > +   struct vb2_buffer *b;
> > int ret;
> >  
> > /*
> >  * If already streaming, give the buffer to driver for processing.
> >  * If not, the buffer will be given to driver on next streamon.
> >  */
> > -   if (q->start_streaming_called)
> > -   __enqueue_in_driver(vb);
> >  
> > -   /*
> > -* If streamon has been called, and we haven't yet called
> > -* start_streaming() since not enough buffers were queued, and
> > -* we now have reached the minimum number of queued buffers,
> > -* then we can finally call start_streaming().
> > -*/
> > -   if (q->streaming && !q->start_streaming_called &&
> > -   q->queued_count >= q->min_buffers_needed) {
> > -   ret = vb2_start_streaming(q);
> > -  

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil :

> On 07/03/17 20:16, Gustavo Padovan wrote:
> >>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned 
> >>> int index, void *pb)
> >>>   if (pb)
> >>>   call_void_bufop(q, fill_user_buffer, vb, pb);
> >>>  
> >>> + vb->in_fence = fence;
> >>> + if (fence && !dma_fence_add_callback(fence, >fence_cb,
> >>> +  vb2_qbuf_fence_cb))
> >>> + return 0;
> >>
> >> Maybe we should provide some error or debug log here or a WARN_ON(), if 
> >> dma_fence_add_callback() fails instead of silently ignore any errors.
> > 
> > This is not an error. If the if succeeds it mean we have installed a
> > callback for the fence. If not, it means the fence signaled already and
> > we don't can call __vb2_core_qbuf right away.
> 
> I had the same question as Mauro. After looking at the dma_fence_add_callback
> code I see what you mean, but a comment would certainly be helpful.

Sure, I'll add a comment explaining it.

> 
> Also, should you set vb->in_fence to NULL if the fence signaled already?
> I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
> You would know that better than I do.

You got it right. I should be doing that.

Gustavo

Re: [PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Gustavo Padovan]

2017-07-06 Hans Verkuil :

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > In order to support explicit synchronization we need to divide
> > vb2_core_qbuf() in two parts, one to be executed before the fence
> > signals and another one to do the actual queueing of the buffer.
> > 
> > Signed-off-by: Gustavo Padovan 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c | 51 
> > ++--
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index 3107e21..ea83126 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
> > return ret;
> >  }
> >  
> > +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> > +{
> > +   int ret;
> > +
> > +   /*
> > +* If already streaming, give the buffer to driver for processing.
> > +* If not, the buffer will be given to driver on next streamon.
> > +*/
> > +   if (q->start_streaming_called)
> > +   __enqueue_in_driver(vb);
> > +
> > +   /*
> > +* If streamon has been called, and we haven't yet called
> > +* start_streaming() since not enough buffers were queued, and
> > +* we now have reached the minimum number of queued buffers,
> > +* then we can finally call start_streaming().
> > +*/
> > +   if (q->streaming && !q->start_streaming_called &&
> > +   q->queued_count >= q->min_buffers_needed) {
> > +   ret = vb2_start_streaming(q);
> > +   if (ret)
> > +   return ret;
> > +   }
> > +
> > +   dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> > +   return 0;
> > +}
> > +
> >  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> > struct vb2_buffer *vb;
> > @@ -1404,32 +1432,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> > index, void *pb)
> >  
> > trace_vb2_qbuf(q, vb);
> >  
> > -   /*
> > -* If already streaming, give the buffer to driver for processing.
> > -* If not, the buffer will be given to driver on next streamon.
> > -*/
> > -   if (q->start_streaming_called)
> > -   __enqueue_in_driver(vb);
> > -
> > /* Fill buffer information for the userspace */
> > if (pb)
> > call_void_bufop(q, fill_user_buffer, vb, pb);
> 
> This should be called *after* the __vb2_core_qbuf call. That call changes
> vb->state which is used by buffer to fill in v4l2_buffer. So the order
> should be swapped here to ensure we return the latest state of the buffer.

I didn't pay attention to that detail. The reason why I put it here is
that later when in-fences are introduced we will only call
__vb2_core_qbuf() after the fence signals and that may happen after we
returned to userspace. I'll look into possible ways to solve this
problem.

Gustavo

Re: [PATCH] rcar_fdp1: constify vb2_ops structure

[Kieran Bingham]

Hi Gustavo,

Thank you for the patch.

On 06/07/17 21:25, Gustavo A. R. Silva wrote:
> Check for vb2_ops structures that are only stored in the ops field of a
> vb2_queue structure. That field is declared const, so vb2_ops structures
> that have this property can be declared as const also.
> 
> This issue was detected using Coccinelle and the following semantic patch:
> 
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct vb2_ops i@p = { ... };
> 
> @ok@
> identifier r.i;
> struct vb2_queue e;
> position p;
> @@
> e.ops = @p;
> 
> @bad@
> position p != {r.p,ok.p};
> identifier r.i;
> struct vb2_ops e;
> @@
> e@i@p
> 
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
> struct vb2_ops i = { ... };
> 
> Signed-off-by: Gustavo A. R. Silva 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/rcar_fdp1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar_fdp1.c 
> b/drivers/media/platform/rcar_fdp1.c
> index 3ee51fc..3245bc4 100644
> --- a/drivers/media/platform/rcar_fdp1.c
> +++ b/drivers/media/platform/rcar_fdp1.c
> @@ -2032,7 +2032,7 @@ static void fdp1_stop_streaming(struct vb2_queue *q)
>   }
>  }
>  
> -static struct vb2_ops fdp1_qops = {
> +static const struct vb2_ops fdp1_qops = {
>   .queue_setup = fdp1_queue_setup,
>   .buf_prepare = fdp1_buf_prepare,
>   .buf_queue   = fdp1_buf_queue,
> 

[PATCH] davinci: vpif_display: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/davinci/vpif_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/davinci/vpif_display.c 
b/drivers/media/platform/davinci/vpif_display.c
index b5ac6ce..1a494d3 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -290,7 +290,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
spin_unlock_irqrestore(>irqlock, flags);
 }
 
-static struct vb2_ops video_qops = {
+static const struct vb2_ops video_qops = {
.queue_setup= vpif_buffer_queue_setup,
.wait_prepare   = vb2_ops_wait_prepare,
.wait_finish= vb2_ops_wait_finish,
-- 
2.5.0

Re: [PATCH v6 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[kbuild test robot]

Hi Jose,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20170706]
[cannot apply to v4.12]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jose-Abreu/Synopsys-Designware-HDMI-Video-Capture-Controller-PHY/20170707-041312
base:   git://linuxtv.org/media_tree.git master
config: blackfin-allyesconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin 

All errors (new ones prefixed by >>):

   drivers/media//platform/dwc/dw-hdmi-rx.c: In function 'dw_hdmi_registered':
>> drivers/media//platform/dwc/dw-hdmi-rx.c:1452:9: error: implicit declaration 
>> of function 'v4l2_async_subnotifier_register' 
>> [-Werror=implicit-function-declaration]
 return v4l2_async_subnotifier_register(_dev->sd,
^~~
   drivers/media//platform/dwc/dw-hdmi-rx.c: In function 'dw_hdmi_unregistered':
>> drivers/media//platform/dwc/dw-hdmi-rx.c:1462:2: error: implicit declaration 
>> of function 'v4l2_async_subnotifier_unregister' 
>> [-Werror=implicit-function-declaration]
 v4l2_async_subnotifier_unregister(_dev->v4l2_notifier);
 ^
   drivers/media//platform/dwc/dw-hdmi-rx.c: In function 'dw_hdmi_parse_dt':
   drivers/media//platform/dwc/dw-hdmi-rx.c:1555:22: warning: unused variable 
'notifier' [-Wunused-variable]
 struct device_node *notifier, *phy_node, *np = dw_dev->of_node;
 ^~~~
   drivers/media//platform/dwc/dw-hdmi-rx.c: In function 'dw_hdmi_rx_probe':
   drivers/media//platform/dwc/dw-hdmi-rx.c:1765:1: warning: label 'err_phy' 
defined but not used [-Wunused-label]
err_phy:
^~~
   cc1: some warnings being treated as errors

vim +/v4l2_async_subnotifier_register +1452 
drivers/media//platform/dwc/dw-hdmi-rx.c

  1446  return ret;
  1447  }
  1448  
  1449  cec_register_cec_notifier(dw_dev->cec_adap, 
dw_dev->cec_notifier);
  1450  dw_dev->registered = true;
  1451  
> 1452  return v4l2_async_subnotifier_register(_dev->sd,
  1453  _dev->v4l2_notifier);
  1454  }
  1455  
  1456  static void dw_hdmi_unregistered(struct v4l2_subdev *sd)
  1457  {
  1458  struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
  1459  
  1460  cec_unregister_adapter(dw_dev->cec_adap);
  1461  cec_notifier_put(dw_dev->cec_notifier);
> 1462  v4l2_async_subnotifier_unregister(_dev->v4l2_notifier);
  1463  }
  1464  
  1465  static const struct v4l2_subdev_core_ops dw_hdmi_sd_core_ops = {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH] mediatek: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c 
b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 451a540..f17a86b 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -756,7 +756,7 @@ static void mtk_jpeg_stop_streaming(struct vb2_queue *q)
pm_runtime_put_sync(ctx->jpeg->dev);
 }
 
-static struct vb2_ops mtk_jpeg_qops = {
+static const struct vb2_ops mtk_jpeg_qops = {
.queue_setup= mtk_jpeg_queue_setup,
.buf_prepare= mtk_jpeg_buf_prepare,
.buf_queue  = mtk_jpeg_buf_queue,
-- 
2.5.0

[PATCH] mtk-mdp: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c 
b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index 13afe48..3038d62 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -621,7 +621,7 @@ static void mtk_mdp_m2m_buf_queue(struct vb2_buffer *vb)
v4l2_m2m_buf_queue(ctx->m2m_ctx, to_vb2_v4l2_buffer(vb));
 }
 
-static struct vb2_ops mtk_mdp_m2m_qops = {
+static const struct vb2_ops mtk_mdp_m2m_qops = {
.queue_setup = mtk_mdp_m2m_queue_setup,
.buf_prepare = mtk_mdp_m2m_buf_prepare,
.buf_queue   = mtk_mdp_m2m_buf_queue,
-- 
2.5.0

[PATCH] pxa_camera: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/pxa_camera.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/pxa_camera.c 
b/drivers/media/platform/pxa_camera.c
index 3990951..5c3b7cf 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -1557,7 +1557,7 @@ static void pxac_vb2_stop_streaming(struct vb2_queue *vq)
pxa_camera_wakeup(pcdev, buf, VB2_BUF_STATE_ERROR);
 }
 
-static struct vb2_ops pxac_vb2_ops = {
+static const struct vb2_ops pxac_vb2_ops = {
.queue_setup= pxac_vb2_queue_setup,
.buf_init   = pxac_vb2_init,
.buf_prepare= pxac_vb2_prepare,
-- 
2.5.0

[PATCH] davinci: vpif_capture: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/davinci/vpif_capture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index d78580f..67fa53d 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -312,7 +312,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
spin_unlock_irqrestore(>irqlock, flags);
 }
 
-static struct vb2_ops video_qops = {
+static const struct vb2_ops video_qops = {
.queue_setup= vpif_buffer_queue_setup,
.buf_prepare= vpif_buffer_prepare,
.start_streaming= vpif_start_streaming,
-- 
2.5.0

Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Sylwester Nawrocki]

On 07/06/2017 12:24 PM, Jose Abreu wrote:
>>> +- edid-phandle: phandle to the EDID handler block.
>>
>> Could you make this property optional and when it is missing assume that 
>> device
>> corresponding to the parent node of this node handles EDID? This way we could
>> avoid having property pointing to the parent node.
>
> Hmm, this is for the CEC notifier. Do you mean I should grab the
> parent device for the notifier? This property is already optional
> if cec is not enabled though.
 
Yes, device associated with the parent node. Something like:

 - edid-phandle - phandle to the EDID handler block; if this property is
  not specified it is assumed that EDID is handled by device described 
  by parent node of the HDMI RX node

Not sure if it is any better than always requiring edid-phandle property,
even when it is pointing to the parent node. We would need a DT maintainer's
opinion on that.

--
Thanks,
Sylwester

[PATCH] atmel-isc: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index d653425..d4df3d4 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -873,7 +873,7 @@ static void isc_buffer_queue(struct vb2_buffer *vb)
spin_unlock_irqrestore(>dma_queue_lock, flags);
 }
 
-static struct vb2_ops isc_vb2_ops = {
+static const struct vb2_ops isc_vb2_ops = {
.queue_setup= isc_queue_setup,
.wait_prepare   = vb2_ops_wait_prepare,
.wait_finish= vb2_ops_wait_finish,
-- 
2.5.0

[PATCH] stm32-dcmi: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
b/drivers/media/platform/stm32/stm32-dcmi.c
index 83d32a5..24ef888 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -662,7 +662,7 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
dcmi->errors_count, dcmi->buffers_count);
 }
 
-static struct vb2_ops dcmi_video_qops = {
+static const struct vb2_ops dcmi_video_qops = {
.queue_setup= dcmi_queue_setup,
.buf_init   = dcmi_buf_init,
.buf_prepare= dcmi_buf_prepare,
-- 
2.5.0

[PATCH] rcar_fdp1: constify vb2_ops structure

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/rcar_fdp1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar_fdp1.c 
b/drivers/media/platform/rcar_fdp1.c
index 3ee51fc..3245bc4 100644
--- a/drivers/media/platform/rcar_fdp1.c
+++ b/drivers/media/platform/rcar_fdp1.c
@@ -2032,7 +2032,7 @@ static void fdp1_stop_streaming(struct vb2_queue *q)
}
 }
 
-static struct vb2_ops fdp1_qops = {
+static const struct vb2_ops fdp1_qops = {
.queue_setup = fdp1_queue_setup,
.buf_prepare = fdp1_buf_prepare,
.buf_queue   = fdp1_buf_queue,
-- 
2.5.0

[PATCH] st-delta: constify vb2_ops structures

[Gustavo A. R. Silva]

Check for vb2_ops structures that are only stored in the ops field of a
vb2_queue structure. That field is declared const, so vb2_ops structures
that have this property can be declared as const also.

This issue was detected using Coccinelle and the following semantic patch:

@r disable optional_qualifier@
identifier i;
position p;
@@
static struct vb2_ops i@p = { ... };

@ok@
identifier r.i;
struct vb2_queue e;
position p;
@@
e.ops = @p;

@bad@
position p != {r.p,ok.p};
identifier r.i;
struct vb2_ops e;
@@
e@i@p

@depends on !bad disable optional_qualifier@
identifier r.i;
@@
static
+const
struct vb2_ops i = { ... };

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/media/platform/sti/delta/delta-v4l2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sti/delta/delta-v4l2.c 
b/drivers/media/platform/sti/delta/delta-v4l2.c
index c6f2e24..ff9850e 100644
--- a/drivers/media/platform/sti/delta/delta-v4l2.c
+++ b/drivers/media/platform/sti/delta/delta-v4l2.c
@@ -1574,7 +1574,7 @@ static void delta_vb2_frame_stop_streaming(struct 
vb2_queue *q)
 }
 
 /* VB2 queue ops */
-static struct vb2_ops delta_vb2_au_ops = {
+static const struct vb2_ops delta_vb2_au_ops = {
.queue_setup = delta_vb2_au_queue_setup,
.buf_prepare = delta_vb2_au_prepare,
.buf_queue = delta_vb2_au_queue,
@@ -1584,7 +1584,7 @@ static struct vb2_ops delta_vb2_au_ops = {
.stop_streaming = delta_vb2_au_stop_streaming,
 };
 
-static struct vb2_ops delta_vb2_frame_ops = {
+static const struct vb2_ops delta_vb2_frame_ops = {
.queue_setup = delta_vb2_frame_queue_setup,
.buf_prepare = delta_vb2_frame_prepare,
.buf_finish = delta_vb2_frame_finish,
-- 
2.5.0

Re: [PATCH v1 1/4] dt-bindings: media: mtk-cir: Add support for MT7622 SoC

[Rob Herring]

On Fri, Jun 30, 2017 at 02:03:04PM +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> Document the devicetree bindings for CIR on MediaTek MT7622
> SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  Documentation/devicetree/bindings/media/mtk-cir.txt | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Acked-by: Rob Herring 

Re: [PATCH v2] [media] uvcvideo: Prevent heap overflow in uvc driver

[Guenter Roeck]

On Fri, Jun 30, 2017 at 09:21:56AM -0700, Guenter Roeck wrote:
> The size of uvc_control_mapping is user controlled leading to a
> potential heap overflow in the uvc driver. This adds a check to verify
> the user provided size fits within the bounds of the defined buffer
> size.
> 
> Originally-from: Richard Simmons 
> Signed-off-by: Guenter Roeck 

Any comments ?

Thanks,
Guenter

> ---
> Fixes CVE-2017-0627.
> 
> v2: Combination of v1 with the fix suggested by Richard Simmons
> Perform validation after uvc_ctrl_fill_xu_info()
> Take into account that ctrl->info.size is in bytes
> Also validate mapping->size
> 
>  drivers/media/usb/uvc/uvc_ctrl.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c 
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index c2ee6e39fd0c..d3e3164f43fd 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2002,6 +2002,13 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>   goto done;
>   }
>  
> + /* validate that the user provided bit-size and offset is valid */
> + if (mapping->size > 32 ||
> + mapping->offset + mapping->size > ctrl->info.size * 8) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
>   list_for_each_entry(map, >info.mappings, list) {
>   if (mapping->id == map->id) {
>   uvc_trace(UVC_TRACE_CONTROL, "Can't add mapping '%s', "
> -- 
> 2.7.4
> 

Re: [PATCH v6] media: platform: Renesas IMR driver

[Sergei Shtylyov]

Hello!

On 07/03/2017 03:43 PM, Hans Verkuil wrote:


Index: media_tree/Documentation/media/v4l-drivers/rcar_imr.rst
===
--- /dev/null
+++ media_tree/Documentation/media/v4l-drivers/rcar_imr.rst
@@ -0,0 +1,86 @@
+Renesas R-Car Image Rendeder (IMR) Driver


Rendeder -> Renderer


   Oops, sorry. :-)


+=
+
+This file documents some driver-specific aspects of the IMR driver, such as
+driver-specific ioctls.
+
+The ioctl reference
+~~~
+
+VIDIOC_IMR_MESH - Set mapping data
+^^
+
+Argument: struct imr_map_desc
+
+**Description**:
+
+This ioctl sets up the mesh using which the input frames will be


s/using/through/


+transformed into the output frames. The mesh can be strictly rectangular
+(when IMR_MAP_MESH bit is set in imr_map_desc::type) or arbitrary (when
+that bit is not set).
+
+A rectangular mesh consists of the imr_mesh structure followed by M*N
+vertex objects (where M is imr_mesh::rows and N is imr_mesh::columns).
+In case either IMR_MAP_AUTOSG or IMR_MAP_AUTODG bits were set in
+imr_map_desc::type, imr_mesh::{x|y}0 specify the coordinates of the top
+left corner of the auto-generated mesh and imr_mesh::d{x|y} specify the
+mesh's X/Y steps.


What if any of the other types are used like IMR_MAP_LUCE?


   IMR_MAP_LUCE only affects the vertex object.


Is this documented in a Renesas datasheet?


   Yes.


If so, add a reference to that in this
documentation.


   Unfortunately it's not publicly available.


+
+An arbitrary mesh consists of the imr_vbo structure followed by N
+triangle objects (where N is imr_vbo::num), consisting of 3 vertex
+objects each.
+
+A vertex object has a complex structure:
+
+.. code-block:: none
+
+__u16vvertical   \ source coordinates (only present
+__u16uhorizontal / if IMR_MAP_AUTOSG isn't set)
+__u16Yvertical   \ destination coordinates (only here
+__u16Xhorizontal / if IMR_MAP_AUTODG isn't set)
+__s8lofstoffset \  luminance correction parameters
+__u8lscalscale   > (only present if IMR_MAP_LUCE
+__u16reserved   /  is set)
+__s8vrofsV value offset \  hue correction parameters
+__u8vrsclV value scale   \ (only present if IMR_MAP_CLCE
+__s8ubofsU value offset  / is set)
+__u8ubsclU value scale  /


Is this the internal structure? Or something that userspace has to fill in?


   Yes, the user space have to pass that to the driver which constructs the 
display lists using these data.



It's not clear at all.

I recommend giving a few code examples of how this should be used.


   Konstantin, can we give some examples?


+
+**Return value**:
+
+On success 0 is returned. On error -1 is returned and errno is set
+appropriately.
+
+**Data types**:
+
+.. code-block:: none
+
+* struct imr_map_desc
+
+__u32typemapping types


This is a bitmask? If so, what combination of bits are allowed?


   Yes, bitmask.


+__u32sizetotal size of the mesh structure
+__u64datamap-specific user-pointer
+
+IMR_MAP_MESHregular mesh specification
+IMR_MAP_AUTOSGauto-generated source coordinates
+IMR_MAP_AUTODGauto-generated destination coordinates
+IMR_MAP_LUCEluminance correction flag
+IMR_MAP_CLCEchromacity correction flag


You probably mean 'chroma'. 'chromacity' isn't a word.


   But it's recognized by all online translators I've tried. :-)


+IMR_MAP_TCMvertex clockwise-mode order
+IMR_MAP_UVDPOR(n)source coordinate decimal point position
+IMR_MAP_DDPdestination coordinate sub-pixel mode
+IMR_MAP_YLDPO(n)luminance correction offset decimal point
+position
+IMR_MAP_UBDPO(n)chromacity (U) correction offset decimal point
+position
+IMR_MAP_VRDPO(n)chromacity (V) correction offset decimal point
+position


There is no documentation what how these types relate to IMR_MAP_MESH and
IMR_MAP_AUTOS/DG.


   They are basically orthogonal, IIRC.


+
+* struct imr_meshregular mesh specification
+
+__u16rows, columnsrectangular mesh sizes
+__u16x0, y0, dx, dyauto-generated mesh parameters
+
+* struct imr_vbovertex-buffer-object (VBO) descriptor
+
+__u16numnumber of triangles


Sorry, this needs more work.


   Sigh, everybody hates writing docs, I guess... :-)


Regards,

Hans


MBR, Sergei

[PATCH] staging: atomisp: gc2235: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  107541360   4   121182f56
drivers/staging/media/atomisp/i2c/gc2235.o

File size After adding 'const':
   textdata bss dec hex filename
  108181296   4   121182f56
drivers/staging/media/atomisp/i2c/gc2235.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/gc2235.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/gc2235.c 
b/drivers/staging/media/atomisp/i2c/gc2235.c
index 50f4317..48ca23b 100644
--- a/drivers/staging/media/atomisp/i2c/gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/gc2235.c
@@ -1183,7 +1183,7 @@ static int gc2235_probe(struct i2c_client *client,
return ret;
 }
 
-static struct acpi_device_id gc2235_acpi_match[] = {
+static const struct acpi_device_id gc2235_acpi_match[] = {
{ "INT33F8" },
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: mt9m114: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  151482640   8   177964584
drivers/staging/media/atomisp/i2c/mt9m114.o

File size After adding 'const':
   textdata bss dec hex filename
  152442512   8   177644564
drivers/staging/media/atomisp/i2c/mt9m114.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/mt9m114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c 
b/drivers/staging/media/atomisp/i2c/mt9m114.c
index ced175c..68980ab 100644
--- a/drivers/staging/media/atomisp/i2c/mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
@@ -1928,7 +1928,7 @@ static int mt9m114_probe(struct i2c_client *client,
 
 MODULE_DEVICE_TABLE(i2c, mt9m114_id);
 
-static struct acpi_device_id mt9m114_acpi_match[] = {
+static const struct acpi_device_id mt9m114_acpi_match[] = {
{ "INT33F0" },
{ "CRMT1040" },
{},
-- 
2.7.4

[PATCH] staging: atomisp: ov5693: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  207293264   0   239935db9
drivers/staging/media/atomisp/i2c/ov5693/ov5693.o

File size After adding 'const':
   textdata bss dec hex filename
  207933200   0   239935db9
drivers/staging/media/atomisp/i2c/ov5693/ov5693.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/ov5693/ov5693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c 
b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
index 5e9dafe..2c79678 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.c
@@ -2032,7 +2032,7 @@ static int ov5693_probe(struct i2c_client *client,
 
 MODULE_DEVICE_TABLE(i2c, ov5693_id);
 
-static struct acpi_device_id ov5693_acpi_match[] = {
+static const struct acpi_device_id ov5693_acpi_match[] = {
{"INT33BE"},
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: ov2722: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  147711880   0   16651410b 
drivers/staging/media/atomisp/i2c/ov2722.o

File size After adding 'const':
   textdata bss dec hex filename
  148351816   0   16651410b 
drivers/staging/media/atomisp/i2c/ov2722.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/ov2722.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov2722.c 
b/drivers/staging/media/atomisp/i2c/ov2722.c
index b7afade..10094ac 100644
--- a/drivers/staging/media/atomisp/i2c/ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/ov2722.c
@@ -1337,7 +1337,7 @@ static int ov2722_probe(struct i2c_client *client,
 
 MODULE_DEVICE_TABLE(i2c, ov2722_id);
 
-static struct acpi_device_id ov2722_acpi_match[] = {
+static const struct acpi_device_id ov2722_acpi_match[] = {
{ "INT33FB" },
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: gc0310: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  102971888   0   121852f99 
drivers/staging/media/atomisp/i2c/gc0310.o

File size After adding 'const':
   textdata bss dec hex filename
  103611824   0   121852f99 
drivers/staging/media/atomisp/i2c/gc0310.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/gc0310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/gc0310.c 
b/drivers/staging/media/atomisp/i2c/gc0310.c
index 1ec616a..c8162bb 100644
--- a/drivers/staging/media/atomisp/i2c/gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/gc0310.c
@@ -1453,7 +1453,7 @@ static int gc0310_probe(struct i2c_client *client,
return ret;
 }
 
-static struct acpi_device_id gc0310_acpi_match[] = {
+static const struct acpi_device_id gc0310_acpi_match[] = {
{"XXGC0310"},
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: ov8858: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  238048448   0   322527dfc 
drivers/staging/media/atomisp/i2c/ov8858.o

File size After adding 'const':
   textdata bss dec hex filename
  238688384   0   322527dfc 
drivers/staging/media/atomisp/i2c/ov8858.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/ov8858.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov8858.c 
b/drivers/staging/media/atomisp/i2c/ov8858.c
index 9574bc4..43e1638 100644
--- a/drivers/staging/media/atomisp/i2c/ov8858.c
+++ b/drivers/staging/media/atomisp/i2c/ov8858.c
@@ -2189,7 +2189,7 @@ static const struct i2c_device_id ov8858_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, ov8858_id);
 
-static struct acpi_device_id ov8858_acpi_match[] = {
+static const struct acpi_device_id ov8858_acpi_match[] = {
{"INT3477"},
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: ov2680: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
  124663120   8   155943cea 
drivers/staging/media/atomisp/i2c/ov2680.o

File size After adding 'const':
   textdata bss dec hex filename
  125303056   8   155943cea 
drivers/staging/media/atomisp/i2c/ov2680.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/ov2680.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov2680.c 
b/drivers/staging/media/atomisp/i2c/ov2680.c
index 5660910..9671876 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/ov2680.c
@@ -1519,7 +1519,7 @@ static int ov2680_probe(struct i2c_client *client,
return ret;
 }
 
-static struct acpi_device_id ov2680_acpi_match[] = {
+static const struct acpi_device_id ov2680_acpi_match[] = {
{"XXOV2680"},
{},
 };
-- 
2.7.4

[PATCH] staging: atomisp: lm3554: constify acpi_device_id.

[Arvind Yadav]

acpi_device_id are not supposed to change at runtime. All functions
working with acpi_device_id provided by  work with
const acpi_device_id. So mark the non-const structs as const.

File size before:
   textdata bss dec hex filename
   53471920  2472911c7b 
drivers/staging/media/atomisp/i2c/lm3554.o

File size After adding 'const':
   textdata bss dec hex filename
   54111856  2472911c7b 
drivers/staging/media/atomisp/i2c/lm3554.o

Signed-off-by: Arvind Yadav 
---
 drivers/staging/media/atomisp/i2c/lm3554.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/lm3554.c 
b/drivers/staging/media/atomisp/i2c/lm3554.c
index dd9c9c3..9ba1037 100644
--- a/drivers/staging/media/atomisp/i2c/lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/lm3554.c
@@ -974,7 +974,7 @@ static const struct dev_pm_ops lm3554_pm_ops = {
.resume = lm3554_resume,
 };
 
-static struct acpi_device_id lm3554_acpi_match[] = {
+static const struct acpi_device_id lm3554_acpi_match[] = {
{ "INTCF1C" },
{},
 };
-- 
2.7.4

Re: [PATCH v2 08/19] media: camss: Add files which handle the video device nodes

[Hans Verkuil]

On 07/06/17 15:44, Todor Tomov wrote:
> Hello Hans,
> 
> Thank you for the time spent to review.
> 
> On 07/03/2017 02:32 PM, Hans Verkuil wrote:
>> On 06/19/2017 04:48 PM, Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>   drivers/media/platform/qcom/camss-8x16/video.c | 629 
>>> +
>>>   drivers/media/platform/qcom/camss-8x16/video.h |  64 +++
>>>   2 files changed, 693 insertions(+)
>>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
>>> b/drivers/media/platform/qcom/camss-8x16/video.c
>>> new file mode 100644
>>> index 000..07175d3
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>>> @@ -0,0 +1,629 @@
>>> +/*
>>> + * video.c
>>> + *
>>> + * Qualcomm MSM Camera Subsystem - V4L2 device node
>>> + *
>>> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>>> + * Copyright (C) 2015-2016 Linaro Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "video.h"
>>> +#include "camss.h"
>>> +
>>> +/*
>>> + * struct format_info - ISP media bus format information
>>> + * @code: V4L2 media bus format code
>>> + * @pixelformat: V4L2 pixel format FCC identifier
>>> + * @bpp: Bits per pixel when stored in memory
>>> + */
>>> +static const struct format_info {
>>> +u32 code;
>>> +u32 pixelformat;
>>> +unsigned int bpp;
>>> +} formats[] = {
>>> +{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
>>> +{ MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
>>> +{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
>>> +{ MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
>>> +{ MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
>>> +{ MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
>>> +{ MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
>>> +{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
>>> +{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
>>> +{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
>>> +{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
>>> +{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
>>> +{ MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
>>> +{ MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
>>> +{ MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
>>> +{ MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
>>> +};
>>> +
>>> +/* 
>>> -
>>> + * Helper functions
>>> + */
>>> +
>>> +/*
>>> + * video_mbus_to_pix_mp - Convert v4l2_mbus_framefmt to 
>>> v4l2_pix_format_mplane
>>> + * @mbus: v4l2_mbus_framefmt format (input)
>>> + * @pix: v4l2_pix_format_mplane format (output)
>>> + *
>>> + * Fill the output pix structure with information from the input mbus 
>>> format.
>>> + *
>>> + * Return 0 on success or a negative error code otherwise
>>> + */
>>> +static unsigned int video_mbus_to_pix_mp(const struct v4l2_mbus_framefmt 
>>> *mbus,
>>> + struct v4l2_pix_format_mplane *pix)
>>> +{
>>> +unsigned int i;
>>> +u32 bytesperline;
>>> +
>>> +memset(pix, 0, sizeof(*pix));
>>> +pix->width = mbus->width;
>>> +pix->height = mbus->height;
>>> +
>>> +for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>>> +if (formats[i].code == mbus->code)
>>> +break;
>>> +}
>>> +
>>> +if (WARN_ON(i == ARRAY_SIZE(formats)))
>>> +return -EINVAL;
>>> +
>>> +pix->pixelformat = formats[i].pixelformat;
>>> +pix->num_planes = 1;
>>> +bytesperline = pix->width * formats[i].bpp / 8;
>>> +bytesperline = ALIGN(bytesperline, 8);
>>> +pix->plane_fmt[0].bytesperline = bytesperline;
>>> +pix->plane_fmt[0].sizeimage = bytesperline * pix->height;
>>> +pix->colorspace = mbus->colorspace;
>>> +pix->field = mbus->field;
>>> +
>>> +return 0;
>>> +}
>>
>> Can you add a v4l2_fill_pix_format_mplane and a v4l2_fill_mbus_format_mplane
>> static inline to media/v4l2-mediabus.h?
>>
>> And use v4l2_fill_pix_format_mplane() here?
> 
> The problem is what to do with the mbus code <=> pixelformat matching? In 

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 11:27 PM, Arnd Bergmann  wrote:
> On Thu, Jul 6, 2017 at 4:06 PM, Tomasz Figa  wrote:
>> On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann  wrote:
>>> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa  wrote:
 On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa  wrote:
>>>
> On the other hand, if it's strictly about base/dma-mapping, we might
> not need it indeed. The driver could call iommu-dma helpers directly,
> without the need to provide its own DMA ops. One caveat, though, we
> are not able to obtain coherent (i.e. uncached) memory with this
> approach, which might have some performance effects and complicates
> the code, that would now need to flush caches even for some small
> internal buffers.

 I think I should add a bit of explanation here:
  1) the device is non-coherent with CPU caches, even on x86,
  2) it looks like x86 does not have non-coherent DMA ops, (but it
 might be something that could be fixed)
>>>
>>> I don't understand what this means here. The PCI on x86 is always
>>> cache-coherent, so why is the device not?
>>>
>>> Do you mean that the device has its own caches that may need
>>> flushing to make the device cache coherent with the CPU cache,
>>> rather than flushing the CPU caches?
>>
>> Sakari might be able to explain this with more technical details, but
>> generally the device is not a standard PCI device one might find on
>> existing x86 systems.
>>
>> It is some kind of embedded subsystem that behaves mostly like a PCI
>> device, with certain exceptions, one being the lack of coherency with
>> CPU caches, at least for certain parts of the subsystem. The reference
>> vendor code disables the coherency completely, for reasons not known
>> to me, but AFAICT this is the preferred operating mode, possibly due
>> to performance effects (this is a memory-heavy image processing
>
> Ok, got it. I think something similar happens on integrated GPUs for
> a certain CPU family. The DRM code has its own ways of dealing with
> this kind of device. If you find that the hardware to be closely
> related (either the implementation, or the location on the internal
> buses) to the GPU on this machine, I'd recommend having a look
> in drivers/gpu/drm to see how it's handled there, and if that code could
> be shared.

I think it's not closely related, but might be a very similar case.

Still, DRM is very liberal in terms of not using common code for doing
things, while V4L2 tries to makes things generic as much as possible.
There is already the vb2_dma_contig backend, which allocates coherent
memory (in case of V4L2-allocated buffers), manages caches (in case of
userptr or DMA-buf buffers) and so on for you. If we can't have the
DMA ops do the right thing, the code there is essentially useless and
you are left with vb2_dma_sg that uses a page allocator and gives the
driver sg tables (it actually can also do cache management for you,
but since dma_sync_sg_*() is essentially a no-op on x86, the driver
would have to do it on its own).

Best regards,
Tomasz

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Arnd Bergmann]

On Thu, Jul 6, 2017 at 4:06 PM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann  wrote:
>> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa  wrote:
>>> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa  wrote:
>>
 On the other hand, if it's strictly about base/dma-mapping, we might
 not need it indeed. The driver could call iommu-dma helpers directly,
 without the need to provide its own DMA ops. One caveat, though, we
 are not able to obtain coherent (i.e. uncached) memory with this
 approach, which might have some performance effects and complicates
 the code, that would now need to flush caches even for some small
 internal buffers.
>>>
>>> I think I should add a bit of explanation here:
>>>  1) the device is non-coherent with CPU caches, even on x86,
>>>  2) it looks like x86 does not have non-coherent DMA ops, (but it
>>> might be something that could be fixed)
>>
>> I don't understand what this means here. The PCI on x86 is always
>> cache-coherent, so why is the device not?
>>
>> Do you mean that the device has its own caches that may need
>> flushing to make the device cache coherent with the CPU cache,
>> rather than flushing the CPU caches?
>
> Sakari might be able to explain this with more technical details, but
> generally the device is not a standard PCI device one might find on
> existing x86 systems.
>
> It is some kind of embedded subsystem that behaves mostly like a PCI
> device, with certain exceptions, one being the lack of coherency with
> CPU caches, at least for certain parts of the subsystem. The reference
> vendor code disables the coherency completely, for reasons not known
> to me, but AFAICT this is the preferred operating mode, possibly due
> to performance effects (this is a memory-heavy image processing

Ok, got it. I think something similar happens on integrated GPUs for
a certain CPU family. The DRM code has its own ways of dealing with
this kind of device. If you find that the hardware to be closely
related (either the implementation, or the location on the internal
buses) to the GPU on this machine, I'd recommend having a look
in drivers/gpu/drm to see how it's handled there, and if that code could
be shared.

Arnd

[PATCH] cec: only increase the seqnr if CEC_TRANSMIT would return 0

[Hans Verkuil]

The transmit code would increase the sequence number first thing, even though
CEC_TRANSMIT would return an error due to a malformatted cec_msg struct later
on.

While valid behavior, this had the disadvantage of producing holes in the
sequence list that made debugging harder.

Only increase the sequence number when the whole message is validated.
When debugging (i.e. with cec-ctl -M) the sequence numbering is now nicely
increasing by 1 per message.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index bf45977b2823..66cd18d1de9e 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -630,9 +630,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
msg->tx_nack_cnt = 0;
msg->tx_low_drive_cnt = 0;
msg->tx_error_cnt = 0;
-   msg->sequence = ++adap->sequence;
-   if (!msg->sequence)
-   msg->sequence = ++adap->sequence;
+   msg->sequence = 0;

if (msg->reply && msg->timeout == 0) {
/* Make sure the timeout isn't 0. */
@@ -671,6 +669,9 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
msg->tx_status = CEC_TX_STATUS_NACK |
 CEC_TX_STATUS_MAX_RETRIES;
msg->tx_nack_cnt = 1;
+   msg->sequence = ++adap->sequence;
+   if (!msg->sequence)
+   msg->sequence = ++adap->sequence;
return 0;
}
}
@@ -705,6 +706,10 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
if (!data)
return -ENOMEM;

+   msg->sequence = ++adap->sequence;
+   if (!msg->sequence)
+   msg->sequence = ++adap->sequence;
+
if (msg->len > 1 && msg->msg[1] == CEC_MSG_CDC_MESSAGE) {
msg->msg[2] = adap->phys_addr >> 8;
msg->msg[3] = adap->phys_addr & 0xff;

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 11:02 PM, Arnd Bergmann  wrote:
> On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa  wrote:
>> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa  wrote:
>
>>> On the other hand, if it's strictly about base/dma-mapping, we might
>>> not need it indeed. The driver could call iommu-dma helpers directly,
>>> without the need to provide its own DMA ops. One caveat, though, we
>>> are not able to obtain coherent (i.e. uncached) memory with this
>>> approach, which might have some performance effects and complicates
>>> the code, that would now need to flush caches even for some small
>>> internal buffers.
>>
>> I think I should add a bit of explanation here:
>>  1) the device is non-coherent with CPU caches, even on x86,
>>  2) it looks like x86 does not have non-coherent DMA ops, (but it
>> might be something that could be fixed)
>
> I don't understand what this means here. The PCI on x86 is always
> cache-coherent, so why is the device not?
>
> Do you mean that the device has its own caches that may need
> flushing to make the device cache coherent with the CPU cache,
> rather than flushing the CPU caches?

Sakari might be able to explain this with more technical details, but
generally the device is not a standard PCI device one might find on
existing x86 systems.

It is some kind of embedded subsystem that behaves mostly like a PCI
device, with certain exceptions, one being the lack of coherency with
CPU caches, at least for certain parts of the subsystem. The reference
vendor code disables the coherency completely, for reasons not known
to me, but AFAICT this is the preferred operating mode, possibly due
to performance effects (this is a memory-heavy image processing
subsystem).

Best regards,
Tomasz

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Arnd Bergmann]

On Thu, Jul 6, 2017 at 3:49 PM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa  wrote:

>> On the other hand, if it's strictly about base/dma-mapping, we might
>> not need it indeed. The driver could call iommu-dma helpers directly,
>> without the need to provide its own DMA ops. One caveat, though, we
>> are not able to obtain coherent (i.e. uncached) memory with this
>> approach, which might have some performance effects and complicates
>> the code, that would now need to flush caches even for some small
>> internal buffers.
>
> I think I should add a bit of explanation here:
>  1) the device is non-coherent with CPU caches, even on x86,
>  2) it looks like x86 does not have non-coherent DMA ops, (but it
> might be something that could be fixed)

I don't understand what this means here. The PCI on x86 is always
cache-coherent, so why is the device not?

Do you mean that the device has its own caches that may need
flushing to make the device cache coherent with the CPU cache,
rather than flushing the CPU caches?

  Arnd

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Arnd Bergmann]

On Thu, Jul 6, 2017 at 3:31 PM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann  wrote:
>> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa  wrote:
>>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa  wrote:
>>>
>>> Sorry, I intended to mean:
>>>
>>> IMHO re-implementing the code that's already there in videobuf2 again
>>> in the driver, only because, for no good reason mentioned as for now,
>>> having a loadable module providing DMA ops was disliked, would make no
>>> sense.
>>
>> Why would we need to duplicate that code? I would expect that the videobuf2
>> core can simply call the regular dma_mapping interfaces, and you handle the
>> IOPTE generation at the point when the buffer is handed off from the core
>> code to the device driver. Am I missing something?
>
> Well, for example, the iommu-dma helpers already implement all the
> IOVA management, SG iterations, IOMMU API calls, sanity checks and so
> on. There is a significant amount of common code.
>
> On the other hand, if it's strictly about base/dma-mapping, we might
> not need it indeed. The driver could call iommu-dma helpers directly,
> without the need to provide its own DMA ops.

Yes, that's what I meant: if using the IOMMU interface helps, I don't
see anything wrong with that, but using the iommu based
dma_map_ops seems like it may introduce more problems than
it solves.

Arnd

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 10:31 PM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann  wrote:
>> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa  wrote:
>>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa  wrote:
 On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann  wrote:
> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa  wrote:
>>

 I'd say that this is something that has been consistently tried to be
 avoided by V4L2 and that's why it's so tightly integrated with DMA
 mapping. IMHO re-implementing the code that's already there in
 videobuf2 again in the driver, only because, for no good reason
 mentioned as for now, having a loadable module providing DMA ops was
 disliked.
>>>
>>> Sorry, I intended to mean:
>>>
>>> IMHO re-implementing the code that's already there in videobuf2 again
>>> in the driver, only because, for no good reason mentioned as for now,
>>> having a loadable module providing DMA ops was disliked, would make no
>>> sense.
>>
>> Why would we need to duplicate that code? I would expect that the videobuf2
>> core can simply call the regular dma_mapping interfaces, and you handle the
>> IOPTE generation at the point when the buffer is handed off from the core
>> code to the device driver. Am I missing something?
>
> Well, for example, the iommu-dma helpers already implement all the
> IOVA management, SG iterations, IOMMU API calls, sanity checks and so
> on. There is a significant amount of common code.
>
> On the other hand, if it's strictly about base/dma-mapping, we might
> not need it indeed. The driver could call iommu-dma helpers directly,
> without the need to provide its own DMA ops. One caveat, though, we
> are not able to obtain coherent (i.e. uncached) memory with this
> approach, which might have some performance effects and complicates
> the code, that would now need to flush caches even for some small
> internal buffers.

I think I should add a bit of explanation here:
 1) the device is non-coherent with CPU caches, even on x86,
 2) it looks like x86 does not have non-coherent DMA ops, (but it
might be something that could be fixed)
 3) one technically could still use __get_vm_area() and map_vm_area(),
which _are_ exported, to create an uncached mapping. I'll leave it to
you to judge if it would be better than using the already available
generic helpers.

Best regards,
Tomasz

Re: [PATCH v2 08/19] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hello Hans,

Thank you for the time spent to review.

On 07/03/2017 02:32 PM, Hans Verkuil wrote:
> On 06/19/2017 04:48 PM, Todor Tomov wrote:
>> These files handle the video device nodes of the camss driver.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>   drivers/media/platform/qcom/camss-8x16/video.c | 629 
>> +
>>   drivers/media/platform/qcom/camss-8x16/video.h |  64 +++
>>   2 files changed, 693 insertions(+)
>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>   create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>
>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c 
>> b/drivers/media/platform/qcom/camss-8x16/video.c
>> new file mode 100644
>> index 000..07175d3
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c
>> @@ -0,0 +1,629 @@
>> +/*
>> + * video.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - V4L2 device node
>> + *
>> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015-2016 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "video.h"
>> +#include "camss.h"
>> +
>> +/*
>> + * struct format_info - ISP media bus format information
>> + * @code: V4L2 media bus format code
>> + * @pixelformat: V4L2 pixel format FCC identifier
>> + * @bpp: Bits per pixel when stored in memory
>> + */
>> +static const struct format_info {
>> +u32 code;
>> +u32 pixelformat;
>> +unsigned int bpp;
>> +} formats[] = {
>> +{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_PIX_FMT_UYVY, 16 },
>> +{ MEDIA_BUS_FMT_VYUY8_2X8, V4L2_PIX_FMT_VYUY, 16 },
>> +{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_PIX_FMT_YUYV, 16 },
>> +{ MEDIA_BUS_FMT_YVYU8_2X8, V4L2_PIX_FMT_YVYU, 16 },
>> +{ MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8 },
>> +{ MEDIA_BUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8 },
>> +{ MEDIA_BUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8 },
>> +{ MEDIA_BUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8 },
>> +{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10P, 10 },
>> +{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10P, 10 },
>> +{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10P, 10 },
>> +{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10P, 10 },
>> +{ MEDIA_BUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 },
>> +{ MEDIA_BUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12P, 12 },
>> +{ MEDIA_BUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12P, 12 },
>> +{ MEDIA_BUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12P, 12 }
>> +};
>> +
>> +/* 
>> -
>> + * Helper functions
>> + */
>> +
>> +/*
>> + * video_mbus_to_pix_mp - Convert v4l2_mbus_framefmt to 
>> v4l2_pix_format_mplane
>> + * @mbus: v4l2_mbus_framefmt format (input)
>> + * @pix: v4l2_pix_format_mplane format (output)
>> + *
>> + * Fill the output pix structure with information from the input mbus 
>> format.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static unsigned int video_mbus_to_pix_mp(const struct v4l2_mbus_framefmt 
>> *mbus,
>> + struct v4l2_pix_format_mplane *pix)
>> +{
>> +unsigned int i;
>> +u32 bytesperline;
>> +
>> +memset(pix, 0, sizeof(*pix));
>> +pix->width = mbus->width;
>> +pix->height = mbus->height;
>> +
>> +for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>> +if (formats[i].code == mbus->code)
>> +break;
>> +}
>> +
>> +if (WARN_ON(i == ARRAY_SIZE(formats)))
>> +return -EINVAL;
>> +
>> +pix->pixelformat = formats[i].pixelformat;
>> +pix->num_planes = 1;
>> +bytesperline = pix->width * formats[i].bpp / 8;
>> +bytesperline = ALIGN(bytesperline, 8);
>> +pix->plane_fmt[0].bytesperline = bytesperline;
>> +pix->plane_fmt[0].sizeimage = bytesperline * pix->height;
>> +pix->colorspace = mbus->colorspace;
>> +pix->field = mbus->field;
>> +
>> +return 0;
>> +}
> 
> Can you add a v4l2_fill_pix_format_mplane and a v4l2_fill_mbus_format_mplane
> static inline to media/v4l2-mediabus.h?
> 
> And use v4l2_fill_pix_format_mplane() here?

The problem is what to do with the mbus code <=> pixelformat matching? In this
version of the driver all used formats can be 1 to 1 matched however this is
not always true. E. g. in the later versions (patch 11/19: Format conversion 
support
using PIX 

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 9:23 PM, Arnd Bergmann  wrote:
> On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa  wrote:
>> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa  wrote:
>>> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann  wrote:
 On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa  wrote:
>
>>>
>>> I'd say that this is something that has been consistently tried to be
>>> avoided by V4L2 and that's why it's so tightly integrated with DMA
>>> mapping. IMHO re-implementing the code that's already there in
>>> videobuf2 again in the driver, only because, for no good reason
>>> mentioned as for now, having a loadable module providing DMA ops was
>>> disliked.
>>
>> Sorry, I intended to mean:
>>
>> IMHO re-implementing the code that's already there in videobuf2 again
>> in the driver, only because, for no good reason mentioned as for now,
>> having a loadable module providing DMA ops was disliked, would make no
>> sense.
>
> Why would we need to duplicate that code? I would expect that the videobuf2
> core can simply call the regular dma_mapping interfaces, and you handle the
> IOPTE generation at the point when the buffer is handed off from the core
> code to the device driver. Am I missing something?

Well, for example, the iommu-dma helpers already implement all the
IOVA management, SG iterations, IOMMU API calls, sanity checks and so
on. There is a significant amount of common code.

On the other hand, if it's strictly about base/dma-mapping, we might
not need it indeed. The driver could call iommu-dma helpers directly,
without the need to provide its own DMA ops. One caveat, though, we
are not able to obtain coherent (i.e. uncached) memory with this
approach, which might have some performance effects and complicates
the code, that would now need to flush caches even for some small
internal buffers.

Best regards,
Tomasz

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Arnd Bergmann]

On Thu, Jul 6, 2017 at 10:36 AM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa  wrote:
>> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann  wrote:
>>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa  wrote:

>>
>> I'd say that this is something that has been consistently tried to be
>> avoided by V4L2 and that's why it's so tightly integrated with DMA
>> mapping. IMHO re-implementing the code that's already there in
>> videobuf2 again in the driver, only because, for no good reason
>> mentioned as for now, having a loadable module providing DMA ops was
>> disliked.
>
> Sorry, I intended to mean:
>
> IMHO re-implementing the code that's already there in videobuf2 again
> in the driver, only because, for no good reason mentioned as for now,
> having a loadable module providing DMA ops was disliked, would make no
> sense.

Why would we need to duplicate that code? I would expect that the videobuf2
core can simply call the regular dma_mapping interfaces, and you handle the
IOPTE generation at the point when the buffer is handed off from the core
code to the device driver. Am I missing something?

   Arnd

Re: [PATCH 1/2] platform: Add Amlogic Meson AO CEC Controller driver

[Neil Armstrong]

On 07/06/2017 12:55 PM, Hans Verkuil wrote:
> On 07/06/17 12:27, Neil Armstrong wrote:
>> The Amlogic SoC embeds a standalone CEC controller, this patch adds a driver
>> for such controller.
>> The controller does not need HPD to be active, and could support up to max
>> 5 logical addresses, but only 1 is handled since the Suspend firmware can
>> make use of this unique logical address to wake up the device.
>>
>> The Suspend firmware configuration will be added in an other patchset.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/media/platform/Kconfig|  11 +
>>  drivers/media/platform/Makefile   |   2 +
>>  drivers/media/platform/meson/Makefile |   1 +
>>  drivers/media/platform/meson/ao-cec.c | 653 
>> ++
>>  4 files changed, 667 insertions(+)
>>  create mode 100644 drivers/media/platform/meson/Makefile
>>  create mode 100644 drivers/media/platform/meson/ao-cec.c
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index 1313cd5..1e67381 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -536,6 +536,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>  
>>  if CEC_PLATFORM_DRIVERS
>>  
>> +config VIDEO_MESON_AO_CEC
>> +tristate "Amlogic Meson AO CEC driver"
>> +depends on ARCH_MESON || COMPILE_TEST
>> +select CEC_CORE
>> +select CEC_NOTIFIER
>> +---help---
>> +  This is a driver for Amlogic Meson SoCs AO CEC interface. It uses the
>> +  generic CEC framework interface.
>> +  CEC bus is present in the HDMI connector and enables communication
>> +  between compatible devices.
>> +
>>  config VIDEO_SAMSUNG_S5P_CEC
>> tristate "Samsung S5P CEC driver"
>> depends on PLAT_S5P || ARCH_EXYNOS || COMPILE_TEST
>> diff --git a/drivers/media/platform/Makefile 
>> b/drivers/media/platform/Makefile
>> index 9beadc7..a52d7b6 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -86,3 +86,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP)   += mtk-mdp/
>>  obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)   += mtk-jpeg/
>>  
>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)  += qcom/venus/
>> +
>> +obj-y   += meson/
>> diff --git a/drivers/media/platform/meson/Makefile 
>> b/drivers/media/platform/meson/Makefile
>> new file mode 100644
>> index 000..597beb8
>> --- /dev/null
>> +++ b/drivers/media/platform/meson/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_VIDEO_MESON_AO_CEC)+= ao-cec.o
>> diff --git a/drivers/media/platform/meson/ao-cec.c 
>> b/drivers/media/platform/meson/ao-cec.c
>> new file mode 100644
>> index 000..26d7c3e8
>> --- /dev/null
>> +++ b/drivers/media/platform/meson/ao-cec.c
>> @@ -0,0 +1,653 @@
>> +/*
>> + * Driver for Amlogic Meson AO CEC Controller
>> + *
>> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved
>> + * Copyright (C) 2017 BayLibre, SAS
>> + * Author: Neil Armstrong 
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/* CEC Registers */
>> +
>> +/*
>> + * [2:1] cntl_clk
>> + *  - 0 = Disable clk (Power-off mode)
>> + *  - 1 = Enable gated clock (Normal mode)
>> + *  - 2 = Enable free-run clk (Debug mode)
>> + */
>> +#define CEC_GEN_CNTL_REG0x00
>> +
>> +#define CEC_GEN_CNTL_RESET  BIT(0)
>> +#define CEC_GEN_CNTL_CLK_DISABLE0
>> +#define CEC_GEN_CNTL_CLK_ENABLE 1
>> +#define CEC_GEN_CNTL_CLK_ENABLE_DBG 2
>> +#define CEC_GEN_CNTL_CLK_CTRL_MASK  GENMASK(2, 1)
>> +
>> +/*
>> + * [7:0] cec_reg_addr
>> + * [15:8] cec_reg_wrdata
>> + * [16] cec_reg_wr
>> + *  - 0 = Read
>> + *  - 1 = Write
>> + * [23] bus free
>> + * [31:24] cec_reg_rddata
>> + */
>> +#define CEC_RW_REG  0x04
>> +
>> +#define CEC_RW_ADDR GENMASK(7, 0)
>> +#define CEC_RW_WR_DATA  GENMASK(15, 8)
>> +#define CEC_RW_WRITE_EN BIT(16)
>> +#define CEC_RW_BUS_BUSY BIT(23)
>> +#define CEC_RW_RD_DATA  GENMASK(31, 24)
>> +
>> +/*
>> + * [1] tx intr
>> + * [2] rx intr
>> + */
>> +#define CEC_INTR_MASKN_REG  0x08
>> +#define CEC_INTR_CLR_REG0x0c
>> +#define CEC_INTR_STAT_REG   0x10
>> +
>> +#define CEC_INTR_TX BIT(1)
>> +#define CEC_INTR_RX BIT(2)
>> +
>> +/* CEC Commands */
>> +
>> +#define CEC_TX_MSG_0_HEADER 0x00
>> +#define CEC_TX_MSG_1_OPCODE 0x01
>> +#define CEC_TX_MSG_2_OP10x02
>> +#define CEC_TX_MSG_3_OP20x03
>> +#define CEC_TX_MSG_4_OP30x04
>> +#define CEC_TX_MSG_5_OP40x05
>> +#define CEC_TX_MSG_6_OP50x06
>> +#define CEC_TX_MSG_7_OP60x07
>> 

Re: [PATCH 7/8] omap3isp: Check for valid port in endpoints

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:18AM +0300, Sakari Ailus wrote:
> Check that we do have a valid port in an endpoint, return an error if not.
> 
> Signed-off-by: Sakari Ailus 

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/media/platform/omap3isp/isp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 2d45bf471c82..0676be725d7c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2081,7 +2081,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwnode,
>   default:
>   dev_warn(dev, "%s: invalid interface %u\n",
>to_of_node(fwnode)->full_name, vep.base.port);
> - break;
> + return -EINVAL;
>   }
>  
>   return 0;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 5/8] v4l: Add support for CSI-1 and CCP2 busses

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:16AM +0300, Sakari Ailus wrote:
> From: Sakari Ailus 
> 
> CCP2 and CSI-1, are older single data lane serial busses.
> 
> Signed-off-by: Sakari Ailus 
> Signed-off-by: Pavel Machek 

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/media/platform/pxa_camera.c  |  3 ++
>  drivers/media/platform/soc_camera/soc_mediabus.c |  3 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c| 58 
> +++-
>  include/media/v4l2-fwnode.h  | 19 
>  include/media/v4l2-mediabus.h|  4 ++
>  5 files changed, 76 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/pxa_camera.c 
> b/drivers/media/platform/pxa_camera.c
> index 399095170b6e..17e797c9559f 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -638,6 +638,9 @@ static unsigned int pxa_mbus_config_compatible(const 
> struct v4l2_mbus_config *cf
>   mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK 
> |
>V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
>   return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> + default:
> + __WARN();
> + return -EINVAL;
>   }
>   return 0;
>  }
> diff --git a/drivers/media/platform/soc_camera/soc_mediabus.c 
> b/drivers/media/platform/soc_camera/soc_mediabus.c
> index 57581f626f4c..43192d80beef 100644
> --- a/drivers/media/platform/soc_camera/soc_mediabus.c
> +++ b/drivers/media/platform/soc_camera/soc_mediabus.c
> @@ -508,6 +508,9 @@ unsigned int soc_mbus_config_compatible(const struct 
> v4l2_mbus_config *cfg,
>   mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK 
> |
>V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
>   return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> + default:
> + __WARN();
> + return -EINVAL;
>   }
>   return 0;
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d71dd3913cd9..76a88f210cb6 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -154,6 +154,31 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
>  
>  }
>  
> +void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwnode,
> +  struct v4l2_fwnode_endpoint *vep,
> +  u32 bus_type)
> +{
> +   struct v4l2_fwnode_bus_mipi_csi1 *bus = >bus.mipi_csi1;
> +   u32 v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "clock-inv", ))
> +   bus->clock_inv = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "strobe", ))
> +   bus->strobe = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "data-lanes", ))
> +   bus->data_lane = v;
> +
> +   if (!fwnode_property_read_u32(fwnode, "clock-lanes", ))
> +   bus->clock_lane = v;
> +
> +   if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2)
> +vep->bus_type = V4L2_MBUS_CCP2;
> +   else
> +vep->bus_type = V4L2_MBUS_CSI1;
> +}
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle
> @@ -187,17 +212,28 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>  
>   fwnode_property_read_u32(fwnode, "bus-type", _type);
>  
> - rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
> - if (rval)
> - return rval;
> - /*
> -  * Parse the parallel video bus properties only if none
> -  * of the MIPI CSI-2 specific properties were found.
> -  */
> - if (vep->bus.mipi_csi2.flags == 0)
> - v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep);
> -
> - return 0;
> + switch (bus_type) {
> + case V4L2_FWNODE_BUS_TYPE_GUESS:
> + rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
> + if (rval)
> + return rval;
> + /*
> +  * Parse the parallel video bus properties only if none
> +  * of the MIPI CSI-2 specific properties were found.
> +  */
> + if (vep->bus.mipi_csi2.flags == 0)
> + v4l2_fwnode_endpoint_parse_parallel_bus(fwnode, vep);
> +
> + return 0;
> + case V4L2_FWNODE_BUS_TYPE_CCP2:
> + case V4L2_FWNODE_BUS_TYPE_CSI1:
> + v4l2_fwnode_endpoint_parse_csi1_bus(fwnode, vep, bus_type);
> +
> + return 0;
> + default:
> + pr_warn("unsupported bus type %u\n", bus_type);
> + return -EINVAL;
> + }
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_parse);
>  
> diff --git a/include/media/v4l2-fwnode.h 

Re: [PATCH 3/8] v4l: fwnode: Call CSI2 bus csi2, not csi

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:14AM +0300, Sakari Ailus wrote:
> The function to parse CSI2 bus parameters was called
> v4l2_fwnode_endpoint_parse_csi_bus(), rename it as
> v4l2_fwnode_endpoint_parse_csi2_bus() in anticipation of CSI1/CCP2
> support.
> 
> Signed-off-by: Sakari Ailus 

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 153c53ca3925..8df26010d006 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,8 +28,8 @@
>  
>  #include 
>  
> -static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwnode,
> -   struct v4l2_fwnode_endpoint *vep)
> +static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
> +struct v4l2_fwnode_endpoint *vep)
>  {
>   struct v4l2_fwnode_bus_mipi_csi2 *bus = >bus.mipi_csi2;
>   bool have_clk_lane = false;
> @@ -176,7 +176,7 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>   memset(>bus_type, 0, sizeof(*vep) -
>  offsetof(typeof(*vep), bus_type));
>  
> - rval = v4l2_fwnode_endpoint_parse_csi_bus(fwnode, vep);
> + rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
>   if (rval)
>   return rval;
>   /*
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

Re: [PATCH 4/8] v4l: fwnode: Obtain data bus type from FW

[Sebastian Reichel]

Hi,

On Thu, Jul 06, 2017 at 02:00:15AM +0300, Sakari Ailus wrote:
> From: Sakari Ailus 
> 
> Just obtain it. It'll actually get used soon with CSI-1/CCP2.
> 
> Signed-off-by: Sakari Ailus 

Reviewed-by: Sebastian Reichel 

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 8df26010d006..d71dd3913cd9 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,6 +28,14 @@
>  
>  #include 
>  
> +enum v4l2_fwnode_bus_type {
> + V4L2_FWNODE_BUS_TYPE_GUESS = 0,
> + V4L2_FWNODE_BUS_TYPE_CSI2_CPHY,
> + V4L2_FWNODE_BUS_TYPE_CSI1,
> + V4L2_FWNODE_BUS_TYPE_CCP2,
> + NR_OF_V4L2_FWNODE_BUS_TYPE,
> +};
> +
>  static int v4l2_fwnode_endpoint_parse_csi2_bus(struct fwnode_handle *fwnode,
>  struct v4l2_fwnode_endpoint *vep)
>  {
> @@ -168,6 +176,7 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
>  int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwnode,
>  struct v4l2_fwnode_endpoint *vep)
>  {
> + u32 bus_type = 0;
>   int rval;
>  
>   fwnode_graph_parse_endpoint(fwnode, >base);
> @@ -176,6 +185,8 @@ int v4l2_fwnode_endpoint_parse(struct fwnode_handle 
> *fwnode,
>   memset(>bus_type, 0, sizeof(*vep) -
>  offsetof(typeof(*vep), bus_type));
>  
> + fwnode_property_read_u32(fwnode, "bus-type", _type);
> +
>   rval = v4l2_fwnode_endpoint_parse_csi2_bus(fwnode, vep);
>   if (rval)
>   return rval;
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

[PATCH v7 3/3] MAINTAINERS: Add ADV748x driver

[Kieran Bingham]

From: Kieran Bingham 

The ADV7481 is an integrated video decoder and combined HDMI/MHL
receiver.

Signed-off-by: Kieran Bingham 
Acked-by: Laurent Pinchart 
---
 MAINTAINERS | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c4be6d4af7d2..741da59b133a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -770,6 +770,12 @@ W: http://ez.analog.com/community/linux-device-drivers
 S: Supported
 F: drivers/media/i2c/adv7180.c
 
+ANALOG DEVICES INC ADV748X DRIVER
+M: Kieran Bingham 
+L: linux-media@vger.kernel.org
+S: Maintained
+F: drivers/media/i2c/adv748x/*
+
 ANALOG DEVICES INC ADV7511 DRIVER
 M: Hans Verkuil 
 L: linux-media@vger.kernel.org
-- 
git-series 0.9.1

[PATCH v7 1/3] media: adv748x: Add adv7481, adv7482 bindings

[Kieran Bingham]

From: Kieran Bingham 

Create device tree bindings documentation for the ADV748x.
The ADV748x supports both the ADV7481 and ADV7482 chips which
provide analogue decoding and HDMI receiving capabilities

Signed-off-by: Kieran Bingham 
Reviewed-by: Laurent Pinchart 

---
v6:
 - Clean up description and remove redundant text regarding optional
   nodes

v6.1:
 - Fix commit title

 Documentation/devicetree/bindings/media/i2c/adv748x.txt | 95 ++-
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/adv748x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt 
b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
new file mode 100644
index ..21ffb5ed8183
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
@@ -0,0 +1,95 @@
+* Analog Devices ADV748X video decoder with HDMI receiver
+
+The ADV7481 and ADV7482 are multi format video decoders with an integrated
+HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB
+from three input sources HDMI, analog and TTL.
+
+Required Properties:
+
+  - compatible: Must contain one of the following
+- "adi,adv7481" for the ADV7481
+- "adi,adv7482" for the ADV7482
+
+  - reg: I2C slave address
+
+Optional Properties:
+
+  - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
+"intrq3". All interrupts are optional. The "intrq3" 
interrupt
+is only available on the adv7481
+  - interrupts: Specify the interrupt lines for the ADV748x
+
+The device node must contain one 'port' child node per device input and output
+port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
+are numbered as follows.
+
+ Name  TypePort
+   ---
+ AIN0  sink0
+ AIN1  sink1
+ AIN2  sink2
+ AIN3  sink3
+ AIN4  sink4
+ AIN5  sink5
+ AIN6  sink6
+ AIN7  sink7
+ HDMI  sink8
+ TTL   sink9
+ TXA   source  10
+ TXB   source  11
+
+The digital output port nodes must contain at least one endpoint.
+
+Ports are optional if they are not connected to anything at the hardware level.
+
+Example:
+
+   video-receiver@70 {
+   compatible = "adi,adv7482";
+   reg = <0x70>;
+
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   interrupt-parent = <>;
+   interrupt-names = "intrq1", "intrq2";
+   interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
+<31 IRQ_TYPE_LEVEL_LOW>;
+
+   port@7 {
+   reg = <7>;
+
+   adv7482_ain7: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+
+   port@8 {
+   reg = <8>;
+
+   adv7482_hdmi: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+
+   port@10 {
+   reg = <10>;
+
+   adv7482_txa: endpoint {
+   clock-lanes = <0>;
+   data-lanes = <1 2 3 4>;
+   remote-endpoint = <_in>;
+   };
+   };
+
+   port@11 {
+   reg = <11>;
+
+   adv7482_txb: endpoint {
+   clock-lanes = <0>;
+   data-lanes = <1>;
+   remote-endpoint = <_in>;
+   };
+   };
+   };
-- 
git-series 0.9.1

[PATCH v7 2/3] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

From: Kieran Bingham 

Provide support for the ADV7481 and ADV7482.

The driver is modelled with 4 subdevices to allow simultaneous streaming
from the AFE (Analog front end) and HDMI inputs though two CSI TX
entities.

The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
to the TXB CSI bus.

The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
and an earlier rework by Niklas Söderlund.

Signed-off-by: Kieran Bingham 

---

v2:
 - Implement DT parsing
 - adv748x: Add CSI2 entity
 - adv748x: Rework pad allocations and fmts
 - Give AFE 8 input pads and move pad defines
 - Use the enums to ensure pads are referenced correctly.
 - adv748x: Rename AFE/HDMI entities
   Now they are 'just afe' and 'just hdmi'
 - Reorder the entity enum and structures
 - Added pad-format for the CSI2 entities
 - CSI2 s_stream pass through
 - CSI2 control pass through (with link following)

v3:
 - dt: Extend DT documentation to specify interrupt mappings
 - simplify adv748x_parse_dt
 - core: Add banner to header file describing ADV748x variants
 - Use entity structure pointers rather than global state pointers where
   possible
 - afe: Reduce indent on afe_status
 - hdmi: Add error checking to the bt->pixelclock values.
 - Remove all unnecessary pad checks: handled by core
 - Fix all probe cleanup paths
 - adv748x_csi2_probe() now fails if it has no endpoint
 - csi2: Fix small oneliners for is_txa and get_remote_sd()
 - csi2: Fix checkpatch warnings
 - csi2: Fix up s_stream pass-through
 - csi2: Fix up Pixel Rate passthrough
 - csi2: simplify adv748x_csi2_get_pad_format()
 - Remove 'async notifiers' from AFE/HDMI
   Using async notifiers was overkill, when we have access to the
   subdevices internally and can register them directly.
 - Use state lock in control handlers and clean up s_ctrls
 - remove _interruptible mutex locks

v4:
 - all: Convert hex 0xXX to lowercase
 - all: Use defines instead of hardcoded register values
 - all: Use regmap
 - afe, csi2, hdmi: _probe -> _init
 - afe, csi2, hdmi: _remove -> _cleanup
 - afe, hdmi: Convert pattern generator to a control
 - afe, hdmi: get/set-fmt refactor
 - afe, hdmi, csi2: Convert to internal calls for pixelrate
 - afe: Allow the AFE to configure the Input Select from DT
 - afe: Reduce indent on adv748x_afe_status switch
 - afe: Remove ununsed macro definitions AIN0-7
 - afe: Remove extraneous control checks handled by core
 - afe: Comment fixups
 - afe: Rename error label
 - afe: Correct control names on the SDP
 - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
 - core: adv748x_parse_dt references to nodes, and catch multiple
   endpoints in a port.
 - core: adv748x_dt_cleanup to simplify releasing DT nodes
 - core: adv748x_print_info renamed to adv748x_identify_chip
 - core: reorganise ordering of probe sequence
 - core: No need for of_match_ptr
 - core: Fix up i2c read/writes (regmap still on todo list)
 - core: Set specific functions from the entities on subdev-init
 - core: Use kzalloc for state instead of devm
 - core: Improve probe error reporting
 - core: Track unknown BIT(6) in tx{a,b}_power
 - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
 - csi2: -EPIPE instead of -ENODEV
 - csi2: adv_dbg, instead of adv_info
 - csi2: adv748x_csi2_set_format fix
 - csi2: remove async notifier and sd member variables
 - csi2: register links from the CSI2
 - csi2: create virtual channel helper function
 - dt: Remove numbering from endpoints
 - dt: describe ports and interrupts as optional
 - dt: Re-tab
 - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
 - hdmi: fix adv748x_hdmi_read_pixelclock return checks
 - hdmi: improve adv748x_hdmi_set_video_timings
 - hdmi: Fix tmp variable as polarity
 - hdmi: Improve adv748x_hdmi_s_stream
 - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
 - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
 - hdmi: Add locking to adv748x_hdmi_g_dv_timings
   writes and locking
 - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
 - hdmi: Use CP in control register naming to match component processor
 - hdmi: clean up adv748x_hdmi_query_dv_timings()
 - KConfig: Fix up dependency and capitalisation

v5:
 - afe,hdmi: _set_pixelrate -> _propagate_pixelrate
 - hdmi: Fix arm32 compilation failure : Use DIV_ROUND_CLOSEST_ULL
 - core: remove unused link functions
 - csi2: Use immutable links for HDMI->TXA, AFE->TXB

v6:
 - hdmi: Provide EDID support
 - afe: Fix InLock inversion bug
 - afe: Prevent autodetection of input format except in querystd
 - afe,hdmi: Improve pattern generator control strings
 - hdmi: Remove interlaced support capability (it's untested)

v7:
 - afe: Initialise std to V4L2_STD_NTSC_M and restore after query
 - afe: use V4L2_FIELD_ALTERNATE
 - hdmi: Define a minimum pixelclock value
 - hdmi: Remove interlaced formats as they 

[PATCH v7 0/3] ADV748x HDMI/Analog video receiver

[Kieran Bingham]

From: Kieran Bingham 

This is a driver for the Analog Devices ADV748x device, and follows on from a
previous posting by Niklas Söderlund [0] of an earlier incarnation of this
driver, and earlier versions posted by myself.

ADV748x
===
The ADV7481 and ADV7482 support two video pipelines which can run independently
of each other, with each pipeline terminating in a CSI-2 output: TXA (4-Lane)
and TXB (1-Lane)

The ADV7480 (Not included here), ADV7481, and ADV7482 are all derivatives,
with the following features

Analog   HDMI  MHL  4-Lane  1-Lane
  In  In CSI CSI
 ADV7480   XX X
 ADV7481  XXX X   X
 ADV7482  XX  X   X

Implementation
==

This driver creates 4 entities. AFE (CVBS/Analog In), HDMI, TXA and TXB.  At
probe time, the DT is parsed to identify the endpoints for each of these nodes,
and those are used for async matching of the CSI2 (TXA/TXB) entities in the
master driver. The HDMI and AFE entities are then registered after a successful
registration of both the CSI2 entities.

HDMI is statically linked to the TXA entity, while the AFE is statically linked
to the TXB entity. Routing the AFE through TXA is not supported.

Support for setting the EDID on HDMI is provided

(Known) Future Todo's
=

Further potential development areas include:
 - ADV7480 Support (No AFE)
 - MHL support (Not present on ADV7482)
 - CEC Support
 - Configurable I2C addressing
 - Interrupt handling for format changes and hotplug detect.

However, this driver is functional without the above, and these developments
can be written when required.

References
==
[0] http://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg05196.html

v1/RFC:
 - Initial posting

v2:
 - Reworked DT parsing and entities

v3:
 - Refreshed with lots of fixups from Sakari's review comments

v4:
 - Many changes all round, following Laurent's review and extensive development
 - Now uses regmap
 - AFE port numbering has been changed to match the entity pads

v5:
 - DT is now based on the latest salvator-common.dtsi
 - Entities are linked with immutable connections

v6:
 - EDID support added to HDMI
 - AFE no longer autodetects input format by default.

v7:
 - Comments from Hans addressed
 - AFE now uses FIELD_ALTERNATE
 - HDMI interlaced support removed (it's untested)

Kieran Bingham (3):
  media: adv748x: Add adv7481, adv7482 bindings
  media: i2c: adv748x: add adv748x driver
  MAINTAINERS: Add ADV748x driver

 Documentation/devicetree/bindings/media/i2c/adv748x.txt |  95 +-
 MAINTAINERS |   6 +-
 drivers/media/i2c/Kconfig   |  12 +-
 drivers/media/i2c/Makefile  |   1 +-
 drivers/media/i2c/adv748x/Makefile  |   7 +-
 drivers/media/i2c/adv748x/adv748x-afe.c | 552 ++-
 drivers/media/i2c/adv748x/adv748x-core.c| 832 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c| 327 -
 drivers/media/i2c/adv748x/adv748x-hdmi.c| 768 -
 drivers/media/i2c/adv748x/adv748x.h | 425 +-
 10 files changed, 3025 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/adv748x.txt
 create mode 100644 drivers/media/i2c/adv748x/Makefile
 create mode 100644 drivers/media/i2c/adv748x/adv748x-afe.c
 create mode 100644 drivers/media/i2c/adv748x/adv748x-core.c
 create mode 100644 drivers/media/i2c/adv748x/adv748x-csi2.c
 create mode 100644 drivers/media/i2c/adv748x/adv748x-hdmi.c
 create mode 100644 drivers/media/i2c/adv748x/adv748x.h

base-commit: 2748e76ddb2967c4030171342ebdd3faa6a5e8e8
-- 
git-series 0.9.1

Re: [PATCH 1/2] platform: Add Amlogic Meson AO CEC Controller driver

[Hans Verkuil]

On 07/06/17 12:27, Neil Armstrong wrote:
> The Amlogic SoC embeds a standalone CEC controller, this patch adds a driver
> for such controller.
> The controller does not need HPD to be active, and could support up to max
> 5 logical addresses, but only 1 is handled since the Suspend firmware can
> make use of this unique logical address to wake up the device.
> 
> The Suspend firmware configuration will be added in an other patchset.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/media/platform/Kconfig|  11 +
>  drivers/media/platform/Makefile   |   2 +
>  drivers/media/platform/meson/Makefile |   1 +
>  drivers/media/platform/meson/ao-cec.c | 653 
> ++
>  4 files changed, 667 insertions(+)
>  create mode 100644 drivers/media/platform/meson/Makefile
>  create mode 100644 drivers/media/platform/meson/ao-cec.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd5..1e67381 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -536,6 +536,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>  
>  if CEC_PLATFORM_DRIVERS
>  
> +config VIDEO_MESON_AO_CEC
> + tristate "Amlogic Meson AO CEC driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + select CEC_CORE
> + select CEC_NOTIFIER
> + ---help---
> +   This is a driver for Amlogic Meson SoCs AO CEC interface. It uses the
> +   generic CEC framework interface.
> +   CEC bus is present in the HDMI connector and enables communication
> +   between compatible devices.
> +
>  config VIDEO_SAMSUNG_S5P_CEC
> tristate "Samsung S5P CEC driver"
> depends on PLAT_S5P || ARCH_EXYNOS || COMPILE_TEST
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 9beadc7..a52d7b6 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -86,3 +86,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP)+= mtk-mdp/
>  obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)+= mtk-jpeg/
>  
>  obj-$(CONFIG_VIDEO_QCOM_VENUS)   += qcom/venus/
> +
> +obj-y+= meson/
> diff --git a/drivers/media/platform/meson/Makefile 
> b/drivers/media/platform/meson/Makefile
> new file mode 100644
> index 000..597beb8
> --- /dev/null
> +++ b/drivers/media/platform/meson/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_MESON_AO_CEC) += ao-cec.o
> diff --git a/drivers/media/platform/meson/ao-cec.c 
> b/drivers/media/platform/meson/ao-cec.c
> new file mode 100644
> index 000..26d7c3e8
> --- /dev/null
> +++ b/drivers/media/platform/meson/ao-cec.c
> @@ -0,0 +1,653 @@
> +/*
> + * Driver for Amlogic Meson AO CEC Controller
> + *
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved
> + * Copyright (C) 2017 BayLibre, SAS
> + * Author: Neil Armstrong 
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* CEC Registers */
> +
> +/*
> + * [2:1] cntl_clk
> + *  - 0 = Disable clk (Power-off mode)
> + *  - 1 = Enable gated clock (Normal mode)
> + *  - 2 = Enable free-run clk (Debug mode)
> + */
> +#define CEC_GEN_CNTL_REG 0x00
> +
> +#define CEC_GEN_CNTL_RESET   BIT(0)
> +#define CEC_GEN_CNTL_CLK_DISABLE 0
> +#define CEC_GEN_CNTL_CLK_ENABLE  1
> +#define CEC_GEN_CNTL_CLK_ENABLE_DBG  2
> +#define CEC_GEN_CNTL_CLK_CTRL_MASK   GENMASK(2, 1)
> +
> +/*
> + * [7:0] cec_reg_addr
> + * [15:8] cec_reg_wrdata
> + * [16] cec_reg_wr
> + *  - 0 = Read
> + *  - 1 = Write
> + * [23] bus free
> + * [31:24] cec_reg_rddata
> + */
> +#define CEC_RW_REG   0x04
> +
> +#define CEC_RW_ADDR  GENMASK(7, 0)
> +#define CEC_RW_WR_DATA   GENMASK(15, 8)
> +#define CEC_RW_WRITE_EN  BIT(16)
> +#define CEC_RW_BUS_BUSY  BIT(23)
> +#define CEC_RW_RD_DATA   GENMASK(31, 24)
> +
> +/*
> + * [1] tx intr
> + * [2] rx intr
> + */
> +#define CEC_INTR_MASKN_REG   0x08
> +#define CEC_INTR_CLR_REG 0x0c
> +#define CEC_INTR_STAT_REG0x10
> +
> +#define CEC_INTR_TX  BIT(1)
> +#define CEC_INTR_RX  BIT(2)
> +
> +/* CEC Commands */
> +
> +#define CEC_TX_MSG_0_HEADER  0x00
> +#define CEC_TX_MSG_1_OPCODE  0x01
> +#define CEC_TX_MSG_2_OP1 0x02
> +#define CEC_TX_MSG_3_OP2 0x03
> +#define CEC_TX_MSG_4_OP3 0x04
> +#define CEC_TX_MSG_5_OP4 0x05
> +#define CEC_TX_MSG_6_OP5 0x06
> +#define CEC_TX_MSG_7_OP6 0x07
> +#define CEC_TX_MSG_8_OP7 0x08
> +#define CEC_TX_MSG_9_OP8 0x09
> +#define CEC_TX_MSG_A_OP9 0x0A
> +#define 

Re: [PATCH 0/8] Prepare for CCP2 / CSI-1 support, omap3isp fixes

[Pavel Machek]

Hi!

> Most of these patches have been posted to the list in some form or other
> already but a lot has happened since. Thus reposting. There are more
> patches in my ccp2 branch but they're not quite ready as such, for the
> reasons discussed previously.

I'm using Sakari's ccp2 branch as a basis of camera support for
N900. camera-fw5-6 branch on kernel.org has the code, and it works
rather well.

Yes, there's more work to be done (finishing the support in omap3isp,
connecting focus, flash, all the userland support, ...), but this is
good basis and is ready now.

Thus (for the series)

Acked-by: Pavel Machek 
Tested-by: Pavel Machek 

Best regards,
Pavel

> 
> Pavel Machek (1):
>   smiapp: add CCP2 support
> 
> Sakari Ailus (7):
>   dt: bindings: Explicitly specify bus type
>   dt: bindings: Add strobe property for CCP2
>   v4l: fwnode: Call CSI2 bus csi2, not csi
>   v4l: fwnode: Obtain data bus type from FW
>   v4l: Add support for CSI-1 and CCP2 busses
>   omap3isp: Check for valid port in endpoints
>   omap3isp: Destroy CSI-2 phy mutexes in error and module removal


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: v4l2-fwnode: status, plans for merge, any branch to merge against?

[Pavel Machek]

Hi!

> > > > > I expect to have most of them in during the next merge window.
> > > > 
> > > > So git://linuxtv.org/media_tree.git branch master is the right one to
> > > > work one?
> > > 
> > > I also pushed the rebased ccp2 branch there:
> > > 
> > > 
> > > 
> > > It's now right on the top of media-tree master.
> > 
> > Is ccp2 branch expected to go into 4.13, too?
> 
> Hi Pavel,
> 
> What I've done is just rebased the ccp2 branch. In other words, the patches
> in that branch are no more ready than they were.

I thought they were ready even back then :-).

> To get these merged we should ideally
> 
> 1) Make sure there will be no regressions,

Well, all I have running recent kernels is N900. If ccp branch works
for you on N9, that's probably as much testing as we can get.

> 2) clean things up in the omap3isp; which resources are needed and when
> (e.g. regulators, PHY configuration) isn't clear at the moment and
> 
> 2) have one driver using the implementation.
> 
> At least 1) is needed. I think a number of framework patches could be
> mergeable before 2) and 3) are done. I can prepare a set later this week.
> But even that'd be likely for 4.14, not 4.13.

Yep, it is too late for v4.13 now. But getting stuff ready for v4.14
would be good.

I started looking through the patches; I believe they are safe, but it
is probably better to review the series you've just mailed.

The driver using the implementation -- yes, I have it all working on
n900 (incuding userland, I can actually take photos.) I can post the
series, or better link to kernel.org.

Right now, my goal would be to get sensor working on N900 with
mainline (without flash and focus).

I'd very much like any comment on patch attached below.

Age   Commit message (Expand)   Author  Files   Lines
2017-06-16   omap3isp: Destroy CSI-2 phy mutexes in error and module
2017-06-16  omap3isp: Skip CSI-2 receiver initialisation in CCP2
2017-06-16  omap3isp: Correctly put the last iterated endpoint
2017-06-16  omap3isp: Always initialise isp and mutex for csiphy1
2017-06-16  omap3isp: Return -EPROBE_DEFER if the required
2017-06-16 omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2
2017-06-16omap3isp: Make external sub-device bus configuration a
2017-06-15omap3isp: Parse CSI1 configuration from the device tree
2017-06-15omap3isp: Check for valid port in endpoints   Sakari
2017-06-15  omap3isp: Ignore endpoints with invalid configuration

# Nothing changes for bus_type == V4L2_MBUS_CSI2. FIXME: Is bus_type
  set correctly?

2017-06-15  smiapp: add CCP2 supportPavel Machek1

# bus_type will be guess, so no code changes on existing system:

2017-06-15  v4l: Add support for CSI-1 and CCP2 busses  Sakari

# Reads unused value -> can't break anything:

2017-06-13  v4l: fwnode: Obtain data bus type from FW   Sakari

# No code changes -> totally safe:

2017-06-13  v4l: fwnode: Call CSI2 bus csi2, not csiSakari
2017-06-13  dt: bindings: Add strobe property for CCP2  Sakari
2017-06-13  dt: bindings: Explicitly specify bus type

Best regards,
Pavel

commit 1220492dd4c1872c8036caa573680f95aabc69bc
Author: Pavel 
Date:   Tue Feb 28 12:02:26 2017 +0100

omap3isp: add CSI1 support

Use proper code path for csi1/ccp2 support.

Signed-off-by: Pavel Machek 

diff --git a/drivers/media/platform/omap3isp/ispccp2.c 
b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..47210b1 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1149,6 +1149,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
}
+   ccp2->phy = >isp_csiphy2;
} else if (isp->revision == ISP_REVISION_15_0) {
ccp2->phy = >isp_csiphy1;
}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c 
b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..862fdd3 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +197,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
}
 
if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+   || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
lanes = >bus.ccp2.lanecfg;
-   else
+   phy->num_data_lanes = 1;
+   } else
lanes = >bus.csi2.lanecfg;
 
/* Clock and data lanes verification */
@@ -302,13 +303,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
if (rval < 0)
goto done;
 
-   rval = csiphy_set_power(phy, 

[PATCH 2/2] dt-bindings: media: Add Amlogic Meson AO-CEC bindings

[Neil Armstrong]

The Amlogic SoCs embeds a standalone CEC Controller, this patch adds this
device bindings.

Signed-off-by: Neil Armstrong 
---
 .../devicetree/bindings/media/meson-ao-cec.txt | 28 ++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/meson-ao-cec.txt

diff --git a/Documentation/devicetree/bindings/media/meson-ao-cec.txt 
b/Documentation/devicetree/bindings/media/meson-ao-cec.txt
new file mode 100644
index 000..8671bdb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/meson-ao-cec.txt
@@ -0,0 +1,28 @@
+* Amlogic Meson AO-CEC driver
+
+The Amlogic Meson AO-CEC module is present is Amlogic SoCs and its purpose is
+to handle communication between HDMI connected devices over the CEC bus.
+
+Required properties:
+  - compatible : value should be following
+   "amlogic,meson-gx-ao-cec"
+
+  - reg : Physical base address of the IP registers and length of memory
+ mapped region.
+
+  - interrupts : AO-CEC interrupt number to the CPU.
+  - clocks : from common clock binding: handle to AO-CEC clock.
+  - clock-names : from common clock binding: must contain "core",
+ corresponding to entry in the clocks property.
+  - hdmi-phandle: phandle to the HDMI controller
+
+Example:
+
+cec_AO: cec@100 {
+   compatible = "amlogic,meson-gx-ao-cec";
+   reg = <0x0 0x00100 0x0 0x14>;
+   interrupts = ;
+   clocks = <_AO CLKID_AO_CEC_32K>;
+   clock-names = "core";
+   hdmi-phandle = <_tx>;
+};
-- 
1.9.1

[PATCH 0/2] media: Add Amlogic Meson AO CEC Controller support

[Neil Armstrong]

The Amlogic SoC embeds a standalone CEC controller, this patch adds a driver
for such controller.
The controller does not need HPD to be active, and could support up to max
5 logical addresses, but only 1 is handled since the Suspend firmware can
make use of this unique logical address to wake up the device.

The Suspend firmware configuration will be added in an other patchset.

Neil Armstrong (2):
  platform: Add Amlogic Meson AO CEC Controller driver
  dt-bindings: media: Add Amlogic Meson AO-CEC bindings

 .../devicetree/bindings/media/meson-ao-cec.txt |  28 +
 drivers/media/platform/Kconfig |  11 +
 drivers/media/platform/Makefile|   2 +
 drivers/media/platform/meson/Makefile  |   1 +
 drivers/media/platform/meson/ao-cec.c  | 653 +
 5 files changed, 695 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/meson-ao-cec.txt
 create mode 100644 drivers/media/platform/meson/Makefile
 create mode 100644 drivers/media/platform/meson/ao-cec.c

-- 
1.9.1

[PATCH 1/2] platform: Add Amlogic Meson AO CEC Controller driver

[Neil Armstrong]

The Amlogic SoC embeds a standalone CEC controller, this patch adds a driver
for such controller.
The controller does not need HPD to be active, and could support up to max
5 logical addresses, but only 1 is handled since the Suspend firmware can
make use of this unique logical address to wake up the device.

The Suspend firmware configuration will be added in an other patchset.

Signed-off-by: Neil Armstrong 
---
 drivers/media/platform/Kconfig|  11 +
 drivers/media/platform/Makefile   |   2 +
 drivers/media/platform/meson/Makefile |   1 +
 drivers/media/platform/meson/ao-cec.c | 653 ++
 4 files changed, 667 insertions(+)
 create mode 100644 drivers/media/platform/meson/Makefile
 create mode 100644 drivers/media/platform/meson/ao-cec.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1313cd5..1e67381 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -536,6 +536,17 @@ menuconfig CEC_PLATFORM_DRIVERS
 
 if CEC_PLATFORM_DRIVERS
 
+config VIDEO_MESON_AO_CEC
+   tristate "Amlogic Meson AO CEC driver"
+   depends on ARCH_MESON || COMPILE_TEST
+   select CEC_CORE
+   select CEC_NOTIFIER
+   ---help---
+ This is a driver for Amlogic Meson SoCs AO CEC interface. It uses the
+ generic CEC framework interface.
+ CEC bus is present in the HDMI connector and enables communication
+ between compatible devices.
+
 config VIDEO_SAMSUNG_S5P_CEC
tristate "Samsung S5P CEC driver"
depends on PLAT_S5P || ARCH_EXYNOS || COMPILE_TEST
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 9beadc7..a52d7b6 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -86,3 +86,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP)  += mtk-mdp/
 obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)  += mtk-jpeg/
 
 obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
+
+obj-y  += meson/
diff --git a/drivers/media/platform/meson/Makefile 
b/drivers/media/platform/meson/Makefile
new file mode 100644
index 000..597beb8
--- /dev/null
+++ b/drivers/media/platform/meson/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_MESON_AO_CEC)   += ao-cec.o
diff --git a/drivers/media/platform/meson/ao-cec.c 
b/drivers/media/platform/meson/ao-cec.c
new file mode 100644
index 000..26d7c3e8
--- /dev/null
+++ b/drivers/media/platform/meson/ao-cec.c
@@ -0,0 +1,653 @@
+/*
+ * Driver for Amlogic Meson AO CEC Controller
+ *
+ * Copyright (C) 2015 Amlogic, Inc. All rights reserved
+ * Copyright (C) 2017 BayLibre, SAS
+ * Author: Neil Armstrong 
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* CEC Registers */
+
+/*
+ * [2:1] cntl_clk
+ *  - 0 = Disable clk (Power-off mode)
+ *  - 1 = Enable gated clock (Normal mode)
+ *  - 2 = Enable free-run clk (Debug mode)
+ */
+#define CEC_GEN_CNTL_REG   0x00
+
+#define CEC_GEN_CNTL_RESET BIT(0)
+#define CEC_GEN_CNTL_CLK_DISABLE   0
+#define CEC_GEN_CNTL_CLK_ENABLE1
+#define CEC_GEN_CNTL_CLK_ENABLE_DBG2
+#define CEC_GEN_CNTL_CLK_CTRL_MASK GENMASK(2, 1)
+
+/*
+ * [7:0] cec_reg_addr
+ * [15:8] cec_reg_wrdata
+ * [16] cec_reg_wr
+ *  - 0 = Read
+ *  - 1 = Write
+ * [23] bus free
+ * [31:24] cec_reg_rddata
+ */
+#define CEC_RW_REG 0x04
+
+#define CEC_RW_ADDRGENMASK(7, 0)
+#define CEC_RW_WR_DATA GENMASK(15, 8)
+#define CEC_RW_WRITE_ENBIT(16)
+#define CEC_RW_BUS_BUSYBIT(23)
+#define CEC_RW_RD_DATA GENMASK(31, 24)
+
+/*
+ * [1] tx intr
+ * [2] rx intr
+ */
+#define CEC_INTR_MASKN_REG 0x08
+#define CEC_INTR_CLR_REG   0x0c
+#define CEC_INTR_STAT_REG  0x10
+
+#define CEC_INTR_TXBIT(1)
+#define CEC_INTR_RXBIT(2)
+
+/* CEC Commands */
+
+#define CEC_TX_MSG_0_HEADER0x00
+#define CEC_TX_MSG_1_OPCODE0x01
+#define CEC_TX_MSG_2_OP1   0x02
+#define CEC_TX_MSG_3_OP2   0x03
+#define CEC_TX_MSG_4_OP3   0x04
+#define CEC_TX_MSG_5_OP4   0x05
+#define CEC_TX_MSG_6_OP5   0x06
+#define CEC_TX_MSG_7_OP6   0x07
+#define CEC_TX_MSG_8_OP7   0x08
+#define CEC_TX_MSG_9_OP8   0x09
+#define CEC_TX_MSG_A_OP9   0x0A
+#define CEC_TX_MSG_B_OP10  0x0B
+#define CEC_TX_MSG_C_OP11  0x0C
+#define CEC_TX_MSG_D_OP12  0x0D
+#define CEC_TX_MSG_E_OP13  0x0E
+#define CEC_TX_MSG_F_OP14  0x0F
+#define CEC_TX_MSG_LENGTH  0x10
+#define CEC_TX_MSG_CMD 0x11

Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Jose Abreu]

Hi Sylwester,


On 05-07-2017 21:52, Sylwester Nawrocki wrote:
> On 07/04/2017 04:11 PM, Jose Abreu wrote:
>> Document the bindings for the Synopsys Designware HDMI RX.
>>
>> Signed-off-by: Jose Abreu 
>> ---
>>   .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 70 
>> ++
>>   1 file changed, 70 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> Could you make the DT binding documentation patch first patch in the series?
> Now checkpatch will complain about undocumented compatible string when 
> the driver patches are applied alone.

Sure.

>
>> diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt 
>> b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> new file mode 100644
>> index 000..449b8a2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
>> @@ -0,0 +1,70 @@
>> +Synopsys DesignWare HDMI RX Decoder
>> +===
>> +
>> +This document defines device tree properties for the Synopsys DesignWare 
>> HDMI
>> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
>> +specification by itself but is meant to be referenced by platform-specific
>> +device tree bindings.
>> +
>> +When referenced from platform device tree bindings the properties defined in
>> +this document are defined as follows.
> It would be good to make it clear which properties are required and which are
> optional. And also to mention the properties below belong to the HDMI RX node.

Ok.

>
>> +- compatible: Shall be "snps,dw-hdmi-rx".
>> +
>> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
>> +
>> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
> s/5v/HDMI 5V ?

Ok.

>
>> +
>> +- clocks: Phandle to the config clock block.
>> +
>> +- clock-names: Shall be "cfg".
>> +
>> +- edid-phandle: phandle to the EDID handler block.
> Could you make this property optional and when it is missing assume that 
> device
> corresponding to the parent node of this node handles EDID? This way we could
> avoid having property pointing to the parent node.

Hmm, this is for the CEC notifier. Do you mean I should grab the
parent device for the notifier? This property is already optional
if cec is not enabled though.

>
>> +- #address-cells: Shall be 1.
>> +
>> +- #size-cells: Shall be 0.
>> +
>> +You also have to create a subnode for phy driver. Phy properties are as 
>> follows.
> s/phy driver. Phy/the PHY device. PHY ?
>
> Might be also worth to make it explicit these are all required properties.

Ok.

>
>> +- compatible: Shall be "snps,dw-hdmi-phy-e405".
>> +
>> +- reg: Shall be JTAG address of phy.
> s/phy/the PHY ?

Ok.

>
>> +- clocks: Phandle for cfg clock.
>> +
>> +- clock-names:Shall be "cfg".
>> +
>> +A sample binding is now provided. The compatible string is for a SoC which 
>> has
>> +has a Synopsys DesignWare HDMI RX decoder inside.
>> +
>> +Example:
>> +
>> +dw_hdmi_soc: dw-hdmi-soc@0 {
>> +compatible = "snps,dw-hdmi-soc";
> Perhaps just make it
>
>   compatible = "...";
> ?

Yeah, probably its better.

>
>> +reg = <0x11c00 0x1000>; /* EDIDs */
> This is not relevant and undocumented, will likely be part of documentation 
> of other binding thus I'd suggest dropping this reg property.

Ok.

>
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +ranges;
>> +
>> +hdmi-rx@0 {
>> +compatible = "snps,dw-hdmi-rx";
>> +reg = <0x0 0x1>;
>> +interrupts = <1 2>;
>> +edid-phandle = <_hdmi_soc>;
>> +
>> +clocks = <_hdmi_refclk>;
>> +clock-names = "cfg";
>> +
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +hdmi-phy@fc {
>> +compatible = "snps,dw-hdmi-phy-e405";
>> +reg = <0xfc>;
>> +
>> +clocks = <_hdmi_refclk>;
>> +clock-names = "cfg";
>> +};
>> +};
>> +};
> Otherwise looks good. I'll likely not have comments to the other patches.

Thanks for the review!

Best regards,
Jose Miguel Abreu

>
> --
> Regards,
> Sylwester
>  

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 07/03/17 20:16, Gustavo Padovan wrote:
>>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
>>> index, void *pb)
>>> if (pb)
>>> call_void_bufop(q, fill_user_buffer, vb, pb);
>>>  
>>> +   vb->in_fence = fence;
>>> +   if (fence && !dma_fence_add_callback(fence, >fence_cb,
>>> +vb2_qbuf_fence_cb))
>>> +   return 0;
>>
>> Maybe we should provide some error or debug log here or a WARN_ON(), if 
>> dma_fence_add_callback() fails instead of silently ignore any errors.
> 
> This is not an error. If the if succeeds it mean we have installed a
> callback for the fence. If not, it means the fence signaled already and
> we don't can call __vb2_core_qbuf right away.

I had the same question as Mauro. After looking at the dma_fence_add_callback
code I see what you mean, but a comment would certainly be helpful.

Also, should you set vb->in_fence to NULL if the fence signaled already?
I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
You would know that better than I do.

Regards,

Hans

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Hans Verkuil]

On 06/30/17 14:04, Mauro Carvalho Chehab wrote:
> Em Fri, 16 Jun 2017 16:39:10 +0900
> Gustavo Padovan  escreveu:
> 
>> From: Gustavo Padovan 
>>
>> Implement the needed pieces to let userspace subscribe for
>> V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the
>> DQEVENT ioctl.
>>
>> Signed-off-by: Gustavo Padovan 
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c |  6 +-
>>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 5aed7bd..f55b5da 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status);
>>  int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
>>  const struct v4l2_event_subscription *sub)
>>  {
>> -if (sub->type == V4L2_EVENT_CTRL)
>> +switch (sub->type) {
>> +case V4L2_EVENT_CTRL:
>>  return v4l2_event_subscribe(fh, sub, 0, _ctrl_sub_ev_ops);
>> +case V4L2_EVENT_BUF_QUEUED:
>> +return v4l2_event_subscribe(fh, sub, 0, NULL);
>> +}
>>  return -EINVAL;
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_subscribe_event);
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 29aa9d4..00d9c35 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -25,6 +25,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include 
>> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, 
>> const void *pb)
>>  return ret;
>>  }
>>  
>> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
>> +{
>> +struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
>> +struct v4l2_event event;
>> +
>> +memset(, 0, sizeof(event));
>> +event.type = V4L2_EVENT_BUF_QUEUED;
>> +event.u.buf_queued.index = vb->index;
>> +
>> +v4l2_event_queue(vdev, );
>> +}
>> +
> 
> It doesn't sound right to add a V4L2 event to VB2 core. The hole point
> of splitting the core from V4L2 specific stuff is to allow VB2 to be
> used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2.

Good point. So this should be a callback to the higher level.

One thing I was wondering about: v4l2_event_queue sends the event to all
open filehandles of the video node that subscribed to this event. Is that
what we want? Or should we use v4l2_event_queue_fh to only send it to the
vb2 queue owner? I don't know what is best. I think it is OK to send it
to anyone that is interested. If nothing else it will help debugging.

Regards,

Hans

> 
> [1] The split happened as part of a patchset meant to make the DVB
> core to use VB2 and provide DMA APIs to it. Unfortunately, the
> developer that worked on this project moved to some other project.
> The final patch was not applied yet. I have it on my patchwork
> queue. I intend to test and apply it sometime this year.
> 
> 
> 
>>  /**
>>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>>   */
>> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>>  trace_vb2_buf_queue(q, vb);
>>  
>>  call_void_vb_qop(vb, buf_queue, vb);
>> +
>> +vb2_buffer_queued_event(vb);
>>  }
>>  
>>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> 
> 

Re: [PATCH 12/12] [media] vb2: add out-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
> an out_fence for the buffer and return it to userspace on the fence_fd
> field. It only works with ordered queues.
> 
> The fence is signaled on buffer_done(), when the job on the buffer is
> finished.
> 
> v2: check if the queue is ordered.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |  6 ++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 22 +-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 



> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
> b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index e6ad77f..e2733dd 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -204,9 +204,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
> void *pb)
>   b->timestamp = ns_to_timeval(vb->timestamp);
>   b->timecode = vbuf->timecode;
>   b->sequence = vbuf->sequence;
> - b->fence_fd = -1;
> + b->fence_fd = vb->out_fence_fd;

I forgot to ask: can a buffer have both an in and an out fence? If so, then we
have a problem here since we can report only one fence fd.

If it is not allowed, then we need a check for that somewhere.

Regards,

Hans

Re: [PATCH 12/12] [media] vb2: add out-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
> an out_fence for the buffer and return it to userspace on the fence_fd
> field. It only works with ordered queues.
> 
> The fence is signaled on buffer_done(), when the job on the buffer is
> finished.
> 
> v2: check if the queue is ordered.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |  6 ++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 22 +-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 21cc4ed..a57902ee 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -356,6 +356,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum 
> vb2_memory memory,
>   vb->planes[plane].length = plane_sizes[plane];
>   vb->planes[plane].min_length = plane_sizes[plane];
>   }
> + vb->out_fence_fd = -1;
>   q->bufs[vb->index] = vb;
>  
>   /* Allocate video buffer memory for the MMAP type */
> @@ -940,6 +941,11 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum 
> vb2_buffer_state state)
>   __enqueue_in_driver(vb);
>   return;
>   default:
> + dma_fence_signal(vb->out_fence);
> + dma_fence_put(vb->out_fence);
> + vb->out_fence = NULL;
> + vb->out_fence_fd = -1;
> +
>   /* Inform any processes that may be waiting for buffers */
>   wake_up(>done_wq);
>   break;

case VB2_BUF_STATE_REQUEUEING: a driver that calls this would break the ordering
requirement. I would suggest a WARN_ON_ONCE(q->ordered) call there together with
a comment.

> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
> b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index e6ad77f..e2733dd 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -204,9 +204,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
> void *pb)
>   b->timestamp = ns_to_timeval(vb->timestamp);
>   b->timecode = vbuf->timecode;
>   b->sequence = vbuf->sequence;
> - b->fence_fd = -1;
> + b->fence_fd = vb->out_fence_fd;
>   b->reserved = 0;
>  
> + if (vb->sync_file) {
> + fd_install(vb->out_fence_fd, vb->sync_file->file);
> + vb->sync_file = NULL;

I'm no fence expert, but this seems the wrong place for this.

> + }
> +

You need to set the OUT_FENCE flag if there is a fence pending.

>   if (q->is_multiplanar) {
>   /*
>* Fill in plane-related data if userspace provided an array
> @@ -581,6 +586,21 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>   }
>   }
>  
> + if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> + if (!q->ordered) {
> + dprintk(1, "can't use out-fences with unordered 
> queues\n");
> + dma_fence_put(fence);
> + return -EINVAL;
> + }
> +
> + ret = vb2_setup_out_fence(q, b->index);
> + if (ret) {
> + dprintk(1, "failed to set up out-fence\n");
> + dma_fence_put(fence);
> + return ret;
> + }
> + }
> +
>   return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);

Wouldn't it make more sense to do the fd_install after this call?

Besides, if vb2_core_qbuf returns with an error, then I expect that
we need to clean up the fence?

>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 

Regards,

Hans

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Receive in-fence from userspace and add support for waiting on them
> before queueing the buffer to the driver. Buffers are only queued
> to the driver once they are ready. A buffer is ready when its
> in-fence signals.
> 
> 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 
> ---
>  drivers/media/Kconfig|  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 
> +---
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 -
>  include/media/videobuf2-core.h   |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 



> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c 
> b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 110fb45..e6ad77f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> + struct dma_fence *fence = NULL;
>   int ret;
>  
>   if (vb2_fileio_is_active(q)) {
> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>   }
>  
>   ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> - return ret ? ret : vb2_core_qbuf(q, b->index, b);
> + if (ret)
> + return ret;
> +
> + if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> + fence = sync_file_get_fence(b->fence_fd);
> + if (!fence) {
> + dprintk(1, "failed to get in-fence from fd\n");
> + return -EINVAL;
> + }
> + }
> +
> + return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);

You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
if there is a fence pending. It should also fill in fence_fd.

Regards,

Hans

Re: [PATCH 08/12] [media] vb2: add 'ordered' property to queues

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> For explicit synchronization (and soon for HAL3/Request API) we need
> the v4l2-driver to guarantee the ordering which the buffer were queued
> by userspace. This is already true for many drivers, but we never had
> the need to say it.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  include/media/videobuf2-core.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index aa43e43..a8b800e 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -491,6 +491,9 @@ struct vb2_buf_ops {
>   * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if 
> the
>   *   last decoded buffer was already dequeued. Set for capture queues
>   *   when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
> + * @ordered: if the driver can guarantee that the queue will be ordered or 
> not.
> + *   The default is not ordered unless the driver sets this flag. It
> + *   is mandatory for using explicit fences.

This needs to be clarified both here and in the commit log.

1) what is meant with 'ordered'? I assume FIFO is meant.
2) is it the order in which buffers are queued in the driver, or the order
in which buffers are queued in userspace? With in-fences it is ordered
in the driver but not in userspace since the in-fence will block a buffer
from being queued to the driver until the fence callback is called.
3) does this apply to in-fences or out-fences or both? It appears that this
only applies to out-fences.

>   * @fileio:  file io emulator internal data, used only if emulator is active
>   * @threadio:thread io internal data, used only if thread is active
>   */
> @@ -541,6 +544,7 @@ struct vb2_queue {
>   unsigned intis_output:1;
>   unsigned intcopy_timestamp:1;
>   unsigned intlast_buffer_dequeued:1;
> + unsigned intordered:1;
>  
>   struct vb2_fileio_data  *fileio;
>   struct vb2_threadio_data*threadio;
> 

Regards,

Hans

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Implement the needed pieces to let userspace subscribe for
> V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the
> DQEVENT ioctl.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c |  6 +-
>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 5aed7bd..f55b5da 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3435,8 +3435,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status);
>  int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
>   const struct v4l2_event_subscription *sub)
>  {
> - if (sub->type == V4L2_EVENT_CTRL)
> + switch (sub->type) {
> + case V4L2_EVENT_CTRL:
>   return v4l2_event_subscribe(fh, sub, 0, _ctrl_sub_ev_ops);
> + case V4L2_EVENT_BUF_QUEUED:
> + return v4l2_event_subscribe(fh, sub, 0, NULL);

This is dangerous. The '0' argument will only allocate room for a single
BUF_QUEUED event. So if two such events are triggered without the application
reading the first event, then the first event will be lost.

I recommend VIDEO_MAX_FRAME instead. I.e. have room for up to the maximum number
of buffers.

> + }
>   return -EINVAL;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_subscribe_event);
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 29aa9d4..00d9c35 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -25,6 +25,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, 
> const void *pb)
>   return ret;
>  }
>  
> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
> +{
> + struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> + struct v4l2_event event;
> +
> + memset(, 0, sizeof(event));
> + event.type = V4L2_EVENT_BUF_QUEUED;
> + event.u.buf_queued.index = vb->index;
> +
> + v4l2_event_queue(vdev, );
> +}
> +
>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>   trace_vb2_buf_queue(q, vb);
>  
>   call_void_vb_qop(vb, buf_queue, vb);
> +
> + vb2_buffer_queued_event(vb);
>  }
>  
>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> 

Regards,

Hans

Re: [PATCH 09/12] [media] vivid: mark vivid queues as ordered

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> To enable vivid to be used with explicit synchronization we need
> to mark its queues as ordered.
> 
> Signed-off-by: Gustavo Padovan 

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/media/platform/vivid/vivid-core.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c 
> b/drivers/media/platform/vivid/vivid-core.c
> index 8843170..c7bef90 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -1063,6 +1063,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE 
> :
>   V4L2_BUF_TYPE_VIDEO_CAPTURE;
>   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
> + q->ordered = 1;
>   q->drv_priv = dev;
>   q->buf_struct_size = sizeof(struct vivid_buffer);
>   q->ops = _vid_cap_qops;
> @@ -1083,6 +1084,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
>   V4L2_BUF_TYPE_VIDEO_OUTPUT;
>   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
> + q->ordered = 1;
>   q->drv_priv = dev;
>   q->buf_struct_size = sizeof(struct vivid_buffer);
>   q->ops = _vid_out_qops;
> @@ -1103,6 +1105,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->type = dev->has_raw_vbi_cap ? V4L2_BUF_TYPE_VBI_CAPTURE :
> V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
>   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
> + q->ordered = 1;
>   q->drv_priv = dev;
>   q->buf_struct_size = sizeof(struct vivid_buffer);
>   q->ops = _vbi_cap_qops;
> @@ -1123,6 +1126,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->type = dev->has_raw_vbi_out ? V4L2_BUF_TYPE_VBI_OUTPUT :
> V4L2_BUF_TYPE_SLICED_VBI_OUTPUT;
>   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
> + q->ordered = 1;
>   q->drv_priv = dev;
>   q->buf_struct_size = sizeof(struct vivid_buffer);
>   q->ops = _vbi_out_qops;
> @@ -1142,6 +1146,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q = >vb_sdr_cap_q;
>   q->type = V4L2_BUF_TYPE_SDR_CAPTURE;
>   q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
> + q->ordered = 1;
>   q->drv_priv = dev;
>   q->buf_struct_size = sizeof(struct vivid_buffer);
>   q->ops = _sdr_cap_qops;
> 

Re: [PATCH 05/12] [media] vivid: assign the specific device to the vb2_queue->dev

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Instead of assigning the global v4l2 device, assign the specific device.
> This was causing trouble when using using V4L2 events with vivid
> devices. The device's queue should be the same we opened in userspace.
> 
> Signed-off-by: Gustavo Padovan 

Can you add a line to the commit log that says that this is needed for
the upcoming V4L2_EVENT_BUF_QUEUED support? This log message suggests that
the current vivid code is wrong, which it isn't. It just needs to be changed
so V4L2_EVENT_BUF_QUEUED can be supported.

After making that change:

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/media/platform/vivid/vivid-core.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c 
> b/drivers/media/platform/vivid/vivid-core.c
> index ef344b9..8843170 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -1070,7 +1070,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>   q->min_buffers_needed = 2;
>   q->lock = >mutex;
> - q->dev = dev->v4l2_dev.dev;
> + q->dev = >vid_cap_dev.dev;
>  
>   ret = vb2_queue_init(q);
>   if (ret)
> @@ -1090,7 +1090,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>   q->min_buffers_needed = 2;
>   q->lock = >mutex;
> - q->dev = dev->v4l2_dev.dev;
> + q->dev = >vid_out_dev.dev;
>  
>   ret = vb2_queue_init(q);
>   if (ret)
> @@ -1110,7 +1110,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>   q->min_buffers_needed = 2;
>   q->lock = >mutex;
> - q->dev = dev->v4l2_dev.dev;
> + q->dev = >vbi_cap_dev.dev;
>  
>   ret = vb2_queue_init(q);
>   if (ret)
> @@ -1130,7 +1130,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>   q->min_buffers_needed = 2;
>   q->lock = >mutex;
> - q->dev = dev->v4l2_dev.dev;
> + q->dev = >vbi_out_dev.dev;
>  
>   ret = vb2_queue_init(q);
>   if (ret)
> @@ -1149,7 +1149,7 @@ static int vivid_create_instance(struct platform_device 
> *pdev, int inst)
>   q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>   q->min_buffers_needed = 8;
>   q->lock = >mutex;
> - q->dev = dev->v4l2_dev.dev;
> + q->dev = >sdr_cap_dev.dev;
>  
>   ret = vb2_queue_init(q);
>   if (ret)
> 

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 5:34 PM, Tomasz Figa  wrote:
> On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann  wrote:
>> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa  wrote:
>>> On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig  wrote:
 On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>>
 In general I think moving dma
 ops and iommu implementations into modules is a bad idea
>>>
>>> Could you elaborate on this? I'd be interested in seeing the reasoning
>>> behind this.
>>>
 but I
 don't want to reject the idea before seeing the code.  Or maybe
 by looking at the user we can come up with an even better idea
 to solve the original issue you're trying to solve, so please also
 explain your rationale.
>>
>> I had pretty much the same thoughts here.
>>
>>> Basically we have an x86 platform with a camera subsystem that is a
>>> PCI device, has its own MMU and needs cache maintenance. Moreover, the
>>> V4L2 subsystem, which is the right place for camera drivers, heavily
>>> relies on DMA mapping as a way to abstract memory allocation, mapping
>>> and cache maintenance. So it feels natural to me to hide the hardware
>>> details (additional cache maintenance, mapping into the built-in
>>> IOMMU) in the DMA mapping ops for this camera subsystem and simply
>>> make V4L2 just work without knowing those details.
>>
>> I can understand your reasoning here, but I'm also not convinced
>> that this is the best approach. There may be a middle ground somewhere
>> though.
>>
>> Generally speaking I don't want to have to deal with the horrors of
>> deciding whether an IOMMU is going to be there eventually or not
>> at probe() time. At some point, we had decided that IOMMUs need to
>> be initialized (almost) as early as irqchips and clocksources so we can
>> rely on them to be there at device discovery time. That got pushed
>> back already, and now we may have to deal with -EPROBE_DEFER
>> when an IOMMU has not been fully initialized at device probe time,
>> but at least we can reliably see if one is there or not. Making IOMMUs
>> modular will add further uncertainty here. Obviously we cannot attach
>> an IOMMU to a device once we have started using DMA mapping
>> calls on it.
>
> The hardware can only work with IOMMU and so the main module is highly
> tied with the IOMMU module and it initialized it directly. There is no
> separate struct driver or device associated with the IOMMU, as it's a
> part of the one and only one PCI device (as visible from the system
> PCI bus point of view) and technically handled by one pci_driver.
>
>>
>> For your particular use case, I would instead leave the knowledge
>> about the IOMMU in the driver itself, like we do for the IOMMUs
>> that are integrated in desktop GPUs, and have the code use the
>> DMA mapping API with the system-provided dma_map_ops to
>> get dma_addr_t tokens which you then program into the device
>> IOMMU.
>>
>> An open question however would be whether to use the IOMMU
>> API without the DMA mapping API here, or whether to completely
>> leave the knowledge of the IOMMU inside of the driver itself.
>> I don't have a strong opinion on that part, and I guess this mostly
>> depends on what the hardware looks like.
>
> + linux-media and some media folks
>
> I'd say that this is something that has been consistently tried to be
> avoided by V4L2 and that's why it's so tightly integrated with DMA
> mapping. IMHO re-implementing the code that's already there in
> videobuf2 again in the driver, only because, for no good reason
> mentioned as for now, having a loadable module providing DMA ops was
> disliked.

Sorry, I intended to mean:

IMHO re-implementing the code that's already there in videobuf2 again
in the driver, only because, for no good reason mentioned as for now,
having a loadable module providing DMA ops was disliked, would make no
sense.

>
> Similarly with IOMMU API. It provides a lot of help in managing the
> mappings and re-implementing this would be IMHO backwards.
>
> Best regards,
> Tomasz

Re: [RFC PATCH 1/5] base: dma-mapping: Export commonly used symbols

[Tomasz Figa]

On Thu, Jul 6, 2017 at 5:26 PM, Arnd Bergmann  wrote:
> On Thu, Jul 6, 2017 at 3:44 AM, Tomasz Figa  wrote:
>> On Thu, Jul 6, 2017 at 2:20 AM, Christoph Hellwig  wrote:
>>> On Thu, Jul 06, 2017 at 12:22:35AM +0900, Tomasz Figa wrote:
>
>>> In general I think moving dma
>>> ops and iommu implementations into modules is a bad idea
>>
>> Could you elaborate on this? I'd be interested in seeing the reasoning
>> behind this.
>>
>>> but I
>>> don't want to reject the idea before seeing the code.  Or maybe
>>> by looking at the user we can come up with an even better idea
>>> to solve the original issue you're trying to solve, so please also
>>> explain your rationale.
>
> I had pretty much the same thoughts here.
>
>> Basically we have an x86 platform with a camera subsystem that is a
>> PCI device, has its own MMU and needs cache maintenance. Moreover, the
>> V4L2 subsystem, which is the right place for camera drivers, heavily
>> relies on DMA mapping as a way to abstract memory allocation, mapping
>> and cache maintenance. So it feels natural to me to hide the hardware
>> details (additional cache maintenance, mapping into the built-in
>> IOMMU) in the DMA mapping ops for this camera subsystem and simply
>> make V4L2 just work without knowing those details.
>
> I can understand your reasoning here, but I'm also not convinced
> that this is the best approach. There may be a middle ground somewhere
> though.
>
> Generally speaking I don't want to have to deal with the horrors of
> deciding whether an IOMMU is going to be there eventually or not
> at probe() time. At some point, we had decided that IOMMUs need to
> be initialized (almost) as early as irqchips and clocksources so we can
> rely on them to be there at device discovery time. That got pushed
> back already, and now we may have to deal with -EPROBE_DEFER
> when an IOMMU has not been fully initialized at device probe time,
> but at least we can reliably see if one is there or not. Making IOMMUs
> modular will add further uncertainty here. Obviously we cannot attach
> an IOMMU to a device once we have started using DMA mapping
> calls on it.

The hardware can only work with IOMMU and so the main module is highly
tied with the IOMMU module and it initialized it directly. There is no
separate struct driver or device associated with the IOMMU, as it's a
part of the one and only one PCI device (as visible from the system
PCI bus point of view) and technically handled by one pci_driver.

>
> For your particular use case, I would instead leave the knowledge
> about the IOMMU in the driver itself, like we do for the IOMMUs
> that are integrated in desktop GPUs, and have the code use the
> DMA mapping API with the system-provided dma_map_ops to
> get dma_addr_t tokens which you then program into the device
> IOMMU.
>
> An open question however would be whether to use the IOMMU
> API without the DMA mapping API here, or whether to completely
> leave the knowledge of the IOMMU inside of the driver itself.
> I don't have a strong opinion on that part, and I guess this mostly
> depends on what the hardware looks like.

+ linux-media and some media folks

I'd say that this is something that has been consistently tried to be
avoided by V4L2 and that's why it's so tightly integrated with DMA
mapping. IMHO re-implementing the code that's already there in
videobuf2 again in the driver, only because, for no good reason
mentioned as for now, having a loadable module providing DMA ops was
disliked.

Similarly with IOMMU API. It provides a lot of help in managing the
mappings and re-implementing this would be IMHO backwards.

Best regards,
Tomasz

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Receive in-fence from userspace and add support for waiting on them
> before queueing the buffer to the driver. Buffers are only queued
> to the driver once they are ready. A buffer is ready when its
> in-fence signals.
> 
> 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 
> ---
>  drivers/media/Kconfig|  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 
> +---
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 -
>  include/media/videobuf2-core.h   |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 55d9c2b..3cd1d3d 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -11,6 +11,7 @@ config CEC_NOTIFIER
>  menuconfig MEDIA_SUPPORT
>   tristate "Multimedia support"
>   depends on HAS_IOMEM
> + select SYNC_FILE

Is this the right place for this? Shouldn't this be selected in
'config VIDEOBUF2_CORE'?

Fences are specific to vb2 after all.

>   help
> If you want to use Webcams, Video grabber devices and/or TV devices
> enable this option and other options below.
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index ea83126..29aa9d4 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const 
> void *pb)
>   return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> + struct vb2_buffer *vb;
> + int ready_count = 0;
> +
> + /* count num of buffers ready in front of the queued_list */
> + list_for_each_entry(vb, >queued_list, queued_entry) {
> + if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> + break;

Obviously the break is wrong as Mauro mentioned.

> +
> + ready_count++;
> + }
> +
> + return ready_count;
> +}
> +
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>   struct vb2_buffer *vb;
> @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
>* If any buffers were queued before streamon,
>* we can now pass them to driver for processing.
>*/
> - list_for_each_entry(vb, >queued_list, queued_entry)
> + list_for_each_entry(vb, >queued_list, queued_entry) {
> + if (vb->state != VB2_BUF_STATE_QUEUED)
> + continue;

I think this test is unnecessary.

> +
> + if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> + break;
> +
>   __enqueue_in_driver(vb);

I would move the above test (after fixing it as Mauro said) to 
__enqueue_in_driver.
I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

> + }
>  
>   /* Tell the driver to start streaming */
>   q->start_streaming_called = 1;
> @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  
>  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
>  {
> + struct vb2_buffer *b;
>   int ret;
>  
>   /*
>* If already streaming, give the buffer to driver for processing.
>* If not, the buffer will be given to driver on next streamon.
>*/
> - if (q->start_streaming_called)
> - __enqueue_in_driver(vb);
>  
> - /*
> -  * If streamon has been called, and we haven't yet called
> -  * start_streaming() since not enough buffers were queued, and
> -  * we now have reached the minimum number of queued buffers,
> -  * then we can finally call start_streaming().
> -  */
> - if (q->streaming && !q->start_streaming_called &&
> - q->queued_count >= q->min_buffers_needed) {
> - ret = vb2_start_streaming(q);
> - if (ret)
> - return ret;
> + if (q->start_streaming_called) {
> + list_for_each_entry(b, >queued_list, queued_entry) {
> + if (b->state != VB2_BUF_STATE_QUEUED)
> + continue;
> +
> + if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> + break;

Again, if this test is in __enqueue_in_driver, then you can keep the
original code. Why would you need to loop over all buffers anyway?

If a fence is ready then the callback will call this function for that
buffer. Everything works fine AFAICT without looping over buffers here.

> +
> +  

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

Hi Sylwester,

Do you have the possibility to check for non-regression of this patchset 
on 9650/52 camera ?

Best regards,
Hugues.

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.
> 
> OV9655 initial support is based on a driver written by H. Nikolaus Schaller 
> [1].
> OV9655 registers sequences come from STM32CubeF7 embedded software [2].
> 
> [1] 
> http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
> [2] 
> https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c
> 
> ===
> = history =
> ===
> version 2:
>- Remove some unneeded semicolons (kbuild test robot):
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114616.html
>- Remove patch [media] ov9650: select the nearest higher resolution:
>  it is up to the application to find the best matching resolution
>  using ENUM_FRAMESIZES/S_FMT/S_SELECTION (S_CROP), see
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114667.html
>- dt-bindings: Fix remarks from Rob Herring about polarity:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114705.html
>- dt-bindings: Add optional regulators avdd, dvdd, dovdd:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114785.html
>- fix missing semicolons in if condition:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114611.html
>- move ov965x_pixfmt relocation in right patch:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114849.html
>- revisit OV965x renaming to ov965x for device id names and DT compatible 
> strings,
>  drop of_device_id .data device identification
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114635.html
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114738.html
>- Add analog power supply and clock gating, needed for GTA04 platform:
>http://www.mail-archive.com/linux-media@vger.kernel.org/msg114519.html
> 
> version 1:
>- Initial submission.
> 
> H. Nikolaus Schaller (1):
>DT bindings: add bindings for ov965x camera module
> 
> Hugues Fruchet (6):
>[media] ov9650: switch i2c device id to lower case
>[media] ov9650: add device tree support
>[media] ov9650: use write_array() for resolution sequences
>[media] ov9650: add multiple variant support
>[media] ov9650: add support of OV9655 variant
>[media] ov9650: add analog power supply and clock gating
> 
>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>   drivers/media/i2c/Kconfig  |   6 +-
>   drivers/media/i2c/ov9650.c | 816 
> +
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 

Re: [PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> In order to support explicit synchronization we need to divide
> vb2_core_qbuf() in two parts, one to be executed before the fence
> signals and another one to do the actual queueing of the buffer.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 51 
> ++--
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 3107e21..ea83126 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
>   return ret;
>  }
>  
> +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> +{
> + int ret;
> +
> + /*
> +  * If already streaming, give the buffer to driver for processing.
> +  * If not, the buffer will be given to driver on next streamon.
> +  */
> + if (q->start_streaming_called)
> + __enqueue_in_driver(vb);
> +
> + /*
> +  * If streamon has been called, and we haven't yet called
> +  * start_streaming() since not enough buffers were queued, and
> +  * we now have reached the minimum number of queued buffers,
> +  * then we can finally call start_streaming().
> +  */
> + if (q->streaming && !q->start_streaming_called &&
> + q->queued_count >= q->min_buffers_needed) {
> + ret = vb2_start_streaming(q);
> + if (ret)
> + return ret;
> + }
> +
> + dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> + return 0;
> +}
> +
>  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>   struct vb2_buffer *vb;
> @@ -1404,32 +1432,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int 
> index, void *pb)
>  
>   trace_vb2_qbuf(q, vb);
>  
> - /*
> -  * If already streaming, give the buffer to driver for processing.
> -  * If not, the buffer will be given to driver on next streamon.
> -  */
> - if (q->start_streaming_called)
> - __enqueue_in_driver(vb);
> -
>   /* Fill buffer information for the userspace */
>   if (pb)
>   call_void_bufop(q, fill_user_buffer, vb, pb);

This should be called *after* the __vb2_core_qbuf call. That call changes
vb->state which is used by buffer to fill in v4l2_buffer. So the order
should be swapped here to ensure we return the latest state of the buffer.

>  
> - /*
> -  * If streamon has been called, and we haven't yet called
> -  * start_streaming() since not enough buffers were queued, and
> -  * we now have reached the minimum number of queued buffers,
> -  * then we can finally call start_streaming().
> -  */
> - if (q->streaming && !q->start_streaming_called &&
> - q->queued_count >= q->min_buffers_needed) {
> - ret = vb2_start_streaming(q);
> - if (ret)
> - return ret;
> - }
> -
> - dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> - return 0;
> + return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  
> 

Regards,

Hans

Re: [PATCH 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver (fwd)

[Julia Lawall]

Please check on this (line 199).

julia

-- Forwarded message --
Date: Thu, 6 Jul 2017 13:58:29 +0800
From: kbuild test robot 
To: kbu...@01.org
Cc: Julia Lawall 
Subject: Re: [PATCH 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

Hi Maxime,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.12]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Maxime-Ripard/dt-bindings-media-Add-Cadence-MIPI-CSI2RX-Device-Tree-bindings/20170705-205643
base:   git://linuxtv.org/media_tree.git master
:: branch date: 17 hours ago
:: commit date: 17 hours ago

>> drivers/media/platform/cadence/cdns-csi2rx.c:199:5-23: WARNING: Unsigned 
>> expression compared with zero: csi2rx -> sensor_pad < 0

git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout fb3d2879ba0623bcdc83c99ba70229ec1713feaf
vim +199 drivers/media/platform/cadence/cdns-csi2rx.c

fb3d2879 Maxime Ripard 2017-07-03  183  return 
media_create_pad_link(>sensor_subdev->entity,
fb3d2879 Maxime Ripard 2017-07-03  184   
csi2rx->sensor_pad,
fb3d2879 Maxime Ripard 2017-07-03  185   
>subdev.entity, 0,
fb3d2879 Maxime Ripard 2017-07-03  186   
MEDIA_LNK_FL_ENABLED |
fb3d2879 Maxime Ripard 2017-07-03  187   
MEDIA_LNK_FL_IMMUTABLE);
fb3d2879 Maxime Ripard 2017-07-03  188  }
fb3d2879 Maxime Ripard 2017-07-03  189
fb3d2879 Maxime Ripard 2017-07-03  190  static int csi2rx_async_bound(struct 
v4l2_async_notifier *notifier,
fb3d2879 Maxime Ripard 2017-07-03  191struct 
v4l2_subdev *subdev,
fb3d2879 Maxime Ripard 2017-07-03  192struct 
v4l2_async_subdev *asd)
fb3d2879 Maxime Ripard 2017-07-03  193  {
fb3d2879 Maxime Ripard 2017-07-03  194  struct csi2rx_priv *csi2rx = 
v4l2_notifier_to_csi2rx(notifier);
fb3d2879 Maxime Ripard 2017-07-03  195
fb3d2879 Maxime Ripard 2017-07-03  196  csi2rx->sensor_pad = 
media_entity_get_fwnode_pad(>entity,
fb3d2879 Maxime Ripard 2017-07-03  197  
 >sensor_node->fwnode,
fb3d2879 Maxime Ripard 2017-07-03  198  
 MEDIA_PAD_FL_SOURCE);
fb3d2879 Maxime Ripard 2017-07-03 @199  if (csi2rx->sensor_pad < 0) {
fb3d2879 Maxime Ripard 2017-07-03  200  dev_err(csi2rx->dev, 
"Couldn't find output pad for subdev %s\n",
fb3d2879 Maxime Ripard 2017-07-03  201  subdev->name);
fb3d2879 Maxime Ripard 2017-07-03  202  return 
csi2rx->sensor_pad;
fb3d2879 Maxime Ripard 2017-07-03  203  }
fb3d2879 Maxime Ripard 2017-07-03  204
fb3d2879 Maxime Ripard 2017-07-03  205  csi2rx->sensor_subdev = subdev;
fb3d2879 Maxime Ripard 2017-07-03  206
fb3d2879 Maxime Ripard 2017-07-03  207  dev_dbg(csi2rx->dev, "Bound %s 
pad: %d\n", subdev->name,

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation