Re: [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver user interace

Hello Ishikawa-san,

On Thu, Feb 02, 2023 at 04:52:56AM +0000, yuji2.ishikawa@toshiba.co.jp wrote:

On Friday, January 27, 2023 5:49 AM, Laurent Pinchart wrote:

quoted

On Wed, Jan 25, 2023 at 10:20:27AM +0000, yuji2.ishikawa@toshiba.co.jp wrote:

quoted

On Wednesday, January 18, 2023 10:04 AM, Laurent Pinchart wrote:

quoted

On Tue, Jan 17, 2023 at 12:47:10PM +0100, Hans Verkuil wrote:

quoted

On 11/01/2023 03:24, Yuji Ishikawa wrote:

quoted

Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
The interface device includes CSI2 Receiver, frame grabber,
video DMAC and image signal processor.
This patch provides the user interface layer.

A driver instance provides three /dev/videoX device files; one
for RGB image capture, another one for optional RGB capture with
different parameters and the last one for RAW capture.

Through the device files, the driver provides streaming (DMA-BUF) interface.
A userland application should feed DMA-BUF instances for capture buffers.

The driver is based on media controller framework.
Its operations are roughly mapped to two subdrivers; one for ISP
and
CSI2 receiver (yields 1 instance), the other for capture (yields
3 instances for each capture mode).

Signed-off-by: Yuji Ishikawa <redacted>
---
Changelog v2:
- Resend v1 because a patch exceeds size limit.

Changelog v3:
- Adapted to media control framework
- Introduced ISP subdevice, capture device
- Remove private IOCTLs and add vendor specific V4L2 controls
- Change function name avoiding camelcase and uppercase letters

Changelog v4:
- Split patches because the v3 patch exceeds size limit
- Stop using ID number to identify driver instance:
  - Use dynamically allocated structure to hold HW specific context,
    instead of static one.
  - Call HW layer functions with the context structure instead of ID number
- Use pm_runtime to trigger initialization of HW
  along with open/close of device files.

Changelog v5:
- Fix coding style problems in viif.c
---
 drivers/media/platform/visconti/Makefile      |    1 +
 drivers/media/platform/visconti/viif.c        |  545 ++++++++
 drivers/media/platform/visconti/viif.h        |  203 +++
 .../media/platform/visconti/viif_capture.c    | 1201 +++++++++++++++++
 drivers/media/platform/visconti/viif_isp.c    |  846 ++++++++++++
 5 files changed, 2796 insertions(+)  create mode 100644
drivers/media/platform/visconti/viif.c
 create mode 100644 drivers/media/platform/visconti/viif.h
 create mode 100644
drivers/media/platform/visconti/viif_capture.c
 create mode 100644 drivers/media/platform/visconti/viif_isp.c

[snip]

quoted

quoted

+static int viif_s_edid(struct file *file, void *fh, struct
+v4l2_edid *edid) {
+	struct viif_device *viif_dev = video_drvdata_to_capdev(file)->viif_dev;
+	struct viif_subdev *viif_sd = viif_dev->sd;
+
+	return v4l2_subdev_call(viif_sd->v4l2_sd, pad, set_edid,
+edid); }

Has this driver been tested with an HDMI receiver? If not, then I
would recommend dropping support for it until you actually can
test with such hardware.

The DV_TIMINGS API is for HDMI/DVI/DisplayPort etc. interfaces,
it's not meant for CSI and similar interfaces.

More than that, for MC-based drivers, the video node should *never*
forward ioctls to a connected subdev. The *only* valid calls to
v4l2_subdev_call() in this file are

- to video.s_stream() in the start and stop streaming handler

- to pad.g_fmt() when starting streaming to validate that the connected
  subdev outputs a format compatible with the format set on the video
  capture device

That's it, nothing else, all other calls to v4l2_subdev_call() must
be dropped from the implementation of the video_device.

Thank you for your comment. I understand the restriction.
I'll remove following functions corresponding to ioctls.

* viif_enum_input
* viif_g_selection
* viif_s_selection
* viif_dv_timings_cap
* viif_enum_dv_timings
* viif_g_dv_timings
* viif_s_dv_timings
* viif_query_dv_timings
* viif_g_edid
* viif_s_edid
* viif_g_parm
* viif_s_parm
* viif_enum_framesizes

This one should stay, it should report the minimum and maximum sizes
supported by the video nodes, regardless of the configuration of the connected
subdev.

I'll keep it.

quoted

quoted

* viif_enum_frameintervals

I can call subdevices directly if I need. Is it a correct understanding?

what do you mean exactly by calling subdevices directly ?

I meant userland can configure subdevices with /dev/v4l-subdev.

Ah yes absolutely, userspace will configure the subdevs.

quoted

quoted

As for viif_try_fmt_vid_cap and viif_s_fmt_vid_cap, I'll remove
pad.g_fmt() call which is for checking pixel format.
The check will be moved to viif_capture_link_validate() validation
routine triggered by a start streaming event.

quoted

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel