diff mbox series

[v2,28/47] staging: vchi: Get rid of vchiq_shim's message callback

Message ID 20200629150945.10720-29-nsaenzjulienne@suse.de (mailing list archive)
State Mainlined
Commit 3c5da06f9969959f5cd85e8baaef9d96059b3924
Headers show
Series staging: vchiq: Getting rid of the vchi/vchiq split | expand

Commit Message

Nicolas Saenz Julienne June 29, 2020, 3:09 p.m. UTC 
As vchiq_shim's callback does nothing aside from pushing messages into
the service's queue, let's bypass it and jump directly to the service's
callbacks, letting them choose whether to use the message queue.

It turns out most services don't need to use the message queue, which
makes for simpler code in the end.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../bcm2835-audio/bcm2835-vchiq.c             | 20 ++---
 .../vc04_services/interface/vchi/vchi.h       |  9 +-
 .../interface/vchi/vchi_common.h              | 12 ---
 .../interface/vchiq_arm/vchiq_shim.c          | 42 ++--------
 .../vc04_services/vchiq-mmal/mmal-vchiq.c     | 83 +++++++++----------
 5 files changed, 59 insertions(+), 107 deletions(-)
 delete mode 100644 drivers/staging/vc04_services/interface/vchi/vchi_common.h

Comments

Jacopo Mondi Aug. 28, 2020, 2:31 p.m. UTC | #1 
Hi Nicolas,

   I'm working on a v2 of the bcm2835-isp support which was sent along
with UNICAM v4l2 driver and some misc changes you have collected in
this series. Reference to v1:
https://lore.kernel.org/linux-media/20200504092611.9798-1-laurent.pinchart@ideasonboard.com/

On Mon, Jun 29, 2020 at 05:09:26PM +0200, Nicolas Saenz Julienne wrote:
> As vchiq_shim's callback does nothing aside from pushing messages into
> the service's queue, let's bypass it and jump directly to the service's
> callbacks, letting them choose whether to use the message queue.

I admit this patch caused me some pain, as after a few days chasing
why the ISP got stuck in importing buffers into the VPU through the vc-sm-cma
driver I realized that this patch removed a significant part of the
process..

>
> It turns out most services don't need to use the message queue, which
> makes for simpler code in the end.
>
> -
> -	if (reason == VCHIQ_MESSAGE_AVAILABLE)
> -		vchiq_msg_queue_push(service->handle, header);

This one '-.-

I wonder if this was intentional and it is expected all services now
handle the message queue (it seems so according to your commit
message).

Fair enough, I could add in the vc-sma-cma callback a call to
vchiq_msg_queue_push() but I wonder if it wouldn't be better to do so
in vchiq_core.c:parse_rx_slots(), just before calling the service's
callback, so that this has not to be re-implemented in all services.

What would you suggest ?

And by the way I see mmal-vchiq.c:service_callback() releasing
messages but never pushing them to the queue. Is this intended as well ?

Thanks
  j
Nicolas Saenz Julienne Aug. 31, 2020, 6:46 p.m. UTC | #2 
Hi Jacopo, sorry if I'm a little late with my replies but I'm on vacation. I'll
be back Sept 7th, but wanted to reply since I don't want to stop your work.

On Fri, 2020-08-28 at 16:31 +0200, Jacopo Mondi wrote:
> Hi Nicolas,
> 
>    I'm working on a v2 of the bcm2835-isp support which was sent along
> with UNICAM v4l2 driver and some misc changes you have collected in
> this series. Reference to v1:
> https://lore.kernel.org/linux-media/20200504092611.9798-1-laurent.pinchart@ideasonboard.com/
> 
> On Mon, Jun 29, 2020 at 05:09:26PM +0200, Nicolas Saenz Julienne wrote:
> > As vchiq_shim's callback does nothing aside from pushing messages into
> > the service's queue, let's bypass it and jump directly to the service's
> > callbacks, letting them choose whether to use the message queue.
> 
> I admit this patch caused me some pain, as after a few days chasing
> why the ISP got stuck in importing buffers into the VPU through the vc-sm-cma
> driver I realized that this patch removed a significant part of the
> process..

Sorry for the pain, I made my best to keep the downstream code in mind, and
also to keep the amount of functional changes needed in the services minimal.
That said, getting rid of VCHI is, IMO, a necessary step towards making VCHIQ
upstreamable.

> > It turns out most services don't need to use the message queue, which
> > makes for simpler code in the end.
> > 
> > -
> > -	if (reason == VCHIQ_MESSAGE_AVAILABLE)
> > -		vchiq_msg_queue_push(service->handle, header);
> 
> This one '-.-
> 
> I wonder if this was intentional and it is expected all services now
> handle the message queue (it seems so according to your commit
> message).

Indeed, it was intentional. Upstream services (mmal & audio) don't need it and
IIRC vchiq's ioctl interface has it's own queue implementation. So I figured it
would be best to keep its usage optional.

> Fair enough, I could add in the vc-sma-cma callback a call to
> vchiq_msg_queue_push() but I wonder if it wouldn't be better to do so
> in vchiq_core.c:parse_rx_slots(), just before calling the service's
> callback, so that this has not to be re-implemented in all services.
> 
> What would you suggest ?

Actually, in hindsight my suggestion would be to get rid of the vchiq message
queue altogether[1], keeping VCHIQ as simple as possible is a must if we want
to get it upstream, and since vc-sma-cma is the only queue user there is little
benefit to having a generic implementation. Let the service do it's own custom
queueing and just force all services to release the messages when they see fit.
It'll make for a simpler VCHIQ usage.

> And by the way I see mmal-vchiq.c:service_callback() releasing
> messages but never pushing them to the queue. Is this intended as well ?

Yes, sorry, it's pretty confusing. That call, vchiq_release_message(), has
nothing to do with the queue. It's the call that really gets the core to
release the message from its slot and the only mandatory thing to do after
receiving a message.

All in all VCHIQ isn't documented an pretty cryptic in its design. I don't
claim to be an expert. So feel free to contradict me anytime, I'll be happy to
work out something that fits all of us.

Regards,
Nicolas

[1] Here I mean vchiq_msg_queue_push() & vchiq_msg_hold()
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 0f97eda4ec90..71750ef891dd 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -89,22 +89,17 @@  static int bcm2835_audio_send_simple(struct bcm2835_audio_instance *instance,
 	return bcm2835_audio_send_msg(instance, &m, wait);
 }
 
-static void audio_vchi_callback(void *param, const enum vchiq_reason reason,
-				void *msg_handle)
+static enum vchiq_status audio_vchi_callback(enum vchiq_reason reason,
+					     struct vchiq_header *header,
+					     unsigned handle, void *userdata)
 {
-	struct bcm2835_audio_instance *instance = param;
-	struct vchi_held_msg handle;
+	struct bcm2835_audio_instance *instance = vchiq_get_service_userdata(handle);
 	struct vc_audio_msg *m;
-	unsigned size;
-	int status;
 
 	if (reason != VCHIQ_MESSAGE_AVAILABLE)
-		return;
-
-	status = vchi_msg_hold(instance->service, (void **)&m, &size, &handle);
-	if (status)
-		return;
+		return VCHIQ_SUCCESS;
 
+	m = (void *)header->data;
 	if (m->type == VC_AUDIO_MSG_TYPE_RESULT) {
 		instance->result = m->result.success;
 		complete(&instance->msg_avail_comp);
@@ -119,7 +114,8 @@  static void audio_vchi_callback(void *param, const enum vchiq_reason reason,
 		dev_err(instance->dev, "unexpected callback type=%d\n", m->type);
 	}
 
-	vchi_held_msg_release(&handle);
+	vchiq_release_message(handle, header);
+	return VCHIQ_SUCCESS;
 }
 
 static int
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 789acb7999d1..fdc243f3f60a 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -4,8 +4,6 @@ 
 #ifndef VCHI_H_
 #define VCHI_H_
 
-#include "vchi_common.h"
-
 /******************************************************************************
  * Global defs
  *****************************************************************************/
@@ -31,16 +29,13 @@  struct vchi_held_msg {
 
 struct vchi_service {
 	unsigned int handle;
-
-	vchi_callback callback;
-	void *callback_param;
 };
 
 // structure used to provide the information needed to open a server or a client
 struct service_creation {
 	struct vchi_version version;
 	int32_t service_id;
-	vchi_callback callback;
+	vchiq_callback callback;
 	void *callback_param;
 };
 
@@ -88,7 +83,7 @@  extern int vchi_queue_kernel_message(struct vchi_service *service, void *data,
 // Routine to look at a message in place.
 // The message is dequeued, so the caller is left holding it; the descriptor is
 // filled in and must be released when the user has finished with the message.
-extern int32_t vchi_msg_hold(struct vchi_service *service,
+extern int32_t vchi_msg_hold(unsigned handle,
 			     void **data,        // } may be NULL, as info can be
 			     uint32_t *msg_size, // } obtained from HELD_MSG_T
 			     struct vchi_held_msg *message_descriptor);
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi_common.h b/drivers/staging/vc04_services/interface/vchi/vchi_common.h
deleted file mode 100644
index 24e8665e214a..000000000000
--- a/drivers/staging/vc04_services/interface/vchi/vchi_common.h
+++ /dev/null
@@ -1,12 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
-/* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
-
-#ifndef VCHI_COMMON_H_
-#define VCHI_COMMON_H_
-
-//Callback used by all services / bulk transfers
-typedef void (*vchi_callback)(void *callback_param, //my service local param
-			      enum vchiq_reason reason,
-			      void *handle); //for transmitting msg's only
-
-#endif // VCHI_COMMON_H_
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index bec78f0ff44b..fb9fd5fec42c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -158,12 +158,12 @@  EXPORT_SYMBOL(vchi_held_msg_release);
  * Returns: int32_t - success == 0
  *
  ***********************************************************/
-int32_t vchi_msg_hold(struct vchi_service *service, void **data,
-		      uint32_t *msg_size, struct vchi_held_msg *message_handle)
+int32_t vchi_msg_hold(unsigned handle, void **data, uint32_t *msg_size,
+		      struct vchi_held_msg *message_handle)
 {
 	struct vchiq_header *header;
 
-	header = vchiq_msg_hold(service->handle);
+	header = vchiq_msg_hold(handle);
 	if (!header)
 		return -ENOENT;
 
@@ -179,7 +179,7 @@  int32_t vchi_msg_hold(struct vchi_service *service, void **data,
 	 */
 
 	message_handle->service =
-		(struct opaque_vchi_service_t *)(long)service->handle;
+		(struct opaque_vchi_service_t *)(long)handle;
 	message_handle->message = header;
 
 	return 0;
@@ -253,33 +253,9 @@  EXPORT_SYMBOL(vchi_disconnect);
  *
  ***********************************************************/
 
-static enum vchiq_status shim_callback(enum vchiq_reason reason,
-				    struct vchiq_header *header,
-				    unsigned int handle,
-				    void *bulk_user)
+static struct vchi_service *service_alloc(void)
 {
-	struct vchi_service *service =
-		(struct vchi_service *)VCHIQ_GET_SERVICE_USERDATA(handle);
-
-	if (reason == VCHIQ_MESSAGE_AVAILABLE)
-		vchiq_msg_queue_push(service->handle, header);
-
-	service->callback(service->callback_param, reason, bulk_user);
-
-	return VCHIQ_SUCCESS;
-}
-
-static struct vchi_service *service_alloc(struct vchiq_instance *instance,
-	struct service_creation *setup)
-{
-	struct vchi_service *service = kzalloc(sizeof(struct vchi_service), GFP_KERNEL);
-
-	if (service) {
-		service->callback = setup->callback;
-		service->callback_param = setup->callback_param;
-	}
-
-	return service;
+	return kzalloc(sizeof(struct vchi_service), GFP_KERNEL);
 }
 
 static void service_free(struct vchi_service *service)
@@ -293,15 +269,15 @@  int32_t vchi_service_open(struct vchiq_instance *instance,
 	struct vchi_service **service)
 {
 
-	*service = service_alloc(instance, setup);
+	*service = service_alloc();
 	if (*service) {
 		struct vchiq_service_params params;
 		enum vchiq_status status;
 
 		memset(&params, 0, sizeof(params));
 		params.fourcc = setup->service_id;
-		params.callback = shim_callback;
-		params.userdata = *service;
+		params.callback = setup->callback;
+		params.userdata = setup->callback_param;
 		params.version = setup->version.version;
 		params.version_min = setup->version.version_min;
 
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index f710ccccb14c..2abf0f6664f9 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -152,7 +152,7 @@  struct mmal_msg_context {
 
 		struct {
 			/* message handle to release */
-			struct vchi_held_msg msg_handle;
+			struct vchiq_header *msg_handle;
 			/* pointer to received message */
 			struct mmal_msg *msg;
 			/* received message length */
@@ -549,54 +549,49 @@  static void bulk_abort_cb(struct vchiq_mmal_instance *instance,
 }
 
 /* incoming event service callback */
-static void service_callback(void *param, const enum vchiq_reason reason,
-			     void *bulk_ctx)
+static enum vchiq_status service_callback(enum vchiq_reason reason,
+					  struct vchiq_header *header,
+					  unsigned handle, void *bulk_ctx)
 {
-	struct vchiq_mmal_instance *instance = param;
-	int status;
+	struct vchiq_mmal_instance *instance = vchiq_get_service_userdata(handle);
 	u32 msg_len;
 	struct mmal_msg *msg;
-	struct vchi_held_msg msg_handle;
 	struct mmal_msg_context *msg_context;
 
 	if (!instance) {
 		pr_err("Message callback passed NULL instance\n");
-		return;
+		return VCHIQ_SUCCESS;
 	}
 
 	switch (reason) {
 	case VCHIQ_MESSAGE_AVAILABLE:
-		status = vchi_msg_hold(instance->service, (void **)&msg,
-				       &msg_len, &msg_handle);
-		if (status) {
-			pr_err("Unable to dequeue a message (%d)\n", status);
-			break;
-		}
+		msg = (void *)header->data;
+		msg_len = header->size;
 
 		DBG_DUMP_MSG(msg, msg_len, "<<< reply message");
 
 		/* handling is different for buffer messages */
 		switch (msg->h.type) {
 		case MMAL_MSG_TYPE_BUFFER_FROM_HOST:
-			vchi_held_msg_release(&msg_handle);
+			vchiq_release_message(handle, header);
 			break;
 
 		case MMAL_MSG_TYPE_EVENT_TO_HOST:
 			event_to_host_cb(instance, msg, msg_len);
-			vchi_held_msg_release(&msg_handle);
+			vchiq_release_message(handle, header);
 
 			break;
 
 		case MMAL_MSG_TYPE_BUFFER_TO_HOST:
 			buffer_to_host_cb(instance, msg, msg_len);
-			vchi_held_msg_release(&msg_handle);
+			vchiq_release_message(handle, header);
 			break;
 
 		default:
 			/* messages dependent on header context to complete */
 			if (!msg->h.context) {
 				pr_err("received message context was null!\n");
-				vchi_held_msg_release(&msg_handle);
+				vchiq_release_message(handle, header);
 				break;
 			}
 
@@ -605,12 +600,12 @@  static void service_callback(void *param, const enum vchiq_reason reason,
 			if (!msg_context) {
 				pr_err("received invalid message context %u!\n",
 				       msg->h.context);
-				vchi_held_msg_release(&msg_handle);
+				vchiq_release_message(handle, header);
 				break;
 			}
 
 			/* fill in context values */
-			msg_context->u.sync.msg_handle = msg_handle;
+			msg_context->u.sync.msg_handle = header;
 			msg_context->u.sync.msg = msg;
 			msg_context->u.sync.msg_len = msg_len;
 
@@ -648,13 +643,15 @@  static void service_callback(void *param, const enum vchiq_reason reason,
 		pr_err("Received unhandled message reason %d\n", reason);
 		break;
 	}
+
+	return VCHIQ_SUCCESS;
 }
 
 static int send_synchronous_mmal_msg(struct vchiq_mmal_instance *instance,
 				     struct mmal_msg *msg,
 				     unsigned int payload_len,
 				     struct mmal_msg **msg_out,
-				     struct vchi_held_msg *msg_handle_out)
+				     struct vchiq_header **msg_handle)
 {
 	struct mmal_msg_context *msg_context;
 	int ret;
@@ -708,7 +705,7 @@  static int send_synchronous_mmal_msg(struct vchiq_mmal_instance *instance,
 	}
 
 	*msg_out = msg_context->u.sync.msg;
-	*msg_handle_out = msg_context->u.sync.msg_handle;
+	*msg_handle = msg_context->u.sync.msg_handle;
 	release_msg_context(msg_context);
 
 	return 0;
@@ -780,7 +777,7 @@  static int port_info_set(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	pr_debug("setting port info port %p\n", port);
 	if (!port)
@@ -829,7 +826,7 @@  static int port_info_set(struct vchiq_mmal_instance *instance,
 		 port->component->handle, port->handle);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -841,7 +838,7 @@  static int port_info_get(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	/* port info time */
 	m.h.type = MMAL_MSG_TYPE_PORT_INFO_GET;
@@ -924,7 +921,7 @@  static int port_info_get(struct vchiq_mmal_instance *instance,
 	pr_debug("%s:result:%d component:0x%x port:%d\n",
 		 __func__, ret, port->component->handle, port->handle);
 
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -937,7 +934,7 @@  static int create_component(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	/* build component create message */
 	m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
@@ -972,7 +969,7 @@  static int create_component(struct vchiq_mmal_instance *instance,
 		 component->inputs, component->outputs, component->clocks);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -984,7 +981,7 @@  static int destroy_component(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_COMPONENT_DESTROY;
 	m.u.component_destroy.component_handle = component->handle;
@@ -1005,7 +1002,7 @@  static int destroy_component(struct vchiq_mmal_instance *instance,
 
 release_msg:
 
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1017,7 +1014,7 @@  static int enable_component(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_COMPONENT_ENABLE;
 	m.u.component_enable.component_handle = component->handle;
@@ -1037,7 +1034,7 @@  static int enable_component(struct vchiq_mmal_instance *instance,
 	ret = -rmsg->u.component_enable_reply.status;
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1049,7 +1046,7 @@  static int disable_component(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_COMPONENT_DISABLE;
 	m.u.component_disable.component_handle = component->handle;
@@ -1070,7 +1067,7 @@  static int disable_component(struct vchiq_mmal_instance *instance,
 
 release_msg:
 
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1082,7 +1079,7 @@  static int get_version(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_GET_VERSION;
 
@@ -1102,7 +1099,7 @@  static int get_version(struct vchiq_mmal_instance *instance,
 	*minor_out = rmsg->u.version.minor;
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1115,7 +1112,7 @@  static int port_action_port(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_PORT_ACTION;
 	m.u.port_action_port.component_handle = port->component->handle;
@@ -1144,7 +1141,7 @@  static int port_action_port(struct vchiq_mmal_instance *instance,
 		 port_action_type_names[action_type], action_type);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1159,7 +1156,7 @@  static int port_action_handle(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_PORT_ACTION;
 
@@ -1192,7 +1189,7 @@  static int port_action_handle(struct vchiq_mmal_instance *instance,
 		 action_type, connect_component_handle, connect_port_handle);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1204,7 +1201,7 @@  static int port_parameter_set(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_PORT_PARAMETER_SET;
 
@@ -1233,7 +1230,7 @@  static int port_parameter_set(struct vchiq_mmal_instance *instance,
 		 ret, port->component->handle, port->handle, parameter_id);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }
@@ -1245,7 +1242,7 @@  static int port_parameter_get(struct vchiq_mmal_instance *instance,
 	int ret;
 	struct mmal_msg m;
 	struct mmal_msg *rmsg;
-	struct vchi_held_msg rmsg_handle;
+	struct vchiq_header *rmsg_handle;
 
 	m.h.type = MMAL_MSG_TYPE_PORT_PARAMETER_GET;
 
@@ -1292,7 +1289,7 @@  static int port_parameter_get(struct vchiq_mmal_instance *instance,
 		 ret, port->component->handle, port->handle, parameter_id);
 
 release_msg:
-	vchi_held_msg_release(&rmsg_handle);
+	vchiq_release_message(instance->service->handle, rmsg_handle);
 
 	return ret;
 }