[PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Dmitry Baryshkov]

As msm_drm_uninit() is called from the msm_drm_init() error path,
additional care should be necessary as not to call the free_irq() for
the IRQ that was not requested before (because an error occured earlier
than the request_irq() call).

This fixed the issue reported with the following backtrace:

[    8.571329] Trying to free already-free IRQ 187
[    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
[    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
[    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
[    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
[    8.641496] Workqueue: events_unbound deferred_probe_work_func
[    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.654681] pc : free_irq+0x1e0/0x35c
[    8.658454] lr : free_irq+0x1e0/0x35c
[    8.662228] sp : ffff800008ab3950
[    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
[    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
[    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
[    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
[    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
[    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
[    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
[    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
[    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
[    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
[    8.739217] Call trace:
[    8.741755]  free_irq+0x1e0/0x35c
[    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
[    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
[    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
[    8.760657]  __component_add+0xa0/0x170
[    8.764626]  component_add+0x14/0x20
[    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
[    8.773242]  platform_probe+0x68/0xe0
[    8.777043]  really_probe.part.0+0x9c/0x28c
[    8.781368]  __driver_probe_device+0x98/0x144
[    8.785871]  driver_probe_device+0x40/0x140
[    8.790191]  __device_attach_driver+0xb4/0x120
[    8.794788]  bus_for_each_drv+0x78/0xd0
[    8.798751]  __device_attach+0xdc/0x184
[    8.802713]  device_initial_probe+0x14/0x20
[    8.807031]  bus_probe_device+0x9c/0xa4
[    8.810991]  deferred_probe_work_func+0x88/0xc0
[    8.815667]  process_one_work+0x1d0/0x320
[    8.819809]  worker_thread+0x14c/0x444
[    8.823688]  kthread+0x10c/0x110
[    8.827036]  ret_from_fork+0x10/0x20

Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 7 ++++++-
 drivers/gpu/drm/msm/msm_kms.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4a3dda23e3e0..44485363f37a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -113,6 +113,8 @@ static int msm_irq_postinstall(struct drm_device *dev)
 
 static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 {
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
 	int ret;
 
 	if (irq == IRQ_NOTCONNECTED)
@@ -124,6 +126,8 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 	if (ret)
 		return ret;
 
+	kms->irq_requested = true;
+
 	ret = msm_irq_postinstall(dev);
 	if (ret) {
 		free_irq(irq, dev);
@@ -139,7 +143,8 @@ static void msm_irq_uninstall(struct drm_device *dev)
 	struct msm_kms *kms = priv->kms;
 
 	kms->funcs->irq_uninstall(kms);
-	free_irq(kms->irq, dev);
+	if (kms->irq_requested)
+		free_irq(kms->irq, dev);
 }
 
 struct msm_vblank_work {
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index ab25fff271f9..f8ed7588928c 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -148,6 +148,7 @@ struct msm_kms {
 
 	/* irq number to be passed on to msm_irq_install */
 	int irq;
+	bool irq_requested;
 
 	/* mapper-id used to request GEM buffer mapped for scanout: */
 	struct msm_gem_address_space *aspace;
-- 
2.35.1

[PATCH 2/2] drm/msm: push IRQ setup into individual drivers

[Dmitry Baryshkov]

Afther the commit f026e431cf86 ("drm/msm: Convert to Linux IRQ
interfaces") converted MSM DRM driver to handle IRQs on it's own (rather
than using the DRM IRQ mid-layer), there is little point in keeping
irq wrapper in the msm_drv.c which just call into individual drivers.
Push respective code into the mdp4/mdp5/dpu drivers and drop
irq_preinstall/irq_postinstall/irq msm_kms funcs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  | 13 +---
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 28 ++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  6 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  7 +++
 drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c      | 30 ++++++++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c      |  4 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h      |  4 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c      | 30 ++++++++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c      |  4 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h      |  4 +-
 drivers/gpu/drm/msm/msm_drv.c                 | 62 +++----------------
 drivers/gpu/drm/msm/msm_kms.h                 |  4 +-
 12 files changed, 105 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
index b5b6e7031fb9..c6938b1f1870 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
@@ -8,13 +8,6 @@
 #include "dpu_kms.h"
 #include "dpu_hw_interrupts.h"
 
-/**
- * dpu_core_irq_preinstall - perform pre-installation of core IRQ handler
- * @kms:		MSM KMS handle
- * @return:		none
- */
-void dpu_core_irq_preinstall(struct msm_kms *kms);
-
 /**
  * dpu_core_irq_uninstall - uninstall core IRQ handler
  * @kms:		MSM KMS handle
@@ -23,11 +16,11 @@ void dpu_core_irq_preinstall(struct msm_kms *kms);
 void dpu_core_irq_uninstall(struct msm_kms *kms);
 
 /**
- * dpu_core_irq - core IRQ handler
+ * dpu_core_irq_install - install core IRQ handler
  * @kms:		MSM KMS handle
- * @return:		interrupt handling status
+ * @return:		non-zero in case of an error
  */
-irqreturn_t dpu_core_irq(struct msm_kms *kms);
+int dpu_core_irq_install(struct msm_kms *kms);
 
 /**
  * dpu_core_irq_read - IRQ helper function for reading IRQ status
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index d6498e45dc2c..fa4f99034a08 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -164,8 +164,9 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, int irq_idx)
 	dpu_kms->hw_intr->irq_tbl[irq_idx].cb(dpu_kms->hw_intr->irq_tbl[irq_idx].arg, irq_idx);
 }
 
-irqreturn_t dpu_core_irq(struct msm_kms *kms)
+static irqreturn_t dpu_irq(int irq, void *arg)
 {
+	struct msm_kms *kms = arg;
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
 	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
 	int reg_idx;
@@ -541,7 +542,7 @@ void dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms,
 }
 #endif
 
-void dpu_core_irq_preinstall(struct msm_kms *kms)
+static void dpu_core_irq_preinstall(struct msm_kms *kms)
 {
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
 	int i;
@@ -570,5 +571,28 @@ void dpu_core_irq_uninstall(struct msm_kms *kms)
 
 	dpu_clear_irqs(dpu_kms);
 	dpu_disable_all_irqs(dpu_kms);
+	if (kms->irq_requested)
+		free_irq(kms->irq, kms);
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
+
+int dpu_core_irq_install(struct msm_kms *kms)
+{
+	int ret;
+
+	dpu_core_irq_preinstall(kms);
+
+	ret = request_irq(kms->irq, dpu_irq, 0, "dpu", kms);
+	if (ret)
+		return ret;
+
+	kms->irq_requested = true;
+
+	ret = dpu_irq_postinstall(kms);
+	if (ret) {
+		free_irq(kms->irq, kms);
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 2b9d931474e0..494978da7785 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -884,7 +884,7 @@ static void dpu_kms_destroy(struct msm_kms *kms)
 		pm_runtime_disable(&dpu_kms->pdev->dev);
 }
 
-static int dpu_irq_postinstall(struct msm_kms *kms)
+int dpu_irq_postinstall(struct msm_kms *kms)
 {
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
@@ -960,10 +960,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 
 static const struct msm_kms_funcs kms_funcs = {
 	.hw_init         = dpu_kms_hw_init,
-	.irq_preinstall  = dpu_core_irq_preinstall,
-	.irq_postinstall = dpu_irq_postinstall,
+	.irq_install     = dpu_core_irq_install,
 	.irq_uninstall   = dpu_core_irq_uninstall,
-	.irq             = dpu_core_irq,
 	.enable_commit   = dpu_kms_enable_commit,
 	.disable_commit  = dpu_kms_disable_commit,
 	.vsync_time      = dpu_kms_vsync_time,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 832a0769f2e7..559184c64045 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -204,4 +204,11 @@ void dpu_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
  */
 u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
 
+/**
+ * dpu_irq_postinstall - perform post-installation of core IRQ handler
+ * @kms:               MSM KMS handle
+ * @return:            non-zero in case of error
+ */
+int dpu_irq_postinstall(struct msm_kms *kms);
+
 #endif /* __dpu_kms_H__ */
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
index 4d49f3ba6a96..87675c162eea 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
@@ -32,7 +32,7 @@ static void mdp4_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus)
 	}
 }
 
-void mdp4_irq_preinstall(struct msm_kms *kms)
+static void mdp4_irq_preinstall(struct msm_kms *kms)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
 	mdp4_enable(mdp4_kms);
@@ -41,7 +41,7 @@ void mdp4_irq_preinstall(struct msm_kms *kms)
 	mdp4_disable(mdp4_kms);
 }
 
-int mdp4_irq_postinstall(struct msm_kms *kms)
+static int mdp4_irq_postinstall(struct msm_kms *kms)
 {
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
@@ -62,10 +62,13 @@ void mdp4_irq_uninstall(struct msm_kms *kms)
 	mdp4_enable(mdp4_kms);
 	mdp4_write(mdp4_kms, REG_MDP4_INTR_ENABLE, 0x00000000);
 	mdp4_disable(mdp4_kms);
+	if (kms->irq_requested)
+		free_irq(kms->irq, kms);
 }
 
-irqreturn_t mdp4_irq(struct msm_kms *kms)
+static irqreturn_t mdp4_irq(int irq, void *arg)
 {
+	struct msm_kms *kms = arg;
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
 	struct drm_device *dev = mdp4_kms->dev;
@@ -88,6 +91,27 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
 	return IRQ_HANDLED;
 }
 
+int mdp4_irq_install(struct msm_kms *kms)
+{
+	int ret;
+
+	mdp4_irq_preinstall(kms);
+
+	ret = request_irq(kms->irq, mdp4_irq, 0, "mdp4", kms);
+	if (ret)
+		return ret;
+
+	kms->irq_requested = true;
+
+	ret = mdp4_irq_postinstall(kms);
+	if (ret) {
+		free_irq(kms->irq, kms);
+		return ret;
+	}
+
+	return 0;
+}
+
 int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index fb48c8c19ec3..b7aced272af9 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -148,10 +148,8 @@ static void mdp4_destroy(struct msm_kms *kms)
 static const struct mdp_kms_funcs kms_funcs = {
 	.base = {
 		.hw_init         = mdp4_hw_init,
-		.irq_preinstall  = mdp4_irq_preinstall,
-		.irq_postinstall = mdp4_irq_postinstall,
+		.irq_install     = mdp4_irq_install,
 		.irq_uninstall   = mdp4_irq_uninstall,
-		.irq             = mdp4_irq,
 		.enable_vblank   = mdp4_enable_vblank,
 		.disable_vblank  = mdp4_disable_vblank,
 		.enable_commit   = mdp4_enable_commit,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
index e8ee92ab7956..b24a63872232 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
@@ -157,10 +157,8 @@ int mdp4_enable(struct mdp4_kms *mdp4_kms);
 
 void mdp4_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
 		uint32_t old_irqmask);
-void mdp4_irq_preinstall(struct msm_kms *kms);
-int mdp4_irq_postinstall(struct msm_kms *kms);
+int mdp4_irq_install(struct msm_kms *kms);
 void mdp4_irq_uninstall(struct msm_kms *kms);
-irqreturn_t mdp4_irq(struct msm_kms *kms);
 int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
 void mdp4_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
index 9b4c8d92ff32..d573ff29d5a4 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
@@ -36,7 +36,7 @@ static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus)
 	}
 }
 
-void mdp5_irq_preinstall(struct msm_kms *kms)
+static void mdp5_irq_preinstall(struct msm_kms *kms)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
 	struct device *dev = &mdp5_kms->pdev->dev;
@@ -47,7 +47,7 @@ void mdp5_irq_preinstall(struct msm_kms *kms)
 	pm_runtime_put_sync(dev);
 }
 
-int mdp5_irq_postinstall(struct msm_kms *kms)
+static int mdp5_irq_postinstall(struct msm_kms *kms)
 {
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
@@ -74,11 +74,14 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
 
 	pm_runtime_get_sync(dev);
 	mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
+	if (kms->irq_requested)
+		free_irq(kms->irq, kms);
 	pm_runtime_put_sync(dev);
 }
 
-irqreturn_t mdp5_irq(struct msm_kms *kms)
+static irqreturn_t mdp5_irq(int irq, void *arg)
 {
+	struct msm_kms *kms = arg;
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
 	struct drm_device *dev = mdp5_kms->dev;
@@ -101,6 +104,27 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
 	return IRQ_HANDLED;
 }
 
+int mdp5_irq_install(struct msm_kms *kms)
+{
+	int ret;
+
+	mdp5_irq_preinstall(kms);
+
+	ret = request_irq(kms->irq, mdp5_irq, 0, "mdp5", kms);
+	if (ret)
+		return ret;
+
+	kms->irq_requested = true;
+
+	ret = mdp5_irq_postinstall(kms);
+	if (ret) {
+		free_irq(kms->irq, kms);
+		return ret;
+	}
+
+	return 0;
+}
+
 int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 3d5621a68f85..18cf0ff4da6c 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -262,10 +262,8 @@ static int mdp5_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
 static const struct mdp_kms_funcs kms_funcs = {
 	.base = {
 		.hw_init         = mdp5_hw_init,
-		.irq_preinstall  = mdp5_irq_preinstall,
-		.irq_postinstall = mdp5_irq_postinstall,
+		.irq_install     = mdp5_irq_install,
 		.irq_uninstall   = mdp5_irq_uninstall,
-		.irq             = mdp5_irq,
 		.enable_vblank   = mdp5_enable_vblank,
 		.disable_vblank  = mdp5_disable_vblank,
 		.flush_commit    = mdp5_flush_commit,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
index 29bf11f08601..630b5f812f24 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
@@ -263,10 +263,8 @@ static inline uint32_t lm2ppdone(struct mdp5_hw_mixer *mixer)
 
 void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
 		uint32_t old_irqmask);
-void mdp5_irq_preinstall(struct msm_kms *kms);
-int mdp5_irq_postinstall(struct msm_kms *kms);
+int mdp5_irq_install(struct msm_kms *kms);
 void mdp5_irq_uninstall(struct msm_kms *kms);
-irqreturn_t mdp5_irq(struct msm_kms *kms);
 int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
 void mdp5_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
 int mdp5_irq_domain_init(struct mdp5_kms *mdp5_kms);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 44485363f37a..d2fbe54fec4d 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -77,64 +77,15 @@ static bool modeset = true;
 MODULE_PARM_DESC(modeset, "Use kernel modesetting [KMS] (1=on (default), 0=disable)");
 module_param(modeset, bool, 0600);
 
-static irqreturn_t msm_irq(int irq, void *arg)
-{
-	struct drm_device *dev = arg;
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	return kms->funcs->irq(kms);
-}
-
-static void msm_irq_preinstall(struct drm_device *dev)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	kms->funcs->irq_preinstall(kms);
-}
-
-static int msm_irq_postinstall(struct drm_device *dev)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	if (kms->funcs->irq_postinstall)
-		return kms->funcs->irq_postinstall(kms);
-
-	return 0;
-}
-
 static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
-	int ret;
-
-	if (irq == IRQ_NOTCONNECTED)
-		return -ENOTCONN;
-
-	msm_irq_preinstall(dev);
-
-	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
-	if (ret)
-		return ret;
-
-	kms->irq_requested = true;
 
-	ret = msm_irq_postinstall(dev);
-	if (ret) {
-		free_irq(irq, dev);
-		return ret;
-	}
+	if (!kms->funcs->irq_install)
+		return -EINVAL;
 
-	return 0;
+	return kms->funcs->irq_install(kms);
 }
 
 static void msm_irq_uninstall(struct drm_device *dev)
@@ -143,8 +94,6 @@ static void msm_irq_uninstall(struct drm_device *dev)
 	struct msm_kms *kms = priv->kms;
 
 	kms->funcs->irq_uninstall(kms);
-	if (kms->irq_requested)
-		free_irq(kms->irq, dev);
 }
 
 struct msm_vblank_work {
@@ -450,6 +399,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	}
 
 	if (kms) {
+		if (kms->irq == IRQ_NOTCONNECTED) {
+			ret = -ENOTCONN;
+			goto err_msm_uninit;
+		}
+
 		pm_runtime_get_sync(dev);
 		ret = msm_irq_install(ddev, kms->irq);
 		pm_runtime_put_sync(dev);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index f8ed7588928c..71d497a8fb8b 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -24,10 +24,8 @@ struct msm_kms_funcs {
 	/* hw initialization: */
 	int (*hw_init)(struct msm_kms *kms);
 	/* irq handling: */
-	void (*irq_preinstall)(struct msm_kms *kms);
-	int (*irq_postinstall)(struct msm_kms *kms);
+	int (*irq_install)(struct msm_kms *kms);
 	void (*irq_uninstall)(struct msm_kms *kms);
-	irqreturn_t (*irq)(struct msm_kms *kms);
 	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 
-- 
2.35.1

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Abhinav Kumar]

On 5/6/2022 6:00 PM, Dmitry Baryshkov wrote:
> As msm_drm_uninit() is called from the msm_drm_init() error path,
> additional care should be necessary as not to call the free_irq() for
> the IRQ that was not requested before (because an error occured earlier
> than the request_irq() call).
> 
> This fixed the issue reported with the following backtrace:
> 
> [    8.571329] Trying to free already-free IRQ 187
> [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.654681] pc : free_irq+0x1e0/0x35c
> [    8.658454] lr : free_irq+0x1e0/0x35c
> [    8.662228] sp : ffff800008ab3950
> [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> [    8.739217] Call trace:
> [    8.741755]  free_irq+0x1e0/0x35c
> [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> [    8.760657]  __component_add+0xa0/0x170
> [    8.764626]  component_add+0x14/0x20
> [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> [    8.773242]  platform_probe+0x68/0xe0
> [    8.777043]  really_probe.part.0+0x9c/0x28c
> [    8.781368]  __driver_probe_device+0x98/0x144
> [    8.785871]  driver_probe_device+0x40/0x140
> [    8.790191]  __device_attach_driver+0xb4/0x120
> [    8.794788]  bus_for_each_drv+0x78/0xd0
> [    8.798751]  __device_attach+0xdc/0x184
> [    8.802713]  device_initial_probe+0x14/0x20
> [    8.807031]  bus_probe_device+0x9c/0xa4
> [    8.810991]  deferred_probe_work_func+0x88/0xc0
> [    8.815667]  process_one_work+0x1d0/0x320
> [    8.819809]  worker_thread+0x14c/0x444
> [    8.823688]  kthread+0x10c/0x110
> [    8.827036]  ret_from_fork+0x10/0x20
> 
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I was looking at how other drm drivers handle this and they also have a 
similar logic. So this is a good addition and thanks to the -EDEFER path 
getting exposed we finally uncovered this.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> ---
>   drivers/gpu/drm/msm/msm_drv.c | 7 ++++++-
>   drivers/gpu/drm/msm/msm_kms.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4a3dda23e3e0..44485363f37a 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -113,6 +113,8 @@ static int msm_irq_postinstall(struct drm_device *dev)
>   
>   static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>   {
> +	struct msm_drm_private *priv = dev->dev_private;
> +	struct msm_kms *kms = priv->kms;
>   	int ret;
>   
>   	if (irq == IRQ_NOTCONNECTED)
> @@ -124,6 +126,8 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>   	if (ret)
>   		return ret;
>   
> +	kms->irq_requested = true;
> +
>   	ret = msm_irq_postinstall(dev);
>   	if (ret) {
>   		free_irq(irq, dev);
> @@ -139,7 +143,8 @@ static void msm_irq_uninstall(struct drm_device *dev)
>   	struct msm_kms *kms = priv->kms;
>   
>   	kms->funcs->irq_uninstall(kms);
> -	free_irq(kms->irq, dev);
> +	if (kms->irq_requested)
> +		free_irq(kms->irq, dev);
>   }
>   
>   struct msm_vblank_work {
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index ab25fff271f9..f8ed7588928c 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -148,6 +148,7 @@ struct msm_kms {
>   
>   	/* irq number to be passed on to msm_irq_install */
>   	int irq;
> +	bool irq_requested;
>   
>   	/* mapper-id used to request GEM buffer mapped for scanout: */
>   	struct msm_gem_address_space *aspace;

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> As msm_drm_uninit() is called from the msm_drm_init() error path,
> additional care should be necessary as not to call the free_irq() for
> the IRQ that was not requested before (because an error occured earlier
> than the request_irq() call).
>
> This fixed the issue reported with the following backtrace:
>
> [    8.571329] Trying to free already-free IRQ 187
> [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.654681] pc : free_irq+0x1e0/0x35c
> [    8.658454] lr : free_irq+0x1e0/0x35c
> [    8.662228] sp : ffff800008ab3950
> [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> [    8.739217] Call trace:
> [    8.741755]  free_irq+0x1e0/0x35c
> [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> [    8.760657]  __component_add+0xa0/0x170
> [    8.764626]  component_add+0x14/0x20
> [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> [    8.773242]  platform_probe+0x68/0xe0
> [    8.777043]  really_probe.part.0+0x9c/0x28c
> [    8.781368]  __driver_probe_device+0x98/0x144
> [    8.785871]  driver_probe_device+0x40/0x140
> [    8.790191]  __device_attach_driver+0xb4/0x120
> [    8.794788]  bus_for_each_drv+0x78/0xd0
> [    8.798751]  __device_attach+0xdc/0x184
> [    8.802713]  device_initial_probe+0x14/0x20
> [    8.807031]  bus_probe_device+0x9c/0xa4
> [    8.810991]  deferred_probe_work_func+0x88/0xc0
> [    8.815667]  process_one_work+0x1d0/0x320
> [    8.819809]  worker_thread+0x14c/0x444
> [    8.823688]  kthread+0x10c/0x110
> [    8.827036]  ret_from_fork+0x10/0x20
>
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")

Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
clearing hw interrupts if hw_intr is null during drm uninit")? I mean
that with this patch applied kms->irq_requested makes the check in
dpu_core_irq_uninstall() irrelevant because it isn't called anymore?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Dmitry Baryshkov]

On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> > As msm_drm_uninit() is called from the msm_drm_init() error path,
> > additional care should be necessary as not to call the free_irq() for
> > the IRQ that was not requested before (because an error occured earlier
> > than the request_irq() call).
> >
> > This fixed the issue reported with the following backtrace:
> >
> > [    8.571329] Trying to free already-free IRQ 187
> > [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> > [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> > [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> > [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> > [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> > [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    8.654681] pc : free_irq+0x1e0/0x35c
> > [    8.658454] lr : free_irq+0x1e0/0x35c
> > [    8.662228] sp : ffff800008ab3950
> > [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> > [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> > [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> > [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> > [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> > [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> > [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> > [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> > [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> > [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> > [    8.739217] Call trace:
> > [    8.741755]  free_irq+0x1e0/0x35c
> > [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> > [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> > [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> > [    8.760657]  __component_add+0xa0/0x170
> > [    8.764626]  component_add+0x14/0x20
> > [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> > [    8.773242]  platform_probe+0x68/0xe0
> > [    8.777043]  really_probe.part.0+0x9c/0x28c
> > [    8.781368]  __driver_probe_device+0x98/0x144
> > [    8.785871]  driver_probe_device+0x40/0x140
> > [    8.790191]  __device_attach_driver+0xb4/0x120
> > [    8.794788]  bus_for_each_drv+0x78/0xd0
> > [    8.798751]  __device_attach+0xdc/0x184
> > [    8.802713]  device_initial_probe+0x14/0x20
> > [    8.807031]  bus_probe_device+0x9c/0xa4
> > [    8.810991]  deferred_probe_work_func+0x88/0xc0
> > [    8.815667]  process_one_work+0x1d0/0x320
> > [    8.819809]  worker_thread+0x14c/0x444
> > [    8.823688]  kthread+0x10c/0x110
> > [    8.827036]  ret_from_fork+0x10/0x20
> >
> > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
>
> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> that with this patch applied kms->irq_requested makes the check in
> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?

Yes.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>



-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> >
> > Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> > clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> > that with this patch applied kms->irq_requested makes the check in
> > dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>
> Yes.
>

I didn't see it deleted in the second patch so is a revert going to be
sent?

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Dmitry Baryshkov]

On 12/05/2022 04:29, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
>> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>>>
>>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
>>>
>>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
>>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
>>> that with this patch applied kms->irq_requested makes the check in
>>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>>
>> Yes.
>>
> 
> I didn't see it deleted in the second patch so is a revert going to be
> sent?

No need to. They are separate checks. The older one is superseded (as it 
will be never hit),  but it is still valid and useful (to protect from 
the crash if this code changes again).


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-11 18:30:55)
> On 12/05/2022 04:29, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
> >> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
> >>>
> >>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> >>>
> >>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> >>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> >>> that with this patch applied kms->irq_requested makes the check in
> >>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
> >>
> >> Yes.
> >>
> >
> > I didn't see it deleted in the second patch so is a revert going to be
> > sent?
>
> No need to. They are separate checks. The older one is superseded (as it
> will be never hit),  but it is still valid and useful (to protect from
> the crash if this code changes again).
>

Ew, gross. The extra conditionals everywhere really makes it hard to
follow along.

Re: [PATCH 2/2] drm/msm: push IRQ setup into individual drivers

[Abhinav Kumar]

On 5/6/2022 6:00 PM, Dmitry Baryshkov wrote:
> Afther the commit f026e431cf86 ("drm/msm: Convert to Linux IRQ
> interfaces") converted MSM DRM driver to handle IRQs on it's own (rather
> than using the DRM IRQ mid-layer), there is little point in keeping
> irq wrapper in the msm_drv.c which just call into individual drivers.
> Push respective code into the mdp4/mdp5/dpu drivers and drop
> irq_preinstall/irq_postinstall/irq msm_kms funcs.

Isnt this change causing a lot of code duplication across mdp5/dpu/mdp4?

Do you have any future plans with respect to this separation?

I am missing why this separation into respective mdp4/5/dpu is necessary 
because struct msm_kms remains a common struct in all of this so it 
remaining in msm_drv will avoid duplication.

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  | 13 +---
>   .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 28 ++++++++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  6 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  7 +++
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c      | 30 ++++++++-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c      |  4 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h      |  4 +-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c      | 30 ++++++++-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c      |  4 +-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h      |  4 +-
>   drivers/gpu/drm/msm/msm_drv.c                 | 62 +++----------------
>   drivers/gpu/drm/msm/msm_kms.h                 |  4 +-
>   12 files changed, 105 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
> index b5b6e7031fb9..c6938b1f1870 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
> @@ -8,13 +8,6 @@
>   #include "dpu_kms.h"
>   #include "dpu_hw_interrupts.h"
>   
> -/**
> - * dpu_core_irq_preinstall - perform pre-installation of core IRQ handler
> - * @kms:		MSM KMS handle
> - * @return:		none
> - */
> -void dpu_core_irq_preinstall(struct msm_kms *kms);
> -
>   /**
>    * dpu_core_irq_uninstall - uninstall core IRQ handler
>    * @kms:		MSM KMS handle
> @@ -23,11 +16,11 @@ void dpu_core_irq_preinstall(struct msm_kms *kms);
>   void dpu_core_irq_uninstall(struct msm_kms *kms);
>   
>   /**
> - * dpu_core_irq - core IRQ handler
> + * dpu_core_irq_install - install core IRQ handler
>    * @kms:		MSM KMS handle
> - * @return:		interrupt handling status
> + * @return:		non-zero in case of an error
>    */
> -irqreturn_t dpu_core_irq(struct msm_kms *kms);
> +int dpu_core_irq_install(struct msm_kms *kms);
>   
>   /**
>    * dpu_core_irq_read - IRQ helper function for reading IRQ status
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index d6498e45dc2c..fa4f99034a08 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -164,8 +164,9 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, int irq_idx)
>   	dpu_kms->hw_intr->irq_tbl[irq_idx].cb(dpu_kms->hw_intr->irq_tbl[irq_idx].arg, irq_idx);
>   }
>   
> -irqreturn_t dpu_core_irq(struct msm_kms *kms)
> +static irqreturn_t dpu_irq(int irq, void *arg)
>   {
> +	struct msm_kms *kms = arg;
>   	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>   	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
>   	int reg_idx;
> @@ -541,7 +542,7 @@ void dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms,
>   }
>   #endif
>   
> -void dpu_core_irq_preinstall(struct msm_kms *kms)
> +static void dpu_core_irq_preinstall(struct msm_kms *kms)
>   {
>   	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>   	int i;
> @@ -570,5 +571,28 @@ void dpu_core_irq_uninstall(struct msm_kms *kms)
>   
>   	dpu_clear_irqs(dpu_kms);
>   	dpu_disable_all_irqs(dpu_kms);
> +	if (kms->irq_requested)
> +		free_irq(kms->irq, kms);
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
> +
> +int dpu_core_irq_install(struct msm_kms *kms)
> +{
> +	int ret;
> +
> +	dpu_core_irq_preinstall(kms);
> +
> +	ret = request_irq(kms->irq, dpu_irq, 0, "dpu", kms);
> +	if (ret)
> +		return ret;
> +
> +	kms->irq_requested = true;
> +
> +	ret = dpu_irq_postinstall(kms);
> +	if (ret) {
> +		free_irq(kms->irq, kms);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 2b9d931474e0..494978da7785 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -884,7 +884,7 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>   		pm_runtime_disable(&dpu_kms->pdev->dev);
>   }
>   
> -static int dpu_irq_postinstall(struct msm_kms *kms)
> +int dpu_irq_postinstall(struct msm_kms *kms)
>   {
>   	struct msm_drm_private *priv;
>   	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -960,10 +960,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   
>   static const struct msm_kms_funcs kms_funcs = {
>   	.hw_init         = dpu_kms_hw_init,
> -	.irq_preinstall  = dpu_core_irq_preinstall,
> -	.irq_postinstall = dpu_irq_postinstall,
> +	.irq_install     = dpu_core_irq_install,
>   	.irq_uninstall   = dpu_core_irq_uninstall,
> -	.irq             = dpu_core_irq,
>   	.enable_commit   = dpu_kms_enable_commit,
>   	.disable_commit  = dpu_kms_disable_commit,
>   	.vsync_time      = dpu_kms_vsync_time,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 832a0769f2e7..559184c64045 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -204,4 +204,11 @@ void dpu_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>    */
>   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
>   
> +/**
> + * dpu_irq_postinstall - perform post-installation of core IRQ handler
> + * @kms:               MSM KMS handle
> + * @return:            non-zero in case of error
> + */
> +int dpu_irq_postinstall(struct msm_kms *kms);
> +
>   #endif /* __dpu_kms_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
> index 4d49f3ba6a96..87675c162eea 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
> @@ -32,7 +32,7 @@ static void mdp4_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus)
>   	}
>   }
>   
> -void mdp4_irq_preinstall(struct msm_kms *kms)
> +static void mdp4_irq_preinstall(struct msm_kms *kms)
>   {
>   	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
>   	mdp4_enable(mdp4_kms);
> @@ -41,7 +41,7 @@ void mdp4_irq_preinstall(struct msm_kms *kms)
>   	mdp4_disable(mdp4_kms);
>   }
>   
> -int mdp4_irq_postinstall(struct msm_kms *kms)
> +static int mdp4_irq_postinstall(struct msm_kms *kms)
>   {
>   	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>   	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
> @@ -62,10 +62,13 @@ void mdp4_irq_uninstall(struct msm_kms *kms)
>   	mdp4_enable(mdp4_kms);
>   	mdp4_write(mdp4_kms, REG_MDP4_INTR_ENABLE, 0x00000000);
>   	mdp4_disable(mdp4_kms);
> +	if (kms->irq_requested)
> +		free_irq(kms->irq, kms);
>   }
>   
> -irqreturn_t mdp4_irq(struct msm_kms *kms)
> +static irqreturn_t mdp4_irq(int irq, void *arg)
>   {
> +	struct msm_kms *kms = arg;
>   	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>   	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
>   	struct drm_device *dev = mdp4_kms->dev;
> @@ -88,6 +91,27 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
>   	return IRQ_HANDLED;
>   }
>   
> +int mdp4_irq_install(struct msm_kms *kms)
> +{
> +	int ret;
> +
> +	mdp4_irq_preinstall(kms);
> +
> +	ret = request_irq(kms->irq, mdp4_irq, 0, "mdp4", kms);
> +	if (ret)
> +		return ret;
> +
> +	kms->irq_requested = true;
> +
> +	ret = mdp4_irq_postinstall(kms);
> +	if (ret) {
> +		free_irq(kms->irq, kms);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
>   {
>   	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index fb48c8c19ec3..b7aced272af9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -148,10 +148,8 @@ static void mdp4_destroy(struct msm_kms *kms)
>   static const struct mdp_kms_funcs kms_funcs = {
>   	.base = {
>   		.hw_init         = mdp4_hw_init,
> -		.irq_preinstall  = mdp4_irq_preinstall,
> -		.irq_postinstall = mdp4_irq_postinstall,
> +		.irq_install     = mdp4_irq_install,
>   		.irq_uninstall   = mdp4_irq_uninstall,
> -		.irq             = mdp4_irq,
>   		.enable_vblank   = mdp4_enable_vblank,
>   		.disable_vblank  = mdp4_disable_vblank,
>   		.enable_commit   = mdp4_enable_commit,
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
> index e8ee92ab7956..b24a63872232 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
> @@ -157,10 +157,8 @@ int mdp4_enable(struct mdp4_kms *mdp4_kms);
>   
>   void mdp4_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>   		uint32_t old_irqmask);
> -void mdp4_irq_preinstall(struct msm_kms *kms);
> -int mdp4_irq_postinstall(struct msm_kms *kms);
> +int mdp4_irq_install(struct msm_kms *kms);
>   void mdp4_irq_uninstall(struct msm_kms *kms);
> -irqreturn_t mdp4_irq(struct msm_kms *kms);
>   int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>   void mdp4_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>   
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
> index 9b4c8d92ff32..d573ff29d5a4 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
> @@ -36,7 +36,7 @@ static void mdp5_irq_error_handler(struct mdp_irq *irq, uint32_t irqstatus)
>   	}
>   }
>   
> -void mdp5_irq_preinstall(struct msm_kms *kms)
> +static void mdp5_irq_preinstall(struct msm_kms *kms)
>   {
>   	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
>   	struct device *dev = &mdp5_kms->pdev->dev;
> @@ -47,7 +47,7 @@ void mdp5_irq_preinstall(struct msm_kms *kms)
>   	pm_runtime_put_sync(dev);
>   }
>   
> -int mdp5_irq_postinstall(struct msm_kms *kms)
> +static int mdp5_irq_postinstall(struct msm_kms *kms)
>   {
>   	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>   	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> @@ -74,11 +74,14 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
>   
>   	pm_runtime_get_sync(dev);
>   	mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
> +	if (kms->irq_requested)
> +		free_irq(kms->irq, kms);
>   	pm_runtime_put_sync(dev);
>   }
>   
> -irqreturn_t mdp5_irq(struct msm_kms *kms)
> +static irqreturn_t mdp5_irq(int irq, void *arg)
>   {
> +	struct msm_kms *kms = arg;
>   	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>   	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>   	struct drm_device *dev = mdp5_kms->dev;
> @@ -101,6 +104,27 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
>   	return IRQ_HANDLED;
>   }
>   
> +int mdp5_irq_install(struct msm_kms *kms)
> +{
> +	int ret;
> +
> +	mdp5_irq_preinstall(kms);
> +
> +	ret = request_irq(kms->irq, mdp5_irq, 0, "mdp5", kms);
> +	if (ret)
> +		return ret;
> +
> +	kms->irq_requested = true;
> +
> +	ret = mdp5_irq_postinstall(kms);
> +	if (ret) {
> +		free_irq(kms->irq, kms);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
>   {
>   	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 3d5621a68f85..18cf0ff4da6c 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -262,10 +262,8 @@ static int mdp5_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
>   static const struct mdp_kms_funcs kms_funcs = {
>   	.base = {
>   		.hw_init         = mdp5_hw_init,
> -		.irq_preinstall  = mdp5_irq_preinstall,
> -		.irq_postinstall = mdp5_irq_postinstall,
> +		.irq_install     = mdp5_irq_install,
>   		.irq_uninstall   = mdp5_irq_uninstall,
> -		.irq             = mdp5_irq,
>   		.enable_vblank   = mdp5_enable_vblank,
>   		.disable_vblank  = mdp5_disable_vblank,
>   		.flush_commit    = mdp5_flush_commit,
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
> index 29bf11f08601..630b5f812f24 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
> @@ -263,10 +263,8 @@ static inline uint32_t lm2ppdone(struct mdp5_hw_mixer *mixer)
>   
>   void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>   		uint32_t old_irqmask);
> -void mdp5_irq_preinstall(struct msm_kms *kms);
> -int mdp5_irq_postinstall(struct msm_kms *kms);
> +int mdp5_irq_install(struct msm_kms *kms);
>   void mdp5_irq_uninstall(struct msm_kms *kms);
> -irqreturn_t mdp5_irq(struct msm_kms *kms);
>   int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>   void mdp5_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>   int mdp5_irq_domain_init(struct mdp5_kms *mdp5_kms);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 44485363f37a..d2fbe54fec4d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -77,64 +77,15 @@ static bool modeset = true;
>   MODULE_PARM_DESC(modeset, "Use kernel modesetting [KMS] (1=on (default), 0=disable)");
>   module_param(modeset, bool, 0600);
>   
> -static irqreturn_t msm_irq(int irq, void *arg)
> -{
> -	struct drm_device *dev = arg;
> -	struct msm_drm_private *priv = dev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -
> -	BUG_ON(!kms);
> -
> -	return kms->funcs->irq(kms);
> -}
> -
> -static void msm_irq_preinstall(struct drm_device *dev)
> -{
> -	struct msm_drm_private *priv = dev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -
> -	BUG_ON(!kms);
> -
> -	kms->funcs->irq_preinstall(kms);
> -}
> -
> -static int msm_irq_postinstall(struct drm_device *dev)
> -{
> -	struct msm_drm_private *priv = dev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -
> -	BUG_ON(!kms);
> -
> -	if (kms->funcs->irq_postinstall)
> -		return kms->funcs->irq_postinstall(kms);
> -
> -	return 0;
> -}
> -
>   static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
> -	int ret;
> -
> -	if (irq == IRQ_NOTCONNECTED)
> -		return -ENOTCONN;
> -
> -	msm_irq_preinstall(dev);
> -
> -	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
> -	if (ret)
> -		return ret;
> -
> -	kms->irq_requested = true;
>   
> -	ret = msm_irq_postinstall(dev);
> -	if (ret) {
> -		free_irq(irq, dev);
> -		return ret;
> -	}
> +	if (!kms->funcs->irq_install)
> +		return -EINVAL;
>   
> -	return 0;
> +	return kms->funcs->irq_install(kms);
>   }
>   
>   static void msm_irq_uninstall(struct drm_device *dev)
> @@ -143,8 +94,6 @@ static void msm_irq_uninstall(struct drm_device *dev)
>   	struct msm_kms *kms = priv->kms;
>   
>   	kms->funcs->irq_uninstall(kms);
> -	if (kms->irq_requested)
> -		free_irq(kms->irq, dev);
>   }
>   
>   struct msm_vblank_work {
> @@ -450,6 +399,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	}
>   
>   	if (kms) {
> +		if (kms->irq == IRQ_NOTCONNECTED) {
> +			ret = -ENOTCONN;
> +			goto err_msm_uninit;
> +		}
> +
>   		pm_runtime_get_sync(dev);
>   		ret = msm_irq_install(ddev, kms->irq);
>   		pm_runtime_put_sync(dev);
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index f8ed7588928c..71d497a8fb8b 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -24,10 +24,8 @@ struct msm_kms_funcs {
>   	/* hw initialization: */
>   	int (*hw_init)(struct msm_kms *kms);
>   	/* irq handling: */
> -	void (*irq_preinstall)(struct msm_kms *kms);
> -	int (*irq_postinstall)(struct msm_kms *kms);
> +	int (*irq_install)(struct msm_kms *kms);
>   	void (*irq_uninstall)(struct msm_kms *kms);
> -	irqreturn_t (*irq)(struct msm_kms *kms);
>   	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
>   	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
>   

Re: [PATCH 2/2] drm/msm: push IRQ setup into individual drivers

[Dmitry Baryshkov]

On 13/05/2022 01:54, Abhinav Kumar wrote:
> 
> 
> On 5/6/2022 6:00 PM, Dmitry Baryshkov wrote:
>> Afther the commit f026e431cf86 ("drm/msm: Convert to Linux IRQ
>> interfaces") converted MSM DRM driver to handle IRQs on it's own (rather
>> than using the DRM IRQ mid-layer), there is little point in keeping
>> irq wrapper in the msm_drv.c which just call into individual drivers.
>> Push respective code into the mdp4/mdp5/dpu drivers and drop
>> irq_preinstall/irq_postinstall/irq msm_kms funcs.
> 
> Isnt this change causing a lot of code duplication across mdp5/dpu/mdp4?
> 
> Do you have any future plans with respect to this separation?
> 
> I am missing why this separation into respective mdp4/5/dpu is necessary 
> because struct msm_kms remains a common struct in all of this so it 
> remaining in msm_drv will avoid duplication.

I wanted to remove back-and-forth calls between msm core and individual 
drivers. But I agree that it comes by the cost of code duplication. 
Let's drop this patch.

> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  | 13 +---
>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 28 ++++++++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  6 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  7 +++
>>   drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c      | 30 ++++++++-
>>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c      |  4 +-
>>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h      |  4 +-
>>   drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c      | 30 ++++++++-
>>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c      |  4 +-
>>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h      |  4 +-
>>   drivers/gpu/drm/msm/msm_drv.c                 | 62 +++----------------
>>   drivers/gpu/drm/msm/msm_kms.h                 |  4 +-
>>   12 files changed, 105 insertions(+), 91 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
>> index b5b6e7031fb9..c6938b1f1870 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h
>> @@ -8,13 +8,6 @@
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_interrupts.h"
>> -/**
>> - * dpu_core_irq_preinstall - perform pre-installation of core IRQ 
>> handler
>> - * @kms:        MSM KMS handle
>> - * @return:        none
>> - */
>> -void dpu_core_irq_preinstall(struct msm_kms *kms);
>> -
>>   /**
>>    * dpu_core_irq_uninstall - uninstall core IRQ handler
>>    * @kms:        MSM KMS handle
>> @@ -23,11 +16,11 @@ void dpu_core_irq_preinstall(struct msm_kms *kms);
>>   void dpu_core_irq_uninstall(struct msm_kms *kms);
>>   /**
>> - * dpu_core_irq - core IRQ handler
>> + * dpu_core_irq_install - install core IRQ handler
>>    * @kms:        MSM KMS handle
>> - * @return:        interrupt handling status
>> + * @return:        non-zero in case of an error
>>    */
>> -irqreturn_t dpu_core_irq(struct msm_kms *kms);
>> +int dpu_core_irq_install(struct msm_kms *kms);
>>   /**
>>    * dpu_core_irq_read - IRQ helper function for reading IRQ status
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>> index d6498e45dc2c..fa4f99034a08 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
>> @@ -164,8 +164,9 @@ static void dpu_core_irq_callback_handler(struct 
>> dpu_kms *dpu_kms, int irq_idx)
>>       
>> dpu_kms->hw_intr->irq_tbl[irq_idx].cb(dpu_kms->hw_intr->irq_tbl[irq_idx].arg, 
>> irq_idx);
>>   }
>> -irqreturn_t dpu_core_irq(struct msm_kms *kms)
>> +static irqreturn_t dpu_irq(int irq, void *arg)
>>   {
>> +    struct msm_kms *kms = arg;
>>       struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>       struct dpu_hw_intr *intr = dpu_kms->hw_intr;
>>       int reg_idx;
>> @@ -541,7 +542,7 @@ void dpu_debugfs_core_irq_init(struct dpu_kms 
>> *dpu_kms,
>>   }
>>   #endif
>> -void dpu_core_irq_preinstall(struct msm_kms *kms)
>> +static void dpu_core_irq_preinstall(struct msm_kms *kms)
>>   {
>>       struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>>       int i;
>> @@ -570,5 +571,28 @@ void dpu_core_irq_uninstall(struct msm_kms *kms)
>>       dpu_clear_irqs(dpu_kms);
>>       dpu_disable_all_irqs(dpu_kms);
>> +    if (kms->irq_requested)
>> +        free_irq(kms->irq, kms);
>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>   }
>> +
>> +int dpu_core_irq_install(struct msm_kms *kms)
>> +{
>> +    int ret;
>> +
>> +    dpu_core_irq_preinstall(kms);
>> +
>> +    ret = request_irq(kms->irq, dpu_irq, 0, "dpu", kms);
>> +    if (ret)
>> +        return ret;
>> +
>> +    kms->irq_requested = true;
>> +
>> +    ret = dpu_irq_postinstall(kms);
>> +    if (ret) {
>> +        free_irq(kms->irq, kms);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 2b9d931474e0..494978da7785 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -884,7 +884,7 @@ static void dpu_kms_destroy(struct msm_kms *kms)
>>           pm_runtime_disable(&dpu_kms->pdev->dev);
>>   }
>> -static int dpu_irq_postinstall(struct msm_kms *kms)
>> +int dpu_irq_postinstall(struct msm_kms *kms)
>>   {
>>       struct msm_drm_private *priv;
>>       struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>> @@ -960,10 +960,8 @@ static void dpu_kms_mdp_snapshot(struct 
>> msm_disp_state *disp_state, struct msm_k
>>   static const struct msm_kms_funcs kms_funcs = {
>>       .hw_init         = dpu_kms_hw_init,
>> -    .irq_preinstall  = dpu_core_irq_preinstall,
>> -    .irq_postinstall = dpu_irq_postinstall,
>> +    .irq_install     = dpu_core_irq_install,
>>       .irq_uninstall   = dpu_core_irq_uninstall,
>> -    .irq             = dpu_core_irq,
>>       .enable_commit   = dpu_kms_enable_commit,
>>       .disable_commit  = dpu_kms_disable_commit,
>>       .vsync_time      = dpu_kms_vsync_time,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> index 832a0769f2e7..559184c64045 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> @@ -204,4 +204,11 @@ void dpu_disable_vblank(struct msm_kms *kms, 
>> struct drm_crtc *crtc);
>>    */
>>   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
>> +/**
>> + * dpu_irq_postinstall - perform post-installation of core IRQ handler
>> + * @kms:               MSM KMS handle
>> + * @return:            non-zero in case of error
>> + */
>> +int dpu_irq_postinstall(struct msm_kms *kms);
>> +
>>   #endif /* __dpu_kms_H__ */
>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c 
>> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
>> index 4d49f3ba6a96..87675c162eea 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
>> @@ -32,7 +32,7 @@ static void mdp4_irq_error_handler(struct mdp_irq 
>> *irq, uint32_t irqstatus)
>>       }
>>   }
>> -void mdp4_irq_preinstall(struct msm_kms *kms)
>> +static void mdp4_irq_preinstall(struct msm_kms *kms)
>>   {
>>       struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
>>       mdp4_enable(mdp4_kms);
>> @@ -41,7 +41,7 @@ void mdp4_irq_preinstall(struct msm_kms *kms)
>>       mdp4_disable(mdp4_kms);
>>   }
>> -int mdp4_irq_postinstall(struct msm_kms *kms)
>> +static int mdp4_irq_postinstall(struct msm_kms *kms)
>>   {
>>       struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>>       struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
>> @@ -62,10 +62,13 @@ void mdp4_irq_uninstall(struct msm_kms *kms)
>>       mdp4_enable(mdp4_kms);
>>       mdp4_write(mdp4_kms, REG_MDP4_INTR_ENABLE, 0x00000000);
>>       mdp4_disable(mdp4_kms);
>> +    if (kms->irq_requested)
>> +        free_irq(kms->irq, kms);
>>   }
>> -irqreturn_t mdp4_irq(struct msm_kms *kms)
>> +static irqreturn_t mdp4_irq(int irq, void *arg)
>>   {
>> +    struct msm_kms *kms = arg;
>>       struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>>       struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
>>       struct drm_device *dev = mdp4_kms->dev;
>> @@ -88,6 +91,27 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
>>       return IRQ_HANDLED;
>>   }
>> +int mdp4_irq_install(struct msm_kms *kms)
>> +{
>> +    int ret;
>> +
>> +    mdp4_irq_preinstall(kms);
>> +
>> +    ret = request_irq(kms->irq, mdp4_irq, 0, "mdp4", kms);
>> +    if (ret)
>> +        return ret;
>> +
>> +    kms->irq_requested = true;
>> +
>> +    ret = mdp4_irq_postinstall(kms);
>> +    if (ret) {
>> +        free_irq(kms->irq, kms);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
>>   {
>>       struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c 
>> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>> index fb48c8c19ec3..b7aced272af9 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>> @@ -148,10 +148,8 @@ static void mdp4_destroy(struct msm_kms *kms)
>>   static const struct mdp_kms_funcs kms_funcs = {
>>       .base = {
>>           .hw_init         = mdp4_hw_init,
>> -        .irq_preinstall  = mdp4_irq_preinstall,
>> -        .irq_postinstall = mdp4_irq_postinstall,
>> +        .irq_install     = mdp4_irq_install,
>>           .irq_uninstall   = mdp4_irq_uninstall,
>> -        .irq             = mdp4_irq,
>>           .enable_vblank   = mdp4_enable_vblank,
>>           .disable_vblank  = mdp4_disable_vblank,
>>           .enable_commit   = mdp4_enable_commit,
>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h 
>> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
>> index e8ee92ab7956..b24a63872232 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
>> @@ -157,10 +157,8 @@ int mdp4_enable(struct mdp4_kms *mdp4_kms);
>>   void mdp4_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>>           uint32_t old_irqmask);
>> -void mdp4_irq_preinstall(struct msm_kms *kms);
>> -int mdp4_irq_postinstall(struct msm_kms *kms);
>> +int mdp4_irq_install(struct msm_kms *kms);
>>   void mdp4_irq_uninstall(struct msm_kms *kms);
>> -irqreturn_t mdp4_irq(struct msm_kms *kms);
>>   int mdp4_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>>   void mdp4_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c 
>> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
>> index 9b4c8d92ff32..d573ff29d5a4 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
>> @@ -36,7 +36,7 @@ static void mdp5_irq_error_handler(struct mdp_irq 
>> *irq, uint32_t irqstatus)
>>       }
>>   }
>> -void mdp5_irq_preinstall(struct msm_kms *kms)
>> +static void mdp5_irq_preinstall(struct msm_kms *kms)
>>   {
>>       struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
>>       struct device *dev = &mdp5_kms->pdev->dev;
>> @@ -47,7 +47,7 @@ void mdp5_irq_preinstall(struct msm_kms *kms)
>>       pm_runtime_put_sync(dev);
>>   }
>> -int mdp5_irq_postinstall(struct msm_kms *kms)
>> +static int mdp5_irq_postinstall(struct msm_kms *kms)
>>   {
>>       struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>>       struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>> @@ -74,11 +74,14 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
>>       pm_runtime_get_sync(dev);
>>       mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
>> +    if (kms->irq_requested)
>> +        free_irq(kms->irq, kms);
>>       pm_runtime_put_sync(dev);
>>   }
>> -irqreturn_t mdp5_irq(struct msm_kms *kms)
>> +static irqreturn_t mdp5_irq(int irq, void *arg)
>>   {
>> +    struct msm_kms *kms = arg;
>>       struct mdp_kms *mdp_kms = to_mdp_kms(kms);
>>       struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
>>       struct drm_device *dev = mdp5_kms->dev;
>> @@ -101,6 +104,27 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
>>       return IRQ_HANDLED;
>>   }
>> +int mdp5_irq_install(struct msm_kms *kms)
>> +{
>> +    int ret;
>> +
>> +    mdp5_irq_preinstall(kms);
>> +
>> +    ret = request_irq(kms->irq, mdp5_irq, 0, "mdp5", kms);
>> +    if (ret)
>> +        return ret;
>> +
>> +    kms->irq_requested = true;
>> +
>> +    ret = mdp5_irq_postinstall(kms);
>> +    if (ret) {
>> +        free_irq(kms->irq, kms);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
>>   {
>>       struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
>> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> index 3d5621a68f85..18cf0ff4da6c 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> @@ -262,10 +262,8 @@ static int mdp5_kms_debugfs_init(struct msm_kms 
>> *kms, struct drm_minor *minor)
>>   static const struct mdp_kms_funcs kms_funcs = {
>>       .base = {
>>           .hw_init         = mdp5_hw_init,
>> -        .irq_preinstall  = mdp5_irq_preinstall,
>> -        .irq_postinstall = mdp5_irq_postinstall,
>> +        .irq_install     = mdp5_irq_install,
>>           .irq_uninstall   = mdp5_irq_uninstall,
>> -        .irq             = mdp5_irq,
>>           .enable_vblank   = mdp5_enable_vblank,
>>           .disable_vblank  = mdp5_disable_vblank,
>>           .flush_commit    = mdp5_flush_commit,
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h 
>> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
>> index 29bf11f08601..630b5f812f24 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h
>> @@ -263,10 +263,8 @@ static inline uint32_t lm2ppdone(struct 
>> mdp5_hw_mixer *mixer)
>>   void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask,
>>           uint32_t old_irqmask);
>> -void mdp5_irq_preinstall(struct msm_kms *kms);
>> -int mdp5_irq_postinstall(struct msm_kms *kms);
>> +int mdp5_irq_install(struct msm_kms *kms);
>>   void mdp5_irq_uninstall(struct msm_kms *kms);
>> -irqreturn_t mdp5_irq(struct msm_kms *kms);
>>   int mdp5_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>>   void mdp5_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
>>   int mdp5_irq_domain_init(struct mdp5_kms *mdp5_kms);
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index 44485363f37a..d2fbe54fec4d 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -77,64 +77,15 @@ static bool modeset = true;
>>   MODULE_PARM_DESC(modeset, "Use kernel modesetting [KMS] (1=on 
>> (default), 0=disable)");
>>   module_param(modeset, bool, 0600);
>> -static irqreturn_t msm_irq(int irq, void *arg)
>> -{
>> -    struct drm_device *dev = arg;
>> -    struct msm_drm_private *priv = dev->dev_private;
>> -    struct msm_kms *kms = priv->kms;
>> -
>> -    BUG_ON(!kms);
>> -
>> -    return kms->funcs->irq(kms);
>> -}
>> -
>> -static void msm_irq_preinstall(struct drm_device *dev)
>> -{
>> -    struct msm_drm_private *priv = dev->dev_private;
>> -    struct msm_kms *kms = priv->kms;
>> -
>> -    BUG_ON(!kms);
>> -
>> -    kms->funcs->irq_preinstall(kms);
>> -}
>> -
>> -static int msm_irq_postinstall(struct drm_device *dev)
>> -{
>> -    struct msm_drm_private *priv = dev->dev_private;
>> -    struct msm_kms *kms = priv->kms;
>> -
>> -    BUG_ON(!kms);
>> -
>> -    if (kms->funcs->irq_postinstall)
>> -        return kms->funcs->irq_postinstall(kms);
>> -
>> -    return 0;
>> -}
>> -
>>   static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>>   {
>>       struct msm_drm_private *priv = dev->dev_private;
>>       struct msm_kms *kms = priv->kms;
>> -    int ret;
>> -
>> -    if (irq == IRQ_NOTCONNECTED)
>> -        return -ENOTCONN;
>> -
>> -    msm_irq_preinstall(dev);
>> -
>> -    ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
>> -    if (ret)
>> -        return ret;
>> -
>> -    kms->irq_requested = true;
>> -    ret = msm_irq_postinstall(dev);
>> -    if (ret) {
>> -        free_irq(irq, dev);
>> -        return ret;
>> -    }
>> +    if (!kms->funcs->irq_install)
>> +        return -EINVAL;
>> -    return 0;
>> +    return kms->funcs->irq_install(kms);
>>   }
>>   static void msm_irq_uninstall(struct drm_device *dev)
>> @@ -143,8 +94,6 @@ static void msm_irq_uninstall(struct drm_device *dev)
>>       struct msm_kms *kms = priv->kms;
>>       kms->funcs->irq_uninstall(kms);
>> -    if (kms->irq_requested)
>> -        free_irq(kms->irq, dev);
>>   }
>>   struct msm_vblank_work {
>> @@ -450,6 +399,11 @@ static int msm_drm_init(struct device *dev, const 
>> struct drm_driver *drv)
>>       }
>>       if (kms) {
>> +        if (kms->irq == IRQ_NOTCONNECTED) {
>> +            ret = -ENOTCONN;
>> +            goto err_msm_uninit;
>> +        }
>> +
>>           pm_runtime_get_sync(dev);
>>           ret = msm_irq_install(ddev, kms->irq);
>>           pm_runtime_put_sync(dev);
>> diff --git a/drivers/gpu/drm/msm/msm_kms.h 
>> b/drivers/gpu/drm/msm/msm_kms.h
>> index f8ed7588928c..71d497a8fb8b 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> @@ -24,10 +24,8 @@ struct msm_kms_funcs {
>>       /* hw initialization: */
>>       int (*hw_init)(struct msm_kms *kms);
>>       /* irq handling: */
>> -    void (*irq_preinstall)(struct msm_kms *kms);
>> -    int (*irq_postinstall)(struct msm_kms *kms);
>> +    int (*irq_install)(struct msm_kms *kms);
>>       void (*irq_uninstall)(struct msm_kms *kms);
>> -    irqreturn_t (*irq)(struct msm_kms *kms);
>>       int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
>>       void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/msm: don't free the IRQ if it was not requested

[Dmitry Baryshkov]

On 12/05/2022 04:41, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-11 18:30:55)
>> On 12/05/2022 04:29, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
>>>> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>>>>>
>>>>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
>>>>>
>>>>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
>>>>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
>>>>> that with this patch applied kms->irq_requested makes the check in
>>>>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>>>>
>>>> Yes.
>>>>
>>>
>>> I didn't see it deleted in the second patch so is a revert going to be
>>> sent?
>>
>> No need to. They are separate checks. The older one is superseded (as it
>> will be never hit),  but it is still valid and useful (to protect from
>> the crash if this code changes again).
>>
> 
> Ew, gross. The extra conditionals everywhere really makes it hard to
> follow along.

With the second patch being dropped I'd prefer to leave both conditions 
in place.

-- 
With best wishes
Dmitry