Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
On Mon, Dec 3, 2018 at 7:56 PM Rob Clark wrote: > > On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy wrote: > > > > Hi Rob, > > > > On 01/12/2018 16:53, Rob Clark wrote: > > > 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. > > > > Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it > > really shouldn't. > > > > > 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. > > > > s/solved/worked around/ > > > > If you want a guarantee of actually getting a specific hardware context > > allocated for a given domain, there needs to be code in the IOMMU driver > > to understand and honour that. Implicitly depending on whatever happens > > to fall out of current driver behaviour on current systems is not a real > > solution. > > > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in > > > of_dma_configure > > > > That's rather misleading, since the crash described above depends on at > > least two other major changes which came long after that commit. > > > > It's not that I don't understand exactly what you want here - just that > > this commit message isn't a coherent justification for that ;) > > > > > 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
[Freedreno] [PATCH v19 3/5] iommu/arm-smmu: Add the device_link between masters and smmu
From: Sricharan R Finally add the device link between the master device and smmu, so that the smmu gets runtime enabled/disabled only when the master needs it. This is done from add_device callback which gets called once when the master is added to the smmu. Signed-off-by: Sricharan R Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla Reviewed-by: Robin Murphy --- Changes since v18: None. drivers/iommu/arm-smmu.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 1917d214c4d9..b6b11642b3a9 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1500,6 +1500,9 @@ static int arm_smmu_add_device(struct device *dev) iommu_device_link(&smmu->iommu, dev); + device_link_add(dev, smmu->dev, + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER); + return 0; out_cfg_free: -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v19 5/5] iommu/arm-smmu: Add support for qcom, smmu-v2 variant
qcom,smmu-v2 is an arm,smmu-v2 implementation with specific clock and power requirements. On msm8996, multiple cores, viz. mdss, video, etc. use this smmu. On sdm845, this smmu is used with gpu. Add bindings for the same. Signed-off-by: Vivek Gautam Reviewed-by: Rob Herring Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla Reviewed-by: Robin Murphy --- Changes since v18: None. drivers/iommu/arm-smmu.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index b6b11642b3a9..ba18d89d4732 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -120,6 +120,7 @@ enum arm_smmu_implementation { GENERIC_SMMU, ARM_MMU500, CAVIUM_SMMUV2, + QCOM_SMMUV2, }; struct arm_smmu_s2cr { @@ -2030,6 +2031,7 @@ ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU); ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU); ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500); ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2); +ARM_SMMU_MATCH_DATA(qcom_smmuv2, ARM_SMMU_V2, QCOM_SMMUV2); static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 }, @@ -2038,6 +2040,7 @@ static const struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,mmu-401", .data = &arm_mmu401 }, { .compatible = "arm,mmu-500", .data = &arm_mmu500 }, { .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 }, + { .compatible = "qcom,smmu-v2", .data = &qcom_smmuv2 }, { }, }; MODULE_DEVICE_TABLE(of, arm_smmu_of_match); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v19 4/5] dt-bindings: arm-smmu: Add bindings for qcom, smmu-v2
Add bindings doc for Qcom's smmu-v2 implementation. Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla Reviewed-by: Rob Herring Reviewed-by: Robin Murphy --- Changes since v18: None. .../devicetree/bindings/iommu/arm,smmu.txt | 39 ++ 1 file changed, 39 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt index 8a6ffce12af5..a6504b37cc21 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt @@ -17,10 +17,16 @@ conditions. "arm,mmu-401" "arm,mmu-500" "cavium,smmu-v2" +"qcom,smmu-v2" depending on the particular implementation and/or the version of the architecture implemented. + Qcom SoCs must contain, as below, SoC-specific compatibles + along with "qcom,smmu-v2": + "qcom,msm8996-smmu-v2", "qcom,smmu-v2", + "qcom,sdm845-smmu-v2", "qcom,smmu-v2". + - reg : Base address and size of the SMMU. - #global-interrupts : The number of global interrupts exposed by the @@ -71,6 +77,22 @@ conditions. or using stream matching with #iommu-cells = <2>, and may be ignored if present in such cases. +- clock-names:List of the names of clocks input to the device. The + required list depends on particular implementation and + is as follows: + - for "qcom,smmu-v2": +- "bus": clock required for downstream bus access and + for the smmu ptw, +- "iface": clock required to access smmu's registers + through the TCU's programming interface. + - unspecified for other implementations. + +- clocks: Specifiers for all clocks listed in the clock-names property, + as per generic clock bindings. + +- power-domains: Specifiers for power domains required to be powered on for + the SMMU to operate, as per generic power domain bindings. + ** Deprecated properties: - mmu-masters (deprecated in favour of the generic "iommus" binding) : @@ -137,3 +159,20 @@ conditions. iommu-map = <0 &smmu3 0 0x400>; ... }; + + /* Qcom's arm,smmu-v2 implementation */ + smmu4: iommu@d0 { + compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2"; + reg = <0xd0 0x1>; + + #global-interrupts = <1>; + interrupts = , +, +; + #iommu-cells = <1>; + power-domains = <&mmcc MDSS_GDSC>; + + clocks = <&mmcc SMMU_MDP_AXI_CLK>, +<&mmcc SMMU_MDP_AHB_CLK>; + clock-names = "bus", "iface"; + }; -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v19 2/5] iommu/arm-smmu: Invoke pm_runtime across the driver
From: Sricharan R Enable pm-runtime on devices that implement a pm domain. Then, add pm runtime hooks to several iommu_ops to power cycle the smmu device for explicit TLB invalidation requests, and register space accesses, etc. We need these hooks when the smmu, linked to its master through device links, has to be powered-up without the master device being in context. Signed-off-by: Sricharan R [vivek: Cleanup pm runtime calls] Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla Reviewed-by: Robin Murphy --- Changes since v18: None. drivers/iommu/arm-smmu.c | 108 ++- 1 file changed, 98 insertions(+), 10 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 602b67d4f2d6..1917d214c4d9 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -270,6 +270,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { { 0, NULL}, }; +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) +{ + if (pm_runtime_enabled(smmu->dev)) + return pm_runtime_get_sync(smmu->dev); + + return 0; +} + +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) +{ + if (pm_runtime_enabled(smmu->dev)) + pm_runtime_put(smmu->dev); +} + static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) { return container_of(dom, struct arm_smmu_domain, domain); @@ -929,11 +943,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_device *smmu = smmu_domain->smmu; struct arm_smmu_cfg *cfg = &smmu_domain->cfg; - int irq; + int ret, irq; if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) return; + ret = arm_smmu_rpm_get(smmu); + if (ret < 0) + return; + /* * Disable the context bank and free the page tables before freeing * it. @@ -948,6 +966,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) free_io_pgtable_ops(smmu_domain->pgtbl_ops); __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); + + arm_smmu_rpm_put(smmu); } static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) @@ -1229,10 +1249,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) return -ENODEV; smmu = fwspec_smmu(fwspec); + + ret = arm_smmu_rpm_get(smmu); + if (ret < 0) + return ret; + /* Ensure that the domain is finalised */ ret = arm_smmu_init_domain_context(domain, smmu); if (ret < 0) - return ret; + goto rpm_put; /* * Sanity check the domain. We don't support domains across @@ -1242,49 +1267,74 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) dev_err(dev, "cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n", dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev)); - return -EINVAL; + ret = -EINVAL; + goto rpm_put; } /* Looks ok, so add the device to the domain */ - return arm_smmu_domain_add_master(smmu_domain, fwspec); + ret = arm_smmu_domain_add_master(smmu_domain, fwspec); + +rpm_put: + arm_smmu_rpm_put(smmu); + return ret; } static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t size, int prot) { struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops; + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu; + int ret; if (!ops) return -ENODEV; - return ops->map(ops, iova, paddr, size, prot); + arm_smmu_rpm_get(smmu); + ret = ops->map(ops, iova, paddr, size, prot); + arm_smmu_rpm_put(smmu); + + return ret; } static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size) { struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops; + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu; + size_t ret; if (!ops) return 0; - return ops->unmap(ops, iova, size); + arm_smmu_rpm_get(smmu); + ret = ops->unmap(ops, iova, size); + arm_smmu_rpm_put(smmu); + + return ret; } static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain) { struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + struct arm_smmu_device *smmu = smmu_domain->smmu; - if (smmu_domain->tlb_ops) + if (smmu_domain->tlb_ops) { + arm_smmu_rpm_get(smmu); smmu_domain-
[Freedreno] [PATCH v19 1/5] iommu/arm-smmu: Add pm_runtime/sleep ops
From: Sricharan R The smmu needs to be functional only when the respective master's using it are active. The device_link feature helps to track such functional dependencies, so that the iommu gets powered when the master device enables itself using pm_runtime. So by adapting the smmu driver for runtime pm, above said dependency can be addressed. This patch adds the pm runtime/sleep callbacks to the driver and the corresponding bulk clock handling for all the clocks needed by smmu. Also, while we enable the runtime pm, add a pm sleep suspend callback that pushes devices to low power state by turning the clocks off in a system sleep. Add corresponding clock enable path in resume callback as well. Signed-off-by: Sricharan R Signed-off-by: Archit Taneja [Thor: Rework to get clocks from device tree] Signed-off-by: Thor Thayer [vivek: rework for clock and pm ops] Signed-off-by: Vivek Gautam Reviewed-by: Tomasz Figa Tested-by: Srinivas Kandagatla Reviewed-by: Robin Murphy Tested-by: Thor Thayer --- Changes since v18: - Replaced the entire clock bulk data filling and handling with devm_clk_bulk_get_all(). drivers/iommu/arm-smmu.c | 58 +--- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 5a28ae892504..602b67d4f2d6 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -48,6 +48,7 @@ #include #include #include +#include #include #include @@ -206,6 +207,8 @@ struct arm_smmu_device { u32 num_global_irqs; u32 num_context_irqs; unsigned int*irqs; + struct clk_bulk_data*clks; + int num_clks; u32 cavium_id_base; /* Specific to Cavium */ @@ -1947,7 +1950,7 @@ struct arm_smmu_match_data { }; #define ARM_SMMU_MATCH_DATA(name, ver, imp)\ -static struct arm_smmu_match_data name = { .version = ver, .model = imp } +static const struct arm_smmu_match_data name = { .version = ver, .model = imp } ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU); ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU); @@ -2150,6 +2153,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev) smmu->irqs[i] = irq; } + err = devm_clk_bulk_get_all(dev, &smmu->clks); + if (err < 0) { + dev_err(dev, "failed to get clocks %d\n", err); + return err; + } + smmu->num_clks = err; + + err = clk_bulk_prepare_enable(smmu->num_clks, smmu->clks); + if (err) + return err; + err = arm_smmu_device_cfg_probe(smmu); if (err) return err; @@ -2236,6 +2250,9 @@ static int arm_smmu_device_remove(struct platform_device *pdev) /* Turn the thing off */ writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); + + clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks); + return 0; } @@ -2244,15 +2261,50 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev) arm_smmu_device_remove(pdev); } -static int __maybe_unused arm_smmu_pm_resume(struct device *dev) +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) { struct arm_smmu_device *smmu = dev_get_drvdata(dev); + int ret; + + ret = clk_bulk_enable(smmu->num_clks, smmu->clks); + if (ret) + return ret; arm_smmu_device_reset(smmu); + return 0; } -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev) +{ + struct arm_smmu_device *smmu = dev_get_drvdata(dev); + + clk_bulk_disable(smmu->num_clks, smmu->clks); + + return 0; +} + +static int __maybe_unused arm_smmu_pm_resume(struct device *dev) +{ + if (pm_runtime_suspended(dev)) + return 0; + + return arm_smmu_runtime_resume(dev); +} + +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) +{ + if (pm_runtime_suspended(dev)) + return 0; + + return arm_smmu_runtime_suspend(dev); +} + +static const struct dev_pm_ops arm_smmu_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume) + SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend, + arm_smmu_runtime_resume, NULL) +}; static struct platform_driver arm_smmu_driver = { .driver = { -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v19 0/5] iommu/arm-smmu: Add runtime pm/sleep support
Changes since v18: - Addressing Stephen's comment [5]: Replaced the entire clock bulk data filling and handling with devm_clk_bulk_get_all(). Changes since v17: - Addressing Will's comment to embed Thor's change [2] for pulling clocks information from device tree. This is done by squashing Thor's change [2] in v17's 1/5 patch [3]. - Another minor change is addition of runtime pm hooks to arm_smmu_iova_to_phys_hard(). Previous version of this patch series is @ [1]. Also refer to [4] for change logs for previous versions. [1] https://lore.kernel.org/patchwork/cover/1017699/ [2] https://lore.kernel.org/patchwork/patch/996143/ [3] https://lore.kernel.org/patchwork/patch/1013167/ [4] https://lore.kernel.org/patchwork/cover/979429/ [5] https://lore.kernel.org/patchwork/patch/1017700/ Sricharan R (3): iommu/arm-smmu: Add pm_runtime/sleep ops iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam (2): dt-bindings: arm-smmu: Add bindings for qcom,smmu-v2 iommu/arm-smmu: Add support for qcom,smmu-v2 variant .../devicetree/bindings/iommu/arm,smmu.txt | 39 + drivers/iommu/arm-smmu.c | 170 +++-- 2 files changed, 197 insertions(+), 12 deletions(-) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm/dpu: add display port support in DPU
On 2018-12-03 06:47, Sean Paul wrote: On Tue, Nov 27, 2018 at 02:28:30PM -0800, Jeykumar Sankaran wrote: Add display port support in DPU by creating hooks for DP encoder enumeration and encoder mode initialization. This change is based on the SDM845 Display port driver changes[1]. changes in v2: - rebase on [2] (Sean Paul) - remove unwanted error checks and switch cases (Jordan Crouse) [1] https://lwn.net/Articles/768265/ [2] https://lkml.org/lkml/2018/11/17/87 Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 47 + 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index d3f4501..1f6b4b1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2015,7 +2015,7 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, { int ret = 0; int i = 0; - enum dpu_intf_type intf_type; + enum dpu_intf_type intf_type = INTF_NONE; dpu_intf_type seems unnecessary, you could just use the DRM_MODE_ENCODER_* value directly? struct dpu_enc_phys_init_params phys_params; if (!dpu_enc || !dpu_kms) { @@ -2038,9 +2038,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, case DRM_MODE_ENCODER_DSI: intf_type = INTF_DSI; break; - default: - DPU_ERROR_ENC(dpu_enc, "unsupported display interface type\n"); - return -EINVAL; + case DRM_MODE_ENCODER_TMDS: + intf_type = INTF_DP; + break; } WARN_ON(disp_info->num_of_h_tiles < 1); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 985c855..7d931ae 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -473,6 +473,32 @@ static void _dpu_kms_initialize_dsi(struct drm_device *dev, } } +static void _dpu_kms_initialize_displayport(struct drm_device *dev, + struct msm_drm_private *priv, + struct dpu_kms *dpu_kms) +{ + struct drm_encoder *encoder = NULL; + int rc; + + if (!priv->dp) + return; + + encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS); + if (IS_ERR(encoder)) { + DPU_ERROR("encoder init failed for dsi display\n"); + return; + } + + rc = msm_dp_modeset_init(priv->dp, dev, encoder); + if (rc) { + DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); + drm_encoder_cleanup(encoder); + return; + } + + priv->encoders[priv->num_encoders++] = encoder; No need to keep track of drm resources at the driver level, the core will do this for you. So can you please add a patch preceding this one to remove the priv->encoders/crtc/planes/connectors arrays? priv arrays for tracking drm components and priv->num_** counters are introduced by mdp4/5. DPU just adapted the implementation. De-coupling DPU from these arrays is much easier than fixing them in mdp4/5. I see mdp5 is using it to track the max resources and also in ./msm_fbdev.c. Thanks and Regards, Jeykumar S. +} + /** * _dpu_kms_setup_displays - create encoders, bridges and connectors * for underlying displays @@ -487,6 +513,8 @@ static void _dpu_kms_setup_displays(struct drm_device *dev, Why are these functions voids? Seems like there are plenty of places for them to fail :) Let's add a patch to the beginning of this series to properly handle failures in setup_displays and initialize_dsi { _dpu_kms_initialize_dsi(dev, priv, dpu_kms); + _dpu_kms_initialize_displayport(dev, priv, dpu_kms); + /** * Extend this function to initialize other * types of displays @@ -723,13 +751,20 @@ static void _dpu_kms_set_encoder_mode(struct msm_kms *kms, info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE : MSM_DISPLAY_CAP_VID_MODE; - /* TODO: No support for DSI swap */ - for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { - if (priv->dsi[i]) { - info.h_tile_instance[info.num_of_h_tiles] = i; - info.num_of_h_tiles++; + switch (info.intf_type) { + case DRM_MODE_ENCODER_DSI: + /* TODO: No support for DSI swap */ + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { + if (priv->dsi[i]) { + info.h_tile_instance[info.num_of_h_tiles] = i; + info.num_of_h_tiles++; + }
Re: [Freedreno] [PATCH v5 1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file
On 2018-12-03 16:57, Doug Anderson wrote: Hi, On Mon, Dec 3, 2018 at 2:27 PM Jeykumar Sankaran wrote: + dsi0: dsi@ae94000 { + compatible = "qcom,mdss-dsi-ctrl"; + reg = <0xae94000 0x400>; + reg-names = "dsi_ctrl"; + + interrupt-parent = <&mdss>; + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>, +<&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>, +<&dispcc DISP_CC_MDSS_PCLK0_CLK>, +<&dispcc DISP_CC_MDSS_ESC0_CLK>, +<&dispcc DISP_CC_MDSS_AHB_CLK>, +<&dispcc DISP_CC_MDSS_AXI_CLK>; + clock-names = "byte", + "byte_intf", + "pixel", + "core", + "iface", + "bus"; + + phys = <&dsi0_phy>; + phy-names = "dsi0"; I'm pretty sure that this should just be "dsi" and the one below in dsi1 should also be called "dsi". +Jordan should confirm. Makes sense. I can fix that! + dsi1: dsi@ae96000 { + compatible = "qcom,mdss-dsi-ctrl"; + reg = <0xae96000 0x400>; + reg-names = "dsi_ctrl"; + + interrupt-parent = <&mdss>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&dispcc DISP_CC_MDSS_BYTE1_CLK>, +<&dispcc DISP_CC_MDSS_BYTE1_INTF_CLK>, +<&dispcc DISP_CC_MDSS_PCLK1_CLK>, +<&dispcc DISP_CC_MDSS_ESC1_CLK>, +<&dispcc DISP_CC_MDSS_AHB_CLK>, +<&dispcc DISP_CC_MDSS_AXI_CLK>; + clock-names = "byte", + "byte_intf", + "pixel", + "core", + "iface", + "bus"; + + phys = <&dsi1_phy>; + phy-names = "dsi1"; + + status = "disabled"; This "disabled" is causing me problems. I don't actually need "dsi1" but if I don't enable "dsi1" then my display doesn't come up. :( I ran out of time to debug but I wonder if this is this the standard thing where DRM needs to wait for all the components to probe until it can finish? If nobody on this list just knows I'll dig tomorrow and confirm that my memory isn't faulty and see what we've done about this in the past. https://patchwork.kernel.org/patch/10467895/ Can you try out with this change (reviewed but not merged yet). It validates the nodes before adding to the DSI list. One last note: it's pretty weird that you sent out only 1/3 and not 2/3 and 3/3. If you're not ready to send out MTP stuff yet then you should send out v6 as just a singleton patch. Yes. I was trying to separate this one out as an independent change. Sandeep is working on the comments on removing the pinctrl nodes and updated mtp nodes. He should be posting 2/3 and 3/3 in the next couple of days. Thanks, Jeykumar S. -Doug -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5 1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file
Hi, On Mon, Dec 3, 2018 at 2:27 PM Jeykumar Sankaran wrote: > + dsi0: dsi@ae94000 { > + compatible = "qcom,mdss-dsi-ctrl"; > + reg = <0xae94000 0x400>; > + reg-names = "dsi_ctrl"; > + > + interrupt-parent = <&mdss>; > + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; > + > + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>, > +<&dispcc > DISP_CC_MDSS_BYTE0_INTF_CLK>, > +<&dispcc DISP_CC_MDSS_PCLK0_CLK>, > +<&dispcc DISP_CC_MDSS_ESC0_CLK>, > +<&dispcc DISP_CC_MDSS_AHB_CLK>, > +<&dispcc DISP_CC_MDSS_AXI_CLK>; > + clock-names = "byte", > + "byte_intf", > + "pixel", > + "core", > + "iface", > + "bus"; > + > + phys = <&dsi0_phy>; > + phy-names = "dsi0"; I'm pretty sure that this should just be "dsi" and the one below in dsi1 should also be called "dsi". +Jordan should confirm. > + dsi1: dsi@ae96000 { > + compatible = "qcom,mdss-dsi-ctrl"; > + reg = <0xae96000 0x400>; > + reg-names = "dsi_ctrl"; > + > + interrupt-parent = <&mdss>; > + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; > + > + clocks = <&dispcc DISP_CC_MDSS_BYTE1_CLK>, > +<&dispcc > DISP_CC_MDSS_BYTE1_INTF_CLK>, > +<&dispcc DISP_CC_MDSS_PCLK1_CLK>, > +<&dispcc DISP_CC_MDSS_ESC1_CLK>, > +<&dispcc DISP_CC_MDSS_AHB_CLK>, > +<&dispcc DISP_CC_MDSS_AXI_CLK>; > + clock-names = "byte", > + "byte_intf", > + "pixel", > + "core", > + "iface", > + "bus"; > + > + phys = <&dsi1_phy>; > + phy-names = "dsi1"; > + > + status = "disabled"; This "disabled" is causing me problems. I don't actually need "dsi1" but if I don't enable "dsi1" then my display doesn't come up. :( I ran out of time to debug but I wonder if this is this the standard thing where DRM needs to wait for all the components to probe until it can finish? If nobody on this list just knows I'll dig tomorrow and confirm that my memory isn't faulty and see what we've done about this in the past. One last note: it's pretty weird that you sent out only 1/3 and not 2/3 and 3/3. If you're not ready to send out MTP stuff yet then you should send out v6 as just a singleton patch. -Doug ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v18 1/5] iommu/arm-smmu: Add pm_runtime/sleep ops
Quoting Vivek Gautam (2018-12-02 22:43:38) > On Fri, Nov 30, 2018 at 11:45 PM Will Deacon wrote: > > > > On Thu, Nov 29, 2018 at 08:25:20PM +0530, Vivek Gautam wrote: > > > clk_bulk_get_all() seems like going only the OF way. > > > Is there another way here to have something common between ACPI > > > and OF, and then do the clk_bulk_get? > > > > I'd say just go with clk_bulk_get_all() and if somebody really wants to > > mess with the SMMU clocks on a system booted via ACPI, then it's their > > problem to solve. My understanding is that the design of IORT makes this > > next to impossible to solve anyway, because a static table is used and > > therefore we're unable to run whatever ASL methods need to be invoked to > > mess with the clocks. > > Sure then. I will respin this patch-series. > Right. The idea is to add non-OF support to clk_bulk_get_all() if/when we get the requirement. Sounds like we can keep waiting a little longer for that to happen. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
On Mon, Dec 3, 2018 at 8:24 PM Jonathan marek wrote: > > I can't find the dt-bindings for these compatible entries. Have you > > documented them? > > > > It is the same as qcom,adreno which is documented here: > > Documentation/devicetree/bindings/display/msm/gpu.txt > > I guess I should add amd,imageon there. Yes, please. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] gpu/drm: remove DEFINE_DPU_DEBUGFS_SEQ_FOPS()
We already have the DEFINE_SHOW_ATTRIBUTE.There is no need to define such a macro separately,so remove DEFINE_DPU_DEBUGFS_SEQ_FOPS. Also use DEFINE_SHOW_ATTRIBUTE to simplify some code. Signed-off-by: Yangtao Li --- drivers/gpu/drm/armada/armada_debugfs.c | 21 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 14 +--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 33 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 36 drivers/gpu/host1x/debug.c | 34 -- 6 files changed, 29 insertions(+), 126 deletions(-) diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 6758c3a83de2..64dccb7c8be2 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -28,7 +28,7 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data) return 0; } -static int armada_debugfs_reg_show(struct seq_file *m, void *data) +static int reg_show(struct seq_file *m, void *data) { struct drm_device *dev = m->private; struct armada_private *priv = dev->dev_private; @@ -50,18 +50,7 @@ static int armada_debugfs_reg_show(struct seq_file *m, void *data) return 0; } -static int armada_debugfs_reg_r_open(struct inode *inode, struct file *file) -{ - return single_open(file, armada_debugfs_reg_show, inode->i_private); -} - -static const struct file_operations fops_reg_r = { - .owner = THIS_MODULE, - .open = armada_debugfs_reg_r_open, - .read = seq_read, - .llseek = seq_lseek, - .release = single_release, -}; +DEFINE_SHOW_ATTRIBUTE(reg); static int armada_debugfs_write(struct file *file, const char __user *ptr, size_t len, loff_t *off) @@ -119,12 +108,14 @@ int armada_drm_debugfs_init(struct drm_minor *minor) return ret; de = debugfs_create_file("reg", S_IFREG | S_IRUSR, -minor->debugfs_root, minor->dev, &fops_reg_r); +minor->debugfs_root, minor->dev, +®_fops); if (!de) return -ENOMEM; de = debugfs_create_file("reg_wr", S_IFREG | S_IWUSR, -minor->debugfs_root, minor->dev, &fops_reg_w); +minor->debugfs_root, minor->dev, +&fops_reg_w); if (!de) return -ENOMEM; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c index 879c13fe74e0..7607aac9449c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c @@ -299,18 +299,6 @@ static void dpu_disable_all_irqs(struct dpu_kms *dpu_kms) } #ifdef CONFIG_DEBUG_FS -#define DEFINE_DPU_DEBUGFS_SEQ_FOPS(__prefix) \ -static int __prefix ## _open(struct inode *inode, struct file *file) \ -{ \ - return single_open(file, __prefix ## _show, inode->i_private); \ -} \ -static const struct file_operations __prefix ## _fops = { \ - .owner = THIS_MODULE, \ - .open = __prefix ## _open, \ - .release = single_release, \ - .read = seq_read, \ - .llseek = seq_lseek,\ -} static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) { @@ -341,7 +329,7 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) return 0; } -DEFINE_DPU_DEBUGFS_SEQ_FOPS(dpu_debugfs_core_irq); +DEFINE_SHOW_ATTRIBUTE(dpu_debugfs_core_irq); int dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, struct dentry *parent) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index d4530d60767b..e705bad980ee 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1309,7 +1309,7 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) } #ifdef CONFIG_DEBUG_FS -static int _dpu_debugfs_status_show(struct seq_file *s, void *data) +static int status_show(struct seq_file *s, void *data) { struct dpu_crtc *dpu_crtc; struct dpu_plane_state *pstate = NULL; @@ -1428,24 +1428,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) return 0; } -static int _dpu_debugfs_status_open(struct inode *inode, struct file *file) -{ - return single_open(file, _dpu_debugfs_status_show, inode->i_private); -} - -#define DEFINE_DPU_DEBUGFS_SEQ_FOPS(__prefix)
Re: [Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
Hi Jonathan, Thanks for working on this. Really nice to see GPU support for mx51/mx53! On Mon, Dec 3, 2018 at 7:21 PM Jonathan Marek wrote: Please add a commit log. > Signed-off-by: Jonathan Marek > --- > arch/arm/boot/dts/imx51.dtsi | 17 + > arch/arm/boot/dts/imx53.dtsi | 17 + > 2 files changed, 34 insertions(+) > > diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi > index 67d462715..e9a7bbce9 100644 > --- a/arch/arm/boot/dts/imx51.dtsi > +++ b/arch/arm/boot/dts/imx51.dtsi > @@ -628,5 +628,22 @@ > clock-names = "ipg", "ahb"; > }; > }; > + > + gpu: gpu@3000 { We put the peripheral nodes in address order, so this one should go prior to the IPU node. > + compatible = "amd,imageon-200.1", "amd,imageon"; I can't find the dt-bindings for these compatible entries. Have you documented them? > + reg = <0x3000 0x2>; > + reg-names = "kgsl_3d0_reg_memory"; > + interrupts = <12>; > + interrupt-names = "kgsl_3d0_irq"; > + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks > IMX5_CLK_GARB_GATE>; > + clock-names = "core_clk", "mem_iface_clk"; > + > + qcom,gpu-pwrlevels { > + compatible = "qcom,gpu-pwrlevels"; > + qcom,gpu-pwrlevel@0 { You could drop this @0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 01/10] drm/msm/dpu: Remove dpu_dbg
The functions in dpu_dbg.c aren't used. The two main dump functions fail after a lookup from dpu_dbg_base.reg_base_list which turns out to never be populated and once those are removed the rest of the file doesn't make any sense. v3: No changes v2: Moved some unrelated changes to another patch Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/Makefile |3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 2393 - drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h | 103 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |4 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |1 - .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c|1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c |1 - .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c|3 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 20 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |1 - 15 files changed, 4 insertions(+), 2531 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 7d02ef3655b5..b3c45130a5e2 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -91,8 +91,7 @@ msm-y := \ msm_ringbuffer.o \ msm_submitqueue.o -msm-$(CONFIG_DEBUG_FS) += adreno/a5xx_debugfs.o \ - disp/dpu1/dpu_dbg.o +msm-$(CONFIG_DEBUG_FS) += adreno/a5xx_debugfs.o msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o msm-$(CONFIG_COMMON_CLK) += disp/mdp4/mdp4_lvds_pll.o diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c deleted file mode 100644 index a85078123119.. --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c +++ /dev/null @@ -1,2393 +0,0 @@ -/* Copyright (c) 2009-2018, The Linux Foundation. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#define pr_fmt(fmt)"[drm:%s:%d] " fmt, __func__, __LINE__ - -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include "dpu_dbg.h" -#include "disp/dpu1/dpu_hw_catalog.h" - - -#define DEFAULT_DBGBUS_DPU DPU_DBG_DUMP_IN_MEM -#define DEFAULT_DBGBUS_VBIFRT DPU_DBG_DUMP_IN_MEM -#define REG_BASE_NAME_LEN 80 - -#define DBGBUS_FLAGS_DSPP BIT(0) -#define DBGBUS_DSPP_STATUS 0x34C - -#define DBGBUS_NAME_DPU"dpu" -#define DBGBUS_NAME_VBIF_RT"vbif_rt" - -/* offsets from dpu top address for the debug buses */ -#define DBGBUS_SSPP0 0x188 -#define DBGBUS_AXI_INTF0x194 -#define DBGBUS_SSPP1 0x298 -#define DBGBUS_DSPP0x348 -#define DBGBUS_PERIPH 0x418 - -#define TEST_MASK(id, tp) ((id << 4) | (tp << 1) | BIT(0)) - -/* following offsets are with respect to MDP VBIF base for DBG BUS access */ -#define MMSS_VBIF_CLKON0x4 -#define MMSS_VBIF_TEST_BUS_OUT_CTRL0x210 -#define MMSS_VBIF_TEST_BUS_OUT 0x230 - -/* Vbif error info */ -#define MMSS_VBIF_PND_ERR 0x190 -#define MMSS_VBIF_SRC_ERR 0x194 -#define MMSS_VBIF_XIN_HALT_CTRL1 0x204 -#define MMSS_VBIF_ERR_INFO 0X1a0 -#define MMSS_VBIF_ERR_INFO_1 0x1a4 -#define MMSS_VBIF_CLIENT_NUM 14 - -/** - * struct dpu_dbg_reg_base - register region base. - * may sub-ranges: sub-ranges are used for dumping - * or may not have sub-ranges: dumping is base -> max_offset - * @reg_base_head: head of this node - * @name: register base name - * @base: base pointer - * @off: cached offset of region for manual register dumping - * @cnt: cached range of region for manual register dumping - * @max_offset: length of region - * @buf: buffer used for manual register dumping - * @buf_len: buffer length used for manual register dumping - * @cb: callback for external dump function, null if not defined - * @cb_ptr: private pointer to callback function - */ -struct dpu_dbg_reg_base { - struct list_head reg_base_head; - char name[REG_BASE_NAME_LEN]; - void __iomem *base; - size_t off; - size_t cnt; - size_t max_offset; - char *buf; - size_t buf_len; - void (*cb)(void *ptr); - void *cb_ptr; -}; - -s
[Freedreno] [PATCH v3 10/10] drm/msm/dpu: Clean up dpu_media_info.h static inline functions
Do some cleanup in the static inline functions defined in dpu_media_info.h by cleaning up gotos and unneeded local variables. v3: Added spaces between operators per Seal Paul and Sam Ravnborg Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- .../gpu/drm/msm/disp/dpu1/msm_media_info.h| 164 ++ 1 file changed, 57 insertions(+), 107 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/msm_media_info.h b/drivers/gpu/drm/msm/disp/dpu1/msm_media_info.h index 75470ee5b18f..9fc9dbde8a27 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/msm_media_info.h +++ b/drivers/gpu/drm/msm/disp/dpu1/msm_media_info.h @@ -822,36 +822,30 @@ enum color_fmts { */ static unsigned int VENUS_Y_STRIDE(int color_fmt, int width) { - unsigned int alignment, stride = 0; + unsigned int stride = 0; if (!width) - goto invalid_input; + return 0; switch (color_fmt) { case COLOR_FMT_NV21: case COLOR_FMT_NV12: case COLOR_FMT_NV12_MVTB: case COLOR_FMT_NV12_UBWC: - alignment = 128; - stride = MSM_MEDIA_ALIGN(width, alignment); + stride = MSM_MEDIA_ALIGN(width, 128); break; case COLOR_FMT_NV12_BPP10_UBWC: - alignment = 256; stride = MSM_MEDIA_ALIGN(width, 192); - stride = MSM_MEDIA_ALIGN(stride * 4/3, alignment); + stride = MSM_MEDIA_ALIGN(stride * 4 / 3, 256); break; case COLOR_FMT_P010_UBWC: - alignment = 256; - stride = MSM_MEDIA_ALIGN(width * 2, alignment); + stride = MSM_MEDIA_ALIGN(width * 2, 256); break; case COLOR_FMT_P010: - alignment = 128; - stride = MSM_MEDIA_ALIGN(width*2, alignment); - break; - default: + stride = MSM_MEDIA_ALIGN(width * 2, 128); break; } -invalid_input: + return stride; } @@ -864,36 +858,30 @@ static unsigned int VENUS_Y_STRIDE(int color_fmt, int width) */ static unsigned int VENUS_UV_STRIDE(int color_fmt, int width) { - unsigned int alignment, stride = 0; + unsigned int stride = 0; if (!width) - goto invalid_input; + return 0; switch (color_fmt) { case COLOR_FMT_NV21: case COLOR_FMT_NV12: case COLOR_FMT_NV12_MVTB: case COLOR_FMT_NV12_UBWC: - alignment = 128; - stride = MSM_MEDIA_ALIGN(width, alignment); + stride = MSM_MEDIA_ALIGN(width, 128); break; case COLOR_FMT_NV12_BPP10_UBWC: - alignment = 256; stride = MSM_MEDIA_ALIGN(width, 192); - stride = MSM_MEDIA_ALIGN(stride * 4/3, alignment); + stride = MSM_MEDIA_ALIGN(stride * 4 / 3, 256); break; case COLOR_FMT_P010_UBWC: - alignment = 256; - stride = MSM_MEDIA_ALIGN(width * 2, alignment); + stride = MSM_MEDIA_ALIGN(width * 2, 256); break; case COLOR_FMT_P010: - alignment = 128; - stride = MSM_MEDIA_ALIGN(width*2, alignment); - break; - default: + stride = MSM_MEDIA_ALIGN(width * 2, 128); break; } -invalid_input: + return stride; } @@ -906,10 +894,10 @@ static unsigned int VENUS_UV_STRIDE(int color_fmt, int width) */ static unsigned int VENUS_Y_SCANLINES(int color_fmt, int height) { - unsigned int alignment, sclines = 0; + unsigned int sclines = 0; if (!height) - goto invalid_input; + return 0; switch (color_fmt) { case COLOR_FMT_NV21: @@ -917,17 +905,14 @@ static unsigned int VENUS_Y_SCANLINES(int color_fmt, int height) case COLOR_FMT_NV12_MVTB: case COLOR_FMT_NV12_UBWC: case COLOR_FMT_P010: - alignment = 32; + sclines = MSM_MEDIA_ALIGN(height, 32); break; case COLOR_FMT_NV12_BPP10_UBWC: case COLOR_FMT_P010_UBWC: - alignment = 16; + sclines = MSM_MEDIA_ALIGN(height, 16); break; - default: - return 0; } - sclines = MSM_MEDIA_ALIGN(height, alignment); -invalid_input: + return sclines; } @@ -940,10 +925,10 @@ static unsigned int VENUS_Y_SCANLINES(int color_fmt, int height) */ static unsigned int VENUS_UV_SCANLINES(int color_fmt, int height) { - unsigned int alignment, sclines = 0; + unsigned int sclines = 0; if (!height) - goto invalid_input; + return 0; switch (color_fmt) { case COLOR_FMT_NV21: @@ -952,18 +937,13 @@ static unsigned int VENUS_UV_SCANLINES(int color_fmt, int height) case COLOR_FMT_NV12_BPP10_UBWC:
[Freedreno] [PATCH v3 06/10] drm/msm: Make irq_postinstall optional
Allow the KMS operation 'irq_postinstall' to be optional so that the target display drivers don't need to define a dummy function if they don't need one. v3: No changes Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/msm_drv.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 9c9f7ff6960b..d8af97407b3c 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -724,7 +724,11 @@ 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); - return kms->funcs->irq_postinstall(kms); + + if (kms->funcs->irq_postinstall) + return kms->funcs->irq_postinstall(kms); + + return 0; } static void msm_irq_uninstall(struct drm_device *dev) -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 04/10] drm/msm/dpu: Remove unused functions
Remove some unused container_of() helper functions. v3: No changes v2: Retained still used helper functions in the name of readability Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 10 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 10 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 10 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 10 -- 4 files changed, 40 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h index 3b77df460dea..a2b0dbc23058 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h @@ -91,16 +91,6 @@ struct dpu_hw_intf { struct dpu_hw_intf_ops ops; }; -/** - * to_dpu_hw_intf - convert base object dpu_hw_base to container - * @hw: Pointer to base hardware block - * return: Pointer to hardware block container - */ -static inline struct dpu_hw_intf *to_dpu_hw_intf(struct dpu_hw_blk *hw) -{ - return container_of(hw, struct dpu_hw_intf, base); -} - /** * dpu_hw_intf_init(): Initializes the intf driver for the passed * interface idx. diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h index 3caccd7d6a3e..0e02e43cee14 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h @@ -104,16 +104,6 @@ struct dpu_hw_pingpong { struct dpu_hw_pingpong_ops ops; }; -/** - * dpu_hw_pingpong - convert base object dpu_hw_base to container - * @hw: Pointer to base hardware block - * return: Pointer to hardware block container - */ -static inline struct dpu_hw_pingpong *to_dpu_hw_pingpong(struct dpu_hw_blk *hw) -{ - return container_of(hw, struct dpu_hw_pingpong, base); -} - /** * dpu_hw_pingpong_init - initializes the pingpong driver for the passed * pingpong idx. diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h index 4d81e5f5ce1b..119b4e1c16be 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h @@ -391,16 +391,6 @@ struct dpu_hw_pipe { struct dpu_hw_sspp_ops ops; }; -/** - * dpu_hw_pipe - convert base object dpu_hw_base to container - * @hw: Pointer to base hardware block - * return: Pointer to hardware block container - */ -static inline struct dpu_hw_pipe *to_dpu_hw_pipe(struct dpu_hw_blk *hw) -{ - return container_of(hw, struct dpu_hw_pipe, base); -} - /** * dpu_hw_sspp_init - initializes the sspp hw driver object. * Should be called once before accessing every pipe. diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h index 192e338f20bb..aa21fd834398 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h @@ -160,16 +160,6 @@ struct dpu_hw_mdp { struct dpu_hw_mdp_ops ops; }; -/** - * to_dpu_hw_mdp - convert base object dpu_hw_base to container - * @hw: Pointer to base hardware block - * return: Pointer to hardware block container - */ -static inline struct dpu_hw_mdp *to_dpu_hw_mdp(struct dpu_hw_blk *hw) -{ - return container_of(hw, struct dpu_hw_mdp, base); -} - /** * dpu_hw_mdptop_init - initializes the top driver for the passed idx * @idx: Interface index for which driver object is required -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 00/10] drm/msm/dpu: cleanups
This is a rebase of https://patchwork.freedesktop.org/series/51214/ On top of https://gitlab.freedesktop.org/seanpaul/dpu-staging/tags/for_jcrouse Which should be up to date for the latest and greatest of the DPU. This should be mostly unchanged since the last revision with the exception that I dropped ("drm/msm/dpu: Use DEFINE_SHOW_ATTRIBUTE") in favor of https://patchwork.freedesktop.org/patch/265159/ Which does the same thing but is a little bit more tree-wide in its efforts. Jordan Crouse (10): drm/msm/dpu: Remove dpu_dbg drm/msm/dpu: Remove dpu_crtc_get_mixer_height drm/msm/dpu: Remove dpu_crtc_is_enabled() drm/msm/dpu: Remove unused functions drm/msm/dpu: Cleanup callers of dpu_hw_blk_init drm/msm: Make irq_postinstall optional drm/msm/dpu: Remove dpu_irq and unused functions drm/msm/dpu: Cleanup the debugfs functions drm/msm/dpu: Further cleanups for static inline functions drm/msm/dpu: Clean up dpu_media_info.h static inline functions drivers/gpu/drm/msm/Makefile |4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 45 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h | 16 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 122 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 45 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 32 - drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 2393 - drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h | 103 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 +- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |2 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 12 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c| 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h|2 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h|9 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 10 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 24 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h |5 - .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 18 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 10 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 21 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 10 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 20 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h| 10 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c |1 - drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c | 66 - drivers/gpu/drm/msm/disp/dpu1/dpu_irq.h | 59 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 154 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |8 - drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c |8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 108 +- drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 24 +- drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h | 15 +- .../gpu/drm/msm/disp/dpu1/msm_media_info.h| 164 +- drivers/gpu/drm/msm/msm_drv.c |6 +- 38 files changed, 221 insertions(+), 3394 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.h -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 05/10] drm/msm/dpu: Cleanup callers of dpu_hw_blk_init
Outside of superfluous parameter checks the dpu_hw_blk_init() doesn't have any failure paths. Switch it over to be a void function and we can remove error handling paths in all the functions that call it. While we're in those functions remove unneeded initialization for a static variable. v3: No changes v2: Removed a cleanup intended for a different patch Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c | 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 17 ++--- 8 files changed, 14 insertions(+), 100 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c index 58d29e43faef..92f1c4241b9a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.c @@ -30,16 +30,10 @@ static LIST_HEAD(dpu_hw_blk_list); * @type: hw block type - enum dpu_hw_blk_type * @id: instance id of the hw block * @ops: Pointer to block operations - * return: 0 if success; error code otherwise */ -int dpu_hw_blk_init(struct dpu_hw_blk *hw_blk, u32 type, int id, +void dpu_hw_blk_init(struct dpu_hw_blk *hw_blk, u32 type, int id, struct dpu_hw_blk_ops *ops) { - if (!hw_blk) { - pr_err("invalid parameters\n"); - return -EINVAL; - } - INIT_LIST_HEAD(&hw_blk->list); hw_blk->type = type; hw_blk->id = id; @@ -51,8 +45,6 @@ int dpu_hw_blk_init(struct dpu_hw_blk *hw_blk, u32 type, int id, mutex_lock(&dpu_hw_blk_lock); list_add(&hw_blk->list, &dpu_hw_blk_list); mutex_unlock(&dpu_hw_blk_lock); - - return 0; } /** diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h index 0f4ca8af1ec5..1934c2f7e8fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_blk.h @@ -44,7 +44,7 @@ struct dpu_hw_blk { struct dpu_hw_blk_ops ops; }; -int dpu_hw_blk_init(struct dpu_hw_blk *hw_blk, u32 type, int id, +void dpu_hw_blk_init(struct dpu_hw_blk *hw_blk, u32 type, int id, struct dpu_hw_blk_ops *ops); void dpu_hw_blk_destroy(struct dpu_hw_blk *hw_blk); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c index 4aab04335c6d..1068b4b7940f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -483,10 +483,7 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, ops->get_bitmask_intf = dpu_hw_ctl_get_bitmask_intf; }; -static struct dpu_hw_blk_ops dpu_hw_ops = { - .start = NULL, - .stop = NULL, -}; +static struct dpu_hw_blk_ops dpu_hw_ops; struct dpu_hw_ctl *dpu_hw_ctl_init(enum dpu_ctl idx, void __iomem *addr, @@ -494,7 +491,6 @@ struct dpu_hw_ctl *dpu_hw_ctl_init(enum dpu_ctl idx, { struct dpu_hw_ctl *c; struct dpu_ctl_cfg *cfg; - int rc; c = kzalloc(sizeof(*c), GFP_KERNEL); if (!c) @@ -513,18 +509,9 @@ struct dpu_hw_ctl *dpu_hw_ctl_init(enum dpu_ctl idx, c->mixer_count = m->mixer_count; c->mixer_hw_caps = m->mixer; - rc = dpu_hw_blk_init(&c->base, DPU_HW_BLK_CTL, idx, &dpu_hw_ops); - if (rc) { - DPU_ERROR("failed to init hw blk %d\n", rc); - goto blk_init_error; - } + dpu_hw_blk_init(&c->base, DPU_HW_BLK_CTL, idx, &dpu_hw_ops); return c; - -blk_init_error: - kzfree(c); - - return ERR_PTR(rc); } void dpu_hw_ctl_destroy(struct dpu_hw_ctl *ctx) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 695d27a730e8..f6a83daa385b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -264,10 +264,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, ops->get_line_count = dpu_hw_intf_get_line_count; } -static struct dpu_hw_blk_ops dpu_hw_ops = { - .start = NULL, - .stop = NULL, -}; +static struct dpu_hw_blk_ops dpu_hw_ops; struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx, void __iomem *addr, @@ -275,7 +272,6 @@ struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx, { struct dpu_hw_intf *c; struct dpu_intf_cfg *cfg; - int rc; c = kzalloc(sizeof(*c), GFP_KERNEL); if (!c) @@ -296,18 +292,9 @@ struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx, c-
[Freedreno] [PATCH v3 09/10] drm/msm/dpu: Further cleanups for static inline functions
Remove more static inline functions that are lightly used and/or very simple and easy to build into the calling functions. v3: Fix a nit from Sean Paul v2: Removed another unused function from dpu_hw_lm.c and add back dpu_crtc_get_client_type() since there was a question regarding its usefulness. Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 10 -- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 2 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 11 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 9 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 5 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c| 3 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 8 +--- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 -- 10 files changed, 13 insertions(+), 71 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 306e3df6fd63..f9a7479b8f1c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -46,12 +46,6 @@ #define LEFT_MIXER 0 #define RIGHT_MIXER 1 -static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate, - struct drm_display_mode *mode) -{ - return mode->hdisplay / cstate->num_mixers; -} - static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) { struct msm_drm_private *priv = crtc->dev->dev_private; @@ -497,7 +491,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, { struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); struct drm_display_mode *adj_mode = &state->adjusted_mode; - u32 crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode); + u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers; int i; for (i = 0; i < cstate->num_mixers; i++) { @@ -940,7 +934,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, memset(pipe_staged, 0, sizeof(pipe_staged)); - mixer_width = _dpu_crtc_get_mixer_width(cstate, mode); + mixer_width = mode->hdisplay / cstate->num_mixers; _dpu_crtc_setup_lm_bounds(crtc, state); @@ -1181,7 +1175,7 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) cstate = to_dpu_crtc_state(crtc->state); mode = &crtc->state->adjusted_mode; - out_width = _dpu_crtc_get_mixer_width(cstate, mode); + out_width = mode->hdisplay / cstate->num_mixers; seq_printf(s, "crtc:%d width:%d height:%d\n", crtc->base.id, mode->hdisplay, mode->vdisplay); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 94f5cea4e0d2..dbfb38a1986c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -226,16 +226,6 @@ struct dpu_crtc_state { #define to_dpu_crtc_state(x) \ container_of(x, struct dpu_crtc_state, base) -/** - * dpu_crtc_state_is_stereo - Is crtc virtualized with two mixers? - * @cstate: Pointer to dpu crtc state - * @Return: true - has two mixers, false - has one mixer - */ -static inline bool dpu_crtc_state_is_stereo(struct dpu_crtc_state *cstate) -{ - return cstate->num_mixers == CRTC_DUAL_MIXERS; -} - /** * dpu_crtc_frame_pending - retun the number of pending frames * @crtc: Pointer to drm crtc object diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index 3a67bb9f9d9d..44e6f8b68e70 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -350,7 +350,7 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode( dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state); if (phys_enc->split_role == ENC_ROLE_SOLO && - dpu_crtc_state_is_stereo(dpu_cstate)) + dpu_cstate->num_mixers == CRTC_DUAL_MIXERS) return BLEND_3D_H_ROW_INT; return BLEND_3D_NONE; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index b37a0992e326..99ab5ca9bed3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -44,14 +44,7 @@ #define DPU_ENC_WR_PTR_START_TIMEOUT_US 2 -static inline int _dpu_encoder_phys_cmd_get_idle_timeout( - struct dpu_encoder_phys_cmd *cmd_enc) -{ - return KICKOFF_TIMEOUT_MS; -} - -static inline bool dpu_encoder_phys_cmd_is_master( - struct dpu_encoder_phys *phys_enc) +static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc) {
[Freedreno] [PATCH v3 03/10] drm/msm/dpu: Remove dpu_crtc_is_enabled()
The static inline function dpu_crtc_enabled() is only called once and the function that calls it in turn is only called once and the return value can be easily checked in the calling functions so collapse everything down. v3: No changes Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 17 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 9 - 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c index bffc51e496e7..e8a87f4b8e0e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -57,18 +57,13 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) return to_dpu_kms(priv->kms); } -static bool _dpu_core_perf_crtc_is_power_on(struct drm_crtc *crtc) -{ - return dpu_crtc_is_enabled(crtc); -} - static bool _dpu_core_video_mode_intf_connected(struct drm_crtc *crtc) { struct drm_crtc *tmp_crtc; drm_for_each_crtc(tmp_crtc, crtc->dev) { if ((dpu_crtc_get_intf_mode(tmp_crtc) == INTF_MODE_VIDEO) && - _dpu_core_perf_crtc_is_power_on(tmp_crtc)) { + tmp_crtc->enabled) { DPU_DEBUG("video interface connected crtc:%d\n", tmp_crtc->base.id); return true; @@ -164,7 +159,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc, curr_client_type = dpu_crtc_get_client_type(crtc); drm_for_each_crtc(tmp_crtc, crtc->dev) { - if (_dpu_core_perf_crtc_is_power_on(tmp_crtc) && + if (tmp_crtc->enabled && (dpu_crtc_get_client_type(tmp_crtc) == curr_client_type) && (tmp_crtc != crtc)) { @@ -223,7 +218,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms, int ret = 0; drm_for_each_crtc(tmp_crtc, crtc->dev) { - if (_dpu_core_perf_crtc_is_power_on(tmp_crtc) && + if (tmp_crtc->enabled && curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) { dpu_cstate = to_dpu_crtc_state(tmp_crtc->state); @@ -280,7 +275,7 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc) */ if (dpu_crtc_get_intf_mode(crtc) == INTF_MODE_CMD) drm_for_each_crtc(tmp_crtc, crtc->dev) { - if (_dpu_core_perf_crtc_is_power_on(tmp_crtc) && + if (tmp_crtc->enabled && dpu_crtc_get_intf_mode(tmp_crtc) == INTF_MODE_VIDEO) return; @@ -315,7 +310,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) struct dpu_crtc_state *dpu_cstate; drm_for_each_crtc(crtc, kms->dev) { - if (_dpu_core_perf_crtc_is_power_on(crtc)) { + if (crtc->enabled) { dpu_cstate = to_dpu_crtc_state(crtc->state); clk_rate = max(dpu_cstate->new_perf.core_clk_rate, clk_rate); @@ -366,7 +361,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc, old = &dpu_crtc->cur_perf; new = &dpu_cstate->new_perf; - if (_dpu_core_perf_crtc_is_power_on(crtc) && !stop_req) { + if (crtc->enabled && !stop_req) { for (i = 0; i < DPU_CORE_PERF_DATA_BUS_ID_MAX; i++) { /* * cases for bus bandwidth update. diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index b84dc5730a0e..94f5cea4e0d2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -309,13 +309,4 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type( return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT; } -/** - * dpu_crtc_is_enabled - check if dpu crtc is enabled or not - * @crtc: Pointer to crtc - */ -static inline bool dpu_crtc_is_enabled(struct drm_crtc *crtc) -{ - return crtc ? crtc->enabled : false; -} - #endif /* _DPU_CRTC_H_ */ -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 02/10] drm/msm/dpu: Remove dpu_crtc_get_mixer_height
dpu_crtc_get_mixer_height() is only used once and the value it returns can be easily derived from the calling function. v3: No changes Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 13 - 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 949517b25431..39a0a6854b6c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -495,7 +495,6 @@ static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, struct drm_crtc_state *state) { - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); struct drm_display_mode *adj_mode = &state->adjusted_mode; u32 crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode); @@ -506,7 +505,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, r->x1 = crtc_split_width * i; r->y1 = 0; r->x2 = r->x1 + crtc_split_width; - r->y2 = dpu_crtc_get_mixer_height(dpu_crtc, cstate, adj_mode); + r->y2 = adj_mode->vdisplay; trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index fc7123573891..b84dc5730a0e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -236,19 +236,6 @@ static inline bool dpu_crtc_state_is_stereo(struct dpu_crtc_state *cstate) return cstate->num_mixers == CRTC_DUAL_MIXERS; } -/** - * dpu_crtc_get_mixer_height - get the mixer height - * Mixer height will be same as panel height - */ -static inline int dpu_crtc_get_mixer_height(struct dpu_crtc *dpu_crtc, - struct dpu_crtc_state *cstate, struct drm_display_mode *mode) -{ - if (!dpu_crtc || !cstate || !mode) - return 0; - - return mode->vdisplay; -} - /** * dpu_crtc_frame_pending - retun the number of pending frames * @crtc: Pointer to drm crtc object -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 08/10] drm/msm/dpu: Cleanup the debugfs functions
Do some debugfs cleanups from across the DPU driver. The DRM destroy functions will do a recursive delete on the entire debugfs node so there is no need to store dentry pointers for the debugfs files that are persistent for the life of the driver. This also means that the destroy functions can go away too. Also, use standard API functions where applicable instead of using hand written code. v3: No changes v2: Add more code; most of the dpu debugfs files should be addressed now. Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 30 + drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h | 9 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 105 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 30 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 31 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 104 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 6 - drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 90 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 24 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h | 15 +-- 12 files changed, 93 insertions(+), 361 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c index 9d5a8d217bc6..e45c69044935 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c @@ -319,10 +319,8 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) unsigned long irq_flags; int i, irq_count, enable_count, cb_count; - if (!irq_obj || !irq_obj->enable_counts || !irq_obj->irq_cb_tbl) { - DPU_ERROR("invalid parameters\n"); + if (WARN_ON(!irq_obj->enable_counts || !irq_obj->irq_cb_tbl)) return 0; - } for (i = 0; i < irq_obj->total_irqs; i++) { spin_lock_irqsave(&irq_obj->cb_lock, irq_flags); @@ -343,31 +341,11 @@ static int dpu_debugfs_core_irq_show(struct seq_file *s, void *v) DEFINE_DPU_DEBUGFS_SEQ_FOPS(dpu_debugfs_core_irq); -int dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, - struct dentry *parent) -{ - dpu_kms->irq_obj.debugfs_file = debugfs_create_file("core_irq", 0600, - parent, &dpu_kms->irq_obj, - &dpu_debugfs_core_irq_fops); - - return 0; -} - -void dpu_debugfs_core_irq_destroy(struct dpu_kms *dpu_kms) -{ - debugfs_remove(dpu_kms->irq_obj.debugfs_file); - dpu_kms->irq_obj.debugfs_file = NULL; -} - -#else -int dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, +void dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, struct dentry *parent) { - return 0; -} - -void dpu_debugfs_core_irq_destroy(struct dpu_kms *dpu_kms) -{ + debugfs_create_file("core_irq", 0600, parent, &dpu_kms->irq_obj, + &dpu_debugfs_core_irq_fops); } #endif 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 884f77fa3eb6..e9015a2b23fe 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h @@ -132,15 +132,8 @@ int dpu_core_irq_unregister_callback( * dpu_debugfs_core_irq_init - register core irq debugfs * @dpu_kms: pointer to kms * @parent: debugfs directory root - * @Return: 0 on success */ -int dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, +void dpu_debugfs_core_irq_init(struct dpu_kms *dpu_kms, struct dentry *parent); -/** - * dpu_debugfs_core_irq_destroy - deregister core irq debugfs - * @dpu_kms: pointer to kms - */ -void dpu_debugfs_core_irq_destroy(struct dpu_kms *dpu_kms); - #endif /* __DPU_CORE_IRQ_H__ */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c index e8a87f4b8e0e..9f20f397f77d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -24,8 +24,6 @@ #include "dpu_crtc.h" #include "dpu_core_perf.h" -#define DPU_PERF_MODE_STRING_SIZE 128 - /** * enum dpu_perf_mode - performance tuning mode * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client @@ -451,24 +449,14 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file, struct dpu_core_perf *perf = file->private_data; struct dpu_perf_cfg *cfg = &perf->catalog->perf; u32 perf_mode = 0; - char buf[10]; - - if (!perf) - return -ENODEV; - - if (count >= sizeof(buf)) - return -EFAULT; - - if (copy_from_user(buf, user_buf, count)) - return -EFAULT; - - buf[count] = 0; /* end of string */ + int ret; - if (kstrtouint(buf, 0, &perf_mode)) - return -EFAULT; +
[Freedreno] [PATCH v3 07/10] drm/msm/dpu: Remove dpu_irq and unused functions
dpu_irq.c does some unneeded checks and passes control to dpu_core_irq.c The simple functions can be defined in the same file where we use them and the files and their associated hangers on can be deleted. Additionally the postinstall hook isn't used even in dpu_core_irq.c so zap that entire path. v3: No changes Reviewed-by: Sean Paul Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c | 15 + drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h | 7 --- drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c | 66 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.h | 59 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 5 ++ 8 files changed, 28 insertions(+), 148 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.h diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index b3c45130a5e2..ae70fca6ee3b 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -68,7 +68,6 @@ msm-y := \ disp/dpu1/dpu_hw_util.o \ disp/dpu1/dpu_hw_vbif.o \ disp/dpu1/dpu_io_util.o \ - disp/dpu1/dpu_irq.o \ disp/dpu1/dpu_kms.o \ disp/dpu1/dpu_mdss.o \ disp/dpu1/dpu_plane.o \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c index 879c13fe74e0..9d5a8d217bc6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c @@ -376,10 +376,7 @@ void dpu_core_irq_preinstall(struct dpu_kms *dpu_kms) struct msm_drm_private *priv; int i; - if (!dpu_kms) { - DPU_ERROR("invalid dpu_kms\n"); - return; - } else if (!dpu_kms->dev) { + if (!dpu_kms->dev) { DPU_ERROR("invalid drm device\n"); return; } else if (!dpu_kms->dev->dev_private) { @@ -410,20 +407,12 @@ void dpu_core_irq_preinstall(struct dpu_kms *dpu_kms) } } -int dpu_core_irq_postinstall(struct dpu_kms *dpu_kms) -{ - return 0; -} - void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms) { struct msm_drm_private *priv; int i; - if (!dpu_kms) { - DPU_ERROR("invalid dpu_kms\n"); - return; - } else if (!dpu_kms->dev) { + if (!dpu_kms->dev) { DPU_ERROR("invalid drm device\n"); return; } else if (!dpu_kms->dev->dev_private) { 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 5e98bba46af5..884f77fa3eb6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h @@ -23,13 +23,6 @@ */ void dpu_core_irq_preinstall(struct dpu_kms *dpu_kms); -/** - * dpu_core_irq_postinstall - perform post-installation of core IRQ handler - * @dpu_kms: DPU handle - * @return:0 if success; error code otherwise - */ -int dpu_core_irq_postinstall(struct dpu_kms *dpu_kms); - /** * dpu_core_irq_uninstall - uninstall core IRQ handler * @dpu_kms: DPU handle diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c deleted file mode 100644 index d5e6ce0140cf.. --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c +++ /dev/null @@ -1,66 +0,0 @@ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#define pr_fmt(fmt)"[drm:%s:%d] " fmt, __func__, __LINE__ - -#include -#include -#include - -#include "dpu_irq.h" -#include "dpu_core_irq.h" - -irqreturn_t dpu_irq(struct msm_kms *kms) -{ - struct dpu_kms *dpu_kms = to_dpu_kms(kms); - - return dpu_core_irq(dpu_kms); -} - -void dpu_irq_preinstall(struct msm_kms *kms) -{ - struct dpu_kms *dpu_kms = to_dpu_kms(kms); - - if (!dpu_kms->dev || !dpu_kms->dev->dev) { - pr_err("invalid device handles\n"); - return; - } - - dpu_core_irq_preinstall(dpu_kms); -} - -int dpu_irq_postinstall(struct msm_kms *kms) -{ - struct dpu_kms *dpu_kms = to_dpu_kms(kms); - int rc; - - if (!kms) { - DPU_ERROR("invalid parameters\n"); - return -EINVAL; - } - - rc = dpu_core_irq_postinstall(dpu_kms); - -
[Freedreno] [PATCH v5 1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file
DPU is short for the Display Processing Unit. It is the display controller on Qualcomm SDM845 chips. This change adds MDSS and DSI nodes to enable display on the target device. Changes in v2: - Beefed up commit message - Use SoC specific compatibles for mdss and dpu (Rob H) - Use assigned-clocks to set initial clock frequency(Rob H) Changes in v3: - added IOMMU node - Fix device naming (remove _phys) - Use correct IRQ_TYPE in interrupt specifiers Changes in v4: - move mdss node to preserve the unit address sort order - remove _clk suffix from dsi clocks (both the comments are from Doug Anderson) Changes in v5: - Keep the device status "disabled" by default (Bjorn Andersson) - Use MDSS_GDSC macro (Jordan) - Fix phy-names (Jordan) - List reg ranges in numerical order (Jordan) Signed-off-by: Jeykumar Sankaran Signed-off-by: Sean Paul --- arch/arm64/boot/dts/qcom/sdm845.dtsi | 203 +++ 1 file changed, 203 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index 1419b00..a16f1fe 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -1256,6 +1256,209 @@ }; }; + mdss: mdss@ae0 { + compatible = "qcom,sdm845-mdss"; + reg = <0xae0 0x1000>; + reg-names = "mdss"; + + power-domains = <&dispcc MDSS_GDSC>; + + clocks = <&gcc GCC_DISP_AHB_CLK>, +<&gcc GCC_DISP_AXI_CLK>, +<&dispcc DISP_CC_MDSS_MDP_CLK>; + clock-names = "iface", "bus", "core"; + + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; + assigned-clock-rates = <3>; + + interrupts = ; + interrupt-controller; + #interrupt-cells = <1>; + + iommus = <&apps_smmu 0x880 0x8>, +<&apps_smmu 0xc80 0x8>; + + status = "disabled"; + + #address-cells = <1>; + #size-cells = <1>; + ranges; + + mdss_mdp: mdp@ae01000 { + compatible = "qcom,sdm845-dpu"; + reg = <0x0ae01000 0x8f000>, + <0x0aeb 0x2008>; + reg-names = "mdp", "vbif"; + + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, +<&dispcc DISP_CC_MDSS_AXI_CLK>, +<&dispcc DISP_CC_MDSS_MDP_CLK>, +<&dispcc DISP_CC_MDSS_VSYNC_CLK>; + clock-names = "iface", "bus", "core", "vsync"; + + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>, + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; + assigned-clock-rates = <3>, + <1920>; + + interrupt-parent = <&mdss>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; + + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + dpu_intf1_out: endpoint { + remote-endpoint = <&dsi0_in>; + }; + }; + + port@1 { + reg = <1>; + dpu_intf2_out: endpoint { + remote-endpoint = <&dsi1_in>; + }; + }; + }; + }; + + dsi0: dsi@ae94000 { + compatible = "qcom,mdss-dsi-ctrl"; + reg = <0xae94000 0x400>; + reg-names = "dsi_ctrl"; + + interrupt-parent = <&mdss>; + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>, +
Re: [Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
On 12/03/2018 05:10 PM, Jordan Crouse wrote: On Mon, Dec 03, 2018 at 04:18:16PM -0500, Jonathan Marek wrote: Signed-off-by: Jonathan Marek --- arch/arm/boot/dts/imx51.dtsi | 17 + arch/arm/boot/dts/imx53.dtsi | 17 + 2 files changed, 34 insertions(+) diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi index 67d462715..e9a7bbce9 100644 --- a/arch/arm/boot/dts/imx51.dtsi +++ b/arch/arm/boot/dts/imx51.dtsi @@ -628,5 +628,22 @@ clock-names = "ipg", "ahb"; }; }; + + gpu: gpu@3000 { + compatible = "amd,imageon-200.1", "amd,imageon"; + reg = <0x3000 0x2>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <12>; + interrupt-names = "kgsl_3d0_irq"; + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks IMX5_CLK_GARB_GATE>; + clock-names = "core_clk", "mem_iface_clk"; + + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { + qcom,gpu-freq = <16625>; + }; + }; There shouldn't be any incremental cost in the source code to use OPP; it should just work. And then this won't give us a further reason to keep the legacy code around when we decide to dump any pretense of downstream "compatibility". I will switch to OPP. + }; }; }; diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi index 207eb557c..586d45586 100644 --- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -838,5 +838,22 @@ reg = <0xf800 0x2>; clocks = <&clks IMX5_CLK_OCRAM>; }; + + gpu: gpu@3000 { + compatible = "amd,imageon-200.0", "amd,imageon"; + reg = <0x3000 0x2>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <12>; + interrupt-names = "kgsl_3d0_irq"; + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks IMX5_CLK_GARB_GATE>; + clock-names = "core_clk", "mem_iface_clk"; + + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { + qcom,gpu-freq = <2>; + }; Same. + }; + }; }; }; -- 2.17.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
On 12/03/2018 05:16 PM, Fabio Estevam wrote: Hi Jonathan, Thanks for working on this. Really nice to see GPU support for mx51/mx53! On Mon, Dec 3, 2018 at 7:21 PM Jonathan Marek wrote: Please add a commit log. Signed-off-by: Jonathan Marek --- arch/arm/boot/dts/imx51.dtsi | 17 + arch/arm/boot/dts/imx53.dtsi | 17 + 2 files changed, 34 insertions(+) diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi index 67d462715..e9a7bbce9 100644 --- a/arch/arm/boot/dts/imx51.dtsi +++ b/arch/arm/boot/dts/imx51.dtsi @@ -628,5 +628,22 @@ clock-names = "ipg", "ahb"; }; }; + + gpu: gpu@3000 { We put the peripheral nodes in address order, so this one should go prior to the IPU node. + compatible = "amd,imageon-200.1", "amd,imageon"; I can't find the dt-bindings for these compatible entries. Have you documented them? It is the same as qcom,adreno which is documented here: Documentation/devicetree/bindings/display/msm/gpu.txt I guess I should add amd,imageon there. + reg = <0x3000 0x2>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <12>; + interrupt-names = "kgsl_3d0_irq"; + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks IMX5_CLK_GARB_GATE>; + clock-names = "core_clk", "mem_iface_clk"; + + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { You could drop this @0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
On Mon, Dec 03, 2018 at 04:18:16PM -0500, Jonathan Marek wrote: > Signed-off-by: Jonathan Marek > --- > arch/arm/boot/dts/imx51.dtsi | 17 + > arch/arm/boot/dts/imx53.dtsi | 17 + > 2 files changed, 34 insertions(+) > > diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi > index 67d462715..e9a7bbce9 100644 > --- a/arch/arm/boot/dts/imx51.dtsi > +++ b/arch/arm/boot/dts/imx51.dtsi > @@ -628,5 +628,22 @@ > clock-names = "ipg", "ahb"; > }; > }; > + > + gpu: gpu@3000 { > + compatible = "amd,imageon-200.1", "amd,imageon"; > + reg = <0x3000 0x2>; > + reg-names = "kgsl_3d0_reg_memory"; > + interrupts = <12>; > + interrupt-names = "kgsl_3d0_irq"; > + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks > IMX5_CLK_GARB_GATE>; > + clock-names = "core_clk", "mem_iface_clk"; > + > + qcom,gpu-pwrlevels { > + compatible = "qcom,gpu-pwrlevels"; > + qcom,gpu-pwrlevel@0 { > + qcom,gpu-freq = <16625>; > + }; > + }; There shouldn't be any incremental cost in the source code to use OPP; it should just work. And then this won't give us a further reason to keep the legacy code around when we decide to dump any pretense of downstream "compatibility". > + }; > }; > }; > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi > index 207eb557c..586d45586 100644 > --- a/arch/arm/boot/dts/imx53.dtsi > +++ b/arch/arm/boot/dts/imx53.dtsi > @@ -838,5 +838,22 @@ > reg = <0xf800 0x2>; > clocks = <&clks IMX5_CLK_OCRAM>; > }; > + > + gpu: gpu@3000 { > + compatible = "amd,imageon-200.0", "amd,imageon"; > + reg = <0x3000 0x2>; > + reg-names = "kgsl_3d0_reg_memory"; > + interrupts = <12>; > + interrupt-names = "kgsl_3d0_irq"; > + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks > IMX5_CLK_GARB_GATE>; > + clock-names = "core_clk", "mem_iface_clk"; > + > + qcom,gpu-pwrlevels { > + compatible = "qcom,gpu-pwrlevels"; > + qcom,gpu-pwrlevel@0 { > + qcom,gpu-freq = <2>; > + }; Same. > + }; > + }; > }; > }; > -- > 2.17.1 -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote: > Quite late, hopefully not too late. > > > On 21.11.2018 12:51, Ville Syrjälä wrote: > > On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote: > >> > >>> return; > >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c > >>> b/drivers/gpu/drm/bridge/sil-sii8620.c > >>> index a6e8f4591e63..0cc293a6ac24 100644 > >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c > >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > >>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620 > >>> *ctx, > >>> int ret; > >>> > >>> ret = drm_hdmi_avi_infoframe_from_display_mode(&frm.avi, > >>> -mode, > >>> -true); > >>> +NULL, mode); > >>> if (ctx->use_packed_pixel) > >>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422; > >>> > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> index 64c3cf027518..88b720b63126 100644 > >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, > >>> struct drm_display_mode *mode) > >>> u8 val; > >>> > >>> /* Initialise info frame from DRM mode */ > >>> - drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false); > >>> + drm_hdmi_avi_infoframe_from_display_mode(&frame, > >>> + &hdmi->connector, mode); > >>> > >>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format)) > >>> frame.colorspace = HDMI_COLORSPACE_YUV444; > >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >>> index b506e3622b08..501ac05ba7da 100644 > >>> --- a/drivers/gpu/drm/drm_edid.c > >>> +++ b/drivers/gpu/drm/drm_edid.c > >>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector > >>> *connector, > >>> } > >>> EXPORT_SYMBOL(drm_set_preferred_mode); > >>> > >>> +static bool is_hdmi2_sink(struct drm_connector *connector) > >> You're usually known for adding const all around, why not const pointer > >> here and in all the other drm_* functions that call this? > > My current approach is to constify states/fbs/etc. but not so much > > crtcs/connectors/etc. Too much const can sometimes get in the way > > of things requiring that you remove the const later. But I guess > > in this case the const shouldn't really get in the way of anything > > because these are pretty much supposed to be pure functions. > > > >>> +{ > >>> + /* > >>> + * FIXME: sil-sii8620 doesn't have a connector around when > >>> + * we need one, so we have to be prepared for a NULL connector. > >>> + */ > >>> + if (!connector) > >>> + return false; > >> This actually changes the is_hdmi2_sink value for sil-sii8620. > > Hmm. No idea why they would have set that to true when everyone else is > > passing false. > > > Because false does not work :) More precisely MHLv3 (used in Sii8620) > uses CTA-861-F standard for infoframes, which is specific to HDMI2.0. > > Unfortunately I have no access to MHL specs, but my experiments and > vendor drivers strongly suggests it is done this way. > > This is important in case of 4K modes which are handled differently by > HDMI 1.4 and HDMI2.0. HDMI 2.0 handles 4k just like 1.4 handled it when you use one of the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we switch over to the HDMI 2.0 specific signalling. > > The pipeline looks like (in parenthesis HDMI version on the stream): > > exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV > > > > I guess I can change this to true to not change it. IIRC > > that was the only driver that didn't have a connector around. > > > > That said, I was actually thinking of removing this hdmi2 vs. not > > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it > > "just in case", but we already have a similar issue with earlier > > cea-861 revisions and haven't seen any bugs where an older monitor > > would get confused by a VIC not yet defined when the monitor was > > produced. > > > Are you sure this is a good idea? As I said earlier 4K modes are present > in HDMI 1.4 and 2.0 but they are handled differently, so this is not > only matter of unknown VIC in AVIF. > > > Regards > > Andrzej -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 14/24] drm/msm: dpu: Grab the modeset locks in frame_event
On Fri, Nov 30, 2018 at 05:00:02PM -0500, Sean Paul wrote: > From: Sean Paul > > This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks > since it digs into the state objects. > > Changes in v2: > - None > Changes in v3: > - Use those nifty new DRM_MODESET_LOCK_ALL_* helpers (Daniel) > > Cc: Daniel Vetter > Cc: Jeykumar Sankaran > Signed-off-by: Sean Paul > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 74ef384d9cd6a..03ddd281a354f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -306,6 +306,19 @@ static void dpu_crtc_vblank_cb(void *data) > trace_dpu_crtc_vblank_cb(DRMID(crtc)); > } > > +static void dpu_crtc_release_bw_unlocked(struct drm_crtc *crtc) > +{ > + int ret = 0; > + struct drm_modeset_acquire_ctx ctx; > + > + DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, ret); > + dpu_core_perf_crtc_release_bw(crtc); > + DRM_MODESET_LOCK_ALL_END(ctx, ret); So pretty, and correct even :-) Acked-by: Daniel Vetter > + if (ret) > + DRM_ERROR("Failed to acquire modeset locks to release bw, %d\n", > + ret); > +} > + > static void dpu_crtc_frame_event_work(struct kthread_work *work) > { > struct dpu_crtc_frame_event *fevent = container_of(work, > @@ -335,7 +348,7 @@ static void dpu_crtc_frame_event_work(struct kthread_work > *work) > /* release bandwidth and other resources */ > trace_dpu_crtc_frame_event_done(DRMID(crtc), > fevent->event); > - dpu_core_perf_crtc_release_bw(crtc); > + dpu_crtc_release_bw_unlocked(crtc); > } else { > trace_dpu_crtc_frame_event_more_pending(DRMID(crtc), > fevent->event); > -- > Sean Paul, Software Engineer, Google / Chromium OS > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > On 21.11.2018 19:19, Laurent Pinchart wrote: > > Hi Ville, > > > > Thank you for the patch. > > > > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >> From: Ville Syrjälä > >> > >> Make life easier for drivers by simply passing the connector > >> to drm_hdmi_avi_infoframe_from_display_mode() and > >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >> need to worry about is_hdmi2_sink mess. > > While this is good for display controller drivers, the change isn't great > > for > > bridge drivers. Down the road we're looking at moving connector support out > > of > > the bridge drivers. Adding an additional dependency to connectors in the > > bridges will make that more difficult. Ideally bridges should retrieve the > > information from their sink, regardless of whether it is a connector or > > another bridge. > > > I agree with it, and case of sii8620 shows that there are cases where > bridge has no direct access to the connector. It's just a matter of plumbing it through. > > On the other side, since you are passing connector to > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > parameter and rename the function to > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > mode set on the connector differs? Connectors don't have a mode. > > > Regards > > Andrzej > > > > > > Please see below for an additional comment. > > > >> Cc: Alex Deucher > >> Cc: "Christian König" > >> Cc: "David (ChunMing) Zhou" > >> Cc: Archit Taneja > >> Cc: Andrzej Hajda > >> Cc: Laurent Pinchart > >> Cc: Inki Dae > >> Cc: Joonyoung Shim > >> Cc: Seung-Woo Kim > >> Cc: Kyungmin Park > >> Cc: Russell King > >> Cc: CK Hu > >> Cc: Philipp Zabel > >> Cc: Rob Clark > >> Cc: Ben Skeggs > >> Cc: Tomi Valkeinen > >> Cc: Sandy Huang > >> Cc: "Heiko Stübner" > >> Cc: Benjamin Gaignard > >> Cc: Vincent Abriou > >> Cc: Thierry Reding > >> Cc: Eric Anholt > >> Cc: Shawn Guo > >> Cc: Ilia Mirkin > >> Cc: amd-...@lists.freedesktop.org > >> Cc: linux-arm-...@vger.kernel.org > >> Cc: freedreno@lists.freedesktop.org > >> Cc: nouv...@lists.freedesktop.org > >> Cc: linux-te...@vger.kernel.org > >> Signed-off-by: Ville Syrjälä > >> --- > >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > >> drivers/gpu/drm/bridge/sii902x.c | 3 ++- > >> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > >> drivers/gpu/drm/drm_edid.c| 33 ++- > >> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > >> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > >> drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > >> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > >> drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > >> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > >> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > >> drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > >> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > >> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > >> drivers/gpu/drm/tegra/hdmi.c | 3 ++- > >> drivers/gpu/drm/tegra/sor.c | 3 ++- > >> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > >> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > >> include/drm/drm_edid.h| 8 +++--- > >> 27 files changed, 94 insertions(+), 66 deletions(-) > > For dw-hdmi and omapdrm, > > > > Reviewed-by: Laurent Pinchart > > -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 3/4] drm/msm: implement a2xx mmu
A2XX has its own very simple MMU. Added a msm_use_mmu() function because we can't rely on iommu_present to decide to use MMU or not. Signed-off-by: Jonathan Marek --- v3: rebased on msm-next-staging and moved is_a2xx initialization earlier drivers/gpu/drm/msm/Makefile | 3 +- drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 50 - drivers/gpu/drm/msm/adreno/adreno_device.c | 3 + drivers/gpu/drm/msm/adreno/adreno_gpu.c| 3 + drivers/gpu/drm/msm/msm_drv.c | 11 +- drivers/gpu/drm/msm/msm_drv.h | 8 ++ drivers/gpu/drm/msm/msm_gem.c | 4 +- drivers/gpu/drm/msm/msm_gem_vma.c | 23 drivers/gpu/drm/msm/msm_gpu.c | 31 -- drivers/gpu/drm/msm/msm_gpummu.c | 122 + drivers/gpu/drm/msm/msm_mmu.h | 3 + 11 files changed, 241 insertions(+), 20 deletions(-) create mode 100644 drivers/gpu/drm/msm/msm_gpummu.c diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 61e76f87a..1b26c4105 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -93,7 +93,8 @@ msm-y := \ msm_rd.o \ msm_ringbuffer.o \ msm_submitqueue.o \ - msm_gpu_tracepoints.o + msm_gpu_tracepoints.o \ + msm_gpummu.o msm-$(CONFIG_DEBUG_FS) += adreno/a5xx_debugfs.o \ disp/dpu1/dpu_dbg.o diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c index 5eddcf14e..1f83bc18d 100644 --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c @@ -2,6 +2,8 @@ /* Copyright (c) 2018 The Linux Foundation. All rights reserved. */ #include "a2xx_gpu.h" +#include "msm_gem.h" +#include "msm_mmu.h" extern bool hang_debug; @@ -58,9 +60,12 @@ static bool a2xx_me_init(struct msm_gpu *gpu) static int a2xx_hw_init(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + dma_addr_t pt_base, tran_error; uint32_t *ptr, len; int i, ret; + msm_gpummu_params(gpu->aspace->mmu, &pt_base, &tran_error); + DBG("%s", gpu->name); /* halt ME to avoid ucode upload issues on a20x */ @@ -80,9 +85,34 @@ static int a2xx_hw_init(struct msm_gpu *gpu) /* note: kgsl uses 0x for a20x */ gpu_write(gpu, REG_A2XX_RBBM_CNTL, 0x4442); - gpu_write(gpu, REG_A2XX_MH_MMU_CONFIG, 0); - gpu_write(gpu, REG_A2XX_MH_MMU_MPU_BASE, 0); + /* MPU: physical range */ + gpu_write(gpu, REG_A2XX_MH_MMU_MPU_BASE, 0x); gpu_write(gpu, REG_A2XX_MH_MMU_MPU_END, 0xf000); + + gpu_write(gpu, REG_A2XX_MH_MMU_CONFIG, A2XX_MH_MMU_CONFIG_MMU_ENABLE | + A2XX_MH_MMU_CONFIG_RB_W_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_W_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_R0_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_R1_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_R2_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_R3_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_CP_R4_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_VGT_R0_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_VGT_R1_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_TC_R_CLNT_BEHAVIOR(BEH_TRAN_RNG) | + A2XX_MH_MMU_CONFIG_PA_W_CLNT_BEHAVIOR(BEH_TRAN_RNG)); + + /* same as parameters in adreno_gpu */ + gpu_write(gpu, REG_A2XX_MH_MMU_VA_RANGE, SZ_16M | + A2XX_MH_MMU_VA_RANGE_NUM_64KB_REGIONS(0xfff)); + + gpu_write(gpu, REG_A2XX_MH_MMU_PT_BASE, pt_base); + gpu_write(gpu, REG_A2XX_MH_MMU_TRAN_ERROR, tran_error); + + gpu_write(gpu, REG_A2XX_MH_MMU_INVALIDATE, + A2XX_MH_MMU_INVALIDATE_INVALIDATE_ALL | + A2XX_MH_MMU_INVALIDATE_INVALIDATE_TC); + gpu_write(gpu, REG_A2XX_MH_ARBITER_CONFIG, A2XX_MH_ARBITER_CONFIG_SAME_PAGE_LIMIT(16) | A2XX_MH_ARBITER_CONFIG_L1_ARB_ENABLE | @@ -109,9 +139,21 @@ static int a2xx_hw_init(struct msm_gpu *gpu) /* note: gsl doesn't set this */ gpu_write(gpu, REG_A2XX_RBBM_DEBUG, 0x0008); - gpu_write(gpu, REG_A2XX_RBBM_INT_CNTL, 0); - gpu_write(gpu, REG_AXXX_CP_INT_CNTL, 0x8000); /* RB INT */ + gpu_write(gpu, REG_A2XX_RBBM_INT_CNTL, + A2XX_RBBM_INT_CNTL_RDERR_INT_MASK); + gpu_write(gpu, REG_AXXX_CP_INT_CNTL, + AXXX_CP_INT_CNTL_T0_PACKET_IN_IB_MASK | + AXXX_CP_INT_CNTL_OPCODE_ERROR_MASK | + AXXX_CP_INT_CNTL_PROTECTED_MODE_ERROR_MASK | + AXXX_CP_INT_CNTL_RESERVED_BIT_ERROR_MASK | + AXXX_CP_INT_CNTL_IB_ERROR_MASK | + AXXX_CP_INT_CNTL_IB1_INT_MASK | + AXXX_CP_INT_CNTL_RB_INT_MASK); gpu_write(gpu, REG
[Freedreno] [PATCH v3 1/4] drm/msm/mdp4: add lcdc-align-lsb flag to control lane alignment
This allows controlling which of the 8 lanes are used for 6 bit color. Signed-off-by: Jonathan Marek --- v3: removed empty line and added documentation .../devicetree/bindings/display/msm/mdp4.txt | 2 ++ .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 21 --- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/display/msm/mdp4.txt b/Documentation/devicetree/bindings/display/msm/mdp4.txt index 3c341a15c..b07eeb38f 100644 --- a/Documentation/devicetree/bindings/display/msm/mdp4.txt +++ b/Documentation/devicetree/bindings/display/msm/mdp4.txt @@ -38,6 +38,8 @@ Required properties: Optional properties: - clock-names: the following clocks are optional: * "lut_clk" +- qcom,lcdc-align-lsb: Boolean value indicating that LSB alignment should be + used for LCDC. This is only valid for 18bpp panels. Example: diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c index 9e08c2efa..c9e34501a 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c @@ -377,20 +377,25 @@ static void mdp4_lcdc_encoder_enable(struct drm_encoder *encoder) unsigned long pc = mdp4_lcdc_encoder->pixclock; struct mdp4_kms *mdp4_kms = get_kms(encoder); struct drm_panel *panel; + uint32_t config; int i, ret; if (WARN_ON(mdp4_lcdc_encoder->enabled)) return; /* TODO: hard-coded for 18bpp: */ - mdp4_crtc_set_config(encoder->crtc, - MDP4_DMA_CONFIG_R_BPC(BPC6) | - MDP4_DMA_CONFIG_G_BPC(BPC6) | - MDP4_DMA_CONFIG_B_BPC(BPC6) | - MDP4_DMA_CONFIG_PACK_ALIGN_MSB | - MDP4_DMA_CONFIG_PACK(0x21) | - MDP4_DMA_CONFIG_DEFLKR_EN | - MDP4_DMA_CONFIG_DITHER_EN); + config = + MDP4_DMA_CONFIG_R_BPC(BPC6) | + MDP4_DMA_CONFIG_G_BPC(BPC6) | + MDP4_DMA_CONFIG_B_BPC(BPC6) | + MDP4_DMA_CONFIG_PACK(0x21) | + MDP4_DMA_CONFIG_DEFLKR_EN | + MDP4_DMA_CONFIG_DITHER_EN; + + if (!of_property_read_bool(dev->dev->of_node, "qcom,lcdc-align-lsb")) + config |= MDP4_DMA_CONFIG_PACK_ALIGN_MSB; + + mdp4_crtc_set_config(encoder->crtc, config); mdp4_crtc_set_intf(encoder->crtc, INTF_LCDC_DTV, 0); bs_set(mdp4_lcdc_encoder, 1); -- 2.17.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 4/4] ARM: dts: imx5: add gpu nodes
Signed-off-by: Jonathan Marek --- arch/arm/boot/dts/imx51.dtsi | 17 + arch/arm/boot/dts/imx53.dtsi | 17 + 2 files changed, 34 insertions(+) diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi index 67d462715..e9a7bbce9 100644 --- a/arch/arm/boot/dts/imx51.dtsi +++ b/arch/arm/boot/dts/imx51.dtsi @@ -628,5 +628,22 @@ clock-names = "ipg", "ahb"; }; }; + + gpu: gpu@3000 { + compatible = "amd,imageon-200.1", "amd,imageon"; + reg = <0x3000 0x2>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <12>; + interrupt-names = "kgsl_3d0_irq"; + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks IMX5_CLK_GARB_GATE>; + clock-names = "core_clk", "mem_iface_clk"; + + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { + qcom,gpu-freq = <16625>; + }; + }; + }; }; }; diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi index 207eb557c..586d45586 100644 --- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -838,5 +838,22 @@ reg = <0xf800 0x2>; clocks = <&clks IMX5_CLK_OCRAM>; }; + + gpu: gpu@3000 { + compatible = "amd,imageon-200.0", "amd,imageon"; + reg = <0x3000 0x2>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <12>; + interrupt-names = "kgsl_3d0_irq"; + clocks = <&clks IMX5_CLK_GPU3D_GATE>, <&clks IMX5_CLK_GARB_GATE>; + clock-names = "core_clk", "mem_iface_clk"; + + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { + qcom,gpu-freq = <2>; + }; + }; + }; }; }; -- 2.17.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 2/4] drm/msm: add headless gpu device for imx5
This patch allows using drm/msm without qcom display hardware. It adds a amd,imageon compatible, which is used instead of qcom,adreno, but does not require a top level msm node. Signed-off-by: Jonathan Marek --- v3: reworked to work with only a amd,imageon node drivers/gpu/drm/msm/Kconfig| 4 +-- drivers/gpu/drm/msm/adreno/adreno_device.c | 35 -- drivers/gpu/drm/msm/msm_debugfs.c | 2 +- drivers/gpu/drm/msm/msm_drv.c | 21 +++-- 4 files changed, 46 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig index 843a9d40c..cf549f1ed 100644 --- a/drivers/gpu/drm/msm/Kconfig +++ b/drivers/gpu/drm/msm/Kconfig @@ -2,7 +2,7 @@ config DRM_MSM tristate "MSM DRM" depends on DRM - depends on ARCH_QCOM || (ARM && COMPILE_TEST) + depends on ARCH_QCOM || SOC_IMX5 || (ARM && COMPILE_TEST) depends on OF && COMMON_CLK depends on MMU select QCOM_MDT_LOADER if ARCH_QCOM @@ -11,7 +11,7 @@ config DRM_MSM select DRM_PANEL select SHMEM select TMPFS - select QCOM_SCM + select QCOM_SCM if ARCH_QCOM select WANT_DEV_COREDUMP select SND_SOC_HDMI_CODEC if SND_SOC select SYNC_FILE diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index adc442f73..99e363c3d 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -271,7 +271,8 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev) if (ret == 0) { unsigned int r, patch; - if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2) { + if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 || + sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) { rev->core = r / 100; r %= 100; rev->major = r / 10; @@ -356,9 +357,37 @@ static const struct component_ops a3xx_ops = { .unbind = adreno_unbind, }; +static void adreno_device_register_headless(void) +{ + /* on imx5, we don't have a top-level mdp/dpu node +* this creates a dummy node for the driver for that case +*/ + struct platform_device_info dummy_info = { + .parent = NULL, + .name = "msm", + .id = -1, + .res = NULL, + .num_res = 0, + .data = NULL, + .size_data = 0, + .dma_mask = ~0, + }; + platform_device_register_full(&dummy_info); +} + static int adreno_probe(struct platform_device *pdev) { - return component_add(&pdev->dev, &a3xx_ops); + + int ret; + + ret = component_add(&pdev->dev, &a3xx_ops); + if (ret) + return ret; + + if (of_device_is_compatible(pdev->dev.of_node, "amd,imageon")) + adreno_device_register_headless(); + + return 0; } static int adreno_remove(struct platform_device *pdev) @@ -370,6 +399,8 @@ static int adreno_remove(struct platform_device *pdev) static const struct of_device_id dt_match[] = { { .compatible = "qcom,adreno" }, { .compatible = "qcom,adreno-3xx" }, + /* for compatibility with imx5 gpu: */ + { .compatible = "amd,imageon" }, /* for backwards compat w/ downstream kgsl DT files: */ { .compatible = "qcom,kgsl-3d0" }, {} diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c index 9a7cf9fe2..fb423d309 100644 --- a/drivers/gpu/drm/msm/msm_debugfs.c +++ b/drivers/gpu/drm/msm/msm_debugfs.c @@ -242,7 +242,7 @@ int msm_debugfs_init(struct drm_minor *minor) debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root, dev, &msm_gpu_fops); - if (priv->kms->funcs->debugfs_init) { + if (priv->kms && priv->kms->funcs->debugfs_init) { ret = priv->kms->funcs->debugfs_init(priv->kms, minor); if (ret) return ret; diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 81bfac744..7842518a9 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -510,17 +510,13 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->kms = kms; break; default: - kms = ERR_PTR(-ENODEV); + /* valid only for the dummy headless case, where of_node=NULL */ + WARN_ON(dev->of_node); + kms = NULL; break; } if (IS_ERR(kms)) { - /* -* NOTE: once we have GPU support, having no kms should not -* be considered fatal.. ideally we would still support gpu -* and (for example) use dmabuf/prime to share buffers
Re: [Freedreno] [PATCH v2 5/5] drm/msm: subclass work object for vblank events
On 2018-12-03 06:21, Sean Paul wrote: On Fri, Nov 30, 2018 at 04:21:15PM -0800, Jeykumar Sankaran wrote: On 2018-11-30 12:07, Sean Paul wrote: > On Fri, Nov 30, 2018 at 11:45:55AM -0800, Jeykumar Sankaran wrote: > > On 2018-11-29 14:15, Sean Paul wrote: > > > On Tue, Nov 20, 2018 at 02:04:14PM -0800, Jeykumar Sankaran wrote: > > > > On 2018-11-07 07:55, Sean Paul wrote: > > > > > On Tue, Nov 06, 2018 at 02:36:30PM -0800, Jeykumar Sankaran wrote: > > > > > > msm maintains a separate structure to define vblank > > > > > > work definitions and a list to track events submitted > > > > > > to the workqueue. We can avoid this redundant list > > > > > > and its protection mechanism, if we subclass the > > > > > > work object to encapsulate vblank event parameters. > > > > > > > > > > > > changes in v2: > > > > > > - subclass optimization on system wq (Sean Paul) > > > > > > > > > > I wouldn't do it like this, tbh. One problem is that you've lost > your > > > > > flush() on > > > > > unbind, so there's no way to know if you have workers in the wild > > > > > waiting > > > > > to > > > > > enable/disable vblank. > > > > > > > > > > Another issues is that AFAICT, we don't need a queue of > > > > > enables/disables, > > > > > but > > > > > rather just the last requested state (ie: should we be on or off). > So > > > > > things > > > > > don't need to be this complicated (and we're possibly thrashing > vblank > > > > > on/off > > > > > for no reason). > > > > > > > > > > I'm still of the mind that you should just make this synchronous > and > > > be > > > > > done > > > > > with the threads (especially since we're still > uncovering/introducing > > > > > races!). > > > > > > > > > While scoping out the effort to make vblank events synchronous, I > > > > found > > > > that the spinlock locking order of vblank request sequence and > vblank > > > > callback > > > > sequences are the opposite. > > > > > > > > In DPU, drm_vblank_enable acquires vblank_time_lock before > registering > > > > the crtc to encoder which happens after acquiring encoder_spinlock. > > > > But > > > > the vblank_callback acquires encoder_spinlock before accessing the > > > > registered > > > > crtc and calling into drm_vblank_handler which tries to acquire > > > > vblank_time_lock. > > > > Acquiring both vblank_time_lock and encoder_spinlock in the same > > > > thread > > > > is leading to deadlock. > > > > > > Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps > > > with > > > enable/disable? I hacked in sync vblank enable/disable quickly to see > if > > > I > > > could > > > reproduce what you're seeing, but things seemed well behaved. > > > > > > > The race is between drm_vblank_get/put and vblank_handler contexts. > > > > When made synchronous: > > > > while calling drm_vblank_get, the callstack looks like below: > > drm_vblank_get -> drm_vblank_enable (acquires vblank_time_lock) -> > > __enable_vblank -> dpu_crtc_vblank -> > > dpu_encoder_toggle_vblank_for_crtc > > (tries to acquire enc_spinlock) > > > > In vblank handler, the call stack will be: > > dpu_encoder_phys_vid_vblank_irq -> dpu_encoder_vblank_callback > > (acquires > > enc_spinlock) -> dpu_crtc_vblank_callback -> drm_handle_vblank > > (tries to > > acquire vblank_time_lock) > > Hmm, I'm not sure how this can happen. We acquire and release the > enc_spinlock > before enabling the irq, yes we will hold on to the vbl_time_lock, but > we > shouldn't be trying to reacquire an encoder's spinlock after we've > enabled > it. In the synchronous approach dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock) will be called while we are holding the vbl_time_lock. > I don't know how that can deadlock, since we should never be running > enable and > the handler concurrently. > I agree that vblank_irq handler should not be running before the enable sequence. But don't you expect the handler to be running while calling the vblank_disable sequence? This is an entirely different problem though. It's also one that is easier to fix. I think we could probably grab the enc_spinlock in disable and clear the crtc pointer. we do hold enc_spinlock in dpu_encoder_assign_crtc (drm/msm: dpu: Remove vblank_callback from encoder) where we clear the crtc pointer. What I'm getting at is that there's no fundamental reason why we need to have async vblank enable/disable. Sean There is really no *need* to have them async. But I believe the reason why they are implemented this way is to avoid deadlock between the below two paths. Restating the above findings: vblank_handlers and vblank enable/disable can run concurrently. The first trying to acquire vbl_time_lock holding enc_spinlock. Other trying to acquire enc_spinlock holding vbl_time_lock. Thanks, Jeykumar S. vbl disable will try to acquire the locks in the opposite order to that of irq_handler and the same issue is bound to happen. With your patch, you should be able to sim
[Freedreno] [PATCH] drm/msm: dpu: Allocate proper amount for dpu_crtc_state
From: Sean Paul Since dpu_crtc subclasses crtc_state, we need a custom .reset hook in order to allocate the right amount of memory to accommodate the additional struct members in dpu_crtc_state. So bring it [partially] back. Relevant KASAN splat: [ 10.82] == [ 10.344288] BUG: KASAN: slab-out-of-bounds in kmemdup+0x50/0x80 [ 10.350390] Read of size 736 at addr ffc0d9f06080 by task frecon/394 [ 10.358861] CPU: 6 PID: 394 Comm: frecon Tainted: GW 4.19.4 #121 [ 10.366476] Hardware name: Google Cheza (rev2) (DT) [ 10.371514] Call trace: [ 10.374087] dump_backtrace+0x0/0x194 [ 10.377878] show_stack+0x20/0x28 [ 10.381330] dump_stack+0xa0/0xc8 [ 10.384783] print_address_description+0x78/0x2e0 [ 10.389639] kasan_report+0x290/0x2d0 [ 10.393428] check_memory_region+0x20/0x14c [ 10.397740] __asan_loadN+0x14/0x1c [ 10.401345] kmemdup+0x50/0x80 [ 10.404524] dpu_crtc_duplicate_state+0x58/0xa0 [ 10.409228] drm_atomic_get_crtc_state+0xac/0x178 [ 10.414095] __drm_atomic_helper_set_config+0x54/0x4a4 [ 10.419393] drm_atomic_helper_set_config+0x60/0xb4 [ 10.424435] drm_mode_setcrtc+0x720/0x760 [ 10.428570] drm_ioctl_kernel+0xd8/0x13c [ 10.432617] drm_ioctl+0x380/0x4f4 [ 10.436150] drm_compat_ioctl+0x54/0x13c [ 10.440219] __arm64_compat_sys_ioctl+0x1d8/0xef4 [ 10.445086] el0_svc_common+0xd8/0x138 [ 10.448961] el0_svc_compat_handler+0x58/0x68 [ 10.453463] el0_svc_compat+0x8/0x18 [ 10.458712] Allocated by task 56: [ 10.462148] kasan_kmalloc.part.4+0x48/0xf4 [ 10.466465] kasan_kmalloc+0x8c/0xa0 [ 10.470165] kmem_cache_alloc_trace+0x25c/0x27c [ 10.474848] drm_atomic_helper_crtc_reset+0x68/0x98 [ 10.479877] drm_mode_config_reset+0xc4/0x19c [ 10.484383] msm_drm_bind+0x814/0x8dc [ 10.488169] try_to_bring_up_master.part.7+0x48/0xac [ 10.493282] component_master_add_with_match+0x158/0x198 [ 10.498758] msm_pdev_probe+0x328/0x348 [ 10.502736] platform_drv_probe+0x74/0xc8 [ 10.506877] really_probe+0x1ac/0x35c [ 10.510659] driver_probe_device+0xd4/0x118 [ 10.514975] __device_attach_driver+0xc8/0xf4 [ 10.519477] bus_for_each_drv+0xb4/0xe4 [ 10.523439] __device_attach+0xd0/0x158 [ 10.527394] device_initial_probe+0x24/0x30 [ 10.531715] bus_probe_device+0x50/0xe4 [ 10.535681] deferred_probe_work_func+0xac/0xdc [ 10.540376] process_one_work+0x3f0/0x6d4 [ 10.544521] worker_thread+0x3f4/0x520 [ 10.548399] kthread+0x1b4/0x1c8 [ 10.551740] ret_from_fork+0x10/0x18 [ 10.556986] Freed by task 0: [ 10.559967] (stack is not available) [ 10.565216] The buggy address belongs to the object at ffc0d9f06080 which belongs to the cache kmalloc-1024 of size 1024 [ 10.578268] The buggy address is located 0 bytes inside of 1024-byte region [ffc0d9f06080, ffc0d9f06480) [ 10.590248] The buggy address belongs to the page: [ 10.595195] page:ffbf0367c000 count:1 mapcount:0 mapping:ffc0de40f680 index:0x0 compound_mapcount: 0 [ 10.605321] flags: 0x40008100(slab|head) [ 10.610100] raw: 40008100 ffbf0369fa08 ffbf0367f008 ffc0de40f680 [ 10.618077] raw: 00150015 0001 [ 10.626049] page dumped because: kasan: bad access detected [ 10.633341] Memory state around the buggy address: [ 10.638282] ffc0d9f06180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 10.645710] ffc0d9f06200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 10.653139] >ffc0d9f06280: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc [ 10.660571] ^ [ 10.665774] ffc0d9f06300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 10.673210] ffc0d9f06380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 10.680639] == Fixes: a6ba45afda41 ("drm/msm/dpu: Replace dpu_crtc_reset by atomic helper") Cc: Sean Paul Cc: Bruce Wang Cc: Rob Clark Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 1e3e57817b72b..d27ea2a95f85b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -819,6 +819,18 @@ static void _dpu_crtc_vblank_enable_no_lock( } } +static void dpu_crtc_reset(struct drm_crtc *crtc) +{ + struct dpu_crtc_state *cstate; + + if (crtc->state) + dpu_crtc_destroy_state(crtc, crtc->state); + + crtc->state = kzalloc(sizeof(*cstate), GFP_KERNEL); + if (crtc->state) + crtc->state->crtc = crtc; +} + /** * dpu_crtc_duplicate_state - state duplicate hook * @crtc: Pointer to drm crtc
Re: [Freedreno] [PATCH v3 14/24] drm/msm: dpu: Grab the modeset locks in frame_event
On 2018-11-30 14:00, Sean Paul wrote: From: Sean Paul This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects. Changes in v2: - None Changes in v3: - Use those nifty new DRM_MODESET_LOCK_ALL_* helpers (Daniel) Cc: Daniel Vetter Cc: Jeykumar Sankaran Signed-off-by: Sean Paul --- I see Daniel's comments are addressed. So .. Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 74ef384d9cd6a..03ddd281a354f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -306,6 +306,19 @@ static void dpu_crtc_vblank_cb(void *data) trace_dpu_crtc_vblank_cb(DRMID(crtc)); } +static void dpu_crtc_release_bw_unlocked(struct drm_crtc *crtc) +{ + int ret = 0; + struct drm_modeset_acquire_ctx ctx; + + DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, ret); + dpu_core_perf_crtc_release_bw(crtc); + DRM_MODESET_LOCK_ALL_END(ctx, ret); + if (ret) + DRM_ERROR("Failed to acquire modeset locks to release bw, %d\n", + ret); +} + static void dpu_crtc_frame_event_work(struct kthread_work *work) { struct dpu_crtc_frame_event *fevent = container_of(work, @@ -335,7 +348,7 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work) /* release bandwidth and other resources */ trace_dpu_crtc_frame_event_done(DRMID(crtc), fevent->event); - dpu_core_perf_crtc_release_bw(crtc); + dpu_crtc_release_bw_unlocked(crtc); } else { trace_dpu_crtc_frame_event_more_pending(DRMID(crtc), fevent->event); -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 12/24] drm/msm: dpu: Move crtc runtime resume to encoder
On 2018-11-16 10:42, Sean Paul wrote: From: Sean Paul The crtc runtime resume doesn't actually operate on the crtc, but rather its encoders. The problem with this is that we need to inspect the crtc state to get the currently connected encoders. Since runtime resume isn't guaranteed to be called while holding the modeset locks (although it sometimes is), this presents a race condition. Now that we have ->enabled on the virtual encoders, and a lock to protect it, just call resume on each encoder and only restore the ones that are enabled. Changes in v2: - None Cc: Jeykumar Sankaran Signed-off-by: Sean Paul --- Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 24 - drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++--- 5 files changed, 15 insertions(+), 49 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index d8f58caf2772..9be24907f8c1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -841,30 +841,6 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc) return &cstate->base; } -void dpu_crtc_runtime_resume(struct drm_crtc *crtc) -{ - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); - struct drm_encoder *encoder; - - mutex_lock(&dpu_crtc->crtc_lock); - - if (!dpu_crtc->enabled) - goto end; - - trace_dpu_crtc_runtime_resume(DRMID(crtc)); - - /* restore encoder; crtc will be programmed during commit */ - drm_for_each_encoder(encoder, crtc->dev) { - if (encoder->crtc != crtc) - continue; - - dpu_encoder_virt_restore(encoder); - } - -end: - mutex_unlock(&dpu_crtc->crtc_lock); -} - static void dpu_crtc_disable(struct drm_crtc *crtc) { struct dpu_crtc *dpu_crtc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 1dca91d1210f..93d21a61a040 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct drm_crtc *crtc) return crtc ? crtc->enabled : false; } -/** - * dpu_crtc_runtime_resume - called by the top-level on pm_runtime_resume - * @crtc: CRTC to resume - */ -void dpu_crtc_runtime_resume(struct drm_crtc *crtc); - #endif /* _DPU_CRTC_H_ */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 3daa86220d47..d89ac520f7e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1089,28 +1089,24 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc) _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); } -void dpu_encoder_virt_restore(struct drm_encoder *drm_enc) +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) { - struct dpu_encoder_virt *dpu_enc = NULL; - int i; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - dpu_enc = to_dpu_encoder_virt(drm_enc); + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - for (i = 0; i < dpu_enc->num_phys_encs; i++) { - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + mutex_lock(&dpu_enc->enc_lock); - if (phys && (phys != dpu_enc->cur_master) && phys->ops.restore) - phys->ops.restore(phys); - } + if (!dpu_enc->enabled) + goto out; + if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore) + dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave); if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore) dpu_enc->cur_master->ops.restore(dpu_enc->cur_master); _dpu_encoder_virt_enable_helper(drm_enc); + +out: + mutex_unlock(&dpu_enc->enc_lock); } static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9dbf38f446d9..aa4f135218fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder *drm_encoder, enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder); /** - * dpu_encoder_virt_restore - restore the encoder configs + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder configs * @encoder: encoder pointer */ -void dpu_encoder_virt_restore(struct drm_encoder *encoder); +void dpu_encoder_virt_runtime_re
Re: [Freedreno] [PATCH v2 11/24] drm/msm: dpu: Add ->enabled to dpu_encoder_virt
On 2018-11-16 10:42, Sean Paul wrote: From: Sean Paul Add a bool to dpu_encoder_virt to track whether the encoder is enabled or not. Repurpose the enc_lock mutex to ensure that it is consistent with the hw state. Changes in v2: - None Cc: Jeykumar Sankaran Signed-off-by: Sean Paul --- Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 + 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 10a0676d1dcf..3daa86220d47 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -132,6 +132,7 @@ enum dpu_enc_rc_states { * @base: drm_encoder base class for registration with DRM * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes * @bus_scaling_client:Client handle to the bus scaling interface + * @enabled: True if the encoder is active, protected by enc_lock * @num_phys_encs: Actual number of physical encoders contained. * @phys_encs: Container of physical encoders managed. * @cur_master:Pointer to the current master in this mode. Optimization @@ -148,8 +149,8 @@ enum dpu_enc_rc_states { * all CTL paths * @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb * @debugfs_root: Debug file system root file node - * @enc_lock: Lock around physical encoder create/destroy and - access. + * @enc_lock: Lock around physical encoder + * create/destroy/enable/disable * @frame_busy_mask: Bitmask tracking which phys_enc we are still * busy processing current command. * Bit0 = phys_encs[0] etc. @@ -175,6 +176,8 @@ struct dpu_encoder_virt { spinlock_t enc_spinlock; uint32_t bus_scaling_client; + bool enabled; + unsigned int num_phys_encs; struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; struct dpu_encoder_phys *cur_master; @@ -1121,6 +1124,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) return; } dpu_enc = to_dpu_encoder_virt(drm_enc); + + mutex_lock(&dpu_enc->enc_lock); cur_mode = &dpu_enc->base.crtc->state->adjusted_mode; trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay, @@ -1137,10 +1142,15 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) if (ret) { DPU_ERROR_ENC(dpu_enc, "dpu resource control failed: %d\n", ret); - return; + goto out; } _dpu_encoder_virt_enable_helper(drm_enc); + + dpu_enc->enabled = true; + +out: + mutex_unlock(&dpu_enc->enc_lock); } static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) @@ -1162,11 +1172,14 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) return; } - mode = &drm_enc->crtc->state->adjusted_mode; - dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n"); + mutex_lock(&dpu_enc->enc_lock); + dpu_enc->enabled = false; + + mode = &drm_enc->crtc->state->adjusted_mode; + priv = drm_enc->dev->dev_private; dpu_kms = to_dpu_kms(priv->kms); @@ -1200,6 +1213,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); dpu_rm_release(&dpu_kms->rm, drm_enc); + + mutex_unlock(&dpu_enc->enc_lock); } static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, @@ -2233,6 +2248,8 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs); + dpu_enc->enabled = false; + return &dpu_enc->base; } -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 04/24] drm/msm: dpu: Don't use power_event for vbif_init_memtypes
On 2018-11-16 10:42, Sean Paul wrote: From: Sean Paul power_events are only used for pm_runtime, and that's all handled in dpu_kms. So just call vbif_init_memtypes at the correct times. Changes in v2: - Removed obsolete comment (Jeykumar) Cc: Jeykumar Sankaran Signed-off-by: Sean Paul --- Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 24 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 - 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 23094d108e81..ab8574ab8327 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -651,10 +651,6 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms) dpu_hw_intr_destroy(dpu_kms->hw_intr); dpu_kms->hw_intr = NULL; - if (dpu_kms->power_event) - dpu_power_handle_unregister_event( - &dpu_kms->phandle, dpu_kms->power_event); - /* safe to call these more than once during shutdown */ _dpu_debugfs_destroy(dpu_kms); _dpu_kms_mmu_destroy(dpu_kms); @@ -832,16 +828,6 @@ u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name) return clk_get_rate(clk->clk); } -static void dpu_kms_handle_power_event(u32 event_type, void *usr) -{ - struct dpu_kms *dpu_kms = usr; - - if (!dpu_kms) - return; - - dpu_vbif_init_memtypes(dpu_kms); -} - static int dpu_kms_hw_init(struct msm_kms *kms) { struct dpu_kms *dpu_kms; @@ -1012,13 +998,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) */ dev->mode_config.allow_fb_modifiers = true; - /* -* Handle (re)initializations during power enable -*/ - dpu_kms_handle_power_event(DPU_POWER_EVENT_ENABLE, dpu_kms); - dpu_kms->power_event = dpu_power_handle_register_event( - &dpu_kms->phandle, DPU_POWER_EVENT_ENABLE, - dpu_kms_handle_power_event, dpu_kms, "kms"); + dpu_vbif_init_memtypes(dpu_kms); pm_runtime_put_sync(&dpu_kms->pdev->dev); @@ -1172,6 +1152,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) return rc; } + dpu_vbif_init_memtypes(dpu_kms); + rc = dpu_power_resource_enable(&dpu_kms->phandle, true); if (rc) DPU_ERROR("resource enable failed: %d\n", rc); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index f2c78deb0854..5f08be187c86 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -114,7 +114,6 @@ struct dpu_kms { struct dpu_mdss_cfg *catalog; struct dpu_power_handle phandle; - struct dpu_power_event *power_event; /* directory entry for debugfs */ struct dentry *debugfs_root; -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
On 2018-11-26 13:53, Sean Paul wrote: On Mon, Nov 19, 2018 at 12:03:53PM -0800, Jeykumar Sankaran wrote: On 2018-11-16 13:14, Sean Paul wrote: > On Fri, Nov 16, 2018 at 12:05:09PM -0800, Jeykumar Sankaran wrote: > > On 2018-11-16 10:42, Sean Paul wrote: > > > From: Sean Paul > > > > > > It's for legacy drivers, for atomic drivers crtc->state->encoder_mask > > > should be used to map encoder to crtc. > > > > > > Changes in v2: > > > - None > > > > > > Signed-off-by: Sean Paul > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 +++--- > > > 2 files changed, 29 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > index 156f4c77ca44..a008a87a8113 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct > > > drm_crtc *crtc) > > > return INTF_MODE_NONE; > > > } > > > > > > -drm_for_each_encoder(encoder, crtc->dev) > > > -if (encoder->crtc == crtc) > > > -return dpu_encoder_get_intf_mode(encoder); > > > +/* TODO: Returns the first INTF_MODE, could there be multiple > > > values? */ > > > +drm_for_each_encoder_mask(encoder, crtc->dev, > > > crtc->state->encoder_mask) > > > +return dpu_encoder_get_intf_mode(encoder); > > > > > > return INTF_MODE_NONE; > > > } > > > @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc > > > *crtc, > > > spin_unlock_irqrestore(&dev->event_lock, flags); > > > } > > > > > > -list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > > > { > > > -if (encoder->crtc != crtc) > > > -continue; > > > - > > > -/* encoder will trigger pending mask now */ > > > +/* encoder will trigger pending mask now */ > > > +drm_for_each_encoder_mask(encoder, crtc->dev, > > > crtc->state->encoder_mask) > > > dpu_encoder_trigger_kickoff_pending(encoder); > > > -} > > > > > > /* > > > * If no mixers have been allocated in dpu_crtc_atomic_check(), > > > @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct > > > drm_crtc *crtc) > > > void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) > > > { > > > struct drm_encoder *encoder; > > > -struct drm_device *dev = crtc->dev; > > > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > > > struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > > > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > > > @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc > > > *crtc) > > > > > > DPU_ATRACE_BEGIN("crtc_commit"); > > > > > > -list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > > > { > > > +/* > > > + * Encoder will flush/start now, unless it has a tx pending. If > > > so, it > > > + * may delay and flush at an irq event (e.g. ppdone) > > > + */ > > > +drm_for_each_encoder_mask(encoder, crtc->dev, > > > + crtc->state->encoder_mask) { > > > struct dpu_encoder_kickoff_params params = { 0 }; > > > - > > > -if (encoder->crtc != crtc) > > > -continue; > > > - > > > -/* > > > - * Encoder will flush/start now, unless it has a tx > > > pending. > > > - * If so, it may delay and flush at an irq event (e.g. > > > ppdone) > > > - */ > > > dpu_encoder_prepare_for_kickoff(encoder, ¶ms); > > > } > > > > > > @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc > *crtc) > > > > > > dpu_vbif_clear_errors(dpu_kms); > > > > > > -list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > > > { > > > -if (encoder->crtc != crtc) > > > -continue; > > > - > > > +drm_for_each_encoder_mask(encoder, crtc->dev, > > > crtc->state->encoder_mask) > > > dpu_encoder_kickoff(encoder); > > > -} > > We wont be holding the modeset locks here (and in crtc_atomic_begin) > > in > the > > display thread. Is > > it safe to iterate over encoder_mask? > > Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for > dpu_crtc_commit_kickoff(): > > 1- dpu_kms_encoder_enable() which is called via the > encoder->funcs->enable > hook > 2- dpu_kms_commit() which is called in the > mode_config->funcs->atomic_commit_tail atomic_commit_tail will be called from the WQ. Do we hold the modeset locks in the WQ? I believe userspace thread will dr
[Freedreno] [PATCH] drm/msm/a6xx: Remove unwanted regulator code
The GMU code currently has some misguided code to try to work around a hardware quirk that requires the power domains on the GPU be collapsed in a certain order. Future changes will do this the right way so get rid of the unused and unwanted regulator code. Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 -- 2 files changed, 6 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index c58e953fefa3..4a989d4e65f9 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -671,9 +671,6 @@ int a6xx_gmu_reset(struct a6xx_gpu *a6xx_gpu) gmu_poll_timeout(gmu, REG_A6XX_RSCC_TCS3_DRV0_STATUS, val, (val & 1), 100, 1000); - /* Force off the GX GSDC */ - regulator_force_disable(gmu->gx); - /* Disable the resources */ clk_bulk_disable_unprepare(gmu->nr_clocks, gmu->clocks); pm_runtime_put_sync(gmu->dev); @@ -1214,7 +1211,6 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node) gmu->idle_level = GMU_IDLE_STATE_ACTIVE; pm_runtime_enable(gmu->dev); - gmu->gx = devm_regulator_get(gmu->dev, "vdd"); /* Get the list of clocks */ ret = a6xx_gmu_clocks_probe(gmu); diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h index c721d9165d8e..8081083cd062 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h @@ -52,8 +52,6 @@ struct a6xx_gmu { int hfi_irq; int gmu_irq; - struct regulator *gx; - struct iommu_domain *domain; u64 uncached_iova_base; -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/a6xx: Add a name for the crashdumper buffer
Add a buffer object name for the a6xx crashdumper so it can be seen with the changes introduced by 7799a98edd ("drm/msm: Add a name field for gem objects"). Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c index 716595b664dd..e686331fa089 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c @@ -116,7 +116,10 @@ static int a6xx_crashdumper_init(struct msm_gpu *gpu, SZ_1M, MSM_BO_UNCACHED, gpu->aspace, &dumper->bo, &dumper->iova); - return IS_ERR(dumper->ptr) ? PTR_ERR(dumper->ptr) : 0; + if (!IS_ERR(dumper->ptr)) + msm_gem_object_set_name(dumper->bo, "crashdump"); + + return PTR_ERR_OR_ZERO(dumper->ptr); } static int a6xx_crashdumper_run(struct msm_gpu *gpu, -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/a6xx: Use new kernel API free function for gpu state
dadb36b7ec42 ("drm/msm: Add a common function to free kernel buffer objects") missed freeing the crashdumper state for a6xx. Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 11 +-- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c index df6308e7ea67..716595b664dd 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c @@ -149,15 +149,6 @@ static int a6xx_crashdumper_run(struct msm_gpu *gpu, return ret; } -static void a6xx_crashdumper_free(struct msm_gpu *gpu, - struct a6xx_crashdumper *dumper) -{ - msm_gem_unpin_iova(dumper->bo, gpu->aspace); - msm_gem_put_vaddr(dumper->bo); - - drm_gem_object_unreference(dumper->bo); -} - /* read a value from the GX debug bus */ static int debugbus_read(struct msm_gpu *gpu, u32 block, u32 offset, u32 *data) @@ -900,7 +891,7 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu) a6xx_get_clusters(gpu, a6xx_state, &dumper); a6xx_get_dbgahb_clusters(gpu, a6xx_state, &dumper); - a6xx_crashdumper_free(gpu, &dumper); + msm_gem_kernel_put(dumper.bo, gpu->aspace, true); } a6xx_get_debugbus(gpu, a6xx_state); -- 2.18.0 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm/dpu: add display port support in DPU
On Tue, Nov 27, 2018 at 02:28:30PM -0800, Jeykumar Sankaran wrote: > Add display port support in DPU by creating hooks > for DP encoder enumeration and encoder mode > initialization. > > This change is based on the SDM845 Display port > driver changes[1]. > > changes in v2: > - rebase on [2] (Sean Paul) > - remove unwanted error checks and > switch cases (Jordan Crouse) > > [1] https://lwn.net/Articles/768265/ > [2] https://lkml.org/lkml/2018/11/17/87 > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 ++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 47 > + > 2 files changed, 45 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index d3f4501..1f6b4b1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -2015,7 +2015,7 @@ static int dpu_encoder_setup_display(struct > dpu_encoder_virt *dpu_enc, > { > int ret = 0; > int i = 0; > - enum dpu_intf_type intf_type; > + enum dpu_intf_type intf_type = INTF_NONE; dpu_intf_type seems unnecessary, you could just use the DRM_MODE_ENCODER_* value directly? > struct dpu_enc_phys_init_params phys_params; > > if (!dpu_enc || !dpu_kms) { > @@ -2038,9 +2038,9 @@ static int dpu_encoder_setup_display(struct > dpu_encoder_virt *dpu_enc, > case DRM_MODE_ENCODER_DSI: > intf_type = INTF_DSI; > break; > - default: > - DPU_ERROR_ENC(dpu_enc, "unsupported display interface type\n"); > - return -EINVAL; > + case DRM_MODE_ENCODER_TMDS: > + intf_type = INTF_DP; > + break; > } > > WARN_ON(disp_info->num_of_h_tiles < 1); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 985c855..7d931ae 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -473,6 +473,32 @@ static void _dpu_kms_initialize_dsi(struct drm_device > *dev, > } > } > > +static void _dpu_kms_initialize_displayport(struct drm_device *dev, > + struct msm_drm_private *priv, > + struct dpu_kms *dpu_kms) > +{ > + struct drm_encoder *encoder = NULL; > + int rc; > + > + if (!priv->dp) > + return; > + > + encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS); > + if (IS_ERR(encoder)) { > + DPU_ERROR("encoder init failed for dsi display\n"); > + return; > + } > + > + rc = msm_dp_modeset_init(priv->dp, dev, encoder); > + if (rc) { > + DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); > + drm_encoder_cleanup(encoder); > + return; > + } > + > + priv->encoders[priv->num_encoders++] = encoder; No need to keep track of drm resources at the driver level, the core will do this for you. So can you please add a patch preceding this one to remove the priv->encoders/crtc/planes/connectors arrays? > +} > + > /** > * _dpu_kms_setup_displays - create encoders, bridges and connectors > * for underlying displays > @@ -487,6 +513,8 @@ static void _dpu_kms_setup_displays(struct drm_device > *dev, Why are these functions voids? Seems like there are plenty of places for them to fail :) Let's add a patch to the beginning of this series to properly handle failures in setup_displays and initialize_dsi > { > _dpu_kms_initialize_dsi(dev, priv, dpu_kms); > > + _dpu_kms_initialize_displayport(dev, priv, dpu_kms); > + > /** >* Extend this function to initialize other >* types of displays > @@ -723,13 +751,20 @@ static void _dpu_kms_set_encoder_mode(struct msm_kms > *kms, > info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE : > MSM_DISPLAY_CAP_VID_MODE; > > - /* TODO: No support for DSI swap */ > - for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { > - if (priv->dsi[i]) { > - info.h_tile_instance[info.num_of_h_tiles] = i; > - info.num_of_h_tiles++; > + switch (info.intf_type) { > + case DRM_MODE_ENCODER_DSI: > + /* TODO: No support for DSI swap */ > + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { > + if (priv->dsi[i]) { > + info.h_tile_instance[info.num_of_h_tiles] = i; > + info.num_of_h_tiles++; > + } > } > - } > + break; > + case DRM_MODE_ENCODER_TMDS: > + info.num_of_h_tiles = 1; > + break; > + }; > > rc = dpu_encoder_setup(encoder->dev, encoder, &info); > if (rc) > -- > The Qualcomm Innovatio
Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy wrote: > > Hi Rob, > > On 01/12/2018 16:53, Rob Clark wrote: > > 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. > > Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it > really shouldn't. > > > 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. > > s/solved/worked around/ > > If you want a guarantee of actually getting a specific hardware context > allocated for a given domain, there needs to be code in the IOMMU driver > to understand and honour that. Implicitly depending on whatever happens > to fall out of current driver behaviour on current systems is not a real > solution. > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in > > of_dma_configure > > That's rather misleading, since the crash described above depends on at > least two other major changes which came long after that commit. > > It's not that I don't understand exactly what you want here - just that > this commit message isn't a coherent justification for that ;) > > > 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 > >
Re: [Freedreno] [PATCH v2 5/5] drm/msm: subclass work object for vblank events
On Fri, Nov 30, 2018 at 04:21:15PM -0800, Jeykumar Sankaran wrote: > On 2018-11-30 12:07, Sean Paul wrote: > > On Fri, Nov 30, 2018 at 11:45:55AM -0800, Jeykumar Sankaran wrote: > > > On 2018-11-29 14:15, Sean Paul wrote: > > > > On Tue, Nov 20, 2018 at 02:04:14PM -0800, Jeykumar Sankaran wrote: > > > > > On 2018-11-07 07:55, Sean Paul wrote: > > > > > > On Tue, Nov 06, 2018 at 02:36:30PM -0800, Jeykumar Sankaran wrote: > > > > > > > msm maintains a separate structure to define vblank > > > > > > > work definitions and a list to track events submitted > > > > > > > to the workqueue. We can avoid this redundant list > > > > > > > and its protection mechanism, if we subclass the > > > > > > > work object to encapsulate vblank event parameters. > > > > > > > > > > > > > > changes in v2: > > > > > > > - subclass optimization on system wq (Sean Paul) > > > > > > > > > > > > I wouldn't do it like this, tbh. One problem is that you've lost > > your > > > > > > flush() on > > > > > > unbind, so there's no way to know if you have workers in the wild > > > > > > waiting > > > > > > to > > > > > > enable/disable vblank. > > > > > > > > > > > > Another issues is that AFAICT, we don't need a queue of > > > > > > enables/disables, > > > > > > but > > > > > > rather just the last requested state (ie: should we be on or off). > > So > > > > > > things > > > > > > don't need to be this complicated (and we're possibly thrashing > > vblank > > > > > > on/off > > > > > > for no reason). > > > > > > > > > > > > I'm still of the mind that you should just make this synchronous > > and > > > > be > > > > > > done > > > > > > with the threads (especially since we're still > > uncovering/introducing > > > > > > races!). > > > > > > > > > > > While scoping out the effort to make vblank events synchronous, I > > > > > found > > > > > that the spinlock locking order of vblank request sequence and > > vblank > > > > > callback > > > > > sequences are the opposite. > > > > > > > > > > In DPU, drm_vblank_enable acquires vblank_time_lock before > > registering > > > > > the crtc to encoder which happens after acquiring encoder_spinlock. > > > > > But > > > > > the vblank_callback acquires encoder_spinlock before accessing the > > > > > registered > > > > > crtc and calling into drm_vblank_handler which tries to acquire > > > > > vblank_time_lock. > > > > > Acquiring both vblank_time_lock and encoder_spinlock in the same > > > > > thread > > > > > is leading to deadlock. > > > > > > > > Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps > > > > with > > > > enable/disable? I hacked in sync vblank enable/disable quickly to see > > if > > > > I > > > > could > > > > reproduce what you're seeing, but things seemed well behaved. > > > > > > > > > > The race is between drm_vblank_get/put and vblank_handler contexts. > > > > > > When made synchronous: > > > > > > while calling drm_vblank_get, the callstack looks like below: > > > drm_vblank_get -> drm_vblank_enable (acquires vblank_time_lock) -> > > > __enable_vblank -> dpu_crtc_vblank -> > > > dpu_encoder_toggle_vblank_for_crtc > > > (tries to acquire enc_spinlock) > > > > > > In vblank handler, the call stack will be: > > > dpu_encoder_phys_vid_vblank_irq -> dpu_encoder_vblank_callback > > > (acquires > > > enc_spinlock) -> dpu_crtc_vblank_callback -> drm_handle_vblank > > > (tries to > > > acquire vblank_time_lock) > > > > Hmm, I'm not sure how this can happen. We acquire and release the > > enc_spinlock > > before enabling the irq, yes we will hold on to the vbl_time_lock, but > > we > > shouldn't be trying to reacquire an encoder's spinlock after we've > > enabled > > it. > In the synchronous approach dpu_encoder_toggle_vblank_for_crtc(which > acquires the enc_spinlock) will be called while we > are holding the vbl_time_lock. > > > I don't know how that can deadlock, since we should never be running > > enable and > > the handler concurrently. > > > I agree that vblank_irq handler should not be running before the enable > sequence. But > don't you expect the handler to be running while calling the vblank_disable > sequence? This is an entirely different problem though. It's also one that is easier to fix. I think we could probably grab the enc_spinlock in disable and clear the crtc pointer. What I'm getting at is that there's no fundamental reason why we need to have async vblank enable/disable. Sean > vbl disable will try to acquire the locks in the opposite order to that of > irq_handler and the > same issue is bound to happen. > > With your patch, you should be able to simulate this deadlock if you can > inject a delay > by adding a pr_err log in vblank_ctrl_queue_work > > Thanks, > Jeykumar S. > > > The only thing I can think of is that the vblank interrupts are firing > > after > > vblank has been disabled? In that case, it seems like we should properly > > flush > > them. > > > > Sean > > > > > > > > > > > > > > I do see that there is a chance to
Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy wrote: > > Hi Rob, > > On 01/12/2018 16:53, Rob Clark wrote: > > 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. > > Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it > really shouldn't. for this hw, I'm still on 4.19, although that does look like it would avoid the issue. > > 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. > > s/solved/worked around/ > > If you want a guarantee of actually getting a specific hardware context > allocated for a given domain, there needs to be code in the IOMMU driver > to understand and honour that. Implicitly depending on whatever happens > to fall out of current driver behaviour on current systems is not a real > solution. ok, fair.. but I'll settle for "works" in the absence of better options.. At some level, it would be nice to be able to optionally specify a context-bank in the iommu bindings. But not sure how to make that fit w/ cb allocated per domain. And I assume I'm the only one who has this problem? > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in > > of_dma_configure > > That's rather misleading, since the crash described above depends on at > least two other major changes which came long after that commit. Fair, when I realized it was the difference in where the iommu attaches between dpu1 (sdm845) and everything coming before, I should have removed the tag. BR, -R > It's not that I don't understand exactly what you want here - just that > this commit message isn't a coheren
Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
Hi Rob, On 01/12/2018 16:53, Rob Clark wrote: 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. Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it really shouldn't. 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. s/solved/worked around/ If you want a guarantee of actually getting a specific hardware context allocated for a given domain, there needs to be code in the IOMMU driver to understand and honour that. Implicitly depending on whatever happens to fall out of current driver behaviour on current systems is not a real solution. Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure That's rather misleading, since the crash described above depends on at least two other major changes which came long after that commit. It's not that I don't understand exactly what you want here - just that this commit message isn't a coherent justification for that ;) 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(&ofdev->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 configurati
Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops
Hi Tomasz, On 2018-12-03 01:10, Tomasz Figa wrote: > On Sat, Dec 1, 2018 at 8:54 AM Rob Clark wrote: >> 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(&ofdev->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 >> +* int