| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| hverkuil | mchehab: hverkuil: why include/media/v4l2-tpg-colors.h is at include/media?
koike moved it there if memory serves, but it can safely be moved to media/common/v4l2-tpg I think it ended up in include/media because v4l2-tpg.h includes it, but there is no need for that header to include v4l2-tpg-colors.h. So v4l2-tpg-colors.h can be moved to media/common/v4l2-tpg, dropped from v4l2-tpg.h and included explicitly to the v4l2-tpg sources. ndufresne: I completely missed that the venus driver uses data_offset. svarbanov: why is data_offset needed for this driver? | [07:54] |
| BanHammor | Hey, so i want my m2m driver to time out if the device doesn't generate interrupt in about a second. My calls to v4l2_m2m_job_finish() happen in IRQ. Is there already an implementation that does this? | [08:03] |
| hverkuil | In case svarbanov missed my question (I noticed a reconnect):
svarbanov: why is data_offset needed for the venus driver? | [08:04] |
| svarbanov | hverkuil: data_offset is used by the encoder for the compressed buffers, and the offset in the buffer is returned from the firmware side. | [08:06] |
| hverkuil | Does the decoder also use it? | [08:07] |
| svarbanov | hverkuil: I cannot remember right now, have to check | [08:08] |
| hverkuil | Note that the V4L2 API requires that bytesused should include data_offset. E.g. if the actual data is N bytes, but it starts at offset O in the buffer, then the bytesused field in the vb2_buffer planes array should be set to O + N.
Based on ndufresne's comments it appears that bytesused is just set to N instead of O + N. | [08:09] |
| svarbanov | hverkuil: looks like driver bug then, infact most of the codecs returns data_offset = 0, but there few exceptions, will check right now | [08:14] |
| hverkuil | I have to run an errand. Will be back in an hour. | [08:14] |
| .......... (idle for 45mn) | ||
| mchehab | hverkuil: ok, I'll prepare a patch moving v4l2-tpg-colors out of include/media. Let's keep include/media a place where we'll place only kAPI headers... | [08:59] |
| hverkuil | Makes sense. | [08:59] |
| ...... (idle for 25mn) | ||
| svarbanov | hverkuil: seems that venus decoder returns always data_offset = 0 for the capture queue, and on the encoder side data_offset is != 0 only for VP8, and yes data_offset is not included in bytesused | [09:24] |
| hverkuil | What is in that data_offset area exactly? Is it meta information, or just garbage? It's strange, I haven't seen codecs that do this before.
The problem is that the venus driver is the first mainlined driver for actual hardware that uses this. | [09:26] |
| svarbanov | hverkuil: I didn't debug so deeply, but in case of vp8 it looks like valid data but without some vp8 specific headers (if I remember correctly)
hverkuil: yeah, and this data_offset is used only for vp8, so seems like firmware side decision | [09:28] |
| hverkuil | how large can data_offset get? Is this accounted for when you set the imagesize field? (I.e. data_offset + bytesused should always be <= imagesize). Is it checked if it exceeds imagesize? | [09:31] |
| svarbanov | hverkuil: I think it is possible that the fw use the buffer for some reference frames or something, vp8 is targeted for low memory devices and is optimized for them | [09:32] |
| hverkuil | I missed the used of data_offset when I reviewed the code, sorry about that.
Or I would have asked these questions at review time. | [09:32] |
| svarbanov | hverkuil: the encoder part of the driver is less tested, so I'm expecting bugs there. Also there were no support in gstreamer at the time of implementing it too
hverkuil: but now when we have camera support and gstreamer v4l2 encoder plugin we could catch more bugs hopefully :) hverkuil: the v4l2 encoder in ffmpeg is also merged ;) | [09:35] |
| hverkuil | svarbanov: OK, so we need a patch fixing the bytesused field for the encoder somewhat urgently since that is clearly wrong. | [09:42] |
| mchehab | hverkuil: moving it is not trivial... v4l2-tpg.h uses TPG_COLOR_MAX
I really hate this: struct color { "color" is something that could cause conflicts :-) v4l2_tpg_color would be better by having this together with kAPI, people may eventually use it elsewhere | [09:44] |
| hverkuil | mchehab: Feel free to change it. This used to be local to the vivid driver, but since it is now shared with vimc as well it makes sense to rename it. | [09:50] |
| mchehab | I'll place it at the end... there isn't a very good reason to document those
...except that I'd like deny accepting patches to include/media without everything documented I guess I'll just create a tpg subdir there and put those things inside it the sub-directories of include/media contain things that aren't part of kAPI | [09:51] |
| hverkuil | I missed the TPG_COLOR_MAX define. So v4l2-tpg.h still needs to include v4l2-tpg-color.h. It is also fine to just merge v4l2-tpg-color.h into v4l2-tpg.h. | [09:56] |
| mchehab | yeah, I guess it probably makes sense to do that
ok, I'll merge v4l2-tpg-color.h into v4l2-tpg.h and move them to include/media/tpg | [09:56] |
| hverkuil | and rename struct color to tpg_rgb_color and color16 to tpg_rgb16_color. | [09:58] |
| mchehab | yes | [10:01] |
| tgb_rbg_color8 and tgb_rbg_color16 makes more sense
ops s/tgb/tpg/ | [10:10] | |
| hverkuil | that's fine too. | [10:12] |
| mchehab | patches written
ssh://linuxtv.org/git/mchehab/experimental.git v4l-docs-part2-v1 | [10:13] |
| hverkuil | I'm not sure why color16 uses int instead of s16: there might be overflows in intermediate calculations, I would have to check. | [10:13] |
| mchehab | yeah, s16 would be better - or rename this to color32 :-)
anyway, I'm sending the patches I probably won't have much time this week for documentation stuff | [10:14] |
| hverkuil | int in color16 can be changed to __u16 safely. | [10:18] |
| mchehab | ok, will add an extra patch with such change
https://git.linuxtv.org/mchehab/experimental.git/log/?h=v4l-docs-part2-v1 I'm not sure what to do with v4l2-clk.h there are still some drivers using it ah, btw, there are several undocumented things at cec*.h headers :-) I'm skipping them, assuming that you'll document them when you have some time actually, I converted one existing documentation here: 41e0821221f6 media: cec-pin.h: convert comments for cec_pin_state into kernel-doc to kernel-doc but kept the rest undocumented | [10:22] |
| hverkuil | mchehab: I can't document the CEC_MSG_ and CEC_OP_ defines: these are all documented in the HDMI specification. Documenting them in the header might well break copyright. The 1.3 (and with some googling the 1.4) HDMI specs are available, so they are easy to get.
https://hverkuil.home.xs4all.nl/spec/uapi/cec/cec-intro.html refers to those specifications. | [10:33] |
| mchehab | well, then document that those structs match the HDMI spec :-)
E.g. something like: /** * struct cec_msg_entry - matches HDMI spec 1.3 struct foo */ struct cec_msg_entry { /* private: see HDMI spec 1.3 for definitions of the stuff below */ struct list_head list; struct cec_msg msg; }; the above is just a random (bad) example, as, in this specific struct, you can probably document as list_header is not part of HDMI spec | [10:34] |
| hverkuil | I'm talking about the CEC_MSG_* and CEC_OP_* defines in uapi/linux/cec.h. | [10:38] |
| mchehab | ah, you're looking at uAPI
I'm talking about kAPI include/media/cec.h for example: struct cec_adapter | [10:40] |
| hverkuil | Yeah. Some of that is documented in Documentation/media/kapi/cec-core.rst. | [10:43] |
| mchehab | the thing of documenting it outside the header is that it easy to make them out of sync
so, IMHO, we should document the structs/functions using kernel-doc, keeping the ReST files for a more functional description | [10:44] |
| hverkuil | syoung: finished review of your patch series. | [10:51] |
| .............. (idle for 1h9mn) | ||
| syoung | hverkuil: thank you very much, very good/valid points | [12:00] |
| ..... (idle for 20mn) | ||
| *** | LazyGrizzly has left | [12:20] |
| ...................... (idle for 1h46mn) | ||
| hverkuil | mchehab: I plan to review your v7 patch series (document types of hardware control for V4L2) later this week. I hope Wed or Thu. | [14:06] |
| ................ (idle for 1h16mn) | ||
| *** | benjiG has left | [15:22] |
| mchehab | ok, thanks! | [15:25] |
| ............................................................. (idle for 5h4mn) | ||
| ndufresne | svarbanov, thanks, will give your patch a try soon | [20:29] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |