On Thu, Jan 04, 2018 at 03:06:25PM +0100, Maxime Ripard wrote:

On Mon, Dec 25, 2017 at 09:58:02AM +0100, Ond?ej Jirman wrote:

quoted

Hello,

On Mon, Dec 25, 2017 at 11:15:26AM +0800, Yong wrote:

quoted

Hi,

On Fri, 22 Dec 2017 14:46:48 +0100
Ond?ej Jirman [off-list ref] wrote:

quoted

Hello,

Yong Deng p??e v P? 22. 12. 2017 v 17:32 +0800:

quoted

Test input 0:

        Control ioctls:
                test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
                test VIDIOC_QUERYCTRL: OK (Not Supported)
                test VIDIOC_G/S_CTRL: OK (Not Supported)
                test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
                test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
                test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
                Standard Controls: 0 Private Controls: 0

I'm not sure if your driver passes control queries to the subdev. It
did not originally, and I'm not sure you picked up the change from my
version of the driver. "Not supported" here seems to indicate that it
does not.

I'd be interested what's the recommended practice here. It sure helps
with some apps that expect to be able to modify various input controls
directly on the /dev/video# device. These are then supported out of the
box.

It's a one-line change. See:

https://www.kernel.org/doc/html/latest/media/kapi/v4l2-controls.html#in
heriting-controls

I think this is a feature and not affect the driver's main function.
I just focused on making the CSI main function to work properly in 
the initial version. Is this feature mandatory or most commonly used?

I grepped the platform/ code and it seems, that inheriting controls
from subdevs is pretty common for input drivers. (there are varying
approaches though, some inherit by hand in the link function, some
just register and empty ctrl_handler on the v4l2_dev and leave the
rest to the core).

Practically, I haven't found a common app that would allow me to enter
both /dev/video0 and /dev/v4l-subdevX. I'm sure anyone can write one
themselves, but it would be better if current controls were available
at the /dev/video0 device automatically.

It's much simpler for the userspace apps than the alternative, which
is trying to identify the correct subdev that is currently
associated with the CSI driver at runtime, which is not exactly
straightforward and requires much more code, than a few lines in
the kernel, that are required to inherit controls:

And it becomes much more complicated once you have the same controls
on the v4l2 device and subdevice, which is not that uncommon.

Hi Maxime,

I don't think you understand the issue. In your hypothetical situation, if the
CSI device will have any controls in the future, the merging of controls from
subdev will be done automatically anyway, it's not some optional feature.

Also userspace will not get any more complicated than without my proposed change
to the driver. It will be at most the same as without the change if any subdev
controls are masked by the CSI device controls.

This CSI driver has no controls anyway. All my change does is create an empty
handler for future controls of the CSI driver, so that apps can depend on this
merging behavior right now, and not wait until someone adds the first control
to the CSI driver.

regards,
  o.j.

Maxime



-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com