| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| jeremymc | Hi, I have added support for the adv7619 . Based on the docs I managed to add "enough" to the adv7604 driver for initial support | [04:12] |
| lrusak | what device is that chip used in? | [04:12] |
| jeremymc | data seems to be flowing. But the problem I am having is I keep seeing this, "i2c i2c-9: Invalid 7-bit I2C address 0x00 " and the addition of a "dummy" i2c driver
lrusak, its in a PCIe capture card I am writing the driver for and plan o _TRYING_ to get it mainlined ;-) Here is some debug spew to help with context: https://hastebin.com/licovizabe.sql | [04:14] |
| ..... (idle for 21mn) | ||
| larsc | jeremymc: well 0x00 is not a valid 7-bit address ;)
I think the driver reads back the address from the register map if no address is otherwise provided maybe that particular sub I2C device does not exist in this device and has no default in the register check which sub-register map fails in adv76xx_dummy_client() | [04:39] |
| jeremymc | ah. Thats an idea. | [04:42] |
| larsc | or maybe the register just always reads 0
I think to remember that they changed this | [04:42] |
| jeremymc | Where are the register maps | [04:43] |
| larsc | the device has multiple I2C addresses
each address for one of the register maps and the addresses can be configured through the main map | [04:43] |
| jeremymc | I know the device has 2 addresses 0x98 and 0x9C
But it uses 0x98 jeremymc goes and poke drivers/base/regmap/regmap.c | [04:44] |
| larsc | you have multiple options for the main i2c address
and have to choose one that's 0x98 but the device has another couple of I2C addresses that are used to access other parts of the device it basically looks like multiple I2C devices | [04:45] |
| jeremymc | ok. | [04:46] |
| larsc | and those other addresses need to be configured | [04:46] |
| jeremymc | That sounds like I need to update the config then.
hmmm. | [04:46] |
| larsc | http://elixir.free-electrons.com/linux/latest/source/drivers/media/i2c/adv7604.c#L3117
this part is only used for devicetree | [04:47] |
| jeremymc | I am running this on a PCIe on an x86
only ACPI Cool I was on the right line | [04:48] |
| larsc | you have to add something similar for where you create the device
you probably have a PCI driver where you create the adv7619 I2C device | [04:48] |
| jeremymc | larsc, sounds right. Here is what I have https://hastebin.com/iyenopenoj.cpp
easier then trying to explain it. | [04:53] |
| larsc | yeah, just add the addresses to your pdata
if there are no other i2c devices on the bus you have free choice | [04:54] |
| jeremymc | So just all the items starting at line 3117 until "Hardcode the remaining platform data fields" | [04:55] |
| larsc | yes | [04:57] |
| jeremymc | larsc, thanks. | [04:59] |
| ........................................................................ (idle for 5h55mn) | ||
| hverkuil | Link to the mailinglist VIDIOC_SUBDEV_QUERYCAP discussion: http://www.mail-archive.com/linux-media@vger.kernel.org/msg116330.html
(for those who need to refresh their memory for the upcoming irc discussion about this) | [10:54] |
| pinchartl: sailus: mchehab: ping | [10:59] | |
| pinchartl | I'm here | [11:00] |
| hverkuil | 1 down, 2 to go... | [11:02] |
| mchehab | i'm here | [11:04] |
| hverkuil | I'll start with a quick background, sailus can read that when he hopefully joins.
There are two issues that I'd like to resolve regarding v4l-subdevX devices: 1) there is no ioctl that all v4l-subdev devices support. That makes it difficult for utilities like v4l2-ctl and v4l2-compliance (and other applications) to verify that the device is actually a v4l-subdev device. | [11:04] |
| sailus | Hello! | [11:06] |
| hverkuil | 2) it is IMHO highly desirable that both v4l-subdev and video devices that use the media controller can provide information of the media device they are part of and their entity ID. I.e., basically the parent information. | [11:08] |
| mchehab | with regards to (1), why an application should verify that the device is actually a v4l-subdev via an ioctl?
I mean, this could be done via udev with is, actually, the standard way used on modern Linux to identify a device type | [11:08] |
| sailus | hverkuil: Could we take a small step back in formulating the problem statement? | [11:10] |
| mchehab | (and, btw, parent information can also be retrieved via sysfs - with is what udev actually uses) | [11:10] |
| pinchartl | mchehab: udev isn't present in all embedded systems | [11:11] |
| sailus | I remember it was related to the v4l2-compliance tool. | [11:11] |
| mchehab | but sysfs is present
and the info is actually stored at sysfs | [11:11] |
| sailus | For what it's worth,libmediactl uses udev to look up the device nodes if it's available and if not, it uses sysfs. | [11:11] |
| hverkuil | I don't think the media device is the parent. | [11:12] |
| pinchartl | mchehab: /query sailus
oops :-) mchehab: yes, sysfs is present | [11:13] |
| mchehab | hverkuil: it should be easy to add the media device
ops | [11:13] |
| pinchartl | mchehab: how do you propose doing this through sysfs ? | [11:14] |
| mchehab | hverkuil: it should be easy to add the media device's parent to be reported at sysfs, at uevent | [11:14] |
| hverkuil | The information to get hold of the entity + media major/minor number is directly available in the media core. No need to write code for that, just add a core ioctl that returns that information. That provides a unique ioctl, and you can directly look up the entity information from the correct media device. | [11:14] |
| pinchartl | hverkuil: I believe there's a better idea | [11:15] |
| mchehab | hverkuil: true, but before poking around with ioctls, you need first to know the devnode's type
your assumption is that you don't know the type the only sane place where you can identify the type of a devnode is at sysfs | [11:15] |
| pinchartl | mchehab: how ? | [11:16] |
| hverkuil | Personally I think it is in general a good idea to have a QUERYCAP-like ioctl for device nodes, but that's a nice-to-have. | [11:16] |
| pinchartl | through /sys/dev/$major:$minor/subsystem ? | [11:16] |
| mchehab | pinchartl: sysfs fills it based on the class of the device | [11:17] |
| hverkuil | Today these v4l2- utilities check if VIDIOC_QUERYCAP exists. If it does, they assume it is a valid v4l2 node. | [11:17] |
| pinchartl | mchehab: sysfs fills *what* ?
mchehab: I'm not challenging your idea, I'd like to understand what you propose exactly | [11:17] |
| sailus | Today we have /sys/class/video4linux/videodevnode/device/mediadevnode . | [11:18] |
| mchehab | yes...
basically /sys/class/* is generated automatically when a kobject is registered based on the class of the object | [11:18] |
| pinchartl | mchehab: we start with a device node, right ? | [11:19] |
| mchehab | yes | [11:19] |
| hverkuil | E.g.: v4l2-ctl -d /dev/v4l-subdev0 | [11:19] |
| mchehab | at register_device | [11:20] |
| pinchartl | so what we have is a string that indicates the path of the device node | [11:20] |
| mchehab | (or similar) | [11:20] |
| pinchartl | note that in theory at least its name can be modified through udev rules
so it might not be called v4l-subdev* let's assume we have /dev/foo what's the next step ? | [11:20] |
| mchehab | on systems with udev, udev provides it for us via libudev | [11:21] |
| hverkuil | For reference: today I call VIDIOC_QUERYCAP on the device node. If it works, it's a v4l2 device, otherwise I bail out. | [11:21] |
| mchehab | on systems without udev, you'll still have /sys/class/video4linux/ | [11:21] |
| pinchartl | mchehab: let's assume we don't have udev
you are v4l2-compliance and you want to know the type of "/dev/foo" how do you proceed ? | [11:22] |
| mchehab | cat /sys/dev/char/81:0/uevent
that will give the information that was registered at the V4L class more info can be easily added there | [11:24] |
| pinchartl | # cat /sys/dev/char/81:77/uevent
MAJOR=81 MINOR=77 DEVNAME=v4l-subdev49 | [11:24] |
| mchehab | (right now, we just have the "default" ones)
DEVNAME doesn't change, even if udev changes the /dev/foo that's the kernel's "internal" name | [11:25] |
| pinchartl | # cat /sys/dev/char/81:0/uevent
MAJOR=81 MINOR=0 DEVNAME=video0 that actually sounds quite sensible to me | [11:25] |
| hverkuil | And /sys/class/video4linux/v4l-subdev0/device/mediaX/uevent gives the media info. | [11:26] |
| pinchartl | if we're unsure that a device node is of the correct type, verifying its type externally seems better than calling an ioctl | [11:26] |
| mchehab | hverkuil: we can easily add a rule there that would do something like:
MC=mc0 | [11:26] |
| pinchartl | as in theory at least, if we get passed a wrong device node, the ioctl number could correspond to a valid ioctl for that device node (sure, that's unlikely in practice)
and it could have bad side effects | [11:27] |
| mchehab | yes | [11:27] |
| hverkuil | At least, that's the case for vimc. | [11:27] |
| pinchartl | I don't expect that it will ever in practice | [11:27] |
| mchehab | (btw, 'V' is used on other subsystems as well, as far as I remember) | [11:27] |
| pinchartl | but verifying the type externally seems a safer approach to me
hverkuil: what do you think ? would external type verification sound like a better practice to you ? | [11:27] |
| mchehab | Documentation/ioctl/ioctl-number.txt:'V' all linux/vt.h conflict!
Documentation/ioctl/ioctl-number.txt:'V' all linux/videodev2.h conflict! Documentation/ioctl/ioctl-number.txt:'V' C0 linux/ivtvfb.h conflict! Documentation/ioctl/ioctl-number.txt:'V' C0 linux/ivtv.h conflict! Documentation/ioctl/ioctl-number.txt:'V' C0 media/davinci/vpfe_capture.h conflict! Documentation/ioctl/ioctl-number.txt:'V' C0 media/si4713.h conflict! | [11:28] |
| hverkuil | pinchartl: does the media info for a v4l-subdev always appear in /sys/class/video4linux/v4l-subdev0/device/mediaX? | [11:28] |
| mchehab | (actually, the real conflict is with VT) | [11:28] |
| pinchartl | hverkuil: as long as the driver sets the right parent of the MC device | [11:29] |
| hverkuil | Is the entity information available somewhere through sysfs? | [11:29] |
| pinchartl | I don't see a case where it wouldn't work | [11:29] |
| hverkuil | If not, should we add that? | [11:29] |
| pinchartl | entity information isn't, no | [11:29] |
| sailus | pinchartl: If there is a way to verify the node type externally, I think it'd be quite nice. | [11:29] |
| pinchartl | and I'd rather not add it to sysfs | [11:29] |
| hverkuil | I mean entity ID, to be precise. | [11:29] |
| pinchartl | but one thing at a time
for type verification do we agree that sysfs is fine ? hverkuil: that was item 1 in your list | [11:30] |
| hverkuil | I think looking up the type via sysfs will work. I can implement that in v4l-utils. | [11:30] |
| pinchartl | ok | [11:31] |
| sailus | sysfs is good for this IMO. It's practically available everywhere. | [11:31] |
| pinchartl | should we move to item 2 then ? | [11:31] |
| mchehab | hverkuil: take a look at rc/rc-main.c
seek for ADD_HOTPLUG_VAR we have some code there that adds extra stuff to uevent's content | [11:31] |
| sailus | (Or better yet, through udev. But still the info originates from sysfs I presume.) | [11:32] |
| hverkuil | I wasn't aware of /sys/dev/char/<major>:<minor>/uevent | [11:32] |
| sailus | Me neither. | [11:32] |
| mchehab | In the case of RC, we add two extra vars to it:
if (dev->rc_map.name) ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name); if (dev->driver_name) ADD_HOTPLUG_VAR("DRV_NAME=%s", dev->driver_name); (that is used by rc-keytable when called from udev, in order to detect the correct RC keymap) $ cat /sys/class/rc/rc0/uevent NAME=rc-rc6-mce DRV_NAME=nuvoton-cir | [11:32] |
| hverkuil | mchehab: interesting. Good to know. | [11:34] |
| mchehab | you can easily do that to place the MC name | [11:35] |
| pinchartl | hverkuil: are we good with type verification ? | [11:35] |
| hverkuil | pinchartl: yes. | [11:35] |
| mchehab | (in the case of RC, as this is virtual, there's no major, minor) | [11:35] |
| hverkuil | I learned something today :-) | [11:35] |
| pinchartl | now to the second item, going from subdev device node to media controller device node
I understand what you want to achieve but I think there's a better idea especially given that your use case is test applications if you have other use cases, we can discuss them first of all, we have agreed before I believe that subdev nodes should depend on MC. there are very few drivers today that don't register a media device and still use subdev nodes, and they should be fixed | [11:36] |
| hverkuil | It's testing, but also for simple quick lookups: | [11:37] |
| pinchartl | do I recall correctly that's what we agreed on ? | [11:38] |
| hverkuil | e.g. you want to know about a v4l-subdev node by simply calling 'v4l2-ctl -D -d /dev/v4l-subdevX' and it comes back with the information about it. It's sort of related to testing I guess.
Yes, I think we should modify the few drivers that have subdev nodes but no MC. Do you have that list of drivers that need to be converted? All I remember is cobalt (and I can do that). | [11:38] |
| pinchartl | no I don't, but that should be easy to find out | [11:39] |
| sailus | I only learned we had such drivers last week. :-) | [11:39] |
| pinchartl | rcar_drif
atmel-isc | [11:40] |
| hverkuil | There were only 3 or 4, I think.
It's likely that atmel-isc was unintended. | [11:40] |
| pinchartl | I think that's it
so, let's assume in this discussion that all drivers that register subdev nodes also register a media device (otherwise there's no point trying to get from the subdev node to the media device anyway) those drivers register a media device node subdev nodes and video nodes I call those drivers mc-centric as opposed to the video-centric drivers that have no media device or subdev node | [11:41] |
| mchehab | an easy way to do that would be to do something like:
if (dev->mdev) ADD_HOTPLUG_VAR("MC=%s", dev->mdev->name); | [11:43] |
| pinchartl | v4l2-compliance can test video nodes, regardless of whether they belong to a video-centric or mc-centric driver
but in the latter case, v4l2-compliance is limited in the tests it can perform as it can't configure the MC pipeline so it can't run the full set of tests hverkuil: is my understanding correct so far ? | [11:43] |
| mchehab | pinchartl: all video-centric drivers now have media device as well (if Kernel compiled with MC and MC-DVB support) | [11:45] |
| hverkuil | I want to have the media name for both v4l-subdev and video devices (if present). Not just v4l-subdev. But with your approach it works for both. | [11:45] |
| pinchartl | mchehab: even non-DVB devices such as bt878 ?
hverkuil: that doesn't tell me if my understanding is correct so far :) | [11:45] |
| hverkuil | mchehab: I'm pretty sure it is limited to DVB devices. | [11:46] |
| mchehab | bt878 may have DVB | [11:46] |
| hverkuil | pinchartl: yes, your understanding is correct. | [11:46] |
| pinchartl | mchehab: and I assume that DVB devices don't expose subdev nodes to userspace | [11:46] |
| mchehab | no | [11:46] |
| pinchartl | (at least most of them) | [11:46] |
| mchehab | neither video-centric ones | [11:46] |
| pinchartl | those DVB devices are video-centric in my mind, as they're fully controlled through the video device
well, the DVB device :-) | [11:47] |
| mchehab | pinchartl: let correct myself: not all video-centric devices have MC... yet | [11:47] |
| pinchartl | but they don't require MC-based configuration of formats on the pipeline | [11:47] |
| mchehab | but that's not preventing them to have... it is just a couple of lines to be added at the driver | [11:48] |
| pinchartl | mchehab: yes, and I'd like to see all drivers getting a media device at some point, but that's a different issue | [11:48] |
| mchehab | pinchartl: all pure DVB drivers have MC, as the feature is inside the DVB core | [11:48] |
| hverkuil | Right, can we get back to my issue? :-) | [11:48] |
| pinchartl | hverkuil: trying to :) | [11:48] |
| hverkuil | I think we're almost there. | [11:48] |
| pinchartl | hverkuil: so, your issue with v4l2 subdev compliance is related to MC-centric devices | [11:49] |
| mchehab | several video-centric V4L2 drivers also support MC
none of them export subdevs | [11:49] |
| hverkuil | mchehab: one issue at a time! | [11:49] |
| pinchartl | and for those, I believe a better idea would be for userspace to be MC-centric too
not just for subdevs but for video device compliance testing too | [11:49] |
| hverkuil | pinchartl: one issue at a time! | [11:49] |
| pinchartl | hverkuil: it's the same issue
with an MC-centric userspace approach, you can test the whole device and with such an approach, you don't need to go from subdev node to MC that's why I believe we shouldn't expose that information | [11:49] |
| hverkuil | All I want is to go from a v4l-subdev and/or video device to the media device. | [11:50] |
| mchehab | pinchartl: agreed. v4l2-compliance can work better if it is performed under /dev/mc0 | [11:50] |
| pinchartl | because we shouldn't use it in userspace | [11:50] |
| mchehab | (just my 5 cents) | [11:50] |
| pinchartl | hverkuil: I understand that, but I think it's not the right way to look at the problem
how can we convince userspace developers to implement their applications correctly, going from MC to devnodes, if we don't do it in the compliance tools we write ourselves ? :-) | [11:50] |
| sailus | Sub-device nodes are a pain to find. You practically need MC for that. I presume the intent would typically be to test sub-devices related to a particular media device. | [11:51] |
| mchehab | hverkuil: IMHO, you can use ADD_HOTPLUG_VAR() to do such mapping
then start querying the device from the MC node I mean, if someone does: | [11:51] |
| sailus | I mean, sub-device nodes are a pain to find if the first thing you're looking at are the device nodes under /dev. | [11:52] |
| mchehab | v4l2-ctl -D -d /dev/v4l-subdevX | [11:52] |
| hverkuil | And you *never* can replace video-centric drivers by mc-centric drivers. It will break userspace. It's not an option. And there are no resources to do something like that, even if we'd wanted to do this. | [11:52] |
| pinchartl | hverkuil: from years of MC-centric device testing, I can tell you that using a subdev device node path on the command line is very painful | [11:52] |
| mchehab | it would retrieve the MC node from sysfs | [11:52] |
| pinchartl | they can change at every bood
s/bood/boot/ | [11:52] |
| mchehab | and dig the device from MC
printing only the things that are specific to the subdev | [11:52] |
| pinchartl | in practice you'd have to run media-ctl to find the subdev node associated with the subdev you want to test
and then use that subdev node in a command line every time let's not inflict such pain on ourselves with a tool that takes as arguments a media device and a subdev name instead of a subdev node path, our lives will be much easier mchehab: we could pass the information through sysfs instead of an ioctl, but my point is that we shouldn't pass that information at all, as it just encourages userspace to do things in the wrong order hverkuil: I used to configure controls on a subdev using yavta + the subdev node name in a test script | [11:52] |
| mchehab | hverkuil: I see your point | [11:54] |
| pinchartl | I had to change my test script every other boot as the node changed
(that was during development) | [11:54] |
| hverkuil | pinchartl: I KNOW!
This is not how applications will use it. | [11:54] |
| pinchartl | and I quickly started using media-ctl -e in my script to get the subdev node path automatically
so what are the use cases you want to have this for ? my problem with passing that information to userspace isn't so much about how to pass it but it's that I don't see a use case | [11:55] |
| mchehab | hverkuil: what about something like:
v4l2-ctl -D -d /dev/mcX -s Y # If you want v4l2-ctl to only work with subdev Y | [11:56] |
| sailus | hverkuil: How will applications use sub-device nodes? I'd hope the typical usage pattern wouldn't be to just to open a node based on a node name under /dev. | [11:57] |
| hverkuil | But all I want to provide is a quick way for users to see what a video/v4l-subdev belongs to. Normally you use media-ctl to find the subdev, but if e.g. you have a script or kernel log that talks about a subdev node and you want to quickly see what it belongs to, then having the backlink is quite useful. And adding it to uevent seems very easy to do. | [11:57] |
| sailus | I know a lot of applications that are not test programs do this for video devices. | [11:57] |
| hverkuil | I just want to do 'v4l2-ctl -D -d /dev/v4l-subdevX' and have it return the driver + entity information. | [11:58] |
| pinchartl | hverkuil: I understand what you want to do, but I don't see why we would need that :-S | [11:58] |
| mchehab | I don't see myself any harm doing that | [11:58] |
| pinchartl | I don't see what it would bring us, except making it easier for applications or scripts to be developed in the wrong direction
the harm isn't so much as the API level, to me the harm is in giving userspace a way to shoot itself in the foot without any real benefit to compensate that | [11:59] |
| sailus | There might not be harm in providing that information as such, but it encourages a less-than-good usage pattern for accessing device nodes. | [11:59] |
| pinchartl | because I believe that userspace should be MC-centric for MC-centric devices | [12:00] |
| mchehab | pinchartl, sailus: I can't see how an userspace application/script won't be MC-centric with subdevs... | [12:00] |
| hverkuil | I truly can't see any reason why userspace would EVER want to use it in that way, it makes no sense whatsoever. | [12:00] |
| mchehab | I mean, even if it starts from a subdev's devnode, it should go to MC, in order to get the other subdevs at the pipeline | [12:00] |
| pinchartl | mchehab: v4l2-subdev-compliance -d /dev/v4l-subdev31 isn't MC-centric | [12:01] |
| hverkuil | I'm happy to add all sorts of disclaimers to the code in v4l2-ctl and v4l2-compliance for this. | [12:01] |
| pinchartl | v4l2-subdev-compliance -d /dev/media0 -Y "subdev name" is MC-centric | [12:01] |
| sailus | hverkuil: Could we make the test programs exemplary users instead? | [12:02] |
| pinchartl | hverkuil: but why do you need it in the v4l2-ctl and v4l2-compliance code to start with, if we can implement an MC-centric approach instead, without any (at least to me) drawback ? | [12:02] |
| sailus | What I find myself doing with yavta (to access device nodes) is just find out the sub-device node using media-ctl -e, because yavta doesn't do that itself. At least not yet.
media-ctl uses media device + entity name. That's quite a bit more practical. | [12:02] |
| pinchartl | hverkuil: do you think the MC-centric approach in v4l2-compliance (or a related separate subdev compliance tool, if we end up implementing it that way) would be impractical ?
if so I'm open to discussing the problems, as I think having good compliance tools is very important | [12:05] |
| hverkuil | v4l2-compliance is *just* for testing the given device node. For compliance testing an MC device we need a separate mc-compliance utility.
The test approach is quite different for MC devices (on a much higher level). Trying to cram that into v4l2-compliance would make it very messy. | [12:06] |
| pinchartl | while video nodes can be tested stand-alone to some extent (and, as we've discussed before, in a suboptimal way) with MC-centric devices, I don't think it makes much sense to test subdevs standalone | [12:07] |
| sailus | hverkuil: It'd be practical for a single tool to test compliancy of the related interfaces as well. | [12:08] |
| pinchartl | one reason is that subdev nodes are always part of an MC-centric device | [12:08] |
| sailus | In other words, why would you, by default, test only a sub-set of interfaces a device provides? | [12:08] |
| pinchartl | and the other reason is that they are usually tightly coupled
sailus: because we have to start somewhere. but even starting with partial compliance testing, I think it should be implemented with the longer term goal in mind, which is MC-centric testing | [12:08] |
| hverkuil | there is still a lot that can be tested: controls, formats, all that will work quite well. It's the divide and conquer approach: don't try to do everything in one huge utility. | [12:09] |
| mchehab | hverkuil: that's true, but, still, it is a way easier for us if one would just run: | [12:10] |
| pinchartl | hverkuil: I'm not saying we should add full MC testing to v4l2-compliance, the tool would get quite messy indeed. but we should be able to share code between v4l2-compliance and mc-v4l2-compliance | [12:10] |
| sailus | pinchartl: Yes, makes sense. This would be the ultimate goal indeed. | [12:10] |
| mchehab | v4l-mc-compliance /dev/mc0
than asking to run a v4l-mc-subdev-compliance for every /dev/subdev* | [12:10] |
| pinchartl | hverkuil: I think that started with a standalone subdev compliance is a dead end, and it requires adding a new API that we don't want userspace to use
that's why I think going straight for an MC-centric approach is better and it's easy to do if you restrict it to subdevs | [12:11] |
| sailus | I think the test program can be split between multiple modules (or libraries or whatever) if needed, the parts don't necessarily need to be separate binaries. | [12:11] |
| pinchartl | you don't have to perform complex pipeline setup, or handle everything | [12:11] |
| mchehab | sailus: agreed | [12:11] |
| pinchartl | it's just a matter of taking two arguments on the command line instead of a single one
MC device node path + entity name instead of V4L2 subdev device node path and as an added bonus, if the entity name argument is made optional, the tool could loop over all subdevs for that media device and test them all that's better compliance coverage, and we get it for free | [12:11] |
| mchehab | and the full test is what we really want when someone submits us a new driver | [12:13] |
| pinchartl | think about driver authors who submit patches
we require them to run compliance tools it's much easier for them if they can run the subdev compliance tool with the name of the MC device node and get all subdevs tested instead of having to look up all subdev node names and run the tool manually on each of them and if we implement the tool that way, we can easily extend it later with tests that cross subdev boundaries such as format propagation tests on the whole pipeline and the driver authors would get those new tests for free when upgrading the compliance tool without needing to even change the command line arguments or use a separate tool to put it differently, I support you idea of starting somewhere, with that somewhere being subdev and testing subdevs in isolation that's a good first step but let's do it with an MC-centric tool as that approach only has benefits as far as I can see | [12:13] |
| mchehab | pinchartl: I see just one drawback on it: the tool should be able to detect if a MC devnode is for a MC-centric or a video-centric device, in order to do the right thing (or to bail out, if we end by having 2 separate command line tools) | [12:16] |
| pinchartl | mchehab: that's easy, the MC graph reports major:minor for subdev nodes
and they're 0:0 if there's no subdev node | [12:16] |
| hverkuil | I hate that v4l devices are not able to provide the 'parent' link to the MC. It goes against everything I believe a good API should be. :-( | [12:17] |
| pinchartl | (or possibly -1:-1, I don't remember the details) | [12:17] |
| hverkuil | It seems I'm going to lose this battle, though. | [12:17] |
| pinchartl | hverkuil: believe me, I have my fair share of things I hate in V4L2 and MC
and I'd like to discuss them in Prague I will propose, once the dates are set, to allocate time to discuss issues with our current APIs things that people are not happy with and would like to change | [12:17] |
| hverkuil | ack | [12:18] |
| pinchartl | and I'm certainly open to add this to the list
but I'd like to add that feature only if it's useful | [12:18] |
| mchehab | pinchartl: that's true for our current way, but what happens if we end by having video-centric devices with subdev nodes in the future? | [12:19] |
| pinchartl | I'm not entirely against features that have very few users, at least not all the time | [12:19] |
| hverkuil | Anyway, for the initial work I can use sysfs to determine the type, then I can add support for v4l-subdevX devices to v4l2-ctl and v4l2-compliance, without initially knowing the media device. | [12:19] |
| pinchartl | but when they make it possible for userspace to easily do things wrong, and don't bring much benefits, I'm even more reluctant | [12:19] |
| hverkuil | At least it can be used to set/get controls and EDIDs from the command line for a v4l-subdev. | [12:20] |
| pinchartl | mchehab: I don't think that will happen, as the whole point of subdev nodes is to make the device MC-centric
hverkuil: sounds good to me. and if you can implement the code in a way that will make it easily useable in an MC compliance tool, that's even better I'd start with v4l2-ctl first as that's really standalone | [12:20] |
| hverkuil | Actually, I hope a colleague can work on this, not me. | [12:21] |
| pinchartl | then, for the compliance tool, I'm not sure if extending v4l2-compliance is the best option
maybe a separate tool for subdevs would be best | [12:21] |
| hverkuil | It's why I wanted this discussion since he's now back from vacation. | [12:21] |
| pinchartl | or naming it directly v4l2-mc-compliance, even if it tests subdevs only | [12:21] |
| mchehab | I would break v4l2-compliance into a sort of library | [12:21] |
| pinchartl | I don't think we should bikeshed naming now though :-) | [12:21] |
| mchehab | but keep just one command line for both cases | [12:21] |
| hverkuil | Yeah, yeah, one thing at a time. | [12:22] |
| mchehab | sudh command line should be smart enough to select the MC-centric library or the video-centric library | [12:22] |
| pinchartl | hverkuil: another thing to keep in mind, we might want to have standard functional testing too in the future
not just compliance testing so I expect we'll need to refactor the v4l2-compliance tool at some point in which direction, I don't know yet it might be too early to tell | [12:22] |
| mchehab | pinchartl: what do you mean by "standard functional"? | [12:23] |
| pinchartl | but once your colleague has an initial draft of what he would like to implement for subdev compliance testing, it would be nice to discuss it | [12:23] |
| mchehab | (standard for me is v4l2_std :-) ) | [12:23] |
| pinchartl | instead of waiting for the final patches and then realizing we have fundamental disagreements :)
mchehab: I mean standard tests (as opposed to device-specific tests) for functional testing things like running 100 start/capture 10 frames/stop cycles stress-testing the device in multiple ways to catch driver bugs it's not API compliance testing | [12:23] |
| mchehab | stress testing is something that we need
not sure it should be at the same tool, though | [12:25] |
| pinchartl | no it shouldn't, but I wouldn't be surprised if some of the code could be shared
hence the comment about refactoring but that's for later | [12:25] |
| mchehab | I guess the code there for v4l2-ctl is more likely to be shared for stress testing | [12:25] |
| pinchartl | mchehab: possibly, yes
but in any case I'd like to keep this in mind when developing test tools | [12:26] |
| mchehab | sure | [12:26] |
| pinchartl | but I certainly don't want to create internal libraries with a single user
otherwise it's guaranteed that the API will be wrong | [12:26] |
| hverkuil | Out of scope. ENOTIME. ENORESOURCES. Please, it took years of on-and-off work before we had a really useful v4l2-compliance test. Stick to things that are realistic and achievable. | [12:27] |
| pinchartl | hverkuil: my point is that it would be nice to keep in mind when making design decisions that code will be refactored later, and try not to make it difficult to refactor
hverkuil: as I said, it's long term and as for ENOTIME or ENORESOURCES I'm working on such a tool that is currently device-specific and I want to make it generic where it makes sense so there's time and resources :-) | [12:27] |
| mchehab | pinchartl: that's good news | [12:28] |
| hverkuil | Ah, that changes things. I didn't know. | [12:28] |
| pinchartl | the current version is implemented in bash and uses media-ctl and yavta as helpers
I'm now working on a Python-based test tool for KMS v4l2-compliance is C++-based and the DRM/KMS igt test tools are written in C | [12:28] |
| mchehab | pinchartl: one think you could add there is binding/unbinding devices... that's a good stress test for register/unregister logic | [12:29] |
| hverkuil | OK, can I summarize?
and if we all agree, then we can close this meeting. | [12:29] |
| pinchartl | mchehab: there's a large patch series from Sakari that we need to merge first...
hverkuil: please go ahead | [12:30] |
| sailus | pinchartl: Which one of them? :-) | [12:30] |
| hverkuil | 1) v4l2-ctl and v4l2-compliance will be modified to use sysfs to verify the device node type (using uevent information)
2) v4l2-ctl will be modified so it can handle a subdev device node for ioctls it has in common with a video device (i.e. controls, EDID, others?) 3) ditto for v4l2-compliance, only test those ioctls for which we already have test code initially. After that we'll see what the next step will be. | [12:30] |
| mchehab | sounds ok
pinchartl, sailus: if it is the series I'm thinking of - he needs to finish that series first... from what I understood, it is still a RFC and it will cause regressions on existing video-centric devices | [12:33] |
| hverkuil | Since my colleague is new to all this, I think this is a good starting point. | [12:33] |
| sailus | hverkuil: Looks good to me. | [12:34] |
| pinchartl | hverkuil: if it was me I wouldn't bother with 3, I would go for v4l2-mc-compliance directly, but I'm not opposed to that
1 and 2 are certainly fine | [12:34] |
| sailus | pinchartl, mchehab: Do you mean the media device referencing patchset? | [12:34] |
| mchehab | hverkuil: an additional bonus is if your colleague could break it into modules that could later be used for a MC compliance tool | [12:34] |
| pinchartl | mchehab: the last time we discussed it there was no agreement on the 3 reverts that started the patch series | [12:35] |
| mchehab | sailus: that was my understanding | [12:35] |
| pinchartl | mchehab: and yes, DVB drivers need to be handled as part of that patch series, it's currently missing | [12:35] |
| sailus | I've recently rebased it but unfortunately I haven't had time to work on it further. Properly addressing the problems require changing DVB drivers. | [12:35] |
| hverkuil | OK, thank you all for your input! | [12:35] |
| pinchartl | hverkuil: you're welcome | [12:35] |
| sailus | It's been a while, but they seemed to need reference counting for the node data structures as well. | [12:36] |
| pinchartl | mchehab: regarding the 3 reverts, to be very frank and direct, the current code base is in such a bad state that it hardly makes a difference if we break hotplug bisection in that series
the patches you merged were a fiasco for a variety of reasons (including review) I'm not interested in discussing responsabilities here as that would be pointless but we now have to fix the fallout and we have to make sure this will never happen again | [12:37] |
| mchehab | pinchartl: the patches applied fixed a serious regression that were preventing removing USB devices
whatever solution should not break USB device removal | [12:38] |
| pinchartl | they fixed a regression introduced in other patches you wrote :-)
and they introduced other problems they moved the race window | [12:39] |
| sailus | mchehab: I think it's fair to say this was very badly broken to begin with; it wasn't regression. | [12:39] |
| pinchartl | made it smaller in some cases
but they were not correct | [12:39] |
| mchehab | sailus: MC was ever broken for device's removal | [12:39] |
| sailus | Yes. | [12:39] |
| pinchartl | mchehab: correct
we all agree on that | [12:39] |
| mchehab | but USB drivers got broken when MC support was added | [12:39] |
| pinchartl | yes | [12:39] |
| mchehab | the patches fixed that | [12:40] |
| pinchartl | they fixed part of the problem
hotunplug is still broken but with a smaller race window | [12:40] |
| mchehab | you can safely remove/reinsert USB devices without breaking | [12:40] |
| pinchartl | we shouldn't have rushed to merge hacks that shortened the race window, what we should have done was to fix the problem correctly and then only add MC support to those DVB USB drivers
mchehab: no you can't. it's still not safe. it's safer, as the race window is smaller, but it's not safe you could say that shortening the race window is a good step forward but not when the implementation goes in the opposite direction of what is needed to completely close the window anyway, that's what happened, and we can't change it but we have to fix it now | [12:40] |
| mchehab | pinchartl: on my race regression tests, it was safe... tried bind/unbind 10.000 times on several devices, running multiple threads | [12:42] |
| pinchartl | mchehab: as I said, the race window is now small :-) | [12:42] |
| mchehab | I don't remember a single bug report related to it either | [12:42] |
| pinchartl | it's easier to reproduce when adding a sleep in the kernel to make it larger
and it's easier to reproduce on some devices the USB DVB devices are not the worst affected ones but the changes you merged didn't go in the direction of closing the race window completely | [12:42] |
| mchehab | the mc-centric devices were already broken, since ever | [12:43] |
| pinchartl | to close it completely we have to open it again fist
*first yes, they were broken, we all agree on that | [12:43] |
| mchehab | well, let's fix it | [12:43] |
| pinchartl | Sakari has submitted an RFC series for that
it needs more work | [12:43] |
| mchehab | I'm all for fixing it anyway that works and won't cause regressions to USB drivers | [12:43] |
| pinchartl | and I believe he plans to work on it
but closing the race window completely now requires opening it back | [12:44] |
| mchehab | I don't care much for whatever solution is taken, provided that it won't introduce regressions on USB drivers | [12:44] |
| pinchartl | and then closing it correctly
I'd really appreciate if you agreed on a patch series that temporarily increase the race window to close it properly assuming that it then closes it completely of course | [12:44] |
| mchehab | pinchartl: I would prefer the "reverse" approach...
I mean, place the revert patches at the end | [12:45] |
| pinchartl | mchehab: it's very difficult | [12:46] |
| mchehab | I suspect it is doable, and won't cause regressions | [12:46] |
| pinchartl | to give you an image
assume you're building a house and you install the window inside-out it's easier to take the window out and put it back the right way temporarily leaving a hole in the wall instead of keeping the window in place and moving the rest of the house around it | [12:46] |
| hverkuil | (Sorry, I can't really contribute to this. I would need to see the patch series (rebased) again and do a careful review and test it myself.) | [12:47] |
| pinchartl | it's not just a matter of moving the reverts at the end of the series
if it was that simple it would be done already | [12:47] |
| sailus | The crash can be reproduced easily by adding a small delay in current mediatree master.
https://www.mail-archive.com/linux-media@vger.kernel.org/msg106390.html | [12:48] |
| pinchartl | it's that the patch that are reverted modify the internal APIs in such a way that it's very difficult to fix it properly
s/patch/patches/ | [12:48] |
| sailus | Without a delay it might be difficult, but not impossible. | [12:48] |
| mchehab | sailus: you should try this on an USB driver | [12:48] |
| pinchartl | mchehab: I'm not calling for reverts just for the sake of it
without the reverts I expect one week of extra work at least | [12:48] |
| mchehab | I suspect that, on USB drivers, you won't be able to reproduce it | [12:49] |
| sailus | From kernel PoV it's not different, in both cases the hardware device disappears while device nodes are still open. | [12:49] |
| mchehab | sailus: no, the USB core takes care of it | [12:50] |
| sailus | How? These device nodes are created and removed by the driver. | [12:50] |
| mchehab | via the callback that it is called when the hardware is removed | [12:50] |
| sailus | I can try to do the same with a USB device, too.
I have one, I think it was em28xx or such. | [12:51] |
| mchehab | pinchartl: as we need to move on, while I hate this approach, if the entire series fix the issues, I'll accept it starting with the revert patches
(although IMHO it is the wrong way to do it) | [12:51] |
| sailus | I'll try to find the time for making the required DVB changes but I can't say now when those could be ready. | [12:51] |
| pinchartl | mchehab: thank you. I'm sure that Sakari will try not to revert patches if possible | [12:52] |
| mchehab | I would be a way more comfortable if Sakari would try to do the revert patches at the end
(or not to revert at all, if possible) anyway, let's move on and solve this issue as soon as we can sailus: yeah, you can try it with em28xx or even with some UVC camera | [12:52] |
| pinchartl | mchehab: if they're at the end there would be no revert. it's not for the sake of reverting patches, it's that without reverts it much more difficult to refactor the code in the right direction | [12:54] |
| mchehab | em28xx is more complex, with that matter - as it needs to unregister on several different cores | [12:54] |
| sailus | Oh, good point. | [12:54] |
| mchehab | if you're using em28xx, I suggest you to disable RC
RC unregister is tricky, and you might end to trigger some other race condition | [12:54] |
| sailus | mchehab: I actually tried to avoid reverts, but the resulting patches will be come hard to review. The changes will mix changing the approach as well as internal APIs.
So I reckoned with was better to start with reverts. | [12:55] |
| mchehab | (the problem with RC is that it has a kthread that polls the device on every 100ms) | [12:55] |
| sailus | Let's see how the result will look like with the DVB changes eventually.
And perhaps before that, what kind of DVB changes will be needed. I think we'll need a similar release callback as we have for V4L2. | [12:55] |
| mchehab | DVB has its own release logic inside DVB's kthread
it is very sensitive the less you touch there, the better it took *years* to make it reliable | [12:56] |
| sailus | It's a lot more straightforward to construct object dependencies before the patches limiting the time window during which bad stuff would happen. | [12:57] |
| mchehab | the kthread is at dvb-frontends.c
dvb_frontend.c | [12:57] |
| sailus | I hope I won't need to. :-) | [12:57] |
| mchehab | I have one pending patch touching it that I'm delaying for a long time... as I'm almost sure it will cause other regressions
it is currently based on semaphores | [12:58] |
| sailus | Uh-oh. | [13:00] |
| mchehab | and contains other black magic, like: set_freezable();
and mb(); fyi, that's the pending patch: https://patchwork.linuxtv.org/patch/27914/ I received another patch meant to solve it on some other way - adding mb() - I ended applying the second one, but I'm not sure if the issue pointed there were completely solved that's why I'm keeping this old patch queued I need some stress testing tool and time to setup some testbenchs to be sure about that on one of the bugs related to it, it was mentioned that the issue happened only on ARM | [13:00] |
| pinchartl | mchehab: I haven't read the patch, but if you need such black magic, isn't it a sign that something is wrong ? | [13:06] |
| mchehab | pinchartl: it is a sign that the thread there is complex
on DVB, when you open a /dev/dvb/adapterX/frontendY, it starts a kthread (if a kthread is not running) when all frontends are closed, or when the device is removed/unbound, the kthread stops it also stops before suspend and restarted at resume such thread polls the driver on every X seconds | [13:09] |
| pinchartl | but maybe it's overly complex. generally speaking, when you need to do something that nobody else has to do, then you're doing it wrong ("you" doesn't mean you here) | [13:11] |
| mchehab | I wouldn't have it design the way it is
on the bright side, at DVB, the drivers don't need to implement one callback per ioctl nor need a compliance tool, as the core itself implements all it is needed to support the userspace API so, it is a different design that the one we use at V4L2 | [13:12] |
| pinchartl | "no need for a test tool" is never right :-)
even if "everything" is implemented in the core | [13:15] |
| mchehab | yeah, I agree
I meant to say that it is not as important as for V4L2 two of the authors of the DVB core were part of the team that designed V4L2 API - I guess they wanted a more core-centric approach | [13:16] |
| pinchartl | I wonder what they would say about V4L3 | [13:17] |
| mchehab | as you may remember, in the past, every V4L2 driver needed to implement itself the V4L2 ioctl parser
even after our implementation of v4l2 framework, still most a lot of userspace API implementation actually happens at driver's level s/still most a lot of/still a lot of/ | [13:17] |
| hverkuil | mchehab: not all that much. I think the one area that could do with better core support is format negotiation, esp. when combined with crop/compose support. The other part is DT graph parsing. | [13:29] |
| mchehab | hverkuil: yeah, things have been improved. Still, some parts of the framework are not mandatory... drivers may still implement their own VB handling. same happens for control framework | [13:31] |
| hverkuil | mchehab: Not really. New drivers will be rejected if they don't use those frameworks. I haven't seen any driver submissions in a long time that didn't use it.
The only reason we still allow it is for unconverted old drivers (and uvc which is very difficult to convert to the control framework). | [13:36] |
| pinchartl | hverkuil: formats and selection rectangles are very painful with MC-centric devices. the API document is quite unclear in a few places, and many drivers don't comply
thats in my list of APIs we have to rework | [13:40] |
| hverkuil | I agree. It's one of the reasons why I want to work on the compliance utilities: you can't write code for that unless you make it absolutely clear what it is supposed to do.
the subdev framesizes/intervals ioctls are particularly, um, let's say 'unfortunate'. :-) | [13:42] |
| pinchartl | yes they are, very much
we need to get mentally prepared for breaking backward compatibility at some point we've accumulated too much crap :-/ | [13:44] |
| hverkuil | snawrocki: can you take a quick look at this one: https://patchwork.linuxtv.org/patch/42946/ | [13:47] |
| mchehab | breaking backward compat is always painful
from V4L1->V4L2, we took ~20 years if we can make all drivers to work equally well, using a framework that hides ioctl, a transition to some new API could be smoother... | [13:49] |
| pinchartl | mchehab: V4L2 was introduced in v2.4.0, in 2002
and we deprecated V4L in 2006 | [13:50] |
| mchehab | ops, sorry, I meant ~10 years
I guess we only got rid of the last V4L1 driver by 2010 or so | [13:50] |
| pinchartl | but for the future, we will likely be able to implement some level of backward compatibility, but 100% coverage won't be possible | [13:51] |
| mchehab | (or V4L1 api call from the core)
if we don't give 100% coverage, Linus won't likely allow pushing a new API at least for some time, we need to provide 100% coverage | [13:51] |
| hverkuil | mchehab: when do you plan to process pull requests? | [13:52] |
| mchehab | as we did with V4L1
hverkuil: this week, I'll probably be able to handle only on Friday been busy preparing for a trip this week (starting tomorrow night, I guess - still waiting for the air tickets) | [13:52] |
| hverkuil | OK. I did the small stuff now, but for the larger driver patch series I'll try to do that Thursday. I had planned to process patches last Friday, but that didn't work out. | [13:55] |
| pinchartl | mchehab: Linus hasn't rejected V4L2 or KMS, even though they didn't provide backward-compatibility with V4L1 and FBDEV | [13:57] |
| mchehab | pinchartl: we provided V4L1 backward compatibility when V4L2 was added | [13:57] |
| hverkuil | Linus either didn't know or didn't care, or most likely both :-)
that backward code never quite worked 100% as I remember. | [13:57] |
| mchehab | hverkuil: once he gave V4L1->V4L2 example as something he didn't want anymore
hverkuil: yes, but most drivers had both V4L1 and V4L2 ioctls the big problem we had on V4L1 backward compatible is with bttv | [13:58] |
| pinchartl | I'm fine calling it something else than V4L3 and moving it to a separate directory if it makes him happy :) | [13:59] |
| mchehab | on V4L1, it used to accept start streaming then resizing the buffers | [13:59] |
| hverkuil | We've broken drivers before in the past, usually removing custom ioctls or other weird stuff. | [13:59] |
| pinchartl | but the transition will be similar to the FBDEV to KMS transition
and I don't expect most drivers to be ported | [14:00] |
| mchehab | pinchartl: before starting a V4L3 (or whatever you want to call it), we need *a lot* of discussions
and a proposal for it :-) | [14:00] |
| hverkuil | Anyway, ideally you can implement the new API in drivers and provide backwards compat code in the v4l2 core. We do that already in some cases and it works fine. | [14:02] |
| mchehab | yes
I suspect that a transition like that nowadays won't be as painful as most drivers use the core s/core/framework/ | [14:02] |
| pinchartl | mchehab: I want to discuss that in Prague
or, rather, gather a list of issues | [14:03] |
| mchehab | pinchartl: OK
I think that we should do it in a way that all drivers could be converted | [14:04] |
| pinchartl | and I expect we'll need at least half a day just to discuss issues, without even discussing solutions
let's first gather issues, use cases and requirements and then we'll see what we can do | [14:04] |
| mchehab | ok | [14:05] |
| .................................... (idle for 2h59mn) | ||
| zrafa | pinchartl: hello
pinchartl: thanks for the advices yesterday, to start to debug my ov2680 sensor/camera in my atom isp / mipi pinchartl: I do not see atomisp module like you suggested (at menuconfig) pinchartl: I see staging drivers -> media staging drivers -> enable support for intel mipi camera drivers -> ov2680 I also see, cheking the linux kernel source files: | [17:04] |
| some atomisp2 folders
In .config I also see: CONFIG_STAGING_MEDIA=Y CONFIG_INTEL_ATOMISP=y CONFIG_VIDEO_OV2680=m So I have the ov2680 modules. But what about atomisp? Is that option selected automatically? (I did not find it at menuconfig) So I do not know if I can build atomisp as module | [17:11] | |
| ............ (idle for 56mn) | ||
| python476 | hi there
I found an old pinnacle usb pctv (em2870 chip), I run arch, loading the em28xx driver complains it cannot recognize the card. I rmmod... ;modprobe em28xx card=45 and still no /dev/video | [18:08] |
| .................................... (idle for 2h55mn) | ||
| h3lios | hi
I have a question about building linuxtv I'm following the basic instructions from https://www.linuxtv.org/wiki/index.php/How_to_Obtain,_Build_and_Install_V4L-DVB_Device_Drivers I have a binary kernel without a uvcvideo.ko and tried to build it for armhf however, when I run ./build it doesn't build any modules I tried enabling the modules with CONFIG_USB_VIDEO_CLASS=m and CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV=m in v4l/.config and v4l/.myconfig respectively but it seems to be ignored and taken from /lib/modules/<version>/build/config I tried adding it there as well (also tried /boot/config) but it seems to be ignored everywhere so I guess the high-level question is: how do I build a driver for something that was disabled in the kernel I'm building against | [21:04] |
| ..................... (idle for 1h43mn) | ||
| jeremymc | I am filling the adv76xx_platform_data for my adv7619 and there are bunch of items that are not listed in my recommended settings manual p4/14
i.e. .i2c_addresses[ADV7604_PAGE_VDP] = 0x24, is not available in my settings guide. Can I just remove them? | [22:53] |
| ............ (idle for 57mn) | ||
| *** | thiblahute has quit IRC (*.net *.split)
bengan has quit IRC (*.net *.split) | [23:51] |
| orwell.freenode.net sets mode: +o ChanServ | [23:56] | |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |