[PATCH v2 0/5] drm/msm: fixes for KMS iommu handling

[PATCH v2 0/5] drm/msm: fixes for KMS iommu handling

[Dmitry Baryshkov]

This series started from the applied and then reverted [2] patch by
Robin Murphy [1]. After the MDSS rework [3] has landed it is now
possible to reapply the extended version of the original patch. While we
are at it, also rework the IOMMU init code for DPU and MDP5 drivers.

For MDP5 this moves iommu_domain_alloc() call and removes struct
mdp5_cfg_platform remains.

For DPU this allows specifying the iommus = <...> either in the DPU
device (like all DPU devices do) or in the MDSS device (like MDP5
devices do).

Changes since v1:
 - Move aspace init to common helper
 - Use device_iommu_mapped() rather than semi-internal
   dev_iommu_fwspec_get() (suggested by Robin Murphy)

[1] https://patchwork.freedesktop.org/patch/480707/
[2] https://patchwork.freedesktop.org/patch/482453/
[3] https://patchwork.freedesktop.org/series/98525/

Dmitry Baryshkov (5):
  drm/msm/dpu: check both DPU and MDSS devices for the IOMMU
  drm/msm/mdp5: move iommu_domain_alloc() call close to its usage
  drm/msm: Stop using iommu_present()
  drm/msm: move KMS aspace init to the separate helper
  drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 24 ++---------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 --------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 31 +++-----------
 drivers/gpu/drm/msm/msm_drv.c            | 51 +++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 6 files changed, 59 insertions(+), 70 deletions(-)

-- 
2.35.1

[PATCH v2 0/5] drm/msm: fixes for KMS iommu handling

[Dmitry Baryshkov]

This series started from the applied and then reverted [2] patch by
Robin Murphy [1]. After the MDSS rework [3] has landed it is now
possible to reapply the extended version of the original patch. While we
are at it, also rework the IOMMU init code for DPU and MDP5 drivers.

For MDP5 this moves iommu_domain_alloc() call and removes struct
mdp5_cfg_platform remains.

For DPU this allows specifying the iommus = <...> either in the DPU
device (like all DPU devices do) or in the MDSS device (like MDP5
devices do).

Changes since v1:
 - Move aspace init to common helper
 - Use device_iommu_mapped() rather than semi-internal
   dev_iommu_fwspec_get() (suggested by Robin Murphy)

[1] https://patchwork.freedesktop.org/patch/480707/
[2] https://patchwork.freedesktop.org/patch/482453/
[3] https://patchwork.freedesktop.org/series/98525/

Dmitry Baryshkov (5):
  drm/msm/dpu: check both DPU and MDSS devices for the IOMMU
  drm/msm/mdp5: move iommu_domain_alloc() call close to its usage
  drm/msm: Stop using iommu_present()
  drm/msm: move KMS aspace init to the separate helper
  drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 24 ++---------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 --------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 31 +++-----------
 drivers/gpu/drm/msm/msm_drv.c            | 51 +++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 6 files changed, 59 insertions(+), 70 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Dmitry Baryshkov]

Follow the lead of MDP5 driver and check both DPU and MDSS devices for
the IOMMU specifiers.

Historically DPU devices had IOMMU specified in the MDSS device tree
node, but as some of MDP5 devices are being converted to the supported
by the DPU driver, the driver should adapt and check both devices.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 143d6643be53..5ccda0766f6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
 	struct msm_mmu *mmu;
 	struct device *dpu_dev = dpu_kms->dev->dev;
 	struct device *mdss_dev = dpu_dev->parent;
+	struct device *iommu_dev;
 
 	domain = iommu_domain_alloc(&platform_bus_type);
 	if (!domain)
 		return 0;
 
-	/* IOMMUs are a part of MDSS device tree binding, not the
-	 * MDP/DPU device. */
-	mmu = msm_iommu_new(mdss_dev, domain);
+	/*
+	 * IOMMUs can be a part of MDSS device tree binding, or the
+	 * MDP/DPU device.
+	 */
+	if (dev_iommu_fwspec_get(dpu_dev))
+		iommu_dev = dpu_dev;
+	else
+		iommu_dev = mdss_dev;
+
+	mmu = msm_iommu_new(iommu_dev, domain);
 	if (IS_ERR(mmu)) {
 		iommu_domain_free(domain);
 		return PTR_ERR(mmu);
-- 
2.35.1

[PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Dmitry Baryshkov]

Follow the lead of MDP5 driver and check both DPU and MDSS devices for
the IOMMU specifiers.

Historically DPU devices had IOMMU specified in the MDSS device tree
node, but as some of MDP5 devices are being converted to the supported
by the DPU driver, the driver should adapt and check both devices.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 143d6643be53..5ccda0766f6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
 	struct msm_mmu *mmu;
 	struct device *dpu_dev = dpu_kms->dev->dev;
 	struct device *mdss_dev = dpu_dev->parent;
+	struct device *iommu_dev;
 
 	domain = iommu_domain_alloc(&platform_bus_type);
 	if (!domain)
 		return 0;
 
-	/* IOMMUs are a part of MDSS device tree binding, not the
-	 * MDP/DPU device. */
-	mmu = msm_iommu_new(mdss_dev, domain);
+	/*
+	 * IOMMUs can be a part of MDSS device tree binding, or the
+	 * MDP/DPU device.
+	 */
+	if (dev_iommu_fwspec_get(dpu_dev))
+		iommu_dev = dpu_dev;
+	else
+		iommu_dev = mdss_dev;
+
+	mmu = msm_iommu_new(iommu_dev, domain);
 	if (IS_ERR(mmu)) {
 		iommu_domain_free(domain);
 		return PTR_ERR(mmu);
-- 
2.35.1

[PATCH v2 2/5] drm/msm/mdp5: move iommu_domain_alloc() call close to its usage

[Dmitry Baryshkov]

Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
This allows us to drop final bits of struct mdp5_cfg_platform which
remained from the pre-DT days.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 ----------------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++--
 3 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index 1bf9ff5dbabc..714effb967ff 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -1248,8 +1248,6 @@ static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
 	{ .revision = 3, .config = { .hw = &sdm630_config } },
 };
 
-static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev);
-
 const struct mdp5_cfg_hw *mdp5_cfg_get_hw_config(struct mdp5_cfg_handler *cfg_handler)
 {
 	return cfg_handler->config.hw;
@@ -1274,10 +1272,8 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 		uint32_t major, uint32_t minor)
 {
 	struct drm_device *dev = mdp5_kms->dev;
-	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct mdp5_cfg_handler *cfg_handler;
 	const struct mdp5_cfg_handler *cfg_handlers;
-	struct mdp5_cfg_platform *pconfig;
 	int i, ret = 0, num_handlers;
 
 	cfg_handler = kzalloc(sizeof(*cfg_handler), GFP_KERNEL);
@@ -1320,9 +1316,6 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 	cfg_handler->revision = minor;
 	cfg_handler->config.hw = mdp5_cfg;
 
-	pconfig = mdp5_get_config(pdev);
-	memcpy(&cfg_handler->config.platform, pconfig, sizeof(*pconfig));
-
 	DBG("MDP5: %s hw config selected", mdp5_cfg->name);
 
 	return cfg_handler;
@@ -1333,12 +1326,3 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 
 	return ERR_PTR(ret);
 }
-
-static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
-{
-	static struct mdp5_cfg_platform config = {};
-
-	config.iommu = iommu_domain_alloc(&platform_bus_type);
-
-	return &config;
-}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
index 6b03d7899309..c2502cc33864 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
@@ -104,14 +104,8 @@ struct mdp5_cfg_hw {
 	uint32_t max_clk;
 };
 
-/* platform config data (ie. from DT, or pdata) */
-struct mdp5_cfg_platform {
-	struct iommu_domain *iommu;
-};
-
 struct mdp5_cfg {
 	const struct mdp5_cfg_hw *hw;
-	struct mdp5_cfg_platform platform;
 };
 
 struct mdp5_kms;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 9b7bbc3adb97..1c67c2c828cd 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -558,6 +558,7 @@ static int mdp5_kms_init(struct drm_device *dev)
 	struct msm_gem_address_space *aspace;
 	int irq, i, ret;
 	struct device *iommu_dev;
+	struct iommu_domain *iommu;
 
 	ret = mdp5_init(to_platform_device(dev->dev), dev);
 
@@ -601,14 +602,15 @@ static int mdp5_kms_init(struct drm_device *dev)
 	}
 	mdelay(16);
 
-	if (config->platform.iommu) {
+	iommu = iommu_domain_alloc(&platform_bus_type);
+	if (iommu) {
 		struct msm_mmu *mmu;
 
 		iommu_dev = &pdev->dev;
 		if (!dev_iommu_fwspec_get(iommu_dev))
 			iommu_dev = iommu_dev->parent;
 
-		mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
+		mmu = msm_iommu_new(iommu_dev, iommu);
 
 		aspace = msm_gem_address_space_create(mmu, "mdp5",
 			0x1000, 0x100000000 - 0x1000);
-- 
2.35.1

[PATCH v2 2/5] drm/msm/mdp5: move iommu_domain_alloc() call close to its usage

[Dmitry Baryshkov]

Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
This allows us to drop final bits of struct mdp5_cfg_platform which
remained from the pre-DT days.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 ----------------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++--
 3 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index 1bf9ff5dbabc..714effb967ff 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -1248,8 +1248,6 @@ static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
 	{ .revision = 3, .config = { .hw = &sdm630_config } },
 };
 
-static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev);
-
 const struct mdp5_cfg_hw *mdp5_cfg_get_hw_config(struct mdp5_cfg_handler *cfg_handler)
 {
 	return cfg_handler->config.hw;
@@ -1274,10 +1272,8 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 		uint32_t major, uint32_t minor)
 {
 	struct drm_device *dev = mdp5_kms->dev;
-	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct mdp5_cfg_handler *cfg_handler;
 	const struct mdp5_cfg_handler *cfg_handlers;
-	struct mdp5_cfg_platform *pconfig;
 	int i, ret = 0, num_handlers;
 
 	cfg_handler = kzalloc(sizeof(*cfg_handler), GFP_KERNEL);
@@ -1320,9 +1316,6 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 	cfg_handler->revision = minor;
 	cfg_handler->config.hw = mdp5_cfg;
 
-	pconfig = mdp5_get_config(pdev);
-	memcpy(&cfg_handler->config.platform, pconfig, sizeof(*pconfig));
-
 	DBG("MDP5: %s hw config selected", mdp5_cfg->name);
 
 	return cfg_handler;
@@ -1333,12 +1326,3 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
 
 	return ERR_PTR(ret);
 }
-
-static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
-{
-	static struct mdp5_cfg_platform config = {};
-
-	config.iommu = iommu_domain_alloc(&platform_bus_type);
-
-	return &config;
-}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
index 6b03d7899309..c2502cc33864 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
@@ -104,14 +104,8 @@ struct mdp5_cfg_hw {
 	uint32_t max_clk;
 };
 
-/* platform config data (ie. from DT, or pdata) */
-struct mdp5_cfg_platform {
-	struct iommu_domain *iommu;
-};
-
 struct mdp5_cfg {
 	const struct mdp5_cfg_hw *hw;
-	struct mdp5_cfg_platform platform;
 };
 
 struct mdp5_kms;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 9b7bbc3adb97..1c67c2c828cd 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -558,6 +558,7 @@ static int mdp5_kms_init(struct drm_device *dev)
 	struct msm_gem_address_space *aspace;
 	int irq, i, ret;
 	struct device *iommu_dev;
+	struct iommu_domain *iommu;
 
 	ret = mdp5_init(to_platform_device(dev->dev), dev);
 
@@ -601,14 +602,15 @@ static int mdp5_kms_init(struct drm_device *dev)
 	}
 	mdelay(16);
 
-	if (config->platform.iommu) {
+	iommu = iommu_domain_alloc(&platform_bus_type);
+	if (iommu) {
 		struct msm_mmu *mmu;
 
 		iommu_dev = &pdev->dev;
 		if (!dev_iommu_fwspec_get(iommu_dev))
 			iommu_dev = iommu_dev->parent;
 
-		mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
+		mmu = msm_iommu_new(iommu_dev, iommu);
 
 		aspace = msm_gem_address_space_create(mmu, "mdp5",
 			0x1000, 0x100000000 - 0x1000);
-- 
2.35.1

[PATCH v2 3/5] drm/msm: Stop using iommu_present()

[Dmitry Baryshkov]

Even if some IOMMU has registered itself on the platform "bus", that
doesn't necessarily mean it provides translation for the device we
care about. Replace iommu_present() with a more appropriate check.

On Qualcomm platforms the IOMMU can be specified either for the MDP/DPU
device or for its parent MDSS device depending on the actual platform.
Check both of them, since that is how both DPU and MDP5 drivers work.

Co-developed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4a3dda23e3e0..a37a3bbc04d9 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -266,8 +266,14 @@ bool msm_use_mmu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 
-	/* a2xx comes with its own MMU */
-	return priv->is_a2xx || iommu_present(&platform_bus_type);
+	/*
+	 * a2xx comes with its own MMU
+	 * On other platforms IOMMU can be declared specified either for the
+	 * MDP/DPU device or for its parent, MDSS device.
+	 */
+	return priv->is_a2xx ||
+		device_iommu_mapped(dev->dev) ||
+		device_iommu_mapped(dev->dev->parent);
 }
 
 static int msm_init_vram(struct drm_device *dev)
-- 
2.35.1

[PATCH v2 3/5] drm/msm: Stop using iommu_present()

[Dmitry Baryshkov]

Even if some IOMMU has registered itself on the platform "bus", that
doesn't necessarily mean it provides translation for the device we
care about. Replace iommu_present() with a more appropriate check.

On Qualcomm platforms the IOMMU can be specified either for the MDP/DPU
device or for its parent MDSS device depending on the actual platform.
Check both of them, since that is how both DPU and MDP5 drivers work.

Co-developed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4a3dda23e3e0..a37a3bbc04d9 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -266,8 +266,14 @@ bool msm_use_mmu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 
-	/* a2xx comes with its own MMU */
-	return priv->is_a2xx || iommu_present(&platform_bus_type);
+	/*
+	 * a2xx comes with its own MMU
+	 * On other platforms IOMMU can be declared specified either for the
+	 * MDP/DPU device or for its parent, MDSS device.
+	 */
+	return priv->is_a2xx ||
+		device_iommu_mapped(dev->dev) ||
+		device_iommu_mapped(dev->dev->parent);
 }
 
 static int msm_init_vram(struct drm_device *dev)
-- 
2.35.1

[PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Dmitry Baryshkov]

MDP5 and DPU drivers have the same piece of code now to initialize
IOMMU and GEM address space. Move it to the msm_drv.c

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 32 ++----------------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 33 ++++---------------
 drivers/gpu/drm/msm/msm_drv.c            | 41 ++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 4 files changed, 51 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 5ccda0766f6c..45afe260329a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -999,40 +999,14 @@ static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
 
 static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
 {
-	struct iommu_domain *domain;
 	struct msm_gem_address_space *aspace;
-	struct msm_mmu *mmu;
-	struct device *dpu_dev = dpu_kms->dev->dev;
-	struct device *mdss_dev = dpu_dev->parent;
-	struct device *iommu_dev;
-
-	domain = iommu_domain_alloc(&platform_bus_type);
-	if (!domain)
-		return 0;
-
-	/*
-	 * IOMMUs can be a part of MDSS device tree binding, or the
-	 * MDP/DPU device.
-	 */
-	if (dev_iommu_fwspec_get(dpu_dev))
-		iommu_dev = dpu_dev;
-	else
-		iommu_dev = mdss_dev;
-
-	mmu = msm_iommu_new(iommu_dev, domain);
-	if (IS_ERR(mmu)) {
-		iommu_domain_free(domain);
-		return PTR_ERR(mmu);
-	}
-	aspace = msm_gem_address_space_create(mmu, "dpu1",
-		0x1000, 0x100000000 - 0x1000);
 
-	if (IS_ERR(aspace)) {
-		mmu->funcs->destroy(mmu);
+	aspace = msm_kms_init_aspace(dpu_kms->dev);
+	if (IS_ERR(aspace))
 		return PTR_ERR(aspace);
-	}
 
 	dpu_kms->base.aspace = aspace;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 1c67c2c828cd..1a4d02c353e0 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -557,8 +557,6 @@ static int mdp5_kms_init(struct drm_device *dev)
 	struct msm_kms *kms;
 	struct msm_gem_address_space *aspace;
 	int irq, i, ret;
-	struct device *iommu_dev;
-	struct iommu_domain *iommu;
 
 	ret = mdp5_init(to_platform_device(dev->dev), dev);
 
@@ -602,33 +600,14 @@ static int mdp5_kms_init(struct drm_device *dev)
 	}
 	mdelay(16);
 
-	iommu = iommu_domain_alloc(&platform_bus_type);
-	if (iommu) {
-		struct msm_mmu *mmu;
-
-		iommu_dev = &pdev->dev;
-		if (!dev_iommu_fwspec_get(iommu_dev))
-			iommu_dev = iommu_dev->parent;
-
-		mmu = msm_iommu_new(iommu_dev, iommu);
-
-		aspace = msm_gem_address_space_create(mmu, "mdp5",
-			0x1000, 0x100000000 - 0x1000);
-
-		if (IS_ERR(aspace)) {
-			if (!IS_ERR(mmu))
-				mmu->funcs->destroy(mmu);
-			ret = PTR_ERR(aspace);
-			goto fail;
-		}
-
-		kms->aspace = aspace;
-	} else {
-		DRM_DEV_INFO(&pdev->dev,
-			 "no iommu, fallback to phys contig buffers for scanout\n");
-		aspace = NULL;
+	aspace = msm_kms_init_aspace(mdp5_kms->dev);
+	if (IS_ERR(aspace)) {
+		ret = PTR_ERR(aspace);
+		goto fail;
 	}
 
+	kms->aspace = aspace;
+
 	pm_runtime_put_sync(&pdev->dev);
 
 	ret = modeset_init(mdp5_kms);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index a37a3bbc04d9..98ae0036ab57 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -26,6 +26,7 @@
 #include "msm_gem.h"
 #include "msm_gpu.h"
 #include "msm_kms.h"
+#include "msm_mmu.h"
 #include "adreno/adreno_gpu.h"
 
 /*
@@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
 
 #include <linux/of_address.h>
 
+struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
+{
+	struct iommu_domain *domain;
+	struct msm_gem_address_space *aspace;
+	struct msm_mmu *mmu;
+	struct device *mdp_dev = dev->dev;
+	struct device *mdss_dev = mdp_dev->parent;
+	struct device *iommu_dev;
+
+	domain = iommu_domain_alloc(&platform_bus_type);
+	if (!domain) {
+		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
+		return NULL;
+	}
+
+	/*
+	 * IOMMUs can be a part of MDSS device tree binding, or the
+	 * MDP/DPU device.
+	 */
+	if (dev_iommu_fwspec_get(mdp_dev))
+		iommu_dev = mdp_dev;
+	else
+		iommu_dev = mdss_dev;
+
+	mmu = msm_iommu_new(iommu_dev, domain);
+	if (IS_ERR(mmu)) {
+		iommu_domain_free(domain);
+		return ERR_CAST(mmu);
+	}
+
+	aspace = msm_gem_address_space_create(mmu, "mdp_kms",
+		0x1000, 0x100000000 - 0x1000);
+	if (IS_ERR(aspace)) {
+		mmu->funcs->destroy(mmu);
+		return aspace;
+	}
+
+	return aspace;
+}
+
 bool msm_use_mmu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index fdbaad53eb84..99c2c14c558d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -234,6 +234,7 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc);
 int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
 void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
 
+struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev);
 bool msm_use_mmu(struct drm_device *dev);
 
 int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
-- 
2.35.1

[PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Dmitry Baryshkov]

MDP5 and DPU drivers have the same piece of code now to initialize
IOMMU and GEM address space. Move it to the msm_drv.c

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 32 ++----------------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 33 ++++---------------
 drivers/gpu/drm/msm/msm_drv.c            | 41 ++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 4 files changed, 51 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 5ccda0766f6c..45afe260329a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -999,40 +999,14 @@ static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
 
 static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
 {
-	struct iommu_domain *domain;
 	struct msm_gem_address_space *aspace;
-	struct msm_mmu *mmu;
-	struct device *dpu_dev = dpu_kms->dev->dev;
-	struct device *mdss_dev = dpu_dev->parent;
-	struct device *iommu_dev;
-
-	domain = iommu_domain_alloc(&platform_bus_type);
-	if (!domain)
-		return 0;
-
-	/*
-	 * IOMMUs can be a part of MDSS device tree binding, or the
-	 * MDP/DPU device.
-	 */
-	if (dev_iommu_fwspec_get(dpu_dev))
-		iommu_dev = dpu_dev;
-	else
-		iommu_dev = mdss_dev;
-
-	mmu = msm_iommu_new(iommu_dev, domain);
-	if (IS_ERR(mmu)) {
-		iommu_domain_free(domain);
-		return PTR_ERR(mmu);
-	}
-	aspace = msm_gem_address_space_create(mmu, "dpu1",
-		0x1000, 0x100000000 - 0x1000);
 
-	if (IS_ERR(aspace)) {
-		mmu->funcs->destroy(mmu);
+	aspace = msm_kms_init_aspace(dpu_kms->dev);
+	if (IS_ERR(aspace))
 		return PTR_ERR(aspace);
-	}
 
 	dpu_kms->base.aspace = aspace;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 1c67c2c828cd..1a4d02c353e0 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -557,8 +557,6 @@ static int mdp5_kms_init(struct drm_device *dev)
 	struct msm_kms *kms;
 	struct msm_gem_address_space *aspace;
 	int irq, i, ret;
-	struct device *iommu_dev;
-	struct iommu_domain *iommu;
 
 	ret = mdp5_init(to_platform_device(dev->dev), dev);
 
@@ -602,33 +600,14 @@ static int mdp5_kms_init(struct drm_device *dev)
 	}
 	mdelay(16);
 
-	iommu = iommu_domain_alloc(&platform_bus_type);
-	if (iommu) {
-		struct msm_mmu *mmu;
-
-		iommu_dev = &pdev->dev;
-		if (!dev_iommu_fwspec_get(iommu_dev))
-			iommu_dev = iommu_dev->parent;
-
-		mmu = msm_iommu_new(iommu_dev, iommu);
-
-		aspace = msm_gem_address_space_create(mmu, "mdp5",
-			0x1000, 0x100000000 - 0x1000);
-
-		if (IS_ERR(aspace)) {
-			if (!IS_ERR(mmu))
-				mmu->funcs->destroy(mmu);
-			ret = PTR_ERR(aspace);
-			goto fail;
-		}
-
-		kms->aspace = aspace;
-	} else {
-		DRM_DEV_INFO(&pdev->dev,
-			 "no iommu, fallback to phys contig buffers for scanout\n");
-		aspace = NULL;
+	aspace = msm_kms_init_aspace(mdp5_kms->dev);
+	if (IS_ERR(aspace)) {
+		ret = PTR_ERR(aspace);
+		goto fail;
 	}
 
+	kms->aspace = aspace;
+
 	pm_runtime_put_sync(&pdev->dev);
 
 	ret = modeset_init(mdp5_kms);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index a37a3bbc04d9..98ae0036ab57 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -26,6 +26,7 @@
 #include "msm_gem.h"
 #include "msm_gpu.h"
 #include "msm_kms.h"
+#include "msm_mmu.h"
 #include "adreno/adreno_gpu.h"
 
 /*
@@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
 
 #include <linux/of_address.h>
 
+struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
+{
+	struct iommu_domain *domain;
+	struct msm_gem_address_space *aspace;
+	struct msm_mmu *mmu;
+	struct device *mdp_dev = dev->dev;
+	struct device *mdss_dev = mdp_dev->parent;
+	struct device *iommu_dev;
+
+	domain = iommu_domain_alloc(&platform_bus_type);
+	if (!domain) {
+		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
+		return NULL;
+	}
+
+	/*
+	 * IOMMUs can be a part of MDSS device tree binding, or the
+	 * MDP/DPU device.
+	 */
+	if (dev_iommu_fwspec_get(mdp_dev))
+		iommu_dev = mdp_dev;
+	else
+		iommu_dev = mdss_dev;
+
+	mmu = msm_iommu_new(iommu_dev, domain);
+	if (IS_ERR(mmu)) {
+		iommu_domain_free(domain);
+		return ERR_CAST(mmu);
+	}
+
+	aspace = msm_gem_address_space_create(mmu, "mdp_kms",
+		0x1000, 0x100000000 - 0x1000);
+	if (IS_ERR(aspace)) {
+		mmu->funcs->destroy(mmu);
+		return aspace;
+	}
+
+	return aspace;
+}
+
 bool msm_use_mmu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index fdbaad53eb84..99c2c14c558d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -234,6 +234,7 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc);
 int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
 void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
 
+struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev);
 bool msm_use_mmu(struct drm_device *dev);
 
 int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
-- 
2.35.1

[PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Dmitry Baryshkov]

Change msm_kms_init_aspace() to use generic function
device_iommu_mapped() instead of the fwnode-specific interface
dev_iommu_fwspec_get(). While we are at it, stop referencing
platform_bus_type directly and use the bus of the IOMMU device.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 98ae0036ab57..2fc3f820cd59 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
 	struct device *mdss_dev = mdp_dev->parent;
 	struct device *iommu_dev;
 
-	domain = iommu_domain_alloc(&platform_bus_type);
-	if (!domain) {
-		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
-		return NULL;
-	}
-
 	/*
 	 * IOMMUs can be a part of MDSS device tree binding, or the
 	 * MDP/DPU device.
 	 */
-	if (dev_iommu_fwspec_get(mdp_dev))
+	if (device_iommu_mapped(mdp_dev))
 		iommu_dev = mdp_dev;
 	else
 		iommu_dev = mdss_dev;
 
+	domain = iommu_domain_alloc(iommu_dev->bus);
+	if (!domain) {
+		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
+		return NULL;
+	}
+
 	mmu = msm_iommu_new(iommu_dev, domain);
 	if (IS_ERR(mmu)) {
 		iommu_domain_free(domain);
-- 
2.35.1

[PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Dmitry Baryshkov]

Change msm_kms_init_aspace() to use generic function
device_iommu_mapped() instead of the fwnode-specific interface
dev_iommu_fwspec_get(). While we are at it, stop referencing
platform_bus_type directly and use the bus of the IOMMU device.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 98ae0036ab57..2fc3f820cd59 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
 	struct device *mdss_dev = mdp_dev->parent;
 	struct device *iommu_dev;
 
-	domain = iommu_domain_alloc(&platform_bus_type);
-	if (!domain) {
-		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
-		return NULL;
-	}
-
 	/*
 	 * IOMMUs can be a part of MDSS device tree binding, or the
 	 * MDP/DPU device.
 	 */
-	if (dev_iommu_fwspec_get(mdp_dev))
+	if (device_iommu_mapped(mdp_dev))
 		iommu_dev = mdp_dev;
 	else
 		iommu_dev = mdss_dev;
 
+	domain = iommu_domain_alloc(iommu_dev->bus);
+	if (!domain) {
+		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
+		return NULL;
+	}
+
 	mmu = msm_iommu_new(iommu_dev, domain);
 	if (IS_ERR(mmu)) {
 		iommu_domain_free(domain);
-- 
2.35.1

Re: [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Robin Murphy]

On 2022-05-05 01:16, Dmitry Baryshkov wrote:
> Change msm_kms_init_aspace() to use generic function
> device_iommu_mapped() instead of the fwnode-specific interface
> dev_iommu_fwspec_get(). While we are at it, stop referencing
> platform_bus_type directly and use the bus of the IOMMU device.

FWIW, I'd have squashed these changes across the previous patches, such 
that the dodgy fwspec calls are never introduced in the first place, but 
it's your driver, and if that's the way you want to work it and Rob's 
happy with it too, then fine by me.

For the end result,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well, 
but unless there's any other reason to respin this series, that's 
something we could do as a follow-up. Thanks for sorting this out!

Robin.

> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 98ae0036ab57..2fc3f820cd59 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>   	struct device *mdss_dev = mdp_dev->parent;
>   	struct device *iommu_dev;
>   
> -	domain = iommu_domain_alloc(&platform_bus_type);
> -	if (!domain) {
> -		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> -		return NULL;
> -	}
> -
>   	/*
>   	 * IOMMUs can be a part of MDSS device tree binding, or the
>   	 * MDP/DPU device.
>   	 */
> -	if (dev_iommu_fwspec_get(mdp_dev))
> +	if (device_iommu_mapped(mdp_dev))
>   		iommu_dev = mdp_dev;
>   	else
>   		iommu_dev = mdss_dev;
>   
> +	domain = iommu_domain_alloc(iommu_dev->bus);
> +	if (!domain) {
> +		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> +		return NULL;
> +	}
> +
>   	mmu = msm_iommu_new(iommu_dev, domain);
>   	if (IS_ERR(mmu)) {
>   		iommu_domain_free(domain);

Re: [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Robin Murphy]

On 2022-05-05 01:16, Dmitry Baryshkov wrote:
> Change msm_kms_init_aspace() to use generic function
> device_iommu_mapped() instead of the fwnode-specific interface
> dev_iommu_fwspec_get(). While we are at it, stop referencing
> platform_bus_type directly and use the bus of the IOMMU device.

FWIW, I'd have squashed these changes across the previous patches, such 
that the dodgy fwspec calls are never introduced in the first place, but 
it's your driver, and if that's the way you want to work it and Rob's 
happy with it too, then fine by me.

For the end result,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well, 
but unless there's any other reason to respin this series, that's 
something we could do as a follow-up. Thanks for sorting this out!

Robin.

> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 98ae0036ab57..2fc3f820cd59 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>   	struct device *mdss_dev = mdp_dev->parent;
>   	struct device *iommu_dev;
>   
> -	domain = iommu_domain_alloc(&platform_bus_type);
> -	if (!domain) {
> -		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> -		return NULL;
> -	}
> -
>   	/*
>   	 * IOMMUs can be a part of MDSS device tree binding, or the
>   	 * MDP/DPU device.
>   	 */
> -	if (dev_iommu_fwspec_get(mdp_dev))
> +	if (device_iommu_mapped(mdp_dev))
>   		iommu_dev = mdp_dev;
>   	else
>   		iommu_dev = mdss_dev;
>   
> +	domain = iommu_domain_alloc(iommu_dev->bus);
> +	if (!domain) {
> +		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> +		return NULL;
> +	}
> +
>   	mmu = msm_iommu_new(iommu_dev, domain);
>   	if (IS_ERR(mmu)) {
>   		iommu_domain_free(domain);

Re: [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Dmitry Baryshkov]

On Thu, 5 May 2022 at 13:27, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-05-05 01:16, Dmitry Baryshkov wrote:
> > Change msm_kms_init_aspace() to use generic function
> > device_iommu_mapped() instead of the fwnode-specific interface
> > dev_iommu_fwspec_get(). While we are at it, stop referencing
> > platform_bus_type directly and use the bus of the IOMMU device.
>
> FWIW, I'd have squashed these changes across the previous patches, such
> that the dodgy fwspec calls are never introduced in the first place, but
> it's your driver, and if that's the way you want to work it and Rob's
> happy with it too, then fine by me.

I thought about this. But as the calls were already there (in the
mdp5), it was easier for me to merge the code and to update it
afterwards.

>
> For the end result,
>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well,
> but unless there's any other reason to respin this series, that's
> something we could do as a follow-up. Thanks for sorting this out!

Not really. MDP4 doesn't have the parent MDSS device, so it doesn't
need all these troubles.

>
> Robin.
>
> > Suggested-by: Robin Murphy <robin.murphy@arm.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 98ae0036ab57..2fc3f820cd59 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
> >       struct device *mdss_dev = mdp_dev->parent;
> >       struct device *iommu_dev;
> >
> > -     domain = iommu_domain_alloc(&platform_bus_type);
> > -     if (!domain) {
> > -             drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> > -             return NULL;
> > -     }
> > -
> >       /*
> >        * IOMMUs can be a part of MDSS device tree binding, or the
> >        * MDP/DPU device.
> >        */
> > -     if (dev_iommu_fwspec_get(mdp_dev))
> > +     if (device_iommu_mapped(mdp_dev))
> >               iommu_dev = mdp_dev;
> >       else
> >               iommu_dev = mdss_dev;
> >
> > +     domain = iommu_domain_alloc(iommu_dev->bus);
> > +     if (!domain) {
> > +             drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> > +             return NULL;
> > +     }
> > +
> >       mmu = msm_iommu_new(iommu_dev, domain);
> >       if (IS_ERR(mmu)) {
> >               iommu_domain_free(domain);



-- 
With best wishes
Dmitry

Re: [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()

[Dmitry Baryshkov]

On Thu, 5 May 2022 at 13:27, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-05-05 01:16, Dmitry Baryshkov wrote:
> > Change msm_kms_init_aspace() to use generic function
> > device_iommu_mapped() instead of the fwnode-specific interface
> > dev_iommu_fwspec_get(). While we are at it, stop referencing
> > platform_bus_type directly and use the bus of the IOMMU device.
>
> FWIW, I'd have squashed these changes across the previous patches, such
> that the dodgy fwspec calls are never introduced in the first place, but
> it's your driver, and if that's the way you want to work it and Rob's
> happy with it too, then fine by me.

I thought about this. But as the calls were already there (in the
mdp5), it was easier for me to merge the code and to update it
afterwards.

>
> For the end result,
>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well,
> but unless there's any other reason to respin this series, that's
> something we could do as a follow-up. Thanks for sorting this out!

Not really. MDP4 doesn't have the parent MDSS device, so it doesn't
need all these troubles.

>
> Robin.
>
> > Suggested-by: Robin Murphy <robin.murphy@arm.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_drv.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 98ae0036ab57..2fc3f820cd59 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
> >       struct device *mdss_dev = mdp_dev->parent;
> >       struct device *iommu_dev;
> >
> > -     domain = iommu_domain_alloc(&platform_bus_type);
> > -     if (!domain) {
> > -             drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> > -             return NULL;
> > -     }
> > -
> >       /*
> >        * IOMMUs can be a part of MDSS device tree binding, or the
> >        * MDP/DPU device.
> >        */
> > -     if (dev_iommu_fwspec_get(mdp_dev))
> > +     if (device_iommu_mapped(mdp_dev))
> >               iommu_dev = mdp_dev;
> >       else
> >               iommu_dev = mdss_dev;
> >
> > +     domain = iommu_domain_alloc(iommu_dev->bus);
> > +     if (!domain) {
> > +             drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
> > +             return NULL;
> > +     }
> > +
> >       mmu = msm_iommu_new(iommu_dev, domain);
> >       if (IS_ERR(mmu)) {
> >               iommu_domain_free(domain);



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
> the IOMMU specifiers.
> 
> Historically DPU devices had IOMMU specified in the MDSS device tree
> node, but as some of MDP5 devices are being converted to the supported
> by the DPU driver, the driver should adapt and check both devices.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 143d6643be53..5ccda0766f6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>   	struct msm_mmu *mmu;
>   	struct device *dpu_dev = dpu_kms->dev->dev;
>   	struct device *mdss_dev = dpu_dev->parent;
> +	struct device *iommu_dev;
>   
>   	domain = iommu_domain_alloc(&platform_bus_type);
>   	if (!domain)
>   		return 0;
>   
> -	/* IOMMUs are a part of MDSS device tree binding, not the
> -	 * MDP/DPU device. */
> -	mmu = msm_iommu_new(mdss_dev, domain);
> +	/*
> +	 * IOMMUs can be a part of MDSS device tree binding, or the
> +	 * MDP/DPU device.
> +	 */

Can you please explain this a little more?

So even if some of the mdp5 devices are getting converted to use DPU 
driver, their device trees are also updated right?

In other words, if DPU driver was using mdss_dev to initialize the 
iommu, why should the new devices which are going to use DPU have the 
binding in the dpu_dev?


> +	if (dev_iommu_fwspec_get(dpu_dev))
> +		iommu_dev = dpu_dev;
> +	else
> +		iommu_dev = mdss_dev;
> +
> +	mmu = msm_iommu_new(iommu_dev, domain);
>   	if (IS_ERR(mmu)) {
>   		iommu_domain_free(domain);
>   		return PTR_ERR(mmu);

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
> the IOMMU specifiers.
> 
> Historically DPU devices had IOMMU specified in the MDSS device tree
> node, but as some of MDP5 devices are being converted to the supported
> by the DPU driver, the driver should adapt and check both devices.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 143d6643be53..5ccda0766f6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>   	struct msm_mmu *mmu;
>   	struct device *dpu_dev = dpu_kms->dev->dev;
>   	struct device *mdss_dev = dpu_dev->parent;
> +	struct device *iommu_dev;
>   
>   	domain = iommu_domain_alloc(&platform_bus_type);
>   	if (!domain)
>   		return 0;
>   
> -	/* IOMMUs are a part of MDSS device tree binding, not the
> -	 * MDP/DPU device. */
> -	mmu = msm_iommu_new(mdss_dev, domain);
> +	/*
> +	 * IOMMUs can be a part of MDSS device tree binding, or the
> +	 * MDP/DPU device.
> +	 */

Can you please explain this a little more?

So even if some of the mdp5 devices are getting converted to use DPU 
driver, their device trees are also updated right?

In other words, if DPU driver was using mdss_dev to initialize the 
iommu, why should the new devices which are going to use DPU have the 
binding in the dpu_dev?


> +	if (dev_iommu_fwspec_get(dpu_dev))
> +		iommu_dev = dpu_dev;
> +	else
> +		iommu_dev = mdss_dev;
> +
> +	mmu = msm_iommu_new(iommu_dev, domain);
>   	if (IS_ERR(mmu)) {
>   		iommu_domain_free(domain);
>   		return PTR_ERR(mmu);

Re: [PATCH v2 2/5] drm/msm/mdp5: move iommu_domain_alloc() call close to its usage

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
> This allows us to drop final bits of struct mdp5_cfg_platform which
> remained from the pre-DT days.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 ----------------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++--
>   3 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 1bf9ff5dbabc..714effb967ff 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -1248,8 +1248,6 @@ static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
>   	{ .revision = 3, .config = { .hw = &sdm630_config } },
>   };
>   
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev);
> -
>   const struct mdp5_cfg_hw *mdp5_cfg_get_hw_config(struct mdp5_cfg_handler *cfg_handler)
>   {
>   	return cfg_handler->config.hw;
> @@ -1274,10 +1272,8 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   		uint32_t major, uint32_t minor)
>   {
>   	struct drm_device *dev = mdp5_kms->dev;
> -	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct mdp5_cfg_handler *cfg_handler;
>   	const struct mdp5_cfg_handler *cfg_handlers;
> -	struct mdp5_cfg_platform *pconfig;
>   	int i, ret = 0, num_handlers;
>   
>   	cfg_handler = kzalloc(sizeof(*cfg_handler), GFP_KERNEL);
> @@ -1320,9 +1316,6 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   	cfg_handler->revision = minor;
>   	cfg_handler->config.hw = mdp5_cfg;
>   
> -	pconfig = mdp5_get_config(pdev);
> -	memcpy(&cfg_handler->config.platform, pconfig, sizeof(*pconfig));
> -
>   	DBG("MDP5: %s hw config selected", mdp5_cfg->name);
>   
>   	return cfg_handler;
> @@ -1333,12 +1326,3 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   
>   	return ERR_PTR(ret);
>   }
> -
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
> -{
> -	static struct mdp5_cfg_platform config = {};
> -
> -	config.iommu = iommu_domain_alloc(&platform_bus_type);
> -
> -	return &config;
> -}
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> index 6b03d7899309..c2502cc33864 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> @@ -104,14 +104,8 @@ struct mdp5_cfg_hw {
>   	uint32_t max_clk;
>   };
>   
> -/* platform config data (ie. from DT, or pdata) */
> -struct mdp5_cfg_platform {
> -	struct iommu_domain *iommu;
> -};
> -
>   struct mdp5_cfg {
>   	const struct mdp5_cfg_hw *hw;
> -	struct mdp5_cfg_platform platform;
>   };
>   
>   struct mdp5_kms;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 9b7bbc3adb97..1c67c2c828cd 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -558,6 +558,7 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	struct msm_gem_address_space *aspace;
>   	int irq, i, ret;
>   	struct device *iommu_dev;
> +	struct iommu_domain *iommu;
>   
>   	ret = mdp5_init(to_platform_device(dev->dev), dev);
>   
> @@ -601,14 +602,15 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	}
>   	mdelay(16);
>   
> -	if (config->platform.iommu) {
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (iommu) {
>   		struct msm_mmu *mmu;
>   
>   		iommu_dev = &pdev->dev;
>   		if (!dev_iommu_fwspec_get(iommu_dev))
>   			iommu_dev = iommu_dev->parent;
>   
> -		mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
> +		mmu = msm_iommu_new(iommu_dev, iommu);
>   
>   		aspace = msm_gem_address_space_create(mmu, "mdp5",
>   			0x1000, 0x100000000 - 0x1000);

Re: [PATCH v2 2/5] drm/msm/mdp5: move iommu_domain_alloc() call close to its usage

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
> This allows us to drop final bits of struct mdp5_cfg_platform which
> remained from the pre-DT days.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 ----------------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++--
>   3 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 1bf9ff5dbabc..714effb967ff 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -1248,8 +1248,6 @@ static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
>   	{ .revision = 3, .config = { .hw = &sdm630_config } },
>   };
>   
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev);
> -
>   const struct mdp5_cfg_hw *mdp5_cfg_get_hw_config(struct mdp5_cfg_handler *cfg_handler)
>   {
>   	return cfg_handler->config.hw;
> @@ -1274,10 +1272,8 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   		uint32_t major, uint32_t minor)
>   {
>   	struct drm_device *dev = mdp5_kms->dev;
> -	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct mdp5_cfg_handler *cfg_handler;
>   	const struct mdp5_cfg_handler *cfg_handlers;
> -	struct mdp5_cfg_platform *pconfig;
>   	int i, ret = 0, num_handlers;
>   
>   	cfg_handler = kzalloc(sizeof(*cfg_handler), GFP_KERNEL);
> @@ -1320,9 +1316,6 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   	cfg_handler->revision = minor;
>   	cfg_handler->config.hw = mdp5_cfg;
>   
> -	pconfig = mdp5_get_config(pdev);
> -	memcpy(&cfg_handler->config.platform, pconfig, sizeof(*pconfig));
> -
>   	DBG("MDP5: %s hw config selected", mdp5_cfg->name);
>   
>   	return cfg_handler;
> @@ -1333,12 +1326,3 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   
>   	return ERR_PTR(ret);
>   }
> -
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
> -{
> -	static struct mdp5_cfg_platform config = {};
> -
> -	config.iommu = iommu_domain_alloc(&platform_bus_type);
> -
> -	return &config;
> -}
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> index 6b03d7899309..c2502cc33864 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> @@ -104,14 +104,8 @@ struct mdp5_cfg_hw {
>   	uint32_t max_clk;
>   };
>   
> -/* platform config data (ie. from DT, or pdata) */
> -struct mdp5_cfg_platform {
> -	struct iommu_domain *iommu;
> -};
> -
>   struct mdp5_cfg {
>   	const struct mdp5_cfg_hw *hw;
> -	struct mdp5_cfg_platform platform;
>   };
>   
>   struct mdp5_kms;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 9b7bbc3adb97..1c67c2c828cd 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -558,6 +558,7 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	struct msm_gem_address_space *aspace;
>   	int irq, i, ret;
>   	struct device *iommu_dev;
> +	struct iommu_domain *iommu;
>   
>   	ret = mdp5_init(to_platform_device(dev->dev), dev);
>   
> @@ -601,14 +602,15 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	}
>   	mdelay(16);
>   
> -	if (config->platform.iommu) {
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (iommu) {
>   		struct msm_mmu *mmu;
>   
>   		iommu_dev = &pdev->dev;
>   		if (!dev_iommu_fwspec_get(iommu_dev))
>   			iommu_dev = iommu_dev->parent;
>   
> -		mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
> +		mmu = msm_iommu_new(iommu_dev, iommu);
>   
>   		aspace = msm_gem_address_space_create(mmu, "mdp5",
>   			0x1000, 0x100000000 - 0x1000);

Re: [PATCH v2 3/5] drm/msm: Stop using iommu_present()

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Even if some IOMMU has registered itself on the platform "bus", that
> doesn't necessarily mean it provides translation for the device we
> care about. Replace iommu_present() with a more appropriate check.
> 
> On Qualcomm platforms the IOMMU can be specified either for the MDP/DPU
> device or for its parent MDSS device depending on the actual platform.
> Check both of them, since that is how both DPU and MDP5 drivers work.
> 
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
I have the same question as patch (1) of this series here but i will let 
you answer it there, so this one is,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4a3dda23e3e0..a37a3bbc04d9 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -266,8 +266,14 @@ bool msm_use_mmu(struct drm_device *dev)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
>   
> -	/* a2xx comes with its own MMU */
> -	return priv->is_a2xx || iommu_present(&platform_bus_type);
> +	/*
> +	 * a2xx comes with its own MMU
> +	 * On other platforms IOMMU can be declared specified either for the
> +	 * MDP/DPU device or for its parent, MDSS device.
> +	 */
> +	return priv->is_a2xx ||
> +		device_iommu_mapped(dev->dev) ||
> +		device_iommu_mapped(dev->dev->parent);
>   }
>   
>   static int msm_init_vram(struct drm_device *dev)

Re: [PATCH v2 3/5] drm/msm: Stop using iommu_present()

[Abhinav Kumar]

On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
> Even if some IOMMU has registered itself on the platform "bus", that
> doesn't necessarily mean it provides translation for the device we
> care about. Replace iommu_present() with a more appropriate check.
> 
> On Qualcomm platforms the IOMMU can be specified either for the MDP/DPU
> device or for its parent MDSS device depending on the actual platform.
> Check both of them, since that is how both DPU and MDP5 drivers work.
> 
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
I have the same question as patch (1) of this series here but i will let 
you answer it there, so this one is,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4a3dda23e3e0..a37a3bbc04d9 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -266,8 +266,14 @@ bool msm_use_mmu(struct drm_device *dev)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
>   
> -	/* a2xx comes with its own MMU */
> -	return priv->is_a2xx || iommu_present(&platform_bus_type);
> +	/*
> +	 * a2xx comes with its own MMU
> +	 * On other platforms IOMMU can be declared specified either for the
> +	 * MDP/DPU device or for its parent, MDSS device.
> +	 */
> +	return priv->is_a2xx ||
> +		device_iommu_mapped(dev->dev) ||
> +		device_iommu_mapped(dev->dev->parent);
>   }
>   
>   static int msm_init_vram(struct drm_device *dev)

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Dmitry Baryshkov]

On 15/06/2022 20:55, Abhinav Kumar wrote:
> 
> 
> On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
>> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
>> the IOMMU specifiers.
>>
>> Historically DPU devices had IOMMU specified in the MDSS device tree
>> node, but as some of MDP5 devices are being converted to the supported
>> by the DPU driver, the driver should adapt and check both devices.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 143d6643be53..5ccda0766f6c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms 
>> *dpu_kms)
>>       struct msm_mmu *mmu;
>>       struct device *dpu_dev = dpu_kms->dev->dev;
>>       struct device *mdss_dev = dpu_dev->parent;
>> +    struct device *iommu_dev;
>>       domain = iommu_domain_alloc(&platform_bus_type);
>>       if (!domain)
>>           return 0;
>> -    /* IOMMUs are a part of MDSS device tree binding, not the
>> -     * MDP/DPU device. */
>> -    mmu = msm_iommu_new(mdss_dev, domain);
>> +    /*
>> +     * IOMMUs can be a part of MDSS device tree binding, or the
>> +     * MDP/DPU device.
>> +     */
> 
> Can you please explain this a little more?
> 
> So even if some of the mdp5 devices are getting converted to use DPU 
> driver, their device trees are also updated right?

No. The DT describes the device, not the Linux drivers. So while 
updating the drivers we should not change the DT.

> 
> In other words, if DPU driver was using mdss_dev to initialize the 
> iommu, why should the new devices which are going to use DPU have the 
> binding in the dpu_dev?
> 
> 
>> +    if (dev_iommu_fwspec_get(dpu_dev))
>> +        iommu_dev = dpu_dev;
>> +    else
>> +        iommu_dev = mdss_dev;
>> +
>> +    mmu = msm_iommu_new(iommu_dev, domain);
>>       if (IS_ERR(mmu)) {
>>           iommu_domain_free(domain);
>>           return PTR_ERR(mmu);


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Dmitry Baryshkov]

On 15/06/2022 20:55, Abhinav Kumar wrote:
> 
> 
> On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
>> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
>> the IOMMU specifiers.
>>
>> Historically DPU devices had IOMMU specified in the MDSS device tree
>> node, but as some of MDP5 devices are being converted to the supported
>> by the DPU driver, the driver should adapt and check both devices.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 143d6643be53..5ccda0766f6c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms 
>> *dpu_kms)
>>       struct msm_mmu *mmu;
>>       struct device *dpu_dev = dpu_kms->dev->dev;
>>       struct device *mdss_dev = dpu_dev->parent;
>> +    struct device *iommu_dev;
>>       domain = iommu_domain_alloc(&platform_bus_type);
>>       if (!domain)
>>           return 0;
>> -    /* IOMMUs are a part of MDSS device tree binding, not the
>> -     * MDP/DPU device. */
>> -    mmu = msm_iommu_new(mdss_dev, domain);
>> +    /*
>> +     * IOMMUs can be a part of MDSS device tree binding, or the
>> +     * MDP/DPU device.
>> +     */
> 
> Can you please explain this a little more?
> 
> So even if some of the mdp5 devices are getting converted to use DPU 
> driver, their device trees are also updated right?

No. The DT describes the device, not the Linux drivers. So while 
updating the drivers we should not change the DT.

> 
> In other words, if DPU driver was using mdss_dev to initialize the 
> iommu, why should the new devices which are going to use DPU have the 
> binding in the dpu_dev?
> 
> 
>> +    if (dev_iommu_fwspec_get(dpu_dev))
>> +        iommu_dev = dpu_dev;
>> +    else
>> +        iommu_dev = mdss_dev;
>> +
>> +    mmu = msm_iommu_new(iommu_dev, domain);
>>       if (IS_ERR(mmu)) {
>>           iommu_domain_free(domain);
>>           return PTR_ERR(mmu);


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Abhinav Kumar]

On 6/15/2022 11:42 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 20:55, Abhinav Kumar wrote:
>>
>>
>> On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
>>> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
>>> the IOMMU specifiers.
>>>
>>> Historically DPU devices had IOMMU specified in the MDSS device tree
>>> node, but as some of MDP5 devices are being converted to the supported
>>> by the DPU driver, the driver should adapt and check both devices.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 143d6643be53..5ccda0766f6c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms 
>>> *dpu_kms)
>>>       struct msm_mmu *mmu;
>>>       struct device *dpu_dev = dpu_kms->dev->dev;
>>>       struct device *mdss_dev = dpu_dev->parent;
>>> +    struct device *iommu_dev;
>>>       domain = iommu_domain_alloc(&platform_bus_type);
>>>       if (!domain)
>>>           return 0;
>>> -    /* IOMMUs are a part of MDSS device tree binding, not the
>>> -     * MDP/DPU device. */
>>> -    mmu = msm_iommu_new(mdss_dev, domain);
>>> +    /*
>>> +     * IOMMUs can be a part of MDSS device tree binding, or the
>>> +     * MDP/DPU device.
>>> +     */
>>
>> Can you please explain this a little more?
>>
>> So even if some of the mdp5 devices are getting converted to use DPU 
>> driver, their device trees are also updated right?
> 
> No. The DT describes the device, not the Linux drivers. So while 
> updating the drivers we should not change the DT.
> 
Alright, agreed

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> In other words, if DPU driver was using mdss_dev to initialize the 
>> iommu, why should the new devices which are going to use DPU have the 
>> binding in the dpu_dev?
>>
>>
>>> +    if (dev_iommu_fwspec_get(dpu_dev))
>>> +        iommu_dev = dpu_dev;
>>> +    else
>>> +        iommu_dev = mdss_dev;
>>> +
>>> +    mmu = msm_iommu_new(iommu_dev, domain);
>>>       if (IS_ERR(mmu)) {
>>>           iommu_domain_free(domain);
>>>           return PTR_ERR(mmu);
> 
> 

Re: [Freedreno] [PATCH v2 1/5] drm/msm/dpu: check both DPU and MDSS devices for the IOMMU

[Abhinav Kumar]

On 6/15/2022 11:42 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 20:55, Abhinav Kumar wrote:
>>
>>
>> On 5/4/2022 5:16 PM, Dmitry Baryshkov wrote:
>>> Follow the lead of MDP5 driver and check both DPU and MDSS devices for
>>> the IOMMU specifiers.
>>>
>>> Historically DPU devices had IOMMU specified in the MDSS device tree
>>> node, but as some of MDP5 devices are being converted to the supported
>>> by the DPU driver, the driver should adapt and check both devices.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 143d6643be53..5ccda0766f6c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -1004,14 +1004,22 @@ static int _dpu_kms_mmu_init(struct dpu_kms 
>>> *dpu_kms)
>>>       struct msm_mmu *mmu;
>>>       struct device *dpu_dev = dpu_kms->dev->dev;
>>>       struct device *mdss_dev = dpu_dev->parent;
>>> +    struct device *iommu_dev;
>>>       domain = iommu_domain_alloc(&platform_bus_type);
>>>       if (!domain)
>>>           return 0;
>>> -    /* IOMMUs are a part of MDSS device tree binding, not the
>>> -     * MDP/DPU device. */
>>> -    mmu = msm_iommu_new(mdss_dev, domain);
>>> +    /*
>>> +     * IOMMUs can be a part of MDSS device tree binding, or the
>>> +     * MDP/DPU device.
>>> +     */
>>
>> Can you please explain this a little more?
>>
>> So even if some of the mdp5 devices are getting converted to use DPU 
>> driver, their device trees are also updated right?
> 
> No. The DT describes the device, not the Linux drivers. So while 
> updating the drivers we should not change the DT.
> 
Alright, agreed

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> In other words, if DPU driver was using mdss_dev to initialize the 
>> iommu, why should the new devices which are going to use DPU have the 
>> binding in the dpu_dev?
>>
>>
>>> +    if (dev_iommu_fwspec_get(dpu_dev))
>>> +        iommu_dev = dpu_dev;
>>> +    else
>>> +        iommu_dev = mdss_dev;
>>> +
>>> +    mmu = msm_iommu_new(iommu_dev, domain);
>>>       if (IS_ERR(mmu)) {
>>>           iommu_domain_free(domain);
>>>           return PTR_ERR(mmu);
> 
> 

Re: [PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-04 17:16:04)
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index a37a3bbc04d9..98ae0036ab57 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
>
>  #include <linux/of_address.h>
>
> +struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
> +{
[...]
> +       }
> +
> +       aspace = msm_gem_address_space_create(mmu, "mdp_kms",
> +               0x1000, 0x100000000 - 0x1000);
> +       if (IS_ERR(aspace)) {
> +               mmu->funcs->destroy(mmu);


> +               return aspace;
> +       }
> +
> +       return aspace;

This can be 'return aspace' one time instead of two.

> +}
> +
>  bool msm_use_mmu(struct drm_device *dev)
>  {
>         struct msm_drm_private *priv = dev->dev_private;

Re: [PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-05-04 17:16:04)
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index a37a3bbc04d9..98ae0036ab57 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
>
>  #include <linux/of_address.h>
>
> +struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
> +{
[...]
> +       }
> +
> +       aspace = msm_gem_address_space_create(mmu, "mdp_kms",
> +               0x1000, 0x100000000 - 0x1000);
> +       if (IS_ERR(aspace)) {
> +               mmu->funcs->destroy(mmu);


> +               return aspace;
> +       }
> +
> +       return aspace;

This can be 'return aspace' one time instead of two.

> +}
> +
>  bool msm_use_mmu(struct drm_device *dev)
>  {
>         struct msm_drm_private *priv = dev->dev_private;

Re: [PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Dmitry Baryshkov]

On 16/06/2022 05:34, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-04 17:16:04)
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index a37a3bbc04d9..98ae0036ab57 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
>>
>>   #include <linux/of_address.h>
>>
>> +struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>> +{
> [...]
>> +       }
>> +
>> +       aspace = msm_gem_address_space_create(mmu, "mdp_kms",
>> +               0x1000, 0x100000000 - 0x1000);
>> +       if (IS_ERR(aspace)) {
>> +               mmu->funcs->destroy(mmu);
> 
> 
>> +               return aspace;
>> +       }
>> +
>> +       return aspace;
> 
> This can be 'return aspace' one time instead of two.

Yes. I was just always in favour of explicit error returns rather than 
falling through. I'll send v2.

> 
>> +}
>> +
>>   bool msm_use_mmu(struct drm_device *dev)
>>   {
>>          struct msm_drm_private *priv = dev->dev_private;


-- 
With best wishes
Dmitry

Re: [PATCH v2 4/5] drm/msm: move KMS aspace init to the separate helper

[Dmitry Baryshkov]

On 16/06/2022 05:34, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-04 17:16:04)
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index a37a3bbc04d9..98ae0036ab57 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -262,6 +263,46 @@ static int msm_drm_uninit(struct device *dev)
>>
>>   #include <linux/of_address.h>
>>
>> +struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>> +{
> [...]
>> +       }
>> +
>> +       aspace = msm_gem_address_space_create(mmu, "mdp_kms",
>> +               0x1000, 0x100000000 - 0x1000);
>> +       if (IS_ERR(aspace)) {
>> +               mmu->funcs->destroy(mmu);
> 
> 
>> +               return aspace;
>> +       }
>> +
>> +       return aspace;
> 
> This can be 'return aspace' one time instead of two.

Yes. I was just always in favour of explicit error returns rather than 
falling through. I'll send v2.

> 
>> +}
>> +
>>   bool msm_use_mmu(struct drm_device *dev)
>>   {
>>          struct msm_drm_private *priv = dev->dev_private;


-- 
With best wishes
Dmitry