Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.  Does the driver Oops all the
time or what?

Anyway, minor comments inline.

On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:
> diff --git a/drivers/staging/tegra-vde/Kconfig b/drivers/staging/tegra-vde/Kconfig
> new file mode 100644
> index 000000000000..b947c012a373
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Kconfig
> @@ -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?

> +	help
> +	    Say Y here to enable support for a NVIDIA Tegra20 video decoder
> +	    driver.
> diff --git a/drivers/staging/tegra-vde/Makefile b/drivers/staging/tegra-vde/Makefile
> new file mode 100644
> index 000000000000..e7c0df1174bf
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TEGRA_VDE)	+= vde.o
> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO
> new file mode 100644
> index 000000000000..533ddfc5190e
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/TODO
> @@ -0,0 +1,8 @@
> +All TODO's require reverse-engineering to be done first, it is very
> +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.

> +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));
> +
> +	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.

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

Same for all.

> +		return -EIO;
> +
> +	if (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))
> +		return -EIO;
> +
> +	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.

> +	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));
> +	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.

> +		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));
> +}
> +
> +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.

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

s/Sparsed/Sparse/

> +		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;
	}



> +	}
> +
> +	*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;

> +}
> +
> +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)
> +		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.

> +	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:


> +
> +	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;


> +}
> +
> +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.


> +}
> +
> +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))) {
> +		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.

> +	}
> +
> +	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)
> +		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.

> +
> +	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;
> +
> +	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?

> +		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
> +	 * 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().

> +
> +	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);
> +	}
> +
> +	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.

> +
> +	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);
> +
> +	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.

> +
> +	return 0;
> +}
> +

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux