Re: [PATCH 2/3] drm/msm/dpu: Integrate interconnect API in MDSS

[skolluku-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2018-10-08 20:09, Jordan Crouse wrote:
> On Mon, Oct 08, 2018 at 02:57:29PM +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.
>> 
>> Signed-off-by: Sravanthi Kollukuduru <skolluku@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 56 
>> +++++++++++++++++++++++++++++---
>>  1 file changed, 52 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..8391e5c1e559 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 <linux/interconnect.h>
>> 
>>  #define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
>> 
>>  #define HW_INTR_STATUS			0x0010
>> +#define MAX_AXI_PORT_COUNT 3
>> 
>>  struct dpu_mdss {
>>  	struct msm_mdss base;
>> @@ -16,8 +18,36 @@ struct dpu_mdss {
>>  	u32 hwversion;
>>  	struct dss_module_power mp;
>>  	struct dpu_irq_controller irq_controller;
>> +	struct icc_path *path[MAX_AXI_PORT_COUNT];
>> +	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++;
>> +	}
>> +
>> +	if (total_num_paths  > MAX_AXI_PORT_COUNT)
>> +		return -EINVAL;
> 
> Since you are using fixed names you know that by design this isn't 
> possible.
> 
Ok, will remove the check and the macro.

>> +	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 +157,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 +175,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 +193,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 +202,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 +245,12 @@ 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) {
>> +		DPU_ERROR("failed to parse icc path, ret=%d\n", ret);
> 
> Depending on the ordering between DPU and interconnect, this might 
> return
> -EPROBE_DEFER for a few times before working. You should skip printing
> an error message on -EPROBE_DEFER so it doesn't spam the log.
> 
Ok, will remove it.
>> +		return ret;
>> +	}
>> +
>>  	mp = &dpu_mdss->mp;
>>  	ret = msm_dss_parse_clock(pdev, mp);
>>  	if (ret) {
>> @@ -224,14 +272,14 @@ int dpu_mdss_init(struct drm_device *dev)
>>  		goto irq_error;
>>  	}
>> 
>> +	priv->mdss = &dpu_mdss->base;
>> +
> 
> This seems like it might have snuck in from a different patch?
No, this was included as part of this patch to ensure the mdss structure 
is
intialized before the first pm runtime resume call. If not, the 
dpu_mdss_enable
will not be called which has the logic to vote for bw and subsequently, 
enable
the clocks.
> 
>>  	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:
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno