.../chips-media/wave5/wave5-vpu-enc.c | 74 +++++++++++++------ 1 file changed, 51 insertions(+), 23 deletions(-)
Wave5 encoder driver only changed states to PIC_RUN in start streaming by
making the assumption that VIDIOC STREAMON call has already been called.
In frameworks like FFMPEG, this condition has not been met when in the
start streaming function resulting in the application hanging. Therefore,
job_ready and device_run need to be extended to have support for this case.
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---
.../chips-media/wave5/wave5-vpu-enc.c | 74 +++++++++++++------
1 file changed, 51 insertions(+), 23 deletions(-)
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
index a02853d42d61..3a3b585ceb8e 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -705,6 +705,11 @@ static int wave5_vpu_enc_encoder_cmd(struct file *file, void *fh, struct v4l2_en
m2m_ctx->last_src_buf = v4l2_m2m_last_src_buf(m2m_ctx);
m2m_ctx->is_draining = true;
+
+ if (v4l2_m2m_num_dst_bufs_ready(m2m_ctx) > 0) {
+ dev_dbg(inst->dev->dev, "Forcing job run for draining\n");
+ v4l2_m2m_try_schedule(m2m_ctx);
+ }
break;
case V4L2_ENC_CMD_START:
break;
@@ -1411,6 +1416,34 @@ static int prepare_fb(struct vpu_instance *inst)
return ret;
}
+static int wave5_vpu_enc_prepare_cap_seq(struct vpu_instance *inst)
+{
+ int ret = 0;
+
+ ret = initialize_sequence(inst);
+ if (ret) {
+ dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
+ return ret;
+ }
+ ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
+ if (ret)
+ return ret;
+
+ /*
+ * The sequence must be analyzed first to calculate the proper
+ * size of the auxiliary buffers.
+ */
+ ret = prepare_fb(inst);
+ if (ret) {
+ dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
+ return ret;
+ }
+
+ ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
+
+ return ret;
+}
+
static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count)
{
struct vpu_instance *inst = vb2_get_drv_priv(q);
@@ -1453,27 +1486,8 @@ static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
if (ret)
goto return_buffers;
}
- if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {
- ret = initialize_sequence(inst);
- if (ret) {
- dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
- goto return_buffers;
- }
- ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
- if (ret)
- goto return_buffers;
- /*
- * The sequence must be analyzed first to calculate the proper
- * size of the auxiliary buffers.
- */
- ret = prepare_fb(inst);
- if (ret) {
- dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
- goto return_buffers;
- }
-
- ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
- }
+ if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming)
+ ret = wave5_vpu_enc_prepare_cap_seq(inst);
if (ret)
goto return_buffers;
@@ -1598,6 +1612,14 @@ static void wave5_vpu_enc_device_run(void *priv)
pm_runtime_resume_and_get(inst->dev->dev);
switch (inst->state) {
+ case VPU_INST_STATE_OPEN:
+ ret = wave5_vpu_enc_prepare_cap_seq(inst);
+ if (ret) {
+ dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
+ switch_state(inst, VPU_INST_STATE_STOP);
+ break;
+ }
+ fallthrough;
case VPU_INST_STATE_PIC_RUN:
ret = start_encode(inst, &fail_res);
if (ret) {
@@ -1633,6 +1655,12 @@ static int wave5_vpu_enc_job_ready(void *priv)
case VPU_INST_STATE_NONE:
dev_dbg(inst->dev->dev, "Encoder must be open to start queueing M2M jobs!\n");
return false;
+ case VPU_INST_STATE_OPEN:
+ if (wave5_vpu_both_queues_are_streaming(inst)) {
+ dev_dbg(inst->dev->dev, "Both queues have been turned on now, M2M job can occur\n");
+ return true;
+ }
+ return false;
case VPU_INST_STATE_PIC_RUN:
if (m2m_ctx->is_draining || v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
dev_dbg(inst->dev->dev, "Encoder ready for a job, state: %s\n",
@@ -1642,9 +1670,9 @@ static int wave5_vpu_enc_job_ready(void *priv)
fallthrough;
default:
dev_dbg(inst->dev->dev,
- "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready\n",
+ "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready, %d dst bufs ready\n",
state_to_str(inst->state), m2m_ctx->is_draining ? "is" : "is not",
- v4l2_m2m_num_src_bufs_ready(m2m_ctx));
+ v4l2_m2m_num_src_bufs_ready(m2m_ctx), v4l2_m2m_num_dst_bufs_ready(m2m_ctx));
break;
}
return false;
--
2.34.1Hi Brandon,
Le lundi 20 octobre 2025 à 12:33 -0500, Brandon Brnich a écrit :
> Wave5 encoder driver only changed states to PIC_RUN in start streaming by
> making the assumption that VIDIOC STREAMON call has already been called.
> In frameworks like FFMPEG, this condition has not been met when in the
> start streaming function resulting in the application hanging. Therefore,
> job_ready and device_run need to be extended to have support for this case.
I'm afraid you will have to rework that commit message in V2, I could not make
much sense out of it. See comments below, but by spliting your patch, it might
get easier to explain what you are trying to fix.
>
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
> .../chips-media/wave5/wave5-vpu-enc.c | 74 +++++++++++++------
> 1 file changed, 51 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> index a02853d42d61..3a3b585ceb8e 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> @@ -705,6 +705,11 @@ static int wave5_vpu_enc_encoder_cmd(struct file *file, void *fh, struct v4l2_en
>
> m2m_ctx->last_src_buf = v4l2_m2m_last_src_buf(m2m_ctx);
> m2m_ctx->is_draining = true;
> +
> + if (v4l2_m2m_num_dst_bufs_ready(m2m_ctx) > 0) {
Its job_ready callback and framework task to check this, I think you can go
directly to try to schedule.
> + dev_dbg(inst->dev->dev, "Forcing job run for draining\n");
> + v4l2_m2m_try_schedule(m2m_ctx);
This is fair, and the decoder does the same. Though, it has nothing to do with
the transition from OPEN -> SEQ_INIT -> PIC_RUN. Do this in its own commit with
its own explanation.
> + }
> break;
> case V4L2_ENC_CMD_START:
> break;
> @@ -1411,6 +1416,34 @@ static int prepare_fb(struct vpu_instance *inst)
> return ret;
> }
>
> +static int wave5_vpu_enc_prepare_cap_seq(struct vpu_instance *inst)
> +{
Factor-out in its own commit, with a message this is preparation work and with
no function changes. Its really hard to review code that moves around and may
have changes in it.
> + int ret = 0;
> +
> + ret = initialize_sequence(inst);
> + if (ret) {
> + dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
> + return ret;
> + }
> + ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> + if (ret)
> + return ret;
> +
> + /*
> + * The sequence must be analyzed first to calculate the proper
> + * size of the auxiliary buffers.
> + */
> + ret = prepare_fb(inst);
> + if (ret) {
> + dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> + return ret;
> + }
> +
> + ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
> +
> + return ret;
> +}
> +
> static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count)
> {
> struct vpu_instance *inst = vb2_get_drv_priv(q);
> @@ -1453,27 +1486,8 @@ static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
> if (ret)
> goto return_buffers;
> }
> - if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {
> - ret = initialize_sequence(inst);
> - if (ret) {
> - dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
> - goto return_buffers;
> - }
> - ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> - if (ret)
> - goto return_buffers;
> - /*
> - * The sequence must be analyzed first to calculate the proper
> - * size of the auxiliary buffers.
> - */
> - ret = prepare_fb(inst);
> - if (ret) {
> - dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> - goto return_buffers;
> - }
> -
> - ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
> - }
> + if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming)
> + ret = wave5_vpu_enc_prepare_cap_seq(inst);
> if (ret)
> goto return_buffers;
>
> @@ -1598,6 +1612,14 @@ static void wave5_vpu_enc_device_run(void *priv)
>
> pm_runtime_resume_and_get(inst->dev->dev);
> switch (inst->state) {
> + case VPU_INST_STATE_OPEN:
> + ret = wave5_vpu_enc_prepare_cap_seq(inst);
> + if (ret) {
> + dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
> + switch_state(inst, VPU_INST_STATE_STOP);
> + break;
> + }
> + fallthrough;
> case VPU_INST_STATE_PIC_RUN:
> ret = start_encode(inst, &fail_res);
> if (ret) {
> @@ -1633,6 +1655,12 @@ static int wave5_vpu_enc_job_ready(void *priv)
> case VPU_INST_STATE_NONE:
> dev_dbg(inst->dev->dev, "Encoder must be open to start queueing M2M jobs!\n");
> return false;
> + case VPU_INST_STATE_OPEN:
> + if (wave5_vpu_both_queues_are_streaming(inst)) {
> + dev_dbg(inst->dev->dev, "Both queues have been turned on now, M2M job can occur\n");
> + return true;
> + }
> + return false;
> case VPU_INST_STATE_PIC_RUN:
> if (m2m_ctx->is_draining || v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
> dev_dbg(inst->dev->dev, "Encoder ready for a job, state: %s\n",
> @@ -1642,9 +1670,9 @@ static int wave5_vpu_enc_job_ready(void *priv)
> fallthrough;
> default:
> dev_dbg(inst->dev->dev,
> - "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready\n",
> + "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready, %d dst bufs ready\n",
> state_to_str(inst->state), m2m_ctx->is_draining ? "is" : "is not",
> - v4l2_m2m_num_src_bufs_ready(m2m_ctx));
> + v4l2_m2m_num_src_bufs_ready(m2m_ctx), v4l2_m2m_num_dst_bufs_ready(m2m_ctx));
> break;
> }
> return false;
Perhaps its going to be clear with proper commit message, but I'm still not
clear how you can endup with both queues streaming without two calls to
wave5_vpu_enc_start_streaming(). I don't deny the condition might be broken
then, but the intent is for this code to bring the driver to PIC_RUN on the
second call.
From VPU_INST_STATE_NONE:
Case 1:
STREAMON(CAP)
- bring it to OPEN state
STREAMON(OUT)
- Initialize the sequence and prepare the FB
- Leaving with PIC_RUN state
Case 2:
STREAMON(OUT)
- no-op
STREAMON(CAP)
- To OPEN
- To INIT_SEQ
- To PIC_RUN
So in case 2, the code fails this condition:
if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {
Basically type == CAP, and vb2 won't be setting the .streaming state before this
function returns. A possible solution would be:
if (inst->state == VPU_INST_STATE_OPEN &&
(m2m_ctx->cap_q_ctx.q.streaming || type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)) {
cheers,
Nicolas
Hi Nicolas,
On 10/20/2025 1:41 PM, Nicolas Dufresne wrote:
> Hi Brandon,
>
>
> Le lundi 20 octobre 2025 à 12:33 -0500, Brandon Brnich a écrit :
>> Wave5 encoder driver only changed states to PIC_RUN in start streaming by
>> making the assumption that VIDIOC STREAMON call has already been called.
>> In frameworks like FFMPEG, this condition has not been met when in the
>> start streaming function resulting in the application hanging. Therefore,
>> job_ready and device_run need to be extended to have support for this case.
>
> I'm afraid you will have to rework that commit message in V2, I could not make
> much sense out of it. See comments below, but by spliting your patch, it might
> get easier to explain what you are trying to fix.
Understood, I provide a better explanation in next patch. I can see how
some of the below can be confusing.
>
>>
>> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
>> ---
>> .../chips-media/wave5/wave5-vpu-enc.c | 74 +++++++++++++------
>> 1 file changed, 51 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> index a02853d42d61..3a3b585ceb8e 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> @@ -705,6 +705,11 @@ static int wave5_vpu_enc_encoder_cmd(struct file *file, void *fh, struct v4l2_en
>>
>> m2m_ctx->last_src_buf = v4l2_m2m_last_src_buf(m2m_ctx);
>> m2m_ctx->is_draining = true;
>> +
>> + if (v4l2_m2m_num_dst_bufs_ready(m2m_ctx) > 0) {
>
> Its job_ready callback and framework task to check this, I think you can go
> directly to try to schedule.
>
>> + dev_dbg(inst->dev->dev, "Forcing job run for draining\n");
>> + v4l2_m2m_try_schedule(m2m_ctx);
>
> This is fair, and the decoder does the same. Though, it has nothing to do with
> the transition from OPEN -> SEQ_INIT -> PIC_RUN. Do this in its own commit with
> its own explanation.
Understood
>
>> + }
>> break;
>> case V4L2_ENC_CMD_START:
>> break;
>> @@ -1411,6 +1416,34 @@ static int prepare_fb(struct vpu_instance *inst)
>> return ret;
>> }
>>
>> +static int wave5_vpu_enc_prepare_cap_seq(struct vpu_instance *inst)
>> +{
>
> Factor-out in its own commit, with a message this is preparation work and with
> no function changes. Its really hard to review code that moves around and may
> have changes in it.
Might not be important to do anymore if your suggestions on the
conditional at the bottom of this patch work. I only moved this code to
it's own function since I updated device_run to have capability to
support the same state change if it was not achieved in start_streaming.
>
>> + int ret = 0;
>> +
>> + ret = initialize_sequence(inst);
>> + if (ret) {
>> + dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
>> + return ret;
>> + }
>> + ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * The sequence must be analyzed first to calculate the proper
>> + * size of the auxiliary buffers.
>> + */
>> + ret = prepare_fb(inst);
>> + if (ret) {
>> + dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
>> +
>> + return ret;
>> +}
>> +
>> static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count)
>> {
>> struct vpu_instance *inst = vb2_get_drv_priv(q);
>> @@ -1453,27 +1486,8 @@ static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
>> if (ret)
>> goto return_buffers;
>> }
>> - if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {
>> - ret = initialize_sequence(inst);
>> - if (ret) {
>> - dev_warn(inst->dev->dev, "Sequence not found: %d\n", ret);
>> - goto return_buffers;
>> - }
>> - ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
>> - if (ret)
>> - goto return_buffers;
>> - /*
>> - * The sequence must be analyzed first to calculate the proper
>> - * size of the auxiliary buffers.
>> - */
>> - ret = prepare_fb(inst);
>> - if (ret) {
>> - dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
>> - goto return_buffers;
>> - }
>> -
>> - ret = switch_state(inst, VPU_INST_STATE_PIC_RUN);
>> - }
>> + if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming)
>> + ret = wave5_vpu_enc_prepare_cap_seq(inst);
>> if (ret)
>> goto return_buffers;
>>
>> @@ -1598,6 +1612,14 @@ static void wave5_vpu_enc_device_run(void *priv)
>>
>> pm_runtime_resume_and_get(inst->dev->dev);
>> switch (inst->state) {
>> + case VPU_INST_STATE_OPEN:
>> + ret = wave5_vpu_enc_prepare_cap_seq(inst);
>> + if (ret) {
>> + dev_warn(inst->dev->dev, "Framebuffer preparation, fail: %d\n", ret);
>> + switch_state(inst, VPU_INST_STATE_STOP);
>> + break;
>> + }
>> + fallthrough;
>> case VPU_INST_STATE_PIC_RUN:
>> ret = start_encode(inst, &fail_res);
>> if (ret) {
>> @@ -1633,6 +1655,12 @@ static int wave5_vpu_enc_job_ready(void *priv)
>> case VPU_INST_STATE_NONE:
>> dev_dbg(inst->dev->dev, "Encoder must be open to start queueing M2M jobs!\n");
>> return false;
>> + case VPU_INST_STATE_OPEN:
>> + if (wave5_vpu_both_queues_are_streaming(inst)) {
>> + dev_dbg(inst->dev->dev, "Both queues have been turned on now, M2M job can occur\n");
>> + return true;
>> + }
>> + return false;
>> case VPU_INST_STATE_PIC_RUN:
>> if (m2m_ctx->is_draining || v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
>> dev_dbg(inst->dev->dev, "Encoder ready for a job, state: %s\n",
>> @@ -1642,9 +1670,9 @@ static int wave5_vpu_enc_job_ready(void *priv)
>> fallthrough;
>> default:
>> dev_dbg(inst->dev->dev,
>> - "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready\n",
>> + "Encoder not ready for a job, state: %s, %s draining, %d src bufs ready, %d dst bufs ready\n",
>> state_to_str(inst->state), m2m_ctx->is_draining ? "is" : "is not",
>> - v4l2_m2m_num_src_bufs_ready(m2m_ctx));
>> + v4l2_m2m_num_src_bufs_ready(m2m_ctx), v4l2_m2m_num_dst_bufs_ready(m2m_ctx));
>> break;
>> }
>> return false;
>
> Perhaps its going to be clear with proper commit message, but I'm still not
> clear how you can endup with both queues streaming without two calls to
> wave5_vpu_enc_start_streaming(). I don't deny the condition might be broken
> then, but the intent is for this code to bring the driver to PIC_RUN on the
> second call.>
> From VPU_INST_STATE_NONE:
>
> Case 1:
> STREAMON(CAP)
> - bring it to OPEN state
Wouldn't this be the no-op and cause no state change? VPU only goes to
OPEN when start_streaming is called on the OUTPUT plane.
> STREAMON(OUT)
> - Initialize the sequence and prepare the FB
> - Leaving with PIC_RUN state
So this state would be opening and leaving in state OPEN, but since CAP
was called first, it should be streaming so check would happen.
>
>
> Case 2:
> STREAMON(OUT)
> - no-op
This would put into state open.
> STREAMON(CAP)
> - To OPEN
It would already be in state open since STREAMON(OUT) put it there.
> - To INIT_SEQ
> - To PIC_RUN
Then the above two don't happen due to below condition failing.
>
>
> So in case 2, the code fails this condition:
>
> if (inst->state == VPU_INST_STATE_OPEN && m2m_ctx->cap_q_ctx.q.streaming) {>
>
> Basically type == CAP, and vb2 won't be setting the .streaming state before this
> function returns. A possible solution would be:
>
> if (inst->state == VPU_INST_STATE_OPEN &&
> (m2m_ctx->cap_q_ctx.q.streaming || type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)) {
I can test this and I'm sure it will work because this would force it to
enter that code block to regardless on the 2nd start_streaming case.
This appears to be the cleaner solution without messing around with the
state's in job_ready and device_run. So this patch becomes much simpler.
I will split into two: 1. updating this conditional (assuming it works)
and 2. updating cmd_stop to go directly to try_schedule. Both patches
with better commit messages.
Best,
Brandon
>
> cheers,
> Nicolas
© 2016 - 2026 Red Hat, Inc.