[07:02] <ttomov> hverkuil: I was thinking the same about the crop in ov5645. I'll check again and probably remove it. Thanks for suggestion :)
[09:27] <paulk-leonov> is it correct that PIX_FMT_NV12M should be used with the multi-planar API while PIX_FMT_NV12 is for the single-planar API?
[09:30] <paulk-leonov> or maybe I'm confused by how contiguous planes have to be handled wrt the allocator
[09:31] <hverkuil> Yes, NV12M is for use with the multiplanar API.
[09:45] <paulk-leonov> mhh so it looks like I'm using vb2_dma_contig_memops already
[09:46] <paulk-leonov> and my planes are not allocated contiguous
[09:46] <paulk-leonov> is there anything special I need to be doing?
[09:49] <hverkuil> NV12M allocates two buffers: one for the luma plane, one for the chroma plane.
[09:50] <hverkuil> The memory of each buffer is contiguous (your using videobuf2-dma-contig), but each buffer is allocated separately, so they are in different places in memory.
[09:51] <hverkuil> NV12 allocates a single buffer (again contiguous since you use dma-contig) containing both planes where the chroma plane follows the luma plane directly.
[09:52] <paulk-leonov> hverkuil, thanks -- I don't understand how the latter case takes place, since __vb2_buf_mem_alloc iterates over planes in that case too
[09:53] <paulk-leonov> or does the vb2_buffer need to have num_planes == 1?
[09:53] <hverkuil> The terminology is very confusing and that's our fault. vb->num_planes == 1 for NV12 and 2 for NV12M.
[09:54] <hverkuil> It really means 'num_buffers'.
[09:54] <paulk-leonov> ah! I see :)
[09:55] <paulk-leonov> hverkuil, but the format's num_planes stays 2 in both cases?
[09:55] <paulk-leonov> I'd really like to expose by two planes to userspace with NV12
[09:55] <paulk-leonov> my two planes*
[09:56] <hverkuil> yes, the data is organized into two planes in both cases, but NV12 puts them both in one buffer, and NV12M puts them in two buffers, one for each plane.
[09:57] <hverkuil> for NV12 you set *nplanes to 1 in cedrus_queue_setup and set sizes[0] to the combines size of both planes.
[09:57] <hverkuil> And the DMA engine should fill up the buffer correctly (i.e. no gap between the luma and chroma data)
[09:59] <paulk-leonov> hverkuil, okay, then my next question is how this is reflected when exporting the buffer and how I can get a DMA address to each plane
[09:59] <paulk-leonov> in v4l2_exportbuffer, is "planes" relative to the buffer or the format?
[09:59] <paulk-leonov> "plane"
[09:59] <paulk-leonov> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-expbuf.html?highlight=v4l2_exportbuffer#c.v4l2_exportbuffer
[10:00] <hverkuil> buffer
[10:00] <hverkuil> so for NV12M it will export two dmabufs, one for each buffer.
[10:00] <hverkuil> for NV12 it exports only one.
[10:01] <paulk-leonov> okay, and then I need to translate that into 2 DRM handles
[10:01] <paulk-leonov> I'm not sure there's a way to introduce an offset or something that would do the trick
[10:02] <paulk-leonov> to get two distinct handles out of a single dmabuf fd
[10:02] <hverkuil> It's all a mess. One of the things that is high on my todo list (after the request API) is to try and unify the single/multi plane APIs.
[10:03] <hverkuil> An initial start is here: https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3
[10:03] <paulk-leonov> that would be a very welcome change, for sure
[10:04] <hverkuil> Won't help yet with 1 or 2 drm handles.
[11:20] <tfiga> paulk-leonov: you get 1 FD from v4l2_exportbuffer, offset[0] = 0 and offset[1] = height * bytesperline
[11:20] <tfiga> and you import that to DRM
[11:23] <tfiga>  paulk-leonov: I believe you just import the FD once using drmPrimeFdToHandle()=handle and then, if you want to use that with KMS, you use drmModeAddFb2 with handles[0]=handles[1]=handle and offsets, as above
[11:24] <tfiga> if you want to use with GL, I think eglCreateImageKHR has very similar semantics
[11:40] <paulk-leonov> ahhh there are offsets in DRM, right right
[11:46] <svarbanov> hverkuil: ok, I've compiled smatch and will try to fix the errors
[12:18] <paulk-leonov> tfiga, new issue for cedrus: height for the two YUV planes is not the same, so bytesperline * height doesn't really do the trick
[12:18] <paulk-leonov> luma size is: ALIGN(width, 32) * ALIGN(height, 32) and chroma size is ALIGN(width, 32) * ALIGN(height / 2, 32)
[12:22] <paulk-leonov> ah wait, no, nevermind
[12:22] <paulk-leonov> my offset is with ALIGN(height, 32) so I can just set height to that
[12:49] <hverkuil> svarbanov: ping
[12:49] <svarbanov> hverkuil: pong
[12:50] <hverkuil> Isn't get_cap a bit big to keep as a static inline?
[12:50] <hverkuil> Esp. since venus_caps_by_codec is also already an inline.
[12:51] <svarbanov> you might be right
[12:51] <hverkuil> I would prefer if it is moved to the .c.
[12:51] <svarbanov> you propose to move it in the hfi_parser.c
[12:51] <hverkuil> right
[12:51] <svarbanov> ok, fair enough
[12:51] <svarbanov> let me do it
[12:52] <hverkuil> The other inlines (cap_min etc) are all fine as inlines.
[12:52] <hverkuil> You'd probably want to call it hfi_get_cap or something. Up to you.
[12:54] <hverkuil> Once I have a v7 of that patch I'll make a  pull request. Everything else looks fine to me.
[12:58] <svarbanov> hverkuil: hmm, when moving into get_cap in .c cap_min/max and so on cannot be seen by venus-enc.ko and venus-dec.ko
[12:58] <svarbanov> the hfi_parser is part of venus-core.ko
[13:00] <hverkuil> Ah, OK.
[13:01] <hverkuil> I'll take v6 in that case.
[13:01] <svarbanov> export_symbol fixes the issue
[13:02] <hverkuil> svarbanov: do you want to do a v7?
[13:03] <svarbanov> just wonder does EXPORT_SYMBOL_GPL is a good option
[13:04] <hverkuil> It has the advantage that venus_caps_by_codec() can be moved to core.c as well, if I understand this correctly.
[13:05] <hverkuil> I think you should do this, it improves the code. These are just weird static inlines.
[13:06] <svarbanov> hverkuil: ok, but such a change will take more time, I have to test everything one more time
[13:07] <hverkuil> An alternative is a follow-up patch. I'm OK with that as well.
[13:08] <svarbanov> hverkuil: I think you should take v6 and after that I will try to solve that mess
[13:08] <hverkuil> OK, will do.
[13:08] <svarbanov> Thx
[13:33] <hverkuil> svarbanov: can you take a look at https://patchwork.linuxtv.org/patch/50718/ and https://patchwork.linuxtv.org/patch/50719/?
[13:45] <hverkuil> sailus: any reason why this video-mux patch hasn't been picked up? https://patchwork.linuxtv.org/patch/49875/
[13:48] <svarbanov> hverkuil: are you applying venus updates on media-tree.git?
[13:51] <hverkuil> svarbanov: I'm going through 'random' pending patches and I came across those two.
[13:52] <hverkuil> I'm not sure what to do with them. I don't know if they conflict with your series.
[13:52] <hverkuil> Or what the status is of those patches for that matter.
[13:53] <svarbanov> hverkuil: I'm asking because I cannot apply venus-updates v5 on media-tree.git, git am on patch #12 fails
[13:55] <svarbanov> hverkuil: yes, I will look them
[13:55] <svarbanov> hverkuil: just wonder did I based venus-updates v5 on the correct branch, at least they applied cleanly on current mainline
[13:56] <hverkuil> Yeah, I had the same problem. Patching with patch -p1 worked.
[13:56] <hverkuil> Always base your work on the media master branch if at all possible.
[14:01] <svarbanov> hverkuil: this is the patch which makes git am to fail (even with --3way) "media: venus: keep resolution when adjusting format"
[14:52] *** benjiG has left