[PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Chen-Yu Tsai]

Hi everyone,

While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
behavioral differences compared to Hantro and Cedrus:

1. The driver always overrides the sizeimage setting given by userspace
   for the output format. This results in insufficient buffer space when
   running the ChromeOS video_decode_accelerator_tests test program,
   likely due to a small initial resolution followed by dynamic
   resolution change.

2. Doesn't support dynamic resolution change.

This small series fixes both and aligns the behavior with the other two
stateless decoder drivers. This was tested on the downstream ChromeOS
5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
have any other RK3399 devices set up to test video stuff, so testing
would be very much appreciated.

Also, I'm not sure if user applications are required to check the value
of sizeimage upon S_FMT return. If the value is different or too small,
what can the application do besides fail? AFAICT it can't split the
data of one frame (or slice) between different buffers.

Andrzej, I believe the second patch would conflict with your VP9 series.


Regards
ChenYu

Chen-Yu Tsai (2):
  media: rkvdec: Do not override sizeimage for output format
  media: rkvdec: Support dynamic resolution changes

 drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
 drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
 2 files changed, 23 insertions(+), 22 deletions(-)

-- 
2.33.0.882.g93a45727a2-goog

[PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

[Chen-Yu Tsai]

The rkvdec H.264 decoder currently overrides sizeimage for the output
format. This causes issues when userspace requires and requests a larger
buffer, but ends up with one of insufficient size.

Instead, only provide a default size if none was requested. This fixes
the video_decode_accelerator_tests from Chromium failing on the first
frame due to insufficient buffer space. It also aligns the behavior
of the rkvdec driver with the Hantro and Cedrus drivers.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 76e97cbe2512..951e19231da2 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
 
 	fmt->num_planes = 1;
-	fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
-				      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
+	if (!fmt->plane_fmt[0].sizeimage)
+		fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
+					      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
 	return 0;
 }
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 2/2] media: rkvdec: Support dynamic resolution changes

[Chen-Yu Tsai]

The mem-to-mem stateless decoder API specifies support for dynamic
resolution changes. In particular, the decoder should accept format
changes on the OUTPUT queue even when buffers have been allocated,
as long as it is not streaming.

Relax restrictions for S_FMT as described in the previous paragraph,
and as long as the codec format remains the same. This aligns it with
the Hantro and Cedrus decoders. This change was mostly based on commit
ae02d49493b5 ("media: hantro: Fix s_fmt for dynamic resolution changes").

Since rkvdec_s_fmt() is now just a wrapper around the output/capture
variants without any additional shared functionality, drop the wrapper
and call the respective functions directly.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/staging/media/rkvdec/rkvdec.c | 40 +++++++++++++--------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7131156c1f2c..3f3f96488d74 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -280,31 +280,20 @@ static int rkvdec_try_output_fmt(struct file *file, void *priv,
 	return 0;
 }
 
-static int rkvdec_s_fmt(struct file *file, void *priv,
-			struct v4l2_format *f,
-			int (*try_fmt)(struct file *, void *,
-				       struct v4l2_format *))
+static int rkvdec_s_capture_fmt(struct file *file, void *priv,
+				struct v4l2_format *f)
 {
 	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
 	struct vb2_queue *vq;
+	int ret;
 
-	if (!try_fmt)
-		return -EINVAL;
-
-	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
+	/* Change not allowed if queue is busy */
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 	if (vb2_is_busy(vq))
 		return -EBUSY;
 
-	return try_fmt(file, priv, f);
-}
-
-static int rkvdec_s_capture_fmt(struct file *file, void *priv,
-				struct v4l2_format *f)
-{
-	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
-	int ret;
-
-	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_capture_fmt);
+	ret = rkvdec_try_capture_fmt(file, priv, f);
 	if (ret)
 		return ret;
 
@@ -319,9 +308,20 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
 	const struct rkvdec_coded_fmt_desc *desc;
 	struct v4l2_format *cap_fmt;
-	struct vb2_queue *peer_vq;
+	struct vb2_queue *peer_vq, *vq;
 	int ret;
 
+	/*
+	 * In order to support dynamic resolution change, the decoder admits
+	 * a resolution change, as long as the pixelformat remains. Can't be
+	 * done if streaming.
+	 */
+	vq = v4l2_m2m_get_vq(m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
+	if (vb2_is_streaming(vq) ||
+	    (vb2_is_busy(vq) &&
+	     f->fmt.pix_mp.pixelformat != ctx->coded_fmt.fmt.pix_mp.pixelformat))
+		return -EBUSY;
+
 	/*
 	 * Since format change on the OUTPUT queue will reset the CAPTURE
 	 * queue, we can't allow doing so when the CAPTURE queue has buffers
@@ -331,7 +331,7 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
 	if (vb2_is_busy(peer_vq))
 		return -EBUSY;
 
-	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_output_fmt);
+	ret = rkvdec_try_output_fmt(file, priv, f);
 	if (ret)
 		return ret;
 
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Andrzej Pietrasiewicz]

Hi Chen-Yu Tsai,

W dniu 08.10.2021 o 12:04, Chen-Yu Tsai pisze:
> Hi everyone,
> 
> While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> behavioral differences compared to Hantro and Cedrus:
> 
> 1. The driver always overrides the sizeimage setting given by userspace
>     for the output format. This results in insufficient buffer space when
>     running the ChromeOS video_decode_accelerator_tests test program,
>     likely due to a small initial resolution followed by dynamic
>     resolution change.
> 
> 2. Doesn't support dynamic resolution change.
> 
> This small series fixes both and aligns the behavior with the other two
> stateless decoder drivers. This was tested on the downstream ChromeOS
> 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> have any other RK3399 devices set up to test video stuff, so testing
> would be very much appreciated.
> 
> Also, I'm not sure if user applications are required to check the value
> of sizeimage upon S_FMT return. If the value is different or too small,
> what can the application do besides fail? AFAICT it can't split the
> data of one frame (or slice) between different buffers.
> 
> Andrzej, I believe the second patch would conflict with your VP9 series.
> 

The conflict is rather trivial to solve. Adopting your version does not
change in any way (neither for better nor for worse) the fluster score
I get for vp9 with rkvdec on a rockpi4 using my vp9 series.

Regards,

Andrzej

> 
> Regards
> ChenYu
> 
> Chen-Yu Tsai (2):
>    media: rkvdec: Do not override sizeimage for output format
>    media: rkvdec: Support dynamic resolution changes
> 
>   drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
>   drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
>   2 files changed, 23 insertions(+), 22 deletions(-)
> 

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Nicolas Dufresne]

Hi Chen-Yu,

thanks for looking into this.

Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> Hi everyone,
> 
> While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> behavioral differences compared to Hantro and Cedrus:
> 
> 1. The driver always overrides the sizeimage setting given by userspace
>    for the output format. This results in insufficient buffer space when
>    running the ChromeOS video_decode_accelerator_tests test program,
>    likely due to a small initial resolution followed by dynamic
>    resolution change.
> 
> 2. Doesn't support dynamic resolution change.
> 
> This small series fixes both and aligns the behavior with the other two
> stateless decoder drivers. This was tested on the downstream ChromeOS
> 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> have any other RK3399 devices set up to test video stuff, so testing
> would be very much appreciated.
> 
> Also, I'm not sure if user applications are required to check the value
> of sizeimage upon S_FMT return. If the value is different or too small,
> what can the application do besides fail? AFAICT it can't split the
> data of one frame (or slice) between different buffers.

While most software out there just assumes that driver will do it right and
crash when it's not the case, application that do map the buffer to CPU must
read back the fmt structure as the drivers are all fail-safe and will modify
that structure to a set of valid value s for the context.

As for opposite direction (output vs capture) format being changed, this should
be documented in the spec, if you find it too unclear or missing for sateless
codec (I know it's there for stateful but can't remember, would have to re-read,
for stateless) let us know.

regards,
Nicolas

> 
> Andrzej, I believe the second patch would conflict with your VP9 series.
> 
> 
> Regards
> ChenYu
> 
> Chen-Yu Tsai (2):
>   media: rkvdec: Do not override sizeimage for output format
>   media: rkvdec: Support dynamic resolution changes
> 
>  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
>  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
>  2 files changed, 23 insertions(+), 22 deletions(-)
> 

Re: [PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

[Nicolas Dufresne]

Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> The rkvdec H.264 decoder currently overrides sizeimage for the output
> format. This causes issues when userspace requires and requests a larger
> buffer, but ends up with one of insufficient size.
> 
> Instead, only provide a default size if none was requested. This fixes
> the video_decode_accelerator_tests from Chromium failing on the first
> frame due to insufficient buffer space. It also aligns the behavior
> of the rkvdec driver with the Hantro and Cedrus drivers.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 76e97cbe2512..951e19231da2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
>  	struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
>  
>  	fmt->num_planes = 1;
> -	fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> -				      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
> +	if (!fmt->plane_fmt[0].sizeimage)
> +		fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> +					      RKVDEC_H264_MAX_DEPTH_IN_BYTES;

Note that the test is more strict then the spec, since this behaviour is within
spec. But in general, the application may have more information about the
incoming stream, the maximum encoded frame size would even be encoded in the
container (which is parsed in userspace). So I agree it will be more flexible to
accept userspace desired size. If that size is too small, userspace will fail at
filling it in the first place, and decoding won't be possible, that's all.

Perhaps we could make a recommendation in that sense in the spec ?

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

>  	return 0;
>  }
>  

Re: [PATCH 2/2] media: rkvdec: Support dynamic resolution changes

[Nicolas Dufresne]

Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> The mem-to-mem stateless decoder API specifies support for dynamic
> resolution changes. In particular, the decoder should accept format
> changes on the OUTPUT queue even when buffers have been allocated,
> as long as it is not streaming.

As commented in the code, it also requires the CAPTURE side not to be busy, not
sure if its worth clarifying, I don't really mind.

> 
> Relax restrictions for S_FMT as described in the previous paragraph,
> and as long as the codec format remains the same. This aligns it with
> the Hantro and Cedrus decoders. This change was mostly based on commit
> ae02d49493b5 ("media: hantro: Fix s_fmt for dynamic resolution changes").
> 
> Since rkvdec_s_fmt() is now just a wrapper around the output/capture
> variants without any additional shared functionality, drop the wrapper
> and call the respective functions directly.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 40 +++++++++++++--------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 7131156c1f2c..3f3f96488d74 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -280,31 +280,20 @@ static int rkvdec_try_output_fmt(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int rkvdec_s_fmt(struct file *file, void *priv,
> -			struct v4l2_format *f,
> -			int (*try_fmt)(struct file *, void *,
> -				       struct v4l2_format *))
> +static int rkvdec_s_capture_fmt(struct file *file, void *priv,
> +				struct v4l2_format *f)
>  {
>  	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
>  	struct vb2_queue *vq;
> +	int ret;
>  
> -	if (!try_fmt)
> -		return -EINVAL;
> -
> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> +	/* Change not allowed if queue is busy */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>  	if (vb2_is_busy(vq))
>  		return -EBUSY;
>  
> -	return try_fmt(file, priv, f);
> -}
> -
> -static int rkvdec_s_capture_fmt(struct file *file, void *priv,
> -				struct v4l2_format *f)
> -{
> -	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
> -	int ret;
> -
> -	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_capture_fmt);
> +	ret = rkvdec_try_capture_fmt(file, priv, f);
>  	if (ret)
>  		return ret;
>  
> @@ -319,9 +308,20 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
>  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
>  	const struct rkvdec_coded_fmt_desc *desc;
>  	struct v4l2_format *cap_fmt;
> -	struct vb2_queue *peer_vq;
> +	struct vb2_queue *peer_vq, *vq;
>  	int ret;
>  
> +	/*
> +	 * In order to support dynamic resolution change, the decoder admits
> +	 * a resolution change, as long as the pixelformat remains. Can't be
> +	 * done if streaming.
> +	 */
> +	vq = v4l2_m2m_get_vq(m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> +	if (vb2_is_streaming(vq) ||
> +	    (vb2_is_busy(vq) &&
> +	     f->fmt.pix_mp.pixelformat != ctx->coded_fmt.fmt.pix_mp.pixelformat))
> +		return -EBUSY;
> +
>  	/*
>  	 * Since format change on the OUTPUT queue will reset the CAPTURE
>  	 * queue, we can't allow doing so when the CAPTURE queue has buffers
> @@ -331,7 +331,7 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
>  	if (vb2_is_busy(peer_vq))
>  		return -EBUSY;
>  
> -	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_output_fmt);
> +	ret = rkvdec_try_output_fmt(file, priv, f);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Chen-Yu Tsai]

Hi,

On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Chen-Yu,
>
> thanks for looking into this.
>
> Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > Hi everyone,
> >
> > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > behavioral differences compared to Hantro and Cedrus:
> >
> > 1. The driver always overrides the sizeimage setting given by userspace
> >    for the output format. This results in insufficient buffer space when
> >    running the ChromeOS video_decode_accelerator_tests test program,
> >    likely due to a small initial resolution followed by dynamic
> >    resolution change.
> >
> > 2. Doesn't support dynamic resolution change.
> >
> > This small series fixes both and aligns the behavior with the other two
> > stateless decoder drivers. This was tested on the downstream ChromeOS
> > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > have any other RK3399 devices set up to test video stuff, so testing
> > would be very much appreciated.
> >
> > Also, I'm not sure if user applications are required to check the value
> > of sizeimage upon S_FMT return. If the value is different or too small,
> > what can the application do besides fail? AFAICT it can't split the
> > data of one frame (or slice) between different buffers.
>
> While most software out there just assumes that driver will do it right and
> crash when it's not the case, application that do map the buffer to CPU must
> read back the fmt structure as the drivers are all fail-safe and will modify
> that structure to a set of valid value s for the context.

I believe what is happening in Chromium is that the decoder is opened with
some default settings, including the smallest viable resolution for the
output side, and the buffers allocated accordingly. When dynamic resolution
change happens, the decoder does not check if the current buffers are
sufficiently sized; it just assumes that they are. And when it starts
pushing data into the buffers, it realizes they are too small and fails.

The spec also says:

    Clients are allowed to set the sizeimage field for variable length
    compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
    VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
    or it may modify the provided value based on alignment requirements or
    minimum/maximum size requirements.

The spec only guarantees that the buffers are of sufficient size for the
resolution configured at the time they were allocated/requested.

So I think my first patch is a workaround for a somewhat broken userspace.
But it seems the other stateless drivers are providing similar behavior,
as I previously mentioned.

> As for opposite direction (output vs capture) format being changed, this should
> be documented in the spec, if you find it too unclear or missing for sateless
> codec (I know it's there for stateful but can't remember, would have to re-read,
> for stateless) let us know.

AFAICT the capture side is working OK and to spec.


Regards
ChenYu

> regards,
> Nicolas
>
> >
> > Andrzej, I believe the second patch would conflict with your VP9 series.
> >
> >
> > Regards
> > ChenYu
> >
> > Chen-Yu Tsai (2):
> >   media: rkvdec: Do not override sizeimage for output format
> >   media: rkvdec: Support dynamic resolution changes
> >
> >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> >  2 files changed, 23 insertions(+), 22 deletions(-)
> >
>
>

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Nicolas Dufresne]

Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> Hi,
> 
> On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > 
> > Hi Chen-Yu,
> > 
> > thanks for looking into this.
> > 
> > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > Hi everyone,
> > > 
> > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > behavioral differences compared to Hantro and Cedrus:
> > > 
> > > 1. The driver always overrides the sizeimage setting given by userspace
> > >    for the output format. This results in insufficient buffer space when
> > >    running the ChromeOS video_decode_accelerator_tests test program,
> > >    likely due to a small initial resolution followed by dynamic
> > >    resolution change.
> > > 
> > > 2. Doesn't support dynamic resolution change.
> > > 
> > > This small series fixes both and aligns the behavior with the other two
> > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > have any other RK3399 devices set up to test video stuff, so testing
> > > would be very much appreciated.
> > > 
> > > Also, I'm not sure if user applications are required to check the value
> > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > what can the application do besides fail? AFAICT it can't split the
> > > data of one frame (or slice) between different buffers.
> > 
> > While most software out there just assumes that driver will do it right and
> > crash when it's not the case, application that do map the buffer to CPU must
> > read back the fmt structure as the drivers are all fail-safe and will modify
> > that structure to a set of valid value s for the context.
> 
> I believe what is happening in Chromium is that the decoder is opened with
> some default settings, including the smallest viable resolution for the
> output side, and the buffers allocated accordingly. When dynamic resolution
> change happens, the decoder does not check if the current buffers are
> sufficiently sized; it just assumes that they are. And when it starts
> pushing data into the buffers, it realizes they are too small and fails.
> 
> The spec also says:
> 
>     Clients are allowed to set the sizeimage field for variable length
>     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
>     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
>     or it may modify the provided value based on alignment requirements or
>     minimum/maximum size requirements.
> 
> The spec only guarantees that the buffers are of sufficient size for the
> resolution configured at the time they were allocated/requested.
> 
> So I think my first patch is a workaround for a somewhat broken userspace.
> But it seems the other stateless drivers are providing similar behavior,
> as I previously mentioned.

That's what I mean, this is not a driver bug strictly speaking (assuming it does
guaranty the buffer size is sufficient) but it is without your change
inconvenient, as userspace may be aware of the largest resolution it will
decode, and may want to allocate larger buffer upfront.

As per Chromium bug, this is being addressed already. Thanks for this driver
improvement.

> 
> > As for opposite direction (output vs capture) format being changed, this should
> > be documented in the spec, if you find it too unclear or missing for sateless
> > codec (I know it's there for stateful but can't remember, would have to re-read,
> > for stateless) let us know.
> 
> AFAICT the capture side is working OK and to spec.
> 
> 
> Regards
> ChenYu
> 
> > regards,
> > Nicolas
> > 
> > > 
> > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > 
> > > 
> > > Regards
> > > ChenYu
> > > 
> > > Chen-Yu Tsai (2):
> > >   media: rkvdec: Do not override sizeimage for output format
> > >   media: rkvdec: Support dynamic resolution changes
> > > 
> > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > 
> > 
> > 

Re: [PATCH 1/2] media: rkvdec: Do not override sizeimage for output format

[Ezequiel Garcia]

Hi Chen-Yu,

On Fri, Oct 08, 2021 at 06:04:22PM +0800, Chen-Yu Tsai wrote:
> The rkvdec H.264 decoder currently overrides sizeimage for the output
> format. This causes issues when userspace requires and requests a larger
> buffer, but ends up with one of insufficient size.
> 
> Instead, only provide a default size if none was requested. This fixes
> the video_decode_accelerator_tests from Chromium failing on the first
> frame due to insufficient buffer space. It also aligns the behavior
> of the rkvdec driver with the Hantro and Cedrus drivers.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

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

Thanks for taking care of this!

Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 76e97cbe2512..951e19231da2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1015,8 +1015,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
>  	struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
>  
>  	fmt->num_planes = 1;
> -	fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> -				      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
> +	if (!fmt->plane_fmt[0].sizeimage)
> +		fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
> +					      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
>  	return 0;
>  }
>  
> -- 
> 2.33.0.882.g93a45727a2-goog
> 

Re: [PATCH 2/2] media: rkvdec: Support dynamic resolution changes

[Ezequiel Garcia]

Hi Chen-Yu,

On Fri, Oct 08, 2021 at 06:04:23PM +0800, Chen-Yu Tsai wrote:
> The mem-to-mem stateless decoder API specifies support for dynamic
> resolution changes. In particular, the decoder should accept format
> changes on the OUTPUT queue even when buffers have been allocated,
> as long as it is not streaming.
> 
> Relax restrictions for S_FMT as described in the previous paragraph,
> and as long as the codec format remains the same. This aligns it with
> the Hantro and Cedrus decoders. This change was mostly based on commit
> ae02d49493b5 ("media: hantro: Fix s_fmt for dynamic resolution changes").
> 
> Since rkvdec_s_fmt() is now just a wrapper around the output/capture
> variants without any additional shared functionality, drop the wrapper
> and call the respective functions directly.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

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

Seems we forgot to port Hantro changes over here, so thanks a lot
for picking this up.

Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 40 +++++++++++++--------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 7131156c1f2c..3f3f96488d74 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -280,31 +280,20 @@ static int rkvdec_try_output_fmt(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int rkvdec_s_fmt(struct file *file, void *priv,
> -			struct v4l2_format *f,
> -			int (*try_fmt)(struct file *, void *,
> -				       struct v4l2_format *))
> +static int rkvdec_s_capture_fmt(struct file *file, void *priv,
> +				struct v4l2_format *f)
>  {
>  	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
>  	struct vb2_queue *vq;
> +	int ret;
>  
> -	if (!try_fmt)
> -		return -EINVAL;
> -
> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> +	/* Change not allowed if queue is busy */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>  	if (vb2_is_busy(vq))
>  		return -EBUSY;
>  
> -	return try_fmt(file, priv, f);
> -}
> -
> -static int rkvdec_s_capture_fmt(struct file *file, void *priv,
> -				struct v4l2_format *f)
> -{
> -	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
> -	int ret;
> -
> -	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_capture_fmt);
> +	ret = rkvdec_try_capture_fmt(file, priv, f);
>  	if (ret)
>  		return ret;
>  
> @@ -319,9 +308,20 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
>  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
>  	const struct rkvdec_coded_fmt_desc *desc;
>  	struct v4l2_format *cap_fmt;
> -	struct vb2_queue *peer_vq;
> +	struct vb2_queue *peer_vq, *vq;
>  	int ret;
>  
> +	/*
> +	 * In order to support dynamic resolution change, the decoder admits
> +	 * a resolution change, as long as the pixelformat remains. Can't be
> +	 * done if streaming.
> +	 */
> +	vq = v4l2_m2m_get_vq(m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> +	if (vb2_is_streaming(vq) ||
> +	    (vb2_is_busy(vq) &&
> +	     f->fmt.pix_mp.pixelformat != ctx->coded_fmt.fmt.pix_mp.pixelformat))
> +		return -EBUSY;
> +
>  	/*
>  	 * Since format change on the OUTPUT queue will reset the CAPTURE
>  	 * queue, we can't allow doing so when the CAPTURE queue has buffers
> @@ -331,7 +331,7 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
>  	if (vb2_is_busy(peer_vq))
>  		return -EBUSY;
>  
> -	ret = rkvdec_s_fmt(file, priv, f, rkvdec_try_output_fmt);
> +	ret = rkvdec_try_output_fmt(file, priv, f);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.33.0.882.g93a45727a2-goog
> 

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Chen-Yu Tsai]

On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> > Hi,
> >
> > On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > >
> > > Hi Chen-Yu,
> > >
> > > thanks for looking into this.
> > >
> > > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > > Hi everyone,
> > > >
> > > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > > behavioral differences compared to Hantro and Cedrus:
> > > >
> > > > 1. The driver always overrides the sizeimage setting given by userspace
> > > >    for the output format. This results in insufficient buffer space when
> > > >    running the ChromeOS video_decode_accelerator_tests test program,
> > > >    likely due to a small initial resolution followed by dynamic
> > > >    resolution change.
> > > >
> > > > 2. Doesn't support dynamic resolution change.
> > > >
> > > > This small series fixes both and aligns the behavior with the other two
> > > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > > have any other RK3399 devices set up to test video stuff, so testing
> > > > would be very much appreciated.
> > > >
> > > > Also, I'm not sure if user applications are required to check the value
> > > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > > what can the application do besides fail? AFAICT it can't split the
> > > > data of one frame (or slice) between different buffers.
> > >
> > > While most software out there just assumes that driver will do it right and
> > > crash when it's not the case, application that do map the buffer to CPU must
> > > read back the fmt structure as the drivers are all fail-safe and will modify
> > > that structure to a set of valid value s for the context.
> >
> > I believe what is happening in Chromium is that the decoder is opened with
> > some default settings, including the smallest viable resolution for the
> > output side, and the buffers allocated accordingly. When dynamic resolution
> > change happens, the decoder does not check if the current buffers are
> > sufficiently sized; it just assumes that they are. And when it starts
> > pushing data into the buffers, it realizes they are too small and fails.
> >
> > The spec also says:
> >
> >     Clients are allowed to set the sizeimage field for variable length
> >     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
> >     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
> >     or it may modify the provided value based on alignment requirements or
> >     minimum/maximum size requirements.
> >
> > The spec only guarantees that the buffers are of sufficient size for the
> > resolution configured at the time they were allocated/requested.
> >
> > So I think my first patch is a workaround for a somewhat broken userspace.
> > But it seems the other stateless drivers are providing similar behavior,
> > as I previously mentioned.
>
> That's what I mean, this is not a driver bug strictly speaking (assuming it does
> guaranty the buffer size is sufficient) but it is without your change
> inconvenient, as userspace may be aware of the largest resolution it will
> decode, and may want to allocate larger buffer upfront.

Thinking about this more, I think a few follow up fixes for each driver
are in order. The spec implies that the driver should override the value
should userspace give some unrealistic value, such as asking for a 256 byte
buffer for a 4K frame size.

Cedrus (CCing Jernej) comes close, but a 1K buffer might not be enough for
really large frames, even though it's slice based?

ChenYu


> As per Chromium bug, this is being addressed already. Thanks for this driver
> improvement.
>
> >
> > > As for opposite direction (output vs capture) format being changed, this should
> > > be documented in the spec, if you find it too unclear or missing for sateless
> > > codec (I know it's there for stateful but can't remember, would have to re-read,
> > > for stateless) let us know.
> >
> > AFAICT the capture side is working OK and to spec.
> >
> >
> > Regards
> > ChenYu
> >
> > > regards,
> > > Nicolas
> > >
> > > >
> > > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > >
> > > >
> > > > Regards
> > > > ChenYu
> > > >
> > > > Chen-Yu Tsai (2):
> > > >   media: rkvdec: Do not override sizeimage for output format
> > > >   media: rkvdec: Support dynamic resolution changes
> > > >
> > > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > >
> > >
> > >
>
>

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Ezequiel Garcia]

On Thu, 14 Oct 2021 at 04:31, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> >
> > Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> > > Hi,
> > >
> > > On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > >
> > > > Hi Chen-Yu,
> > > >
> > > > thanks for looking into this.
> > > >
> > > > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > > > Hi everyone,
> > > > >
> > > > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > > > behavioral differences compared to Hantro and Cedrus:
> > > > >
> > > > > 1. The driver always overrides the sizeimage setting given by userspace
> > > > >    for the output format. This results in insufficient buffer space when
> > > > >    running the ChromeOS video_decode_accelerator_tests test program,
> > > > >    likely due to a small initial resolution followed by dynamic
> > > > >    resolution change.
> > > > >
> > > > > 2. Doesn't support dynamic resolution change.
> > > > >
> > > > > This small series fixes both and aligns the behavior with the other two
> > > > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > > > have any other RK3399 devices set up to test video stuff, so testing
> > > > > would be very much appreciated.
> > > > >
> > > > > Also, I'm not sure if user applications are required to check the value
> > > > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > > > what can the application do besides fail? AFAICT it can't split the
> > > > > data of one frame (or slice) between different buffers.
> > > >
> > > > While most software out there just assumes that driver will do it right and
> > > > crash when it's not the case, application that do map the buffer to CPU must
> > > > read back the fmt structure as the drivers are all fail-safe and will modify
> > > > that structure to a set of valid value s for the context.
> > >
> > > I believe what is happening in Chromium is that the decoder is opened with
> > > some default settings, including the smallest viable resolution for the
> > > output side, and the buffers allocated accordingly. When dynamic resolution
> > > change happens, the decoder does not check if the current buffers are
> > > sufficiently sized; it just assumes that they are. And when it starts
> > > pushing data into the buffers, it realizes they are too small and fails.
> > >
> > > The spec also says:
> > >
> > >     Clients are allowed to set the sizeimage field for variable length
> > >     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
> > >     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
> > >     or it may modify the provided value based on alignment requirements or
> > >     minimum/maximum size requirements.
> > >
> > > The spec only guarantees that the buffers are of sufficient size for the
> > > resolution configured at the time they were allocated/requested.
> > >
> > > So I think my first patch is a workaround for a somewhat broken userspace.
> > > But it seems the other stateless drivers are providing similar behavior,
> > > as I previously mentioned.
> >
> > That's what I mean, this is not a driver bug strictly speaking (assuming it does
> > guaranty the buffer size is sufficient) but it is without your change
> > inconvenient, as userspace may be aware of the largest resolution it will
> > decode, and may want to allocate larger buffer upfront.
>
> Thinking about this more, I think a few follow up fixes for each driver
> are in order. The spec implies that the driver should override the value
> should userspace give some unrealistic value, such as asking for a 256 byte
> buffer for a 4K frame size.
>

Where is the spec implying that?

This is encoded content, so I'm really inclined to avoid this path.
Having the driver decide what is "unrealistic" would mean some
heuristics in the drivers for something that should really come from userspace.

Thanks,
Ezequiel

> Cedrus (CCing Jernej) comes close, but a 1K buffer might not be enough for
> really large frames, even though it's slice based?
>
> ChenYu
>
>
> > As per Chromium bug, this is being addressed already. Thanks for this driver
> > improvement.
> >
> > >
> > > > As for opposite direction (output vs capture) format being changed, this should
> > > > be documented in the spec, if you find it too unclear or missing for sateless
> > > > codec (I know it's there for stateful but can't remember, would have to re-read,
> > > > for stateless) let us know.
> > >
> > > AFAICT the capture side is working OK and to spec.
> > >
> > >
> > > Regards
> > > ChenYu
> > >
> > > > regards,
> > > > Nicolas
> > > >
> > > > >
> > > > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > > >
> > > > >
> > > > > Regards
> > > > > ChenYu
> > > > >
> > > > > Chen-Yu Tsai (2):
> > > > >   media: rkvdec: Do not override sizeimage for output format
> > > > >   media: rkvdec: Support dynamic resolution changes
> > > > >
> > > > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > > > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > > > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > > >
> > > >
> > > >
> >
> >

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Chen-Yu Tsai]

On Thu, Oct 14, 2021 at 4:46 PM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> On Thu, 14 Oct 2021 at 04:31, Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > >
> > > Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> > > > Hi,
> > > >
> > > > On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > > >
> > > > > Hi Chen-Yu,
> > > > >
> > > > > thanks for looking into this.
> > > > >
> > > > > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > > > > Hi everyone,
> > > > > >
> > > > > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > > > > behavioral differences compared to Hantro and Cedrus:
> > > > > >
> > > > > > 1. The driver always overrides the sizeimage setting given by userspace
> > > > > >    for the output format. This results in insufficient buffer space when
> > > > > >    running the ChromeOS video_decode_accelerator_tests test program,
> > > > > >    likely due to a small initial resolution followed by dynamic
> > > > > >    resolution change.
> > > > > >
> > > > > > 2. Doesn't support dynamic resolution change.
> > > > > >
> > > > > > This small series fixes both and aligns the behavior with the other two
> > > > > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > > > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > > > > have any other RK3399 devices set up to test video stuff, so testing
> > > > > > would be very much appreciated.
> > > > > >
> > > > > > Also, I'm not sure if user applications are required to check the value
> > > > > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > > > > what can the application do besides fail? AFAICT it can't split the
> > > > > > data of one frame (or slice) between different buffers.
> > > > >
> > > > > While most software out there just assumes that driver will do it right and
> > > > > crash when it's not the case, application that do map the buffer to CPU must
> > > > > read back the fmt structure as the drivers are all fail-safe and will modify
> > > > > that structure to a set of valid value s for the context.
> > > >
> > > > I believe what is happening in Chromium is that the decoder is opened with
> > > > some default settings, including the smallest viable resolution for the
> > > > output side, and the buffers allocated accordingly. When dynamic resolution
> > > > change happens, the decoder does not check if the current buffers are
> > > > sufficiently sized; it just assumes that they are. And when it starts
> > > > pushing data into the buffers, it realizes they are too small and fails.
> > > >
> > > > The spec also says:
> > > >
> > > >     Clients are allowed to set the sizeimage field for variable length
> > > >     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
> > > >     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
> > > >     or it may modify the provided value based on alignment requirements or
> > > >     minimum/maximum size requirements.
> > > >
> > > > The spec only guarantees that the buffers are of sufficient size for the
> > > > resolution configured at the time they were allocated/requested.
> > > >
> > > > So I think my first patch is a workaround for a somewhat broken userspace.
> > > > But it seems the other stateless drivers are providing similar behavior,
> > > > as I previously mentioned.
> > >
> > > That's what I mean, this is not a driver bug strictly speaking (assuming it does
> > > guaranty the buffer size is sufficient) but it is without your change
> > > inconvenient, as userspace may be aware of the largest resolution it will
> > > decode, and may want to allocate larger buffer upfront.
> >
> > Thinking about this more, I think a few follow up fixes for each driver
> > are in order. The spec implies that the driver should override the value
> > should userspace give some unrealistic value, such as asking for a 256 byte
> > buffer for a 4K frame size.
> >
>
> Where is the spec implying that?

In Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst:

    Clients are allowed to set the sizeimage field for variable length
    compressed data flagged with ``V4L2_FMT_FLAG_COMPRESSED`` at
    :ref:`VIDIOC_ENUM_FMT`, but the driver may ignore it and set the
    value itself, or it may modify the provided value based on
    alignment requirements or minimum/maximum size requirements.

I guess I read "minimum/maximum size requirements" a bit liberally.
Maybe it refers to how much buffer space the hardware can address or
program for each request?

> This is encoded content, so I'm really inclined to avoid this path.
> Having the driver decide what is "unrealistic" would mean some
> heuristics in the drivers for something that should really come from userspace.

And if the driver refuses to give adequate buffer space, then it's a bug?


Regards
ChenYu


> Thanks,
> Ezequiel
>
> > Cedrus (CCing Jernej) comes close, but a 1K buffer might not be enough for
> > really large frames, even though it's slice based?
> >
> > ChenYu
> >
> >
> > > As per Chromium bug, this is being addressed already. Thanks for this driver
> > > improvement.
> > >
> > > >
> > > > > As for opposite direction (output vs capture) format being changed, this should
> > > > > be documented in the spec, if you find it too unclear or missing for sateless
> > > > > codec (I know it's there for stateful but can't remember, would have to re-read,
> > > > > for stateless) let us know.
> > > >
> > > > AFAICT the capture side is working OK and to spec.
> > > >
> > > >
> > > > Regards
> > > > ChenYu
> > > >
> > > > > regards,
> > > > > Nicolas
> > > > >
> > > > > >
> > > > > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > > > >
> > > > > >
> > > > > > Regards
> > > > > > ChenYu
> > > > > >
> > > > > > Chen-Yu Tsai (2):
> > > > > >   media: rkvdec: Do not override sizeimage for output format
> > > > > >   media: rkvdec: Support dynamic resolution changes
> > > > > >
> > > > > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > > > > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > > > > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > > > >
> > > > >
> > > > >
> > >
> > >

Re: [PATCH 0/2] media: rkvdec: Align decoder behavior with Hantro and Cedrus

[Ezequiel Garcia]

On Thu, 14 Oct 2021 at 05:57, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Thu, Oct 14, 2021 at 4:46 PM Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> >
> > On Thu, 14 Oct 2021 at 04:31, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > On Wed, Oct 13, 2021 at 9:40 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > >
> > > > Le mercredi 13 octobre 2021 à 15:05 +0800, Chen-Yu Tsai a écrit :
> > > > > Hi,
> > > > >
> > > > > On Fri, Oct 8, 2021 at 11:42 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > > > >
> > > > > > Hi Chen-Yu,
> > > > > >
> > > > > > thanks for looking into this.
> > > > > >
> > > > > > Le vendredi 08 octobre 2021 à 18:04 +0800, Chen-Yu Tsai a écrit :
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > While working on the rkvdec H.264 decoder for ChromeOS, I noticed some
> > > > > > > behavioral differences compared to Hantro and Cedrus:
> > > > > > >
> > > > > > > 1. The driver always overrides the sizeimage setting given by userspace
> > > > > > >    for the output format. This results in insufficient buffer space when
> > > > > > >    running the ChromeOS video_decode_accelerator_tests test program,
> > > > > > >    likely due to a small initial resolution followed by dynamic
> > > > > > >    resolution change.
> > > > > > >
> > > > > > > 2. Doesn't support dynamic resolution change.
> > > > > > >
> > > > > > > This small series fixes both and aligns the behavior with the other two
> > > > > > > stateless decoder drivers. This was tested on the downstream ChromeOS
> > > > > > > 5.10 kernel with ChromeOS. Also compiled tested on mainline but I don't
> > > > > > > have any other RK3399 devices set up to test video stuff, so testing
> > > > > > > would be very much appreciated.
> > > > > > >
> > > > > > > Also, I'm not sure if user applications are required to check the value
> > > > > > > of sizeimage upon S_FMT return. If the value is different or too small,
> > > > > > > what can the application do besides fail? AFAICT it can't split the
> > > > > > > data of one frame (or slice) between different buffers.
> > > > > >
> > > > > > While most software out there just assumes that driver will do it right and
> > > > > > crash when it's not the case, application that do map the buffer to CPU must
> > > > > > read back the fmt structure as the drivers are all fail-safe and will modify
> > > > > > that structure to a set of valid value s for the context.
> > > > >
> > > > > I believe what is happening in Chromium is that the decoder is opened with
> > > > > some default settings, including the smallest viable resolution for the
> > > > > output side, and the buffers allocated accordingly. When dynamic resolution
> > > > > change happens, the decoder does not check if the current buffers are
> > > > > sufficiently sized; it just assumes that they are. And when it starts
> > > > > pushing data into the buffers, it realizes they are too small and fails.
> > > > >
> > > > > The spec also says:
> > > > >
> > > > >     Clients are allowed to set the sizeimage field for variable length
> > > > >     compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at ioctl
> > > > >     VIDIOC_ENUM_FMT, but the driver may ignore it and set the value itself,
> > > > >     or it may modify the provided value based on alignment requirements or
> > > > >     minimum/maximum size requirements.
> > > > >
> > > > > The spec only guarantees that the buffers are of sufficient size for the
> > > > > resolution configured at the time they were allocated/requested.
> > > > >
> > > > > So I think my first patch is a workaround for a somewhat broken userspace.
> > > > > But it seems the other stateless drivers are providing similar behavior,
> > > > > as I previously mentioned.
> > > >
> > > > That's what I mean, this is not a driver bug strictly speaking (assuming it does
> > > > guaranty the buffer size is sufficient) but it is without your change
> > > > inconvenient, as userspace may be aware of the largest resolution it will
> > > > decode, and may want to allocate larger buffer upfront.
> > >
> > > Thinking about this more, I think a few follow up fixes for each driver
> > > are in order. The spec implies that the driver should override the value
> > > should userspace give some unrealistic value, such as asking for a 256 byte
> > > buffer for a 4K frame size.
> > >
> >
> > Where is the spec implying that?
>
> In Documentation/userspace-api/media/v4l/pixfmt-v4l2-mplane.rst:
>
>     Clients are allowed to set the sizeimage field for variable length
>     compressed data flagged with ``V4L2_FMT_FLAG_COMPRESSED`` at
>     :ref:`VIDIOC_ENUM_FMT`, but the driver may ignore it and set the
>     value itself, or it may modify the provided value based on
>     alignment requirements or minimum/maximum size requirements.
>
> I guess I read "minimum/maximum size requirements" a bit liberally.
> Maybe it refers to how much buffer space the hardware can address or
> program for each request?
>

My interpretation is that the driver may modify the value to adjust
for hardware requirements in terms of buffer alignment hardware requirements
and buffer size hardware requirements.

> > This is encoded content, so I'm really inclined to avoid this path.
> > Having the driver decide what is "unrealistic" would mean some
> > heuristics in the drivers for something that should really come from userspace.
>
> And if the driver refuses to give adequate buffer space, then it's a bug?
>

Not sure what you have in mind here. S_FMT typically won't fail,
as the spec mentions
https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-fmt.html?highlight=s_fmt#c.VIDIOC_S_FMT.
So the only way a driver can refuse, as you mentioned,
is by overriding a parameter (which is what you are fixing).

"""
Drivers should not return an error code unless the type field is
invalid, this is a mechanism to fathom device capabilities and to
approach parameters acceptable for both the application and driver. On
success the driver may program the hardware, allocate resources and
generally prepare for data exchange.
"""

The driver should just use what application provided for the buffer sizeimage.
Let me quote your cover letter question:

"""
Also, I'm not sure if user applications are required to check the value
of sizeimage upon S_FMT return. If the value is different or too small,
what can the application do besides fail? AFAICT it can't split the
data of one frame (or slice) between different buffers.
"""

If the sizeimage is not adequate and the application knows this,
then it can only conclude the operation is not possible on this hardware.

However, if I understand correctly, that's why you are fixing rkvdec.
Because the rkvdec driver shouldn't override the value from the application,
since that may be wrong (besides being suboptimal).

Hope this makes sense...

Thanks,
Ezequiel

>
> Regards
> ChenYu
>
>
> > Thanks,
> > Ezequiel
> >
> > > Cedrus (CCing Jernej) comes close, but a 1K buffer might not be enough for
> > > really large frames, even though it's slice based?
> > >
> > > ChenYu
> > >
> > >
> > > > As per Chromium bug, this is being addressed already. Thanks for this driver
> > > > improvement.
> > > >
> > > > >
> > > > > > As for opposite direction (output vs capture) format being changed, this should
> > > > > > be documented in the spec, if you find it too unclear or missing for sateless
> > > > > > codec (I know it's there for stateful but can't remember, would have to re-read,
> > > > > > for stateless) let us know.
> > > > >
> > > > > AFAICT the capture side is working OK and to spec.
> > > > >
> > > > >
> > > > > Regards
> > > > > ChenYu
> > > > >
> > > > > > regards,
> > > > > > Nicolas
> > > > > >
> > > > > > >
> > > > > > > Andrzej, I believe the second patch would conflict with your VP9 series.
> > > > > > >
> > > > > > >
> > > > > > > Regards
> > > > > > > ChenYu
> > > > > > >
> > > > > > > Chen-Yu Tsai (2):
> > > > > > >   media: rkvdec: Do not override sizeimage for output format
> > > > > > >   media: rkvdec: Support dynamic resolution changes
> > > > > > >
> > > > > > >  drivers/staging/media/rkvdec/rkvdec-h264.c |  5 +--
> > > > > > >  drivers/staging/media/rkvdec/rkvdec.c      | 40 +++++++++++-----------
> > > > > > >  2 files changed, 23 insertions(+), 22 deletions(-)
> > > > > > >
> > > > > >
> > > > > >
> > > >
> > > >