I'd invert the test:
if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return -EINVAL;
and now you can just set ctx->enc_params.
We will fix this in next version.
quoted
quoted
+ return 0;
+}
And if there is an s_parm, then there should be a g_parm as well!
Now our driver does not support g_parm, our use cases do not use g_parm
too.
Do we need to add g_parm at this moment? Or we could add it when we need
g_parm?
No, you need it. You can see why if you look at the v4l2-compliance output:
test VIDIOC_G/S_PARM: OK (Not Supported)
Why does it think it is unsupported? Because (just like most applications) it
tries to call G_PARM first, and if that succeeds it tries to call S_PARM with
the value it got from G_PARM. Thus ensuring the application doesn't change the
driver state. So you can have a 'get' ioctl without the 'set' ioctl, but if
there is a 'set' ioctl there must always be a 'get' ioctl.
<snip>
Why support s_selection if you can only return the current width and height?
And why support g_selection if you can't change the selection?
In other words, why implement this at all?
Unless I am missing something here, I would just drop this.
Now our driver do not support these capabilities, but userspace app will
check whether g/s_crop are implemented when using encoder.
Because g/s_crop are deprecated as you mentioned in previous v2 review
comments. We change to use g_s_selection.
We will check if we could add this capability.
It's true that you should use g/s_selection instead of g/s_crop, but only if
there is actually something to select. As long as you don't offer this capability,
just drop this for now.
When you add the capability later you can just add the g/s_selection functions.
Getting selection right can be tricky. I wouldn't mind if this is done later in a
separate patch.
Please add vidioc_create_bufs and vidioc_prepare_buf as well.
Currently we do not support these use cases, do we need to add
vidioc_create_bufs and vidioc_prepare_buf now?
I would suggest you do. The vb2 framework gives it (almost) for free.
prepare_buf is completely free (just add the helper) and create_bufs
needs a few small changes in the queue_setup function, that's all.
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
I'd invert the test:
if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return -EINVAL;
and now you can just set ctx->enc_params.
We will fix this in next version.
quoted
quoted
+ return 0;
+}
And if there is an s_parm, then there should be a g_parm as well!
Now our driver does not support g_parm, our use cases do not use g_parm
too.
Do we need to add g_parm at this moment? Or we could add it when we need
g_parm?
No, you need it. You can see why if you look at the v4l2-compliance output:
test VIDIOC_G/S_PARM: OK (Not Supported)
Why does it think it is unsupported? Because (just like most applications) it
tries to call G_PARM first, and if that succeeds it tries to call S_PARM with
the value it got from G_PARM. Thus ensuring the application doesn't change the
driver state. So you can have a 'get' ioctl without the 'set' ioctl, but if
there is a 'set' ioctl there must always be a 'get' ioctl.
Why support s_selection if you can only return the current width and height?
And why support g_selection if you can't change the selection?
In other words, why implement this at all?
Unless I am missing something here, I would just drop this.
Now our driver do not support these capabilities, but userspace app will
check whether g/s_crop are implemented when using encoder.
Because g/s_crop are deprecated as you mentioned in previous v2 review
comments. We change to use g_s_selection.
We will check if we could add this capability.
It's true that you should use g/s_selection instead of g/s_crop, but only if
there is actually something to select. As long as you don't offer this capability,
just drop this for now.
When you add the capability later you can just add the g/s_selection functions.
Getting selection right can be tricky. I wouldn't mind if this is done later in a
separate patch.
Please add vidioc_create_bufs and vidioc_prepare_buf as well.
Currently we do not support these use cases, do we need to add
vidioc_create_bufs and vidioc_prepare_buf now?
I would suggest you do. The vb2 framework gives it (almost) for free.
prepare_buf is completely free (just add the helper) and create_bufs
needs a few small changes in the queue_setup function, that's all.
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Understood now. Thanks for your explanation.
Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops.
I don't know why that dma-contig driver do not reject it.
I will try to figure it out.
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Understood now. Thanks for your explanation.
Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops.
I don't know why that dma-contig driver do not reject it.
I will try to figure it out.
Is there an iommu involved that turns the scatter-gather list into what looks like
contiguous memory for the DMA?
At the end of vb2_dc_get_userptr() in videobuf2-dma-contig.c there is a check
'if (contig_size < size)' that verifies that the sg DMA is contiguous. This would
work if there is an iommu involved (if I understand it correctly).
If that's the case, then it's OK to keep VB2_USERPTR because you have real sg
support (although not via the DMA engine, but via iommu mappings).
Regards,
Hans
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Understood now. Thanks for your explanation.
Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops.
I don't know why that dma-contig driver do not reject it.
I will try to figure it out.
Is there an iommu involved that turns the scatter-gather list into what looks like
contiguous memory for the DMA?
Yes, We have iommu that could make scatter-gather list looks like
contiguous memory.
At the end of vb2_dc_get_userptr() in videobuf2-dma-contig.c there is a check
'if (contig_size < size)' that verifies that the sg DMA is contiguous. This would
work if there is an iommu involved (if I understand it correctly).
I see. We saw this error before we add iommu support.
If that's the case, then it's OK to keep VB2_USERPTR because you have real sg
support (although not via the DMA engine, but via iommu mappings).
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Understood now. Thanks for your explanation.
Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops.
I don't know why that dma-contig driver do not reject it.
I will try to figure it out.
Is there an iommu involved that turns the scatter-gather list into what looks like
contiguous memory for the DMA?
Yes, We have iommu that could make scatter-gather list looks like
contiguous memory.
quoted
At the end of vb2_dc_get_userptr() in videobuf2-dma-contig.c there is a check
'if (contig_size < size)' that verifies that the sg DMA is contiguous. This would
work if there is an iommu involved (if I understand it correctly).
I see. We saw this error before we add iommu support.
quoted
If that's the case, then it's OK to keep VB2_USERPTR because you have real sg
support (although not via the DMA engine, but via iommu mappings).
Got it. We will keep VB2_USERPTR.
Can you add a comment here mentioning that VB2_USERPTR works with dma-contig because
there is an iommu? That should clarify this.
Regards,
Hans
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Understood now. Thanks for your explanation.
Now our app use malloc()ed buffers and we hook vb2_dma_contig_memops.
I don't know why that dma-contig driver do not reject it.
I will try to figure it out.
Is there an iommu involved that turns the scatter-gather list into what looks like
contiguous memory for the DMA?
Yes, We have iommu that could make scatter-gather list looks like
contiguous memory.
quoted
At the end of vb2_dc_get_userptr() in videobuf2-dma-contig.c there is a check
'if (contig_size < size)' that verifies that the sg DMA is contiguous. This would
work if there is an iommu involved (if I understand it correctly).
I see. We saw this error before we add iommu support.
quoted
If that's the case, then it's OK to keep VB2_USERPTR because you have real sg
support (although not via the DMA engine, but via iommu mappings).
Got it. We will keep VB2_USERPTR.
Can you add a comment here mentioning that VB2_USERPTR works with dma-contig because
there is an iommu? That should clarify this.
Please add vidioc_create_bufs and vidioc_prepare_buf as well.
Currently we do not support these use cases, do we need to add
vidioc_create_bufs and vidioc_prepare_buf now?
I would suggest you do. The vb2 framework gives it (almost) for free.
prepare_buf is completely free (just add the helper) and create_bufs
needs a few small changes in the queue_setup function, that's all.
After try to add vidioc_create_bufs directly using
vb2_ioctl_create_bufs, it will have problem in
int res = vb2_verify_memory_type(vdev->queue, p->memory,
p->format.type);
We do not init our video_device queue in device probe function.
Our vb2_queues for OUTPUT and CAPTURE are initialized in
v4l2_m2m_ctx_init when ctx instance open.
What is queue in video_device for?
If we should init vdev->queue in probe function, this queue format
should be CAPTURE queue or OUTPUT queue?
best regards,
Tiffany
I recomment dropping VB2_USERPTR. That only makes sense for scatter-gather dma,
and you use physically contiguous DMA.
Now our userspace app use VB2_USERPTR. I need to check if we could drop
VB2_USERPTR.
We use src_vq->mem_ops = &vb2_dma_contig_memops;
And there are
.get_userptr = vb2_dc_get_userptr,
.put_userptr = vb2_dc_put_userptr,
I was confused why it only make sense for scatter-gather.
Could you kindly explain more?
VB2_USERPTR indicates that the application can use malloc to allocate buffers
and pass those to the driver. Since malloc uses virtual memory the physical
memory is scattered all over. And the first page typically does not start at
the beginning of the page but at a random offset.
To support that the DMA generally has to be able to do scatter-gather.
Now, where things get ugly is that a hack was added to the USERPTR support where
apps could pass a pointer to physically contiguous memory as a user pointer. This
was a hack for embedded systems that preallocated a pool of buffers and needed to
pass those pointers around somehow. So the dma-contig USERPTR support is for that
'feature'. If you try to pass a malloc()ed buffer to a dma-contig driver it will
reject it. One big problem is that this specific hack isn't signaled anywhere, so
applications have no way of knowing if the USERPTR support is the proper version
or the hack where physically contiguous memory is expected.
This hack has been replaced with DMABUF which is the proper way of passing buffers
around.
New dma-contig drivers should not use that old hack anymore. Use dmabuf to pass
external buffers around.
How do you use it in your app? With malloc()ed buffers? Or with 'special' pointers
to physically contiguous buffers?
Please add vidioc_create_bufs and vidioc_prepare_buf as well.
Currently we do not support these use cases, do we need to add
vidioc_create_bufs and vidioc_prepare_buf now?
I would suggest you do. The vb2 framework gives it (almost) for free.
prepare_buf is completely free (just add the helper) and create_bufs
needs a few small changes in the queue_setup function, that's all.
After try to add vidioc_create_bufs directly using
vb2_ioctl_create_bufs, it will have problem in
This is a m2m device, so you should use the m2m variant of this:
v4l2_m2m_ioctl_create_bufs
That should solve this problem.
Ditto for prepare_buf: you need to use v4l2_m2m_ioctl_prepare_buf.
Regards,
Hans
Please add vidioc_create_bufs and vidioc_prepare_buf as well.
Currently we do not support these use cases, do we need to add
vidioc_create_bufs and vidioc_prepare_buf now?
I would suggest you do. The vb2 framework gives it (almost) for free.
prepare_buf is completely free (just add the helper) and create_bufs
needs a few small changes in the queue_setup function, that's all.
After try to add vidioc_create_bufs directly using
vb2_ioctl_create_bufs, it will have problem in
This is a m2m device, so you should use the m2m variant of this:
v4l2_m2m_ioctl_create_bufs
That should solve this problem.
Ditto for prepare_buf: you need to use v4l2_m2m_ioctl_prepare_buf.
Got it. After using v4l2_m2m_ioctl_create_bufs, the problem was solved.
Thanks for your help.