<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>

   hverkuil: <u>mchehab</u>: one outstanding atomisp issue in the daily build: Check for strcpy/strncpy/strlcpy: WARNINGS: found 4 strcpy(), 4 strncpy(), 4 strlcpy()
   mchehab: <u>hverkuil</u>: yeah, this driver is on a bad shape
   hverkuil: Can you fix that? atomisp should use strscpy only.
   <br> Should be trivial.
   mchehab: btw, just checking for the above is probably not enough... it also uses memcpy for copying strings
   <br> with some weird logic to fill the remaing stuff with zero
   hverkuil: Fixing those will shut up the daily build warning :-)
   mchehab: :-)
   <br> ok, will add it on my atomisp internal todo list :-)
   <br> to solve at least the trivial cases
   hverkuil: Thanks!
   <br> <u>tfiga</u>: ping
   tfiga: <u>hverkuil</u>: pong
   hverkuil: Hi Tomasz! I just wanted to know if v3 of the 'Stateful Encoding: final bits' is OK for you, or if there are still changes needed.
   <br> I can try to improve the S_PARM encoder documentation a bit, perhaps?
   tfiga: I think it's probably as good as we can get with all the legacy :)
   <br> so feel free to go ahead
   hverkuil: Can you ack the last three patches?
   <br> I'll make a PR for 5.9 once I have that.
   <br> Thanks!
   ndufresne: thanks hverkuil for finalizing this
   <br> <u>tfiga</u>: we are toying with the hantro encoder from chromeos, due to emulation layer limitation we are pretty much forced to implement the drain flow from what is normally only required by stateful drivers
   <br> In the chromeos plugin it's using CMD_STOP + byteused=0, we are poring to the LAST flag/epipe now, question is do you you think it's a problem to include CMD_STOP support in these drivers in order to support simplier libv4l plugins ?
   <br> same would apply if we wanted a libv4l plugin for decoders, just wondering, as I think there is some value in supporting that, at least for encoders in the short term
   <br> what I think will happen, is that stateless native implementation would simply not use that, and so it's not a problem to have this
   tfiga: <u>hverkuil</u>: you're welcome
   <br> thanks for helping with finalizing that
   <br> it's going to be great to see the codec specs completed in a next kernel release :)
   <br> <u>ndufresne</u>: I think that's what we've done in our kernels
   <br> <u>ndufresne</u>: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f2659523f0f94b4769590e32d63d4c324f3f0f81/drivers/media/platform/rockchip-vpu/rockchip_vpu_enc.c#1054
   ndufresne: <u>tfiga</u>: ah, thanks, I missed this flags check before, it's strange it accepts the command with flag, but does not seem to check any flags ....
   tfiga: the implementation is a bit dated, so might not fully follow the latest spec, though
   <br> but yeah, basically we tried to make the driver behave as much as a stateful encoder +/- extra controls for the low level data
   ndufresne: ok, so I guess the flags abuse was just so CMD start/stop would only fork in the plugin
   <br> as it should not in general be needed
   <br> I see there is old crop implementation too, I don't remember what we decided, if the bitstream written crop is now taking from CAPTURE FMT or not ...
   <br> hmm, wait, this is likely unrelated, crop in stateless make no sense, bitstream is userspace written
   <br> so now I'm guessing there was crop support with the pre-processor ?
   taliho: Hi, what's the proper way to ping a patch? on the ML or here on IRC?
   <br> https://patchwork.linuxtv.org/patch/63518/
   ndufresne: <u>taliho</u>: both ;-P
   <br> but for this one, ping again on the ML, since the maintainers for that one are not super active, meaning someone else will have to take care
   <br> <u>taliho</u>: maybe I should just make a review, as this patch is incorrect ;-P
   taliho: thanks ndufresne :)
   ndufresne: <u>taliho</u>: can you explain briefly how you use this flag in userspace ? or do you set this flag only for the purpose of setting vb2-&gt;last_buffer_dequeued ?
   taliho: in the documentation it says it says that the last dequeued packet should have a payload of zero size and set V4L2_BUF_FLAG_LAST flag
   <br> the user space can use this info to decide when the draining of the capture buffers is finished
   ndufresne: <u>taliho</u>: well, not exactly
   <br> <u>taliho</u>: this empty buffer is totally optional, it's there for backward compat
   <br> <u>taliho</u>: what you should look for in userspace is EPIPE, as this is the only thing that you are guarantied to receive
   <br> I'm re-reading the spec now, and I must admit it's not that clear
   taliho: sure, EPIPE is another option
   <br> but it doesn't work on s5p-mfc for both encoding and decoding
   ndufresne: consider that the queue could have been completely empty by the time you call CMD_STOP
   <br> in which case, your driver should simply unblock on poll(read) and set last_buffer_dequeued = true on vb2 so that further dqbuf returns EPIPE
   <br> then it means that if you handle EPIPE in userspace, you handle all possible cases
   taliho: I don't remember whether encoding/decoding is the problem, but on s5p-mfc it hangs in poll() without returning EPIPE (sorry was a long time ago when I tested)
   ndufresne: the LAST flag is basically just to allow sooner detection when this is possible in the IP, and MFC does not provide this information, instead it signal there is no more buffer after sending the last one
   <br> <u>taliho</u>: that's my poin, you need to fix the cmd_stop implementation with that above
   taliho: ok I see. just to clarify,  you suggest to the solve the EPIPE issue instead of raising the LAST flag?
   ndufresne: <u>taliho</u>: or maybe both ?
   <br> does the LAST flag on empty buffer properly sets last_buffer_dequeued for you ?
   <br> as you want that anyway, and you can't remove the empty buffer in that driver either
   taliho: yeah, it was working for me when I tested on Odroid XU4
   ndufresne: do you have information about the userspace that was working ?
   <br> <u>taliho</u>: I'm just trying to see if this is enough to cover for all cases
   <br> as I see a lot more code in other drivers
   taliho: I counted the number of frames. and it matched up with the original
   <br> these are the ffmpeg patches I used for testing: (they have been merged)
   <br> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-1-andriy.gelman@gmail.com/
   <br> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-2-andriy.gelman@gmail.com/
   <br> <u>ndufresne</u>: thanks for reviewing :)  I'll look over the code and try to solve the EPIPE problem too
   ndufresne: ok, so the first ffmpeg patch is implementing legacy draining flow, it should not be needed after you fix
   <br> have you verified that it is not needed ?
   <br> normally, simple implementation should be able to only handle EPIPE, otherwise the driver is buggy
   <br> if ffmpeg wants these hacks in, I guess it does not matter much to you ...
   taliho: I'm not sure why you say they are hacks.. they are according to v4l2 documentation.. no? :)
   <br> true :) with the ffmpeg patches, you do not need the LAST flag. But I also wanted to solve the problem the root cause in the driver
   ndufresne: byteused == 0 is deprecated, so that's can be qualified as a hack if a newly written driver
   <br> now, the second, in my opinion is just duplicating effort, but folks wanted that flag in the kernel, so they have it
   <br> in practice, it will save you 1 poll() and 1 dqbuf call per drain
   <br> there might be some gapless transition case that may benefit, but that would need to be proven
   taliho: I completely agree, EPIPE should be enough
   ndufresne: <u>taliho</u>: btw, I've replied on the thread, with what I believe should be verified, if you can check that up, specially the case where you emit cmd_stop while there is nothing left to produce or dq
   <br> if that works, then I'll acked, I'm just not setup to test this myself, haven't fired up any exynos in the last 2 years
   <br> <u>taliho</u>: but if I followed well, this empty buffer + flag will set vb2-&gt;last_buffer_dequeued, and unblock the queue, which is basically what you want to happen, question is if that works for all cases
   taliho: thanks ndufresne, I'll do more testing
   ndufresne: <u>taliho</u>: perfect, and be positive, it's likely this is all right, but in absence of a maintainer, it's hard to confidently ack a patch
   <br> and worst case, you just need a special case in cmd_stop implementation
   taliho: yes sure. I understand it's tough to merge without active maintainer. I'm just using this as opportunity to learn about driver implementations :)
   ndufresne: ok, I was wondering if you had to handle CMD_START, but no, this is done by VB2, it will call b2_clear_last_buffer_dequeued(dst_vq) to unblock the queue
   <br> but MFC might stay stuck in it's "finished" state (can't remember the name in the state machine there)
   <br> I don't use that in gst, it's probably not used by ffmpeg either
   taliho: I'll try to debug.. but I don't think that's handled in ffmpeg
   tfiga: <u>ndufresne</u>: the crop was for the encoder
   <br> but it indeed should be replaced with selection CROP target on OUTPUT nowadays
   <br> also not sure what flags abuse you're referring to
   <br> the spec says that the flags must be 0 and the code just checks that
   ndufresne: <u>tfiga</u>: nevermind for the flags, I notice my mistake when reading
   <br> it's checked by the m2m framework already, so that should not be needed in the new driver
   <br> <u>hverkuil</u>: picking your brain a little, when you have time, why is vicodec_encoder_cmd/vicodec_decoder_cmd code not the default implementation in m2m ?
   tfiga: <u>ndufresne</u>: m2m is primarily for non-codecs
   <br> codecs actually need their own helpers, but someone is yet to come up with them
   <br> well, at least stateful ones
   ndufresne: <u>tfiga</u>: my point was mostly the the CMD_STOP handling implement in simple codec vicodec seems to be pretty generic, and stateless are simple, but the code is in the driver ....
   <br> and there is no other users of the existing cmd_stop helper, so I'm curious why the abstraction was cut like this
   <br> it looks overall quite painful, if you want to override one of the vb2 ops, you have to fill the entire ops table ...
   <br> made me surprise that there isn't some "fill" function ...
   -: ndufresne usually delegate this work, so I don't get to look at it too often
   ***: benjiG has left
   tfiga: <u>ndufresne</u>: I think that's a usual pattern in the kernel, but IMHO the verbosity isn't that bad, as it makes it clear what callbacks are used.