[RFC PATCH v6 0/9] Add Tegra driver for video capture

[RFC PATCH v6 0/9] Add Tegra driver for video capture

[Sowjanya Komatineni]

This series adds Tegra210 VI and CSI driver for built-in test pattern
generator (TPG) capture.

Tegra210 supports max 6 channels on VI and 6 ports on CSI where each
CSI port is one-to-one mapped to VI channel for video capture.

This series has TPG support only where it creates hard media links
between CSI subdevice and VI video device without device graphs.

v4l2-compliance results are available below the patch diff.

[v6]:	Includes,
	- v5 feedback
	- fix for csi_tpg clock parent
	- fix to free channel resources in video device release callback
	  for registered video devices as resource might still be in use
	  when application holds handle to it during driver unbind.
	- added blanking intervals based on resolution and bpp for csi
	  internal tpg.
	- added implementation for subdev pad ops enum_frame_size and
	  enum_frame_interval.

[v5]:	Includes,
	- v4 feedback
	- fix for venc powergate mc reset order.
	- fix to have unbind and bind work during v4l2-ctl sleep and streaming.

[v4]:	Includes,
	- v3 feedback changes and some improvements
	- Fixes tegra_channel_buffer struct to use v4l2 buffer as first
	  member. This also fixes crash of unable to handle kernel write
	  to read-only memory.
	- Uses separate host1x sync ids for frame start and memory write
	  ack as single sync id for both can cause sync loss between exact
	  frame start and memory write ack events.
	- Uses client managed host1x syncpoints.
	- Includes fix to increment syncpoint counter to match cached value
	  to synchronize in case of timeouts or missed hardware triggers.
	- Frame start and memory write ack syncpoint FIFO's are of size 2.
	  So, updated capture logic to avoid adding more than 2 sync point
	  condition requests to FIFOs to avoid overflow.
	- Implemented PM ops for runtime suspend and resume along with generic
	  power domains to allow proper power gate and ungate sequencing along
	  with MC VI flush during power gate.
	- Fixed Tegra210 device tree sor power domain clocks.
	- Added missing reset-cells to mc node.

[v3]:	Includes,
	- video device node handling set/get formats of all devices
	  in the pipeline.
	- Removed subdev nodes.
	- Fixed frame sync timeout issue due to CSI clocks not properly
	  set for corresponding blocks.
	- uses minimum 3 buffers to be queued to fixed memory race between
	  DMA writes and userspace reads causing kernel hang reporting
	  kernel write to read-only memory.
	- Improved capture threads and done threads to avoid possible
	  race conditions and added recovery in case of frame sync timeout.
	- Passes all the V4L compliance tests.

[v2]:	Includes,
	- v0 feedback
	- Merged files to have Tegra specific separately
	- Moved CSI device as child to VI as Tegra210 CSI is
	  part of VI sharing same host interface and register
	  space.
	- Added link_validate for format validation.
	- Fixes for passing v4l2-compliance for media, video,
	  and subdevices.

[v1]:	Includes,
	- Adds CSI TPG clock to Tegra210 clock driver
	- Host1x video driver with VI and CSI clients.
	- Support for Tegra210 only.
	- VI CSI TPG support with hard media links in driver.
	- Video formats supported by Tegra210 VI
	- CSI TPG supported video formats


Sowjanya Komatineni (9):
  arm64: tegra: Fix sor powergate clocks and reset
  arm64: tegra: Add reset-cells to mc
  dt-bindings: clock: tegra: Add clk id for CSI TPG clock
  clk: tegra: Add Tegra210 CSI TPG clock gate
  dt-binding: tegra: Add VI and CSI bindings
  media: tegra: Add Tegra210 Video input driver
  MAINTAINERS: Add Tegra Video driver section
  dt-bindings: reset: Add ID for Tegra210 VI reset
  arm64: tegra: Add Tegra VI CSI support in device tree

 .../display/tegra/nvidia,tegra20-host1x.txt        |   73 +-
 MAINTAINERS                                        |   10 +
 arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi     |   10 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi           |   52 +-
 drivers/clk/tegra/clk-tegra210.c                   |    7 +
 drivers/staging/media/Kconfig                      |    2 +
 drivers/staging/media/Makefile                     |    1 +
 drivers/staging/media/tegra/Kconfig                |   10 +
 drivers/staging/media/tegra/Makefile               |    8 +
 drivers/staging/media/tegra/TODO                   |   13 +
 drivers/staging/media/tegra/tegra-common.h         |  263 +++++
 drivers/staging/media/tegra/tegra-csi.c            |  639 +++++++++++
 drivers/staging/media/tegra/tegra-csi.h            |  147 +++
 drivers/staging/media/tegra/tegra-vi.c             | 1181 ++++++++++++++++++++
 drivers/staging/media/tegra/tegra-vi.h             |   83 ++
 drivers/staging/media/tegra/tegra-video.c          |  131 +++
 drivers/staging/media/tegra/tegra-video.h          |   32 +
 drivers/staging/media/tegra/tegra210.c             |  718 ++++++++++++
 drivers/staging/media/tegra/tegra210.h             |  192 ++++
 include/dt-bindings/clock/tegra210-car.h           |    2 +-
 include/dt-bindings/reset/tegra210-car.h           |    1 +
 21 files changed, 3558 insertions(+), 17 deletions(-)
 create mode 100644 drivers/staging/media/tegra/Kconfig
 create mode 100644 drivers/staging/media/tegra/Makefile
 create mode 100644 drivers/staging/media/tegra/TODO
 create mode 100644 drivers/staging/media/tegra/tegra-common.h
 create mode 100644 drivers/staging/media/tegra/tegra-csi.c
 create mode 100644 drivers/staging/media/tegra/tegra-csi.h
 create mode 100644 drivers/staging/media/tegra/tegra-vi.c
 create mode 100644 drivers/staging/media/tegra/tegra-vi.h
 create mode 100644 drivers/staging/media/tegra/tegra-video.c
 create mode 100644 drivers/staging/media/tegra/tegra-video.h
 create mode 100644 drivers/staging/media/tegra/tegra210.c
 create mode 100644 drivers/staging/media/tegra/tegra210.h


v4l2-compliance SHA: 81e45d957c4db39397f893100b3d2729ef39b052, 32 bits, 32-bit time_t

Compliance test for tegra-video device /dev/media0:

Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0

Required ioctls:
        test MEDIA_IOC_DEVICE_INFO: OK

Allow for multiple opens:
        test second /dev/media0 open: OK
        test MEDIA_IOC_DEVICE_INFO: OK
        test for unlimited opens: OK

Media Controller ioctls:
        test MEDIA_IOC_G_TOPOLOGY: OK
        Entities: 12 Interfaces: 6 Pads: 12 Links: 12
        test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK
        test MEDIA_IOC_SETUP_LINK: OK
        test invalid ioctls: OK

Total for tegra-video device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video0:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-0
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x03000003
        Type             : V4L Video
Entity Info:
        ID               : 0x00000001 (1)
        Name             : 54080000.vi-output-0
        Function         : V4L2 I/O
        Pad 0x01000002   : 0: Sink
          Link 0x02000007: from remote pad 0x1000006 of entity 'tpg-0': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video0 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video0: 53, Succeeded: 53, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video1:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-1
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x0300000b
        Type             : V4L Video
Entity Info:
        ID               : 0x00000009 (9)
        Name             : 54080000.vi-output-1
        Function         : V4L2 I/O
        Pad 0x0100000a   : 0: Sink
          Link 0x0200000f: from remote pad 0x100000e of entity 'tpg-1': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video1 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video1: 53, Succeeded: 53, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video2:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-2
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x03000013
        Type             : V4L Video
Entity Info:
        ID               : 0x00000011 (17)
        Name             : 54080000.vi-output-2
        Function         : V4L2 I/O
        Pad 0x01000012   : 0: Sink
          Link 0x02000017: from remote pad 0x1000016 of entity 'tpg-2': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video2 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video2: 53, Succeeded: 53, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video3:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-3
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x0300001b
        Type             : V4L Video
Entity Info:
        ID               : 0x00000019 (25)
        Name             : 54080000.vi-output-3
        Function         : V4L2 I/O
        Pad 0x0100001a   : 0: Sink
          Link 0x0200001f: from remote pad 0x100001e of entity 'tpg-3': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video3 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video3: 53, Succeeded: 53, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video4:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-4
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x03000023
        Type             : V4L Video
Entity Info:
        ID               : 0x00000021 (33)
        Name             : 54080000.vi-output-4
        Function         : V4L2 I/O
        Pad 0x01000022   : 0: Sink
          Link 0x02000027: from remote pad 0x1000026 of entity 'tpg-4': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video4 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video4: 53, Succeeded: 53, Failed: 0, Warnings: 0
--------------------------------------------------------------------------------
Compliance test for tegra-video device /dev/video5:

Driver Info:
        Driver name      : tegra-video
        Card type        : 54080000.vi-output-5
        Bus info         : platform:54080000.vi
        Driver version   : 5.6.0
        Capabilities     : 0x85200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x05200001
                Video Capture
                Read/Write
                Streaming
                Extended Pix Format
Media Driver Info:
        Driver name      : tegra-video
        Model            : NVIDIA Tegra Video Input Device
        Serial           : 
        Bus info         : platform:54080000.vi
        Media version    : 5.6.0
        Hardware revision: 0x00000003 (3)
        Driver version   : 5.6.0
Interface Info:
        ID               : 0x0300002b
        Type             : V4L Video
Entity Info:
        ID               : 0x00000029 (41)
        Name             : 54080000.vi-output-5
        Function         : V4L2 I/O
        Pad 0x0100002a   : 0: Sink
          Link 0x0200002f: from remote pad 0x100002e of entity 'tpg-5': Data, Enabled

Required ioctls:
        test MC information (see 'Media Driver Info' above): OK
        test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
        test second /dev/video5 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

        test invalid ioctls: OK
Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
        test read/write: OK
        test blocking wait: OK
        test MMAP (no poll): OK                           
        test MMAP (select): OK                            
        test MMAP (epoll): OK                             
        test USERPTR (no poll): OK (Not Supported)
        test USERPTR (select): OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

Total for tegra-video device /dev/video5: 53, Succeeded: 53, Failed: 0, Warnings: 0

Grand Total for tegra-video device /dev/media0: 326, Succeeded: 326, Failed: 0, Warnings: 0

-- 
2.7.4

[RFC PATCH v6 1/9] arm64: tegra: Fix sor powergate clocks and reset

[Sowjanya Komatineni]

Tegra210 device tree lists csi clock and reset under SOR powergate
node.

But Tegra210 has csicil in SOR partition and csi in VENC partition.

So, this patch includes fix for sor powergate node.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index 64c46ce..d0eff92 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -796,7 +796,9 @@
 			pd_sor: sor {
 				clocks = <&tegra_car TEGRA210_CLK_SOR0>,
 					 <&tegra_car TEGRA210_CLK_SOR1>,
-					 <&tegra_car TEGRA210_CLK_CSI>,
+					 <&tegra_car TEGRA210_CLK_CILAB>,
+					 <&tegra_car TEGRA210_CLK_CILCD>,
+					 <&tegra_car TEGRA210_CLK_CILE>,
 					 <&tegra_car TEGRA210_CLK_DSIA>,
 					 <&tegra_car TEGRA210_CLK_DSIB>,
 					 <&tegra_car TEGRA210_CLK_DPAUX>,
@@ -804,7 +806,6 @@
 					 <&tegra_car TEGRA210_CLK_MIPI_CAL>;
 				resets = <&tegra_car TEGRA210_CLK_SOR0>,
 					 <&tegra_car TEGRA210_CLK_SOR1>,
-					 <&tegra_car TEGRA210_CLK_CSI>,
 					 <&tegra_car TEGRA210_CLK_DSIA>,
 					 <&tegra_car TEGRA210_CLK_DSIB>,
 					 <&tegra_car TEGRA210_CLK_DPAUX>,
-- 
2.7.4

[RFC PATCH v6 2/9] arm64: tegra: Add reset-cells to mc

[Sowjanya Komatineni]

Tegra210 device tree is missing reset-cells property for mc node.

This patch fixes it.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index d0eff92..5b1dfd8 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -894,6 +894,7 @@
 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
 
 		#iommu-cells = <1>;
+		#reset-cells = <1>;
 	};
 
 	sata@70020000 {
-- 
2.7.4

[RFC PATCH v6 3/9] dt-bindings: clock: tegra: Add clk id for CSI TPG clock

[Sowjanya Komatineni]

Tegra210 uses PLLD out internally for CSI TPG.

This patch adds clk id for this CSI TPG clock from PLLD.

Acked-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 include/dt-bindings/clock/tegra210-car.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/dt-bindings/clock/tegra210-car.h b/include/dt-bindings/clock/tegra210-car.h
index 7a8f10b..d8909e0 100644
--- a/include/dt-bindings/clock/tegra210-car.h
+++ b/include/dt-bindings/clock/tegra210-car.h
@@ -351,7 +351,7 @@
 #define TEGRA210_CLK_PLL_P_OUT_XUSB 317
 #define TEGRA210_CLK_XUSB_SSP_SRC 318
 #define TEGRA210_CLK_PLL_RE_OUT1 319
-/* 320 */
+#define TEGRA210_CLK_CSI_TPG 320
 /* 321 */
 #define TEGRA210_CLK_ISP 322
 #define TEGRA210_CLK_PLL_A_OUT_ADSP 323
-- 
2.7.4

[RFC PATCH v6 4/9] clk: tegra: Add Tegra210 CSI TPG clock gate

[Sowjanya Komatineni]

Tegra210 CSI hardware internally uses PLLD for internal test pattern
generator logic.

PLLD_BASE register in CAR has a bit CSI_CLK_SOURCE to enable PLLD
out to CSI during TPG mode.

This patch adds this CSI TPG clock gate to Tegra210 clock driver
to allow Tegra video driver to ungate CSI TPG clock during TPG mode
and gate during non TPG mode.

Acked-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/clk/tegra/clk-tegra210.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index defe3b7..81a879b 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -3035,6 +3035,13 @@ static __init void tegra210_periph_clk_init(void __iomem *clk_base,
 					     periph_clk_enb_refcnt);
 	clks[TEGRA210_CLK_DSIB] = clk;
 
+	/* csi_tpg */
+	clk = clk_register_gate(NULL, "csi_tpg", "pll_d",
+				CLK_SET_RATE_PARENT, clk_base + PLLD_BASE,
+				23, 0, &pll_d_lock);
+	clk_register_clkdev(clk, "csi_tpg", NULL);
+	clks[TEGRA210_CLK_CSI_TPG] = clk;
+
 	/* la */
 	clk = tegra_clk_register_periph("la", la_parents,
 			ARRAY_SIZE(la_parents), &tegra210_la, clk_base,
-- 
2.7.4

[RFC PATCH v6 5/9] dt-binding: tegra: Add VI and CSI bindings

[Sowjanya Komatineni]

Tegra contains VI controller which can support up to 6 MIPI CSI
camera sensors.

Each Tegra CSI port from CSI unit can be one-to-one mapper to
VI channel and can capture from an external camera sensor or
from built-in test pattern generator.

This patch adds dt-bindings for Tegra VI and CSI.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../display/tegra/nvidia,tegra20-host1x.txt        | 73 ++++++++++++++++++----
 1 file changed, 60 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
index 9999255..4731921 100644
--- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
@@ -40,14 +40,30 @@ of the following host1x client modules:
 
   Required properties:
   - compatible: "nvidia,tegra<chip>-vi"
-  - reg: Physical base address and length of the controller's registers.
+  - reg: Physical base address and length of the controller registers.
   - interrupts: The interrupt outputs from the controller.
-  - clocks: Must contain one entry, for the module clock.
+  - clocks: 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:
-    - vi
+  - Tegra20/Tegra30/Tegra114/Tegra124:
+    - resets: Must contain an entry for each entry in reset-names.
+      See ../reset/reset.txt for details.
+    - reset-names: Must include the following entries:
+      - vi
+  - Tegra210:
+    - power-domains: Must include venc powergate node as vi is in VE partition.
+  - Tegra210 has CSI part of VI sharing same host interface and register space.
+    So, VI device node should have CSI child node.
+
+    - csi: mipi csi interface to vi
+
+      Required properties:
+      - compatible: "nvidia,tegra210-csi"
+      - reg: Physical base address offset to parent and length of the controller
+        registers.
+      - clocks: Must contain entries csi, cilab, cilcd, cile, csi_tpg clocks.
+        See ../clocks/clock-bindings.txt for details.
+      - power-domains: Must include sor powergate node as csicil is in
+        SOR partition.
 
 - epp: encoder pre-processor
 
@@ -309,13 +325,44 @@ Example:
 			reset-names = "mpe";
 		};
 
-		vi {
-			compatible = "nvidia,tegra20-vi";
-			reg = <0x54080000 0x00040000>;
-			interrupts = <0 69 0x04>;
-			clocks = <&tegra_car TEGRA20_CLK_VI>;
-			resets = <&tegra_car 100>;
-			reset-names = "vi";
+		vi@54080000 {
+			compatible = "nvidia,tegra210-vi";
+			reg = <0x0 0x54080000 0x0 0x700>;
+			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			assigned-clocks = <&tegra_car TEGRA210_CLK_VI>;
+			assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
+
+			clocks = <&tegra_car TEGRA210_CLK_VI>;
+			power-domains = <&pd_venc>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			ranges = <0x0 0x0 0x54080000 0x2000>;
+
+			csi@838 {
+				compatible = "nvidia,tegra210-csi";
+				reg = <0x838 0x1300>;
+				assigned-clocks = <&tegra_car TEGRA210_CLK_CILAB>,
+						  <&tegra_car TEGRA210_CLK_CILCD>,
+						  <&tegra_car TEGRA210_CLK_CILE>,
+						  <&tegra_car TEGRA210_CLK_CSI_TPG>;
+				assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_P>,
+							 <&tegra_car TEGRA210_CLK_PLL_P>,
+							 <&tegra_car TEGRA210_CLK_PLL_P>;
+				assigned-clock-rates = <102000000>,
+						       <102000000>,
+						       <102000000>,
+						       <972000000>;
+
+				clocks = <&tegra_car TEGRA210_CLK_CSI>,
+					 <&tegra_car TEGRA210_CLK_CILAB>,
+					 <&tegra_car TEGRA210_CLK_CILCD>,
+					 <&tegra_car TEGRA210_CLK_CILE>,
+					 <&tegra_car TEGRA210_CLK_CSI_TPG>;
+				clock-names = "csi", "cilab", "cilcd", "cile", "csi_tpg";
+				power-domains = <&pd_sor>;
+			};
 		};
 
 		epp {
-- 
2.7.4

[RFC PATCH v6 7/9] MAINTAINERS: Add Tegra Video driver section

[Sowjanya Komatineni]

Add maintainers and mailing list entries to Tegra Video driver section.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5fd1240..9ae3ce7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16644,6 +16644,16 @@ M:	Laxman Dewangan <ldewangan@nvidia.com>
 S:	Supported
 F:	drivers/spi/spi-tegra*
 
+TEGRA VIDEO DRIVER
+M:	Thierry Reding <thierry.reding@gmail.com>
+M:	Jonathan Hunter <jonathanh@nvidia.com>
+M:	Sowjanya Komatineni <skomatineni@nvidia.com>
+L:	linux-media@vger.kernel.org
+L:	linux-tegra@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
+F:	drivers/staging/media/tegra/
+
 TEGRA XUSB PADCTL DRIVER
 M:	JC Kuo <jckuo@nvidia.com>
 S:	Supported
-- 
2.7.4

[RFC PATCH v6 8/9] dt-bindings: reset: Add ID for Tegra210 VI reset

[Sowjanya Komatineni]

This patch adds ID for Tegra210 VI controller reset to use with
device tree.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 include/dt-bindings/reset/tegra210-car.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/reset/tegra210-car.h b/include/dt-bindings/reset/tegra210-car.h
index 9dc84ec..8755946 100644
--- a/include/dt-bindings/reset/tegra210-car.h
+++ b/include/dt-bindings/reset/tegra210-car.h
@@ -10,5 +10,6 @@
 #define TEGRA210_RESET(x)		(7 * 32 + (x))
 #define TEGRA210_RST_DFLL_DVCO		TEGRA210_RESET(0)
 #define TEGRA210_RST_ADSP		TEGRA210_RESET(1)
+#define TEGRA210_RST_VI			20
 
 #endif	/* _DT_BINDINGS_RESET_TEGRA210_CAR_H */
-- 
2.7.4

[RFC PATCH v6 9/9] arm64: tegra: Add Tegra VI CSI support in device tree

[Sowjanya Komatineni]

Tegra210 contains VI controller for video input capture from MIPI
CSI camera sensors and also supports built-in test pattern generator.

CSI ports can be one-to-one mapped to VI channels for capturing from
an external sensor or from built-in test pattern generator.

This patch adds support for VI and CSI and enables them in Tegra210
device tree.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 10 ++++++
 arch/arm64/boot/dts/nvidia/tegra210.dtsi       | 46 +++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
index 313a4c2..b57d837 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
@@ -14,6 +14,16 @@
 			status = "okay";
 		};
 
+		vi@54080000 {
+			status = "okay";
+
+			avdd-dsi-csi-supply = <&vdd_dsi_csi>;
+
+			csi@838 {
+				status = "okay";
+			};
+		};
+
 		sor@54580000 {
 			status = "okay";
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index 5b1dfd8..cad42a7 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -137,9 +137,44 @@
 
 		vi@54080000 {
 			compatible = "nvidia,tegra210-vi";
-			reg = <0x0 0x54080000 0x0 0x00040000>;
+			reg = <0x0 0x54080000 0x0 0x700>;
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			status = "disabled";
+			assigned-clocks = <&tegra_car TEGRA210_CLK_VI>;
+			assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
+
+			clocks = <&tegra_car TEGRA210_CLK_VI>;
+			power-domains = <&pd_venc>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			ranges = <0x0 0x0 0x54080000 0x2000>;
+
+			csi@838 {
+				compatible = "nvidia,tegra210-csi";
+				reg = <0x838 0x1300>;
+				status = "disabled";
+				assigned-clocks = <&tegra_car TEGRA210_CLK_CILAB>,
+						  <&tegra_car TEGRA210_CLK_CILCD>,
+						  <&tegra_car TEGRA210_CLK_CILE>,
+						  <&tegra_car TEGRA210_CLK_CSI_TPG>;
+				assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_P>,
+							 <&tegra_car TEGRA210_CLK_PLL_P>,
+							 <&tegra_car TEGRA210_CLK_PLL_P>;
+				assigned-clock-rates = <102000000>,
+						       <102000000>,
+						       <102000000>,
+						       <972000000>;
+
+				clocks = <&tegra_car TEGRA210_CLK_CSI>,
+					 <&tegra_car TEGRA210_CLK_CILAB>,
+					 <&tegra_car TEGRA210_CLK_CILCD>,
+					 <&tegra_car TEGRA210_CLK_CILE>,
+					 <&tegra_car TEGRA210_CLK_CSI_TPG>;
+				clock-names = "csi", "cilab", "cilcd", "cile", "csi_tpg";
+				power-domains = <&pd_sor>;
+			};
 		};
 
 		tsec@54100000 {
@@ -839,6 +874,15 @@
 				reset-names = "vic";
 				#power-domain-cells = <0>;
 			};
+
+			pd_venc: venc {
+				clocks = <&tegra_car TEGRA210_CLK_VI>,
+					 <&tegra_car TEGRA210_CLK_CSI>;
+				resets = <&mc TEGRA210_MC_RESET_VI>,
+					 <&tegra_car TEGRA210_RST_VI>,
+					 <&tegra_car TEGRA210_CLK_CSI>;
+				#power-domain-cells = <0>;
+			};
 		};
 
 		sdmmc1_3v3: sdmmc1-3v3 {
-- 
2.7.4

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int tegra_csi_init(struct host1x_client *client)
> +{
> +	struct tegra_csi *csi = host1x_client_to_csi(client);
> +	struct tegra_video_device *vid = dev_get_drvdata(client->host);
> +	int ret;
> +
> +	vid->csi = csi;
> +
> +	INIT_LIST_HEAD(&csi->csi_chans);
> +
> +	if (pm_runtime_enabled(csi->dev)) {
> +		ret = pm_runtime_get_sync(csi->dev);
> +		if (ret < 0) {
> +			dev_err(csi->dev,
> +				"failed to get runtime PM: %d\n", ret);
> +			pm_runtime_put_noidle(csi->dev);
> +			return ret;
> +		}
> +	} else {

RPM is supposed to be always available on Tegra nowadays.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int tegra_vi_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct tegra_vi *vi;
> +	int ret;
> +
> +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);

devm_kzalloc()?

> +	if (!vi)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vi->iomem = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()?

> +	if (IS_ERR(vi->iomem)) {
> +		ret = PTR_ERR(vi->iomem);
> +		goto cleanup;
> +	}
> +
> +	vi->soc = of_device_get_match_data(&pdev->dev);

This can't fail because match already happened.

> +	if (!vi->soc) {
> +		ret = -ENODATA;
> +		goto cleanup;
> +	}
> +
> +	vi->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(vi->clk)) {
> +		ret = PTR_ERR(vi->clk);
> +		dev_err(&pdev->dev, "failed to get vi clock: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	vi->vdd = devm_regulator_get(&pdev->dev, "avdd-dsi-csi");
> +	if (IS_ERR(vi->vdd)) {
> +		ret = PTR_ERR(vi->vdd);
> +		dev_err(&pdev->dev, "failed to get VDD supply: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	if (!pdev->dev.pm_domain) {
> +		ret = -ENOENT;
> +		dev_warn(&pdev->dev, "PM domain is not attached: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to populate vi child device: %d\n", ret);
> +		goto cleanup;
> +	}
> +
> +	vi->dev = &pdev->dev;
> +	vi->ops = vi->soc->ops;
> +	platform_set_drvdata(pdev, vi);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	/* initialize host1x interface */
> +	INIT_LIST_HEAD(&vi->client.list);
> +	vi->client.ops = &vi_client_ops;
> +	vi->client.dev = &pdev->dev;
> +
> +	ret = host1x_client_register(&vi->client);
> +	if (ret < 0) {
> +		dev_err(vi->dev,
> +			"failed to register host1x client: %d\n", ret);
> +		ret = -ENODEV;
> +		goto rpm_disable;
> +	}
> +
> +	return 0;
> +
> +rpm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	of_platform_depopulate(vi->dev);
> +cleanup:
> +	kfree(vi);
> +	return ret;
> +}
> +
> +static int tegra_vi_remove(struct platform_device *pdev)
> +{
> +	struct tegra_vi *vi = platform_get_drvdata(pdev);
> +	int err;
> +
> +	pm_runtime_disable(vi->dev);
> +
> +	err = host1x_client_unregister(&vi->client);
> +	if (err < 0) {
> +		dev_err(vi->dev,
> +			"failed to unregister host1x client: %d\n", err);
> +		return err;
> +	}

The removal order should be opposite to the registration order.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +/* Tegra210 VI registers accessors */
> +static void tegra_vi_write(struct tegra_vi_channel *chan, unsigned int addr,
> +			   u32 val)
> +{
> +	writel(val, chan->vi->iomem + addr);
> +}
> +
> +static u32 tegra_vi_read(struct tegra_vi_channel *chan, unsigned int addr)
> +{
> +	return readl(chan->vi->iomem + addr);
> +}
...

Perhaps all reads and writes should be relaxed?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

05.04.2020 22:45, Dmitry Osipenko пишет:
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int tegra_vi_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct tegra_vi *vi;
>> +	int ret;
>> +
>> +	vi = kzalloc(sizeof(*vi), GFP_KERNEL);
> 
> devm_kzalloc()?
> 
>> +	if (!vi)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	vi->iomem = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_ioremap_resource()?
> 
>> +	if (IS_ERR(vi->iomem)) {
>> +		ret = PTR_ERR(vi->iomem);
>> +		goto cleanup;
>> +	}
>> +
>> +	vi->soc = of_device_get_match_data(&pdev->dev);
> 
> This can't fail because match already happened.
> 
>> +	if (!vi->soc) {
>> +		ret = -ENODATA;
>> +		goto cleanup;
>> +	}
>> +
>> +	vi->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(vi->clk)) {
>> +		ret = PTR_ERR(vi->clk);
>> +		dev_err(&pdev->dev, "failed to get vi clock: %d\n", ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	vi->vdd = devm_regulator_get(&pdev->dev, "avdd-dsi-csi");
>> +	if (IS_ERR(vi->vdd)) {
>> +		ret = PTR_ERR(vi->vdd);
>> +		dev_err(&pdev->dev, "failed to get VDD supply: %d\n", ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	if (!pdev->dev.pm_domain) {
>> +		ret = -ENOENT;
>> +		dev_warn(&pdev->dev, "PM domain is not attached: %d\n", ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"failed to populate vi child device: %d\n", ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	vi->dev = &pdev->dev;
>> +	vi->ops = vi->soc->ops;
>> +	platform_set_drvdata(pdev, vi);
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	/* initialize host1x interface */
>> +	INIT_LIST_HEAD(&vi->client.list);
>> +	vi->client.ops = &vi_client_ops;
>> +	vi->client.dev = &pdev->dev;
>> +
>> +	ret = host1x_client_register(&vi->client);
>> +	if (ret < 0) {
>> +		dev_err(vi->dev,
>> +			"failed to register host1x client: %d\n", ret);
>> +		ret = -ENODEV;
>> +		goto rpm_disable;
>> +	}
>> +
>> +	return 0;
>> +
>> +rpm_disable:
>> +	pm_runtime_disable(&pdev->dev);
>> +	of_platform_depopulate(vi->dev);
>> +cleanup:
>> +	kfree(vi);
>> +	return ret;
>> +}
>> +
>> +static int tegra_vi_remove(struct platform_device *pdev)
>> +{
>> +	struct tegra_vi *vi = platform_get_drvdata(pdev);
>> +	int err;
>> +
>> +	pm_runtime_disable(vi->dev);
>> +
>> +	err = host1x_client_unregister(&vi->client);
>> +	if (err < 0) {
>> +		dev_err(vi->dev,
>> +			"failed to unregister host1x client: %d\n", err);
>> +		return err;
>> +	}
> 
> The removal order should be opposite to the registration order.
> 

All the same to the tegra_csi, btw.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan,
> +				       struct tegra_channel_buffer *buf)
> +{
> +	int err = 0;
> +	u32 thresh, value, frame_start, mw_ack_done;
> +	int bytes_per_line = chan->format.bytesperline;
> +
> +	/* program buffer address by using surface 0 */
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB,
> +		     (u64)buf->addr >> 32);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
> +
> +	/*
> +	 * Tegra VI block interacts with host1x syncpt for synchronizing
> +	 * programmed condition of capture state and hardware operation.
> +	 * Frame start and Memory write acknowledge syncpts has their own
> +	 * FIFO of depth 2.
> +	 *
> +	 * Syncpoint trigger conditions set through VI_INCR_SYNCPT register
> +	 * are added to HW syncpt FIFO and when the HW triggers, syncpt
> +	 * condition is removed from the FIFO and counter at syncpoint index
> +	 * will be incremented by the hardware and software can wait for
> +	 * counter to reach threshold to synchronize capturing frame with the
> +	 * hardware capture events.
> +	 */
> +
> +	/* increase channel syncpoint threshold for FRAME_START */
> +	thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1);
> +
> +	/* Program FRAME_START trigger condition syncpt request */
> +	frame_start = VI_CSI_PP_FRAME_START(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
> +		host1x_syncpt_id(chan->frame_start_sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	/* increase channel syncpoint threshold for MW_ACK_DONE */
> +	buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1);
> +
> +	/* Program MW_ACK_DONE trigger condition syncpt request */
> +	mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
> +		host1x_syncpt_id(chan->mw_ack_sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	/* enable single shot capture */
> +	vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
> +	chan->capture_reqs++;
> +
> +	/* wait for syncpt counter to reach frame start event threshold */
> +	err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
> +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> +	if (err) {
> +		dev_err(&chan->video.dev,
> +			"frame start syncpt timeout: %d\n", err);

> +		/* increment syncpoint counter for timedout events */
> +		host1x_syncpt_incr(chan->frame_start_sp);

Why incrementing is done while hardware is still active?

The sync point's state needs to be completely reset after resetting
hardware. But I don't think that the current upstream host1x driver
supports doing that, it's one of the known-long-standing problems of the
host1x driver.

At least the sp->max_val incrementing should be done based on the actual
syncpoint value and this should be done after resetting hardware.

> +		spin_lock(&chan->sp_incr_lock);
> +		host1x_syncpt_incr(chan->mw_ack_sp);
> +		spin_unlock(&chan->sp_incr_lock);
> +		/* clear errors and recover */
> +		tegra_channel_capture_error_recover(chan);
> +		release_buffer(chan, buf, VB2_BUF_STATE_ERROR);
> +		return err;
> +	}

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +config VIDEO_TEGRA
> +	tristate "NVIDIA Tegra VI driver"
> +	depends on ARCH_TEGRA || (ARM && COMPILE_TEST)

Why COMPILE_TEST depends on ARM?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
> +{
> +	struct tegra_vi_channel *chan, *tmp;
> +	unsigned int port_num;
> +	unsigned int nchannels = vi->soc->vi_max_channels;
> +	int ret = 0;
> +
> +	for (port_num = 0; port_num < nchannels; port_num++) {
> +		/*
> +		 * Do not use devm_kzalloc as memory is freed immediately
> +		 * when device instance is unbound but application might still
> +		 * be holding the device node open. Channel memory allocated
> +		 * with kzalloc is freed during video device release callback.
> +		 */
> +		chan = kzalloc(sizeof(*chan), GFP_KERNEL);

Why anyone would want to unbind this driver in practice?

I think it should make more sense to set suppress_bind_attrs=true.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/5/20 1:35 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan,
>> +                                    struct tegra_channel_buffer *buf)
>> +{
>> +     int err = 0;
>> +     u32 thresh, value, frame_start, mw_ack_done;
>> +     int bytes_per_line = chan->format.bytesperline;
>> +
>> +     /* program buffer address by using surface 0 */
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB,
>> +                  (u64)buf->addr >> 32);
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
>> +
>> +     /*
>> +      * Tegra VI block interacts with host1x syncpt for synchronizing
>> +      * programmed condition of capture state and hardware operation.
>> +      * Frame start and Memory write acknowledge syncpts has their own
>> +      * FIFO of depth 2.
>> +      *
>> +      * Syncpoint trigger conditions set through VI_INCR_SYNCPT register
>> +      * are added to HW syncpt FIFO and when the HW triggers, syncpt
>> +      * condition is removed from the FIFO and counter at syncpoint index
>> +      * will be incremented by the hardware and software can wait for
>> +      * counter to reach threshold to synchronize capturing frame with the
>> +      * hardware capture events.
>> +      */
>> +
>> +     /* increase channel syncpoint threshold for FRAME_START */
>> +     thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1);
>> +
>> +     /* Program FRAME_START trigger condition syncpt request */
>> +     frame_start = VI_CSI_PP_FRAME_START(chan->portno);
>> +     value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
>> +             host1x_syncpt_id(chan->frame_start_sp);
>> +     tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
>> +
>> +     /* increase channel syncpoint threshold for MW_ACK_DONE */
>> +     buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1);
>> +
>> +     /* Program MW_ACK_DONE trigger condition syncpt request */
>> +     mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno);
>> +     value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
>> +             host1x_syncpt_id(chan->mw_ack_sp);
>> +     tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
>> +
>> +     /* enable single shot capture */
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
>> +     chan->capture_reqs++;
>> +
>> +     /* wait for syncpt counter to reach frame start event threshold */
>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>> +     if (err) {
>> +             dev_err(&chan->video.dev,
>> +                     "frame start syncpt timeout: %d\n", err);
>> +             /* increment syncpoint counter for timedout events */
>> +             host1x_syncpt_incr(chan->frame_start_sp);
> Why incrementing is done while hardware is still active?
>
> The sync point's state needs to be completely reset after resetting
> hardware. But I don't think that the current upstream host1x driver
> supports doing that, it's one of the known-long-standing problems of the
> host1x driver.
>
> At least the sp->max_val incrementing should be done based on the actual
> syncpoint value and this should be done after resetting hardware.

upstream host1x driver don't have API to reset or to equalize max value 
with min/load value.

So to synchronize missed event, incrementing HW syncpt counter.

This should not impact as we increment this in case of missed events only.

>> +             spin_lock(&chan->sp_incr_lock);
>> +             host1x_syncpt_incr(chan->mw_ack_sp);
>> +             spin_unlock(&chan->sp_incr_lock);
>> +             /* clear errors and recover */
>> +             tegra_channel_capture_error_recover(chan);
>> +             release_buffer(chan, buf, VB2_BUF_STATE_ERROR);
>> +             return err;
>> +     }

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/5/20 2:11 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
>> +{
>> +     struct tegra_vi_channel *chan, *tmp;
>> +     unsigned int port_num;
>> +     unsigned int nchannels = vi->soc->vi_max_channels;
>> +     int ret = 0;
>> +
>> +     for (port_num = 0; port_num < nchannels; port_num++) {
>> +             /*
>> +              * Do not use devm_kzalloc as memory is freed immediately
>> +              * when device instance is unbound but application might still
>> +              * be holding the device node open. Channel memory allocated
>> +              * with kzalloc is freed during video device release callback.
>> +              */
>> +             chan = kzalloc(sizeof(*chan), GFP_KERNEL);
> Why anyone would want to unbind this driver in practice?
>
> I think it should make more sense to set suppress_bind_attrs=true.

 From the previous feedback of patch series, we need to support 
unbind/bind and looks like this driver should also support to built as a 
module.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 18:35, Sowjanya Komatineni пишет:
...
>>> +     /* wait for syncpt counter to reach frame start event threshold */
>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>> +     if (err) {
>>> +             dev_err(&chan->video.dev,
>>> +                     "frame start syncpt timeout: %d\n", err);
>>> +             /* increment syncpoint counter for timedout events */
>>> +             host1x_syncpt_incr(chan->frame_start_sp);
>> Why incrementing is done while hardware is still active?
>>
>> The sync point's state needs to be completely reset after resetting
>> hardware. But I don't think that the current upstream host1x driver
>> supports doing that, it's one of the known-long-standing problems of the
>> host1x driver.
>>
>> At least the sp->max_val incrementing should be done based on the actual
>> syncpoint value and this should be done after resetting hardware.
> 
> upstream host1x driver don't have API to reset or to equalize max value
> with min/load value.
> 
> So to synchronize missed event, incrementing HW syncpt counter.
> 
> This should not impact as we increment this in case of missed events only.

It's wrong to touch sync point while hardware is active and it's active
until being reset.

You should re-check the timeout after hw resetting and manually put the
syncpoint counter back into sync only if needed.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 18:41, Sowjanya Komatineni пишет:
> 
> On 4/5/20 2:11 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>> ...
>>> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
>>> +{
>>> +     struct tegra_vi_channel *chan, *tmp;
>>> +     unsigned int port_num;
>>> +     unsigned int nchannels = vi->soc->vi_max_channels;
>>> +     int ret = 0;
>>> +
>>> +     for (port_num = 0; port_num < nchannels; port_num++) {
>>> +             /*
>>> +              * Do not use devm_kzalloc as memory is freed immediately
>>> +              * when device instance is unbound but application
>>> might still
>>> +              * be holding the device node open. Channel memory
>>> allocated
>>> +              * with kzalloc is freed during video device release
>>> callback.
>>> +              */
>>> +             chan = kzalloc(sizeof(*chan), GFP_KERNEL);
>> Why anyone would want to unbind this driver in practice?
>>
>> I think it should make more sense to set suppress_bind_attrs=true.
> 
> From the previous feedback of patch series, we need to support
> unbind/bind and looks like this driver should also support to built as a
> module.

If module unloading is also affected, then perhaps you should use
get/put_device() to not allow freeing the resources until they're still
in-use.

I suppose that it should be up to the V4L core to keep the device alive
while needed, rather than to put the burden to the individual drivers.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 18:35, Sowjanya Komatineni пишет:
> ...
>>>> +     /* wait for syncpt counter to reach frame start event threshold */
>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>> +     if (err) {
>>>> +             dev_err(&chan->video.dev,
>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>> +             /* increment syncpoint counter for timedout events */
>>>> +             host1x_syncpt_incr(chan->frame_start_sp);
>>> Why incrementing is done while hardware is still active?
>>>
>>> The sync point's state needs to be completely reset after resetting
>>> hardware. But I don't think that the current upstream host1x driver
>>> supports doing that, it's one of the known-long-standing problems of the
>>> host1x driver.
>>>
>>> At least the sp->max_val incrementing should be done based on the actual
>>> syncpoint value and this should be done after resetting hardware.
>> upstream host1x driver don't have API to reset or to equalize max value
>> with min/load value.
>>
>> So to synchronize missed event, incrementing HW syncpt counter.
>>
>> This should not impact as we increment this in case of missed events only.
> It's wrong to touch sync point while hardware is active and it's active
> until being reset.
>
> You should re-check the timeout after hw resetting and manually put the
> syncpoint counter back into sync only if needed.

There is possibility of timeout to happen any time even during the 
capture also and is not related to hw reset.

Manual synchronization is needed when timeout of any frame events happen 
otherwise all subsequence frames will timeout due to mismatch in event 
counters.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 19:12, Sowjanya Komatineni пишет:
> 
> On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 06.04.2020 18:35, Sowjanya Komatineni пишет:
>> ...
>>>>> +     /* wait for syncpt counter to reach frame start event
>>>>> threshold */
>>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>>> +     if (err) {
>>>>> +             dev_err(&chan->video.dev,
>>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>>> +             /* increment syncpoint counter for timedout events */
>>>>> +             host1x_syncpt_incr(chan->frame_start_sp);
>>>> Why incrementing is done while hardware is still active?
>>>>
>>>> The sync point's state needs to be completely reset after resetting
>>>> hardware. But I don't think that the current upstream host1x driver
>>>> supports doing that, it's one of the known-long-standing problems of
>>>> the
>>>> host1x driver.
>>>>
>>>> At least the sp->max_val incrementing should be done based on the
>>>> actual
>>>> syncpoint value and this should be done after resetting hardware.
>>> upstream host1x driver don't have API to reset or to equalize max value
>>> with min/load value.
>>>
>>> So to synchronize missed event, incrementing HW syncpt counter.
>>>
>>> This should not impact as we increment this in case of missed events
>>> only.
>> It's wrong to touch sync point while hardware is active and it's active
>> until being reset.
>>
>> You should re-check the timeout after hw resetting and manually put the
>> syncpoint counter back into sync only if needed.
> 
> There is possibility of timeout to happen any time even during the
> capture also and is not related to hw reset.
> 
> Manual synchronization is needed when timeout of any frame events happen
> otherwise all subsequence frames will timeout due to mismatch in event
> counters.

My point is that hardware is stopped only after being reset, until then
you should assume that sync point could be incremented by HW at any time.

And if this happens that HW increments sync point after the timeout,
then the sync point counter should become out-of-sync in yours case,
IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 9:29 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 19:12, Sowjanya Komatineni пишет:
>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 06.04.2020 18:35, Sowjanya Komatineni пишет:
>>> ...
>>>>>> +     /* wait for syncpt counter to reach frame start event
>>>>>> threshold */
>>>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>>>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>>>> +     if (err) {
>>>>>> +             dev_err(&chan->video.dev,
>>>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>>>> +             /* increment syncpoint counter for timedout events */
>>>>>> +             host1x_syncpt_incr(chan->frame_start_sp);
>>>>> Why incrementing is done while hardware is still active?
>>>>>
>>>>> The sync point's state needs to be completely reset after resetting
>>>>> hardware. But I don't think that the current upstream host1x driver
>>>>> supports doing that, it's one of the known-long-standing problems of
>>>>> the
>>>>> host1x driver.
>>>>>
>>>>> At least the sp->max_val incrementing should be done based on the
>>>>> actual
>>>>> syncpoint value and this should be done after resetting hardware.
>>>> upstream host1x driver don't have API to reset or to equalize max value
>>>> with min/load value.
>>>>
>>>> So to synchronize missed event, incrementing HW syncpt counter.
>>>>
>>>> This should not impact as we increment this in case of missed events
>>>> only.
>>> It's wrong to touch sync point while hardware is active and it's active
>>> until being reset.
>>>
>>> You should re-check the timeout after hw resetting and manually put the
>>> syncpoint counter back into sync only if needed.
>> There is possibility of timeout to happen any time even during the
>> capture also and is not related to hw reset.
>>
>> Manual synchronization is needed when timeout of any frame events happen
>> otherwise all subsequence frames will timeout due to mismatch in event
>> counters.
> My point is that hardware is stopped only after being reset, until then
> you should assume that sync point could be incremented by HW at any time.
>
> And if this happens that HW increments sync point after the timeout,
> then the sync point counter should become out-of-sync in yours case,
> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.

We wait for enough time based on frame rate for syncpt increment to 
happen and if it doesn't happen by then definitely its missed event and 
we increment HW syncpoint for this timed event.

cached value gets updated during syncpt wait for subsequent event.

syncpt increment happens for all subsequent frame events during video 
capture.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 9:37 AM, Sowjanya Komatineni wrote:
>
> On 4/6/20 9:29 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 06.04.2020 19:12, Sowjanya Komatineni пишет:
>>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 06.04.2020 18:35, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> +     /* wait for syncpt counter to reach frame start event
>>>>>>> threshold */
>>>>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>>>>> +     if (err) {
>>>>>>> +             dev_err(&chan->video.dev,
>>>>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>>>>> +             /* increment syncpoint counter for timedout events */
>>>>>>> + host1x_syncpt_incr(chan->frame_start_sp);
>>>>>> Why incrementing is done while hardware is still active?
>>>>>>
>>>>>> The sync point's state needs to be completely reset after resetting
>>>>>> hardware. But I don't think that the current upstream host1x driver
>>>>>> supports doing that, it's one of the known-long-standing problems of
>>>>>> the
>>>>>> host1x driver.
>>>>>>
>>>>>> At least the sp->max_val incrementing should be done based on the
>>>>>> actual
>>>>>> syncpoint value and this should be done after resetting hardware.
>>>>> upstream host1x driver don't have API to reset or to equalize max 
>>>>> value
>>>>> with min/load value.
>>>>>
>>>>> So to synchronize missed event, incrementing HW syncpt counter.
>>>>>
>>>>> This should not impact as we increment this in case of missed events
>>>>> only.
>>>> It's wrong to touch sync point while hardware is active and it's 
>>>> active
>>>> until being reset.
>>>>
>>>> You should re-check the timeout after hw resetting and manually put 
>>>> the
>>>> syncpoint counter back into sync only if needed.
>>> There is possibility of timeout to happen any time even during the
>>> capture also and is not related to hw reset.
>>>
>>> Manual synchronization is needed when timeout of any frame events 
>>> happen
>>> otherwise all subsequence frames will timeout due to mismatch in event
>>> counters.
>> My point is that hardware is stopped only after being reset, until then
>> you should assume that sync point could be incremented by HW at any 
>> time.
>>
>> And if this happens that HW increments sync point after the timeout,
>> then the sync point counter should become out-of-sync in yours case,
>> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.
>
> We wait for enough time based on frame rate for syncpt increment to 
> happen and if it doesn't happen by then definitely its missed event 
> and we increment HW syncpoint for this timed event.
>
> cached value gets updated during syncpt wait for subsequent event.
>
> syncpt increment happens for all subsequent frame events during video 
> capture.
>
Just to be clear, syncpt max value increment happens first and syncpt 
trigger condition is programmed. hw syncpt increment happens based on HW 
events.

Wait time for HW syncpt to reach threshold is tuned to work for all 
frame rates. So if increment doesn't happen by then, its definitely 
missed event.

In case of missed HW event corresponding to syncpt condition, hw syncpt 
increment does not happen and driver increments it on timeout.

As there is not API to equialize max with min incase of timeout/reset, 
incrementing HW syncpt for timed out event.

syncpt cached value gets updated during syncpt wait when it loads from 
HW syncpt.

As syncpt condition is already triggered, without compensating timeout 
events or leaving syncpt max and hw syncpt in non synchronized state for 
missed events, subsequent streamings will all timeout even on real events.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/5/20 12:37 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int tegra_csi_init(struct host1x_client *client)
>> +{
>> +     struct tegra_csi *csi = host1x_client_to_csi(client);
>> +     struct tegra_video_device *vid = dev_get_drvdata(client->host);
>> +     int ret;
>> +
>> +     vid->csi = csi;
>> +
>> +     INIT_LIST_HEAD(&csi->csi_chans);
>> +
>> +     if (pm_runtime_enabled(csi->dev)) {
>> +             ret = pm_runtime_get_sync(csi->dev);
>> +             if (ret < 0) {
>> +                     dev_err(csi->dev,
>> +                             "failed to get runtime PM: %d\n", ret);
>> +                     pm_runtime_put_noidle(csi->dev);
>> +                     return ret;
>> +             }
>> +     } else {
> RPM is supposed to be always available on Tegra nowadays.

Sorry I was not sure if its all the time enabled, so added in v6.

Will remove check and explicit runtime calls...

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan,
> +				       struct tegra_channel_buffer *buf)
> +{
> +	int err = 0;
> +	u32 thresh, value, frame_start, mw_ack_done;
> +	int bytes_per_line = chan->format.bytesperline;
> +
> +	/* program buffer address by using surface 0 */
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB,
> +		     (u64)buf->addr >> 32);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
> +
> +	/*
> +	 * Tegra VI block interacts with host1x syncpt for synchronizing
> +	 * programmed condition of capture state and hardware operation.
> +	 * Frame start and Memory write acknowledge syncpts has their own
> +	 * FIFO of depth 2.
> +	 *
> +	 * Syncpoint trigger conditions set through VI_INCR_SYNCPT register
> +	 * are added to HW syncpt FIFO and when the HW triggers, syncpt
> +	 * condition is removed from the FIFO and counter at syncpoint index
> +	 * will be incremented by the hardware and software can wait for
> +	 * counter to reach threshold to synchronize capturing frame with the
> +	 * hardware capture events.
> +	 */
> +
> +	/* increase channel syncpoint threshold for FRAME_START */
> +	thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1);
> +
> +	/* Program FRAME_START trigger condition syncpt request */
> +	frame_start = VI_CSI_PP_FRAME_START(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
> +		host1x_syncpt_id(chan->frame_start_sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	/* increase channel syncpoint threshold for MW_ACK_DONE */
> +	buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1);
> +
> +	/* Program MW_ACK_DONE trigger condition syncpt request */
> +	mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
> +		host1x_syncpt_id(chan->mw_ack_sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	/* enable single shot capture */
> +	vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
> +	chan->capture_reqs++;
> +
> +	/* wait for syncpt counter to reach frame start event threshold */
> +	err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
> +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);

What is the point of waiting for the frame-start? Why just not to wait
for the frame-end?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 20:02, Sowjanya Komatineni пишет:
> 
> On 4/6/20 9:37 AM, Sowjanya Komatineni wrote:
>>
>> On 4/6/20 9:29 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 06.04.2020 19:12, Sowjanya Komatineni пишет:
>>>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 06.04.2020 18:35, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> +     /* wait for syncpt counter to reach frame start event
>>>>>>>> threshold */
>>>>>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>>>>>> +     if (err) {
>>>>>>>> +             dev_err(&chan->video.dev,
>>>>>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>>>>>> +             /* increment syncpoint counter for timedout events */
>>>>>>>> + host1x_syncpt_incr(chan->frame_start_sp);
>>>>>>> Why incrementing is done while hardware is still active?
>>>>>>>
>>>>>>> The sync point's state needs to be completely reset after resetting
>>>>>>> hardware. But I don't think that the current upstream host1x driver
>>>>>>> supports doing that, it's one of the known-long-standing problems of
>>>>>>> the
>>>>>>> host1x driver.
>>>>>>>
>>>>>>> At least the sp->max_val incrementing should be done based on the
>>>>>>> actual
>>>>>>> syncpoint value and this should be done after resetting hardware.
>>>>>> upstream host1x driver don't have API to reset or to equalize max
>>>>>> value
>>>>>> with min/load value.
>>>>>>
>>>>>> So to synchronize missed event, incrementing HW syncpt counter.
>>>>>>
>>>>>> This should not impact as we increment this in case of missed events
>>>>>> only.
>>>>> It's wrong to touch sync point while hardware is active and it's
>>>>> active
>>>>> until being reset.
>>>>>
>>>>> You should re-check the timeout after hw resetting and manually put
>>>>> the
>>>>> syncpoint counter back into sync only if needed.
>>>> There is possibility of timeout to happen any time even during the
>>>> capture also and is not related to hw reset.
>>>>
>>>> Manual synchronization is needed when timeout of any frame events
>>>> happen
>>>> otherwise all subsequence frames will timeout due to mismatch in event
>>>> counters.
>>> My point is that hardware is stopped only after being reset, until then
>>> you should assume that sync point could be incremented by HW at any
>>> time.
>>>
>>> And if this happens that HW increments sync point after the timeout,
>>> then the sync point counter should become out-of-sync in yours case,
>>> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.
>>
>> We wait for enough time based on frame rate for syncpt increment to
>> happen and if it doesn't happen by then definitely its missed event
>> and we increment HW syncpoint for this timed event.
>>
>> cached value gets updated during syncpt wait for subsequent event.
>>
>> syncpt increment happens for all subsequent frame events during video
>> capture.
>>
> Just to be clear, syncpt max value increment happens first and syncpt
> trigger condition is programmed. hw syncpt increment happens based on HW
> events.
> 
> Wait time for HW syncpt to reach threshold is tuned to work for all
> frame rates. So if increment doesn't happen by then, its definitely
> missed event.

This is questionable. Technically, speculating about whether the tuned
value is good for all possible cases is incorrect thing to do.

Although, I guess in practice it should be good enough for the starter
and could be improved later on, once the host1x driver will be improved.

> In case of missed HW event corresponding to syncpt condition, hw syncpt
> increment does not happen and driver increments it on timeout.
> 
> As there is not API to equialize max with min incase of timeout/reset,
> incrementing HW syncpt for timed out event.
> 
> syncpt cached value gets updated during syncpt wait when it loads from
> HW syncpt.
> 
> As syncpt condition is already triggered, without compensating timeout
> events or leaving syncpt max and hw syncpt in non synchronized state for
> missed events, subsequent streamings will all timeout even on real events.
> 

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 12:48 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan,
>> +                                    struct tegra_channel_buffer *buf)
>> +{
>> +     int err = 0;
>> +     u32 thresh, value, frame_start, mw_ack_done;
>> +     int bytes_per_line = chan->format.bytesperline;
>> +
>> +     /* program buffer address by using surface 0 */
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB,
>> +                  (u64)buf->addr >> 32);
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
>> +
>> +     /*
>> +      * Tegra VI block interacts with host1x syncpt for synchronizing
>> +      * programmed condition of capture state and hardware operation.
>> +      * Frame start and Memory write acknowledge syncpts has their own
>> +      * FIFO of depth 2.
>> +      *
>> +      * Syncpoint trigger conditions set through VI_INCR_SYNCPT register
>> +      * are added to HW syncpt FIFO and when the HW triggers, syncpt
>> +      * condition is removed from the FIFO and counter at syncpoint index
>> +      * will be incremented by the hardware and software can wait for
>> +      * counter to reach threshold to synchronize capturing frame with the
>> +      * hardware capture events.
>> +      */
>> +
>> +     /* increase channel syncpoint threshold for FRAME_START */
>> +     thresh = host1x_syncpt_incr_max(chan->frame_start_sp, 1);
>> +
>> +     /* Program FRAME_START trigger condition syncpt request */
>> +     frame_start = VI_CSI_PP_FRAME_START(chan->portno);
>> +     value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
>> +             host1x_syncpt_id(chan->frame_start_sp);
>> +     tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
>> +
>> +     /* increase channel syncpoint threshold for MW_ACK_DONE */
>> +     buf->mw_ack_sp_thresh = host1x_syncpt_incr_max(chan->mw_ack_sp, 1);
>> +
>> +     /* Program MW_ACK_DONE trigger condition syncpt request */
>> +     mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno);
>> +     value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
>> +             host1x_syncpt_id(chan->mw_ack_sp);
>> +     tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
>> +
>> +     /* enable single shot capture */
>> +     vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);
>> +     chan->capture_reqs++;
>> +
>> +     /* wait for syncpt counter to reach frame start event threshold */
>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>> +                              TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> What is the point of waiting for the frame-start? Why just not to wait
> for the frame-end?

Tegra vi supports double buffering where up on receiving frame start 
before HW received frame end and finish writing capture data to memory, 
we can issue next frame as well a head.

Also MW_ACK timeout can happen incase of HDMI2CSI bridges as well when 
hdmi hot plug happens.

For some sensors down the road we may need to skip few frames in case of 
frame start timeout as well which comes later with subsequent patch series.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static int chan_capture_kthread_start(void *data)
> +{
> +	struct tegra_vi_channel *chan = data;
> +	struct tegra_channel_buffer *buf;
> +	int err = 0;
> +	int caps_inflight;
> +
> +	set_freezable();
> +
> +	while (1) {
> +		try_to_freeze();
> +
> +		wait_event_interruptible(chan->start_wait,
> +					 !list_empty(&chan->capture) ||
> +					 kthread_should_stop());

Is it really okay that list_empty() isn't protected with a lock?

Why wait_event is "interruptible"?

> +		/*
> +		 * Frame start and MW_ACK_DONE syncpoint condition FIFOs are
> +		 * of max depth 2. So make sure max 2 capture requests are
> +		 * in process by the hardware at a time.
> +		 */
> +		while (!(kthread_should_stop() || list_empty(&chan->capture))) {
> +			caps_inflight = chan->capture_reqs - chan->sequence;
> +			/*
> +			 * Source is not streaming if error is non-zero.
> +			 * So, do not dequeue buffers on capture error or when
> +			 * syncpoint requests in FIFO are full.
> +			 */
> +			if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
> +				break;
> +
> +			/* dequeue the buffer and start capture */
> +			spin_lock(&chan->start_lock);
> +			if (list_empty(&chan->capture))
> +				break;
> +			buf = list_entry(chan->capture.next,
> +					 struct tegra_channel_buffer, queue);

list_first_entry()

> +			list_del_init(&buf->queue);
> +			spin_unlock(&chan->start_lock);
> +
> +			err = tegra_channel_capture_frame(chan, buf);
> +			if (err)
> +				vb2_queue_error(&chan->queue);
> +		}
> +
> +		if (kthread_should_stop())
> +			break;
> +	}
> +
> +	return 0;
> +}

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 12:53 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 20:02, Sowjanya Komatineni пишет:
>> On 4/6/20 9:37 AM, Sowjanya Komatineni wrote:
>>> On 4/6/20 9:29 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 06.04.2020 19:12, Sowjanya Komatineni пишет:
>>>>> On 4/6/20 9:05 AM, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 06.04.2020 18:35, Sowjanya Komatineni пишет:
>>>>>> ...
>>>>>>>>> +     /* wait for syncpt counter to reach frame start event
>>>>>>>>> threshold */
>>>>>>>>> +     err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
>>>>>>>>> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
>>>>>>>>> +     if (err) {
>>>>>>>>> +             dev_err(&chan->video.dev,
>>>>>>>>> +                     "frame start syncpt timeout: %d\n", err);
>>>>>>>>> +             /* increment syncpoint counter for timedout events */
>>>>>>>>> + host1x_syncpt_incr(chan->frame_start_sp);
>>>>>>>> Why incrementing is done while hardware is still active?
>>>>>>>>
>>>>>>>> The sync point's state needs to be completely reset after resetting
>>>>>>>> hardware. But I don't think that the current upstream host1x driver
>>>>>>>> supports doing that, it's one of the known-long-standing problems of
>>>>>>>> the
>>>>>>>> host1x driver.
>>>>>>>>
>>>>>>>> At least the sp->max_val incrementing should be done based on the
>>>>>>>> actual
>>>>>>>> syncpoint value and this should be done after resetting hardware.
>>>>>>> upstream host1x driver don't have API to reset or to equalize max
>>>>>>> value
>>>>>>> with min/load value.
>>>>>>>
>>>>>>> So to synchronize missed event, incrementing HW syncpt counter.
>>>>>>>
>>>>>>> This should not impact as we increment this in case of missed events
>>>>>>> only.
>>>>>> It's wrong to touch sync point while hardware is active and it's
>>>>>> active
>>>>>> until being reset.
>>>>>>
>>>>>> You should re-check the timeout after hw resetting and manually put
>>>>>> the
>>>>>> syncpoint counter back into sync only if needed.
>>>>> There is possibility of timeout to happen any time even during the
>>>>> capture also and is not related to hw reset.
>>>>>
>>>>> Manual synchronization is needed when timeout of any frame events
>>>>> happen
>>>>> otherwise all subsequence frames will timeout due to mismatch in event
>>>>> counters.
>>>> My point is that hardware is stopped only after being reset, until then
>>>> you should assume that sync point could be incremented by HW at any
>>>> time.
>>>>
>>>> And if this happens that HW increments sync point after the timeout,
>>>> then the sync point counter should become out-of-sync in yours case,
>>>> IIUC. Because host1x_syncpt_incr() doesn't update the cached counter.
>>> We wait for enough time based on frame rate for syncpt increment to
>>> happen and if it doesn't happen by then definitely its missed event
>>> and we increment HW syncpoint for this timed event.
>>>
>>> cached value gets updated during syncpt wait for subsequent event.
>>>
>>> syncpt increment happens for all subsequent frame events during video
>>> capture.
>>>
>> Just to be clear, syncpt max value increment happens first and syncpt
>> trigger condition is programmed. hw syncpt increment happens based on HW
>> events.
>>
>> Wait time for HW syncpt to reach threshold is tuned to work for all
>> frame rates. So if increment doesn't happen by then, its definitely
>> missed event.
> This is questionable. Technically, speculating about whether the tuned
> value is good for all possible cases is incorrect thing to do.
>
> Although, I guess in practice it should be good enough for the starter
> and could be improved later on, once the host1x driver will be improved.

By tuned value I meant about 200ms wait timeout for frame event to 
happen is what we have been using in downstream and with BSP release 
images which works good for all sensors and bridges we supported so far.

>
>> In case of missed HW event corresponding to syncpt condition, hw syncpt
>> increment does not happen and driver increments it on timeout.
>>
>> As there is not API to equialize max with min incase of timeout/reset,
>> incrementing HW syncpt for timed out event.
>>
>> syncpt cached value gets updated during syncpt wait when it loads from
>> HW syncpt.
>>
>> As syncpt condition is already triggered, without compensating timeout
>> events or leaving syncpt max and hw syncpt in non synchronized state for
>> missed events, subsequent streamings will all timeout even on real events.
>>

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static int chan_capture_kthread_start(void *data)
>> +{
>> +     struct tegra_vi_channel *chan = data;
>> +     struct tegra_channel_buffer *buf;
>> +     int err = 0;
>> +     int caps_inflight;
>> +
>> +     set_freezable();
>> +
>> +     while (1) {
>> +             try_to_freeze();
>> +
>> +             wait_event_interruptible(chan->start_wait,
>> +                                      !list_empty(&chan->capture) ||
>> +                                      kthread_should_stop());
> Is it really okay that list_empty() isn't protected with a lock?
>
> Why wait_event is "interruptible"?

To allow it to sleep until wakeup on thread it to avoid constant 
checking for condition even when no buffers are ready, basically to 
prevent blocking.

>
>> +             /*
>> +              * Frame start and MW_ACK_DONE syncpoint condition FIFOs are
>> +              * of max depth 2. So make sure max 2 capture requests are
>> +              * in process by the hardware at a time.
>> +              */
>> +             while (!(kthread_should_stop() || list_empty(&chan->capture))) {
>> +                     caps_inflight = chan->capture_reqs - chan->sequence;
>> +                     /*
>> +                      * Source is not streaming if error is non-zero.
>> +                      * So, do not dequeue buffers on capture error or when
>> +                      * syncpoint requests in FIFO are full.
>> +                      */
>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>> +                             break;
>> +
>> +                     /* dequeue the buffer and start capture */
>> +                     spin_lock(&chan->start_lock);
>> +                     if (list_empty(&chan->capture))
>> +                             break;
>> +                     buf = list_entry(chan->capture.next,
>> +                                      struct tegra_channel_buffer, queue);
> list_first_entry()
>
>> +                     list_del_init(&buf->queue);
>> +                     spin_unlock(&chan->start_lock);
>> +
>> +                     err = tegra_channel_capture_frame(chan, buf);
>> +                     if (err)
>> +                             vb2_queue_error(&chan->queue);
>> +             }
>> +
>> +             if (kthread_should_stop())
>> +                     break;
>> +     }
>> +
>> +     return 0;
>> +}

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 23:05, Sowjanya Komatineni пишет:
..
>>> Wait time for HW syncpt to reach threshold is tuned to work for all
>>> frame rates. So if increment doesn't happen by then, its definitely
>>> missed event.
>> This is questionable. Technically, speculating about whether the tuned
>> value is good for all possible cases is incorrect thing to do.
>>
>> Although, I guess in practice it should be good enough for the starter
>> and could be improved later on, once the host1x driver will be improved.
> 
> By tuned value I meant about 200ms wait timeout for frame event to
> happen is what we have been using in downstream and with BSP release
> images which works good for all sensors and bridges we supported so far.

I don't know anything about the state of today's downstream, but
downstream of older Tegra SoCs was pretty awful in regards to the host1x
syncing, unfortunately it was borrowed into the upstream host1x years
ago and nothing was done about it so far. I'd suggest to be careful
about it.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:28 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 23:05, Sowjanya Komatineni пишет:
> ..
>>>> Wait time for HW syncpt to reach threshold is tuned to work for all
>>>> frame rates. So if increment doesn't happen by then, its definitely
>>>> missed event.
>>> This is questionable. Technically, speculating about whether the tuned
>>> value is good for all possible cases is incorrect thing to do.
>>>
>>> Although, I guess in practice it should be good enough for the starter
>>> and could be improved later on, once the host1x driver will be improved.
>> By tuned value I meant about 200ms wait timeout for frame event to
>> happen is what we have been using in downstream and with BSP release
>> images which works good for all sensors and bridges we supported so far.
> I don't know anything about the state of today's downstream, but
> downstream of older Tegra SoCs was pretty awful in regards to the host1x
> syncing, unfortunately it was borrowed into the upstream host1x years
> ago and nothing was done about it so far. I'd suggest to be careful
> about it.
200ms timeout we wait for event to happen is the case even with 
T186/T194 as well and internally it was tuned from lots of testing with 
various sensors and frame rate computations which is known to work good.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 23:20, Sowjanya Komatineni пишет:
> 
> On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>> ...
>>> +static int chan_capture_kthread_start(void *data)
>>> +{
>>> +     struct tegra_vi_channel *chan = data;
>>> +     struct tegra_channel_buffer *buf;
>>> +     int err = 0;
>>> +     int caps_inflight;
>>> +
>>> +     set_freezable();
>>> +
>>> +     while (1) {
>>> +             try_to_freeze();
>>> +
>>> +             wait_event_interruptible(chan->start_wait,
>>> +                                      !list_empty(&chan->capture) ||
>>> +                                      kthread_should_stop());
>> Is it really okay that list_empty() isn't protected with a lock?
>>
>> Why wait_event is "interruptible"?
> 
> To allow it to sleep until wakeup on thread it to avoid constant
> checking for condition even when no buffers are ready, basically to
> prevent blocking.

So the "interrupt" is for getting event about kthread_should_stop(),
correct?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:37 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 23:20, Sowjanya Komatineni пишет:
>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>> ...
>>>> +static int chan_capture_kthread_start(void *data)
>>>> +{
>>>> +     struct tegra_vi_channel *chan = data;
>>>> +     struct tegra_channel_buffer *buf;
>>>> +     int err = 0;
>>>> +     int caps_inflight;
>>>> +
>>>> +     set_freezable();
>>>> +
>>>> +     while (1) {
>>>> +             try_to_freeze();
>>>> +
>>>> +             wait_event_interruptible(chan->start_wait,
>>>> +                                      !list_empty(&chan->capture) ||
>>>> +                                      kthread_should_stop());
>>> Is it really okay that list_empty() isn't protected with a lock?
>>>
>>> Why wait_event is "interruptible"?
>> To allow it to sleep until wakeup on thread it to avoid constant
>> checking for condition even when no buffers are ready, basically to
>> prevent blocking.
> So the "interrupt" is for getting event about kthread_should_stop(),
> correct?
also to prevent blocking and to let is sleep and wakeup based on wait 
queue to evaluate condition to proceed with the task
>

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:38 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 1:37 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 06.04.2020 23:20, Sowjanya Komatineni пишет:
>>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>> ...
>>>>> +static int chan_capture_kthread_start(void *data)
>>>>> +{
>>>>> +     struct tegra_vi_channel *chan = data;
>>>>> +     struct tegra_channel_buffer *buf;
>>>>> +     int err = 0;
>>>>> +     int caps_inflight;
>>>>> +
>>>>> +     set_freezable();
>>>>> +
>>>>> +     while (1) {
>>>>> +             try_to_freeze();
>>>>> +
>>>>> + wait_event_interruptible(chan->start_wait,
>>>>> + !list_empty(&chan->capture) ||
>>>>> + kthread_should_stop());
>>>> Is it really okay that list_empty() isn't protected with a lock?

wakeup on thread happens either when buffer is moved to capture list or 
on stop signaling event.

So in this specific case we may not need to check for lock on capture 
list as if wakeup happens from start wait queue, then buffer is already 
moved to capture list by then.

>>>>
>>>> Why wait_event is "interruptible"?
>>> To allow it to sleep until wakeup on thread it to avoid constant
>>> checking for condition even when no buffers are ready, basically to
>>> prevent blocking.
>> So the "interrupt" is for getting event about kthread_should_stop(),
>> correct?
> also to prevent blocking and to let is sleep and wakeup based on wait 
> queue to evaluate condition to proceed with the task
>>

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
> +static int chan_capture_kthread_start(void *data)
> +{
> +	struct tegra_vi_channel *chan = data;
> +	struct tegra_channel_buffer *buf;
> +	int err = 0;
> +	int caps_inflight;
> +
> +	set_freezable();
> +
> +	while (1) {
> +		try_to_freeze();
> +
> +		wait_event_interruptible(chan->start_wait,
> +					 !list_empty(&chan->capture) ||
> +					 kthread_should_stop());
> +		/*
> +		 * Frame start and MW_ACK_DONE syncpoint condition FIFOs are
> +		 * of max depth 2. So make sure max 2 capture requests are
> +		 * in process by the hardware at a time.
> +		 */
> +		while (!(kthread_should_stop() || list_empty(&chan->capture))) {
> +			caps_inflight = chan->capture_reqs - chan->sequence;
> +			/*
> +			 * Source is not streaming if error is non-zero.
> +			 * So, do not dequeue buffers on capture error or when
> +			 * syncpoint requests in FIFO are full.
> +			 */
> +			if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
> +				break;

Am I understanding correctly that this thread will take 100% CPU,
spinning here, if more than 2 frame-captures queued?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:45 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>> +static int chan_capture_kthread_start(void *data)
>> +{
>> +     struct tegra_vi_channel *chan = data;
>> +     struct tegra_channel_buffer *buf;
>> +     int err = 0;
>> +     int caps_inflight;
>> +
>> +     set_freezable();
>> +
>> +     while (1) {
>> +             try_to_freeze();
>> +
>> +             wait_event_interruptible(chan->start_wait,
>> +                                      !list_empty(&chan->capture) ||
>> +                                      kthread_should_stop());
>> +             /*
>> +              * Frame start and MW_ACK_DONE syncpoint condition FIFOs are
>> +              * of max depth 2. So make sure max 2 capture requests are
>> +              * in process by the hardware at a time.
>> +              */
>> +             while (!(kthread_should_stop() || list_empty(&chan->capture))) {
>> +                     caps_inflight = chan->capture_reqs - chan->sequence;
>> +                     /*
>> +                      * Source is not streaming if error is non-zero.
>> +                      * So, do not dequeue buffers on capture error or when
>> +                      * syncpoint requests in FIFO are full.
>> +                      */
>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>> +                             break;
> Am I understanding correctly that this thread will take 100% CPU,
> spinning here, if more than 2 frame-captures queued?
on more than 2 frames captures, it breaks thread and on next wakeup it 
continues

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 23:50, Sowjanya Komatineni пишет:
> 
> On 4/6/20 1:45 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>> +static int chan_capture_kthread_start(void *data)
>>> +{
>>> +     struct tegra_vi_channel *chan = data;
>>> +     struct tegra_channel_buffer *buf;
>>> +     int err = 0;
>>> +     int caps_inflight;
>>> +
>>> +     set_freezable();
>>> +
>>> +     while (1) {
>>> +             try_to_freeze();
>>> +
>>> +             wait_event_interruptible(chan->start_wait,
>>> +                                      !list_empty(&chan->capture) ||
>>> +                                      kthread_should_stop());
>>> +             /*
>>> +              * Frame start and MW_ACK_DONE syncpoint condition
>>> FIFOs are
>>> +              * of max depth 2. So make sure max 2 capture requests are
>>> +              * in process by the hardware at a time.
>>> +              */
>>> +             while (!(kthread_should_stop() ||
>>> list_empty(&chan->capture))) {
>>> +                     caps_inflight = chan->capture_reqs -
>>> chan->sequence;
>>> +                     /*
>>> +                      * Source is not streaming if error is non-zero.
>>> +                      * So, do not dequeue buffers on capture error
>>> or when
>>> +                      * syncpoint requests in FIFO are full.
>>> +                      */
>>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>>> +                             break;
>> Am I understanding correctly that this thread will take 100% CPU,
>> spinning here, if more than 2 frame-captures queued?
> on more than 2 frames captures, it breaks thread and on next wakeup it
> continues

The wait_event() won't wait if condition is true.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 23:38, Sowjanya Komatineni пишет:
> 
> On 4/6/20 1:37 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 06.04.2020 23:20, Sowjanya Komatineni пишет:
>>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>> ...
>>>>> +static int chan_capture_kthread_start(void *data)
>>>>> +{
>>>>> +     struct tegra_vi_channel *chan = data;
>>>>> +     struct tegra_channel_buffer *buf;
>>>>> +     int err = 0;
>>>>> +     int caps_inflight;
>>>>> +
>>>>> +     set_freezable();
>>>>> +
>>>>> +     while (1) {
>>>>> +             try_to_freeze();
>>>>> +
>>>>> +             wait_event_interruptible(chan->start_wait,
>>>>> +                                      !list_empty(&chan->capture) ||
>>>>> +                                      kthread_should_stop());
>>>> Is it really okay that list_empty() isn't protected with a lock?
>>>>
>>>> Why wait_event is "interruptible"?
>>> To allow it to sleep until wakeup on thread it to avoid constant
>>> checking for condition even when no buffers are ready, basically to
>>> prevent blocking.
>> So the "interrupt" is for getting event about kthread_should_stop(),
>> correct?
> also to prevent blocking and to let is sleep and wakeup based on wait
> queue to evaluate condition to proceed with the task
>>

This looks suspicious, the comment to wait_event_interruptible() says
that it will return ERESTARTSYS if signal is recieved..

Does this mean that I can send signal from userspace to wake it up?

The "interruptible" part looks wrong to me.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:53 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 23:50, Sowjanya Komatineni пишет:
>> On 4/6/20 1:45 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>> +static int chan_capture_kthread_start(void *data)
>>>> +{
>>>> +     struct tegra_vi_channel *chan = data;
>>>> +     struct tegra_channel_buffer *buf;
>>>> +     int err = 0;
>>>> +     int caps_inflight;
>>>> +
>>>> +     set_freezable();
>>>> +
>>>> +     while (1) {
>>>> +             try_to_freeze();
>>>> +
>>>> +             wait_event_interruptible(chan->start_wait,
>>>> +                                      !list_empty(&chan->capture) ||
>>>> +                                      kthread_should_stop());
>>>> +             /*
>>>> +              * Frame start and MW_ACK_DONE syncpoint condition
>>>> FIFOs are
>>>> +              * of max depth 2. So make sure max 2 capture requests are
>>>> +              * in process by the hardware at a time.
>>>> +              */
>>>> +             while (!(kthread_should_stop() ||
>>>> list_empty(&chan->capture))) {
>>>> +                     caps_inflight = chan->capture_reqs -
>>>> chan->sequence;
>>>> +                     /*
>>>> +                      * Source is not streaming if error is non-zero.
>>>> +                      * So, do not dequeue buffers on capture error
>>>> or when
>>>> +                      * syncpoint requests in FIFO are full.
>>>> +                      */
>>>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>>>> +                             break;
>>> Am I understanding correctly that this thread will take 100% CPU,
>>> spinning here, if more than 2 frame-captures queued?
>> on more than 2 frames captures, it breaks thread and on next wakeup it
>> continues
> The wait_event() won't wait if condition is true.
condition is checked when waitqueue is woken up

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

06.04.2020 23:55, Sowjanya Komatineni пишет:
> 
> On 4/6/20 1:53 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 06.04.2020 23:50, Sowjanya Komatineni пишет:
>>> On 4/6/20 1:45 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>>> +static int chan_capture_kthread_start(void *data)
>>>>> +{
>>>>> +     struct tegra_vi_channel *chan = data;
>>>>> +     struct tegra_channel_buffer *buf;
>>>>> +     int err = 0;
>>>>> +     int caps_inflight;
>>>>> +
>>>>> +     set_freezable();
>>>>> +
>>>>> +     while (1) {
>>>>> +             try_to_freeze();
>>>>> +
>>>>> +             wait_event_interruptible(chan->start_wait,
>>>>> +                                      !list_empty(&chan->capture) ||
>>>>> +                                      kthread_should_stop());
>>>>> +             /*
>>>>> +              * Frame start and MW_ACK_DONE syncpoint condition
>>>>> FIFOs are
>>>>> +              * of max depth 2. So make sure max 2 capture
>>>>> requests are
>>>>> +              * in process by the hardware at a time.
>>>>> +              */
>>>>> +             while (!(kthread_should_stop() ||
>>>>> list_empty(&chan->capture))) {
>>>>> +                     caps_inflight = chan->capture_reqs -
>>>>> chan->sequence;
>>>>> +                     /*
>>>>> +                      * Source is not streaming if error is non-zero.
>>>>> +                      * So, do not dequeue buffers on capture error
>>>>> or when
>>>>> +                      * syncpoint requests in FIFO are full.
>>>>> +                      */
>>>>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>>>>> +                             break;
>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>> spinning here, if more than 2 frame-captures queued?
>>> on more than 2 frames captures, it breaks thread and on next wakeup it
>>> continues
>> The wait_event() won't wait if condition is true.
> condition is checked when waitqueue is woken up

https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:56 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 23:55, Sowjanya Komatineni пишет:
>> On 4/6/20 1:53 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 06.04.2020 23:50, Sowjanya Komatineni пишет:
>>>> On 4/6/20 1:45 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>>>> +static int chan_capture_kthread_start(void *data)
>>>>>> +{
>>>>>> +     struct tegra_vi_channel *chan = data;
>>>>>> +     struct tegra_channel_buffer *buf;
>>>>>> +     int err = 0;
>>>>>> +     int caps_inflight;
>>>>>> +
>>>>>> +     set_freezable();
>>>>>> +
>>>>>> +     while (1) {
>>>>>> +             try_to_freeze();
>>>>>> +
>>>>>> +             wait_event_interruptible(chan->start_wait,
>>>>>> +                                      !list_empty(&chan->capture) ||
>>>>>> +                                      kthread_should_stop());
>>>>>> +             /*
>>>>>> +              * Frame start and MW_ACK_DONE syncpoint condition
>>>>>> FIFOs are
>>>>>> +              * of max depth 2. So make sure max 2 capture
>>>>>> requests are
>>>>>> +              * in process by the hardware at a time.
>>>>>> +              */
>>>>>> +             while (!(kthread_should_stop() ||
>>>>>> list_empty(&chan->capture))) {
>>>>>> +                     caps_inflight = chan->capture_reqs -
>>>>>> chan->sequence;
>>>>>> +                     /*
>>>>>> +                      * Source is not streaming if error is non-zero.
>>>>>> +                      * So, do not dequeue buffers on capture error
>>>>>> or when
>>>>>> +                      * syncpoint requests in FIFO are full.
>>>>>> +                      */
>>>>>> +                     if (err || caps_inflight >= SYNCPT_FIFO_DEPTH)
>>>>>> +                             break;
>>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>>> spinning here, if more than 2 frame-captures queued?
>>>> on more than 2 frames captures, it breaks thread and on next wakeup it
>>>> continues
>>> The wait_event() won't wait if condition is true.
>> condition is checked when waitqueue is woken up
> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462

process is put to sleep until the condition evaluates to true or signal 
is received.

condition is checked each time the waitqueue head is woken up.

Also capture list may keep on getting updated with buffers from userspace.

but at a time we only limit 2 frames as VI supports double buffering and 
syncpt fifo's max depth is 2

Any more buffers waiting will be processing on subsequent iterations.

So basically thread run time is depending on buffers getting queued from 
userspace.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>> on more than 2 frames captures, it breaks thread and on next wakeup it
>>>>> continues
>>>> The wait_event() won't wait if condition is true.
>>> condition is checked when waitqueue is woken up
>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462
> 
> process is put to sleep until the condition evaluates to true or signal
> is received.
> 
> condition is checked each time the waitqueue head is woken up.

This is a wrong assumption in accordance to the code.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>> on more than 2 frames captures, it breaks thread and on next wakeup it
>>>>>> continues
>>>>> The wait_event() won't wait if condition is true.
>>>> condition is checked when waitqueue is woken up
>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462
>> process is put to sleep until the condition evaluates to true or signal
>> is received.
>>
>> condition is checked each time the waitqueue head is woken up.
> This is a wrong assumption in accordance to the code.

when every buffer is available as long as we are in streaming, we should 
process it.

So if wake up happens when list has buffer, it will be processed but at 
a time we limit processing 2 simultaneous buffer capture starts only.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 1:54 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 23:38, Sowjanya Komatineni пишет:
>> On 4/6/20 1:37 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 06.04.2020 23:20, Sowjanya Komatineni пишет:
>>>> On 4/6/20 1:02 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>> +static int chan_capture_kthread_start(void *data)
>>>>>> +{
>>>>>> +     struct tegra_vi_channel *chan = data;
>>>>>> +     struct tegra_channel_buffer *buf;
>>>>>> +     int err = 0;
>>>>>> +     int caps_inflight;
>>>>>> +
>>>>>> +     set_freezable();
>>>>>> +
>>>>>> +     while (1) {
>>>>>> +             try_to_freeze();
>>>>>> +
>>>>>> +             wait_event_interruptible(chan->start_wait,
>>>>>> +                                      !list_empty(&chan->capture) ||
>>>>>> +                                      kthread_should_stop());
>>>>> Is it really okay that list_empty() isn't protected with a lock?
>>>>>
>>>>> Why wait_event is "interruptible"?
>>>> To allow it to sleep until wakeup on thread it to avoid constant
>>>> checking for condition even when no buffers are ready, basically to
>>>> prevent blocking.
>>> So the "interrupt" is for getting event about kthread_should_stop(),
>>> correct?
>> also to prevent blocking and to let is sleep and wakeup based on wait
>> queue to evaluate condition to proceed with the task
> This looks suspicious, the comment to wait_event_interruptible() says
> that it will return ERESTARTSYS if signal is recieved..
>
> Does this mean that I can send signal from userspace to wake it up?
>
> The "interruptible" part looks wrong to me.

We are not checking for wait_event_interruptible to handle case when it 
returns ERESTARTSYS.

So, signals sent from user space are ignore and we check if when wakeup 
happens if kthread_stop has requested to stop thread.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>> on more than 2 frames captures, it breaks thread and on next 
>>>>>>> wakeup it
>>>>>>> continues
>>>>>> The wait_event() won't wait if condition is true.
>>>>> condition is checked when waitqueue is woken up
>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 
>>>>
>>> process is put to sleep until the condition evaluates to true or signal
>>> is received.
>>>
>>> condition is checked each time the waitqueue head is woken up.
>> This is a wrong assumption in accordance to the code.
>
> when every buffer is available as long as we are in streaming, we 
> should process it.
>
> So if wake up happens when list has buffer, it will be processed but 
> at a time we limit processing 2 simultaneous buffer capture starts only.
>
Fixing typo.

I meant when ever buffer is available as long as we are in streaming, we 
should process it.

So capture thread processes as long as buffers are available from user 
space limiting 2 simultaneous trigger of captures and thread will be in 
sleep when capture buffers list is empty or no stop thread is signaled.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>
>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>> Am I understanding correctly that this thread will take 100% CPU,
>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>> on more than 2 frames captures, it breaks thread and on next 
>>>>>>>> wakeup it
>>>>>>>> continues
>>>>>>> The wait_event() won't wait if condition is true.
>>>>>> condition is checked when waitqueue is woken up
>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 
>>>>>
>>>> process is put to sleep until the condition evaluates to true or 
>>>> signal
>>>> is received.
>>>>
>>>> condition is checked each time the waitqueue head is woken up.
>>> This is a wrong assumption in accordance to the code.

process is in sleep until the condition is evaluated and when condition 
is true wakeup still happens only when wake_up on waitqueue is called

This is the reason for using this to prevent blocking while waiting for 
the buffers.

>>
>> when every buffer is available as long as we are in streaming, we 
>> should process it.
>>
>> So if wake up happens when list has buffer, it will be processed but 
>> at a time we limit processing 2 simultaneous buffer capture starts only.
>>
> Fixing typo.
>
> I meant when ever buffer is available as long as we are in streaming, 
> we should process it.
>
> So capture thread processes as long as buffers are available from user 
> space limiting 2 simultaneous trigger of captures and thread will be 
> in sleep when capture buffers list is empty or no stop thread is 
> signaled.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 3:00 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>>
>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>>
>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>>> Am I understanding correctly that this thread will take 100% 
>>>>>>>>>> CPU,
>>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>>> on more than 2 frames captures, it breaks thread and on next 
>>>>>>>>> wakeup it
>>>>>>>>> continues
>>>>>>>> The wait_event() won't wait if condition is true.
>>>>>>> condition is checked when waitqueue is woken up
>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 
>>>>>>
>>>>> process is put to sleep until the condition evaluates to true or 
>>>>> signal
>>>>> is received.
>>>>>
>>>>> condition is checked each time the waitqueue head is woken up.
>>>> This is a wrong assumption in accordance to the code.
>
> process is in sleep until the condition is evaluated and when 
> condition is true wakeup still happens only when wake_up on waitqueue 
> is called
>
> This is the reason for using this to prevent blocking while waiting 
> for the buffers.

w.r.t capture list update, wakeup happens when wake_up on waitqueue is 
called.

wakeup also happens on kthread stop signal event.

>
>
>>>
>>> when every buffer is available as long as we are in streaming, we 
>>> should process it.
>>>
>>> So if wake up happens when list has buffer, it will be processed but 
>>> at a time we limit processing 2 simultaneous buffer capture starts 
>>> only.
>>>
>> Fixing typo.
>>
>> I meant when ever buffer is available as long as we are in streaming, 
>> we should process it.
>>
>> So capture thread processes as long as buffers are available from 
>> user space limiting 2 simultaneous trigger of captures and thread 
>> will be in sleep when capture buffers list is empty or no stop thread 
>> is signaled.
>
>
>

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

07.04.2020 01:07, Sowjanya Komatineni пишет:
> 
> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote:
>>
>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>>>
>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>>>
>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>>>> Am I understanding correctly that this thread will take 100%
>>>>>>>>>>> CPU,
>>>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next
>>>>>>>>>> wakeup it
>>>>>>>>>> continues
>>>>>>>>> The wait_event() won't wait if condition is true.
>>>>>>>> condition is checked when waitqueue is woken up
>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462
>>>>>>>
>>>>>> process is put to sleep until the condition evaluates to true or
>>>>>> signal
>>>>>> is received.
>>>>>>
>>>>>> condition is checked each time the waitqueue head is woken up.
>>>>> This is a wrong assumption in accordance to the code.
>>
>> process is in sleep until the condition is evaluated and when
>> condition is true wakeup still happens only when wake_up on waitqueue
>> is called
>>
>> This is the reason for using this to prevent blocking while waiting
>> for the buffers.
> 
> w.r.t capture list update, wakeup happens when wake_up on waitqueue is
> called.
> 
> wakeup also happens on kthread stop signal event.
> 
>>
>>
>>>>
>>>> when every buffer is available as long as we are in streaming, we
>>>> should process it.
>>>>
>>>> So if wake up happens when list has buffer, it will be processed but
>>>> at a time we limit processing 2 simultaneous buffer capture starts
>>>> only.
>>>>
>>> Fixing typo.
>>>
>>> I meant when ever buffer is available as long as we are in streaming,
>>> we should process it.
>>>
>>> So capture thread processes as long as buffers are available from
>>> user space limiting 2 simultaneous trigger of captures and thread
>>> will be in sleep when capture buffers list is empty or no stop thread
>>> is signaled.

IIUC, the waiting won't happen if more than 2 captures are queued and
thread will be spinning until captures are processed.

I think you need a semaphore with resource count = 2.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 4:18 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 07.04.2020 01:07, Sowjanya Komatineni пишет:
>> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote:
>>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>>>>> Am I understanding correctly that this thread will take 100%
>>>>>>>>>>>> CPU,
>>>>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next
>>>>>>>>>>> wakeup it
>>>>>>>>>>> continues
>>>>>>>>>> The wait_event() won't wait if condition is true.
>>>>>>>>> condition is checked when waitqueue is woken up
>>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462
>>>>>>>>
>>>>>>> process is put to sleep until the condition evaluates to true or
>>>>>>> signal
>>>>>>> is received.
>>>>>>>
>>>>>>> condition is checked each time the waitqueue head is woken up.
>>>>>> This is a wrong assumption in accordance to the code.
>>> process is in sleep until the condition is evaluated and when
>>> condition is true wakeup still happens only when wake_up on waitqueue
>>> is called
>>>
>>> This is the reason for using this to prevent blocking while waiting
>>> for the buffers.
>> w.r.t capture list update, wakeup happens when wake_up on waitqueue is
>> called.
>>
>> wakeup also happens on kthread stop signal event.
>>
>>>
>>>>> when every buffer is available as long as we are in streaming, we
>>>>> should process it.
>>>>>
>>>>> So if wake up happens when list has buffer, it will be processed but
>>>>> at a time we limit processing 2 simultaneous buffer capture starts
>>>>> only.
>>>>>
>>>> Fixing typo.
>>>>
>>>> I meant when ever buffer is available as long as we are in streaming,
>>>> we should process it.
>>>>
>>>> So capture thread processes as long as buffers are available from
>>>> user space limiting 2 simultaneous trigger of captures and thread
>>>> will be in sleep when capture buffers list is empty or no stop thread
>>>> is signaled.
> IIUC, the waiting won't happen if more than 2 captures are queued and
> thread will be spinning until captures are processed.
>
> I think you need a semaphore with resource count = 2.
we hold on to issuing capture if more than 2 buffers are queued and it 
continues only after fifo has min 1 slot empty

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 4:48 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 4:18 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 07.04.2020 01:07, Sowjanya Komatineni пишет:
>>> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote:
>>>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>>>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> Am I understanding correctly that this thread will take 100%
>>>>>>>>>>>>> CPU,
>>>>>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next
>>>>>>>>>>>> wakeup it
>>>>>>>>>>>> continues
>>>>>>>>>>> The wait_event() won't wait if condition is true.
>>>>>>>>>> condition is checked when waitqueue is woken up
>>>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 
>>>>>>>>>
>>>>>>>>>
>>>>>>>> process is put to sleep until the condition evaluates to true or
>>>>>>>> signal
>>>>>>>> is received.
>>>>>>>>
>>>>>>>> condition is checked each time the waitqueue head is woken up.
>>>>>>> This is a wrong assumption in accordance to the code.
>>>> process is in sleep until the condition is evaluated and when
>>>> condition is true wakeup still happens only when wake_up on waitqueue
>>>> is called
>>>>
>>>> This is the reason for using this to prevent blocking while waiting
>>>> for the buffers.
>>> w.r.t capture list update, wakeup happens when wake_up on waitqueue is
>>> called.
>>>
>>> wakeup also happens on kthread stop signal event.
>>>
>>>>
>>>>>> when every buffer is available as long as we are in streaming, we
>>>>>> should process it.
>>>>>>
>>>>>> So if wake up happens when list has buffer, it will be processed but
>>>>>> at a time we limit processing 2 simultaneous buffer capture starts
>>>>>> only.
>>>>>>
>>>>> Fixing typo.
>>>>>
>>>>> I meant when ever buffer is available as long as we are in streaming,
>>>>> we should process it.
>>>>>
>>>>> So capture thread processes as long as buffers are available from
>>>>> user space limiting 2 simultaneous trigger of captures and thread
>>>>> will be in sleep when capture buffers list is empty or no stop thread
>>>>> is signaled.
>> IIUC, the waiting won't happen if more than 2 captures are queued and
>> thread will be spinning until captures are processed.
>>
>> I think you need a semaphore with resource count = 2.
> we hold on to issuing capture if more than 2 buffers are queued and it 
> continues only after fifo has min 1 slot empty
caps_inflight gets updated based on requested frame and finished frames 
and capture will happen only for 2 frames at a time but not more

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 9:11 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 06.04.2020 18:41, Sowjanya Komatineni пишет:
>> On 4/5/20 2:11 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 04.04.2020 04:25, Sowjanya Komatineni пишет:
>>> ...
>>>> +static int tegra_vi_tpg_channels_alloc(struct tegra_vi *vi)
>>>> +{
>>>> +     struct tegra_vi_channel *chan, *tmp;
>>>> +     unsigned int port_num;
>>>> +     unsigned int nchannels = vi->soc->vi_max_channels;
>>>> +     int ret = 0;
>>>> +
>>>> +     for (port_num = 0; port_num < nchannels; port_num++) {
>>>> +             /*
>>>> +              * Do not use devm_kzalloc as memory is freed immediately
>>>> +              * when device instance is unbound but application
>>>> might still
>>>> +              * be holding the device node open. Channel memory
>>>> allocated
>>>> +              * with kzalloc is freed during video device release
>>>> callback.
>>>> +              */
>>>> +             chan = kzalloc(sizeof(*chan), GFP_KERNEL);
>>> Why anyone would want to unbind this driver in practice?
>>>
>>> I think it should make more sense to set suppress_bind_attrs=true.
>>  From the previous feedback of patch series, we need to support
>> unbind/bind and looks like this driver should also support to built as a
>> module.
> If module unloading is also affected, then perhaps you should use
> get/put_device() to not allow freeing the resources until they're still
> in-use.
>
> I suppose that it should be up to the V4L core to keep the device alive
> while needed, rather than to put the burden to the individual drivers.

Hans/Thierry, Can you please comment on this?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
...
> +static const struct dev_pm_ops tegra_vi_pm_ops = {
> +	SET_RUNTIME_PM_OPS(vi_runtime_suspend, vi_runtime_resume, NULL)
> +};

Aren't the suspend/resume ops needed?

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 12:39 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 04:25, Sowjanya Komatineni пишет:
> ...
>> +static const struct dev_pm_ops tegra_vi_pm_ops = {
>> +     SET_RUNTIME_PM_OPS(vi_runtime_suspend, vi_runtime_resume, NULL)
>> +};
> Aren't the suspend/resume ops needed?
Complete driver suspend/resume will be implemented later after next 
series of sensor support

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/6/20 4:48 PM, Sowjanya Komatineni wrote:
>
> On 4/6/20 4:18 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 07.04.2020 01:07, Sowjanya Komatineni пишет:
>>> On 4/6/20 3:00 PM, Sowjanya Komatineni wrote:
>>>> On 4/6/20 2:39 PM, Sowjanya Komatineni wrote:
>>>>> On 4/6/20 2:15 PM, Sowjanya Komatineni wrote:
>>>>>> On 4/6/20 2:11 PM, Dmitry Osipenko wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> 07.04.2020 00:02, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> Am I understanding correctly that this thread will take 100%
>>>>>>>>>>>>> CPU,
>>>>>>>>>>>>> spinning here, if more than 2 frame-captures queued?
>>>>>>>>>>>> on more than 2 frames captures, it breaks thread and on next
>>>>>>>>>>>> wakeup it
>>>>>>>>>>>> continues
>>>>>>>>>>> The wait_event() won't wait if condition is true.
>>>>>>>>>> condition is checked when waitqueue is woken up
>>>>>>>>> https://elixir.bootlin.com/linux/v5.6.2/source/include/linux/wait.h#L462 
>>>>>>>>>
>>>>>>>>>
>>>>>>>> process is put to sleep until the condition evaluates to true or
>>>>>>>> signal
>>>>>>>> is received.
>>>>>>>>
>>>>>>>> condition is checked each time the waitqueue head is woken up.
>>>>>>> This is a wrong assumption in accordance to the code.
>>>> process is in sleep until the condition is evaluated and when
>>>> condition is true wakeup still happens only when wake_up on waitqueue
>>>> is called
>>>>
>>>> This is the reason for using this to prevent blocking while waiting
>>>> for the buffers.
>>> w.r.t capture list update, wakeup happens when wake_up on waitqueue is
>>> called.
>>>
>>> wakeup also happens on kthread stop signal event.
>>>
>>>>
>>>>>> when every buffer is available as long as we are in streaming, we
>>>>>> should process it.
>>>>>>
>>>>>> So if wake up happens when list has buffer, it will be processed but
>>>>>> at a time we limit processing 2 simultaneous buffer capture starts
>>>>>> only.
>>>>>>
>>>>> Fixing typo.
>>>>>
>>>>> I meant when ever buffer is available as long as we are in streaming,
>>>>> we should process it.
>>>>>
>>>>> So capture thread processes as long as buffers are available from
>>>>> user space limiting 2 simultaneous trigger of captures and thread
>>>>> will be in sleep when capture buffers list is empty or no stop thread
>>>>> is signaled.
>> IIUC, the waiting won't happen if more than 2 captures are queued and
>> thread will be spinning until captures are processed.
>>
>> I think you need a semaphore with resource count = 2.
> we hold on to issuing capture if more than 2 buffers are queued and it 
> continues only after fifo has min 1 slot empty


Just want to close on this part of feedback. Hope above explanation is 
clear regarding triggering/issuing at max 2 frame capture to VI HW and 
also regarding capture threads where they use wait_event_interruptible 
to prevent blocking waiting for buffers to be available for captures.

So no changes related to this part are needed in v7.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

08.04.2020 00:08, Sowjanya Komatineni пишет:
...
>>> I think you need a semaphore with resource count = 2.
>> we hold on to issuing capture if more than 2 buffers are queued and it
>> continues only after fifo has min 1 slot empty
> 
> 
> Just want to close on this part of feedback. Hope above explanation is
> clear regarding triggering/issuing at max 2 frame capture to VI HW and
> also regarding capture threads where they use wait_event_interruptible
> to prevent blocking waiting for buffers to be available for captures.
> 
> So no changes related to this part are needed in v7.
From what I see in the code, you "hold on" by making kthread to spin in
a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
should be needed to prevent this.

The wait_event_interruptible seems should be okay.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

08.04.2020 01:08, Dmitry Osipenko пишет:
> 08.04.2020 00:08, Sowjanya Komatineni пишет:
> ...
>>>> I think you need a semaphore with resource count = 2.
>>> we hold on to issuing capture if more than 2 buffers are queued and it
>>> continues only after fifo has min 1 slot empty
>>
>>
>> Just want to close on this part of feedback. Hope above explanation is
>> clear regarding triggering/issuing at max 2 frame capture to VI HW and
>> also regarding capture threads where they use wait_event_interruptible
>> to prevent blocking waiting for buffers to be available for captures.
>>
>> So no changes related to this part are needed in v7.
> From what I see in the code, you "hold on" by making kthread to spin in
> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
> should be needed to prevent this.

Looks like some other media drivers do:

	schedule_timeout_uninterruptible(1);

to avoid CPU hogging when contention is detected.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 08.04.2020 00:08, Sowjanya Komatineni пишет:
> ...
>>>> I think you need a semaphore with resource count = 2.
>>> we hold on to issuing capture if more than 2 buffers are queued and it
>>> continues only after fifo has min 1 slot empty
>>
>> Just want to close on this part of feedback. Hope above explanation is
>> clear regarding triggering/issuing at max 2 frame capture to VI HW and
>> also regarding capture threads where they use wait_event_interruptible
>> to prevent blocking waiting for buffers to be available for captures.
>>
>> So no changes related to this part are needed in v7.
>  From what I see in the code, you "hold on" by making kthread to spin in
> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
> should be needed to prevent this.
>
> The wait_event_interruptible seems should be okay.

We don't want to prevent that as we already have buffers available for 
capture so as soon as VI HW issuing single shot is done and when min 1 
slot is empty we should continue with issuing for another capture.

As long as buffers are available, we should continue to capture and 
should not hold

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

08.04.2020 01:22, Sowjanya Komatineni пишет:
> 
> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>> ...
>>>>> I think you need a semaphore with resource count = 2.
>>>> we hold on to issuing capture if more than 2 buffers are queued and it
>>>> continues only after fifo has min 1 slot empty
>>>
>>> Just want to close on this part of feedback. Hope above explanation is
>>> clear regarding triggering/issuing at max 2 frame capture to VI HW and
>>> also regarding capture threads where they use wait_event_interruptible
>>> to prevent blocking waiting for buffers to be available for captures.
>>>
>>> So no changes related to this part are needed in v7.
>>  From what I see in the code, you "hold on" by making kthread to spin in
>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>> should be needed to prevent this.
>>
>> The wait_event_interruptible seems should be okay.
> 
> We don't want to prevent that as we already have buffers available for
> capture so as soon as VI HW issuing single shot is done and when min 1
> slot is empty we should continue with issuing for another capture.
> 
> As long as buffers are available, we should continue to capture and
> should not hold
> 

I suppose that taking a shot takes at least few milliseconds, which
should be unacceptable to waste.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 4:36 PM, Sowjanya Komatineni wrote:
>
> On 4/7/20 4:12 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 08.04.2020 01:22, Sowjanya Komatineni пишет:
>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>> I think you need a semaphore with resource count = 2.
>>>>>> we hold on to issuing capture if more than 2 buffers are queued 
>>>>>> and it
>>>>>> continues only after fifo has min 1 slot empty
>>>>> Just want to close on this part of feedback. Hope above 
>>>>> explanation is
>>>>> clear regarding triggering/issuing at max 2 frame capture to VI HW 
>>>>> and
>>>>> also regarding capture threads where they use 
>>>>> wait_event_interruptible
>>>>> to prevent blocking waiting for buffers to be available for captures.
>>>>>
>>>>> So no changes related to this part are needed in v7.
>>>>   From what I see in the code, you "hold on" by making kthread to 
>>>> spin in
>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>>>> should be needed to prevent this.
>>>>
>>>> The wait_event_interruptible seems should be okay.
>>> We don't want to prevent that as we already have buffers available for
>>> capture so as soon as VI HW issuing single shot is done and when min 1
>>> slot is empty we should continue with issuing for another capture.
>>>
>>> As long as buffers are available, we should continue to capture and
>>> should not hold
>>>
>> I suppose that taking a shot takes at least few milliseconds, which
>> should be unacceptable to waste.
> As long as buffers are in queue we have to keep processing each buffer 
> and between buffers obviously we have to wait for previous frames to 
> finish and this why we have separate thread for frame finish where we 
> can have next buffer capture ready and issue while previous frame 
> memory write happens

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 4:38 PM, Sowjanya Komatineni wrote:
>
> On 4/7/20 4:36 PM, Sowjanya Komatineni wrote:
>>
>> On 4/7/20 4:12 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 08.04.2020 01:22, Sowjanya Komatineni пишет:
>>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> I think you need a semaphore with resource count = 2.
>>>>>>> we hold on to issuing capture if more than 2 buffers are queued 
>>>>>>> and it
>>>>>>> continues only after fifo has min 1 slot empty
>>>>>> Just want to close on this part of feedback. Hope above 
>>>>>> explanation is
>>>>>> clear regarding triggering/issuing at max 2 frame capture to VI 
>>>>>> HW and
>>>>>> also regarding capture threads where they use 
>>>>>> wait_event_interruptible
>>>>>> to prevent blocking waiting for buffers to be available for 
>>>>>> captures.
>>>>>>
>>>>>> So no changes related to this part are needed in v7.
>>>>>   From what I see in the code, you "hold on" by making kthread to 
>>>>> spin in
>>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>>>>> should be needed to prevent this.
>>>>>
>>>>> The wait_event_interruptible seems should be okay.
>>>> We don't want to prevent that as we already have buffers available for
>>>> capture so as soon as VI HW issuing single shot is done and when min 1
>>>> slot is empty we should continue with issuing for another capture.
>>>>
>>>> As long as buffers are available, we should continue to capture and
>>>> should not hold
>>>>
>>> I suppose that taking a shot takes at least few milliseconds, which
>>> should be unacceptable to waste.
>> As long as buffers are in queue we have to keep processing each 
>> buffer and between buffers obviously we have to wait for previous 
>> frames to finish and this why we have separate thread for frame 
>> finish where we can have next buffer capture ready and issue while 
>> previous frame memory write happens
Also we specified numbers buffers as 3 to vb2 queue. So this is rare 
case but to prevent issuing more than 2 at a time as VI HW is only 
double buffered and syncpt fifo max depth is 2 added this to be safer.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 4:38 PM, Sowjanya Komatineni wrote:
>
> On 4/7/20 4:36 PM, Sowjanya Komatineni wrote:
>>
>> On 4/7/20 4:12 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 08.04.2020 01:22, Sowjanya Komatineni пишет:
>>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> I think you need a semaphore with resource count = 2.
>>>>>>> we hold on to issuing capture if more than 2 buffers are queued 
>>>>>>> and it
>>>>>>> continues only after fifo has min 1 slot empty
>>>>>> Just want to close on this part of feedback. Hope above 
>>>>>> explanation is
>>>>>> clear regarding triggering/issuing at max 2 frame capture to VI 
>>>>>> HW and
>>>>>> also regarding capture threads where they use 
>>>>>> wait_event_interruptible
>>>>>> to prevent blocking waiting for buffers to be available for 
>>>>>> captures.
>>>>>>
>>>>>> So no changes related to this part are needed in v7.
>>>>>   From what I see in the code, you "hold on" by making kthread to 
>>>>> spin in
>>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>>>>> should be needed to prevent this.
>>>>>
>>>>> The wait_event_interruptible seems should be okay.
>>>> We don't want to prevent that as we already have buffers available for
>>>> capture so as soon as VI HW issuing single shot is done and when min 1
>>>> slot is empty we should continue with issuing for another capture.
>>>>
>>>> As long as buffers are available, we should continue to capture and
>>>> should not hold
>>>>
>>> I suppose that taking a shot takes at least few milliseconds, which
>>> should be unacceptable to waste.
>> As long as buffers are in queue we have to keep processing each 
>> buffer and between buffers obviously we have to wait for previous 
>> frames to finish and this why we have separate thread for frame 
>> finish where we can have next buffer capture ready and issue while 
>> previous frame memory write happens
Also we specified numbers buffers as 3 to vb2 queue. So this is rare 
case but to prevent issuing more than 2 at a time as VI HW is only 
double buffered and syncpt fifo max depth is 2 added this to be safer.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 4:38 PM, Sowjanya Komatineni wrote:
>
> On 4/7/20 4:36 PM, Sowjanya Komatineni wrote:
>>
>> On 4/7/20 4:12 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 08.04.2020 01:22, Sowjanya Komatineni пишет:
>>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>>>>> ...
>>>>>>>> I think you need a semaphore with resource count = 2.
>>>>>>> we hold on to issuing capture if more than 2 buffers are queued 
>>>>>>> and it
>>>>>>> continues only after fifo has min 1 slot empty
>>>>>> Just want to close on this part of feedback. Hope above 
>>>>>> explanation is
>>>>>> clear regarding triggering/issuing at max 2 frame capture to VI 
>>>>>> HW and
>>>>>> also regarding capture threads where they use 
>>>>>> wait_event_interruptible
>>>>>> to prevent blocking waiting for buffers to be available for 
>>>>>> captures.
>>>>>>
>>>>>> So no changes related to this part are needed in v7.
>>>>>   From what I see in the code, you "hold on" by making kthread to 
>>>>> spin in
>>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>>>>> should be needed to prevent this.
>>>>>
>>>>> The wait_event_interruptible seems should be okay.
>>>> We don't want to prevent that as we already have buffers available for
>>>> capture so as soon as VI HW issuing single shot is done and when min 1
>>>> slot is empty we should continue with issuing for another capture.
>>>>
>>>> As long as buffers are available, we should continue to capture and
>>>> should not hold
>>>>
>>> I suppose that taking a shot takes at least few milliseconds, which
>>> should be unacceptable to waste.
>> As long as buffers are in queue we have to keep processing each 
>> buffer and between buffers obviously we have to wait for previous 
>> frames to finish and this why we have separate thread for frame 
>> finish where we can have next buffer capture ready and issue while 
>> previous frame memory write happens
Also we specified numbers buffers as 3 to vb2 queue. So this is rare 
case but to prevent issuing more than 2 at a time as VI HW is only 
double buffered and syncpt fifo max depth is 2 added this to be safer.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/7/20 4:59 PM, Sowjanya Komatineni wrote:
>
> On 4/7/20 4:38 PM, Sowjanya Komatineni wrote:
>>
>> On 4/7/20 4:36 PM, Sowjanya Komatineni wrote:
>>>
>>> On 4/7/20 4:12 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 08.04.2020 01:22, Sowjanya Komatineni пишет:
>>>>> On 4/7/20 3:08 PM, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 08.04.2020 00:08, Sowjanya Komatineni пишет:
>>>>>> ...
>>>>>>>>> I think you need a semaphore with resource count = 2.
>>>>>>>> we hold on to issuing capture if more than 2 buffers are queued 
>>>>>>>> and it
>>>>>>>> continues only after fifo has min 1 slot empty
>>>>>>> Just want to close on this part of feedback. Hope above 
>>>>>>> explanation is
>>>>>>> clear regarding triggering/issuing at max 2 frame capture to VI 
>>>>>>> HW and
>>>>>>> also regarding capture threads where they use 
>>>>>>> wait_event_interruptible
>>>>>>> to prevent blocking waiting for buffers to be available for 
>>>>>>> captures.
>>>>>>>
>>>>>>> So no changes related to this part are needed in v7.
>>>>>>   From what I see in the code, you "hold on" by making kthread to 
>>>>>> spin in
>>>>>> a busy-loop while caps_inflight >= SYNCPT_FIFO_DEPTH. So some change
>>>>>> should be needed to prevent this.
>>>>>>
>>>>>> The wait_event_interruptible seems should be okay.
>>>>> We don't want to prevent that as we already have buffers available 
>>>>> for
>>>>> capture so as soon as VI HW issuing single shot is done and when 
>>>>> min 1
>>>>> slot is empty we should continue with issuing for another capture.
>>>>>
>>>>> As long as buffers are available, we should continue to capture and
>>>>> should not hold
>>>>>
>>>> I suppose that taking a shot takes at least few milliseconds, which
>>>> should be unacceptable to waste.
>>> As long as buffers are in queue we have to keep processing each 
>>> buffer and between buffers obviously we have to wait for previous 
>>> frames to finish and this why we have separate thread for frame 
>>> finish where we can have next buffer capture ready and issue while 
>>> previous frame memory write happens
> Also we specified numbers buffers as 3 to vb2 queue. So this is rare 
> case but to prevent issuing more than 2 at a time as VI HW is only 
> double buffered and syncpt fifo max depth is 2 added this to be safer.

To be more clear, when more buffers are enqueued from userspace always 
capture list will be full and thread will be busy in capture till either 
error or stop stream request happens.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

08.04.2020 03:00, Sowjanya Komatineni пишет:
...
>>>>> I suppose that taking a shot takes at least few milliseconds, which
>>>>> should be unacceptable to waste.
>>>> As long as buffers are in queue we have to keep processing each
>>>> buffer and between buffers obviously we have to wait for previous
>>>> frames to finish and this why we have separate thread for frame
>>>> finish where we can have next buffer capture ready and issue while
>>>> previous frame memory write happens
>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare
>> case but to prevent issuing more than 2 at a time as VI HW is only
>> double buffered and syncpt fifo max depth is 2 added this to be safer.
> 
> To be more clear, when more buffers are enqueued from userspace always
> capture list will be full and thread will be busy in capture till either
> error or stop stream request happens.
> 

If kthreads take more than 1% of CPU time during capture (video) with
more than 2 buffers in queue, then it's not good and I think you should
do something about it. If kthreads stay at ~0%, then it should be okay
as-is.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/8/20 7:21 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 08.04.2020 03:00, Sowjanya Komatineni пишет:
> ...
>>>>>> I suppose that taking a shot takes at least few milliseconds, which
>>>>>> should be unacceptable to waste.
>>>>> As long as buffers are in queue we have to keep processing each
>>>>> buffer and between buffers obviously we have to wait for previous
>>>>> frames to finish and this why we have separate thread for frame
>>>>> finish where we can have next buffer capture ready and issue while
>>>>> previous frame memory write happens
>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare
>>> case but to prevent issuing more than 2 at a time as VI HW is only
>>> double buffered and syncpt fifo max depth is 2 added this to be safer.
>> To be more clear, when more buffers are enqueued from userspace always
>> capture list will be full and thread will be busy in capture till either
>> error or stop stream request happens.
>>
> If kthreads take more than 1% of CPU time during capture (video) with
> more than 2 buffers in queue, then it's not good and I think you should
> do something about it. If kthreads stay at ~0%, then it should be okay
> as-is.

VI outstanding requests max can only be 2  as syncpt fifo depth is 2  
and waiting to issue next capture when already 2 captures are inflight 
happens only during beginning of streaming where buffers allocated go 
thru capture for first time after queuing.

same buffers are returned to userspace after capture and same allocated 
buffers will be queued back for subsequent captures.

So this case of holding to issue single shot when already single shot is 
issue for 2 frames simultaneous happens only during beginning of start 
stream and also we set num_buffers to allocate for queue as 3 although 2 
is good enough where we will not hit this case even during streaming 
start with 2 buffers

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/8/20 10:45 AM, Sowjanya Komatineni wrote:
>
> On 4/8/20 7:21 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 08.04.2020 03:00, Sowjanya Komatineni пишет:
>> ...
>>>>>>> I suppose that taking a shot takes at least few milliseconds, which
>>>>>>> should be unacceptable to waste.
>>>>>> As long as buffers are in queue we have to keep processing each
>>>>>> buffer and between buffers obviously we have to wait for previous
>>>>>> frames to finish and this why we have separate thread for frame
>>>>>> finish where we can have next buffer capture ready and issue while
>>>>>> previous frame memory write happens
>>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare
>>>> case but to prevent issuing more than 2 at a time as VI HW is only
>>>> double buffered and syncpt fifo max depth is 2 added this to be safer.
>>> To be more clear, when more buffers are enqueued from userspace always
>>> capture list will be full and thread will be busy in capture till 
>>> either
>>> error or stop stream request happens.
>>>
>> If kthreads take more than 1% of CPU time during capture (video) with
>> more than 2 buffers in queue, then it's not good and I think you should
>> do something about it. If kthreads stay at ~0%, then it should be okay
>> as-is.
>
> VI outstanding requests max can only be 2  as syncpt fifo depth is 2  
> and waiting to issue next capture when already 2 captures are inflight 
> happens only during beginning of streaming where buffers allocated go 
> thru capture for first time after queuing.
>
> same buffers are returned to userspace after capture and same 
> allocated buffers will be queued back for subsequent captures.
>
> So this case of holding to issue single shot when already single shot 
> is issue for 2 frames simultaneous happens only during beginning of 
> start stream and also we set num_buffers to allocate for queue as 3 
> although 2 is good enough where we will not hit this case even during 
> streaming start with 2 buffers
>
As 2 buffers are good enough to be clear will update in v7 to use 2 
buffers so we don't need to check for more than 2 outstanding buffers.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/8/20 11:58 AM, Sowjanya Komatineni wrote:
>
> On 4/8/20 10:45 AM, Sowjanya Komatineni wrote:
>>
>> On 4/8/20 7:21 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 08.04.2020 03:00, Sowjanya Komatineni пишет:
>>> ...
>>>>>>>> I suppose that taking a shot takes at least few milliseconds, 
>>>>>>>> which
>>>>>>>> should be unacceptable to waste.
>>>>>>> As long as buffers are in queue we have to keep processing each
>>>>>>> buffer and between buffers obviously we have to wait for previous
>>>>>>> frames to finish and this why we have separate thread for frame
>>>>>>> finish where we can have next buffer capture ready and issue while
>>>>>>> previous frame memory write happens
>>>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare
>>>>> case but to prevent issuing more than 2 at a time as VI HW is only
>>>>> double buffered and syncpt fifo max depth is 2 added this to be 
>>>>> safer.
>>>> To be more clear, when more buffers are enqueued from userspace always
>>>> capture list will be full and thread will be busy in capture till 
>>>> either
>>>> error or stop stream request happens.
>>>>
>>> If kthreads take more than 1% of CPU time during capture (video) with
>>> more than 2 buffers in queue, then it's not good and I think you should
>>> do something about it. If kthreads stay at ~0%, then it should be okay
>>> as-is.
>>
>> VI outstanding requests max can only be 2  as syncpt fifo depth is 2  
>> and waiting to issue next capture when already 2 captures are 
>> inflight happens only during beginning of streaming where buffers 
>> allocated go thru capture for first time after queuing.
>>
>> same buffers are returned to userspace after capture and same 
>> allocated buffers will be queued back for subsequent captures.
>>
>> So this case of holding to issue single shot when already single shot 
>> is issue for 2 frames simultaneous happens only during beginning of 
>> start stream and also we set num_buffers to allocate for queue as 3 
>> although 2 is good enough where we will not hit this case even during 
>> streaming start with 2 buffers
>>
> As 2 buffers are good enough to be clear will update in v7 to use 2 
> buffers so we don't need to check for more than 2 outstanding buffers.

correction: With 3 buffers, as soon as buffer is available capture 
starts. So right most times I see it waiting for few ms before 3rd 
capture to get through.

As only 2 frames single shot can be issued in sequence (inflight 
requests), instead of waiting for 1 of the request to finish, we can use 
2 buffers and avoid waiting as 2 buffers are good enough. Will change 
this in v7.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/8/20 12:38 PM, Sowjanya Komatineni wrote:
>
> On 4/8/20 11:58 AM, Sowjanya Komatineni wrote:
>>
>> On 4/8/20 10:45 AM, Sowjanya Komatineni wrote:
>>>
>>> On 4/8/20 7:21 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 08.04.2020 03:00, Sowjanya Komatineni пишет:
>>>> ...
>>>>>>>>> I suppose that taking a shot takes at least few milliseconds, 
>>>>>>>>> which
>>>>>>>>> should be unacceptable to waste.
>>>>>>>> As long as buffers are in queue we have to keep processing each
>>>>>>>> buffer and between buffers obviously we have to wait for previous
>>>>>>>> frames to finish and this why we have separate thread for frame
>>>>>>>> finish where we can have next buffer capture ready and issue while
>>>>>>>> previous frame memory write happens
>>>>>> Also we specified numbers buffers as 3 to vb2 queue. So this is rare
>>>>>> case but to prevent issuing more than 2 at a time as VI HW is only
>>>>>> double buffered and syncpt fifo max depth is 2 added this to be 
>>>>>> safer.
>>>>> To be more clear, when more buffers are enqueued from userspace 
>>>>> always
>>>>> capture list will be full and thread will be busy in capture till 
>>>>> either
>>>>> error or stop stream request happens.
>>>>>
>>>> If kthreads take more than 1% of CPU time during capture (video) with
>>>> more than 2 buffers in queue, then it's not good and I think you 
>>>> should
>>>> do something about it. If kthreads stay at ~0%, then it should be okay
>>>> as-is.
>>>
>>> VI outstanding requests max can only be 2  as syncpt fifo depth is 
>>> 2  and waiting to issue next capture when already 2 captures are 
>>> inflight happens only during beginning of streaming where buffers 
>>> allocated go thru capture for first time after queuing.
>>>
>>> same buffers are returned to userspace after capture and same 
>>> allocated buffers will be queued back for subsequent captures.
>>>
>>> So this case of holding to issue single shot when already single 
>>> shot is issue for 2 frames simultaneous happens only during 
>>> beginning of start stream and also we set num_buffers to allocate 
>>> for queue as 3 although 2 is good enough where we will not hit this 
>>> case even during streaming start with 2 buffers
>>>
>> As 2 buffers are good enough to be clear will update in v7 to use 2 
>> buffers so we don't need to check for more than 2 outstanding buffers.
>
> correction: With 3 buffers, as soon as buffer is available capture 
> starts. So right most times I see it waiting for few ms before 3rd 
> capture to get through.
>
> As only 2 frames single shot can be issued in sequence (inflight 
> requests), instead of waiting for 1 of the request to finish, we can 
> use 2 buffers and avoid waiting as 2 buffers are good enough. Will 
> change this in v7.
>
>
>
Tested with 3 buffers and by checking outstanding buffers in process by 
VI hw and holding to start capture till one outstanding buffer in 
process by HW.
Also tested with 2 buffers without checking for outstanding buffers.

In both cases, I see same %CPU for the kthreads and is < 1%

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

09.04.2020 06:38, Sowjanya Komatineni пишет:
...
> Tested with 3 buffers and by checking outstanding buffers in process by
> VI hw and holding to start capture till one outstanding buffer in
> process by HW.
> Also tested with 2 buffers without checking for outstanding buffers.
> 
> In both cases, I see same %CPU for the kthreads and is < 1%
> 

I don't see where buffers queue max limit is set to 3 in the code, but
should be okay if CPU isn't getting hogged. Looking forward to v7.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/9/20 7:50 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 09.04.2020 06:38, Sowjanya Komatineni пишет:
> ...
>> Tested with 3 buffers and by checking outstanding buffers in process by
>> VI hw and holding to start capture till one outstanding buffer in
>> process by HW.
>> Also tested with 2 buffers without checking for outstanding buffers.
>>
>> In both cases, I see same %CPU for the kthreads and is < 1%
>>
> I don't see where buffers queue max limit is set to 3 in the code, but
> should be okay if CPU isn't getting hogged. Looking forward to v7.
Sorry, correction I meant to say pre-queued buffers before streaming not 
num_buffers.
vb2 queue min_buffers_needed was set to 3 as part of one of the issue 
debug in earlier version which actually was irrelevant to that issue and 
should have been removed. Will remove min_buffers_needed in v7.

I added checking for outstanding requests by hardware just to be safer 
although we may not hit this case of issuing more than 1 outstanding 
frame capture to VI hardware as capture_frame() waits till it sees frame 
start event through HW syncpt increment before proceeding for memory 
write and issuing next frame capture.

So issuing frame captures are synchronized with frame start and frame end.

Will remove min_buffers_needed and also explicit check for outstanding 
buffers in v7.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

09.04.2020 21:28, Sowjanya Komatineni пишет:
> 
> On 4/9/20 7:50 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.04.2020 06:38, Sowjanya Komatineni пишет:
>> ...
>>> Tested with 3 buffers and by checking outstanding buffers in process by
>>> VI hw and holding to start capture till one outstanding buffer in
>>> process by HW.
>>> Also tested with 2 buffers without checking for outstanding buffers.
>>>
>>> In both cases, I see same %CPU for the kthreads and is < 1%
>>>
>> I don't see where buffers queue max limit is set to 3 in the code, but
>> should be okay if CPU isn't getting hogged. Looking forward to v7.
> Sorry, correction I meant to say pre-queued buffers before streaming not
> num_buffers.
> vb2 queue min_buffers_needed was set to 3 as part of one of the issue
> debug in earlier version which actually was irrelevant to that issue and
> should have been removed. Will remove min_buffers_needed in v7.
> 
> I added checking for outstanding requests by hardware just to be safer
> although we may not hit this case of issuing more than 1 outstanding
> frame capture to VI hardware as capture_frame() waits till it sees frame
> start event through HW syncpt increment before proceeding for memory
> write and issuing next frame capture.
> 
> So issuing frame captures are synchronized with frame start and frame end.
> 
> Will remove min_buffers_needed and also explicit check for outstanding
> buffers in v7.

It's still not clear to me how the "pre-queued buffers" will be limited.
I'll take another look at the v7.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/10/20 11:47 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 09.04.2020 21:28, Sowjanya Komatineni пишет:
>> On 4/9/20 7:50 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 09.04.2020 06:38, Sowjanya Komatineni пишет:
>>> ...
>>>> Tested with 3 buffers and by checking outstanding buffers in process by
>>>> VI hw and holding to start capture till one outstanding buffer in
>>>> process by HW.
>>>> Also tested with 2 buffers without checking for outstanding buffers.
>>>>
>>>> In both cases, I see same %CPU for the kthreads and is < 1%
>>>>
>>> I don't see where buffers queue max limit is set to 3 in the code, but
>>> should be okay if CPU isn't getting hogged. Looking forward to v7.
>> Sorry, correction I meant to say pre-queued buffers before streaming not
>> num_buffers.
>> vb2 queue min_buffers_needed was set to 3 as part of one of the issue
>> debug in earlier version which actually was irrelevant to that issue and
>> should have been removed. Will remove min_buffers_needed in v7.
>>
>> I added checking for outstanding requests by hardware just to be safer
>> although we may not hit this case of issuing more than 1 outstanding
>> frame capture to VI hardware as capture_frame() waits till it sees frame
>> start event through HW syncpt increment before proceeding for memory
>> write and issuing next frame capture.
>>
>> So issuing frame captures are synchronized with frame start and frame end.
>>
>> Will remove min_buffers_needed and also explicit check for outstanding
>> buffers in v7.
> It's still not clear to me how the "pre-queued buffers" will be limited.
> I'll take another look at the v7.

OK, but I don't understand what you mean by limit on pre-queued buffers.

I was saying vb2 queue has min_buffers_needed which was set to 3 where 
streaming will start only after 3 buffers got queued up.

Regarding outstanding condition check to make sure no more than 2 syncpt 
trigger requests are in FIFO I added it to be safe where mostly we may 
not hit and also I only see capture start thread holding for it during 
initial frame capture as it issues single shot for 1st 2 buffers capture 
and holds 3 buffers which is already queued till at least one of those 2 
issued capture is done to make sure of not triggering syncpt condition 
when fifo already has 2 pending.

In v7, will remove setting min_buffers_needed and also outstanding 
syncpt trigger check.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

10.04.2020 21:59, Sowjanya Komatineni пишет:
...
>> It's still not clear to me how the "pre-queued buffers" will be limited.
>> I'll take another look at the v7.
> 
> OK, but I don't understand what you mean by limit on pre-queued buffers.
> 
> I was saying vb2 queue has min_buffers_needed which was set to 3 where
> streaming will start only after 3 buffers got queued up.
> 
> Regarding outstanding condition check to make sure no more than 2 syncpt
> trigger requests are in FIFO I added it to be safe where mostly we may
> not hit and also I only see capture start thread holding for it during
> initial frame capture as it issues single shot for 1st 2 buffers capture
> and holds 3 buffers which is already queued till at least one of those 2
> issued capture is done to make sure of not triggering syncpt condition
> when fifo already has 2 pending.
> 
> In v7, will remove setting min_buffers_needed and also outstanding
> syncpt trigger check.

Okay, seems I got what you're saying. Yes, the check should be removed.
It's impossible to get the frame-start event while capture of the
previous buffer is in-progress.

Re: [RFC PATCH v6 6/9] media: tegra: Add Tegra210 Video input driver

[Dmitry Osipenko]

04.04.2020 04:25, Sowjanya Komatineni пишет:
> +	/* wait for syncpt counter to reach frame start event threshold */
> +	err = host1x_syncpt_wait(chan->frame_start_sp, thresh,
> +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> +	if (err) {
> +		dev_err(&chan->video.dev,
> +			"frame start syncpt timeout: %d\n", err);

I guess this and the other timeout should be dev_err_ratelimited().

Re: [RFC PATCH v6 3/9] dt-bindings: clock: tegra: Add clk id for CSI TPG clock

[Rob Herring]

On Fri, 3 Apr 2020 18:25:01 -0700, Sowjanya Komatineni wrote:
> Tegra210 uses PLLD out internally for CSI TPG.
> 
> This patch adds clk id for this CSI TPG clock from PLLD.
> 
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  include/dt-bindings/clock/tegra210-car.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC PATCH v6 5/9] dt-binding: tegra: Add VI and CSI bindings

[Rob Herring]

On Fri, 3 Apr 2020 18:25:03 -0700, Sowjanya Komatineni wrote:
> Tegra contains VI controller which can support up to 6 MIPI CSI
> camera sensors.
> 
> Each Tegra CSI port from CSI unit can be one-to-one mapper to
> VI channel and can capture from an external camera sensor or
> from built-in test pattern generator.
> 
> This patch adds dt-bindings for Tegra VI and CSI.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  .../display/tegra/nvidia,tegra20-host1x.txt        | 73 ++++++++++++++++++----
>  1 file changed, 60 insertions(+), 13 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [RFC PATCH v6 8/9] dt-bindings: reset: Add ID for Tegra210 VI reset

[Rob Herring]

On Fri, 3 Apr 2020 18:25:06 -0700, Sowjanya Komatineni wrote:
> This patch adds ID for Tegra210 VI controller reset to use with
> device tree.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  include/dt-bindings/reset/tegra210-car.h | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>