Re: [PATCH v9 4/4] media: cedrus: Add H264 decoding support

[Hans Verkuil]

Hi Maxime,

While rebasing my cedrus-h264 pull request and running sparse over the
newly rebased code I found these sparse warnings:

SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:80:34:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:81:37:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:82:25:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:84:23:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:85:25:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:86:29:  warning: incorrect type in assignment (different base types)
SPARSE:drivers/staging/media/sunxi/cedrus/cedrus_h264.c drivers/staging/media/sunxi/cedrus/cedrus_h264.c:87:29:  warning: incorrect type in assignment (different base types)

These warnings seem valid:

On 4/11/19 11:37 AM, Maxime Ripard wrote:
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> new file mode 100644
> index 000000000000..2c98a3e46d2b
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -0,0 +1,574 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Cedrus VPU driver
> + *
> + * Copyright (c) 2013 Jens Kuske <jenskuske@xxxxxxxxx>
> + * Copyright (c) 2018 Bootlin
> + */
> +
> +#include <linux/types.h>
> +
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "cedrus.h"
> +#include "cedrus_hw.h"
> +#include "cedrus_regs.h"
> +
> +enum cedrus_h264_sram_off {
> +	CEDRUS_SRAM_H264_PRED_WEIGHT_TABLE	= 0x000,
> +	CEDRUS_SRAM_H264_FRAMEBUFFER_LIST	= 0x100,
> +	CEDRUS_SRAM_H264_REF_LIST_0		= 0x190,
> +	CEDRUS_SRAM_H264_REF_LIST_1		= 0x199,
> +	CEDRUS_SRAM_H264_SCALING_LIST_8x8_0	= 0x200,
> +	CEDRUS_SRAM_H264_SCALING_LIST_8x8_1	= 0x210,
> +	CEDRUS_SRAM_H264_SCALING_LIST_4x4	= 0x220,
> +};
> +
> +struct cedrus_h264_sram_ref_pic {
> +	__le32	top_field_order_cnt;
> +	__le32	bottom_field_order_cnt;
> +	__le32	frame_info;
> +	__le32	luma_ptr;
> +	__le32	chroma_ptr;
> +	__le32	mv_col_top_ptr;
> +	__le32	mv_col_bot_ptr;
> +	__le32	reserved;

These types are __le32, but...

> +} __packed;
> +
> +#define CEDRUS_H264_FRAME_NUM		18
> +
> +#define CEDRUS_NEIGHBOR_INFO_BUF_SIZE	(16 * SZ_1K)
> +#define CEDRUS_PIC_INFO_BUF_SIZE	(128 * SZ_1K)
> +
> +static void cedrus_h264_write_sram(struct cedrus_dev *dev,
> +				   enum cedrus_h264_sram_off off,
> +				   const void *data, size_t len)
> +{
> +	const u32 *buffer = data;
> +	size_t count = DIV_ROUND_UP(len, 4);
> +
> +	cedrus_write(dev, VE_AVC_SRAM_PORT_OFFSET, off << 2);
> +
> +	while (count--)
> +		cedrus_write(dev, VE_AVC_SRAM_PORT_DATA, *buffer++);
> +}
> +
> +static dma_addr_t cedrus_h264_mv_col_buf_addr(struct cedrus_ctx *ctx,
> +					      unsigned int position,
> +					      unsigned int field)
> +{
> +	dma_addr_t addr = ctx->codec.h264.mv_col_buf_dma;
> +
> +	/* Adjust for the position */
> +	addr += position * ctx->codec.h264.mv_col_buf_field_size * 2;
> +
> +	/* Adjust for the field */
> +	addr += field * ctx->codec.h264.mv_col_buf_field_size;
> +
> +	return addr;
> +}
> +
> +static void cedrus_fill_ref_pic(struct cedrus_ctx *ctx,
> +				struct cedrus_buffer *buf,
> +				unsigned int top_field_order_cnt,
> +				unsigned int bottom_field_order_cnt,
> +				struct cedrus_h264_sram_ref_pic *pic)
> +{
> +	struct vb2_buffer *vbuf = &buf->m2m_buf.vb.vb2_buf;
> +	unsigned int position = buf->codec.h264.position;
> +
> +	pic->top_field_order_cnt = top_field_order_cnt;
> +	pic->bottom_field_order_cnt = bottom_field_order_cnt;
> +	pic->frame_info = buf->codec.h264.pic_type << 8;
> +
> +	pic->luma_ptr = cedrus_buf_addr(vbuf, &ctx->dst_fmt, 0);
> +	pic->chroma_ptr = cedrus_buf_addr(vbuf, &ctx->dst_fmt, 1);
> +	pic->mv_col_top_ptr = cedrus_h264_mv_col_buf_addr(ctx, position, 0);
> +	pic->mv_col_bot_ptr = cedrus_h264_mv_col_buf_addr(ctx, position, 1);

... here regular types are assigned to the pic fields.

I assume that cpu_to_le32() is needed here.

I'm not sure how I missed this when I made the initial pull request,
but I think it would make sense if you post a rebased and fixed v10.

Regards,

	Hans