Re: [Freedreno] [Patch v3 ] drm/msm/dpu: Correct dpu destroy and disable order
On 2018-11-18 22:25, Jayant Shekhar wrote: In case of msm drm bind failure, pm runtime put sync is called from dsi driver which issues an asynchronous put on mdss device. Subsequently when dpu_mdss_destroy is triggered the change will make sure to put the mdss device in suspend and clearing pending work if not scheduled. Changes in v2: - Removed double spacings [Jeykumar] Changes in v3: - Fix clock on issue during bootup [Rajendra] Signed-off-by: Jayant Shekhar --- Acked-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c index fd9c893..df8127b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c @@ -156,18 +156,16 @@ static void dpu_mdss_destroy(struct drm_device *dev) struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss); struct dss_module_power *mp = _mdss->mp; + pm_runtime_suspend(dev->dev); + pm_runtime_disable(dev->dev); _dpu_mdss_irq_domain_fini(dpu_mdss); - free_irq(platform_get_irq(pdev, 0), dpu_mdss); - msm_dss_put_clk(mp->clk_config, mp->num_clk); devm_kfree(>dev, mp->clk_config); if (dpu_mdss->mmio) devm_iounmap(>dev, dpu_mdss->mmio); dpu_mdss->mmio = NULL; - - pm_runtime_disable(dev->dev); priv->mdss = NULL; } -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
This solves a problem we see with drm/msm, caused by getting
iommu_dma_ops while we attach our own domain and manage it directly at
the iommu API level:
[0038] user address but active_mm is swapper
Internal error: Oops: 9605 [#1] PREEMPT SMP
Modules linked in:
CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: GW 4.19.3 #90
Hardware name: xxx (DT)
Workqueue: events deferred_probe_work_func
pstate: 80c9 (Nzcv daif +PAN +UAO)
pc : iommu_dma_map_sg+0x7c/0x2c8
lr : iommu_dma_map_sg+0x40/0x2c8
sp : ff80095eb4f0
x29: ff80095eb4f0 x28:
x27: ffc0f9431578 x26:
x25: x24: 0003
x23: 0001 x22: ffc0fa9ac010
x21: x20: ffc0fab40980
x19: ffc0fab40980 x18: 0003
x17: 01c4 x16: 0007
x15: 000e x14:
x13: x12: 0028
x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
x9 : x8 : ffc0fab409a0
x7 : x6 : 0002
x5 : 0001 x4 :
x3 : 0001 x2 : 0002
x1 : ffc0f9431578 x0 :
Process kworker/7:1 (pid: 70, stack limit = 0x17d08ffb)
Call trace:
iommu_dma_map_sg+0x7c/0x2c8
__iommu_map_sg_attrs+0x70/0x84
get_pages+0x170/0x1e8
msm_gem_get_iova+0x8c/0x128
_msm_gem_kernel_new+0x6c/0xc8
msm_gem_kernel_new+0x4c/0x58
dsi_tx_buf_alloc_6g+0x4c/0x8c
msm_dsi_host_modeset_init+0xc8/0x108
msm_dsi_modeset_init+0x54/0x18c
_dpu_kms_drm_obj_init+0x430/0x474
dpu_kms_hw_init+0x5f8/0x6b4
msm_drm_bind+0x360/0x6c8
try_to_bring_up_master.part.7+0x28/0x70
component_master_add_with_match+0xe8/0x124
msm_pdev_probe+0x294/0x2b4
platform_drv_probe+0x58/0xa4
really_probe+0x150/0x294
driver_probe_device+0xac/0xe8
__device_attach_driver+0xa4/0xb4
bus_for_each_drv+0x98/0xc8
__device_attach+0xac/0x12c
device_initial_probe+0x24/0x30
bus_probe_device+0x38/0x98
deferred_probe_work_func+0x78/0xa4
process_one_work+0x24c/0x3dc
worker_thread+0x280/0x360
kthread+0x134/0x13c
ret_from_fork+0x10/0x18
Code: d284 91000725 6b17039f 5400048a (f9401f40)
---[ end trace f22dda57f3648e2c ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x0,22802a18
Memory Limit: none
The problem is that when drm/msm does it's own iommu_attach_device(),
now the domain returned by iommu_get_domain_for_dev() is drm/msm's
domain, and it doesn't have domain->iova_cookie.
We kind of avoided this problem prior to sdm845/dpu because the iommu
was attached to the mdp node in dt, which is a child of the toplevel
mdss node (which corresponds to the dev passed in dma_map_sg()). But
with sdm845, now the iommu is attached at the mdss level so we hit the
iommu_dma_ops in dma_map_sg().
But auto allocating/attaching a domain before the driver is probed was
already a blocking problem for enabling per-context pagetables for the
GPU. This problem is also now solved with this patch.
Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
Tested-by: Douglas Anderson
Signed-off-by: Rob Clark
---
This is an alternative/replacement for [1]. What it lacks in elegance
it makes up for in practicality ;-)
[1] https://patchwork.freedesktop.org/patch/264930/
drivers/of/device.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5957cd4fa262..15ffee00fb22 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
return device_add(>dev);
}
+static const struct of_device_id iommu_blacklist[] = {
+ { .compatible = "qcom,mdp4" },
+ { .compatible = "qcom,mdss" },
+ { .compatible = "qcom,sdm845-mdss" },
+ { .compatible = "qcom,adreno" },
+ {}
+};
+
/**
* of_dma_configure - Setup DMA configuration
* @dev: Device to apply DMA configuration
@@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct
device_node *np, bool force_dma)
dev_dbg(dev, "device is%sbehind an iommu\n",
iommu ? " " : " not ");
+ /*
+* There is at least one case where the driver wants to directly
+* manage the IOMMU, but if we end up with iommu dma_ops, that
+* interferes with the drivers ability to use dma_map_sg() for
+* cache operations. Since we don't currently have a better
+* solution, and this code runs before the driver is probed and
+* has a chance to intervene, use a simple blacklist to avoid
+* ending up with iommu dma_ops:
+*/
+ if (of_match_device(iommu_blacklist, dev)) {
+ dev_dbg(dev, "skipping iommu hookup\n");
+ iommu = NULL;
+
Re: [Freedreno] [PATCH v3 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*
On Fri, Nov 30, 2018 at 9:05 PM Tomasz Figa wrote: > > On Thu, Nov 29, 2018 at 4:23 PM Tomasz Figa wrote: > > > > On Thu, Nov 29, 2018 at 12:03 PM Robin Murphy wrote: > > > > > > On 29/11/2018 19:57, Tomasz Figa wrote: > > > > On Thu, Nov 29, 2018 at 11:40 AM Jordan Crouse > > > > wrote: > > > >> > > > >> On Thu, Nov 29, 2018 at 01:48:15PM -0500, Rob Clark wrote: > > > >>> On Thu, Nov 29, 2018 at 10:54 AM Christoph Hellwig > > > >>> wrote: > > > > > > On Thu, Nov 29, 2018 at 09:42:50AM -0500, Rob Clark wrote: > > > > Maybe the thing we need to do is just implement a blacklist of > > > > compatible strings for devices which should skip the automatic > > > > iommu/dma hookup. Maybe a bit ugly, but it would also solve a > > > > problem > > > > preventing us from enabling per-process pagetables for a5xx (where > > > > we > > > > need to control the domain/context-bank that is allocated by the dma > > > > api). > > > > > > You can detach from the dma map attachment using > > > arm_iommu_detach_device, > > > which a few drm drivers do, but I don't think this is the problem. > > > >>> > > > >>> I think even with detach, we wouldn't end up with the context-bank > > > >>> that the gpu firmware was hard-coded to expect, and so it would > > > >>> overwrite the incorrect page table address register. (I could be > > > >>> mis-remembering that, Jordan spent more time looking at that. But it > > > >>> was something along those lines.) > > > >> > > > >> Right - basically the DMA domain steals context bank 0 and the GPU is > > > >> hard coded > > > >> to use that context bank for pagetable switching. > > > >> > > > >> I believe the Tegra guys also had a similar problem with a hard coded > > > >> context > > > >> bank. > > > > > > AIUI, they don't need a specific hardware context, they just need to > > > know which one they're actually using, which the domain abstraction hides. > > > > > > > Wait, if we detach the GPU/display struct device from the default > > > > domain and attach it to a newly allocated domain, wouldn't the newly > > > > allocated domain use the context bank we need? Note that we're already > > > > > > The arm-smmu driver doesn't, but there's no fundamental reason it > > > couldn't. That should just need code to refcount domain users and > > > release hardware contexts for domains with no devices currently attached. > > > > > > Robin. > > > > > > > doing that, except that we're doing it behind the back of the DMA > > > > mapping subsystem, so that it keeps using the IOMMU version of the DMA > > > > ops for the device and doing any mapping operations on the default > > > > domain. If we ask the DMA mapping to detach, wouldn't it essentially > > > > solve the problem? > > > > Thanks Robin. > > > > Still, my point is that the MSM DRM driver attaches the GPU struct > > device to a new domain it allocates using iommu_domain_alloc() and it > > seems to work fine, so I believe it's not the problem we're looking > > into with this patch. > > Could we just make the MSM DRM call arch_teardown_dma_ops() and then > arch_setup_dma_ops() with the `iommu` argument set to NULL and be done > with it? I don't think those are exported to modules? I have actually a simpler patch, that adds a small blacklist to check in of_dma_configure() before calling arch_setup_dma_ops(), which can replace this patch. It also solves the problem of dma api allocating the context bank that he gpu wants to use for context-switching, and should be a simple thing to backport to stable branches. I was just spending some time trying to figure out what changed recently to start causing dma_map_sg() to opps on boot for us, so I could write a more detailed commit msg. BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
