Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
Hey Tomasz, On Thu, 2019-08-01 at 13:06 +0900, Tomasz Figa wrote: > Hi Boris, > > On Wed, Jun 19, 2019 at 9:15 PM Boris Brezillon > wrote: > [snip] > > @@ -533,10 +535,21 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned int > > *num_buffers, > > return -EINVAL; > > } > > > > + /* The H264 decoder needs extra size on the output buffer. */ > > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE_RAW) > > + extra_size0 = 128 * DIV_ROUND_UP(pixfmt->width, 16) * > > + DIV_ROUND_UP(pixfmt->height, 16); > > + > > I wonder if this shouldn't be accounted for already in the sizeimage > returned by TRY_/S_FMT, so that the application can know the required > buffer size if it uses some external allocator and DMABUF memory type. > I know we had it like this in our downstream code, but it wasn't the > problem because we use minigbm, where we explicitly add the same > padding in the rockchip backend. Any thoughts? > Nice catch. This should be fixed and accounted in TRY_FMT as you suggest. Thanks, Eze
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
On Thu, Aug 1, 2019 at 4:04 PM Paul Kocialkowski wrote: > > Hi, > > On Thu 01 Aug 19, 13:06, Tomasz Figa wrote: > > Hi Boris, > > > > On Wed, Jun 19, 2019 at 9:15 PM Boris Brezillon > > wrote: > > [snip] > > > @@ -533,10 +535,21 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned > > > int *num_buffers, > > > return -EINVAL; > > > } > > > > > > + /* The H264 decoder needs extra size on the output buffer. */ > > > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE_RAW) > > > + extra_size0 = 128 * DIV_ROUND_UP(pixfmt->width, 16) * > > > + DIV_ROUND_UP(pixfmt->height, 16); > > > + > > > > I wonder if this shouldn't be accounted for already in the sizeimage > > returned by TRY_/S_FMT, so that the application can know the required > > buffer size if it uses some external allocator and DMABUF memory type. > > I know we had it like this in our downstream code, but it wasn't the > > problem because we use minigbm, where we explicitly add the same > > padding in the rockchip backend. Any thoughts? > > Does the extra size have to be allocated along with the buffer? > > On cedrus, we have a need for a similar side-buffer but give it a dedicated > CMA > allocation, which should allow dma-buf-imported buffers. Yes, the decoder stores motion vectors (IIRC) after the image data. Best regards, Tomasz
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
Hi, On Thu 01 Aug 19, 13:06, Tomasz Figa wrote: > Hi Boris, > > On Wed, Jun 19, 2019 at 9:15 PM Boris Brezillon > wrote: > [snip] > > @@ -533,10 +535,21 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned int > > *num_buffers, > > return -EINVAL; > > } > > > > + /* The H264 decoder needs extra size on the output buffer. */ > > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE_RAW) > > + extra_size0 = 128 * DIV_ROUND_UP(pixfmt->width, 16) * > > + DIV_ROUND_UP(pixfmt->height, 16); > > + > > I wonder if this shouldn't be accounted for already in the sizeimage > returned by TRY_/S_FMT, so that the application can know the required > buffer size if it uses some external allocator and DMABUF memory type. > I know we had it like this in our downstream code, but it wasn't the > problem because we use minigbm, where we explicitly add the same > padding in the rockchip backend. Any thoughts? Does the extra size have to be allocated along with the buffer? On cedrus, we have a need for a similar side-buffer but give it a dedicated CMA allocation, which should allow dma-buf-imported buffers. Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
On Thu, 1 Aug 2019 13:06:10 +0900 Tomasz Figa wrote: > Hi Boris, > > On Wed, Jun 19, 2019 at 9:15 PM Boris Brezillon > wrote: > [snip] > > @@ -533,10 +535,21 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned int > > *num_buffers, > > return -EINVAL; > > } > > > > + /* The H264 decoder needs extra size on the output buffer. */ > > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE_RAW) > > + extra_size0 = 128 * DIV_ROUND_UP(pixfmt->width, 16) * > > + DIV_ROUND_UP(pixfmt->height, 16); > > + > > I wonder if this shouldn't be accounted for already in the sizeimage > returned by TRY_/S_FMT, so that the application can know the required > buffer size if it uses some external allocator and DMABUF memory type. > I know we had it like this in our downstream code, but it wasn't the > problem because we use minigbm, where we explicitly add the same > padding in the rockchip backend. Any thoughts? Actually, I was wondering why it was not counted in ->sizeimage. I thought you had a good reason to not expose the extra size to userspace so I kept it like that.
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
Hi Boris, On Wed, Jun 19, 2019 at 9:15 PM Boris Brezillon wrote: [snip] > @@ -533,10 +535,21 @@ hantro_queue_setup(struct vb2_queue *vq, unsigned int > *num_buffers, > return -EINVAL; > } > > + /* The H264 decoder needs extra size on the output buffer. */ > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE_RAW) > + extra_size0 = 128 * DIV_ROUND_UP(pixfmt->width, 16) * > + DIV_ROUND_UP(pixfmt->height, 16); > + I wonder if this shouldn't be accounted for already in the sizeimage returned by TRY_/S_FMT, so that the application can know the required buffer size if it uses some external allocator and DMABUF memory type. I know we had it like this in our downstream code, but it wasn't the problem because we use minigbm, where we explicitly add the same padding in the rockchip backend. Any thoughts? Best regards, Tomasz
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
On Thu, 25 Jul 2019 15:31:41 +0200
Rasmus Villemoes wrote:
> On 19/06/2019 14.15, Boris Brezillon wrote:
> > From: Hertz Wong
> >
> > Add helpers and patch hantro_{drv,v4l2}.c to prepare addition of H264
> > decoding support.
> >
> > Signed-off-by: Hertz Wong
> > Signed-off-by: Boris Brezillon
> > ---
> > +
> > + /*
> > +* Short term pics in descending pic num order, long term ones in
> > +* ascending order.
> > +*/
> > + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
> > + return b->frame_num - a->frame_num;
> > +
> > + return a->pic_num - b->pic_num;
> > +}
>
> Pet peeve: This works because ->frame_num and ->pic_num are u16, so
> their difference is guaranteed to fit in an int.
>
> > +static int b0_ref_list_cmp(const void *ptra, const void *ptrb, const void
> > *data)
> > +{
> > + const struct hantro_h264_reflist_builder *builder = data;
> > + const struct v4l2_h264_dpb_entry *a, *b;
> > + s32 poca, pocb;
> > + u8 idxa, idxb;
> > +
> > + idxa = *((u8 *)ptra);
> > + idxb = *((u8 *)ptrb);
> > + a = >dpb[idxa];
> > + b = >dpb[idxb];
> > +
> > + if ((a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) !=
> > + (b->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)) {
> > + /* Short term pics firt. */
> > + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
> > + return -1;
> > + else
> > + return 1;
> > + }
> > +
> > + /* Long term pics in ascending pic num order. */
> > + if (a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > + return a->pic_num - b->pic_num;
> > +
> > + poca = builder->pocs[idxa];
> > + pocb = builder->pocs[idxb];
> > +
> > + /*
> > +* Short term pics with POC < cur POC first in POC descending order
> > +* followed by short term pics with POC > cur POC in POC ascending
> > +* order.
> > +*/
> > + if ((poca < builder->curpoc) != (pocb < builder->curpoc))
> > + return poca - pocb;
> > + else if (poca < builder->curpoc)
> > + return pocb - poca;
> > +
> > + return poca - pocb;
> > +}
>
> Here, however, poca and pocb are ints. What guarantees that their values
> are not more than 2^31 apart?
Good question. Both should normally be >= 0, which I guess prevents the
s32 overflow. This being said, it's something passed by userspace, and
I don't think we check the value (yet).
> I know absolutely nothing about this code
> or what these numbers represent, so it may be obvious that they are
> smallish.
Well, a safe approach would be to replace those subtraction by a
ternary operator.
Re: [PATCH 7/9] media: hantro: Add core bits to support H264 decoding
On 19/06/2019 14.15, Boris Brezillon wrote:
> From: Hertz Wong
>
> Add helpers and patch hantro_{drv,v4l2}.c to prepare addition of H264
> decoding support.
>
> Signed-off-by: Hertz Wong
> Signed-off-by: Boris Brezillon
> ---
> +
> + /*
> + * Short term pics in descending pic num order, long term ones in
> + * ascending order.
> + */
> + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
> + return b->frame_num - a->frame_num;
> +
> + return a->pic_num - b->pic_num;
> +}
Pet peeve: This works because ->frame_num and ->pic_num are u16, so
their difference is guaranteed to fit in an int.
> +static int b0_ref_list_cmp(const void *ptra, const void *ptrb, const void
> *data)
> +{
> + const struct hantro_h264_reflist_builder *builder = data;
> + const struct v4l2_h264_dpb_entry *a, *b;
> + s32 poca, pocb;
> + u8 idxa, idxb;
> +
> + idxa = *((u8 *)ptra);
> + idxb = *((u8 *)ptrb);
> + a = >dpb[idxa];
> + b = >dpb[idxb];
> +
> + if ((a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) !=
> + (b->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)) {
> + /* Short term pics firt. */
> + if (!(a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
> + return -1;
> + else
> + return 1;
> + }
> +
> + /* Long term pics in ascending pic num order. */
> + if (a->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> + return a->pic_num - b->pic_num;
> +
> + poca = builder->pocs[idxa];
> + pocb = builder->pocs[idxb];
> +
> + /*
> + * Short term pics with POC < cur POC first in POC descending order
> + * followed by short term pics with POC > cur POC in POC ascending
> + * order.
> + */
> + if ((poca < builder->curpoc) != (pocb < builder->curpoc))
> + return poca - pocb;
> + else if (poca < builder->curpoc)
> + return pocb - poca;
> +
> + return poca - pocb;
> +}
Here, however, poca and pocb are ints. What guarantees that their values
are not more than 2^31 apart? I know absolutely nothing about this code
or what these numbers represent, so it may be obvious that they are
smallish.
Rasmus
