Re: [PATCH v2 4/6] media: verisilicon: rockchip: bound VPU981 AV1 tile loop and guard divisor

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 14/06/2026 à 17:56, Michael Bommarito a écrit :
> rockchip_vpu981_av1_dec_set_tile_info() divides context_update_tile_id by
> tile_info->tile_cols and writes one descriptor per tile into the tile_info
> DMA buffer, sized for AV1_MAX_TILES. tile_cols / tile_rows come straight
> from the bitstream; reject a zero column or row count and bound the grid to
> AV1_MAX_TILES so the division is safe and the writes stay in the buffer.
>
> Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> ---
>   .../verisilicon/rockchip_vpu981_hw_av1_dec.c  | 29 +++++++++++++------
>   1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> index e4e21ad373233..71d2ef72c4402 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> @@ -578,21 +578,32 @@ static void rockchip_vpu981_av1_dec_set_tile_info(struct hantro_ctx *ctx)
>   	const struct v4l2_av1_tile_info *tile_info = &ctrls->frame->tile_info;
>   	const struct v4l2_ctrl_av1_tile_group_entry *group_entry =
>   	    ctrls->tile_group_entry;
> -	int context_update_y =
> -	    tile_info->context_update_tile_id / tile_info->tile_cols;
> -	int context_update_x =
> -	    tile_info->context_update_tile_id % tile_info->tile_cols;
> -	int context_update_tile_id =
> -	    context_update_x * tile_info->tile_rows + context_update_y;
> +	unsigned int tile_cols, tile_rows;
> +	int context_update_y, context_update_x, context_update_tile_id;
>   	u8 *dst = av1_dec->tile_info.cpu;
>   	struct hantro_dev *vpu = ctx->dev;
>   	int tile0, tile1;
>   
> +	/* Guard the divisor and bound the grid to the tile_info buffer. */
> +	tile_cols = tile_info->tile_cols;
> +	tile_rows = tile_info->tile_rows;
> +	if (!tile_cols || !tile_rows)
> +		return;

NACK
because you completely ignore how these values are used later in this function
to set registers.

> +	if (tile_cols * tile_rows > AV1_MAX_TILES) {
> +		tile_cols = min_t(unsigned int, tile_cols, AV1_MAX_TILES);
> +		tile_rows = min_t(unsigned int, tile_rows,
> +				  AV1_MAX_TILES / tile_cols);
> +	}
> +

It isn't possible to recompute tile_cols and tile_rows like that.
Please add this check in validate_av1_tile_info().

> +	context_update_y = tile_info->context_update_tile_id / tile_cols;
> +	context_update_x = tile_info->context_update_tile_id % tile_cols;
> +	context_update_tile_id = context_update_x * tile_rows + context_update_y;

To fix the possible division by zero: initialize the variable to zero when declare them
and only do the division if tile_cols isn't zero.

Thanks,
Benjamin

> +
>   	memset(dst, 0, av1_dec->tile_info.size);
>   
> -	for (tile0 = 0; tile0 < tile_info->tile_cols; tile0++) {
> -		for (tile1 = 0; tile1 < tile_info->tile_rows; tile1++) {
> -			int tile_id = tile1 * tile_info->tile_cols + tile0;
> +	for (tile0 = 0; tile0 < tile_cols; tile0++) {
> +		for (tile1 = 0; tile1 < tile_rows; tile1++) {
> +			int tile_id = tile1 * tile_cols + tile0;
>   			u32 start, end;
>   			u32 y0 =
>   			    tile_info->height_in_sbs_minus_1[tile1] + 1;

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip