Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-24 Thread maowenan 


On 2019/6/22 22:00, Julia Lawall wrote:
> 
> 
> On Sat, 22 Jun 2019, maowenan wrote:
> 
>>
>>
>> On 2019/6/22 21:06, Julia Lawall wrote:
>>>
>>>
>>> On Sat, 22 Jun 2019, Mao Wenan wrote:
>>>
 There is one warning:
 drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
 drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
 but not used [-Wunused-but-set-variable]
   int ret = 0;
   ^
 amdgpu_pmu_init() is called by amdgpu_device_init() in 
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c,
 which will use the return value. So it returns 'ret' for caller.
 amdgpu_device_init()
r = amdgpu_pmu_init(adev);

 Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters")

 Signed-off-by: Mao Wenan 
 ---
  v1->v2: change the subject for this patch; change the indenting when it 
 calls init_pmu_by_type; use the value 'ret' in
  amdgpu_pmu_init().
  drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
 b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
 index 0e6dba9..145e720 100644
 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
 @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
case CHIP_VEGA20:
/* init df */
ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
 - "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
 - DF_V3_6_MAX_COUNTERS);
 + "DF", "amdgpu_df", 
 PERF_TYPE_AMDGPU_DF,
 + 
 DF_V3_6_MAX_COUNTERS);

/* other pmu types go here*/
>>>
>>> I don't know what is the impact of the other pmu types that are planned
>>> for the future.  Perhaps it would be better to abort the function
>>> immediately in the case of a failure.
>>

OK, v3 will be sent.

>> I guess it would be better to use new function or new switch case clause to 
>> process different PMU types
>> in future.
> 
> I don't know.  But normally when an error may occur it is checked for
> immediately, rather than just letting it go until the end of the function.
> But maybe the developer know what is planned for the future for this
> function.
> 
> julia
> 
>>
>>>
>>> julia
>>>
break;
 @@ -261,7 +261,7 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
return 0;
}

 -  return 0;
 +  return ret;
  }


 --
 2.7.4


>>>
>>
>>
> 

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-23 Thread Joe Perches 
On Mon, 2019-06-24 at 11:41 +0800, maowenan wrote:
> 
> On 2019/6/23 2:13, Joe Perches wrote:
> > On Sat, 2019-06-22 at 21:05 +0800, Mao Wenan wrote:
> > > There is one warning:
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ 
> > > set but not used [-Wunused-but-set-variable]
> > >   int ret = 0;
> > []
> > >  v1->v2: change the subject for this patch; change the indenting when it 
> > > calls init_pmu_by_type; use the value 'ret' in
> > >  amdgpu_pmu_init().
> > []
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> > []
> > > @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
> > >   case CHIP_VEGA20:
> > >   /* init df */
> > >   ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
> > > -"DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
> > > -DF_V3_6_MAX_COUNTERS);
> > > +"DF", "amdgpu_df", 
> > > PERF_TYPE_AMDGPU_DF,
> > > +
> > > DF_V3_6_MAX_COUNTERS);
> > 
> > trivia:
> > 
> > The indentation change seems superfluous and
> > appears to make the code harder to read.
> > 
> > You could also cc Jonathan Kim who wrote all of this.
> I think this is just display issue in mail format. It is correct that in 
> vi/vim.
> The arguments are line up with '(' after my change.

Use 8 character tabs and try again please.

> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)$
>  ^Icase CHIP_VEGA20:$
>  ^I^I/* init df */$
>  ^I^Iret = init_pmu_by_type(adev, df_v3_6_attr_groups,$
> -^I^I^I^I   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$
> -^I^I^I^I   DF_V3_6_MAX_COUNTERS);$
> +^I^I^I^I^I^I^I   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$
> +^I^I^I^I^I^I^I   DF_V3_6_MAX_COUNTERS);$

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-23 Thread maowenan 



On 2019/6/23 2:13, Joe Perches wrote:
> On Sat, 2019-06-22 at 21:05 +0800, Mao Wenan wrote:
>> There is one warning:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
>> but not used [-Wunused-but-set-variable]
>>   int ret = 0;
> []
>>  v1->v2: change the subject for this patch; change the indenting when it 
>> calls init_pmu_by_type; use the value 'ret' in
>>  amdgpu_pmu_init().
> []
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> []
>> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>>  case CHIP_VEGA20:
>>  /* init df */
>>  ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
>> -   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
>> -   DF_V3_6_MAX_COUNTERS);
>> +   "DF", "amdgpu_df", 
>> PERF_TYPE_AMDGPU_DF,
>> +   
>> DF_V3_6_MAX_COUNTERS);
> 
> trivia:
> 
> The indentation change seems superfluous and
> appears to make the code harder to read.
> 
> You could also cc Jonathan Kim who wrote all of this.
I think this is just display issue in mail format. It is correct that in vi/vim.
The arguments are line up with '(' after my change.


@@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)$
 ^Icase CHIP_VEGA20:$
 ^I^I/* init df */$
 ^I^Iret = init_pmu_by_type(adev, df_v3_6_attr_groups,$
-^I^I^I^I   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$
-^I^I^I^I   DF_V3_6_MAX_COUNTERS);$
+^I^I^I^I^I^I^I   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$
+^I^I^I^I^I^I^I   DF_V3_6_MAX_COUNTERS);$
 $
 ^I^I/* other pmu types go here*/$
 ^I^Ibreak;$





> 
> 
> 
> .
>

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-22 Thread Joe Perches 
On Sat, 2019-06-22 at 21:05 +0800, Mao Wenan wrote:
> There is one warning:
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
> but not used [-Wunused-but-set-variable]
>   int ret = 0;
[]
>  v1->v2: change the subject for this patch; change the indenting when it 
> calls init_pmu_by_type; use the value 'ret' in
>  amdgpu_pmu_init().
[]
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
[]
> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>   case CHIP_VEGA20:
>   /* init df */
>   ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
> -"DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
> -DF_V3_6_MAX_COUNTERS);
> +"DF", "amdgpu_df", 
> PERF_TYPE_AMDGPU_DF,
> +
> DF_V3_6_MAX_COUNTERS);

trivia:

The indentation change seems superfluous and
appears to make the code harder to read.

You could also cc Jonathan Kim who wrote all of this.

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-22 Thread Julia Lawall 


On Sat, 22 Jun 2019, maowenan wrote:

>
>
> On 2019/6/22 21:06, Julia Lawall wrote:
> >
> >
> > On Sat, 22 Jun 2019, Mao Wenan wrote:
> >
> >> There is one warning:
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
> >> but not used [-Wunused-but-set-variable]
> >>   int ret = 0;
> >>   ^
> >> amdgpu_pmu_init() is called by amdgpu_device_init() in 
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c,
> >> which will use the return value. So it returns 'ret' for caller.
> >> amdgpu_device_init()
> >>r = amdgpu_pmu_init(adev);
> >>
> >> Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters")
> >>
> >> Signed-off-by: Mao Wenan 
> >> ---
> >>  v1->v2: change the subject for this patch; change the indenting when it 
> >> calls init_pmu_by_type; use the value 'ret' in
> >>  amdgpu_pmu_init().
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> >> index 0e6dba9..145e720 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> >> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
> >>case CHIP_VEGA20:
> >>/* init df */
> >>ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
> >> - "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
> >> - DF_V3_6_MAX_COUNTERS);
> >> + "DF", "amdgpu_df", 
> >> PERF_TYPE_AMDGPU_DF,
> >> + 
> >> DF_V3_6_MAX_COUNTERS);
> >>
> >>/* other pmu types go here*/
> >
> > I don't know what is the impact of the other pmu types that are planned
> > for the future.  Perhaps it would be better to abort the function
> > immediately in the case of a failure.
>
> I guess it would be better to use new function or new switch case clause to 
> process different PMU types
> in future.

I don't know.  But normally when an error may occur it is checked for
immediately, rather than just letting it go until the end of the function.
But maybe the developer know what is planned for the future for this
function.

julia

>
> >
> > julia
> >
> >>break;
> >> @@ -261,7 +261,7 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
> >>return 0;
> >>}
> >>
> >> -  return 0;
> >> +  return ret;
> >>  }
> >>
> >>
> >> --
> >> 2.7.4
> >>
> >>
> >
>
>

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-22 Thread maowenan 



On 2019/6/22 21:06, Julia Lawall wrote:
> 
> 
> On Sat, 22 Jun 2019, Mao Wenan wrote:
> 
>> There is one warning:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
>> but not used [-Wunused-but-set-variable]
>>   int ret = 0;
>>   ^
>> amdgpu_pmu_init() is called by amdgpu_device_init() in 
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c,
>> which will use the return value. So it returns 'ret' for caller.
>> amdgpu_device_init()
>>  r = amdgpu_pmu_init(adev);
>>
>> Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters")
>>
>> Signed-off-by: Mao Wenan 
>> ---
>>  v1->v2: change the subject for this patch; change the indenting when it 
>> calls init_pmu_by_type; use the value 'ret' in
>>  amdgpu_pmu_init().
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
>> index 0e6dba9..145e720 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
>> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>>  case CHIP_VEGA20:
>>  /* init df */
>>  ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
>> -   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
>> -   DF_V3_6_MAX_COUNTERS);
>> +   "DF", "amdgpu_df", 
>> PERF_TYPE_AMDGPU_DF,
>> +   
>> DF_V3_6_MAX_COUNTERS);
>>
>>  /* other pmu types go here*/
> 
> I don't know what is the impact of the other pmu types that are planned
> for the future.  Perhaps it would be better to abort the function
> immediately in the case of a failure.

I guess it would be better to use new function or new switch case clause to 
process different PMU types
in future.

> 
> julia
> 
>>  break;
>> @@ -261,7 +261,7 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>>  return 0;
>>  }
>>
>> -return 0;
>> +return ret;
>>  }
>>
>>
>> --
>> 2.7.4
>>
>>
>

Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-22 Thread Julia Lawall 


On Sat, 22 Jun 2019, Mao Wenan wrote:

> There is one warning:
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
> but not used [-Wunused-but-set-variable]
>   int ret = 0;
>   ^
> amdgpu_pmu_init() is called by amdgpu_device_init() in 
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c,
> which will use the return value. So it returns 'ret' for caller.
> amdgpu_device_init()
>   r = amdgpu_pmu_init(adev);
>
> Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters")
>
> Signed-off-by: Mao Wenan 
> ---
>  v1->v2: change the subject for this patch; change the indenting when it 
> calls init_pmu_by_type; use the value 'ret' in
>  amdgpu_pmu_init().
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> index 0e6dba9..145e720 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>   case CHIP_VEGA20:
>   /* init df */
>   ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
> -"DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
> -DF_V3_6_MAX_COUNTERS);
> +"DF", "amdgpu_df", 
> PERF_TYPE_AMDGPU_DF,
> +
> DF_V3_6_MAX_COUNTERS);
>
>   /* other pmu types go here*/

I don't know what is the impact of the other pmu types that are planned
for the future.  Perhaps it would be better to abort the function
immediately in the case of a failure.

julia

>   break;
> @@ -261,7 +261,7 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
>   return 0;
>   }
>
> - return 0;
> + return ret;
>  }
>
>
> --
> 2.7.4
>
>

[PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init

2019-06-22 Thread Mao Wenan 
There is one warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set but 
not used [-Wunused-but-set-variable]
  int ret = 0;
  ^
amdgpu_pmu_init() is called by amdgpu_device_init() in 
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c,
which will use the return value. So it returns 'ret' for caller.
amdgpu_device_init()
r = amdgpu_pmu_init(adev);

Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters")

Signed-off-by: Mao Wenan 
---
 v1->v2: change the subject for this patch; change the indenting when it calls 
init_pmu_by_type; use the value 'ret' in
 amdgpu_pmu_init().
 drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
index 0e6dba9..145e720 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
@@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
case CHIP_VEGA20:
/* init df */
ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
-  "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
-  DF_V3_6_MAX_COUNTERS);
+  "DF", "amdgpu_df", 
PERF_TYPE_AMDGPU_DF,
+  
DF_V3_6_MAX_COUNTERS);
 
/* other pmu types go here*/
break;
@@ -261,7 +261,7 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)
return 0;
}
 
-   return 0;
+   return ret;
 }
 
 
-- 
2.7.4