Re: [PATCH 04/13] drm/msm/dcp: Add IPCC power notification

[Dmitry Baryshkov]

On Thu, Oct 15, 2026 at 01:08:24PM +0530, Jayant Shekhar wrote:
> DCP may be power-collapsed while the VirtIO transport remains available.
> The host therefore needs the platform IPCC channel to wake the firmware and
> to receive notification that it can service HFI queues.

Is it allowed per the VirtIO spec?

> 
> Add the IPCC mailbox client and power-notify interrupt handling. Wake DCP
> before VirtIO queue activity, wait for the firmware acknowledgment, and
> notify the response queue when the IPCC receive path completes.

Don't describe patch contents. Describe your design decisions. The
reasons for the flags, init/deinit functions, etc.

> 
> Signed-off-by: Jayant Shekhar <jayant.shekhar@xxxxxxxxxxxxxxxx>
> Assisted-by: LLM
> ---
>  drivers/gpu/drm/msm/Makefile       |   1 +
>  drivers/gpu/drm/msm/dcp/dcp_drv.h  |   1 +
>  drivers/gpu/drm/msm/dcp/dcp_ipcc.c | 212 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dcp/dcp_ipcc.h |  56 ++++++++++
>  4 files changed, 270 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index bdc34babfc21..4195bc0e1fc9 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -71,6 +71,7 @@ msm-display-$(CONFIG_DRM_MSM_MDP5) += \
>  
>  msm-display-$(CONFIG_DRM_MSM_DCP) += \
>  	dcp/dcp_hfi_core.o \
> +	dcp/dcp_ipcc.o \
>  	dcp/hfi/hfi_packer.o \
>  	dcp/hfi/hfi_unpacker.o \
>  	dcp/hfi/hfi_utils.o
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_drv.h b/drivers/gpu/drm/msm/dcp/dcp_drv.h
> index bf6c34af43a0..af2db3cf9066 100644
> --- a/drivers/gpu/drm/msm/dcp/dcp_drv.h
> +++ b/drivers/gpu/drm/msm/dcp/dcp_drv.h
> @@ -13,6 +13,7 @@
>  #include <linux/virtio.h>
>  
>  #include "dcp_hfi.h"
> +#include "dcp_ipcc.h"
>  
>  struct dcp_dpu_kms;
>  
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_ipcc.c b/drivers/gpu/drm/msm/dcp/dcp_ipcc.c
> new file mode 100644
> index 000000000000..e60a3ecbe3d8
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_ipcc.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * DCP IPCC power-notify mailbox client.
> + *
> + * The APSS-to-DCP channel must be signaled before a VirtIO queue kick.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/jiffies.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/of_irq.h>
> +#include <linux/string.h>
> +#include <linux/virtio.h>
> +
> +#include "dcp_ipcc.h"
> +
> +#define DCP_IPCC_POWER_NOTIFY_TIMEOUT_MS	10000
> +
> +static void dcp_ipcc_handle_rx(struct dcp_ipcc *ipcc)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ipcc->state_lock, flags);
> +	if (ipcc->shutting_down) {
> +		spin_unlock_irqrestore(&ipcc->state_lock, flags);
> +		return;
> +	}
> +
> +	if (atomic_xchg(&ipcc->power_notify_pending, 0))
> +		complete(&ipcc->power_notify_done);

Can there be more than one power notification pending? Can't we just
call complete() here?

> +
> +	if (ipcc->resp_vq && virtqueue_kick_prepare(ipcc->resp_vq))
> +		virtqueue_notify(ipcc->resp_vq);
> +	spin_unlock_irqrestore(&ipcc->state_lock, flags);
> +}
> +
> +static irqreturn_t dcp_ipcc_power_notify_irq(int irq, void *data)
> +{
> +	struct dcp_ipcc *ipcc = data;
> +
> +	dcp_ipcc_handle_rx(ipcc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void dcp_ipcc_rx_callback(struct mbox_client *cl, void *data)
> +{
> +	struct dcp_ipcc *ipcc = container_of(cl, struct dcp_ipcc, client);
> +
> +	dcp_ipcc_handle_rx(ipcc);
> +}
> +
> +int dcp_ipcc_init(struct dcp_ipcc *ipcc, struct device *dev)
> +{
> +	int irq, ret;
> +
> +	if (!ipcc || !dev)
> +		return -EINVAL;
> +
> +	memset(ipcc, 0, sizeof(*ipcc));
> +	init_completion(&ipcc->power_notify_done);
> +	mutex_init(&ipcc->power_notify_lock);
> +	spin_lock_init(&ipcc->state_lock);
> +	atomic_set(&ipcc->power_notify_pending, 0);
> +
> +	ipcc->client.dev          = dev;
> +	ipcc->client.tx_block     = false;
> +	ipcc->client.tx_tout      = 0;
> +	ipcc->client.knows_txdone = true;
> +	ipcc->client.rx_callback  = dcp_ipcc_rx_callback;
> +
> +	ipcc->power_chan = mbox_request_channel_byname(&ipcc->client,
> +						       "dcp-power-tx");
> +	if (IS_ERR(ipcc->power_chan)) {
> +		ret = PTR_ERR(ipcc->power_chan);
> +		ipcc->power_chan = NULL;
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "IPCC: failed to get 'dcp-power-tx': %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = of_irq_get_byname(dev->of_node, "dcp_power_notify_irq");
> +	if (irq <= 0) {
> +		ret = irq ? irq : -EINVAL;
> +		ret = dev_err_probe(dev, ret,
> +				    "failed to get required power notify IRQ\n");
> +		goto err_free_channel;
> +	}
> +
> +	ret = request_irq(irq, dcp_ipcc_power_notify_irq,
> +			  0, "dcp-power-notify", ipcc);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret,
> +				    "failed to request power notify IRQ %d\n", irq);
> +		goto err_free_channel;
> +	}
> +	ipcc->power_notify_irq = irq;
> +
> +	ret = enable_irq_wake(irq);

This is typically called from the suspend callback. Why are you calling
it in init path?

> +	if (ret) {
> +		dev_err(dev, "failed to enable power notify IRQ wake: %d\n", ret);
> +		goto err_free_irq;
> +	}
> +	ipcc->power_notify_irq_wake = true;
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(irq, ipcc);
> +	ipcc->power_notify_irq = 0;
> +err_free_channel:
> +	mbox_free_channel(ipcc->power_chan);
> +	ipcc->power_chan = NULL;
> +	return ret;
> +}
> +
> +void dcp_ipcc_set_resp_vq(struct dcp_ipcc *ipcc, struct virtqueue *resp_vq)
> +{
> +	unsigned long flags;
> +
> +	if (!ipcc)
> +		return;
> +
> +	spin_lock_irqsave(&ipcc->state_lock, flags);
> +	if (!ipcc->shutting_down)
> +		ipcc->resp_vq = resp_vq;
> +	spin_unlock_irqrestore(&ipcc->state_lock, flags);
> +}
> +
> +void dcp_ipcc_deinit(struct dcp_ipcc *ipcc)
> +{
> +	struct mbox_chan *power_chan;
> +	int irq;
> +	bool irq_wake;
> +	unsigned long flags;
> +
> +	if (!ipcc)
> +		return;

Oh, you again ignored the feedback. Can ipcc be NULL here? What would
that mean?

> +
> +	mutex_lock(&ipcc->power_notify_lock);
> +	spin_lock_irqsave(&ipcc->state_lock, flags);
> +	ipcc->shutting_down = true;
> +	ipcc->resp_vq = NULL;
> +	irq = ipcc->power_notify_irq;
> +	ipcc->power_notify_irq = 0;
> +	irq_wake = ipcc->power_notify_irq_wake;
> +	ipcc->power_notify_irq_wake = false;
> +	power_chan = ipcc->power_chan;
> +	ipcc->power_chan = NULL;
> +	spin_unlock_irqrestore(&ipcc->state_lock, flags);

Ugh. No.

> +
> +	if (irq > 0) {

Can it be 0 here?

> +		disable_irq(irq);

What for?

> +		if (irq_wake)
> +			disable_irq_wake(irq);

It is enabled at this point. Drop the useless flags.

> +		free_irq(irq, ipcc);

We have devm_foo() functions to simplify the teardown. Can we use them
here?

> +	}
> +	if (power_chan)
> +		mbox_free_channel(power_chan);
> +	mutex_unlock(&ipcc->power_notify_lock);
> +}
> +
> +int dcp_ipcc_send_power_notify(struct dcp_ipcc *ipcc)
> +{
> +	unsigned long flags, timeout;
> +	int ret;
> +
> +	if (!ipcc)
> +		return -ENODEV;
> +
> +	/* Wait for the doorbell acknowledgment before returning to VirtIO. */
> +	mutex_lock(&ipcc->power_notify_lock);
> +	spin_lock_irqsave(&ipcc->state_lock, flags);
> +	if (ipcc->shutting_down || !ipcc->power_chan) {
> +		spin_unlock_irqrestore(&ipcc->state_lock, flags);
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +	spin_unlock_irqrestore(&ipcc->state_lock, flags);
> +
> +	reinit_completion(&ipcc->power_notify_done);
> +	atomic_set(&ipcc->power_notify_pending, 1);
> +
> +	ret = mbox_send_message(ipcc->power_chan, NULL);
> +	mbox_client_txdone(ipcc->power_chan, ret);
> +	if (ret < 0) {
> +		atomic_set(&ipcc->power_notify_pending, 0);
> +		dev_err(ipcc->client.dev,
> +			"IPCC TX: power notify failed: %d\n", ret);
> +		goto out_unlock;
> +	}
> +
> +	timeout = wait_for_completion_timeout(&ipcc->power_notify_done,
> +					      msecs_to_jiffies(DCP_IPCC_POWER_NOTIFY_TIMEOUT_MS));
> +	atomic_set(&ipcc->power_notify_pending, 0);
> +	if (!timeout) {
> +		dev_err(ipcc->client.dev,
> +			"IPCC power notify acknowledgment timed out\n");
> +		ret = -ETIMEDOUT;
> +	} else {
> +		ret = 0;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&ipcc->power_notify_lock);
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_ipcc.h b/drivers/gpu/drm/msm/dcp/dcp_ipcc.h
> new file mode 100644
> index 000000000000..a3430e5cbe72
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_ipcc.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * IPCC power-notify mailbox interface for DCP firmware.
> + *
> + * Channel 5 carries APSS-to-DCP wake notifications; responses arrive through
> + * the mailbox client's RX callback.
> + */
> +
> +#ifndef __DCP_IPCC_H__
> +#define __DCP_IPCC_H__
> +
> +#include <linux/atomic.h>
> +#include <linux/completion.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +
> +struct device;
> +struct virtqueue;
> +
> +/**
> + * struct dcp_ipcc - DCP IPCC power-notification state
> + * @client: Mailbox client used for APSS-to-DCP notifications.
> + * @power_chan: Mailbox channel carrying power notifications.
> + * @resp_vq: VirtIO response queue notified by the IPCC receive path.
> + * @power_notify_done: Completion for the firmware acknowledgment.
> + * @power_notify_lock: Serializes power notifications and teardown.
> + * @power_notify_pending: Indicates an outstanding acknowledgment.
> + * @state_lock: Protects shutdown state and response queue access.
> + * @shutting_down: Prevents callbacks from accessing teardown state.

What is going to access it during teardown? Can we shutdown in a logical
way, so that there are no accessors after starting the teardown?

> + * @power_notify_irq_wake: Whether IRQ wake was enabled.
> + * @power_notify_irq: Power-notify acknowledgment IRQ number.
> + */
> +struct dcp_ipcc {
> +	struct mbox_client client;
> +	struct mbox_chan *power_chan;
> +	struct virtqueue *resp_vq;
> +	struct completion power_notify_done;
> +	/* Serializes power notifications and teardown. */
> +	struct mutex power_notify_lock;
> +	atomic_t power_notify_pending;
> +	/* Protects shutdown state and response queue access. */
> +	spinlock_t state_lock;
> +	bool shutting_down;
> +	bool power_notify_irq_wake;
> +	int power_notify_irq;
> +};
> +
> +int dcp_ipcc_init(struct dcp_ipcc *ipcc, struct device *dev);
> +void dcp_ipcc_set_resp_vq(struct dcp_ipcc *ipcc, struct virtqueue *resp_vq);
> +void dcp_ipcc_deinit(struct dcp_ipcc *ipcc);
> +int dcp_ipcc_send_power_notify(struct dcp_ipcc *ipcc);
> +
> +#endif /* __DCP_IPCC_H__ */
> 
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry