On Fri, 13 Dec 2019 15:21:05 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> > +/*
> > + * dpb poc related registers table
> > + */
>
> Shouldn't the next two arrays be const?
Absolutely. I'll fix that in v4.
>
> > +static u32 poc_reg_tbl_top_field[16] = {
> > + RKVDEC_REG_H264_POC_REFER0(0),
> > + RKVDEC_REG_H264_POC_REFER0(2),
> > + RKVDEC_REG_H264_POC_REFER0(4),
> > + RKVDEC_REG_H264_POC_REFER0(6),
> > + RKVDEC_REG_H264_POC_REFER0(8),
> > + RKVDEC_REG_H264_POC_REFER0(10),
> > + RKVDEC_REG_H264_POC_REFER0(12),
> > + RKVDEC_REG_H264_POC_REFER0(14),
> > + RKVDEC_REG_H264_POC_REFER1(1),
> > + RKVDEC_REG_H264_POC_REFER1(3),
> > + RKVDEC_REG_H264_POC_REFER1(5),
> > + RKVDEC_REG_H264_POC_REFER1(7),
> > + RKVDEC_REG_H264_POC_REFER1(9),
> > + RKVDEC_REG_H264_POC_REFER1(11),
> > + RKVDEC_REG_H264_POC_REFER1(13),
> > + RKVDEC_REG_H264_POC_REFER2(0)
> > +};
> > +
> > +static u32 poc_reg_tbl_bottom_field[16] = {
> > + RKVDEC_REG_H264_POC_REFER0(1),
> > + RKVDEC_REG_H264_POC_REFER0(3),
> > + RKVDEC_REG_H264_POC_REFER0(5),
> > + RKVDEC_REG_H264_POC_REFER0(7),
> > + RKVDEC_REG_H264_POC_REFER0(9),
> > + RKVDEC_REG_H264_POC_REFER0(11),
> > + RKVDEC_REG_H264_POC_REFER0(13),
> > + RKVDEC_REG_H264_POC_REFER1(0),
> > + RKVDEC_REG_H264_POC_REFER1(2),
> > + RKVDEC_REG_H264_POC_REFER1(4),
> > + RKVDEC_REG_H264_POC_REFER1(6),
> > + RKVDEC_REG_H264_POC_REFER1(8),
> > + RKVDEC_REG_H264_POC_REFER1(10),
> > + RKVDEC_REG_H264_POC_REFER1(12),
> > + RKVDEC_REG_H264_POC_REFER1(14),
> > + RKVDEC_REG_H264_POC_REFER2(1)
> > +};
[...]
> > +static int rkvdec_queue_setup(struct vb2_queue *vq, unsigned int *num_buffers,
> > + unsigned int *num_planes, unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct rkvdec_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct v4l2_pix_format_mplane *pixfmt;
> > + struct v4l2_format *f;
> > + unsigned int i;
> > +
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + f = &ctx->coded_fmt;
> > + else
> > + f = &ctx->decoded_fmt;
> > +
> > + if (*num_planes) {
> > + if (*num_planes != f->fmt.pix_mp.num_planes)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < f->fmt.pix_mp.num_planes; i++) {
> > + if (sizes[i] < f->fmt.pix_mp.plane_fmt[i].sizeimage)
> > + return -EINVAL;
> > + }
>
> Shouldn't there be a 'return 0' here? In the CREATE_BUFS case all you do is check
> if the given size is large enough, and if so then you are done.
I end up returning 0 anyway, it's just that size[0] is updated to
account for the extra MV size, is that a problem?
>
> > + } else {
> > + *num_planes = f->fmt.pix_mp.num_planes;
> > + for (i = 0; i < f->fmt.pix_mp.num_planes; i++)
> > + sizes[i] = f->fmt.pix_mp.plane_fmt[i].sizeimage;
> > + }
> > +
> > + if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > + return 0;
> > +
> > + pixfmt = &ctx->decoded_fmt.fmt.pix_mp;
> > + sizes[0] += 128 * DIV_ROUND_UP(pixfmt->width, 16) *
> > + DIV_ROUND_UP(pixfmt->height, 16);
This makes me realize we decided to take the MV extra size into account
in our ->sizeimage calculation in the hantro driver, so I should
probably move this code to try_s_capture_fmt().
