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

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

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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno