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

Hello,

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Wednesday, January 18, 2023 10:04 AM
To: Hans Verkuil <redacted>
Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]; Mauro Carvalho Chehab
[off-list ref]; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
[off-list ref]; Rob Herring [off-list ref];
Krzysztof Kozlowski [off-list ref]; Rafael J . Wysocki
[off-list ref]; Mark Brown [off-list ref];
linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti
Video Input Interface driver user interace

Hello,

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

quoted

More comments below:

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.

quoted

quoted

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

+++++++++++++++++

quoted

quoted

 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.

quoted

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
* viif_enum_frameintervals

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

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.

[snip]

--
Regards,

Laurent Pinchart

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