<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
lrusak: what device is that chip used in?
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
<br> lrusak, its in a PCIe capture card I am writing the driver for and plan o _TRYING_ to get it mainlined ;-)
<br> Here is some debug spew to help with context:
<br> https://hastebin.com/licovizabe.sql
larsc: <u>jeremymc</u>: well 0x00 is not a valid 7-bit address ;)
<br> I think the driver reads back the address from the register map
<br> if no address is otherwise provided
<br> maybe that particular sub I2C device does not exist in this device and has no default in the register
<br> check which sub-register map fails
<br> in adv76xx_dummy_client()
jeremymc: ah. Thats an idea.
larsc: or maybe the register just always reads 0
<br> I think to remember that they changed this
jeremymc: Where are the register maps
larsc: the device has multiple I2C addresses
<br> each address for one of the register maps
<br> and the addresses can be configured through the main map
jeremymc: I know the device has 2 addresses 0x98 and 0x9C
<br> But it uses 0x98
-: jeremymc goes and poke drivers/base/regmap/regmap.c
larsc: you have multiple options for the main i2c address
<br> and have to choose one
<br> that's 0x98
<br> but the device has another couple of I2C addresses that are used to access other parts of the device
<br> it basically looks like multiple I2C devices
jeremymc: ok.
larsc: and those other addresses need to be configured
jeremymc: That sounds like I need to update the config then.
<br> hmmm.
larsc: http://elixir.free-electrons.com/linux/latest/source/drivers/media/i2c/adv7604.c#L3117
<br> this part is only used for devicetree
jeremymc: I am running this on a PCIe on an x86
<br> only ACPI
<br> Cool I was on the right line
larsc: you have to add something similar for where you create the device
<br> you probably have a PCI driver where you create the adv7619 I2C device
jeremymc: larsc, sounds right. Here is what I have https://hastebin.com/iyenopenoj.cpp
<br> easier then trying to explain it.
larsc: yeah, just add the addresses to your pdata
<br> if there are no other i2c devices on the bus you have free choice
jeremymc: So just all the items starting at line 3117 until "Hardcode the remaining platform data fields"
larsc: yes
jeremymc: larsc, thanks.
hverkuil: Link to the mailinglist VIDIOC_SUBDEV_QUERYCAP discussion: http://www.mail-archive.com/linux-media@vger.kernel.org/msg116330.html
<br> (for those who need to refresh their memory for the upcoming irc discussion about this)
<br> <u>pinchartl</u>: sailus: mchehab: ping
pinchartl: I'm here
hverkuil: 1 down, 2 to go...
mchehab: i'm here
hverkuil: I'll start with a quick background, sailus can read that when he hopefully joins.
<br> There are two issues that I'd like to resolve regarding v4l-subdevX devices:
<br> 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.
sailus: Hello!
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.
mchehab: with regards to (1), why an application should verify that the device is actually a v4l-subdev via an ioctl?
<br> I mean, this could be done via udev
<br> with is, actually, the standard way used on modern Linux to identify a device type
sailus: <u>hverkuil</u>: Could we take a small step back in formulating the problem statement?
mchehab: (and, btw, parent information can also be retrieved via sysfs - with is what udev actually uses)
pinchartl: <u>mchehab</u>: udev isn't present in all embedded systems
sailus: I remember it was related to the v4l2-compliance tool.
mchehab: but sysfs is present
<br> and the info is actually stored at sysfs
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.
hverkuil: I don't think the media device is the parent.
pinchartl: <u>mchehab</u>: /query sailus
<br> oops :-)
<br> <u>mchehab</u>: yes, sysfs is present
mchehab: <u>hverkuil</u>: it should be easy to add the media device
<br> ops
pinchartl: <u>mchehab</u>: how do you propose doing this through sysfs ?
mchehab: <u>hverkuil</u>: it should be easy to add the media device's parent to be reported at sysfs, at uevent
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.
pinchartl: <u>hverkuil</u>: I believe there's a better idea
mchehab: <u>hverkuil</u>: true, but before poking around with ioctls, you need first to know the devnode's type
<br> your assumption is that you don't know the type
<br> the only sane place where you can identify the type of a devnode is at sysfs
pinchartl: <u>mchehab</u>: how ?
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.
pinchartl: through /sys/dev/$major:$minor/subsystem ?
mchehab: <u>pinchartl</u>: sysfs fills it based on the class of the device
hverkuil: Today these v4l2- utilities check if VIDIOC_QUERYCAP exists. If it does, they assume it is a valid v4l2 node.
pinchartl: <u>mchehab</u>: sysfs fills *what* ?
<br> <u>mchehab</u>: I'm not challenging your idea, I'd like to understand what you propose exactly
sailus: Today we have /sys/class/video4linux/videodevnode/device/mediadevnode .
mchehab: yes...
<br> basically /sys/class/* is generated automatically when a kobject is registered
<br> based on the class of the object
pinchartl: <u>mchehab</u>: we start with a device node, right ?
mchehab: yes
hverkuil: E.g.: v4l2-ctl -d /dev/v4l-subdev0
mchehab: at register_device
pinchartl: so what we have is a string that indicates the path of the device node
mchehab: (or similar)
pinchartl: note that in theory at least its name can be modified through udev rules
<br> so it might not be called v4l-subdev*
<br> let's assume we have /dev/foo
<br> what's the next step ?
mchehab: on systems with udev, udev provides it for us via libudev
hverkuil: For reference: today I call VIDIOC_QUERYCAP on the device node. If it works, it's a v4l2 device, otherwise I bail out.
mchehab: on systems without udev, you'll still have /sys/class/video4linux/
pinchartl: <u>mchehab</u>: let's assume we don't have udev
<br> you are v4l2-compliance
<br> and you want to know the type of "/dev/foo"
<br> how do you proceed ?
mchehab: cat /sys/dev/char/81:0/uevent
<br> that will give the information that was registered at the V4L class
<br> more info can be easily added there
pinchartl: # cat /sys/dev/char/81:77/uevent
<br> MAJOR=81
<br> MINOR=77
<br> DEVNAME=v4l-subdev49
mchehab: (right now, we just have the "default" ones)
<br> DEVNAME doesn't change, even if udev changes the /dev/foo
<br> that's the kernel's "internal" name
pinchartl: # cat /sys/dev/char/81:0/uevent
<br> MAJOR=81
<br> MINOR=0
<br> DEVNAME=video0
<br> that actually sounds quite sensible to me
hverkuil: And /sys/class/video4linux/v4l-subdev0/device/mediaX/uevent gives the media info.
pinchartl: if we're unsure that a device node is of the correct type, verifying its type externally seems better than calling an ioctl
mchehab: <u>hverkuil</u>: we can easily add a rule there that would do something like:
<br> MC=mc0
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)
<br> and it could have bad side effects
mchehab: yes
hverkuil: At least, that's the case for vimc.
pinchartl: I don't expect that it will ever in practice
mchehab: (btw, 'V' is used on other subsystems as well, as far as I remember)
pinchartl: but verifying the type externally seems a safer approach to me
<br> <u>hverkuil</u>: what do you think ? would external type verification sound like a better practice to you ?
mchehab: Documentation/ioctl/ioctl-number.txt:'V' all linux/vt.h conflict!
<br> Documentation/ioctl/ioctl-number.txt:'V' all linux/videodev2.h conflict!
<br> Documentation/ioctl/ioctl-number.txt:'V' C0 linux/ivtvfb.h conflict!
<br> Documentation/ioctl/ioctl-number.txt:'V' C0 linux/ivtv.h conflict!
<br> Documentation/ioctl/ioctl-number.txt:'V' C0 media/davinci/vpfe_capture.h conflict!
<br> Documentation/ioctl/ioctl-number.txt:'V' C0 media/si4713.h conflict!
hverkuil: <u>pinchartl</u>: does the media info for a v4l-subdev always appear in /sys/class/video4linux/v4l-subdev0/device/mediaX?
mchehab: (actually, the real conflict is with VT)
pinchartl: <u>hverkuil</u>: as long as the driver sets the right parent of the MC device
hverkuil: Is the entity information available somewhere through sysfs?
pinchartl: I don't see a case where it wouldn't work
hverkuil: If not, should we add that?
pinchartl: entity information isn't, no
sailus: <u>pinchartl</u>: If there is a way to verify the node type externally, I think it'd be quite nice.
pinchartl: and I'd rather not add it to sysfs
hverkuil: I mean entity ID, to be precise.
pinchartl: but one thing at a time
<br> for type verification
<br> do we agree that sysfs is fine ?
<br> <u>hverkuil</u>: that was item 1 in your list
hverkuil: I think looking up the type via sysfs will work. I can implement that in v4l-utils.
pinchartl: ok
sailus: sysfs is good for this IMO. It's practically available everywhere.
pinchartl: should we move to item 2 then ?
mchehab: <u>hverkuil</u>: take a look at rc/rc-main.c
<br> seek for ADD_HOTPLUG_VAR
<br> we have some code there that adds extra stuff to uevent's content
sailus: (Or better yet, through udev. But still the info originates from sysfs I presume.)
hverkuil: I wasn't aware of /sys/dev/char/<major>:<minor>/uevent
sailus: Me neither.
mchehab: In the case of RC, we add two extra vars to it:
<br> if (dev->rc_map.name)
<br> ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name);
<br> if (dev->driver_name)
<br> ADD_HOTPLUG_VAR("DRV_NAME=%s", dev->driver_name);
<br> (that is used by rc-keytable when called from udev, in order to detect the correct RC keymap)
<br> $ cat /sys/class/rc/rc0/uevent
<br> NAME=rc-rc6-mce
<br> DRV_NAME=nuvoton-cir
hverkuil: <u>mchehab</u>: interesting. Good to know.
mchehab: you can easily do that to place the MC name
pinchartl: <u>hverkuil</u>: are we good with type verification ?
hverkuil: <u>pinchartl</u>: yes.
mchehab: (in the case of RC, as this is virtual, there's no major, minor)
hverkuil: I learned something today :-)
pinchartl: now to the second item, going from subdev device node to media controller device node
<br> I understand what you want to achieve
<br> but I think there's a better idea
<br> especially given that your use case is test applications
<br> if you have other use cases, we can discuss them
<br> 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
hverkuil: It's testing, but also for simple quick lookups:
pinchartl: do I recall correctly that's what we agreed on ?
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.
<br> Yes, I think we should modify the few drivers that have subdev nodes but no MC.
<br> Do you have that list of drivers that need to be converted?
<br> All I remember is cobalt (and I can do that).
pinchartl: no I don't, but that should be easy to find out
sailus: I only learned we had such drivers last week. :-)
pinchartl: rcar_drif
<br> atmel-isc
hverkuil: There were only 3 or 4, I think.
<br> It's likely that atmel-isc was unintended.
pinchartl: I think that's it
<br> so, let's assume in this discussion that all drivers that register subdev nodes also register a media device
<br> (otherwise there's no point trying to get from the subdev node to the media device anyway)
<br> those drivers register a media device node
<br> subdev nodes
<br> and video nodes
<br> I call those drivers mc-centric
<br> as opposed to the video-centric drivers that have no media device or subdev node
mchehab: an easy way to do that would be to do something like:
<br> if (dev->mdev)
<br> ADD_HOTPLUG_VAR("MC=%s", dev->mdev->name);
pinchartl: v4l2-compliance can test video nodes, regardless of whether they belong to a video-centric or mc-centric driver
<br> but in the latter case, v4l2-compliance is limited in the tests it can perform
<br> as it can't configure the MC pipeline
<br> so it can't run the full set of tests
<br> <u>hverkuil</u>: is my understanding correct so far ?
mchehab: <u>pinchartl</u>: all video-centric drivers now have media device as well (if Kernel compiled with MC and MC-DVB support)
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.
pinchartl: <u>mchehab</u>: even non-DVB devices such as bt878 ?
<br> <u>hverkuil</u>: that doesn't tell me if my understanding is correct so far :)
hverkuil: <u>mchehab</u>: I'm pretty sure it is limited to DVB devices.
mchehab: bt878 may have DVB
hverkuil: <u>pinchartl</u>: yes, your understanding is correct.
pinchartl: <u>mchehab</u>: and I assume that DVB devices don't expose subdev nodes to userspace
mchehab: no
pinchartl: (at least most of them)
mchehab: neither video-centric ones
pinchartl: those DVB devices are video-centric in my mind, as they're fully controlled through the video device
<br> well, the DVB device :-)
mchehab: <u>pinchartl</u>: let correct myself: not all video-centric devices have MC... yet
pinchartl: but they don't require MC-based configuration of formats on the pipeline
mchehab: but that's not preventing them to have... it is just a couple of lines to be added at the driver
pinchartl: <u>mchehab</u>: yes, and I'd like to see all drivers getting a media device at some point, but that's a different issue
mchehab: <u>pinchartl</u>: all pure DVB drivers have MC, as the feature is inside the DVB core
hverkuil: Right, can we get back to my issue? :-)
pinchartl: <u>hverkuil</u>: trying to :)
hverkuil: I think we're almost there.
pinchartl: <u>hverkuil</u>: so, your issue with v4l2 subdev compliance is related to MC-centric devices
mchehab: several video-centric V4L2 drivers also support MC
<br> none of them export subdevs
hverkuil: <u>mchehab</u>: one issue at a time!
pinchartl: and for those, I believe a better idea would be for userspace to be MC-centric too
<br> not just for subdevs
<br> but for video device compliance testing too
hverkuil: <u>pinchartl</u>: one issue at a time!
pinchartl: <u>hverkuil</u>: it's the same issue
<br> with an MC-centric userspace approach, you can test the whole device
<br> and with such an approach, you don't need to go from subdev node to MC
<br> that's why I believe we shouldn't expose that information
hverkuil: All I want is to go from a v4l-subdev and/or video device to the media device.
mchehab: <u>pinchartl</u>: agreed. v4l2-compliance can work better if it is performed under /dev/mc0
pinchartl: because we shouldn't use it in userspace
mchehab: (just my 5 cents)
pinchartl: <u>hverkuil</u>: I understand that, but I think it's not the right way to look at the problem
<br> 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 ? :-)
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.
mchehab: <u>hverkuil</u>: IMHO, you can use ADD_HOTPLUG_VAR() to do such mapping
<br> then start querying the device from the MC node
<br> I mean, if someone does:
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.
mchehab: v4l2-ctl -D -d /dev/v4l-subdevX
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.
pinchartl: <u>hverkuil</u>: 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
mchehab: it would retrieve the MC node from sysfs
pinchartl: they can change at every bood
<br> s/bood/boot/
mchehab: and dig the device from MC
<br> printing only the things that are specific to the subdev
pinchartl: in practice you'd have to run media-ctl to find the subdev node associated with the subdev you want to test
<br> and then use that subdev node in a command line
<br> every time
<br> let's not inflict such pain on ourselves
<br> 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
<br> <u>mchehab</u>: 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
<br> <u>hverkuil</u>: I used to configure controls on a subdev using yavta + the subdev node name
<br> in a test script
mchehab: <u>hverkuil</u>: I see your point
pinchartl: I had to change my test script every other boot as the node changed
<br> (that was during development)
hverkuil: <u>pinchartl</u>: I KNOW!
<br> This is not how applications will use it.
pinchartl: and I quickly started using media-ctl -e in my script to get the subdev node path automatically
<br> so what are the use cases you want to have this for ?
<br> my problem with passing that information to userspace isn't so much about how to pass it
<br> but it's that I don't see a use case
mchehab: <u>hverkuil</u>: what about something like:
<br> v4l2-ctl -D -d /dev/mcX -s Y # If you want v4l2-ctl to only work with subdev Y
sailus: <u>hverkuil</u>: 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.
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.
sailus: I know a lot of applications that are not test programs do this for video devices.
hverkuil: I just want to do 'v4l2-ctl -D -d /dev/v4l-subdevX' and have it return the driver + entity information.
pinchartl: <u>hverkuil</u>: I understand what you want to do, but I don't see why we would need that :-S
mchehab: I don't see myself any harm doing that
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
<br> 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
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.
pinchartl: because I believe that userspace should be MC-centric for MC-centric devices
mchehab: pinchartl, sailus: I can't see how an userspace application/script won't be MC-centric with subdevs...
hverkuil: I truly can't see any reason why userspace would EVER want to use it in that way, it makes no sense whatsoever.
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
pinchartl: <u>mchehab</u>: v4l2-subdev-compliance -d /dev/v4l-subdev31 isn't MC-centric
hverkuil: I'm happy to add all sorts of disclaimers to the code in v4l2-ctl and v4l2-compliance for this.
pinchartl: v4l2-subdev-compliance -d /dev/media0 -Y "subdev name" is MC-centric
sailus: <u>hverkuil</u>: Could we make the test programs exemplary users instead?
pinchartl: <u>hverkuil</u>: 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 ?
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.
<br> media-ctl uses media device + entity name. That's quite a bit more practical.
pinchartl: <u>hverkuil</u>: 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 ?
<br> if so I'm open to discussing the problems, as I think having good compliance tools is very important
hverkuil: v4l2-compliance is *just* for testing the given device node. For compliance testing an MC device we need a separate mc-compliance utility.
<br> The test approach is quite different for MC devices (on a much higher level).
<br> Trying to cram that into v4l2-compliance would make it very messy.
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
sailus: <u>hverkuil</u>: It'd be practical for a single tool to test compliancy of the related interfaces as well.
pinchartl: one reason is that subdev nodes are always part of an MC-centric device
sailus: In other words, why would you, by default, test only a sub-set of interfaces a device provides?
pinchartl: and the other reason is that they are usually tightly coupled
<br> <u>sailus</u>: 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
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.
mchehab: <u>hverkuil</u>: that's true, but, still, it is a way easier for us if one would just run:
pinchartl: <u>hverkuil</u>: 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
sailus: <u>pinchartl</u>: Yes, makes sense. This would be the ultimate goal indeed.
mchehab: v4l-mc-compliance /dev/mc0
<br> than asking to run a v4l-mc-subdev-compliance for every /dev/subdev*
pinchartl: <u>hverkuil</u>: 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
<br> that's why I think going straight for an MC-centric approach is better
<br> and it's easy to do if you restrict it to subdevs
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.
pinchartl: you don't have to perform complex pipeline setup, or handle everything
mchehab: <u>sailus</u>: agreed
pinchartl: it's just a matter of taking two arguments on the command line instead of a single one
<br> MC device node path + entity name instead of V4L2 subdev device node path
<br> 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
<br> that's better compliance coverage, and we get it for free
mchehab: and the full test is what we really want when someone submits us a new driver
pinchartl: think about driver authors who submit patches
<br> we require them to run compliance tools
<br> it's much easier for them if they can run the subdev compliance tool with the name of the MC device node
<br> and get all subdevs tested
<br> instead of having to look up all subdev node names and run the tool manually on each of them
<br> and if we implement the tool that way, we can easily extend it later with tests that cross subdev boundaries
<br> such as format propagation tests on the whole pipeline
<br> and the driver authors would get those new tests for free when upgrading the compliance tool
<br> without needing to even change the command line arguments
<br> or use a separate tool
<br> to put it differently, I support you idea of starting somewhere, with that somewhere being subdev
<br> and testing subdevs in isolation
<br> that's a good first step
<br> but let's do it with an MC-centric tool
<br> as that approach only has benefits as far as I can see
mchehab: <u>pinchartl</u>: 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)
pinchartl: <u>mchehab</u>: that's easy, the MC graph reports major:minor for subdev nodes
<br> and they're 0:0 if there's no subdev node
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. :-(
pinchartl: (or possibly -1:-1, I don't remember the details)
hverkuil: It seems I'm going to lose this battle, though.
pinchartl: <u>hverkuil</u>: believe me, I have my fair share of things I hate in V4L2 and MC
<br> and I'd like to discuss them in Prague
<br> I will propose, once the dates are set, to allocate time to discuss issues with our current APIs
<br> things that people are not happy with and would like to change
hverkuil: ack
pinchartl: and I'm certainly open to add this to the list
<br> but I'd like to add that feature only if it's useful
mchehab: <u>pinchartl</u>: that's true for our current way, but what happens if we end by having video-centric devices with subdev nodes in the future?
pinchartl: I'm not entirely against features that have very few users, at least not all the time
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.
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
hverkuil: At least it can be used to set/get controls and EDIDs from the command line for a v4l-subdev.
pinchartl: <u>mchehab</u>: I don't think that will happen, as the whole point of subdev nodes is to make the device MC-centric
<br> <u>hverkuil</u>: 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
<br> I'd start with v4l2-ctl first
<br> as that's really standalone
hverkuil: Actually, I hope a colleague can work on this, not me.
pinchartl: then, for the compliance tool, I'm not sure if extending v4l2-compliance is the best option
<br> maybe a separate tool for subdevs would be best
hverkuil: It's why I wanted this discussion since he's now back from vacation.
pinchartl: or naming it directly v4l2-mc-compliance, even if it tests subdevs only
mchehab: I would break v4l2-compliance into a sort of library
pinchartl: I don't think we should bikeshed naming now though :-)
mchehab: but keep just one command line for both cases
hverkuil: Yeah, yeah, one thing at a time.
mchehab: sudh command line should be smart enough to select the MC-centric library or the video-centric library
pinchartl: <u>hverkuil</u>: another thing to keep in mind, we might want to have standard functional testing too in the future
<br> not just compliance testing
<br> so I expect we'll need to refactor the v4l2-compliance tool at some point
<br> in which direction, I don't know yet
<br> it might be too early to tell
mchehab: <u>pinchartl</u>: what do you mean by "standard functional"?
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
mchehab: (standard for me is v4l2_std :-) )
pinchartl: instead of waiting for the final patches and then realizing we have fundamental disagreements :)
<br> <u>mchehab</u>: I mean standard tests (as opposed to device-specific tests) for functional testing
<br> things like running 100 start/capture 10 frames/stop cycles
<br> stress-testing the device in multiple ways
<br> to catch driver bugs
<br> it's not API compliance testing
mchehab: stress testing is something that we need
<br> not sure it should be at the same tool, though
pinchartl: no it shouldn't, but I wouldn't be surprised if some of the code could be shared
<br> hence the comment about refactoring
<br> but that's for later
mchehab: I guess the code there for v4l2-ctl is more likely to be shared for stress testing
pinchartl: <u>mchehab</u>: possibly, yes
<br> but in any case
<br> I'd like to keep this in mind when developing test tools
mchehab: sure
pinchartl: but I certainly don't want to create internal libraries with a single user
<br> otherwise it's guaranteed that the API will be wrong
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.
pinchartl: <u>hverkuil</u>: 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
<br> <u>hverkuil</u>: as I said, it's long term
<br> and as for ENOTIME or ENORESOURCES
<br> I'm working on such a tool that is currently device-specific
<br> and I want to make it generic where it makes sense
<br> so there's time and resources :-)
mchehab: <u>pinchartl</u>: that's good news
hverkuil: Ah, that changes things. I didn't know.
pinchartl: the current version is implemented in bash and uses media-ctl and yavta as helpers
<br> I'm now working on a Python-based test tool for KMS
<br> v4l2-compliance is C++-based
<br> and the DRM/KMS igt test tools are written in C
mchehab: <u>pinchartl</u>: one think you could add there is binding/unbinding devices... that's a good stress test for register/unregister logic
hverkuil: OK, can I summarize?
<br> and if we all agree, then we can close this meeting.
pinchartl: <u>mchehab</u>: there's a large patch series from Sakari that we need to merge first...
<br> <u>hverkuil</u>: please go ahead
sailus: <u>pinchartl</u>: Which one of them? :-)
hverkuil: 1) v4l2-ctl and v4l2-compliance will be modified to use sysfs to verify the device node type (using uevent information)
<br> 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?)
<br> 3) ditto for v4l2-compliance, only test those ioctls for which we already have test code initially.
<br> After that we'll see what the next step will be.
mchehab: sounds ok
<br> 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
hverkuil: Since my colleague is new to all this, I think this is a good starting point.
sailus: <u>hverkuil</u>: Looks good to me.
pinchartl: <u>hverkuil</u>: 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
<br> 1 and 2 are certainly fine
sailus: pinchartl, mchehab: Do you mean the media device referencing patchset?
mchehab: <u>hverkuil</u>: an additional bonus is if your colleague could break it into modules that could later be used for a MC compliance tool
pinchartl: <u>mchehab</u>: the last time we discussed it there was no agreement on the 3 reverts that started the patch series
mchehab: <u>sailus</u>: that was my understanding
pinchartl: <u>mchehab</u>: and yes, DVB drivers need to be handled as part of that patch series, it's currently missing
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.
hverkuil: OK, thank you all for your input!
pinchartl: <u>hverkuil</u>: you're welcome
sailus: It's been a while, but they seemed to need reference counting for the node data structures as well.
pinchartl: <u>mchehab</u>: 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
<br> the patches you merged were a fiasco for a variety of reasons (including review)
<br> I'm not interested in discussing responsabilities here as that would be pointless
<br> but we now have to fix the fallout
<br> and we have to make sure this will never happen again
mchehab: <u>pinchartl</u>: the patches applied fixed a serious regression that were preventing removing USB devices
<br> whatever solution should not break USB device removal
pinchartl: they fixed a regression introduced in other patches you wrote :-)
<br> and they introduced other problems
<br> they moved the race window
sailus: <u>mchehab</u>: I think it's fair to say this was very badly broken to begin with; it wasn't regression.
pinchartl: made it smaller in some cases
<br> but they were not correct
mchehab: <u>sailus</u>: MC was ever broken for device's removal
sailus: Yes.
pinchartl: <u>mchehab</u>: correct
<br> we all agree on that
mchehab: but USB drivers got broken when MC support was added
pinchartl: yes
mchehab: the patches fixed that
pinchartl: they fixed part of the problem
<br> hotunplug is still broken
<br> but with a smaller race window
mchehab: you can safely remove/reinsert USB devices without breaking
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
<br> <u>mchehab</u>: no you can't. it's still not safe. it's safer, as the race window is smaller, but it's not safe
<br> you could say that shortening the race window is a good step forward
<br> but not when the implementation goes in the opposite direction of what is needed to completely close the window
<br> anyway, that's what happened, and we can't change it
<br> but we have to fix it now
mchehab: <u>pinchartl</u>: on my race regression tests, it was safe... tried bind/unbind 10.000 times on several devices, running multiple threads
pinchartl: <u>mchehab</u>: as I said, the race window is now small :-)
mchehab: I don't remember a single bug report related to it either
pinchartl: it's easier to reproduce when adding a sleep in the kernel to make it larger
<br> and it's easier to reproduce on some devices
<br> the USB DVB devices are not the worst affected ones
<br> but the changes you merged didn't go in the direction of closing the race window completely
mchehab: the mc-centric devices were already broken, since ever
pinchartl: to close it completely we have to open it again fist
<br> *first
<br> yes, they were broken, we all agree on that
mchehab: well, let's fix it
pinchartl: Sakari has submitted an RFC series for that
<br> it needs more work
mchehab: I'm all for fixing it anyway that works and won't cause regressions to USB drivers
pinchartl: and I believe he plans to work on it
<br> but closing the race window completely now requires opening it back
mchehab: I don't care much for whatever solution is taken, provided that it won't introduce regressions on USB drivers
pinchartl: and then closing it correctly
<br> I'd really appreciate if you agreed on a patch series that temporarily increase the race window to close it properly
<br> assuming that it then closes it completely of course
mchehab: <u>pinchartl</u>: I would prefer the "reverse" approach...
<br> I mean, place the revert patches at the end
pinchartl: <u>mchehab</u>: it's very difficult
mchehab: I suspect it is doable, and won't cause regressions
pinchartl: to give you an image
<br> assume you're building a house
<br> and you install the window inside-out
<br> it's easier to take the window out and put it back the right way
<br> temporarily leaving a hole in the wall
<br> instead of keeping the window in place and moving the rest of the house around it
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.)
pinchartl: it's not just a matter of moving the reverts at the end of the series
<br> if it was that simple it would be done already
sailus: The crash can be reproduced easily by adding a small delay in current mediatree master.
<br> https://www.mail-archive.com/linux-media@vger.kernel.org/msg106390.html
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
<br> s/patch/patches/
sailus: Without a delay it might be difficult, but not impossible.
mchehab: <u>sailus</u>: you should try this on an USB driver
pinchartl: <u>mchehab</u>: I'm not calling for reverts just for the sake of it
<br> without the reverts I expect one week of extra work
<br> at least
mchehab: I suspect that, on USB drivers, you won't be able to reproduce it
sailus: From kernel PoV it's not different, in both cases the hardware device disappears while device nodes are still open.
mchehab: <u>sailus</u>: no, the USB core takes care of it
sailus: How? These device nodes are created and removed by the driver.
mchehab: via the callback that it is called when the hardware is removed
sailus: I can try to do the same with a USB device, too.
<br> I have one, I think it was em28xx or such.
mchehab: <u>pinchartl</u>: 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
<br> (although IMHO it is the wrong way to do it)
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.
pinchartl: <u>mchehab</u>: thank you. I'm sure that Sakari will try not to revert patches if possible
mchehab: I would be a way more comfortable if Sakari would try to do the revert patches at the end
<br> (or not to revert at all, if possible)
<br> anyway, let's move on and solve this issue as soon as we can
<br> <u>sailus</u>: yeah, you can try it with em28xx
<br> or even with some UVC camera
pinchartl: <u>mchehab</u>: 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
mchehab: em28xx is more complex, with that matter - as it needs to unregister on several different cores
sailus: Oh, good point.
mchehab: if you're using em28xx, I suggest you to disable RC
<br> RC unregister is tricky, and you might end to trigger some other race condition
sailus: <u>mchehab</u>: 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.
<br> So I reckoned with was better to start with reverts.
mchehab: (the problem with RC is that it has a kthread that polls the device on every 100ms)
sailus: Let's see how the result will look like with the DVB changes eventually.
<br> 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.
mchehab: DVB has its own release logic inside DVB's kthread
<br> it is very sensitive
<br> the less you touch there, the better
<br> it took *years* to make it reliable
sailus: It's a lot more straightforward to construct object dependencies before the patches limiting the time window during which bad stuff would happen.
mchehab: the kthread is at dvb-frontends.c
<br> dvb_frontend.c
sailus: I hope I won't need to. :-)
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
<br> it is currently based on semaphores
sailus: Uh-oh.
mchehab: and contains other black magic, like: set_freezable();
<br> and mb();
<br> fyi, that's the pending patch: https://patchwork.linuxtv.org/patch/27914/
<br> 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
<br> that's why I'm keeping this old patch queued
<br> I need some stress testing tool and time to setup some testbenchs to be sure about that
<br> on one of the bugs related to it, it was mentioned that the issue happened only on ARM
pinchartl: <u>mchehab</u>: I haven't read the patch, but if you need such black magic, isn't it a sign that something is wrong ?
mchehab: <u>pinchartl</u>: it is a sign that the thread there is complex
<br> on DVB, when you open a /dev/dvb/adapterX/frontendY, it starts a kthread
<br> (if a kthread is not running)
<br> when all frontends are closed, or when the device is removed/unbound, the kthread stops
<br> it also stops before suspend
<br> and restarted at resume
<br> such thread polls the driver on every X seconds
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)
mchehab: I wouldn't have it design the way it is
<br> on the bright side, at DVB, the drivers don't need to implement one callback per ioctl
<br> nor need a compliance tool, as the core itself implements all it is needed to support the userspace API
<br> so, it is a different design that the one we use at V4L2
pinchartl: "no need for a test tool" is never right :-)
<br> even if "everything" is implemented in the core
mchehab: yeah, I agree
<br> I meant to say that it is not as important as for V4L2
<br> 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
pinchartl: I wonder what they would say about V4L3
mchehab: as you may remember, in the past, every V4L2 driver needed to implement itself the V4L2 ioctl parser
<br> even after our implementation of v4l2 framework, still most a lot of userspace API implementation actually happens at driver's level
<br> s/still most a lot of/still a lot of/
hverkuil: <u>mchehab</u>: 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.
mchehab: <u>hverkuil</u>: 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
hverkuil: <u>mchehab</u>: 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.
<br> The only reason we still allow it is for unconverted old drivers (and uvc which is very difficult to convert to the control framework).
pinchartl: <u>hverkuil</u>: 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
<br> thats in my list of APIs we have to rework
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.
<br> the subdev framesizes/intervals ioctls are particularly, um, let's say 'unfortunate'. :-)
pinchartl: yes they are, very much
<br> we need to get mentally prepared for breaking backward compatibility at some point
<br> we've accumulated too much crap :-/
hverkuil: <u>snawrocki</u>: can you take a quick look at this one: https://patchwork.linuxtv.org/patch/42946/
mchehab: breaking backward compat is always painful
<br> from V4L1->V4L2, we took ~20 years
<br> 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...
pinchartl: <u>mchehab</u>: V4L2 was introduced in v2.4.0, in 2002
<br> and we deprecated V4L in 2006
mchehab: ops, sorry, I meant ~10 years
<br> I guess we only got rid of the last V4L1 driver by 2010 or so
pinchartl: but for the future, we will likely be able to implement some level of backward compatibility, but 100% coverage won't be possible
mchehab: (or V4L1 api call from the core)
<br> if we don't give 100% coverage, Linus won't likely allow pushing a new API
<br> at least for some time, we need to provide 100% coverage
hverkuil: <u>mchehab</u>: when do you plan to process pull requests?
mchehab: as we did with V4L1
<br> <u>hverkuil</u>: this week, I'll probably be able to handle only on Friday
<br> been busy preparing for a trip this week (starting tomorrow night, I guess - still waiting for the air tickets)
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.
pinchartl: <u>mchehab</u>: Linus hasn't rejected V4L2 or KMS, even though they didn't provide backward-compatibility with V4L1 and FBDEV
mchehab: <u>pinchartl</u>: we provided V4L1 backward compatibility when V4L2 was added
hverkuil: Linus either didn't know or didn't care, or most likely both :-)
<br> that backward code never quite worked 100% as I remember.
mchehab: <u>hverkuil</u>: once he gave V4L1->V4L2 example as something he didn't want anymore
<br> <u>hverkuil</u>: yes, but most drivers had both V4L1 and V4L2 ioctls
<br> the big problem we had on V4L1 backward compatible is with bttv
pinchartl: I'm fine calling it something else than V4L3 and moving it to a separate directory if it makes him happy :)
mchehab: on V4L1, it used to accept start streaming then resizing the buffers
hverkuil: We've broken drivers before in the past, usually removing custom ioctls or other weird stuff.
pinchartl: but the transition will be similar to the FBDEV to KMS transition
<br> and I don't expect most drivers to be ported
mchehab: <u>pinchartl</u>: before starting a V4L3 (or whatever you want to call it), we need *a lot* of discussions
<br> and a proposal for it :-)
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.
mchehab: yes
<br> I suspect that a transition like that nowadays won't be as painful
<br> as most drivers use the core
<br> s/core/framework/
pinchartl: <u>mchehab</u>: I want to discuss that in Prague
<br> or, rather, gather a list of issues
mchehab: <u>pinchartl</u>: OK
<br> I think that we should do it in a way that all drivers could be converted
pinchartl: and I expect we'll need at least half a day just to discuss issues, without even discussing solutions
<br> let's first gather issues, use cases and requirements
<br> and then we'll see what we can do
mchehab: ok
zrafa: <u>pinchartl</u>: hello
<br> <u>pinchartl</u>: thanks for the advices yesterday, to start to debug my ov2680 sensor/camera in my atom isp / mipi
<br> <u>pinchartl</u>: I do not see atomisp module like you suggested
<br> (at menuconfig)
<br> <u>pinchartl</u>: I see staging drivers -> media staging drivers -> enable support for intel mipi camera drivers -> ov2680
<br> I also see, cheking the linux kernel source files:
<br> some atomisp2 folders
<br> In .config I also see:
<br> CONFIG_STAGING_MEDIA=Y
<br> CONFIG_INTEL_ATOMISP=y
<br> CONFIG_VIDEO_OV2680=m
<br> So I have the ov2680 modules. But what about atomisp? Is that option selected automatically? (I did not find it at menuconfig)
<br> So I do not know if I can build atomisp as module
python476: hi there
<br> 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
h3lios: hi
<br> I have a question about building linuxtv
<br> I'm following the basic instructions from https://www.linuxtv.org/wiki/index.php/How_to_Obtain,_Build_and_Install_V4L-DVB_Device_Drivers
<br> I have a binary kernel without a uvcvideo.ko and tried to build it for armhf
<br> however, when I run ./build it doesn't build any modules
<br> 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
<br> but it seems to be ignored and taken from /lib/modules/<version>/build/config
<br> I tried adding it there as well (also tried /boot/config) but it seems to be ignored everywhere
<br> 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
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
<br> i.e. .i2c_addresses[ADV7604_PAGE_VDP] = 0x24, is not available in my settings guide.
<br> Can I just remove them?
***: thiblahute has quit IRC (*.net *.split)
<br> bengan has quit IRC (*.net *.split)
<br> orwell.freenode.net sets mode: +o ChanServ