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 {\ >>>