diff mbox series

[05/20] media: iris: Send V4L2_BUF_FLAG_ERROR for buffers with 0 filled length

Message ID 20250408-iris-dec-hevc-vp9-v1-5-acd258778bd6@quicinc.com
State New
Headers show
Series Add support for HEVC and VP9 codecs in decoder | expand

Commit Message

Dikshita Agarwal April 8, 2025, 3:54 p.m. UTC
Firmware sends buffers with 0 filled length which needs to be dropped,
to achieve the same, add V4L2_BUF_FLAG_ERROR to such buffers.
Also make sure:
- These 0 length buffers are not returned as result of flush.
- Its not a buffer with LAST flag enabled which will also have 0 filled
  length.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Bryan O'Donoghue April 11, 2025, 12:51 p.m. UTC | #1
On 08/04/2025 16:54, Dikshita Agarwal wrote:
> Firmware sends buffers with 0 filled length which needs to be dropped,
> to achieve the same, add V4L2_BUF_FLAG_ERROR to such buffers.
> Also make sure:
> - These 0 length buffers are not returned as result of flush.
> - Its not a buffer with LAST flag enabled which will also have 0 filled
>    length.

Any buffer with a zero length must be flagged as LAST, else that buffer 
should be discarded.

Is this another bugfix ? Feels like one, processing redundant packets.

> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>   drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> index b75a01641d5d..91c5f04dd926 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> @@ -377,6 +377,12 @@ static int iris_hfi_gen2_handle_output_buffer(struct iris_inst *inst,
>   
>   	buf->flags = iris_hfi_gen2_get_driver_buffer_flags(inst, hfi_buffer->flags);
>   
> +	if (!buf->data_size && inst->state == IRIS_INST_STREAMING &&
> +	    !(hfi_buffer->flags & HFI_BUF_FW_FLAG_LAST) &&
> +	    !(inst->sub_state & IRIS_INST_SUB_DRC)) {
> +		buf->flags |= V4L2_BUF_FLAG_ERROR;
> +	}
> +

Is this hypothetical or does it happen in real life ?

>   	return 0;
>   }
>   
> 

---
bod
Nicolas Dufresne April 16, 2025, 1:42 p.m. UTC | #2
Le mardi 15 avril 2025 à 10:01 +0530, Dikshita Agarwal a écrit :
> 
> 
> On 4/11/2025 6:21 PM, Bryan O'Donoghue wrote:
> > On 08/04/2025 16:54, Dikshita Agarwal wrote:
> > > Firmware sends buffers with 0 filled length which needs to be dropped,
> > > to achieve the same, add V4L2_BUF_FLAG_ERROR to such buffers.
> > > Also make sure:
> > > - These 0 length buffers are not returned as result of flush.
> > > - Its not a buffer with LAST flag enabled which will also have 0 filled
> > >    length.
> > 
> > Any buffer with a zero length must be flagged as LAST, else that buffer
> > should be discarded.
> > 
> > Is this another bugfix ? Feels like one, processing redundant packets.
> > 
> > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> > > ---
> > >   drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> > > b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> > > index b75a01641d5d..91c5f04dd926 100644
> > > --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> > > +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
> > > @@ -377,6 +377,12 @@ static int iris_hfi_gen2_handle_output_buffer(struct
> > > iris_inst *inst,
> > >         buf->flags = iris_hfi_gen2_get_driver_buffer_flags(inst,
> > > hfi_buffer->flags);
> > >   +    if (!buf->data_size && inst->state == IRIS_INST_STREAMING &&
> > > +        !(hfi_buffer->flags & HFI_BUF_FW_FLAG_LAST) &&
> > > +        !(inst->sub_state & IRIS_INST_SUB_DRC)) {
> > > +        buf->flags |= V4L2_BUF_FLAG_ERROR;
> > > +    }
> > > +
> > 
> > Is this hypothetical or does it happen in real life ?
> Yes, it does. As part of flush, firmware returns the buffers with 0 filled
> length but those shouldn't be marked as ERROR, same applies for buffer with
> LAST flag.
> This conditional check make sure the ERROR flag is associated with only
> frames which are supposed to be dropped/discarded.

informative: Due to legacy behaviour of MFC and Coda, you should never
push a zero-length buffer without one of LAST or ERROR flag. It may be
interpreted as last by some userspace otherwise.

Can you eventually write some doc on the double state machine you have
? Having two state in a state machine makes reading the code quite
harder for reviewers.

Nicolas

> 
> Thanks,
> Dikshita
> > 
> > >       return 0;
> > >   }
> > >  
> > 
> > ---
> > bod
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
index b75a01641d5d..91c5f04dd926 100644
--- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
+++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
@@ -377,6 +377,12 @@  static int iris_hfi_gen2_handle_output_buffer(struct iris_inst *inst,
 
 	buf->flags = iris_hfi_gen2_get_driver_buffer_flags(inst, hfi_buffer->flags);
 
+	if (!buf->data_size && inst->state == IRIS_INST_STREAMING &&
+	    !(hfi_buffer->flags & HFI_BUF_FW_FLAG_LAST) &&
+	    !(inst->sub_state & IRIS_INST_SUB_DRC)) {
+		buf->flags |= V4L2_BUF_FLAG_ERROR;
+	}
+
 	return 0;
 }