> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Thursday, January 26, 2023 8:36 PM
> To: Hans Verkuil <hverkuil@xxxxxxxxx>
> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; mchehab@xxxxxxxxxx; iwamatsu nobuhiro(岩松
> 信洋 □ＳＷＣ◯ＡＣＴ) <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> rafael.j.wysocki@xxxxxxxxx; broonie@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> 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:
> > On 26/01/2023 01:38, yuji2.ishikawa@xxxxxxxxxxxxx wrote:
> > >>> +#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)
> > >>> +{
> > >>> + 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,
> > >>> +
> u64_to_user_ptr(l1_dpc->table_h_addr),
> > >>> +
> VISCONTI_VIIF_DPC_TABLE_SIZE))
> > >>> + 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.
> > >
> > >> 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?
> > > -- 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
> > >= 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
