subject:"Re\: \[PATCH 1\/3\] \[media\] v4l\: tegra\: Add NVIDIA Tegra VI driver"

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-12-10 Thread Hans Verkuil

Hi Bryan,

I came across a few bugs, see comments in the code:

On 11/11/15 20:50, Bryan Wu wrote:
> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
> controller which can support up to 6 MIPI CSI camera sensors.
> 
> This patch adds a V4L2 media controller and capture driver to support
> Tegra VI hardware. It's verified with Tegra built-in test pattern
> generator.
> 
> Signed-off-by: Bryan Wu 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/tegra/Kconfig |  10 +
>  drivers/media/platform/tegra/Makefile|   3 +
>  drivers/media/platform/tegra/tegra-channel.c | 849 
> +++
>  drivers/media/platform/tegra/tegra-core.c| 254 
>  drivers/media/platform/tegra/tegra-core.h| 162 +
>  drivers/media/platform/tegra/tegra-csi.c | 560 ++
>  drivers/media/platform/tegra/tegra-vi.c  | 732 +++
>  drivers/media/platform/tegra/tegra-vi.h  | 213 +++
>  10 files changed, 2786 insertions(+)
>  create mode 100644 drivers/media/platform/tegra/Kconfig
>  create mode 100644 drivers/media/platform/tegra/Makefile
>  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.h
>  create mode 100644 drivers/media/platform/tegra/tegra-csi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.h
> 



> diff --git a/drivers/media/platform/tegra/tegra-channel.c 
> b/drivers/media/platform/tegra/tegra-channel.c
> new file mode 100644
> index 000..63eb942
> --- /dev/null
> +++ b/drivers/media/platform/tegra/tegra-channel.c
> @@ -0,0 +1,849 @@



> +void tegra_channel_fmts_bitmap_init(struct tegra_channel *chan,
> + struct tegra_vi_graph_entity *entity)
> +{
> + int ret, index;
> + struct v4l2_subdev *subdev = entity->subdev;
> + struct v4l2_subdev_mbus_code_enum code = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> +
> + bitmap_zero(chan->fmts_bitmap, MAX_FORMAT_NUM);
> +
> + while (1) {
> + ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
> +NULL, );
> + if (ret < 0)
> + /* no more formats */
> + break;
> +
> + index = tegra_core_get_idx_by_code(code.code);
> + if (index >= 0)
> + bitmap_set(chan->fmts_bitmap, index, 1);
> +
> + code.index++;
> + }
> +}

See comments below in tegra_channel_init().



> +static int
> +tegra_channel_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc 
> *f)
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct tegra_channel *chan = to_tegra_channel(vfh->vdev);
> + unsigned int index = 0, i;
> + unsigned long *fmts_bitmap = NULL;
> +
> + if (chan->vi->pg_mode)
> + fmts_bitmap = chan->vi->tpg_fmts_bitmap;
> + else
> + fmts_bitmap = chan->fmts_bitmap;
> +
> + if (f->index > bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM) - 1)

This should be:

if (f->index >= bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM))

If fmts_bitmap is all zeroes then this condition worked out to 'if (0 > -1)'
where 0 is unsigned, so this goes into an infinite loop.

> + return -EINVAL;
> +
> + for (i = 0; i < f->index + 1; i++, index++)
> + index = find_next_bit(fmts_bitmap, MAX_FORMAT_NUM, index);
> +
> + f->pixelformat = tegra_core_get_fourcc_by_idx(index - 1);
> +
> + return 0;
> +}



> +int tegra_channel_init(struct tegra_vi *vi, unsigned int port)
> +{
> + int ret;
> + struct tegra_channel *chan  = >chans[port];
> +
> + chan->vi = vi;
> + chan->port = port;
> +
> + /* Init channel register base */
> + chan->csi = vi->iomem + TEGRA_VI_CSI_BASE(port);
> +
> + /* VI Channel is 64 bytes alignment */
> + chan->align = 64;
> + chan->io_id = tegra_io_rail_csi_ids[chan->port];
> +
> + mutex_init(>video_lock);
> + INIT_LIST_HEAD(>capture);
> + INIT_LIST_HEAD(>done);
> + spin_lock_init(>start_lock);
> + spin_lock_init(>done_lock);
> + init_waitqueue_head(>start_wait);
> + init_waitqueue_head(>done_wait);
> +
> + /* Init video format */
> + chan->fmtinfo = tegra_core_get_format_by_code(TEGRA_VF_DEF);
> + chan->format.pixelformat = chan->fmtinfo->fourcc;
> + chan->format.colorspace = V4L2_COLORSPACE_SRGB;
> + chan->format.field = V4L2_FIELD_NONE;
> + chan->format.width = TEGRA_DEF_WIDTH;
> + chan->format.height = TEGRA_DEF_HEIGHT;
> + chan->format.bytesperline = chan->format.width * chan->fmtinfo->bpp;
> +

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-09-22 Thread Bryan Wu


On 09/22/2015 04:47 AM, Thierry Reding wrote:

On Mon, Sep 21, 2015 at 11:55:53AM -0700, Bryan Wu wrote:
[...]

+static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+   struct tegra_csi_device *csi = to_csi(subdev);
+   struct tegra_channel *chan = subdev->host_priv;
+   enum tegra_csi_port_num port_num = (chan->port & 1) ? PORT_B : PORT_A;
+   struct tegra_csi_port *port = >ports[port_num];
+   int ret;
+
+   if (enable) {

[...]

+   } else {
+   u32 val = pp_read(port, TEGRA_CSI_PIXEL_PARSER_STATUS);
+   dev_dbg(csi->dev, "TEGRA_CSI_PIXEL_PARSER_STATUS 0x%08x\n", 
val);
+
+   val = cil_read(port, TEGRA_CSI_CIL_STATUS);
+   dev_dbg(csi->dev, "TEGRA_CSI_CIL_STATUS 0x%08x\n", val);
+
+   val = cil_read(port, TEGRA_CSI_CILX_STATUS);
+   dev_dbg(csi->dev, "TEGRA_CSI_CILX_STATUS 0x%08x\n", val);
+   

I was going to apply this and give it a spin, but then git am complained
about trailing whitespace above...


+#ifdef DEBUG
+   val = csi_read(csi, TEGRA_CSI_DEBUG_COUNTER_0);
+   dev_err(>dev, "TEGRA_CSI_DEBUG_COUNTER_0 0x%08x\n", val);
+#endif
+
+   pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
+(0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
+CSI_PP_DISABLE);
+
+   clk_disable_unprepare(csi->clk);
+   }
+   

and here, ...


+static int tegra_csi_probe(struct platform_device *pdev)
+{

[...]

+   for (i = 0; i < TEGRA_CSI_PORTS_NUM; i++) {
+   /* Initialize the default format */
+   csi->ports[i].format.code = TEGRA_VF_DEF;
+   csi->ports[i].format.field = V4L2_FIELD_NONE;
+   csi->ports[i].format.colorspace = V4L2_COLORSPACE_SRGB;
+   csi->ports[i].format.width = TEGRA_DEF_WIDTH;
+   csi->ports[i].format.height = TEGRA_DEF_HEIGHT;
+
+   /* Initialize port register bases */
+   csi->ports[i].pixel_parser = csi->iomem +
+(i & 1) * TEGRA_CSI_PORT_OFFSET;
+   csi->ports[i].cil = csi->iomem + TEGRA_CSI_CIL_OFFSET +

here and...


+   (i & 1) * TEGRA_CSI_PORT_OFFSET;
+   csi->ports[i].tpg = csi->iomem + TEGRA_CSI_TPG_OFFSET +

here.

Might be worth fixing those up if you'll respin anyway.

Thierry
Thanks for pointing out this, I will rerun the check-patch.pl and fix 
all those coding style errors.


-Bryan

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-09-22 Thread Hans Verkuil

Hi Bryan,

Thanks for this v3 patch series. It looks very good now. I have a few comments,
I think they are trivial to add and then I would just wait for the new MC code
to be merged. I hope it will be soon, but it's a bit unpredictable.

On 21-09-15 20:55, Bryan Wu wrote:
> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
> controller which can support up to 6 MIPI CSI camera sensors.
> 
> This patch adds a V4L2 media controller and capture driver to support
> Tegra VI hardware. It's verified with Tegra built-in test pattern
> generator.
> 
> Signed-off-by: Bryan Wu 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/tegra/Kconfig |  10 +
>  drivers/media/platform/tegra/Makefile|   3 +
>  drivers/media/platform/tegra/tegra-channel.c | 782 
> +++
>  drivers/media/platform/tegra/tegra-core.c| 252 +
>  drivers/media/platform/tegra/tegra-core.h| 162 ++
>  drivers/media/platform/tegra/tegra-csi.c | 566 +++
>  drivers/media/platform/tegra/tegra-vi.c  | 581 
>  drivers/media/platform/tegra/tegra-vi.h  | 209 +++
>  10 files changed, 2568 insertions(+)
>  create mode 100644 drivers/media/platform/tegra/Kconfig
>  create mode 100644 drivers/media/platform/tegra/Makefile
>  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.h
>  create mode 100644 drivers/media/platform/tegra/tegra-csi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index f6bed19..553867f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -119,6 +119,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
>  source "drivers/media/platform/s5p-tv/Kconfig"
>  source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
> +source "drivers/media/platform/tegra/Kconfig"
>  
>  endif # V4L_PLATFORM_DRIVERS
>  



> diff --git a/drivers/media/platform/tegra/tegra-channel.c 
> b/drivers/media/platform/tegra/tegra-channel.c
> new file mode 100644
> index 000..37a7017
> --- /dev/null
> +++ b/drivers/media/platform/tegra/tegra-channel.c
> @@ -0,0 +1,782 @@
> +/*
> + * NVIDIA Tegra Video Input Device
> + *
> + * Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author: Bryan Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "tegra-vi.h"
> +
> +#define TEGRA_VI_SYNCPT_WAIT_TIMEOUT 200
> +
> +/* VI registers */
> +#define TEGRA_VI_CFG_VI_INCR_SYNCPT  0x000
> +#define   VI_CFG_VI_INCR_SYNCPT_COND(x)  (((x) & 0xff) 
> << 8)
> +#define   VI_CSI_PP_LINE_START(port) (4 + (port) * 4)
> +#define   VI_CSI_PP_FRAME_START(port)(5 + (port) * 4)
> +#define   VI_CSI_MW_REQ_DONE(port)   (6 + (port) * 4)
> +#define   VI_CSI_MW_ACK_DONE(port)   (7 + (port) * 4)
> +
> +#define TEGRA_VI_CFG_VI_INCR_SYNCPT_CNTRL0x004
> +#define TEGRA_VI_CFG_VI_INCR_SYNCPT_ERROR0x008
> +#define TEGRA_VI_CFG_CTXSW   0x020
> +#define TEGRA_VI_CFG_INTSTATUS   0x024
> +#define TEGRA_VI_CFG_PWM_CONTROL 0x038
> +#define TEGRA_VI_CFG_PWM_HIGH_PULSE  0x03c
> +#define TEGRA_VI_CFG_PWM_LOW_PULSE   0x040
> +#define TEGRA_VI_CFG_PWM_SELECT_PULSE_A  0x044
> +#define TEGRA_VI_CFG_PWM_SELECT_PULSE_B  0x048
> +#define TEGRA_VI_CFG_PWM_SELECT_PULSE_C  0x04c
> +#define TEGRA_VI_CFG_PWM_SELECT_PULSE_D  0x050
> +#define TEGRA_VI_CFG_VGP10x064
> +#define TEGRA_VI_CFG_VGP20x068
> +#define TEGRA_VI_CFG_VGP30x06c
> +#define TEGRA_VI_CFG_VGP40x070
> +#define TEGRA_VI_CFG_VGP50x074
> +#define TEGRA_VI_CFG_VGP60x078
> +#define TEGRA_VI_CFG_INTERRUPT_MASK  0x08c
> +#define

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-09-22 Thread Thierry Reding

On Mon, Sep 21, 2015 at 11:55:53AM -0700, Bryan Wu wrote:
[...]
> +static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
> +{
> + struct tegra_csi_device *csi = to_csi(subdev);
> + struct tegra_channel *chan = subdev->host_priv;
> + enum tegra_csi_port_num port_num = (chan->port & 1) ? PORT_B : PORT_A;
> + struct tegra_csi_port *port = >ports[port_num];
> + int ret;
> +
> + if (enable) {
[...]
> + } else {
> + u32 val = pp_read(port, TEGRA_CSI_PIXEL_PARSER_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_PIXEL_PARSER_STATUS 0x%08x\n", 
> val);
> +
> + val = cil_read(port, TEGRA_CSI_CIL_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_CIL_STATUS 0x%08x\n", val);
> +
> + val = cil_read(port, TEGRA_CSI_CILX_STATUS);
> + dev_dbg(csi->dev, "TEGRA_CSI_CILX_STATUS 0x%08x\n", val);
> + 

I was going to apply this and give it a spin, but then git am complained
about trailing whitespace above...

> +#ifdef DEBUG
> + val = csi_read(csi, TEGRA_CSI_DEBUG_COUNTER_0);
> + dev_err(>dev, "TEGRA_CSI_DEBUG_COUNTER_0 0x%08x\n", val);
> +#endif
> +
> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
> +  (0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
> +  CSI_PP_DISABLE);
> +
> + clk_disable_unprepare(csi->clk);
> + }
> + 

and here, ...

> +static int tegra_csi_probe(struct platform_device *pdev)
> +{
[...]
> + for (i = 0; i < TEGRA_CSI_PORTS_NUM; i++) {
> + /* Initialize the default format */
> + csi->ports[i].format.code = TEGRA_VF_DEF;
> + csi->ports[i].format.field = V4L2_FIELD_NONE;
> + csi->ports[i].format.colorspace = V4L2_COLORSPACE_SRGB;
> + csi->ports[i].format.width = TEGRA_DEF_WIDTH;
> + csi->ports[i].format.height = TEGRA_DEF_HEIGHT;
> +
> + /* Initialize port register bases */
> + csi->ports[i].pixel_parser = csi->iomem +
> +  (i & 1) * TEGRA_CSI_PORT_OFFSET;
> + csi->ports[i].cil = csi->iomem + TEGRA_CSI_CIL_OFFSET + 

here and...

> + (i & 1) * TEGRA_CSI_PORT_OFFSET;
> + csi->ports[i].tpg = csi->iomem + TEGRA_CSI_TPG_OFFSET + 

here.

Might be worth fixing those up if you'll respin anyway.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-09-21 Thread Bryan Wu


On 09/16/2015 02:13 AM, Hans Verkuil wrote:

Hi Bryan,

Thanks for this patch series!

The switch to a kthread helps a lot, but I still have a number of comments
about it, primarily locking related.

On 09/16/2015 03:35 AM, Bryan Wu wrote:

NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
controller which can support up to 6 MIPI CSI camera sensors.

This patch adds a V4L2 media controller and capture driver to support
Tegra VI hardware. It's verified with Tegra built-in test pattern
generator.

Signed-off-by: Bryan Wu
Reviewed-by: Hans Verkuil
---
  drivers/media/platform/Kconfig   |   1 +
  drivers/media/platform/Makefile  |   2 +
  drivers/media/platform/tegra/Kconfig |  10 +
  drivers/media/platform/tegra/Makefile|   3 +
  drivers/media/platform/tegra/tegra-channel.c | 802 +++
  drivers/media/platform/tegra/tegra-core.c| 252 +
  drivers/media/platform/tegra/tegra-core.h| 162 ++
  drivers/media/platform/tegra/tegra-csi.c | 566 +++
  drivers/media/platform/tegra/tegra-vi.c  | 581 +++
  drivers/media/platform/tegra/tegra-vi.h  | 213 +++
  10 files changed, 2592 insertions(+)
  create mode 100644 drivers/media/platform/tegra/Kconfig
  create mode 100644 drivers/media/platform/tegra/Makefile
  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
  create mode 100644 drivers/media/platform/tegra/tegra-core.c
  create mode 100644 drivers/media/platform/tegra/tegra-core.h
  create mode 100644 drivers/media/platform/tegra/tegra-csi.c
  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
  create mode 100644 drivers/media/platform/tegra/tegra-vi.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index f6bed19..553867f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -119,6 +119,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
  source "drivers/media/platform/s5p-tv/Kconfig"
  source "drivers/media/platform/am437x/Kconfig"
  source "drivers/media/platform/xilinx/Kconfig"
+source "drivers/media/platform/tegra/Kconfig"
  
  endif # V4L_PLATFORM_DRIVERS
  




diff --git a/drivers/media/platform/tegra/tegra-channel.c 
b/drivers/media/platform/tegra/tegra-channel.c
new file mode 100644
index 000..8149016
--- /dev/null
+++ b/drivers/media/platform/tegra/tegra-channel.c
@@ -0,0 +1,802 @@




+static int tegra_channel_capture_frame(struct tegra_channel *chan)
+{
+   struct tegra_channel_buffer *buf = chan->active;

I think this can just be passed as an argument instead of being a field
in the chan struct. I'm not 100% certain, you'd have to check this.

Good point. I will remove the active of chan and pass a buffer pointer here.


+   struct vb2_buffer *vb = >buf;
+   int err = 0;
+   u32 thresh, value, frame_start;
+   int bytes_per_line = chan->format.bytesperline;
+
+   if (!vb2_start_streaming_called(>queue) || !buf)
+   return -EINVAL;

Can this ever happen? Perhaps this should be using WARN_ON?

This check should be redundant. Let me remove them.



+
+   /* Program buffer address by using surface 0 */
+   csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, 0x0);
+   csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
+   csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
+
+   /* Program syncpoint */
+   frame_start = VI_CSI_PP_FRAME_START(chan->port);
+   value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
+   host1x_syncpt_id(chan->sp);
+   tegra_channel_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
+
+   csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
+
+   /* Use syncpoint to wake up */
+   thresh = host1x_syncpt_incr_max(chan->sp, 1);
+   err = host1x_syncpt_wait(chan->sp, thresh,
+TEGRA_VI_SYNCPT_WAIT_TIMEOUT, );
+   if (err) {
+   dev_err(>video.dev, "frame start syncpt timeout!\n");
+   tegra_channel_capture_error(chan);
+   }
+
+   /* Captured one frame */
+   spin_lock_irq(>queued_lock);

Why use spinlock_irq? You're not in interrupt context. A normal spinlock is
sufficient. Or perhaps the mutex_lock is enough (this function is already
called with that lock held).


I will review the locking code here. looks like mutex_lock(chan->lock) 
is not required here. Or just keep one spinlock should be OK.



+   vb->v4l2_buf.sequence = chan->sequence++;
+   vb->v4l2_buf.field = V4L2_FIELD_NONE;

Add a comment here that this will need to be changed if you ever start 
supporting
interlaced formats.


Sure. fixed.


+   v4l2_get_timestamp(>v4l2_buf.timestamp);
+   vb2_set_plane_payload(vb, 0, chan->format.sizeimage);
+   vb2_buffer_done(vb, err < 0 ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
+

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

2015-09-16 Thread Hans Verkuil

Hi Bryan,

Thanks for this patch series!

The switch to a kthread helps a lot, but I still have a number of comments
about it, primarily locking related.

On 09/16/2015 03:35 AM, Bryan Wu wrote:
> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
> controller which can support up to 6 MIPI CSI camera sensors.
> 
> This patch adds a V4L2 media controller and capture driver to support
> Tegra VI hardware. It's verified with Tegra built-in test pattern
> generator.
> 
> Signed-off-by: Bryan Wu 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/tegra/Kconfig |  10 +
>  drivers/media/platform/tegra/Makefile|   3 +
>  drivers/media/platform/tegra/tegra-channel.c | 802 
> +++
>  drivers/media/platform/tegra/tegra-core.c| 252 +
>  drivers/media/platform/tegra/tegra-core.h| 162 ++
>  drivers/media/platform/tegra/tegra-csi.c | 566 +++
>  drivers/media/platform/tegra/tegra-vi.c  | 581 +++
>  drivers/media/platform/tegra/tegra-vi.h  | 213 +++
>  10 files changed, 2592 insertions(+)
>  create mode 100644 drivers/media/platform/tegra/Kconfig
>  create mode 100644 drivers/media/platform/tegra/Makefile
>  create mode 100644 drivers/media/platform/tegra/tegra-channel.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.c
>  create mode 100644 drivers/media/platform/tegra/tegra-core.h
>  create mode 100644 drivers/media/platform/tegra/tegra-csi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.c
>  create mode 100644 drivers/media/platform/tegra/tegra-vi.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index f6bed19..553867f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -119,6 +119,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
>  source "drivers/media/platform/s5p-tv/Kconfig"
>  source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
> +source "drivers/media/platform/tegra/Kconfig"
>  
>  endif # V4L_PLATFORM_DRIVERS
>  



> diff --git a/drivers/media/platform/tegra/tegra-channel.c 
> b/drivers/media/platform/tegra/tegra-channel.c
> new file mode 100644
> index 000..8149016
> --- /dev/null
> +++ b/drivers/media/platform/tegra/tegra-channel.c
> @@ -0,0 +1,802 @@



> +static int tegra_channel_capture_frame(struct tegra_channel *chan)
> +{
> + struct tegra_channel_buffer *buf = chan->active;

I think this can just be passed as an argument instead of being a field
in the chan struct. I'm not 100% certain, you'd have to check this.

> + struct vb2_buffer *vb = >buf;
> + int err = 0;
> + u32 thresh, value, frame_start;
> + int bytes_per_line = chan->format.bytesperline;
> +
> + if (!vb2_start_streaming_called(>queue) || !buf)
> + return -EINVAL;

Can this ever happen? Perhaps this should be using WARN_ON?

> +
> + /* Program buffer address by using surface 0 */
> + csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, 0x0);
> + csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
> + csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
> +
> + /* Program syncpoint */
> + frame_start = VI_CSI_PP_FRAME_START(chan->port);
> + value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
> + host1x_syncpt_id(chan->sp);
> + tegra_channel_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> + csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
> +
> + /* Use syncpoint to wake up */
> + thresh = host1x_syncpt_incr_max(chan->sp, 1);
> + err = host1x_syncpt_wait(chan->sp, thresh,
> +  TEGRA_VI_SYNCPT_WAIT_TIMEOUT, );
> + if (err) {
> + dev_err(>video.dev, "frame start syncpt timeout!\n");
> + tegra_channel_capture_error(chan);
> + }
> +
> + /* Captured one frame */
> + spin_lock_irq(>queued_lock);

Why use spinlock_irq? You're not in interrupt context. A normal spinlock is
sufficient. Or perhaps the mutex_lock is enough (this function is already
called with that lock held).

> + vb->v4l2_buf.sequence = chan->sequence++;
> + vb->v4l2_buf.field = V4L2_FIELD_NONE;

Add a comment here that this will need to be changed if you ever start 
supporting
interlaced formats.

> + v4l2_get_timestamp(>v4l2_buf.timestamp);
> + vb2_set_plane_payload(vb, 0, chan->format.sizeimage);
> + vb2_buffer_done(vb, err < 0 ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> + spin_unlock_irq(>queued_lock);
> +
> + return err;
> +}
> +
> +static int tegra_channel_kthread_capture(void *data)
> +{
> + struct tegra_channel *chan = data;
> +
> + set_freezable();
> +
> + while (1) {
> +

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

Re: [PATCH 1/3] [media] v4l: tegra: Add NVIDIA Tegra VI driver

6 matches

Site Navigation

Mail list logo

Footer information