[Intel-xe] [PATCH 1/3] drm/xe: Use packed bitfields for xe->info feature flags

[Lucas De Marchi]

On Mon, Apr 10, 2023 at 11:39:08AM -0700, Matt Roper wrote:
>Replace 'bool' fields with single bits to allow the various device
>feature flags to pack more tightly.

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

but a  digression:

for structs like the descriptors in xe_pci.c, this justification is
enough as we will be maintaining several of those as they extend to
each platform. Also the access to struct members is contained in
one place.

However this reasoning can't be generalized to structs like xe_device,
that has one allocated per device. The gain should be very minimal if
any at all.

1) Each place accessing one of these fields will have more
instructions generated to use the right bit although in most cases
the compiler should swap a cmpb with testb since we are likely just
checking if the feature is available or not.

2) It also limits the ability to pass them by address.

3) We also need to be careful when changing
bool -> u8 as they don't have the same semantics in cases like
`b = true; b++;`, which may not be obvious in some cases.

for (1), the pro/con should be really small, (2) we should get a
compiler error if we tried. But for (3)... we need to check all the
fields we are converting to make sure this doesn't introduce bugs.  I'm
on the fence on the need for this change, but I'm ok with it.  I double
checked all the members in the struct and didn't find use cases that
would introduce a bug, hence my r-b above.


Lucas De Marchi

>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_device_types.h | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index f5399b284e3b..9ce6e348dd29 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -67,8 +67,6 @@ struct xe_device {
> 		u32 media_verx100;
> 		/** @mem_region_mask: mask of valid memory regions */
> 		u32 mem_region_mask;
>-		/** @is_dgfx: is discrete device */
>-		bool is_dgfx;
> 		/** @platform: XE platform enum */
> 		enum xe_platform platform;
> 		/** @subplatform: XE subplatform enum */
>@@ -87,22 +85,25 @@ struct xe_device {
> 		u8 tile_count;
> 		/** @vm_max_level: Max VM level */
> 		u8 vm_max_level;
>+
>+		/** @is_dgfx: is discrete device */
>+		u8 is_dgfx:1;
> 		/** @supports_usm: Supports unified shared memory */
>-		bool supports_usm;
>+		u8 supports_usm:1;
> 		/** @has_asid: Has address space ID */
>-		bool has_asid;
>+		u8 has_asid:1;
> 		/** @enable_guc: GuC submission enabled */
>-		bool enable_guc;
>+		u8 enable_guc:1;
> 		/** @has_flat_ccs: Whether flat CCS metadata is used */
>-		bool has_flat_ccs;
>+		u8 has_flat_ccs:1;
> 		/** @has_4tile: Whether tile-4 tiling is supported */
>-		bool has_4tile;
>+		u8 has_4tile:1;
> 		/** @has_range_tlb_invalidation: Has range based TLB invalidations */
>-		bool has_range_tlb_invalidation;
>+		u8 has_range_tlb_invalidation:1;
> 		/** @has_link_copy_engines: Whether the platform has link copy engines */
>-		bool has_link_copy_engine;
>+		u8 has_link_copy_engine:1;
> 		/** @enable_display: display enabled */
>-		bool enable_display;
>+		u8 enable_display:1;
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> 		struct xe_device_display_info {
>-- 
>2.39.2
>