| Message ID | 20181010092434.853-3-skolluku@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use interconnect API in MDSS on SDM845 | expand |
Hi Sravanthi, On Wed, Oct 10, 2018 at 02:54:33PM +0530, Sravanthi Kollukuduru wrote: > 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) > > Signed-off-by: Sravanthi Kollukuduru <skolluku@codeaurora.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 +++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > index 2235ef8129f4..27c2594e5133 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > @@ -4,6 +4,7 @@ > */ > > #include "dpu_kms.h" > +#include <linux/interconnect.h> > > #define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base) > > @@ -16,8 +17,33 @@ 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"); > + int total_num_paths = 0; initialization is not needed nit: the 'total_' prefix doesn't add any value here, just 'num_paths' would be easier to parse. Actually you could get rid of the variable completely by just initializing and incrementing dpu_mdss->num_paths. > static irqreturn_t dpu_mdss_irq(int irq, void *arg) > { > struct dpu_mdss *dpu_mdss = arg; > @@ -127,7 +153,12 @@ 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 ab = (dpu_mdss->num_paths) ? 6800000000/dpu_mdss->num_paths : 0; parentheses are not needed > + u64 ib = 6800000000; There should probably be a define for 6800000000. nit: names like avg_bw and peak_pw would be clearer than ab and ib, without being excessively verbose. Cheers Matthias
On 2018-10-24 22:09, Matthias Kaehlcke wrote: > Hi Sravanthi, > > On Wed, Oct 10, 2018 at 02:54:33PM +0530, Sravanthi Kollukuduru wrote: >> 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) >> >> Signed-off-by: Sravanthi Kollukuduru <skolluku@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 >> +++++++++++++++++++++++++++++--- >> 1 file changed, 46 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c >> index 2235ef8129f4..27c2594e5133 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c >> @@ -4,6 +4,7 @@ >> */ >> >> #include "dpu_kms.h" >> +#include <linux/interconnect.h> >> >> #define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base) >> >> @@ -16,8 +17,33 @@ 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"); >> + int total_num_paths = 0; > > initialization is not needed > > nit: the 'total_' prefix doesn't add any value here, just 'num_paths' > would be easier to parse. Actually you could get rid of the variable > completely by just initializing and incrementing dpu_mdss->num_paths. > Okay. >> static irqreturn_t dpu_mdss_irq(int irq, void *arg) >> { >> struct dpu_mdss *dpu_mdss = arg; >> @@ -127,7 +153,12 @@ 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 ab = (dpu_mdss->num_paths) ? 6800000000/dpu_mdss->num_paths : 0; > > parentheses are not needed > >> + u64 ib = 6800000000; > > There should probably be a define for 6800000000. > Okay. > nit: names like avg_bw and peak_pw would be clearer than ab and ib, > without being excessively verbose. > Sure, will make the changes. > Cheers > > Matthias > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c index 2235ef8129f4..27c2594e5133 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c @@ -4,6 +4,7 @@ */ #include "dpu_kms.h" +#include <linux/interconnect.h> #define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base) @@ -16,8 +17,33 @@ 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"); + int total_num_paths = 0; + + if (IS_ERR(path0)) + return PTR_ERR(path0); + + dpu_mdss->path[0] = path0; + total_num_paths = 1; + + if (!IS_ERR(path1)) { + dpu_mdss->path[1] = path1; + total_num_paths++; + } + + dpu_mdss->num_paths = total_num_paths; + + return 0; +} + static irqreturn_t dpu_mdss_irq(int irq, void *arg) { struct dpu_mdss *dpu_mdss = arg; @@ -127,7 +153,12 @@ 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 ab = (dpu_mdss->num_paths) ? 6800000000/dpu_mdss->num_paths : 0; + u64 ib = 6800000000; + + for (i = 0; i < dpu_mdss->num_paths; i++) + icc_set(dpu_mdss->path[i], ab, ib); ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true); if (ret) @@ -140,12 +171,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 +189,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; _dpu_mdss_irq_domain_fini(dpu_mdss); @@ -163,6 +198,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; @@ -203,6 +241,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) { @@ -224,14 +266,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 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) Signed-off-by: Sravanthi Kollukuduru <skolluku@codeaurora.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 50 +++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 4 deletions(-)