[PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Arnd Bergmann]

It seems impossible to build this driver without setting either
CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:

drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 'set_reg_field_value_ex':
drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit declaration of function 'ASSERT'; did you mean 'IS_ERR'? [-Werror=implicit-function-declaration]

This moves the ASSERT() macro and related helpers outside of
the #ifdef to get it to build again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 86170b40b5c5..499bd52004cf 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -61,8 +61,6 @@
  * general debug capabilities
  *
  */
-#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
-
 #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
 #define ASSERT_CRITICAL(expr) do {	\
 	if (WARN_ON(!(expr))) { \
@@ -75,7 +73,7 @@
 		; \
 	} \
 } while (0)
-#endif
+#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
 
 #if defined(CONFIG_DEBUG_KERNEL_DC)
 #define ASSERT(expr) ASSERT_CRITICAL(expr)
@@ -86,8 +84,6 @@
 
 #define BREAK_TO_DEBUGGER() ASSERT(0)
 
-#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
-
 #define DC_ERR(...)  do { \
 	dm_error(__VA_ARGS__); \
 	BREAK_TO_DEBUGGER(); \
-- 
2.9.0

[PATCH 2/3] drm/amdgpu/display: remove unused REG_OFFSET macro

[Arnd Bergmann]

The name conflicts with another macro of the same name on the ARM ixp4xx
platform, leading to build errors.
Neither of the users actually should use a name that generic, but the
other one was here first and the dc driver doesn't actually use it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/display/dc/dm_services.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dm_services.h b/drivers/gpu/drm/amd/display/dc/dm_services.h
index c8190c38a644..d4917037ac42 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_services.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_services.h
@@ -160,9 +160,6 @@ unsigned int generic_reg_wait(const struct dc_context *ctx,
 /* These macros need to be used with soc15 registers in order to retrieve
  * the actual offset.
  */
-#define REG_OFFSET(reg) (reg + DCE_BASE.instance[0].segment[reg##_BASE_IDX])
-#define REG_BIF_OFFSET(reg) (reg + NBIF_BASE.instance[0].segment[reg##_BASE_IDX])
-
 #define dm_write_reg_soc15(ctx, reg, inst_offset, value)	\
 		dm_write_reg_func(ctx, reg + DCE_BASE.instance[0].segment[reg##_BASE_IDX] + inst_offset, value, __func__)
 
-- 
2.9.0

[PATCH 3/3] drm/amdgpu/display: fix integer arithmetic problem

[Arnd Bergmann]

gcc warns about an ambiguous integer calculation:

drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c: In function 'calculate_bandwidth':
drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c:534:5: error: this decimal constant is unsigned only in ISO C90 [-Werror]
     data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
     ^~~~

Marking the constant as explicitly unsigned makes it work fine everywhere
without warnings.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
index 15cbfc400633..4f8a95368ffc 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
+++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
@@ -531,7 +531,7 @@ static void calculate_bandwidth(
 			}
 			switch (data->lb_bpc[i]) {
 			case 8:
-				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
+				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875ul, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
 				break;
 			case 10:
 				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(300234375, 10000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
-- 
2.9.0

Re: [PATCH 3/3] drm/amdgpu/display: fix integer arithmetic problem

[Harry Wentland]

On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
> gcc warns about an ambiguous integer calculation:
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c: In function 'calculate_bandwidth':
> drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dce_calcs.c:534:5: error: this decimal constant is unsigned only in ISO C90 [-Werror]
>      data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
>      ^~~~
> 
> Marking the constant as explicitly unsigned makes it work fine everywhere
> without warnings.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for these fixes.

Series is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> index 15cbfc400633..4f8a95368ffc 100644
> --- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> +++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
> @@ -531,7 +531,7 @@ static void calculate_bandwidth(
>  			}
>  			switch (data->lb_bpc[i]) {
>  			case 8:
> -				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
> +				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(2401171875ul, 100000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
>  				break;
>  			case 10:
>  				data->lb_line_pitch = bw_ceil2(bw_mul(bw_div(bw_frc_to_fixed(300234375, 10000000), bw_int_to_fixed(3)), bw_ceil2(data->source_width_in_lb, bw_int_to_fixed(8))), bw_int_to_fixed(48));
> 

Re: [PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Felix Kuehling]

On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
> It seems impossible to build this driver without setting either
> CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 'set_reg_field_value_ex':
> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit declaration of function 'ASSERT'; did you mean 'IS_ERR'? [-Werror=implicit-function-declaration]
>
> This moves the ASSERT() macro and related helpers outside of
> the #ifdef to get it to build again.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 86170b40b5c5..499bd52004cf 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -61,8 +61,6 @@
>   * general debug capabilities
>   *
>   */
> -#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
> -
>  #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
>  #define ASSERT_CRITICAL(expr) do {	\
>  	if (WARN_ON(!(expr))) { \
> @@ -75,7 +73,7 @@
>  		; \
>  	} \
>  } while (0)
> -#endif
> +#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */

Is this the right comment here? Looks like it should be /*
defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB) */.

Regards,
  Felix

>  
>  #if defined(CONFIG_DEBUG_KERNEL_DC)
>  #define ASSERT(expr) ASSERT_CRITICAL(expr)
> @@ -86,8 +84,6 @@
>  
>  #define BREAK_TO_DEBUGGER() ASSERT(0)
>  
> -#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
> -
>  #define DC_ERR(...)  do { \
>  	dm_error(__VA_ARGS__); \
>  	BREAK_TO_DEBUGGER(); \

Re: [PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Arnd Bergmann]

On Thu, Nov 2, 2017 at 3:09 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
>> It seems impossible to build this driver without setting either
>> CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:
>>
>> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 'set_reg_field_value_ex':
>> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit declaration of function 'ASSERT'; did you mean 'IS_ERR'? [-Werror=implicit-function-declaration]
>>
>> This moves the ASSERT() macro and related helpers outside of
>> the #ifdef to get it to build again.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
>> index 86170b40b5c5..499bd52004cf 100644
>> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
>> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
>> @@ -61,8 +61,6 @@
>>   * general debug capabilities
>>   *
>>   */
>> -#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
>> -
>>  #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
>>  #define ASSERT_CRITICAL(expr) do {   \
>>       if (WARN_ON(!(expr))) { \
>> @@ -75,7 +73,7 @@
>>               ; \
>>       } \
>>  } while (0)
>> -#endif
>> +#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
>
> Is this the right comment here? Looks like it should be /*
> defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB) */.

Right, my mistake. I had tried a couple of different versions before I found
one that built in all configurations. This slipped through. I'll send a fixed
version of this one patch but not the other two in the series if that's ok.

      Arnd

Re: [PATCH 1/3] drm/amdgpu/display: provide ASSERT macros unconditionally

[Alex Deucher]

On Thu, Nov 2, 2017 at 10:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Nov 2, 2017 at 3:09 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
>> On 2017-11-02 07:26 AM, Arnd Bergmann wrote:
>>> It seems impossible to build this driver without setting either
>>> CONFIG_DEBUG_KERNEL or CONFIG_DEBUG_DRIVER:
>>>
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h: In function 'set_reg_field_value_ex':
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:132:2: error: implicit declaration of function 'ASSERT'; did you mean 'IS_ERR'? [-Werror=implicit-function-declaration]
>>>
>>> This moves the ASSERT() macro and related helpers outside of
>>> the #ifdef to get it to build again.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>  drivers/gpu/drm/amd/display/dc/os_types.h | 6 +-----
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> index 86170b40b5c5..499bd52004cf 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> @@ -61,8 +61,6 @@
>>>   * general debug capabilities
>>>   *
>>>   */
>>> -#if defined(CONFIG_DEBUG_KERNEL) || defined(CONFIG_DEBUG_DRIVER)
>>> -
>>>  #if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
>>>  #define ASSERT_CRITICAL(expr) do {   \
>>>       if (WARN_ON(!(expr))) { \
>>> @@ -75,7 +73,7 @@
>>>               ; \
>>>       } \
>>>  } while (0)
>>> -#endif
>>> +#endif /* CONFIG_DEBUG_KERNEL || CONFIG_DEBUG_DRIVER */
>>
>> Is this the right comment here? Looks like it should be /*
>> defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB) */.
>
> Right, my mistake. I had tried a couple of different versions before I found
> one that built in all configurations. This slipped through. I'll send a fixed
> version of this one patch but not the other two in the series if that's ok.

Sounds good.  Thanks!

Alex