On 09/14/2018 01:56 AM, Hans Verkuil wrote:
On 09/13/2018 09:11 PM, Eddie James wrote:
On 09/03/2018 06:57 AM, Hans Verkuil wrote:
Hi Eddie,
Thank you for your work on this. Interesting to see support for this SoC :-)
On 08/29/2018 11:09 PM, Eddie James wrote:
The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
can capture and compress video data from digital or analog sources. With
the Aspeed chip acting as a service processor, the Video Engine can
capture the host processor graphics output.
This series adds a V4L2 driver for the VE, providing a read() interface
only. The driver triggers the hardware to capture the host graphics output
and compress it to JPEG format.
Testing on an AST2500 determined that the videobuf/streaming/mmap interface
was significantly slower than the simple read() interface, so I have not
included the streaming part.
Do you know why? It should be equal or faster, not slower.
Yes, it seems to be an issue with the timing of the video engine
interrupts compared with how a normal v4l2 application queues buffers.
With the simple read() application, the driver can swap between DMA
buffers freely and get a frame ahead. With the streaming buffers, I
found the driver ran through the queue quite quickly, but then, once
userspace queues again, we had to wait for the next frame, as I couldn't
get a frame ahead since no buffers were available during that time
period. This could possibly be solved with more buffers but this gets to
require a lot of memory, since each buffer is allocated for the full
frame size even though we only fill a fraction of it with JPEG data...
For stream I/O you usually need 3 buffers: one is being DMAed, one is
the next queued up frame for the DMA and the third is being processed
in userspace. If userspace doesn't process buffers fast enough, then
the driver will need to capture into the same buffer over and over again
until userspace finally queues another buffer.
What I don't understand here is what the frame rate is. Is the capture
framerate the same as the host processor graphics output? Or is it
unrelated to that?
Yes, the frame rate is the capture frame rate, unrelated to the host
graphics output frame rate. It's really just a bandwidth-saving measure.
The problem with read() is that 1) it requires copying the video data, and
2) you cannot use dmabuf for zero-copying pipelines. Whether or not 2 is
needed depends on your hardware.
I reviewed about half of the driver, but then I stopped since there were too
many things missing.
First of all, you need to test your driver with v4l2-compliance (available here:
https://git.linuxtv.org/v4l-utils.git/). Always compile from the git repo since
the versions from distros tend to be too old.
Just run 'v4l2-compliance -d /dev/videoX' and fix all issues. Then run
'v4l2-compliance -s -d /dev/videoX' to test streaming.
This utility checks if the driver follows the V4L2 API correctly, implements
all ioctls that it should and fills in all the fields that it should.
Please add the output of 'v4l2-compliance -s' to future versions of this patch
series: I don't accept V4L2 drivers without a clean report of this utility.
Sure thing. Thanks for the guidance.
If you have any questions, then mail me or (usually quicker) ask on the #v4l
freenode irc channel (I'm in the CET timezone).
One thing that needs more explanation: from what I could tell from the driver
the VIDIOC_G_FMT ioctl returns the detected format instead of the current
format. This is wrong. Instead you should implement the VIDIOC_*_DV_TIMINGS
ioctls and the V4L2_EVENT_SOURCE_CHANGE event.
The normal sequence is that userspace queries the current timings with
VIDIOC_QUERY_DV_TIMINGS, if it finds valid timings, then it sets these
timings with _S_DV_TIMINGS. Now it can call G/S_FMT. If the timings
change, then the driver should detect that and send a V4L2_EVENT_SOURCE_CHANGE
event.
OK I see. I ended up simplifying this part anyway since it's not
possible to change the video size from the driver. I don't think there
is a need for VIDIOC_QUERY_DV_TIMINGS now, but feel free to review.
You are capturing the host graphics output, right? So you need to know
the resolution, timings, etc. of that output? What if the host changes
resolution? That's something you need to know, or am I missing something?
Yes, that's why I had those extra lines in set_format, so that if the
set format resolution matches the actual frame size, then it can
change... I guess I'm probably mis-using the API. Will look into the
source event change and timings calls.
This is obviously a somewhat different environment than what I am used to,
so bear with me if I ask stupid questions...
No problem!
Thanks,
Eddie
Regards,
Hans
Thanks again,
Eddie
When the application receives this event it can take action, such as
increasing the size of the buffer for the jpeg data that it reads into.
The reason for this sequence of events is that you can't just change the
format/resolution mid-stream without giving userspace the chance to
reconfigure.
Regards,
Hans
It's also possible to use an automatic mode for the VE such that
re-triggering the HW every frame isn't necessary. However this wasn't
reliable on the AST2400, and probably used more CPU anyway due to excessive
interrupts. It was approximately 15% faster.
The series also adds the necessary parent clock definitions to the Aspeed
clock driver, with both a mux and clock divider.
Eddie James (4):
clock: aspeed: Add VIDEO reset index definition
clock: aspeed: Setup video engine clocking
dt-bindings: media: Add Aspeed Video Engine binding documentation
media: platform: Add Aspeed Video Engine driver
.../devicetree/bindings/media/aspeed-video.txt | 23 +
drivers/clk/clk-aspeed.c | 41 +-
drivers/media/platform/Kconfig | 8 +
drivers/media/platform/Makefile | 1 +
drivers/media/platform/aspeed-video.c | 1307 ++++++++++++++++++++
include/dt-bindings/clock/aspeed-clock.h | 1 +
6 files changed, 1379 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
create mode 100644 drivers/media/platform/aspeed-video.c
