[PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[Dan Carpenter]

The GUC_HXG_MSG_0_ORIGIN definition should be unsigned.  Currently it is
defined as INT_MIN.  This doesn't cause a problem currently but it's
still worth cleaning up.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
index 3d199016cf88..c04606872e48 100644
--- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
+++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
@@ -40,7 +40,7 @@
  */
 
 #define GUC_HXG_MSG_MIN_LEN			1u
-#define GUC_HXG_MSG_0_ORIGIN			(0x1 << 31)
+#define GUC_HXG_MSG_0_ORIGIN			(0x1U << 31)
 #define   GUC_HXG_ORIGIN_HOST			0u
 #define   GUC_HXG_ORIGIN_GUC			1u
 #define GUC_HXG_MSG_0_TYPE			(0x7 << 28)
-- 
2.42.0

Re: [PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[Lucas De Marchi]

On Mon, Jan 08, 2024 at 12:05:57PM +0300, Dan Carpenter wrote:
>The GUC_HXG_MSG_0_ORIGIN definition should be unsigned.  Currently it is
>defined as INT_MIN.  This doesn't cause a problem currently but it's
>still worth cleaning up.
>
>Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

it seems there are a few more places to change to follow what was done
in commit 962bd34bb457 ("drm/i915/uc: Fix undefined behavior due to shift overflowing the constant").

+Michal

Could we eventually share these abi includes with i915 so we don't
keep fixing the same thing in 2 places?

Lucas De Marchi

>---
> drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>index 3d199016cf88..c04606872e48 100644
>--- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>+++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>@@ -40,7 +40,7 @@
>  */
>
> #define GUC_HXG_MSG_MIN_LEN			1u
>-#define GUC_HXG_MSG_0_ORIGIN			(0x1 << 31)
>+#define GUC_HXG_MSG_0_ORIGIN			(0x1U << 31)
> #define   GUC_HXG_ORIGIN_HOST			0u
> #define   GUC_HXG_ORIGIN_GUC			1u
> #define GUC_HXG_MSG_0_TYPE			(0x7 << 28)
>-- 
>2.42.0
>

Re: [PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[Michal Wajdeczko]

On 08.01.2024 15:07, Lucas De Marchi wrote:
> On Mon, Jan 08, 2024 at 12:05:57PM +0300, Dan Carpenter wrote:
>> The GUC_HXG_MSG_0_ORIGIN definition should be unsigned.  Currently it is
>> defined as INT_MIN.  This doesn't cause a problem currently but it's
>> still worth cleaning up.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> it seems there are a few more places to change to follow what was done
> in commit 962bd34bb457 ("drm/i915/uc: Fix undefined behavior due to
> shift overflowing the constant").
> 
> +Michal
> 
> Could we eventually share these abi includes with i915 so we don't
> keep fixing the same thing in 2 places?

it should be possible and I guess we should plan for that while
discussing all this new xe driver...

anyway, what about creating new intel/ folder under drm/ ?

 - drm/intel/include/abi
        guc_actions_abi.h
        guc_klvs_abi.h
        ...

the only question would be what prefix should be used for macros:
just GUC_ or INTEL_GUC_ or XE_GUC_ ?

then we can also think of creating library with common helpers for GuC
(for encoding/decoding HXG messages, preparing ADS, reading logs, etc)

btw, we can also consider sharing register definitions:

 - drm/intel/include/regs
        xe_engine_regs.h
        xe_gt_regs.h
        xe_regs_defs.h

Michal

> 
> Lucas De Marchi
> 
>> ---
>> drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>> b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>> index 3d199016cf88..c04606872e48 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>> @@ -40,7 +40,7 @@
>>  */
>>
>> #define GUC_HXG_MSG_MIN_LEN            1u
>> -#define GUC_HXG_MSG_0_ORIGIN            (0x1 << 31)
>> +#define GUC_HXG_MSG_0_ORIGIN            (0x1U << 31)
>> #define   GUC_HXG_ORIGIN_HOST            0u
>> #define   GUC_HXG_ORIGIN_GUC            1u
>> #define GUC_HXG_MSG_0_TYPE            (0x7 << 28)
>> -- 
>> 2.42.0
>>

Re: [PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[Lucas De Marchi]

On Mon, Jan 08, 2024 at 09:46:47PM +0100, Michal Wajdeczko wrote:
>
>
>On 08.01.2024 15:07, Lucas De Marchi wrote:
>> On Mon, Jan 08, 2024 at 12:05:57PM +0300, Dan Carpenter wrote:
>>> The GUC_HXG_MSG_0_ORIGIN definition should be unsigned.  Currently it is
>>> defined as INT_MIN.  This doesn't cause a problem currently but it's
>>> still worth cleaning up.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>
>> it seems there are a few more places to change to follow what was done
>> in commit 962bd34bb457 ("drm/i915/uc: Fix undefined behavior due to
>> shift overflowing the constant").
>>
>> +Michal
>>
>> Could we eventually share these abi includes with i915 so we don't
>> keep fixing the same thing in 2 places?
>
>it should be possible and I guess we should plan for that while
>discussing all this new xe driver...
>
>anyway, what about creating new intel/ folder under drm/ ?

include/drm/intel/?

>
> - drm/intel/include/abi
>        guc_actions_abi.h
>        guc_klvs_abi.h
>        ...
>
>the only question would be what prefix should be used for macros:
>just GUC_ or INTEL_GUC_ or XE_GUC_ ?

if using a intel/ dir, probably better with INTEL_ prefix

>
>then we can also think of creating library with common helpers for GuC
>(for encoding/decoding HXG messages, preparing ADS, reading logs, etc)

with the other differences we have, I don't see much benefit,
particularly as it won't change for i915 wrt supported platforms.

>
>btw, we can also consider sharing register definitions:
>
> - drm/intel/include/regs
>        xe_engine_regs.h
>        xe_gt_regs.h
>        xe_regs_defs.h

same as above, I don't think it's worth it as xe will keep adding to it
and it doesn't care for all the previous platforms. For those files we
may eventually autogen them like done by mesa.

Lucas De Marchi

>
>Michal
>
>>
>> Lucas De Marchi
>>
>>> ---
>>> drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>> b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>> index 3d199016cf88..c04606872e48 100644
>>> --- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>> +++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>> @@ -40,7 +40,7 @@
>>>  */
>>>
>>> #define GUC_HXG_MSG_MIN_LEN            1u
>>> -#define GUC_HXG_MSG_0_ORIGIN            (0x1 << 31)
>>> +#define GUC_HXG_MSG_0_ORIGIN            (0x1U << 31)
>>> #define   GUC_HXG_ORIGIN_HOST            0u
>>> #define   GUC_HXG_ORIGIN_GUC            1u
>>> #define GUC_HXG_MSG_0_TYPE            (0x7 << 28)
>>> -- 
>>> 2.42.0
>>>

Re: [PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN

[Michal Wajdeczko]

On 08.01.2024 22:24, Lucas De Marchi wrote:
> On Mon, Jan 08, 2024 at 09:46:47PM +0100, Michal Wajdeczko wrote:
>>
>>
>> On 08.01.2024 15:07, Lucas De Marchi wrote:
>>> On Mon, Jan 08, 2024 at 12:05:57PM +0300, Dan Carpenter wrote:
>>>> The GUC_HXG_MSG_0_ORIGIN definition should be unsigned.  Currently
>>>> it is
>>>> defined as INT_MIN.  This doesn't cause a problem currently but it's
>>>> still worth cleaning up.
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>
>>> it seems there are a few more places to change to follow what was done
>>> in commit 962bd34bb457 ("drm/i915/uc: Fix undefined behavior due to
>>> shift overflowing the constant").
>>>
>>> +Michal
>>>
>>> Could we eventually share these abi includes with i915 so we don't
>>> keep fixing the same thing in 2 places?
>>
>> it should be possible and I guess we should plan for that while
>> discussing all this new xe driver...
>>
>> anyway, what about creating new intel/ folder under drm/ ?
> 
> include/drm/intel/?

maybe, but then we will be limited to pure definitions/inlines, while I
hope we could separate more GuC firmware specific, but still driver
agnostic, code and place it under drivers/gpu/drm/intel/

drivers/gpu/drm/intel/
   include/
      abi/
         guc_actions_abi.h
         guc_errors_abi.h
         guc_klvs_abi.h
   guc/
      guc_hxg_helpers.c
      guc_log_helpers.c

note that AMD has its definitions in drm/amd/include/ not under include/

> 
>>
>> - drm/intel/include/abi
>>        guc_actions_abi.h
>>        guc_klvs_abi.h
>>        ...
>>
>> the only question would be what prefix should be used for macros:
>> just GUC_ or INTEL_GUC_ or XE_GUC_ ?
> 
> if using a intel/ dir, probably better with INTEL_ prefix
> 
>>
>> then we can also think of creating library with common helpers for GuC
>> (for encoding/decoding HXG messages, preparing ADS, reading logs, etc)
> 
> with the other differences we have, I don't see much benefit,
> particularly as it won't change for i915 wrt supported platforms.

we are still using unified firmware versions across different platforms,
so any newer firmware version drops could still be beneficial for the
i915 and those legacy platforms

> 
>>
>> btw, we can also consider sharing register definitions:
>>
>> - drm/intel/include/regs
>>        xe_engine_regs.h
>>        xe_gt_regs.h
>>        xe_regs_defs.h
> 
> same as above, I don't think it's worth it as xe will keep adding to it
> and it doesn't care for all the previous platforms. For those files we
> may eventually autogen them like done by mesa.

autogen sounds promising, so lets wait and once this will happen we can
abandon xe/regs

> 
> Lucas De Marchi
> 
>>
>> Michal
>>
>>>
>>> Lucas De Marchi
>>>
>>>> ---
>>>> drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>>> b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>>> index 3d199016cf88..c04606872e48 100644
>>>> --- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>>> +++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h
>>>> @@ -40,7 +40,7 @@
>>>>  */
>>>>
>>>> #define GUC_HXG_MSG_MIN_LEN            1u
>>>> -#define GUC_HXG_MSG_0_ORIGIN            (0x1 << 31)
>>>> +#define GUC_HXG_MSG_0_ORIGIN            (0x1U << 31)
>>>> #define   GUC_HXG_ORIGIN_HOST            0u
>>>> #define   GUC_HXG_ORIGIN_GUC            1u
>>>> #define GUC_HXG_MSG_0_TYPE            (0x7 << 28)
>>>> -- 
>>>> 2.42.0
>>>>