ndufresne: does the TRY_FMT issue in libv4l2 occur with a uvc webcam? I see that v4lconvert_do_try_format_uvc() just sets it to 0, but that can easily be fixed in libv4l2.
hverkuil: a link from an entity to itself is allowed right?
dafna21: good question. It doesn't make much sense. sailus?
(make much sense in real HW)
away
I don't think anyone has thought about that. :-)
In general circular graphs may have issues.
The graph traversal algorithm keeps track of the entities it has seen, so it won't end up in a loop (until the maximum parse depth is reached), but it may not work correctly either.
It hasn't been tested at least.
dafna21: All I see after the first message is: * dafna21 sent a long message:  <  >
Lunch time. I'll be back soon.
Back.
oh,
Ill cut it to several: there is a problem with it in vimc, when I add a link from the scaler to itself, like this: http://ix.io/22fQ
I get error when printing the topology: `media-ctl -d0 --print-dot`
free(): invalid next size (fast)
Aborted
and a crash when unloading the module: http://ix.io/22fR
Hmm.
The code in __media_entity_remove_link() seems to assume there are two entities involved.
So I presume it removes two entries in the list in a single iteration, including the next entry to be iterated over.
That should be fixed.
Why would you like to create a link to the entity itself?
I write the configfs feature to vimc which allow userspace to configure topology as they wish
and now I write tests for it which includes topologies with entities to themself
https://patchwork.kernel.org/cover/11153253/
Ok.
I think the kernel side *might* be good with this if __media_entity_remove_link() and other link related code is verified it works.
When a link is added, a backlink is created to the remote entity. That does not make sense if the remote entity is the same.
Generally code that is dealing with remote entities needs to be checked, too.
in case connecting an entity to itself does not make sense in any hardware then maybe it will be easyer to just not allow it
ndufresne: STREAMOFF(CAPTURE) was defined to reset because you may lose some bitstream when you STREAMOFF
because STREAMOFF just discards any buffers available to be dequeued
and resets them into dequeued state
so for reallocation without a reset I'd indeed go with what hverkuil suggested
hverkuil: I see in your vb2 cpu access series that you mention that it's possible to defer end_cpu_access call to buf_finish
I guess that's because we only allow CPU access to buffers allocated from vb2-vmalloc?
hverkuil, yes, you can reproduce it with a UVC camera
hverkuil, and indeed, it might be fixable, someone need to sit and thing a little how to emulate colorimetry in general, but the CSC don't expose anything atm
(it's trivial to make userspace happy, it's not has trivial to make it work properly, since CSC may change/apply the colorimetry)
tfiga, noted
ndufresne: also AFAIR none of the existing encoders actually keep the OUTPUT buffers for reference
they have internal buffers
not 100% sure about coda
looking at coda, it also allocates internal framebuffers
I guess most of the encoders may be blitting the source frame to a local buffer in a tiled (or otherwise more efficient) format
I wonder what's the exact lists now
though I doubt the Xilinx encoder have an intermediate
for sure Coda uses it's IPP, since it only deals with some Coda specific tiles
would need to ask svarbanov if that's the case for Venus
still, I think that even if we have an encoder that needs to hold to a source buffer, we just wouldn't return it to the userspace before it releases it
for Nvidia, the driver is a bit non-standard, and obviously downstream
and if one calls STREAMOFF(CAPTURE) that triggers a reset, such buffers could be released
that's not exactly correct
one raw image can produce multiple output buffers in real life
hence in theory, you could have a backward mode, where the output buffer can be held in order to complete it before pushing it downstream (for AU alignment)
doing streamoff(capture) would break that AU
STREAMOFF(CAPTURE) basically means I don't care about what was being encoded anymore
which is exactly the definition of STREAMOFF
on m2m, only STREAMOFF(CAPTURE) + STREAMOFF(OUTPUT) has strictly meant that
not really
and that's why we do that on both side in user space (ffmpeg/gstreamer at least)
in V4L2 in general STREAMOFF discards any buffers on the queue
in dequeue the buffers
that makes the whole m2m processing that was happening meaningless
most encoder will have a ringbuffer on the encoded side
so I'm not sure what it would be discarding
I think that's only coda :P
did you heard about amlogic ?
haven't seen anything :)
still, what STREAMOFF discards is the data that is already out of the ringbuffer
so you end up with a hole in the stream
it's common for HW with a HW demuxers (aka STB hardware)
basically STREAMOFF discards buffers that are past vb2_buffer_done() but before the userspace DQBUF
well, userspace can dequeue them before doing streamoff
it needs to do a drain
otherwise it doesn't know when to end dequeuing
sure
but drain is a special case
on the other hand, it probably doesn't matter...
what I'm saying is that associating a full reset with the CAPTURE queue is arbitrary, why not on the OUTPUT Queue ?
one needs to do a drain anyway, if they want to reset in a deterministic point
good point
it's not arbitrary - it's as I said, STREAMOFF can drop data
but I think it's good if we just go ahead and say you can allocate and S_FMT without streamoff
dropping bitstream means the stream isn't valid anymore
tfiga: cpu access is also done for non-vmalloc drivers. But apparently you can call vb2_plane_end_cpu_access in buf_finish only for vmalloc drivers. Sorry, It is too long ago to remember why that is so. I wrote it, so it must be true :-)
sure, but the encoder state is still valid, encoder don't care about the encoded bits
but the only way to resume meaningful encoding is to reset
sure, for which you will streamoff the output queue too
cause the cookies/timestamp make no sense anymore if you reset
but what should happen if you just streamon the CAPTURE queue again?
that would lead to some kind of undefined behavior
sounds like that, it's also undefined if you just cycle the CAPTURE queue imho
(but less dramatic maybe)
in current spec it's defined to do a reset :)
reset doesn't have to mean a full encoder reset, though
reset is defined as producing a stream that is valid on its own
without any dependencies on previous bitstream
it's just really bad guideline for userspace dev, since it will cause issue if one didn't "reset" both
why it would cause any issue?
hverkuil: what was in general the idea behind vb2-vmalloc? was it only for drivers that copy from/to those buffers?
tfiga, it's not very deterministic which frames remains in the queue
userspace would have to query each buffers to check the state, in order to know how many remains
I would just flush them all really
tfiga: originally it was for drivers that do not use direct DMA into the buffer, but instead copy the data to the buffer (or from, depending on the direction).
tfiga, thanks for the chat, have to leave now, I'll try and think about it, I'm still a little worried about "partial" reset of the queues, but in the end, it seems like userspace issue
hverkuil: okay, so that's what I was suspecting
it's interesting that we can export those buffers as DMA-bufs, though
which then makes it possible for them to go to some hardware DMA
tfiga: This doesn't really hold true anymore. Right now it is just memory that can be allocated with vmalloc and doesn't need either contiguous DMA or scatter-gather DMA.
Although if it is imported as a dmabuf to another driver then scatter-gather is required again.
That was a later addition though. And I am not 100% certain this works perfectly in all circumstances.
Specifically it makes me wonder if the actual vmalloc shouldn't at least round up to the next page size.
hmm, any reason to use vb2-vmalloc instead of vb2-dma-sg for a driver that does a DMA?
the DMA still needs some way to address the buffer
Probably not.
vmalloc is typically used by usb devices (since the usb packets need to be parsed) and of course virtual drivers.
if we limit it to the case of CPU access only, we could just make vb2-vmalloc call dma_buf_begin/end_cpu_access() in a generic way instead of the drivers
I was also thinking about changing the concept a bit to having a vb2_queue flag that means that the driver needs CPU access in buf_prepare and another flag for buf_finish
then the vb2 core could just call the begin and end functions appropriately
we would also reuse those flags for allowing cached MMAP buffers
tfiga: just to make sure: you've seen the link to Sakari's version, developed from my original code? Just want to make sure you've looked at that as well.
https://git.linuxtv.org/sailus/media_tree.git/log/?h=vb2-dc-noncoherent
hverkuil: I think that reached RFCv4 on the lists in the end
which I reviewed
and which we took over recently
I'm actually not sure if that's the same as the use of cpu access.
I think it's slightly different, but actually related
Sakari's series is about handling cached buffers
but cpu access is also about cached buffers, but imported ones
Anyway, go wild. It's all too long ago since I worked on this, so my memory of it is rather vague. And things have changed since then, so some things might not make sense anymore, or can be done better.
we're going to merge this into one series
sounds good.
I hope we can get something posted around next week
at least an RFC :)
it might take a while to inspect all the drivers for what they're doing in their buf_prepare and buf_finish
alternatively we could just assume by default that all drivers do some access in their callbacks
I guess that's the existing behavior
and gradually change drivers to set the queue flags appropraitely
Actually, outside usb and virtual drivers there aren't many that need cpu access.
In the remaining cases it's most commonly needed for compressed formats where the driver needs to patch a header.
You can make it generic for vmalloc, but for the others I would patch the drivers.
hi guys! I'm trying to solve a problem with a v4l2 platform device which sometimes is not properly initialized in the media controller (about 10% of the times). This means without changing anything, just rebooting the system and check. Any hint?
dagmcr: what do you mean?
The `/dev/mediaX` node is not created even though the rest of the `/dev/videoX` are listed. The weird is that only happens around 10% of the times you boot the system (without changing anything). The rest, works fines (so `/dev/mediaX` is created). And sadly, I have only tested on a 4.14 kernel version
sounds like a pretty regular race somewhere.
not sure it's strictly media related.
have you done any debugging/tracing?
I just enabled dynamic debugging in the drivers/media/* but no tracing yet
which driver is that?
qcam-camss
dagmcr: i suspect sometimes not all your subdevs register, and thus v4l2_async_notifier_operations.complete is not called.
add some prints there.
cool! thanks ezequielg for the tip. I will start debugging that part for all the subdevs.