Re: [PATCH] media: cedrus: don't initialize pointers with zero

[Ian Arkver]

On 07/12/2018 11:37, Hans Verkuil wrote:

On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote:

Em Fri, 7 Dec 2018 12:14:50 +0100
Hans Verkuil  escreveu:


On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:

A common mistake is to assume that initializing a var with:
struct foo f = { 0 };

Would initialize a zeroed struct. Actually, what this does is
to initialize the first element of the struct to zero.

According to C99 Standard 6.7.8.21:

 "If there are fewer initializers in a brace-enclosed
  list than there are elements or members of an aggregate,
  or fewer characters in a string literal used to initialize
  an array of known size than there are elements in the array,
  the remainder of the aggregate shall be initialized implicitly
  the same as objects that have static storage duration."

So, in practice, it could zero the entire struct, but, if the
first element is not an integer, it will produce warnings:


drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
  warning: Using plain integer as NULL pointer

drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
  warning: Using plain integer as NULL pointer

A proper way to initialize it with gcc is to use:

struct foo f = { };

But that seems to be a gcc extension. So, I decided to check upstream


No, this is not a gcc extension. It's part of the latest C standard.


Sure? Where the C standard spec states that? I've been seeking for
such info for a while, as '= {}' is also my personal preference.


I believe it was added in C11, but I've loaned the book that stated
that to someone else, so I can't check.


Sadly I don't see mention of empty initializer lists in section 6.7.9 of
ISO/IEC 9899:2011, though I've only got a draft version.

I had a play with Compiler Explorer[1] and it seems like clang is OK
with the {} form though just out of interest msvc isn't. I didn't try
exploring other command line options.

[1] https://gcc.godbolt.org/z/XIDC7W

Regards,
Ian


In any case, it's used everywhere:

git grep '= *{ *}' drivers/

So it is really pointless to *not* use it.

Regards,

Hans


I tried to build the Kernel with clang, just to be sure that this
won't cause issues with the clang support, but, unfortunately, the
clang compiler (not even the upstream version) is able to build
the upstream Kernel yet, as it lacks asm-goto support (there is an
OOT patch for llvm solving it - but it sounds too much effort for
my time to have to build llvm from their sources just for a simple
check like this).


It's used all over in the kernel, so please use {} instead of { NULL }.

Personally I think {} is a fantastic invention and I wish C++ had it as
well.


Fully agreed on that.



Regards,

Hans


what's the most commonly pattern. The gcc pattern has about 2000 entries:

$ git grep -E "=\s*\{\s*\}"|wc -l
1951

The standard-C compliant pattern has about 2500 entries:

$ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l
137
$ git grep -E "=\s*\{\s*0\s*\}"|wc -l
2323

So, let's initialize those structs with:
 = { NULL }

Signed-off-by: Mauro Carvalho Chehab 
---
  drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +-
  drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c 
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index b538eb0321d8..44c45c687503 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct 
cedrus_ctx *ctx)
memset(ctx->ctrls, 0, ctrl_size);
  
  	for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {

-   struct v4l2_ctrl_config cfg = { 0 };
+   struct v4l2_ctrl_config cfg = { NULL };
  
  		cfg.elem_size = cedrus_controls[i].elem_size;

cfg.id = cedrus_controls[i].id;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c 
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..4099a42dba2d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -26,7 +26,7 @@ void cedrus_device_run(void *priv)
  {
struct cedrus_ctx *ctx = priv;
struct cedrus_dev *dev = ctx->dev;
-   struct cedrus_run run = { 0 };
+   struct cedrus_run run = { NULL };
struct media_request *src_req;
unsigned long flags;
  
   






Thanks,
Mauro

Re: [PATCH 01/15] media: coda: fix memory corruption in case more than 32 instances are opened

[Ian Arkver]

Hi Philipp,

On 05/11/2018 15:24, Philipp Zabel wrote:

The ffz() return value is undefined if the instance mask does not
contain any zeros. If it returned 32, the following set_bit would
corrupt the debugfs_root pointer.
Switch to IDA for context index allocation. This also removes the
artificial 32 instance limit for all except CodaDx6.

Signed-off-by: Philipp Zabel 
---
  drivers/media/platform/coda/coda-common.c | 25 ---
  drivers/media/platform/coda/coda.h|  2 +-
  2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 2848ea5f464d..cbb59c2f3a82 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2099,17 +2099,6 @@ int coda_decoder_queue_init(void *priv, struct vb2_queue 
*src_vq,
return coda_queue_init(priv, dst_vq);
  }
  
-static int coda_next_free_instance(struct coda_dev *dev)

-{
-   int idx = ffz(dev->instance_mask);
-
-   if ((idx < 0) ||
-   (dev->devtype->product == CODA_DX6 && idx > CODADX6_MAX_INSTANCES))
-   return -EBUSY;
-
-   return idx;
-}
-
  /*
   * File operations
   */
@@ -2118,7 +2107,8 @@ static int coda_open(struct file *file)
  {
struct video_device *vdev = video_devdata(file);
struct coda_dev *dev = video_get_drvdata(vdev);
-   struct coda_ctx *ctx = NULL;
+   struct coda_ctx *ctx;
+   unsigned int max = ~0;
char *name;
int ret;
int idx;
@@ -2127,12 +2117,13 @@ static int coda_open(struct file *file)
if (!ctx)
return -ENOMEM;
  
-	idx = coda_next_free_instance(dev);

+   if (dev->devtype->product == CODA_DX6)
+   max = CODADX6_MAX_INSTANCES - 1;
+   idx = ida_alloc_max(>ida, max, GFP_KERNEL);
if (idx < 0) {
ret = idx;
goto err_coda_max;
}
-   set_bit(idx, >instance_mask);
  
  	name = kasprintf(GFP_KERNEL, "context%d", idx);

if (!name) {
@@ -2241,8 +2232,8 @@ static int coda_open(struct file *file)
  err_pm_get:
v4l2_fh_del(>fh);
v4l2_fh_exit(>fh);
-   clear_bit(ctx->idx, >instance_mask);
  err_coda_name_init:
+   ida_free(>ida, ctx->idx);
  err_coda_max:
kfree(ctx);
return ret;
@@ -2284,7 +2275,7 @@ static int coda_release(struct file *file)
pm_runtime_put_sync(>plat_dev->dev);
v4l2_fh_del(>fh);
v4l2_fh_exit(>fh);
-   clear_bit(ctx->idx, >instance_mask);
+   ida_free(>ida, ctx->idx);
if (ctx->ops->release)
ctx->ops->release(ctx);
debugfs_remove_recursive(ctx->debugfs_entry);
@@ -2745,6 +2736,7 @@ static int coda_probe(struct platform_device *pdev)
  
  	mutex_init(>dev_mutex);

mutex_init(>coda_mutex);
+   ida_init(>ida);
  
  	dev->debugfs_root = debugfs_create_dir("coda", NULL);

if (!dev->debugfs_root)
@@ -2832,6 +2824,7 @@ static int coda_remove(struct platform_device *pdev)
coda_free_aux_buf(dev, >tempbuf);
coda_free_aux_buf(dev, >workbuf);
debugfs_remove_recursive(dev->debugfs_root);
+   ida_destroy(>ida);
return 0;
  }
  
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h

index 19ac0b9dc6eb..b6cd14ee91ea 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h


Should you add:
#include 
to this header?

Regards,
Ian


@@ -95,7 +95,7 @@ struct coda_dev {
struct workqueue_struct *workqueue;
struct v4l2_m2m_dev *m2m_dev;
struct list_headinstances;
-   unsigned long   instance_mask;
+   struct ida  ida;
struct dentry   *debugfs_root;
  };
  

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi Ezequiel,

On 03/09/2018 18:15, Ezequiel Garcia wrote:

On Mon, 2018-09-03 at 16:51 +0100, Ian Arkver wrote:

Hi Hans, Ezequiel,

On 03/09/2018 16:33, Hans Verkuil wrote:

On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:

Hi Ian, Hans:

On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:

Hi,

On 03/09/2018 10:50, Hans Verkuil wrote:

On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:

From: Shunqian Zheng 

Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
configure the JPEG quantization tables.

Signed-off-by: Shunqian Zheng 
Signed-off-by: Ezequiel Garcia 
---
.../media/uapi/v4l/extended-controls.rst  | 23 +++
.../media/videodev2.h.rst.exceptions  |  1 +
drivers/media/v4l2-core/v4l2-ctrls.c  | 10 
include/uapi/linux/v4l2-controls.h|  5 
include/uapi/linux/videodev2.h|  1 +
5 files changed, 40 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst 
b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..e0dd03e452de 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3354,7 +3354,30 @@ JPEG Control IDs
Specify which JPEG markers are included in compressed stream. This
control is valid only for encoders.

+.. _jpeg-quant-tables-control:

+``V4L2_CID_JPEG_QUANTIZATION (struct)``

+Specifies the luma and chroma quantization matrices for encoding
+or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
+must be set in JPEG zigzag order, as per the JPEG specification.


Can you change "JPEG specification" to a reference to the JPEG spec entry
in bibio.rst?


+
+
+.. c:type:: struct v4l2_ctrl_jpeg_quantization
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_jpeg_quantization
+:header-rows:  0
+:stub-columns: 0
+:widths:   1 1 2
+
+* - __u8
+  - ``luma_quantization_matrix[64]``
+  - Sets the luma quantization table.
+
+* - __u8
+  - ``chroma_quantization_matrix[64]``
+  - Sets the chroma quantization table.


Just checking: the JPEG standard specifies this as unsigned 8-bit values as 
well?


I thought this was already discussed, but I think the only thing I've added
is this comment in one of the driver's headers:

   JPEG encoder
   
   The VPU JPEG encoder produces JPEG baseline sequential format.
   The quantization coefficients are 8-bit values, complying with
   the baseline specification. Therefore, it requires application-defined
   luma and chroma quantization tables. The hardware does entrophy
   encoding using internal Huffman tables, as specified in the JPEG
   specification.

Certainly controls should be specified better.


As far as I can see ISO/IEC 10918-1 does not specify the precision or
signedness of the quantisation value Qvu. The default tables for 8-bit
baseline JPEG all fit into __u8 though.



Paragraph 4.7 of that spec, indicates the "sample" precision:
8-bit for baseline; 8-bit or 12-bit for extended.

For the quantization coefficients, the DQT segment contains a bit
that indicates if the quantization coefficients are 8-bit or 16-bit.
See B.2.4.1 for details.


See below (and Tq which follows the Pq field)




However there can be four sets of tables in non-baseline JPEG and it's


You lost me here, which four sets of tables are you refering to?


not clear (to me) whether 12-bit JPEG would need more precision (I'd
guess it would).


It seems it would. From B.2.4.1:

"An 8-bit DCT-based process shall not use a 16-bit precision quantization 
table."


Since this patch is defining UAPI I think it might be
good to build in some additional information, eg. number of tables,
element size. Maybe this can all be inferred from the selected pixel
format? If so then it would need documented that the above structure
only applies to baseline.



For quantization coefficients, I can only see two tables: one for luma
one for chroma. Huffman coefficients are a different story and we are
not really adding them here.


I was looking at the definition of Tqi in the frame header in B.2.2
which seems to allow up to four (sets of?) quantization tables.
Rereading it, it seems these are per component. Table B.2 implies that
this applies to Baseline Sequential too. In the DQT marker description
there's a Tq field to specify the destination for the new table. I think
this means that an encoder can use up to four (sets of) tables and a
decoder should be able to store four from the stream.

This may well not be relevant to the encoder under discussion, but if
it's not allowed for in UAPI it's almost a given that it'll need to be
added later.



Hm, I think it's not really relevant for us, either on the encoding
or decoding side. Let me explain how I read the spec.

First of all, keep in mind it seems to be written with streams
in mind, 

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi Hans, Ezequiel,

On 03/09/2018 16:33, Hans Verkuil wrote:

On 09/03/2018 05:27 PM, Ezequiel Garcia wrote:

Hi Ian, Hans:

On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote:

Hi,

On 03/09/2018 10:50, Hans Verkuil wrote:

On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:

From: Shunqian Zheng 

Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
configure the JPEG quantization tables.

Signed-off-by: Shunqian Zheng 
Signed-off-by: Ezequiel Garcia 
---
   .../media/uapi/v4l/extended-controls.rst  | 23 +++
   .../media/videodev2.h.rst.exceptions  |  1 +
   drivers/media/v4l2-core/v4l2-ctrls.c  | 10 
   include/uapi/linux/v4l2-controls.h|  5 
   include/uapi/linux/videodev2.h|  1 +
   5 files changed, 40 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst 
b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..e0dd03e452de 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3354,7 +3354,30 @@ JPEG Control IDs
   Specify which JPEG markers are included in compressed stream. This
   control is valid only for encoders.
   
+.. _jpeg-quant-tables-control:
   
+``V4L2_CID_JPEG_QUANTIZATION (struct)``

+Specifies the luma and chroma quantization matrices for encoding
+or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
+must be set in JPEG zigzag order, as per the JPEG specification.


Can you change "JPEG specification" to a reference to the JPEG spec entry
in bibio.rst?


+
+
+.. c:type:: struct v4l2_ctrl_jpeg_quantization
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_jpeg_quantization
+:header-rows:  0
+:stub-columns: 0
+:widths:   1 1 2
+
+* - __u8
+  - ``luma_quantization_matrix[64]``
+  - Sets the luma quantization table.
+
+* - __u8
+  - ``chroma_quantization_matrix[64]``
+  - Sets the chroma quantization table.


Just checking: the JPEG standard specifies this as unsigned 8-bit values as 
well?




I thought this was already discussed, but I think the only thing I've added
is this comment in one of the driver's headers:

  JPEG encoder
  
  The VPU JPEG encoder produces JPEG baseline sequential format.
  The quantization coefficients are 8-bit values, complying with
  the baseline specification. Therefore, it requires application-defined
  luma and chroma quantization tables. The hardware does entrophy
  encoding using internal Huffman tables, as specified in the JPEG
  specification.

Certainly controls should be specified better.


As far as I can see ISO/IEC 10918-1 does not specify the precision or
signedness of the quantisation value Qvu. The default tables for 8-bit
baseline JPEG all fit into __u8 though.



Paragraph 4.7 of that spec, indicates the "sample" precision:
8-bit for baseline; 8-bit or 12-bit for extended.

For the quantization coefficients, the DQT segment contains a bit
that indicates if the quantization coefficients are 8-bit or 16-bit.
See B.2.4.1 for details.


See below (and Tq which follows the Pq field)




However there can be four sets of tables in non-baseline JPEG and it's


You lost me here, which four sets of tables are you refering to?


not clear (to me) whether 12-bit JPEG would need more precision (I'd
guess it would).


It seems it would. From B.2.4.1:

"An 8-bit DCT-based process shall not use a 16-bit precision quantization 
table."


Since this patch is defining UAPI I think it might be
good to build in some additional information, eg. number of tables,
element size. Maybe this can all be inferred from the selected pixel
format? If so then it would need documented that the above structure
only applies to baseline.



For quantization coefficients, I can only see two tables: one for luma
one for chroma. Huffman coefficients are a different story and we are
not really adding them here.


I was looking at the definition of Tqi in the frame header in B.2.2 
which seems to allow up to four (sets of?) quantization tables. 
Rereading it, it seems these are per component. Table B.2 implies that 
this applies to Baseline Sequential too. In the DQT marker description 
there's a Tq field to specify the destination for the new table. I think 
this means that an encoder can use up to four (sets of) tables and a 
decoder should be able to store four from the stream.


This may well not be relevant to the encoder under discussion, but if 
it's not allowed for in UAPI it's almost a given that it'll need to be 
added later.


BTW, my copy of the spec is dated 1993(E). Maybe it's out of date?



Since (if I understand this correctly) we would need u16 for extended precision
JPEG, shouldn't we use u16 instead of u8? That makes the control more generic.


This seems like a safer option to me.

Regards,
Ian



BTW, are the coefficients always uns

Re: [PATCH v4 5/6] media: Add controls for JPEG quantization tables

[Ian Arkver]

Hi,

On 03/09/2018 10:50, Hans Verkuil wrote:

On 08/31/2018 05:52 PM, Ezequiel Garcia wrote:

From: Shunqian Zheng 

Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
configure the JPEG quantization tables.

Signed-off-by: Shunqian Zheng 
Signed-off-by: Ezequiel Garcia 
---
  .../media/uapi/v4l/extended-controls.rst  | 23 +++
  .../media/videodev2.h.rst.exceptions  |  1 +
  drivers/media/v4l2-core/v4l2-ctrls.c  | 10 
  include/uapi/linux/v4l2-controls.h|  5 
  include/uapi/linux/videodev2.h|  1 +
  5 files changed, 40 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst 
b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..e0dd03e452de 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3354,7 +3354,30 @@ JPEG Control IDs
  Specify which JPEG markers are included in compressed stream. This
  control is valid only for encoders.
  
+.. _jpeg-quant-tables-control:
  
+``V4L2_CID_JPEG_QUANTIZATION (struct)``

+Specifies the luma and chroma quantization matrices for encoding
+or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The two matrices
+must be set in JPEG zigzag order, as per the JPEG specification.


Can you change "JPEG specification" to a reference to the JPEG spec entry
in bibio.rst?


+
+
+.. c:type:: struct v4l2_ctrl_jpeg_quantization
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_jpeg_quantization
+:header-rows:  0
+:stub-columns: 0
+:widths:   1 1 2
+
+* - __u8
+  - ``luma_quantization_matrix[64]``
+  - Sets the luma quantization table.
+
+* - __u8
+  - ``chroma_quantization_matrix[64]``
+  - Sets the chroma quantization table.


Just checking: the JPEG standard specifies this as unsigned 8-bit values as 
well?


As far as I can see ISO/IEC 10918-1 does not specify the precision or 
signedness of the quantisation value Qvu. The default tables for 8-bit 
baseline JPEG all fit into __u8 though.


However there can be four sets of tables in non-baseline JPEG and it's 
not clear (to me) whether 12-bit JPEG would need more precision (I'd 
guess it would). Since this patch is defining UAPI I think it might be 
good to build in some additional information, eg. number of tables, 
element size. Maybe this can all be inferred from the selected pixel 
format? If so then it would need documented that the above structure 
only applies to baseline.


Regards,
Ian



  
  .. flat-table::

  :header-rows:  0
diff --git a/Documentation/media/videodev2.h.rst.exceptions 
b/Documentation/media/videodev2.h.rst.exceptions
index ca9f0edc579e..a0a38e92bf38 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -129,6 +129,7 @@ replace symbol V4L2_CTRL_TYPE_STRING 
:c:type:`v4l2_ctrl_type`
  replace symbol V4L2_CTRL_TYPE_U16 :c:type:`v4l2_ctrl_type`
  replace symbol V4L2_CTRL_TYPE_U32 :c:type:`v4l2_ctrl_type`
  replace symbol V4L2_CTRL_TYPE_U8 :c:type:`v4l2_ctrl_type`
+replace symbol V4L2_CTRL_TYPE_JPEG_QUANTIZATION :c:type:`v4l2_ctrl_type`
  
  # V4L2 capability defines

  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 599c1cbff3b9..305bd7a9b7f1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -999,6 +999,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_JPEG_RESTART_INTERVAL:return "Restart Interval";
case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality";
case V4L2_CID_JPEG_ACTIVE_MARKER:   return "Active Markers";
+   case V4L2_CID_JPEG_QUANTIZATION:return "JPEG Quantization 
Tables";
  
  	/* Image source controls */

/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1286,6 +1287,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum 
v4l2_ctrl_type *type,
case V4L2_CID_DETECT_MD_REGION_GRID:
*type = V4L2_CTRL_TYPE_U8;
break;
+   case V4L2_CID_JPEG_QUANTIZATION:
+   *type = V4L2_CTRL_TYPE_JPEG_QUANTIZATION;
+   break;
case V4L2_CID_DETECT_MD_THRESHOLD_GRID:
*type = V4L2_CTRL_TYPE_U16;
break;
@@ -1612,6 +1616,9 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 
idx,
return -ERANGE;
return 0;
  
+	case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:

+   return 0;
+
default:
return -EINVAL;
}
@@ -2133,6 +2140,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct 
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_U32:
elem_size = sizeof(u32);
break;
+   case V4L2_CTRL_TYPE_JPEG_QUANTIZATION:
+  

Re: [PATCH] media: imx: shut up a false positive warning

[Ian Arkver]

Hi Mauro,

On 07/08/18 10:58, Mauro Carvalho Chehab wrote:

With imx, gcc produces a false positive warning:

drivers/staging/media/imx/imx-media-csi.c: In function 
'csi_idmac_setup_channel':
drivers/staging/media/imx/imx-media-csi.c:457:6: warning: this 
statement may fall through [-Wimplicit-fallthrough=]
   if (passthrough) {
  ^
drivers/staging/media/imx/imx-media-csi.c:464:2: note: here
  default:
  ^~~

That's because the regex it uses for fall trough is not
good enough. So, rearrange the fall through comment in a way
that gcc will recognize.

Signed-off-by: Mauro Carvalho Chehab 
---
  drivers/staging/media/imx/imx-media-csi.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 4647206f92ca..b7ffd231c64b 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -460,7 +460,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_cycles = incc->cycles;
break;
}
-   /* fallthrough for non-passthrough RGB565 (CSI-2 bus) */
+   /* for non-passthrough RGB565 (CSI-2 bus) */
+   /* Falls through */


Adding a '-' to the fallthrough seems to meet the regex requirements at 
level 3 of the warning. Eg...


/* fallthrough- for non-passthrough RGB565 (CSI-2 bus) */

Not sure if this is an improvement though.

Regards,
Ian


default:
burst_size = (image.pix.width & 0xf) ? 8 : 16;
passthrough_bits = 16;

Re: [PATCH 16/22] [media] tvp5150: add querystd

[Ian Arkver]

On 02/08/18 10:49, Mauro Carvalho Chehab wrote:

Em Thu, 2 Aug 2018 10:01:01 +0200
Marco Felsch  escreveu:


Hi Mauro,

On 18-08-01 12:50, Mauro Carvalho Chehab wrote:

Em Wed, 1 Aug 2018 16:49:26 +0200
Marco Felsch  escreveu:
   

Hi Mauro,

On 18-08-01 11:22, Mauro Carvalho Chehab wrote:

Em Wed, 1 Aug 2018 15:21:25 +0200
Marco Felsch  escreveu:
 

Hi Mauro,

On 18-07-30 15:09, Mauro Carvalho Chehab wrote:

Em Thu, 28 Jun 2018 18:20:48 +0200
Marco Felsch  escreveu:
   

From: Philipp Zabel 

Add the querystd video_op and make it return V4L2_STD_UNKNOWN while the
TVP5150 is not locked to a signal.

Signed-off-by: Philipp Zabel 
Signed-off-by: Marco Felsch 
---
  drivers/media/i2c/tvp5150.c | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 99d887936ea0..1990aaa17749 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd)
}
  }
  
+static int tvp5150_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)

+{
+   struct tvp5150 *decoder = to_tvp5150(sd);
+
+   *std_id = decoder->lock ? tvp5150_read_std(sd) : V4L2_STD_UNKNOWN;


This patch requires rework. What happens when a device doesn't have
IRQ enabled? Perhaps it should, instead, read some register in order
to check for the locking status, as this would work on both cases.


If IRQ isn't enabled, decoder->lock is set to always true during
probe(). So this case should be fine.


Not sure if tvp5150_read_std() will do the right thing. If it does,
the above could simply be:
std_id = tvp5150_read_std(sd);

But, as there are 3 variants of this chipset, it sounds safer to check
if the device is locked before calling tvp5150_read_std().


Yes, I'm with you.
   


IMHO, the best would be to have a patch like the one below.

Regards,
Mauro

[PATCH] media: tvp5150: implement decoder lock when irq is not used

When irq is used, the lock is set via IRQ code. When it isn't,
the driver just assumes it is always locked. Instead, read the
lock status from the status register.


Yes, that is a better solution.
   


Compile-tested only.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 75e5ffc6573d..e07020d4053d 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -811,11 +811,24 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev 
*sd)
}
  }
  
+static int query_lock(struct v4l2_subdev *sd)

+{
+   struct tvp5150 *decoder = to_tvp5150(sd);
+   int status;
+
+   if (decoder->irq)
+   return decoder->lock;
+
+   regmap_read(map, TVP5150_INT_STATUS_REG_A, );
+
+   return (status & 0x06) == 0x06;


Typo? It should be 0x80, as described in the datasheet (SLES209E) or
just use the TVP5150_INT_A_LOCK_STATUS define. This avoid datasheet
cross check during reading.


Yes, it is a typo, but at the other line... I meant to use the register
0x88, e. g.:

regmap_read(decoder->regmap, TVP5150_STATUS_REG_1, );


During my development I tried this status register too, as descibed on
the community website [1]. But that wasn't that good, because the look
will be lost very often. Bit7 of Interrupt Status Reg A (0xc0) is more
robust for that kind of work, since it covers the whole signal.

[1] http://e2e.ti.com/support/data_converters/videoconverters/f/918/p/ \
617120/2273276?keyMatch=tvp5150%20lock%20lost=Search-EN-Support


Bit 4 is not reliable for such purpose, but, on my tests, when video is
present, bits 1, 2 and 3 are present when there is a proper signal.
Basically, all the times I issued a std query ioctl, it reads 0x1e.

When the signal is removed, I get a 0x00.

Here, reading int status reg A returns 0x40 with or without signal,
probably because IRQ is not enabled. See, for USB devices like em28xx,
we don't have direct access to tvp5051 IRQ line. If the IRQ lines is
somewhat wired to em28xx, it could be possible that the em28xx would
handle it, but we would need to know a way to setup em28xx to handle irqs.
I don't know if this is possible, and, if so, how em28xx would be
notifying such interrupts via some URB packet.


The interrupt status register bits are latched and need a 1 written to 
clear them. Normally this would be done by the interrupt handler. Maybe 
this is why you're seeing the LOCK bit stuck?


Regards,
Ian





I ran some tests here: the int status reg is not updated.

Also, after thinking a little bit, I opted to not use the query_lock()
at s_stream. It makes no sense there without adding a status polling
logic. I also opted to remove initializing decoder->lock to true, as
this is very counter-intuitive. Instead, I'm adding a test at s_stream
if decoder->irq is set. This makes easier to understand the code.


Yes, you're right.


Btw, on my tests here, I noticed a problem with S-Video... at least with

Re: [PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Ian Arkver]

On 28/05/18 08:00, Philipp Zabel wrote:

On Fri, 2018-05-25 at 16:53 -0700, Steve Longerbeam wrote:

Interlaced scan, a.k.a. interweave, should be enabled at the CSI IDMAC
output pad if the input field type is 'alternate' (in addition to field
types 'seq-tb' and 'seq-bt').

Which brings up whether V4L2_FIELD_HAS_BOTH() macro should be used
to determine enabling interlaced/interweave scan. That macro
includes the 'interlaced' field types, and in those cases the data
is already interweaved with top/bottom field lines. A heads-up for
now that this if statement may need to call V4L2_FIELD_IS_SEQUENTIAL()
instead, I have no sensor hardware that sends 'interlaced' data, so can't
test.


I agree, the check here should be IS_SEQUENTIAL || ALTERNATE, and
interlaced_scan should also be enabled if image.pix.field is
INTERLACED_TB/BT.
And for INTERLACED_TB/BT input, the logic should be inverted: then we'd
have to enable interlaced_scan whenever image.pix.field is SEQ_BT/TB.


Hi Philipp,

If your intent here is to de-interweave the two fields back to two 
sequential fields, I don't believe the IDMAC operation would achieve 
that. It's basically line stride doubling and a line offset for the 
lines in the 2nd half of the incoming frame, right?


Regards,
Ian


regards
Philipp


Signed-off-by: Steve Longerbeam 
---
  drivers/staging/media/imx/imx-media-csi.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 9bc555c..eef3483 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -477,7 +477,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
ipu_smfc_set_burstsize(priv->smfc, burst_size);
  
  	if (image.pix.field == V4L2_FIELD_NONE &&

-   V4L2_FIELD_HAS_BOTH(infmt->field))
+   (V4L2_FIELD_HAS_BOTH(infmt->field) ||
+infmt->field == V4L2_FIELD_ALTERNATE))
ipu_cpmem_interlaced_scan(priv->idmac_ch,
  image.pix.bytesperline);
  

Re: [PATCH v4 09/10] v4l: xilinx: dma: Add multi-planar support

[Ian Arkver]

Hi Satish,

On 01/05/18 02:35, Satish Kumar Nagireddy wrote:

The current v4l driver supports single plane formats. This patch
adds support to handle multi-planar formats. Driver can handle
both single and multi-planar formats.

Signed-off-by: Satish Kumar Nagireddy 
---
  drivers/media/platform/xilinx/xilinx-dma.c  | 159 ++--
  drivers/media/platform/xilinx/xilinx-dma.h  |   4 +-
  drivers/media/platform/xilinx/xilinx-vipp.c |  16 +--
  3 files changed, 111 insertions(+), 68 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-dma.c 
b/drivers/media/platform/xilinx/xilinx-dma.c
index 658586e..a714057 100644
--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -74,8 +74,8 @@ static int xvip_dma_verify_format(struct xvip_dma *dma)
return ret == -ENOIOCTLCMD ? -EINVAL : ret;
  
  	if (dma->fmtinfo->code != fmt.format.code ||

-   dma->format.height != fmt.format.height ||
-   dma->format.width != fmt.format.width)
+   dma->format.fmt.pix_mp.width != fmt.format.width ||
+   dma->format.fmt.pix_mp.height != fmt.format.height)
return -EINVAL;
  
  	return 0;

@@ -310,7 +310,8 @@ static void xvip_dma_complete(void *param)
buf->buf.field = V4L2_FIELD_NONE;
buf->buf.sequence = dma->sequence++;
buf->buf.vb2_buf.timestamp = ktime_get_ns();
-   vb2_set_plane_payload(>buf.vb2_buf, 0, dma->format.sizeimage);
+   vb2_set_plane_payload(>buf.vb2_buf, 0,
+ dma->format.fmt.pix_mp.plane_fmt[0].sizeimage);
vb2_buffer_done(>buf.vb2_buf, VB2_BUF_STATE_DONE);
  }
  
@@ -320,13 +321,15 @@ xvip_dma_queue_setup(struct vb2_queue *vq,

 unsigned int sizes[], struct device *alloc_devs[])
  {
struct xvip_dma *dma = vb2_get_drv_priv(vq);
+   s32 sizeimage;
  
  	/* Make sure the image size is large enough. */

+   sizeimage = dma->format.fmt.pix_mp.plane_fmt[0].sizeimage;
if (*nplanes)
-   return sizes[0] < dma->format.sizeimage ? -EINVAL : 0;
+   return sizes[0] < sizeimage ? -EINVAL : 0;
  
  	*nplanes = 1;

-   sizes[0] = dma->format.sizeimage;
+   sizes[0] = sizeimage;
  
  	return 0;

  }
@@ -350,14 +353,15 @@ static void xvip_dma_buffer_queue(struct vb2_buffer *vb)
struct dma_async_tx_descriptor *desc;
dma_addr_t addr = vb2_dma_contig_plane_dma_addr(vb, 0);
u32 flags;
+   struct v4l2_pix_format_mplane *pix_mp = >format.fmt.pix_mp;
  
-	if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {

+   if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
dma->xt.dir = DMA_DEV_TO_MEM;
dma->xt.src_sgl = false;
dma->xt.dst_sgl = true;
dma->xt.dst_start = addr;
-   } else {
+   } else if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
dma->xt.dir = DMA_MEM_TO_DEV;
dma->xt.src_sgl = true;
@@ -365,10 +369,11 @@ static void xvip_dma_buffer_queue(struct vb2_buffer *vb)
dma->xt.src_start = addr;
}
  
-	dma->xt.frame_size = 1;

-   dma->sgl[0].size = dma->format.width * dma->fmtinfo->bpp[0];
-   dma->sgl[0].icg = dma->format.bytesperline - dma->sgl[0].size;
-   dma->xt.numf = dma->format.height;
+   dma->xt.frame_size = dma->fmtinfo->num_planes;
+   dma->sgl[0].size = pix_mp->width * dma->fmtinfo->bpp[0];
+   dma->sgl[0].icg = pix_mp->plane_fmt[0].bytesperline - dma->sgl[0].size;
+   dma->xt.numf = pix_mp->height;
+   dma->sgl[0].dst_icg = 0;
  
  	desc = dmaengine_prep_interleaved_dma(dma->dma, >xt, flags);

if (!desc) {
@@ -497,10 +502,15 @@ xvip_dma_querycap(struct file *file, void *fh, struct 
v4l2_capability *cap)
cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
  | dma->xdev->v4l2_caps;
  
-	if (dma->queue.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)

-   cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
-   else
-   cap->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
+   cap->device_caps = V4L2_CAP_STREAMING;
+   switch (dma->queue.type) {
+   case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+   cap->device_caps |= V4L2_CAP_VIDEO_CAPTURE_MPLANE;
+   break;
+   case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+   cap->device_caps |= V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+   break;
+   }
  
  	strlcpy(cap->driver, "xilinx-vipp", sizeof(cap->driver));

strlcpy(cap->card, dma->video.name, sizeof(cap->card));
@@ -524,7 +534,7 @@ xvip_dma_enum_format(struct file *file, void *fh, struct 
v4l2_fmtdesc *f)
if (f->index > 0)
return 

Re: [PATCH] media: coda: do not try to propagate format if capture queue busy

[Ian Arkver]

On 28/03/18 18:12, Philipp Zabel wrote:

The driver helpfully resets the capture queue format and selection
rectangle whenever output format is changed. This only works while
the capture queue is not busy.

Signed-off-by: Philipp Zabel 
---
  drivers/media/platform/coda/coda-common.c | 28 +++-
  1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 04e35d70ce2e..d3e22c14fad4 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -786,9 +786,8 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
  struct v4l2_format *f)
  {
struct coda_ctx *ctx = fh_to_ctx(priv);
-   struct coda_q_data *q_data_src;

see below

struct v4l2_format f_cap;
-   struct v4l2_rect r;
+   struct vb2_queue *dst_vq;
int ret;
  
  	ret = coda_try_fmt_vid_out(file, priv, f);

@@ -804,23 +803,26 @@ static int coda_s_fmt_vid_out(struct file *file, void 
*priv,
ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
ctx->quantization = f->fmt.pix.quantization;
  
+	dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);

+   if (!dst_vq)
+   return -EINVAL;
+
+   /*
+* Setting the capture queue format is not possible while the capture
+* queue is still busy. This is not an error, but the user will have to
+* make sure themselves that the capture format is set correctly before
+* starting the output queue again.
+*/
+   if (vb2_is_busy(dst_vq))
+   return 0;
+
memset(_cap, 0, sizeof(f_cap));
f_cap.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
coda_g_fmt(file, priv, _cap);
f_cap.fmt.pix.width = f->fmt.pix.width;
f_cap.fmt.pix.height = f->fmt.pix.height;
  
-	ret = coda_try_fmt_vid_cap(file, priv, _cap);

-   if (ret)
-   return ret;
-
-   q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
-   r.left = 0;
-   r.top = 0;
-   r.width = q_data_src->width;
-   r.height = q_data_src->height;
-
-   return coda_s_fmt(ctx, _cap, );
+   return coda_s_fmt_vid_cap(file, priv, _cap);


Is this chunk (and removal of q_data_src above) part of the same patch? 
It doesn't seem covered by the subject line.


Regards,
Ian


  }
  
  static int coda_reqbufs(struct file *file, void *priv,

Re: [PATCH] media: imx-media-internal-sd: Use memset to clear pdevinfo struct

[Ian Arkver]

On 09/02/18 21:17, Ian Arkver wrote:

On 09/02/18 16:36, Fabio Estevam wrote:

When building with W=1 the following warning shows up:

drivers/staging/media/imx/imx-media-internal-sd.c:274:49: warning: 
Using plain integer as NULL pointer


Fix this problem by using memset() to clear the pdevinfo structure.


Hi Fabio,

I thought initializers were preferred to memset. I think the problem 
here is the first element of the struct is a pointer. Maybe this would 
be better?


struct platform_device_info pdevinfo = {NULL};


Actually an empty initialiser list would be better again.

eg:
https://patchwork.kernel.org/patch/9480131/


I see similar patches, for example:
https://patchwork.kernel.org/patch/10095129/

Regards,
IanJ


Signed-off-by: Fabio Estevam <fabio.este...@nxp.com>
---
  drivers/staging/media/imx/imx-media-internal-sd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c 
b/drivers/staging/media/imx/imx-media-internal-sd.c

index 70833fe503b5..377c20863b76 100644
--- a/drivers/staging/media/imx/imx-media-internal-sd.c
+++ b/drivers/staging/media/imx/imx-media-internal-sd.c
@@ -271,7 +271,7 @@ static int add_internal_subdev(struct 
imx_media_dev *imxmd,

 int ipu_id)
  {
  struct imx_media_internal_sd_platformdata pdata;
-    struct platform_device_info pdevinfo = {0};
+    struct platform_device_info pdevinfo;
  struct platform_device *pdev;
  pdata.grp_id = isd->id->grp_id;
@@ -283,6 +283,7 @@ static int add_internal_subdev(struct 
imx_media_dev *imxmd,

  imx_media_grp_id_to_sd_name(pdata.sd_name, sizeof(pdata.sd_name),
  pdata.grp_id, ipu_id);
+    memset(, 0, sizeof(pdevinfo));
  pdevinfo.name = isd->id->name;
  pdevinfo.id = ipu_id * num_isd + isd->id->index;
  pdevinfo.parent = imxmd->md.dev;

Re: [PATCH] media: imx-media-internal-sd: Use memset to clear pdevinfo struct

[Ian Arkver]

On 09/02/18 16:36, Fabio Estevam wrote:

When building with W=1 the following warning shows up:

drivers/staging/media/imx/imx-media-internal-sd.c:274:49: warning: Using plain 
integer as NULL pointer

Fix this problem by using memset() to clear the pdevinfo structure.


Hi Fabio,

I thought initializers were preferred to memset. I think the problem 
here is the first element of the struct is a pointer. Maybe this would 
be better?


struct platform_device_info pdevinfo = {NULL};

I see similar patches, for example:
https://patchwork.kernel.org/patch/10095129/

Regards,
IanJ


Signed-off-by: Fabio Estevam 
---
  drivers/staging/media/imx/imx-media-internal-sd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c 
b/drivers/staging/media/imx/imx-media-internal-sd.c
index 70833fe503b5..377c20863b76 100644
--- a/drivers/staging/media/imx/imx-media-internal-sd.c
+++ b/drivers/staging/media/imx/imx-media-internal-sd.c
@@ -271,7 +271,7 @@ static int add_internal_subdev(struct imx_media_dev *imxmd,
   int ipu_id)
  {
struct imx_media_internal_sd_platformdata pdata;
-   struct platform_device_info pdevinfo = {0};
+   struct platform_device_info pdevinfo;
struct platform_device *pdev;
  
  	pdata.grp_id = isd->id->grp_id;

@@ -283,6 +283,7 @@ static int add_internal_subdev(struct imx_media_dev *imxmd,
imx_media_grp_id_to_sd_name(pdata.sd_name, sizeof(pdata.sd_name),
pdata.grp_id, ipu_id);
  
+	memset(, 0, sizeof(pdevinfo));

pdevinfo.name = isd->id->name;
pdevinfo.id = ipu_id * num_isd + isd->id->index;
pdevinfo.parent = imxmd->md.dev;

Re: [PATCH v2 9/9] [media] Add documentation for YUV420 bus format

[Ian Arkver]

On 09/02/18 01:22, Satish Kumar Nagireddy wrote:

The code is MEDIA_BUS_FMT_VYYUYY8_1X24

Signed-off-by: Satish Kumar Nagireddy 
---
  Documentation/media/uapi/v4l/subdev-formats.rst | 34 +
  1 file changed, 34 insertions(+)

diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst 
b/Documentation/media/uapi/v4l/subdev-formats.rst
index b1eea44..a4d7d87 100644
--- a/Documentation/media/uapi/v4l/subdev-formats.rst
+++ b/Documentation/media/uapi/v4l/subdev-formats.rst
@@ -7283,6 +7283,40 @@ The following table list existing packed 48bit wide YUV 
formats.
- y\ :sub:`1`
- y\ :sub:`0`

+  - MEDIA_BUS_FMT_VYYUYY8_1X24
+  - 0x202c
+  -
+  -
+  -
+  -
+  -
+  -
+  -
+  -
+  - v\ :sub:`7`
+  - v\ :sub:`6`
+  - v\ :sub:`5`
+  - v\ :sub:`4`
+  - v\ :sub:`3`
+  - v\ :sub:`2`
+  - v\ :sub:`1`
+  - v\ :sub:`0`
+  - u\ :sub:`7`
+  - u\ :sub:`6`
+  - u\ :sub:`5`
+  - u\ :sub:`4`
+  - u\ :sub:`3`
+  - u\ :sub:`2`
+  - u\ :sub:`1`
+  - u\ :sub:`0`
+  - y\ :sub:`7`
+  - y\ :sub:`6`
+  - y\ :sub:`5`
+  - y\ :sub:`4`
+  - y\ :sub:`3`
+  - y\ :sub:`2`
+  - y\ :sub:`1`
+  - y\ :sub:`0`


If this bus format name doesn't really describe how the pixels are sent 
on the bus, maybe the documentation should? Is this for something like 
MIPI CSI-2 YUV420 non-legacy modes where the bus format alternates on 
odd/even lines?


Regards,
IanJ



  .. raw:: latex

--
2.7.4

This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Ian Arkver]

On 21/09/17 12:04, Ian Arkver wrote:

Hi Philipp

On 21/09/17 11:24, Philipp Zabel wrote:

g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the tc358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported.
To allow communicating the number of currently active lanes, add a new
bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
a better solution is found.

Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
  drivers/media/i2c/tc358743.c  | 22 --
  include/media/v4l2-mediabus.h |  8 
  2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c363ccab5..e2a9e6a18a49d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct 
v4l2_subdev *sd,
  /* Support for non-continuous CSI-2 clock is missing in the 
driver */

  cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
-    switch (state->csi_lanes_in_use) {
-    case 1:
+    if (state->bus.num_data_lanes > 0)
  cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
-    break;
-    case 2:
+    if (state->bus.num_data_lanes > 1)
  cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
-    break;
-    case 3:
+    if (state->bus.num_data_lanes > 2)
  cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
-    break;
-    case 4:
+    if (state->bus.num_data_lanes > 3)
  cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
-    break;
-    default:
+
+    if (state->csi_lanes_in_use > 4)
  return -EINVAL;
+


My understanding of Hans' comment:
"I'd also add a comment that all other flags must be 0 if the device 
tree is used. This to avoid mixing the two."


is that all the above should only happen if (!!state->pdata).


Except that state->pdata is a copy of the pdata, not a pointer, but you 
know what I mean. Some other check for DT needed here.



I don't know if this would break any existing DT-using bridge drivers.

Regards,
Ian

[snip]

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Ian Arkver]

Hi Philipp

On 21/09/17 11:24, Philipp Zabel wrote:

g_mbus_config was supposed to indicate all supported lane numbers, not
only the number of those currently in active use. Since the tc358743
can dynamically reduce the number of active lanes if the required
bandwidth allows for it, report all lane numbers up to the connected
number of lanes as supported.
To allow communicating the number of currently active lanes, add a new
bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
a better solution is found.

Signed-off-by: Philipp Zabel 
---
  drivers/media/i2c/tc358743.c  | 22 --
  include/media/v4l2-mediabus.h |  8 
  2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c363ccab5..e2a9e6a18a49d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct v4l2_subdev 
*sd,
/* Support for non-continuous CSI-2 clock is missing in the driver */
cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
  
-	switch (state->csi_lanes_in_use) {

-   case 1:
+   if (state->bus.num_data_lanes > 0)
cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
-   break;
-   case 2:
+   if (state->bus.num_data_lanes > 1)
cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
-   break;
-   case 3:
+   if (state->bus.num_data_lanes > 2)
cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
-   break;
-   case 4:
+   if (state->bus.num_data_lanes > 3)
cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
-   break;
-   default:
+
+   if (state->csi_lanes_in_use > 4)
return -EINVAL;
+


My understanding of Hans' comment:
"I'd also add a comment that all other flags must be 0 if the device 
tree is used. This to avoid mixing the two."


is that all the above should only happen if (!!state->pdata).

I don't know if this would break any existing DT-using bridge drivers.

Regards,
Ian


+   if (state->csi_lanes_in_use < state->bus.num_data_lanes) {
+   const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
+
+   cfg->flags |= (state->csi_lanes_in_use << __ffs(mask)) & mask;
}
  
  	return 0;

@@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
if (pdata) {
state->pdata = *pdata;
state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   state->bus.num_data_lanes = 4;
} else {
err = tc358743_probe_of(state);
if (err == -ENODEV)
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 93f8afcb7a220..3467d97be5f5b 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -63,6 +63,14 @@
 V4L2_MBUS_CSI2_3_LANE | 
V4L2_MBUS_CSI2_4_LANE)
  #define V4L2_MBUS_CSI2_CHANNELS   (V4L2_MBUS_CSI2_CHANNEL_0 | 
V4L2_MBUS_CSI2_CHANNEL_1 | \
 V4L2_MBUS_CSI2_CHANNEL_2 | 
V4L2_MBUS_CSI2_CHANNEL_3)
+/*
+ * Number of lanes in use, 0 == use all available lanes (default)
+ *
+ * This is a temporary fix for devices that need to reduce the number of active
+ * lanes for certain modes, until g_mbus_config() can be replaced with a better
+ * solution.
+ */
+#define V4L2_MBUS_CSI2_LANE_MASK(3 << 10)
  
  /**

   * enum v4l2_mbus_type - media bus type

Re: [PATCH] [media] coda: disable BWB only while decoding on CODA 960

[Ian Arkver]

On 19/07/17 11:06, Philipp Zabel wrote:

Disabling the BWB works around hangups observed while decoding. Since no
issues have been observed while encoding, and disabling BWB also reduces
encoding performance, reenable it for encoding.


Thanks for the speedy patch Philipp.

I can only test encode, but I can confirm that this patch restores the 
VPU encode performance. With that limitation...


Tested-by: Ian Arkver <ian.arkver@gmail.com>


Cc: Ian Arkver <ian.arkver@gmail.com>
Reported-by: Ian Arkver <ian.arkver@gmail.com>
Fixes: 89ed025d5c53 ("[media] coda: disable BWB for all codecs on CODA 960")
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

On 19/07/17 10:32, Philipp Zabel wrote:

On Wed, 2017-07-19 at 10:15 +0100, Ian Arkver wrote:

On 19/07/17 09:09, Philipp Zabel wrote:

Hi Ian,

On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:

Hi Philipp,

On 02/03/17 10:19, Philipp Zabel wrote:

Side effects are reduced burst lengths when writing out decoded frames
to memory, so there is an "enable_bwb" module parameter to turn it back
on.


These side effects are dramatically reducing the VPU throughput during
H.264 encode as well. Prior to this patch I was just about managing to
capture and stream 1080p25 H.264. After this change it fell to about
19fps. Reverting this patch (or presumably using the module param)
restores the frame rate.

Can we at least make this decode specific? The VPU library patches do it
in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream
start would be OK.


Yes, since ENGR00293425 only talks about decoders, and I haven't seen
any BWB related hangups during encoding yet, I'm inclined to agree.


I took a look at where ctx->frame_mem_ctrl is used, i.e.
coda_start_encoding and __coda_start_decoding. Is it possible to have
one instance of coda open for encode and one for decode at the same
time?


Sure, when transcoding with a decoder / encoder pair, for example.


If so, how is the CODA_REG_BIT_FRAME_MEM_CTRL setting
updated between runs, eg. for differing CODA_FRAME_CHROMA_INTERLEAVE?


Yes. coda_command_async writes ctx->frame_mem_ctrl to
CODA_REG_BIT_FRAME_MEM_CTRL with each PIC_RUN command at from
prepare_encode/decode.


Totally missed that somehow. Thanks for the pointer.


This code is in the start_streaming path rather than the prepare_run
path. Does each context switch stop & restart streaming?


No, see above.

regards
Philipp

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

On 19/07/17 09:09, Philipp Zabel wrote:

Hi Ian,

On Wed, 2017-07-19 at 08:16 +0100, Ian Arkver wrote:

Hi Philipp,

On 02/03/17 10:19, Philipp Zabel wrote:

Side effects are reduced burst lengths when writing out decoded frames
to memory, so there is an "enable_bwb" module parameter to turn it back
on.


These side effects are dramatically reducing the VPU throughput during
H.264 encode as well. Prior to this patch I was just about managing to
capture and stream 1080p25 H.264. After this change it fell to about
19fps. Reverting this patch (or presumably using the module param)
restores the frame rate.

Can we at least make this decode specific? The VPU library patches do it
in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream
start would be OK.


Yes, since ENGR00293425 only talks about decoders, and I haven't seen
any BWB related hangups during encoding yet, I'm inclined to agree.


I took a look at where ctx->frame_mem_ctrl is used, i.e.
coda_start_encoding and __coda_start_decoding. Is it possible to have
one instance of coda open for encode and one for decode at the same
time? If so, how is the CODA_REG_BIT_FRAME_MEM_CTRL setting
updated between runs, eg. for differing CODA_FRAME_CHROMA_INTERLEAVE?
This code is in the start_streaming path rather than the prepare_run
path. Does each context switch stop & restart streaming?

Regards,
Ian

Re: [PATCH] [media] coda: disable BWB for all codecs on CODA 960

[Ian Arkver]

Hi Philipp,

On 02/03/17 10:19, Philipp Zabel wrote:

I don't know what the BWB unit is, I guess W is for write and one of the
Bs is for burst. All I know is that there repeatedly have been issues
with it hanging on certain streams (ENGR00223231, ENGR00293425), with
various firmware versions, sometimes blocking something related to the
GDI bus or the GDI AXI adapter. There are some error cases that we don't
know how to recover from without a reboot. Apparently this unit can be
disabled by setting bit 12 in the FRAME_MEM_CTRL mailbox register to
zero, so do that to avoid crashes.


Both those FSL ENGR patches to Android and VPU lib are specific to 
decode, with the first being specific to VC1 decode only.



Side effects are reduced burst lengths when writing out decoded frames
to memory, so there is an "enable_bwb" module parameter to turn it back
on.


These side effects are dramatically reducing the VPU throughput during 
H.264 encode as well. Prior to this patch I was just about managing to 
capture and stream 1080p25 H.264. After this change it fell to about 
19fps. Reverting this patch (or presumably using the module param) 
restores the frame rate.


Can we at least make this decode specific? The VPU library patches do it 
in vpu_DecOpen. I'd guess disabling the BWB any time prior to stream 
start would be OK.


It might also be worth adding some sort of /* HACK */ marker, since 
disabling the BWB feels very like a hack to me.


Regards,
Ian

Re: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ian Arkver]

On 14/06/17 08:18, Ramesh Shanmugasundaram wrote:

Subject: Re: [PATCH v2 0/2] Avoid namespace collision within macros &
tidyup

On 13/06/17 14:33, Ramesh Shanmugasundaram wrote:

Hi All,

The readx_poll_timeout & similar macros defines local variable that
can cause name space collision with the caller. Fixed this issue by
prefixing them with underscores.


The compound statement has a local variable scope, so these won't collide
with the caller I believe.


But xxx_poll_timeout is a macro??

Usage regmap_read_poll_timeout(..., timeout) with variable name "timeout" in 
the caller results in

include/linux/regmap.h:123:20: warning: 'timeout' is used uninitialized in this 
function [-Wuninitialized]
   ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \



Oh right, collide with a passed in variable, yes. Sorry.




Also tidied couple of instances where the macro arguments are used in
expressions without paranthesis.

This patchset is based on top of today's linux-next repo.
commit bc4c75f41a1c ("Add linux-next specific files for 20170613")

Change history:

v2:
   - iopoll.h:
- Enclosed timeout_us & sleep_us arguments with paranthesis
   - regmap.h:
- Enclosed timeout_us & sleep_us arguments with paranthesis
- Renamed pollret to __ret

Note: timeout_us cause spare check warning as identified here [1].

[1]
https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg1513
8.html

Thanks,
Ramesh

Ramesh Shanmugasundaram (2):
iopoll: Avoid namespace collision within macros & tidyup
regmap: Avoid namespace collision within macro & tidyup

   include/linux/iopoll.h | 12 +++-
   include/linux/regmap.h | 17 +
   2 files changed, 16 insertions(+), 13 deletions(-)

Re: [PATCH v2 0/2] Avoid namespace collision within macros & tidyup

[Ian Arkver]

On 13/06/17 14:33, Ramesh Shanmugasundaram wrote:

Hi All,

The readx_poll_timeout & similar macros defines local variable that can
cause name space collision with the caller. Fixed this issue by prefixing
them with underscores.


The compound statement has a local variable scope, so these won't 
collide with the caller I believe.



Also tidied couple of instances where the macro
arguments are used in expressions without paranthesis.

This patchset is based on top of today's linux-next repo.
commit bc4c75f41a1c ("Add linux-next specific files for 20170613")

Change history:

v2:
  - iopoll.h:
- Enclosed timeout_us & sleep_us arguments with paranthesis
  - regmap.h:
- Enclosed timeout_us & sleep_us arguments with paranthesis
- Renamed pollret to __ret

Note: timeout_us cause spare check warning as identified here [1].

[1] https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg15138.html

Thanks,
Ramesh

Ramesh Shanmugasundaram (2):
   iopoll: Avoid namespace collision within macros & tidyup
   regmap: Avoid namespace collision within macro & tidyup

  include/linux/iopoll.h | 12 +++-
  include/linux/regmap.h | 17 +
  2 files changed, 16 insertions(+), 13 deletions(-)

Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver

[Ian Arkver]

On 31/01/17 22:04, Russell King - ARM Linux wrote:

On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote:

On 31/01/17 20:33, Russell King - ARM Linux wrote:

On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote:

On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote:

On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote:

Should be set to something like 'platform:imx-media-camif'. v4l2-compliance
should complain about this.

... and more.


Right, in version 3 that you are working with, no v4l2-compliance fixes were
in yet. A lot of the compliance errors are fixed, please look in latest
branch
imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git.


Sorry, I'm not prepared to pull random trees from github as there's
no easy way to see what's in the branch.

I've always disliked github because its web interface makes it soo
difficult to navigate around git trees hosted there.  You can see
a commit, you can see a diff of the commit.  You can get a list of
branches.  But there seems to be no way to get a list of commits
similar to "git log" or even a one-line summary of each commit on
a branch.  If there is, it's completely non-obvious (which I think is
much of the problem with github, it's web interface is horrendous.)

Or you can clone/pull the tree without knowing what you're fetching
(eg, what the tree is based upon.)

Or you can waste time clicking repeatedly on the "parent" commit link
on each patch working your way back through the history...

Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work

>from the linux-media tree (I didn't try and go back any further.)

As I don't want to take a whole pile of other changes into my tree,
I'm certainly not going to pull from your github tree.  Sorry.



https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip

It's under the "Compare" button from the main view. It would be nice though
if the first commit's parent was some clearly tagged start point.


I don't want master though, I want v4.10-rc1, and if I ask for that
it tells me it knows nothing about v4.10-rc1, despite the fact that's
a tag in the mainline kernel repository which was merged into the
linux-media tree that this tree is based upon.



Yeah, that's what I meant about the first parent's commit not being a 
clearly tagged branch point. At least you get the series on one page. 
Maybe it's time for a rebase or a v4 series Steve?


Personally, I use a bare repo with multiple remotes and fetch branches 
from various trees. Then gitk --all --since(etc) is pretty good at 
giving the overview picture. You don't need to pull the commits over 
into any of your working branches if you don't want to.


Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver

[Ian Arkver]

On 31/01/17 20:33, Russell King - ARM Linux wrote:

On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote:

On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote:

On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote:

Should be set to something like 'platform:imx-media-camif'. v4l2-compliance
should complain about this.

... and more.


Right, in version 3 that you are working with, no v4l2-compliance fixes were
in yet. A lot of the compliance errors are fixed, please look in latest
branch
imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git.


Sorry, I'm not prepared to pull random trees from github as there's
no easy way to see what's in the branch.

I've always disliked github because its web interface makes it soo
difficult to navigate around git trees hosted there.  You can see
a commit, you can see a diff of the commit.  You can get a list of
branches.  But there seems to be no way to get a list of commits
similar to "git log" or even a one-line summary of each commit on
a branch.  If there is, it's completely non-obvious (which I think is
much of the problem with github, it's web interface is horrendous.)

Or you can clone/pull the tree without knowing what you're fetching
(eg, what the tree is based upon.)

Or you can waste time clicking repeatedly on the "parent" commit link
on each patch working your way back through the history...

Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work
from the linux-media tree (I didn't try and go back any further.)
As I don't want to take a whole pile of other changes into my tree,
I'm certainly not going to pull from your github tree.  Sorry.



https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip

It's under the "Compare" button from the main view. It would be nice 
though if the first commit's parent was some clearly tagged start point.


Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] [media] s5k6aa: set usleep_range greater 0

[Ian Arkver]

On 13/12/16 09:43, Sakari Ailus wrote:

Hi Nicholas,

On Tue, Dec 13, 2016 at 02:58:02AM +0100, Nicholas Mc Guire wrote:

As this is not in atomic context and it does not seem like a critical
timing setting a range of 1ms allows the timer subsystem to optimize
the hrtimer here.

I'd suggest not to. These delays are often directly visible to the user in
use cases where attention is indeed paid to milliseconds.

The same applies to register accesses. An delay of 0 to 100 µs isn't much as
such, but when you multiply that with the number of accesses it begins to
add up.

Data sheet for this device [1] says STBYN deassertion to RSTN 
deassertion should be >50us, though this is actually referenced to MCLK 
startup. See Figure 36, Power-Up Sequence, page 42.


I think the usleep range here could be greatly reduced and opened up to 
allow hr timer tweaks if desired.


[1] http://www.bdtic.com/DataSheet/SAMSUNG/S5K6AAFX13.pdf

Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Ian Arkver]

On 26/10/16 15:07, Todor Tomov wrote:

Hi,

On 10/26/2016 03:48 PM, Ian Arkver wrote:

[snip]

+static int ov5645_regulators_enable(struct ov5645 *ov5645)
+{
+int ret;
+
+ret = regulator_enable(ov5645->io_regulator);
+if (ret < 0) {
+dev_err(ov5645->dev, "set io voltage failed\n");
+return ret;
+}
+
+ret = regulator_enable(ov5645->core_regulator);
+if (ret) {
+dev_err(ov5645->dev, "set core voltage failed\n");
+goto err_disable_io;
+}
+
+ret = regulator_enable(ov5645->analog_regulator);
+if (ret) {
+dev_err(ov5645->dev, "set analog voltage failed\n");
+goto err_disable_core;
+}

How about using the regulator bulk API ? This would simplify the enable
and disable functions.

The driver must enable the regulators in this order. I can see in the
implementation of the bulk api that they are enabled again in order
but I don't see stated anywhere that it is guaranteed to follow the
same order in future. I'd prefer to keep it explicit as it is now.

I believe it should be an API guarantee, otherwise many drivers using the bulk
API would break. Mark, could you please comment on that ?

Ok, let's wait for a response from Mark.

I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
datasheets. Both of these show that AVDD should come up before DVDD where DVDD 
is externally supplied, although the minimum delay between them is 0ms. Both 
datasheets also imply that this requirement is only to allow host SCCB access 
on a shared I2C bus while the device is powering up. They imply that if one 
waits 20ms after power on then SCCB will be fine without this sequencing. Your 
code includes an msleep(20);

There is also requirement that DOVDD should become stable before AVDD (in both 
cases -
external or internal DVDD).

Aren't these requirements needed to allow I2C access to another device on the 
same I2C bus even during these 20ms?
Well, it's a really obscure corner case where these rails are actually 
switched and the rise times are all available to the regulator framework 
(so that there's a difference between three distinct calls to 
regulator_enable and one call to the ASYNC_DOMAIN driven bulk enable) 
and the I2C bus is shared with another device that is being accessed at 
the same time as the sensor is enabled and the sensor breaks that access.


Having said that, really obscure corner cases are what lurk around and 
catch you unawares years in the future, so maybe three calls is 
necessary here. However, I think analog should be enabled before core - 
check with your datasheet if you have the correct one.


Regards,
Ian




Further, the reference schematic for the OV5647 shows three separate LDOs with 
no sequencing between them.

I've no comment on whether the bulk regulator API needs a "keep sequencing" 
flag or somesuch, but I don't think this device will drive that requirement.

Regards,
Ian


Best regards,
Todor



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] media: Add a driver for the ov5645 camera sensor.

[Ian Arkver]

[snip]

+static int ov5645_regulators_enable(struct ov5645 *ov5645)
+{
+   int ret;
+
+   ret = regulator_enable(ov5645->io_regulator);
+   if (ret < 0) {
+   dev_err(ov5645->dev, "set io voltage failed\n");
+   return ret;
+   }
+
+   ret = regulator_enable(ov5645->core_regulator);
+   if (ret) {
+   dev_err(ov5645->dev, "set core voltage failed\n");
+   goto err_disable_io;
+   }
+
+   ret = regulator_enable(ov5645->analog_regulator);
+   if (ret) {
+   dev_err(ov5645->dev, "set analog voltage failed\n");
+   goto err_disable_core;
+   }

How about using the regulator bulk API ? This would simplify the enable
and disable functions.

The driver must enable the regulators in this order. I can see in the
implementation of the bulk api that they are enabled again in order
but I don't see stated anywhere that it is guaranteed to follow the
same order in future. I'd prefer to keep it explicit as it is now.

I believe it should be an API guarantee, otherwise many drivers using the bulk
API would break. Mark, could you please comment on that ?

Ok, let's wait for a response from Mark.
I don't have the OV5645 datasheet, but I do have the OV5640 and OV5647 
datasheets. Both of these show that AVDD should come up before DVDD 
where DVDD is externally supplied, although the minimum delay between 
them is 0ms. Both datasheets also imply that this requirement is only to 
allow host SCCB access on a shared I2C bus while the device is powering 
up. They imply that if one waits 20ms after power on then SCCB will be 
fine without this sequencing. Your code includes an msleep(20);


Further, the reference schematic for the OV5647 shows three separate 
LDOs with no sequencing between them.


I've no comment on whether the bulk regulator API needs a "keep 
sequencing" flag or somesuch, but I don't think this device will drive 
that requirement.


Regards,
Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/22] [media] imx: Add IPUv3 media common code

[Ian Arkver]

On 07/10/16 17:00, Philipp Zabel wrote:

From: Sascha Hauer 

Add video4linux API routines common to drivers for units that
accept or provide video data via the i.MX IPU IDMAC channels,
such as capture, mem2mem scaler or deinterlacer drivers.

Signed-off-by: Sascha Hauer 
Signed-off-by: Lucas Stach 
Signed-off-by: Philipp Zabel 
---
  drivers/media/platform/imx/Kconfig   |   3 +
  drivers/media/platform/imx/Makefile  |   1 +
  drivers/media/platform/imx/imx-ipu.c | 321 +++
  drivers/media/platform/imx/imx-ipu.h |  34 
  4 files changed, 359 insertions(+)
  create mode 100644 drivers/media/platform/imx/imx-ipu.c
  create mode 100644 drivers/media/platform/imx/imx-ipu.h

diff --git a/drivers/media/platform/imx/Kconfig 
b/drivers/media/platform/imx/Kconfig
index 3bd699c..1662bb0b 100644
--- a/drivers/media/platform/imx/Kconfig
+++ b/drivers/media/platform/imx/Kconfig
@@ -5,3 +5,6 @@ config MEDIA_IMX
---help---
  This driver provides a SoC wide media controller device that all
  multimedia components in i.MX5 and i.MX6 SoCs can register with.
+
+config VIDEO_IMX_IPU_COMMON
+   tristate
diff --git a/drivers/media/platform/imx/Makefile 
b/drivers/media/platform/imx/Makefile
index 74bed76..0ba601a 100644
--- a/drivers/media/platform/imx/Makefile
+++ b/drivers/media/platform/imx/Makefile
@@ -1 +1,2 @@
  obj-$(CONFIG_MEDIA_IMX)   += imx-media.o
+obj-$(CONFIG_VIDEO_IMX_IPU_COMMON) += imx-ipu.o
diff --git a/drivers/media/platform/imx/imx-ipu.c 
b/drivers/media/platform/imx/imx-ipu.c
new file mode 100644
index 000..da1deb0
--- /dev/null
+++ b/drivers/media/platform/imx/imx-ipu.c
@@ -0,0 +1,321 @@
+/*
+ * i.MX IPUv3 common v4l2 support
+ *
+ * Copyright (C) 2011 Pengutronix, Sascha Hauer 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 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 "imx-ipu.h"
+
+/*
+ * These formats are in order of preference: interleaved YUV first,
+ * because those are the most bandwidth efficient, followed by
+ * chroma-interleaved formats, and planar formats last.
+ * In each category, YUV 4:2:0 may be preferrable to 4:2:2 for bandwidth
+ * reasons, if the IDMAC channel supports double read/write reduction
+ * (all write channels, VDIC read channels).
+ */
+static struct ipu_fmt ipu_fmt_yuv[] = {
+   {
+   .fourcc = V4L2_PIX_FMT_YUYV,
+   .bytes_per_pixel = 2,
+   }, {
+   .fourcc = V4L2_PIX_FMT_UYVY,
+   .bytes_per_pixel = 2,
+   }, {
+   .fourcc = V4L2_PIX_FMT_NV12,
+   .bytes_per_pixel = 1,
+   }, {
+   .fourcc = V4L2_PIX_FMT_NV16,
+   .bytes_per_pixel = 1,
+   }, {
+   .fourcc = V4L2_PIX_FMT_YUV420,
+   .bytes_per_pixel = 1,
+   }, {
+   .fourcc = V4L2_PIX_FMT_YVU420,
+   .bytes_per_pixel = 1,
+   }, {
+   .fourcc = V4L2_PIX_FMT_YUV422P,
+   .bytes_per_pixel = 1,
+   },
+};
+
+static struct ipu_fmt ipu_fmt_rgb[] = {
+   {
+   .fourcc = V4L2_PIX_FMT_RGB32,
+   .bytes_per_pixel = 4,
+   }, {
+   .fourcc = V4L2_PIX_FMT_RGB24,
+   .bytes_per_pixel = 3,
+   }, {
+   .fourcc = V4L2_PIX_FMT_BGR24,
+   .bytes_per_pixel = 3,
+   }, {
+   .fourcc = V4L2_PIX_FMT_RGB565,
+   .bytes_per_pixel = 2,
+   },
+   {
+   .fourcc = V4L2_PIX_FMT_BGR32,
+   .bytes_per_pixel = 4,
+   },
+};


Maybe a trivial comment, but is it worthwhile to constify these two?

Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/22] [media] v4l2-async: allow subdevices to add further subdevices to the notifier waiting list

[Ian Arkver]

On 14/10/16 16:48, Philipp Zabel wrote:

Am Samstag, den 08.10.2016, 01:43 +0300 schrieb Sakari Ailus:
[...]
[...]

diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 8e2a236..e4e4b11 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -114,6 +114,18 @@ int v4l2_async_notifier_register(struct v4l2_device 
*v4l2_dev,
 struct v4l2_async_notifier *notifier);
  
  /**

+ * __v4l2_async_notifier_add_subdev - adds a subdevice to the notifier waitlist
+ *
+ * @v4l2_notifier: notifier the calling subdev is bound to

s/v4l2_//

I'd be happy to, but why should the v4l2 prefix be removed?

regards
Philipp
I think Sakari is just pointing out that the comment doesn't match the 
function argument name.


Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Ian Arkver]

On 03/08/16 15:23, Lars-Peter Clausen wrote:

On 08/03/2016 04:11 PM, Hans Verkuil wrote:


On 08/03/2016 03:21 PM, Niklas Söderlund wrote:

On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:

[...]

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index a8b434b..c6fed71 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
switch (format->format.field) {
case V4L2_FIELD_NONE:
if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-   format->format.field = V4L2_FIELD_INTERLACED;
+   format->format.field = V4L2_FIELD_ALTERNATE;
break;
default:
-   format->format.field = V4L2_FIELD_INTERLACED;
+   if (state->chip_info->flags & ADV7180_FLAG_I2P)
+   format->format.field = V4L2_FIELD_INTERLACED;

I'm not convinced this is correct. As far as I understand it when the I2P
feature is enabled the core outputs full progressive frames at the full
framerate. If it is bypassed it outputs half-frames. So we have the option
of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
think this branch should setup the field format to be ALTERNATE regardless
of whether the I2P feature is available.

Actually, that's not true. If the progressive frame is obtained by combining
two fields, then it should return FIELD_INTERLACED. This is how most SDTV
receivers operate.

This is definitely not covered by the current definition of INTERLACED. It
says that the temporal order of the odd and even lines is the same for each
frame. Whereas for a deinterlaced frame the temporal order changes from
frame to frame.

E.g. lets say you have half frames A, B, C, D, E, F ...

The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
INTERLACED_TB again and so on. Also you get the same amount of pixels as for
a progressive setup so the data-output-rate is higher. Maybe we need a
FIELD_DEINTERLACED to denote such a setup?
I don't think this is correct. The ADV7280 has no framestore, just a 
small linebuffer. It does I2P by line doubling plus some filtering and a 
little bit of proprietary magic, allegedly.


I believe the output in I2P mode for your example would be (AA) (BB) 
(CC). The clock rate and pixel rate is doubled since it sends a full 
(faked up) frame per field time.


I don't know what the FIELD_* mode is for line doubled pseudo-progressive.

Also, I don't know why anyone would use this mode. I don't see a 
scenario where it would actually improve video quality over a more 
sophisticated motion adaptive deinterlace and to restore a 25/30fps feed 
you'd need to decimate and lose information.


Quote from "Rob.Analog", who uses the word "frame" freely, here: 
https://ez.analog.com/thread/39382


"2) In I2P mode the number of lines per frame doubles. The ADV7280 still 
outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame 
now consists of twice as many lines. e.g. if a frame consisted of 288 
lines of active video in interlaced mode, this is doubled to 576 lines 
of active video in progressive mode. The line doubling is achieved by 
the ADV7280 interpolating between two lines of video (e.g. between lines 
1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). 
There are also some ADI propriety algorithms that prevent low angle 
noise artifacts.


In order to achieve this line doubling, the LLC clock doubles from a 
nominal 27MHz to a nominal 54MHz"



Regards,
IanJ



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/9] media: adv7180: add support for NEWAVMODE

[Ian Arkver]

On 25/07/16 23:04, Steve Longerbeam wrote:



On 07/25/2016 12:36 PM, Ian Arkver wrote:

On 25/07/16 18:55, Steve Longerbeam wrote:

On 07/25/2016 05:04 AM, Ian Arkver wrote:

On 23/07/16 18:00, Steve Longerbeam wrote:


+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02

See below re this value.


Hi Ian, I double-checked the ADV7180 datasheet, this value is
correct. Bit 4, when cleared, _enables_ NEWAVMODE.


Hah, ok. I'm not familiar enough with the history of this chip and 
didn't

know what "OLDAVMODE" was. So, to enable NEWAVMODE you clear
the NEWAVMODE bit. That makes perfect sense.

Anyway, I still don't see what NEWAVMODE gets you.


Hi Ian,

With video standard auto-detect disabled in the chip (VID_SEL > 2), 
captured NTSC
images by the i.mx6q SabreAuto are corrupted, best I can describe it 
as "extremely
fuzzy". Only when newavmode is enabled do the images look good again, 
in manual
mode. With auto-detect enabled, images look good with or without 
newavmode.


The strange this is, the auto-detected standard is identical to the 
standard set
explicitly in manual mode (NTSC-M). I did a complete i2c dump of the 
registers
for both auto-detect and manual mode, and found no other differences 
besides

the auto-detect/manual setting.

Trying to track this down further would probably require a logic 
analyzer on the

bt.656 bus, which I don't have access to.

I will not be debugging this further so NEWAVMODE it will have to remain.

Steve


OK, interesting. And weird indeed.

I may be interfacing an ADV7280 to the i.MX6 in the August timeframe, 
depending on
project needs etc. I'll see if I hit this with that chip. My test app 
does use autodetect.


Incidentally, looking at the BT656-5 spec and comparing to the tvp5150, 
I see that
the spec calls for 244 and 243 lines per field for NTSC, and the tvp5150 
provides

that number of lines. However this write...

adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);

where NVEND is 0x4f, configures the adv7180 to send only 242 lines in 
each field.

Not sure if this is significant.

Regards,
IanJ.




As
far as I can see it just locks down the timings and removes the 
flexibility

the chip otherwise offers to move the BT656 SAV and EAV codes around
relative to the incoming video.

In what circumstances would you need to set the newavmode property
and change this default behaviour? We're not coupling the adv7180
back-to-back with an ADV video encoder here, which is what
NEWAVMODE is for and is presumably why AD recommend it for their
eval boards. We're trying to get a BT656 compliant stream, which is
what the default mode purports to generate.

Regards,
IanJ






--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/9] media: adv7180: add support for NEWAVMODE

[Ian Arkver]

On 25/07/16 18:55, Steve Longerbeam wrote:

On 07/25/2016 05:04 AM, Ian Arkver wrote:

On 23/07/16 18:00, Steve Longerbeam wrote:

Parse the optional v4l2 endpoint DT node. If the bus type is
V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
configure the BT.656 bus in NEWAVMODE.

Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>

---

v3:
- the newavmode endpoint property is now private to adv7180.
---
  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
  drivers/media/i2c/adv7180.c| 46 --
  2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt 
b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
index 0d50115..6c175d2 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
@@ -15,6 +15,10 @@ Required Properties :
"adi,adv7282"
"adi,adv7282-m"
  
+Optional Endpoint Properties :

+- newavmode: a boolean property to indicate the BT.656 bus is operating
+  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.
+
  Example:
  
  	i2c0@1c22000 {

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index cb83ebb..3067d5f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -31,6 +31,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -106,6 +107,7 @@

  #define ADV7180_REG_SHAP_FILTER_CTL_1 0x0017
  #define ADV7180_REG_CTRL_20x001d
  #define ADV7180_REG_VSYNC_FIELD_CTL_1 0x0031
+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02

See below re this value.


Hi Ian, I double-checked the ADV7180 datasheet, this value is
correct. Bit 4, when cleared, _enables_ NEWAVMODE.


Hah, ok. I'm not familiar enough with the history of this chip and didn't
know what "OLDAVMODE" was. So, to enable NEWAVMODE you clear
the NEWAVMODE bit. That makes perfect sense.

Anyway, I still don't see what NEWAVMODE gets you. As
far as I can see it just locks down the timings and removes the flexibility
the chip otherwise offers to move the BT656 SAV and EAV codes around
relative to the incoming video.

In what circumstances would you need to set the newavmode property
and change this default behaviour? We're not coupling the adv7180
back-to-back with an ADV video encoder here, which is what
NEWAVMODE is for and is presumably why AD recommend it for their
eval boards. We're trying to get a BT656 compliant stream, which is
what the default mode purports to generate.

Regards,
IanJ



  #define ADV7180_REG_MANUAL_WIN_CTL_1  0x003d
  #define ADV7180_REG_MANUAL_WIN_CTL_2  0x003e
  #define ADV7180_REG_MANUAL_WIN_CTL_3  0x003f
@@ -214,6 +216,7 @@ struct adv7180_state {
struct mutexmutex; /* mutual excl. when accessing chip */
int irq;
v4l2_std_id curr_norm;
+   boolnewavmode;
boolpowered;
boolstreaming;
u8  input;
@@ -864,9 +867,15 @@ static int adv7180_init(struct adv7180_state *state)
if (ret < 0)
return ret;
  
-	/* Manually set V bit end position in NTSC mode */

-   return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
-   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (!state->newavmode) {
+   /* Manually set V bit end position in NTSC mode */
+   ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
+   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (ret < 0)
+   return ret;
+   }
+
+   return 0;

According to the ADV7180 datasheet, rev. J, page 48 [1], when NEWAVMODE is 0,
no adjustments are possible to the output timings, which would imply this write
is ignored after your edit.

That's correct, when NEWAVMODE is enabled (by clearing reg 0x31 bit 4), no
adjustments are possible to the output timings:

 From the rev J datasheet, page 91, when reg 0x31 bit 4 is set (NEAVMODE 
disabled):

"Manual VS/FIELD position controlled by Register 0x32, Register 0x33, and
Register 0xE5 to Register 0xEA"


1: www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf


  }
  
  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)

@@ -1217,6 +1226,13 @@ static int init_device(struct adv7180_state *state)
if (ret)
goto out_unlock;
  
+	if (state->newavmode) {

+   ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
+   ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
+   if (ret < 0)
+   goto out_unlock;
+   }
+

Again according to the DS, NEWAVMODE is set by default and enable

Re: [PATCH v3 3/9] media: adv7180: add support for NEWAVMODE

[Ian Arkver]

Resend due to HTML email bounce.

On 23/07/16 18:00, Steve Longerbeam wrote:

Parse the optional v4l2 endpoint DT node. If the bus type is
V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
configure the BT.656 bus in NEWAVMODE.

Signed-off-by: Steve Longerbeam 

---

v3:
- the newavmode endpoint property is now private to adv7180.
---
  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
  drivers/media/i2c/adv7180.c| 46 --
  2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt 
b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
index 0d50115..6c175d2 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
@@ -15,6 +15,10 @@ Required Properties :
"adi,adv7282"
"adi,adv7282-m"
  
+Optional Endpoint Properties :

+- newavmode: a boolean property to indicate the BT.656 bus is operating
+  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.
+
  Example:
  
  	i2c0@1c22000 {

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index cb83ebb..3067d5f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -31,6 +31,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -106,6 +107,7 @@

  #define ADV7180_REG_SHAP_FILTER_CTL_1 0x0017
  #define ADV7180_REG_CTRL_20x001d
  #define ADV7180_REG_VSYNC_FIELD_CTL_1 0x0031
+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02

See below re this value.

  #define ADV7180_REG_MANUAL_WIN_CTL_1  0x003d
  #define ADV7180_REG_MANUAL_WIN_CTL_2  0x003e
  #define ADV7180_REG_MANUAL_WIN_CTL_3  0x003f
@@ -214,6 +216,7 @@ struct adv7180_state {
struct mutexmutex; /* mutual excl. when accessing chip */
int irq;
v4l2_std_id curr_norm;
+   boolnewavmode;
boolpowered;
boolstreaming;
u8  input;
@@ -864,9 +867,15 @@ static int adv7180_init(struct adv7180_state *state)
if (ret < 0)
return ret;
  
-	/* Manually set V bit end position in NTSC mode */

-   return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
-   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (!state->newavmode) {
+   /* Manually set V bit end position in NTSC mode */
+   ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
+   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (ret < 0)
+   return ret;
+   }
+
+   return 0;


According to the ADV7180 datasheet, rev. J, page 48 [1], when NEWAVMODE 
is 0,
no adjustments are possible to the output timings, which would imply 
this write

is ignored after your edit.

1: www.analog.com/media/en/technical-documentation/data-sheets/ADV7180.pdf

  }
  
  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)

@@ -1217,6 +1226,13 @@ static int init_device(struct adv7180_state *state)
if (ret)
goto out_unlock;
  
+	if (state->newavmode) {

+   ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
+   ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
+   if (ret < 0)
+   goto out_unlock;
+   }
+


Again according to the DS, NEWAVMODE is set by default and enables a
stream which is by default CCIR656 compliant. Your edit writes 0x02 to
this register which actually clears NEWAVMODE (bit 4).

ret = adv7180_program_std(state);
if (ret)
goto out_unlock;
@@ -1257,6 +1273,28 @@ out_unlock:
return ret;
  }
  
+static void adv7180_of_parse(struct adv7180_state *state)

+{
+   struct i2c_client *client = state->client;
+   struct device_node *np = client->dev.of_node;
+   struct device_node *endpoint;
+   struct v4l2_of_endpoint ep;
+
+   endpoint = of_graph_get_next_endpoint(np, NULL);
+   if (!endpoint) {
+   v4l_warn(client, "endpoint node not found\n");
+   return;
+   }
+
+   v4l2_of_parse_endpoint(endpoint, );
+   if (ep.bus_type == V4L2_MBUS_BT656) {
+   if (of_property_read_bool(endpoint, "newavmode"))
+   state->newavmode = true;
+   }
+
+   of_node_put(endpoint);
+}
+


I'm not sure what the use case is for clearing NEWAVMODE, in which case
this code and the call below are not needed.

Maybe this particular patch should be dropped?

  static int adv7180_probe(struct i2c_client *client,
 const struct i2c_device_id *id)
  {
@@ -1279,6 +1317,8 @@ static int adv7180_probe(struct i2c_client *client,
state->field = 

Re: [PATCH v3 3/9] DocBook/v4l: Add compressed video formats used on MT8173 codec driver

[Ian Arkver]

On 12/07/16 20:14, Nicolas Dufresne wrote:

Le mardi 12 juillet 2016 à 15:08 -0400, Nicolas Dufresne a écrit :

Le mardi 12 juillet 2016 à 16:16 +0800, Wu-Cheng Li (李務誠) a écrit :

Decoder hardware produces MT21 (compressed). Image processor can
convert it to a format that can be input of display driver.
Tiffany.
When do you plan to upstream image processor (mtk-mdp)?

It can be as input format for encoder, MDP and display drivers in

our

platform.

I remember display driver can only accept uncompressed MT21. Right?
Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
format. It's not usable until it's decompressed and converted by
image
processor.

Previously it was described as MediaTek block mode, and now as a
MediaTek compressed format. It makes me think you have no idea what
this pixel format really is. Is that right ?

The main reason why I keep asking, is that we often find similarities
between what vendor like to call their proprietary formats. Doing the
proper research helps not creating a mess like in Android where you
have a lot of formats that all point to the same format. I believe
there was the same concern when Samsung wanted to introduce their Z-
flip-Z NV12 tile format. In the end they simply provided sufficient
documentation so we could document it and implement software
converters
for test and validation purpose.

Here's the kind of information we want in the documentation.

https://chromium.googlesource.com/chromium/src/media/+/master/base/vide
o_types.h#40

   // MediaTek proprietary format. MT21 is similar to NV21 except the memory
   // layout and pixel layout (swizzles). 12bpp with Y plane followed by a 2x2
   // interleaved VU plane. Each image contains two buffers -- Y plane and VU
   // plane. Two planes can be non-contiguous in memory. The starting addresses
   // of Y plane and VU plane are 4KB alignment.
   // Suppose image dimension is (width, height). For both Y plane and VU plane:
   // Row pitch = ((width+15)/16) * 16.
   // Plane size = Row pitch * (((height+31)/32)*32)

Now obviously this is incomplete, as the swizzling need to be documented of 
course.
Not sure where that chromium comment came from, but if MT21 really is 
similar to NV21 (a 4:2:0 format) and has 2x2 downsampled chroma, then 
the combined VU plane will be half the size of the Y plane.


Maybe that's not relevant to this discussion.

Regards,
Ian



regards,
Nicolas

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

This conversation has drifted off topic, sorry.
It now relates to code in drivers/gpu/ipu-v3/ipu-csi.c, so adding 
Philipp Zabel.


On 11/07/16 23:03, Steve Longerbeam wrote:

On 07/11/2016 12:06 AM, Ian Arkver wrote:

On 10/07/16 23:34, Steve Longerbeam wrote:


On 07/10/2016 07:30 AM, Hans Verkuil wrote:

On 07/10/2016 04:17 PM, Ian Arkver wrote:

On 10/07/16 13:55, Hans Verkuil wrote:

On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:

On 07/09/2016 11:36 PM, Steve Longerbeam wrote:

On 07/09/2016 02:10 PM, Steve Longerbeam wrote:

On 07/09/2016 11:59 AM, Steve Longerbeam wrote:

On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:

Add a device tree boolean property "bt656-4" to allow setting
the ITU-R BT.656-4 compatible bit.

Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>

+/* select ITU-R BT.656-4 compatible? */

Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
number of the standard (and 5 is the latest anyway).

   From the ADV7180 DS [1]:

BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]

Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU
has changed the toggling position for the V bit within the SAV EAV codes
for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
output mode that is compliant with either the previous or new standard.
For further information, visit the International Telecommunication Union
website.

Note that the standard change only affects NTSC and has no bearing on PAL.

When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
used. The V bit goes low at EAV of Line 10 and Line 273.

When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
V bit goes low at EAV of Line 20 and Line 283.

[1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf

Rev 4 came in in 1998. I would say that that is the default. We have had any
problems with this and I would need some proof that this is really needed.

Hi Hans, yeah I agree with you that new capture drivers should not
expect BT.656-3 compatible buss' any longer. The problem with i.MX6
however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
about the AV code positions, are very poorly documented. In order for
i.MX6 to support BT.656-4 it would require very time-consuming reverse
engineering to find the right values to plug into those registers to conform
to the BT.656-4 AV code positions.

Steve


Hi Steve,

Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to 
the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit),  those 
registers make more sense. Pity that's
not mentioned in the Ref Manual.

Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value
programmed to CCIR_CODE_1 register (according to imx6 ref manual is
values for first field), for NTSC : 0xD07DF.  Comparing with the definition of
the H/V/F bits in the AV codes in the bt.656 spec:

F = 1 during field 2, 0 during field 1
V = 1 during field blanking, 0 elsewhere
H = 1 in EAV, 0 in SAV

There's no correspondence, for example F bit should be zero everywhere in
CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about?
None of it makes any sense to me.

Hi,

d07df = 001 101  011 111 011 111

  H V F
CSI0_STRT_FLD0_ACTV   0 0 1
CSI0_END_FLD0_ACTV1 0 1
CSI0_STRT_FLD0_BLNK_2ND   0 1 1
CSI0_END_FLD0_BLNK_2ND1 1 1
CSI0_STRT_FLD0_BLNK_1ST   0 1 1
CSI0_END_FLD0_BLNK_1ST1 1 1

40596 = 000 100  010 110 010 110
405A6 = 000 100  010 110 100 110

  H V F
CSI0_STRT_FLD1_ACTV   0 0 0
CSI0_END_FLD1_ACTV1 0 0
CSI0_STRT_FLD1_BLNK_2ND   0 1 0
CSI0_END_FLD1_BLNK_2ND1 1 0
CSI0_STRT_FLD1_BLNK_1ST   0 1 0or 1 0 0 ?
CSI0_END_FLD1_BLNK_1ST1 1 0

For PAL:  IPU_CSI0_CCIR_CODE_1 = 0x40596, IPU_CSI0_CCIR_CODE_1 = 0xd07df
For NTSC: IPU_CSI0_CCIR_CODE_1 = 0xd07df, IPU_CSI0_CCIR_CODE_1 = 0x405A6

So, for the PAL case the field values make sense to me. The F bit is 
constant throughout each field.
I'm not sure why the NTSC case has 405A6 instead of 40596 again. This 
looks like a bug to me.
If you look back at the original Freescale capture driver[1], they use 
40596 for both PAL and NTSC.


The values are swapped between NTSC and PAL to account for the differing 
field order. I think using
the capture width and height to do this is poor as any "bt.656-like" 
source with non standard
dimensions will end up in the dev_err("Unsupported") case. Also, looking 
at BT.1120-8, for interlaced
video the same SAV and EAV codes as PAL are used, so 
IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR and

IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR modes are currently broken.

I think the 1st and 2nd blanking values might be needed for some odd non 
656 streams maybe? Also for

progressive (originally) the v

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

On 10/07/16 23:34, Steve Longerbeam wrote:



On 07/10/2016 07:30 AM, Hans Verkuil wrote:

On 07/10/2016 04:17 PM, Ian Arkver wrote:

On 10/07/16 13:55, Hans Verkuil wrote:

On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:

On 07/09/2016 11:36 PM, Steve Longerbeam wrote:

On 07/09/2016 02:10 PM, Steve Longerbeam wrote:

On 07/09/2016 11:59 AM, Steve Longerbeam wrote:

On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:

Add a device tree boolean property "bt656-4" to allow setting
the ITU-R BT.656-4 compatible bit.

Signed-off-by: Steve Longerbeam <steve_longerb...@mentor.com>

+/* select ITU-R BT.656-4 compatible? */
Please don't use the '-4': BT.656 is sufficient. The -4 is just the 
version

number of the standard (and 5 is the latest anyway).

  From the ADV7180 DS [1]:

BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]

Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the 
ITU
has changed the toggling position for the V bit within the SAV EAV 
codes

for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
output mode that is compliant with either the previous or new standard.
For further information, visit the International Telecommunication 
Union

website.

Note that the standard change only affects NTSC and has no bearing 
on PAL.


When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
used. The V bit goes low at EAV of Line 10 and Line 273.

When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
V bit goes low at EAV of Line 20 and Line 283.

[1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf 

Rev 4 came in in 1998. I would say that that is the default. We have 
had any
problems with this and I would need some proof that this is really 
needed.


Hi Hans, yeah I agree with you that new capture drivers should not
expect BT.656-3 compatible buss' any longer. The problem with i.MX6
however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
about the AV code positions, are very poorly documented. In order for
i.MX6 to support BT.656-4 it would require very time-consuming reverse
engineering to find the right values to plug into those registers to 
conform

to the BT.656-4 AV code positions.

Steve


Hi Steve,

Once you dsicover that the 3-bit fields in each CCIR_CODE register 
correspond to the H, V and F flags in the SAV/EAV code (in that order, 
MSbit->LSbit),  those registers make more sense. Pity that's not 
mentioned in the Ref Manual.


However, I don't think that's relevant here, since the spec revision 
didn't change the codes, but just moved the lines the V bit is sent on. 
I believe the spec change switched the NTSC timing from VSYNC to VBLANK, 
but the net effect was 10 fewer "active" video lines per field. Looking 
at a sample of one other video decoder (tvp5150), the default seems to 
be to change V on lines 20 and 283, as per the newer revision of the 
spec., though again bets may have been hedged with a programmable override.


In any case, I'm wondering if your extra ten lines per field are more 
related to this snippet from calc_default_crop in imx-camif.c, which 
seems like it would break other decoder front ends and adv7180 in 
bt656-4 mode...


/*
 * FIXME: For NTSC standards, top must be set to an
 * offset of 13 lines to match fixed CCIR programming
 * in the IPU.
 */
if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60))
rect->top = 13;

I believe tvp5150 at least sends 243 active lines per field for an NTSC 
source and the top 3 lines are generally dropped.


Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

On 10/07/16 13:55, Hans Verkuil wrote:

On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:

On 07/09/2016 11:36 PM, Steve Longerbeam wrote:


On 07/09/2016 02:10 PM, Steve Longerbeam wrote:


On 07/09/2016 11:59 AM, Steve Longerbeam wrote:


On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:

Add a device tree boolean property "bt656-4" to allow setting
the ITU-R BT.656-4 compatible bit.

Signed-off-by: Steve Longerbeam 

+/* select ITU-R BT.656-4 compatible? */

Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
number of the standard (and 5 is the latest anyway).

From the ADV7180 DS [1]:

BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]

Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU 
has changed the toggling position for the V bit within the SAV EAV codes 
for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an 
output mode that is compliant with either the previous or new standard. 
For further information, visit the International Telecommunication Union 
website.


Note that the standard change only affects NTSC and has no bearing on PAL.

When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is 
used. The V bit goes low at EAV of Line 10 and Line 273.


When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The 
V bit goes low at EAV of Line 20 and Line 283.


[1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf 




+if (of_property_read_bool(client->dev.of_node, "bt656-4"))
+state->bt656_4 = true;
This property needs to be documented. In my opinion it should also be a
property of the endpoint sub-node rather than the toplevel device node
since
this is a configuration of the endpoint format.

Agreed, it's really a config of the backend capture endpoint. I'll move it
there and also document it in
Documentation/devicetree/bindings/media/i2c/adv7180.txt.

er, scratch that. ITU-R BT.656 compatibility is really a property of the
bt.656 bus. It
should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint().
I've created
a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all
the maintainers.

But I'm going back and forth on whether this property should be added to the
adv7180's
local endpoint, or to the remote endpoint. I'm leaning towards the remote
endpoint, since
this is more about how the backend wants the bus configured.

I think it should be set for both, as both endpoints need to agree on the
format.

Is it needed at all? Setting the polarity flags to H/VSYNC_ACTIVE_HIGH/LOW
will already signal BT.656 mode. See include/media/v4l2-mediabus.h and
v4l2-of.c.

I haven't looked in detail at adv7180, so I may have missed something, but this
looks strange.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 31/38] media: imx: Add video switch

[Ian Arkver]

For me this fails when I try to enable both video muxes (mx6dl, though 
mx6q should be the same).


I get a sysfs duplicate name failure for 34.videomux. I realise passing 
the GPR13 register offset and a bitfield mask as a tuple in the reg 
value of the of_node is handy, but how should we account for multiple 
devices with the same name and address?


A quick and dirty hack would be to have of_get_reg_field do something like

field->reg = reg_bit_mask[0] & 0xff;

and then use values in the DT that differ in the bits masked off, but 
there must be a nicer way.


Trace below, fyi. This is from the v2 patches posted here, not your v2.1 
tree.


Regards,
IanJ


[0.096004] [ cut here ]
[0.096035] WARNING: CPU: 0 PID: 1 at 
/home/ian/tx6/yoctomaster/build/tmp/work-shared/tx6u-vid/kernel-source/fs/sysfs/dir.c:31 
sysfs_warn_dup+0x70/0x80
[0.096046] sysfs: cannot create duplicate filename 
'/devices/soc0/34.videomux'

[0.096053] Modules linked in:
[0.096071] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
4.7.0-rc1-yocto-standard #1

[0.096079] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[0.096116] [<80018d8c>] (unwind_backtrace) from [<8001427c>] 
(show_stack+0x18/0x1c)
[0.096138] [<8001427c>] (show_stack) from [<802c85fc>] 
(dump_stack+0x88/0x9c)
[0.096157] [<802c85fc>] (dump_stack) from [<80024d90>] 
(__warn+0xf4/0x10c)
[0.096175] [<80024d90>] (__warn) from [<80024de8>] 
(warn_slowpath_fmt+0x40/0x50)
[0.096194] [<80024de8>] (warn_slowpath_fmt) from [<80176118>] 
(sysfs_warn_dup+0x70/0x80)
[0.096212] [<80176118>] (sysfs_warn_dup) from [<80176204>] 
(sysfs_create_dir_ns+0x8c/0x9c)
[0.096231] [<80176204>] (sysfs_create_dir_ns) from [<802cafd0>] 
(kobject_add_internal+0xc0/0x360)
[0.096249] [<802cafd0>] (kobject_add_internal) from [<802cb2b8>] 
(kobject_add+0x48/0x98)
[0.096269] [<802cb2b8>] (kobject_add) from [<803b90e0>] 
(device_add+0xf0/0x5a0)
[0.096295] [<803b90e0>] (device_add) from [<8051ae20>] 
(of_platform_device_create_pdata+0x8c/0xc4)
[0.096316] [<8051ae20>] (of_platform_device_create_pdata) from 
[<8051af7c>] (of_platform_bus_create+0x110/0x2a0)
[0.096333] [<8051af7c>] (of_platform_bus_create) from [<8051b29c>] 
(of_platform_populate+0x64/0xb4)
[0.096358] [<8051b29c>] (of_platform_populate) from [<808f1a88>] 
(imx6q_init_machine+0x104/0x2b4)
[0.096377] [<808f1a88>] (imx6q_init_machine) from [<808eba7c>] 
(customize_machine+0x24/0x44)
[0.096395] [<808eba7c>] (customize_machine) from [<8000980c>] 
(do_one_initcall+0x4c/0x174)
[0.096414] [<8000980c>] (do_one_initcall) from [<808ead60>] 
(kernel_init_freeable+0x158/0x1e8)
[0.096435] [<808ead60>] (kernel_init_freeable) from [<8062b4b0>] 
(kernel_init+0x14/0x100)
[0.096457] [<8062b4b0>] (kernel_init) from [<800104b8>] 
(ret_from_fork+0x14/0x3c)

[0.096482] ---[ end trace 394e7b4d22c2be44 ]---
[0.096491] [ cut here ]
[0.096507] WARNING: CPU: 0 PID: 1 at 
/home/ian/tx6/yoctomaster/build/tmp/work-shared/tx6u-vid/kernel-source/lib/kobject.c:240 
kobject_add_internal+0x2e0/0x360
[0.096518] kobject_add_internal failed for 34.videomux with -EEXIST, 
don't try to register things with the same name in the same directory.

[0.096525] Modules linked in:
[0.096539] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W   
4.7.0-rc1-yocto-standard #1

[0.096548] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[0.096570] [<80018d8c>] (unwind_backtrace) from [<8001427c>] 
(show_stack+0x18/0x1c)
[0.096585] [<8001427c>] (show_stack) from [<802c85fc>] 
(dump_stack+0x88/0x9c)
[0.096601] [<802c85fc>] (dump_stack) from [<80024d90>] 
(__warn+0xf4/0x10c)
[0.096616] [<80024d90>] (__warn) from [<80024de8>] 
(warn_slowpath_fmt+0x40/0x50)
[0.096632] [<80024de8>] (warn_slowpath_fmt) from [<802cb1f0>] 
(kobject_add_internal+0x2e0/0x360)
[0.096647] [<802cb1f0>] (kobject_add_internal) from [<802cb2b8>] 
(kobject_add+0x48/0x98)
[0.096664] [<802cb2b8>] (kobject_add) from [<803b90e0>] 
(device_add+0xf0/0x5a0)
[0.096681] [<803b90e0>] (device_add) from [<8051ae20>] 
(of_platform_device_create_pdata+0x8c/0xc4)
[0.096700] [<8051ae20>] (of_platform_device_create_pdata) from 
[<8051af7c>] (of_platform_bus_create+0x110/0x2a0)
[0.096716] [<8051af7c>] (of_platform_bus_create) from [<8051b29c>] 
(of_platform_populate+0x64/0xb4)
[0.096735] [<8051b29c>] (of_platform_populate) from [<808f1a88>] 
(imx6q_init_machine+0x104/0x2b4)
[0.096752] [<808f1a88>] (imx6q_init_machine) from [<808eba7c>] 
(customize_machine+0x24/0x44)
[0.096767] [<808eba7c>] (customize_machine) from [<8000980c>] 
(do_one_initcall+0x4c/0x174)
[0.096782] [<8000980c>] (do_one_initcall) from [<808ead60>] 
(kernel_init_freeable+0x158/0x1e8)
[0.096798] [<808ead60>] (kernel_init_freeable) from [<8062b4b0>] 
(kernel_init+0x14/0x100)
[0.096814] [<8062b4b0>] (kernel_init) from [<800104b8>] 

Re: [PATCH] v4l2-ctrl.h: fix comments

[Ian Arkver]

On 15/06/16 11:10, Hans Verkuil wrote:

The comments for the unlocked v4l2_ctrl_s_ctrl* functions were wrong (copy
and pasted from the locked variants). Fix this, since it is confusing.

Signed-off-by: Hans Verkuil 

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 0bc9b35..e9e87e023 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -759,9 +759,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   * @ctrl: The control.
   * @val:  The new value.
   *
- * This set the control's new value safely by going through the control
- * framework. This function will lock the control's handler, so it cannot be
- * used from within the _ctrl_ops functions.
+ * This sets the control's new value safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
   *
   * This function is for integer type controls only.
   */
@@ -771,7 +771,7 @@ int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
   * @ctrl: The control.
   * @val:  The new value.
   *
- * This set the control's new value safely by going through the control
+ * This sets the control's new value safely by going through the control
   * framework. This function will lock the control's handler, so it cannot be
   * used from within the _ctrl_ops functions.
   *
@@ -807,9 +807,9 @@ s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
   * @ctrl: The control.
   * @val:  The new value.
   *
- * This set the control's new value safely by going through the control
- * framework. This function will lock the control's handler, so it cannot be
- * used from within the _ctrl_ops functions.
+ * This sets the control's new value safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
   *
   * This function is for 64-bit integer type controls only.
   */
@@ -821,9 +821,9 @@ int __v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 
val);
   * @ctrl: The control.
   * @val:  The new value.
   *
- * This set the control's new value safely by going through the control
- * framework. This function will lock the control's handler, so it cannot be
- * used from within the _ctrl_ops functions.
+ * This sets the control's new value safely by going through the control
+ * framework. This function does not lock the control's handler, allowing it to
+ * be used from within the _ctrl_ops functions.
   *
   * This function is for 64-bit integer type controls only.
   */
I think this comment is above v4l2_ctrl_s_ctrl_int64, without the 
underscores. Doesn't this fn take the lock, or have I missed a patch 
that removes that?



@@ -843,9 +843,9 @@ static inline int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl 
*ctrl, s64 val)
   * @ctrl: The control.
   * @s:The new string.
   *
- * This set the control's new string safely by going through the control
- * framework. This function will lock the control's handler, so it cannot be
- * used from within the _ctrl_ops functions.
+ * This sets the control's new string safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
   *
   * This function is for string type controls only.
   */
@@ -857,7 +857,7 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const 
char *s);
   * @ctrl: The control.
   * @s:The new string.
   *
- * This set the control's new string safely by going through the control
+ * This sets the control's new string safely by going through the control
   * framework. This function will lock the control's handler, so it cannot be
   * used from within the _ctrl_ops functions.
   *
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html