question about MMAP buffers: is DQBUF expected to fill in offset/mem_offset fields? I know the API spec explicitly states QUERYBUF fills them in, but the spec on DQBUF only says that drivers fill in _some_ fields
wens: good question. I think vb2 will fill the offset (to be verified). we may want to specify that in the documentation, although I'm not sure we want to push applications to use the offset at DQBUF time
looks like it does, so maybe I'm missing something downstream :/
pinchartl: what I am actually interested in is DST_QUEUE_OFF_BASE applied to offset/mem_offset, which does seem to only be done for QUERYBUF in v4l2-mem2mem. See https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-mem2mem.c#L603
ah for M2M
I haven't looked at that
I'm guessing we should apply DST_QUEUE_OFF_BASE in the other code paths that touch buffers as well?
for consistency that is
wens: no, the offsets are only used by mmap, and there you are expected to pass the value given to you by QUERYBUF. For QBUF/DQBUF etc. these fields are ignored.
I think the spec is clear on this: if I look at the VIDIOC_DQBUF description it says: "They just set the type, memory and reserved fields of a struct v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to this structure the driver fills the remaining fields or returns an error code."
Is there any documentation on set of controls generally expected on a camera sensor driver?
Looking more at the ancillary links stuff; there'll be some necessary changes to v4l-utils tools too. Can they go in the same series?
I guess not actually
djrscally: separate series, but to the same mailinglist.
ack, ta
hverkuil: "remaining fields" meaning whatever fields the driver would have filled in for QBUF? or every field? For capture buffers it would also fill in bytesused? It seems a bit ambiguous to me.
I guess it shoudl read: 'all remaining fields'
shouldn't that include mem_offset then? with the offset applied?
tfiga seems to think so # https://crrev.com/c/3308617/comments/de3aee79_4f2072ba
Yes, that includes mem_offset. That said, v4l2-mem2mem.c doesn't add the DST_QUEUE_OFF_BASE in v4l2_m2m_dqbuf or v4l2_m2m_prepare_buf. That's a bug in v4l2-mem2mem.c
hverkuil: Right. I'll send a patch to fix said bug. Thanks for clearing things up.
Note: v4l2_m2m_querybuf, v4l2_m2m_dqbuf and v4l2_m2m_prepare_buf all need to adjust the offsets.
Good catch.
If you are going to make a patch, then can you also update the VIDIOC_DQBUF doc to say "all remaining"?
Should qbuf do it as well? for consistency? AFAICT __fill_v4l2_buffers is called in that code path as well.
hverkuil: Sure.
wens: indeed, v4l2_m2m_qbuf calls it as well.
ezequielg: hi, can you review this patch ? https://patchwork.kernel.org/project/linux-media/list/?series=589375 without it HEVC is broken, thanks 
GBenji: oh! how come HEVC is broken1
!
GBenji: the patch doesn't have a Fixes tag, and it also doesn't say that it's fixing anything that is currently broken.
ezequielg: I haven't notice before this morning (while testing Adam patches for power domain) but since the changes about G2 postproc for VP9, HEVC pixel format is wrong (NV12 vs NV12_4L4)
that's clearly unintended.
so I that means we didn't test HEVC properly after merging VP9?
well, before not after.
I guess it's natural given HEVC userspace isn't easy to build.
ndufresne: not having HEVC in GStreamer (or Ffmpeg for that matter) really makes our life harder supporting HEVC.
i am not sure there's any way we can avoid HEVC from breaking often.
GBenji: if HEVC is broken, the standard path is to fix it with a small change that only fixes that, and which has a Fixes tag. That's because patches need to go stable, and your patch above is not -stable material.  
I have MR for HEVC decoder: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
Don't feel shame, I failed to notice we had broken MPEG2 in gstreamer by merging the VP9 support
(all fixed now)
we all suffer the lack of CI ;-P
well, no shame, it's a staging driver, no CI, bleeding-edge stuff. Just trying to think how to approach this properly.
ezequielg: would you really go backport HEVC patches here ?
absolutely.
but in this case, I'm 99% sure getting a simple fix is easy.
since VP9 and HEVC share the same structure it won't be simple to fix the problem. 
ezequeilg: I believe it is coming from patch "media: hantro: Add quirk for NV12/NV12_4L4 capture format" but I'm not sure 
this patch isn't yet in mainline, only on media_tree
actually, since we added this specifically for that... how come this is broken?
it was meant to avoid this issue in particular.
if you can be more specific about the issue I have some time now to look at the code.
... I'll get suck into meetings in a few.
the output pixel format isn't correct 
ioctl / traces?
no
the output format you mean the CAPTURE queue format?
yes, HEVC part of the driver alway generate NV12 but the driver claims it is NV12_4L4
so this is likely correct on vidioc_enum_fmt
but maybe it's incorrect on vidioc_try_fmt
anyway, I can't fix without traces.
ndufresne: I know it's kind of extreme, but the other option I have been considering is to get rid of HEVC in the driver :(
I have no opinion on that, won't that generate more work then reduce ?
my general recommendation is break the API one last time, and port drivers
maybe just review my patch that fix the problem and use the same method than VP9 ? won't it be simple ? 
About this NV12_4L4, how come it didn't break in VP9 ?
GBenji: I just explained why your patch cannot be merged as-is.
and can't be backported to -stable.
the issue isn't yet in -stable anyway 
merging things blindly, without detailed info about what is currently wrong and what they are fixing......
if you send a new version, with a detailed commit description, with traces about what exactly is broken.
so just a better commit message ?
.. plus a Fixes: tag.
Fixes is good, even if not for backports, its a cross-reference
this is specifically taken care of in the driver, and the driver should enumerate NV12 for G2/HEVC.
so if this is broken, it's clearly a bug that we failed to test.
the negotiation is just 3 or 4 ioctls, one of them is wrong :)
getting a decent commit is really easy, and it's actually daily business.
ezequielg: I have added log and tag in v5
mchehab: hverkuil: any chance we get this hotfix queued https://patchwork.linuxtv.org/project/linux-media/patch/20211208164418.848790-1-benjamin.gaignard@collabora.com/ ?