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