NVIDIA Tegra20 video decoder driver

Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only.

Quoted 3 lines · Dmitry Osipenko

Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only.

Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding:

Quoted 3 lines

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt line 8

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +8 -0

+NVIDIA Tegra Video Decoder Engine
+
+Required properties:
+- compatible : "nvidia,tegra20-vde"
+- reg : Must contain 2 register ranges: registers and IRAM area.
+- reg-names : Must include the following entries:
+  - regs
+  - iram

I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW?

Quoted 6 lines

+- clocks : Must contain one entry, for the module clock. +  See ../clocks/clock-bindings.txt for details.
+- resets : Must contain an entry for each entry in reset-names. +  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - vde

Let's require a clock-names property too.

Quoted 13 lines · Stephen Warren

Quoted 3 lines · Dmitry Osipenko

Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only.

Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding:

Quoted 3 lines

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt

Quoted 12 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt line 8

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +8 -0

+NVIDIA Tegra Video Decoder Engine
+
+Required properties:
+- compatible : "nvidia,tegra20-vde"
+- reg : Must contain 2 register ranges: registers and IRAM area.
+- reg-names : Must include the following entries:
+  - regs
+  - iram

I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW?

IRAM region is used by Video Decoder HW for internal use and some of decoding parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. Should it be explained in the binding?

Quoted 27 lines · Dmitry Osipenko

Quoted 13 lines · Stephen Warren

Quoted 3 lines · Dmitry Osipenko

Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only.

Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding:

Quoted 3 lines

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt

Quoted 12 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt line 8

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt new file +8 -0

+NVIDIA Tegra Video Decoder Engine
+
+Required properties:
+- compatible : "nvidia,tegra20-vde"
+- reg : Must contain 2 register ranges: registers and IRAM area.
+- reg-names : Must include the following entries:
+  - regs
+  - iram

I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW?

Quoted 4 lines

IRAM region is used by Video Decoder HW for internal use and some of decoding parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. Should it be explained in the binding?

I think this should be briefly mentioned, yes. Otherwise at least people who don't know the VDE HW well (like me) will wonder why on earth VDE interacts with IRAM at all. I would have assumed all parameters were supplied via registers or via descriptors in DRAM.

Thanks.

drivers/staging/tegra-vde/Kconfig new file +3 -0

+	help
+	    Say Y here to enable support for a NVIDIA Tegra20 video decoder
+	    driver.

drivers/staging/tegra-vde/Makefile new file +1 -0

new file mode 100644
index 000000000000..e7c0df1174bf
@@ -0,0 +1 @@
+obj-$(CONFIG_TEGRA_VDE)	+= vde.o

drivers/staging/tegra-vde/TODO new file +0 -0

new file mode 100644
index 000000000000..533ddfc5190e

drivers/staging/tegra-vde/TODO new file +48 -0

+static int tegra_vde_setup_context(struct tegra_vde *vde,
+				   struct tegra_vde_h264_decoder_ctx *ctx,
+				   struct video_frame *dpb_frames,
+				   phys_addr_t bitstream_data_paddr,
+				   int bitstream_data_size,
+				   int macroblocks_nb)
+{
+	struct device *dev = vde->miscdev.parent;
+	u32 value;
+
+	tegra_vde_set_bits(vde->regs,    0xA, SXE(0xF0));
+	tegra_vde_set_bits(vde->regs,    0xB, BSEV(CMDQUE_CONTROL));
+	tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
+	tegra_vde_set_bits(vde->regs,    0xA, MBE(0xA0));
+	tegra_vde_set_bits(vde->regs,    0xA, PPE(0x14));
+	tegra_vde_set_bits(vde->regs,    0xA, PPE(0x28));
+	tegra_vde_set_bits(vde->regs,  0xA00, MCE(0x08));
+	tegra_vde_set_bits(vde->regs,    0xA, TFE(0x00));
+	tegra_vde_set_bits(vde->regs,    0x5, VDMA(0x04));
+
+	VDE_WR(0x00000000, vde->regs + VDMA(0x1C));
+	VDE_WR(0x00000000, vde->regs + VDMA(0x00));
+	VDE_WR(0x00000007, vde->regs + VDMA(0x04));
+	VDE_WR(0x00000007, vde->regs + FRAMEID(0x200));
+	VDE_WR(0x00000005, vde->regs + TFE(0x04));
+	VDE_WR(0x00000000, vde->regs + MBE(0x84));
+	VDE_WR(0x00000010, vde->regs + SXE(0x08));
+	VDE_WR(0x00000150, vde->regs + SXE(0x54));
+	VDE_WR(0x0000054C, vde->regs + SXE(0x58));
+	VDE_WR(0x00000E34, vde->regs + SXE(0x5C));
+	VDE_WR(0x063C063C, vde->regs + MCE(0x10));
+	VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
+	VDE_WR(0x0000150D, vde->regs + BSEV(BSE_CONFIG));
+	VDE_WR(0x00000100, vde->regs + BSEV(BSE_INT_ENB));
+	VDE_WR(0x00000000, vde->regs + BSEV(0x98));
+	VDE_WR(0x00000060, vde->regs + BSEV(0x9C));
+
+	memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
+
+	tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
+			     ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
+
+	tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
+				    ctx->dpb_frames_nb - 1,
+				    ctx->dpb_ref_frames_with_earlier_poc_nb);
+
+	VDE_WR(0x00000000, vde->regs + BSEV(0x8C));
+	VDE_WR(bitstream_data_paddr + bitstream_data_size,

drivers/staging/tegra-vde/TODO new file +4 -0

+		return -EIO;
+
+	if (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))
+		return -EIO;

drivers/staging/tegra-vde/TODO new file +53 -0

+	value |= ctx->pic_width_in_mbs << 11;
+	value |= ctx->pic_height_in_mbs << 3;
+
+	VDE_WR(value, vde->regs + SXE(0x10));
+
+	value = !ctx->is_baseline_profile << 17;
+	value |= ctx->level_idc << 13;
+	value |= ctx->log2_max_pic_order_cnt_lsb << 7;
+	value |= ctx->pic_order_cnt_type << 5;
+	value |= ctx->log2_max_frame_num;
+
+	VDE_WR(value, vde->regs + SXE(0x40));
+
+	value = ctx->pic_init_qp << 25;
+	value |= !!(ctx->deblocking_filter_control_present_flag) << 2;
+	value |= !!ctx->pic_order_present_flag;
+
+	VDE_WR(value, vde->regs + SXE(0x44));
+
+	value = ctx->chroma_qp_index_offset;
+	value |= ctx->num_ref_idx_l0_active_minus1 << 5;
+	value |= ctx->num_ref_idx_l1_active_minus1 << 10;
+	value |= !!ctx->constrained_intra_pred_flag << 15;
+
+	VDE_WR(value, vde->regs + SXE(0x48));
+
+	value = 0x0C000000;
+	value |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 24;
+
+	VDE_WR(value, vde->regs + SXE(0x4C));
+
+	value = 0x03800000;
+	value |= min(bitstream_data_size, SZ_1M);
+
+	VDE_WR(value, vde->regs + SXE(0x68));
+
+	VDE_WR(bitstream_data_paddr, vde->regs + SXE(0x6C));
+
+	value = (1 << 28) | 5;
+	value |= ctx->pic_width_in_mbs << 11;
+	value |= ctx->pic_height_in_mbs << 3;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	value = 0x26800000;
+	value |= ctx->level_idc << 4;
+	value |= !ctx->is_baseline_profile << 1;
+	value |= !!ctx->direct_8x8_inference_flag;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	VDE_WR(0xF4000001, vde->regs + MBE(0x80));
+	VDE_WR(0x20000000, vde->regs + MBE(0x80));

drivers/staging/tegra-vde/TODO new file +27 -0

+		dev_err(dev, "MBE frames setup failed\n");
+		return -EIO;
+	}
+
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 0, 0x000009FC);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 2, 0xF1DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 4, 0xF2DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 6, 0xF3DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 8, dpb_frames[0].aux_paddr);
+
+	value = 0xFC000000;
+	value |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 2;
+
+	if (!ctx->is_baseline_profile)
+		value |= !!(dpb_frames[0].flags & FLAG_IS_REFERENCE) << 1;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	if (tegra_vde_wait_mbe(vde->regs)) {
+		dev_err(dev, "MBE programming failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void tegra_vde_decode_frame(struct tegra_vde *vde, int macroblocks_nb)

drivers/staging/tegra-vde/TODO new file +39 -0

+		return;
+
+	dma_buf_detach(dmabuf, a);
+	dma_buf_put(dmabuf);
+}
+
+static int tegra_vde_attach_dmabuf(struct device *dev, int fd,
+				   unsigned long offset, int min_size,
+				   struct dma_buf_attachment **a,
+				   phys_addr_t *paddr, u32 *size,
+				   enum dma_data_direction dma_dir)
+{
+	struct dma_buf_attachment *attachment;
+	struct dma_buf *dmabuf;
+	struct sg_table *sgt;
+
+	*a = NULL;
+	*paddr = 0xFBDEAD00;
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf)) {
+		dev_err(dev, "Invalid dmabuf FD\n");
+		return PTR_ERR(dmabuf);
+	}
+
+	if ((u64)offset + min_size > dmabuf->size) {
+		dev_err(dev, "Too small dmabuf size %d @0x%lX, "
+			     "should be at least %d\n",
+			dmabuf->size, offset, min_size);
+		return -EINVAL;
+	}
+
+	attachment = dma_buf_attach(dmabuf, dev);
+	if (IS_ERR(attachment)) {
+		dev_err(dev, "Failed to attach dmabuf\n");
+		dma_buf_put(dmabuf);
+		return PTR_ERR(attachment);
+	}
+

drivers/staging/tegra-vde/TODO new file +23 -0

+}
+
+static int tegra_vde_attach_frame_dmabufs(struct device *dev,
+					  struct video_frame *frame,
+					  struct tegra_vde_h264_frame *source,
+					  enum dma_data_direction dma_dir,
+					  int is_baseline_profile, int csize)
+{
+	int ret;
+
+	ret = tegra_vde_attach_dmabuf(dev, source->y_fd,
+				      source->y_offset, csize * 4,
+				      &frame->y_dmabuf_attachment,
+				      &frame->y_paddr, NULL, dma_dir);
+	if (ret)
+		return ret;
+
+	ret = tegra_vde_attach_dmabuf(dev, source->cb_fd,
+				      source->cb_offset, csize,
+				      &frame->cb_dmabuf_attachment,
+				      &frame->cb_paddr, NULL, dma_dir);
+	if (ret)
+		return ret;

drivers/staging/tegra-vde/TODO new file +117 -0

+	}
+
+	if (frame->frame_num > 0x7FFFFF) {
+		dev_err(dev, "Bad frame_num %u\n", frame->frame_num);
+		return -EINVAL;
+	}
+
+	if (frame->y_offset & 0xFF) {
+		dev_err(dev, "Bad y_offset 0x%X\n", frame->y_offset);
+		return -EINVAL;
+	}
+
+	if (frame->cb_offset & 0xFF) {
+		dev_err(dev, "Bad cb_offset 0x%X\n", frame->cb_offset);
+		return -EINVAL;
+	}
+
+	if (frame->cr_offset & 0xFF) {
+		dev_err(dev, "Bad cr_offset 0x%X\n", frame->cr_offset);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tegra_vde_validate_h264_ctx(struct device *dev,
+				       struct tegra_vde_h264_decoder_ctx *ctx)
+{
+	if (ctx->dpb_frames_nb == 0 || ctx->dpb_frames_nb > 17) {
+		dev_err(dev, "Bad DPB size %u\n", ctx->dpb_frames_nb);
+		return -EINVAL;
+	}
+
+	if (ctx->level_idc > 15) {
+		dev_err(dev, "Bad level value %u\n", ctx->level_idc);
+		return -EINVAL;
+	}
+
+	if (ctx->pic_init_qp > 52) {
+		dev_err(dev, "Bad pic_init_qp value %u\n", ctx->pic_init_qp);
+		return -EINVAL;
+	}
+
+	if (ctx->log2_max_pic_order_cnt_lsb > 16) {
+		dev_err(dev, "Bad log2_max_pic_order_cnt_lsb value %u\n",
+			ctx->log2_max_pic_order_cnt_lsb);
+		return -EINVAL;
+	}
+
+	if (ctx->log2_max_frame_num > 16) {
+		dev_err(dev, "Bad log2_max_frame_num value %u\n",
+			ctx->log2_max_frame_num);
+		return -EINVAL;
+	}
+
+	if (ctx->chroma_qp_index_offset > 31) {
+		dev_err(dev, "Bad chroma_qp_index_offset value %u\n",
+			ctx->chroma_qp_index_offset);
+		return -EINVAL;
+	}
+
+	if (ctx->pic_order_cnt_type > 2) {
+		dev_err(dev, "Bad pic_order_cnt_type value %u\n",
+			ctx->pic_order_cnt_type);
+		return -EINVAL;
+	}
+
+	if (ctx->num_ref_idx_l0_active_minus1 > 15) {
+		dev_err(dev, "Bad num_ref_idx_l0_active_minus1 value %u\n",
+			ctx->num_ref_idx_l0_active_minus1);
+		return -EINVAL;
+	}
+
+	if (ctx->num_ref_idx_l1_active_minus1 > 15) {
+		dev_err(dev, "Bad num_ref_idx_l1_active_minus1 value %u\n",
+			ctx->num_ref_idx_l1_active_minus1);
+		return -EINVAL;
+	}
+
+	if (!ctx->pic_width_in_mbs || ctx->pic_width_in_mbs > 127) {
+		dev_err(dev, "Bad pic_width_in_mbs value %u, min 1 max 127\n",
+			ctx->pic_width_in_mbs);
+		return -EINVAL;
+	}
+
+	if (!ctx->pic_height_in_mbs || ctx->pic_height_in_mbs > 127) {
+		dev_err(dev, "Bad pic_height_in_mbs value %u, min 1 max 127\n",
+			ctx->pic_height_in_mbs);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
+				       unsigned long vaddr)
+{
+	struct tegra_vde_h264_decoder_ctx ctx;
+	struct tegra_vde_h264_frame frame;
+	struct device *dev = vde->miscdev.parent;
+	struct video_frame *dpb_frames = NULL;
+	struct dma_buf_attachment *bitstream_data_dmabuf_attachment = NULL;
+	enum dma_data_direction dma_dir;
+	phys_addr_t bitstream_data_paddr;
+	phys_addr_t bsev_paddr;
+	u32 bitstream_data_size;
+	u32 macroblocks_nb;
+	bool timeout = false;
+	int i, ret;
+
+	if (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx))) {
+		dev_err(dev, "Copying of H.264 CTX from user failed\n");
+		return -EFAULT;
+	}
+
+	ret = tegra_vde_validate_h264_ctx(dev, &ctx);
+	if (ret)

drivers/staging/tegra-vde/TODO new file +38 -0

+
+	dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
+			     GFP_KERNEL);
+	if (!dpb_frames) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
+	macroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;
+	vaddr = ctx.dpb_frames_ptr;
+
+	for (i = 0; i < ctx.dpb_frames_nb; i++) {
+		ret = tegra_vde_copy_and_validate_frame(dev, &frame, vaddr);
+		if (ret)
+			goto cleanup;
+
+		dpb_frames[i].flags = frame.flags;
+		dpb_frames[i].frame_num = frame.frame_num;
+
+		dma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+		ret = tegra_vde_attach_frame_dmabufs(dev,
+						     &dpb_frames[i], &frame,
+						     dma_dir,
+						     ctx.is_baseline_profile,
+						     macroblocks_nb * 64);
+		if (ret) {
+			tegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);
+			goto cleanup;
+		}
+
+		vaddr += sizeof(frame);
+	}
+
+	ret = clk_prepare_enable(vde->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clk: %d\n", ret);
+		goto cleanup;

drivers/staging/tegra-vde/TODO new file +22 -0

+		goto unlock;
+	}
+
+	ret = tegra_vde_setup_context(vde, &ctx, dpb_frames,
+				      bitstream_data_paddr,
+				      bitstream_data_size,
+				      macroblocks_nb);
+	if (ret)
+		goto reset;
+
+	tegra_vde_decode_frame(vde, macroblocks_nb);
+
+	timeout = !wait_for_completion_io_timeout(&vde->decode_completion,
+						  TEGRA_VDE_TIMEOUT);
+	if (timeout) {
+		bsev_paddr = readl(vde->regs + BSEV(0x10));
+		macroblocks_nb = readl(vde->regs + SXE(0xC8)) & 0x1FFF;
+
+		dev_err(dev, "Decoding failed, "
+				"read 0x%X bytes : %u macroblocks parsed\n",
+			bsev_paddr ? bsev_paddr - bitstream_data_paddr : 0,
+			macroblocks_nb);

drivers/staging/tegra-vde/TODO new file +93 -0

+
+	if (timeout)
+		ret = -EIO;
+
+unlock:
+	mutex_unlock(&vde->lock);
+
+clkgate:
+	clk_disable_unprepare(vde->clk);
+
+cleanup:
+	if (dpb_frames)
+		while (i--)
+			tegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);
+
+	kfree(dpb_frames);
+
+	tegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment);
+
+	return ret;
+}
+
+static long tegra_vde_unlocked_ioctl(struct file *filp,
+				     unsigned int cmd, unsigned long arg)
+{
+	struct miscdevice *miscdev = filp->private_data;
+	struct tegra_vde *vde = container_of(miscdev, struct tegra_vde,
+					     miscdev);
+
+	switch (cmd) {
+	case TEGRA_VDE_IOCTL_DECODE_H264:
+		return tegra_vde_ioctl_decode_h264(vde, arg);
+	}
+
+	dev_err(miscdev->parent, "Invalid IOCTL command %u\n", cmd);
+
+	return -ENOTTY;
+}
+
+static const struct file_operations tegra_vde_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tegra_vde_unlocked_ioctl,
+};
+
+static irqreturn_t tegra_vde_isr(int irq, void *data)
+{
+	struct tegra_vde *vde = data;
+
+	tegra_vde_set_bits(vde->regs, 0, FRAMEID(0x208));
+	complete(&vde->decode_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int tegra_vde_probe(struct platform_device *pdev)
+{
+	struct resource *res_regs, *res_iram;
+	struct device *dev = &pdev->dev;
+	struct tegra_vde *vde;
+	int ret;
+
+	vde = devm_kzalloc(&pdev->dev, sizeof(*vde), GFP_KERNEL);
+	if (!vde)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, vde);
+
+	res_regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	if (!res_regs)
+		return -ENODEV;
+
+	res_iram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iram");
+	if (!res_iram)
+		return -ENODEV;
+
+	vde->irq = platform_get_irq_byname(pdev, "sync-token");
+	if (vde->irq < 0)
+		return vde->irq;
+
+	vde->regs = devm_ioremap_resource(dev, res_regs);
+	if (IS_ERR(vde->regs))
+		return PTR_ERR(vde->regs);
+
+	vde->iram = devm_ioremap_resource(dev, res_iram);
+	if (IS_ERR(vde->iram))
+		return PTR_ERR(vde->iram);
+
+	vde->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(vde->clk)) {
+		dev_err(dev, "Could not get VDE clk\n");
+		return PTR_ERR(vde->clk);
+	}
+

drivers/staging/tegra-vde/TODO new file +18 -0

+
+	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,
+						vde->clk, vde->rst);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to power up VDE unit\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(vde->rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert reset: %d\n", ret);
+		return ret;
+	}
+
+	clk_disable_unprepare(vde->clk);
+
+	mutex_init(&vde->lock);
+	init_completion(&vde->decode_completion);

I feel like this code is good enough to go into the regular kernel instead of staging, but I don't really know what "- properly handle decoding faults" means in this context.

time or what?

Anyway, minor comments inline.

drivers/staging/tegra-vde/Kconfig line 3

Quoted 9 lines

drivers/staging/tegra-vde/Kconfig new file +3 -0

new file mode 100644
index 000000000000..b947c012a373
@@ -0,0 +1,6 @@
+config TEGRA_VDE
+	tristate "NVIDIA Tegra20 video decoder driver"
+	depends on ARCH_TEGRA_2x_SOC

Could we get a || COMPILE_TEST here as well?

Quoted 17 lines

drivers/staging/tegra-vde/Kconfig new file +3 -0

+	help
+	    Say Y here to enable support for a NVIDIA Tegra20 video decoder
+	    driver.

drivers/staging/tegra-vde/Makefile new file +1 -0

new file mode 100644
index 000000000000..e7c0df1174bf
@@ -0,0 +1 @@
+obj-$(CONFIG_TEGRA_VDE)	+= vde.o

drivers/staging/tegra-vde/TODO new file +1 -0

new file mode 100644
index 000000000000..533ddfc5190e
@@ -0,0 +1,8 @@
+All TODO's require reverse-engineering to be done first, it is very

drivers/staging/tegra-vde/TODO line 6

Quoted 5 lines

drivers/staging/tegra-vde/TODO new file +5 -0

+unlikely that NVIDIA would ever release HW specs to public.
+
+TODO:
+	- properly handle decoding faults
+	- support more formats

Adding more formats is not a reason to put this into staging instead of the normal drivers/ dir.

Quoted 53 lines

drivers/staging/tegra-vde/TODO new file +53 -0

+static int tegra_vde_setup_context(struct tegra_vde *vde,
+				   struct tegra_vde_h264_decoder_ctx *ctx,
+				   struct video_frame *dpb_frames,
+				   phys_addr_t bitstream_data_paddr,
+				   int bitstream_data_size,
+				   int macroblocks_nb)
+{
+	struct device *dev = vde->miscdev.parent;
+	u32 value;
+
+	tegra_vde_set_bits(vde->regs,    0xA, SXE(0xF0));
+	tegra_vde_set_bits(vde->regs,    0xB, BSEV(CMDQUE_CONTROL));
+	tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
+	tegra_vde_set_bits(vde->regs,    0xA, MBE(0xA0));
+	tegra_vde_set_bits(vde->regs,    0xA, PPE(0x14));
+	tegra_vde_set_bits(vde->regs,    0xA, PPE(0x28));
+	tegra_vde_set_bits(vde->regs,  0xA00, MCE(0x08));
+	tegra_vde_set_bits(vde->regs,    0xA, TFE(0x00));
+	tegra_vde_set_bits(vde->regs,    0x5, VDMA(0x04));
+
+	VDE_WR(0x00000000, vde->regs + VDMA(0x1C));
+	VDE_WR(0x00000000, vde->regs + VDMA(0x00));
+	VDE_WR(0x00000007, vde->regs + VDMA(0x04));
+	VDE_WR(0x00000007, vde->regs + FRAMEID(0x200));
+	VDE_WR(0x00000005, vde->regs + TFE(0x04));
+	VDE_WR(0x00000000, vde->regs + MBE(0x84));
+	VDE_WR(0x00000010, vde->regs + SXE(0x08));
+	VDE_WR(0x00000150, vde->regs + SXE(0x54));
+	VDE_WR(0x0000054C, vde->regs + SXE(0x58));
+	VDE_WR(0x00000E34, vde->regs + SXE(0x5C));
+	VDE_WR(0x063C063C, vde->regs + MCE(0x10));
+	VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
+	VDE_WR(0x0000150D, vde->regs + BSEV(BSE_CONFIG));
+	VDE_WR(0x00000100, vde->regs + BSEV(BSE_INT_ENB));
+	VDE_WR(0x00000000, vde->regs + BSEV(0x98));
+	VDE_WR(0x00000060, vde->regs + BSEV(0x9C));
+
+	memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
+
+	tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
+			     ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
+
+	tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
+				    ctx->dpb_frames_nb - 1,
+				    ctx->dpb_ref_frames_with_earlier_poc_nb);
+
+	VDE_WR(0x00000000, vde->regs + BSEV(0x8C));
+	VDE_WR(bitstream_data_paddr + bitstream_data_size,
+	       vde->regs + BSEV(0x54));
+
+	value = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;
+
+	VDE_WR(value, vde->regs + BSEV(0x88));

drivers/staging/tegra-vde/TODO

Quoted 5 lines

drivers/staging/tegra-vde/TODO new file +5 -0

+
+	if (tegra_vde_wait_bsev(vde, false))
+		return -EIO;
+
+	if (tegra_vde_push_bsev_icmdqueue(vde, 0x800003FC, false))

Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's still -EIO so this doesn't affect runtime.

drivers/staging/tegra-vde/TODO

Quoted 6 lines

drivers/staging/tegra-vde/TODO new file +6 -0

+		return -EIO;
+
+	value = 0x01500000;
+	value |= ((vde->iram_lists_paddr + 512) >> 2) & 0xFFFF;
+
+	if (tegra_vde_push_bsev_icmdqueue(vde, value, true))

Same for all.

Quoted 4 lines

drivers/staging/tegra-vde/TODO new file +4 -0

+		return -EIO;
+
+	if (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))
+		return -EIO;

drivers/staging/tegra-vde/TODO

Quoted 10 lines

drivers/staging/tegra-vde/TODO new file +10 -0

+
+	if (tegra_vde_push_bsev_icmdqueue(vde, 0x80000080, false))
+		return -EIO;
+
+	value = 0x0E340000 | ((vde->iram_lists_paddr >> 2) & 0xFFFF);
+
+	if (tegra_vde_push_bsev_icmdqueue(vde, value, true))
+		return -EIO;
+
+	value = (1 << 23) | 5;

I don't like these magic numbers.

Quoted 53 lines

drivers/staging/tegra-vde/TODO new file +53 -0

+	value |= ctx->pic_width_in_mbs << 11;
+	value |= ctx->pic_height_in_mbs << 3;
+
+	VDE_WR(value, vde->regs + SXE(0x10));
+
+	value = !ctx->is_baseline_profile << 17;
+	value |= ctx->level_idc << 13;
+	value |= ctx->log2_max_pic_order_cnt_lsb << 7;
+	value |= ctx->pic_order_cnt_type << 5;
+	value |= ctx->log2_max_frame_num;
+
+	VDE_WR(value, vde->regs + SXE(0x40));
+
+	value = ctx->pic_init_qp << 25;
+	value |= !!(ctx->deblocking_filter_control_present_flag) << 2;
+	value |= !!ctx->pic_order_present_flag;
+
+	VDE_WR(value, vde->regs + SXE(0x44));
+
+	value = ctx->chroma_qp_index_offset;
+	value |= ctx->num_ref_idx_l0_active_minus1 << 5;
+	value |= ctx->num_ref_idx_l1_active_minus1 << 10;
+	value |= !!ctx->constrained_intra_pred_flag << 15;
+
+	VDE_WR(value, vde->regs + SXE(0x48));
+
+	value = 0x0C000000;
+	value |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 24;
+
+	VDE_WR(value, vde->regs + SXE(0x4C));
+
+	value = 0x03800000;
+	value |= min(bitstream_data_size, SZ_1M);
+
+	VDE_WR(value, vde->regs + SXE(0x68));
+
+	VDE_WR(bitstream_data_paddr, vde->regs + SXE(0x6C));
+
+	value = (1 << 28) | 5;
+	value |= ctx->pic_width_in_mbs << 11;
+	value |= ctx->pic_height_in_mbs << 3;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	value = 0x26800000;
+	value |= ctx->level_idc << 4;
+	value |= !ctx->is_baseline_profile << 1;
+	value |= !!ctx->direct_8x8_inference_flag;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	VDE_WR(0xF4000001, vde->regs + MBE(0x80));
+	VDE_WR(0x20000000, vde->regs + MBE(0x80));

drivers/staging/tegra-vde/TODO

Quoted 10 lines

drivers/staging/tegra-vde/TODO new file +10 -0

+	VDE_WR(0xF4000101, vde->regs + MBE(0x80));
+
+	value = 0x20000000;
+	value |= ctx->chroma_qp_index_offset << 8;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	if (tegra_vde_setup_mbe_frame_idx(vde->regs,
+					  ctx->pic_order_cnt_type == 0,
+					  ctx->dpb_frames_nb - 1)) {

Preserve the error code.

Quoted 31 lines

drivers/staging/tegra-vde/TODO new file +31 -0

+		dev_err(dev, "MBE frames setup failed\n");
+		return -EIO;
+	}
+
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 0, 0x000009FC);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 2, 0xF1DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 4, 0xF2DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 6, 0xF3DEAD00);
+	tegra_vde_mbe_set_0xa_reg(vde->regs, 8, dpb_frames[0].aux_paddr);
+
+	value = 0xFC000000;
+	value |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 2;
+
+	if (!ctx->is_baseline_profile)
+		value |= !!(dpb_frames[0].flags & FLAG_IS_REFERENCE) << 1;
+
+	VDE_WR(value, vde->regs + MBE(0x80));
+
+	if (tegra_vde_wait_mbe(vde->regs)) {
+		dev_err(dev, "MBE programming failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void tegra_vde_decode_frame(struct tegra_vde *vde, int macroblocks_nb)
+{
+	VDE_WR(0x00000001, vde->regs + BSEV(0x8C));
+	VDE_WR(0x20000000 | (macroblocks_nb - 1), vde->regs + SXE(0x00));
+}

drivers/staging/tegra-vde/TODO

Quoted 6 lines

drivers/staging/tegra-vde/TODO new file +6 -0

+
+static void tegra_vde_detach_and_put_dmabuf(struct dma_buf_attachment *a)
+{
+	struct dma_buf *dmabuf = a->dmabuf;
+
+	if (IS_ERR_OR_NULL(a))

You just dereferenced this on the line before so it's too late.

Obviously we don't want to dereference either an error pointer or a NULL but I'm wondering the significance of having it be both. Normally that would mean that NULL is a special case of success. In other words, error pointer means the hardware is broken but NULL means it's missing and not required. I guess I'm suggesting to just delete the check and make sure we never set this to either NULL or ERR_PTR.

Quoted 43 lines

drivers/staging/tegra-vde/TODO new file +43 -0

+		return;
+
+	dma_buf_detach(dmabuf, a);
+	dma_buf_put(dmabuf);
+}
+
+static int tegra_vde_attach_dmabuf(struct device *dev, int fd,
+				   unsigned long offset, int min_size,
+				   struct dma_buf_attachment **a,
+				   phys_addr_t *paddr, u32 *size,
+				   enum dma_data_direction dma_dir)
+{
+	struct dma_buf_attachment *attachment;
+	struct dma_buf *dmabuf;
+	struct sg_table *sgt;
+
+	*a = NULL;
+	*paddr = 0xFBDEAD00;
+
+	dmabuf = dma_buf_get(fd);
+	if (IS_ERR(dmabuf)) {
+		dev_err(dev, "Invalid dmabuf FD\n");
+		return PTR_ERR(dmabuf);
+	}
+
+	if ((u64)offset + min_size > dmabuf->size) {
+		dev_err(dev, "Too small dmabuf size %d @0x%lX, "
+			     "should be at least %d\n",
+			dmabuf->size, offset, min_size);
+		return -EINVAL;
+	}
+
+	attachment = dma_buf_attach(dmabuf, dev);
+	if (IS_ERR(attachment)) {
+		dev_err(dev, "Failed to attach dmabuf\n");
+		dma_buf_put(dmabuf);
+		return PTR_ERR(attachment);
+	}
+
+	sgt = dma_buf_map_attachment(attachment, dma_dir);
+	if (IS_ERR(sgt)) {
+		dev_err(dev, "Failed to get dmabuf sg_table\n");
+		dma_buf_detach(dmabuf, attachment);

drivers/staging/tegra-vde/TODO

Quoted 6 lines

drivers/staging/tegra-vde/TODO new file +6 -0

+		dma_buf_put(dmabuf);
+		return PTR_ERR(sgt);
+	}
+
+	if (sgt->nents != 1) {
+		dev_err(dev, "Sparsed DMA area is unsupported\n");

s/Sparsed/Sparse/

drivers/staging/tegra-vde/TODO

Quoted 4 lines

drivers/staging/tegra-vde/TODO new file +4 -0

+		dma_buf_unmap_attachment(attachment, sgt, dma_dir);
+		dma_buf_detach(dmabuf, attachment);
+		dma_buf_put(dmabuf);
+		return -EINVAL;

This function would be cleaner using gotos to unwind. Pick the goto name to reflect what the goto does. For example, here it would be:

	if (sgt->nents != 1) {
		dev_err(dev, "Sparse DMA area is unsupported\n");
		ret = -EINVAL;
		goto err_umap;
	}

drivers/staging/tegra-vde/TODO

Quoted 12 lines

drivers/staging/tegra-vde/TODO new file +12 -0

+	}
+
+	*paddr = sg_dma_address(sgt->sgl) + offset;
+
+	dma_buf_unmap_attachment(attachment, sgt, dma_dir);
+
+	*a = attachment;
+
+	if (size)
+		*size = dmabuf->size - offset;
+
+	return 0;

	return 0;

err_unmap:

	dma_buf_unmap_attachment(attachment, sgt, dma_dir);
err_detach:
	dma_buf_detach(dmabuf, attachment);
err_put:
	dma_buf_put(dmabuf);
	return ret;

Quoted 29 lines

drivers/staging/tegra-vde/TODO new file +29 -0

+}
+
+static int tegra_vde_attach_frame_dmabufs(struct device *dev,
+					  struct video_frame *frame,
+					  struct tegra_vde_h264_frame *source,
+					  enum dma_data_direction dma_dir,
+					  int is_baseline_profile, int csize)
+{
+	int ret;
+
+	ret = tegra_vde_attach_dmabuf(dev, source->y_fd,
+				      source->y_offset, csize * 4,
+				      &frame->y_dmabuf_attachment,
+				      &frame->y_paddr, NULL, dma_dir);
+	if (ret)
+		return ret;
+
+	ret = tegra_vde_attach_dmabuf(dev, source->cb_fd,
+				      source->cb_offset, csize,
+				      &frame->cb_dmabuf_attachment,
+				      &frame->cb_paddr, NULL, dma_dir);
+	if (ret)
+		return ret;
+
+	ret = tegra_vde_attach_dmabuf(dev, source->cr_fd,
+				      source->cr_offset, csize,
+				      &frame->cr_dmabuf_attachment,
+				      &frame->cr_paddr, NULL, dma_dir);
+	if (ret)

drivers/staging/tegra-vde/TODO

Quoted 4 lines

drivers/staging/tegra-vde/TODO new file +4 -0

+		return ret;
+
+	if (is_baseline_profile)
+		frame->aux_paddr = 0xF4DEAD00;

The handling of is_baseline_profile is strange to me. It feels like we should always check it before we use ->aux_paddr but we don't ever do that.

drivers/staging/tegra-vde/TODO

Quoted 5 lines

drivers/staging/tegra-vde/TODO new file +5 -0

+	else
+		ret = tegra_vde_attach_dmabuf(dev, source->aux_fd,
+					      source->aux_offset, csize,
+					      &frame->aux_dmabuf_attachment,
+					      &frame->aux_paddr, NULL, dma_dir);

This function should have some error handling to undo the earlier attach calls if the latter ones fail. See below:

drivers/staging/tegra-vde/TODO

Quoted 2 lines

drivers/staging/tegra-vde/TODO new file +2 -0

+
+	return ret;

	return 0;

err_detach_cr:

	tegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);
err_detach_cb:
	tegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);
err_detach_y:
	tegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);

	return ret;

drivers/staging/tegra-vde/TODO

Quoted 8 lines

drivers/staging/tegra-vde/TODO new file +8 -0

+}
+
+static void tegra_vde_deattach_frame_dmabufs(struct video_frame *frame)
+{
+	tegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);
+	tegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);
+	tegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);
+	tegra_vde_detach_and_put_dmabuf(frame->aux_dmabuf_attachment);

It would be happier to write this in the reverse order so it matches the error handling that I wrote for you.

Quoted 7 lines

drivers/staging/tegra-vde/TODO new file +7 -0

+}
+
+static int tegra_vde_copy_and_validate_frame(struct device *dev,
+					     struct tegra_vde_h264_frame *frame,
+					     unsigned long vaddr)
+{
+	if (copy_from_user(frame, (void __user *)vaddr, sizeof(*frame))) {

drivers/staging/tegra-vde/TODO

Quoted 2 lines

drivers/staging/tegra-vde/TODO new file +2 -0

+		dev_err(dev, "Copying of H.264 frame from user failed\n");
+		return -EFAULT;

Error message are a funny thing and different people feel different ways. These can be triggered by the user so they let you spam dmesg but I can see how many of them would be useful. These ones for copy_from_user() are not useful since we assume the programmer should know that stuff. The error code seems enough.

Quoted 117 lines

drivers/staging/tegra-vde/TODO new file +117 -0

+	}
+
+	if (frame->frame_num > 0x7FFFFF) {
+		dev_err(dev, "Bad frame_num %u\n", frame->frame_num);
+		return -EINVAL;
+	}
+
+	if (frame->y_offset & 0xFF) {
+		dev_err(dev, "Bad y_offset 0x%X\n", frame->y_offset);
+		return -EINVAL;
+	}
+
+	if (frame->cb_offset & 0xFF) {
+		dev_err(dev, "Bad cb_offset 0x%X\n", frame->cb_offset);
+		return -EINVAL;
+	}
+
+	if (frame->cr_offset & 0xFF) {
+		dev_err(dev, "Bad cr_offset 0x%X\n", frame->cr_offset);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tegra_vde_validate_h264_ctx(struct device *dev,
+				       struct tegra_vde_h264_decoder_ctx *ctx)
+{
+	if (ctx->dpb_frames_nb == 0 || ctx->dpb_frames_nb > 17) {
+		dev_err(dev, "Bad DPB size %u\n", ctx->dpb_frames_nb);
+		return -EINVAL;
+	}
+
+	if (ctx->level_idc > 15) {
+		dev_err(dev, "Bad level value %u\n", ctx->level_idc);
+		return -EINVAL;
+	}
+
+	if (ctx->pic_init_qp > 52) {
+		dev_err(dev, "Bad pic_init_qp value %u\n", ctx->pic_init_qp);
+		return -EINVAL;
+	}
+
+	if (ctx->log2_max_pic_order_cnt_lsb > 16) {
+		dev_err(dev, "Bad log2_max_pic_order_cnt_lsb value %u\n",
+			ctx->log2_max_pic_order_cnt_lsb);
+		return -EINVAL;
+	}
+
+	if (ctx->log2_max_frame_num > 16) {
+		dev_err(dev, "Bad log2_max_frame_num value %u\n",
+			ctx->log2_max_frame_num);
+		return -EINVAL;
+	}
+
+	if (ctx->chroma_qp_index_offset > 31) {
+		dev_err(dev, "Bad chroma_qp_index_offset value %u\n",
+			ctx->chroma_qp_index_offset);
+		return -EINVAL;
+	}
+
+	if (ctx->pic_order_cnt_type > 2) {
+		dev_err(dev, "Bad pic_order_cnt_type value %u\n",
+			ctx->pic_order_cnt_type);
+		return -EINVAL;
+	}
+
+	if (ctx->num_ref_idx_l0_active_minus1 > 15) {
+		dev_err(dev, "Bad num_ref_idx_l0_active_minus1 value %u\n",
+			ctx->num_ref_idx_l0_active_minus1);
+		return -EINVAL;
+	}
+
+	if (ctx->num_ref_idx_l1_active_minus1 > 15) {
+		dev_err(dev, "Bad num_ref_idx_l1_active_minus1 value %u\n",
+			ctx->num_ref_idx_l1_active_minus1);
+		return -EINVAL;
+	}
+
+	if (!ctx->pic_width_in_mbs || ctx->pic_width_in_mbs > 127) {
+		dev_err(dev, "Bad pic_width_in_mbs value %u, min 1 max 127\n",
+			ctx->pic_width_in_mbs);
+		return -EINVAL;
+	}
+
+	if (!ctx->pic_height_in_mbs || ctx->pic_height_in_mbs > 127) {
+		dev_err(dev, "Bad pic_height_in_mbs value %u, min 1 max 127\n",
+			ctx->pic_height_in_mbs);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
+				       unsigned long vaddr)
+{
+	struct tegra_vde_h264_decoder_ctx ctx;
+	struct tegra_vde_h264_frame frame;
+	struct device *dev = vde->miscdev.parent;
+	struct video_frame *dpb_frames = NULL;
+	struct dma_buf_attachment *bitstream_data_dmabuf_attachment = NULL;
+	enum dma_data_direction dma_dir;
+	phys_addr_t bitstream_data_paddr;
+	phys_addr_t bsev_paddr;
+	u32 bitstream_data_size;
+	u32 macroblocks_nb;
+	bool timeout = false;
+	int i, ret;
+
+	if (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx))) {
+		dev_err(dev, "Copying of H.264 CTX from user failed\n");
+		return -EFAULT;
+	}
+
+	ret = tegra_vde_validate_h264_ctx(dev, &ctx);
+	if (ret)

drivers/staging/tegra-vde/TODO

Quoted 10 lines

drivers/staging/tegra-vde/TODO new file +10 -0

+		return -EINVAL;
+
+	ret = tegra_vde_attach_dmabuf(dev, ctx.bitstream_data_fd,
+				      ctx.bitstream_data_offset, 0,
+				      &bitstream_data_dmabuf_attachment,
+				      &bitstream_data_paddr,
+				      &bitstream_data_size,
+				      DMA_TO_DEVICE);
+	if (ret)
+		goto cleanup;

I hate this label name. What are we cleaning up??? Now I have to set a bookmark so I can remember where I left and then scroll down to the bottom of the function and take a look.

[pause]

OK. I'm back. I call this "one err" style error handling. And it's very bug prone. Please read my essay on the topic:

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

The bug is that we call tegra_vde_detach_and_put_dmabuf() with a NULL pointer and there was that dereference before check that I mentioned earlier.

Quoted 43 lines

drivers/staging/tegra-vde/TODO new file +43 -0

+
+	dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
+			     GFP_KERNEL);
+	if (!dpb_frames) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
+	macroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;
+	vaddr = ctx.dpb_frames_ptr;
+
+	for (i = 0; i < ctx.dpb_frames_nb; i++) {
+		ret = tegra_vde_copy_and_validate_frame(dev, &frame, vaddr);
+		if (ret)
+			goto cleanup;
+
+		dpb_frames[i].flags = frame.flags;
+		dpb_frames[i].frame_num = frame.frame_num;
+
+		dma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+		ret = tegra_vde_attach_frame_dmabufs(dev,
+						     &dpb_frames[i], &frame,
+						     dma_dir,
+						     ctx.is_baseline_profile,
+						     macroblocks_nb * 64);
+		if (ret) {
+			tegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);
+			goto cleanup;
+		}
+
+		vaddr += sizeof(frame);
+	}
+
+	ret = clk_prepare_enable(vde->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clk: %d\n", ret);
+		goto cleanup;
+	}
+
+	ret = mutex_lock_interruptible(&vde->lock);
+	if (ret)
+		goto clkgate;

drivers/staging/tegra-vde/TODO

Quoted 5 lines

drivers/staging/tegra-vde/TODO new file +5 -0

+
+	ret = reset_control_deassert(vde->rst);
+	if (ret) {
+		dev_err(dev, "Failed to deassert reset: %d\n", ret);
+		clk_disable_unprepare(vde->clk);

We do a second clk_disable_unprepare(vde->clk); after the unlock. Delete this?

Quoted 27 lines

drivers/staging/tegra-vde/TODO new file +27 -0

+		goto unlock;
+	}
+
+	ret = tegra_vde_setup_context(vde, &ctx, dpb_frames,
+				      bitstream_data_paddr,
+				      bitstream_data_size,
+				      macroblocks_nb);
+	if (ret)
+		goto reset;
+
+	tegra_vde_decode_frame(vde, macroblocks_nb);
+
+	timeout = !wait_for_completion_io_timeout(&vde->decode_completion,
+						  TEGRA_VDE_TIMEOUT);
+	if (timeout) {
+		bsev_paddr = readl(vde->regs + BSEV(0x10));
+		macroblocks_nb = readl(vde->regs + SXE(0xC8)) & 0x1FFF;
+
+		dev_err(dev, "Decoding failed, "
+				"read 0x%X bytes : %u macroblocks parsed\n",
+			bsev_paddr ? bsev_paddr - bitstream_data_paddr : 0,
+			macroblocks_nb);
+	}
+
+reset:
+	/*
+	 * We rely on the VDE registers reset value, otherwise VDE would

drivers/staging/tegra-vde/TODO

Quoted 5 lines

drivers/staging/tegra-vde/TODO new file +5 -0

+	 * cause bus lockup.
+	 */
+	ret = reset_control_assert(vde->rst);
+	if (ret)
+		dev_err(dev, "Failed to assert reset: %d\n", ret);

We're overwriting "ret" here when we probably want to preserve the error code from reset_control_deassert().

Quoted 93 lines

drivers/staging/tegra-vde/TODO new file +93 -0

+
+	if (timeout)
+		ret = -EIO;
+
+unlock:
+	mutex_unlock(&vde->lock);
+
+clkgate:
+	clk_disable_unprepare(vde->clk);
+
+cleanup:
+	if (dpb_frames)
+		while (i--)
+			tegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);
+
+	kfree(dpb_frames);
+
+	tegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment);
+
+	return ret;
+}
+
+static long tegra_vde_unlocked_ioctl(struct file *filp,
+				     unsigned int cmd, unsigned long arg)
+{
+	struct miscdevice *miscdev = filp->private_data;
+	struct tegra_vde *vde = container_of(miscdev, struct tegra_vde,
+					     miscdev);
+
+	switch (cmd) {
+	case TEGRA_VDE_IOCTL_DECODE_H264:
+		return tegra_vde_ioctl_decode_h264(vde, arg);
+	}
+
+	dev_err(miscdev->parent, "Invalid IOCTL command %u\n", cmd);
+
+	return -ENOTTY;
+}
+
+static const struct file_operations tegra_vde_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tegra_vde_unlocked_ioctl,
+};
+
+static irqreturn_t tegra_vde_isr(int irq, void *data)
+{
+	struct tegra_vde *vde = data;
+
+	tegra_vde_set_bits(vde->regs, 0, FRAMEID(0x208));
+	complete(&vde->decode_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int tegra_vde_probe(struct platform_device *pdev)
+{
+	struct resource *res_regs, *res_iram;
+	struct device *dev = &pdev->dev;
+	struct tegra_vde *vde;
+	int ret;
+
+	vde = devm_kzalloc(&pdev->dev, sizeof(*vde), GFP_KERNEL);
+	if (!vde)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, vde);
+
+	res_regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	if (!res_regs)
+		return -ENODEV;
+
+	res_iram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "iram");
+	if (!res_iram)
+		return -ENODEV;
+
+	vde->irq = platform_get_irq_byname(pdev, "sync-token");
+	if (vde->irq < 0)
+		return vde->irq;
+
+	vde->regs = devm_ioremap_resource(dev, res_regs);
+	if (IS_ERR(vde->regs))
+		return PTR_ERR(vde->regs);
+
+	vde->iram = devm_ioremap_resource(dev, res_iram);
+	if (IS_ERR(vde->iram))
+		return PTR_ERR(vde->iram);
+
+	vde->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(vde->clk)) {
+		dev_err(dev, "Could not get VDE clk\n");
+		return PTR_ERR(vde->clk);
+	}
+

drivers/staging/tegra-vde/TODO

Quoted 10 lines

drivers/staging/tegra-vde/TODO new file +10 -0

+	vde->rst = devm_reset_control_get(dev, "vde");
+	if (IS_ERR(vde->rst)) {
+		dev_err(dev, "Could not get VDE reset\n");
+		return PTR_ERR(vde->rst);
+	}
+
+	ret = devm_request_irq(dev, vde->irq, tegra_vde_isr, IRQF_SHARED,
+			       dev_name(dev), vde);
+	if (ret)
+		return -ENODEV;

Presever the error code.

Quoted 18 lines

drivers/staging/tegra-vde/TODO new file +18 -0

+
+	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,
+						vde->clk, vde->rst);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to power up VDE unit\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(vde->rst);
+	if (ret) {
+		dev_err(dev, "Failed to assert reset: %d\n", ret);
+		return ret;
+	}
+
+	clk_disable_unprepare(vde->clk);
+
+	mutex_init(&vde->lock);
+	init_completion(&vde->decode_completion);

drivers/staging/tegra-vde/TODO

Quoted 10 lines

drivers/staging/tegra-vde/TODO new file +10 -0

+
+	vde->iram_lists_paddr = res_iram->start;
+	vde->miscdev.minor = MISC_DYNAMIC_MINOR;
+	vde->miscdev.name = "tegra_vde";
+	vde->miscdev.fops = &tegra_vde_fops;
+	vde->miscdev.parent = dev;
+
+	ret = misc_register(&vde->miscdev);
+	if (ret)
+		return -ENODEV;

Preserve the error code.

Quoted 4 lines

drivers/staging/tegra-vde/TODO new file +4 -0

+
+	return 0;
+}
+

Quoted 7 lines

drivers/staging/tegra-vde/vde.c line 577

Quoted 2 lines

drivers/staging/tegra-vde/vde.c new file +2 -0

+	if (is_baseline_profile)
+		frame->aux_paddr = 0xF4DEAD00;

The handling of is_baseline_profile is strange to me. It feels like we should always check it before we use ->aux_paddr but we don't ever do that.

In a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux phys address is set to a predefined and invalid address, so that in a case of VDE trying to use it, its invalid memory accesses would be reported in KMSG by memory controller driver and the reported invalid addresses would be known to be associated with the aux buffer. I'm not sure what you are meaning.

Quoted 8 lines · Dmitry Osipenko

Quoted 7 lines

drivers/staging/tegra-vde/vde.c line 577

Quoted 2 lines

drivers/staging/tegra-vde/vde.c new file +2 -0

+	if (is_baseline_profile)
+		frame->aux_paddr = 0xF4DEAD00;

The handling of is_baseline_profile is strange to me. It feels like we should always check it before we use ->aux_paddr but we don't ever do that.

Quoted 5 lines

In a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux phys address is set to a predefined and invalid address, so that in a case of VDE trying to use it, its invalid memory accesses would be reported in KMSG by memory controller driver and the reported invalid addresses would be known to be associated with the aux buffer. I'm not sure what you are meaning.

It's not used perhaps, but we do write it to the hardware, right?

	tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);

It's just strange.

drivers/staging/tegra-vde/Kconfig line 3

Quoted 6 lines

drivers/staging/tegra-vde/Kconfig new file +3 -0

@@ -0,0 +1,6 @@
+config TEGRA_VDE
+	tristate "NVIDIA Tegra20 video decoder driver"
+	depends on ARCH_TEGRA_2x_SOC

Could we get a || COMPILE_TEST here as well?

This driver provides accelerated video decoding to NVIDIA Tegra20 SoC's, it is a result of reverse-engineering efforts. Driver has been tested on Toshiba AC100 and Acer A500, it should work on any Tegra20 device.

In userspace this driver is utilized by libvdpau-tegra [0] that implements VDPAU interface, so any video player that supports VDPAU can provide accelerated video decoding on Tegra20 on Linux.

Quoted 4 lines · Dmitry Osipenko

This driver provides accelerated video decoding to NVIDIA Tegra20 SoC's, it is a result of reverse-engineering efforts. Driver has been tested on Toshiba AC100 and Acer A500, it should work on any Tegra20 device.

Quoted 3 lines

In userspace this driver is utilized by libvdpau-tegra [0] that implements VDPAU interface, so any video player that supports VDPAU can provide accelerated video decoding on Tegra20 on Linux.

Why not use the v4l2 api instead? Doesn't that provide the same needed user/kernel api here instead of creating yet-another-custom ioctl?

Quoted 5 lines · Greg Kroah-Hartman

Quoted 4 lines · Dmitry Osipenko

This driver provides accelerated video decoding to NVIDIA Tegra20 SoC's, it is a result of reverse-engineering efforts. Driver has been tested on Toshiba AC100 and Acer A500, it should work on any Tegra20 device.

Quoted 7 lines

Quoted 3 lines

In userspace this driver is utilized by libvdpau-tegra [0] that implements VDPAU interface, so any video player that supports VDPAU can provide accelerated video decoding on Tegra20 on Linux.

Why not use the v4l2 api instead? Doesn't that provide the same needed user/kernel api here instead of creating yet-another-custom ioctl?

1) The HW doesn't generalize for the common API. Like for example, it isn't capable of unpacking bitstream encoded with CABAC (Context-adaptive binary arithmetic coding), so unpacking should be done in software and then VDE HW isn't capable of decoding such a stream in a fully-automated manner, software would have to feed engine with a chunks of macroblocks untill the whole frame is decoded. That lameness is partially hidden in the BLOB's firmware, that firmware actually is just a driver BTW.

2) We want to have decoding integrated with the presentation of the decoded video frame. So having v4l interface for decoding would be just an extra unnecessary shim, increasing CPU / memory resources usage and complexity of the code.

3) The decoding and presentation are already implemented using VDPAU API and proven to work decently in that way.

Quoted 27 lines · Dmitry Osipenko

Quoted 5 lines · Greg Kroah-Hartman

Quoted 4 lines · Dmitry Osipenko

This driver provides accelerated video decoding to NVIDIA Tegra20 SoC's, it is a result of reverse-engineering efforts. Driver has been tested on Toshiba AC100 and Acer A500, it should work on any Tegra20 device.

Quoted 7 lines

Quoted 3 lines

In userspace this driver is utilized by libvdpau-tegra [0] that implements VDPAU interface, so any video player that supports VDPAU can provide accelerated video decoding on Tegra20 on Linux.

Why not use the v4l2 api instead? Doesn't that provide the same needed user/kernel api here instead of creating yet-another-custom ioctl?

1) The HW doesn't generalize for the common API. Like for example, it isn't capable of unpacking bitstream encoded with CABAC (Context-adaptive binary arithmetic coding), so unpacking should be done in software and then VDE HW isn't capable of decoding such a stream in a fully-automated manner, software would have to feed engine with a chunks of macroblocks untill the whole frame is decoded. That lameness is partially hidden in the BLOB's firmware, that firmware actually is just a driver BTW.

2) We want to have decoding integrated with the presentation of the decoded video frame. So having v4l interface for decoding would be just an extra unnecessary shim, increasing CPU / memory resources usage and complexity of the code.

Quoted 2 lines

3) The decoding and presentation are already implemented using VDPAU API and proven to work decently in that way.

This sounds like something you should be talking over with the media driver developers, why are they not even cc:ed on this submission?

I need their ack on this new api before I can take this.