[v2,15/34] staging: mmal-vchiq: Use vc-sm-cma to support zero copy
Commit Message
From: Dave Stevenson <dave.stevenson@raspberrypi.org>
With the vc-sm-cma driver we can support zero copy of buffers between
the kernel and VPU. Add this support to vchiq-mmal.
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
staging: vc-sm-cma: Use a void* pointer as the handle within the kernel
The driver was using an unsigned int as the handle to the outside world,
and doing a nasty cast to the struct dmabuf when handed it back.
This breaks badly with a 64 bit kernel where the pointer doesn't fit
in an unsigned int.
Switch to using a void* within the kernel. Reality is that it is
a struct dma_buf*, but advertising it as such to other drivers seems
to encourage the use of it as such, and I'm not sure on the implications
of that.
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
.../vc04_services/vchiq-mmal/mmal-common.h | 4 ++
.../vc04_services/vchiq-mmal/mmal-vchiq.c | 66 ++++++++++++++++++-
.../vc04_services/vchiq-mmal/mmal-vchiq.h | 1 +
3 files changed, 69 insertions(+), 2 deletions(-)
Comments
On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
>
> With the vc-sm-cma driver we can support zero copy of buffers between
> the kernel and VPU. Add this support to vchiq-mmal.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>
> staging: vc-sm-cma: Use a void* pointer as the handle within the kernel
>
> The driver was using an unsigned int as the handle to the outside world,
> and doing a nasty cast to the struct dmabuf when handed it back.
> This breaks badly with a 64 bit kernel where the pointer doesn't fit
> in an unsigned int.
>
> Switch to using a void* within the kernel. Reality is that it is
> a struct dma_buf*, but advertising it as such to other drivers seems
> to encourage the use of it as such, and I'm not sure on the implications
> of that.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
I think this patch's description needs to be updated.
Regards,
Nicolas
Hi Laurent,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.7-rc4]
[cannot apply to linuxtv-media/master anholt/for-next next-20200505]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Laurent-Pinchart/Drivers-for-the-BCM283x-CSI-2-CCP2-receiver-and-ISP/20200505-054310
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9851a0dee7c28514f149f7e4f60ec1b06286cc1b
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-191-gc51a0382-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1796:36: sparse: sparse: Using plain integer as NULL pointer
drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:472:48: sparse: sparse: non size-preserving integer to pointer cast
vim +1796 drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
1779
1780 int mmal_vchi_buffer_cleanup(struct mmal_buffer *buf)
1781 {
1782 struct mmal_msg_context *msg_context = buf->msg_context;
1783
1784 if (msg_context)
1785 release_msg_context(msg_context);
1786 buf->msg_context = NULL;
1787
1788 if (buf->vcsm_handle) {
1789 int ret;
1790
1791 pr_debug("%s: vc_sm_cma_free on handle %p\n", __func__,
1792 buf->vcsm_handle);
1793 ret = vc_sm_cma_free(buf->vcsm_handle);
1794 if (ret)
1795 pr_err("%s: vcsm_free failed, ret %d\n", __func__, ret);
> 1796 buf->vcsm_handle = 0;
1797 }
1798 return 0;
1799 }
1800 EXPORT_SYMBOL_GPL(mmal_vchi_buffer_cleanup);
1801
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Dave,
some questions.
On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
>
> With the vc-sm-cma driver we can support zero copy of buffers between
> the kernel and VPU. Add this support to vchiq-mmal.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>
> staging: vc-sm-cma: Use a void* pointer as the handle within the kernel
>
> The driver was using an unsigned int as the handle to the outside world,
> and doing a nasty cast to the struct dmabuf when handed it back.
> This breaks badly with a 64 bit kernel where the pointer doesn't fit
> in an unsigned int.
>
> Switch to using a void* within the kernel. Reality is that it is
> a struct dma_buf*, but advertising it as such to other drivers seems
> to encourage the use of it as such, and I'm not sure on the implications
> of that.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
[...]
> @@ -419,8 +422,13 @@ buffer_from_host(struct vchiq_mmal_instance *instance,
>
> /* buffer header */
> m.u.buffer_from_host.buffer_header.cmd = 0;
> - m.u.buffer_from_host.buffer_header.data =
> - (u32)(unsigned long)buf->buffer;
> + if (port->zero_copy) {
> + m.u.buffer_from_host.buffer_header.data = buf->vc_handle;
> + } else {
> + m.u.buffer_from_host.buffer_header.data =
> + (u32)(unsigned long)buf->buffer;
> + }
Just for my education, where exactly does the copying happen, IIUC this headers
are copied into a vchiq slot and then it's up to vc4 to do whatever it pleases
with the buffer. Also, what happens while importing a dmabuf on vc4's side and
why is the buffer import needed before queueing this?
Regards,
Nicolas
@@ -50,6 +50,10 @@ struct mmal_buffer {
struct mmal_msg_context *msg_context;
+ struct dma_buf *dma_buf;/* Exported dmabuf fd from videobuf2 */
+ void *vcsm_handle; /* VCSM handle having imported the dmabuf */
+ u32 vc_handle; /* VC handle to that dmabuf */
+
u32 cmd; /* MMAL command. 0=data. */
unsigned long length;
u32 mmal_flags;
@@ -26,9 +26,12 @@
#include <media/videobuf2-vmalloc.h>
#include "mmal-common.h"
+#include "mmal-parameters.h"
#include "mmal-vchiq.h"
#include "mmal-msg.h"
+#include "vc-sm-cma/vc_sm_knl.h"
+
#define USE_VCHIQ_ARM
#include "interface/vchi/vchi.h"
@@ -419,8 +422,13 @@ buffer_from_host(struct vchiq_mmal_instance *instance,
/* buffer header */
m.u.buffer_from_host.buffer_header.cmd = 0;
- m.u.buffer_from_host.buffer_header.data =
- (u32)(unsigned long)buf->buffer;
+ if (port->zero_copy) {
+ m.u.buffer_from_host.buffer_header.data = buf->vc_handle;
+ } else {
+ m.u.buffer_from_host.buffer_header.data =
+ (u32)(unsigned long)buf->buffer;
+ }
+
m.u.buffer_from_host.buffer_header.alloc_size = buf->buffer_size;
if (port->type == MMAL_PORT_TYPE_OUTPUT) {
m.u.buffer_from_host.buffer_header.length = 0;
@@ -585,6 +593,22 @@ static void buffer_to_host_cb(struct vchiq_mmal_instance *instance,
msg_context->u.bulk.status = msg->h.status;
+ } else if (msg->u.buffer_from_host.is_zero_copy) {
+ /*
+ * Zero copy buffer, so nothing to do.
+ * Copy buffer info and make callback.
+ */
+ msg_context->u.bulk.buffer_used =
+ msg->u.buffer_from_host.buffer_header.length;
+ msg_context->u.bulk.mmal_flags =
+ msg->u.buffer_from_host.buffer_header.flags;
+ msg_context->u.bulk.dts =
+ msg->u.buffer_from_host.buffer_header.dts;
+ msg_context->u.bulk.pts =
+ msg->u.buffer_from_host.buffer_header.pts;
+ msg_context->u.bulk.cmd =
+ msg->u.buffer_from_host.buffer_header.cmd;
+
} else if (msg->u.buffer_from_host.buffer_header.length == 0) {
/* empty buffer */
if (msg->u.buffer_from_host.buffer_header.flags &
@@ -1532,6 +1556,9 @@ int vchiq_mmal_port_parameter_set(struct vchiq_mmal_instance *instance,
mutex_unlock(&instance->vchiq_mutex);
+ if (parameter == MMAL_PARAMETER_ZERO_COPY && !ret)
+ port->zero_copy = !!(*(bool *)value);
+
return ret;
}
EXPORT_SYMBOL_GPL(vchiq_mmal_port_parameter_set);
@@ -1700,6 +1727,31 @@ int vchiq_mmal_submit_buffer(struct vchiq_mmal_instance *instance,
unsigned long flags = 0;
int ret;
+ /*
+ * We really want to do this in mmal_vchi_buffer_init but can't as
+ * videobuf2 won't let us have the dmabuf there.
+ */
+ if (port->zero_copy && buffer->dma_buf && !buffer->vcsm_handle) {
+ pr_debug("%s: import dmabuf %p\n", __func__, buffer->dma_buf);
+ ret = vc_sm_cma_import_dmabuf(buffer->dma_buf,
+ &buffer->vcsm_handle);
+ if (ret) {
+ pr_err("%s: vc_sm_import_dmabuf_fd failed, ret %d\n",
+ __func__, ret);
+ return ret;
+ }
+
+ buffer->vc_handle = vc_sm_cma_int_handle(buffer->vcsm_handle);
+ if (!buffer->vc_handle) {
+ pr_err("%s: vc_sm_int_handle failed %d\n",
+ __func__, ret);
+ vc_sm_cma_free(buffer->vcsm_handle);
+ return ret;
+ }
+ pr_debug("%s: import dmabuf %p - got vc handle %08X\n",
+ __func__, buffer->dma_buf, buffer->vc_handle);
+ }
+
ret = buffer_from_host(instance, port, buffer);
if (ret == -EINVAL) {
/* Port is disabled. Queue for when it is enabled. */
@@ -1733,6 +1785,16 @@ int mmal_vchi_buffer_cleanup(struct mmal_buffer *buf)
release_msg_context(msg_context);
buf->msg_context = NULL;
+ if (buf->vcsm_handle) {
+ int ret;
+
+ pr_debug("%s: vc_sm_cma_free on handle %p\n", __func__,
+ buf->vcsm_handle);
+ ret = vc_sm_cma_free(buf->vcsm_handle);
+ if (ret)
+ pr_err("%s: vcsm_free failed, ret %d\n", __func__, ret);
+ buf->vcsm_handle = 0;
+ }
return 0;
}
EXPORT_SYMBOL_GPL(mmal_vchi_buffer_cleanup);
@@ -49,6 +49,7 @@ typedef void (*vchiq_mmal_buffer_cb)(
struct vchiq_mmal_port {
u32 enabled:1;
+ u32 zero_copy:1;
u32 handle;
u32 type; /* port type, cached to use on port info set */
u32 index; /* port index, cached to use on port info set */