[PATCH v1 1/5] media: rkvdec: Disable H.264 error detection

[PATCH v1 1/5] media: rkvdec: Disable H.264 error detection

[Nicolas Dufresne]

Quite often, the HW get stuck in error condition if a stream error
was detected. As documented, the HW should stop immediately and self
reset. There is likely a problem or a miss-understanding of the self
self reset mechanism, as unless we make a long pause, the next command
will then report an error even if there is no error in it.

Disabling error detection fixes the issue, and let the decoder continue
after an error. This patch is safe for backport into older kernels.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 2992fb87cf72..55596ce6bb6e 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1175,8 +1175,8 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
 
-	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
-	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
+	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
+	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
 	writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
 	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
 
-- 
2.36.1

Re: [PATCH v1 1/5] media: rkvdec: Disable H.264 error detection

[Dmitry Osipenko]

On 6/10/22 15:52, Nicolas Dufresne wrote:
> Quite often, the HW get stuck in error condition if a stream error
> was detected. As documented, the HW should stop immediately and self
> reset. There is likely a problem or a miss-understanding of the self
> self reset mechanism, as unless we make a long pause, the next command
> will then report an error even if there is no error in it.
> 
> Disabling error detection fixes the issue, and let the decoder continue
> after an error. This patch is safe for backport into older kernels.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---

Nit: won't hurt to add the explicit stable tag if you'll make the v2.

Cc: stable@vger.kernel.org

-- 
Best regards,
Dmitry

Re: [PATCH v1 1/5] media: rkvdec: Disable H.264 error detection

[Brian Norris]

On Fri, Jun 10, 2022 at 08:52:11AM -0400, Nicolas Dufresne wrote:
> Quite often, the HW get stuck in error condition if a stream error
> was detected. As documented, the HW should stop immediately and self
> reset. There is likely a problem or a miss-understanding of the self
> self reset mechanism, as unless we make a long pause, the next command
> will then report an error even if there is no error in it.
> 
> Disabling error detection fixes the issue, and let the decoder continue
> after an error. This patch is safe for backport into older kernels.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This is effectively how ChromeOS previously was using this hardware for
years. When moving to the upstream/staging driver, this started giving
us problems. This fix is helpful; we'd rather sacrifice error detection
for now, to avoid hanging the hardware in error cases ;)

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v1 1/5] media: rkvdec: Disable H.264 error detection

[Ezequiel Garcia]

Hi Hans,

On Fri, Jun 10, 2022 at 09:39:19AM -0700, Brian Norris wrote:
> On Fri, Jun 10, 2022 at 08:52:11AM -0400, Nicolas Dufresne wrote:
> > Quite often, the HW get stuck in error condition if a stream error
> > was detected. As documented, the HW should stop immediately and self
> > reset. There is likely a problem or a miss-understanding of the self
> > self reset mechanism, as unless we make a long pause, the next command
> > will then report an error even if there is no error in it.
> > 
> > Disabling error detection fixes the issue, and let the decoder continue
> > after an error. This patch is safe for backport into older kernels.
> > 
> > Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This is effectively how ChromeOS previously was using this hardware for
> years. When moving to the upstream/staging driver, this started giving
> us problems. This fix is helpful; we'd rather sacrifice error detection
> for now, to avoid hanging the hardware in error cases ;)
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Given this is stable material, looks like we should queue it,
while the rest of the series is still being discussed.

Thanks,
Ezequiel

[PATCH v1 2/5] media: rkvdec: Add an ops to check for decode errors

[Nicolas Dufresne]

This optional internal ops allow each codec to do their own
error status checking. The presence of an error is reported
using the ERROR buffer state. This patch have no functional
changes.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 10 ++++++----
 drivers/staging/media/rkvdec/rkvdec.h |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7bab7586918c..7e76f8b72885 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -950,6 +950,7 @@ static void rkvdec_v4l2_cleanup(struct rkvdec_dev *rkvdec)
 static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
 {
 	struct rkvdec_dev *rkvdec = priv;
+	struct rkvdec_ctx *ctx;
 	enum vb2_buffer_state state;
 	u32 status;
 
@@ -958,12 +959,13 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
 		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
 
 	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
-	if (cancel_delayed_work(&rkvdec->watchdog_work)) {
-		struct rkvdec_ctx *ctx;
+	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
 
-		ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
+	if (ctx->coded_fmt_desc->ops->check_error_info)
+		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
+
+	if (cancel_delayed_work(&rkvdec->watchdog_work))
 		rkvdec_job_finish(ctx, state);
-	}
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 633335ebb9c4..4ae8e6c6b03c 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
 		     struct vb2_v4l2_buffer *dst_buf,
 		     enum vb2_buffer_state result);
 	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
+	/* called from IRQ handler */
+	int (*check_error_info)(struct rkvdec_ctx *ctx);
 };
 
 struct rkvdec_coded_fmt_desc {
-- 
2.36.1

Re: [PATCH v1 2/5] media: rkvdec: Add an ops to check for decode errors

[Hans Verkuil]

On 6/10/22 14:52, Nicolas Dufresne wrote:
> This optional internal ops allow each codec to do their own
> error status checking. The presence of an error is reported
> using the ERROR buffer state. This patch have no functional
> changes.

If a buffer is returned with state ERROR, then that means that it is
seriously corrupt and userspace is expected to drop it. You might still
want to show it for debugging purposes, but the normal action is to drop it.

So this is not a valid approach for a decoder that can still produce a
decent picture, albeit with macroblock artifacts.

A separate control that can be returned as part of the request and contains
some sort of error indication would be more appropriate.

Buffer state ERROR is really meant for e.g. DMA errors and it shouldn't
be mixed with decode errors that still produce a valid picture.

Regards,

	Hans

> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 10 ++++++----
>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 7bab7586918c..7e76f8b72885 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -950,6 +950,7 @@ static void rkvdec_v4l2_cleanup(struct rkvdec_dev *rkvdec)
>  static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>  {
>  	struct rkvdec_dev *rkvdec = priv;
> +	struct rkvdec_ctx *ctx;
>  	enum vb2_buffer_state state;
>  	u32 status;
>  
> @@ -958,12 +959,13 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>  		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
>  
>  	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
> -	if (cancel_delayed_work(&rkvdec->watchdog_work)) {
> -		struct rkvdec_ctx *ctx;
> +	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>  
> -		ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
> +	if (ctx->coded_fmt_desc->ops->check_error_info)
> +		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
> +
> +	if (cancel_delayed_work(&rkvdec->watchdog_work))
>  		rkvdec_job_finish(ctx, state);
> -	}
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 633335ebb9c4..4ae8e6c6b03c 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>  		     struct vb2_v4l2_buffer *dst_buf,
>  		     enum vb2_buffer_state result);
>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> +	/* called from IRQ handler */
> +	int (*check_error_info)(struct rkvdec_ctx *ctx);
>  };
>  
>  struct rkvdec_coded_fmt_desc {

Re: [PATCH v1 2/5] media: rkvdec: Add an ops to check for decode errors

[Nicolas Dufresne]

Le mardi 14 juin 2022 à 16:44 +0200, Hans Verkuil a écrit :
> On 6/10/22 14:52, Nicolas Dufresne wrote:
> > This optional internal ops allow each codec to do their own
> > error status checking. The presence of an error is reported
> > using the ERROR buffer state. This patch have no functional
> > changes.
> 
> If a buffer is returned with state ERROR, then that means that it is
> seriously corrupt and userspace is expected to drop it. You might still
> want to show it for debugging purposes, but the normal action is to drop it.

The discussion should be around the ERROR flag, and not the error state. Error
state is just an internal thing that have no meaning API wise, but turns out to
be the only way to get the ERROR flag to be set. With that in mind, this is not
what V4L2_BUF_FLAG_ERROR specification says:

> When this flag is set, the buffer has been dequeued successfully, although
> the data might have been corrupted. This is recoverable, streaming may
> continue as normal and the buffer may be reused normally. Drivers set 
> this flag when the VIDIOC_DQBUF ioctl is called.

For me "seriously corrupt" and "might have been corrupted" is very different.

> 
> So this is not a valid approach for a decoder that can still produce a
> decent picture, albeit with macroblock artifacts.
> 
> A separate control that can be returned as part of the request and contains
> some sort of error indication would be more appropriate.
> 
> Buffer state ERROR is really meant for e.g. DMA errors and it shouldn't
> be mixed with decode errors that still produce a valid picture.

The ERROR flag has been used for many years by the UVC driver to indicate a
partially received image (possibly due to DMA error). That driver went even
further and set the bytesused to the amount of bytes that was received. How this
have been interpreted (mostly due to how the spec around ERROR flag is written)
in GStreamer is that the buffer contains "some valid" data unless payload size
is 0.

As explained earlier, the decision to display "some valid" data is not something
we should decided for our users. This should be left to the application to
decide. Same goes for GStreamer, if a buffer exist but has "some valid data", we
have a GST_BUFFER_FLAG_CORRUPTED flag for it. It is then up for the application
to drop if needed for the application. I'm pretty sure some stateful decoders
also behaves like this (simply because an error is an error, regardless of the
nature of it).

It might be different today, but few years ago, dropping or not dropping was the
main difference between Apple Facetime (dropping) and the other video streaming
applications. One would freeze, the other would show "some valid data".

If you look at the outcome of a partially corrupted decoded images and the
outcome of a mid-frame DMA error (typically from a camera stream), you'll find
that these are visually the same. So it is unfair to consider these two error so
different that a new mechanism must be added. In my opinion, adding RO controls
to signal these corruption only make sense if the hardware can provide detailed
reports of what is corrupted (list/range of macro-blocks, or CTU that are
affected). Then you could measure the level of corruption, but in reality, I
doubt there would be a vast usage of this, specially that the report will likely
be inconsistent due to limited HW support.

Finally, in the bitstream decoder world, including all software decoders I've
worked with, the decode is a success only if all bits are perfectly decoded.
This is the baseline for good vs bad. Userland expected an image, and whatever
happened, in real-time scenario, it must send an image. Sending a corrupted
image, or sending the previously good image remains a decision to be made by
application. As application exist around the implemented mechanism here, I'd
prefer to go for that rather then adding a new API.

Nicolas

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec.c | 10 ++++++----
> >  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > index 7bab7586918c..7e76f8b72885 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -950,6 +950,7 @@ static void rkvdec_v4l2_cleanup(struct rkvdec_dev *rkvdec)
> >  static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
> >  {
> >  	struct rkvdec_dev *rkvdec = priv;
> > +	struct rkvdec_ctx *ctx;
> >  	enum vb2_buffer_state state;
> >  	u32 status;
> >  
> > @@ -958,12 +959,13 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
> >  		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
> >  
> >  	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
> > -	if (cancel_delayed_work(&rkvdec->watchdog_work)) {
> > -		struct rkvdec_ctx *ctx;
> > +	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
> >  
> > -		ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
> > +	if (ctx->coded_fmt_desc->ops->check_error_info)
> > +		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
> > +
> > +	if (cancel_delayed_work(&rkvdec->watchdog_work))
> >  		rkvdec_job_finish(ctx, state);
> > -	}
> >  
> >  	return IRQ_HANDLED;
> >  }
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > index 633335ebb9c4..4ae8e6c6b03c 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
> >  		     struct vb2_v4l2_buffer *dst_buf,
> >  		     enum vb2_buffer_state result);
> >  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> > +	/* called from IRQ handler */
> > +	int (*check_error_info)(struct rkvdec_ctx *ctx);
> >  };
> >  
> >  struct rkvdec_coded_fmt_desc {
> 

Re: [PATCH v1 2/5] media: rkvdec: Add an ops to check for decode errors

[Hans Verkuil]

Hi Nicolas,

On 14/06/2022 18:14, Nicolas Dufresne wrote:
> Le mardi 14 juin 2022 à 16:44 +0200, Hans Verkuil a écrit :
>> On 6/10/22 14:52, Nicolas Dufresne wrote:
>>> This optional internal ops allow each codec to do their own
>>> error status checking. The presence of an error is reported
>>> using the ERROR buffer state. This patch have no functional
>>> changes.
>>
>> If a buffer is returned with state ERROR, then that means that it is
>> seriously corrupt and userspace is expected to drop it. You might still
>> want to show it for debugging purposes, but the normal action is to drop it.
> 
> The discussion should be around the ERROR flag, and not the error state. Error
> state is just an internal thing that have no meaning API wise, but turns out to
> be the only way to get the ERROR flag to be set. With that in mind, this is not
> what V4L2_BUF_FLAG_ERROR specification says:
> 
>> When this flag is set, the buffer has been dequeued successfully, although
>> the data might have been corrupted. This is recoverable, streaming may
>> continue as normal and the buffer may be reused normally. Drivers set 
>> this flag when the VIDIOC_DQBUF ioctl is called.
> 
> For me "seriously corrupt" and "might have been corrupted" is very different.

So I did some more digging (better late than never): the documentation for
the stateful/stateless codecs explicitly states that ERROR should be used
to indicate de/encoding errors.

I am actually not that happy about that, since the original purpose of ERROR
was really to indicate that there is something seriously wrong with the
captured data, and applications should skip it, except for debugging purposes.

But since it has been adopted in the codec documentation I have to accept this
behavior for codecs.

So I am OK with this series. There were some comments about some typos, so
I will mark it as 'Changes Requested' in patchwork, but if you post a v2, then
I'll take it.

Regards,

	Hans

> 
>>
>> So this is not a valid approach for a decoder that can still produce a
>> decent picture, albeit with macroblock artifacts.
>>
>> A separate control that can be returned as part of the request and contains
>> some sort of error indication would be more appropriate.
>>
>> Buffer state ERROR is really meant for e.g. DMA errors and it shouldn't
>> be mixed with decode errors that still produce a valid picture.
> 
> The ERROR flag has been used for many years by the UVC driver to indicate a
> partially received image (possibly due to DMA error). That driver went even
> further and set the bytesused to the amount of bytes that was received. How this
> have been interpreted (mostly due to how the spec around ERROR flag is written)
> in GStreamer is that the buffer contains "some valid" data unless payload size
> is 0.
> 
> As explained earlier, the decision to display "some valid" data is not something
> we should decided for our users. This should be left to the application to
> decide. Same goes for GStreamer, if a buffer exist but has "some valid data", we
> have a GST_BUFFER_FLAG_CORRUPTED flag for it. It is then up for the application
> to drop if needed for the application. I'm pretty sure some stateful decoders
> also behaves like this (simply because an error is an error, regardless of the
> nature of it).
> 
> It might be different today, but few years ago, dropping or not dropping was the
> main difference between Apple Facetime (dropping) and the other video streaming
> applications. One would freeze, the other would show "some valid data".
> 
> If you look at the outcome of a partially corrupted decoded images and the
> outcome of a mid-frame DMA error (typically from a camera stream), you'll find
> that these are visually the same. So it is unfair to consider these two error so
> different that a new mechanism must be added. In my opinion, adding RO controls
> to signal these corruption only make sense if the hardware can provide detailed
> reports of what is corrupted (list/range of macro-blocks, or CTU that are
> affected). Then you could measure the level of corruption, but in reality, I
> doubt there would be a vast usage of this, specially that the report will likely
> be inconsistent due to limited HW support.
> 
> Finally, in the bitstream decoder world, including all software decoders I've
> worked with, the decode is a success only if all bits are perfectly decoded.
> This is the baseline for good vs bad. Userland expected an image, and whatever
> happened, in real-time scenario, it must send an image. Sending a corrupted
> image, or sending the previously good image remains a decision to be made by
> application. As application exist around the implemented mechanism here, I'd
> prefer to go for that rather then adding a new API.
> 
> Nicolas
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> ---
>>>  drivers/staging/media/rkvdec/rkvdec.c | 10 ++++++----
>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>> index 7bab7586918c..7e76f8b72885 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>> @@ -950,6 +950,7 @@ static void rkvdec_v4l2_cleanup(struct rkvdec_dev *rkvdec)
>>>  static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>>>  {
>>>  	struct rkvdec_dev *rkvdec = priv;
>>> +	struct rkvdec_ctx *ctx;
>>>  	enum vb2_buffer_state state;
>>>  	u32 status;
>>>  
>>> @@ -958,12 +959,13 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>>>  		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
>>>  
>>>  	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
>>> -	if (cancel_delayed_work(&rkvdec->watchdog_work)) {
>>> -		struct rkvdec_ctx *ctx;
>>> +	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>>>  
>>> -		ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>>> +	if (ctx->coded_fmt_desc->ops->check_error_info)
>>> +		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
>>> +
>>> +	if (cancel_delayed_work(&rkvdec->watchdog_work))
>>>  		rkvdec_job_finish(ctx, state);
>>> -	}
>>>  
>>>  	return IRQ_HANDLED;
>>>  }
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>> index 633335ebb9c4..4ae8e6c6b03c 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>  		     enum vb2_buffer_state result);
>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>> +	/* called from IRQ handler */
>>> +	int (*check_error_info)(struct rkvdec_ctx *ctx);
>>>  };
>>>  
>>>  struct rkvdec_coded_fmt_desc {
>>
> 

[PATCH v1 3/5] media: rkvdec: Fix RKVDEC_ERR_PKT_NUM macro

[Nicolas Dufresne]

This information is expressed by bits [29:16], but the actual
implementation was reading bits [13:0] and shifting that 16
bits to the left.

Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-regs.h b/drivers/staging/media/rkvdec/rkvdec-regs.h
index 15b9bee92016..14530b81560e 100644
--- a/drivers/staging/media/rkvdec/rkvdec-regs.h
+++ b/drivers/staging/media/rkvdec/rkvdec-regs.h
@@ -212,7 +212,7 @@
 #define RKVDEC_REG_H264_ERRINFO_NUM			0x130
 #define RKVDEC_SLICEDEC_NUM(x)				((x) & 0x3fff)
 #define RKVDEC_STRMD_DECT_ERR_FLAG			BIT(15)
-#define RKVDEC_ERR_PKT_NUM(x)				(((x) & 0x3fff) << 16)
+#define RKVDEC_ERR_PKT_NUM(x)				((x >> 16) & 0x3fff)
 
 #define RKVDEC_REG_H264_ERR_E				0x134
 #define RKVDEC_H264_ERR_EN_HIGHBITS(x)			((x) & 0x3fffffff)
-- 
2.36.1

[PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Nicolas Dufresne]

This re-enables H.264 error detection, but using the other error mode.
In that mode, the decoder will skip over the error macro-block or
slices and complete the decoding. As a side effect, the error status
is not set in the interrupt status register, and instead errors are
detected per format. Using this mode workaround the issue that the
HW get stuck in error stated and allow reporting that some corruption
may be present in the buffer returned to userland.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 55596ce6bb6e..60a89918e2c1 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
 
-	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
-	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
+	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
+	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
 	writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
 	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
 
 	/* Start decoding! */
 	writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
-	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
+	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
+	       RKVDEC_H264ORVP9_ERR_MODE,
 	       rkvdec->regs + RKVDEC_REG_INTERRUPT);
 
 	return 0;
@@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
+{
+	struct rkvdec_dev *rkvdec = ctx->dev;
+	int err;
+
+	err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
+	if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
+		pr_debug("Decoded picture have %i/%i slices with errors.\n",
+			 RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
+		return VB2_BUF_STATE_ERROR;
+	}
+
+	return VB2_BUF_STATE_DONE;
+}
+
 const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
 	.adjust_fmt = rkvdec_h264_adjust_fmt,
 	.start = rkvdec_h264_start,
 	.stop = rkvdec_h264_stop,
 	.run = rkvdec_h264_run,
 	.try_ctrl = rkvdec_h264_try_ctrl,
+	.check_error_info = rkvdec_h264_check_error_info,
 };
-- 
2.36.1

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Dan Carpenter]

On Fri, Jun 10, 2022 at 08:52:14AM -0400, Nicolas Dufresne wrote:
> This re-enables H.264 error detection, but using the other error mode.
> In that mode, the decoder will skip over the error macro-block or
> slices and complete the decoding. As a side effect, the error status
> is not set in the interrupt status register, and instead errors are
> detected per format. Using this mode workaround the issue that the
> HW get stuck in error stated and allow reporting that some corruption
> may be present in the buffer returned to userland.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 55596ce6bb6e..60a89918e2c1 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  
>  	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
>  
> -	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> -	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);

This reverts the changes in patch 1/5.  Could we just skip patch 1/5
instead?

regards,
dan carpenter

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Dmitry Osipenko]

On 6/10/22 16:20, Dan Carpenter wrote:
> On Fri, Jun 10, 2022 at 08:52:14AM -0400, Nicolas Dufresne wrote:
>> This re-enables H.264 error detection, but using the other error mode.
>> In that mode, the decoder will skip over the error macro-block or
>> slices and complete the decoding. As a side effect, the error status
>> is not set in the interrupt status register, and instead errors are
>> detected per format. Using this mode workaround the issue that the
>> HW get stuck in error stated and allow reporting that some corruption
>> may be present in the buffer returned to userland.
>>
>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> ---
>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> index 55596ce6bb6e..60a89918e2c1 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>>  
>>  	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
>>  
>> -	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
>> -	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
>> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
>> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> 
> This reverts the changes in patch 1/5.  Could we just skip patch 1/5
> instead?

The first patch must go to the stable kernels, hence we couldn't skip it.

-- 
Best regards,
Dmitry

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Nicolas Dufresne]

Le vendredi 10 juin 2022 à 16:20 +0300, Dan Carpenter a écrit :
> On Fri, Jun 10, 2022 at 08:52:14AM -0400, Nicolas Dufresne wrote:
> > This re-enables H.264 error detection, but using the other error mode.
> > In that mode, the decoder will skip over the error macro-block or
> > slices and complete the decoding. As a side effect, the error status
> > is not set in the interrupt status register, and instead errors are
> > detected per format. Using this mode workaround the issue that the
> > HW get stuck in error stated and allow reporting that some corruption
> > may be present in the buffer returned to userland.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 55596ce6bb6e..60a89918e2c1 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> >  
> >  	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
> >  
> > -	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > -	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> > +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> 
> This reverts the changes in patch 1/5.  Could we just skip patch 1/5
> instead?

As documented, this is for back-porting purpose. The first patch is what has
been running for 7 years in Chromebook, so I'm fully confident it is safe to
backport it into our stable kernel. The second is like a new feature, which I'm
confident works, but didn't get as much testing as I just wrote it. So what I'm
doing here is giving a same thing to backport, and a better fix for the next
kernel. You are otherwise right that this will revert it.

Nicolas

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Ezequiel Garcia]

Hi Nicolas,

Great stuff! See below for some ideas how to expose errors.

On Fri, Jun 10, 2022 at 9:52 AM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> This re-enables H.264 error detection, but using the other error mode.
> In that mode, the decoder will skip over the error macro-block or
> slices and complete the decoding. As a side effect, the error status
> is not set in the interrupt status register, and instead errors are
> detected per format. Using this mode workaround the issue that the
> HW get stuck in error stated and allow reporting that some corruption
> may be present in the buffer returned to userland.
>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 55596ce6bb6e..60a89918e2c1 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>
>         schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
>
> -       writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> -       writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
>         writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
>         writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
>
>         /* Start decoding! */
>         writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> -              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> +              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
> +              RKVDEC_H264ORVP9_ERR_MODE,
>                rkvdec->regs + RKVDEC_REG_INTERRUPT);
>
>         return 0;
> @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
>         return 0;
>  }
>
> +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> +{
> +       struct rkvdec_dev *rkvdec = ctx->dev;
> +       int err;
> +
> +       err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
> +       if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
> +               pr_debug("Decoded picture have %i/%i slices with errors.\n",
> +                        RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));

It's more useful friendly to just keep a counter somewhere. In the past,
we've created a user control, which has the advantage of leveraging
an existing mechanism, and already being per-fd.

See:

commit b2d3bef1aa7858b2ae5e0d01adb214121ba00b9f
"media: coda: Add a V4L2 user for control error macroblocks count".

I would drop the pr_debug, or if you think it's really useful for users
and developers, go with v4l2_dbg. In which case, how do you ensure
a corrupted stream won't flood the logs?

Thanks,
Ezequiel


> +               return VB2_BUF_STATE_ERROR;
> +       }
> +
> +       return VB2_BUF_STATE_DONE;
> +}
> +
>  const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
>         .adjust_fmt = rkvdec_h264_adjust_fmt,
>         .start = rkvdec_h264_start,
>         .stop = rkvdec_h264_stop,
>         .run = rkvdec_h264_run,
>         .try_ctrl = rkvdec_h264_try_ctrl,
> +       .check_error_info = rkvdec_h264_check_error_info,
>  };
> --
> 2.36.1
>

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Nicolas Dufresne]

Le vendredi 10 juin 2022 à 12:01 -0300, Ezequiel Garcia a écrit :
> Hi Nicolas,
> 
> Great stuff! See below for some ideas how to expose errors.
> 
> On Fri, Jun 10, 2022 at 9:52 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > This re-enables H.264 error detection, but using the other error mode.
> > In that mode, the decoder will skip over the error macro-block or
> > slices and complete the decoding. As a side effect, the error status
> > is not set in the interrupt status register, and instead errors are
> > detected per format. Using this mode workaround the issue that the
> > HW get stuck in error stated and allow reporting that some corruption
> > may be present in the buffer returned to userland.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 55596ce6bb6e..60a89918e2c1 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> > 
> >         schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
> > 
> > -       writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > -       writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> > +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> >         writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
> >         writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
> > 
> >         /* Start decoding! */
> >         writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> > -              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> > +              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
> > +              RKVDEC_H264ORVP9_ERR_MODE,
> >                rkvdec->regs + RKVDEC_REG_INTERRUPT);
> > 
> >         return 0;
> > @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> >         return 0;
> >  }
> > 
> > +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> > +{
> > +       struct rkvdec_dev *rkvdec = ctx->dev;
> > +       int err;
> > +
> > +       err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
> > +       if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
> > +               pr_debug("Decoded picture have %i/%i slices with errors.\n",
> > +                        RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
> 
> It's more useful friendly to just keep a counter somewhere. In the past,
> we've created a user control, which has the advantage of leveraging
> an existing mechanism, and already being per-fd.
> 
> See:
> 
> commit b2d3bef1aa7858b2ae5e0d01adb214121ba00b9f
> "media: coda: Add a V4L2 user for control error macroblocks count".
> 
> I would drop the pr_debug, or if you think it's really useful for users
> and developers, go with v4l2_dbg. In which case, how do you ensure
> a corrupted stream won't flood the logs?

There is no use case to make this a control, but yes, a corrupted stream can
flood, but isn't the point of pr_debug() that it won't show if you don't enabled
it ?

As for v4l2_dbg I'm not familiar with that and its not used in this driver for
traces. I've use pr_debug for reference list tracing previously and flooding was
not considered a problem despite being a per-frame trace. You can even out-
compile these if you need to.

Let me know if you have further rationale in the suggestion direction. The
rationale in my coding for such trace is that if I read 1 bit of a register, and
trace the surrounding value, I can validate (as a human) that the rest of the
register isn't complete garbage, and that I'm not basically reading a random
bit. Leaving the trace there, allow other developer on other variant of these
SoC to also notice if that register becomes garbage. This is in contrast of just
telling others "trust me, I tested it".

Nicolas

> 
> Thanks,
> Ezequiel
> 
> 
> > +               return VB2_BUF_STATE_ERROR;
> > +       }
> > +
> > +       return VB2_BUF_STATE_DONE;
> > +}
> > +
> >  const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
> >         .adjust_fmt = rkvdec_h264_adjust_fmt,
> >         .start = rkvdec_h264_start,
> >         .stop = rkvdec_h264_stop,
> >         .run = rkvdec_h264_run,
> >         .try_ctrl = rkvdec_h264_try_ctrl,
> > +       .check_error_info = rkvdec_h264_check_error_info,
> >  };
> > --
> > 2.36.1
> > 

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Alex Bee]

Am 10.06.22 um 14:52 schrieb Nicolas Dufresne:
> This re-enables H.264 error detection, but using the other error mode.
> In that mode, the decoder will skip over the error macro-block or
> slices and complete the decoding. As a side effect, the error status
> is not set in the interrupt status register, and instead errors are
> detected per format. Using this mode workaround the issue that the
> HW get stuck in error stated and allow reporting that some corruption
> may be present in the buffer returned to userland.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 55596ce6bb6e..60a89918e2c1 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  
>  	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
>  
> -	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> -	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
>  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
>  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
>  
>  	/* Start decoding! */
>  	writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> -	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> +	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
> +	       RKVDEC_H264ORVP9_ERR_MODE,
>  	       rkvdec->regs + RKVDEC_REG_INTERRUPT);
>  
>  	return 0;
> @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> +{
> +	struct rkvdec_dev *rkvdec = ctx->dev;
> +	int err;
> +
> +	err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
> +	if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
> +		pr_debug("Decoded picture have %i/%i slices with errors.\n",
> +			 RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
> +		return VB2_BUF_STATE_ERROR;
> +	}
> +
> +	return VB2_BUF_STATE_DONE;
> +}
> +
>  const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
>  	.adjust_fmt = rkvdec_h264_adjust_fmt,
>  	.start = rkvdec_h264_start,
>  	.stop = rkvdec_h264_stop,
>  	.run = rkvdec_h264_run,
>  	.try_ctrl = rkvdec_h264_try_ctrl,
> +	.check_error_info = rkvdec_h264_check_error_info,
>  };

Actually I'm not sure I fully understand what you are expecting the
userspace to do with the information that there was an (HW!) error,
which might or might not be bitstrean related. Resending the
corrupted(?) frame until the HW fully hangs?
As the interrupt reports an HW error it should (at least also) be
handled driver-side and the HW is known not to be able to fully reset
itself in case of an error.
I think this will make behavior worse than it is now (for real-life
users) where errors are eventually just ignored.

Alex

Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Nicolas Dufresne]

Le samedi 11 juin 2022 à 14:08 +0200, Alex Bee a écrit :
> Am 10.06.22 um 14:52 schrieb Nicolas Dufresne:
> > This re-enables H.264 error detection, but using the other error mode.
> > In that mode, the decoder will skip over the error macro-block or
> > slices and complete the decoding. As a side effect, the error status
> > is not set in the interrupt status register, and instead errors are
> > detected per format. Using this mode workaround the issue that the
> > HW get stuck in error stated and allow reporting that some corruption
> > may be present in the buffer returned to userland.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 55596ce6bb6e..60a89918e2c1 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> >  
> >  	schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
> >  
> > -	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > -	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> > +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > +	writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> >  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
> >  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
> >  
> >  	/* Start decoding! */
> >  	writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> > -	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> > +	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
> > +	       RKVDEC_H264ORVP9_ERR_MODE,
> >  	       rkvdec->regs + RKVDEC_REG_INTERRUPT);
> >  
> >  	return 0;
> > @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> >  	return 0;
> >  }
> >  
> > +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> > +{
> > +	struct rkvdec_dev *rkvdec = ctx->dev;
> > +	int err;
> > +
> > +	err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
> > +	if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
> > +		pr_debug("Decoded picture have %i/%i slices with errors.\n",
> > +			 RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
> > +		return VB2_BUF_STATE_ERROR;
> > +	}
> > +
> > +	return VB2_BUF_STATE_DONE;
> > +}
> > +
> >  const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
> >  	.adjust_fmt = rkvdec_h264_adjust_fmt,
> >  	.start = rkvdec_h264_start,
> >  	.stop = rkvdec_h264_stop,
> >  	.run = rkvdec_h264_run,
> >  	.try_ctrl = rkvdec_h264_try_ctrl,
> > +	.check_error_info = rkvdec_h264_check_error_info,
> >  };
> 
> Actually I'm not sure I fully understand what you are expecting the
> userspace to do with the information that there was an (HW!) error,
> which might or might not be bitstrean related. Resending the
> corrupted(?) frame until the HW fully hangs?
> As the interrupt reports an HW error it should (at least also) be
> handled driver-side and the HW is known not to be able to fully reset
> itself in case of an error.
> I think this will make behavior worse than it is now (for real-life
> users) where errors are eventually just ignored.

I've changed the decoding mode, see bit 19 or swreg1. In that mode, the decoder
will behave just like if error detection was off. It will just keep going and
produce "something". With the set of corrupted streams we had, we found that the
decoder no longer get stuck, and we are aware of the possibly corrupted buffers.
In current mode, the decoder tries to stop as soon as an error is met, which
most of the time means nothing is every written to the buffer. And as you
mention, it often fail at "self reset".

In streaming there is two style of handling corrupted buffers, one is to skip
until valid state, and the other is to show them even if corrupted. In stack
like GStreamer we just flag the corrupted frames based on the ERROR flag (unless
payload size is 0) and let the user chose to drop them or not.


> I think this will make behavior worse than it is now (for real-life users)
where errors are eventually just ignored.

Just ignoring the errors is way better then an infinite row of errors. At the
moment, FFMPEG/Chromium and GStreamer ignores errors indeed. I got some work in
progress patch in GStreamer I've used to test this, but its not ready yet. In
the current behaviour, if you hit an error, you basically have 9 chances out of
10 to keep replaying ancient buffers in loop till the end of time. This is
because the self reset never completes, and you get the same error over and over
regardless what you pass to the decoder.

regards,
Nicolas

p.s. the tests should land (if not already) in ChromeOS taste suite.

> 
> Alex

[PATCH v1 5/5] media: rkvdec: Improve error handling

[Nicolas Dufresne]

There is two way decode errors can occur. In one case, the ready
status is not set and nothing have been written into the destination,
while in the other case, the buffer is written but may contain a
certain amount of errors. In order to differentiate these, we set
the payload for the first case to 0.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7e76f8b72885..27f1f7276dd2 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -954,14 +954,32 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
 	enum vb2_buffer_state state;
 	u32 status;
 
+	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
 	status = readl(rkvdec->regs + RKVDEC_REG_INTERRUPT);
-	state = (status & RKVDEC_RDY_STA) ?
-		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
+
+	if (!(status & RKVDEC_RDY_STA)) {
+		struct vb2_v4l2_buffer *dst_buf = NULL;
+
+		if (status & RKVDEC_TIMEOUT_STA)
+			pr_debug("Decoder stopped due to internal timeout.");
+		else
+			pr_debug("Decoder stopped due to internal error.");
+
+		/*
+		 * When this happens, the buffer is left unmodified. As it
+		 * contains no meaningful data we mark is a empty.
+		 */
+		dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
+		state = VB2_BUF_STATE_ERROR;
+	} else {
+		state = VB2_BUF_STATE_DONE;
+	}
 
 	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
-	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
 
-	if (ctx->coded_fmt_desc->ops->check_error_info)
+	if (ctx->coded_fmt_desc->ops->check_error_info &&
+	    state == VB2_BUF_STATE_DONE)
 		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
 
 	if (cancel_delayed_work(&rkvdec->watchdog_work))
-- 
2.36.1

Re: [PATCH v1 5/5] media: rkvdec: Improve error handling

[Sebastian Fricke]

Hey Nicolas,

Tested with: python3 fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -ts JVT-AVC_
V1 -so /tmp/h264_test.csv -f csv -j1

Ran 129/135 tests successfully               in 82.280 secs

On 10.06.2022 08:52, Nicolas Dufresne wrote:
>There is two way decode errors can occur. In one case, the ready

s/There is two way decode/There are two ways decoding/


>status is not set and nothing have been written into the destination,

s/nothing have been/nothing has been/

>while in the other case, the buffer is written but may contain a
>certain amount of errors. In order to differentiate these, we set
>the payload for the first case to 0.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/staging/media/rkvdec/rkvdec.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>index 7e76f8b72885..27f1f7276dd2 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.c
>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>@@ -954,14 +954,32 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
> 	enum vb2_buffer_state state;
> 	u32 status;
>
>+	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
> 	status = readl(rkvdec->regs + RKVDEC_REG_INTERRUPT);
>-	state = (status & RKVDEC_RDY_STA) ?
>-		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
>+
>+	if (!(status & RKVDEC_RDY_STA)) {
>+		struct vb2_v4l2_buffer *dst_buf = NULL;
>+
>+		if (status & RKVDEC_TIMEOUT_STA)
>+			pr_debug("Decoder stopped due to internal timeout.");
>+		else
>+			pr_debug("Decoder stopped due to internal error.");

(Just personal preference.. I would prefer "due to an internal" over
"due to internal")

>+
>+		/*
>+		 * When this happens, the buffer is left unmodified. As it
>+		 * contains no meaningful data we mark is a empty.

s/is a empty/it as empty/

The rest looks nice. Thanks.

Greetings,
Sebastian

>+		 */
>+		dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>+		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
>+		state = VB2_BUF_STATE_ERROR;
>+	} else {
>+		state = VB2_BUF_STATE_DONE;
>+	}
>
> 	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
>-	ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>
>-	if (ctx->coded_fmt_desc->ops->check_error_info)
>+	if (ctx->coded_fmt_desc->ops->check_error_info &&
>+	    state == VB2_BUF_STATE_DONE)
> 		state = ctx->coded_fmt_desc->ops->check_error_info(ctx);
>
> 	if (cancel_delayed_work(&rkvdec->watchdog_work))
>-- 
>2.36.1
>