Re: [PATCH 2/3] media: venus: hfi_parser: size a raw format property by its own entries

[Konrad Dybcio]

On 8/12/26 10:01 PM, Dmitry Baryshkov wrote:
> parse_raw_formats() walks the format entries one at a time, each of
> which is as long as its own plane count makes it, but then computes the
> length of the whole property as if every entry had as many planes as the
> last one it looked at.  The result is only correct when all the entries
> agree, and there is nothing in the interface that says they must.
> 
> On MSM8996 they do not, and the parser walks off into the middle of the
> message as a result.  The raw format property of the HEVC decoder is 120
> bytes long and the walk claims 152, so hfi_parser() resumes 28 bytes
> past the end of it, skipping the codec mask that follows and landing
> inside the property after that.  It resynchronises eventually, because
> an unrecognized word is skipped one at a time, but everything it jumped
> over is lost.
> 
> What is lost matters: the property skipped that way is the capability
> set of the codec mask 0x7002, which is to say the frame size, macroblock
> and frame rate limits of H.264, VP8, VP9 and HEVC decoding, all four of
> them described in one block.  Those decoders end up holding a bitrate
> and nothing else, so frame_width_min() and friends return zero and
> VIDIOC_ENUM_FRAMESIZES advertises a stepwise range of 0x0 to 0x0 with a
> step of 0.
> 
> Userspace cannot negotiate against that.  GStreamer builds the sink caps
> of its V4L2 decoders from the enumerated frame sizes, an empty integer
> range collapses them to EMPTY, and no pad is found to be compatible with
> the parser feeding the decoder, so a pipeline as simple as
> 
>   filesrc ! parsebin ! v4l2vp9dec ! videoconvert ! fakesink
> 
> fails to link and the stream stops with "not-linked" before a single
> buffer is queued.  Hardware decoding is unavailable on the board for
> every codec in that block.
> 
> An earlier overshoot loses a raw format property too, leaving the
> decoders with two of the five formats the firmware describes and HEVC
> with four of seven.
> 
> Accumulate the length of the entries as they are walked, the way the
> downstream driver does, rather than extrapolating from the last one.
> 
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/venus/hfi_parser.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 3413b91b0b7e..f79c368647d7 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -171,7 +171,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	u32 entries = fmt->format_entries;
>  	unsigned int i = 0;
>  	u32 num_planes = 0;
> -	u32 size;
> +	u32 size = 2 * sizeof(u32);
>  
>  	while (entries) {
>  		num_planes = pinfo->num_planes;
> @@ -183,9 +183,10 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  		if (i >= MAX_FMT_ENTRIES)
>  			return -EINVAL;
>  
> -		if (pinfo->num_planes > MAX_PLANES)
> +		if (num_planes > MAX_PLANES)

Most of this patch (minus the line above) is in next as:

bd595b745eb7 ("media: venus: fix payload size calculation in parse_raw_formats()")

Konrad