Re: [PATCH 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Sylwester Nawrocki]

Hi Philipp,

On 25/06/15 17:12, Philipp Zabel wrote:
 Am Donnerstag, den 25.06.2015, 15:11 +0200 schrieb Sylwester Nawrocki:
 On 25/06/15 12:01, Philipp Zabel wrote:
 Signed-off-by: Philipp Zabel p.za...@pengutronix.de
 ---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

 diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
 b/drivers/media/v4l2-core/v4l2-mem2mem.c
 index dc853e5..511caaa 100644
 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
 +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
 @@ -357,9 +357,16 @@ int v4l2_m2m_reqbufs(struct file *file, struct 
 v4l2_m2m_ctx *m2m_ctx,
  struct v4l2_requestbuffers *reqbufs)
  {
 struct vb2_queue *vq;
 +   int ret;
  
 vq = v4l2_m2m_get_vq(m2m_ctx, reqbufs-type);
 -   return vb2_reqbufs(vq, reqbufs);
 +   ret = vb2_reqbufs(vq, reqbufs);
 +   /* If count == 0, then the owner has released all buffers and he
 +  is no longer owner of the queue. Otherwise we have a new owner. */
 +   if (ret == 0)
 +   vq-owner = reqbufs-count ? file-private_data : NULL;
 +
 +   return ret;
  }

 How about modifying v4l2_m2m_ioctl_reqbufs() instead ?
 
 The coda, gsc-m2m, m2m-deinterlace, mx2_emmaprp, and sh_veu drivers all
 have their own implementation of vidioc_reqbufs that call
 v4l2_m2m_reqbufs directly.
 Maybe this should be moved into v4l2_m2m_ioctl_reqbufs after all drivers
 are updated to use it instead of v4l2_m2m_reqbufs.

In case of some of the above listed drivers it shouldn't be difficult
and would be nice to convert to the generic v4l2_m2m_ioctl* callbacks.

Anyway, I guess your code change makes sense, just the comment might
be a little bit misleading. vq-owner will always be one and the same
file handle, unless I'm missing something.

 Moreover, does it really makes sense when a new m2m device context
 is being created during each video device open()?
 
 Having the queue owner's device minor in the trace output is very useful
 when tracing a single stream across multiple devices. To discern events
 from multiple simultaneous contexts I have added the context id to the
 coda driver specific trace events.

OK, I understand now, you are just using this stored file handle for traces.

--
Regards,
Sylwester
--
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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Sylwester Nawrocki]

On 26/06/15 11:02, Philipp Zabel wrote:
 Am Freitag, den 26.06.2015, 10:28 +0200 schrieb Sylwester Nawrocki:
 [...]
   How about modifying v4l2_m2m_ioctl_reqbufs() instead ?
   
   The coda, gsc-m2m, m2m-deinterlace, mx2_emmaprp, and sh_veu drivers all
   have their own implementation of vidioc_reqbufs that call
   v4l2_m2m_reqbufs directly.
   Maybe this should be moved into v4l2_m2m_ioctl_reqbufs after all drivers
   are updated to use it instead of v4l2_m2m_reqbufs.
  
  In case of some of the above listed drivers it shouldn't be difficult
  and would be nice to convert to the generic v4l2_m2m_ioctl* callbacks.
  
  Anyway, I guess your code change makes sense, just the comment might
  be a little bit misleading. vq-owner will always be one and the same
  file handle, unless I'm missing something.

 True. Since the m2m_ctx containing the vb2_queue is attached to the file
 handle, this will only ever get called with the same file handle for a
 given queue. s/we have a new owner/we have an owner/ ?

Sounds good enough to me.

--
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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Philipp Zabel]

Am Freitag, den 26.06.2015, 10:28 +0200 schrieb Sylwester Nawrocki:
[...]
  How about modifying v4l2_m2m_ioctl_reqbufs() instead ?
  
  The coda, gsc-m2m, m2m-deinterlace, mx2_emmaprp, and sh_veu drivers all
  have their own implementation of vidioc_reqbufs that call
  v4l2_m2m_reqbufs directly.
  Maybe this should be moved into v4l2_m2m_ioctl_reqbufs after all drivers
  are updated to use it instead of v4l2_m2m_reqbufs.
 
 In case of some of the above listed drivers it shouldn't be difficult
 and would be nice to convert to the generic v4l2_m2m_ioctl* callbacks.
 
 Anyway, I guess your code change makes sense, just the comment might
 be a little bit misleading. vq-owner will always be one and the same
 file handle, unless I'm missing something.

True. Since the m2m_ctx containing the vb2_queue is attached to the file
handle, this will only ever get called with the same file handle for a
given queue. s/we have a new owner/we have an owner/ ?

[...]
  Having the queue owner's device minor in the trace output is very useful
  when tracing a single stream across multiple devices. To discern events
  from multiple simultaneous contexts I have added the context id to the
  coda driver specific trace events.
 
 OK, I understand now, you are just using this stored file handle for traces.

I should mention this in the patch description.

regards
Philipp

--
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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Sylwester Nawrocki]

Hello Philipp,

On 25/06/15 12:01, Philipp Zabel wrote:
 Signed-off-by: Philipp Zabel p.za...@pengutronix.de
 ---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
 b/drivers/media/v4l2-core/v4l2-mem2mem.c
 index dc853e5..511caaa 100644
 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
 +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
 @@ -357,9 +357,16 @@ int v4l2_m2m_reqbufs(struct file *file, struct 
 v4l2_m2m_ctx *m2m_ctx,
struct v4l2_requestbuffers *reqbufs)
  {
   struct vb2_queue *vq;
 + int ret;
  
   vq = v4l2_m2m_get_vq(m2m_ctx, reqbufs-type);
 - return vb2_reqbufs(vq, reqbufs);
 + ret = vb2_reqbufs(vq, reqbufs);
 + /* If count == 0, then the owner has released all buffers and he
 +is no longer owner of the queue. Otherwise we have a new owner. */
 + if (ret == 0)
 + vq-owner = reqbufs-count ? file-private_data : NULL;
 +
 + return ret;
  }

How about modifying v4l2_m2m_ioctl_reqbufs() instead ?

Moreover, does it really makes sense when a new m2m device context
is being created during each video device open()?

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

[PATCH 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Philipp Zabel]

Signed-off-by: Philipp Zabel p.za...@pengutronix.de
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dc853e5..511caaa 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -357,9 +357,16 @@ int v4l2_m2m_reqbufs(struct file *file, struct 
v4l2_m2m_ctx *m2m_ctx,
 struct v4l2_requestbuffers *reqbufs)
 {
struct vb2_queue *vq;
+   int ret;
 
vq = v4l2_m2m_get_vq(m2m_ctx, reqbufs-type);
-   return vb2_reqbufs(vq, reqbufs);
+   ret = vb2_reqbufs(vq, reqbufs);
+   /* If count == 0, then the owner has released all buffers and he
+  is no longer owner of the queue. Otherwise we have a new owner. */
+   if (ret == 0)
+   vq-owner = reqbufs-count ? file-private_data : NULL;
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_reqbufs);
 
-- 
2.1.4

--
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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Kamil Debski]

Hi Philipp,

On 25 June 2015 at 11:01, Philipp Zabel p.za...@pengutronix.de wrote:
 Signed-off-by: Philipp Zabel p.za...@pengutronix.de

Please add the patch description no matter how simple it is and how
well the subject covers the content of the patch.

Best wishes,
Kamil

 ---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

 diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
 b/drivers/media/v4l2-core/v4l2-mem2mem.c
 index dc853e5..511caaa 100644
 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
 +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
 @@ -357,9 +357,16 @@ int v4l2_m2m_reqbufs(struct file *file, struct 
 v4l2_m2m_ctx *m2m_ctx,
  struct v4l2_requestbuffers *reqbufs)
  {
 struct vb2_queue *vq;
 +   int ret;

 vq = v4l2_m2m_get_vq(m2m_ctx, reqbufs-type);
 -   return vb2_reqbufs(vq, reqbufs);
 +   ret = vb2_reqbufs(vq, reqbufs);
 +   /* If count == 0, then the owner has released all buffers and he
 +  is no longer owner of the queue. Otherwise we have a new owner. */
 +   if (ret == 0)
 +   vq-owner = reqbufs-count ? file-private_data : NULL;
 +
 +   return ret;
  }
  EXPORT_SYMBOL_GPL(v4l2_m2m_reqbufs);

 --
 2.1.4

 --
 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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Philipp Zabel]

Hi Sylwester,

Am Donnerstag, den 25.06.2015, 15:11 +0200 schrieb Sylwester Nawrocki:
 Hello Philipp,
 
 On 25/06/15 12:01, Philipp Zabel wrote:
  Signed-off-by: Philipp Zabel p.za...@pengutronix.de
  ---
   drivers/media/v4l2-core/v4l2-mem2mem.c | 9 -
   1 file changed, 8 insertions(+), 1 deletion(-)
  
  diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
  b/drivers/media/v4l2-core/v4l2-mem2mem.c
  index dc853e5..511caaa 100644
  --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
  +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
  @@ -357,9 +357,16 @@ int v4l2_m2m_reqbufs(struct file *file, struct 
  v4l2_m2m_ctx *m2m_ctx,
   struct v4l2_requestbuffers *reqbufs)
   {
  struct vb2_queue *vq;
  +   int ret;
   
  vq = v4l2_m2m_get_vq(m2m_ctx, reqbufs-type);
  -   return vb2_reqbufs(vq, reqbufs);
  +   ret = vb2_reqbufs(vq, reqbufs);
  +   /* If count == 0, then the owner has released all buffers and he
  +  is no longer owner of the queue. Otherwise we have a new owner. */
  +   if (ret == 0)
  +   vq-owner = reqbufs-count ? file-private_data : NULL;
  +
  +   return ret;
   }
 
 How about modifying v4l2_m2m_ioctl_reqbufs() instead ?

The coda, gsc-m2m, m2m-deinterlace, mx2_emmaprp, and sh_veu drivers all
have their own implementation of vidioc_reqbufs that call
v4l2_m2m_reqbufs directly.
Maybe this should be moved into v4l2_m2m_ioctl_reqbufs after all drivers
are updated to use it instead of v4l2_m2m_reqbufs.

 Moreover, does it really makes sense when a new m2m device context
 is being created during each video device open()?

Having the queue owner's device minor in the trace output is very useful
when tracing a single stream across multiple devices. To discern events
from multiple simultaneous contexts I have added the context id to the
coda driver specific trace events.

regards
Philipp


--
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 1/2] [media] v4l2-mem2mem: set the queue owner field just as vb2_ioctl_reqbufs does

[Philipp Zabel]

Hi Kamil,

Am Donnerstag, den 25.06.2015, 15:10 +0100 schrieb Kamil Debski:
 Hi Philipp,
 
 On 25 June 2015 at 11:01, Philipp Zabel p.za...@pengutronix.de wrote:
  Signed-off-by: Philipp Zabel p.za...@pengutronix.de
 
 Please add the patch description no matter how simple it is and how
 well the subject covers the content of the patch.

Will do in the next round.

thanks
Philipp

--
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