| Message ID | 20191213125414.90725-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: rockchip: Add the rkvdec driver | expand |
Hi Boris, A few small comments: On 12/13/19 1:54 PM, Boris Brezillon wrote: > The rockchip vdec block is a stateless decoder that's able to decode > H264, HEVC and VP9 content. This commit adds the core infrastructure > and the H264 backend. Support for VP9 and HEVS will be added later on. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Changes in v3: > * Move the driver to drivers/staging/media/rkvdec/ > * Fix copy&paste error in the Kconfig desc > * Use REC709 color space when resetting the capture format > * Prepare things to support VP9 > * Move H264 priv-table field definition out of rkvdec-regs.h > * Clarify the provenance of the CABAC table > * Ease RPS,PPS_FIELD() definition/manipulation > * Take DPB field flags into account > * Use the generic H264 helpers to build the reflists > --- > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile | 1 + > drivers/staging/media/rkvdec/Kconfig | 15 + > drivers/staging/media/rkvdec/Makefile | 3 + > drivers/staging/media/rkvdec/rkvdec-h264.c | 1154 ++++++++++++++++++++ > drivers/staging/media/rkvdec/rkvdec-regs.h | 239 ++++ > drivers/staging/media/rkvdec/rkvdec.c | 1130 +++++++++++++++++++ > drivers/staging/media/rkvdec/rkvdec.h | 124 +++ > 8 files changed, 2668 insertions(+) > create mode 100644 drivers/staging/media/rkvdec/Kconfig > create mode 100644 drivers/staging/media/rkvdec/Makefile > create mode 100644 drivers/staging/media/rkvdec/rkvdec-h264.c > create mode 100644 drivers/staging/media/rkvdec/rkvdec-regs.h > create mode 100644 drivers/staging/media/rkvdec/rkvdec.c > create mode 100644 drivers/staging/media/rkvdec/rkvdec.h > <snip> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > new file mode 100644 > index 000000000000..6de4bd39f286 > --- /dev/null > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c <snip> > +/* > + * dpb poc related registers table > + */ Shouldn't the next two arrays be const? > +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) > +}; <snip> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c > new file mode 100644 > index 000000000000..97698be9072e > --- /dev/null > +++ b/drivers/staging/media/rkvdec/rkvdec.c > @@ -0,0 +1,1130 @@ <snip> > +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. > + } 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); > + return 0; > +} <snip> Regards, Hans
Hi Boris, On Fri, Dec 13, 2019 at 01:54:08PM +0100, Boris Brezillon wrote: > vb2_request_get_buf() returns the N-th buffer attached to a media > request. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Changes in v3: > * None > > Changes in v2: > * Adjust the kernel doc as suggested by Hans > --- > .../media/common/videobuf2/videobuf2-core.c | 23 +++++++++++++++++++ > include/media/videobuf2-core.h | 11 +++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 4489744fbbd9..c4c7980dcb0d 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -1416,6 +1416,29 @@ unsigned int vb2_request_buffer_cnt(struct media_request *req) > } > EXPORT_SYMBOL_GPL(vb2_request_buffer_cnt); > > +struct vb2_buffer *vb2_request_get_buf(struct media_request *req, > + unsigned int n) > +{ > + struct media_request_object *obj; > + struct vb2_buffer *buf = NULL; > + unsigned int nbufs = 0; > + unsigned long flags; > + > + spin_lock_irqsave(&req->lock, flags); > + list_for_each_entry(obj, &req->objects, list) { > + if (!vb2_request_object_is_buffer(obj) || > + nbufs++ < n) > + continue; > + > + buf = container_of(obj, struct vb2_buffer, req_obj); > + break; > + } > + spin_unlock_irqrestore(&req->lock, flags); > + > + return buf; > +} > +EXPORT_SYMBOL_GPL(vb2_request_get_buf); > + > int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) > { > struct vb2_buffer *vb; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index a2b2208b02da..6206e25df764 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -1225,4 +1225,15 @@ bool vb2_request_object_is_buffer(struct media_request_object *obj); > */ > unsigned int vb2_request_buffer_cnt(struct media_request *req); > > +/** > + * vb2_request_get_buf() - return the buffer at index @idx > + * > + * @req: the request. > + * @n: search for the Nth buffer in the req object list It's not very clear to me what "n" is here. Wouldn't it be better to pass the queue pointer instead, to get a buffer for a given queue ? > + * > + * Return a vb2 buffer or NULL if there's no buffer at the specified position > + */ > +struct vb2_buffer *vb2_request_get_buf(struct media_request *req, > + unsigned int n); > + > #endif /* _MEDIA_VIDEOBUF2_CORE_H */
On Fri, 13 Dec 2019 17:09:35 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Fri, Dec 13, 2019 at 01:54:08PM +0100, Boris Brezillon wrote: > > vb2_request_get_buf() returns the N-th buffer attached to a media > > request. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > --- > > Changes in v3: > > * None > > > > Changes in v2: > > * Adjust the kernel doc as suggested by Hans > > --- > > .../media/common/videobuf2/videobuf2-core.c | 23 +++++++++++++++++++ > > include/media/videobuf2-core.h | 11 +++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index 4489744fbbd9..c4c7980dcb0d 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -1416,6 +1416,29 @@ unsigned int vb2_request_buffer_cnt(struct media_request *req) > > } > > EXPORT_SYMBOL_GPL(vb2_request_buffer_cnt); > > > > +struct vb2_buffer *vb2_request_get_buf(struct media_request *req, > > + unsigned int n) > > +{ > > + struct media_request_object *obj; > > + struct vb2_buffer *buf = NULL; > > + unsigned int nbufs = 0; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&req->lock, flags); > > + list_for_each_entry(obj, &req->objects, list) { > > + if (!vb2_request_object_is_buffer(obj) || > > + nbufs++ < n) > > + continue; > > + > > + buf = container_of(obj, struct vb2_buffer, req_obj); > > + break; > > + } > > + spin_unlock_irqrestore(&req->lock, flags); > > + > > + return buf; > > +} > > +EXPORT_SYMBOL_GPL(vb2_request_get_buf); > > + > > int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) > > { > > struct vb2_buffer *vb; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index a2b2208b02da..6206e25df764 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -1225,4 +1225,15 @@ bool vb2_request_object_is_buffer(struct media_request_object *obj); > > */ > > unsigned int vb2_request_buffer_cnt(struct media_request *req); > > > > +/** > > + * vb2_request_get_buf() - return the buffer at index @idx > > + * > > + * @req: the request. > > + * @n: search for the Nth buffer in the req object list > > It's not very clear to me what "n" is here. Wouldn't it be better to > pass the queue pointer instead, to get a buffer for a given queue ? Yep, that would work too and would be much clearer. I'll do that, thanks for the suggestion.