media: iris: set default size when S_FMT is called with zero size

[PATCH] media: iris: set default size when S_FMT is called with zero size

Posted by Val Packett 3 months, 3 weeks ago

According to 4.5.1.5 of the M2M stateful decoder UAPI documentation,
providing the width and the height is "required only if it cannot be
parsed from the stream", otherwise zero can be passed.

The iris driver would only set the state fields to DEFAULT_WIDTH/HEIGHT
once upon init, but they would get overwritten with zeroes from one S_FMT
call and the next S_FMT call would already see zeroes in place of the
defaults. For clients that used that sequence and did not pass a size,
such as rpi-ffmpeg, this would then result in REQBUFS failing due to
the zero size remembered in the state.

Fix by explicitly falling back to the defaults in S_FMT.

Fixes: b530b95de22c ("media: iris: implement s_fmt, g_fmt and try_fmt ioctls")
Link: https://github.com/jc-kynesim/rpi-ffmpeg/issues/103
Signed-off-by: Val Packett <val@packett.cool>
---
Somehow Venus didn't have this issue and didn't explicitly handle this..

I'm not familiar with this code so if there's a better way to comply
with the UAPI requirements by just not even getting to overwrite the
state with the provided 0 size, I could not figure it out.

Still, let's get this fixed one way or another.

Thanks,
~val
---
 drivers/media/platform/qcom/iris/iris_vdec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c
index ae13c3e1b426..6be09d82e24d 100644
--- a/drivers/media/platform/qcom/iris/iris_vdec.c
+++ b/drivers/media/platform/qcom/iris/iris_vdec.c
@@ -196,6 +196,11 @@ int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f)
 	if (vb2_is_busy(q))
 		return -EBUSY;
 
+	if (f->fmt.pix_mp.width == 0 && f->fmt.pix_mp.height == 0) {
+		f->fmt.pix_mp.width = DEFAULT_WIDTH;
+		f->fmt.pix_mp.height = DEFAULT_HEIGHT;
+	}
+
 	iris_vdec_try_fmt(inst, f);
 
 	switch (f->type) {
-- 
2.51.0

Re: [PATCH] media: iris: set default size when S_FMT is called with zero size

Posted by Bryan O'Donoghue 3 months, 1 week ago

On 13/10/2025 00:50, Val Packett wrote:
>   drivers/media/platform/qcom/iris/iris_vdec.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c
> index ae13c3e1b426..6be09d82e24d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vdec.c
> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c
> @@ -196,6 +196,11 @@ int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f)
>   	if (vb2_is_busy(q))
>   		return -EBUSY;
> 
> +	if (f->fmt.pix_mp.width == 0 && f->fmt.pix_mp.height == 0) {
> +		f->fmt.pix_mp.width = DEFAULT_WIDTH;
> +		f->fmt.pix_mp.height = DEFAULT_HEIGHT;
> +	}
> +
>   	iris_vdec_try_fmt(inst, f);
> 
>   	switch (f->type) {
> --

Doesn't venus do

orig_pixmap = *pixmap;

try_fmt();

format.fmt.pix_mp.width = orig_pixmp.width;
format.fmt.pix_mp.height = orig_pixmp.height;

should you fall back to DEFAULT_WIDTH/HEIGHT or to orig_pixmp.width/height ?

---
bod

Re: [PATCH] media: iris: set default size when S_FMT is called with zero size

Posted by Val Packett 1 month, 1 week ago

On 10/28/25 10:19 AM, Bryan O'Donoghue wrote:
> On 13/10/2025 00:50, Val Packett wrote:
>> drivers/media/platform/qcom/iris/iris_vdec.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c 
>> b/drivers/media/platform/qcom/iris/iris_vdec.c
>> index ae13c3e1b426..6be09d82e24d 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vdec.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c
>> @@ -196,6 +196,11 @@ int iris_vdec_s_fmt(struct iris_inst *inst, 
>> struct v4l2_format *f)
>>       if (vb2_is_busy(q))
>>           return -EBUSY;
>>
>> +    if (f->fmt.pix_mp.width == 0 && f->fmt.pix_mp.height == 0) {
>> +        f->fmt.pix_mp.width = DEFAULT_WIDTH;
>> +        f->fmt.pix_mp.height = DEFAULT_HEIGHT;
>> +    }
>> +
>>       iris_vdec_try_fmt(inst, f);
>>
>>       switch (f->type) {
>> -- 
>
> Doesn't venus do
>
> orig_pixmap = *pixmap;
>
> try_fmt();
>
> format.fmt.pix_mp.width = orig_pixmp.width;
> format.fmt.pix_mp.height = orig_pixmp.height;
>
> should you fall back to DEFAULT_WIDTH/HEIGHT or to 
> orig_pixmp.width/height ? 

Hi– sorry for the late reply and happy holidays :)

The patch does the fallback *before* try_fmt().

I have just retested everything, even with yesterday's new "misc fixes" 
patchset 
(https://lore.kernel.org/all/20251224-iris-fixes-v1-0-5f79861700ec@oss.qualcomm.com/). 
The issue is still present, the 0x0 size is what iris_vdec_s_fmt gets 
before try_fmt().

What the rpi-ffmpeg developer said 
(https://github.com/jc-kynesim/rpi-ffmpeg/issues/103) is:

- V4L2 very clearly says that you should be able to pass anything into 
S_FMT and it should set the returned values to something valid
- S_FMT should not only not complain, it should return valid values in 
the structure passed to it which will then be used in the REQBUFS
- the buffers being allocated are for the coded bitstream (remember in 
V4L2 speak OUTPUT=source, CAPTURE=destination); width/height doesn't 
really mean a lot here but buffer size does
- 
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-decoder.html 
4.5.1.5 states "width, height: coded resolution of the stream; required 
only if it cannot be parsed from the stream for the given coded format; 
otherwise the decoder will use this resolution as a placeholder 
resolution that will likely change as soon as it can parse the actual 
coded resolution from the stream"

So since this is a deliberate zero request, and V4L2 requires returning 
a valid placeholder resolution, it should be DEFAULT indeed.

I'll resend with a comment as requested by Dikshita.

~val

Re: [PATCH] media: iris: set default size when S_FMT is called with zero size

Posted by Dikshita Agarwal 3 months, 1 week ago


On 10/13/2025 5:20 AM, Val Packett wrote:
> According to 4.5.1.5 of the M2M stateful decoder UAPI documentation,
> providing the width and the height is "required only if it cannot be
> parsed from the stream", otherwise zero can be passed.
> 
> The iris driver would only set the state fields to DEFAULT_WIDTH/HEIGHT
> once upon init, but they would get overwritten with zeroes from one S_FMT
> call and the next S_FMT call would already see zeroes in place of the
> defaults. For clients that used that sequence and did not pass a size,
> such as rpi-ffmpeg, this would then result in REQBUFS failing due to
> the zero size remembered in the state.
> 
> Fix by explicitly falling back to the defaults in S_FMT.
> 
> Fixes: b530b95de22c ("media: iris: implement s_fmt, g_fmt and try_fmt ioctls")
> Link: https://github.com/jc-kynesim/rpi-ffmpeg/issues/103
> Signed-off-by: Val Packett <val@packett.cool>
> ---
> Somehow Venus didn't have this issue and didn't explicitly handle this..

In venus driver, try_fmt is clamping the resolution between min and max
supported which might be handling this case.

> 
> I'm not familiar with this code so if there's a better way to comply
> with the UAPI requirements by just not even getting to overwrite the
> state with the provided 0 size, I could not figure it out.
> 
> Still, let's get this fixed one way or another.
> 
> Thanks,
> ~val
> ---
>  drivers/media/platform/qcom/iris/iris_vdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c
> index ae13c3e1b426..6be09d82e24d 100644
> --- a/drivers/media/platform/qcom/iris/iris_vdec.c
> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c
> @@ -196,6 +196,11 @@ int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f)
>  	if (vb2_is_busy(q))
>  		return -EBUSY;
>  

Can you pls add a small comment here.

with that,

Reviewed-by: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

> +	if (f->fmt.pix_mp.width == 0 && f->fmt.pix_mp.height == 0) {
> +		f->fmt.pix_mp.width = DEFAULT_WIDTH;
> +		f->fmt.pix_mp.height = DEFAULT_HEIGHT;
> +	}
> +
>  	iris_vdec_try_fmt(inst, f);
>  
>  	switch (f->type) {