mbox series

[RFC,v10,0/9] Add Tegra driver for video capture

Message ID 1587700513-28449-1-git-send-email-skomatineni@nvidia.com (mailing list archive)
Headers show
Series Add Tegra driver for video capture | expand

Message

Sowjanya Komatineni April 24, 2020, 3:55 a.m. UTC
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.

[v10]:	Includes,
	- updated patches for latest linux-next base
	- Kconfig update
	- minor cleanup/improvements

[v9]:	Includes,
	- small fix to explicitly check for both vi and csi channels
	  availability before TPG setup and cleanup so in the cases
	  of later Tegras where CSI is not child to VI and if either
	  of the platform drivers are not registered, TPG setup will be
	  skipped.

[v8]:	Includes,
	- minor change to use device managed allocation fo vi and csi for now.
	  May need to change back to non device managed allocation later when
	  support for direct host1x client driver unbind and bind is added.

[v7]:	Includes,
	- v6 feedback
	- moved registering v4l2 nodes and creating tpg media links to happen
	  after both host1x client inits so during direct host1x client driver
	  unbind and bind order of client unregister/register will not impact.
	- All channels resources and freeing happens during v4l2 device release
	  callback.
	- module unload/load works with below host1x bus driver fix.
	  http://patchwork.ozlabs.org/patch/1268191/

[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                |   12 +
 drivers/staging/media/tegra/Makefile               |    8 +
 drivers/staging/media/tegra/TODO                   |   10 +
 drivers/staging/media/tegra/common.h               |  259 +++++
 drivers/staging/media/tegra/csi.c                  |  604 +++++++++++
 drivers/staging/media/tegra/csi.h                  |  144 +++
 drivers/staging/media/tegra/tegra210.c             |  708 ++++++++++++
 drivers/staging/media/tegra/tegra210.h             |  190 ++++
 drivers/staging/media/tegra/vi.c                   | 1127 ++++++++++++++++++++
 drivers/staging/media/tegra/vi.h                   |   72 ++
 drivers/staging/media/tegra/video.c                |  153 +++
 drivers/staging/media/tegra/video.h                |   29 +
 include/dt-bindings/clock/tegra210-car.h           |    2 +-
 include/dt-bindings/reset/tegra210-car.h           |    1 +
 21 files changed, 3457 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/common.h
 create mode 100644 drivers/staging/media/tegra/csi.c
 create mode 100644 drivers/staging/media/tegra/csi.h
 create mode 100644 drivers/staging/media/tegra/tegra210.c
 create mode 100644 drivers/staging/media/tegra/tegra210.h
 create mode 100644 drivers/staging/media/tegra/vi.c
 create mode 100644 drivers/staging/media/tegra/vi.h
 create mode 100644 drivers/staging/media/tegra/video.c
 create mode 100644 drivers/staging/media/tegra/video.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

Comments

Dmitry Osipenko April 24, 2020, 3:07 p.m. UTC | #1
24.04.2020 06:55, Sowjanya Komatineni пишет:

Is this driver compiled as a single kernel module file?

> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
> +MODULE_LICENSE("GPL v2");
...
> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
> +MODULE_LICENSE("GPL v2");

I don't think that these macros are needed in that case.
The video.c should be enough, isn't it?
Dmitry Osipenko April 24, 2020, 3:11 p.m. UTC | #2
24.04.2020 06:55, Sowjanya Komatineni пишет:
> Tegra210 contains a powerful Video Input (VI) hardware controller
> which can support up to 6 MIPI CSI camera sensors.
> 
> Each Tegra CSI port can be one-to-one mapped to VI channel and can
> capture from an external camera sensor connected to CSI or from
> built-in test pattern generator.
> 
> Tegra210 supports built-in test pattern generator from CSI to VI.
> 
> This patch adds a v4l2 capture driver with media interface for
> Tegra210 built-in CSI to VI test pattern generator.
> 
> This patch includes TPG support only and all the video pipeline
> configuration happens through the video device node.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/staging/media/Kconfig          |    2 +
>  drivers/staging/media/Makefile         |    1 +
>  drivers/staging/media/tegra/Kconfig    |   12 +
>  drivers/staging/media/tegra/Makefile   |    8 +
>  drivers/staging/media/tegra/TODO       |   10 +
>  drivers/staging/media/tegra/common.h   |  259 ++++++++
>  drivers/staging/media/tegra/csi.c      |  604 +++++++++++++++++
>  drivers/staging/media/tegra/csi.h      |  144 ++++
>  drivers/staging/media/tegra/tegra210.c |  708 ++++++++++++++++++++
>  drivers/staging/media/tegra/tegra210.h |  190 ++++++
>  drivers/staging/media/tegra/vi.c       | 1127 ++++++++++++++++++++++++++++++++
>  drivers/staging/media/tegra/vi.h       |   72 ++
>  drivers/staging/media/tegra/video.c    |  153 +++++
>  drivers/staging/media/tegra/video.h    |   29 +

The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
should better reflect the driver, IMO.

It also should be better to name the compiled kernel module as tegra-vi,
IMO.
Sowjanya Komatineni April 24, 2020, 10 p.m. UTC | #3
On 4/24/20 8:07 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>
> Is this driver compiled as a single kernel module file?
>
>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
>> +MODULE_LICENSE("GPL v2");
> ...
>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
>> +MODULE_LICENSE("GPL v2");
> I don't think that these macros are needed in that case.
> The video.c should be enough, isn't it?
yes these can be removed
Hans Verkuil April 25, 2020, 9:36 a.m. UTC | #4
On 24/04/2020 17:11, Dmitry Osipenko wrote:
> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>> Tegra210 contains a powerful Video Input (VI) hardware controller
>> which can support up to 6 MIPI CSI camera sensors.
>>
>> Each Tegra CSI port can be one-to-one mapped to VI channel and can
>> capture from an external camera sensor connected to CSI or from
>> built-in test pattern generator.
>>
>> Tegra210 supports built-in test pattern generator from CSI to VI.
>>
>> This patch adds a v4l2 capture driver with media interface for
>> Tegra210 built-in CSI to VI test pattern generator.
>>
>> This patch includes TPG support only and all the video pipeline
>> configuration happens through the video device node.
>>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  drivers/staging/media/Kconfig          |    2 +
>>  drivers/staging/media/Makefile         |    1 +
>>  drivers/staging/media/tegra/Kconfig    |   12 +
>>  drivers/staging/media/tegra/Makefile   |    8 +
>>  drivers/staging/media/tegra/TODO       |   10 +
>>  drivers/staging/media/tegra/common.h   |  259 ++++++++
>>  drivers/staging/media/tegra/csi.c      |  604 +++++++++++++++++
>>  drivers/staging/media/tegra/csi.h      |  144 ++++
>>  drivers/staging/media/tegra/tegra210.c |  708 ++++++++++++++++++++
>>  drivers/staging/media/tegra/tegra210.h |  190 ++++++
>>  drivers/staging/media/tegra/vi.c       | 1127 ++++++++++++++++++++++++++++++++
>>  drivers/staging/media/tegra/vi.h       |   72 ++
>>  drivers/staging/media/tegra/video.c    |  153 +++++
>>  drivers/staging/media/tegra/video.h    |   29 +
> 
> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
> should better reflect the driver, IMO.
> 
> It also should be better to name the compiled kernel module as tegra-vi,
> IMO.
> 

The driver name and the directory should be in sync, so either tegra-video
or tegra-vi for both. I have no preference myself, as long as they are the
same.

This can be done as a follow-up patch.

Regards,

	Hans
Dmitry Osipenko April 25, 2020, 10:08 p.m. UTC | #5
25.04.2020 01:00, Sowjanya Komatineni пишет:
> 
> On 4/24/20 8:07 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>
>> Is this driver compiled as a single kernel module file?
>>
>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
>>> +MODULE_LICENSE("GPL v2");
>> ...
>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
>>> +MODULE_LICENSE("GPL v2");
>> I don't think that these macros are needed in that case.
>> The video.c should be enough, isn't it?
> yes these can be removed

It will be nice to factor out the Tegra210-specific VI/CSI OPS into a
separate driver module (say tegra210-vi) to ease supporting of other
Tegra versions. Of course this could be done later on, although I
suppose the amount of hassle could be reduced if it's done from the start.
Sowjanya Komatineni April 25, 2020, 10:11 p.m. UTC | #6
On 4/25/20 3:08 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 25.04.2020 01:00, Sowjanya Komatineni пишет:
>> On 4/24/20 8:07 AM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>>
>>> Is this driver compiled as a single kernel module file?
>>>
>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>> ...
>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>> I don't think that these macros are needed in that case.
>>> The video.c should be enough, isn't it?
>> yes these can be removed
> It will be nice to factor out the Tegra210-specific VI/CSI OPS into a
> separate driver module (say tegra210-vi) to ease supporting of other
> Tegra versions. Of course this could be done later on, although I
> suppose the amount of hassle could be reduced if it's done from the start.
vi/csi.c are common drivers for all Tegras. All Tegra chip specific 
related programming for both vi and csi were already moved to Tegra210.c 
based on prior feedbacks.
Dmitry Osipenko April 25, 2020, 11:13 p.m. UTC | #7
24.04.2020 06:55, Sowjanya Komatineni пишет:
> +static int __maybe_unused vi_runtime_resume(struct device *dev)
> +{
> +	struct tegra_vi *vi = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_enable(vi->vdd);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDD supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
> +	if (ret) {
> +		dev_err(dev, "failed to set vi clock rate: %d\n", ret);
> +		goto disable_vdd;
> +	}

Isn't setting clock rate using assigned-clocks in a device-tree enough?
Could you please clarify why this vi_max_clk_hz is needed?
Sowjanya Komatineni April 25, 2020, 11:19 p.m. UTC | #8
On 4/25/20 4:13 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>> +static int __maybe_unused vi_runtime_resume(struct device *dev)
>> +{
>> +     struct tegra_vi *vi = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     ret = regulator_enable(vi->vdd);
>> +     if (ret) {
>> +             dev_err(dev, "failed to enable VDD supply: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
>> +     if (ret) {
>> +             dev_err(dev, "failed to set vi clock rate: %d\n", ret);
>> +             goto disable_vdd;
>> +     }
> Isn't setting clock rate using assigned-clocks in a device-tree enough?
> Could you please clarify why this vi_max_clk_hz is needed?

Max clock rate with sensor support will be 998Mhz.

Later when sensor support is added, based on TPG or Sensor mode clock 
rate will be set here
Dmitry Osipenko April 25, 2020, 11:25 p.m. UTC | #9
26.04.2020 02:13, Dmitry Osipenko пишет:
> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>> +static int __maybe_unused vi_runtime_resume(struct device *dev)
>> +{
>> +	struct tegra_vi *vi = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = regulator_enable(vi->vdd);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable VDD supply: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
>> +	if (ret) {
>> +		dev_err(dev, "failed to set vi clock rate: %d\n", ret);
>> +		goto disable_vdd;
>> +	}
> 
> Isn't setting clock rate using assigned-clocks in a device-tree enough?
> Could you please clarify why this vi_max_clk_hz is needed?
> 

In that case it should be wrong to set the clock rate in the RPM
callback because RPM works asynchronously and RPM may not be suspended
on TGP -> sensor source switch.
Sowjanya Komatineni April 25, 2020, 11:27 p.m. UTC | #10
On 4/25/20 4:25 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 02:13, Dmitry Osipenko пишет:
>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>> +static int __maybe_unused vi_runtime_resume(struct device *dev)
>>> +{
>>> +    struct tegra_vi *vi = dev_get_drvdata(dev);
>>> +    int ret;
>>> +
>>> +    ret = regulator_enable(vi->vdd);
>>> +    if (ret) {
>>> +            dev_err(dev, "failed to enable VDD supply: %d\n", ret);
>>> +            return ret;
>>> +    }
>>> +
>>> +    ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
>>> +    if (ret) {
>>> +            dev_err(dev, "failed to set vi clock rate: %d\n", ret);
>>> +            goto disable_vdd;
>>> +    }
>> Isn't setting clock rate using assigned-clocks in a device-tree enough?
>> Could you please clarify why this vi_max_clk_hz is needed?
>>
> In that case it should be wrong to set the clock rate in the RPM
> callback because RPM works asynchronously and RPM may not be suspended
> on TGP -> sensor source switch.

Driver will not do TPG and Sensor switch dynamically.

Based on kconfig, it will only do TPG or Sensor and sensor will be 
default all the time once sensor support is added in next series.
Dmitry Osipenko April 25, 2020, 11:29 p.m. UTC | #11
24.04.2020 06:55, 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;
> +
> +	INIT_LIST_HEAD(&csi->csi_chans);
> +
> +	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;
> +	}

The whole point of RPM is to keep hardware enabled only when needed,
i.e. during of the capture process in this case. You should move all RPM
handling to the capture start / stop functions.
Dmitry Osipenko April 25, 2020, 11:40 p.m. UTC | #12
26.04.2020 01:11, Sowjanya Komatineni пишет:
> 
> On 4/25/20 3:08 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 25.04.2020 01:00, Sowjanya Komatineni пишет:
>>> On 4/24/20 8:07 AM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>>>
>>>> Is this driver compiled as a single kernel module file?
>>>>
>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>> ...
>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>> I don't think that these macros are needed in that case.
>>>> The video.c should be enough, isn't it?
>>> yes these can be removed
>> It will be nice to factor out the Tegra210-specific VI/CSI OPS into a
>> separate driver module (say tegra210-vi) to ease supporting of other
>> Tegra versions. Of course this could be done later on, although I
>> suppose the amount of hassle could be reduced if it's done from the
>> start.
> vi/csi.c are common drivers for all Tegras. All Tegra chip specific
> related programming for both vi and csi were already moved to Tegra210.c
> based on prior feedbacks.

Judging by the code's structure the VI/CSI drivers aren't planned to be
reused by older pre-Terga210 SoCs, correct?

How much of the T210 code could be reused by T186/194?
Sowjanya Komatineni April 25, 2020, 11:44 p.m. UTC | #13
On 4/25/20 4:40 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 01:11, Sowjanya Komatineni пишет:
>> On 4/25/20 3:08 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 25.04.2020 01:00, Sowjanya Komatineni пишет:
>>>> On 4/24/20 8:07 AM, Dmitry Osipenko wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>>>>
>>>>> Is this driver compiled as a single kernel module file?
>>>>>
>>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra CSI Device Driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>> ...
>>>>>> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
>>>>>> +MODULE_DESCRIPTION("NVIDIA Tegra Video Input Device Driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>> I don't think that these macros are needed in that case.
>>>>> The video.c should be enough, isn't it?
>>>> yes these can be removed
>>> It will be nice to factor out the Tegra210-specific VI/CSI OPS into a
>>> separate driver module (say tegra210-vi) to ease supporting of other
>>> Tegra versions. Of course this could be done later on, although I
>>> suppose the amount of hassle could be reduced if it's done from the
>>> start.
>> vi/csi.c are common drivers for all Tegras. All Tegra chip specific
>> related programming for both vi and csi were already moved to Tegra210.c
>> based on prior feedbacks.
> Judging by the code's structure the VI/CSI drivers aren't planned to be
> reused by older pre-Terga210 SoCs, correct?
>
> How much of the T210 code could be reused by T186/194?

vi/csi are common driver where soc structure should be populated for 
T186/T194

Tegra210.c can't be reused for Tegra186/t194 as programming seq is a 
whole lot diff
Dmitry Osipenko April 25, 2020, 11:44 p.m. UTC | #14
26.04.2020 02:27, Sowjanya Komatineni пишет:
> 
> On 4/25/20 4:25 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 26.04.2020 02:13, Dmitry Osipenko пишет:
>>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>>> +static int __maybe_unused vi_runtime_resume(struct device *dev)
>>>> +{
>>>> +    struct tegra_vi *vi = dev_get_drvdata(dev);
>>>> +    int ret;
>>>> +
>>>> +    ret = regulator_enable(vi->vdd);
>>>> +    if (ret) {
>>>> +            dev_err(dev, "failed to enable VDD supply: %d\n", ret);
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
>>>> +    if (ret) {
>>>> +            dev_err(dev, "failed to set vi clock rate: %d\n", ret);
>>>> +            goto disable_vdd;
>>>> +    }
>>> Isn't setting clock rate using assigned-clocks in a device-tree enough?
>>> Could you please clarify why this vi_max_clk_hz is needed?
>>>
>> In that case it should be wrong to set the clock rate in the RPM
>> callback because RPM works asynchronously and RPM may not be suspended
>> on TGP -> sensor source switch.
> 
> Driver will not do TPG and Sensor switch dynamically.
> 
> Based on kconfig, it will only do TPG or Sensor and sensor will be
> default all the time once sensor support is added in next series.
> 

Doesn't V4L have a native support for the capture source selection? Why
it needs to be a compile-time option?

I think other drivers use a generic V4L "Image Processing Controls" with
a configurable test_pattern option.
Sowjanya Komatineni April 25, 2020, 11:47 p.m. UTC | #15
On 4/25/20 4:44 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 02:27, Sowjanya Komatineni пишет:
>> On 4/25/20 4:25 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 26.04.2020 02:13, Dmitry Osipenko пишет:
>>>> 24.04.2020 06:55, Sowjanya Komatineni пишет:
>>>>> +static int __maybe_unused vi_runtime_resume(struct device *dev)
>>>>> +{
>>>>> +    struct tegra_vi *vi = dev_get_drvdata(dev);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regulator_enable(vi->vdd);
>>>>> +    if (ret) {
>>>>> +            dev_err(dev, "failed to enable VDD supply: %d\n", ret);
>>>>> +            return ret;
>>>>> +    }
>>>>> +
>>>>> +    ret = clk_set_rate(vi->clk, vi->soc->vi_max_clk_hz);
>>>>> +    if (ret) {
>>>>> +            dev_err(dev, "failed to set vi clock rate: %d\n", ret);
>>>>> +            goto disable_vdd;
>>>>> +    }
>>>> Isn't setting clock rate using assigned-clocks in a device-tree enough?
>>>> Could you please clarify why this vi_max_clk_hz is needed?
>>>>
>>> In that case it should be wrong to set the clock rate in the RPM
>>> callback because RPM works asynchronously and RPM may not be suspended
>>> on TGP -> sensor source switch.
>> Driver will not do TPG and Sensor switch dynamically.
>>
>> Based on kconfig, it will only do TPG or Sensor and sensor will be
>> default all the time once sensor support is added in next series.
>>
> Doesn't V4L have a native support for the capture source selection? Why
> it needs to be a compile-time option?
>
> I think other drivers use a generic V4L "Image Processing Controls" with
> a configurable test_pattern option.

selecting test pattern thru v4l2 controls is once device graph is 
already set for TPG to choose test pattern modes.

But as internal TPG is b/w CSI and VI only, device graph is diff and for 
sensor device graph is different.


Based on internal discussion and discussion with Hans, decided to use 
kconfig for TPG Vs Sensor and TPG is rarely used only to test driver 
without sensor
Dmitry Osipenko April 26, 2020, 12:19 a.m. UTC | #16
26.04.2020 02:44, Sowjanya Komatineni пишет:
...
>> How much of the T210 code could be reused by T186/194?
> 
> vi/csi are common driver where soc structure should be populated for
> T186/T194
> 
> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a
> whole lot diff
> 

How are you going to separate Tegra210/186/194 drivers from each other?
I don't think you'll want to have one "fat" driver that covers all those
SoCs, won't you?

In the end it should be three modules: tegra210-video.ko
tegra186-video.ko tegra194-video.ko.

Using a per-SoC OPS doesn't allow you to do that because the "root"
driver will have to lookup OPS' code symbols of every SoC, and thus, the
unwanted driver modules will get auto-loaded if you'll try to factor out
the OPS into a separate driver modules.
Sowjanya Komatineni April 26, 2020, 12:24 a.m. UTC | #17
On 4/25/20 5:19 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 02:44, Sowjanya Komatineni пишет:
> ...
>>> How much of the T210 code could be reused by T186/194?
>> vi/csi are common driver where soc structure should be populated for
>> T186/T194
>>
>> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a
>> whole lot diff
>>
> How are you going to separate Tegra210/186/194 drivers from each other?
> I don't think you'll want to have one "fat" driver that covers all those
> SoCs, won't you?
>
> In the end it should be three modules: tegra210-video.ko
> tegra186-video.ko tegra194-video.ko.
>
> Using a per-SoC OPS doesn't allow you to do that because the "root"
> driver will have to lookup OPS' code symbols of every SoC, and thus, the
> unwanted driver modules will get auto-loaded if you'll try to factor out
> the OPS into a separate driver modules.

tegra-video driver will be common for t210/186/194

we add corresponding compatibles to tegra-video and vi/csi drivers along 
with that need to add tegra186_vi_soc, tegra194_vi_soc and implement 
tegra186.c/tegra194.c

tegra-video driver updated for later tegra include updating drivers list 
and subdevs list to add t186/t194 compatibles
Dmitry Osipenko April 26, 2020, 12:36 a.m. UTC | #18
25.04.2020 12:36, Hans Verkuil пишет:
...
>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
>> should better reflect the driver, IMO.
>>
>> It also should be better to name the compiled kernel module as tegra-vi,
>> IMO.
>>
> 
> The driver name and the directory should be in sync, so either tegra-video
> or tegra-vi for both. I have no preference myself, as long as they are the
> same.
> 
> This can be done as a follow-up patch.

Given that this driver isn't going to be reused by older pre-Tegra210
SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or
tegra210-video.
Dmitry Osipenko April 26, 2020, 12:38 a.m. UTC | #19
26.04.2020 03:24, Sowjanya Komatineni пишет:
> 
> On 4/25/20 5:19 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 26.04.2020 02:44, Sowjanya Komatineni пишет:
>> ...
>>>> How much of the T210 code could be reused by T186/194?
>>> vi/csi are common driver where soc structure should be populated for
>>> T186/T194
>>>
>>> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a
>>> whole lot diff
>>>
>> How are you going to separate Tegra210/186/194 drivers from each other?
>> I don't think you'll want to have one "fat" driver that covers all those
>> SoCs, won't you?
>>
>> In the end it should be three modules: tegra210-video.ko
>> tegra186-video.ko tegra194-video.ko.
>>
>> Using a per-SoC OPS doesn't allow you to do that because the "root"
>> driver will have to lookup OPS' code symbols of every SoC, and thus, the
>> unwanted driver modules will get auto-loaded if you'll try to factor out
>> the OPS into a separate driver modules.
> 
> tegra-video driver will be common for t210/186/194

Oh, well.

> we add corresponding compatibles to tegra-video and vi/csi drivers along
> with that need to add tegra186_vi_soc, tegra194_vi_soc and implement
> tegra186.c/tegra194.c
> 
> tegra-video driver updated for later tegra include updating drivers list
> and subdevs list to add t186/t194 compatibles
Sowjanya Komatineni April 26, 2020, 12:41 a.m. UTC | #20
On 4/25/20 5:36 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 25.04.2020 12:36, Hans Verkuil пишет:
> ...
>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
>>> should better reflect the driver, IMO.
>>>
>>> It also should be better to name the compiled kernel module as tegra-vi,
>>> IMO.
>>>
>> The driver name and the directory should be in sync, so either tegra-video
>> or tegra-vi for both. I have no preference myself, as long as they are the
>> same.
>>
>> This can be done as a follow-up patch.
> Given that this driver isn't going to be reused by older pre-Tegra210
> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or
> tegra210-video.

Can you explain what do you meant by can't be reused for pre-tegra210 or 
for tegra186/194?

support for other tegra's can be added to same tegra-video driver. 
tegra-video host1x driver can be updated to add other tegra's vi and csi 
compatibles to host1x subdevs and vi and csi driver can be updated to 
add other tegra soc data and need to add coresponding tegra186/194/xxx.c 
file with tegra specific prog sequence
Sowjanya Komatineni April 26, 2020, 1:04 a.m. UTC | #21
On 4/25/20 4:29 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 24.04.2020 06:55, 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;
>> +
>> +     INIT_LIST_HEAD(&csi->csi_chans);
>> +
>> +     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;
>> +     }
> The whole point of RPM is to keep hardware enabled only when needed,
> i.e. during of the capture process in this case. You should move all RPM
> handling to the capture start / stop functions.
Will move it to handle during stream start/stop
Sowjanya Komatineni April 26, 2020, 1:08 a.m. UTC | #22
On 4/25/20 5:41 PM, Sowjanya Komatineni wrote:
>
> On 4/25/20 5:36 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 25.04.2020 12:36, Hans Verkuil пишет:
>> ...
>>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
>>>> should better reflect the driver, IMO.
>>>>
>>>> It also should be better to name the compiled kernel module as 
>>>> tegra-vi,
>>>> IMO.
>>>>
>>> The driver name and the directory should be in sync, so either 
>>> tegra-video
>>> or tegra-vi for both. I have no preference myself, as long as they 
>>> are the
>>> same.
>>>
>>> This can be done as a follow-up patch.
>> Given that this driver isn't going to be reused by older pre-Tegra210
>> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or
>> tegra210-video.
>
> Can you explain what do you meant by can't be reused for pre-tegra210 
> or for tegra186/194?
>
> support for other tegra's can be added to same tegra-video driver. 
> tegra-video host1x driver can be updated to add other tegra's vi and 
> csi compatibles to host1x subdevs and vi and csi driver can be updated 
> to add other tegra soc data and need to add coresponding 
> tegra186/194/xxx.c file with tegra specific prog sequence
>
Same tegra-video.ko can be used for all tegra soc as driver supports 
adding other soc related as well.

Also was thinking instead of renaming media/tegra as media/tegra-vi, 
probably we can rename as media/tegra-video so it will be inline with 
module name we already chosen and also mainly we have vi and csi with in 
that so instead of tegra-vi probably we can use media/tegra-video?
Dmitry Osipenko April 26, 2020, 1:26 a.m. UTC | #23
26.04.2020 04:08, Sowjanya Komatineni пишет:
> 
> On 4/25/20 5:41 PM, Sowjanya Komatineni wrote:
>>
>> On 4/25/20 5:36 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 25.04.2020 12:36, Hans Verkuil пишет:
>>> ...
>>>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
>>>>> should better reflect the driver, IMO.
>>>>>
>>>>> It also should be better to name the compiled kernel module as
>>>>> tegra-vi,
>>>>> IMO.
>>>>>
>>>> The driver name and the directory should be in sync, so either
>>>> tegra-video
>>>> or tegra-vi for both. I have no preference myself, as long as they
>>>> are the
>>>> same.
>>>>
>>>> This can be done as a follow-up patch.
>>> Given that this driver isn't going to be reused by older pre-Tegra210
>>> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or
>>> tegra210-video.
>>
>> Can you explain what do you meant by can't be reused for pre-tegra210
>> or for tegra186/194?

It looks to me that at least all those hardcoded HW format IDs do not
match the older SoCs.

>> support for other tegra's can be added to same tegra-video driver.
>> tegra-video host1x driver can be updated to add other tegra's vi and
>> csi compatibles to host1x subdevs and vi and csi driver can be updated
>> to add other tegra soc data and need to add coresponding
>> tegra186/194/xxx.c file with tegra specific prog sequence
>>
> Same tegra-video.ko can be used for all tegra soc as driver supports
> adding other soc related as well.

Well, I'm still not sure why you would want to have all the unnecessary
code of a different SoCs shared within the same kernel module, it will
be quite be a lot wasted space in comparison to a used part of the driver.

The driver will need to have a bit better separation if it's supposed to
have a common core for all SoCs. Each incompatible VI/CSI hardware
version should have its own kernel module.

> Also was thinking instead of renaming media/tegra as media/tegra-vi,
> probably we can rename as media/tegra-video so it will be inline with
> module name we already chosen and also mainly we have vi and csi with in
> that so instead of tegra-vi probably we can use media/tegra-video?

The tegra-video should be okay, although the "video" part sounds a bit
too broad since video could mean a lot of things. I think downstream
kernel uses (or at least used) the tegra-camera name for the driver,
perhaps it could be a reasonable variant as well.
Sowjanya Komatineni April 26, 2020, 1:43 a.m. UTC | #24
On 4/25/20 6:26 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 04:08, Sowjanya Komatineni пишет:
>> On 4/25/20 5:41 PM, Sowjanya Komatineni wrote:
>>> On 4/25/20 5:36 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 25.04.2020 12:36, Hans Verkuil пишет:
>>>> ...
>>>>>> The media/tegra/ sounds a bit too generic, the media/tegra-vi/ path
>>>>>> should better reflect the driver, IMO.
>>>>>>
>>>>>> It also should be better to name the compiled kernel module as
>>>>>> tegra-vi,
>>>>>> IMO.
>>>>>>
>>>>> The driver name and the directory should be in sync, so either
>>>>> tegra-video
>>>>> or tegra-vi for both. I have no preference myself, as long as they
>>>>> are the
>>>>> same.
>>>>>
>>>>> This can be done as a follow-up patch.
>>>> Given that this driver isn't going to be reused by older pre-Tegra210
>>>> SoCs, perhaps it will also be worthwhile to name it as tegra210-vi or
>>>> tegra210-video.
>>> Can you explain what do you meant by can't be reused for pre-tegra210
>>> or for tegra186/194?
> It looks to me that at least all those hardcoded HW format IDs do not
> match the older SoCs.

TPG hard coded formats are supported on prior Tegra.

Other supported formats are SoC dependent and  part of soc data in the 
driver already.

>>> support for other tegra's can be added to same tegra-video driver.
>>> tegra-video host1x driver can be updated to add other tegra's vi and
>>> csi compatibles to host1x subdevs and vi and csi driver can be updated
>>> to add other tegra soc data and need to add coresponding
>>> tegra186/194/xxx.c file with tegra specific prog sequence
>>>
>> Same tegra-video.ko can be used for all tegra soc as driver supports
>> adding other soc related as well.
> Well, I'm still not sure why you would want to have all the unnecessary
> code of a different SoCs shared within the same kernel module, it will
> be quite be a lot wasted space in comparison to a used part of the driver.
>
> The driver will need to have a bit better separation if it's supposed to
> have a common core for all SoCs. Each incompatible VI/CSI hardware
> version should have its own kernel module.

currently other Tegra host1x driver (drm) also does similar. Single 
module for all Tegra SoCs.

With current tegra-video, all the v4l2 related common part of 
implementation is same for all tegra's and only 
tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc 
specific vi/csi programming sequence.

>> Also was thinking instead of renaming media/tegra as media/tegra-vi,
>> probably we can rename as media/tegra-video so it will be inline with
>> module name we already chosen and also mainly we have vi and csi with in
>> that so instead of tegra-vi probably we can use media/tegra-video?
> The tegra-video should be okay, although the "video" part sounds a bit
> too broad since video could mean a lot of things. I think downstream
> kernel uses (or at least used) the tegra-camera name for the driver,
> perhaps it could be a reasonable variant as well.
prior feedback suggests not to use camera variant instead to use video
Dmitry Osipenko April 26, 2020, 2:10 a.m. UTC | #25
26.04.2020 04:43, Sowjanya Komatineni пишет:
...
>> It looks to me that at least all those hardcoded HW format IDs do not
>> match the older SoCs.
> 
> TPG hard coded formats are supported on prior Tegra.
> 
> Other supported formats are SoC dependent and  part of soc data in the
> driver already.

But I don't see where that SoC-dependent definition is made in
terga210.c. That tegra_image_format enum looks T210-specific, isn't it?

...
>> The driver will need to have a bit better separation if it's supposed to
>> have a common core for all SoCs. Each incompatible VI/CSI hardware
>> version should have its own kernel module.
> 
> currently other Tegra host1x driver (drm) also does similar. Single
> module for all Tegra SoCs.

DRM driver has a proper separation of the sub-drivers where sub-driver
won't load on unsupported hardware. The tegra-video driver should do the
same, i.e. VI and CSI should be individual drivers (and not OPS). There
could be a some common core, but for now it's not obvious to me what
that core should be, maybe just the video.c.

> With current tegra-video, all the v4l2 related common part of
> implementation is same for all tegra's and only
> tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc
> specific vi/csi programming sequence.

This code shouldn't be shared within the same driver module, IMO.


>> The tegra-video should be okay, although the "video" part sounds a bit
>> too broad since video could mean a lot of things. I think downstream
>> kernel uses (or at least used) the tegra-camera name for the driver,
>> perhaps it could be a reasonable variant as well.
> prior feedback suggests not to use camera variant instead to use video

Alright, then the tegra-video should be fine.
Sowjanya Komatineni April 26, 2020, 2:19 a.m. UTC | #26
On 4/25/20 7:10 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 04:43, Sowjanya Komatineni пишет:
> ...
>>> It looks to me that at least all those hardcoded HW format IDs do not
>>> match the older SoCs.
>> TPG hard coded formats are supported on prior Tegra.
>>
>> Other supported formats are SoC dependent and  part of soc data in the
>> driver already.
> But I don't see where that SoC-dependent definition is made in
> terga210.c. That tegra_image_format enum looks T210-specific, isn't it?
>
> ...

Video formats which are SoC variants are made soc specific in driver 
already tegra_vi_soc structure member video_formats

tegra_image_format enum is same for T210 and T186

For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT 
using corresponding Tegra194 video format enums

>>> The driver will need to have a bit better separation if it's supposed to
>>> have a common core for all SoCs. Each incompatible VI/CSI hardware
>>> version should have its own kernel module.
>> currently other Tegra host1x driver (drm) also does similar. Single
>> module for all Tegra SoCs.
> DRM driver has a proper separation of the sub-drivers where sub-driver
> won't load on unsupported hardware. The tegra-video driver should do the
> same, i.e. VI and CSI should be individual drivers (and not OPS). There
> could be a some common core, but for now it's not obvious to me what
> that core should be, maybe just the video.c.
>
>> With current tegra-video, all the v4l2 related common part of
>> implementation is same for all tegra's and only
>> tegra210.c/tegra186.c/tegra194.c will have corresponding tegra soc
>> specific vi/csi programming sequence.
> This code shouldn't be shared within the same driver module, IMO.
>
>
>>> The tegra-video should be okay, although the "video" part sounds a bit
>>> too broad since video could mean a lot of things. I think downstream
>>> kernel uses (or at least used) the tegra-camera name for the driver,
>>> perhaps it could be a reasonable variant as well.
>> prior feedback suggests not to use camera variant instead to use video
> Alright, then the tegra-video should be fine.
Dmitry Osipenko April 26, 2020, 2:19 a.m. UTC | #27
26.04.2020 05:10, Dmitry Osipenko пишет:
...
>> currently other Tegra host1x driver (drm) also does similar. Single
>> module for all Tegra SoCs.
> 
> DRM driver has a proper separation of the sub-drivers where sub-driver
> won't load on unsupported hardware. The tegra-video driver should do the
> same, i.e. VI and CSI should be individual drivers (and not OPS). There
> could be a some common core, but for now it's not obvious to me what
> that core should be, maybe just the video.c.

Maybe video.c csi.c vi.c could be moved into a separate module, somewhat
like a common driver framework. Then the individual CSI/VI drivers will
use those common helpers.. Just a quick thought.
Dmitry Osipenko April 26, 2020, 2:38 a.m. UTC | #28
26.04.2020 05:19, Sowjanya Komatineni пишет:
> 
> On 4/25/20 7:10 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 26.04.2020 04:43, Sowjanya Komatineni пишет:
>> ...
>>>> It looks to me that at least all those hardcoded HW format IDs do not
>>>> match the older SoCs.
>>> TPG hard coded formats are supported on prior Tegra.
>>>
>>> Other supported formats are SoC dependent and  part of soc data in the
>>> driver already.
>> But I don't see where that SoC-dependent definition is made in
>> terga210.c. That tegra_image_format enum looks T210-specific, isn't it?
>>
>> ...
> 
> Video formats which are SoC variants are made soc specific in driver
> already tegra_vi_soc structure member video_formats
> 
> tegra_image_format enum is same for T210 and T186
> 
> For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT
> using corresponding Tegra194 video format enums
But it is also not the same for older SoCs, correct? All the
T210-specific things should be separated better, unique parts shouldn't
be kept in the common code.

Hence the tegra_image_format should be renamed to tegra210_image_format
and moved out to t210.h, since it's not common. But then you'll probably
need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable
by T186.

Also, in the end it may not worth the effort to share anything at all,
it could be cleaner to have a bit of duplication. Although, I have no
idea how T186 code will look like and what other parts of T210 could be
reused by T186. All this needs to be taken into account in order to
avoid struggling with the code's reshuffling in the future.
Sowjanya Komatineni April 26, 2020, 2:48 a.m. UTC | #29
On 4/25/20 7:38 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 05:19, Sowjanya Komatineni пишет:
>> On 4/25/20 7:10 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 26.04.2020 04:43, Sowjanya Komatineni пишет:
>>> ...
>>>>> It looks to me that at least all those hardcoded HW format IDs do not
>>>>> match the older SoCs.
>>>> TPG hard coded formats are supported on prior Tegra.
>>>>
>>>> Other supported formats are SoC dependent and  part of soc data in the
>>>> driver already.
>>> But I don't see where that SoC-dependent definition is made in
>>> terga210.c. That tegra_image_format enum looks T210-specific, isn't it?
>>>
>>> ...
>> Video formats which are SoC variants are made soc specific in driver
>> already tegra_vi_soc structure member video_formats
>>
>> tegra_image_format enum is same for T210 and T186
>>
>> For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT
>> using corresponding Tegra194 video format enums
> But it is also not the same for older SoCs, correct? All the
> T210-specific things should be separated better, unique parts shouldn't
> be kept in the common code.
>
> Hence the tegra_image_format should be renamed to tegra210_image_format
> and moved out to t210.h, since it's not common. But then you'll probably
> need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable
> by T186.

We can't make t210.h reusable for t186 as all register defines are diff.

only video format enums are same b/w them so to reuse that for t186 I 
had that in common.

Regarding defines, will change prefix as Tegra210

>
> Also, in the end it may not worth the effort to share anything at all,
> it could be cleaner to have a bit of duplication. Although, I have no
> idea how T186 code will look like and what other parts of T210 could be
> reused by T186. All this needs to be taken into account in order to
> avoid struggling with the code's reshuffling in the future.

Currently as image formats are same for t210 and t186 I had them in 
common.h and for t194 where they are diff new enums will be added.

Other tegra210 soc specific only are all part of tegra210.c/h
Sowjanya Komatineni April 26, 2020, 3:03 a.m. UTC | #30
On 4/25/20 7:48 PM, Sowjanya Komatineni wrote:
>
> On 4/25/20 7:38 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 26.04.2020 05:19, Sowjanya Komatineni пишет:
>>> On 4/25/20 7:10 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 26.04.2020 04:43, Sowjanya Komatineni пишет:
>>>> ...
>>>>>> It looks to me that at least all those hardcoded HW format IDs do 
>>>>>> not
>>>>>> match the older SoCs.
>>>>> TPG hard coded formats are supported on prior Tegra.
>>>>>
>>>>> Other supported formats are SoC dependent and  part of soc data in 
>>>>> the
>>>>> driver already.
>>>> But I don't see where that SoC-dependent definition is made in
>>>> terga210.c. That tegra_image_format enum looks T210-specific, isn't 
>>>> it?
>>>>
>>>> ...
>>> Video formats which are SoC variants are made soc specific in driver
>>> already tegra_vi_soc structure member video_formats
>>>
>>> tegra_image_format enum is same for T210 and T186
>>>
>>> For T194, enums will be diff and will have diff TEGRA194_VIDEO_FORMAT
>>> using corresponding Tegra194 video format enums
>> But it is also not the same for older SoCs, correct? All the
>> T210-specific things should be separated better, unique parts shouldn't
>> be kept in the common code.
>>
>> Hence the tegra_image_format should be renamed to tegra210_image_format
>> and moved out to t210.h, since it's not common. But then you'll probably
>> need to rename all TEGRA_ defines to TEGRA210_ to make t210.h reusable
>> by T186.
>
> We can't make t210.h reusable for t186 as all register defines are diff.
>
> only video format enums are same b/w them so to reuse that for t186 I 
> had that in common.
>
> Regarding defines, will change prefix as Tegra210
>
>>
>> Also, in the end it may not worth the effort to share anything at all,
>> it could be cleaner to have a bit of duplication. Although, I have no
>> idea how T186 code will look like and what other parts of T210 could be
>> reused by T186. All this needs to be taken into account in order to
>> avoid struggling with the code's reshuffling in the future.
>
> Currently as image formats are same for t210 and t186 I had them in 
> common.h and for t194 where they are diff new enums will be added.
>
> Other tegra210 soc specific only are all part of tegra210.c/h
>
Will move video formats to tegra specific file (tegra210.c)...
Sowjanya Komatineni April 26, 2020, 4:23 a.m. UTC | #31
On 4/25/20 7:19 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 05:10, Dmitry Osipenko пишет:
> ...
>>> currently other Tegra host1x driver (drm) also does similar. Single
>>> module for all Tegra SoCs.
>> DRM driver has a proper separation of the sub-drivers where sub-driver
>> won't load on unsupported hardware. The tegra-video driver should do the
>> same, i.e. VI and CSI should be individual drivers (and not OPS). There
>> could be a some common core, but for now it's not obvious to me what
>> that core should be, maybe just the video.c.
> Maybe video.c csi.c vi.c could be moved into a separate module, somewhat
> like a common driver framework. Then the individual CSI/VI drivers will
> use those common helpers.. Just a quick thought.

structure of driver is based on prior feedback.

How about instead of re-structuring whole driver again, probably we can 
use conditional compatibles and also corresponding tegra210.o in 
Makefile based on ARCH_TEGRA?

#if defined(CONFIG_ARCH_TEGRA_210_SOC)
     { .compatible = "nvidia,tegra210-vi", .data = &tegra210_vi_soc },
#endif
Dmitry Osipenko April 26, 2020, 4:42 a.m. UTC | #32
26.04.2020 05:10, Dmitry Osipenko пишет:
> 26.04.2020 04:43, Sowjanya Komatineni пишет:
> ...
>>> It looks to me that at least all those hardcoded HW format IDs do not
>>> match the older SoCs.
>>
>> TPG hard coded formats are supported on prior Tegra.
>>
>> Other supported formats are SoC dependent and  part of soc data in the
>> driver already.
> 
> But I don't see where that SoC-dependent definition is made in
> terga210.c. That tegra_image_format enum looks T210-specific, isn't it?
> 
> ...
>>> The driver will need to have a bit better separation if it's supposed to
>>> have a common core for all SoCs. Each incompatible VI/CSI hardware
>>> version should have its own kernel module.
>>
>> currently other Tegra host1x driver (drm) also does similar. Single
>> module for all Tegra SoCs.
> 
> DRM driver has a proper separation of the sub-drivers where sub-driver
> won't load on unsupported hardware. The tegra-video driver should do the
> same, i.e. VI and CSI should be individual drivers (and not OPS). There
> could be a some common core, but for now it's not obvious to me what
> that core should be, maybe just the video.c.

Although, you're right that tegra_drm is compiled as a single module.
That's not good, I'm actually not sure now whether it is possible to
modularize host1x drivers properly without changing the whole host1x bus.
Sowjanya Komatineni April 26, 2020, 4:47 a.m. UTC | #33
On 4/25/20 9:42 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 05:10, Dmitry Osipenko пишет:
>> 26.04.2020 04:43, Sowjanya Komatineni пишет:
>> ...
>>>> It looks to me that at least all those hardcoded HW format IDs do not
>>>> match the older SoCs.
>>> TPG hard coded formats are supported on prior Tegra.
>>>
>>> Other supported formats are SoC dependent and  part of soc data in the
>>> driver already.
>> But I don't see where that SoC-dependent definition is made in
>> terga210.c. That tegra_image_format enum looks T210-specific, isn't it?
>>
>> ...
>>>> The driver will need to have a bit better separation if it's supposed to
>>>> have a common core for all SoCs. Each incompatible VI/CSI hardware
>>>> version should have its own kernel module.
>>> currently other Tegra host1x driver (drm) also does similar. Single
>>> module for all Tegra SoCs.
>> DRM driver has a proper separation of the sub-drivers where sub-driver
>> won't load on unsupported hardware. The tegra-video driver should do the
>> same, i.e. VI and CSI should be individual drivers (and not OPS). There
>> could be a some common core, but for now it's not obvious to me what
>> that core should be, maybe just the video.c.
> Although, you're right that tegra_drm is compiled as a single module.
> That's not good, I'm actually not sure now whether it is possible to
> modularize host1x drivers properly without changing the whole host1x bus.

We already went thru driver structure changes during earlier versions 
and internal feedbacks.

Not sure of restructuring again now. Also reg wastage of space, tegra 
specific implementation is not huge also for T186 and later we have 
RTCPU firmware that driver communicates with and it light weight. Based 
on internal discussion, no upstream support for prior Tegra210.

So, probably what we have regarding structure is ok except video formats 
I will move to Tegra210 and change defines to use Tegra210 prefix.
Dmitry Osipenko April 26, 2020, 5:04 a.m. UTC | #34
26.04.2020 07:47, Sowjanya Komatineni пишет:
...
>> Although, you're right that tegra_drm is compiled as a single module.
>> That's not good, I'm actually not sure now whether it is possible to
>> modularize host1x drivers properly without changing the whole host1x bus.
> 
> We already went thru driver structure changes during earlier versions
> and internal feedbacks.
> 
> Not sure of restructuring again now. Also reg wastage of space, tegra
> specific implementation is not huge also for T186 and later we have
> RTCPU firmware that driver communicates with and it light weight. Based
> on internal discussion, no upstream support for prior Tegra210.

NVIDIA doesn't support older SoCs in downstream, but this doesn't apply
to the upstream where anyone could write a driver.

> So, probably what we have regarding structure is ok except video formats
> I will move to Tegra210 and change defines to use Tegra210 prefix.

Seems so.
Dmitry Osipenko April 26, 2020, 5:48 a.m. UTC | #35
26.04.2020 07:23, Sowjanya Komatineni пишет:
> 
> On 4/25/20 7:19 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 26.04.2020 05:10, Dmitry Osipenko пишет:
>> ...
>>>> currently other Tegra host1x driver (drm) also does similar. Single
>>>> module for all Tegra SoCs.
>>> DRM driver has a proper separation of the sub-drivers where sub-driver
>>> won't load on unsupported hardware. The tegra-video driver should do the
>>> same, i.e. VI and CSI should be individual drivers (and not OPS). There
>>> could be a some common core, but for now it's not obvious to me what
>>> that core should be, maybe just the video.c.
>> Maybe video.c csi.c vi.c could be moved into a separate module, somewhat
>> like a common driver framework. Then the individual CSI/VI drivers will
>> use those common helpers.. Just a quick thought.
> 
> structure of driver is based on prior feedback.
> 
> How about instead of re-structuring whole driver again, probably we can
> use conditional compatibles and also corresponding tegra210.o in
> Makefile based on ARCH_TEGRA?
> 
> #if defined(CONFIG_ARCH_TEGRA_210_SOC)
>     { .compatible = "nvidia,tegra210-vi", .data = &tegra210_vi_soc },
> #endif

Yes, allowing user to disable the unneeded code should be good.
Dmitry Osipenko April 26, 2020, 5:51 a.m. UTC | #36
26.04.2020 07:47, Sowjanya Komatineni пишет:
> 
> So, probably what we have regarding structure is ok except video formats
> I will move to Tegra210 and change defines to use Tegra210 prefix.

If those defines aren't reusable by T186, then you could move them all
to t210.c.
Hans Verkuil April 26, 2020, 8:07 a.m. UTC | #37
On 26/04/2020 02:19, Dmitry Osipenko wrote:
> 26.04.2020 02:44, Sowjanya Komatineni пишет:
> ...
>>> How much of the T210 code could be reused by T186/194?
>>
>> vi/csi are common driver where soc structure should be populated for
>> T186/T194
>>
>> Tegra210.c can't be reused for Tegra186/t194 as programming seq is a
>> whole lot diff
>>
> 
> How are you going to separate Tegra210/186/194 drivers from each other?
> I don't think you'll want to have one "fat" driver that covers all those
> SoCs, won't you?

As long as the differences between SoCs are small, the media subsystem policy
is to keep it all in one driver. You might split off some of it into separate
SoC-specific sources that are included only if selected in the Kconfig (see
e.g. drivers/staging/media/hantro/ or drivers/staging/media/imx/). If that
makes sense for the Tegra, then that's a perfectly fine option. But creating
multiple drivers for SoCs that only differ in relatively minor ways is not
recommended.

Also, these drivers allocate *huge* amounts of memory when streaming video,
so a somewhat bigger driver is not something you'll notice. Keeping things
readable, simple and maintainable is much more important.

Regards,

	Hans

> 
> In the end it should be three modules: tegra210-video.ko
> tegra186-video.ko tegra194-video.ko.
> 
> Using a per-SoC OPS doesn't allow you to do that because the "root"
> driver will have to lookup OPS' code symbols of every SoC, and thus, the
> unwanted driver modules will get auto-loaded if you'll try to factor out
> the OPS into a separate driver modules.
>
Sowjanya Komatineni April 26, 2020, 8:27 a.m. UTC | #38
On 4/25/20 10:51 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 26.04.2020 07:47, Sowjanya Komatineni пишет:
>> So, probably what we have regarding structure is ok except video formats
>> I will move to Tegra210 and change defines to use Tegra210 prefix.
> If those defines aren't reusable by T186, then you could move them all
> to t210.c.

ok, will combine defines and soc specific programming into tegra210.c

Also will then get rid of common.h with video formats moving to tegra210