drm: apple: av: let the AV endpoint's AFK slots be reused

drivers/gpu/drm/apple/afk.c

@@ -52,6 +52,9 @@ enum rbep_msg_type {
 
 #define SEND_WPTR GENMASK(31, 0)
 
+/* retcode of a reply too short to carry a nonzero IOReturn */
+#define AFK_RETCODE_SHORT_REPLY 0xffffffff
+
 static void afk_service_worker(struct work_struct *work);
 static void afk_discard_pending_open(struct apple_epic_service *service);
 
@@ -413,6 +416,7 @@ static void afk_recv_handle_init(struct apple_dcp_afkep *ep, u32 channel,
 	struct apple_epic_service *service = NULL, *previous;
 	unsigned long flags;
 	bool retired = false;
+	bool reused = false;
 	u32 ch_idx;
 	const char *service_name = name;
 	const char *epic_name = NULL, *epic_class = NULL;
@@ -491,6 +495,8 @@ static void afk_recv_handle_init(struct apple_dcp_afkep *ep, u32 channel,
 	for (ch_idx = 0; ch_idx < AFK_MAX_CHANNEL; ch_idx++) {
 		struct apple_epic_service *candidate = &ep->services[ch_idx];
 
+		/* only this rx worker sets ops: no lock needed for the log */
+		reused = !!candidate->ops;
 		if (afk_service_reinit(candidate, ep, ops, channel)) {
 			service = candidate;
 			break;
@@ -550,8 +556,9 @@ static void afk_recv_handle_init(struct apple_dcp_afkep *ep, u32 channel,
 		afk_service_publish(service);
 	if (ops->init)
 		ops->init(service, epic_name, epic_class, epic_unit);
-	dev_info(ep->dcp->dev, "AFK[ep:%02x]: new service %s on channel %d\n",
-		 ep->endpoint, service_name, channel);
+	dev_info(ep->dcp->dev, "AFK[ep:%02x]: new service %s on channel %d, slot %u%s\n",
+		 ep->endpoint, service_name, channel, ch_idx,
+		 reused ? " (reused)" : "");
 
 	afk_populate_service_debugfs(service);
 
@@ -605,6 +612,7 @@ static void afk_recv_handle_reply(struct apple_dcp_afkep *ep, u32 channel,
 	struct apple_epic_service *service;
 	unsigned long flags;
 	u8 idx = tag & (ep->wire_v2 ? 0xf : 0xff);
+	u32 retcode;
 	void *rxbuf, *txbuf;
 	dma_addr_t rxbuf_dma, txbuf_dma;
 	size_t rxlen, txlen;
@@ -616,11 +624,21 @@ static void afk_recv_handle_reply(struct apple_dcp_afkep *ep, u32 channel,
 		return;
 	}
 
+	/*
+	 * A reply shorter than the command header completes the command as
+	 * failed, so that its caller returns and a timed-out command's
+	 * buffers and cmd_map bit are freed. The 4-byte replies seen look
+	 * like an IOReturn: a nonzero one is passed on.
+	 */
 	if (payload_size < sizeof(*cmd)) {
 		dev_err(ep->dcp->dev,
 			"AFK[ep:%02x]: command reply on channel %d too small: %ld\n",
 			ep->endpoint, channel, payload_size);
-		return;
+		retcode = payload_size >= sizeof(cmd->retcode) ?
+			  le32_to_cpu(cmd->retcode) : 0;
+		retcode = retcode ?: AFK_RETCODE_SHORT_REPLY;
+	} else {
+		retcode = le32_to_cpu(cmd->retcode);
 	}
 
 	if (idx >= MAX_PENDING_CMDS) {
@@ -648,7 +666,7 @@ static void afk_recv_handle_reply(struct apple_dcp_afkep *ep, u32 channel,
 	}
 
 	service->cmds[idx].done = true;
-	service->cmds[idx].retcode = le32_to_cpu(cmd->retcode);
+	service->cmds[idx].retcode = retcode;
 	if (service->cmds[idx].free_on_ack) {
 		/* defer freeing until we're no longer in atomic context */
 		rxbuf = service->cmds[idx].rxbuf;

drivers/gpu/drm/apple/av.c

@@ -67,7 +67,11 @@ struct audiosrv_data {
 
 	bool warned_get_elements;
 	bool warned_get_product_attrs;
-	bool is_open;
+	/* generation of srv, and the one that was opened (0: none) */
+	u64 gen;
+	u64 open_gen;
+	/* serialises the open and the close; never taken on the AFK rx worker */
+	struct mutex open_lock;
 };
 
 static void av_interface_init(struct apple_epic_service *service, const char *name,
@@ -96,12 +100,23 @@ static void av_audiosrv_init(struct apple_epic_service *service, const char *nam
 {
 	struct apple_dcp *dcp = service->ep->dcp;
 	struct audiosrv_data *asrv = dcp->audiosrv;
+	struct apple_epic_service *old;
 
 	mutex_lock(&asrv->plug_lock);
 
+	/*
+	 * An owner pin keeps the slot from being reused while srv points to
+	 * it. DCP tears the service down before announcing it again.
+	 */
 	down_write(&asrv->srv_rwsem);
-	asrv->srv = service;
+	old = asrv->srv;
+	asrv->srv = afk_service_get(service);
+	asrv->gen++;
 	up_write(&asrv->srv_rwsem);
+	if (old) {
+		dev_warn_once(dcp->dev, "audiosrv: announced again without a teardown\n");
+		afk_service_put(old);
+	}
 
 	asrv->plugged = true;
 	mutex_unlock(&asrv->plug_lock);
@@ -112,18 +127,48 @@ static void av_audiosrv_teardown(struct apple_epic_service *service)
 {
 	struct apple_dcp *dcp = service->ep->dcp;
 	struct audiosrv_data *asrv = dcp->audiosrv;
+	bool owned;
 
 	mutex_lock(&asrv->plug_lock);
 
 	down_write(&asrv->srv_rwsem);
-	asrv->srv = NULL;
+	owned = asrv->srv == service;
+	if (owned) {
+		asrv->srv = NULL;
+	} else if (asrv->srv) {
+		/* announced again without a teardown: the newer one is live */
+		up_write(&asrv->srv_rwsem);
+		mutex_unlock(&asrv->plug_lock);
+		return;
+	}
 	up_write(&asrv->srv_rwsem);
 
 	asrv->plugged = false;
 	if (asrv->audio_dev)
 		dcpaud_disconnect(asrv->audio_dev);
 
 	mutex_unlock(&asrv->plug_lock);
+
+	if (owned)
+		afk_service_put(service);
+}
+
+/* after afk_shutdown() on unbind: drop the owner pin */
+void av_service_detach(struct apple_dcp *dcp)
+{
+	struct audiosrv_data *asrv = dcp->audiosrv;
+	struct apple_epic_service *service;
+
+	if (!asrv)
+		return;
+	scoped_guard(rwsem_write, &asrv->srv_rwsem) {
+		service = asrv->srv;
+		asrv->srv = NULL;
+	}
+	if (service) {
+		afk_service_disable(service);
+		afk_service_put(service);
+	}
 }
 
 int dcp_audiosrv_prepare(struct device *dev, struct dcp_sound_cookie *cookie)
@@ -279,11 +324,15 @@ static int av_audiosrv_report(struct apple_epic_service *service, u32 idx,
 static const struct apple_epic_service_ops avep_ops[] = {
 	{
 		.name = "DCPAVSimpleVideoInterface",
+		/* keeps no pointer to the service */
+		.reusable = true,
 		.init = av_interface_init,
 		.teardown = av_interface_teardown,
 	},
 	{
 		.name = "DCPAVAudioInterface",
+		/* pinned by asrv->srv, and by each call made without srv_rwsem */
+		.reusable = true,
 		.init = av_audiosrv_init,
 		.report = av_audiosrv_report,
 		.teardown = av_audiosrv_teardown,
@@ -295,49 +344,69 @@ void av_service_connect(struct apple_dcp *dcp)
 {
 	struct apple_epic_service *service;
 	struct audiosrv_data *asrv = dcp->audiosrv;
+	u64 gen;
 	int ret;
 
-	scoped_guard(rwsem_write, &asrv->srv_rwsem) {
-		if (!asrv->srv)
+	/* pinned for the open below, made without srv_rwsem */
+	scoped_guard(rwsem_read, &asrv->srv_rwsem) {
+		service = afk_service_get(asrv->srv);
+		if (!service)
 			return;
-		service = asrv->srv;
+		gen = asrv->gen;
 	}
 
 	/* open AV audio service */
 	dev_info(dcp->dev, "%s: starting audio service, plugged:%d\n", __func__,  asrv->plugged);
-	if (asrv->is_open)
+	mutex_lock(&asrv->open_lock);
+	if (asrv->open_gen == gen) {
+		mutex_unlock(&asrv->open_lock);
+		afk_service_put(service);
 		return;
+	}
 
 	ret = afk_service_call(service, 0, asrv->cmds.open, NULL, 0, 32,
 			       NULL, 0, 32);
+	afk_service_put(service);
 	if (ret) {
+		mutex_unlock(&asrv->open_lock);
 		dev_err(dcp->dev, "error opening audio service: %d\n", ret);
 		return;
 	}
 	mutex_lock(&asrv->plug_lock);
-	asrv->is_open = true;
-
-	if (asrv->audio_dev)
-		dcpaud_connect(asrv->audio_dev, asrv->plugged);
+	/* torn down during the open: the next generation is opened afresh */
+	if (asrv->gen == gen) {
+		asrv->open_gen = gen;
+		if (asrv->audio_dev)
+			dcpaud_connect(asrv->audio_dev, asrv->plugged);
+	}
 	mutex_unlock(&asrv->plug_lock);
+	mutex_unlock(&asrv->open_lock);
 }
 
 void av_service_disconnect(struct apple_dcp *dcp)
 {
 	struct apple_epic_service *service;
 	struct audiosrv_data *asrv = dcp->audiosrv;
+	u64 gen;
 	int ret;
 
-	scoped_guard(rwsem_write, &asrv->srv_rwsem) {
-		if (!asrv->srv)
+	/* pinned for the close below, made without srv_rwsem */
+	scoped_guard(rwsem_read, &asrv->srv_rwsem) {
+		service = afk_service_get(asrv->srv);
+		if (!service)
 			return;
-		service = asrv->srv;
+		gen = asrv->gen;
 	}
 
 	/* close AV audio service */
 	dev_info(dcp->dev, "%s: stopping audio service\n", __func__);
-	if (!asrv->is_open)
+	mutex_lock(&asrv->open_lock);
+	if (asrv->open_gen != gen) {
+		mutex_unlock(&asrv->open_lock);
+		afk_service_put(service);
 		return;
+	}
+	asrv->open_gen = 0;
 
 	mutex_lock(&asrv->plug_lock);
 
@@ -351,9 +420,8 @@ void av_service_disconnect(struct apple_dcp *dcp)
 	if (ret) {
 		dev_err(dcp->dev, "error closing audio service: %d\n", ret);
 	}
-	if (service->torndown)
-		afk_service_disable(service);
-	asrv->is_open = false;
+	afk_service_put(service);
+	mutex_unlock(&asrv->open_lock);
 }
 
 static void av_work_service_start(struct work_struct *work)
@@ -388,6 +456,7 @@ int avep_init(struct apple_dcp *dcp)
 		return -ENOMEM;
 	init_rwsem(&audiosrv_data->srv_rwsem);
 	mutex_init(&audiosrv_data->plug_lock);
+	mutex_init(&audiosrv_data->open_lock);
 
 	switch (dcp->fw_compat) {
 	case DCP_FIRMWARE_V_12_3:

drivers/gpu/drm/apple/av.h

@@ -9,9 +9,11 @@
 #if IS_ENABLED(CONFIG_DRM_APPLE_AUDIO)
 void av_service_connect(struct apple_dcp *dcp);
 void av_service_disconnect(struct apple_dcp *dcp);
+void av_service_detach(struct apple_dcp *dcp);
 #else
 static inline void av_service_connect(struct apple_dcp *dcp) { }
 static inline void av_service_disconnect(struct apple_dcp *dcp) { }
+static inline void av_service_detach(struct apple_dcp *dcp) { }
 #endif
 
 #endif /* __AV_H__ */

drivers/gpu/drm/apple/dcp.c

@@ -4245,6 +4245,7 @@ static void dcp_comp_unbind(struct device *dev, struct device *main, void *data)
 	if (dcp->avep) {
 		av_service_disconnect(dcp);
 		afk_shutdown(dcp->avep);
+		av_service_detach(dcp);
 		dcp->avep = NULL;
 	}