ok, got a fix for that one, gst-play-1.0 file just work now
where do I find the latest firmwares ?
(nervermind, found it)
ndufresne: I set it to 1/1 during start_streaming, so you should get a proper value when probing it after a dqbuf. But I'll initialize it to 1/1 anyway before that if it breaks current gst.
mchehab: mripard: paulk-leonov: posted cedrus driver pull request.
Paul, it was a pleasure working with you on this! Thank you very much for all your work!
hverkuil1, yipee! That was a shared pleasure, thank-you for taking the time to guide me through all of this!
hverkuil1: awesome, thanks!
Hi, folks. Just trying to get some clues about power management on a video pipe. Actually it's Xilinx video pipe (xilinx-vipp.c).
Basically I have a chain of video blocks (AFAICT each one is an v4l2 entity and subdev). In short, should I power each block up/down in:
1) v4l2_internal_ops.close()/open()
2) v4l2_subdev_video_ops.s_stream()
3) v4l2_subdev_core_ops.s_power()
The latter seems the most appropriate, but I never see it called.. shuould I call v4l2_pipeline_pm_use() somewhere?
.. Thanks :)
paulk-leonov: regarding the discussion above about display/pixel aspect ratios encoded in an MPEG bitstream: how is that handled by cedrus? Did we think about that?
hverkuil1, mhh the aspect_ratio_information bitstream element is not part of the controls, but I'm wondering if it's relevant to have it there, since it might have more to do with presentation than decoding
if the issue is exposing it to userspace, it's definitely not an issue as the player will have parsed the bitstream anyway, so it has the info already and doesn't need the driver to report it back
(in the stateless vpu case)
Ah, true.
It's a different matter for stateful codecs.
But for stateless codecs it's a userspace issue, not a driver issue. Good point.
Congratz guys!
I just check my mail archive: the first patch series for "configuration store support" (predecessor of the Request API) was Jan 6th, 2014.
check -> checked
paulk-leonov: in your test application for the cedrus driver, do you use V4L2_MEMORY_MMAP or DMABUF for the VIDEO_OUTPUT buffers?
hverkuil1, that would be MMAP in that case, I don't think I ever tried DMABUF for output
so it has taken 4 years to get there, wow
I was wondering about the DMABUF case: v4l2_ctrl_mpeg2_slice_params provides buffer indices for backward and forward buffers. But what happens if the application would close the dmabuf fd of such a referenced buffer?
well, we assumed the buffers will stay intact between streamon and off, but maybe that's abusing the spec
does something special happen when there is no reference left to a buffer?
Looking at the driver code I see that the buffer indices refer to the capture buffer indices, not the output, right?
yes that's right
it's decoded pictures
And you provide the address of the referenced capture buffer to the HW. So I need to rephrase my question: do you use MMAP or DMABUF for capture buffers in your test application?
I think that if userspace closed the dmabuf, then vb2_dma_contig_plane_dma_addr() would return 0.
ah, I'm starting to understand -- the buffers are always created with MEMORY_MMAP in the test util
indeed
You'll have to test this.
also we need the buffers allocated in our CMA memory pool because it can only be in the lower 256 MiB of RAM
would it work to check the refcount of the reference buffers at request validation time?
The driver has to detect if the buffer was deleted in the DMABUF case, otherwise things will really go wrong.
right
OK, checked the vb2 code. AFAICS vb2_dma_contig_plane_dma_addr will return 0 if the buffer memory has been released. So you will have to check for that.
The other thing I didn't really like is that the reference buffer indices refer to the capture buffers, but the control is part of a request for the output queue.
This assumes that you can always predict what the capture buffer index will be. I wonder if it isn't better to refer to the output buffer index, and let the driver remember the mapping of output index to capture index.
This would also help in verifying stupid actions from userspace such as requeueing a reference capture buffer before it is no longer in use.
Never trust userspace, just think evil thoughts...
BTW, I suspect that it is impossible to use USERPTR streaming for these devices, since there is no way to know when a dequeued malloc-ed buffer is freed. Once it is dequeued you have no control over it anymore.
So the RFC for the stateless codec API (posted by Alexandre) says:
``CAPTURE`` buffers must not be part of the request, but must be queued independently. The driver will pick one of the queued ``CAPTURE`` buffers and decode the frame into it. Although the client has no control over which ``CAPTURE`` buffer will be used with a given ``OUTPUT`` buffer, it is guaranteed that ``CAPTURE`` buffers will be returned in decode order (i.e. the same order as ``OUTPUT`` buffers were submitted), so it is trivial to ass
ociate a dequeued ``CAPTURE`` buffer to its originating request and ``OUTPUT`` buffer.
So it would make sense to reference output indices, not capture indices as per this proposed spec.
okay I understand
so let's have output-capture association in the driver then
BTW, what happens if there is a bit error in the stream and the frame fails to decode? Is the ERROR buffer flag set in that case?
yes the flag is set
I don't see it?
it's set through the m2m helper IIRC
Ah, I got it.
What happens if this broken frame is also a reference frame for B/P frames?
They will fail as well?
currently, the data will be used nevertheless
it would probably make sense to discard all frames that use it as reference
So you just get broken frames? I think it would be a good idea to remember that the reference frame had an error and mark those that refer to it as error as well.
right
I agree
I'm not sure you should discard them, it is better to use it (you might still get a partial valid frame), but just mark as error.
oh ok, that works for me too
Userspace can decide whether or not to throw them away.
yes
looks like I have enough to make another follow-up series
That's why it's in staging :-)
hverkuil1: would that pixel aspect ratio just have to be returned by cropcap or it also affects some other parameters, such as crop rectangles?
Just cropcap.
The crop rectangles all refer to the pixels, so they don't care about the aspect ratio.
I thought so, but was confused by cropcap spec saying
"Default cropping rectangle, it shall cover the “whole picture”. Assuming pixel aspect 1/1 this could be for example a 640 × 480 rectangle for NTSC, a 768 × 576 rectangle for PAL and SECAM centered over the active picture area."
for defrect
Analog (i.e. composite) video has weird behavior which you can completely ignore here. For codecs all the selection rectangles are pixel based and are independent of the pixel aspect ratio.
okay, great, thanks :)
Basically with analog video there is no such thing as an official 'width': it's the digitizer that turns an analog signal into discrete pixels.
Sort of similar to asking what the bitrate is of an analog vinyl LP.
Luckily, since HW codecs are completely digital we can ignore all that.
paulk-leonov: I just realized that the reference frame dmabuf refcount has to be increased as well while decoding.
Otherwise userspace could close the dmabuf fd while the HW is still accessing the buffer.
We might need to add some vb2 support for that.
pH5: ping
paulk-leonov: I'm still not sure whether a buffer index that refers to a reference frame should be an output or capture queue index. I'll ask on the mailinglist.
hverkuil1, I guess I don't have any strong opinion on that, both are manageable
hverkuil1: pong
pH5: I assume that the dts patch for the pxp driver goes through a different tree?
hverkuil1: in Chrome OS, we have a hack for vb2 to keep buffers referenced until another buffer replaces it on the same index or reqbufs(0)
but technically one could still do the former while the hardware is still acessing it...
hverkuil1: yes, thanks. I'll ask Shawn to merge it.
we should bump reference count for the time of hardware access
hverkuil1: hi
ezequielg: hi, do you have a short question? Otherwise it is better to postpone it for an hour or so.
no short question, figured it would be easier to explain the jpeg 8-bit/16-bit thing here.
but i can send a mail
ok
hverkuil1: should it be possible to build adv7604 without CEC support?
the driver does not does not select CEC_CORE but uses cec functions unconditionally
hverkuil1: I found out why v4l2 compliance gives me a "VIDIOC_CREATE_BUFS returned -1 (Cannot allocate memory)" test fail
(it only happens for H264 and HEVC, which is why it's reported as OK in the amlogic vdec patch series)
v4l2-compliance calls create_bufs twice in a row on the capture queue, and it breaks the v4l2 queue buffer limit of 32.
This is because my driver has a minimum of 20 buffers for H.264 and HEVC, so it ends up requesting 40 buffers total -> second alloc fails.
Either this is a false positive, or is there a way to know in queue_setup if buffers have already been created ?
hverkuil1: the problem is that probe() fails with -22 if CEC_CORE is not enabled.
hverkuil1: it is due to this https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/adv7604.c#n2316
when CEC_CORE is not enabled spa_loc will always be 0
I'd argue that that should not be an error. If a EDID is loaded that does not support CEC, CEC should just not be enabled in the device
paulk-leonov: sorry, I know you gave it to me before, but what was the link to the cedrus test utility again?
larsc: Hmm, that's a bug.
hverkuil1: which part?
what I'd do is just make the whole CEC setup optional and skip it if CEC support is disabled
hverkuil1: https://github.com/bootlin/v4l2-request-test
mripard: thanks!
larsc: you need the offset to the SPA that cec_get_edid_phys_addr provides, regardless of whether CEC_CORE is set or not.
(adv7842 has the same issue)
cec_get_edid_phys_addr() is stubbed out when CEC_CORE is not enabled
returns always 0 in that case
I don't understand why I need the offset
the driver worked fine when there was no CEC support at all
and I guess in that case it wasn't handled either
ok, I see the spa location lookup function was part of the driver before the CEC framework
Yes, it was. The problem is that cec_get_edid_phys_addr() was part of the adv drivers so always there, now it is part of the CEC core and stubbed out if CEC_CORE is disabled. That's not good.
It is rarely needed, only for receivers that can have more than one input.
So that's adv7604/7842.
I'm inclined to just make adv-specific copies of that code again. I'll need to think about this a bit.
it used to work up until https://git.linuxtv.org/media_tree.git/commit/?id=56a263aaa0a5f58d70517fae2bdd63fc1e17efec
Yeah, makes sense.
cec-edid was its own module
can we maybe bring it back?
That's when the stubs were introduced. There were problems with this as a separate module as well. It's also very specific to HDMI receivers, transmitters do not need this information.
let me think about it, I'll get back to this later this week.
ok thanks
I think that finding the SPA location doesn't belong in the CEC core at all, I think it should be done in v4l2-dv-timings.c as a new helper function since this is entirely V4L2 specific.
Same for a few other related functions (cec_phys_addr_for_input)
hverkuil1: i've been testing vicodec, and noticed that state->info is always NULL for me. In fact, I can't see where it's set. But I know you are using this, so I'm kind of confused.
I have a local fix, but I suspect it might be plain wrong.
It's set in open() and s_fmt() through find_fmt or v4l2_fwht_get_pixfmt().
larsc: yeah, it's the right approach to move this to v4l2-dv-timings.
hm, perhaps I am missing some patches? I thought I had picked them all.
you're backporting to an older kernel?
no.
Last vicodec commits were August 31.
i am using http://ix.io/1moE on git://linuxtv.org/media_tree.git
testing with gstreamer v4l2fwhtenc ! v4l2fwhtdec
wait, state->info, not q_data->info. Sorry, I misread.
@#$!%@#%$@!!! Grrr, I tested this extensively with streaming in userspace (by copying the fwht codec code), but failed to test with 'v4l2-compliance -s'.
very embarrassed...
well, you'll now be able to test with gstreamer :-)
what's nice about this, is that you can test in qemu. using qemu wrappers such as virtme, makes the whole thing insanely easy.
hverkuil1: do you want us (ADI) to give this a shot?
virtme, launches a gstreamer sandbox with latest build, and a test pipeline to encode ! decode
larsc: yes, go ahead.
larsc: cec_phys_addr_validate, cec_phys_addr_for_input and cec_set_edid_phys_addr can all be moved out of cec-edid.c and into v4l2-dv-timings.c (rename prefix to v4l2_something since this now in a different module)
cec_get_edid_phys_addr should stay as-is for now in cec-edid, but there should be a new function in v4l2-dv-timings.c that returns the SPA location.
ok thanks
I think the cec_get_edid_phys_addr should be changed and drop the offset argument altogether, but that's something I will take care of later.
ezequielg: posted vicodec fixes.
ezequielg: can you test my vicodec fixes? If it works for you as well, then I'll make a pull request for is (it's a nasty bug)
as a matter of fact, I am doing exactly that.
ezequielg: be aware that vicodec is not fully compliant to the proposed stateful codec specification yet. I'm waiting until that's finalized before bringing it fully up to spec.
ah, noted. maybe that's why gstreamer misbehaves, see my mail.
ezequielg: I'm pretty sure that is indeed the reason why gstreamer misbehaves. That part isn't fully implemented yet.
if this is something you want to see working soon, then don't wait for me. I'm not sure when I'll get around to it, and I wasn't planning on starting on this until the codec spec is merged.
got it
i am happy to help, if i can