Hi, I have added support for the adv7619 .  Based on the docs I managed to add "enough" to the adv7604 driver for initial support
what device is that chip used in?
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
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()
ah.  Thats an idea.
or maybe the register just always reads 0
I think to remember that they changed this
Where are the register maps
the device has multiple I2C addresses
each address for one of the register maps
and the addresses can be configured through the main map
I know the device has 2 addresses 0x98 and 0x9C
But it uses 0x98
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
ok.
and those other addresses need to be configured
That sounds like I need to update the config then.
hmmm.
http://elixir.free-electrons.com/linux/latest/source/drivers/media/i2c/adv7604.c#L3117
this part is only used for devicetree
I am running this on a PCIe on an x86
only ACPI
Cool I was on the right line
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
larsc, sounds right.   Here is what I have  https://hastebin.com/iyenopenoj.cpp
easier then trying to explain it.
yeah, just add the addresses to your pdata
if there are no other i2c devices on the bus you have free choice
So just all the items starting at line 3117  until "Hardcode the remaining platform data fields"
yes
larsc, thanks.
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)
pinchartl: sailus: mchehab: ping
I'm here
1 down, 2 to go...
i'm here
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.
Hello!
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.
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
hverkuil: Could we take a small step back in formulating the problem statement?
(and, btw, parent information can also be retrieved via sysfs - with is what udev actually uses)
mchehab: udev isn't present in all embedded systems
I remember it was related to the v4l2-compliance tool.
but sysfs is present
and the info is actually stored at sysfs
For what it's worth,libmediactl uses udev to look up the device nodes if it's available and if not, it uses sysfs.
I don't think the media device is the parent.
mchehab: /query sailus
oops :-)
mchehab: yes, sysfs is present
hverkuil: it should be easy to add the media device
ops
mchehab: how do you propose doing this through sysfs ?
hverkuil: it should be easy to add the media device's parent to be reported at sysfs, at uevent
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.
hverkuil: I believe there's a better idea
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
mchehab: how ?
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.
through /sys/dev/$major:$minor/subsystem ?
pinchartl: sysfs fills it based on the class of the device
Today these v4l2- utilities check if VIDIOC_QUERYCAP exists. If it does, they assume it is a valid v4l2 node.
mchehab: sysfs fills *what* ?
mchehab: I'm not challenging your idea, I'd like to understand what you propose exactly
Today we have /sys/class/video4linux/videodevnode/device/mediadevnode .
yes...
basically /sys/class/* is generated automatically when a kobject is registered
based on the class of the object
mchehab: we start with a device node, right ?
yes
E.g.: v4l2-ctl -d /dev/v4l-subdev0
at register_device
so what we have is a string that indicates the path of the device node
(or similar)
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 ?
on systems with udev, udev provides it for us via libudev
For reference: today I call VIDIOC_QUERYCAP on the device node. If it works, it's a v4l2 device, otherwise I bail out.
on systems without udev, you'll still have /sys/class/video4linux/
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 ?
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
# cat /sys/dev/char/81:77/uevent
MAJOR=81
MINOR=77
DEVNAME=v4l-subdev49
(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
# cat /sys/dev/char/81:0/uevent
MAJOR=81
MINOR=0
DEVNAME=video0
that actually sounds quite sensible to me
And /sys/class/video4linux/v4l-subdev0/device/mediaX/uevent gives the media info.
if we're unsure that a device node is of the correct type, verifying its type externally seems better than calling an ioctl
hverkuil: we can easily add a rule there that would do something like:
MC=mc0
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
yes
At least, that's the case for vimc.
I don't expect that it will ever in practice
(btw, 'V' is used on other subsystems as well, as far as I remember)
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 ?
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!
pinchartl: does the media info for a v4l-subdev always appear in /sys/class/video4linux/v4l-subdev0/device/mediaX?
(actually, the real conflict is with VT)
hverkuil: as long as the driver sets the right parent of the MC device
Is the entity information available somewhere through sysfs?
I don't see a case where it wouldn't work
If not, should we add that?
entity information isn't, no
pinchartl: If there is a way to verify the node type externally, I think it'd be quite nice.
and I'd rather not add it to sysfs
I mean entity ID, to be precise.
but one thing at a time
for type verification
do we agree that sysfs is fine ?
hverkuil: that was item 1 in your list
I think looking up the type via sysfs will work. I can implement that in v4l-utils.
ok
sysfs is good for this IMO. It's practically available everywhere.
should we move to item 2 then ?
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
(Or better yet, through udev. But still the info originates from sysfs I presume.)
I wasn't aware of /sys/dev/char/<major>:<minor>/uevent
Me neither.
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
mchehab: interesting. Good to know.
you can easily do that to place the MC name
hverkuil: are we good with type verification ?
pinchartl: yes.
(in the case of RC, as this is virtual, there's no major, minor)
I learned something today :-)
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
It's testing, but also for simple quick lookups:
do I recall correctly that's what we agreed on ?
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).
no I don't, but that should be easy to find out
I only learned we had such drivers last week. :-)
rcar_drif
atmel-isc
There were only 3 or 4, I think.
It's likely that atmel-isc was unintended.
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
an easy way to do that would be to do something like:
if (dev->mdev)
    ADD_HOTPLUG_VAR("MC=%s", dev->mdev->name);
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 ?
pinchartl: all video-centric drivers now have media device as well (if Kernel compiled with MC and MC-DVB support)
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.
mchehab: even non-DVB devices such as bt878 ?
hverkuil: that doesn't tell me if my understanding is correct so far :)
mchehab: I'm pretty sure it is limited to DVB devices.
bt878 may have DVB
pinchartl: yes, your understanding is correct.
mchehab: and I assume that DVB devices don't expose subdev nodes to userspace
no
(at least most of them)
neither video-centric ones
those DVB devices are video-centric in my mind, as they're fully controlled through the video device
well, the DVB device :-)
pinchartl: let correct myself: not all video-centric devices have MC... yet
but they don't require MC-based configuration of formats on the pipeline
but that's not preventing them to have... it is just a couple of lines to be added at the driver
mchehab: yes, and I'd like to see all drivers getting a media device at some point, but that's a different issue
pinchartl: all pure DVB drivers have MC, as the feature is inside the DVB core
Right, can we get back to my issue? :-)
hverkuil: trying to :)
I think we're almost there.
hverkuil: so, your issue with v4l2 subdev compliance is related to MC-centric devices
several video-centric V4L2 drivers also support MC
none of them export subdevs
mchehab: one issue at a time!
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
pinchartl: one issue at a time!
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
All I want is to go from a v4l-subdev and/or video device to the media device.
pinchartl: agreed. v4l2-compliance can work better if it is performed under /dev/mc0
because we shouldn't use it in userspace
(just my 5 cents)
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 ? :-)
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.
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:
I mean, sub-device nodes are a pain to find if the first thing you're looking at are the device nodes under /dev.
v4l2-ctl -D -d /dev/v4l-subdevX
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.
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
it would retrieve the MC node from sysfs
they can change at every bood
s/bood/boot/
and dig the device from MC
printing only the things that are specific to the subdev
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
hverkuil: I see your point
I had to change my test script every other boot as the node changed
(that was during development)
pinchartl:  I KNOW!
This is not how applications will use it.
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
hverkuil: what about something like:
v4l2-ctl -D -d /dev/mcX -s Y # If you want v4l2-ctl to only work with subdev Y
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.
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.
I know a lot of applications that are not test programs do this for video devices.
I just want to do 'v4l2-ctl -D -d /dev/v4l-subdevX' and have it return the driver + entity information.
hverkuil: I understand what you want to do, but I don't see why we would need that :-S
I don't see myself any harm doing that
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
There might not be harm in providing that information as such, but it encourages a less-than-good usage pattern for accessing device nodes.
because I believe that userspace should be MC-centric for MC-centric devices
pinchartl, sailus: I can't see how an userspace application/script won't be MC-centric with subdevs...
I truly can't see any reason why userspace would EVER want to use it in that way, it makes no sense whatsoever.
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
mchehab: v4l2-subdev-compliance -d /dev/v4l-subdev31 isn't MC-centric
I'm happy to add all sorts of disclaimers to the code in v4l2-ctl and v4l2-compliance for this.
v4l2-subdev-compliance -d /dev/media0 -Y "subdev name" is MC-centric
hverkuil: Could we make the test programs exemplary users instead?
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 ?
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.
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
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.
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
hverkuil: It'd be practical for a single tool to test compliancy of the related interfaces as well.
one reason is that subdev nodes are always part of an MC-centric device
In other words, why would you, by default, test only a sub-set of interfaces a device provides?
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
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.
hverkuil: that's true, but, still, it is a way easier for us if one would just run:
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
pinchartl: Yes, makes sense. This would be the ultimate goal indeed.
v4l-mc-compliance /dev/mc0
than asking to run a v4l-mc-subdev-compliance for every /dev/subdev*
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
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.
you don't have to perform complex pipeline setup, or handle everything
sailus: agreed
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
and the full test is what we really want when someone submits us a new driver
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
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)
mchehab: that's easy, the MC graph reports major:minor for subdev nodes
and they're 0:0 if there's no subdev node
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. :-(
(or possibly -1:-1, I don't remember the details)
It seems I'm going to lose this battle, though.
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
ack
and I'm certainly open to add this to the list
but I'd like to add that feature only if it's useful
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?
I'm not entirely against features that have very few users, at least not all the time
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.
but when they make it possible for userspace to easily do things wrong, and don't bring much benefits, I'm even more reluctant
At least it can be used to set/get controls and EDIDs from the command line for a v4l-subdev.
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
Actually, I hope a colleague can work on this, not me.
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
It's why I wanted this discussion since he's now back from vacation.
or naming it directly v4l2-mc-compliance, even if it tests subdevs only
I would break v4l2-compliance into a sort of library
I don't think we should bikeshed naming now though :-)
but keep just one command line for both cases
Yeah, yeah, one thing at a time.
sudh command line should be smart enough to select the MC-centric library or the video-centric library
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
pinchartl: what do you mean by "standard functional"?
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
(standard for me is v4l2_std :-) )
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
stress testing is something that we need
not sure it should be at the same tool, though
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
I guess the code there for v4l2-ctl is more likely to be shared for stress testing
mchehab: possibly, yes
but in any case
I'd like to keep this in mind when developing test tools
sure
but I certainly don't want to create internal libraries with a single user
otherwise it's guaranteed that the API will be wrong
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.
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 :-)
pinchartl: that's good news
Ah, that changes things. I didn't know.
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
pinchartl: one think you could add there is binding/unbinding devices... that's a good stress test for register/unregister logic
OK, can I summarize?
and if we all agree, then we can close this meeting.
mchehab: there's a large patch series from Sakari that we need to merge first...
hverkuil: please go ahead
pinchartl: Which one of them? :-)
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.
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
Since my colleague is new to all this, I think this is a good starting point.
hverkuil: Looks good to me.
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
pinchartl, mchehab: Do you mean the media device referencing patchset?
hverkuil: an additional bonus is if your colleague could break it into modules that could later be used for a MC compliance tool
mchehab: the last time we discussed it there was no agreement on the 3 reverts that started the patch series
sailus: that was my understanding
mchehab: and yes, DVB drivers need to be handled as part of that patch series, it's currently missing
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.
OK, thank you all for your input!
hverkuil: you're welcome
It's been a while, but they seemed to need reference counting for the node data structures as well.
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
pinchartl: the patches applied fixed a serious regression that were preventing removing USB devices
whatever solution should not break USB device removal
they fixed a regression introduced in other patches you wrote :-)
and they introduced other problems
they moved the race window
mchehab: I think it's fair to say this was very badly broken to begin with; it wasn't regression.
made it smaller in some cases
but they were not correct
sailus: MC was ever broken for device's removal
Yes.
mchehab: correct
we all agree on that
but USB drivers got broken when MC support was added
yes
the patches fixed that
they fixed part of the problem
hotunplug is still broken
but with a smaller race window
you can safely remove/reinsert USB devices without breaking
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
pinchartl: on my race regression tests, it was safe... tried bind/unbind 10.000 times on several devices, running multiple threads
mchehab: as I said, the race window is now small :-)
I don't remember a single bug report related to it either
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
the mc-centric devices were already broken, since ever
to close it completely we have to open it again fist
*first
yes, they were broken, we all agree on that
well, let's fix it
Sakari has submitted an RFC series for that
it needs more work
I'm all for fixing it anyway that works and won't cause regressions to USB drivers
and I believe he plans to work on it
but closing the race window completely now requires opening it back
I don't care much for whatever solution is taken, provided that it won't introduce regressions on USB drivers
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
pinchartl: I would prefer the "reverse" approach...
I mean, place the revert patches at the end
mchehab: it's very difficult
I suspect it is doable, and won't cause regressions
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
(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.)
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
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
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/
Without a delay it might be difficult, but not impossible.
sailus: you should try this on an USB driver
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
I suspect that, on USB drivers, you won't be able to reproduce it
From kernel PoV it's not different, in both cases the hardware device disappears while device nodes are still open.
sailus: no, the USB core takes care of it
How? These device nodes are created and removed by the driver.
via the callback that it is called when the hardware is removed
I can try to do the same with a USB device, too.
I have one, I think it was em28xx or such.
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)
I'll try to find the time for making the required DVB changes but I can't say now when those could be ready.
mchehab: thank you. I'm sure that Sakari will try not to revert patches if possible
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
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
em28xx is more complex, with that matter - as it needs to unregister on several different cores
Oh, good point.
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
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.
(the problem with RC is that it has a kthread that polls the device on every 100ms)
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.
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
It's a lot more straightforward to construct object dependencies before the patches limiting the time window during which bad stuff would happen.
the kthread is at dvb-frontends.c
dvb_frontend.c
I hope I won't need to. :-)
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
Uh-oh.
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
mchehab: I haven't read the patch, but if you need such black magic, isn't it a sign that something is wrong ?
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
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)
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
"no need for a test tool" is never right :-)
even if "everything" is implemented in the core
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
I wonder what they would say about V4L3
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/
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.
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
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).
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
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'. :-)
yes they are, very much
we need to get mentally prepared for breaking backward compatibility at some point
we've accumulated too much crap :-/
snawrocki: can you take a quick look at this one: https://patchwork.linuxtv.org/patch/42946/
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...
mchehab: V4L2 was introduced in v2.4.0, in 2002
and we deprecated V4L in 2006
ops, sorry, I meant ~10 years
I guess we only got rid of the last V4L1 driver by 2010 or so
but for the future, we will likely be able to implement some level of backward compatibility, but 100% coverage won't be possible
(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
mchehab: when do you plan to process pull requests?
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)
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.
mchehab: Linus hasn't rejected V4L2 or KMS, even though they didn't provide backward-compatibility with V4L1 and FBDEV
pinchartl: we provided V4L1 backward compatibility when V4L2 was added
Linus either didn't know or didn't care, or most likely both :-)
that backward code never quite worked 100% as I remember.
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
I'm fine calling it something else than V4L3 and moving it to a separate directory if it makes him happy :)
on V4L1, it used to accept start streaming then resizing the buffers
We've broken drivers before in the past, usually removing custom ioctls or other weird stuff.
but the transition will be similar to the FBDEV to KMS transition
and I don't expect most drivers to be ported
pinchartl: before starting a V4L3 (or whatever you want to call it), we need *a lot* of discussions
and a proposal for it :-)
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.
yes
I suspect that a transition like that nowadays won't be as painful
as most drivers use the core
s/core/framework/
mchehab: I want to discuss that in Prague
or, rather, gather a list of issues
pinchartl: OK
I think that we should do it in a way that all drivers could be converted
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
ok
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:
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
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
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
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?