|
|
Index: content/common/gpu/media/tegra_v4l2_video_device.cc |
diff --git a/content/common/gpu/media/tegra_v4l2_video_device.cc b/content/common/gpu/media/tegra_v4l2_video_device.cc |
new file mode 100644 |
index 0000000000000000000000000000000000000000..b7b7cddd14b0749d0719eeab03b1655814c0eeac |
--- /dev/null |
+++ b/content/common/gpu/media/tegra_v4l2_video_device.cc |
@@ -0,0 +1,172 @@ |
+#include <dlfcn.h> |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
missing copyright header
missing copyright header
|
+#include <errno.h> |
+#include <fcntl.h> |
+#include <libdrm/drm_fourcc.h> |
Pawel Osciak
2014/02/10 06:36:17
Not needed?
Not needed?
|
+#include <linux/videodev2.h> |
+#include <poll.h> |
Pawel Osciak
2014/02/10 06:36:17
Not needed.
Not needed.
|
+#include <sys/eventfd.h> |
Pawel Osciak
2014/02/10 06:36:17
Not needed.
Not needed.
|
+#include <sys/ioctl.h> |
+#include <sys/mman.h> |
Pawel Osciak
2014/02/10 06:36:17
Not needed?
Please clean up the headers in this f
Not needed?
Please clean up the headers in this file to a minimal set of required ones only
(also applies to the Chrome headers below).
|
+ |
+#include "base/bind.h" |
+#include "base/debug/trace_event.h" |
+#include "base/memory/shared_memory.h" |
+#include "base/message_loop/message_loop.h" |
+#include "base/message_loop/message_loop_proxy.h" |
+#include "base/posix/eintr_wrapper.h" |
+#include "content/common/gpu/media/tegra_v4l2_video_device.h" |
+#include "ui/gl/gl_bindings.h" |
+ |
+namespace content { |
+ |
+namespace { |
+const char kDevice[] = "/dev/video0"; |
Pawel Osciak
2014/02/10 06:36:17
Is this is the codec device exposed by Tegra kerne
Is this is the codec device exposed by Tegra kernel driver?
You can't assume it will be this on all configurations.
Please use udev rules to create a codec specific device (see Exynos example at
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
shivdasp
2014/02/10 13:31:17
This is a v4l2 decoder device name which we use to
This is a v4l2 decoder device name which we use to initialize a decoder context
within the libtegrav4l2 library.
This can be anything really as long as decoder and encoder device names are
different since we do not open a v4l2 video device underneath. Libtegrav4l2 is
really a pseudo implementation. I can change it /dev/tegra-dec and
/dev/tegra-enc for it to mean tegra specific.
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> Is this is the codec device exposed by Tegra kernel driver?
>
> You can't assume it will be this on all configurations.
> Please use udev rules to create a codec specific device (see Exynos example at
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
Pawel Osciak
2014/02/12 09:15:13
Which device is actually being used? Does the libr
On 2014/02/10 13:31:17, shivdasp wrote:
> This is a v4l2 decoder device name which we use to initialize a decoder
context
> within the libtegrav4l2 library.
> This can be anything really as long as decoder and encoder device names are
> different since we do not open a v4l2 video device underneath. Libtegrav4l2 is
> really a pseudo implementation. I can change it /dev/tegra-dec and
> /dev/tegra-enc for it to mean tegra specific.
Which device is actually being used? Does the library just talk to DRM driver
via custom ioctls?
>
> On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > Is this is the codec device exposed by Tegra kernel driver?
> >
> > You can't assume it will be this on all configurations.
> > Please use udev rules to create a codec specific device (see Exynos example
at
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
>
shivdasp
2014/02/12 10:11:55
This library internally talks to MM layer which ta
This library internally talks to MM layer which talks to the device
(/dev/tegra_avpchannel) which is the nvavp driver.
On 2014/02/12 09:15:13, Pawel Osciak wrote:
> On 2014/02/10 13:31:17, shivdasp wrote:
> > This is a v4l2 decoder device name which we use to initialize a decoder
> context
> > within the libtegrav4l2 library.
> > This can be anything really as long as decoder and encoder device names are
> > different since we do not open a v4l2 video device underneath. Libtegrav4l2
is
> > really a pseudo implementation. I can change it /dev/tegra-dec and
> > /dev/tegra-enc for it to mean tegra specific.
>
> Which device is actually being used? Does the library just talk to DRM driver
> via custom ioctls?
>
> >
> > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > Is this is the codec device exposed by Tegra kernel driver?
> > >
> > > You can't assume it will be this on all configurations.
> > > Please use udev rules to create a codec specific device (see Exynos
example
> at
> > >
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
> >
>
Pawel Osciak
2014/02/13 10:42:54
This means you will have to add it to sandbox rule
On 2014/02/12 10:11:55, shivdasp wrote:
> This library internally talks to MM layer which talks to the device
> (/dev/tegra_avpchannel) which is the nvavp driver.
This means you will have to add it to sandbox rules in Chrome, right? So the
library should actually use the device path string provided from Chrome to
Open() and not have the string hardcoded in the library please.
> On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > On 2014/02/10 13:31:17, shivdasp wrote:
> > > This is a v4l2 decoder device name which we use to initialize a decoder
> > context
> > > within the libtegrav4l2 library.
> > > This can be anything really as long as decoder and encoder device names
are
> > > different since we do not open a v4l2 video device underneath.
Libtegrav4l2
> is
> > > really a pseudo implementation. I can change it /dev/tegra-dec and
> > > /dev/tegra-enc for it to mean tegra specific.
> >
> > Which device is actually being used? Does the library just talk to DRM
driver
> > via custom ioctls?
> >
> > >
> > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > Is this is the codec device exposed by Tegra kernel driver?
> > > >
> > > > You can't assume it will be this on all configurations.
> > > > Please use udev rules to create a codec specific device (see Exynos
> example
> > at
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
> > >
> >
>
shivdasp
2014/02/14 03:06:45
Yes /dev/tegra_avpchannel will be added in the san
Yes /dev/tegra_avpchannel will be added in the sandbox whitelist for now.
Work is in progress to make this sandbox friendly in our MM stack. Most probably
by the time we get this change merged we should have completed it so even
whitelisting may not be required. In worst case we will whitelist it sandbox
code.
As I said earlier this "device name" sent to the library is just dummy for the
libtegrav4l2 to create a decoder instance. It just has to be different than the
encoder "device name".
Shall I just keep it "tegra_dec" and "tegra_enc" to not mislead it for something
like a true v4l2 device name ?
On 2014/02/13 10:42:54, Pawel Osciak wrote:
> On 2014/02/12 10:11:55, shivdasp wrote:
> > This library internally talks to MM layer which talks to the device
> > (/dev/tegra_avpchannel) which is the nvavp driver.
>
> This means you will have to add it to sandbox rules in Chrome, right? So the
> library should actually use the device path string provided from Chrome to
> Open() and not have the string hardcoded in the library please.
>
> > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > This is a v4l2 decoder device name which we use to initialize a decoder
> > > context
> > > > within the libtegrav4l2 library.
> > > > This can be anything really as long as decoder and encoder device names
> are
> > > > different since we do not open a v4l2 video device underneath.
> Libtegrav4l2
> > is
> > > > really a pseudo implementation. I can change it /dev/tegra-dec and
> > > > /dev/tegra-enc for it to mean tegra specific.
> > >
> > > Which device is actually being used? Does the library just talk to DRM
> driver
> > > via custom ioctls?
> > >
> > > >
> > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > Is this is the codec device exposed by Tegra kernel driver?
> > > > >
> > > > > You can't assume it will be this on all configurations.
> > > > > Please use udev rules to create a codec specific device (see Exynos
> > example
> > > at
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
> > > >
> > >
> >
>
Pawel Osciak
2014/02/14 07:36:10
Why it may not be required? How else would it be a
On 2014/02/14 03:06:45, shivdasp wrote:
> Yes /dev/tegra_avpchannel will be added in the sandbox whitelist for now.
> Work is in progress to make this sandbox friendly in our MM stack. Most
probably
> by the time we get this change merged we should have completed it so even
> whitelisting may not be required. In worst case we will whitelist it sandbox
> code.
Why it may not be required? How else would it be accessible if it's not
whitelisted?
> As I said earlier this "device name" sent to the library is just dummy for the
> libtegrav4l2 to create a decoder instance. It just has to be different than
the
> encoder "device name".
> Shall I just keep it "tegra_dec" and "tegra_enc" to not mislead it for
something
> like a true v4l2 device name ?
Please, the library has to open the device with the same name as this method
provides to it.
> On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > On 2014/02/12 10:11:55, shivdasp wrote:
> > > This library internally talks to MM layer which talks to the device
> > > (/dev/tegra_avpchannel) which is the nvavp driver.
> >
> > This means you will have to add it to sandbox rules in Chrome, right? So the
> > library should actually use the device path string provided from Chrome to
> > Open() and not have the string hardcoded in the library please.
> >
> > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > This is a v4l2 decoder device name which we use to initialize a
decoder
> > > > context
> > > > > within the libtegrav4l2 library.
> > > > > This can be anything really as long as decoder and encoder device
names
> > are
> > > > > different since we do not open a v4l2 video device underneath.
> > Libtegrav4l2
> > > is
> > > > > really a pseudo implementation. I can change it /dev/tegra-dec and
> > > > > /dev/tegra-enc for it to mean tegra specific.
> > > >
> > > > Which device is actually being used? Does the library just talk to DRM
> > driver
> > > > via custom ioctls?
> > > >
> > > > >
> > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > Is this is the codec device exposed by Tegra kernel driver?
> > > > > >
> > > > > > You can't assume it will be this on all configurations.
> > > > > > Please use udev rules to create a codec specific device (see Exynos
> > > example
> > > > at
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
> > > > >
> > > >
> > >
> >
>
shivdasp
2014/02/14 09:18:58
Whitelisted will not be required because of pre-ac
Whitelisted will not be required because of pre-acquire the resources (including
open() for /dev/tegra_avpchannel) in the libtegrav4l2.so which we load before
the sandbox will kick in.
Hence the only place we will have this device name will be in TegraV4L2Device
which will be used to open the decoder instance. (The pre-opened fd will be
used).
Okay I will keep the same device name (/dev/tegra_avpchannel).
On 2014/02/14 07:36:10, Pawel Osciak wrote:
> On 2014/02/14 03:06:45, shivdasp wrote:
> > Yes /dev/tegra_avpchannel will be added in the sandbox whitelist for now.
> > Work is in progress to make this sandbox friendly in our MM stack. Most
> probably
> > by the time we get this change merged we should have completed it so even
> > whitelisting may not be required. In worst case we will whitelist it sandbox
> > code.
>
> Why it may not be required? How else would it be accessible if it's not
> whitelisted?
>
> > As I said earlier this "device name" sent to the library is just dummy for
the
> > libtegrav4l2 to create a decoder instance. It just has to be different than
> the
> > encoder "device name".
> > Shall I just keep it "tegra_dec" and "tegra_enc" to not mislead it for
> something
> > like a true v4l2 device name ?
>
> Please, the library has to open the device with the same name as this method
> provides to it.
>
> > On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > > On 2014/02/12 10:11:55, shivdasp wrote:
> > > > This library internally talks to MM layer which talks to the device
> > > > (/dev/tegra_avpchannel) which is the nvavp driver.
> > >
> > > This means you will have to add it to sandbox rules in Chrome, right? So
the
> > > library should actually use the device path string provided from Chrome to
> > > Open() and not have the string hardcoded in the library please.
> > >
> > > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > > This is a v4l2 decoder device name which we use to initialize a
> decoder
> > > > > context
> > > > > > within the libtegrav4l2 library.
> > > > > > This can be anything really as long as decoder and encoder device
> names
> > > are
> > > > > > different since we do not open a v4l2 video device underneath.
> > > Libtegrav4l2
> > > > is
> > > > > > really a pseudo implementation. I can change it /dev/tegra-dec and
> > > > > > /dev/tegra-enc for it to mean tegra specific.
> > > > >
> > > > > Which device is actually being used? Does the library just talk to DRM
> > > driver
> > > > > via custom ioctls?
> > > > >
> > > > > >
> > > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > > Is this is the codec device exposed by Tegra kernel driver?
> > > > > > >
> > > > > > > You can't assume it will be this on all configurations.
> > > > > > > Please use udev rules to create a codec specific device (see
Exynos
> > > > example
> > > > > at
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/HEAD/o...)
> > > > > >
> > > > >
> > > >
> > >
> >
>
|
+} |
+ |
+TegraV4L2Device::TegraV4L2Device(EGLContext egl_context) |
+ : device_fd_(-1), egl_context_(egl_context) {} |
+ |
+TegraV4L2Device::~TegraV4L2Device() { |
+ if (device_fd_ != -1) { |
+ TegraV4L2Close(device_fd_); |
+ device_fd_ = -1; |
+ } |
+} |
+ |
+int TegraV4L2Device::Ioctl(int flags, void* arg) { |
+ return TegraV4L2Ioctl(device_fd_, flags, arg); |
+} |
+ |
+bool TegraV4L2Device::Poll(bool poll_device, bool* event_pending) { |
+ if (TegraV4L2Poll(device_fd_, poll_device, event_pending) == -1) { |
+ DLOG(ERROR) << "TegraV4L2Poll returned -1 "; |
+ return false; |
+ } |
+ return true; |
+} |
+ |
+void* TegraV4L2Device::Mmap(void* addr, |
+ unsigned int len, |
+ int prot, |
+ int flags, |
+ unsigned int offset) { |
+ // No real mmap for tegrav4l2 device, the offset is itself the address. |
+ return (void*)offset; |
Pawel Osciak
2014/02/10 06:36:17
QUERYBUFS from V4L2VDA is used to acquire offsets
QUERYBUFS from V4L2VDA is used to acquire offsets for mmap, but from
CreateEGLImage to pass them for texture binding to the library? I'm guessing
that this works, because for the former you call QUERYBUFS with OUTPUT buffer,
while in the latter you call it with CAPTURE?
Please don't do this. Suggested changes for EGLImages in my comments below.
As for mmap, how (and from what) is the memory mapping actually acquired for the
buffer in the library? Why do this on QUERYBUFS and not on Mmap()? How is it
managed and when it is destroyed?
shivdasp
2014/02/10 13:31:17
When REQBUFS is called for OUTPUT_PLANE, the libra
When REQBUFS is called for OUTPUT_PLANE, the library creates internal buffers
which can be shared with the AVP for decoding. AVP is a video processor which
runs the firmware that actually does all the decoding. While creating this
buffers, they are already mmapped to get a virtual address. The library returns
this address in QUERYBUF API. Hence there is not real need for mmap.
When REQBUFS with 0 is called on OUTPUT_PLANE, these buffers are internally
unmapped and destroyed.
I will explain the need for CreateEGLImage in later comments.
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> QUERYBUFS from V4L2VDA is used to acquire offsets for mmap, but from
> CreateEGLImage to pass them for texture binding to the library? I'm guessing
> that this works, because for the former you call QUERYBUFS with OUTPUT buffer,
> while in the latter you call it with CAPTURE?
>
> Please don't do this. Suggested changes for EGLImages in my comments below.
>
> As for mmap, how (and from what) is the memory mapping actually acquired for
the
> buffer in the library? Why do this on QUERYBUFS and not on Mmap()? How is it
> managed and when it is destroyed?
Pawel Osciak
2014/02/12 09:15:13
The library must be using some kind of an mmap cal
On 2014/02/10 13:31:17, shivdasp wrote:
> When REQBUFS is called for OUTPUT_PLANE, the library creates internal buffers
> which can be shared with the AVP for decoding. AVP is a video processor which
> runs the firmware that actually does all the decoding. While creating this
> buffers, they are already mmapped to get a virtual address. The library
returns
> this address in QUERYBUF API. Hence there is not real need for mmap.
> When REQBUFS with 0 is called on OUTPUT_PLANE, these buffers are internally
> unmapped and destroyed.
The library must be using some kind of an mmap call to get a mapping though.
Would it be possible to move it to be done on this call, as expected?
Also, how will this work for VEA, where CAPTURE buffers need to be mapped
instead?
> I will explain the need for CreateEGLImage in later comments.
>
> On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > QUERYBUFS from V4L2VDA is used to acquire offsets for mmap, but from
> > CreateEGLImage to pass them for texture binding to the library? I'm guessing
> > that this works, because for the former you call QUERYBUFS with OUTPUT
buffer,
> > while in the latter you call it with CAPTURE?
> >
> > Please don't do this. Suggested changes for EGLImages in my comments below.
> >
> > As for mmap, how (and from what) is the memory mapping actually acquired for
> the
> > buffer in the library? Why do this on QUERYBUFS and not on Mmap()? How is it
> > managed and when it is destroyed?
>
shivdasp
2014/02/12 10:11:55
Okay I will add Mmap and Munmap calls to the libra
Okay I will add Mmap and Munmap calls to the library and have it return the
appropriate value internally.
On 2014/02/12 09:15:13, Pawel Osciak wrote:
> On 2014/02/10 13:31:17, shivdasp wrote:
> > When REQBUFS is called for OUTPUT_PLANE, the library creates internal
buffers
> > which can be shared with the AVP for decoding. AVP is a video processor
which
> > runs the firmware that actually does all the decoding. While creating this
> > buffers, they are already mmapped to get a virtual address. The library
> returns
> > this address in QUERYBUF API. Hence there is not real need for mmap.
> > When REQBUFS with 0 is called on OUTPUT_PLANE, these buffers are internally
> > unmapped and destroyed.
>
> The library must be using some kind of an mmap call to get a mapping though.
> Would it be possible to move it to be done on this call, as expected?
>
> Also, how will this work for VEA, where CAPTURE buffers need to be mapped
> instead?
>
> > I will explain the need for CreateEGLImage in later comments.
> >
> > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > QUERYBUFS from V4L2VDA is used to acquire offsets for mmap, but from
> > > CreateEGLImage to pass them for texture binding to the library? I'm
guessing
> > > that this works, because for the former you call QUERYBUFS with OUTPUT
> buffer,
> > > while in the latter you call it with CAPTURE?
> > >
> > > Please don't do this. Suggested changes for EGLImages in my comments
below.
> > >
> > > As for mmap, how (and from what) is the memory mapping actually acquired
for
> > the
> > > buffer in the library? Why do this on QUERYBUFS and not on Mmap()? How is
it
> > > managed and when it is destroyed?
> >
>
Pawel Osciak
2014/02/13 10:42:54
Great. Thank you!
On 2014/02/12 10:11:55, shivdasp wrote:
> Okay I will add Mmap and Munmap calls to the library and have it return the
> appropriate value internally.
>
Great. Thank you!
> On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > On 2014/02/10 13:31:17, shivdasp wrote:
> > > When REQBUFS is called for OUTPUT_PLANE, the library creates internal
> buffers
> > > which can be shared with the AVP for decoding. AVP is a video processor
> which
> > > runs the firmware that actually does all the decoding. While creating this
> > > buffers, they are already mmapped to get a virtual address. The library
> > returns
> > > this address in QUERYBUF API. Hence there is not real need for mmap.
> > > When REQBUFS with 0 is called on OUTPUT_PLANE, these buffers are
internally
> > > unmapped and destroyed.
> >
> > The library must be using some kind of an mmap call to get a mapping though.
> > Would it be possible to move it to be done on this call, as expected?
> >
> > Also, how will this work for VEA, where CAPTURE buffers need to be mapped
> > instead?
> >
> > > I will explain the need for CreateEGLImage in later comments.
> > >
> > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > QUERYBUFS from V4L2VDA is used to acquire offsets for mmap, but from
> > > > CreateEGLImage to pass them for texture binding to the library? I'm
> guessing
> > > > that this works, because for the former you call QUERYBUFS with OUTPUT
> > buffer,
> > > > while in the latter you call it with CAPTURE?
> > > >
> > > > Please don't do this. Suggested changes for EGLImages in my comments
> below.
> > > >
> > > > As for mmap, how (and from what) is the memory mapping actually acquired
> for
> > > the
> > > > buffer in the library? Why do this on QUERYBUFS and not on Mmap()? How
is
> it
> > > > managed and when it is destroyed?
> > >
> >
>
|
+} |
+ |
+void TegraV4L2Device::Munmap(void* addr, unsigned int len) { |
+ // No real munmap for tegrav4l2 device. |
Pawel Osciak
2014/02/10 06:36:17
If so, how is unmapping handled then? What if we w
If so, how is unmapping handled then? What if we want to free the buffers and
reallocate them? You cannot call REQBUFS(0) without unmapping the buffers
first...
shivdasp
2014/02/10 13:31:17
Buffers are unmapped in REQBUFS(0) call and destro
Buffers are unmapped in REQBUFS(0) call and destroyed.
Since there is no real need for mmap and munmap, we did not implement it in the
library.
So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and mmaps the
buffer whereas REQBUF(0) unmaps and destroys them.
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> If so, how is unmapping handled then? What if we want to free the buffers and
> reallocate them? You cannot call REQBUFS(0) without unmapping the buffers
> first...
Pawel Osciak
2014/02/12 09:15:13
We should not rely on V4L2VDA to be the only place
On 2014/02/10 13:31:17, shivdasp wrote:
> Buffers are unmapped in REQBUFS(0) call and destroyed.
> Since there is no real need for mmap and munmap, we did not implement it in
the
> library.
> So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and mmaps the
> buffer whereas REQBUF(0) unmaps and destroys them.
We should not rely on V4L2VDA to be the only place where the underlying memory
will be destroyed. Even if we REQBUFS(0) on these buffers, the renderer process
may still be keeping ownership of the textures bound to them. Is this taken into
account?
>
> On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > If so, how is unmapping handled then? What if we want to free the buffers
and
> > reallocate them? You cannot call REQBUFS(0) without unmapping the buffers
> > first...
>
shivdasp
2014/02/12 10:11:55
If not in REQBUFS(0) then what will be the appropr
If not in REQBUFS(0) then what will be the appropriate place to destroy the
buffers ?
V4L2VDA::DestroyOutputBuffers() calls REQBUFS(0) and hence we destroy it there.
How does the renderer then inform the ownership of textures ?
On 2014/02/12 09:15:13, Pawel Osciak wrote:
> On 2014/02/10 13:31:17, shivdasp wrote:
> > Buffers are unmapped in REQBUFS(0) call and destroyed.
> > Since there is no real need for mmap and munmap, we did not implement it in
> the
> > library.
> > So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and mmaps the
> > buffer whereas REQBUF(0) unmaps and destroys them.
>
> We should not rely on V4L2VDA to be the only place where the underlying memory
> will be destroyed. Even if we REQBUFS(0) on these buffers, the renderer
process
> may still be keeping ownership of the textures bound to them. Is this taken
into
> account?
>
> >
> > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > If so, how is unmapping handled then? What if we want to free the buffers
> and
> > > reallocate them? You cannot call REQBUFS(0) without unmapping the buffers
> > > first...
> >
>
Pawel Osciak
2014/02/13 10:42:54
Sorry, perhaps I wasn't clear, the term "buffers"
On 2014/02/12 10:11:55, shivdasp wrote:
> If not in REQBUFS(0) then what will be the appropriate place to destroy the
> buffers ?
Sorry, perhaps I wasn't clear, the term "buffers" is a bit overloaded. I mean
the underlying memory. REQBUFS(0) may be called, but the actual memory that
backed the v4l2_buffers may have to live on if it's still tied to the textures.
This will be a common case actually, because we don't explicitly destroy
textures first unless they are dismissed. The memory should be then freed when
the textures are deleted, not on REQBUFS(0). I'm wondering if the library/driver
take this into account.
Of course, it's still possible for REQUBFS(0) to have to trigger destruction of
underlying memory, in case the textures get unbound and deleted before
REQBUFS(0) is called.
> V4L2VDA::DestroyOutputBuffers() calls REQBUFS(0) and hence we destroy it
there.
> How does the renderer then inform the ownership of textures ?
glDeleteTextures(). So the textures and the underlying memory may have to
outlive REQBUFS(0).
>
>
>
> On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > On 2014/02/10 13:31:17, shivdasp wrote:
> > > Buffers are unmapped in REQBUFS(0) call and destroyed.
> > > Since there is no real need for mmap and munmap, we did not implement it
in
> > the
> > > library.
> > > So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and mmaps
the
> > > buffer whereas REQBUF(0) unmaps and destroys them.
> >
> > We should not rely on V4L2VDA to be the only place where the underlying
memory
> > will be destroyed. Even if we REQBUFS(0) on these buffers, the renderer
> process
> > may still be keeping ownership of the textures bound to them. Is this taken
> into
> > account?
> >
> > >
> > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > If so, how is unmapping handled then? What if we want to free the
buffers
> > and
> > > > reallocate them? You cannot call REQBUFS(0) without unmapping the
buffers
> > > > first...
> > >
> >
>
shivdasp
2014/02/14 03:06:45
Understood. I believe the dmabuf export mechanism
Understood. I believe the dmabuf export mechanism takes care of this in Exynos
since the buffer backed memory is in kernel and hence the deletion is kind of
synchronized.
I see in DestroyOutputbuffers(), before calling DismissPicture(), the
eglDestroyImageKHR() is called thereby the textures are unbound or perhaps the
deletion there is also delayed ?
How is the texture being rendered if the eglImage it is bound to is also
destroyed ?
On 2014/02/13 10:42:54, Pawel Osciak wrote:
> On 2014/02/12 10:11:55, shivdasp wrote:
> > If not in REQBUFS(0) then what will be the appropriate place to destroy the
> > buffers ?
>
> Sorry, perhaps I wasn't clear, the term "buffers" is a bit overloaded. I mean
> the underlying memory. REQBUFS(0) may be called, but the actual memory that
> backed the v4l2_buffers may have to live on if it's still tied to the
textures.
> This will be a common case actually, because we don't explicitly destroy
> textures first unless they are dismissed. The memory should be then freed when
> the textures are deleted, not on REQBUFS(0). I'm wondering if the
library/driver
> take this into account.
> Of course, it's still possible for REQUBFS(0) to have to trigger destruction
of
> underlying memory, in case the textures get unbound and deleted before
> REQBUFS(0) is called.
>
> > V4L2VDA::DestroyOutputBuffers() calls REQBUFS(0) and hence we destroy it
> there.
> > How does the renderer then inform the ownership of textures ?
>
> glDeleteTextures(). So the textures and the underlying memory may have to
> outlive REQBUFS(0).
>
> >
> >
> >
> > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > Buffers are unmapped in REQBUFS(0) call and destroyed.
> > > > Since there is no real need for mmap and munmap, we did not implement it
> in
> > > the
> > > > library.
> > > > So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and mmaps
> the
> > > > buffer whereas REQBUF(0) unmaps and destroys them.
> > >
> > > We should not rely on V4L2VDA to be the only place where the underlying
> memory
> > > will be destroyed. Even if we REQBUFS(0) on these buffers, the renderer
> > process
> > > may still be keeping ownership of the textures bound to them. Is this
taken
> > into
> > > account?
> > >
> > > >
> > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > If so, how is unmapping handled then? What if we want to free the
> buffers
> > > and
> > > > > reallocate them? You cannot call REQBUFS(0) without unmapping the
> buffers
> > > > > first...
> > > >
> > >
> >
>
Pawel Osciak
2014/02/14 07:36:10
Yes, although it's not "synchronized", but the own
On 2014/02/14 03:06:45, shivdasp wrote:
> Understood. I believe the dmabuf export mechanism takes care of this in Exynos
> since the buffer backed memory is in kernel and hence the deletion is kind of
> synchronized.
Yes, although it's not "synchronized", but the ownership is managed and
refcounted in the driver and the memory is freed when there is no more users.
How does Tegra manage this?
>
> I see in DestroyOutputbuffers(), before calling DismissPicture(), the
> eglDestroyImageKHR() is called thereby the textures are unbound or perhaps the
> deletion there is also delayed ?
Destroying the image doesn't destroy the textures, but it does unbind them.
> How is the texture being rendered if the eglImage it is bound to is also
> destroyed ?
Texture is a separate entity from the eglImage, they share the underlying memory
after binding, but after eglImage is destroyed, the memory is not freed and it
lives on in the texture.
>
> On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > On 2014/02/12 10:11:55, shivdasp wrote:
> > > If not in REQBUFS(0) then what will be the appropriate place to destroy
the
> > > buffers ?
> >
> > Sorry, perhaps I wasn't clear, the term "buffers" is a bit overloaded. I
mean
> > the underlying memory. REQBUFS(0) may be called, but the actual memory that
> > backed the v4l2_buffers may have to live on if it's still tied to the
> textures.
> > This will be a common case actually, because we don't explicitly destroy
> > textures first unless they are dismissed. The memory should be then freed
when
> > the textures are deleted, not on REQBUFS(0). I'm wondering if the
> library/driver
> > take this into account.
> > Of course, it's still possible for REQUBFS(0) to have to trigger destruction
> of
> > underlying memory, in case the textures get unbound and deleted before
> > REQBUFS(0) is called.
> >
> > > V4L2VDA::DestroyOutputBuffers() calls REQBUFS(0) and hence we destroy it
> > there.
> > > How does the renderer then inform the ownership of textures ?
> >
> > glDeleteTextures(). So the textures and the underlying memory may have to
> > outlive REQBUFS(0).
> >
> > >
> > >
> > >
> > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > Buffers are unmapped in REQBUFS(0) call and destroyed.
> > > > > Since there is no real need for mmap and munmap, we did not implement
it
> > in
> > > > the
> > > > > library.
> > > > > So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and
mmaps
> > the
> > > > > buffer whereas REQBUF(0) unmaps and destroys them.
> > > >
> > > > We should not rely on V4L2VDA to be the only place where the underlying
> > memory
> > > > will be destroyed. Even if we REQBUFS(0) on these buffers, the renderer
> > > process
> > > > may still be keeping ownership of the textures bound to them. Is this
> taken
> > > into
> > > > account?
> > > >
> > > > >
> > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > If so, how is unmapping handled then? What if we want to free the
> > buffers
> > > > and
> > > > > > reallocate them? You cannot call REQBUFS(0) without unmapping the
> > buffers
> > > > > > first...
> > > > >
> > > >
> > >
> >
>
shivdasp
2014/02/14 09:18:58
I checked with the graphics team here. This is han
On 2014/02/14 07:36:10, Pawel Osciak wrote:
> On 2014/02/14 03:06:45, shivdasp wrote:
> > Understood. I believe the dmabuf export mechanism takes care of this in
Exynos
> > since the buffer backed memory is in kernel and hence the deletion is kind
of
> > synchronized.
>
> Yes, although it's not "synchronized", but the ownership is managed and
> refcounted in the driver and the memory is freed when there is no more users.
> How does Tegra manage this?
I checked with the graphics team here. This is handled. Since the memory for
EglImage is refcounted and is backed-up by the graphics library itself, the
texture bound to a destroyed eglImage can still be rendered. The V4L2 REQBUFS(0)
shall de-allocate only the buffer allocated by the "actual decoder" which are
the YUV buffers and since there is no more conversion happening after REQBUFS(0)
this is handled too.
>
> >
> > I see in DestroyOutputbuffers(), before calling DismissPicture(), the
> > eglDestroyImageKHR() is called thereby the textures are unbound or perhaps
the
> > deletion there is also delayed ?
>
> Destroying the image doesn't destroy the textures, but it does unbind them.
>
> > How is the texture being rendered if the eglImage it is bound to is also
> > destroyed ?
>
> Texture is a separate entity from the eglImage, they share the underlying
memory
> after binding, but after eglImage is destroyed, the memory is not freed and it
> lives on in the texture.
>
> >
> > On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > > On 2014/02/12 10:11:55, shivdasp wrote:
> > > > If not in REQBUFS(0) then what will be the appropriate place to destroy
> the
> > > > buffers ?
> > >
> > > Sorry, perhaps I wasn't clear, the term "buffers" is a bit overloaded. I
> mean
> > > the underlying memory. REQBUFS(0) may be called, but the actual memory
that
> > > backed the v4l2_buffers may have to live on if it's still tied to the
> > textures.
> > > This will be a common case actually, because we don't explicitly destroy
> > > textures first unless they are dismissed. The memory should be then freed
> when
> > > the textures are deleted, not on REQBUFS(0). I'm wondering if the
> > library/driver
> > > take this into account.
> > > Of course, it's still possible for REQUBFS(0) to have to trigger
destruction
> > of
> > > underlying memory, in case the textures get unbound and deleted before
> > > REQBUFS(0) is called.
> > >
> > > > V4L2VDA::DestroyOutputBuffers() calls REQBUFS(0) and hence we destroy it
> > > there.
> > > > How does the renderer then inform the ownership of textures ?
> > >
> > > glDeleteTextures(). So the textures and the underlying memory may have to
> > > outlive REQBUFS(0).
> > >
> > > >
> > > >
> > > >
> > > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > > Buffers are unmapped in REQBUFS(0) call and destroyed.
> > > > > > Since there is no real need for mmap and munmap, we did not
implement
> it
> > > in
> > > > > the
> > > > > > library.
> > > > > > So our implementation for REQBUF(x) on OUTPUT_PLANE allocates and
> mmaps
> > > the
> > > > > > buffer whereas REQBUF(0) unmaps and destroys them.
> > > > >
> > > > > We should not rely on V4L2VDA to be the only place where the
underlying
> > > memory
> > > > > will be destroyed. Even if we REQBUFS(0) on these buffers, the
renderer
> > > > process
> > > > > may still be keeping ownership of the textures bound to them. Is this
> > taken
> > > > into
> > > > > account?
> > > > >
> > > > > >
> > > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > > If so, how is unmapping handled then? What if we want to free the
> > > buffers
> > > > > and
> > > > > > > reallocate them? You cannot call REQBUFS(0) without unmapping the
> > > buffers
> > > > > > > first...
> > > > > >
> > > > >
> > > >
> > >
> >
>
|
+ return; |
+} |
+ |
+bool TegraV4L2Device::SetDevicePollInterrupt(void) { |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
drop "void"
drop "void"
|
+ if (HANDLE_EINTR(TegraV4L2SetDevicePollInterrupt(device_fd_) == -1)) { |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
bug: HANDLE_EINTR should not be accepting the ==-1
bug: HANDLE_EINTR should not be accepting the ==-1 as part of its arg.
|
+ DLOG(ERROR) << "Error in calling TegraV4L2SetDevicePollInterrupt"; |
+ return false; |
+ } |
+ return true; |
+} |
+ |
+bool TegraV4L2Device::ClearDevicePollInterrupt(void) { |
+ if (HANDLE_EINTR(TegraV4L2ClearDevicePollInterrupt(device_fd_) == -1)) { |
+ DLOG(ERROR) << "Error in calling TegraV4L2ClearDevicePollInterrupt"; |
+ return false; |
+ } |
+ return true; |
+} |
+ |
+bool TegraV4L2Device::Initialize(void) { |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
drop "void"
drop "void"
|
+ TegraV4L2Open = reinterpret_cast<TegraV4L2OpenFunc>( |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
Please avoid the sort of code duplication below (s
Please avoid the sort of code duplication below (see vaapi_wrapper.cc for
example of dlsym'ing).
shivdasp
2014/02/10 13:31:17
Yes I will do this. That looks very clean.
On 2014
Yes I will do this. That looks very clean.
On 2014/02/07 09:09:30, Ami Fischman wrote:
> Please avoid the sort of code duplication below (see vaapi_wrapper.cc for
> example of dlsym'ing).
|
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_open")); |
+ if (TegraV4L2Open == NULL) { |
+ DLOG(ERROR) << "Unable to get Tegra_v4l2_open handle "; |
+ return false; |
+ } |
+ |
+ TegraV4L2Ioctl = reinterpret_cast<TegraV4L2IoctlFunc>( |
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_ioctl")); |
+ if (TegraV4L2Ioctl == NULL) { |
+ DLOG(ERROR) << "Unable to get tegra_v4l2_ioctl handle "; |
+ return false; |
+ } |
+ |
+ TegraV4L2Close = reinterpret_cast<TegraV4L2CloseFunc>( |
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_close")); |
+ if (TegraV4L2Close == NULL) { |
+ DLOG(ERROR) << "Unable to get tegra_v4l2_close handle "; |
+ return false; |
+ } |
+ |
+ TegraV4L2Poll = reinterpret_cast<TegraV4L2PollFunc>( |
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_poll")); |
+ if (TegraV4L2Poll == NULL) { |
+ DLOG(ERROR) << "Unable to get tegra_v4l2_poll handle "; |
+ return false; |
+ } |
+ |
+ TegraV4L2SetDevicePollInterrupt = |
+ reinterpret_cast<TegraV4L2SetDevicePollInterruptFunc>( |
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_set_device_poll_interrupt")); |
+ if (TegraV4L2SetDevicePollInterrupt == NULL) { |
+ DLOG(ERROR) << "Unable to get tegra_v4l2_set_device_poll_interrupt handle"; |
+ return false; |
+ } |
+ |
+ TegraV4L2ClearDevicePollInterrupt = |
+ reinterpret_cast<TegraV4L2ClearDevicePollInterruptFunc>( |
+ dlsym(RTLD_DEFAULT, "tegra_v4l2_clear_device_poll_interrupt")); |
+ if (TegraV4L2ClearDevicePollInterrupt == NULL) { |
+ DLOG(ERROR) |
+ << "Unable to get tegra_v4l2_clear_device_poll_interrupt handle "; |
+ return false; |
+ } |
+ |
+ device_fd_ = |
+ HANDLE_EINTR(TegraV4L2Open(kDevice, O_RDWR | O_NONBLOCK | O_CLOEXEC)); |
+ if (device_fd_ == -1) { |
+ DLOG(ERROR) << "Unable to open tegra_v4l2_open "; |
+ return false; |
+ } |
+ return true; |
+} |
+ |
+EGLImageKHR TegraV4L2Device::CreateEGLImage(EGLDisplay egl_display, |
+ EGLint attrib[], |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
EGLint[] /* attrib */
EGLint[] /* attrib */
|
+ unsigned int texture_id, |
+ unsigned int buffer_index) { |
Pawel Osciak
2014/02/10 06:36:17
This method should take a v4l2_buffer instead.
De
This method should take a v4l2_buffer instead.
Depending on format and other circumstances format, memory type, etc. change. We
shouldn't hardcode this in the device class.
shivdasp
2014/02/10 13:31:17
Since ExynosV4L2Device does not need the v4l2_buff
Since ExynosV4L2Device does not need the v4l2_buffer like TegraV4L2Device, I
thought it would add less noise to the V4L2VDA code.
Also the fields within the v4l2_buffer (the eglimage handle) is created within
this method so can't fully initialize the structure here.
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> This method should take a v4l2_buffer instead.
> Depending on format and other circumstances format, memory type, etc. change.
We
> shouldn't hardcode this in the device class.
|
+ // Ignore the attributes sent to us since Tegra does not need those. |
+ EGLint attr = EGL_NONE; |
+ EGLImageKHR egl_image = eglCreateImageKHR(egl_display, |
+ egl_context_, |
+ EGL_GL_TEXTURE_2D_KHR, |
+ (EGLClientBuffer)(texture_id), |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
static_cast
static_cast
|
+ &attr); |
+ if (egl_image == EGL_NO_IMAGE_KHR) { |
+ DLOG(ERROR) << "CreateEGLImage(): could not create EGLImageKHR"; |
+ return egl_image; |
+ } |
+ |
+ struct v4l2_plane planes[2]; |
+ struct v4l2_buffer capture_buffer; |
+ |
+ memset(&capture_buffer, 0, sizeof(capture_buffer)); |
+ memset(planes, 0, sizeof(planes)); |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
swap w/ previous line to match declaration order
swap w/ previous line to match declaration order
|
+ capture_buffer.index = buffer_index; |
+ capture_buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; |
+ capture_buffer.memory = V4L2_MEMORY_MMAP; |
+ capture_buffer.m.planes = planes; |
+ capture_buffer.length = 2; |
Pawel Osciak
2014/02/10 06:36:17
arraysize(planes)
arraysize(planes)
|
+ // We send the EGLImage handle in mem_offset for the library to perform |
+ // conversion internally |
+ |
+ planes[0].m.mem_offset = reinterpret_cast<unsigned int>(egl_image); |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
how wide is mem_offset? I'm worried about this ca
how wide is mem_offset? I'm worried about this cast on a 64-bit platform.
If mem_offset is intptr_t-sized, please use that in the cast.
Pawel Osciak
2014/02/10 06:36:17
mem_offset is u32 always, an unsigned int shouldn'
mem_offset is u32 always, an unsigned int shouldn't be assigned to it. Also,
there are two planes, but passing only one offset is a bit inconsistent.
Although, why have two planes, if only one is used?
shivdasp
2014/02/10 13:31:17
We are really passing in the EglImage handle here
We are really passing in the EglImage handle here to the library. The library
associates this with the corresponding v4l2_buffer on the CAPTURE plane and use
the underlying conversion APIs to transform the decoder's yuv output into the
egl image.
We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where the number
of planes are checked with 2 (line #1660 of V4L2VDA).
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> mem_offset is u32 always, an unsigned int shouldn't be assigned to it. Also,
> there are two planes, but passing only one offset is a bit inconsistent.
> Although, why have two planes, if only one is used?
Pawel Osciak
2014/02/12 09:15:13
EGLImageKHR is typedef void*, which can be 64 bit.
On 2014/02/10 13:31:17, shivdasp wrote:
> We are really passing in the EglImage handle here to the library.
EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed in an u32
variable.
The whole idea behind offsets is that they are usually not really offsets, but
sort of platform-independent 32bit IDs. They are acquired from the V4L2 driver
(or library) via QUERYBUFS, and can be passed back to other calls to uniquely
identify the buffers (e.g. to mmap).
The client is not supposed to generate them by itself and pass them to
QUERYBUFS.
> The library
> associates this with the corresponding v4l2_buffer on the CAPTURE plane and
use
> the underlying conversion APIs to transform the decoder's yuv output into the
> egl image.
> We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where the
number
> of planes are checked with 2 (line #1660 of V4L2VDA).
You mean
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
?
This is an overassumption from the time where there was only one format
supported.
The number of planes to be used should be taken from the v4l2_format struct,
returned from G_FMT. This assumption should be fixed.
From what I'm seeing here, your HW doesn't really use V4L2_PIX_FMT_NV12M? Which
fourcc format does it use? Are the planes separate memory buffers?
>
> On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > mem_offset is u32 always, an unsigned int shouldn't be assigned to it. Also,
> > there are two planes, but passing only one offset is a bit inconsistent.
> > Although, why have two planes, if only one is used?
>
shivdasp
2014/02/12 10:11:55
The output is YUV420 planar. I think rather than u
The output is YUV420 planar. I think rather than using the QUERYBUF to pass the
EglImage handles and stuffing the required information I would rather introduce
a custom API (UseEglImage ?).
I hope that is fine.
On 2014/02/12 09:15:13, Pawel Osciak wrote:
> On 2014/02/10 13:31:17, shivdasp wrote:
> > We are really passing in the EglImage handle here to the library.
>
> EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed in an
u32
> variable.
>
> The whole idea behind offsets is that they are usually not really offsets, but
> sort of platform-independent 32bit IDs. They are acquired from the V4L2 driver
> (or library) via QUERYBUFS, and can be passed back to other calls to uniquely
> identify the buffers (e.g. to mmap).
>
> The client is not supposed to generate them by itself and pass them to
> QUERYBUFS.
>
> > The library
> > associates this with the corresponding v4l2_buffer on the CAPTURE plane and
> use
> > the underlying conversion APIs to transform the decoder's yuv output into
the
> > egl image.
> > We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where the
> number
> > of planes are checked with 2 (line #1660 of V4L2VDA).
>
> You mean
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> ?
>
> This is an overassumption from the time where there was only one format
> supported.
> The number of planes to be used should be taken from the v4l2_format struct,
> returned from G_FMT. This assumption should be fixed.
>
> From what I'm seeing here, your HW doesn't really use V4L2_PIX_FMT_NV12M?
Which
> fourcc format does it use? Are the planes separate memory buffers?
>
> >
> > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > mem_offset is u32 always, an unsigned int shouldn't be assigned to it.
Also,
> > > there are two planes, but passing only one offset is a bit inconsistent.
> > > Although, why have two planes, if only one is used?
> >
>
Pawel Osciak
2014/02/13 10:42:54
Are all planes non-interleaved and contiguous in m
On 2014/02/12 10:11:55, shivdasp wrote:
> The output is YUV420 planar.
Are all planes non-interleaved and contiguous in memory? If so, then you need to
use either V4L2_PIX_FMT_YVU420 ('YV12') or V4L2_PIX_FMT_YUV420 ('YU12'), please
see http://linuxtv.org/downloads/v4l-dvb-apis/re23.html.
Please don't use V4L2_PIX_FMT_NV12M if this is not what codec produces.
> I think rather than using the QUERYBUF to pass the
> EglImage handles and stuffing the required information I would rather
introduce
> a custom API (UseEglImage ?).
> I hope that is fine.
Yes, it's preferable over using QUERYBUF for this. But let's agree on the shape
of it. What would UseEglImage do?
Could we instead pass the offsets to eglCreateImageKHR?
Will we be able to also retain texture binding in V4L2VDA then?
>
> On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > On 2014/02/10 13:31:17, shivdasp wrote:
> > > We are really passing in the EglImage handle here to the library.
> >
> > EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed in an
> u32
> > variable.
> >
> > The whole idea behind offsets is that they are usually not really offsets,
but
> > sort of platform-independent 32bit IDs. They are acquired from the V4L2
driver
> > (or library) via QUERYBUFS, and can be passed back to other calls to
uniquely
> > identify the buffers (e.g. to mmap).
> >
> > The client is not supposed to generate them by itself and pass them to
> > QUERYBUFS.
> >
> > > The library
> > > associates this with the corresponding v4l2_buffer on the CAPTURE plane
and
> > use
> > > the underlying conversion APIs to transform the decoder's yuv output into
> the
> > > egl image.
> > > We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where the
> > number
> > > of planes are checked with 2 (line #1660 of V4L2VDA).
> >
> > You mean
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > ?
> >
> > This is an overassumption from the time where there was only one format
> > supported.
> > The number of planes to be used should be taken from the v4l2_format struct,
> > returned from G_FMT. This assumption should be fixed.
> >
> > From what I'm seeing here, your HW doesn't really use V4L2_PIX_FMT_NV12M?
> Which
> > fourcc format does it use? Are the planes separate memory buffers?
> >
> > >
> > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > mem_offset is u32 always, an unsigned int shouldn't be assigned to it.
> Also,
> > > > there are two planes, but passing only one offset is a bit inconsistent.
> > > > Although, why have two planes, if only one is used?
> > >
> >
>
shivdasp
2014/02/14 03:06:45
Okay I will change the pixel format. However there
On 2014/02/13 10:42:54, Pawel Osciak wrote:
> On 2014/02/12 10:11:55, shivdasp wrote:
> > The output is YUV420 planar.
>
> Are all planes non-interleaved and contiguous in memory? If so, then you need
to
> use either V4L2_PIX_FMT_YVU420 ('YV12') or V4L2_PIX_FMT_YUV420 ('YU12'),
please
> see http://linuxtv.org/downloads/v4l-dvb-apis/re23.html.
>
> Please don't use V4L2_PIX_FMT_NV12M if this is not what codec produces.
Okay I will change the pixel format. However there are some DHECK_EQ() code in
V4L2VDA to check against V4L2_PIX_FMT_NV12M.
They also exist for num_planes. I will have to introduce private member
functions for ExynosV4L2Device and TegraV4L2Device to check against them rather
than hardcoded values in V4L2VDA. Will that be fine ?
>
> > I think rather than using the QUERYBUF to pass the
> > EglImage handles and stuffing the required information I would rather
> introduce
> > a custom API (UseEglImage ?).
> > I hope that is fine.
>
> Yes, it's preferable over using QUERYBUF for this. But let's agree on the
shape
> of it. What would UseEglImage do?
> Could we instead pass the offsets to eglCreateImageKHR?
> Will we be able to also retain texture binding in V4L2VDA then?
I was thinking of int UseEglImage(int buffer_index, EGLImageKHR egl_image);
We basically need to send the EglImage created for a particular buffer_index so
the library can convert YUV into its respective EglImage.
We cannot send offsets to eglCreateImageKHR unless we have extension. However
the buffer_index internally is the mapping for identifying the eglImage so in a
way that will work like an offset.
>
> >
> > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > We are really passing in the EglImage handle here to the library.
> > >
> > > EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed in
an
> > u32
> > > variable.
> > >
> > > The whole idea behind offsets is that they are usually not really offsets,
> but
> > > sort of platform-independent 32bit IDs. They are acquired from the V4L2
> driver
> > > (or library) via QUERYBUFS, and can be passed back to other calls to
> uniquely
> > > identify the buffers (e.g. to mmap).
> > >
> > > The client is not supposed to generate them by itself and pass them to
> > > QUERYBUFS.
> > >
> > > > The library
> > > > associates this with the corresponding v4l2_buffer on the CAPTURE plane
> and
> > > use
> > > > the underlying conversion APIs to transform the decoder's yuv output
into
> > the
> > > > egl image.
> > > > We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where
the
> > > number
> > > > of planes are checked with 2 (line #1660 of V4L2VDA).
> > >
> > > You mean
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > > ?
> > >
> > > This is an overassumption from the time where there was only one format
> > > supported.
> > > The number of planes to be used should be taken from the v4l2_format
struct,
> > > returned from G_FMT. This assumption should be fixed.
> > >
> > > From what I'm seeing here, your HW doesn't really use V4L2_PIX_FMT_NV12M?
> > Which
> > > fourcc format does it use? Are the planes separate memory buffers?
> > >
> > > >
> > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > mem_offset is u32 always, an unsigned int shouldn't be assigned to it.
> > Also,
> > > > > there are two planes, but passing only one offset is a bit
inconsistent.
> > > > > Although, why have two planes, if only one is used?
> > > >
> > >
> >
>
Pawel Osciak
2014/02/14 07:36:10
As I said, those checks and should be fixed to use
On 2014/02/14 03:06:45, shivdasp wrote:
> On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > On 2014/02/12 10:11:55, shivdasp wrote:
> > > The output is YUV420 planar.
> >
> > Are all planes non-interleaved and contiguous in memory? If so, then you
need
> to
> > use either V4L2_PIX_FMT_YVU420 ('YV12') or V4L2_PIX_FMT_YUV420 ('YU12'),
> please
> > see http://linuxtv.org/downloads/v4l-dvb-apis/re23.html.
> >
> > Please don't use V4L2_PIX_FMT_NV12M if this is not what codec produces.
> Okay I will change the pixel format. However there are some DHECK_EQ() code in
> V4L2VDA to check against V4L2_PIX_FMT_NV12M.
> They also exist for num_planes. I will have to introduce private member
> functions for ExynosV4L2Device and TegraV4L2Device to check against them
rather
> than hardcoded values in V4L2VDA. Will that be fine ?
As I said, those checks and should be fixed to use the actual format given by
the device.
There is no need to have private member methods for devices. V4L2 API gives you
methods to query and get formats as well as information how many planes each
format uses. Please see documentation for v4l2_pix_format{,_mplane}
in http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-fmt.html.
> >
> > > I think rather than using the QUERYBUF to pass the
> > > EglImage handles and stuffing the required information I would rather
> > introduce
> > > a custom API (UseEglImage ?).
> > > I hope that is fine.
> >
> > Yes, it's preferable over using QUERYBUF for this. But let's agree on the
> shape
> > of it. What would UseEglImage do?
> > Could we instead pass the offsets to eglCreateImageKHR?
> > Will we be able to also retain texture binding in V4L2VDA then?
> I was thinking of int UseEglImage(int buffer_index, EGLImageKHR egl_image);
> We basically need to send the EglImage created for a particular buffer_index
so
> the library can convert YUV into its respective EglImage.
> We cannot send offsets to eglCreateImageKHR unless we have extension. However
> the buffer_index internally is the mapping for identifying the eglImage so in
a
> way that will work like an offset.
I really don't see why it should be an issue to create such an extension with
very minimal effort.
It should be a trivial wrapper around eglCreateImageKHR. Your library has to
call some function in the driver anyway to do this, so why not extract that code
and move it to a special case in eglCreateImageKHR instead? The code is already
written I assume, since you use it, it just needs to be moved to a different
location (i.e. eglCreateImageKHR implementation).
> >
> > >
> > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > We are really passing in the EglImage handle here to the library.
> > > >
> > > > EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed
in
> an
> > > u32
> > > > variable.
> > > >
> > > > The whole idea behind offsets is that they are usually not really
offsets,
> > but
> > > > sort of platform-independent 32bit IDs. They are acquired from the V4L2
> > driver
> > > > (or library) via QUERYBUFS, and can be passed back to other calls to
> > uniquely
> > > > identify the buffers (e.g. to mmap).
> > > >
> > > > The client is not supposed to generate them by itself and pass them to
> > > > QUERYBUFS.
> > > >
> > > > > The library
> > > > > associates this with the corresponding v4l2_buffer on the CAPTURE
plane
> > and
> > > > use
> > > > > the underlying conversion APIs to transform the decoder's yuv output
> into
> > > the
> > > > > egl image.
> > > > > We have two planes on CAPTURE_PLANE to comply with V4L2VDA code where
> the
> > > > number
> > > > > of planes are checked with 2 (line #1660 of V4L2VDA).
> > > >
> > > > You mean
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > > > ?
> > > >
> > > > This is an overassumption from the time where there was only one format
> > > > supported.
> > > > The number of planes to be used should be taken from the v4l2_format
> struct,
> > > > returned from G_FMT. This assumption should be fixed.
> > > >
> > > > From what I'm seeing here, your HW doesn't really use
V4L2_PIX_FMT_NV12M?
> > > Which
> > > > fourcc format does it use? Are the planes separate memory buffers?
> > > >
> > > > >
> > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > mem_offset is u32 always, an unsigned int shouldn't be assigned to
it.
> > > Also,
> > > > > > there are two planes, but passing only one offset is a bit
> inconsistent.
> > > > > > Although, why have two planes, if only one is used?
> > > > >
> > > >
> > >
> >
>
shivdasp
2014/02/14 09:18:58
Okay will use the V4L2 API.
On 2014/02/14 07:36:10, Pawel Osciak wrote:
> On 2014/02/14 03:06:45, shivdasp wrote:
> > On 2014/02/13 10:42:54, Pawel Osciak wrote:
> > > On 2014/02/12 10:11:55, shivdasp wrote:
> > > > The output is YUV420 planar.
> > >
> > > Are all planes non-interleaved and contiguous in memory? If so, then you
> need
> > to
> > > use either V4L2_PIX_FMT_YVU420 ('YV12') or V4L2_PIX_FMT_YUV420 ('YU12'),
> > please
> > > see http://linuxtv.org/downloads/v4l-dvb-apis/re23.html.
> > >
> > > Please don't use V4L2_PIX_FMT_NV12M if this is not what codec produces.
> > Okay I will change the pixel format. However there are some DHECK_EQ() code
in
> > V4L2VDA to check against V4L2_PIX_FMT_NV12M.
> > They also exist for num_planes. I will have to introduce private member
> > functions for ExynosV4L2Device and TegraV4L2Device to check against them
> rather
> > than hardcoded values in V4L2VDA. Will that be fine ?
>
> As I said, those checks and should be fixed to use the actual format given by
> the device.
> There is no need to have private member methods for devices. V4L2 API gives
you
> methods to query and get formats as well as information how many planes each
> format uses. Please see documentation for v4l2_pix_format{,_mplane}
> in http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-fmt.html.
Okay will use the V4L2 API.
>
> > >
> > > > I think rather than using the QUERYBUF to pass the
> > > > EglImage handles and stuffing the required information I would rather
> > > introduce
> > > > a custom API (UseEglImage ?).
> > > > I hope that is fine.
> > >
> > > Yes, it's preferable over using QUERYBUF for this. But let's agree on the
> > shape
> > > of it. What would UseEglImage do?
> > > Could we instead pass the offsets to eglCreateImageKHR?
> > > Will we be able to also retain texture binding in V4L2VDA then?
> > I was thinking of int UseEglImage(int buffer_index, EGLImageKHR egl_image);
> > We basically need to send the EglImage created for a particular buffer_index
> so
> > the library can convert YUV into its respective EglImage.
> > We cannot send offsets to eglCreateImageKHR unless we have extension.
However
> > the buffer_index internally is the mapping for identifying the eglImage so
in
> a
> > way that will work like an offset.
>
> I really don't see why it should be an issue to create such an extension with
> very minimal effort.
> It should be a trivial wrapper around eglCreateImageKHR. Your library has to
> call some function in the driver anyway to do this, so why not extract that
code
> and move it to a special case in eglCreateImageKHR instead? The code is
already
> written I assume, since you use it, it just needs to be moved to a different
> location (i.e. eglCreateImageKHR implementation).
The graphics stack is owned by a separate team so I don't really understand the
implementation issues if any.
I will check if there is such plan in the meanwhile let me address all these
review comments and send out second patchset.
>
> > >
> > > >
> > > > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > > > We are really passing in the EglImage handle here to the library.
> > > > >
> > > > > EGLImageKHR is typedef void*, which can be 64 bit. It cannot be passed
> in
> > an
> > > > u32
> > > > > variable.
> > > > >
> > > > > The whole idea behind offsets is that they are usually not really
> offsets,
> > > but
> > > > > sort of platform-independent 32bit IDs. They are acquired from the
V4L2
> > > driver
> > > > > (or library) via QUERYBUFS, and can be passed back to other calls to
> > > uniquely
> > > > > identify the buffers (e.g. to mmap).
> > > > >
> > > > > The client is not supposed to generate them by itself and pass them to
> > > > > QUERYBUFS.
> > > > >
> > > > > > The library
> > > > > > associates this with the corresponding v4l2_buffer on the CAPTURE
> plane
> > > and
> > > > > use
> > > > > > the underlying conversion APIs to transform the decoder's yuv output
> > into
> > > > the
> > > > > > egl image.
> > > > > > We have two planes on CAPTURE_PLANE to comply with V4L2VDA code
where
> > the
> > > > > number
> > > > > > of planes are checked with 2 (line #1660 of V4L2VDA).
> > > > >
> > > > > You mean
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > > > > ?
> > > > >
> > > > > This is an overassumption from the time where there was only one
format
> > > > > supported.
> > > > > The number of planes to be used should be taken from the v4l2_format
> > struct,
> > > > > returned from G_FMT. This assumption should be fixed.
> > > > >
> > > > > From what I'm seeing here, your HW doesn't really use
> V4L2_PIX_FMT_NV12M?
> > > > Which
> > > > > fourcc format does it use? Are the planes separate memory buffers?
> > > > >
> > > > > >
> > > > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > > > mem_offset is u32 always, an unsigned int shouldn't be assigned to
> it.
> > > > Also,
> > > > > > > there are two planes, but passing only one offset is a bit
> > inconsistent.
> > > > > > > Although, why have two planes, if only one is used?
> > > > > >
> > > > >
> > > >
> > >
> >
>
|
+ if (HANDLE_EINTR(Ioctl(VIDIOC_QUERYBUF, &capture_buffer)) != 0) { |
+ DLOG(ERROR) << "Some error in querybuf"; |
+ return EGL_NO_IMAGE_KHR; |
Ami GONE FROM CHROMIUM
2014/02/07 09:09:30
leak? (no eglDestroyImageKHR needed?)
leak? (no eglDestroyImageKHR needed?)
shivdasp
2014/02/10 13:31:17
Yes will fix this.
On 2014/02/07 09:09:30, Ami Fis
Yes will fix this.
On 2014/02/07 09:09:30, Ami Fischman wrote:
> leak? (no eglDestroyImageKHR needed?)
|
+ } |
+ return egl_image; |
+} |
Pawel Osciak
2014/02/10 06:36:17
After reading through it, this method feels unnece
After reading through it, this method feels unnecessary.
I'm assuming querybufs implementation in the library calls a method in the GPU
driver anyway?
This is basically redefining querybufs to do something completely different than
it normally does, turning it into a custom call. The offsets should be coming
from the callee of QUERYBUFS, not the other way around, and there should be no
custom side effects.
A non-V4L2, library-specific custom call would be better than this.
But why not implement eglCreateImageKHR that accepts dmabufs (or even offsets)
that come from the v4l2 library to create EGL images, just like we do on Mali on
Exynos?
Would it be possible to have an extension for eglCreateImage like Exynos does
instead please? It doesn't seem to be much of a difference, instead of calling
querybufs with custom arguments and having the library call something in the
driver, call eglCreateImage instead with custom arguments and have it do
everything?
shivdasp
2014/02/10 13:31:17
Since we started with implementing this as a V4L2-
Since we started with implementing this as a V4L2-Like library we have tried to
follow V4L2 syntax to provide the input & output buffers.
QUERYBUF can be made into a custom call since it is doing very custom thing
here.
If introducing another API is acceptable I can do that.
We do not yet have eglCreateImageKHR that accepts dmabufs. I can check with the
our Graphics team but I don't there is any such plan to implement such
extension.
On 2014/02/10 06:36:17, Pawel Osciak wrote:
> After reading through it, this method feels unnecessary.
>
> I'm assuming querybufs implementation in the library calls a method in the GPU
> driver anyway?
>
> This is basically redefining querybufs to do something completely different
than
> it normally does, turning it into a custom call. The offsets should be coming
> from the callee of QUERYBUFS, not the other way around, and there should be no
> custom side effects.
> A non-V4L2, library-specific custom call would be better than this.
>
> But why not implement eglCreateImageKHR that accepts dmabufs (or even offsets)
> that come from the v4l2 library to create EGL images, just like we do on Mali
on
> Exynos?
>
> Would it be possible to have an extension for eglCreateImage like Exynos does
> instead please? It doesn't seem to be much of a difference, instead of calling
> querybufs with custom arguments and having the library call something in the
> driver, call eglCreateImage instead with custom arguments and have it do
> everything?
Pawel Osciak
2014/02/12 09:15:13
Please understand that:
1. We are asking for a V4L
On 2014/02/10 13:31:17, shivdasp wrote:
> Since we started with implementing this as a V4L2-Like library we have tried
to
> follow V4L2 syntax to provide the input & output buffers.
> QUERYBUF can be made into a custom call since it is doing very custom thing
> here.
Please understand that:
1. We are asking for a V4L2-like interface that conforms to the V4L2 API. A call
documented to work in the same way regardless of buffer type passed should not
do otherwise, if it can be prevented. If it's expected to return values, it
shouldn't be accepting them instead. And so on.
Of course, this is an adapter class, so the actual inner workings may be
different and it's not always possible to do exactly the same thing, but from
the point of view of the client the effects should be as close to what each call
is documented to do, as possible.
Otherwise this whole exercise of using V4L2 API is doing us more bad than good.
Please understand that the V4L2VDA and V4L2VEA classes will live on and will
work with multiple platforms. There will be many changes to them. People working
on them will expect behavior as documented in V4L2 API. Otherwise things will
break (and not only for other platforms, but Tegra too) and it will be very
difficult to reason why.
So it's very important for Tegra V4L2Device to behave like V4L2 API specifies
and not be tailored to how things are laid out in V4L2VDA currently.
2. This V4L2Device class should work with V4L2VEA class as well. I don't think
we can make it work if this hack on QUERYBUFS is here.
> If introducing another API is acceptable I can do that.
>
> We do not yet have eglCreateImageKHR that accepts dmabufs. I can check with
the
> our Graphics team but I don't there is any such plan to implement such
> extension.
>
That's why I gave the option of using offsets. If you prefer not to use dmabufs,
could we please:
- provide offsets via querybufs from the driver/library
- pass those offsets to a new eglCreateImage extension and move it back to
V4L2VDA
- keep using texture binding API?
This should eliminate the need for this method as well.
> On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > After reading through it, this method feels unnecessary.
> >
> > I'm assuming querybufs implementation in the library calls a method in the
GPU
> > driver anyway?
> >
> > This is basically redefining querybufs to do something completely different
> than
> > it normally does, turning it into a custom call. The offsets should be
coming
> > from the callee of QUERYBUFS, not the other way around, and there should be
no
> > custom side effects.
> > A non-V4L2, library-specific custom call would be better than this.
> >
> > But why not implement eglCreateImageKHR that accepts dmabufs (or even
offsets)
> > that come from the v4l2 library to create EGL images, just like we do on
Mali
> on
> > Exynos?
> >
> > Would it be possible to have an extension for eglCreateImage like Exynos
does
> > instead please? It doesn't seem to be much of a difference, instead of
calling
> > querybufs with custom arguments and having the library call something in the
> > driver, call eglCreateImage instead with custom arguments and have it do
> > everything?
>
shivdasp
2014/02/12 10:11:55
There is no extension to create EglImages from dma
There is no extension to create EglImages from dmabufs or the offsets at the
moment unfortunately.
I agree using the QUERYBUF for sending the EglImage can be misleading and I will
change it. V4L2VEA should not affected since we use the QUERYBUF for providing
the actual offsets. This hack was only for sending the EglImages in case of
CAPTURE PLANE of decoder.
As I said earlier will adding a custom API (for now) to send the EglImages be
okay ?
On 2014/02/12 09:15:13, Pawel Osciak wrote:
> On 2014/02/10 13:31:17, shivdasp wrote:
> > Since we started with implementing this as a V4L2-Like library we have tried
> to
> > follow V4L2 syntax to provide the input & output buffers.
> > QUERYBUF can be made into a custom call since it is doing very custom thing
> > here.
>
> Please understand that:
> 1. We are asking for a V4L2-like interface that conforms to the V4L2 API. A
call
> documented to work in the same way regardless of buffer type passed should not
> do otherwise, if it can be prevented. If it's expected to return values, it
> shouldn't be accepting them instead. And so on.
> Of course, this is an adapter class, so the actual inner workings may be
> different and it's not always possible to do exactly the same thing, but from
> the point of view of the client the effects should be as close to what each
call
> is documented to do, as possible.
>
> Otherwise this whole exercise of using V4L2 API is doing us more bad than
good.
>
> Please understand that the V4L2VDA and V4L2VEA classes will live on and will
> work with multiple platforms. There will be many changes to them. People
working
> on them will expect behavior as documented in V4L2 API. Otherwise things will
> break (and not only for other platforms, but Tegra too) and it will be very
> difficult to reason why.
>
> So it's very important for Tegra V4L2Device to behave like V4L2 API specifies
> and not be tailored to how things are laid out in V4L2VDA currently.
>
> 2. This V4L2Device class should work with V4L2VEA class as well. I don't think
> we can make it work if this hack on QUERYBUFS is here.
>
>
> > If introducing another API is acceptable I can do that.
> >
> > We do not yet have eglCreateImageKHR that accepts dmabufs. I can check with
> the
> > our Graphics team but I don't there is any such plan to implement such
> > extension.
> >
>
> That's why I gave the option of using offsets. If you prefer not to use
dmabufs,
> could we please:
>
> - provide offsets via querybufs from the driver/library
> - pass those offsets to a new eglCreateImage extension and move it back to
> V4L2VDA
> - keep using texture binding API?
>
> This should eliminate the need for this method as well.
>
> > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > After reading through it, this method feels unnecessary.
> > >
> > > I'm assuming querybufs implementation in the library calls a method in the
> GPU
> > > driver anyway?
> > >
> > > This is basically redefining querybufs to do something completely
different
> > than
> > > it normally does, turning it into a custom call. The offsets should be
> coming
> > > from the callee of QUERYBUFS, not the other way around, and there should
be
> no
> > > custom side effects.
> > > A non-V4L2, library-specific custom call would be better than this.
> > >
> > > But why not implement eglCreateImageKHR that accepts dmabufs (or even
> offsets)
> > > that come from the v4l2 library to create EGL images, just like we do on
> Mali
> > on
> > > Exynos?
> > >
> > > Would it be possible to have an extension for eglCreateImage like Exynos
> does
> > > instead please? It doesn't seem to be much of a difference, instead of
> calling
> > > querybufs with custom arguments and having the library call something in
the
> > > driver, call eglCreateImage instead with custom arguments and have it do
> > > everything?
> >
>
Pawel Osciak
2014/02/13 10:42:54
Could you explain why is it not affected? VEA call
On 2014/02/12 10:11:55, shivdasp wrote:
> There is no extension to create EglImages from dmabufs or the offsets at the
> moment unfortunately.
> I agree using the QUERYBUF for sending the EglImage can be misleading and I
will
> change it. V4L2VEA should not affected since we use the QUERYBUF for providing
> the actual offsets. This hack was only for sending the EglImages in case of
> CAPTURE PLANE of decoder.
Could you explain why is it not affected? VEA calls QUERYBUF on CAPTURE buffers.
> As I said earlier will adding a custom API (for now) to send the EglImages be
> okay ?
>
>
> On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > On 2014/02/10 13:31:17, shivdasp wrote:
> > > Since we started with implementing this as a V4L2-Like library we have
tried
> > to
> > > follow V4L2 syntax to provide the input & output buffers.
> > > QUERYBUF can be made into a custom call since it is doing very custom
thing
> > > here.
> >
> > Please understand that:
> > 1. We are asking for a V4L2-like interface that conforms to the V4L2 API. A
> call
> > documented to work in the same way regardless of buffer type passed should
not
> > do otherwise, if it can be prevented. If it's expected to return values, it
> > shouldn't be accepting them instead. And so on.
> > Of course, this is an adapter class, so the actual inner workings may be
> > different and it's not always possible to do exactly the same thing, but
from
> > the point of view of the client the effects should be as close to what each
> call
> > is documented to do, as possible.
> >
> > Otherwise this whole exercise of using V4L2 API is doing us more bad than
> good.
> >
> > Please understand that the V4L2VDA and V4L2VEA classes will live on and will
> > work with multiple platforms. There will be many changes to them. People
> working
> > on them will expect behavior as documented in V4L2 API. Otherwise things
will
> > break (and not only for other platforms, but Tegra too) and it will be very
> > difficult to reason why.
> >
> > So it's very important for Tegra V4L2Device to behave like V4L2 API
specifies
> > and not be tailored to how things are laid out in V4L2VDA currently.
> >
> > 2. This V4L2Device class should work with V4L2VEA class as well. I don't
think
> > we can make it work if this hack on QUERYBUFS is here.
> >
> >
> > > If introducing another API is acceptable I can do that.
> > >
> > > We do not yet have eglCreateImageKHR that accepts dmabufs. I can check
with
> > the
> > > our Graphics team but I don't there is any such plan to implement such
> > > extension.
> > >
> >
> > That's why I gave the option of using offsets. If you prefer not to use
> dmabufs,
> > could we please:
> >
> > - provide offsets via querybufs from the driver/library
> > - pass those offsets to a new eglCreateImage extension and move it back to
> > V4L2VDA
> > - keep using texture binding API?
> >
> > This should eliminate the need for this method as well.
> >
> > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > After reading through it, this method feels unnecessary.
> > > >
> > > > I'm assuming querybufs implementation in the library calls a method in
the
> > GPU
> > > > driver anyway?
> > > >
> > > > This is basically redefining querybufs to do something completely
> different
> > > than
> > > > it normally does, turning it into a custom call. The offsets should be
> > coming
> > > > from the callee of QUERYBUFS, not the other way around, and there should
> be
> > no
> > > > custom side effects.
> > > > A non-V4L2, library-specific custom call would be better than this.
> > > >
> > > > But why not implement eglCreateImageKHR that accepts dmabufs (or even
> > offsets)
> > > > that come from the v4l2 library to create EGL images, just like we do on
> > Mali
> > > on
> > > > Exynos?
> > > >
> > > > Would it be possible to have an extension for eglCreateImage like Exynos
> > does
> > > > instead please? It doesn't seem to be much of a difference, instead of
> > calling
> > > > querybufs with custom arguments and having the library call something in
> the
> > > > driver, call eglCreateImage instead with custom arguments and have it do
> > > > everything?
> > >
> >
>
shivdasp
2014/02/14 03:06:45
The fd returned by TegraV4L2Open() knows whether i
The fd returned by TegraV4L2Open() knows whether it was for a decoder instance
or encoder instance.
Hence QUERYBUF on CAPTURE_PLANE for an encoder will behave as per the V4L2
specification. An alternate hacky behavior was needed for decoder instance to
send EglImages which we are now sending in custom call.
On 2014/02/13 10:42:54, Pawel Osciak wrote:
> On 2014/02/12 10:11:55, shivdasp wrote:
> > There is no extension to create EglImages from dmabufs or the offsets at the
> > moment unfortunately.
> > I agree using the QUERYBUF for sending the EglImage can be misleading and I
> will
> > change it. V4L2VEA should not affected since we use the QUERYBUF for
providing
> > the actual offsets. This hack was only for sending the EglImages in case of
> > CAPTURE PLANE of decoder.
>
> Could you explain why is it not affected? VEA calls QUERYBUF on CAPTURE
buffers.
>
> > As I said earlier will adding a custom API (for now) to send the EglImages
be
> > okay ?
> >
> >
> > On 2014/02/12 09:15:13, Pawel Osciak wrote:
> > > On 2014/02/10 13:31:17, shivdasp wrote:
> > > > Since we started with implementing this as a V4L2-Like library we have
> tried
> > > to
> > > > follow V4L2 syntax to provide the input & output buffers.
> > > > QUERYBUF can be made into a custom call since it is doing very custom
> thing
> > > > here.
> > >
> > > Please understand that:
> > > 1. We are asking for a V4L2-like interface that conforms to the V4L2 API.
A
> > call
> > > documented to work in the same way regardless of buffer type passed should
> not
> > > do otherwise, if it can be prevented. If it's expected to return values,
it
> > > shouldn't be accepting them instead. And so on.
> > > Of course, this is an adapter class, so the actual inner workings may be
> > > different and it's not always possible to do exactly the same thing, but
> from
> > > the point of view of the client the effects should be as close to what
each
> > call
> > > is documented to do, as possible.
> > >
> > > Otherwise this whole exercise of using V4L2 API is doing us more bad than
> > good.
> > >
> > > Please understand that the V4L2VDA and V4L2VEA classes will live on and
will
> > > work with multiple platforms. There will be many changes to them. People
> > working
> > > on them will expect behavior as documented in V4L2 API. Otherwise things
> will
> > > break (and not only for other platforms, but Tegra too) and it will be
very
> > > difficult to reason why.
> > >
> > > So it's very important for Tegra V4L2Device to behave like V4L2 API
> specifies
> > > and not be tailored to how things are laid out in V4L2VDA currently.
> > >
> > > 2. This V4L2Device class should work with V4L2VEA class as well. I don't
> think
> > > we can make it work if this hack on QUERYBUFS is here.
> > >
> > >
> > > > If introducing another API is acceptable I can do that.
> > > >
> > > > We do not yet have eglCreateImageKHR that accepts dmabufs. I can check
> with
> > > the
> > > > our Graphics team but I don't there is any such plan to implement such
> > > > extension.
> > > >
> > >
> > > That's why I gave the option of using offsets. If you prefer not to use
> > dmabufs,
> > > could we please:
> > >
> > > - provide offsets via querybufs from the driver/library
> > > - pass those offsets to a new eglCreateImage extension and move it back to
> > > V4L2VDA
> > > - keep using texture binding API?
> > >
> > > This should eliminate the need for this method as well.
> > >
> > > > On 2014/02/10 06:36:17, Pawel Osciak wrote:
> > > > > After reading through it, this method feels unnecessary.
> > > > >
> > > > > I'm assuming querybufs implementation in the library calls a method in
> the
> > > GPU
> > > > > driver anyway?
> > > > >
> > > > > This is basically redefining querybufs to do something completely
> > different
> > > > than
> > > > > it normally does, turning it into a custom call. The offsets should be
> > > coming
> > > > > from the callee of QUERYBUFS, not the other way around, and there
should
> > be
> > > no
> > > > > custom side effects.
> > > > > A non-V4L2, library-specific custom call would be better than this.
> > > > >
> > > > > But why not implement eglCreateImageKHR that accepts dmabufs (or even
> > > offsets)
> > > > > that come from the v4l2 library to create EGL images, just like we do
on
> > > Mali
> > > > on
> > > > > Exynos?
> > > > >
> > > > > Would it be possible to have an extension for eglCreateImage like
Exynos
> > > does
> > > > > instead please? It doesn't seem to be much of a difference, instead of
> > > calling
> > > > > querybufs with custom arguments and having the library call something
in
> > the
> > > > > driver, call eglCreateImage instead with custom arguments and have it
do
> > > > > everything?
> > > >
> > >
> >
>
|
+ |
+unsigned int TegraV4L2Device::GetTextureTarget() { return GL_TEXTURE_2D; } |
+ |
+} // namespace content |