Re: [PATCH 1/2] drm/amd/powerplay: add prefix for all powerplay pr_* prints
Hi,
I would say drop all the header relocation churn, it distracts away from
the functional changes in this commit. Also see inline comments.
With those fixes,
Acked-by: Edward O'Callaghan
Kindest Regards,
Edward.
On 12/23/2016 01:45 PM, Huang Rui wrote:
> Powerplay will be used them instead of raw printk, and we can dynamic
> change the debug level with it.
>
> The prefix is like below:
>
> [ 197.755167] [powerplay] amdgpu: powerplay initialized
Ideally I think it would be better to look like this:
[ xxx.xx ] amdgpu: [powerplay] initialized.
but certainly drop repeating "powerplay" twice..
>
> Suggested-by: Grazvydas Ignotas
> Signed-off-by: Huang Rui
> Cc: Arindam Nath
> ---
> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 3 +--
> drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 3 ++-
> drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c| 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c | 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c | 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c| 2 +-
> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c| 2 +-
> drivers/gpu/drm/amd/powerplay/inc/pp_debug.h| 10 --
> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smc.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smc.c| 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/tonga_smc.c| 2 +-
> drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 2 +-
> 19 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index cc72190..8b85153 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -20,6 +20,7 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> */
> +#include "pp_debug.h"
> #include
> #include
> #include
> @@ -29,7 +30,6 @@
> #include "pp_instance.h"
> #include "power_state.h"
> #include "eventmanager.h"
> -#include "pp_debug.h"
>
>
> #define PP_CHECK(handle) \
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> index 74dbbd1..d043337 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> @@ -20,13 +20,13 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> */
> +#include "pp_debug.h"
> #include
> #include
> #include
> #include "atom-types.h"
> #include "atombios.h"
> #include "processpptables.h"
> -#include "pp_debug.h"
> #include "cgs_common.h"
> #include "smu/smu_8_0_d.h"
> #include "smu8_fusion.h"
> @@ -38,7 +38,6 @@
> #include "cz_hwmgr.h"
> #include "power_state.h"
> #include "cz_clockpowergating.h"
> -#include "pp_debug.h"
>
> #define ixSMUSVI_NB_CURRENTVID 0xD8230044
> #define CURRENT_NB_VID_MASK 0xff00
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> index c355a0f..0eb8e886 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> @@ -20,11 +20,11 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> */
> +#include "pp_debug.h"
> #include
> #include "hwmgr.h"
> #include "hardwaremanager.h"
> #include "power_state.h"
> -#include "pp_debug.h"
>
> #define PHM_FUNC_CHECK(hw) \
> do {\
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> index b036064..fcfd648 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> @@ -20,6 +20,8 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> */
> +
> +#include "pp_debug.h"
> #include "linux/delay.h"
> #include
> #include
> @@ -29,7 +31,6 @@
> #include "power_state.h"
> #include "hwmgr.h"
> #include "pppcielanes.h"
> -#include "pp_debug.h"
> #include "ppatomctrl.h"
> #include "ppsmc.h"
> #include "pp_acpi.h"
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c
> index ddaea1d..2c60f7b 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c
>
Re: [PATCH 1/2] drm/amd/powerplay: add prefix for all powerplay pr_* prints
On Fri, Dec 23, 2016 at 05:32:58PM +0800, Edward O'Callaghan wrote:
> Hi,
>
> I would say drop all the header relocation churn, it distracts away from
> the functional changes in this commit. Also see inline comments.
>
Yes, if double check undef, it needn't move macro before
> With those fixes,
> Acked-by: Edward O'Callaghan
>
> Kindest Regards,
> Edward.
>
> On 12/23/2016 01:45 PM, Huang Rui wrote:
> > Powerplay will be used them instead of raw printk, and we can dynamic
> > change the debug level with it.
> >
> > The prefix is like below:
> >
> > [ 197.755167] [powerplay] amdgpu: powerplay initialized
>
> Ideally I think it would be better to look like this:
>
> [ xxx.xx ] amdgpu: [powerplay] initialized.
>
> but certainly drop repeating "powerplay" twice..
>
We define it as below:
#define pr_fmt(fmt) "amdgpu: [powerplay] " fmt
But we need refine most detail prints message with more patches in the
codes.
> >
> > Suggested-by: Grazvydas Ignotas
> > Signed-off-by: Huang Rui
> > Cc: Arindam Nath
> > ---
> > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 3 +--
> > drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 3 ++-
> > drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c| 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c| 2 +-
> > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c| 2 +-
> > drivers/gpu/drm/amd/powerplay/inc/pp_debug.h| 10 --
> > drivers/gpu/drm/amd/powerplay/smumgr/fiji_smc.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smc.c| 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/tonga_smc.c| 2 +-
> > drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 2 +-
> > 19 files changed, 27 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > index cc72190..8b85153 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > @@ -20,6 +20,7 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > *
> > */
> > +#include "pp_debug.h"
> > #include
> > #include
> > #include
> > @@ -29,7 +30,6 @@
> > #include "pp_instance.h"
> > #include "power_state.h"
> > #include "eventmanager.h"
> > -#include "pp_debug.h"
> >
> >
> > #define PP_CHECK(handle) \
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > index 74dbbd1..d043337 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> > @@ -20,13 +20,13 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > *
> > */
> > +#include "pp_debug.h"
> > #include
> > #include
> > #include
> > #include "atom-types.h"
> > #include "atombios.h"
> > #include "processpptables.h"
> > -#include "pp_debug.h"
> > #include "cgs_common.h"
> > #include "smu/smu_8_0_d.h"
> > #include "smu8_fusion.h"
> > @@ -38,7 +38,6 @@
> > #include "cz_hwmgr.h"
> > #include "power_state.h"
> > #include "cz_clockpowergating.h"
> > -#include "pp_debug.h"
> >
> > #define ixSMUSVI_NB_CURRENTVID 0xD8230044
> > #define CURRENT_NB_VID_MASK 0xff00
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> > index c355a0f..0eb8e886 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> > @@ -20,11 +20,11 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > *
> > */
> > +#include "pp_debug.h"
> > #include
> > #include "hwmgr.h"
> > #include "hardwaremanager.h"
> > #include "power_state.h"
> > -#include "pp_debug.h"
> >
> > #define PHM_FUNC_CHECK(hw) \
> > do {\
> > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> > b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> > index b036064..fcfd648 100644
> > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> > @@ -20,6 +20,8 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > *
> > */
> > +
> > +#i
Re: [PATCH 1/2] drm/amd/powerplay: add prefix for all powerplay pr_* prints
On 12/23/2016 08:54 PM, Huang Rui wrote:
> On Fri, Dec 23, 2016 at 05:32:58PM +0800, Edward O'Callaghan wrote:
>> Hi,
>>
>> I would say drop all the header relocation churn, it distracts away from
>> the functional changes in this commit. Also see inline comments.
>>
>
> Yes, if double check undef, it needn't move macro before
If you want to reshuffle headers I would say that is a seperate patch
and the functional changes as their own changeset. That's my view any way.
>
>> With those fixes,
>> Acked-by: Edward O'Callaghan
>>
>> Kindest Regards,
>> Edward.
>>
>> On 12/23/2016 01:45 PM, Huang Rui wrote:
>>> Powerplay will be used them instead of raw printk, and we can dynamic
>>> change the debug level with it.
>>>
>>> The prefix is like below:
>>>
>>> [ 197.755167] [powerplay] amdgpu: powerplay initialized
>>
>> Ideally I think it would be better to look like this:
>>
>> [ xxx.xx ] amdgpu: [powerplay] initialized.
>>
>> but certainly drop repeating "powerplay" twice..
>>
>
> We define it as below:
>
> #define pr_fmt(fmt) "amdgpu: [powerplay] " fmt
>
> But we need refine most detail prints message with more patches in the
> codes.
I suggest if your going to touch it you may as well go all the way and
get the thing fixed up properly, then its done.
>
>>>
>>> Suggested-by: Grazvydas Ignotas
>>> Signed-off-by: Huang Rui
>>> Cc: Arindam Nath
>>> ---
>>> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 3 +--
>>> drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 3 ++-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c| 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c| 2 +-
>>> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c| 2 +-
>>> drivers/gpu/drm/amd/powerplay/inc/pp_debug.h| 10 --
>>> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smc.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smc.c| 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/tonga_smc.c| 2 +-
>>> drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 2 +-
>>> 19 files changed, 27 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>>> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>>> index cc72190..8b85153 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>>> @@ -20,6 +20,7 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include
>>> #include
>>> #include
>>> @@ -29,7 +30,6 @@
>>> #include "pp_instance.h"
>>> #include "power_state.h"
>>> #include "eventmanager.h"
>>> -#include "pp_debug.h"
>>>
>>>
>>> #define PP_CHECK(handle) \
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>>> b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>>> index 74dbbd1..d043337 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>>> @@ -20,13 +20,13 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include
>>> #include
>>> #include
>>> #include "atom-types.h"
>>> #include "atombios.h"
>>> #include "processpptables.h"
>>> -#include "pp_debug.h"
>>> #include "cgs_common.h"
>>> #include "smu/smu_8_0_d.h"
>>> #include "smu8_fusion.h"
>>> @@ -38,7 +38,6 @@
>>> #include "cz_hwmgr.h"
>>> #include "power_state.h"
>>> #include "cz_clockpowergating.h"
>>> -#include "pp_debug.h"
>>>
>>> #define ixSMUSVI_NB_CURRENTVID 0xD8230044
>>> #define CURRENT_NB_VID_MASK 0xff00
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
>>> b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
>>> index c355a0f..0eb8e886 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
>>> @@ -20,11 +20,11 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include
>>> #include "hwmgr.h"
>>> #include "hardwaremanager.h"
>>> #include "power_state.h"
>>> -#include "pp_debug.h"
>>>
>>> #define PHM_FUNC_CHECK(hw) \
>>> do {\
>>>
