Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-31 Thread Hans Verkuil
On 08/31/2015 09:56 AM, Junghak Sung wrote:
> 
> 
> On 08/31/2015 11:01 AM, Junghak Sung wrote:
>> Hello Hans,
>>
>> Thank you for your review.
>> I leave some comments in the body for reply.
>>
>> Regards,
>> Junghak
>>
>>
>>
>> On 08/28/2015 10:31 PM, Hans Verkuil wrote:
>>> Hi Junghak,
>>>
>>> Thanks for this patch, it looks much better. I do have a number of
>>> comments, though...
>>>
>>> On 08/26/2015 01:59 PM, Junghak Sung wrote:
 Remove v4l2-specific stuff from struct vb2_buffer and add member
 variables
 related with buffer management.

 struct vb2_plane {
  
  /* plane information */
  unsigned intbytesused;
  unsigned intlength;
  union {
  unsigned intoffset;
  unsigned long   userptr;
  int fd;
  } m;
  unsigned intdata_offset;
 }

 struct vb2_buffer {
  
  /* buffer information */
  unsigned intnum_planes;
  unsigned intindex;
  unsigned inttype;
  unsigned intmemory;

  struct vb2_planeplanes[VIDEO_MAX_PLANES];
  
 };

 And create struct vb2_v4l2_buffer as container buffer for v4l2 use.

 struct vb2_v4l2_buffer {
  struct vb2_buffer   vb2_buf;

  __u32   flags;
  __u32   field;
  struct timeval  timestamp;
  struct v4l2_timecodetimecode;
  __u32   sequence;
 };

 This patch includes only changes inside of videobuf2. So, it is
 required to
 modify all device drivers which use videobuf2.

 Signed-off-by: Junghak Sung 
 Signed-off-by: Geunyoung Kim 
 Acked-by: Seung-Woo Kim 
 Acked-by: Inki Dae 
 ---
   drivers/media/v4l2-core/videobuf2-core.c |  324
 +-
   include/media/videobuf2-core.h   |   50 ++---
   include/media/videobuf2-v4l2.h   |   26 +++
   3 files changed, 236 insertions(+), 164 deletions(-)

 diff --git a/drivers/media/v4l2-core/videobuf2-core.c
 b/drivers/media/v4l2-core/videobuf2-core.c
 index ab00ea0..9266d50 100644
 --- a/drivers/media/v4l2-core/videobuf2-core.c
 +++ b/drivers/media/v4l2-core/videobuf2-core.c
 @@ -35,10 +35,10 @@
   static int debug;
   module_param(debug, int, 0644);

 -#define dprintk(level, fmt, arg...)  \
 -do {  \
 -if (debug >= level)  \
 -pr_info("vb2: %s: " fmt, __func__, ## arg); \
 +#define dprintk(level, fmt, arg...)\
 +do {\
 +if (debug >= level)\
 +pr_info("vb2: %s: " fmt, __func__, ## arg);\
>>>
>>> These are just whitespace changes, and that is something it see *a
>>> lot* in this
>>> patch. And usually for no clear reason.
>>>
>>> Please remove those whitespace changes, it makes a difficult patch
>>> even harder
>>> to read than it already is.
>>>
>>
>> I just wanted to remove unnecessary white spaces or adjust alignment.
>> OK, I will revert those whitespace changes for better review.
>>
   } while (0)

   #ifdef CONFIG_VIDEO_ADV_DEBUG
>>>
>>> 
>>>
 @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q)
*/
   static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct
 v4l2_buffer *b)
   {
 +struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
   struct vb2_queue *q = vb->vb2_queue;
 +unsigned int plane;

   /* Copy back data such as timestamp, flags, etc. */
 -memcpy(b, >v4l2_buf, offsetof(struct v4l2_buffer, m));
 -b->reserved2 = vb->v4l2_buf.reserved2;
 -b->reserved = vb->v4l2_buf.reserved;
>>>
>>> Hmm, I'm not sure why these reserved fields were copied here. I think
>>> it was
>>> for compatibility reasons for some old drivers that abused the
>>> reserved field.
>>> However, in the new code these reserved fields should probably be
>>> explicitly
>>> initialized to 0.
>>>
 +b->index = vb->index;
 +b->type = vb->type;
 +b->memory = vb->memory;
 +b->bytesused = 0;
 +
 +b->flags = vbuf->flags;
 +b->field = vbuf->field;
 +b->timestamp = vbuf->timestamp;
 +b->timecode = vbuf->timecode;
 +b->sequence = vbuf->sequence;

   if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
   /*
 @@ -669,21 +674,33 @@ static void 

Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-31 Thread Junghak Sung



On 08/31/2015 11:01 AM, Junghak Sung wrote:

Hello Hans,

Thank you for your review.
I leave some comments in the body for reply.

Regards,
Junghak



On 08/28/2015 10:31 PM, Hans Verkuil wrote:

Hi Junghak,

Thanks for this patch, it looks much better. I do have a number of
comments, though...

On 08/26/2015 01:59 PM, Junghak Sung wrote:

Remove v4l2-specific stuff from struct vb2_buffer and add member
variables
related with buffer management.

struct vb2_plane {
 
 /* plane information */
 unsigned intbytesused;
 unsigned intlength;
 union {
 unsigned intoffset;
 unsigned long   userptr;
 int fd;
 } m;
 unsigned intdata_offset;
}

struct vb2_buffer {
 
 /* buffer information */
 unsigned intnum_planes;
 unsigned intindex;
 unsigned inttype;
 unsigned intmemory;

 struct vb2_planeplanes[VIDEO_MAX_PLANES];
 
};

And create struct vb2_v4l2_buffer as container buffer for v4l2 use.

struct vb2_v4l2_buffer {
 struct vb2_buffer   vb2_buf;

 __u32   flags;
 __u32   field;
 struct timeval  timestamp;
 struct v4l2_timecodetimecode;
 __u32   sequence;
};

This patch includes only changes inside of videobuf2. So, it is
required to
modify all device drivers which use videobuf2.

Signed-off-by: Junghak Sung 
Signed-off-by: Geunyoung Kim 
Acked-by: Seung-Woo Kim 
Acked-by: Inki Dae 
---
  drivers/media/v4l2-core/videobuf2-core.c |  324
+-
  include/media/videobuf2-core.h   |   50 ++---
  include/media/videobuf2-v4l2.h   |   26 +++
  3 files changed, 236 insertions(+), 164 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index ab00ea0..9266d50 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -35,10 +35,10 @@
  static int debug;
  module_param(debug, int, 0644);

-#define dprintk(level, fmt, arg...)  \
-do {  \
-if (debug >= level)  \
-pr_info("vb2: %s: " fmt, __func__, ## arg); \
+#define dprintk(level, fmt, arg...)\
+do {\
+if (debug >= level)\
+pr_info("vb2: %s: " fmt, __func__, ## arg);\


These are just whitespace changes, and that is something it see *a
lot* in this
patch. And usually for no clear reason.

Please remove those whitespace changes, it makes a difficult patch
even harder
to read than it already is.



I just wanted to remove unnecessary white spaces or adjust alignment.
OK, I will revert those whitespace changes for better review.


  } while (0)

  #ifdef CONFIG_VIDEO_ADV_DEBUG





@@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q)
   */
  static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct
v4l2_buffer *b)
  {
+struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
  struct vb2_queue *q = vb->vb2_queue;
+unsigned int plane;

  /* Copy back data such as timestamp, flags, etc. */
-memcpy(b, >v4l2_buf, offsetof(struct v4l2_buffer, m));
-b->reserved2 = vb->v4l2_buf.reserved2;
-b->reserved = vb->v4l2_buf.reserved;


Hmm, I'm not sure why these reserved fields were copied here. I think
it was
for compatibility reasons for some old drivers that abused the
reserved field.
However, in the new code these reserved fields should probably be
explicitly
initialized to 0.


+b->index = vb->index;
+b->type = vb->type;
+b->memory = vb->memory;
+b->bytesused = 0;
+
+b->flags = vbuf->flags;
+b->field = vbuf->field;
+b->timestamp = vbuf->timestamp;
+b->timecode = vbuf->timecode;
+b->sequence = vbuf->sequence;

  if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
  /*
@@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct
vb2_buffer *vb, struct v4l2_buffer *b)
   * for it. The caller has already verified memory and size.
   */
  b->length = vb->num_planes;
-memcpy(b->m.planes, vb->v4l2_planes,
-b->length * sizeof(struct v4l2_plane));


A similar problem occurs here: the memcpy would have copied the reserved
field in v4l2_plane as well, but that no longer happens, so you need to
do an explicit memset for the reserved field in the new code.



It means that I'd better add reserved fields to struct vb2_buffer and
struct vb2_plane in order to keep the information in struct v4l2_buffer
and struct v4l2_plane???



Oh, I've mistaken your 

Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-30 Thread Junghak Sung

Hello Hans,

Thank you for your review.
I leave some comments in the body for reply.

Regards,
Junghak



On 08/28/2015 10:31 PM, Hans Verkuil wrote:

Hi Junghak,

Thanks for this patch, it looks much better. I do have a number of comments, 
though...

On 08/26/2015 01:59 PM, Junghak Sung wrote:

Remove v4l2-specific stuff from struct vb2_buffer and add member variables
related with buffer management.

struct vb2_plane {
 snip
 /* plane information */
 unsigned intbytesused;
 unsigned intlength;
 union {
 unsigned intoffset;
 unsigned long   userptr;
 int fd;
 } m;
 unsigned intdata_offset;
}

struct vb2_buffer {
 snip
 /* buffer information */
 unsigned intnum_planes;
 unsigned intindex;
 unsigned inttype;
 unsigned intmemory;

 struct vb2_planeplanes[VIDEO_MAX_PLANES];
 snip
};

And create struct vb2_v4l2_buffer as container buffer for v4l2 use.

struct vb2_v4l2_buffer {
 struct vb2_buffer   vb2_buf;

 __u32   flags;
 __u32   field;
 struct timeval  timestamp;
 struct v4l2_timecodetimecode;
 __u32   sequence;
};

This patch includes only changes inside of videobuf2. So, it is required to
modify all device drivers which use videobuf2.

Signed-off-by: Junghak Sung jh1009.s...@samsung.com
Signed-off-by: Geunyoung Kim nenggun@samsung.com
Acked-by: Seung-Woo Kim sw0312@samsung.com
Acked-by: Inki Dae inki@samsung.com
---
  drivers/media/v4l2-core/videobuf2-core.c |  324 +-
  include/media/videobuf2-core.h   |   50 ++---
  include/media/videobuf2-v4l2.h   |   26 +++
  3 files changed, 236 insertions(+), 164 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index ab00ea0..9266d50 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -35,10 +35,10 @@
  static int debug;
  module_param(debug, int, 0644);

-#define dprintk(level, fmt, arg...)  \
-   do {  \
-   if (debug = level)\
-   pr_info(vb2: %s:  fmt, __func__, ## arg); \
+#define dprintk(level, fmt, arg...)\
+   do {\
+   if (debug = level)  \
+   pr_info(vb2: %s:  fmt, __func__, ## arg);   \


These are just whitespace changes, and that is something it see *a lot* in this
patch. And usually for no clear reason.

Please remove those whitespace changes, it makes a difficult patch even harder
to read than it already is.



I just wanted to remove unnecessary white spaces or adjust alignment.
OK, I will revert those whitespace changes for better review.


} while (0)

  #ifdef CONFIG_VIDEO_ADV_DEBUG


snip


@@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q)
   */
  static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
  {
+   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct vb2_queue *q = vb-vb2_queue;
+   unsigned int plane;

/* Copy back data such as timestamp, flags, etc. */
-   memcpy(b, vb-v4l2_buf, offsetof(struct v4l2_buffer, m));
-   b-reserved2 = vb-v4l2_buf.reserved2;
-   b-reserved = vb-v4l2_buf.reserved;


Hmm, I'm not sure why these reserved fields were copied here. I think it was
for compatibility reasons for some old drivers that abused the reserved field.
However, in the new code these reserved fields should probably be explicitly
initialized to 0.


+   b-index = vb-index;
+   b-type = vb-type;
+   b-memory = vb-memory;
+   b-bytesused = 0;
+
+   b-flags = vbuf-flags;
+   b-field = vbuf-field;
+   b-timestamp = vbuf-timestamp;
+   b-timecode = vbuf-timecode;
+   b-sequence = vbuf-sequence;

if (V4L2_TYPE_IS_MULTIPLANAR(q-type)) {
/*
@@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
struct v4l2_buffer *b)
 * for it. The caller has already verified memory and size.
 */
b-length = vb-num_planes;
-   memcpy(b-m.planes, vb-v4l2_planes,
-   b-length * sizeof(struct v4l2_plane));


A similar problem occurs here: the memcpy would have copied the reserved
field in v4l2_plane as well, but that no longer happens, so you need to
do an explicit memset for the reserved field in the new code.



It means that 

Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-28 Thread Hans Verkuil
Hi Junghak,

Thanks for this patch, it looks much better. I do have a number of comments, 
though...

On 08/26/2015 01:59 PM, Junghak Sung wrote:
 Remove v4l2-specific stuff from struct vb2_buffer and add member variables
 related with buffer management.
 
 struct vb2_plane {
 snip
 /* plane information */
 unsigned intbytesused;
 unsigned intlength;
 union {
 unsigned intoffset;
 unsigned long   userptr;
 int fd;
 } m;
 unsigned intdata_offset;
 }
 
 struct vb2_buffer {
 snip
 /* buffer information */
 unsigned intnum_planes;
 unsigned intindex;
 unsigned inttype;
 unsigned intmemory;
 
 struct vb2_planeplanes[VIDEO_MAX_PLANES];
 snip
 };
 
 And create struct vb2_v4l2_buffer as container buffer for v4l2 use.
 
 struct vb2_v4l2_buffer {
 struct vb2_buffer   vb2_buf;
 
 __u32   flags;
 __u32   field;
 struct timeval  timestamp;
 struct v4l2_timecodetimecode;
 __u32   sequence;
 };
 
 This patch includes only changes inside of videobuf2. So, it is required to
 modify all device drivers which use videobuf2.
 
 Signed-off-by: Junghak Sung jh1009.s...@samsung.com
 Signed-off-by: Geunyoung Kim nenggun@samsung.com
 Acked-by: Seung-Woo Kim sw0312@samsung.com
 Acked-by: Inki Dae inki@samsung.com
 ---
  drivers/media/v4l2-core/videobuf2-core.c |  324 
 +-
  include/media/videobuf2-core.h   |   50 ++---
  include/media/videobuf2-v4l2.h   |   26 +++
  3 files changed, 236 insertions(+), 164 deletions(-)
 
 diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
 b/drivers/media/v4l2-core/videobuf2-core.c
 index ab00ea0..9266d50 100644
 --- a/drivers/media/v4l2-core/videobuf2-core.c
 +++ b/drivers/media/v4l2-core/videobuf2-core.c
 @@ -35,10 +35,10 @@
  static int debug;
  module_param(debug, int, 0644);
  
 -#define dprintk(level, fmt, arg...)\
 - do {  \
 - if (debug = level)   \
 - pr_info(vb2: %s:  fmt, __func__, ## arg); \
 +#define dprintk(level, fmt, arg...)  \
 + do {\
 + if (debug = level) \
 + pr_info(vb2: %s:  fmt, __func__, ## arg); \

These are just whitespace changes, and that is something it see *a lot* in this
patch. And usually for no clear reason.

Please remove those whitespace changes, it makes a difficult patch even harder
to read than it already is.

   } while (0)
  
  #ifdef CONFIG_VIDEO_ADV_DEBUG

snip

 @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q)
   */
  static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
  {
 + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
   struct vb2_queue *q = vb-vb2_queue;
 + unsigned int plane;
  
   /* Copy back data such as timestamp, flags, etc. */
 - memcpy(b, vb-v4l2_buf, offsetof(struct v4l2_buffer, m));
 - b-reserved2 = vb-v4l2_buf.reserved2;
 - b-reserved = vb-v4l2_buf.reserved;

Hmm, I'm not sure why these reserved fields were copied here. I think it was
for compatibility reasons for some old drivers that abused the reserved field.
However, in the new code these reserved fields should probably be explicitly
initialized to 0.

 + b-index = vb-index;
 + b-type = vb-type;
 + b-memory = vb-memory;
 + b-bytesused = 0;
 +
 + b-flags = vbuf-flags;
 + b-field = vbuf-field;
 + b-timestamp = vbuf-timestamp;
 + b-timecode = vbuf-timecode;
 + b-sequence = vbuf-sequence;
  
   if (V4L2_TYPE_IS_MULTIPLANAR(q-type)) {
   /*
 @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, 
 struct v4l2_buffer *b)
* for it. The caller has already verified memory and size.
*/
   b-length = vb-num_planes;
 - memcpy(b-m.planes, vb-v4l2_planes,
 - b-length * sizeof(struct v4l2_plane));

A similar problem occurs here: the memcpy would have copied the reserved
field in v4l2_plane as well, but that no longer happens, so you need to
do an explicit memset for the reserved field in the new code.

 + for (plane = 0; plane  vb-num_planes; ++plane) {
 + struct v4l2_plane *pdst = b-m.planes[plane];
 + struct vb2_plane *psrc = vb-planes[plane];
 +
 + pdst-bytesused = psrc-bytesused;
 + pdst-length 

Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-27 Thread Junghak Sung


Hello Mauro,

First of all, thank you for your detailed review.


On 08/27/2015 07:28 PM, Mauro Carvalho Chehab wrote:

Em Wed, 26 Aug 2015 20:59:29 +0900
Junghak Sung jh1009.s...@samsung.com escreveu:


Remove v4l2-specific stuff from struct vb2_buffer and add member variables
related with buffer management.

struct vb2_plane {
 snip
 /* plane information */
 unsigned intbytesused;
 unsigned intlength;
 union {
 unsigned intoffset;
 unsigned long   userptr;
 int fd;
 } m;
 unsigned intdata_offset;
}

struct vb2_buffer {
 snip
 /* buffer information */
 unsigned intnum_planes;
 unsigned intindex;
 unsigned inttype;
 unsigned intmemory;

 struct vb2_planeplanes[VIDEO_MAX_PLANES];
 snip
};

And create struct vb2_v4l2_buffer as container buffer for v4l2 use.

struct vb2_v4l2_buffer {
 struct vb2_buffer   vb2_buf;

 __u32   flags;
 __u32   field;
 struct timeval  timestamp;
 struct v4l2_timecodetimecode;
 __u32   sequence;
};


The comments seemed a little hard for me to read, but the changes
look ok.



Ok, I will modify these comments more clearly at next round.


I made some comments mostly regarding to documentation. See below.


This patch includes only changes inside of videobuf2. So, it is required to
modify all device drivers which use videobuf2.


So, in practice, we need to fold both patches 2 and 3 when merging upstream,
to avoid breaking git bisectability.



I'm sorry, but I can not understand the meaning of fold both patches.
Would you please explain more detailed what should I do at next round.
If these two patches are get together to one patch, the size will
be over 300KB, which causes a size problem to send it to ML.



Signed-off-by: Junghak Sung jh1009.s...@samsung.com
Signed-off-by: Geunyoung Kim nenggun@samsung.com
Acked-by: Seung-Woo Kim sw0312@samsung.com
Acked-by: Inki Dae inki@samsung.com
---
  drivers/media/v4l2-core/videobuf2-core.c |  324 +-
  include/media/videobuf2-core.h   |   50 ++---
  include/media/videobuf2-v4l2.h   |   26 +++
  3 files changed, 236 insertions(+), 164 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index ab00ea0..9266d50 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -35,10 +35,10 @@
  static int debug;
  module_param(debug, int, 0644);

-#define dprintk(level, fmt, arg...)  \
-   do {  \
-   if (debug = level)\
-   pr_info(vb2: %s:  fmt, __func__, ## arg); \
+#define dprintk(level, fmt, arg...)\
+   do {\
+   if (debug = level)  \
+   pr_info(vb2: %s:  fmt, __func__, ## arg);   \
} while (0)

  #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -53,7 +53,7 @@ module_param(debug, int, 0644);

  #define log_memop(vb, op) \
dprintk(2, call_memop(%p, %d, %s)%s\n,  \
-   (vb)-vb2_queue, (vb)-v4l2_buf.index, #op,   \
+   (vb)-vb2_queue, (vb)-index, #op,\
(vb)-vb2_queue-mem_ops-op ?  :  (nop))

  #define call_memop(vb, op, args...)   \
@@ -115,7 +115,7 @@ module_param(debug, int, 0644);

  #define log_vb_qop(vb, op, args...)   \
dprintk(2, call_vb_qop(%p, %d, %s)%s\n, \
-   (vb)-vb2_queue, (vb)-v4l2_buf.index, #op,   \
+   (vb)-vb2_queue, (vb)-index, #op,\
(vb)-vb2_queue-ops-op ?  :  (nop))

  #define call_vb_qop(vb, op, args...)  \
@@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
unsigned long size = PAGE_ALIGN(q-plane_sizes[plane]);

mem_priv = call_ptr_memop(vb, alloc, q-alloc_ctx[plane],
- size, dma_dir, q-gfp_flags);
+   size, dma_dir, q-gfp_flags);
if (IS_ERR_OR_NULL(mem_priv))
goto free;

/* Associate allocator private data with this plane */
vb-planes[plane].mem_priv = mem_priv;
-   vb-v4l2_planes[plane].length = 

Re: [RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-27 Thread Mauro Carvalho Chehab
Em Wed, 26 Aug 2015 20:59:29 +0900
Junghak Sung jh1009.s...@samsung.com escreveu:

 Remove v4l2-specific stuff from struct vb2_buffer and add member variables
 related with buffer management.
 
 struct vb2_plane {
 snip
 /* plane information */
 unsigned intbytesused;
 unsigned intlength;
 union {
 unsigned intoffset;
 unsigned long   userptr;
 int fd;
 } m;
 unsigned intdata_offset;
 }
 
 struct vb2_buffer {
 snip
 /* buffer information */
 unsigned intnum_planes;
 unsigned intindex;
 unsigned inttype;
 unsigned intmemory;
 
 struct vb2_planeplanes[VIDEO_MAX_PLANES];
 snip
 };
 
 And create struct vb2_v4l2_buffer as container buffer for v4l2 use.
 
 struct vb2_v4l2_buffer {
 struct vb2_buffer   vb2_buf;
 
 __u32   flags;
 __u32   field;
 struct timeval  timestamp;
 struct v4l2_timecodetimecode;
 __u32   sequence;
 };

The comments seemed a little hard for me to read, but the changes
look ok.

I made some comments mostly regarding to documentation. See below.

 This patch includes only changes inside of videobuf2. So, it is required to
 modify all device drivers which use videobuf2.

So, in practice, we need to fold both patches 2 and 3 when merging upstream,
to avoid breaking git bisectability.

 
 Signed-off-by: Junghak Sung jh1009.s...@samsung.com
 Signed-off-by: Geunyoung Kim nenggun@samsung.com
 Acked-by: Seung-Woo Kim sw0312@samsung.com
 Acked-by: Inki Dae inki@samsung.com
 ---
  drivers/media/v4l2-core/videobuf2-core.c |  324 
 +-
  include/media/videobuf2-core.h   |   50 ++---
  include/media/videobuf2-v4l2.h   |   26 +++
  3 files changed, 236 insertions(+), 164 deletions(-)
 
 diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
 b/drivers/media/v4l2-core/videobuf2-core.c
 index ab00ea0..9266d50 100644
 --- a/drivers/media/v4l2-core/videobuf2-core.c
 +++ b/drivers/media/v4l2-core/videobuf2-core.c
 @@ -35,10 +35,10 @@
  static int debug;
  module_param(debug, int, 0644);
  
 -#define dprintk(level, fmt, arg...)\
 - do {  \
 - if (debug = level)   \
 - pr_info(vb2: %s:  fmt, __func__, ## arg); \
 +#define dprintk(level, fmt, arg...)  \
 + do {\
 + if (debug = level) \
 + pr_info(vb2: %s:  fmt, __func__, ## arg); \
   } while (0)
  
  #ifdef CONFIG_VIDEO_ADV_DEBUG
 @@ -53,7 +53,7 @@ module_param(debug, int, 0644);
  
  #define log_memop(vb, op)\
   dprintk(2, call_memop(%p, %d, %s)%s\n,\
 - (vb)-vb2_queue, (vb)-v4l2_buf.index, #op, \
 + (vb)-vb2_queue, (vb)-index, #op,  \
   (vb)-vb2_queue-mem_ops-op ?  :  (nop))
  
  #define call_memop(vb, op, args...)  \
 @@ -115,7 +115,7 @@ module_param(debug, int, 0644);
  
  #define log_vb_qop(vb, op, args...)  \
   dprintk(2, call_vb_qop(%p, %d, %s)%s\n,   \
 - (vb)-vb2_queue, (vb)-v4l2_buf.index, #op, \
 + (vb)-vb2_queue, (vb)-index, #op,  \
   (vb)-vb2_queue-ops-op ?  :  (nop))
  
  #define call_vb_qop(vb, op, args...) \
 @@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
   unsigned long size = PAGE_ALIGN(q-plane_sizes[plane]);
  
   mem_priv = call_ptr_memop(vb, alloc, q-alloc_ctx[plane],
 -   size, dma_dir, q-gfp_flags);
 + size, dma_dir, q-gfp_flags);
   if (IS_ERR_OR_NULL(mem_priv))
   goto free;
  
   /* Associate allocator private data with this plane */
   vb-planes[plane].mem_priv = mem_priv;
 - vb-v4l2_planes[plane].length = q-plane_sizes[plane];
 + vb-planes[plane].length = q-plane_sizes[plane];
   }
  
   return 0;
 @@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
   for (plane = 0; plane  vb-num_planes; ++plane) {
   call_void_memop(vb, put, vb-planes[plane].mem_priv);
   vb-planes[plane].mem_priv = NULL;
 - dprintk(3, freed plane %d of buffer %d\n, plane,
 -  

[RFC PATCH v3 2/5] media: videobuf2: Restructure vb2_buffer

2015-08-26 Thread Junghak Sung
Remove v4l2-specific stuff from struct vb2_buffer and add member variables
related with buffer management.

struct vb2_plane {
snip
/* plane information */
unsigned intbytesused;
unsigned intlength;
union {
unsigned intoffset;
unsigned long   userptr;
int fd;
} m;
unsigned intdata_offset;
}

struct vb2_buffer {
snip
/* buffer information */
unsigned intnum_planes;
unsigned intindex;
unsigned inttype;
unsigned intmemory;

struct vb2_planeplanes[VIDEO_MAX_PLANES];
snip
};

And create struct vb2_v4l2_buffer as container buffer for v4l2 use.

struct vb2_v4l2_buffer {
struct vb2_buffer   vb2_buf;

__u32   flags;
__u32   field;
struct timeval  timestamp;
struct v4l2_timecodetimecode;
__u32   sequence;
};

This patch includes only changes inside of videobuf2. So, it is required to
modify all device drivers which use videobuf2.

Signed-off-by: Junghak Sung jh1009.s...@samsung.com
Signed-off-by: Geunyoung Kim nenggun@samsung.com
Acked-by: Seung-Woo Kim sw0312@samsung.com
Acked-by: Inki Dae inki@samsung.com
---
 drivers/media/v4l2-core/videobuf2-core.c |  324 +-
 include/media/videobuf2-core.h   |   50 ++---
 include/media/videobuf2-v4l2.h   |   26 +++
 3 files changed, 236 insertions(+), 164 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index ab00ea0..9266d50 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -35,10 +35,10 @@
 static int debug;
 module_param(debug, int, 0644);
 
-#define dprintk(level, fmt, arg...)  \
-   do {  \
-   if (debug = level)   \
-   pr_info(vb2: %s:  fmt, __func__, ## arg); \
+#define dprintk(level, fmt, arg...)\
+   do {\
+   if (debug = level) \
+   pr_info(vb2: %s:  fmt, __func__, ## arg); \
} while (0)
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -53,7 +53,7 @@ module_param(debug, int, 0644);
 
 #define log_memop(vb, op)  \
dprintk(2, call_memop(%p, %d, %s)%s\n,\
-   (vb)-vb2_queue, (vb)-v4l2_buf.index, #op, \
+   (vb)-vb2_queue, (vb)-index, #op,  \
(vb)-vb2_queue-mem_ops-op ?  :  (nop))
 
 #define call_memop(vb, op, args...)\
@@ -115,7 +115,7 @@ module_param(debug, int, 0644);
 
 #define log_vb_qop(vb, op, args...)\
dprintk(2, call_vb_qop(%p, %d, %s)%s\n,   \
-   (vb)-vb2_queue, (vb)-v4l2_buf.index, #op, \
+   (vb)-vb2_queue, (vb)-index, #op,  \
(vb)-vb2_queue-ops-op ?  :  (nop))
 
 #define call_vb_qop(vb, op, args...)   \
@@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
unsigned long size = PAGE_ALIGN(q-plane_sizes[plane]);
 
mem_priv = call_ptr_memop(vb, alloc, q-alloc_ctx[plane],
- size, dma_dir, q-gfp_flags);
+   size, dma_dir, q-gfp_flags);
if (IS_ERR_OR_NULL(mem_priv))
goto free;
 
/* Associate allocator private data with this plane */
vb-planes[plane].mem_priv = mem_priv;
-   vb-v4l2_planes[plane].length = q-plane_sizes[plane];
+   vb-planes[plane].length = q-plane_sizes[plane];
}
 
return 0;
@@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
for (plane = 0; plane  vb-num_planes; ++plane) {
call_void_memop(vb, put, vb-planes[plane].mem_priv);
vb-planes[plane].mem_priv = NULL;
-   dprintk(3, freed plane %d of buffer %d\n, plane,
-   vb-v4l2_buf.index);
+   dprintk(3, freed plane %d of buffer %d\n, plane, vb-index);
}
 }
 
@@ -269,7 +268,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, 
struct vb2_plane *p)
 
call_void_memop(vb, detach_dmabuf, p-mem_priv);
dma_buf_put(p-dbuf);
-   memset(p, 0, sizeof(*p));
+   p-mem_priv = NULL;
+   p-dbuf =