[PATCH 0/5] s5p-fimc: Misc fixes

[Nicolas Dufresne]

This patch series fixes several bugs found in the s5p-fimc driver. These
bugs relate to bad parameters in the formats definition and short size
of image buffers.

Nicolas Dufresne (5):
  s5p-fimc: Reuse calculated sizes
  s5p-fimc: Iterate for each memory plane
  s5p-fimc: Align imagesize to row size for tiled formats
  s5p-fimc: Fix YUV422P depth
  s5p-fimc: Changed RGB32 to BGR32

 drivers/media/platform/exynos4-is/fimc-core.c | 21 +++--
 drivers/media/platform/exynos4-is/fimc-m2m.c  |  6 +++---
 2 files changed, 18 insertions(+), 9 deletions(-)



signature.asc
Description: This is a digitally signed message part

[PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

Testing showed that HW produces BGR32 rather then RGB32 as exposed
in the driver. The documentation seems to state the pixels are stored
in little endian order.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c
b/drivers/media/platform/exynos4-is/fimc-core.c
index da2fc86..bfb80fb 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -56,8 +56,8 @@ static struct fimc_fmt fimc_formats[] = {
.colplanes  = 1,
.flags  = FMT_FLAGS_M2M,
}, {
-   .name   = ARGB, 32 bpp,
-   .fourcc = V4L2_PIX_FMT_RGB32,
+   .name   = BGRB888, 32 bpp,
+   .fourcc = V4L2_PIX_FMT_BGR32,
.depth  = { 32 },
.color  = FIMC_FMT_RGB888,
.memplanes  = 1,
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 2/5] s5p-fimc: Fix YUV422P depth

[Nicolas Dufresne]

All YUV 422 has 16bit per pixels.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c
b/drivers/media/platform/exynos4-is/fimc-core.c
index bfb80fb..2e70fab 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -122,7 +122,7 @@ static struct fimc_fmt fimc_formats[] = {
}, {
.name   = YUV 4:2:2 planar, Y/Cb/Cr,
.fourcc = V4L2_PIX_FMT_YUV422P,
-   .depth  = { 12 },
+   .depth  = { 16 },
.color  = FIMC_FMT_YCBYCR422,
.memplanes  = 1,
.colplanes  = 3,
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 3/5] s5p-fimc: Align imagesize to row size for tiled formats

[Nicolas Dufresne]

For tiled format, we need to allocated a multiple of the row size. A
good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
the previous code would only have enough space to fit half of the last
row.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index 2e70fab..0e94412 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
width, u32 height,
for (i = 0; i  pix-num_planes; ++i) {
struct v4l2_plane_pix_format *plane_fmt = pix-plane_fmt[i];
u32 bpl = plane_fmt-bytesperline;
+   u32 sizeimage;
 
if (fmt-colplanes  1  (bpl == 0 || bpl  pix-width))
bpl = pix-width; /* Planar */
@@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
width, u32 height,
bytesperline /= 2;
 
plane_fmt-bytesperline = bytesperline;
-   plane_fmt-sizeimage = max((pix-width * pix-height *
-  fmt-depth[i]) / 8, plane_fmt-sizeimage);
+   sizeimage = pix-width * pix-height * fmt-depth[i] / 8;
+
+   /* Ensure full last row for tiled formats */
+   if (tiled_fmt(fmt)) {
+   /* 64 * 32 * plane_fmt-bytesperline / 64 */
+   u32 row_size = plane_fmt-bytesperline * 32;
+   sizeimage = ALIGN(sizeimage, row_size);
+   }
+
+   plane_fmt-sizeimage = max(sizeimage, plane_fmt-sizeimage);
}
 }
 
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 5/5] s5p-fimc: Reuse calculated sizes

[Nicolas Dufresne]

This formula did not take into account the required tiled alignement for
NV12MT format. As this was already computed an stored in payload array
initially, reuse that value.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c 
b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 07b8f97..c648e5f 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -197,7 +197,7 @@ static int fimc_queue_setup(struct vb2_queue *vq, const 
struct v4l2_format *fmt,
 
*num_planes = f-fmt-memplanes;
for (i = 0; i  f-fmt-memplanes; i++) {
-   sizes[i] = (f-f_width * f-f_height * f-fmt-depth[i]) / 8;
+   sizes[i] = f-payload[i];
allocators[i] = ctx-fimc_dev-alloc_ctx;
}
return 0;
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

Testing showed that HW produces BGR32 rather then RGB32 as exposed
in the driver. The documentation seems to state the pixels are stored
in little endian order.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index da2fc86..bfb80fb 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -56,8 +56,8 @@ static struct fimc_fmt fimc_formats[] = {
.colplanes  = 1,
.flags  = FMT_FLAGS_M2M,
}, {
-   .name   = ARGB, 32 bpp,
-   .fourcc = V4L2_PIX_FMT_RGB32,
+   .name   = BGRB888, 32 bpp,
+   .fourcc = V4L2_PIX_FMT_BGR32,
.depth  = { 32 },
.color  = FIMC_FMT_RGB888,
.memplanes  = 1,
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 4/5] s5p-fimc: Iterate for each memory plane

[Nicolas Dufresne]

Depth and payload is defined per memory plane. It's better to iterate using
number of memory planes. This was not causing much issue since the rest
of the arrays involved where intialized to zero.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 drivers/media/platform/exynos4-is/fimc-m2m.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index 0e94412..ab9450b 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -450,7 +450,7 @@ void fimc_prepare_dma_offset(struct fimc_ctx *ctx, struct 
fimc_frame *f)
bool pix_hoff = ctx-fimc_dev-drv_data-dma_pix_hoff;
u32 i, depth = 0;
 
-   for (i = 0; i  f-fmt-colplanes; i++)
+   for (i = 0; i  f-fmt-memplanes; i++)
depth += f-fmt-depth[i];
 
f-dma_offset.y_h = f-offs_h;
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c 
b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 36971d9..07b8f97 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -342,7 +342,7 @@ static void __set_frame_format(struct fimc_frame *frame, 
struct fimc_fmt *fmt,
 {
int i;
 
-   for (i = 0; i  fmt-colplanes; i++) {
+   for (i = 0; i  fmt-memplanes; i++) {
frame-bytesperline[i] = pixm-plane_fmt[i].bytesperline;
frame-payload[i] = pixm-plane_fmt[i].sizeimage;
}
@@ -461,7 +461,7 @@ static int fimc_m2m_try_crop(struct fimc_ctx *ctx, struct 
v4l2_crop *cr)
else
halign = ffs(fimc-variant-min_vsize_align) - 1;
 
-   for (i = 0; i  f-fmt-colplanes; i++)
+   for (i = 0; i  f-fmt-memplanes; i++)
depth += f-fmt-depth[i];
 
v4l_bound_align_image(cr-c.width, min_size, f-o_width,
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

[PATCH 2/5] s5p-fimc: Fix YUV422P depth

[Nicolas Dufresne]

All YUV 422 has 16bit per pixels.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index bfb80fb..2e70fab 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -122,7 +122,7 @@ static struct fimc_fmt fimc_formats[] = {
}, {
.name   = YUV 4:2:2 planar, Y/Cb/Cr,
.fourcc = V4L2_PIX_FMT_YUV422P,
-   .depth  = { 12 },
+   .depth  = { 16 },
.color  = FIMC_FMT_YCBYCR422,
.memplanes  = 1,
.colplanes  = 3,
-- 
1.8.5.3




signature.asc
Description: This is a digitally signed message part

Re: [PATCH 3/5] s5p-fimc: Align imagesize to row size for tiled formats

[Nicolas Dufresne]

Le mardi 25 mars 2014 à 16:51 -0400, Nicolas Dufresne a écrit :
 For tiled format, we need to allocated a multiple of the row size. A
 good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
 this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
 the previous code would only have enough space to fit half of the last
 row.
 
 Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
 ---
  drivers/media/platform/exynos4-is/fimc-core.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
 b/drivers/media/platform/exynos4-is/fimc-core.c
 index 2e70fab..0e94412 100644
 --- a/drivers/media/platform/exynos4-is/fimc-core.c
 +++ b/drivers/media/platform/exynos4-is/fimc-core.c
 @@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
 width, u32 height,
   for (i = 0; i  pix-num_planes; ++i) {
   struct v4l2_plane_pix_format *plane_fmt = pix-plane_fmt[i];
   u32 bpl = plane_fmt-bytesperline;
 + u32 sizeimage;
  
   if (fmt-colplanes  1  (bpl == 0 || bpl  pix-width))
   bpl = pix-width; /* Planar */
 @@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
 width, u32 height,
   bytesperline /= 2;
  
   plane_fmt-bytesperline = bytesperline;
 - plane_fmt-sizeimage = max((pix-width * pix-height *
 -fmt-depth[i]) / 8, plane_fmt-sizeimage);
 + sizeimage = pix-width * pix-height * fmt-depth[i] / 8;
 +
 + /* Ensure full last row for tiled formats */
 + if (tiled_fmt(fmt)) {
 + /* 64 * 32 * plane_fmt-bytesperline / 64 */
 + u32 row_size = plane_fmt-bytesperline * 32;
 + sizeimage = ALIGN(sizeimage, row_size);

I made a mistake here, and it seems I've badly tested it too (was
setting the size from the test application). ALIGN won't work as
row_size is not a power of two. Sorry for that, will send an update.
roundup(sizeimage, row_size) would be the way to go. Will resubmit.

 + }
 +
 + plane_fmt-sizeimage = max(sizeimage, plane_fmt-sizeimage);
   }
  }
  



signature.asc
Description: This is a digitally signed message part

[PATCH 3/5] s5p-fimc: Roundup imagesize to row size for tiled formats

[Nicolas Dufresne]

For tiled format, we need to allocated a multiple of the row size. A
good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
the previous code would only have enough space to fit half of the last
row.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/exynos4-is/fimc-core.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c 
b/drivers/media/platform/exynos4-is/fimc-core.c
index 2e70fab..7ea15ef 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
width, u32 height,
for (i = 0; i  pix-num_planes; ++i) {
struct v4l2_plane_pix_format *plane_fmt = pix-plane_fmt[i];
u32 bpl = plane_fmt-bytesperline;
+   u32 sizeimage;
 
if (fmt-colplanes  1  (bpl == 0 || bpl  pix-width))
bpl = pix-width; /* Planar */
@@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 
width, u32 height,
bytesperline /= 2;
 
plane_fmt-bytesperline = bytesperline;
-   plane_fmt-sizeimage = max((pix-width * pix-height *
-  fmt-depth[i]) / 8, plane_fmt-sizeimage);
+   sizeimage = pix-width * pix-height * fmt-depth[i] / 8;
+
+   /* Ensure full last row for tiled formats */
+   if (tiled_fmt(fmt)) {
+   /* 64 * 32 * plane_fmt-bytesperline / 64 */
+   u32 row_size = plane_fmt-bytesperline * 32;
+   sizeimage = roundup(sizeimage, row_size);
+   }
+
+   plane_fmt-sizeimage = max(sizeimage, plane_fmt-sizeimage);
}
 }
 
-- 
1.8.5.3


signature.asc
Description: This is a digitally signed message part

Re: [PATCH 0/5] s5p-fimc: Misc fixes

[Nicolas Dufresne]

Any comment/input ?

Le mardi 25 mars 2014 à 16:45 -0400, Nicolas Dufresne a écrit :
 This patch series fixes several bugs found in the s5p-fimc driver. These
 bugs relate to bad parameters in the formats definition and short size
 of image buffers.
 
 Nicolas Dufresne (5):
   s5p-fimc: Reuse calculated sizes
   s5p-fimc: Iterate for each memory plane
   s5p-fimc: Align imagesize to row size for tiled formats
   s5p-fimc: Fix YUV422P depth
   s5p-fimc: Changed RGB32 to BGR32
 
  drivers/media/platform/exynos4-is/fimc-core.c | 21 +++--
  drivers/media/platform/exynos4-is/fimc-m2m.c  |  6 +++---
  2 files changed, 18 insertions(+), 9 deletions(-)
 



signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

Le mardi 01 avril 2014 à 16:34 +0200, Sylwester Nawrocki a écrit :
 It should be BGRA, 32 bpp, I can fix it when applying, if
 you won't send next version of this patch until then.

Good catch, I'll let you fix when applying.

thanks,
Nicolas



signature.asc
Description: This is a digitally signed message part

[PATCH] vb2: Update buffer state flags after __vb2_dqbuf

[Nicolas Dufresne]

Previously we where updating the buffer state using __fill_v4l2_buffer
before the state transition was completed through __vb2_dqbuf. This
would cause the V4L2_BUF_FLAG_DONE to be set, which would mean it still
queued. The spec says the dqbuf should clean the DONE flag, right not it
alway set it.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/v4l2-core/videobuf2-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
b/drivers/media/v4l2-core/videobuf2-core.c
index f9059bb..ac5026a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1943,14 +1943,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, 
struct v4l2_buffer *b, bool n
 
call_vb_qop(vb, buf_finish, vb);
 
-   /* Fill buffer information for the userspace */
-   __fill_v4l2_buffer(vb, b);
/* Remove from videobuf queue */
list_del(vb-queued_entry);
q-queued_count--;
/* go back to dequeued state */
__vb2_dqbuf(vb);
 
+   /* Fill buffer information for the userspace */
+   __fill_v4l2_buffer(vb, b);
+
dprintk(1, dqbuf of buffer %d, with state %d\n,
vb-v4l2_buf.index, vb-state);
 
-- 
1.9.0




signature.asc
Description: This is a digitally signed message part

Poll and empty queues

[Nicolas Dufresne]

Hi everyone,

Recently in GStreamer we notice that we where not handling the POLLERR
flag at all. Though we found that what the code do, and what the doc
says is slightly ambiguous.

When the application did not call VIDIOC_QBUF or
VIDIOC_STREAMON yet the poll() function succeeds, but sets the
POLLERR flag in the revents field.

In our case, we first seen this error with a capture device. How things
worked is that we first en-queue all allocated buffers. Our
interpretation was that this would avoid not calling VIDIOC_QBUF [...]
yet, and only then we would call VIDIOC_STREAMON. This way, in our
interpretation we would never get that error.

Though, this is not what the code does. Looking at videobuf2, if simply
return this error when the queue is empty.

/*
 * There is nothing to wait for if no buffers have already been queued.
 */
if (list_empty(q-queued_list))
return res | POLLERR;

So basically, we endup in this situation where as soon as all existing
buffers has been dequeued, we can't rely on the driver to wait for a
buffer to be queued and then filled again. This basically forces us into
adding a new user-space mechanism, to wait for buffer to come back. We
are wandering if this is a bug. If not, maybe it would be nice to
improve the documentation.

cheers,
Nicolas


signature.asc
Description: This is a digitally signed message part

Re: Poll and empty queues

[Nicolas Dufresne]

Le mardi 03 juin 2014 à 08:38 +0200, Hans Verkuil a écrit :
 Hi Nicholas,
 
 On 06/02/2014 09:47 PM, Nicolas Dufresne wrote:
  Hi everyone,
  
  Recently in GStreamer we notice that we where not handling the POLLERR
  flag at all. Though we found that what the code do, and what the doc
  says is slightly ambiguous.
  
  When the application did not call VIDIOC_QBUF or
  VIDIOC_STREAMON yet the poll() function succeeds, but sets the
  POLLERR flag in the revents field.
  
  In our case, we first seen this error with a capture device. How things
  worked is that we first en-queue all allocated buffers. Our
  interpretation was that this would avoid not calling VIDIOC_QBUF [...]
  yet, and only then we would call VIDIOC_STREAMON. This way, in our
  interpretation we would never get that error.
  
  Though, this is not what the code does. Looking at videobuf2, if simply
  return this error when the queue is empty.
  
  /*
   * There is nothing to wait for if no buffers have already been queued.
   */
  if (list_empty(q-queued_list))
  return res | POLLERR;
  
  So basically, we endup in this situation where as soon as all existing
  buffers has been dequeued, we can't rely on the driver to wait for a
  buffer to be queued and then filled again. This basically forces us into
  adding a new user-space mechanism, to wait for buffer to come back. We
  are wandering if this is a bug. If not, maybe it would be nice to
  improve the documentation.
 
 Just for my understanding: I assume that gstreamer polls in one process or
 thread and does the queuing/dequeuing in a different process/thread, is that
 correct?

Yes, in this particular case (polling with queues/thread downstream),
the streaming thread do the polling, and then push the buffers. The
buffer reach a queue element, which will queued and return. Polling
restart at this point. The queue will later pass it downstream from the
next streaming thread, and eventually the buffer will be released. For
capture device, QBUF will be called upon release.

It is assumed that this call to QBUF should take a finite amount of
time. Though, libv4l2 makes this assumption wrong by inter-locking DQBUF
and QBUF, clearly a bug, but not strictly related to this thread. Also,
as we try and avoid blocking in the DQBUF ioctl, it should not be a
problem for us.

 
 If it was all in one process, then it would make no sense polling for a
 buffer to become available if you never queued one.

Not exactly true, the driver may take some time before the buffer we
have queued back is filled and available again. The poll/select FD set
also have a control pipe, so we can stop the process at any moment. Not
polling would mean blocking on an ioctl() which cannot be canceled.

But, without downstream queues (thread), the size of the queue will be
negotiated so that the buffer will be released before we go back
polling. The queue will never be empty in this case.

 
 That is probably the reasoning behind what poll does today. That said, I've
 always thought it was wrong and that it should be replaced by something like:
 
   if (!vb2_is_streaming(q))
   return res | POLLERR;
 
 I.e.: only return an error if we're not streaming.

I think it would be easier for user space and closer to what the doc
says. Though, it's not just about changing that check, there is some
more work involved from what I've seen.

cheers,
Nicolas


signature.asc
Description: This is a digitally signed message part

Re: Poll and empty queues

[Nicolas Dufresne]

Le mardi 03 juin 2014 à 18:11 +0200, Laurent Pinchart a écrit :
 Hi Nicolas,
 
 On Tuesday 03 June 2014 10:37:50 Nicolas Dufresne wrote:
  Le mardi 03 juin 2014 à 08:38 +0200, Hans Verkuil a écrit :
   On 06/02/2014 09:47 PM, Nicolas Dufresne wrote:
Hi everyone,

Recently in GStreamer we notice that we where not handling the POLLERR
flag at all. Though we found that what the code do, and what the doc
says is slightly ambiguous.

When the application did not call VIDIOC_QBUF or
VIDIOC_STREAMON yet the poll() function succeeds, but sets the
POLLERR flag in the revents field.

In our case, we first seen this error with a capture device. How things
worked is that we first en-queue all allocated buffers. Our
interpretation was that this would avoid not calling VIDIOC_QBUF [...]
yet, and only then we would call VIDIOC_STREAMON. This way, in our
interpretation we would never get that error.

Though, this is not what the code does. Looking at videobuf2, if simply
return this error when the queue is empty.

/*
 * There is nothing to wait for if no buffers have already been queued.
 */
if (list_empty(q-queued_list))
return res | POLLERR;

So basically, we endup in this situation where as soon as all existing
buffers has been dequeued, we can't rely on the driver to wait for a
buffer to be queued and then filled again. This basically forces us into
adding a new user-space mechanism, to wait for buffer to come back. We
are wandering if this is a bug. If not, maybe it would be nice to
improve the documentation.
   
   Just for my understanding: I assume that gstreamer polls in one process or
   thread and does the queuing/dequeuing in a different process/thread, is
   that correct?
  
  Yes, in this particular case (polling with queues/thread downstream),
  the streaming thread do the polling, and then push the buffers. The
  buffer reach a queue element, which will queued and return. Polling
  restart at this point. The queue will later pass it downstream from the
  next streaming thread, and eventually the buffer will be released. For
  capture device, QBUF will be called upon release.
  
  It is assumed that this call to QBUF should take a finite amount of
  time. Though, libv4l2 makes this assumption wrong by inter-locking DQBUF
  and QBUF, clearly a bug, but not strictly related to this thread. Also,
  as we try and avoid blocking in the DQBUF ioctl, it should not be a
  problem for us.
  
   If it was all in one process, then it would make no sense polling for a
   buffer to become available if you never queued one.
  
  Not exactly true, the driver may take some time before the buffer we
  have queued back is filled and available again. The poll/select FD set
  also have a control pipe, so we can stop the process at any moment. Not
  polling would mean blocking on an ioctl() which cannot be canceled.
  
  But, without downstream queues (thread), the size of the queue will be
  negotiated so that the buffer will be released before we go back
  polling. The queue will never be empty in this case.
  
   That is probably the reasoning behind what poll does today. That said,
   I've always thought it was wrong and that it should be replaced by
   something like:
  
 if (!vb2_is_streaming(q))
 return res | POLLERR;
   
   I.e.: only return an error if we're not streaming.
  
  I think it would be easier for user space and closer to what the doc says.
 
 I tend to agree, and I'd like to raise a different but related issue.
 
 I've recently run into a problem with a V4L2 device (OMAP4 ISS if you want 
 details) that sometimes crashes during video capture. When this occurs the 
 device is rendered completely unusable, and userspace need to stop the video 
 stream and close the video device node in order to reset the device. That's 
 not ideal, but until I pinpoint the root cause that's what we have to live 
 with.
 
 When the OMAP4 ISS driver detects the error it immediately completes all 
 queued buffers with the V4L2_BUF_FLAG_ERROR flag set, and returns -EIO from 
 all subsequent VIDIOC_QBUF calls. The next few VIDIOC_DQBUF calls will return 
 buffers with the V4L2_BUF_FLAG_ERROR flag set, after which the next 
 VIDIOC_DQBUF call will block in __vb2_wait_for_done_vb() on
 
 ret = wait_event_interruptible(q-done_wq,
 !list_empty(q-done_list) || !q-streaming);
 
 as the queue is still streaming and the done list stays empty.
 
 (Disclaimer : I'm using gstreamer 0.10 for this project due to TI shipping 
 the 
 OMAP4 H.264 codec for this version only)

Nod, nothing I can help with. This is a very similar problem to
out-of-tree kernel drivers. We need to teach vendors to upstream in
gst-plugins-bad, otherwise it becomes un-maintain.

  
 As gstreamer doesn't handle POLLERR in v4l2src the gst_poll_wait() call

Re: [PATCH v2 06/29] [media] coda: Add encoder/decoder support for CODA960

[Nicolas Dufresne]

Le mardi 24 juin 2014 à 16:55 +0200, Philipp Zabel a écrit :
 This patch adds support for the CODA960 VPU in Freescale i.MX6 SoCs.
 
 It enables h.264 and MPEG4 encoding and decoding support. Besides the usual
 register shifting, the CODA960 gains frame memory control and GDI registers
 that are set up for linear mapping right now, needs ENC_PIC_SRC_INDEX to be
 set beyond the number of internal buffers for some reason, and has subsampling
 buffers that need to be set up. Also, the work buffer size is increased to
 80 KiB.
 
 The CODA960 firmware spins if there is not enough input data in the bitstream
 buffer. To make it continue, buffers need to be copied into the bitstream as
 soon as they are queued. As the bitstream fifo is written into from two 
 places,
 it must be protected with a mutex. For that, using a threaded interrupt 
 handler
 is necessary.
 
 Signed-off-by: Philipp Zabel p.za...@pengutronix.de
 ---
  drivers/media/platform/coda.c | 397 
 +-
  drivers/media/platform/coda.h | 115 +++-
  2 files changed, 464 insertions(+), 48 deletions(-)
 
 diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
 index 50051fe..2da7e29 100644
 --- a/drivers/media/platform/coda.c
 +++ b/drivers/media/platform/coda.c
 @@ -44,19 +44,24 @@
  #define CODA_FMO_BUF_SIZE32
  #define CODADX6_WORK_BUF_SIZE(288 * 1024 + CODA_FMO_BUF_SIZE * 8 * 
 1024)
  #define CODA7_WORK_BUF_SIZE  (128 * 1024)
 +#define CODA9_WORK_BUF_SIZE  (80 * 1024)
  #define CODA7_TEMP_BUF_SIZE  (304 * 1024)
 +#define CODA9_TEMP_BUF_SIZE  (204 * 1024)
  #define CODA_PARA_BUF_SIZE   (10 * 1024)
  #define CODA_ISRAM_SIZE  (2048 * 2)
  #define CODADX6_IRAM_SIZE0xb000
  #define CODA7_IRAM_SIZE  0x14000
 +#define CODA9_IRAM_SIZE  0x21000
  
  #define CODA7_PS_BUF_SIZE0x28000
 +#define CODA9_PS_SAVE_SIZE   (512 * 1024)
  
  #define CODA_MAX_FRAMEBUFFERS8
  
  #define CODA_MAX_FRAME_SIZE  0x10
  #define FMO_SLICE_SAVE_BUF_SIZE (32)
  #define CODA_DEFAULT_GAMMA   4096
 +#define CODA9_DEFAULT_GAMMA  24576   /* 0.75 * 32768 */
  
  #define MIN_W 176
  #define MIN_H 144
 @@ -84,6 +89,7 @@ enum coda_inst_type {
  enum coda_product {
   CODA_DX6 = 0xf001,
   CODA_7541 = 0xf012,
 + CODA_960 = 0xf020,
  };
  
  struct coda_fmt {
 @@ -177,6 +183,16 @@ struct coda_iram_info {
   phys_addr_t next_paddr;
  };
  
 +struct gdi_tiled_map {
 + int xy2ca_map[16];
 + int xy2ba_map[16];
 + int xy2ra_map[16];
 + int rbc2axi_map[32];
 + int xy2rbc_config;
 + int map_type;
 +#define GDI_LINEAR_FRAME_MAP 0
 +};
 +
  struct coda_ctx {
   struct coda_dev *dev;
   struct mutexbuffer_mutex;
 @@ -215,8 +231,10 @@ struct coda_ctx {
   int idx;
   int reg_idx;
   struct coda_iram_info   iram_info;
 + struct gdi_tiled_maptiled_map;
   u32 bit_stream_param;
   u32 frm_dis_flg;
 + u32 frame_mem_ctrl;
   int display_idx;
  };
  
 @@ -265,15 +283,23 @@ static void coda_command_async(struct coda_ctx *ctx, 
 int cmd)
  {
   struct coda_dev *dev = ctx-dev;
  
 - if (dev-devtype-product == CODA_7541) {
 + if (dev-devtype-product == CODA_960 ||
 + dev-devtype-product == CODA_7541) {
   /* Restore context related registers to CODA */
   coda_write(dev, ctx-bit_stream_param,
   CODA_REG_BIT_BIT_STREAM_PARAM);
   coda_write(dev, ctx-frm_dis_flg,
   CODA_REG_BIT_FRM_DIS_FLG(ctx-reg_idx));
 + coda_write(dev, ctx-frame_mem_ctrl,
 + CODA_REG_BIT_FRAME_MEM_CTRL);
   coda_write(dev, ctx-workbuf.paddr, CODA_REG_BIT_WORK_BUF_ADDR);
   }
  
 + if (dev-devtype-product == CODA_960) {
 + coda_write(dev, 1, CODA9_GDI_WPROT_ERR_CLR);
 + coda_write(dev, 0, CODA9_GDI_WPROT_RGN_EN);
 + }
 +
   coda_write(dev, CODA_REG_BIT_BUSY_FLAG, CODA_REG_BIT_BUSY);
  
   coda_write(dev, ctx-idx, CODA_REG_BIT_RUN_INDEX);
 @@ -349,6 +375,13 @@ static struct coda_codec coda7_codecs[] = {
   CODA_CODEC(CODA7_MODE_DECODE_MP4,  V4L2_PIX_FMT_MPEG4,  
 V4L2_PIX_FMT_YUV420, 1920, 1080),
  };
  
 +static struct coda_codec coda9_codecs[] = {
 + CODA_CODEC(CODA9_MODE_ENCODE_H264, V4L2_PIX_FMT_YUV420, 
 V4L2_PIX_FMT_H264,   1920, 1080),
 + CODA_CODEC(CODA9_MODE_ENCODE_MP4,  V4L2_PIX_FMT_YUV420, 
 V4L2_PIX_FMT_MPEG4,  1920, 1080),
 + CODA_CODEC(CODA9_MODE_DECODE_H264, V4L2_PIX_FMT_H264,   
 V4L2_PIX_FMT_YUV420, 1920, 1080),
 + CODA_CODEC(CODA9_MODE_DECODE_MP4,  V4L2_PIX_FMT_MPEG4,  
 V4L2_PIX_FMT_YUV420, 1920, 1080),
 +};
 +
  static bool coda_format_is_yuv(u32 

Re: [PATCH 06/30] [media] coda: Add encoder/decoder support for CODA960

[Nicolas Dufresne]

Le vendredi 13 juin 2014 à 18:08 +0200, Philipp Zabel a écrit :
 This patch adds support for the CODA960 VPU in Freescale i.MX6 SoCs.

I might be confused, but is this driver sharing the same device node for
the encoder and the decoder ? If so why ? I know the spec might not be
preventing it, but I don't know how in userspace I'm supposed to figure
the type of m2m node this is ? Other drivers have decided to split
encoding, decoding and transformation into their own node, which made it
easier to use generically.

 
 It enables h.264 and MPEG4 encoding and decoding support. Besides the usual
 register shifting, the CODA960 gains frame memory control and GDI registers
 that are set up for linear mapping right now, needs ENC_PIC_SRC_INDEX to be
 set beyond the number of internal buffers for some reason, and has subsampling
 buffers that need to be set up. Also, the work buffer size is increased to
 80 KiB.
 
 The CODA960 firmware spins if there is not enough input data in the bitstream
 buffer. To make it continue, buffers need to be copied into the bitstream as
 soon as they are queued. As the bitstream fifo is written into from two 
 places,
 it must be protected with a mutex. For that, using a threaded interrupt 
 handler
 is necessary.
 
 Signed-off-by: Philipp Zabel p.za...@pengutronix.de
 ---
  drivers/media/platform/coda.c | 397 
 +-
  drivers/media/platform/coda.h | 115 +++-
  2 files changed, 464 insertions(+), 48 deletions(-)
 
 diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
 index 2b27998..10cc031 100644
 --- a/drivers/media/platform/coda.c
 +++ b/drivers/media/platform/coda.c
 @@ -44,19 +44,24 @@
  #define CODA_FMO_BUF_SIZE32
  #define CODADX6_WORK_BUF_SIZE(288 * 1024 + CODA_FMO_BUF_SIZE * 8 * 
 1024)
  #define CODA7_WORK_BUF_SIZE  (128 * 1024)
 +#define CODA9_WORK_BUF_SIZE  (80 * 1024)
  #define CODA7_TEMP_BUF_SIZE  (304 * 1024)
 +#define CODA9_TEMP_BUF_SIZE  (204 * 1024)
  #define CODA_PARA_BUF_SIZE   (10 * 1024)
  #define CODA_ISRAM_SIZE  (2048 * 2)
  #define CODADX6_IRAM_SIZE0xb000
  #define CODA7_IRAM_SIZE  0x14000
 +#define CODA9_IRAM_SIZE  0x21000
  
  #define CODA7_PS_BUF_SIZE0x28000
 +#define CODA9_PS_SAVE_SIZE   (512 * 1024)
  
  #define CODA_MAX_FRAMEBUFFERS8
  
  #define CODA_MAX_FRAME_SIZE  0x10
  #define FMO_SLICE_SAVE_BUF_SIZE (32)
  #define CODA_DEFAULT_GAMMA   4096
 +#define CODA9_DEFAULT_GAMMA  24576   /* 0.75 * 32768 */
  
  #define MIN_W 176
  #define MIN_H 144
 @@ -84,6 +89,7 @@ enum coda_inst_type {
  enum coda_product {
   CODA_DX6 = 0xf001,
   CODA_7541 = 0xf012,
 + CODA_960 = 0xf020,
  };
  
  struct coda_fmt {
 @@ -177,6 +183,16 @@ struct coda_iram_info {
   phys_addr_t next_paddr;
  };
  
 +struct gdi_tiled_map {
 + int xy2ca_map[16];
 + int xy2ba_map[16];
 + int xy2ra_map[16];
 + int rbc2axi_map[32];
 + int xy2rbc_config;
 + int map_type;
 +#define GDI_LINEAR_FRAME_MAP 0
 +};
 +
  struct coda_ctx {
   struct coda_dev *dev;
   struct mutexbuffer_mutex;
 @@ -215,8 +231,10 @@ struct coda_ctx {
   int idx;
   int reg_idx;
   struct coda_iram_info   iram_info;
 + struct gdi_tiled_maptiled_map;
   u32 bit_stream_param;
   u32 frm_dis_flg;
 + u32 frame_mem_ctrl;
   int display_idx;
  };
  
 @@ -265,15 +283,23 @@ static void coda_command_async(struct coda_ctx *ctx, 
 int cmd)
  {
   struct coda_dev *dev = ctx-dev;
  
 - if (dev-devtype-product == CODA_7541) {
 + if (dev-devtype-product == CODA_960 ||
 + dev-devtype-product == CODA_7541) {
   /* Restore context related registers to CODA */
   coda_write(dev, ctx-bit_stream_param,
   CODA_REG_BIT_BIT_STREAM_PARAM);
   coda_write(dev, ctx-frm_dis_flg,
   CODA_REG_BIT_FRM_DIS_FLG(ctx-reg_idx));
 + coda_write(dev, ctx-frame_mem_ctrl,
 + CODA_REG_BIT_FRAME_MEM_CTRL);
   coda_write(dev, ctx-workbuf.paddr, CODA_REG_BIT_WORK_BUF_ADDR);
   }
  
 + if (dev-devtype-product == CODA_960) {
 + coda_write(dev, 1, CODA9_GDI_WPROT_ERR_CLR);
 + coda_write(dev, 0, CODA9_GDI_WPROT_RGN_EN);
 + }
 +
   coda_write(dev, CODA_REG_BIT_BUSY_FLAG, CODA_REG_BIT_BUSY);
  
   coda_write(dev, ctx-idx, CODA_REG_BIT_RUN_INDEX);
 @@ -349,6 +375,13 @@ static struct coda_codec coda7_codecs[] = {
   CODA_CODEC(CODA7_MODE_DECODE_MP4,  V4L2_PIX_FMT_MPEG4,  
 V4L2_PIX_FMT_YUV420, 1920, 1080),
  };
  
 +static struct coda_codec coda9_codecs[] = {
 + CODA_CODEC(CODA9_MODE_ENCODE_H264, 

UVC driver produce decreasing timestamp

[Nicolas Dufresne]

Hello,

I've been trying to solve some timestamp issues with v4l2 in GStreamer
and tracked down the problem to be the uvc driver giving back
non-monotonic (sometimes decreasing) timestamp. Here's some traces I
have captured. It shows the TS as computed by uvc_video_clock_update().
Notice that before this call the TS are monotonic. This method is far
from trivial, so any help would be appreciated.

[21967.375107] uvcvideo: Built-in iSight: SOF 3140.137924 y 1435759049 ts 
21967.778560 buf ts 21967.338765 (x1 177340416/146/661 x2 179437568/178/692 y1 
10 y2 1032119492)
[21967.426928] uvcvideo: Built-in iSight: SOF 3140.136291 y 1382052758 ts 
21967.776903 buf ts 21967.390830 (x1 180748288/198/712 x2 182845440/230/743 y1 
10 y2 1031993005)
[21967.475141] uvcvideo: Built-in iSight: SOF 3140.137893 y 1332485681 ts 
21967.779230 buf ts 21967.438749 (x1 184156160/250/762 x2 185991168/278/793 y1 
10 y2 1028199123)
[21967.526930] uvcvideo: Built-in iSight: SOF 3140.137878 y 1282023868 ts 
21967.776877 buf ts 21967.490838 (x1 187301888/298/812 x2 189399040/330/843 y1 
10 y2 1031987069)
[21967.566957] uvcvideo: Built-in iSight: SOF 3140.136322 y 1238515894 ts 
21967.777348 buf ts 21967.530792 (x1 190185472/342/854 x2 192020480/370/885 y1 
10 y2 1028044630)
[21967.610926] uvcvideo: Built-in iSight: SOF 3140.136337 y 1198142764 ts 
21967.776997 buf ts 21967.574767 (x1 192806912/382/896 x2 194904064/414/927 y1 
10 y2 1032001038)
[21967.650930] uvcvideo: Built-in iSight: SOF 3140.137817 y 1158528823 ts 
21967.777309 buf ts 21967.614754 (x1 195428352/422/937 x2 197525504/454/968 y1 
10 y2 1032079122)
[21967.694920] uvcvideo: Built-in iSight: SOF 3140.139266 y 1114141225 ts 
21967.776985 buf ts 21967.658756 (x1 198311936/466/979 x2 200409088/498/1010 y1 
10 y2 1032000549)
[21967.734945] uvcvideo: Built-in iSight: SOF 3140.139221 y 1074227386 ts 
21967.777055 buf ts 21967.698774 (x1 200933376/506/1021 x2 203030528/538/1052 
y1 10 y2 1032038054)
[21967.778916] uvcvideo: Built-in iSight: SOF 3140.137802 y 1030136608 ts 
21967.776983 buf ts 21967.738775 (x1 203816960/550/1063 x2 205914112/582/1094 
y1 10 y2 1031998733)
[21967.818956] uvcvideo: Built-in iSight: SOF 3140.136413 y 990110109 ts 
21967.776897 buf ts 21967.782775 (x1 206438400/590/1105 x2 208535552/622/1136 
y1 10 y2 1032085338)
[21967.862985] uvcvideo: Built-in iSight: SOF 3140.137756 y 946020732 ts 
21967.776868 buf ts 21967.822761 (x1 209321984/634/1146 x2 211156992/662/1177 
y1 10 y2 1028060834)
[21967.902940] uvcvideo: Built-in iSight: SOF 3140.137741 y 906078361 ts 
21967.776911 buf ts 21967.866805 (x1 211943424/674/1188 x2 214040576/706/1219 
y1 10 y2 1032020244)
[21967.942962] uvcvideo: Built-in iSight: SOF 3140.137725 y 861471274 ts 
21967.776231 buf ts 21967.906780 (x1 214827008/718/1230 x2 216662016/746/1261 
y1 10 y2 1028135357)
[21967.986951] uvcvideo: Built-in iSight: SOF 3140.138961 y 821880474 ts 
21967.776661 buf ts 21967.950779 (x1 217448448/758/1272 x2 219545600/790/1303 
y1 10 y2 1032046506)
[21968.026956] uvcvideo: Built-in iSight: SOF 3140.138931 y 44906 ts 
21967.776585 buf ts 21967.990787 (x1 220332032/802/1314 x2 222167040/830/1345 
y1 10 y2 1028049728)
[21968.070933] uvcvideo: Built-in iSight: SOF 3140.137695 y 738230942 ts 
21967.777101 buf ts 21968.034766 (x1 222953472/842/1356 x2 225050624/874/1387 
y1 10 y2 1031988605)
[21968.110982] uvcvideo: Built-in iSight: SOF 3140.136535 y 698058220 ts 
21967.776936 buf ts 21968.074822 (x1 225574912/882/1397 x2 227672064/914/1428 
y1 10 y2 1032008302)
[21968.154936] uvcvideo: Built-in iSight: SOF 3140.136535 y 654680157 ts 
21967.777540 buf ts 21968.118769 (x1 228458496/926/1439 x2 230555648/958/1470 
y1 10 y2 1031949703)
[21968.194962] uvcvideo: Built-in iSight: SOF 3140.136550 y 613663361 ts 
21967.776502 buf ts 21968.158787 (x1 231079936/966/1481 x2 233177088/998/1512 
y1 10 y2 1032039242)

cheers,
Nicolas

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

Re: [RFC PATCH] Docbook/media: improve data_offset/bytesused documentation

[Nicolas Dufresne]

Le lundi 21 juillet 2014 à 15:16 +0200, Hans Verkuil a écrit :
 + Note that data_offset is included in 
 structfieldbytesused/structfield.
 + So the size of the image in the plane is
 + 
 structfieldbytesused/structfield-structfielddata_offset/structfield at
 + offset structfielddata_offset/structfield from the start of 
 the plane.

This seem like messing applications a lot. Let's say you have a well
known format, NV12, but your driver add some customer header at the
beginning. Pretty much all the application in the world would work just
fine ignoring that header, but in fact most of them will not work,
because bytesused is including the header. Considering this wasn't
documented before, I would strongly suggest to keep the bytesused as
being the size for the format know by everyone.


--
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] [media] s5p-mfc: Don't allocate codec buffers on STREAMON.

[Nicolas Dufresne]

Le lundi 19 mai 2014 à 18:03 +0530, Arun Kumar K a écrit :
 From: Pawel Osciak posc...@chromium.org
 
 Currently, we allocate private codec buffers on STREAMON, which may fail
 if we are out of memory. We don't check for failure though, which will
 make us crash with the codec accessing random memory.
 
 We shouldn't be failing STREAMON with out of memory errors though. So move
 the allocation of private codec buffers to REQBUFS for OUTPUT queue. Also,
 move MFC instance opening and closing to REQBUFS as well, as it's tied to
 allocation and deallocation of private codec buffers.
 
 Signed-off-by: Pawel Osciak posc...@chromium.org
 Signed-off-by: Arun Kumar K arun...@samsung.com
 ---
  drivers/media/platform/s5p-mfc/s5p_mfc.c  |8 +++
  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |1 +
  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c  |   30 
 +++--
  3 files changed, 18 insertions(+), 21 deletions(-)
 
 diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
 b/drivers/media/platform/s5p-mfc/s5p_mfc.c
 index 861087c..70f728f 100644
 --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
 +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
 @@ -643,6 +643,7 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
  
   case S5P_MFC_R2H_CMD_CLOSE_INSTANCE_RET:
   clear_work_bit(ctx);
 + ctx-inst_no = MFC_NO_INSTANCE_SET;
   ctx-state = MFCINST_FREE;
   wake_up(ctx-queue);
   goto irq_cleanup_hw;
 @@ -763,7 +764,7 @@ static int s5p_mfc_open(struct file *file)
   goto err_bad_node;
   }
   ctx-fh.ctrl_handler = ctx-ctrl_handler;
 - ctx-inst_no = -1;
 + ctx-inst_no = MFC_NO_INSTANCE_SET;
   /* Load firmware if this is the first instance */
   if (dev-num_inst == 1) {
   dev-watchdog_timer.expires = jiffies +
 @@ -873,12 +874,11 @@ static int s5p_mfc_release(struct file *file)
   vb2_queue_release(ctx-vq_dst);
   /* Mark context as idle */
   clear_work_bit_irqsave(ctx);
 - /* If instance was initialised then
 + /* If instance was initialised and not yet freed,
* return instance and free resources */
 - if (ctx-inst_no != MFC_NO_INSTANCE_SET) {
 + if (ctx-state != MFCINST_FREE  ctx-state != MFCINST_INIT) {
   mfc_debug(2, Has to free instance\n);
   s5p_mfc_close_mfc_inst(dev, ctx);
 - ctx-inst_no = MFC_NO_INSTANCE_SET;
   }
   /* hardware locking scheme */
   if (dev-curr_ctx == ctx-num)
 diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c 
 b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
 index 6f6e50a..6c3f8f7 100644
 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
 +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
 @@ -459,5 +459,6 @@ void s5p_mfc_close_mfc_inst(struct s5p_mfc_dev *dev, 
 struct s5p_mfc_ctx *ctx)
   if (ctx-type == MFCINST_DECODER)
   s5p_mfc_hw_call(dev-mfc_ops, release_dec_desc_buffer, ctx);
  
 + ctx-inst_no = MFC_NO_INSTANCE_SET;
   ctx-state = MFCINST_FREE;
  }
 diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c 
 b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
 index 995cee2..a4e6668 100644
 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
 +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
 @@ -475,11 +475,11 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, 
 struct s5p_mfc_ctx *ctx,
   ret = vb2_reqbufs(ctx-vq_src, reqbufs);
   if (ret)
   goto out;
 + s5p_mfc_close_mfc_inst(dev, ctx);

This so far seems to prevent us from probing memory type support. We
Initially call reqbufs(count = 0) for this, but this calls seems to
triggers a firmware error later if we do so. Any advise ?

   ctx-src_bufs_cnt = 0;
 + ctx-output_state = QUEUE_FREE;
   } else if (ctx-output_state == QUEUE_FREE) {
 - /* Can only request buffers after the instance
 -  * has been opened.
 -  */
 + /* Can only request buffers when we have a valid format set. */
   WARN_ON(ctx-src_bufs_cnt != 0);
   if (ctx-state != MFCINST_INIT) {
   mfc_err(Reqbufs called in an invalid state\n);
 @@ -493,6 +493,13 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, 
 struct s5p_mfc_ctx *ctx,
   if (ret)
   goto out;
  
 + ret = s5p_mfc_open_mfc_inst(dev, ctx);
 + if (ret) {
 + reqbufs-count = 0;
 + vb2_reqbufs(ctx-vq_src, reqbufs);
 + goto out;
 + }
 +
   ctx-output_state = QUEUE_BUFS_REQUESTED;
   } else {
   mfc_err(Buffers have already been requested\n);
 @@ -594,7 +601,7 @@ static int vidioc_querybuf(struct file *file, void *priv,
   return -EINVAL;
   }
   mfc_debug(2, State: %d, buf-type: %d\n, ctx-state, buf-type);
 - 

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Nicolas Dufresne]

 
Le Samedi 26 Juillet 2014 12:37 EDT, Philipp Zabel philipp.za...@gmail.com a 
écrit: 

 I have tried the GStreamer v4l2videodec element with the coda driver and
 noticed that GStreamer calls VIDIOC_CROPCAP to obtain the pixel aspect
 ratio. This always fails with -EINVAL because of this issue. Currently 
 GStreamer
 throws a warning if the return value is an error other than -ENOTTY.

But for now, this seems like a fair thing to do. We currently assume that if 
your
driver is not implementing CROPCAP, then pixel aspect ratio at output will be
unchanged at capture. If there is an error though, it's not good sign, and we 
report
it. If that is wrong, let us know why and how to detect your driver error isn't 
a an error.

cheers,
Nicolas 
 
 
 

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

Re: [PATCH 2/3] [media] coda: fix coda_g_selection

[Nicolas Dufresne]

Le dimanche 27 juillet 2014 à 20:21 +0200, Hans Verkuil a écrit :
 If cropcap returns -EINVAL then that means that the current input or
 output does
 not support cropping (for input) or composing (for output). In that case the
 pixel aspect ratio is undefined and you have no way to get hold of that 
 information,
 which is a bug in the V4L2 API.
 
 In the case of an m2m device you can safely assume that whatever the pixel 
 aspect
 is of the image you give to the m2m device, it will still be the same pixel
 aspect when you get it back. In fact, I would say that if an m2m device 
 returns
 cropcap information, then the pixel aspect ratio information is most likely 
 not
 applicable to the device and will typically be 1:1.
 
 Pixel aspect ratio is only relevant if the video comes in or goes out to a 
 physical
 interface (sensor, video receiver/transmitter).

So far not applicable has been interpreted as not implemented /
ENOTTY. Can't CODA just do that and we can close this subject ?

Nicolas


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

Bug: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

Hi Kamil,

after a discussion on IRC, we concluded that s5p-mfc have this bug that 
disallow multiple reqbufs calls before streaming. This has the impact 
that it forces to call REQBUFS(0) before setting the new number of 
buffers during re-negotiation, and is against the spec too.


As an example, in reqbufs_output() REQBUFS is only allowed in QUEUE_FREE 
state, and setting buffers exits this state. We think that the call to 
http://lxr.free-electrons.com/ident?i=reqbufs_outputs5p_mfc_open_mfc_inst() 
should be post-poned until STREAMON is called. 
http://lxr.free-electrons.com/ident?i=reqbufs_output


cheers,
Nicolas
http://lxr.free-electrons.com/ident?i=reqbufs_output
--
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: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

Le 2014-09-01 05:43, Kamil Debski a écrit :

Hi Nicolas,



From: Nicolas Dufresne [mailto:nicolas.dufre...@collabora.com]
Sent: Friday, August 29, 2014 3:47 PM

Hi Kamil,

after a discussion on IRC, we concluded that s5p-mfc have this bug that
disallow multiple reqbufs calls before streaming. This has the impact
that it forces to call REQBUFS(0) before setting the new number of
buffers during re-negotiation, and is against the spec too.

I was out of office last week. Could you shed more light on this subject?
Do you have the irc log?


Sorry I didn't record this one, but feel free to ping Hans V.

As an example, in reqbufs_output() REQBUFS is only allowed in
QUEUE_FREE state, and setting buffers exits this state. We think that
the call to
http://lxr.free-
electrons.com/ident?i=reqbufs_outputs5p_mfc_open_mfc_inst()
should be post-poned until STREAMON is called.
http://lxr.free-electrons.com/ident?i=reqbufs_output

How is this connected to the renegotiation scenario?
Are you sure you wanted to mention s5p_mfc_open_mfc_inst?
This limitation in MFC causes an important conflict between old videobuf 
and new videobuf2 drivers. Old videobuf driver (before Hans G. proposed 
patch) refuse REQBUFS(0). As MFC code has this bug that refuse 
REBBUFS(N) if buffers are already allocated, it makes all this 
completely incompatible. This open_mfc call seems to be the only reason 
REQBUFS() cannot be called multiple time, though I must say you are 
better placed then me to figure this out.


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

Re: s5p-mfc should allow multiple call to REQBUFS before we start streaming

[Nicolas Dufresne]

Le 2014-09-02 05:02, Kamil Debski a écrit

This limitation in MFC causes an important conflict between old
videobuf and new videobuf2 drivers. Old videobuf driver (before Hans G.
proposed
patch) refuse REQBUFS(0). As MFC code has this bug that refuse
REBBUFS(N) if buffers are already allocated, it makes all this
completely incompatible. This open_mfc call seems to be the only reason
REQBUFS() cannot be called multiple time, though I must say you are
better placed then me to figure this out.

After MFCs decoding is initialized it has a fixed number of buffers.
Changing
this can be done when the stream changes its properties and resolution
change is initiated. Even then all buffers have to be
unmapped/reallocated/mapped.

There is a single hardware call that is a part of hardware initialization
that sets the buffers' addresses.

Changing the number of buffers during decoding without explicit reason to do
so (resolution change/stream properties change) would be problematic. For
hardware it is very close to reinitiating decoding - it needs to be done on
an I-frame, the header needs to be present. Otherwise some buffers would be
lost and corruption would be introduced (lack of reference frames).

I think you mentioned negotiating the number of buffers? Did you use the
V4L2_CID_MIN_BUFFERS_FOR_CAPTURE control?
That is true, though the issue isn't there. Let's say you haven't 
started decoding yet. You do REQBUFS(2). because the hardware need that. 
Then  before you start anything else, the topology of your display path 
is change, and the application need an extra 2 buffers to work properly. 
With all other drivers, all we'd have to do is REQBUFS(5), with MFC we 
would need to do REQBUFS(0) and then REQBUFS(5). This is a bug, there is 
no reason to force this extra step.


I understand that before calling REQBUFS(N) with the new N value you
properly
unmap the buffers just like the documentation is telling?

As describe in the scenario, nothing has been mapped yet.


Applications can call VIDIOC_REQBUFS again to change the number of buffers,
however this cannot succeed when any buffers are still mapped. A count value
of zero frees all buffers, after aborting or finishing any DMA in progress,
an implicit VIDIOC_STREAMOFF. [1]
As you can see, the spec says it can be changed, having this extra step 
to change it does not seems compliant to me, at least it's not consistent.



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

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Nicolas Dufresne]

Le 2014-09-15 08:49, Laurent Pinchart a écrit :

Reverting the patch will also be a regression, as that would break
applications that now rely on the new behaviour (I've developed this patch to
fix a problem I've noticed with gstreamer). One way or another, we're screwed
and we'll break userspace.


We have worked around this issue in GStreamer 1.4+, for older version, 
the problem may be faced again by users, specially if using a newer 
libv4l2 where the locking has been fixed (or no libv4l2).


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

Re: [PATCH/RFC v2 1/2] v4l: vb2: Don't return POLLERR during transient buffer underruns

[Nicolas Dufresne]

Le 2014-09-15 09:55, Mauro Carvalho Chehab a écrit :

The DQBUF locking fixup was merged on libv4l2 for version 1.2. So, the
potential breakage happens when libv4l2 is 1.2 and Gstreamer versions
before 1.4.

Do you have any procedure on gstreamer to fix a bug on stable releases?
A backport is possible, even though not trivial. Also, due to resources 
there is no guaranty of a new stable 1.2 release. For GStreamer 0.10, it 
is no longer maintained since 2 years already (mentioning since Laurent 
was using an old vendor specific version base on this). Though this 
situation isn't different from using a vendor specific kernel.

Those VBI applications don't have any, as they're not actively
maintained anymore. Even if we patch them today, I guess it could take
a long time for those changes to be propagated on distros.

So, I guess that the best is to try to fix Gstreamer on the distros
that are using libv4l version 1.2 and a pre-1.4 Gstreamer version.
This seems an unlikely mix, as Gst 1.4 was released at around the same 
moment as libv4l2 1.2.


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

Regression: in v4l2 converter does not set the buffer.length anymore

[Nicolas Dufresne]

This was initially reported to GStreamer project:
https://bugzilla.gnome.org/show_bug.cgi?id=737521

We track this down to be a regression introduced in v4l2-utils from 
version 1.4.0. In recent GStreamer we make sure the buffer.length field 
(retreived with QUERYBUF) is bigger or equal to the expected sizeimage 
(as obtained in S_FMT). This is to fail cleanly and avoid buffer 
overflow if a driver (or libv4l2) endup doing a short allocation. Since 
1.4.0, this field is always 0 if an emulated format is selected.


Reverting patch 10213c brings back normal behaviour:
http://git.linuxtv.org/cgit.cgi/v4l-utils.git/commit/?id=10213c975afdfcc90aa7de39e66c40cd7e8a57f7

This currently makes use of any emulated format impossible in GStreamer. 
v4l2-utils 1.4.0 is being shipped at least in debian/unstable at the moment.


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

Re: uvcvideo fails on 3.16 and 3.17 kernels

[Nicolas Dufresne]

Le 2014-09-30 04:50, Paulo Assis a écrit :

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1362358

I've run some tests and after increasing verbosity for uvcvideo, I get:
EOF on empty payload

this seems consistent with the zero size frames returned by the driver.
After VIDIOC_DQBUF | VIDIOC_QBUF, I get buf.bytesused=0

Testing with an eye toy 2 (gspca), everything works fine, so this is
definitly related to uvcvideo.
This happens on all available formats (YUYV and MJPEG)
Might be related to the libv4l2 bug I reported yesterday, are you using 
libv4l2 in these tests ?


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

Re: [PATCH 00/10] CODA7 JPEG support

[Nicolas Dufresne]

Le 2014-09-30 10:34, Kamil Debski a écrit :

I agree with you Hans. MFC has a single encoder node that supports multiple
codecs and I think this design works well.

JPEG should be separated into separate device.
Having combined encoders and combines decoders works well from 
application / gstreamer point of view too. It's only combine encoder and 
decoder that causes issues with our ability to probe what the HW is 
capable (without a need to know about the platform). Exynos has split 
JPEG decoder because it's not the same HW backing it.


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

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-08 06:24, Kamil Debski a écrit :

Hi,

This patch seems complicated and I do not understand your motives.

Could you explain what is the problem with the current aligning of the
values?
Is this a hardware problem? Which MFC version does it affect?
Is it a software problem? If so, maybe the user space application should
take extra care on what value it passes/receives to try_fmt?
This looks like something I wanted to bring here as an RFC but never 
manage to get the time. In an Odroid Integration we have started using 
the following simple patch to work around this:


https://github.com/dsd/linux-odroid/commit/c76b38c1d682b9870ea3b00093ad6500a9c5f5f6

The context is that right now we have decided that alignment in s_fmt 
shall be done with a closest rounding. So the format returned may be 
bigger, or smaller, that's basically random. I've been digging through a 
lot, and so far I have found no rational that explains this choice other 
that this felt right.


In real life, whenever the resulting format is smaller then request, 
there is little we can do other then fail or try again blindly other 
sizes. But with bigger raw buffers, we can use zero-copy  cropping 
techniques to keep going. Here's a example:


image_generator - hw_converter - display

As hw_converter is a V4L2 M2M, an ideal use case here would be for 
image_generator to use buffers from the hw_converter. For the scenario, 
it is likely that a fixed video size is wanted, but this size is also 
likely not to match HW requirement. If hw_converter decide to give back 
something smaller, there is nothing image_generator can do. It would 
have to try again with random size to find out that best match. It's a 
bit silly to force that on application, as the hw_converter know the 
closest best match, which is simply the next valid bigger size if that 
exist.


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

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-09 06:06, Kamil Debski a écrit :

I think the documentation does not specify how TRY_FMT/S_FMT should adjust
the parameters. Maybe it would a good idea to add some flagS that determine
the behaviour?
A flag could be a good option, maybe we should take a minute and discuss 
this next week.


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

Re: [PATCH v2 14/14] [media] s5p-mfc: Don't change the image size to smaller than the request.

[Nicolas Dufresne]

Le 2014-10-21 07:34, Hans Verkuil a écrit :


I think we should add flags here as well. NEAREST (the default), 
ROUND_DOWN and
ROUND_UP. Existing calls will use NEAREST. I can think of use-cases 
for all
three of these, and I think the caller should just have to specify 
what is

needed.

Just replacing the algorithm used seems asking for problems, you want 
to be
able to select what you want to do. 


One more thing, we realize that in selection scenario, we do want 
nearest or lowest, so indeed a flag that let user space choose is the best.


Nicolas

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

Re: Synching app to live tv

[Nicolas Dufresne]

Please, when cross-posting, make sure to make it really evident, 
otherwise it's a bit of an abuse of people free time.


cheers,
Nicolas

p.s. same message was posted on both GStreamer and linux-media mailing lists

Le 2014-11-01 13:03, Evol Johnson a écrit :

Hi

I'm an app developer and no experience with linuxtv.  I want to build an app 
capable of synch with live tv.

What i figured so far is that i could have a linux computer with a capture 
card,  analyze  the MPEG-2 stream, somehow obtain the PCR(?) and i would be 
able to broadcast the timestamp and maybe other data that to my app.

Questions/comments:
Is this a good approach
This has done before and you only need to download _XYz software__ 
(free or $$)
Not done before, but you here a few resources that should get you started.
I use ubuntu and card recommendations are welcomed.

Thanks in advance


Mark Johnson--
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: Using the coda driver with Gstreamer

[Nicolas Dufresne]

Note, I'm only commenting about the GStreamer side...

Le 2014-11-17 12:29, Fabio Estevam a écrit :
 Hi,

 I am running linux-next 20141117 on a mx6qsabresd board and trying to
 play a mp4 video via Gstreamer 1.4.1, but I am getting the following
 error:
You should update to latest stable version, this is a general rule. Not
keeping track of stable branches is never a good idea. Current stable is
1.4.4.

Note that as of today, there is known issue with supporting this driver
(not that driver itself, I really mean supporting the driver features):

- Need to fix EOS Handling,
https://bugzilla.gnome.org/show_bug.cgi?id=733864
- Output size negotiation https://bugzilla.gnome.org/show_bug.cgi?id=733828
- Output pixel format negotiation
https://bugzilla.gnome.org/show_bug.cgi?id=733827
- Encoding is not yet supported
https://bugzilla.gnome.org/show_bug.cgi?id=728438

First one is the main blocker, but kernel folks will be able to clarify
more. Help is of course appreciated. Note that some of the issues kind
of lack a specification to accomplished. We recently had a meeting in
Dusseldorf to propose solutions.


 root@imx6qsabresd:/mnt/nfs# gst-play-1.0 sample.mp4
 Volume: 100%
 Now playing /mnt/nfs/sample.mp4
 [  506.983809] [ cut here ]
 [  506.988522] WARNING: CPU: 0 PID: 954 at
 drivers/media/v4l2-core/videobuf2-core.c:1781
 vb2_start_streaming+0xc4/0x160()
 [  506.999301] Modules linked in:
 [  507.002489] CPU: 0 PID: 954 Comm: multiqueue0:src Tainted: G
 W  3.18.0-rc4-next-20141117-dirty #2044
 [  507.012660] Backtrace:
 [  507.015253] [80011f44] (dump_backtrace) from [800120e0]
 (show_stack+0x18/0x1c)
 [  507.022891]  r6:06f5 r5: r4: r3:
 [  507.028707] [800120c8] (show_stack) from [806b730c]
 (dump_stack+0x88/0xa4)
 [  507.035954] [806b7284] (dump_stack) from [8002a4dc]
 (warn_slowpath_common+0x80/0xbc)
 [  507.044135]  r5:804a80a8 r4:
 [  507.047802] [8002a45c] (warn_slowpath_common) from [8002a53c]
 (warn_slowpath_null+0x24/0x2c)
 [  507.056605]  r8: r7:bd71c640 r6:bd614ef0 r5:bd614ee0 r4:ffea
 [  507.063470] [8002a518] (warn_slowpath_null) from [804a80a8]
 (vb2_start_streaming+0xc4/0x160)
 [  507.072293] [804a7fe4] (vb2_start_streaming) from [804a9efc]
 (vb2_internal_streamon+0xfc/0x158)
 [  507.081385]  r7:bd71c640 r6:bd6c29ec r5:bd614c00 r4:bd614de0
 [  507.087133] [804a9e00] (vb2_internal_streamon) from [804ab0a8]
 (vb2_streamon+0x34/0x58)
 [  507.095567]  r5:bd614c00 r4:0002
 [  507.099231] [804ab074] (vb2_streamon) from [804a3b10]
 (v4l2_m2m_streamon+0x28/0x40)
 [  507.107287] [804a3ae8] (v4l2_m2m_streamon) from [804a3b40]
 (v4l2_m2m_ioctl_streamon+0x18/0x1c)
 [  507.116292]  r5:bd9083c8 r4:40045612
 [  507.120016] [804a3b28] (v4l2_m2m_ioctl_streamon) from
 [80492e48] (v4l_streamon+0x20/0x24)
 [  507.128693] [80492e28] (v4l_streamon) from [80494dc4]
 (__video_do_ioctl+0x24c/0x2e0)
 [  507.136826] [80494b78] (__video_do_ioctl) from [804953a8]
 (video_usercopy+0x118/0x480)
 [  507.145133]  r10:0001 r9:bd6cbe10 r8:74a1164c r7:
 r6: r5:80494b78
 [  507.153073]  r4:40045612
 [  507.155632] [80495290] (video_usercopy) from [80495724]
 (video_ioctl2+0x14/0x1c)
 [  507.163408]  r10:bd8fccb8 r9:74a1164c r8:bd909064 r7:74a1164c
 r6:40045612 r5:bd71c640
 [  507.171343]  r4:bd9083c8
 [  507.173902] [80495710] (video_ioctl2) from [804918f8]
 (v4l2_ioctl+0x104/0x14c)
 [  507.181512] [804917f4] (v4l2_ioctl) from [800fc944]
 (do_vfs_ioctl+0x80/0x634)
 [  507.189019]  r8:0009 r7:74a1164c r6:0009 r5:800fcf34
 r4:bd71c640 r3:804917f4
 [  507.196870] [800fc8c4] (do_vfs_ioctl) from [800fcf34]
 (SyS_ioctl+0x3c/0x60)
 [  507.204203]  r10: r9:bd6ca000 r8:0009 r7:74a1164c
 r6:40045612 r5:bd71c640
 [  507.212159]  r4:bd71c641
 [  507.214722] [800fcef8] (SyS_ioctl) from [8000ec60]
 (ret_fast_syscall+0x0/0x48)
 [  507.222311]  r8:8000ee24 r7:0036 r6:73c183a0 r5:754248e0
 r4: r3:
 [  507.230168] ---[ end trace c3703a604edaf0d0 ]---
Looks like a backtrace of a warning, though I'm not sure I get what this
warning is about. Maybe you are missing some information, or one need to
look at videobuf2-core.c:178.
 ERROR Failed to connect to X display server for file:///mnt/nfs/sample.mp4
You have built glimagesink (hence libgstgl, part of gst-plugins-bad)
against X11 but you don't have a X11 display running, or DISPLAY
environment isn't set properly.
 ERROR debug information:
 /code/yocto/dizzy/build/tmp/work/cortexa9hf-vfp-neon-mx6qdl-poky-linux-gnueabi/gstreamer1.0-plugins-bad/1.4.1-r0/gst-plugins-bad-1.4.1/ext/gl/gstglimagesink.c(453):
 _ensure_gl_setup ():
 /GstPlayBin:playbin/GstPlaySink:playsink/GstBin:vbin/GstGLImageSink:glimagesink0
 GLib (gthread-posix.c): Unexpected error from C library during
 'pthread_mutex_lock': Invalid argument.  Aborting.
 Aborted
This is most likely fixed in stable releases. It's probably a bug in an
error path (spurious unlock, or use after free). If not fixed, 

Re: Using the coda driver with Gstreamer

[Nicolas Dufresne]

Le 2014-11-18 13:08, Fabio Estevam a écrit :
 On Mon, Nov 17, 2014 at 5:58 PM, Fabio Estevam feste...@gmail.com wrote:
 
 Just a wild guess - we usually test here with dmabuf capable devices and
 without X. As you are using gstglimagesink, the code around
 ext/gl/gstglimagesink.c (453) looks like gst_gl_context_create() went
 wrong. Does your GL work correctly? Maybe you can test the glimagesink
 with a simpler pipeline first.

 Yes, maybe it would be better to remove X from my initial tests. I
 will give it a try.
 
 Now I have a rootfs without X:

Ok, nice, though what is your plan to display the result now ? This is
important information to get any help.

 
 root@imx6qsabresd:/home# gst-inspect-1.0 | grep v4l2
 video4linux2:  v4l2src: Video (video4linux2) Source
 video4linux2:  v4l2sink: Video (video4linux2) Sink
 video4linux2:  v4l2radio: Radio (video4linux2) Tuner
 video4linux2:  v4l2deviceprovider (GstDeviceProviderFactory)
 video4linux2:  v4l2video1dec: V4L2 Video Decoder
 
 Basic test works fine:
 root@imx6qsabresd:/home# gst-launch-1.0 videotestsrc ! fbdevsink
 
 root@imx6qsabresd:/home# gst-launch-1.0 playbin 
 uri=file:///home/H264_test1_Talk
 inghead_mp4_480x360.mp4
 Setting pipeline to PAUSED ...
 Pipeline is PREROLLING ...
 Redistribute latency...
 [  138.267329] coda 204.vpu: CODA PIC_RUN timeout
 
 I was not able to switch to Gstreamer 1.4.4 yet, so this was on 1.4.1.
 

Ok, let us know when the switch is made. Assuming your goal is to get
the HW decoder working, you should test with simpler pipeline. In your
specific case, you should try and get this pipeline to preroll:

gst-launch-1.0 \
  filesrc location=/home/H264_test1_Talk inghead_mp4_480x360.mp4 \
  ! qtdemux ! h264parse ! v4l2video1dec ! fakesink

If that pipeline does not reach PLAYING state, it means the decoder
never produced any output. You should also learn to use the tracing
system in GStreamer and make sure you have the source code next to you.
I would also suggest to try your best to sort as much as you can, make
sure you understand what you are trying to achieve and then come back
with questions.

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gst-running.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: V4l2 state transition

[Nicolas Dufresne]

Le 2014-12-08 00:19, Bin Chen a écrit :

Can anyone comment is following state transition diagram for V4l2 user
space program make sense? Do you see any issues if we were to enforce
this constraint?
I think you should request some buffers before streamon. If in capture, 
you should also queue the minimum amount of buffers.


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

Re: V4l2 state transition

[Nicolas Dufresne]

Le 2014-12-08 09:29, Nicolas Dufresne a écrit :


Le 2014-12-08 00:19, Bin Chen a écrit :

Can anyone comment is following state transition diagram for V4l2 user
space program make sense? Do you see any issues if we were to enforce
this constraint?
I think you should request some buffers before streamon. If in 
capture, you should also queue the minimum amount of buffers.
I forgot, setting input and format isn't strictly required. Driver 
should have decent default configured.


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

LibV4L2 and CREATE_BUFS issues

[Nicolas Dufresne]

Hi,

we recently fixed our CREATE_BUFS support in GStreamer master. It works
nicely with UVC drivers. The problem is that libv4l2 isn't aware of it,
and endup taking terribly decision the least quickly lead to crash.

I'm not sure what that right approach. It seems non-trivial to support
it, at least it would require a bit more knowledge of the converter code
and memory model. Maybe we should at least make sure that CREATE_BUF
fails if we are doing conversion ? Some input on that would be appreciated.

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

Re: coda: not generating EOS event

[Nicolas Dufresne]

Le jeudi 11 décembre 2014 à 11:00 -0200, Fabio Estevam a écrit :
 Hi,
 
 I am running Gstreamer 1.4.4 with on a imx6q-sabresd board and I am
 able to decode a video through the coda driver.
 
 The pipeline I use is:
 gst-launch-1.0 filesrc
 location=/home/H264_test1_Talkinghead_mp4_480x360.mp4 ! qtdemux !
 h264parse ! v4l2video1dec ! videoconvert ! fbdevsink

This is a known issue. The handling of EOS and draining is ill defined
in V4L2 specification. We should be clarifying this soon. Currently
GStreamer implements what was originally done in Exynos MFC driver. This
consist of sending empty buffer to the V4L2 Output (the input), until we
get an empty buffer (bytesused = 0) on the V4L2 Capture (output).

The CODA driver uses the new method to initiate the drain, which is to
send V4L2_DEC_CMD_STOP, this was not implemented by any driver when GST
v4l2 support was added. This is the right thing to do. This is tracked
at (contribution welcome of course):

https://bugzilla.gnome.org/show_bug.cgi?id=733864

Finally, CODA indicate that all buffer has been sent through an event,
V4L2_EVENT_EOS. This event was designed for another use case, it should
in fact be sent when the decoder is done with the encoded buffers,
rather then when the last decoded buffer has been queued. When correctly
implemented, this event cannot be used to figure-out when the last
decoded buffer has been dequeued.

During last workshop, it has been proposed to introduce a flag on the
last decoded buffer, _LAST. This flag could also be put on an empty
buffer to accommodate driver like MFC that only know it's EOS after the
last buffer has been delivered. This all need to be SPECed and
implemented. If you need to get that to run now, the easiest might be to
hack CODA to mimic MFC behavior. Implement DEC_CMD_STOP in GStreamer
shall not be very hard though, it simply need someone to do it.

cheers,
Nicolas

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

Re: LibV4L2 and CREATE_BUFS issues

[Nicolas Dufresne]

Le 2014-12-13 05:41, Hans de Goede a écrit :

I think making CREATE_BUFS fail when doing conversion is probably best,
note that gstreamer should be able to tell which formats will lead to 
doing
conversion, and that it can try to avoid those. 


Those format indeed have a flag. The problem is for HW specific format, 
like few bayers format, which we can't avoid if we need to use such camera.


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

Re: LibV4L2 and CREATE_BUFS issues

[Nicolas Dufresne]

Le 2014-12-14 04:49, Hans de Goede a écrit :

Ah yes I see, so I assume that if libv4l where to return a failure for
CREATE_BUFS when conversion is used, that gstreamer will then fallback to
a regular REQUEST_BUFS call ?

Then that indeed seems the best solution, can you submit patch for this ? 


Exactly, that should work. My concern with application side workaround 
would that the day someone implements CREATE_BUF support in v4l2 this 
application won't benefit without patching. I'll see if I can find time, 
disabling it seems faster then implementing support for it, specially 
that current experiment show that the jpeg code is really fragile. 
Current state is that libv4l2 is causing a buffer overflow, so it is 
harmful library in that sense.


This raise a concern, it would mean that USERPTR, DMABUF, CREATE_BUFS 
will now be lost (in most cases) when enabling libv4l2. This is getting 
a bit annoying. Specially that we are pushing forward having m2m 
decoders to only be usable through libv4l2 (HW specific parsers). Is 
there a long term plan or are we simply pushing the dust toward libv4l2 ?


cheers,
Nicolas

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

[PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

This patchset fixes ability to drain the decoder due to use of wrong
enumeration name and fixes implementation of display delay controls
for MFC firmware v6 and higher.

Note that there is no need in the display delay fix for trying to be
backward compatible with what the comment was saying since the control
properties was preventing it. There was basically no way other then
setting a large delay value to get the frames in display order.

Nicolas Dufresne (3):
  s5p-mfc-v6+: Use display_delay_enable CID
  s5p-mfc-dec: Don't use encoder stop command
  media-doc: Fix MFC display delay control doc

 Documentation/DocBook/media/v4l/controls.xml| 11 +--
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c|  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  6 +-
 3 files changed, 7 insertions(+), 12 deletions(-)

-- 
2.1.0

--
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 2/3] s5p-mfc-dec: Don't use encoder stop command

[Nicolas Dufresne]

The decoder should handle V4L2_DEC_CMD_STOP to trigger drain,
but it currently expecting V4L2_ENC_CMD_STOP.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 99e2e84..98304fc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -816,7 +816,7 @@ static int vidioc_decoder_cmd(struct file *file, void *priv,
unsigned long flags;
 
switch (cmd-cmd) {
-   case V4L2_ENC_CMD_STOP:
+   case V4L2_DEC_CMD_STOP:
if (cmd-flags != 0)
return -EINVAL;
 
-- 
2.1.0

--
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/3] s5p-mfc-v6+: Use display_delay_enable CID

[Nicolas Dufresne]

The MFC driver has two controls, DISPLAY_DELAY and DISPLAY_DELAY_ENABLE
that allow forcing the decoder to return a decoded frame sooner
regardless of the order. The added support for firmware version 6 and
higher was not taking into account the DISPLAY_DELAY_ENABLE boolean.
Instead it had a comment stating that DISPLAY_DELAY should be set to a
negative value to disable it. This is not possible since the control
range is from 0 to 65535. This feature was also supposed to be disabled
by default in order to produce frames in display order.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 92032a0..0675515 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1340,11 +1340,7 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx 
*ctx)
/* FMO_ASO_CTRL - 0: Enable, 1: Disable */
reg |= (fmo_aso_ctrl  S5P_FIMV_D_OPT_FMO_ASO_CTRL_MASK_V6);
 
-   /* When user sets desplay_delay to 0,
-* It works as display_delay enable and delay set to 0.
-* If user wants display_delay disable, It should be
-* set to negative value. */
-   if (ctx-display_delay = 0) {
+   if (ctx-display_delay_enable) {
reg |= (0x1  S5P_FIMV_D_OPT_DDELAY_EN_SHIFT_V6);
writel(ctx-display_delay, mfc_regs-d_display_delay);
}
-- 
2.1.0

--
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 3/3] media-doc: Fix MFC display delay control doc

[Nicolas Dufresne]

The V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE control
is a boolean but was documented as a integer. The documentation was
also slightly miss-leading.

Signed-off-by: Nicolas Dufresne nicolas.dufre...@collabora.com
---
 Documentation/DocBook/media/v4l/controls.xml | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml 
b/Documentation/DocBook/media/v4l/controls.xml
index e013e4b..4e9462f 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2692,12 +2692,11 @@ in the S5P family of SoCs by Samsung.
  rowentry/entry/row
  row
entry 
spanname=idconstantV4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE/constantnbsp;/entry
-   entryinteger/entry
- /rowrowentry spanname=descrIf the display delay is 
enabled then the decoder has to return a
-CAPTURE buffer after processing a certain number of OUTPUT buffers. If this 
number is low, then it may result in
-buffers not being dequeued in display order. In addition hardware may still 
use those buffers as reference, thus
-application should not write to those buffers. This feature can be used for 
example for generating thumbnails of videos.
-Applicable to the H264 decoder.
+   entryboolean/entry
+ /rowrowentry spanname=descrIf the display delay is 
enabled then the decoder is forced to return a
+CAPTURE buffer (decoded frame) after processing a certain number of OUTPUT 
buffers. The delay can be set through
+constantV4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY/constant. 
This feature can be used for example
+for generating thumbnails of videos. Applicable to the H264 decoder.
  /entry
  /row
  rowentry/entry/row
-- 
2.1.0

--
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: coda: Unable to use encoder video_bitrate

[Nicolas Dufresne]

Le 2014-12-22 10:24, Frédéric Sureau a écrit :

Thanks for the patch!
It works fine now after forcing framerate to 30fps (which seems to be 
hardcoded in the driver) 


Can you comment about this on gnome bug. Would make sense for the 
encoder element in GStreamer to relay the framerate to the driver, so 
the driver can make any sense out of the bitrate.


https://bugzilla.gnome.org/show_bug.cgi?id=728438

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

Re: coda: Unable to use encoder video_bitrate

[Nicolas Dufresne]

Le 2014-12-22 11:02, Philipp Zabel a écrit :

That is a good point, rate control can only work if the encoder has
an idea about the framerate. I've sent a patch that allows to use
VIDIOC_S_PARM to set it:
[media] coda: Use S_PARM to set nominal framerate for h.264 encoder
Thanks, I'll sort out what is needed on Gst side. Kamil, do MFC need to 
be looked at too ?


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

Re: [RFC PATCH] [media] coda: Use S_PARM to set nominal framerate for h.264 encoder

[Nicolas Dufresne]

Le 2015-01-15 11:23, Frédéric Sureau a écrit :
Maybe a-parm.output.capability should be set to 
|V4L2_CAP_TIMEPERFRAME| here.

I think it is required by GStreamer V4L2 plugin.
Looking at this, I think output device is indeed the right place to set 
this, and the capability should indeed be updated. Now for the GStreamer 
side, it will need patching. At the moment, this cap is only checked for 
capture device. It's not a major change to enabled that for output 
device with that capability.


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

Re: Using the coda driver with Gstreamer

[Nicolas Dufresne]

Le 2015-02-19 02:24, Zahari Doychev a écrit :

So can you tell me if there are some drivers and plugins that can do this
in efficient way. Is there some work going on in this directions. I suppose
glimagesink maybe will be a good way to go.


There is a lot of work happening, you should notice by searching this 
mailing list archive. The most promising seems to be the one targeting 
Wayland and DMABUF. On the GST side, there is experimental patches to 
handle DMABUF import and render queue, these are not yet fully merged 
into Wayland protocol and Weston. Weston can import DMABUF of various 
format and do the colorspace conversion using shaders. This can 
compensate the lack of hw color converter if your GPU is fast and 
precise enough.


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

Re: Using the coda driver with Gstreamer

[Nicolas Dufresne]

Le 2015-02-18 03:42, Zahari Doychev a écrit :

gst-launch-1.0 filesrc
location=/home/H264_test1_Talkinghead_mp4_480x360.mp4 ! qtdemux !
h264parse ! v4l2video1dec ! videoconvert ! fbdevsink

I am using this pipeline with gstreamer 1.4.5 and current media branch but I am
getting very poor performance 1-2 fps when playing 800x400 video. Is it possible
that fbdevsink is too slow for that? Does anyone know what is going wrong?
In this context, you most likely have a conversion happening in 
videoconvert followed by a copy at fbdevsink. Framebuffer device is not 
a very good solution if performance matter (no possible zero-copy). 
Specially if the selected framebuffer color format does not match the 
decoded format.


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

Re: [PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

Le 2015-01-08 07:51, Kamil Debski a écrit :

I usually don't ack patches that I plan to take into my tree, but it might
be a good idea to let know the submitter that patches are good

Thanks for letting me know, I may ask informally then next time.

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

Re: [PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

Just a friendly reminder that this patch is pending review ;-P

cheers,
Nicolas

Le 2014-12-15 16:10, Nicolas Dufresne a écrit :

This patchset fixes ability to drain the decoder due to use of wrong
enumeration name and fixes implementation of display delay controls
for MFC firmware v6 and higher.

Note that there is no need in the display delay fix for trying to be
backward compatible with what the comment was saying since the control
properties was preventing it. There was basically no way other then
setting a large delay value to get the frames in display order.

Nicolas Dufresne (3):
   s5p-mfc-v6+: Use display_delay_enable CID
   s5p-mfc-dec: Don't use encoder stop command
   media-doc: Fix MFC display delay control doc

  Documentation/DocBook/media/v4l/controls.xml| 11 +--
  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c|  2 +-
  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  6 +-
  3 files changed, 7 insertions(+), 12 deletions(-)



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

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Nicolas Dufresne]

Le vendredi 17 avril 2015 à 15:53 +0300, Laurent Pinchart a écrit :
 It's funny you mention that. I cloned the gstreamer repositories and
 tried to 
 investigate. The gstreamer v4l2 elements started using data_offset a
 year ago 
 in
 
 commit 92bdd596f2b07dbf4ccc9b8bf3d17620d44f131a
 Author: Nicolas Dufresne nicolas.dufre...@collabora.com
 Date:   Fri Apr 11 17:10:11 2014 -0400
 
 v4l2: Add DMABUF and USERPTR importation
 
 (I've CC'ed Nicolas to this e-mail)
 
 I'm not too familiar with the latest gstreamer code, but after a
 first 
 investigation it seems that gstreamer uses the data_offset field for
 the 
 purpose introduced by this patch, not to convey the header size. One
 more 
 argument in favour of repurposing the field ;-)

My impression was that the data before the offset was non-generic and
had to be skipped by applications that aren't aware. An example usage
would be to a camera with custom sensor producing data serialized with
the frames. The sensor data could be set in a header using custom but
documented format, generic application would simply skip that and work
as usual. Be aware that the implementation in GStreamer is incomplete
and untested as all tested drivers where setting this offset to 0.

cheers,
Nicolas


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

Re: [RFC] V4L2 codecs in user space

[Nicolas Dufresne]

Le lundi 08 juin 2015 à 13:42 +0900, Damian Hobson-Garcia a écrit :
 Also, if this method is not recommended, should there be a 1-2 line
 disclaimer on the V4L2_Userspace_Library wiki page that mentions 
 this?

I think you may have got that wrong. The V4L2 userspace library is not
implementing any device drivers. It allow older software to work with
latest V4L2 features by emulating what is possible. It also implement
platform specific setups (media controller) and eventually will contain
needed parsers that would otherwise represent a security threat if ran
inside the Linux Kernel.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: imx53 IPU support on 4.0.4

[Nicolas Dufresne]

Le mercredi 20 mai 2015 à 21:55 +0100, Russell King - ARM Linux a écrit
 :
 The way kernel development works is that patches are sent to mailing
 lists for review.  Kernel developers review the patches and provide
 comments back.  The comments are discussed and actioned, and a new
 set of patches posted for further review.  This cycle repeats.

This patchset is already in the queue, so nothing to be done here.
@Fabio, you simply have to be patient.

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

Re: [PATCH 4/4] v4l2-mem2mem: allow reqbufs(0) with "in use" MMAP buffers

[Nicolas Dufresne]

Le mardi 08 avril 2014 à 12:51 +0200, Marek Szyprowski a écrit :
> Hello,
> 
> On 2014-04-07 16:41, Kamil Debski wrote:
> > Pawel, Marek,
> > 
> > Before taking this to my tree I wanted to get an ACK from one of
> > the
> > videobuf2 maintainers. Could you spare a moment to look through
> > this
> > patch?
> 
> It's not a bug, it is a feature. This was one of the fundamental
> design 
> requirements to allow applications to track if the memory is used or
> not.

I still have the impression it is not fully correct for the case
buffers are exported using DMABUF. Like if the dmabuf was owning the
vb2 buffer instead of the opposite ...

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: v4l2 api: supported resolution negotiation

[Nicolas Dufresne]

Le dimanche 04 octobre 2015 à 21:49 +0300, Sakari Ailus a écrit :
> I think the GStreamer
> v4lsrc tries very small and very large values. The driver will clamp
> them to
> a supported values which are passed to the application from the
> IOCTL.

In GStreamer we try ENUM_FRAMESIZE, and when no supported, we fallback
to try_fmt() with 1x1 (driver will raise it to the minimum) and then
MAX,MAX (driver will lower it to the maximum). We assume that the
driver supports a range, as iterating over all possibilities takes too
much time.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: v4l2 kernel module debugging methods

[Nicolas Dufresne]

Le dimanche 06 décembre 2015 à 00:00 +0200, Ran Shalit a écrit :
> Hello,
> 
> I would like to ask a general question regarding methods to debug a
> v4l2 device driver.
> Since I assume that the kernel driver will probably won't work in
> first try after coding everything inside the device driver...
> 
> 1. Do you think qemu/kgdb debugger is a good method for the device
> driver debugging , or is it plain printing ?
> 
> 2. Is there a simple way to display the image of a YUV-like buffer in
> memory ?

Most Linux distribution ships GStreamer. You can with GStreamer read
and display a raw YUV images (you need to know the specific format)
using videoparse element.

  gst-launch-1.0 filesrc location=my.yuv ! videoparse format=yuy2 width=320 
height=240 ! imagefreeze ! videoconvert ! autovideosink

You could also encode and store to various formats, replacing the
imagefreeze ... section with an encoder and a filesink. Note that
videoparse unfortunatly does not allow passing strides array or
offsets. So it will work only if you set the width/height to padded
width/height.

regards,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: problem with coda when running qt-gstreamer and video reaches its end (resending in plain text)

[Nicolas Dufresne]

Le mercredi 16 décembre 2015 à 15:49 +0100, Philipp Zabel a écrit :
> > # [ 1382.828875] coda 204.vpu: CODA PIC_RUN timeout
> > # [ 1383.938704] coda 204.vpu: CODA PIC_RUN timeout
> > 
> > The video is stopped but I can see last frame on the screen although in 
> > qt application it should receive end-of-stream message and stop the 
> > video (resulting with black screen).
> 
> Looks like the coda driver is constantly fed empty buffers, which don't
> increase the bitstream payload level, and the PIC_RUN times out with a
> bitstream buffer underflow. What GStreamer version is this?

I believe this is side effect of how the MFC driver worked in it's
early stage. We had to keep pushing empty buffer to drain the driver.
So GStreamer still poll/queue/poll/queue/... until all capture buffers
are received. I notice recently that this behaviour can induce high CPU
load with some other drivers that don't do any active wait when a empty
buffer is queued. I would therefor suggest to change this code to only
push one empty buffer for your use case. An submitted patch to support
CMD_STOP can be found here, though is pending a re-submition by it's
author.

https://bugzilla.gnome.org/show_bug.cgi?id=733864

For proper EOS detection with CODA driver (using EPIPE return value),
you indeed need GStreamer 1.6+.

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

VIVID bug in BGRA and ARGB

[Nicolas Dufresne]

Hi Hans,

while testing over color formats VIVID produce, I found that BGRA and
ARGB the alpha component is always 0, which leads to black frames when
composed (when the background is black of course). Is that a bug, or
intended ?

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: v4l2 kernel module debugging methods

[Nicolas Dufresne]

Le jeudi 10 décembre 2015 à 23:46 +0200, Ran Shalit a écrit :
> Thank you for the comment.
> As someone expreinced with v4l2 device driver, do you recommened
> using
> debugging technique such as qemu (or kgdb) or do you rather use plain
> printing ?

I never used that, printing I used. You should also run v4l2-
compliance. It's a test suite, part of v4l-utils.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: Multiple open and read of vivi device

[Nicolas Dufresne]

Le mardi 05 janvier 2016 à 23:18 +0200, Ran Shalit a écrit :
> Does anyone knows why vivi is limited to one open ?
> Is there some way to patch it for multiple opens and reading ?

This is not fully exact. You can open vivid device multiple times.
Though you can only have one instance streaming at one time. This is to
mimic real hardware driver behaviour. Note that you can create multiple
devices using n_devs module parameter.

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 00/38] i.MX5/6 Video Capture

[Nicolas Dufresne]

Le jeudi 16 juin 2016 à 10:02 -0700, Steve Longerbeam a écrit :
> I found the cause at least in my case. After enabling dynamic debug in
> videobuf2-dma-contig.c, "v4l2-ctl -d/dev/video0 --stream-user=8" gives
> me
> 
> [  468.826046] user data must be aligned to 64 bytes
> 
> 
> 
> But even getting past that alignment issue, I've only tested userptr (in 
> mem2mem
> driver) by giving the driver a user address of a mmap'ed kernel contiguous
> buffer. A true discontiguous user buffer may not work, the IPU DMA does not
> support scatter-gather.

If it's dma-contig, you'll need page aligned and contiguous memory.
What some test application do when testing their driver with that, is
to allocate memory using another device, or m2m device, that uses the
same allocator.

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

Re: [PATCH 00/38] i.MX5/6 Video Capture

[Nicolas Dufresne]

Le jeudi 16 juin 2016 à 10:02 -0700, Steve Longerbeam a écrit :
> I found the cause at least in my case. After enabling dynamic debug in
> videobuf2-dma-contig.c, "v4l2-ctl -d/dev/video0 --stream-user=8" gives
> me
> 
> [  468.826046] user data must be aligned to 64 bytes
> 
> 
> 
> But even getting past that alignment issue, I've only tested userptr (in 
> mem2mem
> driver) by giving the driver a user address of a mmap'ed kernel contiguous
> buffer. A true discontiguous user buffer may not work, the IPU DMA does not
> support scatter-gather.

If it's dma-contig, you'll need page aligned and contiguous memory.
What some test application do when testing their driver with that, is
to allocate memory using another device, or m2m device, that uses the
same allocator.

regards,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le lundi 11 avril 2016 à 15:11 +0300, Stanimir Varbanov a écrit :
> adding gstreamer-devel
> 
> On 04/11/2016 03:03 PM, Stanimir Varbanov wrote:
> > 
> > Hi,
> > 
> > I'm working on QCOM v4l2 video decoder/encoder driver and in order
> > to
> > test its functionalities I'm using gstreamer v4l2videodec plugin. I
> > am
> > able to use the v4l2videodec plugin with MMAP, now I want to try
> > the
> > dmabuf export from v4l2 and import dmabuf buffers to glimagesink. I
> > upgraded gst to 1.7.91 so that I have the dmabuf support in
> > glimagesink.
> > Mesa version is 11.1.2.

I'm very happy to see this report. So far, we only had report that this
element works on Freescale IMX.6 (CODA) and Exynos 4/5.

> > 
> > I'm using the following pipeline:
> > 
> > GST_GL_PLATFORM=egl GST_GL_API=gles2 gst-launch-1.0 $GSTDEBUG
> > $GSTFILESRC ! qtdemux name=m m.video_0 ! h264parse ! v4l2video32dec
> > capture-io-mode=dmabuf ! glimagesink
> > 
> > I stalled on this error:
> > 
> > eglimagememory
> > gsteglimagememory.c:473:gst_egl_image_memory_from_dmabuf: > llocator0>
> > eglCreateImage failed: EGL_BAD_MATCH
> > 
> > which in Mesa is:
> > 
> > libEGL debug: EGL user error 0x3009 (EGL_BAD_MATCH) in
> > dri2_create_image_khr_texture
> > 
> > Do someone know how the dmabuf import is tested when the support
> > has
> > been added to glimagesink? Or some pointers how to continue with
> > debugging?

So far the DMABuf support in glimagesink has been tested on Intel/Mesa
and libMALI. There is work in progress in Gallium/Mesa, but until
recently there was no support for offset in imported buffer, which
would result in BAD_MATCH error. I cannot guaranty this is the exact
reason here, BAD_MATCH is used for a very wide variety of reason in
those extensions. The right place to dig into this issue would be
through the Mesa list and/or Mesa code. Find out what is missing for
you driver in Mesa and then I may help you further.

For the reference, the importation strategy we use in GStreamer has
been inspired of Kodi (xmbc). It consist of importing each YUV plane
seperatly using R8 and RG88 textures and doing the color conversion
using shaders. Though, if the frame is allocated as a single DMABuf,
this requires using offset to access the frame data, and that support
had only been recently added in Gallium base code and in Radeon driver
recently. I don't know if Freedreno, VC4 have that, and I know nouveau
don't.

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

Re: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le mardi 12 avril 2016 à 11:57 +0300, Stanimir Varbanov a écrit :
> > I'm very happy to see this report. So far, we only had report that
> this
> > element works on Freescale IMX.6 (CODA) and Exynos 4/5.
> 
> In this context, I would be very happy to see v4l2videoenc merged
> soon :)

That will happen when all review comments are resolved.

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/7] [media]: v4l: add Mediatek MT21 video block format

[Nicolas Dufresne]

Le mercredi 13 avril 2016 à 20:01 +0800, Tiffany Lin a écrit :
> From: Daniel Kurtz 
> 
> Mediatek video format is YVU8_420_2PLANE_PACK8_PROGRESSIVE.
> 
> Create V4L2_PIX_FMT_MT21 and DRM_FORMAT_MT21 to be consistent with
> V4L2_PIX_FMT_NV12 notation.
> 
> Signed-off-by: Tiffany Lin 
> ---
>  include/uapi/drm/drm_fourcc.h  |1 +
>  include/uapi/linux/videodev2.h |2 ++

Might be better to split this patch.

>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 0b69a77..a193905 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -116,6 +116,7 @@
>  #define DRM_FORMAT_NV24  fourcc_code('N', 'V', '2', '4') /* 
> non-subsampled Cr:Cb plane */
>  #define DRM_FORMAT_NV42  fourcc_code('N', 'V', '4', '2') /* 
> non-subsampled Cb:Cr plane */
>  
> +#define DRM_FORMAT_MT21  fourcc_code('M', 'T', '2', '1') /* 
> Mediatek Block Mode */

Please document the tiling format, don't just add a define here.

>  /*
>   * 3 plane YCbCr
>   * index 0: Y plane, [7:0] Y
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index d0acd26..e9e3276 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -527,6 +527,8 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_HM12v4l2_fourcc('H', 'M', '1', '2') /*  8  YUV 
> 4:2:0 16x16 macroblocks */
>  #define V4L2_PIX_FMT_M420v4l2_fourcc('M', '4', '2', '0') /* 12  YUV 
> 4:2:0 2 lines y, 1 line uv interleaved */
>  
> +#define V4L2_PIX_FMT_MT21v4l2_fourcc('M', 'T', '2', '1') /* Mediatek 
> Block Mode  */
> +

Same. On Linux Media side, there is docbook documentation where you can
explain in detail.

>  /* two planes -- one Y, one Cr + Cb interleaved  */
>  #define V4L2_PIX_FMT_NV12v4l2_fourcc('N', 'V', '1', '2') /* 12  Y/CbCr 
> 4:2:0  */
>  #define V4L2_PIX_FMT_NV21v4l2_fourcc('N', 'V', '2', '1') /* 12  Y/CrCb 
> 4:2:0  */
--
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] uvc: Fix bytesperline calculation for planar YUV

[Nicolas Dufresne]

Le mercredi 13 avril 2016 à 17:36 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Thursday 07 Jan 2016 15:43:48 Nicolas Dufresne wrote:
> > 
> > The formula used to calculate bytesperline only works for packed
> > format.
> > So far, all planar format we support have their bytesperline equal
> > to
> > the image width (stride of the Y plane or a line of Y for M420).
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 18 --
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index d7723ce..ceb1d1b 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -142,6 +142,20 @@ static __u32 uvc_try_frame_interval(struct
> > uvc_frame
> > *frame, __u32 interval) return interval;
> >  }
> > 
> > +static __u32 uvc_v4l2_get_bytesperline(struct uvc_format *format,
> > +   struct uvc_frame *frame)
> I'd make the two parameters const.

I agree.

> 
> > 
> > +{
> > +   switch (format->fcc) {
> > +   case V4L2_PIX_FMT_NV12:
> > +   case V4L2_PIX_FMT_YVU420:
> > +   case V4L2_PIX_FMT_YUV420:
> > +   case V4L2_PIX_FMT_M420:
> > +   return frame->wWidth;
> > +   default:
> > +   return format->bpp * frame->wWidth / 8;
> > +   }
> > +}
> > +
> >  static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >     struct v4l2_format *fmt, struct uvc_streaming_control
> > *probe,
> >     struct uvc_format **uvc_format, struct uvc_frame
> > **uvc_frame)
> > @@ -245,7 +259,7 @@ static int uvc_v4l2_try_format(struct
> > uvc_streaming
> > *stream, fmt->fmt.pix.width = frame->wWidth;
> >     fmt->fmt.pix.height = frame->wHeight;
> >     fmt->fmt.pix.field = V4L2_FIELD_NONE;
> > -   fmt->fmt.pix.bytesperline = format->bpp * frame->wWidth /
> > 8;
> > +   fmt->fmt.pix.bytesperline =
> > uvc_v4l2_get_bytesperline(format, frame);
> >     fmt->fmt.pix.sizeimage = probe->dwMaxVideoFrameSize;
> >     fmt->fmt.pix.colorspace = format->colorspace;
> >     fmt->fmt.pix.priv = 0;
> > @@ -282,7 +296,7 @@ static int uvc_v4l2_get_format(struct
> > uvc_streaming
> > *stream, fmt->fmt.pix.width = frame->wWidth;
> >     fmt->fmt.pix.height = frame->wHeight;
> >     fmt->fmt.pix.field = V4L2_FIELD_NONE;
> > -   fmt->fmt.pix.bytesperline = format->bpp * frame->wWidth /
> > 8;
> > +   fmt->fmt.pix.bytesperline =
> > uvc_v4l2_get_bytesperline(format, frame);
> >     fmt->fmt.pix.sizeimage = stream->ctrl.dwMaxVideoFrameSize;
> >     fmt->fmt.pix.colorspace = format->colorspace;
> >     fmt->fmt.pix.priv = 0;
> This looks good to me otherwise.
> 
> If it's fine with you I can fix the above issue while applying.

That would be really nice.

thanks,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le jeudi 19 mai 2016 à 15:25 +0200, Philipp Zabel a écrit :
> Is there any reason not to update the MFC driver to use G_SELECTION?
> The
> g_crop implementation could be kept for backwards compatibility.

Videobuf2 already provide this backward compatible implementation, so
there is no reason not to port that driver (if this is not done
already, maybe ask Kamil ?).

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [RESEND PATCH] [media] s5p-mfc: don't close instance after free OUTPUT buffers

[Nicolas Dufresne]

Le vendredi 06 mai 2016 à 18:11 -0400, Javier Martinez Canillas a
écrit :
> From: ayaka <ay...@soulik.info>
> 
> User-space applications can use the VIDIOC_REQBUFS ioctl to determine
> if a
> memory mapped, user pointer or DMABUF based I/O is supported by the
> driver.
> 
> So a set of VIDIOC_REQBUFS ioctl calls will be made with count 0 and
> then
> the real VIDIOC_REQBUFS call with count == n. But for count 0, the
> driver
> not only frees the buffer but also closes the MFC instance and
> s5p_mfc_ctx
> state is set to MFCINST_FREE.
> 
> The VIDIOC_REQBUFS handler for the output device checks if the
> s5p_mfc_ctx
> state is set to MFCINST_INIT (which happens on an VIDIOC_S_FMT) and
> fails
> otherwise. So after a VIDIOC_REQBUFS(n), future VIDIOC_REQBUFS(n)
> calls
> will fails unless a VIDIOC_S_FMT ioctl calls happens before the
> reqbufs.
> 
> But applications may first set the format and then attempt to
> determine
> the I/O methods supported by the driver (for example Gstramer does
> it) so
> the state won't be set to MFCINST_INIT again and VIDIOC_REQBUFS will
> fail.
> 
> To avoid this issue, only free the buffers on VIDIOC_REQBUFS(0) but
> don't
> close the MFC instance to allow future VIDIOC_REQBUFS(n) calls to
> succeed.
> 
> Signed-off-by: ayaka <ay...@soulik.info>
> [javier: Rewrote changelog to explain the problem more detailed]
> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

I just tested this change, it also seems correct.

Acked-by: Nicolas Dufresne <nico...@collabora.com>

> 
> ---
> Hello,
> 
> This is a resend of a patch posted by Ayaka some time ago [0].
> Without $SUBJECT, trying to decode a video using Gstramer fails
> on an Exynos5422 Odroid XU4 board with following error message:
> 
> $ gst-launch-1.0 filesrc location=test.mov ! qtdemux ! h264parse !
> v4l2video0dec ! videoconvert ! autovideosink
> 
> Setting pipeline to PAUSED ...
> Pipeline is PREROLLING ...
> [ 3947.114756] vidioc_reqbufs:576: Only V4L2_MEMORY_MAP is supported
> [ 3947.114771] vidioc_reqbufs:576: Only V4L2_MEMORY_MAP is supported
> [ 3947.114903] reqbufs_output:484: Reqbufs called in an invalid state
> [ 3947.114913] reqbufs_output:510: Failed allocating buffers for
> OUTPUT queue
> ERROR: from element
> /GstPipeline:pipeline0/v4l2video0dec:v4l2video0dec0: Failed to
> allocate required memory.
> Additional debug info:
> gstv4l2videodec.c(575): gst_v4l2_video_dec_handle_frame ():
> /GstPipeline:pipeline0/v4l2video0dec:v4l2video0dec0:
> Buffer pool activation failed
> ERROR: pipeline doesn't want to preroll.
> Setting pipeline to NULL ...
> Freeing pipeline ...
> 
> [0]: https://patchwork.linuxtv.org/patch/32794/
> 
> Best regards,
> Javier
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index f2d6376ce618..8b9467de2d6a 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -474,7 +474,6 @@ static int reqbufs_output(struct s5p_mfc_dev
> *dev, struct s5p_mfc_ctx *ctx,
>   ret = vb2_reqbufs(>vq_src, reqbufs);
>   if (ret)
>   goto out;
> - s5p_mfc_close_mfc_inst(dev, ctx);
>   ctx->src_bufs_cnt = 0;
>   ctx->output_state = QUEUE_FREE;
>   } else if (ctx->output_state == QUEUE_FREE) {

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] Revert "[media] videobuf2-v4l2: Verify planes array in buffer dequeueing"

[Nicolas Dufresne]

Le mercredi 11 mai 2016 à 13:27 -0300, Mauro Carvalho Chehab a écrit :
> This patch causes a Kernel panic when called on a DVB driver.
> 
> This reverts commit 2c1f6951a8a82e6de0d82b1158b5e493fc6c54ab.

Seems rather tricky, since this commit fixed a possible (user induced)
buffer overflow according to Sakari comment. Would be nice to fix and
resubmit.

> 
> Cc: Sakari Ailus 
> Cc: Hans Verkuil 
> Cc: sta...@vgar.kernel.org
> Fixes: 2c1f6951a8a8 ("[media] videobuf2-v4l2: Verify planes array in
> buffer dequeueing")
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c
> b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 7f366f1b0377..0b1b8c7b6ce5 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -74,11 +74,6 @@ static int __verify_planes_array(struct vb2_buffer
> *vb, const struct v4l2_buffer
>   return 0;
>  }
>  
> -static int __verify_planes_array_core(struct vb2_buffer *vb, const
> void *pb)
> -{
> - return __verify_planes_array(vb, pb);
> -}
> -
>  /**
>   * __verify_length() - Verify that the bytesused value for each
> plane fits in
>   * the plane length and that the data offset doesn't exceed the
> bytesused value.
> @@ -442,7 +437,6 @@ static int __fill_vb2_buffer(struct vb2_buffer
> *vb,
>  }
>  
>  static const struct vb2_buf_ops v4l2_buf_ops = {
> - .verify_planes_array= __verify_planes_array_core,
>   .fill_user_buffer   = __fill_v4l2_buffer,
>   .fill_vb2_buffer= __fill_vb2_buffer,
>   .copy_timestamp = __copy_timestamp,
--
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: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le vendredi 13 mai 2016 à 11:45 +0300, Stanimir Varbanov a écrit :
> One thing which bothers me is how the extra-controls property
> working,
> i.e. I failed to change the h264 profile for example with below
> construct:
> 
> extra-controls="controls,h264_profile=4;"

Yes, and profile should be negotiated with downstream in GStreamer. For
common controls, like bitrate, it should be exposed as separate
properties instead.

Nicolas

signature.asc
Description: This is a digitally signed message part

Re: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le vendredi 13 mai 2016 à 11:45 +0300, Stanimir Varbanov a écrit :
> yes, of course :)
> 
> Just FYI, I applied the WIP patches on my side and I'm very happy to
> report that they just works. So If you need some testing on qcom
> video
> accelerator just ping me.
> 
> One thing which bothers me is how the extra-controls property
> working,
> i.e. I failed to change the h264 profile for example with below
> construct:
> 
> extra-controls="controls,h264_profile=4;"

While I got you. I would be very interested to know who QCOM driver
interpreted the width and height expose on capture side of the decoder.
I'm adding Philippe Zabel in CC here (he's maintaining the
CODA/Freescale decoder). In Samsung MFC driver, the width and height
expose by the driver is the display size. Though, recently, Philippe is
reporting that his driver is exposing the coded size. Fixing one in
GStreamer will break the other, so I was wondering what other drivers
are doing.

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [RFC 08/22] videodev2.h: Add request field to v4l2_pix_format_mplane

[Nicolas Dufresne]

Le vendredi 06 mai 2016 à 13:53 +0300, Sakari Ailus a écrit :
> From: Laurent Pinchart 
> 
> Let userspace specify a request ID when getting or setting formats.
> The
> support is limited to the multi-planar API at the moment, extending
> it
> to the single-planar API is possible if needed.
> 
> From a userspace point of view the API change is also minimized and
> doesn't require any new ioctl.
> 
> Signed-off-by: Laurent Pinchart  d.com>
> ---
>  include/uapi/linux/videodev2.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h
> b/include/uapi/linux/videodev2.h
> index ac28299..6260d0e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1972,6 +1972,7 @@ struct v4l2_plane_pix_format {
>   * @ycbcr_enc:   enum v4l2_ycbcr_encoding, Y'CbCr
> encoding
>   * @quantization:enum v4l2_quantization, colorspace
> quantization
>   * @xfer_func:   enum v4l2_xfer_func, colorspace
> transfer function
> + * @request: request ID
>   */
>  struct v4l2_pix_format_mplane {
>   __u32   width;
> @@ -1986,7 +1987,8 @@ struct v4l2_pix_format_mplane {
>   __u8ycbcr_enc;
>   __u8quantization;
>   __u8xfer_func;
> - __u8reserved[7];
> + __u8reserved[3];
> + __u32   request;

Shouldn't the request member be added before the padding ?

>  } __attribute__ ((packed));
>  
>  /**
--
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: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le jeudi 28 avril 2016 à 14:33 +0300, Stanimir Varbanov a écrit :
> So I'm wondering is that intensional?
> 
> Depending on the answer I should make the same in the Gallium dri2 in
> dri2_from_dma_bufs().

It's DRI format that are confusing. In GStreamer DRI_FORMAT_GR88 would
be named RG88 (if it existed). That's because DRI API present it in the
way it would look on the CPU after loading that 16bit word into a
register. In GStreamer instead, we expose is as the way you'll find it
in the data array. Let's see it this way, DRI present the information
in a way people writing rasterizer see it.


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

Re: [RESEND PATCH] [media] s5p-mfc: don't close instance after free OUTPUT buffers

[Nicolas Dufresne]

Thanks for re-submitting. See inline two typos to fix in teh comment.

cheers,
Nicolas

Le vendredi 06 mai 2016 à 18:11 -0400, Javier Martinez Canillas a écrit :
> From: ayaka 
> 
> User-space applications can use the VIDIOC_REQBUFS ioctl to determine if a
> memory mapped, user pointer or DMABUF based I/O is supported by the driver.
> 
> So a set of VIDIOC_REQBUFS ioctl calls will be made with count 0 and then
> the real VIDIOC_REQBUFS call with count == n. But for count 0, the driver
> not only frees the buffer but also closes the MFC instance and s5p_mfc_ctx
> state is set to MFCINST_FREE.
> 
> The VIDIOC_REQBUFS handler for the output device checks if the s5p_mfc_ctx
> state is set to MFCINST_INIT (which happens on an VIDIOC_S_FMT) and fails
> otherwise. So after a VIDIOC_REQBUFS(n), future VIDIOC_REQBUFS(n) calls
> will fails unless a VIDIOC_S_FMT ioctl calls happens before the reqbufs.
> 
> But applications may first set the format and then attempt to determine
> the I/O methods supported by the driver (for example Gstramer does it) so
 * GStreamer

> the state won't be set to MFCINST_INIT again and VIDIOC_REQBUFS will fail.
> 
> To avoid this issue, only free the buffers on VIDIOC_REQBUFS(0) but don't
> close the MFC instance to allow future VIDIOC_REQBUFS(n) calls to succeed.
> 
> Signed-off-by: ayaka 
> [javier: Rewrote changelog to explain the problem more detailed]
> Signed-off-by: Javier Martinez Canillas 
> 
> ---
> Hello,
> 
> This is a resend of a patch posted by Ayaka some time ago [0].
> Without $SUBJECT, trying to decode a video using Gstramer fails

* GStreamer again 

> on an Exynos5422 Odroid XU4 board with following error message:
> 
> $ gst-launch-1.0 filesrc location=test.mov ! qtdemux ! h264parse ! 
> v4l2video0dec ! videoconvert ! autovideosink
> 
> Setting pipeline to PAUSED ...
> Pipeline is PREROLLING ...
> [ 3947.114756] vidioc_reqbufs:576: Only V4L2_MEMORY_MAP is supported
> [ 3947.114771] vidioc_reqbufs:576: Only V4L2_MEMORY_MAP is supported
> [ 3947.114903] reqbufs_output:484: Reqbufs called in an invalid state
> [ 3947.114913] reqbufs_output:510: Failed allocating buffers for OUTPUT queue
> ERROR: from element /GstPipeline:pipeline0/v4l2video0dec:v4l2video0dec0: 
> Failed to allocate required memory.
> Additional debug info:
> gstv4l2videodec.c(575): gst_v4l2_video_dec_handle_frame (): 
> /GstPipeline:pipeline0/v4l2video0dec:v4l2video0dec0:
> Buffer pool activation failed
> ERROR: pipeline doesn't want to preroll.
> Setting pipeline to NULL ...
> Freeing pipeline ...
> 
> [0]: https://patchwork.linuxtv.org/patch/32794/
> 
> Best regards,
> Javier
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index f2d6376ce618..8b9467de2d6a 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -474,7 +474,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, struct 
> s5p_mfc_ctx *ctx,
>   ret = vb2_reqbufs(>vq_src, reqbufs);
>   if (ret)
>   goto out;
> - s5p_mfc_close_mfc_inst(dev, ctx);
>   ctx->src_bufs_cnt = 0;
>   ctx->output_state = QUEUE_FREE;
>   } else if (ctx->output_state == QUEUE_FREE) {

signature.asc
Description: This is a digitally signed message part

Re: gstreamer: v4l2videodec plugin

[Nicolas Dufresne]

Le vendredi 15 avril 2016 à 11:58 -0400, Rob Clark a écrit :
> The issue is probably the YUV format, which we cannot really deal
> with
> properly in gallium..  it's a similar issue to multi-planer even if
> it
> is in a single buffer.
> 
> The best way to handle this would be to import the same dmabuf fd
> twice, with appropriate offsets, to create one GL_RED eglimage for Y
> and one GL_RG eglimage for UV, and then combine them in shader in a
> similar way to how you'd handle separate Y and UV planes..

That's the strategy we use in GStreamer, as very few GL stack support
implicit color conversions. For that to work you need to implement the
"offset" field in winsys_handle, that was added recently, and make sure
you have R8 and RG88 support (usually this is just mapping).

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 3/7] [Media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver

[Nicolas Dufresne]

Le lundi 18 avril 2016 à 16:22 +0800, tiffany lin a écrit :
> > > We are plaining to remove m2m framework in th feature, although
> we think
> > 
> > Remove it for just the decoder driver or both encoder and decoder?
> > 
> Remove it from decoder driver.

Did you look at how CODA handle it (drivers/media/platform/coda/coda-
common.c) ? I don't know any detail, but they do have the same issue
and use both v4l2_m2m_fop_poll and v4l2_m2m_fop_mmap.

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

Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

[Nicolas Dufresne]

Le lundi 18 juillet 2016 à 10:34 +0200, Hans Verkuil a écrit :
> On 07/15/2016 06:26 PM, Javier Martinez Canillas wrote:
> > The buffer planes' dma-buf are currently mapped when buffers are queued
> > from userspace but it's more appropriate to do the mapping when buffers
> > are queued in the driver since that's when the actual DMA operation are
> > going to happen.
> 
> Does this solve anything? Once the DMA has started the behavior is the same
> as before (QBUF maps the dmabuf), only while the DMA engine hasn't started
> yet are the QBUF calls just accepted and the mapping takes place when the
> DMA is kickstarted. This makes QBUF behave inconsistently.

The expected behaviour would have been to ensure that DMABuf mapping
only happen when the driver need the buffer (as late as possible). As
you describes it, the goal is not met.

> 
> You don't describe here WHY this change is needed.

It should have been explained (just like this patch should have been
marked as RFC). The is numerous reason why you don't want to spend
userspace time mapping a buffers.

First the context, mapping a DMA-Buf is a costy operation. With the
venu of DMA-Buf fences, (in implicit mode) this operation becomes even
more expensive in term of time you block userspace. By mapping in QBUF,
you do block the userspace process for a certain duration.

By delaying the mapping later, the time spent mapping is now in the
driver thread, without blocking the userspace. This allow running with
much lower latency, as the userspace can (if already available) fetch
the following buffer and put it in the queue without further delays. As
the buffers are available earlier, the streaming can be started sooner
and no time is lost.

Another reason, which was not part of our discussion, is that if you
have a display at lower framerate, it would allow appropriate frame
skipping to be implemented. Mapping all the frames, even the one that
won't be displayed would be inefficient.

So basically, what we are saying, is that the currently implemented
mechanism is a was of userspace time, reduce the benefit of using
fences and increase latency.
 
> 
> I'm not sure I agree with the TODO, and even if I did, I'm not sure I agree
> with this solution. Since queuing the buffer to the driver is not the same
> as 'just before the DMA', since there may be many buffers queued up in the
> driver and you don't know in vb2 when the buffer is at the 'just before the 
> DMA'
> stage.

Unfortunate, but it's just software ;-P An idea would be to introduce
some new state for preparing a buffers, so the driver don't endup
waiting at an unfortunate moment. Again, this is all only needed if we
can provide the same level of buffer validation we had at QBUF. As we
don't expose to userspace the information needed to validate if a DMA-
Buf is compatible, we started (not yet merged) implementing fallback at
QBuf. Failing asynchronously would leave userspace with absolutely no
way to handle the case of incompatible DMA-Buf.

Regards,
Nicolas

p.s. I'll be away for the rest of the summer, see you in September.

> 
> Regards,
> 
>   Hans
> 
> > 
> > Suggested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> > 
> > ---
> > 
> > Hello,
> > 
> > A side effect of this change is that if the dmabuf map fails for some
> > reasons (i.e: a driver using the DMA contig memory allocator but CMA
> > not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> > later (i.e: in VIDIOC_STREAMON).
> > 
> > I don't know if that's an issue though but I think is worth mentioning.
> > 
> > Best regards,
> > Javier
> > 
> >  drivers/media/v4l2-core/videobuf2-core.c | 88 
> > 
> >  1 file changed, 54 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c 
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index ca8ffeb56d72..3fdf882bf279 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
> >  })
> >  
> >  static void __vb2_queue_cancel(struct vb2_queue *q);
> > -static void __enqueue_in_driver(struct vb2_buffer *vb);
> > +static int __enqueue_in_driver(struct vb2_buffer *vb);
> >  
> >  /**
> >   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> > @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, 
> > const void *pb)
> >     vb->planes[plane].mem_priv = mem_priv;
> >     }
> >  
> > -   /* TODO: This pins the buffer(s) with  dma_buf_map_attac

Re: [RFT PATCH v2] [media] exynos4-is: Fix fimc_is_parse_sensor_config() nodes handling

[Nicolas Dufresne]

Le jeudi 21 avril 2016 à 09:21 -0400, Javier Martinez Canillas a
écrit :
> Hello Sylwester,
> 
> On 03/23/2016 08:41 PM, Javier Martinez Canillas wrote:
> > The same struct device_node * is used for looking up the I2C
> > sensor, OF
> > graph endpoint and port. So the reference count is incremented but
> > not
> > decremented for the endpoint and port nodes.
> > 
> > Fix this by having separate pointers for each node looked up.
> > 
> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> > 
> 
> Any comments about this patch?

Tested-by: Nicolas Dufresne <nicoas.dufre...@collabora.com>

Note: I could not verify that leak is gone, but I could verify that
this driver is still working properly after the change.

> 
> Best regards,

signature.asc
Description: This is a digitally signed message part

Re: [RFT PATCH v2] [media] exynos4-is: Fix fimc_is_parse_sensor_config() nodes handling

[Nicolas Dufresne]

Le vendredi 15 juillet 2016 à 12:53 -0400, Nicolas Dufresne a écrit :
> Le jeudi 21 avril 2016 à 09:21 -0400, Javier Martinez Canillas a
> écrit :
> > Hello Sylwester,
> > 
> > On 03/23/2016 08:41 PM, Javier Martinez Canillas wrote:
> > > The same struct device_node * is used for looking up the I2C
> > > sensor, OF
> > > graph endpoint and port. So the reference count is incremented
> > > but
> > > not
> > > decremented for the endpoint and port nodes.
> > > 
> > > Fix this by having separate pointers for each node looked up.
> > > 
> > > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> > > 
> > 
> > Any comments about this patch?
> 
> Tested-by: Nicolas Dufresne <nicoas.dufre...@collabora.com>

There a typo here.

Tested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>

> 
> Note: I could not verify that leak is gone, but I could verify that
> this driver is still working properly after the change.
> 
> > 
> > Best regards,

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2 2/3] [media] hva: multi-format video encoder V4L2 driver

[Nicolas Dufresne]

Le lundi 11 juillet 2016 à 17:14 +0200, Jean-Christophe Trotin a écrit :
> This patch adds V4L2 HVA (Hardware Video Accelerator) video encoder
> driver for STMicroelectronics SoC. It uses the V4L2 mem2mem framework.
> 
> This patch only contains the core parts of the driver:
> - the V4L2 interface with the userland (hva-v4l2.c)
> - the hardware services (hva-hw.c)
> - the memory management utilities (hva-mem.c)
> 
> This patch doesn't include the support of specific codec (e.g. H.264)
> video encoding: this support is part of subsequent patches.
> 
> Signed-off-by: Yannick Fertre 
> Signed-off-by: Jean-Christophe Trotin 
> ---
>  drivers/media/platform/Kconfig|   14 +
>  drivers/media/platform/Makefile   |1 +
>  drivers/media/platform/sti/hva/Makefile   |2 +
>  drivers/media/platform/sti/hva/hva-hw.c   |  534 
>  drivers/media/platform/sti/hva/hva-hw.h   |   42 +
>  drivers/media/platform/sti/hva/hva-mem.c  |   60 ++
>  drivers/media/platform/sti/hva/hva-mem.h  |   36 +
>  drivers/media/platform/sti/hva/hva-v4l2.c | 1299 
> +
>  drivers/media/platform/sti/hva/hva.h  |  284 +++
>  9 files changed, 2272 insertions(+)
>  create mode 100644 drivers/media/platform/sti/hva/Makefile
>  create mode 100644 drivers/media/platform/sti/hva/hva-hw.c
>  create mode 100644 drivers/media/platform/sti/hva/hva-hw.h
>  create mode 100644 drivers/media/platform/sti/hva/hva-mem.c
>  create mode 100644 drivers/media/platform/sti/hva/hva-mem.h
>  create mode 100644 drivers/media/platform/sti/hva/hva-v4l2.c
>  create mode 100644 drivers/media/platform/sti/hva/hva.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 382f393..182d63f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -227,6 +227,20 @@ config VIDEO_STI_BDISP
>   help
>     This v4l2 mem2mem driver is a 2D blitter for STMicroelectronics SoC.
>  
> +config VIDEO_STI_HVA
> + tristate "STMicroelectronics STiH41x HVA multi-format video encoder 
> V4L2 driver"
> + depends on VIDEO_DEV && VIDEO_V4L2
> + depends on ARCH_STI || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_MEM2MEM_DEV
> + help
> +   This V4L2 driver enables HVA multi-format video encoder of
> +   STMicroelectronics SoC STiH41x series, allowing hardware encoding of 
> raw
> +   uncompressed formats in various compressed video bitstreams format.
> +
> +   To compile this driver as a module, choose M here:
> +   the module will be called hva.
> +
>  config VIDEO_SH_VEU
>   tristate "SuperH VEU mem2mem video processing driver"
>   depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 99cf315..784dcd4 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_VIDEO_SAMSUNG_S5P_G2D) += s5p-g2d/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC)   += exynos-gsc/
>  
>  obj-$(CONFIG_VIDEO_STI_BDISP)+= sti/bdisp/
> +obj-$(CONFIG_VIDEO_STI_HVA)  += sti/hva/
>  obj-$(CONFIG_DVB_C8SECTPFE)  += sti/c8sectpfe/
>  
>  obj-$(CONFIG_BLACKFIN)  += blackfin/
> diff --git a/drivers/media/platform/sti/hva/Makefile 
> b/drivers/media/platform/sti/hva/Makefile
> new file mode 100644
> index 000..7022a33
> --- /dev/null
> +++ b/drivers/media/platform/sti/hva/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_VIDEO_STI_HVA) := hva.o
> +hva-y := hva-v4l2.o hva-hw.o hva-mem.o
> diff --git a/drivers/media/platform/sti/hva/hva-hw.c 
> b/drivers/media/platform/sti/hva/hva-hw.c
> new file mode 100644
> index 000..fa293c7
> --- /dev/null
> +++ b/drivers/media/platform/sti/hva/hva-hw.c
> @@ -0,0 +1,534 @@
> +/*
> + * Copyright (C) STMicroelectronics SA 2015
> + * Authors: Yannick Fertre 
> + *  Hugues Fruchet 
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "hva.h"
> +#include "hva-hw.h"
> +
> +/* HVA register offsets */
> +#define HVA_HIF_REG_RST 0x0100U
> +#define HVA_HIF_REG_RST_ACK 0x0104U
> +#define HVA_HIF_REG_MIF_CFG 0x0108U
> +#define HVA_HIF_REG_HEC_MIF_CFG 0x010CU
> +#define HVA_HIF_REG_CFL 0x0110U
> +#define HVA_HIF_FIFO_CMD0x0114U
> +#define HVA_HIF_FIFO_STS0x0118U
> +#define HVA_HIF_REG_SFL 0x011CU
> +#define HVA_HIF_REG_IT_ACK  0x0120U
> +#define HVA_HIF_REG_ERR_IT_ACK  0x0124U
> +#define HVA_HIF_REG_LMI_ERR 0x0128U
> +#define HVA_HIF_REG_EMI_ERR 0x012CU
> +#define HVA_HIF_REG_HEC_MIF_ERR 0x0130U
> +#define HVA_HIF_REG_HEC_STS 0x0134U
> +#define 

Re: [PATCH v2 0/3] support of v4l2 encoder for STMicroelectronics SOC

[Nicolas Dufresne]

Le lundi 11 juillet 2016 à 17:14 +0200, Jean-Christophe Trotin a
écrit :
> version 2:
> - List of pixel formats supported by the encoder reduced to NV12 and
> NV21
> - x86_64 compilation warnings corrected
> - V4L2 compliance successfully passed with this version (see report
> below)
> - All remarks about version 1 of hva-v4l2.c taken into account:
> - V4L2 mem2mem framework used 
>   - V4L2 control framework used
>   - allocator context initialized in the probe and cleaned up in
> the remove
>   - start_streaming and stop_streaming ops added
>   - colorspace, bytesperline and sizeimage fields initialized in
> TRY_FMT
>   - better estimation of sizeimage for compressed formats
>   - checks and debugging logs already covered by vb2 removed
>   - some dev_err changed in dev_dbg
>   - typos corrected
> 
> version 1:
> - Initial submission.
> 
> Only one feature supported and tested:
> - encode (NV12, NV21) to H.264 video format
> 
> The driver is mainly implemented across three files:
> - hva-v4l2.c
> - hva-h264.c
> - hva-hw.c
> hva-v4l2.c manages the V4L2 interface with the userland.
> It calls the HW services that are implemented in hva-hw.c.
> hva-h264.c manages specific part of H.264 codec.
> 
> Below is the v4l2-compliance report for the version 2 of the sti hva
> driver:
> 
> 
> root@sti-next:/home/video_test# v4l2-compliance -d /dev/video0
> Driver Info:
>   Driver name   : 8c85000.hva

I think it would be nice to set a driver name that means something.

>   Card type : 8c85000.hva
>   Bus info  : platform:hva
>   Driver version: 4.7.0
>   Capabilities  : 0x84208000
>   Video Memory-to-Memory
>   Streaming
>   Extended Pix Format
>   Device Capabilities
>   Device Caps   : 0x04208000
>   Video Memory-to-Memory
>   Streaming
>   Extended Pix Format
> 
> Compliance test for device /dev/video0 (not using libv4l2):
> 
> Required ioctls:
>   test VIDIOC_QUERYCAP: OK
> 
> Allow for multiple opens:
>   test second video open: OK
>   test VIDIOC_QUERYCAP: OK
>   test VIDIOC_G/S_PRIORITY: OK
> 
> Debug ioctls:
>   test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
>   test VIDIOC_LOG_STATUS: OK (Not Supported)
> 
> Input ioctls:
>   test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>   test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>   test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>   test VIDIOC_ENUMAUDIO: OK (Not Supported)
>   test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
>   test VIDIOC_G/S_AUDIO: OK (Not Supported)
>   Inputs: 0 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
>   test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>   test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>   test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>   test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>   test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>   Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
>   test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>   test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>   test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>   test VIDIOC_G/S_EDID: OK (Not Supported)
> 
>   Control ioctls:
>   test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>   test VIDIOC_QUERYCTRL: OK
>   test VIDIOC_G/S_CTRL: OK
>   test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>   test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
>   test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>   Standard Controls: 16 Private Controls: 0
> 
>   Format ioctls:
>   test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>   test VIDIOC_G/S_PARM: OK
>   test VIDIOC_G_FBUF: OK (Not Supported)
>   test VIDIOC_G_FMT: OK
>   warn: /local/home/frq08988/views/opensdk-
> 2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-
> formats.cpp(716): TRY_FMT cannot handle an invalid pixelformat.
>   warn: /local/home/frq08988/views/opensdk-
> 2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-
> formats.cpp(717): This may or may not be a problem. For more
> information see:
>   warn: /local/home/frq08988/views/opensdk-
> 2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-
> formats.cpp(718): http://www.mail-archive.com/linux-media@vger.kernel
> .org/msg56550.html
>   warn: /local/home/frq08988/views/opensdk-
> 2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-
> formats.cpp(716): TRY_FMT cannot handle an invalid pixelformat.
>   warn: /local/home/frq08988/views/opensdk-
> 2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-
> formats.cpp(717): This may or may not be a problem. For more
> information see:
>   warn: /local/home/frq08988/views/opensdk-
> 

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

[Nicolas Dufresne]

Le mardi 12 juillet 2016 à 16:16 +0800, Wu-Cheng Li (李務誠) a écrit :
> Decoder hardware produces MT21 (compressed). Image processor can
> convert it to a format that can be input of display driver. Tiffany.
> When do you plan to upstream image processor (mtk-mdp)?
> >
> > It can be as input format for encoder, MDP and display drivers in
> our
> > platform.
> I remember display driver can only accept uncompressed MT21. Right?
> Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
> format. It's not usable until it's decompressed and converted by
> image
> processor.

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

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

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

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

[Nicolas Dufresne]

Le mardi 12 juillet 2016 à 15:08 -0400, Nicolas Dufresne a écrit :
> Le mardi 12 juillet 2016 à 16:16 +0800, Wu-Cheng Li (李務誠) a écrit :
> > Decoder hardware produces MT21 (compressed). Image processor can
> > convert it to a format that can be input of display driver.
> > Tiffany.
> > When do you plan to upstream image processor (mtk-mdp)?
> > > 
> > > It can be as input format for encoder, MDP and display drivers in
> > our
> > > platform.
> > I remember display driver can only accept uncompressed MT21. Right?
> > Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
> > format. It's not usable until it's decompressed and converted by
> > image
> > processor.
> 
> Previously it was described as MediaTek block mode, and now as a
> MediaTek compressed format. It makes me think you have no idea what
> this pixel format really is. Is that right ?
> 
> The main reason why I keep asking, is that we often find similarities
> between what vendor like to call their proprietary formats. Doing the
> proper research helps not creating a mess like in Android where you
> have a lot of formats that all point to the same format. I believe
> there was the same concern when Samsung wanted to introduce their Z-
> flip-Z NV12 tile format. In the end they simply provided sufficient
> documentation so we could document it and implement software
> converters
> for test and validation purpose.

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

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

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

Now obviously this is incomplete, as the swizzling need to be documented of 
course.

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

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

[Nicolas Dufresne]

Le mercredi 13 juillet 2016 à 10:00 +0800, tiffany lin a écrit :
> Hi Nicolas,
> 
> On Tue, 2016-07-12 at 15:14 -0400, Nicolas Dufresne wrote:
> > Le mardi 12 juillet 2016 à 15:08 -0400, Nicolas Dufresne a écrit :
> > > Le mardi 12 juillet 2016 à 16:16 +0800, Wu-Cheng Li (李務誠) a écrit
> :
> > > > Decoder hardware produces MT21 (compressed). Image processor
> can
> > > > convert it to a format that can be input of display driver.
> > > > Tiffany.
> > > > When do you plan to upstream image processor (mtk-mdp)?
> > > > > 
> > > > > It can be as input format for encoder, MDP and display
> drivers in
> > > > our
> > > > > platform.
> > > > I remember display driver can only accept uncompressed MT21.
> Right?
> > > > Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
> > > > format. It's not usable until it's decompressed and converted
> by
> > > > image
> > > > processor.
> > > 
> > > Previously it was described as MediaTek block mode, and now as a
> > > MediaTek compressed format. It makes me think you have no idea
> what
> > > this pixel format really is. Is that right ?
> > > 
> > > The main reason why I keep asking, is that we often find
> similarities
> > > between what vendor like to call their proprietary formats. Doing
> the
> > > proper research helps not creating a mess like in Android where
> you
> > > have a lot of formats that all point to the same format. I
> believe
> > > there was the same concern when Samsung wanted to introduce their
> Z-
> > > flip-Z NV12 tile format. In the end they simply provided
> sufficient
> > > documentation so we could document it and implement software
> > > converters
> > > for test and validation purpose.
> > 
> > Here's the kind of information we want in the documentation.
> > 
> > https://chromium.googlesource.com/chromium/src/media/+/master/base/
> vide
> > o_types.h#40
> > 
> >   // MediaTek proprietary format. MT21 is similar to NV21 except
> the memory
> >   // layout and pixel layout (swizzles). 12bpp with Y plane
> followed by a 2x2
> >   // interleaved VU plane. Each image contains two buffers -- Y
> plane and VU
> >   // plane. Two planes can be non-contiguous in memory. The
> starting addresses
> >   // of Y plane and VU plane are 4KB alignment.
> >   // Suppose image dimension is (width, height). For both Y plane
> and VU plane:
> >   // Row pitch = ((width+15)/16) * 16.
> >   // Plane size = Row pitch * (((height+31)/32)*32)
> > 
> > Now obviously this is incomplete, as the swizzling need to be
> documented of course.
> > 
> Because it's finally a compressed format from our codec hw, we cannot
> describe its swizzling.

Can you clarify why it cannot be described ? I don't buy any argument
that it's impossible to convert without hardware. Intel did document
their compressed formats.

Debugging an integration with such format that we have literally
decided to no make public is a pain. A lot of decoder got abandoned or
never used because of that.

It would be nice to explain the architecture you are suggesting to make
this decoder useful. How will userspace application be able to use your
decoder. Will the image processor usable outside the display path ?
People might want to do transcoding, or other relatively common task
that does not imply a display.

regards,
Nicolas

p.s.
A small note to Wu-Cheng, the definition of the DRM format in your
Chromium branch is not correct. DRM uses format modifier, such that the
format is the family, in your case it's most likely NV21, and the
modifier is the compression or tiling algorithm in place (or both as
needed).
--
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 2/2] [media] s5p-g2d: Replace old driver with DRM version

[Nicolas Dufresne]

Le mardi 12 juillet 2016 à 20:02 -0300, Mauro Carvalho Chehab a écrit :
> I suspect that you'll be applying this one via DRM tree, so:
> 
> Em Tue, 24 May 2016 15:28:13 +0200
> Krzysztof Kozlowski  escreveu:
> 
> > Remove the old non-DRM driver because it is now entirely supported
> by
> > exynos_drm_g2d driver.
> > 
> > Cc: Kyungmin Park 
> > Cc: Kamil Debski 
> > Signed-off-by: Krzysztof Kozlowski 
> 
> Acked-by: Mauro Carvalho Chehab 
> 
> PS.: If you prefer to apply this one via my tree, I'm ok too. Just
> let me know when the first patch gets merged upstream.

Just a note that this is effectively an API break.

cheers,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop

[Nicolas Dufresne]

Le dimanche 03 juillet 2016 à 11:43 +0200, Hans Verkuil a écrit :
> Hi Nicolas,
> 
> On 07/02/2016 10:29 PM, Nicolas Dufresne wrote:
> > 
> > Le 30 juin 2016 5:35 PM, "Shuah Khan" <shua...@osg.samsung.com
> > <mailto:shua...@osg.samsung.com>> a écrit :
> > > 
> > > Remove vidioc_g_crop() from s5p-mfc decoder. Without its s_crop
> > > counterpart
> > > g_crop is not useful. Delete it.
> > 
> > G_CROP tell the userspace which portion of the output is to be
> > displayed. Example,  1920x1080 inside a buffer of 1920x1088. It can
> > be
> > implemented using G_SELECTION too, which emulate G_CROP. removing
> > this without implementing G_SEKECTION will break certain software
> > like
> > GStreamer v4l2 based decoder.
> 
> Sorry, but this is not correct.
> 
> G_CROP for VIDEO_OUTPUT returns the output *compose* rectangle, not
> the output
> crop rectangle.
> 
> Don't blame me, this is how it was defined in V4L2. The problem is
> that for video
> output (esp. m2m devices) you usually want to set the crop rectangle,
> and that's
> why the selection API was added so you can unambiguously set the crop
> and compose
> rectangles for both capture and output.
> 
> Unfortunately, the exynos drivers were written before the
> G/S_SELECTION API was
> created, and the crop ioctls in the video output drivers actually set
> the output
> crop rectangle instead of the compose rectangle :-(
> 
> This is a known inconsistency.
> 
> You are right though that we can't remove g_crop here, I had
> forgotten about the
> buffer padding.
> 
> What should happen here is that g_selection support is added to s5p-
> mfc, and
> have that return the proper rectangles. The g_crop can be kept, and a
> comment
> should be added that it returns the wrong thing, but that that is
> needed for
> backwards compat.
> 
> The gstreamer code should use g/s_selection if available. It should
> check how it
> is using g/s_crop for video output devices today and remember that
> for output
> devices g/s_crop is really g/s_compose, except for the exynos
> drivers.

This is already the case. There is other non-mainline driver that do
like exynos (I have been told).

https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys?id=7
4f020fd2f1dc645efe35a7ba1f951f9c5ee7c4c

> 
> It's why I recommend the selection API since it doesn't have these
> problems.
> 
> I think I should do another push towards implementing the selection
> API in all
> drivers. There aren't many left.
> 
> Regards,
> 
>   Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rtl2832_sdr and /dev/swradio0

[Nicolas Dufresne]

Le mardi 31 janvier 2017 à 17:50 +, Russel Winder a écrit :
> Hi,
> 
> Is anyone actively working on the rtl2832_sdr driver?
> 
> I am particularly interested in anyone who has code for turning the
> byte stream from /dev/swradio0 into an ETI stream. Or failing that
> getting enough data about the API for using /dev/swradio0 so as to
> write a byte sequence to ETI driver based on the dab2eti program in
> DABtool (which uses the librtlsdr mechanism instead of the
> /dev/swradio0 one).

This device works like any V4L2 driver, with the differences explained
here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/dev-sdr.html

For the rest I'm no expert. You can probably port dab2eti to use this
interface instead of librtlsdr and keep the rest. You may be able to
skip an fft if your driver supports it, otherwise you'll get RU12, that
you'll probably have to convert to floats before passing to the rest of
the processing code.

regards,
Nicolas

signature.asc
Description: This is a digitally signed message part

Re: [PATCH v5 3/3] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

[Nicolas Dufresne]

Le mercredi 22 février 2017 à 10:10 -0300, Thibault Saunier a écrit :
> Hello,
> 
> On 02/22/2017 06:29 AM, Andrzej Hajda wrote:
> > On 21.02.2017 20:20, Thibault Saunier wrote:
> > > It is required by the standard that the field order is set by the
> > > driver.
> > > 
> > > Signed-off-by: Thibault Saunier  > > >
> > > 
> > > ---
> > > 
> > > Changes in v5:
> > > - Just adapt the field and never error out.
> > > 
> > > Changes in v4: None
> > > Changes in v3:
> > > - Do not check values in the g_fmt functions as Andrzej explained
> > > in previous review
> > > 
> > > Changes in v2:
> > > - Fix a silly build error that slipped in while rebasing the
> > > patches
> > > 
> > >   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > index 0976c3e0a5ce..44ed2afe0780 100644
> > > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > > @@ -386,6 +386,9 @@ static int vidioc_try_fmt(struct file *file,
> > > void *priv, struct v4l2_format *f)
> > >   struct v4l2_pix_format_mplane *pix_mp = >fmt.pix_mp;
> > >   struct s5p_mfc_fmt *fmt;
> > >   
> > > + if (f->fmt.pix.field == V4L2_FIELD_ANY)
> > > + f->fmt.pix.field = V4L2_FIELD_NONE;
> > > +
> > 
> > In previous version the only supported field type was NONE, here
> > you
> > support everything.
> > If the only supported format is none you should set 'field'
> > unconditionally to NONE, nothing more.
> 
> Afaict we  support 2 things:
> 
>    1. NONE
>    2. INTERLACE
> 
> Until now we were not checking what was supported or not and
> basically 
> ignoring that info, this patch
> keeps the old behaviour making sure to be compliant.
> 
> I had a doubt and was pondering doing:
> 
> ``` diff
> 
> + if (f->fmt.pix.field != V4L2_FIELD_INTERLACED)
> + f->fmt.pix.field = V4L2_FIELD_NONE;
> +
> 

This looks better to me.

> ```
> 
> instead, it is probably more correct, do you think it is what should
> be 
> done here?
> 
> Regards,
> 
> Thibault
> 
> > 
> > Regards
> > Andrzej
> > 
> > >   mfc_debug(2, "Type is %d\n", f->type);
> > >   if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > >   fmt = find_format(f, MFC_FMT_DEC);
> 
> 

signature.asc
Description: This is a digitally signed message part

Re: [PATCH 00/15] Exynos MFC v6+ - remove the need for the reserved memory

[Nicolas Dufresne]

Le mardi 14 février 2017 à 08:51 +0100, Marek Szyprowski a écrit :
> Dear All,
> 
> This patchset is a result of my work on enabling full support for MFC device
> (multimedia codec) on Exynos 5433 on ARM64 architecture. Initially I thought
> that to let it working on ARM64 architecture with IOMMU, I would need to
> solve the issue related to the fact that s5p-mfc driver was depending on the
> first-fit allocation method in the DMA-mapping / IOMMU glue code (ARM64 use
> different algorithm). It turned out, that there is a much simpler way.
> 
> During my research I found that some of the requirements for the memory
> buffers for MFC v6+ devices were blindly copied from the previous
> hardware (v5) version and simply turned out to be excessive. It turned out
> that there is no strict requirement for ALL buffers to be allocated on
> the higher addresses than the firmware base. This requirement is true only
> for the device and per-context buffers. All video data buffers can be
> allocated anywhere for all MFC v6+ versions. This heavily simplifies
> memory management in the driver.
> 
> Such relaxed requirements for the memory buffers can be easily fulfilled
> by allocating firmware, device and per-context buffers from the probe-time
> preallocated larger buffer. There is no need to create special reserved
> memory regions. The only case, when those memory regions are needed is an
> oldest Exynos series - Exynos4210 or Exyno4412, which both have MFC v5
> hardware, and only when IOMMU is disabled.
> 
> This patchset has been tested on Odroid U3 (Exynos4412 with MFC v5), Google
> Snow (Exynos5250 with MFC v6), Odroid XU3 (Exynos5422 with MFC v8) and
> TM2 (Exynos5433 with MFC v8, ARM64) boards.
> 
> To get it working on TM2/Exynos5433 with IOMMU enabled, the 'architectural
> clock gating' in SYSMMU has to be disabled. Fixing this will be handled
> separately. As a temporary solution, one need to clear CFG_ACGEN bit in
> REG_MMU_CFG of the SYSMMU, see __sysmmu_init_config function in
> drivers/iommu/exynos-iommu.c.
> 
> Patches are based on linux-next from 9th February 2017 with "media:
> s5p-mfc: Fix initialization of internal structures" patch applied:
> https://patchwork.linuxtv.org/patch/39198/
> 
> I've tried to split changes into small pieces to make it easier to review
> the code. I've also did a bit of cleanup while touching the driver.
> 
> Best regards
> Marek Szyprowski
> Samsung R Institute Poland
> 
> 
> Patch summary:
> 
> Marek Szyprowski (15):
>   media: s5p-mfc: Remove unused structures and dead code
>   media: s5p-mfc: Use generic of_device_get_match_data helper
>   media: s5p-mfc: Replace mem_dev_* entries with an array
>   media: s5p-mfc: Replace bank1/bank2 entries with an array
>   media: s5p-mfc: Simplify alloc/release private buffer functions
>   media: s5p-mfc: Move setting DMA max segmetn size to DMA configure
> function

Just notice this small typo "segmetn", associated patch will need
update too.

>   media: s5p-mfc: Put firmware to private buffer structure
>   media: s5p-mfc: Move firmware allocation to DMA configure function
>   media: s5p-mfc: Allocate firmware with internal private buffer alloc
> function
>   media: s5p-mfc: Reduce firmware buffer size for MFC v6+ variants
>   media: s5p-mfc: Split variant DMA memory configuration into separate
> functions
>   media: s5p-mfc: Add support for probe-time preallocated block based
> allocator
>   media: s5p-mfc: Remove special configuration of IOMMU domain
>   media: s5p-mfc: Use preallocated block allocator always for MFC v6+
>   ARM: dts: exynos: Remove MFC reserved buffers
> 
>  .../devicetree/bindings/media/s5p-mfc.txt  |   2 +-
>  arch/arm/boot/dts/exynos5250-arndale.dts   |   1 -
>  arch/arm/boot/dts/exynos5250-smdk5250.dts  |   1 -
>  arch/arm/boot/dts/exynos5250-spring.dts|   1 -
>  arch/arm/boot/dts/exynos5420-arndale-octa.dts  |   1 -
>  arch/arm/boot/dts/exynos5420-peach-pit.dts |   1 -
>  arch/arm/boot/dts/exynos5420-smdk5420.dts  |   1 -
>  arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi |   1 -
>  arch/arm/boot/dts/exynos5800-peach-pi.dts  |   1 -
>  drivers/media/platform/s5p-mfc/regs-mfc-v6.h   |   2 +-
>  drivers/media/platform/s5p-mfc/regs-mfc-v7.h   |   2 +-
>  drivers/media/platform/s5p-mfc/regs-mfc-v8.h   |   2 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc.c   | 210 
> +
>  drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v5.c|   2 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h|  43 ++---
>  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c  |  71 +++
>  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.h  |   1 -
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c   |   8 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c   |  10 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc_iommu.h |  51 +
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr.c   |  65 +--
>  

Re: [PATCH v5 1/3] [media] exynos-gsc: Use user configured colorspace if provided

[Nicolas Dufresne]

Le mercredi 22 février 2017 à 15:57 -0300, Thibault Saunier a écrit :
> On 02/22/2017 03:06 PM, Hans Verkuil wrote:
> > 
> > On 02/22/2017 05:05 AM, Thibault Saunier wrote:
> > > Hello,
> > > 
> > > 
> > > On 02/21/2017 11:19 PM, Hans Verkuil wrote:
> > > > On 02/21/2017 11:20 AM, Thibault Saunier wrote:
> > > > > Use colorspace provided by the user as we are only doing
> > > > > scaling and
> > > > > color encoding conversion, we won't be able to transform the
> > > > > colorspace
> > > > > itself and the colorspace won't mater in that operation.
> > > > > 
> > > > > Also always use output colorspace on the capture side.
> > > > > 
> > > > > Start using 576p as a threashold to compute the colorspace.
> > > > > The media documentation says that the
> > > > > V4L2_COLORSPACE_SMPTE170M colorspace
> > > > > should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV.
> > > > > But drivers
> > > > > don't agree on the display resolution that should be used as
> > > > > a threshold.
> > > > > 
> > > > >   From EIA CEA 861B about colorimetry for various
> > > > > resolutions:
> > > > > 
> > > > > - 5.1 480p, 480i, 576p, 576i, 240p, and 288p
> > > > >   The color space used by the 480-line, 576-line, 240-
> > > > > line, and 288-line
> > > > >   formats will likely be based on SMPTE 170M [1].
> > > > > - 5.2 1080i, 1080p, and 720p
> > > > >   The color space used by the high definition formats
> > > > > will likely be
> > > > >   based on ITU-R BT.709-4
> > > > > 
> > > > > This indicates that in the case that userspace does not
> > > > > specify what
> > > > > colorspace should be used, we should use 576p  as a threshold
> > > > > to set
> > > > > V4L2_COLORSPACE_REC709 instead of V4L2_COLORSPACE_SMPTE170M.
> > > > > Even if it is
> > > > > only 'likely' and not a requirement it is the best guess we
> > > > > can make.
> > > > > 
> > > > > The stream should have been encoded with the information and
> > > > > userspace
> > > > > has to pass it to the driver if it is not the case, otherwise
> > > > > we won't be
> > > > > able to handle it properly anyhow.
> > > > > 
> > > > > Also, check for the resolution in G_FMT instead
> > > > > unconditionally setting
> > > > > the V4L2_COLORSPACE_REC709 colorspace.
> > > > > 
> > > > > Signed-off-by: Javier Martinez Canillas  > > > > om>
> > > > > Signed-off-by: Thibault Saunier  > > > > .com>
> > > > > Reviewed-by: Andrzej Hajda 
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v5:
> > > > > - Squash commit to always use output colorspace on the
> > > > > capture side
> > > > > inside this one
> > > > > - Fix typo in commit message
> > > > > 
> > > > > Changes in v4:
> > > > > - Reword commit message to better back our assumptions on
> > > > > specifications
> > > > > 
> > > > > Changes in v3:
> > > > > - Do not check values in the g_fmt functions as Andrzej
> > > > > explained in previous review
> > > > > - Added 'Reviewed-by: Andrzej Hajda '
> > > > > 
> > > > > Changes in v2: None
> > > > > 
> > > > >    drivers/media/platform/exynos-gsc/gsc-core.c | 20
> > > > > +++-
> > > > >    drivers/media/platform/exynos-gsc/gsc-core.h |  1 +
> > > > >    2 files changed, 16 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c
> > > > > b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > > > index 59a634201830..772599de8c13 100644
> > > > > --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> > > > > +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> > > > > @@ -454,6 +454,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx
> > > > > *ctx, struct v4l2_format *f)
> > > > >    } else {
> > > > >    min_w = variant->pix_min->target_rot_dis_w;
> > > > >    min_h = variant->pix_min->target_rot_dis_h;
> > > > > +pix_mp->colorspace = ctx->out_colorspace;
> > > > >    }
> > > > >  pr_debug("mod_x: %d, mod_y: %d, max_w: %d, max_h =
> > > > > %d",
> > > > > @@ -472,10 +473,15 @@ int gsc_try_fmt_mplane(struct gsc_ctx
> > > > > *ctx, struct v4l2_format *f)
> > > > >  pix_mp->num_planes = fmt->num_planes;
> > > > >    -if (pix_mp->width >= 1280) /* HD */
> > > > > -pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> > > > > -else /* SD */
> > > > > -pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > > > +if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) {
> > > > > +if (pix_mp->width > 720 && pix_mp->height > 576) /*
> > > > > HD */
> > > > 
> > > > I'd use || instead of && here. Ditto for the next patch.
> > > > 
> > > > > +pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> > > > > +else /* SD */
> > > > > +pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > > > +}
> > > > 
> > > > Are you sure this is in fact how it is used? If the source of
> > > > the video
> > > > is the sensor 

Re: [PATCH v3 1/3] [media] v4l: add parsed MPEG-2 support

[Nicolas Dufresne]

Le jeudi 09 février 2017 à 18:07 +0100, Hugues Fruchet a écrit :
> Add "parsed MPEG-2" pixel format & related controls
> needed by stateless video decoders.
> In order to decode the video bitstream chunk provided
> by user on output queue, stateless decoders require
> also some extra data resulting from this video bitstream
> chunk parsing.
> Those parsed extra data have to be set by user through
> control framework using the dedicated mpeg video extended
> controls introduced in this patchset.
> 
> Signed-off-by: Hugues Fruchet 
> ---
>  Documentation/media/uapi/v4l/extended-controls.rst | 363
> +
>  Documentation/media/uapi/v4l/pixfmt-013.rst|  10 +
>  drivers/media/v4l2-core/v4l2-ctrls.c   |  53 +++
>  drivers/media/v4l2-core/v4l2-ioctl.c   |   2 +
>  include/uapi/linux/v4l2-controls.h |  86 +
>  include/uapi/linux/videodev2.h |   8 +
>  6 files changed, 522 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst
> b/Documentation/media/uapi/v4l/extended-controls.rst
> index abb1057..cd1a8d6 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -1827,6 +1827,369 @@ enum
> v4l2_mpeg_cx2341x_video_median_filter_type -
>  not insert, 1 = insert packets.
>  
>  
> +MPEG-2 Parsed Control Reference
> +-
> +
> +The MPEG-2 parsed decoding controls are needed by stateless video
> decoders.
> +Those decoders expose :ref:`Compressed formats ` 
> :ref:`V4L2_PIX_FMT_MPEG1_PARSED` or
> :ref:`V4L2_PIX_FMT_MPEG2_PARSED`.
> +In order to decode the video bitstream chunk provided by user on
> output queue,
> +stateless decoders require also some extra data resulting from this
> video
> +bitstream chunk parsing. Those parsed extra data have to be set by
> user
> +through control framework using the mpeg video extended controls
> defined
> +in this section. Those controls have been defined based on MPEG-2
> standard
> +ISO/IEC 13818-2, and so derive directly from the MPEG-2 video
> bitstream syntax
> +including how it is coded inside bitstream (enumeration values for
> ex.).
> +
> +MPEG-2 Parsed Control IDs
> +^^^
> +
> +.. _mpeg2-parsed-control-id:
> +
> +``V4L2_CID_MPEG_VIDEO_MPEG2_SEQ_HDR``
> +(enum)
> +
> +.. tabularcolumns:: |p{4.0cm}|p{2.5cm}|p{11.0cm}|
> +
> +.. c:type:: v4l2_mpeg_video_mpeg2_seq_hdr
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_mpeg_video_mpeg2_seq_hdr
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 1 2
> +
> +* - __u16
> +  - ``width``
> +  - Video width in pixels.
> +* - __u16
> +  - ``height``
> +  - Video height in pixels.
> +* - __u8
> +  - ``aspect_ratio_info``
> +  - Aspect ratio code as in the bitstream (1: 1:1 square pixels,
> +2: 4:3 display, 3: 16:9 display, 4: 2.21:1 display)
> +* - __u8
> +  - ``framerate code``
> +  - Framerate code as in the bitstream
> +(1: 24000/1001.0 '23.976 fps, 2: 24.0, 3: 25.0,
> +4: 3/1001.0 '29.97, 5: 30.0, 6: 50.0, 7: 6/1001.0,
> +8: 60.0)
> +* - __u32
> +  - ``bitrate_value``
> +  - Bitrate value as in the bitstream, expressed in 400bps unit
> +* - __u16
> +  - ``vbv_buffer_size``
> +  -  Video Buffering Verifier size, expressed in 16KBytes unit.
> +* - __u8
> +  - ``constrained_parameters_flag``
> +  - Set to 1 if this bitstream uses constrained parameters.
> +* - __u8
> +  - ``load_intra_quantiser_matrix``
> +  - If set to 1, ``intra_quantiser_matrix`` table is to be used
> for
> +decoding.
> +* - __u8
> +  - ``load_non_intra_quantiser_matrix``
> +  - If set to 1, ``non_intra_quantiser_matrix`` table is to be
> used for
> +decoding.
> +* - __u8
> +  - ``intra_quantiser_matrix[64]``
> +  - Intra quantization table, in zig-zag scan order.
> +* - __u8
> +  - ``non_intra_quantiser_matrix[64]``
> +  - Non-intra quantization table, in zig-zag scan order.
> +* - __u32
> +  - ``par_w``
> +  - Pixel aspect ratio width in pixels.
> +* - __u32
> +  - ``par_h``
> +  - Pixel aspect ratio height in pixels.
> +* - __u32
> +  - ``fps_n``
> +  - Framerate nominator.
> +* - __u32
> +  - ``fps_d``
> +  - Framerate denominator.
> +* - __u32
> +  - ``bitrate``
> +  - Bitrate in bps if constant bitrate, 0 otherwise.
> +* - :cspan:`2`
> +
> +
> +``V4L2_CID_MPEG_VIDEO_MPEG2_SEQ_EXT``
> +(enum)
> +
> +.. tabularcolumns:: |p{4.0cm}|p{2.5cm}|p{11.0cm}|
> +
> +.. c:type:: v4l2_mpeg_video_mpeg2_seq_ext
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_mpeg_video_mpeg2_seq_ext
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 1 2
> +
> +* - __u8
> +  - ``profile``
> + 

Re: [PATCH] media: fix s5p_mfc_set_dec_frame_buffer_v6() to print buf size in hex

[Nicolas Dufresne]

Le jeudi 09 février 2017 à 15:10 -0700, Shuah Khan a écrit :
> Fix s5p_mfc_set_dec_frame_buffer_v6() to print buffer size in hex to
> be
> consistent with the rest of the messages in the routine.

Short and long descriptions are miss-leading. This patch will print the
buffer pointer as hex and keep the size as decimal. I think the patch
correctly improves consistency, the comment should match the code.

> 
> Signed-off-by: Shuah Khan 
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> index d6f207e..fc45980 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -497,7 +497,7 @@ static int s5p_mfc_set_dec_frame_buffer_v6(struct
> s5p_mfc_ctx *ctx)
>   }
>   }
>  
> - mfc_debug(2, "Buf1: %zu, buf_size1: %d (frames %d)\n",
> + mfc_debug(2, "Buf1: %zx, buf_size1: %d (frames %d)\n",
>   buf_addr1, buf_size1, ctx->total_dpb_count);
>   if (buf_size1 < 0) {
>   mfc_debug(2, "Not enough memory has been
> allocated.\n");

signature.asc
Description: This is a digitally signed message part

Re: Request API: stateless VPU: the buffer mechanism and DPB management

[Nicolas Dufresne]

Le mardi 17 janvier 2017 à 20:46 +0800, herman.c...@rock-chips.com a
écrit :
> If we move parser or part of DPB management mechanism into kernel we
> will face a issue as follows:
> One customer requires dpb management do a flush when stream occurs in
> order to keep output frame clean.
> While another one requires output frame with error to keep output
> frame smooth.
> And when only one field has a error one customer wants to do a simple
> field copy to recover.

The driver should send all frames and simply mark the corrupted frames
using V4L2_BUF_FLAG_ERROR. This way, the userspace can then make their
own decision. It is also important to keep track and cleanup the
buffers meta's (which are application specific). If the driver silently
drops frame, it makes that management much harder.

About flushing and draining operation, they are respectively signalled
to the driver using STREAMOFF and CMD_STOP.

> 
> These are some operation related to strategy rather then mechanism.
> I think it is not a good idea to bring such kind of flexible process
> to kernel driver.
> 
> So here is the ultimate challenge that how to reasonably move the
> parser and flexible process
> which is encapsuled in firmware to a userspace - kernel stateless
> driver model.

Moving the parsers in the kernel (on the main CPU) is not acceptable.
This is too much of a security threat. Userspace should parse the data
into structures, doing any validation required before end.

My main question and that should have an impact decision, is if those
structures can be made generic. PDB handling is not that trivial (my
reference is VAAPI here, maybe they are doing it wrong) and with driver
specific structures, we would have this code copy-pasted over and over.
So with driver specific structures, it's probably better to keep all
the parsing and reordering logic outside (hence together).

That remains, that some driver will deal with reordering on the
firmware side (even the if they don't parse), hence we need to take
this into consideration.

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

Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

[Nicolas Dufresne]

Le lundi 18 juillet 2016 à 10:34 +0200, Hans Verkuil a écrit :
> On 07/15/2016 06:26 PM, Javier Martinez Canillas wrote:
> > The buffer planes' dma-buf are currently mapped when buffers are queued
> > from userspace but it's more appropriate to do the mapping when buffers
> > are queued in the driver since that's when the actual DMA operation are
> > going to happen.
> 
> Does this solve anything? Once the DMA has started the behavior is the same
> as before (QBUF maps the dmabuf), only while the DMA engine hasn't started
> yet are the QBUF calls just accepted and the mapping takes place when the
> DMA is kickstarted. This makes QBUF behave inconsistently.

The reflection (yes it's just a reflection) is that userspace won't
have to wait for the map before it can go back on retrieving and
submitting the next buffer. I think this could help userspace ability
to fill in the queue on time, without having to introduce threads to
protect against long kernel operations. Basically, it may improve
parallelism between userspace and kernel.

> 
> You don't describe here WHY this change is needed.
> 
> I'm not sure I agree with the TODO, and even if I did, I'm not sure I agree
> with this solution. Since queuing the buffer to the driver is not the same
> as 'just before the DMA', since there may be many buffers queued up in the
> driver and you don't know in vb2 when the buffer is at the 'just before the 
> DMA'
> stage.
> 
> Regards,
> 
>   Hans
> 
> > 
> > Suggested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> > 
> > ---
> > 
> > Hello,
> > 
> > A side effect of this change is that if the dmabuf map fails for
> > some
> > reasons (i.e: a driver using the DMA contig memory allocator but
> > CMA
> > not being enabled), the fail will no longer happen on VIDIOC_QBUF
> > but
> > later (i.e: in VIDIOC_STREAMON).
> > 
> > I don't know if that's an issue though but I think is worth
> > mentioning.
> > 
> > Best regards,
> > Javier
> > 
> >  drivers/media/v4l2-core/videobuf2-core.c | 88
> > 
> >  1 file changed, 54 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index ca8ffeb56d72..3fdf882bf279 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
> >  })
> >  
> >  static void __vb2_queue_cancel(struct vb2_queue *q);
> > -static void __enqueue_in_driver(struct vb2_buffer *vb);
> > +static int __enqueue_in_driver(struct vb2_buffer *vb);
> >  
> >  /**
> >   * __vb2_buf_mem_alloc() - allocate video memory for the given
> > buffer
> > @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer
> > *vb, const void *pb)
> >     vb->planes[plane].mem_priv = mem_priv;
> >     }
> >  
> > -   /* TODO: This pins the buffer(s)
> > with  dma_buf_map_attachment()).. but
> > -    * really we want to do this just before the DMA, not
> > while queueing
> > -    * the buffer(s)..
> > -    */
> > -   for (plane = 0; plane < vb->num_planes; ++plane) {
> > -   ret = call_memop(vb, map_dmabuf, vb-
> > >planes[plane].mem_priv);
> > -   if (ret) {
> > -   dprintk(1, "failed to map dmabuf for plane
> > %d\n",
> > -   plane);
> > -   goto err;
> > -   }
> > -   vb->planes[plane].dbuf_mapped = 1;
> > -   }
> > -
> >     /*
> >      * Now that everything is in order, copy relevant
> > information
> >      * provided by userspace.
> > @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer
> > *vb, const void *pb)
> >     vb->planes[plane].data_offset =
> > planes[plane].data_offset;
> >     }
> >  
> > -   if (reacquired) {
> > -   /*
> > -    * Call driver-specific initialization on the
> > newly acquired buffer,
> > -    * if provided.
> > -    */
> > -   ret = call_vb_qop(vb, buf_init, vb);
> > +   return 0;
> > +err:
> > +   /* In case of errors, release planes that were already
> > acquired */
> > +   __vb2_buf_dmabuf_put(vb);
> > +
> > +   return ret;
> > +}
> > +
> > +/**
> >

Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

[Nicolas Dufresne]

Le lundi 18 juillet 2016 à 12:27 +0200, Marek Szyprowski a écrit :
> Hello,
> 
> 
> On 2016-07-15 18:26, Javier Martinez Canillas wrote:
> > The buffer planes' dma-buf are currently mapped when buffers are queued
> > from userspace but it's more appropriate to do the mapping when buffers
> > are queued in the driver since that's when the actual DMA operation are
> > going to happen.
> > 
> > Suggested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> Sorry, but I don't get why such change is really needed. If possible it is
> better to report errors to userspace as early as possible, not postpone them
> to the moment, when they cannot be easily debugged.

Postponing errors is not the goal. It's an unwanted side effect of this
proposed patch (should have been marque RFQ, as Javier corrected). A
correct solution would figure-out the error without paying the cost of
mapping the memory (which is often expensive).

> 
> > 
> > ---
> > 
> > Hello,
> > 
> > A side effect of this change is that if the dmabuf map fails for
> > some
> > reasons (i.e: a driver using the DMA contig memory allocator but
> > CMA
> > not being enabled), the fail will no longer happen on VIDIOC_QBUF
> > but
> > later (i.e: in VIDIOC_STREAMON).
> > 
> > I don't know if that's an issue though but I think is worth
> > mentioning.
> > 
> > Best regards,
> > Javier
> > 
> >   drivers/media/v4l2-core/videobuf2-core.c | 88
> > 
> >   1 file changed, 54 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index ca8ffeb56d72..3fdf882bf279 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -186,7 +186,7 @@ module_param(debug, int, 0644);
> >   })
> >   
> >   static void __vb2_queue_cancel(struct vb2_queue *q);
> > -static void __enqueue_in_driver(struct vb2_buffer *vb);
> > +static int __enqueue_in_driver(struct vb2_buffer *vb);
> >   
> >   /**
> >    * __vb2_buf_mem_alloc() - allocate video memory for the given
> > buffer
> > @@ -1271,20 +1271,6 @@ static int __qbuf_dmabuf(struct vb2_buffer
> > *vb, const void *pb)
> >     vb->planes[plane].mem_priv = mem_priv;
> >     }
> >   
> > -   /* TODO: This pins the buffer(s)
> > with  dma_buf_map_attachment()).. but
> > -    * really we want to do this just before the DMA, not
> > while queueing
> > -    * the buffer(s)..
> > -    */
> > -   for (plane = 0; plane < vb->num_planes; ++plane) {
> > -   ret = call_memop(vb, map_dmabuf, vb-
> > >planes[plane].mem_priv);
> > -   if (ret) {
> > -   dprintk(1, "failed to map dmabuf for plane
> > %d\n",
> > -   plane);
> > -   goto err;
> > -   }
> > -   vb->planes[plane].dbuf_mapped = 1;
> > -   }
> > -
> >     /*
> >      * Now that everything is in order, copy relevant
> > information
> >      * provided by userspace.
> > @@ -1296,51 +1282,79 @@ static int __qbuf_dmabuf(struct vb2_buffer
> > *vb, const void *pb)
> >     vb->planes[plane].data_offset =
> > planes[plane].data_offset;
> >     }
> >   
> > -   if (reacquired) {
> > -   /*
> > -    * Call driver-specific initialization on the
> > newly acquired buffer,
> > -    * if provided.
> > -    */
> > -   ret = call_vb_qop(vb, buf_init, vb);
> > +   return 0;
> > +err:
> > +   /* In case of errors, release planes that were already
> > acquired */
> > +   __vb2_buf_dmabuf_put(vb);
> > +
> > +   return ret;
> > +}
> > +
> > +/**
> > + * __buf_map_dmabuf() - map dmabuf for buffer planes
> > + */
> > +static int __buf_map_dmabuf(struct vb2_buffer *vb)
> > +{
> > +   int ret;
> > +   unsigned int plane;
> > +
> > +   for (plane = 0; plane < vb->num_planes; ++plane) {
> > +   ret = call_memop(vb, map_dmabuf, vb-
> > >planes[plane].mem_priv);
> >     if (ret) {
> > -   dprintk(1, "buffer initialization
> > failed\n");
> > -   goto err;
> > +   dprintk(1, "failed to map dmabuf for plane
> > %d\n",
&g

Re: [PATCH] [media] vb2: map dmabuf for planes on driver queue instead of vidioc_qbuf

[Nicolas Dufresne]

Le mercredi 20 juillet 2016 à 16:20 +0300, Sakari Ailus a écrit :
> Hi Javier,
> 
> On Fri, Jul 15, 2016 at 12:26:06PM -0400, Javier Martinez Canillas
> wrote:
> > The buffer planes' dma-buf are currently mapped when buffers are queued
> > from userspace but it's more appropriate to do the mapping when buffers
> > are queued in the driver since that's when the actual DMA operation are
> > going to happen.
> > 
> > Suggested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> > 
> > ---
> > 
> > Hello,
> > 
> > A side effect of this change is that if the dmabuf map fails for some
> > reasons (i.e: a driver using the DMA contig memory allocator but CMA
> > not being enabled), the fail will no longer happen on VIDIOC_QBUF but
> > later (i.e: in VIDIOC_STREAMON).
> > 
> > I don't know if that's an issue though but I think is worth mentioning.
> 
> I have the same question has Hans --- why?
> 
> I rather think we should keep the buffers mapped all the time. That'd
> require a bit of extra from the DMA-BUF framework I suppose, to support
> streaming mappings.
> 
> The reason for that is performance. If you're passing the buffer between a
> couple of hardware devices, there's no need to map and unmap it every time
> the buffer is accessed by the said devices. That'd avoid an unnecessary
> cache flush as well, something that tends to be quite expensive. On a PC
> with resolutions typically used on webcams that might not really matter. But
> if you have an embedded system with a relatively modest 10 MP camera sensor,
> it's one of the first things you'll notice if you check where the CPU time
> is being spent.

That is very interesting since the initial discussion started from the
idea of adding an implicit fence wait to the map operation. This way we
could have a dma-buf fence attached without having to modify the
drivers to support it. Buffer handles could be dispatched before there
is any data in it. Though, if we keep it mapped, I believe this idea is
simply incompatible and fences should remain explicit for extra
flexibility.

signature.asc
Description: This is a digitally signed message part