diff mbox series

drm/xe/oa/uapi: Make bit masks unsigned

Message ID 20240729092634.2227611-1-geert+renesas@glider.be (mailing list archive)
State New, archived
Headers show
Series drm/xe/oa/uapi: Make bit masks unsigned | expand

Commit Message

Geert Uytterhoeven July 29, 2024, 9:26 a.m. UTC 
When building with gcc-5:

    In function ‘decode_oa_format.isra.26’,
	inlined from ‘xe_oa_set_prop_oa_format’ at drivers/gpu/drm/xe/xe_oa.c:1664:6:
    ././include/linux/compiler_types.h:510:38: error: call to ‘__compiletime_assert_1336’ declared with attribute error: FIELD_GET: mask is not constant
    [...]
    ./include/linux/bitfield.h:155:3: note: in expansion of macro ‘__BF_FIELD_CHECK’
       __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
       ^
    drivers/gpu/drm/xe/xe_oa.c:1573:18: note: in expansion of macro ‘FIELD_GET’
      u32 bc_report = FIELD_GET(DRM_XE_OA_FORMAT_MASK_BC_REPORT, fmt);
		      ^

Fixes: b6fd51c6211910b1 ("drm/xe/oa/uapi: Define and parse OA stream properties")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 include/uapi/drm/xe_drm.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Lucas De Marchi July 29, 2024, 1:21 p.m. UTC | #1 
On Mon, Jul 29, 2024 at 11:26:34AM GMT, Geert Uytterhoeven wrote:
>When building with gcc-5:
>
>    In function ‘decode_oa_format.isra.26’,
>	inlined from ‘xe_oa_set_prop_oa_format’ at drivers/gpu/drm/xe/xe_oa.c:1664:6:
>    ././include/linux/compiler_types.h:510:38: error: call to ‘__compiletime_assert_1336’ declared with attribute error: FIELD_GET: mask is not constant
>    [...]
>    ./include/linux/bitfield.h:155:3: note: in expansion of macro ‘__BF_FIELD_CHECK’
>       __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
>       ^
>    drivers/gpu/drm/xe/xe_oa.c:1573:18: note: in expansion of macro ‘FIELD_GET’
>      u32 bc_report = FIELD_GET(DRM_XE_OA_FORMAT_MASK_BC_REPORT, fmt);
>		      ^
>
>Fixes: b6fd51c6211910b1 ("drm/xe/oa/uapi: Define and parse OA stream properties")
>Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


That fixes the build, but question to Ashutosh: it's odd to tie the
format to a bspec. What happens on next platform if the HW changes?
Hopefully it doesn't change in an incompatible way, but looking at this
code it seems we could still keep the uapi by untying the HW from the
uapi in the documentation.

Lucas De Marchi

>---
>Compile-tested only.
>---
> include/uapi/drm/xe_drm.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index 19619d4952a863f7..db232a25189eba9f 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -1590,10 +1590,10 @@ enum drm_xe_oa_property_id {
> 	 * b. Counter select c. Counter size and d. BC report. Also refer to the
> 	 * oa_formats array in drivers/gpu/drm/xe/xe_oa.c.
> 	 */
>-#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xff << 0)
>-#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xff << 8)
>-#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xff << 16)
>-#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xff << 24)
>+#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xffu << 0)
>+#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xffu << 8)
>+#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xffu << 16)
>+#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xffu << 24)
>
> 	/**
> 	 * @DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT: Requests periodic OA unit
>-- 
>2.34.1
>
Dixit, Ashutosh July 29, 2024, 9:54 p.m. UTC | #2 
On Mon, 29 Jul 2024 06:21:20 -0700, Lucas De Marchi wrote:
>

Hi Lucas,

> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
> That fixes the build, but question to Ashutosh: it's odd to tie the
> format to a bspec. What happens on next platform if the HW changes?
> Hopefully it doesn't change in an incompatible way, but looking at this
> code it seems we could still keep the uapi by untying the HW from the
> uapi in the documentation.

IMO, in this case, it is not possible to decouple the formats from Bspec
because that is where they are specified (in Bspec 52198/60942).

In i915 the OA formats were specified by an enum (enum drm_i915_oa_format),
but I would argue that enum is meaningful only when we refer back to Bspec.
Also the i915 enum had to constantly updated when HW added new formats.

For Xe, we changed the way the formats are specified in a way which we
believe will make the uapi more robust and uapi header update much less
frequent (hopefully we will never have to update the header and if at all
we have to, we should be able to do it in a backwards compatible way since
we have sufficient number of free bits). HW has followed this scheme for
specifying the formats for years and only recently for Xe2 has added a
couple of bits and introduced new PEC formats which I think it is not going
to change now for some time.

But as I said the formats have to refer back to Bspec since that is where
there are specified and there are too many of them. Any description or enum
is ambiguous unless it refers back to Bspec. So I don't see how not to
refer to Bspec in the documentation. If anyone has any ideas about not
referring to Bspec I am willing to consider it but I think the best way
forward is to leave the documentation as is:

	/*
	 * OA_FORMAT's are specified the same way as in PRM/Bspec 52198/60942,
	 * in terms of the following quantities: a. enum @drm_xe_oa_format_type
	 * b. Counter select c. Counter size and d. BC report. Also refer to the
	 * oa_formats array in drivers/gpu/drm/xe/xe_oa.c.
	 */
#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xff << 0)
#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xff << 8)
#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xff << 16)
#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xff << 24)

Thanks.
--
Ashutosh
Lucas De Marchi July 30, 2024, 8:58 p.m. UTC | #3 
On Mon, Jul 29, 2024 at 02:54:58PM GMT, Ashutosh Dixit wrote:
>On Mon, 29 Jul 2024 06:21:20 -0700, Lucas De Marchi wrote:
>>
>
>Hi Lucas,
>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>> That fixes the build, but question to Ashutosh: it's odd to tie the
>> format to a bspec. What happens on next platform if the HW changes?
>> Hopefully it doesn't change in an incompatible way, but looking at this
>> code it seems we could still keep the uapi by untying the HW from the
>> uapi in the documentation.
>
>IMO, in this case, it is not possible to decouple the formats from Bspec
>because that is where they are specified (in Bspec 52198/60942).
>
>In i915 the OA formats were specified by an enum (enum drm_i915_oa_format),
>but I would argue that enum is meaningful only when we refer back to Bspec.
>Also the i915 enum had to constantly updated when HW added new formats.
>
>For Xe, we changed the way the formats are specified in a way which we
>believe will make the uapi more robust and uapi header update much less
>frequent (hopefully we will never have to update the header and if at all
>we have to, we should be able to do it in a backwards compatible way since
>we have sufficient number of free bits). HW has followed this scheme for
>specifying the formats for years and only recently for Xe2 has added a
>couple of bits and introduced new PEC formats which I think it is not going
>to change now for some time.
>
>But as I said the formats have to refer back to Bspec since that is where
>there are specified and there are too many of them. Any description or enum
>is ambiguous unless it refers back to Bspec. So I don't see how not to
>refer to Bspec in the documentation. If anyone has any ideas about not
>referring to Bspec I am willing to consider it but I think the best way
>forward is to leave the documentation as is:
>
>	/*
>	 * OA_FORMAT's are specified the same way as in PRM/Bspec 52198/60942,
>	 * in terms of the following quantities: a. enum @drm_xe_oa_format_type
>	 * b. Counter select c. Counter size and d. BC report. Also refer to the
>	 * oa_formats array in drivers/gpu/drm/xe/xe_oa.c.
>	 */
>#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xff << 0)
>#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xff << 8)
>#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xff << 16)
>#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xff << 24)

I was under impression that these shifts were something coming from the
HW definition and we were exposing that raw to userspace, rather than
e.g.

struct drm_xe_oa_format {
	__u8 fmt_type;
	__u8 counter_sel;
	__u8 counter_size;
	__u8 bc_report;
	__u32 rsvd;
};

now I see the shifts are not tied to HW and it was only the chosen
format rather than declaring a struct.

Applied this patch to drm-xe-next.

Thanks
Lucas De Marchi

>
>Thanks.
>--
>Ashutosh
diff mbox series

Patch

diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index 19619d4952a863f7..db232a25189eba9f 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -1590,10 +1590,10 @@  enum drm_xe_oa_property_id {
 	 * b. Counter select c. Counter size and d. BC report. Also refer to the
 	 * oa_formats array in drivers/gpu/drm/xe/xe_oa.c.
 	 */
-#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xff << 0)
-#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xff << 8)
-#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xff << 16)
-#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xff << 24)
+#define DRM_XE_OA_FORMAT_MASK_FMT_TYPE		(0xffu << 0)
+#define DRM_XE_OA_FORMAT_MASK_COUNTER_SEL	(0xffu << 8)
+#define DRM_XE_OA_FORMAT_MASK_COUNTER_SIZE	(0xffu << 16)
+#define DRM_XE_OA_FORMAT_MASK_BC_REPORT		(0xffu << 24)
 
 	/**
 	 * @DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT: Requests periodic OA unit