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 <linux/xxx.h>
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 <funfunctor at folklore1984.net>
>>
>> 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.xxxxxx ] 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 <notasas at gmail.com>
>>> Signed-off-by: Huang Rui <ray.huang at amd.com>
>>> Cc: Arindam Nath <Arindam.Nath at amd.com>
>>> ---
>>> 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 <linux/types.h>
>>> #include <linux/kernel.h>
>>> #include <linux/gfp.h>
>>> @@ -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 <linux/types.h>
>>> #include <linux/kernel.h>
>>> #include <linux/slab.h>
>>> #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 0xff000000
>>> 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 <linux/errno.h>
>>> #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 <linux/types.h>
>>> #include <linux/kernel.h>
>>> @@ -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
>>> @@ -20,6 +20,7 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/fb.h>
>>> @@ -27,7 +28,6 @@
>>> #include "ppatomctrl.h"
>>> #include "atombios.h"
>>> #include "cgs_common.h"
>>> -#include "pp_debug.h"
>>> #include "ppevvmath.h"
>>>
>>> #define MEM_ID_MASK 0xff000000
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c b/drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c
>>> index 7925185..f2f0fcc 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c
>>> @@ -20,6 +20,7 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/fb.h>
>>> @@ -27,7 +28,6 @@
>>> #include "process_pptables_v1_0.h"
>>> #include "ppatomctrl.h"
>>> #include "atombios.h"
>>> -#include "pp_debug.h"
>>> #include "hwmgr.h"
>>> #include "cgs_common.h"
>>> #include "pptable_v1_0.h"
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
>>> index a4e9cf4..ed6c934 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
>>> @@ -20,6 +20,7 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include <linux/types.h>
>>> #include <linux/kernel.h>
>>> #include <linux/slab.h>
>>> @@ -27,7 +28,6 @@
>>> #include "processpptables.h"
>>> #include <atom-types.h>
>>> #include <atombios.h>
>>> -#include "pp_debug.h"
>>> #include "pptable.h"
>>> #include "power_state.h"
>>> #include "hwmgr.h"
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> index ae5517a..9dc0e52 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>>> @@ -20,13 +20,13 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/fb.h>
>>> #include <asm/div64.h>
>>> #include "linux/delay.h"
>>> #include "pp_acpi.h"
>>> -#include "pp_debug.h"
>>> #include "ppatomctrl.h"
>>> #include "atombios.h"
>>> #include "pptable_v1_0.h"
>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c
>>> index 6cd1287..0f2325e 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c
>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_powertune.c
>>> @@ -20,11 +20,11 @@
>>> * OTHER DEALINGS IN THE SOFTWARE.
>>> *
>>> */
>>> +#include "pp_debug.h"
>>> #include "hwmgr.h"
>>> #include "smumgr.h"
>>> #include "smu7_hwmgr.h"
>>> #include "smu7_powertune.h"
>>> -#include "pp_debug.h"
>>> #include "smu7_common.h"
>>>
>>> #define VOLTAGE_SCALE 4
>>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/pp_debug.h b/drivers/gpu/drm/amd/powerplay/inc/pp_debug.h
>>> index bfdbec1..8301b82 100644
>>> --- a/drivers/gpu/drm/amd/powerplay/inc/pp_debug.h
>>> +++ b/drivers/gpu/drm/amd/powerplay/inc/pp_debug.h
>>> @@ -24,6 +24,12 @@
>>> #ifndef PP_DEBUG_H
>>> #define PP_DEBUG_H
>>>
>>> +#ifdef pr_fmt
>>> +#undef pr_fmt
>>> +#endif
>>> +
>>> +#define pr_fmt(fmt) "[powerplay] " fmt
>>> +
>>> #include <linux/types.h>
>>> #include <linux/kernel.h>
>>> #include <linux/slab.h>
>>> @@ -31,7 +37,7 @@
>>> #define PP_ASSERT_WITH_CODE(cond, msg, code) \
>>> do { \
>>> if (!(cond)) { \
>>> - printk("%s\n", msg); \
>>> + pr_warning("%s\n", msg); \
>>> code; \
>>> } \
>>> } while (0)
>>> @@ -39,7 +45,7 @@
>>>
>>> #define PP_DBG_LOG(fmt, ...) \
>>> do { \
>>> - if(0)printk(KERN_INFO "[ pp_dbg ] " fmt, ##__VA_ARGS__); \
>>> + if(0)pr_info(fmt, ##__VA_ARGS__); \
>>
>> Just squash the next commit into this one to remove the 'if(0)' cond.
>>
>
> Actually, this patch just adds prefix. "remove 'if(0)'" is another
> behavior even it's minor change.
>
> Anyway, I am fine to squash it.
Yes, just to make clear - squash it with the other functional change as
they are interdependent and keep the pure header churn as a separate patch.
>
> Thanks,
> Rui
>
Cheers,
Edward.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20161223/62acfc50/attachment-0001.sig>
