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 <[email protected]> > 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
