diff mbox series

[v2,05/15] staging: mmal-vchiq: Use vc-sm-cma to support zero copy

Message ID 20231109210309.638594-6-umang.jain@ideasonboard.com
State New
Headers
Series staging: vc04_services: bcm2835-isp support |

Commit Message

Umang Jain Nov. 9, 2023, 9:02 p.m. UTC 
  From: Dave Stevenson <dave.stevenson@raspberrypi.com>

With the vc-sm-cma driver we can support zero copy of buffers between
the kernel and VPU. Add this support to mmal-vchiq.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/vchiq-mmal/Kconfig  |  1 +
 .../vc04_services/vchiq-mmal/mmal-common.h    |  4 +
 .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 84 +++++++++++++++++--
 .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  1 +
 4 files changed, 83 insertions(+), 7 deletions(-)

Comments

Stefan Wahren Nov. 15, 2023, 7:03 p.m. UTC | #1 
Hi Umang,

Am 09.11.23 um 22:02 schrieb Umang Jain:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> With the vc-sm-cma driver we can support zero copy of buffers between
> the kernel and VPU. Add this support to mmal-vchiq.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../staging/vc04_services/vchiq-mmal/Kconfig  |  1 +
>   .../vc04_services/vchiq-mmal/mmal-common.h    |  4 +
>   .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 84 +++++++++++++++++--
>   .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  1 +
>   4 files changed, 83 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/Kconfig b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> index c99525a0bb45..a7c1a7bf516e 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> +++ b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> @@ -1,6 +1,7 @@
>   config BCM2835_VCHIQ_MMAL
>   	tristate "BCM2835 MMAL VCHIQ service"
>   	depends on BCM2835_VCHIQ
> +	select BCM_VC_SM_CMA

i think we need more explanation in the commit message of the relation 
between these both modules.

On the one side BCM_VC_SM_CMA should be a driver, but it's not a driver 
for a specific hardware. It looks like more an extension of VCHIQ MMAL 
or does other (maybe not yet imported) vc04 driver make also use of this.

My question is: is BCM_VC_SM_CMA a real member on the VCHIQ bus and why?

Best regards
Laurent Pinchart Nov. 15, 2023, 7:27 p.m. UTC | #2 
On Wed, Nov 15, 2023 at 08:03:47PM +0100, Stefan Wahren wrote:
> Hi Umang,
> 
> Am 09.11.23 um 22:02 schrieb Umang Jain:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > 
> > With the vc-sm-cma driver we can support zero copy of buffers between
> > the kernel and VPU. Add this support to mmal-vchiq.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >   .../staging/vc04_services/vchiq-mmal/Kconfig  |  1 +
> >   .../vc04_services/vchiq-mmal/mmal-common.h    |  4 +
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 84 +++++++++++++++++--
> >   .../vc04_services/vchiq-mmal/mmal-vchiq.h     |  1 +
> >   4 files changed, 83 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/vchiq-mmal/Kconfig b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> > index c99525a0bb45..a7c1a7bf516e 100644
> > --- a/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> > +++ b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
> > @@ -1,6 +1,7 @@
> >   config BCM2835_VCHIQ_MMAL
> >   	tristate "BCM2835 MMAL VCHIQ service"
> >   	depends on BCM2835_VCHIQ
> > +	select BCM_VC_SM_CMA
> 
> i think we need more explanation in the commit message of the relation 
> between these both modules.
> 
> On the one side BCM_VC_SM_CMA should be a driver, but it's not a driver 
> for a specific hardware. It looks like more an extension of VCHIQ MMAL 
> or does other (maybe not yet imported) vc04 driver make also use of this.
> 
> My question is: is BCM_VC_SM_CMA a real member on the VCHIQ bus and why?

The vchiq_device pointer passed to the vc_sm_cma probe function is used
in the driver for the sole purpose of accessing the underlying struct
device, which, beside being used for dev_*() log calls, is just passed
to dma_buf_attach(). It does indeed appear to be more of a service than
a device.
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/vchiq-mmal/Kconfig b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
index c99525a0bb45..a7c1a7bf516e 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/Kconfig
+++ b/drivers/staging/vc04_services/vchiq-mmal/Kconfig
@@ -1,6 +1,7 @@ 
 config BCM2835_VCHIQ_MMAL
 	tristate "BCM2835 MMAL VCHIQ service"
 	depends on BCM2835_VCHIQ
+	select BCM_VC_SM_CMA
 	help
 	  Enables the MMAL API over VCHIQ interface as used for the
 	  majority of the multimedia services on VideoCore.
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-common.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-common.h
index 0443be8198ea..809634924289 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-common.h
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-common.h
@@ -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;
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 6a270f308cf7..890669f874d9 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -27,9 +27,12 @@ 
 
 #include "../include/linux/raspberrypi/vchiq.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"
+
 /*
  * maximum number of components supported.
  * This matches the maximum permitted by default on the VPU
@@ -416,14 +419,27 @@  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;
-	m.u.buffer_from_host.buffer_header.length = 0;	/* nothing used yet */
-	m.u.buffer_from_host.buffer_header.offset = 0;	/* no offset */
-	m.u.buffer_from_host.buffer_header.flags = 0;	/* no flags */
-	m.u.buffer_from_host.buffer_header.pts = MMAL_TIME_UNKNOWN;
-	m.u.buffer_from_host.buffer_header.dts = MMAL_TIME_UNKNOWN;
+	if (port->type == MMAL_PORT_TYPE_OUTPUT) {
+		m.u.buffer_from_host.buffer_header.length = 0;
+		m.u.buffer_from_host.buffer_header.offset = 0;
+		m.u.buffer_from_host.buffer_header.flags = 0;
+		m.u.buffer_from_host.buffer_header.pts = MMAL_TIME_UNKNOWN;
+		m.u.buffer_from_host.buffer_header.dts = MMAL_TIME_UNKNOWN;
+	} else {
+		m.u.buffer_from_host.buffer_header.length = buf->length;
+		m.u.buffer_from_host.buffer_header.offset = 0;
+		m.u.buffer_from_host.buffer_header.flags = buf->mmal_flags;
+		m.u.buffer_from_host.buffer_header.pts = buf->pts;
+		m.u.buffer_from_host.buffer_header.dts = buf->dts;
+	}
 
 	/* clear buffer type specific data */
 	memset(&m.u.buffer_from_host.buffer_header_type_specific, 0,
@@ -574,6 +590,21 @@  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 &
@@ -1518,6 +1549,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);
@@ -1686,6 +1720,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. */
@@ -1719,6 +1778,17 @@  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);
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
index 7be26cb39165..7aeb38c0c94f 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
@@ -49,6 +49,7 @@  typedef void (*vchiq_mmal_buffer_cb)(
 
 struct vchiq_mmal_port {
 	bool enabled;
+	u32 zero_copy;
 	u32 handle;
 	u32 type; /* port type, cached to use on port info set */
 	u32 index; /* port index, cached to use on port info set */