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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 11 Apr 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> 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@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.

bloat-o-meter results with both might be interesting, for code and
data. Does the data saving matter if we bloat code more?

BR,
Jani.



>
>
> Lucas De Marchi
>
>>
>>Signed-off-by: Matt Roper <matthew.d.roper@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
>>

-- 
Jani Nikula, Intel Open Source Graphics Center