Re: [PATCH 04/10] v4l: vb2: fixes for DMABUF support

2012-01-25 Thread Tomasz Stanislawski


Hi Sumit,
On 01/25/2012 06:35 AM, Semwal, Sumit wrote:

Hi Tomasz,
On Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski
t.stanisl...@samsung.com  wrote:

Hi Mauro,



snip


Ok. I should have given more details about the patch. I am sorry for missing
it. My kernel tree failed to compile after applying patches from

[1]
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968

I had to generate this patch to compile the code and test it. Most of the
fixes refer to Sumit's code and I think he will take care of those bugs.

Is your kernel tree a mainline kernel? I am pretty sure I posted out
the RFC after compile testing.


Our development kernel often contains patches that are not posted to 
opensource. The tree presented in the cover letter contains only patches 
that were approved for opensource submission.


Some of the patches that are not merged into the mainline may break 
compilation if patches from the mailing list are applied on the top. The 
example is 'media: vb2: remove plane argument from call_memop and 
cleanup mempriv usage'. I had to add fixes to compile the code. Moreover 
I had to test a working application that makes use of DMABUF 
exporting/importing via V4L2 API. So I had to fix other issues that are 
not only compilation related.


As I remember we agreed that I had to post an incremental patchset.
Therefore all needed fixes had to be present in the tree.

The fixes were posted in this patchset to keep the whole work together.
I expect that you already prepared a patch fixing majority of issues 
from this patch. Many of them were mentioned in Pawel's and Laurent's 
and Sakari's reviews. If you find fixes in this patch useful you can 
merge them into next version of RFC 'v4l: DMA buffer sharing support as 
a user'.







snip



I wanted to post the complete set of patches that produce compilable kernel.
Therefore most important bugs/issues had to be fixed and attached to the
patchset. Some of the issues in [1] were mentioned by Laurent and Sakari. I
hope Sumit will take care of those problems.

I must've misunderstood when you said 'I would like to take care of
these patches'. Please let me know if you'd like me to submit next
version of my RFC separately with fixes for these issues, or would you
manage that as part of your RFC patch series submission.


This patchset is an RFC. It was my big mistake that I forgot to add this 
to the title of the patchset. I am not going to post the patch with 
fixes to your part any more. It would be great if you merged it into new 
version of 'DMA buffer sharing support as a user'.


IMO, some parts should go as separate threads:
- extension to DMA subsystem, introduction of dma_get_pages. This would 
probably go to DMA mailing list.

- redesign of dma-contig allocator (w/o dmabuf exporting/importing)
- buffer importing via dmabuf in V4L2 and vb2-dma-contig
- buffer exporting via dmabuf in V4L2 and vb2-dma-contig

BTW. Could you state your opinion on presented solution for dma-buf 
exporting in vb2-core and vb2-dma-contig allocator?


Regards,
Tomasz Stanislawski





Failing to do that will mean that important fixes for upstream
will be missed.



Ok. It will be fixed.



Regards,
Mauro



Regards,
Tomasz Stanislawski


Best regards,
~Sumit.


--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-25 Thread Semwal, Sumit
Hi Tomasz,
On Wed, Jan 25, 2012 at 4:04 PM, Tomasz Stanislawski
t.stanisl...@samsung.com wrote:

 Hi Sumit,

 On 01/25/2012 06:35 AM, Semwal, Sumit wrote:

 Hi Tomasz,
 On Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski
 t.stanisl...@samsung.com  wrote:

 Hi Mauro,


 snip


 Ok. I should have given more details about the patch. I am sorry for
 missing
 it. My kernel tree failed to compile after applying patches from

 [1]

 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968

 I had to generate this patch to compile the code and test it. Most of the
 fixes refer to Sumit's code and I think he will take care of those bugs.

 Is your kernel tree a mainline kernel? I am pretty sure I posted out
 the RFC after compile testing.


 Our development kernel often contains patches that are not posted to
 opensource. The tree presented in the cover letter contains only patches
 that were approved for opensource submission.
Right; I understand that - I just wanted to make sure you didn't hit
some problem with my patches that I didn't. Thanks for confirming that
it was your dev kernel.

 Some of the patches that are not merged into the mainline may break
 compilation if patches from the mailing list are applied on the top. The
 example is 'media: vb2: remove plane argument from call_memop and cleanup
 mempriv usage'. I had to add fixes to compile the code. Moreover I had to
 test a working application that makes use of DMABUF exporting/importing via
 V4L2 API. So I had to fix other issues that are not only compilation
 related.

I understand.
 As I remember we agreed that I had to post an incremental patchset.
 Therefore all needed fixes had to be present in the tree.

 The fixes were posted in this patchset to keep the whole work together.
 I expect that you already prepared a patch fixing majority of issues from
 this patch. Many of them were mentioned in Pawel's and Laurent's and
 Sakari's reviews. If you find fixes in this patch useful you can merge them
 into next version of RFC 'v4l: DMA buffer sharing support as a user'.


OK - this makes it quite clear; I will re-work my RFC then.



 snip



 I wanted to post the complete set of patches that produce compilable
 kernel.
 Therefore most important bugs/issues had to be fixed and attached to the
 patchset. Some of the issues in [1] were mentioned by Laurent and Sakari.
 I
 hope Sumit will take care of those problems.

 I must've misunderstood when you said 'I would like to take care of
 these patches'. Please let me know if you'd like me to submit next
 version of my RFC separately with fixes for these issues, or would you
 manage that as part of your RFC patch series submission.


 This patchset is an RFC. It was my big mistake that I forgot to add this to
 the title of the patchset. I am not going to post the patch with fixes to
 your part any more. It would be great if you merged it into new version of
 'DMA buffer sharing support as a user'.

That's OK with me.

 IMO, some parts should go as separate threads:
 - extension to DMA subsystem, introduction of dma_get_pages. This would
 probably go to DMA mailing list.
 - redesign of dma-contig allocator (w/o dmabuf exporting/importing)
 - buffer importing via dmabuf in V4L2 and vb2-dma-contig
 - buffer exporting via dmabuf in V4L2 and vb2-dma-contig

 BTW. Could you state your opinion on presented solution for dma-buf
 exporting in vb2-core and vb2-dma-contig allocator?

I agree with your ordering of these parts; Also, with this ordering, I
guess I should pay more attention to parts 1. (extension to DMA...)
and 2. (redesign of dma-contig allocator...) - which I hope you are
going to do? I can then base out my next version of RFC on these.

I was OoO for past couple of days, hence missed all the action :) - I
will try and go through your approach, and comment as soon as I can.
Hopefully in a couple of days.
 Regards,
 Tomasz Stanislawski


snip
Best regards,
~Sumit.
--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-24 Thread Semwal, Sumit
Hi Tomasz,
On Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski
t.stanisl...@samsung.com wrote:
 Hi Mauro,


snip

 Ok. I should have given more details about the patch. I am sorry for missing
 it. My kernel tree failed to compile after applying patches from

 [1]
 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968

 I had to generate this patch to compile the code and test it. Most of the
 fixes refer to Sumit's code and I think he will take care of those bugs.
Is your kernel tree a mainline kernel? I am pretty sure I posted out
the RFC after compile testing.


snip


 I wanted to post the complete set of patches that produce compilable kernel.
 Therefore most important bugs/issues had to be fixed and attached to the
 patchset. Some of the issues in [1] were mentioned by Laurent and Sakari. I
 hope Sumit will take care of those problems.
I must've misunderstood when you said 'I would like to take care of
these patches'. Please let me know if you'd like me to submit next
version of my RFC separately with fixes for these issues, or would you
manage that as part of your RFC patch series submission.


 Failing to do that will mean that important fixes for upstream
 will be missed.


 Ok. It will be fixed.


 Regards,
 Mauro


 Regards,
 Tomasz Stanislawski

Best regards,
~Sumit.
--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Mauro Carvalho Chehab
Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:
 Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

Please better describe this patch. What is it supposing to fix?

 ---
  drivers/media/video/videobuf2-core.c |   21 +
  include/media/videobuf2-core.h   |6 +++---
  2 files changed, 12 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/media/video/videobuf2-core.c 
 b/drivers/media/video/videobuf2-core.c
 index cb85874..59bb1bc 100644
 --- a/drivers/media/video/videobuf2-core.c
 +++ b/drivers/media/video/videobuf2-core.c
 @@ -119,7 +119,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
   void *mem_priv = vb-planes[plane].mem_priv;
  
   if (mem_priv) {
 - call_memop(q, plane, detach_dmabuf, mem_priv);
 + call_memop(q, detach_dmabuf, mem_priv);

Huh? You're not removing the plane parameter on this patch, but, instead,
on a previous patch.

No patch is allowed to break compilation, as it breaks git bisect.

   dma_buf_put(vb-planes[plane].dbuf);
   vb-planes[plane].dbuf = NULL;
   vb-planes[plane].mem_priv = NULL;
 @@ -907,6 +907,8 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const 
 struct v4l2_buffer *b,
   if (b-memory == V4L2_MEMORY_DMABUF) {
   for (plane = 0; plane  vb-num_planes; ++plane) {
   v4l2_planes[plane].m.fd = 
 b-m.planes[plane].m.fd;
 + v4l2_planes[plane].length =
 + b-m.planes[plane].length;
   }
   }
   } else {
 @@ -1055,15 +1057,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const 
 struct v4l2_buffer *b)
   if (IS_ERR_OR_NULL(dbuf)) {
   dprintk(1, qbuf: invalid dmabuf fd for 
   plane %d\n, plane);
 - ret = PTR_ERR(dbuf);
 + ret = -EINVAL;
   goto err;
   }
  
 - /* this doesn't get filled in until __fill_vb2_buffer(),
 -  * since it isn't known until after dma_buf_get()..
 -  */
 - planes[plane].length = dbuf-size;
 -
   /* Skip the plane if already verified */
   if (dbuf == vb-planes[plane].dbuf) {
   dma_buf_put(dbuf);
 @@ -1075,7 +1072,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const 
 struct v4l2_buffer *b)
  
   /* Release previously acquired memory if present */
   if (vb-planes[plane].mem_priv) {
 - call_memop(q, plane, detach_dmabuf,
 + call_memop(q, detach_dmabuf,
   vb-planes[plane].mem_priv);
   dma_buf_put(vb-planes[plane].dbuf);
   }
 @@ -1083,8 +1080,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const 
 struct v4l2_buffer *b)
   vb-planes[plane].mem_priv = NULL;
  
   /* Acquire each plane's memory */
 - mem_priv = q-mem_ops-attach_dmabuf(
 - q-alloc_ctx[plane], dbuf);
 + mem_priv = q-mem_ops-attach_dmabuf(q-alloc_ctx[plane],
 + dbuf, planes[plane].length, write);
   if (IS_ERR(mem_priv)) {
   dprintk(1, qbuf: failed acquiring dmabuf 
   memory for plane %d\n, plane);
 @@ -1102,7 +1099,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const 
 struct v4l2_buffer *b)
*/
   for (plane = 0; plane  vb-num_planes; ++plane) {
   ret = q-mem_ops-map_dmabuf(
 - vb-planes[plane].mem_priv, write);
 + vb-planes[plane].mem_priv);
   if (ret) {
   dprintk(1, qbuf: failed mapping dmabuf 
   memory for plane %d\n, plane);
 @@ -1497,7 +1494,7 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer 
 *b, bool nonblocking)
*/
   if (q-memory == V4L2_MEMORY_DMABUF)
   for (plane = 0; plane  vb-num_planes; ++plane)
 - call_memop(q, plane, unmap_dmabuf,
 + call_memop(q, unmap_dmabuf,
   vb-planes[plane].mem_priv);
  
   switch (vb-state) {
 diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
 index d8b8171..412c6a4 100644
 --- a/include/media/videobuf2-core.h
 +++ b/include/media/videobuf2-core.h
 @@ -88,10 +88,10 @@ struct vb2_mem_ops {
* in the vb2 core, and vb2_mem_ops really just need to get/put the
* sglist (and make sure that the sglist fits it's needs..)
*/
 - void*(*attach_dmabuf)(void *alloc_ctx,
 -   struct dma_buf *dbuf);
 + void 

Re: [PATCH 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Tomasz Stanislawski

Hi Mauro,

On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:

Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com


Please better describe this patch. What is it supposing to fix?


Usually compilation error or bugs discovered in previous vb2-dma-contig 
patches adding support for dma-buf.





---
  drivers/media/video/videobuf2-core.c |   21 +
  include/media/videobuf2-core.h   |6 +++---
  2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c 
b/drivers/media/video/videobuf2-core.c
index cb85874..59bb1bc 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -119,7 +119,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
void *mem_priv = vb-planes[plane].mem_priv;

if (mem_priv) {
-   call_memop(q, plane, detach_dmabuf, mem_priv);
+   call_memop(q, detach_dmabuf, mem_priv);


Huh? You're not removing the plane parameter on this patch, but, instead,
on a previous patch.

No patch is allowed to break compilation, as it breaks git bisect.


I agree that patches should not break compilation if patches are 
accepted to the mainline. There is a big compilation failure at patch 07 
where videobuf2-dma-contig.c disappears.


Note that these are proof-of-concept patches for support of dma-buf 
exporting/importing dma-buf in V4L2. It would be a waste of time 
polished the patches if they are going to be rejected due to design flaws.


Regards,
Tomasz Stanislawski
--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Mauro Carvalho Chehab
Em 23-01-2012 12:32, Tomasz Stanislawski escreveu:
 Hi Mauro,
 
 On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:
 Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com

 Please better describe this patch. What is it supposing to fix?
 
 Usually compilation error or bugs discovered in previous
 vb2-dma-contig patches adding support for dma-buf.


 ---
   drivers/media/video/videobuf2-core.c |   21 +
   include/media/videobuf2-core.h   |6 +++---
   2 files changed, 12 insertions(+), 15 deletions(-)

 diff --git a/drivers/media/video/videobuf2-core.c 
 b/drivers/media/video/videobuf2-core.c
 index cb85874..59bb1bc 100644
 --- a/drivers/media/video/videobuf2-core.c
 +++ b/drivers/media/video/videobuf2-core.c
 @@ -119,7 +119,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
   void *mem_priv = vb-planes[plane].mem_priv;

   if (mem_priv) {
 -call_memop(q, plane, detach_dmabuf, mem_priv);
 +call_memop(q, detach_dmabuf, mem_priv);

 Huh? You're not removing the plane parameter on this patch, but, instead,
 on a previous patch.

 No patch is allowed to break compilation, as it breaks git bisect.
 
 I agree that patches should not break compilation if patches are accepted to
 the mainline. There is a big compilation failure at patch 07 where 
 videobuf2-dma-contig.c disappears.
 
 Note that these are proof-of-concept patches for support of dma-buf 
 exporting/importing dma-buf in V4L2.
 It would be a waste of time polished the patches if they are going to be 
 rejected due to design flaws.

It is a waste of time for the reviewers to see a patch like this one,
as:
- No description of what is inside the patch is provided;
- changes that should be happening inside the other patches are
  mixed here.

It is also a waste of your time to submit a patch that will need to be later
polished, as you'll need to work with the same thing twice.

So, please fix your patch workflow. As a general rule, you should
compile every patch you're applying and fix the breakages on them.

Also, if you found bugs that need to be fixed and that aren't 
directly related to your current task, those should generate
their own patches, and submitted in separate, in order to be
applied sooner upstream and to stable.

Failing to do that will mean that important fixes for upstream
will be missed.

Regards,
Mauro
--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Tomasz Stanislawski

Hi Mauro,

On 01/23/2012 03:52 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 12:32, Tomasz Stanislawski escreveu:

Hi Mauro,

On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:

Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com


Please better describe this patch. What is it supposing to fix?


Usually compilation error or bugs discovered in previous
vb2-dma-contig patches adding support for dma-buf.




---
   drivers/media/video/videobuf2-core.c |   21 +
   include/media/videobuf2-core.h   |6 +++---
   2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c 
b/drivers/media/video/videobuf2-core.c
index cb85874..59bb1bc 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -119,7 +119,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
   void *mem_priv = vb-planes[plane].mem_priv;

   if (mem_priv) {
-call_memop(q, plane, detach_dmabuf, mem_priv);
+call_memop(q, detach_dmabuf, mem_priv);


Huh? You're not removing the plane parameter on this patch, but, instead,
on a previous patch.

No patch is allowed to break compilation, as it breaks git bisect.


I agree that patches should not break compilation if patches are accepted to
the mainline. There is a big compilation failure at patch 07 where 
videobuf2-dma-contig.c disappears.

Note that these are proof-of-concept patches for support of dma-buf 
exporting/importing dma-buf in V4L2.
It would be a waste of time polished the patches if they are going to be 
rejected due to design flaws.


It is a waste of time for the reviewers to see a patch like this one,
as:
- No description of what is inside the patch is provided;


Ok. I should have given more details about the patch. I am sorry for 
missing it. My kernel tree failed to compile after applying patches from


[1] 
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968


I had to generate this patch to compile the code and test it. Most of 
the fixes refer to Sumit's code and I think he will take care of those bugs.


I admit that I focused on other patches. Like DMA extension, exporting 
in vb2-core and vb2-dma-contig. Sorry for putting so little attention to 
bugfixing patch.



- changes that should be happening inside the other patches are
  mixed here.


Right. I missed call_memop here.



It is also a waste of your time to submit a patch that will need to be later
polished, as you'll need to work with the same thing twice.


The problem is that those patches were not intended to be accepted. The 
were intended for discussion. The other problem is that there are many 
people waiting for those patches. The dma-buf was already accepted to 
the mainline. Me and Sumit are trying to help V4L2 to catch-up. The 
dma-buf and its support in vb2-core seams to change very dynamically. 
Polishing the patch takes much time. If the dma-buf API changes the 
design of vb2-core may have to be changed. Therefore time spent on 
polishing would be lost. I am sorry for patch flaws. All of them would 
be fixed when the design is stabilized.




So, please fix your patch workflow. As a general rule, you should
compile every patch you're applying and fix the breakages on them.

Also, if you found bugs that need to be fixed and that aren't
directly related to your current task, those should generate
their own patches, and submitted in separate, in order to be
applied sooner upstream and to stable.


I wanted to post the complete set of patches that produce compilable 
kernel. Therefore most important bugs/issues had to be fixed and 
attached to the patchset. Some of the issues in [1] were mentioned by 
Laurent and Sakari. I hope Sumit will take care of those problems.




Failing to do that will mean that important fixes for upstream
will be missed.


Ok. It will be fixed.



Regards,
Mauro



Regards,
Tomasz Stanislawski
--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Mauro Carvalho Chehab
Em 23-01-2012 13:25, Tomasz Stanislawski escreveu:
 Hi Mauro,
 
 On 01/23/2012 03:52 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 12:32, Tomasz Stanislawski escreveu:
 Hi Mauro,

 On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:
 Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com

 Please better describe this patch. What is it supposing to fix?

 Usually compilation error or bugs discovered in previous
 vb2-dma-contig patches adding support for dma-buf.


 ---
drivers/media/video/videobuf2-core.c |   21 +
include/media/videobuf2-core.h   |6 +++---
2 files changed, 12 insertions(+), 15 deletions(-)

 diff --git a/drivers/media/video/videobuf2-core.c 
 b/drivers/media/video/videobuf2-core.c
 index cb85874..59bb1bc 100644
 --- a/drivers/media/video/videobuf2-core.c
 +++ b/drivers/media/video/videobuf2-core.c
 @@ -119,7 +119,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer 
 *vb)
void *mem_priv = vb-planes[plane].mem_priv;

if (mem_priv) {
 -call_memop(q, plane, detach_dmabuf, mem_priv);
 +call_memop(q, detach_dmabuf, mem_priv);

 Huh? You're not removing the plane parameter on this patch, but, instead,
 on a previous patch.

 No patch is allowed to break compilation, as it breaks git bisect.

 I agree that patches should not break compilation if patches are accepted to
 the mainline. There is a big compilation failure at patch 07 where 
 videobuf2-dma-contig.c disappears.

 Note that these are proof-of-concept patches for support of dma-buf 
 exporting/importing dma-buf in V4L2.
 It would be a waste of time polished the patches if they are going to be 
 rejected due to design flaws.

 It is a waste of time for the reviewers to see a patch like this one,
 as:
 - No description of what is inside the patch is provided;
 
 Ok. I should have given more details about the patch. I am sorry for missing 
 it. My kernel tree failed to compile after applying patches from
 
 [1] 
 http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968
 
 I had to generate this patch to compile the code and test it. Most of the 
 fixes refer to Sumit's code and I think he will take care of those bugs.

Ok.

 
 I admit that I focused on other patches. Like DMA extension, exporting in 
 vb2-core and vb2-dma-contig. Sorry for putting so little attention to 
 bugfixing patch.
 
 - changes that should be happening inside the other patches are
   mixed here.
 
 Right. I missed call_memop here.
 

 It is also a waste of your time to submit a patch that will need to be later
 polished, as you'll need to work with the same thing twice.
 
 The problem is that those patches were not intended to be accepted. The were 
 intended for discussion. 

The patch subjects were not marked with RFC. Please prefix the subject with 
something like
git send-email --subject-prefix  PATCH RFC

When submitting such patches.

 The other problem is that there are many people waiting for those patches. 
 The dma-buf was already 
 accepted to the mainline. Me and Sumit are trying to help V4L2 to catch-up. 
 The dma-buf and its support
 in vb2-core seams to change very dynamically. Polishing the patch takes much 
 time. If the dma-buf API 
 changes the design of vb2-core may have to be changed. Therefore time spent 
 on polishing would be lost. 

Ok.

 I am sorry for patch flaws. All of them would be fixed when the design is 
 stabilized.

No problem.

 

 So, please fix your patch workflow. As a general rule, you should
 compile every patch you're applying and fix the breakages on them.

 Also, if you found bugs that need to be fixed and that aren't
 directly related to your current task, those should generate
 their own patches, and submitted in separate, in order to be
 applied sooner upstream and to stable.
 
 I wanted to post the complete set of patches that produce compilable kernel. 
 herefore most important bugs/issues had to be fixed and attached to the 
 patchset.
 Some of the issues in [1] were mentioned by Laurent and Sakari. I hope Sumit 
 will
 take care of those problems.

Ok. My main concern was not with the driver bits, but with:

1) if fixes are needed at the vb2 core, to ensure that they'll go
   upstream earlier;

2) The userspace API changes to properly support for dma buffers.

If you're not ready to discuss (2), that's ok, but I'd like to follow
the discussions for it with care, not only for reviewing the actual
patches, but also since I want to be sure that it will address the
needs for xawtv and for the Xorg v4l driver. 

 Failing to do that will mean that important fixes for upstream
 will be missed.
 
 Ok. It will be fixed.

Thanks!

Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo 

Re: [PATCH 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Tomasz Stanislawski

Hi Mauro,
On 01/23/2012 05:06 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 13:25, Tomasz Stanislawski escreveu:

Hi Mauro,

On 01/23/2012 03:52 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 12:32, Tomasz Stanislawski escreveu:

Hi Mauro,

On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:

Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:

Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com


Please better describe this patch. What is it supposing to fix?


[snip]


It is also a waste of your time to submit a patch that will need to be later
polished, as you'll need to work with the same thing twice.


The problem is that those patches were not intended to be accepted. The were 
intended for discussion.


The patch subjects were not marked with RFC. Please prefix the subject with 
something like
git send-email --subject-prefix  PATCH RFC

When submitting such patches.


Right!!!
Sorry, I forgot about it. Probably, if I had added those 3 letter we 
would have avoided this whole misunderstanding about the purpose of the 
patches :)





The other problem is that there are many people waiting for those patches. The 
dma-buf was already
accepted to the mainline. Me and Sumit are trying to help V4L2 to catch-up. The 
dma-buf and its support
in vb2-core seams to change very dynamically. Polishing the patch takes much 
time. If the dma-buf API
changes the design of vb2-core may have to be changed. Therefore time spent on 
polishing would be lost.


Ok.


I am sorry for patch flaws. All of them would be fixed when the design is 
stabilized.


No problem.





So, please fix your patch workflow. As a general rule, you should
compile every patch you're applying and fix the breakages on them.

Also, if you found bugs that need to be fixed and that aren't
directly related to your current task, those should generate
their own patches, and submitted in separate, in order to be
applied sooner upstream and to stable.


I wanted to post the complete set of patches that produce compilable kernel.
herefore most important bugs/issues had to be fixed and attached to the 
patchset.
Some of the issues in [1] were mentioned by Laurent and Sakari. I hope Sumit 
will
take care of those problems.


Ok. My main concern was not with the driver bits, but with:

1) if fixes are needed at the vb2 core, to ensure that they'll go
upstream earlier;



First, we should select patches not related to DMABUF.

The fixes related to DMABUF could be postponed because they will be 
applied in new version for Sumit's patches.


Next videobuf2-dma-contig.c file is patched.

Fixes to handling of mmap and userptr would go first with all DMABUF 
related features removed. Lack of DMABUF related callbacks will not 
break backward compatibility.


Next, DMABUF support is added to vb2-core.

Finally, that DMABUF exporting/importing patches would be applied.


2) The userspace API changes to properly support for dma buffers.

If you're not ready to discuss (2), that's ok, but I'd like to follow
the discussions for it with care, not only for reviewing the actual
patches, but also since I want to be sure that it will address the
needs for xawtv and for the Xorg v4l driver.



The support of dmabuf could be easily added to framebuffer API.
I expect that it would not be difficult to add it to Xv.
The selection API could be used to control scaling and composing
of video stream into framebuffer or a texture for composing manager.

Regards,
Tomasz Stanislawski


Failing to do that will mean that important fixes for upstream
will be missed.


Ok. It will be fixed.


Thanks!

Mauro



--
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 04/10] v4l: vb2: fixes for DMABUF support

2012-01-23 Thread Mauro Carvalho Chehab
Em 23-01-2012 14:37, Tomasz Stanislawski escreveu:
 Hi Mauro,
 On 01/23/2012 05:06 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 13:25, Tomasz Stanislawski escreveu:
 Hi Mauro,

 On 01/23/2012 03:52 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 12:32, Tomasz Stanislawski escreveu:
 Hi Mauro,

 On 01/23/2012 03:22 PM, Mauro Carvalho Chehab wrote:
 Em 23-01-2012 11:51, Tomasz Stanislawski escreveu:
 Signed-off-by: Tomasz Stanislawskit.stanisl...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com

 Please better describe this patch. What is it supposing to fix?
 
 [snip]
 
 It is also a waste of your time to submit a patch that will need to be 
 later
 polished, as you'll need to work with the same thing twice.

 The problem is that those patches were not intended to be accepted. The 
 were intended for discussion.

 The patch subjects were not marked with RFC. Please prefix the subject with 
 something like
 git send-email --subject-prefix  PATCH RFC

 When submitting such patches.
 
 Right!!!
 Sorry, I forgot about it. Probably, if I had added those 3 letter we would 
 have avoided this whole misunderstanding about the purpose of the patches :)
 

 The other problem is that there are many people waiting for those patches. 
 The dma-buf was already
 accepted to the mainline. Me and Sumit are trying to help V4L2 to catch-up. 
 The dma-buf and its support
 in vb2-core seams to change very dynamically. Polishing the patch takes 
 much time. If the dma-buf API
 changes the design of vb2-core may have to be changed. Therefore time spent 
 on polishing would be lost.

 Ok.

 I am sorry for patch flaws. All of them would be fixed when the design is 
 stabilized.

 No problem.



 So, please fix your patch workflow. As a general rule, you should
 compile every patch you're applying and fix the breakages on them.

 Also, if you found bugs that need to be fixed and that aren't
 directly related to your current task, those should generate
 their own patches, and submitted in separate, in order to be
 applied sooner upstream and to stable.

 I wanted to post the complete set of patches that produce compilable kernel.
 herefore most important bugs/issues had to be fixed and attached to the 
 patchset.
 Some of the issues in [1] were mentioned by Laurent and Sakari. I hope 
 Sumit will
 take care of those problems.

 Ok. My main concern was not with the driver bits, but with:

 1) if fixes are needed at the vb2 core, to ensure that they'll go
 upstream earlier;

 
 First, we should select patches not related to DMABUF.
 
 The fixes related to DMABUF could be postponed because they will be applied 
 in new version for Sumit's patches.
 
 Next videobuf2-dma-contig.c file is patched.
 
 Fixes to handling of mmap and userptr would go first with all DMABUF related 
 features removed. Lack of DMABUF related callbacks will not break backward 
 compatibility.
 
 Next, DMABUF support is added to vb2-core.
 
 Finally, that DMABUF exporting/importing patches would be applied.

Sounds like a plan.

Regards,
Mauro
--
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