| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| hverkuil | mchehab: sailus: reviews open.rst patch.
s/reviews/reviewed/ | [09:03] |
| .... (idle for 18mn) | ||
| sailus | hverkuil: We weem to have different kind of comments.
Few conflicts I presume. :-) | [09:23] |
| hverkuil | the one we have in common is the overuse of the word 'device'. Which is the most important comment I had, actually. | [09:25] |
| sailus | Perhaps it'd be useful to discuss the naming (i.e. how MC-centric and non-MC-centric) here.
I think we had slightly different suggestions on that as well. | [09:31] |
| mchehab | hverkuil, sailus: just sent a patch series with the proposed changes
they cover your comments. | [09:40] |
| sailus | Nice! | [09:41] |
| mchehab | I also added two patches adding a way for userspace to detect the kind of control | [09:41] |
| sailus | For some reason the first patch hasn't reached me yet. | [09:43] |
| mchehab | syoung: please take a look at the au0828 RC_CORE fix that Arnd submitted. I suspect that we'll need to review several other drivers for the same issue | [09:43] |
| sailus | I think it may be to some extent a matter of taste, but I would prefer telling a device node is a part of MC-centric device (rather than V4L2).
Or was there a reason why you picked VDEV_CENTERED instead? | [09:44] |
| hverkuil | just made the same comment on the ML. | [09:44] |
| mchehab | there is a reason
I'll answer at the ML, replying to hverkuil's comments :-) | [09:45] |
| sailus | I'm waiting to see the e-mail...
Wouldn't it be faster to have the discussion here, now that we're all online? | [09:46] |
| pinchartl | sailus: let me read the e-mails first | [09:48] |
| sailus | I'm still waiting to receive it. | [09:49] |
| hverkuil | email from Brazil seems slow today.
General note: we should have a bit more time to think about the new capability (even if we all agree). It's a small change that can also be applied after rc1 if we want to. | [09:55] |
| sailus | Or for the next kernel version for that matter. | [09:57] |
| hverkuil | Yes. | [09:57] |
| pinchartl | hverkuil: agreed. this must not be rushed. there's no urgency | [10:06] |
| mchehab | hverkuil: yes, agreed
IMHO, the first patch would be OK to be merged for 4.14, but we shouldn't rush with the new caps just sent the e-mail | [10:07] |
| .... (idle for 16mn) | ||
| sailus | mchehab, hverkuil: I prefer CAP_MC_CENTRIC, on the same grounds.
Beyond that, I understand there are few computers you'd use libv4l currently on a MC-centric device. So it's more important it works well with V4L2-centric drivers. What do you think of calling the non-MC-centric devices as "V4L2-centric" in this case? After all, the Media controller device node is a device node among others. That's actually in the latest version. I'll review the patch. | [10:24] |
| hverkuil | I lean towards V4L2-centric as well, since it is the V4L2 API that controls the hardware, whereas for MC-centric you start with the MC API. | [10:35] |
| mchehab | if you prefer V4L2-centric instead of VDEV-centric, fine for me... still subdevs are part of the V4L2 part at the documentation
that's why I prefer vdev-centric but I can live with V4L2-centric - except that typing it is worse :-) | [10:38] |
| sailus | Radio and SDR are part of V4L2, too.
I understand it was meant to be included. Sub-devices indeed shouldn't be referred to here. | [10:39] |
| mchehab | sailus: yes, but "vdev" in the sense of a shortcut for "V4L2 device node" also covers radio and SDR
with regards with CAP MC_CENTRIC x VDEV_CENTRIC, see my e-mail | [10:40] |
| sailus | Then if we used that, we would need to define what "vdev" precisely mean. My understanding of it was different, and I'm not entirely certain it has a well established meaning among e.g. user space developers. | [10:42] |
| mchehab | MC_CENTRIC prevents libv4l to provide support for mc-centric devices to be controlled by userspace apps if the Kernel is lower than, let's say, 4.15 (when I expect such feature to be aded)
s/aded/added/ sailus: it was defined as such on the latest patch. take a look | [10:42] |
| sailus | Ok. I then missed it. Let me take a look...
mchehab: There are few such systems around in any case. | [10:43] |
| mchehab | > +this kind of control as **V4L2 device node centric** (or, simply,
> +**vdev-centric**). > + | [10:43] |
| sailus | You'd need to upgrade libv4l (plus install yet-to-be-developed library) and _not_ upgrade the kernel. | [10:43] |
| mchehab | yes, but apps will need to check for the Kernel version
to be sure that MC_CENTRIC has meaning (or VDEV) libv4l can rise the flag but making it mask the API version is a way more complex and can cause random troubles see the comments on my email | [10:44] |
| sailus | V4L2 device node --- does that not include sub-devices?
In case it includes radio and SDR? I think we'd need to be more specific about it in any case. | [10:46] |
| mchehab | sailus: feel free to propose a better wording :-) | [10:47] |
| sailus | I'll try to. :-) | [10:48] |
| mchehab | it seems that I'm losing the battle between CAP MC_CENTRIC x VDEV_CENTRIC. Ok, I can live with MC_CENTRIC cap. I'll wait for more comments before doing such change
MC_CENTRIC patch will actually require to also patch staging and all pending platform drivers more work for submaintainers to check it :-p on a side note: all drivers under platform (except for vivid) are mc-centric? | [10:52] |
| sailus | I don't think they all are.
The omap display driver isn't, it seems. | [10:56] |
| mchehab | the soc camera driver wasn't... so the drivers that derivated from it probably aren't as well | [10:59] |
| hverkuil | mchehab: certainly not. Lots of codec drivers there, all v4l2-centric.
Also several simple camera drivers that are v4l2-centric. | [10:59] |
| mchehab | IMHO, the advantage of a VDEV_CENTRIC is that it whitelists the drivers that are vdev centric
while MC_CENTRIC is a blacklist, with is usually harder to maintain | [11:00] |
| sailus | :-)
You're making it sound like that MC-centric drivers are somehow bad. | [11:00] |
| mchehab | It is usually easier to remember that a new feature should be enabled than the reverse
sailus: that was not what I meant | [11:01] |
| sailus | I know. ;-) | [11:01] |
| mchehab | but the hole goal of such flag is to indicate the drivers that are OK to be used by a generic app | [11:01] |
| hverkuil | "that are not OK to be used" :-) | [11:02] |
| mchehab | a MC-centric flag actually does the reverse - it is a blacklist
that would require extra care from maintainers as I doubt that driver developers will remember to blacklist their on drivers s/on/own/ | [11:02] |
| hverkuil | I don't get this. It's a capability, nothing 'whitelist' or 'blacklist' about it.
There are far fewer MC drivers than v4l2 centric drivers, so it is actually much easier to check for maintainers. | [11:04] |
| mchehab | hverkuil: MC-centric is actually a lack of capability
it lacks the capability of being used by a generic app | [11:05] |
| sailus | mchehab: Now you'd definitely making MC-centric drivers sound bad. :-) | [11:05] |
| hverkuil | No, they are just different. | [11:05] |
| mchehab | such capability can be added later (via either kernel or library support)
sailus: it is bad that a mc-centric driver can't be used by all applications :-D | [11:05] |
| hverkuil | Anyway, this will be one of the first things that a media-compliance test will check, so I am not worried about this. | [11:06] |
| mchehab | except that we don't have a media-compliance test check right now | [11:08] |
| sailus | Replied. | [11:08] |
| mchehab | we should likely add it to v4l-compliance while we don't have an specific compliance tool for MC-centric | [11:09] |
| hverkuil | mchehab: hopefully we will have one soon (even if it only check for this capabilility initially!)
capability that's actually a very nice starting point for such a utility. | [11:09] |
| mchehab | I'll rewrite patch 2 to document a MC-centric caps flag. I suspect sailus won't be too happy with the description for the flag...
as it will need to tell that a device with a MC_CENTRIC flag is a device that can't be controlled by a vdev-centric application | [11:16] |
| hverkuil | I'm OK with that. It's the truth, after all.
That's why we call it MC-centric. | [11:17] |
| mchehab | * - ``V4L2_MC_CENTRIC``
- 0x20000000 - Indicates that the device only provides **mc-centric** hardware control, and can't be used by **v4l2-centric** applications. | [11:20] |
| hverkuil | Fine by me. But it needs a reference to the section explaing the difference. | [11:21] |
| mchehab | yes, I'm adding it too | [11:21] |
| hverkuil | And vice versa, actually. | [11:21] |
| mchehab | what do you mean by vice versa?
it is not vice versa a mc-centric driver + a libv4l plugin will provide both mc-centric and v4l2-centric control | [11:21] |
| hverkuil | The section about hardware control types should have a reference to this capability. | [11:22] |
| mchehab | ah, it has already
see patch 2 | [11:22] |
| hverkuil | Ah, missed that. Never mind then :-) | [11:23] |
| mchehab | the new version of the note should be something like:
Devices that are not capable of **vdev-centric** control should report a V4L2_MC_CENTRIC :c:type:`v4l2_capability` flag (see :ref:`VIDIOC_QUERYCAP`). | [11:24] |
| hverkuil | Just say: Devices that require **mc-centric** hardware controls should...
control | [11:25] |
| mchehab | just sent the patch
"require" is an interesting word | [11:31] |
| just sent a new version with "require". | [11:37] | |
| ................ (idle for 1h16mn) | ||
| hverkuil, sailus, pinchartl: just sent a new version of it. I guess this fulfill all of your comments. I opted to keep calling it as vdev-centric, as it doesn't require to play with <shift> <unshift> while typing it
V4L2-centric requires a lot more keys to be typed - I'm a lazy bastard :-p | [12:53] | |
| ....... (idle for 34mn) | ||
| pinchartl | mchehab: I've reviewed 1/3 | [13:28] |
| mchehab | thanks
looking at your comments right now | [13:28] |
| pinchartl | get an azerty keyboard, numbers require shift ;-) | [13:28] |
| mchehab | :-) | [13:28] |
| pinchartl | my main concern is that I think we should define the terms we want to use. "device" is very vague | [13:29] |
| mchehab | I guess version 2 of the patchset solves this issue | [13:29] |
| pinchartl | once we agree on definitions I'll propose enhancements for the documentation itself | [13:29] |
| mchehab | basically, it uses either "V4L2 device node" or "hardware"
and V4L2 device node is now properly described on the next section | [13:29] |
| pinchartl | I still think we need an explicit agreement on a glossary
not just for that small section of the documentation, but for the V4L2 specification in general | [13:29] |
| mchehab | the big question is: who will wrote a glossary and review the entire document to fix it to match the glossary?
s/wrote/write/ | [13:30] |
| pinchartl | (obviously I don't expect you to go through the whole spec and use the right terms everywhere :-))
let's agree on the terms we need first a glossary is easy to write going through the whole specification will take more time and doesn't have to be done right now | [13:30] |
| mchehab | mchehab doesn't thing it is easy to write a glossary
s/thing/think | [13:31] |
| pinchartl | adding a section to our documentation with 5 terms won't be difficult
we should have done that from the very beginning, but we haven't so let's start here, with just the terms we use in this patch and we can then add new terms to the glossary as more work is done on the documentation I'm completely fine with an incremental approach here | [13:32] |
| mchehab | OK, that I can do | [13:33] |
| pinchartl | feel free to comment on my e-mail
I'd like Hans and Sakari to reply too | [13:33] |
| mchehab | I'm doing it right now ;) | [13:33] |
| pinchartl | and once we agree on the 5 (or was it 6 ?) terms we need, you can send a v2
and if you're very eager to typing documentation patches you can already add the glossary.rst :-) | [13:33] |
| mchehab | answer sent | [13:38] |
| ...... (idle for 26mn) | ||
| hverkuil | FYI: I'm out this evening and likely during the weekend as well, so I won't be able to comment on the open.rst patch(es) until Monday. | [14:04] |
| ........... (idle for 53mn) | ||
| *** | prabhakarlad has left | [14:57] |
| .... (idle for 15mn) | ||
| mchehab | pinchartl: v3 sent, starting with the glossary
please notice that I wrote the glossary to be applied after the series, as it adds cross-references to the new hardware control series but I moved the patch to be the first one, as it is likely easier to review the series with it being the first one | [15:12] |
| *** | benjiG has left | [15:13] |
| ................... (idle for 1h30mn) | ||
| pinchartl | mchehab: ok, no worries | [16:43] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |