[PATCH] media: coda: Keep metas sync with hardware fifo

[PATCH] media: coda: Keep metas sync with hardware fifo

[Benjamin Gaignard]

After updating the output fifo position be sure that metas are also
synchronised with this position.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/media/platform/coda/coda-bit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index c484c008ab02..28c56286b0de 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2315,6 +2315,12 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 	/* Update kfifo out pointer from coda bitstream read pointer */
 	coda_kfifo_sync_from_device(ctx);
 
+	/*
+	 * After updating the read pointer, we need to check if
+	 * any metas are consumed and should be released.
+	 */
+	coda_decoder_drop_used_metas(ctx);
+
 	/*
 	 * in stream-end mode, the read pointer can overshoot the write pointer
 	 * by up to 512 bytes
-- 
2.30.2

Re: [PATCH] media: coda: Keep metas sync with hardware fifo

[Philipp Zabel]

Hi Benjamin,

On Mon, 2021-11-08 at 15:24 +0100, Benjamin Gaignard wrote:
> After updating the output fifo position be sure that metas are also
> synchronised with this position.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/media/platform/coda/coda-bit.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
> index c484c008ab02..28c56286b0de 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -2315,6 +2315,12 @@ static void coda_finish_decode(struct coda_ctx *ctx)
>  	/* Update kfifo out pointer from coda bitstream read pointer */
>  	coda_kfifo_sync_from_device(ctx);
>
> +	/*
> +	 * After updating the read pointer, we need to check if
> +	 * any metas are consumed and should be released.
> +	 */
> +	coda_decoder_drop_used_metas(ctx);
> +

This doesn't look right. If you drop all metas seen by the decoder right
away, they can't be copied into the decoded picture's meta slot later in
this function. I'd expect you run into the "empty timestamp list!"
errors if you do this.

regards
Philipp

Re: [PATCH] media: coda: Keep metas sync with hardware fifo

[Benjamin Gaignard]

Le 11/11/2021 à 18:03, Philipp Zabel a écrit :
> Hi Benjamin,
>
> On Mon, 2021-11-08 at 15:24 +0100, Benjamin Gaignard wrote:
>> After updating the output fifo position be sure that metas are also
>> synchronised with this position.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/media/platform/coda/coda-bit.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
>> index c484c008ab02..28c56286b0de 100644
>> --- a/drivers/media/platform/coda/coda-bit.c
>> +++ b/drivers/media/platform/coda/coda-bit.c
>> @@ -2315,6 +2315,12 @@ static void coda_finish_decode(struct coda_ctx *ctx)
>>   	/* Update kfifo out pointer from coda bitstream read pointer */
>>   	coda_kfifo_sync_from_device(ctx);
>>
>> +	/*
>> +	 * After updating the read pointer, we need to check if
>> +	 * any metas are consumed and should be released.
>> +	 */
>> +	coda_decoder_drop_used_metas(ctx);
>> +
> This doesn't look right. If you drop all metas seen by the decoder right
> away, they can't be copied into the decoded picture's meta slot later in
> this function. I'd expect you run into the "empty timestamp list!"
> errors if you do this.

Hi Philipp,

I don't run into the "empty timestamp list!" errors.
The only case I have seen metas been dropped here it is when
an invalid/incomplete frame has been send into the decoder.
When that happens I don't see any interrupt or error message but
the erroneous frame stay in the list.
If that occur 4 times (I'm using CODA 960) then the decoder hang.
Dropping the metas at this moment solve this problem.

Maybe you can guide me to a better solution ?

Regards,
Benjamin

>
> regards
> Philipp

Re: [PATCH] media: coda: Keep metas sync with hardware fifo

[Philipp Zabel]

Hi Benjamin,

On Fri, 2021-11-12 at 09:36 +0100, Benjamin Gaignard wrote:
> Le 11/11/2021 à 18:03, Philipp Zabel a écrit :
> > Hi Benjamin,
> > 
> > On Mon, 2021-11-08 at 15:24 +0100, Benjamin Gaignard wrote:
> > > After updating the output fifo position be sure that metas are also
> > > synchronised with this position.
> > > 
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > ---
> > >   drivers/media/platform/coda/coda-bit.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
> > > index c484c008ab02..28c56286b0de 100644
> > > --- a/drivers/media/platform/coda/coda-bit.c
> > > +++ b/drivers/media/platform/coda/coda-bit.c
> > > @@ -2315,6 +2315,12 @@ static void coda_finish_decode(struct coda_ctx *ctx)
> > >   	/* Update kfifo out pointer from coda bitstream read pointer */
> > >   	coda_kfifo_sync_from_device(ctx);
> > > 
> > > +	/*
> > > +	 * After updating the read pointer, we need to check if
> > > +	 * any metas are consumed and should be released.
> > > +	 */
> > > +	coda_decoder_drop_used_metas(ctx);
> > > +
> > This doesn't look right. If you drop all metas seen by the decoder right
> > away, they can't be copied into the decoded picture's meta slot later in
> > this function. I'd expect you run into the "empty timestamp list!"
> > errors if you do this.
> 
> Hi Philipp,
> 
> I don't run into the "empty timestamp list!" errors.
> The only case I have seen metas been dropped here it is when
> an invalid/incomplete frame has been send into the decoder.
> When that happens I don't see any interrupt or error message but
> the erroneous frame stay in the list.
> If that occur 4 times (I'm using CODA 960) then the decoder hang.
> Dropping the metas at this moment solve this problem.
> 
> Maybe you can guide me to a better solution ?

Hmm, the current code silently assumes that in the good case there's
exactly one used meta (it just picks the first one from the list if
decoded_idx is set).

We could list_cut_before() the used metas into a local list, let the
decoded_idx code pick the timestamp from that list, and then drop the
whole local list of used metas on return from coda_finish_decode().

regards
Philipp