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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 3 Feb 2023 01:35:12 +0000
From:   <yuji2.ishikawa@...hiba.co.jp>
To:     <laurent.pinchart@...asonboard.com>, <hverkuil@...all.nl>
CC:     <mchehab@...nel.org>, <nobuhiro1.iwamatsu@...hiba.co.jp>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <rafael.j.wysocki@...el.com>, <broonie@...nel.org>,
        <linux-media@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: 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@...asonboard.com>
> Sent: Thursday, January 26, 2023 8:36 PM
> To: Hans Verkuil <hverkuil@...all.nl>
> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>; mchehab@...nel.org; iwamatsu nobuhiro(岩松
> 信洋 □ＳＷＣ◯ＡＣＴ) <nobuhiro1.iwamatsu@...hiba.co.jp>;
> robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> rafael.j.wysocki@...el.com; broonie@...nel.org; linux-media@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> devicetree@...r.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:
> > On 26/01/2023 01:38, yuji2.ishikawa@...hiba.co.jp 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