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 = &builder->dpb[idxa]; > > + b = &builder->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 = &builder->dpb[idxa]; > + b = &builder->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
[PATCH 7/9] media: hantro: Add core bits to support H264 decoding
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 --- drivers/staging/media/hantro/Makefile | 1 + drivers/staging/media/hantro/hantro.h | 9 +- drivers/staging/media/hantro/hantro_drv.c | 35 ++ drivers/staging/media/hantro/hantro_h264.c | 638 + drivers/staging/media/hantro/hantro_hw.h | 52 ++ drivers/staging/media/hantro/hantro_v4l2.c | 15 +- 6 files changed, 748 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/media/hantro/hantro_h264.c diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile index a627aee77f75..d63e25682287 100644 --- a/drivers/staging/media/hantro/Makefile +++ b/drivers/staging/media/hantro/Makefile @@ -9,6 +9,7 @@ hantro-vpu-y += \ rk3399_vpu_hw_jpeg_enc.o \ rk3399_vpu_hw_mpeg2_dec.o \ hantro_jpeg.o \ + hantro_h264.o \ hantro_mpeg2.o \ hantro_vp8.o diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index 01b5c8a9aea6..6ffbd9714d5c 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -26,6 +26,10 @@ #include "hantro_hw.h" +#define H264_MB_DIM16 +#define H264_MB_WIDTH(w) DIV_ROUND_UP(w, H264_MB_DIM) +#define H264_MB_HEIGHT(h) DIV_ROUND_UP(h, H264_MB_DIM) + #define MPEG2_MB_DIM 16 #define MPEG2_MB_WIDTH(w) DIV_ROUND_UP(w, MPEG2_MB_DIM) #define MPEG2_MB_HEIGHT(h) DIV_ROUND_UP(h, MPEG2_MB_DIM) @@ -39,9 +43,9 @@ struct hantro_codec_ops; #define HANTRO_JPEG_ENCODERBIT(0) #define HANTRO_ENCODERS0x - #define HANTRO_MPEG2_DECODER BIT(16) #define HANTRO_VP8_DECODER BIT(17) +#define HANTRO_H264_DECODERBIT(18) #define HANTRO_DECODERS0x /** @@ -98,12 +102,14 @@ struct hantro_variant { * enum hantro_codec_mode - codec operating mode. * @HANTRO_MODE_NONE: No operating mode. Used for RAW video formats. * @HANTRO_MODE_JPEG_ENC: JPEG encoder. + * @HANTRO_MODE_H264_DEC: H264 decoder. * @HANTRO_MODE_MPEG2_DEC: MPEG-2 decoder. * @HANTRO_MODE_VP8_DEC: VP8 decoder. */ enum hantro_codec_mode { HANTRO_MODE_NONE = -1, HANTRO_MODE_JPEG_ENC, + HANTRO_MODE_H264_DEC, HANTRO_MODE_MPEG2_DEC, HANTRO_MODE_VP8_DEC, }; @@ -242,6 +248,7 @@ struct hantro_ctx { /* Specific for particular codec modes. */ union { + struct hantro_h264_dec_hw_ctx h264_dec; struct hantro_jpeg_enc_hw_ctx jpeg_enc; struct hantro_mpeg2_dec_hw_ctx mpeg2_dec; struct hantro_vp8_dec_hw_ctx vp8_dec; diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 86302fb356ae..0dec1f52320f 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -312,6 +312,41 @@ static const struct hantro_ctrl controls[] = { .cfg = { .id = V4L2_CID_MPEG_VIDEO_VP8_FRAME_HDR, }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS, + }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS, + .dims[0] = V4L2_H264_MAX_SLICES_PER_FRAME, + }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_SPS, + }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_PPS, + }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX, + }, + }, { + .codec = HANTRO_H264_DECODER, + .cfg = { + .id = V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE, + .max = V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME, + .menu_skip_mask = BIT(V4L2_MPEG_VIDEO_H264_DECODING_PER_SLICE), + .def = V4L2_MPEG_VIDEO_H264_DECODING_PER_FRAME, + }, + }, { }, }; diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c new file mode 100644 index ..9fec1a0fd807 --- /dev/null +++ b/drivers/staging/media/hantro/hantro_h264.c @@ -0,0 +1,638 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Rockchip RK3288 VPU codec driver + * + * Copyright (c) 2014 Rockc