RE: [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver v4l2 controls handler

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Thursday, January 26, 2023 8:36 PM
To: Hans Verkuil <redacted>
Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]; mchehab@kernel.org; iwamatsu nobuhiro(岩松
信洋 □ＳＷＣ◯ＡＣＴ) [off-list ref];
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
rafael.j.wysocki@intel.com; broonie@kernel.org; linux-media@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org
Subject: Re: [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti
Video Input Interface driver v4l2 controls handler

On Thu, Jan 26, 2023 at 09:39:59AM +0100, Hans Verkuil wrote:

quoted

On 26/01/2023 01:38, yuji2.ishikawa@toshiba.co.jp wrote:

quoted

quoted

quoted

+#define VISCONTI_VIIF_DPC_TABLE_SIZE 8192 static int
+viif_l1_set_dpc(struct viif_device *viif_dev, struct
+viif_l1_dpc_config

*l1_dpc)

quoted

+{
+	uintptr_t table_h_paddr = 0;
+	uintptr_t table_m_paddr = 0;
+	uintptr_t table_l_paddr = 0;
+	unsigned long irqflags;
+	int ret;
+
+	if (l1_dpc->table_h_addr) {
+		if

(copy_from_user(viif_dev->table_vaddr->dpc_table_h,

quoted

quoted

quoted

quoted

+

u64_to_user_ptr(l1_dpc->table_h_addr),

quoted

quoted

quoted

quoted

+

VISCONTI_VIIF_DPC_TABLE_SIZE))

quoted

quoted

quoted

quoted

+			return -EFAULT;

NACK!

I thought those addresses in a struct were iffy. This is not
supported, it basically bypasses the whole control framework.

I understand.

quoted

The way to do this is to create separate array controls for these tables.
And table_h_addr becomes a simple 0 or 1 value, indicating whether
to use the table set by that control. For small arrays it is also
an option to embed them in the control structure.

As I wrote in reply for patch 2/6, I thought embedding is the only solution.
Thank you for giving another plan: adding controls for tables.
When I use individual controls for tables, are there some orderings between

controls?

quoted

quoted

 -- such that control DPC_TABLE_{H,M,L} should be configured before
SET_DPC

There is no ordering dependency. But you can cluster controls:

https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-control
s.html#control-clusters

The idea is that userspace sets all the related controls with one
VIDIOC_S_EXT_CTRLS ioctl, and then for the clustered controls the
s_ctrl callback is called only once.

You can also check in try_ctrl if the controls in a cluster are sane. E.g.
if control A has value 1, and that requires that control B has a value

quoted

= 5, then try_ctrl can verify that. Normally controls are independent

from one another, but clustering will link them together.

It's really what you want here. A good example is here:
drivers/media/common/cx2341x.c It's used by several PCI drivers that
use this MPEG codec chipset, and it uses clusters and also implements

try_ctrl.

I think controls are the wrong tool for this job though. The ISP requires a large
number of parameters, which would I think be better suited passed as a
parameters buffer like the ipu3 and rkisp1 driver do for most of the data. Some
parameters may still make sense as controls (possibly mostly for the CSI2RX
parameters), but I haven't checked that in details.

I'm thinking about passing some parameters (especially large tables) with streaming interface (like rkisp1-param).
However this change should be done carefully because 1) HW limitation can be involved and 2) design of userland can change greatly.
Some of my concerns are:
* Some parameters (e.g. L1ISP_INPUT_MODE) should be set before streaming start.
  Are Parameters via streaming interface available before streaming start?
  I suppose vb2_ops::buf_queue() would be called for ioctl(QBUF),
  but I'm not for sure I can use the content stored in vb2_buffer.
* Does streaming interface accept multiple patterns of data layout?
  I suppose it accepts only one data layout which includes all the possible parameters and corresponding enable/disable flags. Perhaps, idea like union can be applied to exclusive parameters?
* Can I call QBUF of parameter buffer multiple times for a frame?
  I suppose it depends on driver's implementation but most of the drivers assume one QBUF for a frame.

I need more knowledge how userland use drivers. Architecture specific codes in libcamera might help me?

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