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

[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.zabel@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

[PATCH 2/2] [media] videobuf2: add trace events

[Philipp Zabel]

Add videobuf2 specific vb2_qbuf and vb2_dqbuf trace events that mirror the
v4l2_qbuf and v4l2_dqbuf trace events, only they include additional
information about queue fill state and are emitted right before the buffer
is enqueued in the driver or userspace is woken up. This allows to make
sense of the timeline of trace events in combination with others that might
be triggered by __enqueue_in_driver.

Also two new trace events vb2_buf_queue and vb2_buf_done are added,
allowing to trace the handover between videobuf2 framework and driver.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/v4l2-core/videobuf2-core.c | 11 ++++
 include/trace/events/v4l2.h              | 99 ++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 93b3154..b866a6b 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -30,6 +30,8 @@
 #include <media/v4l2-common.h>
 #include <media/videobuf2-core.h>
 
+#include <trace/events/v4l2.h>
+
 static int debug;
 module_param(debug, int, 0644);
 
@@ -1207,6 +1209,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	atomic_dec(&q->owned_by_drv_count);
 	spin_unlock_irqrestore(&q->done_lock, flags);
 
+	trace_vb2_buf_done(q, vb);
+
 	if (state == VB2_BUF_STATE_QUEUED) {
 		if (q->start_streaming_called)
 			__enqueue_in_driver(vb);
@@ -1629,6 +1633,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->owned_by_drv_count);
 
+	trace_vb2_buf_queue(q, vb);
+
 	/* sync buffers */
 	for (plane = 0; plane < vb->num_planes; ++plane)
 		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
@@ -1878,6 +1884,8 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 			vb->v4l2_buf.timecode = b->timecode;
 	}
 
+	trace_vb2_qbuf(q, vb);
+
 	/*
 	 * If already streaming, give the buffer to driver for processing.
 	 * If not, the buffer will be given to driver on next streamon.
@@ -2123,6 +2131,9 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	/* Remove from videobuf queue */
 	list_del(&vb->queued_entry);
 	q->queued_count--;
+
+	trace_vb2_dqbuf(q, vb);
+
 	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
 	    vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST)
 		q->last_buffer_dequeued = true;
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
index 89d0497..3d15cf1 100644
--- a/include/trace/events/v4l2.h
+++ b/include/trace/events/v4l2.h
@@ -175,9 +175,108 @@ SHOW_FIELD
 		)							\
 	)
 
+#define VB2_TRACE_EVENT(event_name)					\
+	TRACE_EVENT(event_name,						\
+		TP_PROTO(struct vb2_queue *q, struct vb2_buffer *vb),	\
+									\
+		TP_ARGS(q, vb),						\
+									\
+		TP_STRUCT__entry(					\
+			__field(int, minor)				\
+			__field(u32, queued_count)			\
+			__field(int, owned_by_drv_count)		\
+			__field(u32, index)				\
+			__field(u32, type)				\
+			__field(u32, bytesused)				\
+			__field(u32, flags)				\
+			__field(u32, field)				\
+			__field(s64, timestamp)				\
+			__field(u32, timecode_type)			\
+			__field(u32, timecode_flags)			\
+			__field(u8, timecode_frames)			\
+			__field(u8, timecode_seconds)			\
+			__field(u8, timecode_minutes)			\
+			__field(u8, timecode_hours)			\
+			__field(u8, timecode_userbits0)			\
+			__field(u8, timecode_userbits1)			\
+			__field(u8, timecode_userbits2)			\
+			__field(u8, timecode_userbits3)			\
+			__field(u32, sequence)				\
+		),							\
+									\
+		TP_fast_assign(						\
+			__entry->minor = q->owner ?			\
+				q->owner->vdev->minor : -1;		\
+			__entry->queued_count = q->queued_count;	\
+			__entry->owned_by_drv_count =			\
+				atomic_read(&q->owned_by_drv_count);	\
+			__entry->index = vb->v4l2_buf.index;		\
+			__entry->type = vb->v4l2_buf.type;		\
+			__entry->bytesused = vb->v4l2_buf.bytesused;	\
+			__entry->flags = vb->v4l2_buf.flags;		\
+			__entry->field = vb->v4l2_buf.field;		\
+			__entry->timestamp =				\
+				timeval_to_ns(&vb->v4l2_buf.timestamp);	\
+			__entry->timecode_type =			\
+				vb->v4l2_buf.timecode.type;		\
+			__entry->timecode_flags =			\
+				vb->v4l2_buf.timecode.flags;		\
+			__entry->timecode_frames =			\
+				vb->v4l2_buf.timecode.frames;		\
+			__entry->timecode_seconds =			\
+				vb->v4l2_buf.timecode.seconds;		\
+			__entry->timecode_minutes =			\
+				vb->v4l2_buf.timecode.minutes;		\
+			__entry->timecode_hours =			\
+				vb->v4l2_buf.timecode.hours;		\
+			__entry->timecode_userbits0 =			\
+				vb->v4l2_buf.timecode.userbits[0];	\
+			__entry->timecode_userbits1 =			\
+				vb->v4l2_buf.timecode.userbits[1];	\
+			__entry->timecode_userbits2 =			\
+				vb->v4l2_buf.timecode.userbits[2];	\
+			__entry->timecode_userbits3 =			\
+				vb->v4l2_buf.timecode.userbits[3];	\
+			__entry->sequence = vb->v4l2_buf.sequence;	\
+		),							\
+									\
+		TP_printk("minor = %d, queued = %u, owned_by_drv = %d "	\
+			  "index = %u, type = %s, "			\
+			  "bytesused = %u, flags = %s, "		\
+			  "field = %s, timestamp = %llu, timecode = { "	\
+			  "type = %s, flags = %s, frames = %u, "	\
+			  "seconds = %u, minutes = %u, hours = %u, "	\
+			  "userbits = { %u %u %u %u } }, "		\
+			  "sequence = %u", __entry->minor,		\
+			  __entry->queued_count,			\
+			  __entry->owned_by_drv_count,			\
+			  __entry->index, show_type(__entry->type),	\
+			  __entry->bytesused,				\
+			  show_flags(__entry->flags),			\
+			  show_field(__entry->field),			\
+			  __entry->timestamp,				\
+			  show_timecode_type(__entry->timecode_type),	\
+			  show_timecode_flags(__entry->timecode_flags),	\
+			  __entry->timecode_frames,			\
+			  __entry->timecode_seconds,			\
+			  __entry->timecode_minutes,			\
+			  __entry->timecode_hours,			\
+			  __entry->timecode_userbits0,			\
+			  __entry->timecode_userbits1,			\
+			  __entry->timecode_userbits2,			\
+			  __entry->timecode_userbits3,			\
+			  __entry->sequence				\
+		)							\
+	)
+
 V4L2_TRACE_EVENT(v4l2_dqbuf);
 V4L2_TRACE_EVENT(v4l2_qbuf);
 
+VB2_TRACE_EVENT(vb2_buf_done);
+VB2_TRACE_EVENT(vb2_buf_queue);
+VB2_TRACE_EVENT(vb2_dqbuf);
+VB2_TRACE_EVENT(vb2_qbuf);
+
 #endif /* if !defined(_TRACE_V4L2_H) || defined(TRACE_HEADER_MULTI_READ) */
 
 /* This part must be outside protection */
-- 
2.1.4

Re: [PATCH 2/2] [media] videobuf2: add trace events

[Steven Rostedt]

On Thu, 25 Jun 2015 12:01:27 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
> index 89d0497..3d15cf1 100644
> --- a/include/trace/events/v4l2.h
> +++ b/include/trace/events/v4l2.h
> @@ -175,9 +175,108 @@ SHOW_FIELD
>  		)							\
>  	)
>  
> +#define VB2_TRACE_EVENT(event_name)					\

This is what we have DECLARE_EVENT_CLASS for. Please use that,
otherwise you are adding about 5k of text per event (where as
DEFINE_EVENT adds only about 250 bytes).

DECLARE_EVENT_CLASS(vb2_event_class,
[..]


> +	TRACE_EVENT(event_name,						\
> +		TP_PROTO(struct vb2_queue *q, struct vb2_buffer *vb),	\
> +									\
> +		TP_ARGS(q, vb),						\
> +									\
> +		TP_STRUCT__entry(					\
> +			__field(int, minor)				\
> +			__field(u32, queued_count)			\
> +			__field(int, owned_by_drv_count)		\
> +			__field(u32, index)				\
> +			__field(u32, type)				\
> +			__field(u32, bytesused)				\
> +			__field(u32, flags)				\
> +			__field(u32, field)				\
> +			__field(s64, timestamp)				\
> +			__field(u32, timecode_type)			\
> +			__field(u32, timecode_flags)			\
> +			__field(u8, timecode_frames)			\
> +			__field(u8, timecode_seconds)			\
> +			__field(u8, timecode_minutes)			\
> +			__field(u8, timecode_hours)			\
> +			__field(u8, timecode_userbits0)			\
> +			__field(u8, timecode_userbits1)			\
> +			__field(u8, timecode_userbits2)			\
> +			__field(u8, timecode_userbits3)			\
> +			__field(u32, sequence)				\
> +		),							\
> +									\
> +		TP_fast_assign(						\
> +			__entry->minor = q->owner ?			\
> +				q->owner->vdev->minor : -1;		\
> +			__entry->queued_count = q->queued_count;	\
> +			__entry->owned_by_drv_count =			\
> +				atomic_read(&q->owned_by_drv_count);	\
> +			__entry->index = vb->v4l2_buf.index;		\
> +			__entry->type = vb->v4l2_buf.type;		\
> +			__entry->bytesused = vb->v4l2_buf.bytesused;	\
> +			__entry->flags = vb->v4l2_buf.flags;		\
> +			__entry->field = vb->v4l2_buf.field;		\
> +			__entry->timestamp =				\
> +				timeval_to_ns(&vb->v4l2_buf.timestamp);	\
> +			__entry->timecode_type =			\
> +				vb->v4l2_buf.timecode.type;		\
> +			__entry->timecode_flags =			\
> +				vb->v4l2_buf.timecode.flags;		\
> +			__entry->timecode_frames =			\
> +				vb->v4l2_buf.timecode.frames;		\
> +			__entry->timecode_seconds =			\
> +				vb->v4l2_buf.timecode.seconds;		\
> +			__entry->timecode_minutes =			\
> +				vb->v4l2_buf.timecode.minutes;		\
> +			__entry->timecode_hours =			\
> +				vb->v4l2_buf.timecode.hours;		\
> +			__entry->timecode_userbits0 =			\
> +				vb->v4l2_buf.timecode.userbits[0];	\
> +			__entry->timecode_userbits1 =			\
> +				vb->v4l2_buf.timecode.userbits[1];	\
> +			__entry->timecode_userbits2 =			\
> +				vb->v4l2_buf.timecode.userbits[2];	\
> +			__entry->timecode_userbits3 =			\
> +				vb->v4l2_buf.timecode.userbits[3];	\
> +			__entry->sequence = vb->v4l2_buf.sequence;	\
> +		),							\
> +									\
> +		TP_printk("minor = %d, queued = %u, owned_by_drv = %d "	\
> +			  "index = %u, type = %s, "			\
> +			  "bytesused = %u, flags = %s, "		\
> +			  "field = %s, timestamp = %llu, timecode = { "	\
> +			  "type = %s, flags = %s, frames = %u, "	\
> +			  "seconds = %u, minutes = %u, hours = %u, "	\
> +			  "userbits = { %u %u %u %u } }, "		\
> +			  "sequence = %u", __entry->minor,		\
> +			  __entry->queued_count,			\
> +			  __entry->owned_by_drv_count,			\
> +			  __entry->index, show_type(__entry->type),	\
> +			  __entry->bytesused,				\
> +			  show_flags(__entry->flags),			\
> +			  show_field(__entry->field),			\
> +			  __entry->timestamp,				\
> +			  show_timecode_type(__entry->timecode_type),	\
> +			  show_timecode_flags(__entry->timecode_flags),	\
> +			  __entry->timecode_frames,			\
> +			  __entry->timecode_seconds,			\
> +			  __entry->timecode_minutes,			\
> +			  __entry->timecode_hours,			\
> +			  __entry->timecode_userbits0,			\
> +			  __entry->timecode_userbits1,			\
> +			  __entry->timecode_userbits2,			\
> +			  __entry->timecode_userbits3,			\
> +			  __entry->sequence				\
> +		)							\
> +	)
> +
>  V4L2_TRACE_EVENT(v4l2_dqbuf);
>  V4L2_TRACE_EVENT(v4l2_qbuf);

While you are at it, nuke the above macro and convert that too.

>  
> +VB2_TRACE_EVENT(vb2_buf_done);
> +VB2_TRACE_EVENT(vb2_buf_queue);
> +VB2_TRACE_EVENT(vb2_dqbuf);
> +VB2_TRACE_EVENT(vb2_qbuf);

DEFINE_EVENT(vb2_event_class, vb2_buf_done, ...)
[..]

-- Steve

> +
>  #endif /* if !defined(_TRACE_V4L2_H) || defined(TRACE_HEADER_MULTI_READ) */
>  
>  /* This part must be outside protection */

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.zabel@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

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.zabel@pengutronix.de> wrote:
> Signed-off-by: Philipp Zabel <p.zabel@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 majordomo@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.zabel@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

Re: [PATCH 2/2] [media] videobuf2: add trace events

[Philipp Zabel]

Am Donnerstag, den 25.06.2015, 09:07 -0400 schrieb Steven Rostedt:
> On Thu, 25 Jun 2015 12:01:27 +0200
> Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
> > index 89d0497..3d15cf1 100644
> > --- a/include/trace/events/v4l2.h
> > +++ b/include/trace/events/v4l2.h
> > @@ -175,9 +175,108 @@ SHOW_FIELD
> >  		)							\
> >  	)
> >  
> > +#define VB2_TRACE_EVENT(event_name)					\
> 
> This is what we have DECLARE_EVENT_CLASS for. Please use that,
> otherwise you are adding about 5k of text per event (where as
> DEFINE_EVENT adds only about 250 bytes).
[...]
> While you are at it, nuke the above macro and convert that too.

Thanks, I'll do that.

regards
Philipp

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.zabel@pengutronix.de> wrote:
> > Signed-off-by: Philipp Zabel <p.zabel@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

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.zabel@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

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

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.