* [PATCH 0/4] Enable Hantro G1 post-processor @ 2019-09-03 18:17 Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-03 18:17 UTC (permalink / raw) To: linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy, Ezequiel Garcia Hi all, This series enables the post-processor support available on the Hantro G1 VPU. The post-processor block can be pipelined with the decoder hardware, allowing to perform operations such as color conversion, scaling, rotation, cropping, among others. The decoder hardware needs its own set of NV12 buffers (the native decoder format), and the post-processor is the owner of the CAPTURE buffers. This allows the application get processed (scaled, converted, etc) buffers, completely transparently. This feature is implemented by exposing other CAPTURE pixel formats to the application (ENUM_FMT). When the application sets a pixel format other than NV12, the driver will enable and use the post-processor transparently. Patches 1 to 3 cleanup and organize the code to allow easier integration of the post-processor. Then patch 4 introduces the new pixel formats and enables the post-processor itself. I am aware it's still early for v5.5, yet I'm posting this series to get feedback and allow others to tests. Also, keep in mind these patches are conflict with Jonas' recent series. This is tested on RK3288 platforms with MPEG-2, VP8 and H264 streams, decoding to RGB and YUV-packed surfaces. Thanks, Ezequiel Ezequiel Garcia (4): media: hantro: Simplify macroblock macros media: hantro: Simplify buffer helpers media: hantro: Add helper for the H264 motion vectors allocation media: hantro: Support color conversion via post-processing drivers/staging/media/hantro/Makefile | 1 + drivers/staging/media/hantro/hantro.h | 49 ++- drivers/staging/media/hantro/hantro_drv.c | 27 +- .../staging/media/hantro/hantro_g1_h264_dec.c | 9 +- .../media/hantro/hantro_g1_mpeg2_dec.c | 14 +- .../staging/media/hantro/hantro_g1_vp8_dec.c | 13 +- .../staging/media/hantro/hantro_h1_jpeg_enc.c | 4 +- drivers/staging/media/hantro/hantro_h264.c | 26 +- drivers/staging/media/hantro/hantro_hw.h | 15 +- .../staging/media/hantro/hantro_postproc.c | 316 ++++++++++++++++++ drivers/staging/media/hantro/hantro_v4l2.c | 25 +- drivers/staging/media/hantro/rk3288_vpu_hw.c | 37 +- drivers/staging/media/hantro/rk3399_vpu_hw.c | 12 +- .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 4 +- .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 11 +- .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 12 +- 16 files changed, 483 insertions(+), 92 deletions(-) create mode 100644 drivers/staging/media/hantro/hantro_postproc.c -- 2.22.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] media: hantro: Simplify macroblock macros 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia @ 2019-09-03 18:17 ` Ezequiel Garcia 2019-09-04 10:50 ` Philipp Zabel 2019-09-03 18:17 ` [PATCH 2/4] media: hantro: Simplify buffer helpers Ezequiel Garcia ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-03 18:17 UTC (permalink / raw) To: linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy, Ezequiel Garcia It seems all codecs are using a 16x16 size macroblock, and so it's possible to have just one set of macroblock macros. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/hantro.h | 18 +++--------------- .../staging/media/hantro/hantro_g1_h264_dec.c | 2 +- .../staging/media/hantro/hantro_g1_mpeg2_dec.c | 4 ++-- .../staging/media/hantro/hantro_g1_vp8_dec.c | 4 ++-- .../staging/media/hantro/hantro_h1_jpeg_enc.c | 4 ++-- drivers/staging/media/hantro/rk3288_vpu_hw.c | 16 ++++++++-------- drivers/staging/media/hantro/rk3399_vpu_hw.c | 12 ++++++------ .../media/hantro/rk3399_vpu_hw_jpeg_enc.c | 4 ++-- .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 4 ++-- .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 4 ++-- 10 files changed, 30 insertions(+), 42 deletions(-) diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index f670bbde4159..c151133b8c86 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -26,21 +26,9 @@ #include "hantro_hw.h" -#define VP8_MB_DIM 16 -#define VP8_MB_WIDTH(w) DIV_ROUND_UP(w, VP8_MB_DIM) -#define VP8_MB_HEIGHT(h) DIV_ROUND_UP(h, VP8_MB_DIM) - -#define H264_MB_DIM 16 -#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) - -#define JPEG_MB_DIM 16 -#define JPEG_MB_WIDTH(w) DIV_ROUND_UP(w, JPEG_MB_DIM) -#define JPEG_MB_HEIGHT(h) DIV_ROUND_UP(h, JPEG_MB_DIM) +#define MB_DIM 16 +#define MB_WIDTH(w) DIV_ROUND_UP(w, MB_DIM) +#define MB_HEIGHT(h) DIV_ROUND_UP(h, MB_DIM) struct hantro_ctx; struct hantro_codec_ops; diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c index 7ab534936843..d42c4004fe35 100644 --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c @@ -251,7 +251,7 @@ static void set_buffers(struct hantro_ctx *ctx) size_t mv_offset = round_up(pic_size, 8); if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD) - mv_offset += 32 * H264_MB_WIDTH(ctx->dst_fmt.width); + mv_offset += 32 * MB_WIDTH(ctx->dst_fmt.width); vdpu_write_relaxed(vpu, dst_dma + mv_offset, G1_REG_ADDR_DIR_MV); diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c index 80f0e94f8afa..314a72208812 100644 --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c @@ -207,8 +207,8 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx) G1_REG_DEC_AXI_WR_ID(0); vdpu_write_relaxed(vpu, reg, G1_SWREG(3)); - reg = G1_REG_PIC_MB_WIDTH(MPEG2_MB_WIDTH(ctx->dst_fmt.width)) | - G1_REG_PIC_MB_HEIGHT_P(MPEG2_MB_HEIGHT(ctx->dst_fmt.height)) | + reg = G1_REG_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) | + G1_REG_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) | G1_REG_ALT_SCAN_E(picture->alternate_scan) | G1_REG_TOPFIELDFIRST_E(picture->top_field_first); vdpu_write_relaxed(vpu, reg, G1_SWREG(4)); diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c index 6d99c2be01cf..e9d3361ed385 100644 --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c @@ -470,8 +470,8 @@ void hantro_g1_vp8_dec_run(struct hantro_ctx *ctx) vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0); /* Frame dimensions */ - mb_width = VP8_MB_WIDTH(width); - mb_height = VP8_MB_HEIGHT(height); + mb_width = MB_WIDTH(width); + mb_height = MB_HEIGHT(height); reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(mb_width) | G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(mb_height) | G1_REG_DEC_CTRL1_PIC_MB_W_EXT(mb_width >> 9) | diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c index ecd34a7db190..938b48d4d3d9 100644 --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c @@ -116,8 +116,8 @@ void hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) /* Make sure that all registers are written at this point. */ vepu_write(vpu, reg, H1_REG_AXI_CTRL); - reg = H1_REG_ENC_CTRL_WIDTH(JPEG_MB_WIDTH(ctx->src_fmt.width)) - | H1_REG_ENC_CTRL_HEIGHT(JPEG_MB_HEIGHT(ctx->src_fmt.height)) + reg = H1_REG_ENC_CTRL_WIDTH(MB_WIDTH(ctx->src_fmt.width)) + | H1_REG_ENC_CTRL_HEIGHT(MB_HEIGHT(ctx->src_fmt.height)) | H1_REG_ENC_CTRL_ENC_MODE_JPEG | H1_REG_ENC_PIC_INTRA | H1_REG_ENC_CTRL_EN_BIT; diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c index 6bfcc47d1e58..c0bdd6c02520 100644 --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c @@ -48,10 +48,10 @@ static const struct hantro_fmt rk3288_vpu_enc_fmts[] = { .frmsize = { .min_width = 96, .max_width = 8192, - .step_width = JPEG_MB_DIM, + .step_width = MB_DIM, .min_height = 32, .max_height = 8192, - .step_height = JPEG_MB_DIM, + .step_height = MB_DIM, }, }, }; @@ -68,10 +68,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = { .frmsize = { .min_width = 48, .max_width = 3840, - .step_width = H264_MB_DIM, + .step_width = MB_DIM, .min_height = 48, .max_height = 2160, - .step_height = H264_MB_DIM, + .step_height = MB_DIM, }, }, { @@ -81,10 +81,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = { .frmsize = { .min_width = 48, .max_width = 1920, - .step_width = MPEG2_MB_DIM, + .step_width = MB_DIM, .min_height = 48, .max_height = 1088, - .step_height = MPEG2_MB_DIM, + .step_height = MB_DIM, }, }, { @@ -94,10 +94,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = { .frmsize = { .min_width = 48, .max_width = 3840, - .step_width = VP8_MB_DIM, + .step_width = MB_DIM, .min_height = 48, .max_height = 2160, - .step_height = VP8_MB_DIM, + .step_height = MB_DIM, }, }, }; diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw.c b/drivers/staging/media/hantro/rk3399_vpu_hw.c index 14d14bc6b12b..9ac1f2cb6a16 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw.c @@ -47,10 +47,10 @@ static const struct hantro_fmt rk3399_vpu_enc_fmts[] = { .frmsize = { .min_width = 96, .max_width = 8192, - .step_width = JPEG_MB_DIM, + .step_width = MB_DIM, .min_height = 32, .max_height = 8192, - .step_height = JPEG_MB_DIM, + .step_height = MB_DIM, }, }, }; @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = { .frmsize = { .min_width = 48, .max_width = 1920, - .step_width = MPEG2_MB_DIM, + .step_width = MB_DIM, .min_height = 48, .max_height = 1088, - .step_height = MPEG2_MB_DIM, + .step_height = MB_DIM, }, }, { @@ -80,10 +80,10 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = { .frmsize = { .min_width = 48, .max_width = 3840, - .step_width = VP8_MB_DIM, + .step_width = MB_DIM, .min_height = 48, .max_height = 2160, - .step_height = VP8_MB_DIM, + .step_height = MB_DIM, }, }, }; diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c index 06162f569b5e..067892345b5d 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_jpeg_enc.c @@ -149,8 +149,8 @@ void rk3399_vpu_jpeg_enc_run(struct hantro_ctx *ctx) reg = VEPU_REG_AXI_CTRL_BURST_LEN(16); vepu_write_relaxed(vpu, reg, VEPU_REG_AXI_CTRL); - reg = VEPU_REG_MB_WIDTH(JPEG_MB_WIDTH(ctx->src_fmt.width)) - | VEPU_REG_MB_HEIGHT(JPEG_MB_HEIGHT(ctx->src_fmt.height)) + reg = VEPU_REG_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) + | VEPU_REG_MB_HEIGHT(MB_HEIGHT(ctx->src_fmt.height)) | VEPU_REG_FRAME_TYPE_INTRA | VEPU_REG_ENCODE_FORMAT_JPEG | VEPU_REG_ENCODE_ENABLE; diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c index e7ba5c0441cc..263ec81f209b 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c @@ -223,8 +223,8 @@ void rk3399_vpu_mpeg2_dec_run(struct hantro_ctx *ctx) VDPU_REG_DEC_CLK_GATE_E(1); vdpu_write_relaxed(vpu, reg, VDPU_SWREG(57)); - reg = VDPU_REG_PIC_MB_WIDTH(MPEG2_MB_WIDTH(ctx->dst_fmt.width)) | - VDPU_REG_PIC_MB_HEIGHT_P(MPEG2_MB_HEIGHT(ctx->dst_fmt.height)) | + reg = VDPU_REG_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) | + VDPU_REG_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) | VDPU_REG_ALT_SCAN_E(picture->alternate_scan) | VDPU_REG_TOPFIELDFIRST_E(picture->top_field_first); vdpu_write_relaxed(vpu, reg, VDPU_SWREG(120)); diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c index f17e32620b08..7d32a0283d93 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c @@ -563,8 +563,8 @@ void rk3399_vpu_vp8_dec_run(struct hantro_ctx *ctx) hantro_reg_write(vpu, &vp8_dec_filter_disable, 1); /* Frame dimensions */ - mb_width = VP8_MB_WIDTH(width); - mb_height = VP8_MB_HEIGHT(height); + mb_width = MB_WIDTH(width); + mb_height = MB_HEIGHT(height); hantro_reg_write(vpu, &vp8_dec_mb_width, mb_width); hantro_reg_write(vpu, &vp8_dec_mb_height, mb_height); -- 2.22.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] media: hantro: Simplify macroblock macros 2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia @ 2019-09-04 10:50 ` Philipp Zabel 0 siblings, 0 replies; 18+ messages in thread From: Philipp Zabel @ 2019-09-04 10:50 UTC (permalink / raw) To: Ezequiel Garcia, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Boris Brezillon, Chris Healy On Tue, 2019-09-03 at 15:17 -0300, Ezequiel Garcia wrote: > It seems all codecs are using a 16x16 size macroblock, > and so it's possible to have just one set of macroblock macros. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] media: hantro: Simplify buffer helpers 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia @ 2019-09-03 18:17 ` Ezequiel Garcia 2019-09-04 10:50 ` Philipp Zabel 2019-09-03 18:17 ` [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation Ezequiel Garcia ` (2 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-03 18:17 UTC (permalink / raw) To: linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy, Ezequiel Garcia Modify hantro_get_ref() and hantro_h264_get_ref_buf() helpers to return the buffer DMA address, this makes the code simpler and at the same time will allow easier enablement of the post-processing feature. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/hantro.h | 2 +- drivers/staging/media/hantro/hantro_drv.c | 3 ++- .../staging/media/hantro/hantro_g1_h264_dec.c | 5 ++--- .../media/hantro/hantro_g1_mpeg2_dec.c | 7 ++----- .../staging/media/hantro/hantro_g1_vp8_dec.c | 7 +++---- drivers/staging/media/hantro/hantro_h264.c | 19 ++++++++----------- drivers/staging/media/hantro/hantro_hw.h | 4 ++-- .../media/hantro/rk3399_vpu_hw_mpeg2_dec.c | 7 ++----- .../media/hantro/rk3399_vpu_hw_vp8_dec.c | 8 +++----- 9 files changed, 25 insertions(+), 37 deletions(-) diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index c151133b8c86..deb90ae37859 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -367,7 +367,7 @@ static inline void hantro_reg_write(struct hantro_dev *vpu, bool hantro_is_encoder_ctx(const struct hantro_ctx *ctx); void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id); -dma_addr_t hantro_get_ref(struct vb2_queue *q, u64 ts); +dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts); static inline struct vb2_v4l2_buffer * hantro_get_src_buf(struct hantro_ctx *ctx) diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index d8b6816b643b..f550b68d46ca 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -43,8 +43,9 @@ void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id) return ctrl ? ctrl->p_cur.p : NULL; } -dma_addr_t hantro_get_ref(struct vb2_queue *q, u64 ts) +dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts) { + struct vb2_queue *q = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx); struct vb2_buffer *buf; int index; diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c index d42c4004fe35..29130946dea4 100644 --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c @@ -220,10 +220,9 @@ static void set_ref(struct hantro_ctx *ctx) /* Set up addresses of DPB buffers. */ for (i = 0; i < HANTRO_H264_DPB_SIZE; i++) { - struct vb2_buffer *buf = hantro_h264_get_ref_buf(ctx, i); + dma_addr_t dma_addr = hantro_h264_get_ref_buf(ctx, i); - vdpu_write_relaxed(vpu, vb2_dma_contig_plane_dma_addr(buf, 0), - G1_REG_ADDR_REF(i)); + vdpu_write_relaxed(vpu, dma_addr, G1_REG_ADDR_REF(i)); } } diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c index 314a72208812..f3bf67d8a289 100644 --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c @@ -105,17 +105,14 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx, { dma_addr_t forward_addr = 0, backward_addr = 0; dma_addr_t current_addr, addr; - struct vb2_queue *vq; - - vq = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx); switch (picture->picture_coding_type) { case V4L2_MPEG2_PICTURE_CODING_TYPE_B: - backward_addr = hantro_get_ref(vq, + backward_addr = hantro_get_ref(ctx, slice_params->backward_ref_ts); /* fall-through */ case V4L2_MPEG2_PICTURE_CODING_TYPE_P: - forward_addr = hantro_get_ref(vq, + forward_addr = hantro_get_ref(ctx, slice_params->forward_ref_ts); } diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c index e9d3361ed385..cad18094fee0 100644 --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c @@ -370,19 +370,18 @@ static void cfg_tap(struct hantro_ctx *ctx, static void cfg_ref(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp8_frame_header *hdr) { - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; struct hantro_dev *vpu = ctx->dev; struct vb2_v4l2_buffer *vb2_dst; dma_addr_t ref; vb2_dst = hantro_get_dst_buf(ctx); - ref = hantro_get_ref(cap_q, hdr->last_frame_ts); + ref = hantro_get_ref(ctx, hdr->last_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); vdpu_write_relaxed(vpu, ref, G1_REG_ADDR_REF(0)); - ref = hantro_get_ref(cap_q, hdr->golden_frame_ts); + ref = hantro_get_ref(ctx, hdr->golden_frame_ts); WARN_ON(!ref && hdr->golden_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); @@ -390,7 +389,7 @@ static void cfg_ref(struct hantro_ctx *ctx, ref |= G1_REG_ADDR_REF_TOPC_E; vdpu_write_relaxed(vpu, ref, G1_REG_ADDR_REF(4)); - ref = hantro_get_ref(cap_q, hdr->alt_frame_ts); + ref = hantro_get_ref(ctx, hdr->alt_frame_ts); WARN_ON(!ref && hdr->alt_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c index 0d758e0c0f99..2227b4e12067 100644 --- a/drivers/staging/media/hantro/hantro_h264.c +++ b/drivers/staging/media/hantro/hantro_h264.c @@ -537,22 +537,18 @@ static void update_dpb(struct hantro_ctx *ctx) } } -struct vb2_buffer *hantro_h264_get_ref_buf(struct hantro_ctx *ctx, - unsigned int dpb_idx) +dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx, + unsigned int dpb_idx) { - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb; - struct vb2_buffer *buf; - int buf_idx = -1; + dma_addr_t dma_addr = 0; if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) - buf_idx = vb2_find_timestamp(cap_q, - dpb[dpb_idx].reference_ts, 0); + dma_addr = hantro_get_ref(ctx, dpb[dpb_idx].reference_ts); - if (buf_idx >= 0) { - buf = vb2_get_buffer(cap_q, buf_idx); - } else { + if (!dma_addr) { struct vb2_v4l2_buffer *dst_buf; + struct vb2_buffer *buf; /* * If a DPB entry is unused or invalid, address of current @@ -560,9 +556,10 @@ struct vb2_buffer *hantro_h264_get_ref_buf(struct hantro_ctx *ctx, */ dst_buf = hantro_get_dst_buf(ctx); buf = &dst_buf->vb2_buf; + dma_addr = vb2_dma_contig_plane_dma_addr(buf, 0); } - return buf; + return dma_addr; } int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx) diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h index 2fab655bf098..69b88f4d3fb3 100644 --- a/drivers/staging/media/hantro/hantro_hw.h +++ b/drivers/staging/media/hantro/hantro_hw.h @@ -158,8 +158,8 @@ void rk3399_vpu_jpeg_enc_run(struct hantro_ctx *ctx); int hantro_jpeg_enc_init(struct hantro_ctx *ctx); void hantro_jpeg_enc_exit(struct hantro_ctx *ctx); -struct vb2_buffer *hantro_h264_get_ref_buf(struct hantro_ctx *ctx, - unsigned int dpb_idx); +dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx, + unsigned int dpb_idx); int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx); void hantro_g1_h264_dec_run(struct hantro_ctx *ctx); int hantro_h264_dec_init(struct hantro_ctx *ctx); diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c index 263ec81f209b..b40d2cdf832f 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_mpeg2_dec.c @@ -107,17 +107,14 @@ rk3399_vpu_mpeg2_dec_set_buffers(struct hantro_dev *vpu, { dma_addr_t forward_addr = 0, backward_addr = 0; dma_addr_t current_addr, addr; - struct vb2_queue *vq; - - vq = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx); switch (picture->picture_coding_type) { case V4L2_MPEG2_PICTURE_CODING_TYPE_B: - backward_addr = hantro_get_ref(vq, + backward_addr = hantro_get_ref(ctx, slice_params->backward_ref_ts); /* fall-through */ case V4L2_MPEG2_PICTURE_CODING_TYPE_P: - forward_addr = hantro_get_ref(vq, + forward_addr = hantro_get_ref(ctx, slice_params->forward_ref_ts); } diff --git a/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c b/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c index 7d32a0283d93..76d7ed3fd69a 100644 --- a/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c +++ b/drivers/staging/media/hantro/rk3399_vpu_hw_vp8_dec.c @@ -449,18 +449,16 @@ static void cfg_ref(struct hantro_ctx *ctx, { struct hantro_dev *vpu = ctx->dev; struct vb2_v4l2_buffer *vb2_dst; - struct vb2_queue *cap_q; dma_addr_t ref; - cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); vb2_dst = hantro_get_dst_buf(ctx); - ref = hantro_get_ref(cap_q, hdr->last_frame_ts); + ref = hantro_get_ref(ctx, hdr->last_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); vdpu_write_relaxed(vpu, ref, VDPU_REG_VP8_ADDR_REF0); - ref = hantro_get_ref(cap_q, hdr->golden_frame_ts); + ref = hantro_get_ref(ctx, hdr->golden_frame_ts); WARN_ON(!ref && hdr->golden_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); @@ -468,7 +466,7 @@ static void cfg_ref(struct hantro_ctx *ctx, ref |= VDPU_REG_VP8_GREF_SIGN_BIAS; vdpu_write_relaxed(vpu, ref, VDPU_REG_VP8_ADDR_REF2_5(2)); - ref = hantro_get_ref(cap_q, hdr->alt_frame_ts); + ref = hantro_get_ref(ctx, hdr->alt_frame_ts); WARN_ON(!ref && hdr->alt_frame_ts); if (!ref) ref = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); -- 2.22.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] media: hantro: Simplify buffer helpers 2019-09-03 18:17 ` [PATCH 2/4] media: hantro: Simplify buffer helpers Ezequiel Garcia @ 2019-09-04 10:50 ` Philipp Zabel 0 siblings, 0 replies; 18+ messages in thread From: Philipp Zabel @ 2019-09-04 10:50 UTC (permalink / raw) To: Ezequiel Garcia, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Boris Brezillon, Chris Healy On Tue, 2019-09-03 at 15:17 -0300, Ezequiel Garcia wrote: > Modify hantro_get_ref() and hantro_h264_get_ref_buf() helpers > to return the buffer DMA address, this makes the code simpler > and at the same time will allow easier enablement of the > post-processing feature. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 2/4] media: hantro: Simplify buffer helpers Ezequiel Garcia @ 2019-09-03 18:17 ` Ezequiel Garcia 2019-09-04 10:17 ` Philipp Zabel 2019-09-03 18:17 ` [PATCH 4/4] media: hantro: Support color conversion via post-processing Ezequiel Garcia 2019-09-09 7:07 ` [PATCH 0/4] Enable Hantro G1 post-processor Tomasz Figa 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-03 18:17 UTC (permalink / raw) To: linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy, Ezequiel Garcia Introduce a helper to allow easier enablement of the post-processing feature. No functional changes intended. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/hantro.h | 6 ++++++ drivers/staging/media/hantro/hantro_v4l2.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index deb90ae37859..e8872f24e351 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -381,4 +381,10 @@ hantro_get_dst_buf(struct hantro_ctx *ctx) return v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); } +static inline unsigned int +hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) +{ + return 128 * MB_WIDTH(width) * MB_HEIGHT(height); +} + #endif /* HANTRO_H_ */ diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c index d48b548842cf..59adecba0e85 100644 --- a/drivers/staging/media/hantro/hantro_v4l2.c +++ b/drivers/staging/media/hantro/hantro_v4l2.c @@ -246,8 +246,8 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f, */ if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE) pix_mp->plane_fmt[0].sizeimage += - 128 * DIV_ROUND_UP(pix_mp->width, 16) * - DIV_ROUND_UP(pix_mp->height, 16); + hantro_h264_buffer_extra_size(pix_mp->width, + pix_mp->height); } else if (!pix_mp->plane_fmt[0].sizeimage) { /* * For coded formats the application can specify -- 2.22.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation 2019-09-03 18:17 ` [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation Ezequiel Garcia @ 2019-09-04 10:17 ` Philipp Zabel 2019-09-04 12:50 ` Ezequiel Garcia 0 siblings, 1 reply; 18+ messages in thread From: Philipp Zabel @ 2019-09-04 10:17 UTC (permalink / raw) To: Ezequiel Garcia, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Boris Brezillon, Chris Healy On Tue, 2019-09-03 at 15:17 -0300, Ezequiel Garcia wrote: > Introduce a helper to allow easier enablement of the post-processing > feature. No functional changes intended. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/hantro/hantro.h | 6 ++++++ > drivers/staging/media/hantro/hantro_v4l2.c | 4 ++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index deb90ae37859..e8872f24e351 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -381,4 +381,10 @@ hantro_get_dst_buf(struct hantro_ctx *ctx) > return v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > } > > +static inline unsigned int > +hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) > +{ > + return 128 * MB_WIDTH(width) * MB_HEIGHT(height); > +} This doesn't seem to be used or modified by patch 4 at all? > + > #endif /* HANTRO_H_ */ > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > index d48b548842cf..59adecba0e85 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -246,8 +246,8 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f, > */ > if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE) > pix_mp->plane_fmt[0].sizeimage += > - 128 * DIV_ROUND_UP(pix_mp->width, 16) * > - DIV_ROUND_UP(pix_mp->height, 16); > + hantro_h264_buffer_extra_size(pix_mp->width, > + pix_mp->height); > } else if (!pix_mp->plane_fmt[0].sizeimage) { > /* > * For coded formats the application can specify regards Philipp ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation 2019-09-04 10:17 ` Philipp Zabel @ 2019-09-04 12:50 ` Ezequiel Garcia 0 siblings, 0 replies; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-04 12:50 UTC (permalink / raw) To: Philipp Zabel, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Boris Brezillon, Chris Healy On Wed, 2019-09-04 at 12:17 +0200, Philipp Zabel wrote: > On Tue, 2019-09-03 at 15:17 -0300, Ezequiel Garcia wrote: > > Introduce a helper to allow easier enablement of the post-processing > > feature. No functional changes intended. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/hantro/hantro.h | 6 ++++++ > > drivers/staging/media/hantro/hantro_v4l2.c | 4 ++-- > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > > index deb90ae37859..e8872f24e351 100644 > > --- a/drivers/staging/media/hantro/hantro.h > > +++ b/drivers/staging/media/hantro/hantro.h > > @@ -381,4 +381,10 @@ hantro_get_dst_buf(struct hantro_ctx *ctx) > > return v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > } > > > > +static inline unsigned int > > +hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) > > + > > + return 128 * MB_WIDTH(width) * MB_HEIGHT(height); > > +} > > This doesn't seem to be used or modified by patch 4 at all? > Oh, thanks for spotting this. Indeed, this patch is superflous. The helper was used, but then after cleaning-up hantro_postproc.c I realized it wasn't needed. We can drop this one, but OTOH hiding the H264 extra size seems a nice cleanup. Perhaps Jonas can grab this patch as part of his improvements series. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] media: hantro: Support color conversion via post-processing 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia ` (2 preceding siblings ...) 2019-09-03 18:17 ` [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation Ezequiel Garcia @ 2019-09-03 18:17 ` Ezequiel Garcia 2019-09-09 11:03 ` Hans Verkuil 2019-09-09 7:07 ` [PATCH 0/4] Enable Hantro G1 post-processor Tomasz Figa 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-03 18:17 UTC (permalink / raw) To: linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy, Ezequiel Garcia The Hantro G1 decoder is able to enable a post-processor on the decoding pipeline, which can be used to perform scaling and color conversion. The post-processor is integrated to the decoder, and it's possible to use it in a way that is completely transparent to the user. This commit enables color conversion via post-processing, which means the driver now exposes YUV packed and RGB pixel formats, in addition to NV12. On the YUV-to-RGB path, the post-processing pipeline allows to modify the image contrast, brigthness and saturation, so additional user controls are added to expose them. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/hantro/Makefile | 1 + drivers/staging/media/hantro/hantro.h | 23 +- drivers/staging/media/hantro/hantro_drv.c | 24 +- .../staging/media/hantro/hantro_g1_h264_dec.c | 2 +- .../media/hantro/hantro_g1_mpeg2_dec.c | 3 +- .../staging/media/hantro/hantro_g1_vp8_dec.c | 2 +- drivers/staging/media/hantro/hantro_h264.c | 7 +- drivers/staging/media/hantro/hantro_hw.h | 11 + .../staging/media/hantro/hantro_postproc.c | 316 ++++++++++++++++++ drivers/staging/media/hantro/hantro_v4l2.c | 21 +- drivers/staging/media/hantro/rk3288_vpu_hw.c | 21 ++ 11 files changed, 420 insertions(+), 11 deletions(-) create mode 100644 drivers/staging/media/hantro/hantro_postproc.c diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile index 5d6b0383d280..496b30c3c396 100644 --- a/drivers/staging/media/hantro/Makefile +++ b/drivers/staging/media/hantro/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_VIDEO_HANTRO) += hantro-vpu.o hantro-vpu-y += \ hantro_drv.o \ hantro_v4l2.o \ + hantro_postproc.o \ hantro_h1_jpeg_enc.o \ hantro_g1_h264_dec.o \ hantro_g1_mpeg2_dec.o \ diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h index e8872f24e351..8446a1fa9ab9 100644 --- a/drivers/staging/media/hantro/hantro.h +++ b/drivers/staging/media/hantro/hantro.h @@ -237,6 +237,7 @@ struct hantro_ctx { unsigned int bytesused); const struct hantro_codec_ops *codec_ops; + struct hantro_postproc_ctx postproc; /* Specific for particular codec modes. */ union { @@ -250,7 +251,8 @@ struct hantro_ctx { /** * struct hantro_fmt - information about supported video formats. * @name: Human readable name of the format. - * @fourcc: FourCC code of the format. See V4L2_PIX_FMT_*. + * @fourcc: FourCC code of the post-processed format. See V4L2_PIX_FMT_*. + * @dec_fourcc: FourCC code of the native format. See V4L2_PIX_FMT_*. * @codec_mode: Codec mode related to this format. See * enum hantro_codec_mode. * @header_size: Optional header size. Currently used by JPEG encoder. @@ -261,6 +263,7 @@ struct hantro_ctx { struct hantro_fmt { char *name; u32 fourcc; + u32 dec_fourcc; enum hantro_codec_mode codec_mode; int header_size; int max_depth; @@ -387,4 +390,22 @@ hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) return 128 * MB_WIDTH(width) * MB_HEIGHT(height); } +static inline bool +hantro_use_postproc(struct hantro_ctx *ctx) +{ + return ctx->vpu_dst_fmt->fourcc != ctx->vpu_dst_fmt->dec_fourcc; +} + +static inline dma_addr_t +hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb) +{ + if (hantro_use_postproc(ctx)) + return ctx->postproc.dec_q[vb->index].dma; + return vb2_dma_contig_plane_dma_addr(vb, 0); +} + +void hantro_postproc_setup(struct hantro_ctx *ctx); +void hantro_postproc_free(struct hantro_ctx *ctx); +int hantro_postproc_alloc(struct hantro_ctx *ctx); + #endif /* HANTRO_H_ */ diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index f550b68d46ca..300178562014 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -53,7 +53,7 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts) if (index < 0) return 0; buf = vb2_get_buffer(q, index); - return vb2_dma_contig_plane_dma_addr(buf, 0); + return hantro_get_dec_buf_addr(ctx, buf); } static int @@ -165,6 +165,9 @@ void hantro_finish_run(struct hantro_ctx *ctx) { struct vb2_v4l2_buffer *src_buf; + if (hantro_use_postproc(ctx)) + hantro_postproc_setup(ctx); + src_buf = hantro_get_src_buf(ctx); v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->ctrl_handler); @@ -365,8 +368,9 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, int allowed_codecs) { int i, num_ctrls = ARRAY_SIZE(controls); + int postproc_ctrls = (allowed_codecs & HANTRO_DECODERS) ? 3 : 0; - v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls); + v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls + postproc_ctrls); for (i = 0; i < num_ctrls; i++) { if (!(allowed_codecs & controls[i].codec)) @@ -382,6 +386,22 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, return ctx->ctrl_handler.error; } } + + /* + * Add post-processing controls. + * Only used if the post-processing path is enabled, + * when the application sets a CAPTURE pixel format + * that requires it. + */ + if (postproc_ctrls) { + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, + V4L2_CID_CONTRAST, -64, 64, 1, 0); + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, + V4L2_CID_BRIGHTNESS, -128, 127, 1, 0); + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, + V4L2_CID_SATURATION, -64, 128, 1, 0); + } + return v4l2_ctrl_handler_setup(&ctx->ctrl_handler); } diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c index 29130946dea4..e263a6b50651 100644 --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c @@ -241,7 +241,7 @@ static void set_buffers(struct hantro_ctx *ctx) vdpu_write_relaxed(vpu, src_dma, G1_REG_ADDR_STR); /* Destination (decoded frame) buffer. */ - dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); + dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf); vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); /* Higher profiles require DMV buffer appended to reference frames. */ diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c index f3bf67d8a289..63fe89ef21ae 100644 --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c @@ -121,7 +121,7 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx, vdpu_write_relaxed(vpu, addr, G1_REG_RLC_VLC_BASE); /* Destination frame buffer */ - addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0); + addr = hantro_get_dec_buf_addr(ctx, dst_buf); current_addr = addr; if (picture->picture_structure == PICT_BOTTOM_FIELD) @@ -243,7 +243,6 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx) hantro_g1_mpeg2_dec_set_buffers(vpu, ctx, &src_buf->vb2_buf, &dst_buf->vb2_buf, sequence, picture, slice_params); - hantro_finish_run(ctx); reg = G1_REG_DEC_E(1); diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c index cad18094fee0..e708994d1aba 100644 --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c @@ -422,7 +422,7 @@ static void cfg_buffers(struct hantro_ctx *ctx, } vdpu_write_relaxed(vpu, reg, G1_REG_FWD_PIC(0)); - dst_dma = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); + dst_dma = hantro_get_dec_buf_addr(ctx, &vb2_dst->vb2_buf); vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); } diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c index 2227b4e12067..d00b3096b7ae 100644 --- a/drivers/staging/media/hantro/hantro_h264.c +++ b/drivers/staging/media/hantro/hantro_h264.c @@ -13,6 +13,7 @@ #include <linux/types.h> #include <linux/sort.h> #include <media/v4l2-mem2mem.h> +#include <media/v4l2-common.h> #include "hantro.h" #include "hantro_hw.h" @@ -635,7 +636,11 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx) tbl = priv->cpu; memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table)); - v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat, + /* + * For the decoder picture size, we want the decoder + * native pixel format, so we use dec_fourcc here. + */ + v4l2_fill_pixfmt_mp(&pix_mp, ctx->vpu_dst_fmt->dec_fourcc, ctx->dst_fmt.width, ctx->dst_fmt.height); h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage; diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h index 69b88f4d3fb3..ae1d869e7c28 100644 --- a/drivers/staging/media/hantro/hantro_hw.h +++ b/drivers/staging/media/hantro/hantro_hw.h @@ -28,11 +28,13 @@ struct hantro_variant; * @cpu: CPU pointer to the buffer. * @dma: DMA address of the buffer. * @size: Size of the buffer. + * @attrs: Attributes of the DMA mapping. */ struct hantro_aux_buf { void *cpu; dma_addr_t dma; size_t size; + unsigned long attrs; }; /** @@ -109,6 +111,15 @@ struct hantro_vp8_dec_hw_ctx { struct hantro_aux_buf prob_tbl; }; +/** + * struct hantro_postproc_ctx + * + * @dec_q: References buffers, in decoder format. + */ +struct hantro_postproc_ctx { + struct hantro_aux_buf dec_q[VB2_MAX_FRAME]; +}; + /** * struct hantro_codec_ops - codec mode specific operations * diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c new file mode 100644 index 000000000000..17d023a253a8 --- /dev/null +++ b/drivers/staging/media/hantro/hantro_postproc.c @@ -0,0 +1,316 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Hantro G1 post-processor support + * + * Copyright (C) 2019 Collabora, Ltd. + */ + +#include <linux/dma-mapping.h> +#include <linux/types.h> + +#include "hantro.h" +#include "hantro_hw.h" + +#define G1_SWREG(nr) ((nr) * 4) + +#define G1_REG_PP_INTERRUPT G1_SWREG(60) +#define G1_REG_PP_READY_IRQ BIT(12) +#define G1_REG_PP_IRQ BIT(8) +#define G1_REG_PP_IRQ_DIS BIT(4) +#define G1_REG_PP_PIPELINE_EN BIT(1) +#define G1_REG_PP_EXTERNAL_TRIGGER BIT(0) +#define G1_REG_PP_DEV_CONFIG G1_SWREG(61) +#define G1_REG_PP_AXI_RD_ID(v) (((v) << 24) & GENMASK(31, 24)) +#define G1_REG_PP_AXI_WR_ID(v) (((v) << 16) & GENMASK(23, 16)) +#define G1_REG_PP_INSWAP32_E(v) ((v) ? BIT(10) : 0) +#define G1_REG_PP_DATA_DISC_E(v) ((v) ? BIT(9) : 0) +#define G1_REG_PP_CLK_GATE_E(v) ((v) ? BIT(8) : 0) +#define G1_REG_PP_IN_ENDIAN(v) ((v) ? BIT(7) : 0) +#define G1_REG_PP_OUT_ENDIAN(v) ((v) ? BIT(6) : 0) +#define G1_REG_PP_OUTSWAP32_E(v) ((v) ? BIT(5) : 0) +#define G1_REG_PP_MAX_BURST(v) (((v) << 0) & GENMASK(4, 0)) +#define G1_REG_PP_IN_LUMA_BASE G1_SWREG(63) +#define G1_REG_PP_IN_CB_BASE G1_SWREG(64) +#define G1_REG_PP_IN_CR_BASE G1_SWREG(65) +#define G1_REG_PP_OUT_LUMA_BASE G1_SWREG(66) +#define G1_REG_PP_OUT_CHROMA_BASE G1_SWREG(67) +#define G1_REG_PP_CONTRAST_ADJUST G1_SWREG(68) +#define G1_REG_PP_COLOR_CONVERSION G1_SWREG(69) +#define G1_REG_PP_COLOR_CONVERSION0 G1_SWREG(70) +#define G1_REG_PP_COLOR_CONVERSION1 G1_SWREG(71) +#define G1_REG_PP_INPUT_SIZE G1_SWREG(72) +#define G1_REG_PP_INPUT_SIZE_HEIGHT(v) (((v) << 9) & GENMASK(16, 9)) +#define G1_REG_PP_INPUT_SIZE_WIDTH(v) (((v) << 0) & GENMASK(8, 0)) +#define G1_REG_PP_SCALING0 G1_SWREG(79) +#define G1_REG_PP_PADD_R(v) (((v) << 23) & GENMASK(27, 23)) +#define G1_REG_PP_PADD_G(v) (((v) << 18) & GENMASK(22, 18)) +#define G1_REG_PP_RANGEMAP_Y(v) ((v) ? BIT(31) : 0) +#define G1_REG_PP_RANGEMAP_C(v) ((v) ? BIT(30) : 0) +#define G1_REG_PP_YCBCR_RANGE(v) ((v) ? BIT(29) : 0) +#define G1_REG_PP_RGB_16(v) ((v) ? BIT(28) : 0) +#define G1_REG_PP_SCALING1 G1_SWREG(80) +#define G1_REG_PP_PADD_B(v) (((v) << 18) & GENMASK(22, 18)) +#define G1_REG_PP_MASK_R G1_SWREG(82) +#define G1_REG_PP_MASK_G G1_SWREG(83) +#define G1_REG_PP_MASK_B G1_SWREG(84) +#define G1_REG_PP_CONTROL G1_SWREG(85) +#define G1_REG_PP_CONTROL_IN_FMT(v) (((v) << 29) & GENMASK(31, 29)) +#define G1_REG_PP_CONTROL_OUT_FMT(v) (((v) << 26) & GENMASK(28, 26)) +#define G1_REG_PP_CONTROL_OUT_HEIGHT(v) (((v) << 15) & GENMASK(25, 15)) +#define G1_REG_PP_CONTROL_OUT_WIDTH(v) (((v) << 4) & GENMASK(14, 4)) +#define G1_REG_PP_MASK1_ORIG_WIDTH G1_SWREG(88) +#define G1_REG_PP_ORIG_WIDTH(v) (((v) << 23) & GENMASK(31, 23)) +#define G1_REG_PP_DISPLAY_WIDTH G1_SWREG(92) +#define G1_REG_PP_FUSE G1_SWREG(99) + +#define VPU_PP_IN_YUYV 0x0 +#define VPU_PP_IN_NV12 0x1 +#define VPU_PP_IN_YUV420 0x2 +#define VPU_PP_IN_YUV240_TILED 0x5 +#define VPU_PP_OUT_RGB 0x0 +#define VPU_PP_OUT_YUYV 0x3 + +#define DEF_COLOR_COEFF_A 298 +#define DEF_COLOR_COEFF_B 409 +#define DEF_COLOR_COEFF_C 208 +#define DEF_COLOR_COEFF_D 100 +#define DEF_COLOR_COEFF_E 516 + +static const struct hantro_reg reg_color_a2 = { G1_REG_PP_COLOR_CONVERSION, 18, 0x3ff }; +static const struct hantro_reg reg_color_a1 = { G1_REG_PP_COLOR_CONVERSION, 8, 0x3ff }; +static const struct hantro_reg reg_color_b = { G1_REG_PP_COLOR_CONVERSION0, 0, 0x3ff }; +static const struct hantro_reg reg_color_c = { G1_REG_PP_COLOR_CONVERSION0, 10, 0x3ff }; +static const struct hantro_reg reg_color_d = { G1_REG_PP_COLOR_CONVERSION0, 20, 0x3ff }; +static const struct hantro_reg reg_color_e = { G1_REG_PP_COLOR_CONVERSION1, 0, 0x3ff }; +static const struct hantro_reg reg_color_f = { G1_REG_PP_COLOR_CONVERSION1, 10, 0xff }; +static const struct hantro_reg reg_contrast_thr1 = { G1_REG_PP_CONTRAST_ADJUST, 24, 0xff }; +static const struct hantro_reg reg_contrast_thr2 = { G1_REG_PP_COLOR_CONVERSION, 0, 0xff }; +static const struct hantro_reg reg_contrast_off1 = { G1_REG_PP_CONTRAST_ADJUST, 0, 0x3ff }; +static const struct hantro_reg reg_contrast_off2 = { G1_REG_PP_CONTRAST_ADJUST, 10, 0x3ff }; +static const struct hantro_reg reg_ycbcr_range = { G1_REG_PP_SCALING0, 29, 0x1 }; +static const struct hantro_reg reg_pad_r = { G1_REG_PP_SCALING0, 23, 0x1f }; +static const struct hantro_reg reg_pad_g = { G1_REG_PP_SCALING0, 18, 0x1f }; +static const struct hantro_reg reg_pad_b = { G1_REG_PP_SCALING1, 18, 0x1f }; + +static s32 hantro_postproc_get_ctrl(struct hantro_ctx *ctx, u32 id, s32 def_val) +{ + struct v4l2_ctrl *ctrl; + + ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, id); + if (!ctrl) + return def_val; + return v4l2_ctrl_g_ctrl(ctrl); +} + +static void hantro_postproc_setup_rgb(struct hantro_ctx *ctx) +{ + struct hantro_dev *vpu = ctx->dev; + s32 thr1y, thr2y; + s32 thr1, thr2; + s32 off1, off2; + s32 a1, a2; + s32 tmp, satur; + s32 contrast, saturation, brightness; + + contrast = hantro_postproc_get_ctrl(ctx, V4L2_CID_CONTRAST, 0); + brightness = hantro_postproc_get_ctrl(ctx, V4L2_CID_BRIGHTNESS, 0); + saturation = hantro_postproc_get_ctrl(ctx, V4L2_CID_SATURATION, 0); + + if (ctx->src_fmt.quantization == V4L2_QUANTIZATION_LIM_RANGE) { + s32 tmp1, tmp2; + + thr1 = (219 * (contrast + 128)) / 512; + thr1y = (219 - 2 * thr1) / 2; + thr2 = 219 - thr1; + thr2y = 219 - thr1y; + + tmp1 = (thr1y * 256) / thr1; + tmp2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); + off1 = ((thr1y - ((tmp2 * thr1) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; + off2 = ((thr2y - ((tmp1 * thr2) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; + + tmp1 = (64 * (contrast + 128)) / 128; + tmp2 = 256 * (128 - tmp1); + a1 = (tmp2 + off2) / thr1; + a2 = a1 + (256 * (off2 - 1)) / (thr2 - thr1); + } else { + thr1 = (64 * (contrast + 128)) / 128; + thr1y = 128 - thr1; + thr2 = 256 - thr1; + thr2y = 256 - thr1y; + a1 = (thr1y * 256) / thr1; + a2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); + off1 = thr1y - (a2 * thr1) / 256; + off2 = thr2y - (a1 * thr2) / 256; + } + + a1 = clamp(a1, 0, 1023); + a2 = clamp(a2, 0, 1023); + thr1 = clamp(thr1, 0, 255); + thr2 = clamp(thr2, 0, 255); + off1 = clamp(off1, -512, 511); + off2 = clamp(off2, -512, 511); + + hantro_reg_write(vpu, ®_contrast_thr1, thr1); + hantro_reg_write(vpu, ®_contrast_thr2, thr2); + hantro_reg_write(vpu, ®_contrast_off1, off1); + hantro_reg_write(vpu, ®_contrast_off2, off2); + hantro_reg_write(vpu, ®_color_a1, a1); + hantro_reg_write(vpu, ®_color_a2, a2); + + /* Brightness */ + hantro_reg_write(vpu, ®_color_f, brightness); + + /* Saturation */ + satur = 64 + saturation; + tmp = (satur * (s32)DEF_COLOR_COEFF_B) / 64; + tmp = clamp(tmp, 0, 1023); + hantro_reg_write(vpu, ®_color_b, tmp); + + tmp = (satur * (s32)DEF_COLOR_COEFF_C) / 64; + tmp = clamp(tmp, 0, 1023); + hantro_reg_write(vpu, ®_color_c, tmp); + + tmp = (satur * (s32)DEF_COLOR_COEFF_D) / 64; + tmp = clamp(tmp, 0, 1023); + hantro_reg_write(vpu, ®_color_d, tmp); + + tmp = (satur * (s32)DEF_COLOR_COEFF_E) / 64; + tmp = clamp(tmp, 0, 1023); + hantro_reg_write(vpu, ®_color_e, tmp); + + /* Set RGB padding and mask */ + switch (ctx->vpu_dst_fmt->fourcc) { + case V4L2_PIX_FMT_RGBX32: + hantro_reg_write(vpu, ®_pad_r, 0); + hantro_reg_write(vpu, ®_pad_g, 8); + hantro_reg_write(vpu, ®_pad_b, 16); + + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_R); + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_B); + break; + case V4L2_PIX_FMT_XRGB32: + hantro_reg_write(vpu, ®_pad_r, 8); + hantro_reg_write(vpu, ®_pad_g, 16); + hantro_reg_write(vpu, ®_pad_b, 24); + + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_R); + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_G); + vdpu_write_relaxed(vpu, 0x000000ff, G1_REG_PP_MASK_B); + break; + case V4L2_PIX_FMT_XBGR32: + hantro_reg_write(vpu, ®_pad_b, 0); + hantro_reg_write(vpu, ®_pad_g, 8); + hantro_reg_write(vpu, ®_pad_r, 16); + + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_B); + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_R); + break; + } +} + +void hantro_postproc_setup(struct hantro_ctx *ctx) +{ + struct hantro_dev *vpu = ctx->dev; + struct vb2_v4l2_buffer *dst_buf; + u32 reg, src_pp_fmt, dst_pp_fmt; + dma_addr_t dst_dma; + + /* Turn on pipeline mode. Must be done first. */ + vdpu_write(vpu, G1_REG_PP_PIPELINE_EN, G1_REG_PP_INTERRUPT); + + reg = G1_REG_PP_AXI_RD_ID(0) | + G1_REG_PP_AXI_WR_ID(0) | + G1_REG_PP_INSWAP32_E(1) | + G1_REG_PP_OUTSWAP32_E(1) | + G1_REG_PP_DATA_DISC_E(0) | + G1_REG_PP_CLK_GATE_E(1) | + G1_REG_PP_IN_ENDIAN(1) | + G1_REG_PP_OUT_ENDIAN(1) | + G1_REG_PP_MAX_BURST(16); + vdpu_write_relaxed(vpu, reg, G1_REG_PP_DEV_CONFIG); + + /* Configure picture resolution. */ + reg = G1_REG_PP_INPUT_SIZE_HEIGHT(MB_HEIGHT(ctx->dst_fmt.height)) | + G1_REG_PP_INPUT_SIZE_WIDTH(MB_WIDTH(ctx->dst_fmt.width)); + vdpu_write_relaxed(vpu, reg, G1_REG_PP_INPUT_SIZE); + + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); + dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); + vdpu_write_relaxed(vpu, dst_dma, G1_REG_PP_OUT_LUMA_BASE); + + reg = (ctx->dst_fmt.quantization == + V4L2_QUANTIZATION_LIM_RANGE) ? 0 : 1; + hantro_reg_write(vpu, ®_ycbcr_range, reg); + + /* Clear this register before setting the RGB coefficients. */ + vdpu_write_relaxed(vpu, 0, G1_REG_PP_SCALING1); + + switch (ctx->vpu_dst_fmt->fourcc) { + case V4L2_PIX_FMT_YUYV: + dst_pp_fmt = VPU_PP_OUT_YUYV; + break; + case V4L2_PIX_FMT_RGBX32: + case V4L2_PIX_FMT_XRGB32: + case V4L2_PIX_FMT_XBGR32: + dst_pp_fmt = VPU_PP_OUT_RGB; + hantro_postproc_setup_rgb(ctx); + break; + default: + return; + } + + /* Currently NV12 is the only supported input pixel format. */ + src_pp_fmt = VPU_PP_IN_NV12; + + /* Configure src/dst post-processor formats. */ + reg = G1_REG_PP_CONTROL_IN_FMT(src_pp_fmt) | + G1_REG_PP_CONTROL_OUT_FMT(dst_pp_fmt) | + G1_REG_PP_CONTROL_OUT_HEIGHT(ctx->dst_fmt.height) | + G1_REG_PP_CONTROL_OUT_WIDTH(ctx->dst_fmt.width); + vdpu_write_relaxed(vpu, reg, G1_REG_PP_CONTROL); + + reg = G1_REG_PP_ORIG_WIDTH(MB_WIDTH(ctx->dst_fmt.width)); + vdpu_write_relaxed(vpu, reg, G1_REG_PP_MASK1_ORIG_WIDTH); + vdpu_write_relaxed(vpu, ctx->dst_fmt.width, G1_REG_PP_DISPLAY_WIDTH); +} + +void hantro_postproc_free(struct hantro_ctx *ctx) +{ + struct hantro_dev *vpu = ctx->dev; + unsigned int i; + + for (i = 0; i < VB2_MAX_FRAME; ++i) { + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; + + if (priv->cpu) { + dma_free_attrs(vpu->dev, priv->size, priv->cpu, + priv->dma, priv->attrs); + priv->cpu = NULL; + } + } +} + +int hantro_postproc_alloc(struct hantro_ctx *ctx) +{ + struct hantro_dev *vpu = ctx->dev; + unsigned int i, buf_size; + + buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage; + + for (i = 0; i < VB2_MAX_FRAME; ++i) { + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; + + priv->attrs = DMA_ATTR_NO_KERNEL_MAPPING | + DMA_ATTR_WRITE_COMBINE | + DMA_ATTR_NON_CONSISTENT; + priv->cpu = dma_alloc_attrs(vpu->dev, buf_size, &priv->dma, + GFP_KERNEL, priv->attrs); + if (!priv->cpu) + return -ENOMEM; + priv->size = buf_size; + } + return 0; +} diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c index 59adecba0e85..37a9ba71afa6 100644 --- a/drivers/staging/media/hantro/hantro_v4l2.c +++ b/drivers/staging/media/hantro/hantro_v4l2.c @@ -242,9 +242,10 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f, /* * The H264 decoder needs extra space on the output buffers * to store motion vectors. This is needed for reference - * frames. + * frames and only if the format is non-post-processed (NV12). */ - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE) + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && + fmt->fourcc == V4L2_PIX_FMT_NV12) pix_mp->plane_fmt[0].sizeimage += hantro_h264_buffer_extra_size(pix_mp->width, pix_mp->height); @@ -634,10 +635,23 @@ static int hantro_start_streaming(struct vb2_queue *q, unsigned int count) vpu_debug(4, "Codec mode = %d\n", codec_mode); ctx->codec_ops = &ctx->dev->variant->codec_ops[codec_mode]; - if (ctx->codec_ops->init) + if (ctx->codec_ops->init) { ret = ctx->codec_ops->init(ctx); + if (ret) + return ret; + } + + if (hantro_use_postproc(ctx)) { + ret = hantro_postproc_alloc(ctx); + if (ret) + goto err_codec_exit; + } } + return ret; +err_codec_exit: + if (ctx->codec_ops && ctx->codec_ops->exit) + ctx->codec_ops->exit(ctx); return ret; } @@ -664,6 +678,7 @@ static void hantro_stop_streaming(struct vb2_queue *q) struct hantro_ctx *ctx = vb2_get_drv_priv(q); if (hantro_vq_is_coded(q)) { + hantro_postproc_free(ctx); if (ctx->codec_ops && ctx->codec_ops->exit) ctx->codec_ops->exit(ctx); } diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c index c0bdd6c02520..18d6b725309b 100644 --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c @@ -59,6 +59,27 @@ static const struct hantro_fmt rk3288_vpu_enc_fmts[] = { static const struct hantro_fmt rk3288_vpu_dec_fmts[] = { { .fourcc = V4L2_PIX_FMT_NV12, + .dec_fourcc = V4L2_PIX_FMT_NV12, + .codec_mode = HANTRO_MODE_NONE, + }, + { + .fourcc = V4L2_PIX_FMT_RGBX32, + .dec_fourcc = V4L2_PIX_FMT_NV12, + .codec_mode = HANTRO_MODE_NONE, + }, + { + .fourcc = V4L2_PIX_FMT_XRGB32, + .dec_fourcc = V4L2_PIX_FMT_NV12, + .codec_mode = HANTRO_MODE_NONE, + }, + { + .fourcc = V4L2_PIX_FMT_XBGR32, + .dec_fourcc = V4L2_PIX_FMT_NV12, + .codec_mode = HANTRO_MODE_NONE, + }, + { + .fourcc = V4L2_PIX_FMT_YUYV, + .dec_fourcc = V4L2_PIX_FMT_NV12, .codec_mode = HANTRO_MODE_NONE, }, { -- 2.22.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] media: hantro: Support color conversion via post-processing 2019-09-03 18:17 ` [PATCH 4/4] media: hantro: Support color conversion via post-processing Ezequiel Garcia @ 2019-09-09 11:03 ` Hans Verkuil 2019-09-11 8:34 ` Ezequiel Garcia 0 siblings, 1 reply; 18+ messages in thread From: Hans Verkuil @ 2019-09-09 11:03 UTC (permalink / raw) To: Ezequiel Garcia, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy On 9/3/19 8:17 PM, Ezequiel Garcia wrote: > The Hantro G1 decoder is able to enable a post-processor > on the decoding pipeline, which can be used to perform > scaling and color conversion. > > The post-processor is integrated to the decoder, and it's > possible to use it in a way that is completely transparent > to the user. > > This commit enables color conversion via post-processing, > which means the driver now exposes YUV packed and RGB pixel > formats, in addition to NV12. > > On the YUV-to-RGB path, the post-processing pipeline > allows to modify the image contrast, brigthness and saturation, > so additional user controls are added to expose them. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/hantro/Makefile | 1 + > drivers/staging/media/hantro/hantro.h | 23 +- > drivers/staging/media/hantro/hantro_drv.c | 24 +- > .../staging/media/hantro/hantro_g1_h264_dec.c | 2 +- > .../media/hantro/hantro_g1_mpeg2_dec.c | 3 +- > .../staging/media/hantro/hantro_g1_vp8_dec.c | 2 +- > drivers/staging/media/hantro/hantro_h264.c | 7 +- > drivers/staging/media/hantro/hantro_hw.h | 11 + > .../staging/media/hantro/hantro_postproc.c | 316 ++++++++++++++++++ > drivers/staging/media/hantro/hantro_v4l2.c | 21 +- > drivers/staging/media/hantro/rk3288_vpu_hw.c | 21 ++ > 11 files changed, 420 insertions(+), 11 deletions(-) > create mode 100644 drivers/staging/media/hantro/hantro_postproc.c > > diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile > index 5d6b0383d280..496b30c3c396 100644 > --- a/drivers/staging/media/hantro/Makefile > +++ b/drivers/staging/media/hantro/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_VIDEO_HANTRO) += hantro-vpu.o > hantro-vpu-y += \ > hantro_drv.o \ > hantro_v4l2.o \ > + hantro_postproc.o \ > hantro_h1_jpeg_enc.o \ > hantro_g1_h264_dec.o \ > hantro_g1_mpeg2_dec.o \ > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > index e8872f24e351..8446a1fa9ab9 100644 > --- a/drivers/staging/media/hantro/hantro.h > +++ b/drivers/staging/media/hantro/hantro.h > @@ -237,6 +237,7 @@ struct hantro_ctx { > unsigned int bytesused); > > const struct hantro_codec_ops *codec_ops; > + struct hantro_postproc_ctx postproc; > > /* Specific for particular codec modes. */ > union { > @@ -250,7 +251,8 @@ struct hantro_ctx { > /** > * struct hantro_fmt - information about supported video formats. > * @name: Human readable name of the format. > - * @fourcc: FourCC code of the format. See V4L2_PIX_FMT_*. > + * @fourcc: FourCC code of the post-processed format. See V4L2_PIX_FMT_*. > + * @dec_fourcc: FourCC code of the native format. See V4L2_PIX_FMT_*. > * @codec_mode: Codec mode related to this format. See > * enum hantro_codec_mode. > * @header_size: Optional header size. Currently used by JPEG encoder. > @@ -261,6 +263,7 @@ struct hantro_ctx { > struct hantro_fmt { > char *name; > u32 fourcc; > + u32 dec_fourcc; > enum hantro_codec_mode codec_mode; > int header_size; > int max_depth; > @@ -387,4 +390,22 @@ hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) > return 128 * MB_WIDTH(width) * MB_HEIGHT(height); > } > > +static inline bool > +hantro_use_postproc(struct hantro_ctx *ctx) > +{ > + return ctx->vpu_dst_fmt->fourcc != ctx->vpu_dst_fmt->dec_fourcc; > +} > + > +static inline dma_addr_t > +hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb) > +{ > + if (hantro_use_postproc(ctx)) > + return ctx->postproc.dec_q[vb->index].dma; > + return vb2_dma_contig_plane_dma_addr(vb, 0); > +} > + > +void hantro_postproc_setup(struct hantro_ctx *ctx); > +void hantro_postproc_free(struct hantro_ctx *ctx); > +int hantro_postproc_alloc(struct hantro_ctx *ctx); > + > #endif /* HANTRO_H_ */ > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index f550b68d46ca..300178562014 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -53,7 +53,7 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts) > if (index < 0) > return 0; > buf = vb2_get_buffer(q, index); > - return vb2_dma_contig_plane_dma_addr(buf, 0); > + return hantro_get_dec_buf_addr(ctx, buf); > } > > static int > @@ -165,6 +165,9 @@ void hantro_finish_run(struct hantro_ctx *ctx) > { > struct vb2_v4l2_buffer *src_buf; > > + if (hantro_use_postproc(ctx)) > + hantro_postproc_setup(ctx); > + > src_buf = hantro_get_src_buf(ctx); > v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, > &ctx->ctrl_handler); > @@ -365,8 +368,9 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, > int allowed_codecs) > { > int i, num_ctrls = ARRAY_SIZE(controls); > + int postproc_ctrls = (allowed_codecs & HANTRO_DECODERS) ? 3 : 0; > > - v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls); > + v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls + postproc_ctrls); > > for (i = 0; i < num_ctrls; i++) { > if (!(allowed_codecs & controls[i].codec)) > @@ -382,6 +386,22 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, > return ctx->ctrl_handler.error; > } > } > + > + /* > + * Add post-processing controls. > + * Only used if the post-processing path is enabled, > + * when the application sets a CAPTURE pixel format > + * that requires it. > + */ > + if (postproc_ctrls) { > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > + V4L2_CID_CONTRAST, -64, 64, 1, 0); > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > + V4L2_CID_BRIGHTNESS, -128, 127, 1, 0); > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > + V4L2_CID_SATURATION, -64, 128, 1, 0); > + } > + > return v4l2_ctrl_handler_setup(&ctx->ctrl_handler); > } > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > index 29130946dea4..e263a6b50651 100644 > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > @@ -241,7 +241,7 @@ static void set_buffers(struct hantro_ctx *ctx) > vdpu_write_relaxed(vpu, src_dma, G1_REG_ADDR_STR); > > /* Destination (decoded frame) buffer. */ > - dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); > + dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf); > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > /* Higher profiles require DMV buffer appended to reference frames. */ > diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > index f3bf67d8a289..63fe89ef21ae 100644 > --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > @@ -121,7 +121,7 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx, > vdpu_write_relaxed(vpu, addr, G1_REG_RLC_VLC_BASE); > > /* Destination frame buffer */ > - addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0); > + addr = hantro_get_dec_buf_addr(ctx, dst_buf); > current_addr = addr; > > if (picture->picture_structure == PICT_BOTTOM_FIELD) > @@ -243,7 +243,6 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx) > hantro_g1_mpeg2_dec_set_buffers(vpu, ctx, &src_buf->vb2_buf, > &dst_buf->vb2_buf, > sequence, picture, slice_params); > - > hantro_finish_run(ctx); > > reg = G1_REG_DEC_E(1); > diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > index cad18094fee0..e708994d1aba 100644 > --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > @@ -422,7 +422,7 @@ static void cfg_buffers(struct hantro_ctx *ctx, > } > vdpu_write_relaxed(vpu, reg, G1_REG_FWD_PIC(0)); > > - dst_dma = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); > + dst_dma = hantro_get_dec_buf_addr(ctx, &vb2_dst->vb2_buf); > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > } > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c > index 2227b4e12067..d00b3096b7ae 100644 > --- a/drivers/staging/media/hantro/hantro_h264.c > +++ b/drivers/staging/media/hantro/hantro_h264.c > @@ -13,6 +13,7 @@ > #include <linux/types.h> > #include <linux/sort.h> > #include <media/v4l2-mem2mem.h> > +#include <media/v4l2-common.h> > > #include "hantro.h" > #include "hantro_hw.h" > @@ -635,7 +636,11 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx) > tbl = priv->cpu; > memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table)); > > - v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat, > + /* > + * For the decoder picture size, we want the decoder > + * native pixel format, so we use dec_fourcc here. > + */ > + v4l2_fill_pixfmt_mp(&pix_mp, ctx->vpu_dst_fmt->dec_fourcc, > ctx->dst_fmt.width, ctx->dst_fmt.height); > h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage; > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > index 69b88f4d3fb3..ae1d869e7c28 100644 > --- a/drivers/staging/media/hantro/hantro_hw.h > +++ b/drivers/staging/media/hantro/hantro_hw.h > @@ -28,11 +28,13 @@ struct hantro_variant; > * @cpu: CPU pointer to the buffer. > * @dma: DMA address of the buffer. > * @size: Size of the buffer. > + * @attrs: Attributes of the DMA mapping. > */ > struct hantro_aux_buf { > void *cpu; > dma_addr_t dma; > size_t size; > + unsigned long attrs; > }; > > /** > @@ -109,6 +111,15 @@ struct hantro_vp8_dec_hw_ctx { > struct hantro_aux_buf prob_tbl; > }; > > +/** > + * struct hantro_postproc_ctx > + * > + * @dec_q: References buffers, in decoder format. > + */ > +struct hantro_postproc_ctx { > + struct hantro_aux_buf dec_q[VB2_MAX_FRAME]; > +}; > + > /** > * struct hantro_codec_ops - codec mode specific operations > * > diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c > new file mode 100644 > index 000000000000..17d023a253a8 > --- /dev/null > +++ b/drivers/staging/media/hantro/hantro_postproc.c > @@ -0,0 +1,316 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Hantro G1 post-processor support > + * > + * Copyright (C) 2019 Collabora, Ltd. > + */ > + > +#include <linux/dma-mapping.h> > +#include <linux/types.h> > + > +#include "hantro.h" > +#include "hantro_hw.h" > + > +#define G1_SWREG(nr) ((nr) * 4) > + > +#define G1_REG_PP_INTERRUPT G1_SWREG(60) > +#define G1_REG_PP_READY_IRQ BIT(12) > +#define G1_REG_PP_IRQ BIT(8) > +#define G1_REG_PP_IRQ_DIS BIT(4) > +#define G1_REG_PP_PIPELINE_EN BIT(1) > +#define G1_REG_PP_EXTERNAL_TRIGGER BIT(0) > +#define G1_REG_PP_DEV_CONFIG G1_SWREG(61) > +#define G1_REG_PP_AXI_RD_ID(v) (((v) << 24) & GENMASK(31, 24)) > +#define G1_REG_PP_AXI_WR_ID(v) (((v) << 16) & GENMASK(23, 16)) > +#define G1_REG_PP_INSWAP32_E(v) ((v) ? BIT(10) : 0) > +#define G1_REG_PP_DATA_DISC_E(v) ((v) ? BIT(9) : 0) > +#define G1_REG_PP_CLK_GATE_E(v) ((v) ? BIT(8) : 0) > +#define G1_REG_PP_IN_ENDIAN(v) ((v) ? BIT(7) : 0) > +#define G1_REG_PP_OUT_ENDIAN(v) ((v) ? BIT(6) : 0) > +#define G1_REG_PP_OUTSWAP32_E(v) ((v) ? BIT(5) : 0) > +#define G1_REG_PP_MAX_BURST(v) (((v) << 0) & GENMASK(4, 0)) > +#define G1_REG_PP_IN_LUMA_BASE G1_SWREG(63) > +#define G1_REG_PP_IN_CB_BASE G1_SWREG(64) > +#define G1_REG_PP_IN_CR_BASE G1_SWREG(65) > +#define G1_REG_PP_OUT_LUMA_BASE G1_SWREG(66) > +#define G1_REG_PP_OUT_CHROMA_BASE G1_SWREG(67) > +#define G1_REG_PP_CONTRAST_ADJUST G1_SWREG(68) > +#define G1_REG_PP_COLOR_CONVERSION G1_SWREG(69) > +#define G1_REG_PP_COLOR_CONVERSION0 G1_SWREG(70) > +#define G1_REG_PP_COLOR_CONVERSION1 G1_SWREG(71) > +#define G1_REG_PP_INPUT_SIZE G1_SWREG(72) > +#define G1_REG_PP_INPUT_SIZE_HEIGHT(v) (((v) << 9) & GENMASK(16, 9)) > +#define G1_REG_PP_INPUT_SIZE_WIDTH(v) (((v) << 0) & GENMASK(8, 0)) > +#define G1_REG_PP_SCALING0 G1_SWREG(79) > +#define G1_REG_PP_PADD_R(v) (((v) << 23) & GENMASK(27, 23)) > +#define G1_REG_PP_PADD_G(v) (((v) << 18) & GENMASK(22, 18)) > +#define G1_REG_PP_RANGEMAP_Y(v) ((v) ? BIT(31) : 0) > +#define G1_REG_PP_RANGEMAP_C(v) ((v) ? BIT(30) : 0) > +#define G1_REG_PP_YCBCR_RANGE(v) ((v) ? BIT(29) : 0) > +#define G1_REG_PP_RGB_16(v) ((v) ? BIT(28) : 0) > +#define G1_REG_PP_SCALING1 G1_SWREG(80) > +#define G1_REG_PP_PADD_B(v) (((v) << 18) & GENMASK(22, 18)) > +#define G1_REG_PP_MASK_R G1_SWREG(82) > +#define G1_REG_PP_MASK_G G1_SWREG(83) > +#define G1_REG_PP_MASK_B G1_SWREG(84) > +#define G1_REG_PP_CONTROL G1_SWREG(85) > +#define G1_REG_PP_CONTROL_IN_FMT(v) (((v) << 29) & GENMASK(31, 29)) > +#define G1_REG_PP_CONTROL_OUT_FMT(v) (((v) << 26) & GENMASK(28, 26)) > +#define G1_REG_PP_CONTROL_OUT_HEIGHT(v) (((v) << 15) & GENMASK(25, 15)) > +#define G1_REG_PP_CONTROL_OUT_WIDTH(v) (((v) << 4) & GENMASK(14, 4)) > +#define G1_REG_PP_MASK1_ORIG_WIDTH G1_SWREG(88) > +#define G1_REG_PP_ORIG_WIDTH(v) (((v) << 23) & GENMASK(31, 23)) > +#define G1_REG_PP_DISPLAY_WIDTH G1_SWREG(92) > +#define G1_REG_PP_FUSE G1_SWREG(99) > + > +#define VPU_PP_IN_YUYV 0x0 > +#define VPU_PP_IN_NV12 0x1 > +#define VPU_PP_IN_YUV420 0x2 > +#define VPU_PP_IN_YUV240_TILED 0x5 > +#define VPU_PP_OUT_RGB 0x0 > +#define VPU_PP_OUT_YUYV 0x3 > + > +#define DEF_COLOR_COEFF_A 298 > +#define DEF_COLOR_COEFF_B 409 > +#define DEF_COLOR_COEFF_C 208 > +#define DEF_COLOR_COEFF_D 100 > +#define DEF_COLOR_COEFF_E 516 > + > +static const struct hantro_reg reg_color_a2 = { G1_REG_PP_COLOR_CONVERSION, 18, 0x3ff }; > +static const struct hantro_reg reg_color_a1 = { G1_REG_PP_COLOR_CONVERSION, 8, 0x3ff }; > +static const struct hantro_reg reg_color_b = { G1_REG_PP_COLOR_CONVERSION0, 0, 0x3ff }; > +static const struct hantro_reg reg_color_c = { G1_REG_PP_COLOR_CONVERSION0, 10, 0x3ff }; > +static const struct hantro_reg reg_color_d = { G1_REG_PP_COLOR_CONVERSION0, 20, 0x3ff }; > +static const struct hantro_reg reg_color_e = { G1_REG_PP_COLOR_CONVERSION1, 0, 0x3ff }; > +static const struct hantro_reg reg_color_f = { G1_REG_PP_COLOR_CONVERSION1, 10, 0xff }; > +static const struct hantro_reg reg_contrast_thr1 = { G1_REG_PP_CONTRAST_ADJUST, 24, 0xff }; > +static const struct hantro_reg reg_contrast_thr2 = { G1_REG_PP_COLOR_CONVERSION, 0, 0xff }; > +static const struct hantro_reg reg_contrast_off1 = { G1_REG_PP_CONTRAST_ADJUST, 0, 0x3ff }; > +static const struct hantro_reg reg_contrast_off2 = { G1_REG_PP_CONTRAST_ADJUST, 10, 0x3ff }; > +static const struct hantro_reg reg_ycbcr_range = { G1_REG_PP_SCALING0, 29, 0x1 }; > +static const struct hantro_reg reg_pad_r = { G1_REG_PP_SCALING0, 23, 0x1f }; > +static const struct hantro_reg reg_pad_g = { G1_REG_PP_SCALING0, 18, 0x1f }; > +static const struct hantro_reg reg_pad_b = { G1_REG_PP_SCALING1, 18, 0x1f }; > + > +static s32 hantro_postproc_get_ctrl(struct hantro_ctx *ctx, u32 id, s32 def_val) > +{ > + struct v4l2_ctrl *ctrl; > + > + ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, id); > + if (!ctrl) > + return def_val; > + return v4l2_ctrl_g_ctrl(ctrl); > +} > + > +static void hantro_postproc_setup_rgb(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + s32 thr1y, thr2y; > + s32 thr1, thr2; > + s32 off1, off2; > + s32 a1, a2; > + s32 tmp, satur; > + s32 contrast, saturation, brightness; > + > + contrast = hantro_postproc_get_ctrl(ctx, V4L2_CID_CONTRAST, 0); > + brightness = hantro_postproc_get_ctrl(ctx, V4L2_CID_BRIGHTNESS, 0); > + saturation = hantro_postproc_get_ctrl(ctx, V4L2_CID_SATURATION, 0); > + > + if (ctx->src_fmt.quantization == V4L2_QUANTIZATION_LIM_RANGE) { > + s32 tmp1, tmp2; > + > + thr1 = (219 * (contrast + 128)) / 512; > + thr1y = (219 - 2 * thr1) / 2; > + thr2 = 219 - thr1; > + thr2y = 219 - thr1y; > + > + tmp1 = (thr1y * 256) / thr1; > + tmp2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); > + off1 = ((thr1y - ((tmp2 * thr1) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; > + off2 = ((thr2y - ((tmp1 * thr2) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; > + > + tmp1 = (64 * (contrast + 128)) / 128; > + tmp2 = 256 * (128 - tmp1); > + a1 = (tmp2 + off2) / thr1; > + a2 = a1 + (256 * (off2 - 1)) / (thr2 - thr1); > + } else { > + thr1 = (64 * (contrast + 128)) / 128; > + thr1y = 128 - thr1; > + thr2 = 256 - thr1; > + thr2y = 256 - thr1y; > + a1 = (thr1y * 256) / thr1; > + a2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); > + off1 = thr1y - (a2 * thr1) / 256; > + off2 = thr2y - (a1 * thr2) / 256; > + } > + > + a1 = clamp(a1, 0, 1023); > + a2 = clamp(a2, 0, 1023); > + thr1 = clamp(thr1, 0, 255); > + thr2 = clamp(thr2, 0, 255); > + off1 = clamp(off1, -512, 511); > + off2 = clamp(off2, -512, 511); > + > + hantro_reg_write(vpu, ®_contrast_thr1, thr1); > + hantro_reg_write(vpu, ®_contrast_thr2, thr2); > + hantro_reg_write(vpu, ®_contrast_off1, off1); > + hantro_reg_write(vpu, ®_contrast_off2, off2); > + hantro_reg_write(vpu, ®_color_a1, a1); > + hantro_reg_write(vpu, ®_color_a2, a2); > + > + /* Brightness */ > + hantro_reg_write(vpu, ®_color_f, brightness); > + > + /* Saturation */ > + satur = 64 + saturation; > + tmp = (satur * (s32)DEF_COLOR_COEFF_B) / 64; > + tmp = clamp(tmp, 0, 1023); > + hantro_reg_write(vpu, ®_color_b, tmp); > + > + tmp = (satur * (s32)DEF_COLOR_COEFF_C) / 64; > + tmp = clamp(tmp, 0, 1023); > + hantro_reg_write(vpu, ®_color_c, tmp); > + > + tmp = (satur * (s32)DEF_COLOR_COEFF_D) / 64; > + tmp = clamp(tmp, 0, 1023); > + hantro_reg_write(vpu, ®_color_d, tmp); > + > + tmp = (satur * (s32)DEF_COLOR_COEFF_E) / 64; > + tmp = clamp(tmp, 0, 1023); > + hantro_reg_write(vpu, ®_color_e, tmp); > + > + /* Set RGB padding and mask */ > + switch (ctx->vpu_dst_fmt->fourcc) { > + case V4L2_PIX_FMT_RGBX32: > + hantro_reg_write(vpu, ®_pad_r, 0); > + hantro_reg_write(vpu, ®_pad_g, 8); > + hantro_reg_write(vpu, ®_pad_b, 16); > + > + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_R); > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_B); > + break; > + case V4L2_PIX_FMT_XRGB32: > + hantro_reg_write(vpu, ®_pad_r, 8); > + hantro_reg_write(vpu, ®_pad_g, 16); > + hantro_reg_write(vpu, ®_pad_b, 24); > + > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_R); > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_G); > + vdpu_write_relaxed(vpu, 0x000000ff, G1_REG_PP_MASK_B); > + break; > + case V4L2_PIX_FMT_XBGR32: > + hantro_reg_write(vpu, ®_pad_b, 0); > + hantro_reg_write(vpu, ®_pad_g, 8); > + hantro_reg_write(vpu, ®_pad_r, 16); > + > + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_B); > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_R); > + break; > + } > +} > + > +void hantro_postproc_setup(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + struct vb2_v4l2_buffer *dst_buf; > + u32 reg, src_pp_fmt, dst_pp_fmt; > + dma_addr_t dst_dma; > + > + /* Turn on pipeline mode. Must be done first. */ > + vdpu_write(vpu, G1_REG_PP_PIPELINE_EN, G1_REG_PP_INTERRUPT); > + > + reg = G1_REG_PP_AXI_RD_ID(0) | > + G1_REG_PP_AXI_WR_ID(0) | > + G1_REG_PP_INSWAP32_E(1) | > + G1_REG_PP_OUTSWAP32_E(1) | > + G1_REG_PP_DATA_DISC_E(0) | > + G1_REG_PP_CLK_GATE_E(1) | > + G1_REG_PP_IN_ENDIAN(1) | > + G1_REG_PP_OUT_ENDIAN(1) | > + G1_REG_PP_MAX_BURST(16); > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_DEV_CONFIG); > + > + /* Configure picture resolution. */ > + reg = G1_REG_PP_INPUT_SIZE_HEIGHT(MB_HEIGHT(ctx->dst_fmt.height)) | > + G1_REG_PP_INPUT_SIZE_WIDTH(MB_WIDTH(ctx->dst_fmt.width)); > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_INPUT_SIZE); > + > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > + dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); > + vdpu_write_relaxed(vpu, dst_dma, G1_REG_PP_OUT_LUMA_BASE); > + > + reg = (ctx->dst_fmt.quantization == > + V4L2_QUANTIZATION_LIM_RANGE) ? 0 : 1; Why dst_fmt? Userspace cannot set the quantization range of the capture format, it's either copied from the source format or explicitly set in the driver. I see this code in vidioc_s_fmt_cap_mplane(): /* Colorimetry information are always propagated. */ ctx->src_fmt.colorspace = pix_mp->colorspace; ctx->src_fmt.ycbcr_enc = pix_mp->ycbcr_enc; ctx->src_fmt.xfer_func = pix_mp->xfer_func; ctx->src_fmt.quantization = pix_mp->quantization; I think this is wrong. If I am not mistaken, userspace has parsed this information from the bytestream and should pass it to the driver when it calls S_FMT for the output queue. Please correct me if I am wrong. But this information has to come from somewhere. These values should be propagated to the capture queue. Adding YUV to RGB conversion changes this: in that case you want to convert to full range RGB. So both ycbcr_enc and quantization can be set to the DEFAULT value for the RGB capture format. The colorspace and xfer_func values are copied as before since those are not converted by the post-processing step. Regards, Hans > + hantro_reg_write(vpu, ®_ycbcr_range, reg); > + > + /* Clear this register before setting the RGB coefficients. */ > + vdpu_write_relaxed(vpu, 0, G1_REG_PP_SCALING1); > + > + switch (ctx->vpu_dst_fmt->fourcc) { > + case V4L2_PIX_FMT_YUYV: > + dst_pp_fmt = VPU_PP_OUT_YUYV; > + break; > + case V4L2_PIX_FMT_RGBX32: > + case V4L2_PIX_FMT_XRGB32: > + case V4L2_PIX_FMT_XBGR32: > + dst_pp_fmt = VPU_PP_OUT_RGB; > + hantro_postproc_setup_rgb(ctx); > + break; > + default: > + return; > + } > + > + /* Currently NV12 is the only supported input pixel format. */ > + src_pp_fmt = VPU_PP_IN_NV12; > + > + /* Configure src/dst post-processor formats. */ > + reg = G1_REG_PP_CONTROL_IN_FMT(src_pp_fmt) | > + G1_REG_PP_CONTROL_OUT_FMT(dst_pp_fmt) | > + G1_REG_PP_CONTROL_OUT_HEIGHT(ctx->dst_fmt.height) | > + G1_REG_PP_CONTROL_OUT_WIDTH(ctx->dst_fmt.width); > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_CONTROL); > + > + reg = G1_REG_PP_ORIG_WIDTH(MB_WIDTH(ctx->dst_fmt.width)); > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_MASK1_ORIG_WIDTH); > + vdpu_write_relaxed(vpu, ctx->dst_fmt.width, G1_REG_PP_DISPLAY_WIDTH); > +} > + > +void hantro_postproc_free(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + unsigned int i; > + > + for (i = 0; i < VB2_MAX_FRAME; ++i) { > + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; > + > + if (priv->cpu) { > + dma_free_attrs(vpu->dev, priv->size, priv->cpu, > + priv->dma, priv->attrs); > + priv->cpu = NULL; > + } > + } > +} > + > +int hantro_postproc_alloc(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + unsigned int i, buf_size; > + > + buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage; > + > + for (i = 0; i < VB2_MAX_FRAME; ++i) { > + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; > + > + priv->attrs = DMA_ATTR_NO_KERNEL_MAPPING | > + DMA_ATTR_WRITE_COMBINE | > + DMA_ATTR_NON_CONSISTENT; > + priv->cpu = dma_alloc_attrs(vpu->dev, buf_size, &priv->dma, > + GFP_KERNEL, priv->attrs); > + if (!priv->cpu) > + return -ENOMEM; > + priv->size = buf_size; > + } > + return 0; > +} > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > index 59adecba0e85..37a9ba71afa6 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -242,9 +242,10 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f, > /* > * The H264 decoder needs extra space on the output buffers > * to store motion vectors. This is needed for reference > - * frames. > + * frames and only if the format is non-post-processed (NV12). > */ > - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE) > + if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && > + fmt->fourcc == V4L2_PIX_FMT_NV12) > pix_mp->plane_fmt[0].sizeimage += > hantro_h264_buffer_extra_size(pix_mp->width, > pix_mp->height); > @@ -634,10 +635,23 @@ static int hantro_start_streaming(struct vb2_queue *q, unsigned int count) > > vpu_debug(4, "Codec mode = %d\n", codec_mode); > ctx->codec_ops = &ctx->dev->variant->codec_ops[codec_mode]; > - if (ctx->codec_ops->init) > + if (ctx->codec_ops->init) { > ret = ctx->codec_ops->init(ctx); > + if (ret) > + return ret; > + } > + > + if (hantro_use_postproc(ctx)) { > + ret = hantro_postproc_alloc(ctx); > + if (ret) > + goto err_codec_exit; > + } > } > + return ret; > > +err_codec_exit: > + if (ctx->codec_ops && ctx->codec_ops->exit) > + ctx->codec_ops->exit(ctx); > return ret; > } > > @@ -664,6 +678,7 @@ static void hantro_stop_streaming(struct vb2_queue *q) > struct hantro_ctx *ctx = vb2_get_drv_priv(q); > > if (hantro_vq_is_coded(q)) { > + hantro_postproc_free(ctx); > if (ctx->codec_ops && ctx->codec_ops->exit) > ctx->codec_ops->exit(ctx); > } > diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c > index c0bdd6c02520..18d6b725309b 100644 > --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c > +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c > @@ -59,6 +59,27 @@ static const struct hantro_fmt rk3288_vpu_enc_fmts[] = { > static const struct hantro_fmt rk3288_vpu_dec_fmts[] = { > { > .fourcc = V4L2_PIX_FMT_NV12, > + .dec_fourcc = V4L2_PIX_FMT_NV12, > + .codec_mode = HANTRO_MODE_NONE, > + }, > + { > + .fourcc = V4L2_PIX_FMT_RGBX32, > + .dec_fourcc = V4L2_PIX_FMT_NV12, > + .codec_mode = HANTRO_MODE_NONE, > + }, > + { > + .fourcc = V4L2_PIX_FMT_XRGB32, > + .dec_fourcc = V4L2_PIX_FMT_NV12, > + .codec_mode = HANTRO_MODE_NONE, > + }, > + { > + .fourcc = V4L2_PIX_FMT_XBGR32, > + .dec_fourcc = V4L2_PIX_FMT_NV12, > + .codec_mode = HANTRO_MODE_NONE, > + }, > + { > + .fourcc = V4L2_PIX_FMT_YUYV, > + .dec_fourcc = V4L2_PIX_FMT_NV12, > .codec_mode = HANTRO_MODE_NONE, > }, > { > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] media: hantro: Support color conversion via post-processing 2019-09-09 11:03 ` Hans Verkuil @ 2019-09-11 8:34 ` Ezequiel Garcia 0 siblings, 0 replies; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-11 8:34 UTC (permalink / raw) To: Hans Verkuil, linux-media Cc: kernel, Tomasz Figa, linux-rockchip, Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy On Mon, 2019-09-09 at 13:03 +0200, Hans Verkuil wrote: > On 9/3/19 8:17 PM, Ezequiel Garcia wrote: > > The Hantro G1 decoder is able to enable a post-processor > > on the decoding pipeline, which can be used to perform > > scaling and color conversion. > > > > The post-processor is integrated to the decoder, and it's > > possible to use it in a way that is completely transparent > > to the user. > > > > This commit enables color conversion via post-processing, > > which means the driver now exposes YUV packed and RGB pixel > > formats, in addition to NV12. > > > > On the YUV-to-RGB path, the post-processing pipeline > > allows to modify the image contrast, brigthness and saturation, > > so additional user controls are added to expose them. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/hantro/Makefile | 1 + > > drivers/staging/media/hantro/hantro.h | 23 +- > > drivers/staging/media/hantro/hantro_drv.c | 24 +- > > .../staging/media/hantro/hantro_g1_h264_dec.c | 2 +- > > .../media/hantro/hantro_g1_mpeg2_dec.c | 3 +- > > .../staging/media/hantro/hantro_g1_vp8_dec.c | 2 +- > > drivers/staging/media/hantro/hantro_h264.c | 7 +- > > drivers/staging/media/hantro/hantro_hw.h | 11 + > > .../staging/media/hantro/hantro_postproc.c | 316 ++++++++++++++++++ > > drivers/staging/media/hantro/hantro_v4l2.c | 21 +- > > drivers/staging/media/hantro/rk3288_vpu_hw.c | 21 ++ > > 11 files changed, 420 insertions(+), 11 deletions(-) > > create mode 100644 drivers/staging/media/hantro/hantro_postproc.c > > > > diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile > > index 5d6b0383d280..496b30c3c396 100644 > > --- a/drivers/staging/media/hantro/Makefile > > +++ b/drivers/staging/media/hantro/Makefile > > @@ -3,6 +3,7 @@ obj-$(CONFIG_VIDEO_HANTRO) += hantro-vpu.o > > hantro-vpu-y += \ > > hantro_drv.o \ > > hantro_v4l2.o \ > > + hantro_postproc.o \ > > hantro_h1_jpeg_enc.o \ > > hantro_g1_h264_dec.o \ > > hantro_g1_mpeg2_dec.o \ > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > > index e8872f24e351..8446a1fa9ab9 100644 > > --- a/drivers/staging/media/hantro/hantro.h > > +++ b/drivers/staging/media/hantro/hantro.h > > @@ -237,6 +237,7 @@ struct hantro_ctx { > > unsigned int bytesused); > > > > const struct hantro_codec_ops *codec_ops; > > + struct hantro_postproc_ctx postproc; > > > > /* Specific for particular codec modes. */ > > union { > > @@ -250,7 +251,8 @@ struct hantro_ctx { > > /** > > * struct hantro_fmt - information about supported video formats. > > * @name: Human readable name of the format. > > - * @fourcc: FourCC code of the format. See V4L2_PIX_FMT_*. > > + * @fourcc: FourCC code of the post-processed format. See V4L2_PIX_FMT_*. > > + * @dec_fourcc: FourCC code of the native format. See V4L2_PIX_FMT_*. > > * @codec_mode: Codec mode related to this format. See > > * enum hantro_codec_mode. > > * @header_size: Optional header size. Currently used by JPEG encoder. > > @@ -261,6 +263,7 @@ struct hantro_ctx { > > struct hantro_fmt { > > char *name; > > u32 fourcc; > > + u32 dec_fourcc; > > enum hantro_codec_mode codec_mode; > > int header_size; > > int max_depth; > > @@ -387,4 +390,22 @@ hantro_h264_buffer_extra_size(unsigned int width, unsigned int height) > > return 128 * MB_WIDTH(width) * MB_HEIGHT(height); > > } > > > > +static inline bool > > +hantro_use_postproc(struct hantro_ctx *ctx) > > +{ > > + return ctx->vpu_dst_fmt->fourcc != ctx->vpu_dst_fmt->dec_fourcc; > > +} > > + > > +static inline dma_addr_t > > +hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb) > > +{ > > + if (hantro_use_postproc(ctx)) > > + return ctx->postproc.dec_q[vb->index].dma; > > + return vb2_dma_contig_plane_dma_addr(vb, 0); > > +} > > + > > +void hantro_postproc_setup(struct hantro_ctx *ctx); > > +void hantro_postproc_free(struct hantro_ctx *ctx); > > +int hantro_postproc_alloc(struct hantro_ctx *ctx); > > + > > #endif /* HANTRO_H_ */ > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > index f550b68d46ca..300178562014 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -53,7 +53,7 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts) > > if (index < 0) > > return 0; > > buf = vb2_get_buffer(q, index); > > - return vb2_dma_contig_plane_dma_addr(buf, 0); > > + return hantro_get_dec_buf_addr(ctx, buf); > > } > > > > static int > > @@ -165,6 +165,9 @@ void hantro_finish_run(struct hantro_ctx *ctx) > > { > > struct vb2_v4l2_buffer *src_buf; > > > > + if (hantro_use_postproc(ctx)) > > + hantro_postproc_setup(ctx); > > + > > src_buf = hantro_get_src_buf(ctx); > > v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, > > &ctx->ctrl_handler); > > @@ -365,8 +368,9 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, > > int allowed_codecs) > > { > > int i, num_ctrls = ARRAY_SIZE(controls); > > + int postproc_ctrls = (allowed_codecs & HANTRO_DECODERS) ? 3 : 0; > > > > - v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls); > > + v4l2_ctrl_handler_init(&ctx->ctrl_handler, num_ctrls + postproc_ctrls); > > > > for (i = 0; i < num_ctrls; i++) { > > if (!(allowed_codecs & controls[i].codec)) > > @@ -382,6 +386,22 @@ static int hantro_ctrls_setup(struct hantro_dev *vpu, > > return ctx->ctrl_handler.error; > > } > > } > > + > > + /* > > + * Add post-processing controls. > > + * Only used if the post-processing path is enabled, > > + * when the application sets a CAPTURE pixel format > > + * that requires it. > > + */ > > + if (postproc_ctrls) { > > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > > + V4L2_CID_CONTRAST, -64, 64, 1, 0); > > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > > + V4L2_CID_BRIGHTNESS, -128, 127, 1, 0); > > + v4l2_ctrl_new_std(&ctx->ctrl_handler, NULL, > > + V4L2_CID_SATURATION, -64, 128, 1, 0); > > + } > > + > > return v4l2_ctrl_handler_setup(&ctx->ctrl_handler); > > } > > > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > index 29130946dea4..e263a6b50651 100644 > > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > @@ -241,7 +241,7 @@ static void set_buffers(struct hantro_ctx *ctx) > > vdpu_write_relaxed(vpu, src_dma, G1_REG_ADDR_STR); > > > > /* Destination (decoded frame) buffer. */ > > - dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); > > + dst_dma = hantro_get_dec_buf_addr(ctx, &dst_buf->vb2_buf); > > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > > > /* Higher profiles require DMV buffer appended to reference frames. */ > > diff --git a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > > index f3bf67d8a289..63fe89ef21ae 100644 > > --- a/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > > +++ b/drivers/staging/media/hantro/hantro_g1_mpeg2_dec.c > > @@ -121,7 +121,7 @@ hantro_g1_mpeg2_dec_set_buffers(struct hantro_dev *vpu, struct hantro_ctx *ctx, > > vdpu_write_relaxed(vpu, addr, G1_REG_RLC_VLC_BASE); > > > > /* Destination frame buffer */ > > - addr = vb2_dma_contig_plane_dma_addr(dst_buf, 0); > > + addr = hantro_get_dec_buf_addr(ctx, dst_buf); > > current_addr = addr; > > > > if (picture->picture_structure == PICT_BOTTOM_FIELD) > > @@ -243,7 +243,6 @@ void hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx) > > hantro_g1_mpeg2_dec_set_buffers(vpu, ctx, &src_buf->vb2_buf, > > &dst_buf->vb2_buf, > > sequence, picture, slice_params); > > - > > hantro_finish_run(ctx); > > > > reg = G1_REG_DEC_E(1); > > diff --git a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > > index cad18094fee0..e708994d1aba 100644 > > --- a/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > > +++ b/drivers/staging/media/hantro/hantro_g1_vp8_dec.c > > @@ -422,7 +422,7 @@ static void cfg_buffers(struct hantro_ctx *ctx, > > } > > vdpu_write_relaxed(vpu, reg, G1_REG_FWD_PIC(0)); > > > > - dst_dma = vb2_dma_contig_plane_dma_addr(&vb2_dst->vb2_buf, 0); > > + dst_dma = hantro_get_dec_buf_addr(ctx, &vb2_dst->vb2_buf); > > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > } > > > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c > > index 2227b4e12067..d00b3096b7ae 100644 > > --- a/drivers/staging/media/hantro/hantro_h264.c > > +++ b/drivers/staging/media/hantro/hantro_h264.c > > @@ -13,6 +13,7 @@ > > #include <linux/types.h> > > #include <linux/sort.h> > > #include <media/v4l2-mem2mem.h> > > +#include <media/v4l2-common.h> > > > > #include "hantro.h" > > #include "hantro_hw.h" > > @@ -635,7 +636,11 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx) > > tbl = priv->cpu; > > memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table)); > > > > - v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat, > > + /* > > + * For the decoder picture size, we want the decoder > > + * native pixel format, so we use dec_fourcc here. > > + */ > > + v4l2_fill_pixfmt_mp(&pix_mp, ctx->vpu_dst_fmt->dec_fourcc, > > ctx->dst_fmt.width, ctx->dst_fmt.height); > > h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage; > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > > index 69b88f4d3fb3..ae1d869e7c28 100644 > > --- a/drivers/staging/media/hantro/hantro_hw.h > > +++ b/drivers/staging/media/hantro/hantro_hw.h > > @@ -28,11 +28,13 @@ struct hantro_variant; > > * @cpu: CPU pointer to the buffer. > > * @dma: DMA address of the buffer. > > * @size: Size of the buffer. > > + * @attrs: Attributes of the DMA mapping. > > */ > > struct hantro_aux_buf { > > void *cpu; > > dma_addr_t dma; > > size_t size; > > + unsigned long attrs; > > }; > > > > /** > > @@ -109,6 +111,15 @@ struct hantro_vp8_dec_hw_ctx { > > struct hantro_aux_buf prob_tbl; > > }; > > > > +/** > > + * struct hantro_postproc_ctx > > + * > > + * @dec_q: References buffers, in decoder format. > > + */ > > +struct hantro_postproc_ctx { > > + struct hantro_aux_buf dec_q[VB2_MAX_FRAME]; > > +}; > > + > > /** > > * struct hantro_codec_ops - codec mode specific operations > > * > > diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c > > new file mode 100644 > > index 000000000000..17d023a253a8 > > --- /dev/null > > +++ b/drivers/staging/media/hantro/hantro_postproc.c > > @@ -0,0 +1,316 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Hantro G1 post-processor support > > + * > > + * Copyright (C) 2019 Collabora, Ltd. > > + */ > > + > > +#include <linux/dma-mapping.h> > > +#include <linux/types.h> > > + > > +#include "hantro.h" > > +#include "hantro_hw.h" > > + > > +#define G1_SWREG(nr) ((nr) * 4) > > + > > +#define G1_REG_PP_INTERRUPT G1_SWREG(60) > > +#define G1_REG_PP_READY_IRQ BIT(12) > > +#define G1_REG_PP_IRQ BIT(8) > > +#define G1_REG_PP_IRQ_DIS BIT(4) > > +#define G1_REG_PP_PIPELINE_EN BIT(1) > > +#define G1_REG_PP_EXTERNAL_TRIGGER BIT(0) > > +#define G1_REG_PP_DEV_CONFIG G1_SWREG(61) > > +#define G1_REG_PP_AXI_RD_ID(v) (((v) << 24) & GENMASK(31, 24)) > > +#define G1_REG_PP_AXI_WR_ID(v) (((v) << 16) & GENMASK(23, 16)) > > +#define G1_REG_PP_INSWAP32_E(v) ((v) ? BIT(10) : 0) > > +#define G1_REG_PP_DATA_DISC_E(v) ((v) ? BIT(9) : 0) > > +#define G1_REG_PP_CLK_GATE_E(v) ((v) ? BIT(8) : 0) > > +#define G1_REG_PP_IN_ENDIAN(v) ((v) ? BIT(7) : 0) > > +#define G1_REG_PP_OUT_ENDIAN(v) ((v) ? BIT(6) : 0) > > +#define G1_REG_PP_OUTSWAP32_E(v) ((v) ? BIT(5) : 0) > > +#define G1_REG_PP_MAX_BURST(v) (((v) << 0) & GENMASK(4, 0)) > > +#define G1_REG_PP_IN_LUMA_BASE G1_SWREG(63) > > +#define G1_REG_PP_IN_CB_BASE G1_SWREG(64) > > +#define G1_REG_PP_IN_CR_BASE G1_SWREG(65) > > +#define G1_REG_PP_OUT_LUMA_BASE G1_SWREG(66) > > +#define G1_REG_PP_OUT_CHROMA_BASE G1_SWREG(67) > > +#define G1_REG_PP_CONTRAST_ADJUST G1_SWREG(68) > > +#define G1_REG_PP_COLOR_CONVERSION G1_SWREG(69) > > +#define G1_REG_PP_COLOR_CONVERSION0 G1_SWREG(70) > > +#define G1_REG_PP_COLOR_CONVERSION1 G1_SWREG(71) > > +#define G1_REG_PP_INPUT_SIZE G1_SWREG(72) > > +#define G1_REG_PP_INPUT_SIZE_HEIGHT(v) (((v) << 9) & GENMASK(16, 9)) > > +#define G1_REG_PP_INPUT_SIZE_WIDTH(v) (((v) << 0) & GENMASK(8, 0)) > > +#define G1_REG_PP_SCALING0 G1_SWREG(79) > > +#define G1_REG_PP_PADD_R(v) (((v) << 23) & GENMASK(27, 23)) > > +#define G1_REG_PP_PADD_G(v) (((v) << 18) & GENMASK(22, 18)) > > +#define G1_REG_PP_RANGEMAP_Y(v) ((v) ? BIT(31) : 0) > > +#define G1_REG_PP_RANGEMAP_C(v) ((v) ? BIT(30) : 0) > > +#define G1_REG_PP_YCBCR_RANGE(v) ((v) ? BIT(29) : 0) > > +#define G1_REG_PP_RGB_16(v) ((v) ? BIT(28) : 0) > > +#define G1_REG_PP_SCALING1 G1_SWREG(80) > > +#define G1_REG_PP_PADD_B(v) (((v) << 18) & GENMASK(22, 18)) > > +#define G1_REG_PP_MASK_R G1_SWREG(82) > > +#define G1_REG_PP_MASK_G G1_SWREG(83) > > +#define G1_REG_PP_MASK_B G1_SWREG(84) > > +#define G1_REG_PP_CONTROL G1_SWREG(85) > > +#define G1_REG_PP_CONTROL_IN_FMT(v) (((v) << 29) & GENMASK(31, 29)) > > +#define G1_REG_PP_CONTROL_OUT_FMT(v) (((v) << 26) & GENMASK(28, 26)) > > +#define G1_REG_PP_CONTROL_OUT_HEIGHT(v) (((v) << 15) & GENMASK(25, 15)) > > +#define G1_REG_PP_CONTROL_OUT_WIDTH(v) (((v) << 4) & GENMASK(14, 4)) > > +#define G1_REG_PP_MASK1_ORIG_WIDTH G1_SWREG(88) > > +#define G1_REG_PP_ORIG_WIDTH(v) (((v) << 23) & GENMASK(31, 23)) > > +#define G1_REG_PP_DISPLAY_WIDTH G1_SWREG(92) > > +#define G1_REG_PP_FUSE G1_SWREG(99) > > + > > +#define VPU_PP_IN_YUYV 0x0 > > +#define VPU_PP_IN_NV12 0x1 > > +#define VPU_PP_IN_YUV420 0x2 > > +#define VPU_PP_IN_YUV240_TILED 0x5 > > +#define VPU_PP_OUT_RGB 0x0 > > +#define VPU_PP_OUT_YUYV 0x3 > > + > > +#define DEF_COLOR_COEFF_A 298 > > +#define DEF_COLOR_COEFF_B 409 > > +#define DEF_COLOR_COEFF_C 208 > > +#define DEF_COLOR_COEFF_D 100 > > +#define DEF_COLOR_COEFF_E 516 > > + > > +static const struct hantro_reg reg_color_a2 = { G1_REG_PP_COLOR_CONVERSION, 18, 0x3ff }; > > +static const struct hantro_reg reg_color_a1 = { G1_REG_PP_COLOR_CONVERSION, 8, 0x3ff }; > > +static const struct hantro_reg reg_color_b = { G1_REG_PP_COLOR_CONVERSION0, 0, 0x3ff }; > > +static const struct hantro_reg reg_color_c = { G1_REG_PP_COLOR_CONVERSION0, 10, 0x3ff }; > > +static const struct hantro_reg reg_color_d = { G1_REG_PP_COLOR_CONVERSION0, 20, 0x3ff }; > > +static const struct hantro_reg reg_color_e = { G1_REG_PP_COLOR_CONVERSION1, 0, 0x3ff }; > > +static const struct hantro_reg reg_color_f = { G1_REG_PP_COLOR_CONVERSION1, 10, 0xff }; > > +static const struct hantro_reg reg_contrast_thr1 = { G1_REG_PP_CONTRAST_ADJUST, 24, 0xff }; > > +static const struct hantro_reg reg_contrast_thr2 = { G1_REG_PP_COLOR_CONVERSION, 0, 0xff }; > > +static const struct hantro_reg reg_contrast_off1 = { G1_REG_PP_CONTRAST_ADJUST, 0, 0x3ff }; > > +static const struct hantro_reg reg_contrast_off2 = { G1_REG_PP_CONTRAST_ADJUST, 10, 0x3ff }; > > +static const struct hantro_reg reg_ycbcr_range = { G1_REG_PP_SCALING0, 29, 0x1 }; > > +static const struct hantro_reg reg_pad_r = { G1_REG_PP_SCALING0, 23, 0x1f }; > > +static const struct hantro_reg reg_pad_g = { G1_REG_PP_SCALING0, 18, 0x1f }; > > +static const struct hantro_reg reg_pad_b = { G1_REG_PP_SCALING1, 18, 0x1f }; > > + > > +static s32 hantro_postproc_get_ctrl(struct hantro_ctx *ctx, u32 id, s32 def_val) > > +{ > > + struct v4l2_ctrl *ctrl; > > + > > + ctrl = v4l2_ctrl_find(&ctx->ctrl_handler, id); > > + if (!ctrl) > > + return def_val; > > + return v4l2_ctrl_g_ctrl(ctrl); > > +} > > + > > +static void hantro_postproc_setup_rgb(struct hantro_ctx *ctx) > > +{ > > + struct hantro_dev *vpu = ctx->dev; > > + s32 thr1y, thr2y; > > + s32 thr1, thr2; > > + s32 off1, off2; > > + s32 a1, a2; > > + s32 tmp, satur; > > + s32 contrast, saturation, brightness; > > + > > + contrast = hantro_postproc_get_ctrl(ctx, V4L2_CID_CONTRAST, 0); > > + brightness = hantro_postproc_get_ctrl(ctx, V4L2_CID_BRIGHTNESS, 0); > > + saturation = hantro_postproc_get_ctrl(ctx, V4L2_CID_SATURATION, 0); > > + > > + if (ctx->src_fmt.quantization == V4L2_QUANTIZATION_LIM_RANGE) { > > + s32 tmp1, tmp2; > > + > > + thr1 = (219 * (contrast + 128)) / 512; > > + thr1y = (219 - 2 * thr1) / 2; > > + thr2 = 219 - thr1; > > + thr2y = 219 - thr1y; > > + > > + tmp1 = (thr1y * 256) / thr1; > > + tmp2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); > > + off1 = ((thr1y - ((tmp2 * thr1) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; > > + off2 = ((thr2y - ((tmp1 * thr2) / 256)) * (s32)DEF_COLOR_COEFF_A) / 256; > > + > > + tmp1 = (64 * (contrast + 128)) / 128; > > + tmp2 = 256 * (128 - tmp1); > > + a1 = (tmp2 + off2) / thr1; > > + a2 = a1 + (256 * (off2 - 1)) / (thr2 - thr1); > > + } else { > > + thr1 = (64 * (contrast + 128)) / 128; > > + thr1y = 128 - thr1; > > + thr2 = 256 - thr1; > > + thr2y = 256 - thr1y; > > + a1 = (thr1y * 256) / thr1; > > + a2 = ((thr2y - thr1y) * 256) / (thr2 - thr1); > > + off1 = thr1y - (a2 * thr1) / 256; > > + off2 = thr2y - (a1 * thr2) / 256; > > + } > > + > > + a1 = clamp(a1, 0, 1023); > > + a2 = clamp(a2, 0, 1023); > > + thr1 = clamp(thr1, 0, 255); > > + thr2 = clamp(thr2, 0, 255); > > + off1 = clamp(off1, -512, 511); > > + off2 = clamp(off2, -512, 511); > > + > > + hantro_reg_write(vpu, ®_contrast_thr1, thr1); > > + hantro_reg_write(vpu, ®_contrast_thr2, thr2); > > + hantro_reg_write(vpu, ®_contrast_off1, off1); > > + hantro_reg_write(vpu, ®_contrast_off2, off2); > > + hantro_reg_write(vpu, ®_color_a1, a1); > > + hantro_reg_write(vpu, ®_color_a2, a2); > > + > > + /* Brightness */ > > + hantro_reg_write(vpu, ®_color_f, brightness); > > + > > + /* Saturation */ > > + satur = 64 + saturation; > > + tmp = (satur * (s32)DEF_COLOR_COEFF_B) / 64; > > + tmp = clamp(tmp, 0, 1023); > > + hantro_reg_write(vpu, ®_color_b, tmp); > > + > > + tmp = (satur * (s32)DEF_COLOR_COEFF_C) / 64; > > + tmp = clamp(tmp, 0, 1023); > > + hantro_reg_write(vpu, ®_color_c, tmp); > > + > > + tmp = (satur * (s32)DEF_COLOR_COEFF_D) / 64; > > + tmp = clamp(tmp, 0, 1023); > > + hantro_reg_write(vpu, ®_color_d, tmp); > > + > > + tmp = (satur * (s32)DEF_COLOR_COEFF_E) / 64; > > + tmp = clamp(tmp, 0, 1023); > > + hantro_reg_write(vpu, ®_color_e, tmp); > > + > > + /* Set RGB padding and mask */ > > + switch (ctx->vpu_dst_fmt->fourcc) { > > + case V4L2_PIX_FMT_RGBX32: > > + hantro_reg_write(vpu, ®_pad_r, 0); > > + hantro_reg_write(vpu, ®_pad_g, 8); > > + hantro_reg_write(vpu, ®_pad_b, 16); > > + > > + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_R); > > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); > > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_B); > > + break; > > + case V4L2_PIX_FMT_XRGB32: > > + hantro_reg_write(vpu, ®_pad_r, 8); > > + hantro_reg_write(vpu, ®_pad_g, 16); > > + hantro_reg_write(vpu, ®_pad_b, 24); > > + > > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_R); > > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_G); > > + vdpu_write_relaxed(vpu, 0x000000ff, G1_REG_PP_MASK_B); > > + break; > > + case V4L2_PIX_FMT_XBGR32: > > + hantro_reg_write(vpu, ®_pad_b, 0); > > + hantro_reg_write(vpu, ®_pad_g, 8); > > + hantro_reg_write(vpu, ®_pad_r, 16); > > + > > + vdpu_write_relaxed(vpu, 0xff000000, G1_REG_PP_MASK_B); > > + vdpu_write_relaxed(vpu, 0x00ff0000, G1_REG_PP_MASK_G); > > + vdpu_write_relaxed(vpu, 0x0000ff00, G1_REG_PP_MASK_R); > > + break; > > + } > > +} > > + > > +void hantro_postproc_setup(struct hantro_ctx *ctx) > > +{ > > + struct hantro_dev *vpu = ctx->dev; > > + struct vb2_v4l2_buffer *dst_buf; > > + u32 reg, src_pp_fmt, dst_pp_fmt; > > + dma_addr_t dst_dma; > > + > > + /* Turn on pipeline mode. Must be done first. */ > > + vdpu_write(vpu, G1_REG_PP_PIPELINE_EN, G1_REG_PP_INTERRUPT); > > + > > + reg = G1_REG_PP_AXI_RD_ID(0) | > > + G1_REG_PP_AXI_WR_ID(0) | > > + G1_REG_PP_INSWAP32_E(1) | > > + G1_REG_PP_OUTSWAP32_E(1) | > > + G1_REG_PP_DATA_DISC_E(0) | > > + G1_REG_PP_CLK_GATE_E(1) | > > + G1_REG_PP_IN_ENDIAN(1) | > > + G1_REG_PP_OUT_ENDIAN(1) | > > + G1_REG_PP_MAX_BURST(16); > > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_DEV_CONFIG); > > + > > + /* Configure picture resolution. */ > > + reg = G1_REG_PP_INPUT_SIZE_HEIGHT(MB_HEIGHT(ctx->dst_fmt.height)) | > > + G1_REG_PP_INPUT_SIZE_WIDTH(MB_WIDTH(ctx->dst_fmt.width)); > > + vdpu_write_relaxed(vpu, reg, G1_REG_PP_INPUT_SIZE); > > + > > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0); > > + vdpu_write_relaxed(vpu, dst_dma, G1_REG_PP_OUT_LUMA_BASE); > > + > > + reg = (ctx->dst_fmt.quantization == > > + V4L2_QUANTIZATION_LIM_RANGE) ? 0 : 1; > > Why dst_fmt? Userspace cannot set the quantization range of the capture > format, it's either copied from the source format or explicitly set in the > driver. > > I see this code in vidioc_s_fmt_cap_mplane(): > > /* Colorimetry information are always propagated. */ > ctx->src_fmt.colorspace = pix_mp->colorspace; > ctx->src_fmt.ycbcr_enc = pix_mp->ycbcr_enc; > ctx->src_fmt.xfer_func = pix_mp->xfer_func; > ctx->src_fmt.quantization = pix_mp->quantization; > > I think this is wrong. > > If I am not mistaken, userspace has parsed this information from the bytestream > and should pass it to the driver when it calls S_FMT for the output queue. > Please correct me if I am wrong. But this information has to come from somewhere. > > These values should be propagated to the capture queue. > Yes, I think you are right here. > Adding YUV to RGB conversion changes this: in that case you want to convert to > full range RGB. So both ycbcr_enc and quantization can be set to the DEFAULT > value for the RGB capture format. The colorspace and xfer_func values are copied > as before since those are not converted by the post-processing step. > Right, that makes sense. Thanks for the feedback, Ezequiel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia ` (3 preceding siblings ...) 2019-09-03 18:17 ` [PATCH 4/4] media: hantro: Support color conversion via post-processing Ezequiel Garcia @ 2019-09-09 7:07 ` Tomasz Figa 2019-09-11 8:27 ` Ezequiel Garcia 4 siblings, 1 reply; 18+ messages in thread From: Tomasz Figa @ 2019-09-09 7:07 UTC (permalink / raw) To: Ezequiel Garcia Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy Hi Ezequiel, On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Hi all, > > This series enables the post-processor support available > on the Hantro G1 VPU. The post-processor block can be > pipelined with the decoder hardware, allowing to perform > operations such as color conversion, scaling, rotation, > cropping, among others. > > The decoder hardware needs its own set of NV12 buffers > (the native decoder format), and the post-processor is the > owner of the CAPTURE buffers. This allows the application > get processed (scaled, converted, etc) buffers, completely > transparently. > > This feature is implemented by exposing other CAPTURE pixel > formats to the application (ENUM_FMT). When the application > sets a pixel format other than NV12, the driver will enable > and use the post-processor transparently. I'll try to review the series a bit later, but a general comment here is that the userspace wouldn't have a way to distinguish between the native and post-processed formats. I'm pretty much sure that post-processing at least imposes some power penalty, so it would be good if the userspace could avoid it if unnecessary. Best regards, Tomasz ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-09 7:07 ` [PATCH 0/4] Enable Hantro G1 post-processor Tomasz Figa @ 2019-09-11 8:27 ` Ezequiel Garcia 2019-09-11 19:48 ` Nicolas Dufresne 2019-09-11 19:49 ` Nicolas Dufresne 0 siblings, 2 replies; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-11 8:27 UTC (permalink / raw) To: Tomasz Figa Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: > Hi Ezequiel, > > On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Hi all, > > > > This series enables the post-processor support available > > on the Hantro G1 VPU. The post-processor block can be > > pipelined with the decoder hardware, allowing to perform > > operations such as color conversion, scaling, rotation, > > cropping, among others. > > > > The decoder hardware needs its own set of NV12 buffers > > (the native decoder format), and the post-processor is the > > owner of the CAPTURE buffers. This allows the application > > get processed (scaled, converted, etc) buffers, completely > > transparently. > > > > This feature is implemented by exposing other CAPTURE pixel > > formats to the application (ENUM_FMT). When the application > > sets a pixel format other than NV12, the driver will enable > > and use the post-processor transparently. > > I'll try to review the series a bit later, but a general comment here > is that the userspace wouldn't have a way to distinguish between the > native and post-processed formats. I'm pretty much sure that > post-processing at least imposes some power penalty, so it would be > good if the userspace could avoid it if unnecessary. > Hm, that's true, good catch. So, it would be desirable to retain the current behavior of allowing the application to just set a different pixel format and get a post-processed frame, transparently. But at the same time, it would be nice if the application is somehow aware of the post-processing happening. Maybe we can expose a more accurate media controller topology, have applications enable the post-processing pipeline explicitly. Thanks for the feedback, Ezequiel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-11 8:27 ` Ezequiel Garcia @ 2019-09-11 19:48 ` Nicolas Dufresne 2019-09-11 19:49 ` Nicolas Dufresne 1 sibling, 0 replies; 18+ messages in thread From: Nicolas Dufresne @ 2019-09-11 19:48 UTC (permalink / raw) To: Ezequiel Garcia, Tomasz Figa Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy [-- Attachment #1: Type: text/plain, Size: 2330 bytes --] Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit : > On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: > > Hi Ezequiel, > > > > On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > Hi all, > > > > > > This series enables the post-processor support available > > > on the Hantro G1 VPU. The post-processor block can be > > > pipelined with the decoder hardware, allowing to perform > > > operations such as color conversion, scaling, rotation, > > > cropping, among others. > > > > > > The decoder hardware needs its own set of NV12 buffers > > > (the native decoder format), and the post-processor is the > > > owner of the CAPTURE buffers. This allows the application > > > get processed (scaled, converted, etc) buffers, completely > > > transparently. > > > > > > This feature is implemented by exposing other CAPTURE pixel > > > formats to the application (ENUM_FMT). When the application > > > sets a pixel format other than NV12, the driver will enable > > > and use the post-processor transparently. > > > > I'll try to review the series a bit later, but a general comment here > > is that the userspace wouldn't have a way to distinguish between the > > native and post-processed formats. I'm pretty much sure that > > post-processing at least imposes some power penalty, so it would be > > good if the userspace could avoid it if unnecessary. > > > > Hm, that's true, good catch. > > So, it would be desirable to retain the current behavior of allowing > the application to just set a different pixel format and get > a post-processed frame, transparently. > > But at the same time, it would be nice if the application is somehow > aware of the post-processing happening. Maybe we can expose a more > accurate media controller topology, have applications enable > the post-processing pipeline explicitly. How it works on the stateful side is that userspace set the encoding type (the codec), then passes a header (in our case, there will be parsed structures replacing this) first. The driver then configure capture format, giving a hint of the "default" or "native" format. This may or may not be sufficient, but it does work in giving userspace a hint. > > Thanks for the feedback, > Ezequiel > [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-11 8:27 ` Ezequiel Garcia 2019-09-11 19:48 ` Nicolas Dufresne @ 2019-09-11 19:49 ` Nicolas Dufresne 2019-09-12 5:52 ` Tomasz Figa 1 sibling, 1 reply; 18+ messages in thread From: Nicolas Dufresne @ 2019-09-11 19:49 UTC (permalink / raw) To: Ezequiel Garcia, Tomasz Figa Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy [-- Attachment #1: Type: text/plain, Size: 2330 bytes --] Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit : > On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: > > Hi Ezequiel, > > > > On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > Hi all, > > > > > > This series enables the post-processor support available > > > on the Hantro G1 VPU. The post-processor block can be > > > pipelined with the decoder hardware, allowing to perform > > > operations such as color conversion, scaling, rotation, > > > cropping, among others. > > > > > > The decoder hardware needs its own set of NV12 buffers > > > (the native decoder format), and the post-processor is the > > > owner of the CAPTURE buffers. This allows the application > > > get processed (scaled, converted, etc) buffers, completely > > > transparently. > > > > > > This feature is implemented by exposing other CAPTURE pixel > > > formats to the application (ENUM_FMT). When the application > > > sets a pixel format other than NV12, the driver will enable > > > and use the post-processor transparently. > > > > I'll try to review the series a bit later, but a general comment here > > is that the userspace wouldn't have a way to distinguish between the > > native and post-processed formats. I'm pretty much sure that > > post-processing at least imposes some power penalty, so it would be > > good if the userspace could avoid it if unnecessary. > > > > Hm, that's true, good catch. > > So, it would be desirable to retain the current behavior of allowing > the application to just set a different pixel format and get > a post-processed frame, transparently. > > But at the same time, it would be nice if the application is somehow > aware of the post-processing happening. Maybe we can expose a more > accurate media controller topology, have applications enable > the post-processing pipeline explicitly. How it works on the stateful side is that userspace set the encoding type (the codec), then passes a header (in our case, there will be parsed structures replacing this) first. The driver then configure capture format, giving a hint of the "default" or "native" format. This may or may not be sufficient, but it does work in giving userspace a hint. > > Thanks for the feedback, > Ezequiel > [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-11 19:49 ` Nicolas Dufresne @ 2019-09-12 5:52 ` Tomasz Figa 2019-09-12 11:35 ` Ezequiel Garcia 0 siblings, 1 reply; 18+ messages in thread From: Tomasz Figa @ 2019-09-12 5:52 UTC (permalink / raw) To: Nicolas Dufresne Cc: Ezequiel Garcia, Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit : > > On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: > > > Hi Ezequiel, > > > > > > On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > Hi all, > > > > > > > > This series enables the post-processor support available > > > > on the Hantro G1 VPU. The post-processor block can be > > > > pipelined with the decoder hardware, allowing to perform > > > > operations such as color conversion, scaling, rotation, > > > > cropping, among others. > > > > > > > > The decoder hardware needs its own set of NV12 buffers > > > > (the native decoder format), and the post-processor is the > > > > owner of the CAPTURE buffers. This allows the application > > > > get processed (scaled, converted, etc) buffers, completely > > > > transparently. > > > > > > > > This feature is implemented by exposing other CAPTURE pixel > > > > formats to the application (ENUM_FMT). When the application > > > > sets a pixel format other than NV12, the driver will enable > > > > and use the post-processor transparently. > > > > > > I'll try to review the series a bit later, but a general comment here > > > is that the userspace wouldn't have a way to distinguish between the > > > native and post-processed formats. I'm pretty much sure that > > > post-processing at least imposes some power penalty, so it would be > > > good if the userspace could avoid it if unnecessary. > > > > > > > Hm, that's true, good catch. > > > > So, it would be desirable to retain the current behavior of allowing > > the application to just set a different pixel format and get > > a post-processed frame, transparently. > > > > But at the same time, it would be nice if the application is somehow > > aware of the post-processing happening. Maybe we can expose a more > > accurate media controller topology, have applications enable > > the post-processing pipeline explicitly. > > How it works on the stateful side is that userspace set the encoding > type (the codec), then passes a header (in our case, there will be > parsed structures replacing this) first. The driver then configure > capture format, giving a hint of the "default" or "native" format. This > may or may not be sufficient, but it does work in giving userspace a > hint. The bad side of that is that we can't handle more than 1 native format. For the most backwards-compatible behavior, sorting the results of ENUM_FMT according to format preference would allow the applications that choose the first format returned that works for them to choose the best one. For a further improvement, an ENUM_FMT flag that tells the userspace that a format is preferred could work. That said, modelling the pipeline appropriately using the media controller is the idea I like the most, because it's the most comprehensive solution. That would have to be well specified and documented, though, and sounds like a long term effort. Best regards, Tomasz ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-12 5:52 ` Tomasz Figa @ 2019-09-12 11:35 ` Ezequiel Garcia 2019-09-16 18:18 ` Helen Koike 0 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2019-09-12 11:35 UTC (permalink / raw) To: Tomasz Figa, Nicolas Dufresne Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy On Thu, 2019-09-12 at 14:52 +0900, Tomasz Figa wrote: > On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit : > > > On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: > > > > Hi Ezequiel, > > > > > > > > On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > > Hi all, > > > > > > > > > > This series enables the post-processor support available > > > > > on the Hantro G1 VPU. The post-processor block can be > > > > > pipelined with the decoder hardware, allowing to perform > > > > > operations such as color conversion, scaling, rotation, > > > > > cropping, among others. > > > > > > > > > > The decoder hardware needs its own set of NV12 buffers > > > > > (the native decoder format), and the post-processor is the > > > > > owner of the CAPTURE buffers. This allows the application > > > > > get processed (scaled, converted, etc) buffers, completely > > > > > transparently. > > > > > > > > > > This feature is implemented by exposing other CAPTURE pixel > > > > > formats to the application (ENUM_FMT). When the application > > > > > sets a pixel format other than NV12, the driver will enable > > > > > and use the post-processor transparently. > > > > > > > > I'll try to review the series a bit later, but a general comment here > > > > is that the userspace wouldn't have a way to distinguish between the > > > > native and post-processed formats. I'm pretty much sure that > > > > post-processing at least imposes some power penalty, so it would be > > > > good if the userspace could avoid it if unnecessary. > > > > > > > > > > Hm, that's true, good catch. > > > > > > So, it would be desirable to retain the current behavior of allowing > > > the application to just set a different pixel format and get > > > a post-processed frame, transparently. > > > > > > But at the same time, it would be nice if the application is somehow > > > aware of the post-processing happening. Maybe we can expose a more > > > accurate media controller topology, have applications enable > > > the post-processing pipeline explicitly. > > > > How it works on the stateful side is that userspace set the encoding > > type (the codec), then passes a header (in our case, there will be > > parsed structures replacing this) first. The driver then configure > > capture format, giving a hint of the "default" or "native" format. This > > may or may not be sufficient, but it does work in giving userspace a > > hint. > > The bad side of that is that we can't handle more than 1 native format. > > For the most backwards-compatible behavior, sorting the results of > ENUM_FMT according to format preference would allow the applications > that choose the first format returned that works for them to choose > the best one. > > For a further improvement, an ENUM_FMT flag that tells the userspace > that a format is preferred could work. > > That said, modelling the pipeline appropriately using the media > controller is the idea I like the most, because it's the most > comprehensive solution. That would have to be well specified and > documented, though, and sounds like a long term effort. > Completely agreed. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Enable Hantro G1 post-processor 2019-09-12 11:35 ` Ezequiel Garcia @ 2019-09-16 18:18 ` Helen Koike 0 siblings, 0 replies; 18+ messages in thread From: Helen Koike @ 2019-09-16 18:18 UTC (permalink / raw) To: Ezequiel Garcia, Tomasz Figa, Nicolas Dufresne Cc: Linux Media Mailing List, kernel, open list:ARM/Rockchip SoC..., Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon, Chris Healy Hello, On 9/12/19 8:35 AM, Ezequiel Garcia wrote: > On Thu, 2019-09-12 at 14:52 +0900, Tomasz Figa wrote: >> On Thu, Sep 12, 2019 at 4:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: >>> Le mercredi 11 septembre 2019 à 09:27 +0100, Ezequiel Garcia a écrit : >>>> On Mon, 2019-09-09 at 16:07 +0900, Tomasz Figa wrote: >>>>> Hi Ezequiel, >>>>> >>>>> On Wed, Sep 4, 2019 at 3:17 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: >>>>>> Hi all, >>>>>> >>>>>> This series enables the post-processor support available >>>>>> on the Hantro G1 VPU. The post-processor block can be >>>>>> pipelined with the decoder hardware, allowing to perform >>>>>> operations such as color conversion, scaling, rotation, >>>>>> cropping, among others. >>>>>> >>>>>> The decoder hardware needs its own set of NV12 buffers >>>>>> (the native decoder format), and the post-processor is the >>>>>> owner of the CAPTURE buffers. This allows the application >>>>>> get processed (scaled, converted, etc) buffers, completely >>>>>> transparently. >>>>>> >>>>>> This feature is implemented by exposing other CAPTURE pixel >>>>>> formats to the application (ENUM_FMT). When the application >>>>>> sets a pixel format other than NV12, the driver will enable >>>>>> and use the post-processor transparently. >>>>> >>>>> I'll try to review the series a bit later, but a general comment here >>>>> is that the userspace wouldn't have a way to distinguish between the >>>>> native and post-processed formats. I'm pretty much sure that >>>>> post-processing at least imposes some power penalty, so it would be >>>>> good if the userspace could avoid it if unnecessary. >>>>> >>>> >>>> Hm, that's true, good catch. >>>> >>>> So, it would be desirable to retain the current behavior of allowing >>>> the application to just set a different pixel format and get >>>> a post-processed frame, transparently. >>>> >>>> But at the same time, it would be nice if the application is somehow >>>> aware of the post-processing happening. Maybe we can expose a more >>>> accurate media controller topology, have applications enable >>>> the post-processing pipeline explicitly. >>> >>> How it works on the stateful side is that userspace set the encoding >>> type (the codec), then passes a header (in our case, there will be >>> parsed structures replacing this) first. The driver then configure >>> capture format, giving a hint of the "default" or "native" format. This >>> may or may not be sufficient, but it does work in giving userspace a >>> hint. >> >> The bad side of that is that we can't handle more than 1 native format. >> >> For the most backwards-compatible behavior, sorting the results of >> ENUM_FMT according to format preference would allow the applications >> that choose the first format returned that works for them to choose >> the best one. >> >> For a further improvement, an ENUM_FMT flag that tells the userspace >> that a format is preferred could work. >> >> That said, modelling the pipeline appropriately using the media >> controller is the idea I like the most, because it's the most >> comprehensive solution. That would have to be well specified and >> documented, though, and sounds like a long term effort. >> I was playing with vimc to emulate this topology. What I would suggest is a topology like this: Device topology - entity 1: Decoder (2 pads, 3 links) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "M2M Video Node":1 [ENABLED] pad1: Source -> "M2M Video Node":0 [ENABLED] -> "PostProc":0 [] - entity 4: PostProc (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "Decoder":1 [] pad1: Source -> "M2M Video Node":0 [] - entity 7: M2M Video Node (2 pads, 3 links) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "Decoder":1 [ENABLED] <- "PostProc":1 [] pad1: Source -> "Decoder":0 [ENABLED] Dot file: --------- digraph board { rankdir=TB n00000001 [label="{{<port0> 0} | Decoder\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green] n00000001:port1 -> n00000007 n00000001:port1 -> n00000004:port0 [style=dashed] n00000004 [label="{{<port0> 0} | PostProc\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green] n00000004:port1 -> n00000007 [style=dashed] n00000007 [label="M2M Video Node\n/dev/video0", shape=box, style=filled, fillcolor=yellow] n00000007 -> n00000001:port0 } Also, as you mentioned in patch 4: > On the YUV-to-RGB path, the post-processing pipeline > allows to modify the image contrast, brigthness and saturation, > so additional user controls are added to expose them. So I think it would make sense to expose these controls in the post-processor subdev, through a /dev/v4l-subdev0, this avoids having a dynamic set of controls in the video node depending on the path. You don't even need to implement VIDIOC_{G,S}_FORMAT in the pads (at least v4l2-compliance doesn't complain, it is happy with "Not Supported"). So the post-processor /dev/v4l-subdev0 devnode could be used to expose only these controls, and you don't need to expose a devnode to the Decoder entity above (unless you have another reason). I hope this helps, Helen ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-09-16 18:19 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-03 18:17 [PATCH 0/4] Enable Hantro G1 post-processor Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 1/4] media: hantro: Simplify macroblock macros Ezequiel Garcia 2019-09-04 10:50 ` Philipp Zabel 2019-09-03 18:17 ` [PATCH 2/4] media: hantro: Simplify buffer helpers Ezequiel Garcia 2019-09-04 10:50 ` Philipp Zabel 2019-09-03 18:17 ` [PATCH 3/4] media: hantro: Add helper for the H264 motion vectors allocation Ezequiel Garcia 2019-09-04 10:17 ` Philipp Zabel 2019-09-04 12:50 ` Ezequiel Garcia 2019-09-03 18:17 ` [PATCH 4/4] media: hantro: Support color conversion via post-processing Ezequiel Garcia 2019-09-09 11:03 ` Hans Verkuil 2019-09-11 8:34 ` Ezequiel Garcia 2019-09-09 7:07 ` [PATCH 0/4] Enable Hantro G1 post-processor Tomasz Figa 2019-09-11 8:27 ` Ezequiel Garcia 2019-09-11 19:48 ` Nicolas Dufresne 2019-09-11 19:49 ` Nicolas Dufresne 2019-09-12 5:52 ` Tomasz Figa 2019-09-12 11:35 ` Ezequiel Garcia 2019-09-16 18:18 ` Helen Koike
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).