RE: [PATCH v2] [media] s5p-mfc: Modify encoder buffer alloc sequence
Hi,
From: Arun Kumar K [mailto:arun...@samsung.com]
Sent: Tuesday, March 26, 2013 8:28 AM
MFC v6 needs minimum number of output buffers to be queued for encoder
depending on the stream type and profile.
For achieving this the sequence for allocating buffers at the encoder
is modified similar to that of decoder.
The new sequence is as follows:
1) Set format on CAPTURE plane
2) REQBUF on CAPTURE
3) QBUFS and STREAMON on CAPTURE
4) G_CTRL to get minimum buffers for OUTPUT plane
5) REQBUF on OUTPUT with the minimum buffers given by driver
I don't like the idea of changing the encoding sequence. What if the old
applications rely on a particular sequence?
I see the problem you are addressing, but let's explore other options.
MFC v6 sets the minimum number of buffers needed after the header is
generated, v5 did not provide such information at all.
Also using the variables dpb_count and state HEAD_PARSED seems odd. I guess
that you did reuse the existing variable and state. But the naming used now
is definitely misleading. DPB stands for decoded picture buffer.
I suggest adding a new variable epb_count, or changing dpb_count to pb_count
everywhere would be a good idea. Actually I like the latter more. In case of
HEAD_PARSED I suggest adding a HEAD_PRODUCED state.
This also fixes the crash happeninig during multi instance encoder-
decoder simultaneous run due to memory allocation happening from
interrupt context.
Could you explain this problem more? What was the reason and how did you fix
it?
Also a small inline comment/question below.
Signed-off-by: Arun Kumar K arun...@samsung.com
---
Changes from v1
- Corrected the commit message as pointed out by John Sheu.
http://www.spinics.net/lists/linux-media/msg61477.html
---
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 98
+++
drivers/media/platform/s5p-mfc/s5p_mfc_opr.h|1 +
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |7 ++
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 95 +--
---
4 files changed, 147 insertions(+), 54 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4f6b553..46ca986 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -557,6 +557,16 @@ static struct mfc_control controls[] = {
.step = 1,
.default_value = 0,
},
+ {
+ .id = V4L2_CID_MIN_BUFFERS_FOR_OUTPUT,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ .name = Minimum number of output bufs,
+ .minimum = 1,
+ .maximum = 32,
+ .step = 1,
+ .default_value = 1,
+ .is_volatile = 1,
+ },
};
#define NUM_CTRLS ARRAY_SIZE(controls)
@@ -661,18 +671,17 @@ static int enc_post_seq_start(struct s5p_mfc_ctx
*ctx)
vb2_buffer_done(dst_mb-b, VB2_BUF_STATE_DONE);
spin_unlock_irqrestore(dev-irqlock, flags);
}
- if (IS_MFCV6(dev)) {
- ctx-state = MFCINST_HEAD_PARSED; /* for INIT_BUFFER cmd */
- } else {
+
+ if (!IS_MFCV6(dev)) {
ctx-state = MFCINST_RUNNING;
if (s5p_mfc_ctx_ready(ctx))
set_work_bit_irqsave(ctx);
s5p_mfc_hw_call(dev-mfc_ops, try_run, dev);
- }
-
- if (IS_MFCV6(dev))
+ } else {
ctx-dpb_count = s5p_mfc_hw_call(dev-mfc_ops,
get_enc_dpb_count, dev);
+ ctx-state = MFCINST_HEAD_PARSED;
+ }
return 0;
}
@@ -1055,15 +1064,13 @@ static int vidioc_reqbufs(struct file *file,
void *priv,
}
ctx-capture_state = QUEUE_BUFS_REQUESTED;
- if (!IS_MFCV6(dev)) {
- ret = s5p_mfc_hw_call(ctx-dev-mfc_ops,
- alloc_codec_buffers, ctx);
- if (ret) {
- mfc_err(Failed to allocate encoding
buffers\n);
- reqbufs-count = 0;
- ret = vb2_reqbufs(ctx-vq_dst, reqbufs);
- return -ENOMEM;
- }
+ ret = s5p_mfc_hw_call(ctx-dev-mfc_ops,
+ alloc_codec_buffers, ctx);
+ if (ret) {
+ mfc_err(Failed to allocate encoding buffers\n);
+ reqbufs-count = 0;
+ ret = vb2_reqbufs(ctx-vq_dst, reqbufs);
+ return -ENOMEM;
}
} else if (reqbufs-type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
if (ctx-output_state != QUEUE_FREE) { @@ -1071,12 +1078,35
@@ static int vidioc_reqbufs(struct file *file, void *priv,
ctx-output_state);
return -EINVAL;
Re: [PATCH v2] [media] s5p-mfc: Modify encoder buffer alloc sequence
Hi Kamil,
Thank you for the review.
Please find my response inline.
On Wed, Mar 27, 2013 at 4:54 PM, Kamil Debski k.deb...@samsung.com wrote:
Hi,
From: Arun Kumar K [mailto:arun...@samsung.com]
Sent: Tuesday, March 26, 2013 8:28 AM
MFC v6 needs minimum number of output buffers to be queued for encoder
depending on the stream type and profile.
For achieving this the sequence for allocating buffers at the encoder
is modified similar to that of decoder.
The new sequence is as follows:
1) Set format on CAPTURE plane
2) REQBUF on CAPTURE
3) QBUFS and STREAMON on CAPTURE
4) G_CTRL to get minimum buffers for OUTPUT plane
5) REQBUF on OUTPUT with the minimum buffers given by driver
I don't like the idea of changing the encoding sequence. What if the old
applications rely on a particular sequence?
I see the problem you are addressing, but let's explore other options.
MFC v6 sets the minimum number of buffers needed after the header is
generated, v5 did not provide such information at all.
Yes. The only other way I can see without making this sequence change is
to always keep the maximum number of buffers needed in worst case scenario.
But this is not the optimal solution. You have any other suggestion?
Also using the variables dpb_count and state HEAD_PARSED seems odd. I guess
that you did reuse the existing variable and state. But the naming used now
is definitely misleading. DPB stands for decoded picture buffer.
I suggest adding a new variable epb_count, or changing dpb_count to pb_count
everywhere would be a good idea. Actually I like the latter more. In case of
HEAD_PARSED I suggest adding a HEAD_PRODUCED state.
Ok I will make these changes. But still the encoding sequence will
remain modified.
This also fixes the crash happeninig during multi instance encoder-
decoder simultaneous run due to memory allocation happening from
interrupt context.
Could you explain this problem more? What was the reason and how did you fix
it?
Earlier case, the function s5p_mfc_run_init_enc_buffers() which allocates
encoder scratch buffer was called in the try_run during state HEAD_PARSED.
If only one instance encoder is running, the allocation happens in
reqbufs which calls
try_run. But if multi-instance encoder/decoder is running, the try_run
called from
reqbuf can return due to HW lock. This will cause the function which allocates
memory to be called from interrupt context which generated crash.
Now it is modified such as the memory allocation part happens from
reqbuf directly
and then it calls try_run which only sends the command to firmware.
Also a small inline comment/question below.
Signed-off-by: Arun Kumar K arun...@samsung.com
---
Changes from v1
- Corrected the commit message as pointed out by John Sheu.
http://www.spinics.net/lists/linux-media/msg61477.html
---
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 98
+++
drivers/media/platform/s5p-mfc/s5p_mfc_opr.h|1 +
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |7 ++
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 95 +--
---
4 files changed, 147 insertions(+), 54 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4f6b553..46ca986 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -557,6 +557,16 @@ static struct mfc_control controls[] = {
.step = 1,
.default_value = 0,
},
+ {
+ .id = V4L2_CID_MIN_BUFFERS_FOR_OUTPUT,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ .name = Minimum number of output bufs,
+ .minimum = 1,
+ .maximum = 32,
+ .step = 1,
+ .default_value = 1,
+ .is_volatile = 1,
+ },
};
#define NUM_CTRLS ARRAY_SIZE(controls)
@@ -661,18 +671,17 @@ static int enc_post_seq_start(struct s5p_mfc_ctx
*ctx)
vb2_buffer_done(dst_mb-b, VB2_BUF_STATE_DONE);
spin_unlock_irqrestore(dev-irqlock, flags);
}
- if (IS_MFCV6(dev)) {
- ctx-state = MFCINST_HEAD_PARSED; /* for INIT_BUFFER cmd */
- } else {
+
+ if (!IS_MFCV6(dev)) {
ctx-state = MFCINST_RUNNING;
if (s5p_mfc_ctx_ready(ctx))
set_work_bit_irqsave(ctx);
s5p_mfc_hw_call(dev-mfc_ops, try_run, dev);
- }
-
- if (IS_MFCV6(dev))
+ } else {
ctx-dpb_count = s5p_mfc_hw_call(dev-mfc_ops,
get_enc_dpb_count, dev);
+ ctx-state = MFCINST_HEAD_PARSED;
+ }
return 0;
}
@@ -1055,15 +1064,13 @@ static int vidioc_reqbufs(struct file *file,
void *priv,
}
ctx-capture_state = QUEUE_BUFS_REQUESTED;
- if (!IS_MFCV6(dev)) {
- ret =
