Received: by 2002:a05:6a11:4021:0:0:0:0 with SMTP id ky33csp5171228pxb; Tue, 28 Sep 2021 12:03:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwxYzj/QJhHGSAe9VYhlvIvBJpU1etM1F8erGEb16ofPZjqSp1LaNdvCQIXM2+e6k5bwa/O X-Received: by 2002:a50:bb09:: with SMTP id y9mr9445806ede.89.1632855809988; Tue, 28 Sep 2021 12:03:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1632855809; cv=none; d=google.com; s=arc-20160816; b=Eo1AFxCcSr4zhr2sJOGUMRJhFDxhRmIV2kefmBEAnnrjlCkoPhQrEWuKUK6YECV0Pc gVnOHp8Ku52skyxe3YVXFtqGUwxM56f5dx95qPyGN27K3iyA9XDPqm66jEqQt6KYktOd AQEkHhEQuusdWueUYwek2xItP5YWcionC+x0Hr83TqlWtQODxqAIYI4tp5VvA9fwxozy iPRMQivzg6eK3ClD6PyyKZwA7ROeS2l+0HuawXcRU/agKZKWS0NrrszCbuvauMS5kgtx JN4SayVanPWpE5tDZAex5P7ZRO6ki5KbtLsyyRJDVz3UlCRMjkyznhFeHM9R9rUyEkPj Gfjw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:date:cc:to:from:subject :message-id; bh=aQ26FTnmMQ2a9PiLYBIb4YHE+ChgwDG2WGthceyeFM8=; b=KMx6U77P9A8gJkn3/HEH3EYUB3CbBGpLVfIjevXal3RFWkE62aX67BAWjbmv9dMOXB LyLfiun5RW3VeGfhGm9PNj+qChBFu+AnVap26Ty78cWPlNtyHNXYTF2Q+GdqraG5WB82 U7R+YT+BeaYtYBXORgXAcEW78I5lXR0AsMFtRSZyPUworAH8jPmkK/TdlZ39IBEWO5Tu g3v5LoSYoJ5L801qUE6A3NK/sS06/alKuoUFPlt+JwEVbvjIS+QjE15f3CirQYmTJOTf p0V9EpBhPF7bDbyqepM69GG2td30bvxOY+v9MZaiAlfMrV3zk5meHkd0N9mIHSQ4Nk0o 9Sqw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id q4si4415647edv.275.2021.09.28.12.02.59; Tue, 28 Sep 2021 12:03:29 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242262AbhI1TA7 (ORCPT + 99 others); Tue, 28 Sep 2021 15:00:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242196AbhI1TA6 (ORCPT ); Tue, 28 Sep 2021 15:00:58 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D0D43C06161C; Tue, 28 Sep 2021 11:59:18 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: nicolas) with ESMTPSA id 58B7E1F43BE3 Message-ID: Subject: Re: [PATCH v6 09/10] media: hantro: Support VP9 on the G2 core From: Nicolas Dufresne To: Ezequiel Garcia , Andrzej Pietrasiewicz Cc: linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev, Benjamin Gaignard , Boris Brezillon , Fabio Estevam , Greg Kroah-Hartman , Hans Verkuil , Heiko Stuebner , Jernej Skrabec , Mauro Carvalho Chehab , NXP Linux Team , Pengutronix Kernel Team , Philipp Zabel , Sascha Hauer , Shawn Guo , kernel@collabora.com, bob.beckett@collabora.com Date: Tue, 28 Sep 2021 14:59:05 -0400 In-Reply-To: References: <20210927151958.24426-1-andrzej.p@collabora.com> <20210927151958.24426-10-andrzej.p@collabora.com> <532bfb30-c464-137e-1635-b6a868d5d7e6@collabora.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le mardi 28 septembre 2021 à 13:55 -0300, Ezequiel Garcia a écrit : > On Tue, Sep 28, 2021 at 05:39:02PM +0200, Andrzej Pietrasiewicz wrote: > > Hi Nicolas, > > > > W dniu 27.09.2021 o 21:17, Nicolas Dufresne pisze: > > > Le lundi 27 septembre 2021 à 17:19 +0200, Andrzej Pietrasiewicz a écrit : > > > > VeriSilicon Hantro G2 core supports VP9 codec. > > > > > > > > Signed-off-by: Andrzej Pietrasiewicz > > > > Reviewed-by: Benjamin Gaignard > > > > --- > > > > drivers/staging/media/hantro/Kconfig | 1 + > > > > drivers/staging/media/hantro/Makefile | 6 +- > > > > drivers/staging/media/hantro/hantro.h | 26 + > > > > drivers/staging/media/hantro/hantro_drv.c | 18 +- > > > > drivers/staging/media/hantro/hantro_g2_regs.h | 97 ++ > > > > .../staging/media/hantro/hantro_g2_vp9_dec.c | 978 ++++++++++++++++++ > > > > drivers/staging/media/hantro/hantro_hw.h | 67 ++ > > > > drivers/staging/media/hantro/hantro_v4l2.c | 6 + > > > > drivers/staging/media/hantro/hantro_vp9.c | 240 +++++ > > > > drivers/staging/media/hantro/hantro_vp9.h | 103 ++ > > > > drivers/staging/media/hantro/imx8m_vpu_hw.c | 22 +- > > > > 11 files changed, 1560 insertions(+), 4 deletions(-) > > > > create mode 100644 drivers/staging/media/hantro/hantro_g2_vp9_dec.c > > > > create mode 100644 drivers/staging/media/hantro/hantro_vp9.c > > > > create mode 100644 drivers/staging/media/hantro/hantro_vp9.h > > > > > > > > diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig > > > > index 20b1f6d7b69c..00a57d88c92e 100644 > > > > --- a/drivers/staging/media/hantro/Kconfig > > > > +++ b/drivers/staging/media/hantro/Kconfig > > > > @@ -9,6 +9,7 @@ config VIDEO_HANTRO > > > > select VIDEOBUF2_VMALLOC > > > > select V4L2_MEM2MEM_DEV > > > > select V4L2_H264 > > > > + select V4L2_VP9 > > > > help > > > > Support for the Hantro IP based Video Processing Units present on > > > > Rockchip and NXP i.MX8M SoCs, which accelerate video and image > > > > diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile > > > > index fe6d84871d07..28af0a1ee4bf 100644 > > > > --- a/drivers/staging/media/hantro/Makefile > > > > +++ b/drivers/staging/media/hantro/Makefile > > > > @@ -10,9 +10,10 @@ hantro-vpu-y += \ > > > > hantro_g1.o \ > > > > hantro_g1_h264_dec.o \ > > > > hantro_g1_mpeg2_dec.o \ > > > > - hantro_g2_hevc_dec.o \ > > > > hantro_g1_vp8_dec.o \ > > > > hantro_g2.o \ > > > > + hantro_g2_hevc_dec.o \ > > > > + hantro_g2_vp9_dec.o \ > > > > rockchip_vpu2_hw_jpeg_enc.o \ > > > > rockchip_vpu2_hw_h264_dec.o \ > > > > rockchip_vpu2_hw_mpeg2_dec.o \ > > > > @@ -21,7 +22,8 @@ hantro-vpu-y += \ > > > > hantro_h264.o \ > > > > hantro_hevc.o \ > > > > hantro_mpeg2.o \ > > > > - hantro_vp8.o > > > > + hantro_vp8.o \ > > > > + hantro_vp9.o > > > > hantro-vpu-$(CONFIG_VIDEO_HANTRO_IMX8M) += \ > > > > imx8m_vpu_hw.o > > > > diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h > > > > index d91eb2b1c509..1e8c1a6e3eb0 100644 > > > > --- a/drivers/staging/media/hantro/hantro.h > > > > +++ b/drivers/staging/media/hantro/hantro.h > > > > @@ -36,6 +36,7 @@ struct hantro_postproc_ops; > > > > #define HANTRO_VP8_DECODER BIT(17) > > > > #define HANTRO_H264_DECODER BIT(18) > > > > #define HANTRO_HEVC_DECODER BIT(19) > > > > +#define HANTRO_VP9_DECODER BIT(20) > > > > #define HANTRO_DECODERS 0xffff0000 > > > > /** > > > > @@ -110,6 +111,7 @@ enum hantro_codec_mode { > > > > HANTRO_MODE_MPEG2_DEC, > > > > HANTRO_MODE_VP8_DEC, > > > > HANTRO_MODE_HEVC_DEC, > > > > + HANTRO_MODE_VP9_DEC, > > > > }; > > > > /* > > > > @@ -223,6 +225,7 @@ struct hantro_dev { > > > > * @mpeg2_dec: MPEG-2-decoding context. > > > > * @vp8_dec: VP8-decoding context. > > > > * @hevc_dec: HEVC-decoding context. > > > > + * @vp9_dec: VP9-decoding context. > > > > */ > > > > struct hantro_ctx { > > > > struct hantro_dev *dev; > > > > @@ -250,6 +253,7 @@ struct hantro_ctx { > > > > struct hantro_mpeg2_dec_hw_ctx mpeg2_dec; > > > > struct hantro_vp8_dec_hw_ctx vp8_dec; > > > > struct hantro_hevc_dec_hw_ctx hevc_dec; > > > > + struct hantro_vp9_dec_hw_ctx vp9_dec; > > > > }; > > > > }; > > > > @@ -299,6 +303,22 @@ struct hantro_postproc_regs { > > > > struct hantro_reg display_width; > > > > }; > > > > +struct hantro_vp9_decoded_buffer_info { > > > > + /* Info needed when the decoded frame serves as a reference frame. */ > > > > + unsigned short width; > > > > + unsigned short height; > > > > + u32 bit_depth : 4; > > > > +}; > > > > + > > > > +struct hantro_decoded_buffer { > > > > + /* Must be the first field in this struct. */ > > > > + struct v4l2_m2m_buffer base; > > > > + > > > > + union { > > > > + struct hantro_vp9_decoded_buffer_info vp9; > > > > + }; > > > > +}; > > > > + > > > > /* Logging helpers */ > > > > /** > > > > @@ -436,6 +456,12 @@ hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb) > > > > return vb2_dma_contig_plane_dma_addr(vb, 0); > > > > } > > > > +static inline struct hantro_decoded_buffer * > > > > +vb2_to_hantro_decoded_buf(struct vb2_buffer *buf) > > > > +{ > > > > + return container_of(buf, struct hantro_decoded_buffer, base.vb.vb2_buf); > > > > +} > > > > + > > > > void hantro_postproc_disable(struct hantro_ctx *ctx); > > > > void hantro_postproc_enable(struct hantro_ctx *ctx); > > > > void hantro_postproc_free(struct hantro_ctx *ctx); > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > > > index 8a2edd67f2c6..800c8879aee0 100644 > > > > --- a/drivers/staging/media/hantro/hantro_drv.c > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > > > @@ -232,7 +232,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) > > > > dst_vq->io_modes = VB2_MMAP | VB2_DMABUF; > > > > dst_vq->drv_priv = ctx; > > > > dst_vq->ops = &hantro_queue_ops; > > > > - dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer); > > > > + dst_vq->buf_struct_size = sizeof(struct hantro_decoded_buffer); > > > > dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY; > > > > dst_vq->lock = &ctx->dev->vpu_mutex; > > > > dst_vq->dev = ctx->dev->v4l2_dev.dev; > > > > @@ -266,6 +266,12 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl) > > > > if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) > > > > /* No scaling support */ > > > > return -EINVAL; > > > > + } else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) { > > > > + const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame; > > > > + > > > > + /* We only support profile 0 */ > > > > + if (dec_params->profile != 0) > > > > + return -EINVAL; > > > > } > > > > return 0; > > > > } > > > > @@ -459,6 +465,16 @@ static const struct hantro_ctrl controls[] = { > > > > .step = 1, > > > > .ops = &hantro_hevc_ctrl_ops, > > > > }, > > > > + }, { > > > > + .codec = HANTRO_VP9_DECODER, > > > > + .cfg = { > > > > + .id = V4L2_CID_STATELESS_VP9_FRAME, > > > > + }, > > > > + }, { > > > > + .codec = HANTRO_VP9_DECODER, > > > > + .cfg = { > > > > + .id = V4L2_CID_STATELESS_VP9_COMPRESSED_HDR, > > > > + }, > > > > }, > > > > }; > > > > diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h > > > > index 0ac0ba375e80..21ca21648614 100644 > > > > --- a/drivers/staging/media/hantro/hantro_g2_regs.h > > > > +++ b/drivers/staging/media/hantro/hantro_g2_regs.h > > > > @@ -28,6 +28,7 @@ > > > > #define G2_REG_INTERRUPT_DEC_E BIT(0) > > > > #define HEVC_DEC_MODE 0xc > > > > +#define VP9_DEC_MODE 0xd > > > > #define BUS_WIDTH_32 0 > > > > #define BUS_WIDTH_64 1 > > > > @@ -49,6 +50,7 @@ > > > > #define g2_pic_height_in_cbs G2_DEC_REG(4, 6, 0x1fff) > > > > #define g2_num_ref_frames G2_DEC_REG(4, 0, 0x1f) > > > > +#define g2_start_bit G2_DEC_REG(5, 25, 0x7f) > > > > #define g2_scaling_list_e G2_DEC_REG(5, 24, 0x1) > > > > #define g2_cb_qp_offset G2_DEC_REG(5, 19, 0x1f) > > > > #define g2_cr_qp_offset G2_DEC_REG(5, 14, 0x1f) > > > > @@ -84,6 +86,7 @@ > > > > #define g2_bit_depth_y_minus8 G2_DEC_REG(8, 6, 0x3) > > > > #define g2_bit_depth_c_minus8 G2_DEC_REG(8, 4, 0x3) > > > > #define g2_output_8_bits G2_DEC_REG(8, 3, 0x1) > > > > +#define g2_output_format G2_DEC_REG(8, 0, 0x7) > > > > #define g2_refidx1_active G2_DEC_REG(9, 19, 0x1f) > > > > #define g2_refidx0_active G2_DEC_REG(9, 14, 0x1f) > > > > @@ -96,6 +99,14 @@ > > > > #define g2_tile_e G2_DEC_REG(10, 1, 0x1) > > > > #define g2_entropy_sync_e G2_DEC_REG(10, 0, 0x1) > > > > +#define vp9_transform_mode G2_DEC_REG(11, 27, 0x7) > > > > +#define vp9_filt_sharpness G2_DEC_REG(11, 21, 0x7) > > > > +#define vp9_mcomp_filt_type G2_DEC_REG(11, 8, 0x7) > > > > +#define vp9_high_prec_mv_e G2_DEC_REG(11, 7, 0x1) > > > > +#define vp9_comp_pred_mode G2_DEC_REG(11, 4, 0x3) > > > > +#define vp9_gref_sign_bias G2_DEC_REG(11, 2, 0x1) > > > > +#define vp9_aref_sign_bias G2_DEC_REG(11, 0, 0x1) > > > > + > > > > #define g2_refer_lterm_e G2_DEC_REG(12, 16, 0xffff) > > > > #define g2_min_cb_size G2_DEC_REG(12, 13, 0x7) > > > > #define g2_max_cb_size G2_DEC_REG(12, 10, 0x7) > > > > @@ -154,6 +165,50 @@ > > > > #define g2_partial_ctb_y G2_DEC_REG(20, 30, 0x1) > > > > #define g2_pic_width_4x4 G2_DEC_REG(20, 16, 0xfff) > > > > #define g2_pic_height_4x4 G2_DEC_REG(20, 0, 0xfff) > > > > + > > > > +#define vp9_qp_delta_y_dc G2_DEC_REG(13, 23, 0x3f) > > > > +#define vp9_qp_delta_ch_dc G2_DEC_REG(13, 17, 0x3f) > > > > +#define vp9_qp_delta_ch_ac G2_DEC_REG(13, 11, 0x3f) > > > > +#define vp9_last_sign_bias G2_DEC_REG(13, 10, 0x1) > > > > +#define vp9_lossless_e G2_DEC_REG(13, 9, 0x1) > > > > +#define vp9_comp_pred_var_ref1 G2_DEC_REG(13, 7, 0x3) > > > > +#define vp9_comp_pred_var_ref0 G2_DEC_REG(13, 5, 0x3) > > > > +#define vp9_comp_pred_fixed_ref G2_DEC_REG(13, 3, 0x3) > > > > +#define vp9_segment_temp_upd_e G2_DEC_REG(13, 2, 0x1) > > > > +#define vp9_segment_upd_e G2_DEC_REG(13, 1, 0x1) > > > > +#define vp9_segment_e G2_DEC_REG(13, 0, 0x1) > > > > + > > > > +#define vp9_filt_level G2_DEC_REG(14, 18, 0x3f) > > > > +#define vp9_refpic_seg0 G2_DEC_REG(14, 15, 0x7) > > > > +#define vp9_skip_seg0 G2_DEC_REG(14, 14, 0x1) > > > > +#define vp9_filt_level_seg0 G2_DEC_REG(14, 8, 0x3f) > > > > +#define vp9_quant_seg0 G2_DEC_REG(14, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg1 G2_DEC_REG(15, 15, 0x7) > > > > +#define vp9_skip_seg1 G2_DEC_REG(15, 14, 0x1) > > > > +#define vp9_filt_level_seg1 G2_DEC_REG(15, 8, 0x3f) > > > > +#define vp9_quant_seg1 G2_DEC_REG(15, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg2 G2_DEC_REG(16, 15, 0x7) > > > > +#define vp9_skip_seg2 G2_DEC_REG(16, 14, 0x1) > > > > +#define vp9_filt_level_seg2 G2_DEC_REG(16, 8, 0x3f) > > > > +#define vp9_quant_seg2 G2_DEC_REG(16, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg3 G2_DEC_REG(17, 15, 0x7) > > > > +#define vp9_skip_seg3 G2_DEC_REG(17, 14, 0x1) > > > > +#define vp9_filt_level_seg3 G2_DEC_REG(17, 8, 0x3f) > > > > +#define vp9_quant_seg3 G2_DEC_REG(17, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg4 G2_DEC_REG(18, 15, 0x7) > > > > +#define vp9_skip_seg4 G2_DEC_REG(18, 14, 0x1) > > > > +#define vp9_filt_level_seg4 G2_DEC_REG(18, 8, 0x3f) > > > > +#define vp9_quant_seg4 G2_DEC_REG(18, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg5 G2_DEC_REG(19, 15, 0x7) > > > > +#define vp9_skip_seg5 G2_DEC_REG(19, 14, 0x1) > > > > +#define vp9_filt_level_seg5 G2_DEC_REG(19, 8, 0x3f) > > > > +#define vp9_quant_seg5 G2_DEC_REG(19, 0, 0xff) > > > > + > > > > #define hevc_cur_poc_00 G2_DEC_REG(46, 24, 0xff) > > > > #define hevc_cur_poc_01 G2_DEC_REG(46, 16, 0xff) > > > > #define hevc_cur_poc_02 G2_DEC_REG(46, 8, 0xff) > > > > @@ -174,6 +229,44 @@ > > > > #define hevc_cur_poc_14 G2_DEC_REG(49, 8, 0xff) > > > > #define hevc_cur_poc_15 G2_DEC_REG(49, 0, 0xff) > > > > +#define vp9_refpic_seg6 G2_DEC_REG(31, 15, 0x7) > > > > +#define vp9_skip_seg6 G2_DEC_REG(31, 14, 0x1) > > > > +#define vp9_filt_level_seg6 G2_DEC_REG(31, 8, 0x3f) > > > > +#define vp9_quant_seg6 G2_DEC_REG(31, 0, 0xff) > > > > + > > > > +#define vp9_refpic_seg7 G2_DEC_REG(32, 15, 0x7) > > > > +#define vp9_skip_seg7 G2_DEC_REG(32, 14, 0x1) > > > > +#define vp9_filt_level_seg7 G2_DEC_REG(32, 8, 0x3f) > > > > +#define vp9_quant_seg7 G2_DEC_REG(32, 0, 0xff) > > > > + > > > > +#define vp9_lref_width G2_DEC_REG(33, 16, 0xffff) > > > > +#define vp9_lref_height G2_DEC_REG(33, 0, 0xffff) > > > > + > > > > +#define vp9_gref_width G2_DEC_REG(34, 16, 0xffff) > > > > +#define vp9_gref_height G2_DEC_REG(34, 0, 0xffff) > > > > + > > > > +#define vp9_aref_width G2_DEC_REG(35, 16, 0xffff) > > > > +#define vp9_aref_height G2_DEC_REG(35, 0, 0xffff) > > > > + > > > > +#define vp9_lref_hor_scale G2_DEC_REG(36, 16, 0xffff) > > > > +#define vp9_lref_ver_scale G2_DEC_REG(36, 0, 0xffff) > > > > + > > > > +#define vp9_gref_hor_scale G2_DEC_REG(37, 16, 0xffff) > > > > +#define vp9_gref_ver_scale G2_DEC_REG(37, 0, 0xffff) > > > > + > > > > +#define vp9_aref_hor_scale G2_DEC_REG(38, 16, 0xffff) > > > > +#define vp9_aref_ver_scale G2_DEC_REG(38, 0, 0xffff) > > > > + > > > > +#define vp9_filt_ref_adj_0 G2_DEC_REG(46, 24, 0x7f) > > > > +#define vp9_filt_ref_adj_1 G2_DEC_REG(46, 16, 0x7f) > > > > +#define vp9_filt_ref_adj_2 G2_DEC_REG(46, 8, 0x7f) > > > > +#define vp9_filt_ref_adj_3 G2_DEC_REG(46, 0, 0x7f) > > > > + > > > > +#define vp9_filt_mb_adj_0 G2_DEC_REG(47, 24, 0x7f) > > > > +#define vp9_filt_mb_adj_1 G2_DEC_REG(47, 16, 0x7f) > > > > +#define vp9_filt_mb_adj_2 G2_DEC_REG(47, 8, 0x7f) > > > > +#define vp9_filt_mb_adj_3 G2_DEC_REG(47, 0, 0x7f) > > > > + > > > > #define g2_apf_threshold G2_DEC_REG(55, 0, 0xffff) > > > > #define g2_clk_gate_e G2_DEC_REG(58, 16, 0x1) > > > > @@ -186,6 +279,8 @@ > > > > #define G2_ADDR_DST (G2_SWREG(65)) > > > > #define G2_REG_ADDR_REF(i) (G2_SWREG(67) + ((i) * 0x8)) > > > > +#define VP9_ADDR_SEGMENT_WRITE (G2_SWREG(79)) > > > > +#define VP9_ADDR_SEGMENT_READ (G2_SWREG(81)) > > > > #define G2_ADDR_DST_CHR (G2_SWREG(99)) > > > > #define G2_REG_CHR_REF(i) (G2_SWREG(101) + ((i) * 0x8)) > > > > #define G2_ADDR_DST_MV (G2_SWREG(133)) > > > > @@ -193,6 +288,8 @@ > > > > #define G2_ADDR_TILE_SIZE (G2_SWREG(167)) > > > > > > I think this name can be improved. We don't say "G2 address tile size", but "G2 > > > tile size address". Also, there is multiple tile sizes, so I'd prefer if we say > > > "G2 tile sizes address". May I suggest this naming then ? > > > > > > #define G2_TILE_SIZES_ADDR_LSB ... > > > > > > As a reference, VSI names this sw_tile_base_lsb . That denotes that the name > > > also fails to capture that this is only the lower 32bit of the tile_sizes > > > address and that if someone have a G2 core on a larger amount of RAM it will > > > need to figure where is the second half (spoiler, it looks like it's > > > G1_SWREG(166) ). Anyway, that's why I'm proposing dropping LSB in there. Not > > > sure between LSB_ADDR or ADDR_LSB. > > > > The LSB/MSB point is indeed valid. I think I will change hantro_write_addr() to > > something on the lines of: > > > > static inline void hantro_write_addr(struct hantro_dev *vpu, > > unsigned long offset, > > dma_addr_t addr) > > { > > vdpu_write(vpu, addr & 0xffffffff, offset); > > #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > vdpu_write(vpu, (addr >> 32) & 0xffffffff, offset + 4); > > #endif > > } > > > > Then the #defines will not need the _LSB/_MSB suffixes at all > > (and will remain as they are) and the modified function will > > still do the right thing. > > > > I'd rather not go in this direction. First of all, ifdefs are something > to avoid, and only use if really needed. I know there are other drivers > doing this, but that doesn't mean it's nice. > > And then I'm not sure if this is even correct. The MSB bits are needed > if the device is sitting on a bus that can do 64-bits DMA. > That _may_ be something you can figure out per architecture, > but doesn't have to be like that. > > For instance, the Hantro device could be sitting on PCI (Adding > Robert in Cc who can comment on this). > > For now, I don't think we care given we are setting > the DMA mask to 32-bits (see hantro_probe). So we won't get > 64-bit addresses. I'd really like to see 64-bit support > via the LSB/MSB registers, but I think it's not something > we should cover on this VP9 patchset. Perhaps you wanted to suggest to just clear the MSB to 0 for now ? Or perhaps you forgot to make a suggestion and have something else in mind ? The register names are written as foot guns, they don't match either HW designer doc, or vendor code, and they are missing previous information, like being an address, or having a companion MSB addr. We don't have to fix everything, but I think when we notice such thing, it should at least go into TODOs of the respective staging driver if not already there. > > One more thing: is it correct to assume that the MSB register > is always right after the LSB register? Is that the case > for all buffers and Hantro-like variants? Well, no, since it's always before (e.g. REG 166 is sw_tile_base_msb and REG 167 is sw_tile_base_lsb). I didn't comment as this was commented as "something on the lines of:" and I'm pretty sure Andrzej Pietrasiewicz would have noticed. Though, from the best of my reading of the ref code and register map document I have, it's consistent for all addresses. > > Thanks, > Ezequiel