| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | macmaN_ has quit IRC (Read error: Connection reset by peer) | [08:45] |
| ...... (idle for 25mn) | ||
| tfiga | pinchartl: Hey :) | [09:10] |
| .... (idle for 18mn) | ||
| pinchartl | tfiga: hi ! | [09:28] |
| tfiga | pinchartl: How are you doing? Very busy I guess ;) | [09:28] |
| pinchartl | I've more or less recovered from my holidays
people have a tendency to keep me busy, yes and I have a feeling that you won't be an exception ;-) regarding requests, I've resumed working on them today (and I've already made a pause for lunch, but will continue in the afternoon ;-)) | [09:33] |
| tfiga | pinchartl: I think I might be the complete opposite ;)
I wouldn't mind offloading this work | [09:39] |
| pinchartl | :-) | [09:39] |
| tfiga | and we actually have one more person that could do this | [09:39] |
| pinchartl | I was actually thinking about that | [09:39] |
| tfiga | and, by the way, I think you even know this person | [09:39] |
| pinchartl | I think I do :-)
information spread very very fast, I've heard about it even before that person told me they joined Google :-) I'd like to post my work in progress, then we should discuss how to collaborate if that's fine with you | [09:40] |
| hverkuil | I probably should be involved in that discussion? Since the main missing piece AFAIK is the support for controls in the request API, right? | [09:42] |
| pinchartl | hverkuil: sure, I'd like you to be involved | [09:43] |
| tfiga | pinchartl: yeah, that's what I had in my mind | [09:43] |
| pinchartl | tfiga: while you're here, I was wondering whether your planned to attend LinuxCon Japan this year
if you are, we should have dinner and you're not, we should still have dinner :-) with Pawel and this new person you've been talking about, if they're available (by the way is that official already, or should his name be kept secret until an official announcement ?) | [09:44] |
| tfiga | pinchartl: I'm still wondering
it looks like the conference has become too much container-oriented actually it's not even LinuxCon | [09:51] |
| pinchartl | gnurou: hi. what a surprise :-)
yes, it's OSS now | [09:51] |
| gnurou | pinchartl: is it? :) | [09:51] |
| pinchartl | I'm only attending it as an excuse to go to Japan ;-) | [09:51] |
| tfiga | anyone here going to attend it? hverkuil, mchehab?
sailus? :) | [09:51] |
| hverkuil | tfiga: no, I stopped attending LinuxCon/OSS/whatever conferences altogether. It stopped being interesting several years ago. | [09:52] |
| pinchartl | sailus: I think you should attend OSS Japan :-) | [09:54] |
| hverkuil | ELC(E) are the only conferences I go to (and Kernel Recipes if I can present). | [09:54] |
| gnurou | anyone attending Linuxcon Japan gets a chance to see Marex
's amazing performance I am not telling more - let the hype build | [09:55] |
| larsc | Is he going to dance?
dressed up as tux? while juggling a bunch of boards? | [09:56] |
| snawrocki | oh, it's becoming interesting :) | [09:57] |
| pinchartl | gnurou: you're good :-) | [09:58] |
| gnurou | be quick! I'm not sure how many places are remaining | [09:59] |
| tfiga | oh, snawrocki, how are you doing? :) | [09:59] |
| snawrocki | tfiga: hi, it's Monday, let's leave difficult questions for tomorrow ;) | [10:06] |
| tfiga | snawrocki: :) | [10:08] |
| snawrocki | tfiga: I don't think I'm going to OSS Japan but perhaps you're planning a trip to PL or Europe in near future? :) | [10:10] |
| tfiga | snawrocki: yeah, I might be comming some time around summer | [10:10] |
| ........... (idle for 51mn) | ||
| pinchartl | mchehab: patchwork notified me that you've accepted the uvcvideo pull request
I'm not holding my breath for the VSP one :-) s/not/now/ | [11:01] |
| ...... (idle for 27mn) | ||
| hverkuil | mchehab: posted "[GIT PULL FOR v4.12] ov2640: move out of soc-camera (rebased)" for the remainder of the ov2640 patches. | [11:29] |
| pinchartl | hverkuil: don't distract Mauro from my pull request :-D | [11:31] |
| ........ (idle for 36mn) | ||
| mchehab | pinchartl: partially merged VSP series
but I think we need an extra flag for controls that modify the buffer layout sent you an email about that bbiab | [12:07] |
| ............ (idle for 55mn) | ||
| sailus | hverkuil: Ping? | [13:05] |
| hverkuil | sailus: pong | [13:13] |
| sailus | hverkuil: How are you doing? | [13:16] |
| hverkuil | good, looking forward to Easter vacation :-)
and you? | [13:16] |
| sailus | I'm doing fine.
Is it the Good Friday and the second Easter day in Norway as well? | [13:17] |
| hverkuil | Yes, and Thursday too. And the week after I've taken off as well. | [13:18] |
| sailus | Oh, that's generous indeed. :-) | [13:18] |
| hverkuil | Most take this week off, so it is very quiet at work. | [13:18] |
| sailus | That helps concentration, too.
I'd like to ask a little bit of your help actually. | [13:18] |
| hverkuil | sure | [13:19] |
| sailus | I'm replacing the V4L2 OF framework by a more generic V4L2 fwnode framework that also supports ACPI.
That patchset also converts existing drivers using V4L2 OF framework to V4L2 fwnode framework and removes the V4L2 fwnode framework. I have tested Nokia N9. Albeit the changes are mostly mechanical and no-brainers, it'd be good to have some additional testing coverage. Then I remembered that you had just converted atmel-isi to OF. Would you happen to be able to test the hardware with some new patches? :-) They're here: https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi-merge | [13:19] |
| hverkuil | Next week. One disadvantage of going back and forth between the Netherlands and Norway is that the hardware is always in the wrong country :-) | [13:22] |
| sailus | Ack. | [13:22] |
| hverkuil | I'm going back to the Netherlands Wednesday next week, so I can test on Thursday. | [13:22] |
| sailus | That'd be great! | [13:23] |
| hverkuil | I've made a reminder for myself. | [13:23] |
| sailus | pinchartl has reviewed them and I've addressed his comments, plus converted atmel-isi which you moved to use V4L2 OF framework. | [13:23] |
| pinchartl | hverkuil: what do you think of Mauro's proposal to add a V4L2_CTRL_FLAG_MODIFY_BUF_LAYOUT control flag ?
sailus: ^^ mchehab: if I fix that, can we get the rest of the series merged this week ? | [13:25] |
| sailus | hverkuil: Thank you!
pinchartl: What's the subject on linux-media? | [13:26] |
| hverkuil | pinchartl: I think it is a good idea. I'm not sure about the name of the flag, though.
MODIFY_FORMAT might be better. MODIFIES_FORMAT? | [13:28] |
| mchehab | pinchartl: provided that the remaining patches won't have other surprises, I guess so ;) | [13:30] |
| pinchartl | sailus: "Re: [PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers" | [13:31] |
| hverkuil | V4L2_CTRL_FLAG_MODIFIES_FORMAT works well for both video and v4l-subdev devices. | [13:31] |
| pinchartl | mchehab: could you already have a look at the remaining patches ? if you review one patch per pull request it will take a looooooong time | [13:32] |
| hverkuil | BUF_LAYOUT doesn't really apply to subdevs since those usually only deal with mediabus formats. | [13:32] |
| mchehab | pinchartl: I applied the patches before that one already | [13:32] |
| pinchartl | hverkuil: I'm fine with that name
mchehab: how about the patches after it ? :-) | [13:32] |
| mchehab | but I didn't review the remaining patches at the series | [13:32] |
| pinchartl | mchehab: let's assume we'll add such a flag
could you check if there are other issues ? | [13:33] |
| mchehab | sorry, got sidetracked by other things...
let's first decide with regards to ctrl name not sure about MODIFIES_FORMAT... I don't want to have controls that really modifies the format (doing things like, for example, changing from bayer to RGB) as we have already an API for that | [13:35] |
| pinchartl | controls should not modify the pixel format, I think I agree with that | [13:36] |
| hverkuil | You do: the HFLIP control (not sure about the VFLIP control) can change the Bayer pixelformat | [13:37] |
| pinchartl | and as far as I know, the proposed documentation patch doesn't imply that a control can modify formats
hverkuil: the sensors drivers I wrote adjust the location of the crop rectangle to keep the format unchanged if I recall correctly | [13:37] |
| hverkuil | MODIFIES_SIZE? | [13:38] |
| pinchartl | it's not the size either, it's really the buffer layout | [13:38] |
| sailus | I like the idea of having such a control flag. | [13:40] |
| hverkuil | MODIFIES_LAYOUT? (this works better with subdevs) | [13:40] |
| sailus | V4L2_CTRL_FLAG_BUFFER_LAYOUT? | [13:40] |
| pinchartl | (the layout can influence the size though, but it doesn't have to)
V4L2_CTRL_FLAG_DATA_LAYOUT ? that way we don't mention "buffer", so it's applicable to subdevs | [13:40] |
| sailus | pinchartl: What terms do you use to refer to this in documentation? | [13:41] |
| mchehab | works for me | [13:42] |
| hverkuil | that define doesn't give any indication of the meaning. | [13:42] |
| mchehab | the meaning itself should be written at the docs | [13:43] |
| hverkuil | You need something like MODIFY or MODIFIES in the name, because the fact that the layout is modified is what the flag is about. | [13:43] |
| pinchartl | sailus: the documentation uses "buffer layout" as it focusses on video nodes | [13:43] |
| sailus | pinchartl: Fair enough. | [13:43] |
| pinchartl | V4L2_CTRL_FLAG_MODIFY_DATA_LAYOUT ? | [13:43] |
| mchehab | ok | [13:43] |
| hverkuil | That or just MODIFY_LAYOUT | [13:43] |
| mchehab | also ok | [13:43] |
| sailus | hverkuil: We also have the "UPDATE" flag.
The macro name doesn't really tell the full meaning of the flag as such. | [13:44] |
| mchehab | update just means that other controls should be read
A hint that changing this control may affect the value of other controls within the same control class. Applications should update their user interface accordingly. (from the documentation) | [13:44] |
| pinchartl | I'm fine with V4L2_CTRL_FLAG_MODIFY_LAYOUT too. any preference ?
it's a bit shorter, which is always nice | [13:44] |
| sailus | What I want to say is that you may need to read the documentation to know what the UPDATE flag signals. | [13:45] |
| mchehab | if both work, i would prefer the shorter one | [13:45] |
| hverkuil | ditto | [13:45] |
| sailus | V4L2_CTRL_FLAG_MODIFY_DATA_LAYOUT is getting rather long as well. | [13:45] |
| pinchartl | let's go for the shorter one then | [13:45] |
| sailus | I also prefer V4L2_CTRL_FLAG_MODIFY_LAYOUT (or V4L2_CTRL_FLAG_DATA_LAYOUT). | [13:45] |
| mchehab | ok | [13:45] |
| sailus | Perhaps leaning towards V4L2_CTRL_FLAG_MODIFY_LAYOUT. | [13:46] |
| pinchartl | I'll update the patch to add that control. if preferred I can add the control in a separate patch, but I think it belongs to the documentation patch
as the flag is needed to explain how controls and buffers interact and a patch adding the flag alone wouldn't tell much about how it works | [13:46] |
| mchehab | pinchartl: if you can write such patch and submit today with the remaining patches on the series, I should very likely be able to review it today | [13:46] |
| pinchartl | is that ok ?
mchehab: or would you prefer a separate patch ? | [13:46] |
| mchehab | yes
yes = together with documentation patch no need for a separate patch | [13:47] |
| pinchartl | ok. I'll thus submit a new version of the documentation patch
I'll add support for the flag to the control framework in a separate patch | [13:47] |
| mchehab | ok. I guess you'll also need to change the patches on your series adding support for rotation...
and patch other drivers that use rotation and change buffer layout (not sure how many, if any) such change should be on a separate patch | [13:48] |
| pinchartl | I'll modify the other patches in the series, yes | [13:49] |
| mchehab | $ git grep -l CID_ROTATE drivers/media/
drivers/media/platform/exynos-gsc/gsc-core.c drivers/media/platform/exynos4-is/fimc-core.c drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c drivers/media/platform/omap/omap_vout.c | [13:49] |
| pinchartl | I'll add the flag to existing drivers separately though | [13:49] |
| mchehab | yeah, sure | [13:49] |
| pinchartl | as they need to be carefully reviewed
and possibly tested I don't want to block the VSP series on that if possible | [13:49] |
| mchehab | but we should ensure that this API change will all hapen on a single Kernel version | [13:49] |
| pinchartl | is that an issue if documentation is added in v4.12 and drivers updated in v4.13 ? v4.12 won't have any effect on applications
furthermore the current drivers are broken when it comes to rotation at least most of them as they don't prevent rotation from being changed while streaming but they don't support it either they either ignore the change, or worse, could crash so they need to be fixed and that's definitely not v4.12 material one more thing the flag is only about drivers that implement the more complex sequence described in the documentation drivers that don't support control changes with buffers allocated don't need to be updated as the documentation for that case just describes the current behaviour it doesn't change it so no flag is needed for that case we already handle it | [13:50] |
| mchehab | ok | [13:54] |
| pinchartl | so I don't think we need to update existing drivers for v4.12 | [13:55] |
| mchehab | it would be good to describe it at the control flags session
when you add the description for it | [13:55] |
| pinchartl | yes, I'll explain that | [13:55] |
| mchehab | and likely at the longer explanation chapter you wrote | [13:56] |
| pinchartl | hverkuil: ack on the fact that the flag shouldn't be set when the control can't be modified with buffers allocated ?
sailus: same question | [13:56] |
| mchehab | on such case, I guess such contontrol should use flag V4L2_CTRL_FLAG_INACTIVE
while streaming | [13:57] |
| hverkuil | mchehab: actually, it should use FLAG_GRABBED while streaming. Yeah, ugly name, but I didn't come up with it. | [13:58] |
| mchehab | yeah, that makes more sense | [13:59] |
| pinchartl | that's an interesting idea, but I think it should be discussed separately from the v4.12 pull request :-) | [13:59] |
| mchehab | ugly name | [13:59] |
| hverkuil | Why shouldn't it be set? I would say that it is useful regardless of whether or not the driver can handle this without re-allocating buffers? | [14:00] |
| pinchartl | hverkuil: which flag ? GRABBED OR MODIFY_LAYOUT ? | [14:01] |
| hverkuil | MODIFY_LAYOUT.
MODIFY_LAYOUT indicates that changing the control can modify the layout. GRABBED indicates that you can't change this control while streaming. | [14:01] |
| mchehab | makes sense... | [14:02] |
| pinchartl | how are applications supposed to handle existing kernels where the flag doesn't exist ? | [14:02] |
| hverkuil | Drivers that can handle changing this on the fly wouldn't set the GRABBED flag, existing drivers hopefully always set this flag for ROTATE. | [14:02] |
| pinchartl | hverkuil: no driver sets the GRABBED flag for rotate, ever | [14:02] |
| mchehab | that should be fixed
if they don't support changing it in runtione | [14:02] |
| pinchartl | mchehab: yes, but separately :-) | [14:03] |
| mchehab | *runtime
this is already a violation of V4L2 API | [14:03] |
| pinchartl | it's an existing issue unrelated to the patch series | [14:03] |
| mchehab | if I understood hverkuil's proposal: | [14:03] |
| hverkuil | ROTATE has never been well tested. HW that supports it is rare and usually hard to find. | [14:04] |
| mchehab | if a control like CID_ROTATE would change the buffer, due to the need for PAD, it will rise MODIFY_LAYOUT | [14:04] |
| hverkuil | I'm not surprised support is flaky. | [14:04] |
| mchehab | if streaming, if the driver doesn't allow rotating while streaming, GRABBED will be rised
with that, at application side, if a control comes with MODIFY_LAYOUT... and user requests to change the control, if the buffer is not big enough and GRABBED is set, it will stop/requeue buffers/start. Otherwise, it will just send the control err... | [14:05] |
| pinchartl | I'm not sure to see how MODIFY_LAYOUT will help to be honest. applications need to support controls today, without the MODIFY_LAYOUT flag as it doesn't exist | [14:07] |
| mchehab | if the buffer is not big enough and GRABBED is not set, it will stop/requeue buffers/start. Otherwise, it will just send the control | [14:07] |
| pinchartl | the proposed documentation patch clarifies how the interaction between buffers and controls work today
it doesn't modify the API, it documents it if we add a MODIFY_LAYOUT flag applications will still need to handle controls and buffers interactions without the flag, as existing kernels don't set it | [14:07] |
| mchehab | pinchartl: at the moment we allow a control to modify the buffer size at runtime, we're modifying the API | [14:08] |
| hverkuil | MODIFY_LAYOUT gives additional information. It allows applications to detect that this particular control can have side-effects. That's all. | [14:08] |
| pinchartl | mchehab: it's not disallowed by the API today. it's completely undocumented
the API doesn't say anything about it it doesn't tell how to handle the ROTATE control for instance I'm not extending the API here, I'm documenting it | [14:08] |
| mchehab | pinchartl: the way I see is that CID_ROTATE implementations that would allow format changes currently violates the API | [14:09] |
| pinchartl | at least the common case (controls can't be changed with buffers allocated) is not a violation of the existing API
so it has to keep working without any new flag we can add a flag but we need to keep the existing API working without that new flag if we use the flag for the common, existing case we'll basically tell applications there's a new flag to gives you information but it might or might not be set, regardless of whether it should be set now figure it out that wouldn't be very helpful :-) | [14:10] |
| hverkuil | pinchartl: that happens all the time. The reality is that applications won't use it until we're 2-3 years further and every distro has this support available. | [14:12] |
| pinchartl | hverkuil: still, applications could run on older kernels that don't have this flag
I'm also concerned about the feasibility of implementing such a flag when happens when V4L2_CID_ROTATE is implemented by the sensor subdev ? padding is controlled at the DMA engine level the sensor has no way to know whether the rotate flag will change the buffer size | [14:13] |
| hverkuil | Also, since this flag would be specific to ROTATE I really don't care since any application written for such things will be custom anyway. | [14:14] |
| pinchartl | it can't decide whether to allow changes to the control based on whether buffer are allocated, as it knows nothing about buffers | [14:15] |
| hverkuil | If you rotate by 90 or 270 degrees, wouldn't it always change the layout? I.e. width and height get swapped. The size may be the same, but the layout isn't. | [14:16] |
| pinchartl | the layout is always changed
but the size might not be so whether to allow S_CTRL with buffers allocated is not something that the subdev can decide on its own | [14:16] |
| hverkuil | What should the flag indicate: that the size changes or that the layout changes? You said earlier that it indicates that the layout changes, not necessarily the size. | [14:17] |
| pinchartl | the comment wasn't related to the flag, sorry
the flag should always be set for the rotation control my question was related to how drivers should handle the rotation control when implemented by a subdev | [14:17] |
| hverkuil | Ah. | [14:19] |
| pinchartl | when a driver doesn't want to allow control changes with buffers allocated
how can it do so ? for the VSP driver it's easy there's a single driver, no external subdev | [14:19] |
| hverkuil | Do we need to think about this now? | [14:20] |
| pinchartl | not necessarily, but I'm not sure we should add a flag without thinking about the whole issue
again, the documentation patch doesn't try to extend the API its goal is to document it as it's currently not documented, and drivers do different things so it's not an API extension | [14:20] |
| mchehab | pinchartl: if you think we need more time to discuss, let's keep it out of 4.12 | [14:22] |
| pinchartl | I think we need more time to discuss the flag, yes
but I don't think it should block the VSP pull request | [14:22] |
| mchehab | VSP pull request has dependencies on it | [14:22] |
| pinchartl | no
it depends on the documentation but it does not extend the API at least not that part of the API the histogram formats it adds are API extensions, but they're not related to interactions with buffers mchehab: furthermore, the rotation support patch for the VSP driver doesn't allow rotation to be changed with buffers allocated so it's clearly not a violation of the existing, non-documented API :-) | [14:22] |
| hverkuil | pinchartl: does it set the GRABBED flag? | [14:24] |
| mchehab | this patch:
vsp1: wpf: Implement rotation support seems to assume that it is possible to rotate without stop streaming we need to agree with API bits before adding it | [14:25] |
| pinchartl | hverkuil: no. no driver does currently. that should be fixed, and I will fix that for the VSP driver. I could even fix it today if needed
mchehab: no, rotation can't be modified without stopping the stream it can't even be modified with buffers allocated | [14:25] |
| mchehab | ah, ok | [14:26] |
| pinchartl | in vsp1_wpf_set_rotation()
/* Changing rotation isn't allowed when buffers are allocated. */ mutex_lock(&video->lock); if (vb2_is_busy(&video->queue)) { ret = -EBUSY; goto done; that's very explicit :-) | [14:26] |
| hverkuil | (regarding existing drivers: it's not clear to me when the rotate control is actually programmed: it might only happen when you start streaming. | [14:26] |
| pinchartl | hverkuil: some drivers do that, they accept modifications of controls at any time, but they will only take effect when starting streaming
that's not really V4L2-compliant but it's a common behaviour same for formats or selection rectangles in subdev drivers | [14:26] |
| hverkuil | that seems to be the case for exynos4-is/fimc. | [14:28] |
| pinchartl | when it comes to rotation, different drivers are broken in different ways
and I think we still need to discuss a bit more how to fix them as we need an API that is possible to implement in drivers :-) the VSP rotation patch it least is fine, but that's because it's an easy case it's a single driver and I decided to disallow rotation changes with buffers allocate to keep it simple to start with s/allocate/allocated/ | [14:28] |
| hverkuil | exynos-gsc and mtk-mdp are m2m devices, so that's set per-frame anyway. | [14:30] |
| pinchartl | no external subdevs for them either, so it shouldn't be too difficult to fix
the fimc driver is different but it implements rotation in the ISP so there's a single driver that covers both the video nodes and the rotation that shouldn't be too difficult to fix | [14:30] |
| hverkuil | omap_vout.c I do not trust. | [14:32] |
| pinchartl | still, I'd like to discuss the general case to make sure that the proposed new flag will be useable in practice
no external subdev for omap_vout either and I'd like to get rid of omap_vout completely the only blocker is rotation support in the omapdrm driver once we get that, omapfb + omap_vout can go away but that's not on anyone's to-do list :-S | [14:32] |
| hverkuil | bcm2835 I do not trust either (but it's staging) | [14:33] |
| pinchartl | hverkuil: do you think the new MODIFY_LAYOUT flag is v4.12 material or v4.13 ? I think v4.13 is safer | [14:34] |
| mchehab | pinchartl: IMHO, we should modify the API documentation together with MODIFY_LAYOUT | [14:37] |
| hverkuil | I have no problem with this flag going into 4.12. Signaling that changing a control may change the layout as well (memory or mediabus) is fine and useful IMHO. | [14:38] |
| pinchartl | mchehab: again I'm not modifying the API, I'm only documenting the existing API :-)
most drivers don't allow control changes with buffers allocated some do | [14:38] |
| mchehab | I agree with hverkuil: I don't see any reason why not doing it on 4.12 | [14:38] |
| pinchartl | that's the existing date
s/date/state/ and that's all the documentation patch is doing, it documents the existing API I don't feel comfortable adding a new flag, which is an API extension, two weeks before the merge window | [14:38] |
| hverkuil | The issues raised here have more to do with how rotate is implemented today and what to do about it in the future if a subdev implements this.
Which are valid concerns, but not related to this control flag IMHO. | [14:40] |
| pinchartl | the control flag should be properly reviewed on the list
sending it today to get it merged by the end of the week isn't a good idea I think Linus would yell at us again :-S | [14:40] |
| hverkuil | That's a valid concern. We're at rc6, so that's a bit close. | [14:42] |
| mchehab | pinchartl: my suggestion here is then to delay the documentation patch too | [14:43] |
| pinchartl | mchehab: why ? it only documents the current API | [14:43] |
| mchehab | what you see as "clarify" I see as "modify" | [14:43] |
| pinchartl | why should we delay documenting what is implemented today ?
which part do you see as an API modification ? | [14:43] |
| mchehab | pinchartl: currently, some behaviors are an API violation (if not explicitly allowed, then it is denied) | [14:44] |
| pinchartl | which behaviour(s) ? | [14:45] |
| mchehab | it makes the API more flexible, by explicitly allowing format changes by a control | [14:45] |
| pinchartl | no, the format doesn't change. only the buffer layout does. the pixel format must not be changed by a control | [14:45] |
| mchehab | I nack allowing it without a flag to signalize it
s/format changes/buffer layout changes/ | [14:45] |
| pinchartl | could you tell me clearly which part of the documentation patch you see as an API modification ? | [14:46] |
| mchehab | read the patch description... it clearly says so
if something is not fully specified, and you're specifying things, you're changing the API the way I see, currently CID_ROTATE violates the API (in a number of ways) | [14:47] |
| pinchartl | I've just re-read the commit message and it doesn't tell that it extends the API
we have an existing API and part of it isn't documented in writing | [14:49] |
| mchehab | > The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. | [14:50] |
| pinchartl | you can argue that this patch extends the API in addition to documenting it | [14:50] |
| mchehab | > The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is > also not fully specified. > This patch clearly defines and documents the interactions between > formats, selections, controls and buffers. | [14:50] |
| pinchartl | but you can't say there's no existing API | [14:50] |
| snawrocki | hverkuil: In the exynos4 SoCs within the FIMC IP block there is a resizer after the rotator, so the driver allows changing rotation after STREAMON, any new settings are applied to H/W in VSYNC interrupt handler, V4L2_CID_ROTATE is exposed on the video node. | [14:50] |
| mchehab | no, I'm saying it is modifying the API | [14:50] |
| pinchartl | so, my question is, which parts of the patch do you consider as documenting the existing API, and which parts do you consider as extending it ? | [14:50] |
| mchehab | by defining the behavior
snawrocki: does it add/change PAD bytes when a buffer is rotated? | [14:50] |
| snawrocki | mchehab: no, they are unchanged, but the image aspect ratio is not maintained | [14:52] |
| mchehab | pinchartl: I don't see any sense on your question. On V4L2, the API is what should rule. The drivers should follow it, or they aren't compliant with the API
so, everything added there modifies the API | [14:53] |
| pinchartl | let me ask the question differently
what is the existing API ? | [14:53] |
| mchehab | for example, if we add something like:
.. note:: for CID_ROTATE, drivers shoudn't set CTRL_GRABBER we're modifying the API the reverse would be the same: .. note:: for CID_ROTATE, if drivers don't support rotating while streaming, they should set CTRL_GRABBER both modifies the API the existing API is what's written at the API documentation | [14:53] |
| pinchartl | so, what you're saying, is that drivers can do whatever they want as long as this isn't documented ?
in that case drop the documentation patch and merge the code changes only | [14:55] |
| mchehab | pinchartl: no. what I'm saying is that it is the documentation that rules, not the implementation
if the driver doesn't follow the documentation, it is a driver's issue | [14:55] |
| pinchartl | BUT THERE'S NO DOCUMENTATION ?
s/?/!/ | [14:56] |
| mchehab | ??????????????????????????/ | [14:56] |
| pinchartl | how can a driver follow a documentation that doesn't exist ??? | [14:56] |
| mchehab | https://linuxtv.org/downloads/v4l-dvb-apis-new/ | [14:56] |
| pinchartl | the existing documentation doesn't tell ANYTHING about interactions between controls and buffers | [14:56] |
| mchehab | it should follow whatever is ruled there | [14:56] |
| pinchartl | it's completely silent
nothing, nada | [14:56] |
| mchehab | then documentation should be written first | [14:56] |
| pinchartl | we've had drivers that use controls and buffers for as long as V4L2 has existed | [14:57] |
| mchehab | and v4l2-compliance should detect
yes. we also have a driver that streams MPEG audio under /dev/video0 devnode | [14:57] |
| pinchartl | we're not talking about a single driver that does something wrong | [14:58] |
| mchehab | (with is clearly a violation of the API) | [14:58] |
| pinchartl | we're talking about something fundamental implemented by all drivers | [14:58] |
| mchehab | no, we're talking about 3 drivers that implement CID_ROTATE
and one more that it is adding support for it no other control affects the buffer layout | [14:58] |
| pinchartl | then tell me what how the *existing* API should be documented | [15:00] |
| mchehab | don't have time for this | [15:00] |
| pinchartl | *without* adding a new flag because it's the *existing* API
ah | [15:00] |
| mchehab | already told you
let me keep handling the other patches | [15:00] |
| pinchartl | so you're essentially telling me that what I propose is wrong, but that you don't have time to tell me what is the right solution ?
am I supposed to just guess until I get it right ? | [15:00] |
| mchehab | see the IRC log
we've been discussing it over the last 1:30 hours | [15:01] |
| pinchartl | yes and you still refuse to answer the question | [15:02] |
| mchehab | if you were unable to understand, we'll need to postpone it to some other time
as I have other things to do | [15:02] |
| pinchartl | if you're enable to clearly explain what should be done, how are we supposed to submit patches ? | [15:02] |
| hverkuil | mchehab: pinchartl: to be honest, I've lost the plot as well. | [15:02] |
| mchehab | I did it already. See my e-mail
if we're allowing controls to change format, we need to document it, and have a flag for it. It is as simple as that | [15:02] |
| pinchartl | mchehab: that doesn't answer the question. before *extending* the API, I want to document the *existing* API
*extensions* should come on top of what is *existing* | [15:03] |
| mchehab | The existing API is that controls shouldn't change format
but you know that | [15:04] |
| pinchartl | are you saying that the existing API is that a control that influences the buffer layout should not be allowed to be changed with buffers allocated ? | [15:04] |
| mchehab | anyway, have to go
ttyl | [15:04] |
| hverkuil | mchehab: ??? But they do, and I don't think it is documented anywhere that controls shouldn't change the format.
Sorry, I agree with Laurent here. | [15:05] |
| mchehab | hverkuil: just one control does that | [15:05] |
| hverkuil | So? | [15:06] |
| mchehab | and it doesn't follow what's at the API, as it doesn't rise GRABBER flag | [15:06] |
| pinchartl | mchehab: that's an implementation problem | [15:06] |
| mchehab | yes, the control was added violating the API
and need to be fixed anyway, I don't have time for discussions today. we can schedule discussing it next week | [15:06] |
| pinchartl | mchehab: I don't think I want to discuss V4L2 anymore | [15:07] |
| hverkuil | mchehab: well, you acked it at the time :-) And there is no rule in the API that prohibits controls from changing the format. | [15:07] |
| mchehab | hverkuil: yes. my mistake. I didn't realize that it would be modifying the format
hverkuil: there's no rule at API that allows controls from changing the format adding a rule that would either allow (or deny) is modifying the API | [15:09] |
| hverkuil | Just for the record: I'm not happy with the rotate control either. I wouldn't have used a control for that today. | [15:10] |
| mchehab | the way I read specs is that, everything that it is not explicitly allowed is denyed
no specs will ever have everything documented - so, IMHO, assuming that something not document is invalid usually makes easier for everybody... as everything that it is not document will rise a flag that documentation is needed for such feature | [15:10] |
| pinchartl | no spec can ever document everything with the finest level of details
it's impossible | [15:13] |
| mchehab | yes, that's what I just said | [15:13] |
| pinchartl | and you can't retrospectively say that developers should have read your mind to understand how things should work | [15:13] |
| mchehab | no. they should read the docs | [15:13] |
| pinchartl | the documentation doesn't tell me explicitly that my driver can implement 5 controls. is it forbidden ? | [15:14] |
| hverkuil | V4L2_CID_ROTATE (integer)
Rotates the image by specified angle. Common angles are 90, 270 and 180. Rotating the image to 90 and 270 will reverse the height and width of the display window. It is necessary to set the new height and width of the picture using the VIDIOC_S_FMT ioctl according to the rotation angle selected. ^^^^ That's really vague documentation for this control :-( | [15:14] |
| mchehab | yes :-( | [15:14] |
| hverkuil | OK, this won't go in for 4.12, that's a safe conclusion. | [15:14] |
| pinchartl | it won't go in v4.13 either | [15:15] |
| hverkuil | I suggest we pick a date/time to look at this fresh. | [15:15] |
| pinchartl | without me | [15:15] |
| mchehab | hverkuil: agreed | [15:15] |
| pinchartl | pick any date or time you want
and have fun | [15:15] |
| hverkuil | pinchartl: do the patches after 'v4l: vsp1: wpf: Implement rotation support' depend on that patch? | [15:20] |
| pinchartl | they conflict with it
they could be rebased to go in before rotation support for they add histogram controls that influence the buffer layout at least for one of the two histogram engines the other one has no such control | [15:20] |
| hverkuil | Ah yes. Since there is another histogram control that changes that buffer layout there is not much point in rebasing this. | [15:23] |
| pinchartl | probably not, no
too bad, it will stay out-of-tree | [15:23] |
| hverkuil | I'll discuss this further with Mauro. For some reason discussions between the two of you get out of hand :-( | [15:26] |
| pinchartl | do whatever you want, I don't care much
thanks for trying to help though that I care about, and it's appreciated however, I don't think we'll get anymore please also feel free to take over the request API, I don't think I'll continue working on it or on any other V4L2 API extensions I know how I'd like the request API to look like, but there's no point in working on it myself s/anymore/anywhere/ | [15:26] |
| mchehab | hverkuil: with regards to this pull request:
Subject: [GIT, PULL, FOR, v4.12] Add support for the RainShadow Tech HDMI CEC adapter you're adding there at the pull request e-mail two "patches": one for udev and another one for linuxconsoletools. The best would be to send those to the owners of those apps | [15:29] |
| hverkuil | Of course. I added those for completeness so that someone who wants to use this driver can find this information. | [15:30] |
| mchehab | ok
btw, while you're on it, it would be good to send a patch adding /dev/media too on udev e. g. adding this to /lib/udev/rules.d/50-udev-default.rules: +SUBSYSTEM=="media", GROUP="video" | [15:31] |
| hverkuil | I need to look into udev, I never actually made any changes there (or to systemd for that matter). | [15:32] |
| mchehab | shouldn't be hard. I submitted a patch for them (systemd, I guess) 3 years ago, fixing an issue on my old notebook's keyboard mapping... just don't remember anymore the procedure
hverkuil: back to the previous question, my suggestion would be if you could send a patch, after Lauren't documentation one, adding a proposal for GRABBER/MODIFY_LAYOUT flags | [15:33] |
| hverkuil | A proposal for a MODIFY_LAYOUT flag I understand, but what is there to document for the GRABBER flag? | [15:37] |
| mchehab | I don't think that adding a single new control flag would be too late for 4.12. We already added new FOURCC's on late -rc cycle and this was never a problem
I mean: IMHO, it would be good to add a note at the new documentation section, added by pinchartl's patch... saying that, if a control that modifies the layout can't be changed while stream, it should rise the GRABBER flag | [15:38] |
| hverkuil | ah, you mean that this should be mentioned more explicitly. OK. | [15:39] |
| mchehab | (not really required, as the spec already says that, but IMHO it would help driver developers to not forget of doing it)
yep | [15:39] |
| ........ (idle for 36mn) | ||
| hverkuil | pinchartl: which vsp1 histogram control can change the layout? | [16:16] |
| pinchartl | hverkuil: V4L2_CID_VSP1_HGO_MAX_RGB and V4L2_CID_VSP1_HGO_NUM_BINS | [16:17] |
| hverkuil | Ah, I misread an earlier comment from you. OK, thanks! | [16:18] |
| mchehab | most of media drivers that used to be at staging are now in main line... remaining ones are bcm2048, cxd2099, davinci_vpfe, lirc_zilog and omap4iss (not including the new ones adding for 4.12: atomisp and bcm2835)
sean is already working on converting lirc_zilog. cxd2099 is there due to a misusage of a DVB API - not sure how to address it hverkuil: do you know what's the status of bcm2048 and davinci_vpfe? | [16:19] |
| hverkuil | I don't see bcm2048 going anywhere. | [16:23] |
| mchehab | it seems that nobody is doing anything on omap4iss
sailus did some work on davinci_vpfe last year but its todo list is big (not sure if it is updated) | [16:25] |
| hverkuil | For the imap4iss there is no alternative. I wouldn't remove it. But I wonder what is needed to get that out of staging. | [16:29] |
| mchehab | the omap4iss TODO list is small | [16:29] |
| pinchartl | hverkuil: it shouldn't take too much, but there's nobody to do the work | [16:30] |
| mchehab | basically:
* Fix FIFO/buffer overflows and underflows * Replace dummy resizer code with a real implementation (and coding style, but we've been receiving coding style fixes for it - hopefully, everything is sorted out there already) hmm... it builds fine with COMPILE_TEST | [16:30] |
| .... (idle for 18mn) | ||
| hverkuil | mchehab: pinchartl: posted a patch for the new MODIFY_LAYOUT flag and how to use it. It's an RFC patch only, see the link to my vsp1 branch for the actual code changes.
I'll post a proper patch series once everyone agrees with these documentation changes. | [16:50] |
| pinchartl | hverkuil: thank you, but I think it's too late | [16:51] |
| mchehab | hverkuil: patch looks good on my eye | [16:59] |
| hverkuil | mchehab: so let me get this straight: with this in place you are OK with merging the "Clearly document interactions between formats, controls and buffers
" patch? | [17:10] |
| mchehab | yes, and pinchartl's patch series, provided that it handles both flags on the controls that modify layout
(that could be an extra patch at the end of the series) | [17:11] |
| hverkuil | It's OK if my patches come at the end of pinchartl's original patch series? I don't see any reason why that would be a problem. | [17:13] |
| mchehab | (provided that I won't see any other API change there that would require further discussions)
hverkuil: that's OK I just want them to be merged at the same Kernel version, as I said earlier today e.g. as we're modifying the API, let's do it on just one Kernel version | [17:14] |
| hverkuil | Please review 'v4l: Add metadata buffer type and format' as well, since that's another API addition.
I'll post a patch series adding the MODIFY_LAYOUT flag today or tomorrow at the latest. (need to go shopping now, ttyl) | [17:17] |
| mchehab | mchehab is reviewing it | [17:18] |
| yeah, patch looks OK on my eyes.
I guess I already saw already an earlier version of this patch before just had two minor comments to it | [17:24] | |
| ............................................................. (idle for 5h4mn) | ||
| *** | rellla has quit IRC (Ping timeout: 240 seconds) | [22:29] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |