Re: [Freedreno] [RESEND PATCH v17 2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
Hi Will,
On Wed, Nov 21, 2018 at 11:09 PM Will Deacon wrote:
>
> On Fri, Nov 16, 2018 at 04:54:27PM +0530, Vivek Gautam wrote:
> > From: Sricharan R
> >
> > The smmu device probe/remove and add/remove master device callbacks
> > gets called when the smmu is not linked to its master, that is without
> > the context of the master device. So calling runtime apis in those places
> > separately.
> > Global locks are also initialized before enabling runtime pm as the
> > runtime_resume() calls device_reset() which does tlb_sync_global()
> > that ultimately requires locks to be initialized.
> >
> > 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
> > ---
> > drivers/iommu/arm-smmu.c | 101
> > ++-
> > 1 file changed, 91 insertions(+), 10 deletions(-)
>
> Given that you're doing the get/put in the TLBI ops unconditionally:
>
> > 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->tlb_ops->tlb_flush_all(smmu_domain);
> > + arm_smmu_rpm_put(smmu);
> > + }
> > }
> >
> > static void arm_smmu_iotlb_sync(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->tlb_ops->tlb_sync(smmu_domain);
> > + arm_smmu_rpm_put(smmu);
> > + }
>
> Why do you need them around the map/unmap calls as well?
We still have .tlb_add_flush path?
Thanks
Vivek
>
> Will
> ___
> iommu mailing list
> io...@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
--
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 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*
Hi Tomasz, Jordan,
On 11/21/2018 9:18 AM, Tomasz Figa wrote:
Hi Jordan, Vivek,
On Wed, Nov 21, 2018 at 12:41 AM Jordan Crouse wrote:
On Tue, Nov 20, 2018 at 03:24:37PM +0530, Vivek Gautam wrote:
dma_map_sg() expects a DMA domain. However, the drm devices
have been traditionally using unmanaged iommu domain which
is non-dma type. Using dma mapping APIs with that domain is bad.
Replace dma_map_sg() calls with dma_sync_sg_for_device{|cpu}()
to do the cache maintenance.
Signed-off-by: Vivek Gautam
Suggested-by: Tomasz Figa
---
Tested on an MTP sdm845:
https://github.com/vivekgautam1/linux/tree/v4.19/sdm845-mtp-display-working
drivers/gpu/drm/msm/msm_gem.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 00c795ced02c..d7a7af610803 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -81,6 +81,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
struct drm_device *dev = obj->dev;
struct page **p;
int npages = obj->size >> PAGE_SHIFT;
+ struct scatterlist *s;
+ int i;
if (use_pages(obj))
p = drm_gem_get_pages(obj);
@@ -107,9 +109,19 @@ static struct page **get_pages(struct drm_gem_object *obj)
/* For non-cached buffers, ensure the new pages are clean
* because display controller, GPU, etc. are not coherent:
*/
- if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
- dma_map_sg(dev->dev, msm_obj->sgt->sgl,
- msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+ if (msm_obj->flags & (MSM_BO_WC | MSM_BO_UNCACHED)) {
+ /*
+ * Fake up the SG table so that dma_sync_sg_*()
+ * can be used to flush the pages associated with it.
+ */
We aren't really faking. The table is real, we are just slightly abusing the
sg_dma_address() which makes this comment a bit misleading. Instead I would
probably say something like:
/* dma_sync_sg_* flushes pages using sg_dma_address() so point it at the
* physical page for the right behavior */
Or something like that.
It's actually quite complicated, but I agree that the comment isn't
very precise. The cases are as follows:
- arm64 iommu_dma_ops use sg_phys()
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm64/mm/dma-mapping.c#L599
- swiotlb_dma_ops used on arm64 if no IOMMU is available use
sg->dma_address directly:
https://elixir.bootlin.com/linux/v4.20-rc3/source/kernel/dma/swiotlb.c#L832
- arm_dma_ops use sg_dma_address():
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1130
- arm iommu_ops use sg_page():
https://elixir.bootlin.com/linux/v4.20-rc3/source/arch/arm/mm/dma-mapping.c#L1869
Sounds like a mess...
Thanks for the review.
Technically with the below assignment we address all of the above. How
about an even
simpler version of the suggested comment:
/* dma_sync_sg_* flushes physical pages, so point sg->dma_address to
* the physical one for the right behavior.
*/
+ for_each_sg(msm_obj->sgt->sgl, s,
+ msm_obj->sgt->nents, i)
+ sg_dma_address(s) = sg_phys(s);
+
I'm wondering - wouldn't we want to do this association for cached buffers to so
we could sync them correctly in cpu_prep and cpu_fini? Maybe it wouldn't hurt
to put this association in the main path (obviously the sync should stay inside
the conditional for uncached buffers).
Sure, I will move this out of the conditional check.
I guess it wouldn't hurt indeed. Note that cpu_prep/fini seem to be
missing the sync call currently.
I can't say I understand the usage of cpu_prep and cpu_fini(). But I can add
the necessary support if you can point me in the right direction.
Thanks
Best regards
Vivek
P.S. Jordan, not sure if it's my Gmail or your email client, but your
message had all the recipients in a Reply-to header, except you, so
pressing Reply to all in my case led to a message that didn't have you
in recipients anymore...
Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 2/3] drm/msm/dpu: Integrate interconnect API in MDSS
The interconnect framework is designed to provide a
standard kernel interface to control the settings of
the interconnects on a SoC.
The interconnect API uses a consumer/provider-based model,
where the providers are the interconnect buses and the
consumers could be various drivers.
MDSS is one of the interconnect consumers which uses the
interconnect APIs to get the path between endpoints and
set its bandwidth/latency/QoS requirements for the given
interconnected path.
Changes in v2:
- Remove error log and unnecessary check (Jordan Crouse)
Changes in v3:
- Code clean involving variable name change, removal
of extra paranthesis and variables (Matthias Kaehlcke)
Signed-off-by: Sravanthi Kollukuduru
---
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 49
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index 38576f8b90b6..1387a6b1b39e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -4,10 +4,12 @@
*/
#include "dpu_kms.h"
+#include
#define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
-#define HW_INTR_STATUS 0x0010
+#define HW_INTR_STATUS 0x0010
+#define MAX_BW 680
struct dpu_mdss {
struct msm_mdss base;
@@ -16,8 +18,30 @@ struct dpu_mdss {
u32 hwversion;
struct dss_module_power mp;
struct dpu_irq_controller irq_controller;
+ struct icc_path *path[2];
+ u32 num_paths;
};
+static int dpu_mdss_parse_data_bus_icc_path(
+ struct drm_device *dev, struct dpu_mdss *dpu_mdss)
+{
+ struct icc_path *path0 = of_icc_get(dev->dev, "port0");
+ struct icc_path *path1 = of_icc_get(dev->dev, "port1");
+
+ if (IS_ERR(path0))
+ return PTR_ERR(path0);
+
+ dpu_mdss->path[0] = path0;
+ dpu_mdss->num_paths = 1;
+
+ if (!IS_ERR(path1)) {
+ dpu_mdss->path[1] = path1;
+ dpu_mdss->num_paths++;
+ }
+
+ return 0;
+}
+
static irqreturn_t dpu_mdss_irq(int irq, void *arg)
{
struct dpu_mdss *dpu_mdss = arg;
@@ -127,7 +151,11 @@ static int dpu_mdss_enable(struct msm_mdss *mdss)
{
struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
struct dss_module_power *mp = &dpu_mdss->mp;
- int ret;
+ int ret, i;
+ u64 avg_bw = dpu_mdss->num_paths ? MAX_BW/dpu_mdss->num_paths : 0;
+
+ for (i = 0; i < dpu_mdss->num_paths; i++)
+ icc_set(dpu_mdss->path[i], avg_bw, MAX_BW);
ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
if (ret)
@@ -140,12 +168,15 @@ static int dpu_mdss_disable(struct msm_mdss *mdss)
{
struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
struct dss_module_power *mp = &dpu_mdss->mp;
- int ret;
+ int ret, i;
ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
if (ret)
DPU_ERROR("clock disable failed, ret:%d\n", ret);
+ for (i = 0; i < dpu_mdss->num_paths; i++)
+ icc_set(dpu_mdss->path[i], 0, 0);
+
return ret;
}
@@ -155,6 +186,7 @@ static void dpu_mdss_destroy(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
struct dss_module_power *mp = &dpu_mdss->mp;
+ int i;
pm_runtime_disable(dev->dev);
_dpu_mdss_irq_domain_fini(dpu_mdss);
@@ -162,6 +194,9 @@ static void dpu_mdss_destroy(struct drm_device *dev)
msm_dss_put_clk(mp->clk_config, mp->num_clk);
devm_kfree(&pdev->dev, mp->clk_config);
+ for (i = 0; i < dpu_mdss->num_paths; i++)
+ icc_put(dpu_mdss->path[i]);
+
if (dpu_mdss->mmio)
devm_iounmap(&pdev->dev, dpu_mdss->mmio);
dpu_mdss->mmio = NULL;
@@ -200,6 +235,10 @@ int dpu_mdss_init(struct drm_device *dev)
}
dpu_mdss->mmio_len = resource_size(res);
+ ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+ if (ret)
+ return ret;
+
mp = &dpu_mdss->mp;
ret = msm_dss_parse_clock(pdev, mp);
if (ret) {
@@ -221,14 +260,14 @@ int dpu_mdss_init(struct drm_device *dev)
goto irq_error;
}
+ priv->mdss = &dpu_mdss->base;
+
pm_runtime_enable(dev->dev);
pm_runtime_get_sync(dev->dev);
dpu_mdss->hwversion = readl_relaxed(dpu_mdss->mmio);
pm_runtime_put_sync(dev->dev);
- priv->mdss = &dpu_mdss->base;
-
return ret;
irq_error:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 0/3] Use interconnect API in MDSS on SDM845
The interconnect API provides an interface for consumer drivers to express their bandwidth needs in the SoC. This data is aggregated and the on-chip interconnect hardware is configured to the appropriate power/performance profile. MDSS is one of the interconnect consumers which uses the interconnect APIs to get the path between endpoints and set its bandwidth requirements for the given interconnected path. Subsequently, there is a clean up patch to remove all the references of the DPU custom bus scaling. There is corresponding DT patch with the source and destination ports defined for display driver which will be sent separately. Changes in v2: - Remove error log and unnecessary check (Jordan Crouse) - Fixed build error due to partial clean up Changes in v3: - Remove common property definitions (Rob Herring) - Code clean up involving variable name change, removal of extra paranthesis and variables (Matthias Kaehlcke) - Condense multiple lines into a single line (Sean Paul) Sravanthi Kollukuduru (3): drm/msm/dpu: clean up references of DPU custom bus scaling drm/msm/dpu: Integrate interconnect API in MDSS dt-bindings: msm/disp: Introduce interconnect bindings for MDSS on SDM845 .../devicetree/bindings/display/msm/dpu.txt| 9 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 174 - drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 +- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 49 +- drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 47 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 68 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 22 +-- 8 files changed, 142 insertions(+), 244 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 3/3] dt-bindings: msm/disp: Introduce interconnect bindings for MDSS on SDM845
Add interconnect properties such as interconnect provider specifier , the edge source and destination ports which are required by the interconnect API to configure interconnect path for MDSS. Changes in v2: - none Changes in v3: - Remove common property definitions (Rob Herring) Signed-off-by: Sravanthi Kollukuduru --- Documentation/devicetree/bindings/display/msm/dpu.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt index ad2e8830324e..d75b4360a4be 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu.txt +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt @@ -28,6 +28,11 @@ Required properties: - #address-cells: number of address cells for the MDSS children. Should be 1. - #size-cells: Should be 1. - ranges: parent bus address space is the same as the child bus address space. +- interconnects : interconnect path specifier for MDSS according to + Documentation/devicetree/bindings/interconnect/interconnect.txt. Should be + 2 paths corresponding to 2 AXI ports. +- interconnect-names : MDSS will have 2 port names to differentiate between the + 2 interconnect paths defined with interconnect specifier. Optional properties: - assigned-clocks: list of clock specifiers for clocks needing rate assignment @@ -86,6 +91,10 @@ Example: interrupt-controller; #interrupt-cells = <1>; + interconnects = <&qnoc 38 &qnoc 512>, + <&qnoc 39 &qnoc 512>; + interconnect-names = "port0", "port1"; + iommus = <&apps_iommu 0>; #address-cells = <2>; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v3 1/3] drm/msm/dpu: clean up references of DPU custom bus scaling
Since the upstream interconnect bus framework has landed
upstream, the existing references of custom bus scaling
needs to be cleaned up.
Changes in v2:
- Fixed build error due to partial clean up
Changes in v3:
- Condense multiple lines into a single line (Sean Paul)
Signed-off-by: Sravanthi Kollukuduru
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c| 174 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h| 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 47 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 68 -
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h| 22 +--
6 files changed, 89 insertions(+), 239 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 22e84b3d7f98..c75536ece3ed 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -84,7 +84,6 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
struct dpu_core_perf_params *perf)
{
struct dpu_crtc_state *dpu_cstate;
- int i;
if (!kms || !kms->catalog || !crtc || !state || !perf) {
DPU_ERROR("invalid parameters\n");
@@ -95,35 +94,24 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
memset(perf, 0, sizeof(struct dpu_core_perf_params));
if (!dpu_cstate->bw_control) {
- for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) {
- perf->bw_ctl[i] = kms->catalog->perf.max_bw_high *
+ perf->bw_ctl = kms->catalog->perf.max_bw_high *
1000ULL;
- perf->max_per_pipe_ib[i] = perf->bw_ctl[i];
- }
+ perf->max_per_pipe_ib = perf->bw_ctl;
perf->core_clk_rate = kms->perf.max_core_clk_rate;
} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
- for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) {
- perf->bw_ctl[i] = 0;
- perf->max_per_pipe_ib[i] = 0;
- }
+ perf->bw_ctl = 0;
+ perf->max_per_pipe_ib = 0;
perf->core_clk_rate = 0;
} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
- for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) {
- perf->bw_ctl[i] = kms->perf.fix_core_ab_vote;
- perf->max_per_pipe_ib[i] = kms->perf.fix_core_ib_vote;
- }
+ perf->bw_ctl = kms->perf.fix_core_ab_vote;
+ perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
perf->core_clk_rate = kms->perf.fix_core_clk_rate;
}
DPU_DEBUG(
- "crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu llcc_ib=%llu
llcc_ab=%llu mem_ib=%llu mem_ab=%llu\n",
+ "crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
crtc->base.id, perf->core_clk_rate,
- perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_MNOC],
- perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_MNOC],
- perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_LLCC],
- perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_LLCC],
- perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_EBI],
- perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_EBI]);
+ perf->max_per_pipe_ib, perf->bw_ctl);
}
int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
@@ -136,7 +124,6 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
struct dpu_crtc_state *dpu_cstate;
struct drm_crtc *tmp_crtc;
struct dpu_kms *kms;
- int i;
if (!crtc || !state) {
DPU_ERROR("invalid crtc\n");
@@ -158,31 +145,25 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
/* obtain new values */
_dpu_core_perf_calc_crtc(kms, crtc, state, &dpu_cstate->new_perf);
- for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC;
- i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) {
- bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl[i];
- curr_client_type = dpu_crtc_get_client_type(crtc);
+ bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl;
+ 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) &&
- (dpu_crtc_get_client_type(tmp_crtc) ==
- curr_client_type) &&
- (tmp_crtc != crtc)) {
- struct dpu_crtc_state *tmp_cstate =
- to_dpu_crtc_state(tmp_crtc->state);
-
- DPU
