[PATCH 2/2] drm/amdkfd: Remove cu mask from struct queue_properties

[Yu, Lang]

[AMD Official Use Only]



>-----Original Message-----
>From: Kuehling, Felix <Felix.Kuehling at amd.com>
>Sent: Friday, October 22, 2021 1:11 AM
>To: Yu, Lang <Lang.Yu at amd.com>; amd-gfx at lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Huang, Ray
><Ray.Huang at amd.com>
>Subject: Re: [PATCH 2/2] drm/amdkfd: Remove cu mask from struct
>queue_properties
>
>Am 2021-10-15 um 4:48 a.m. schrieb Lang Yu:
>> +enum queue_update_flag {
>> +	UPDATE_FLAG_PROPERTITY = 0,
>> +	UPDATE_FLAG_CU_MASK,
>> +};
>> +
>> +struct queue_update_info {
>> +	union {
>> +		struct queue_properties properties;
>> +		struct {
>> +			uint32_t count; /* Must be a multiple of 32 */
>> +			uint32_t *ptr;
>> +		} cu_mask;
>> +	};
>> +
>> +	enum queue_update_flag update_flag;
>> +};
>> +
>
>This doesn't make sense to me. As I understand it, queue_update_info is for
>information that is not stored in queue_properties but only in the MQDs.
>Therefore, it should not include the queue_properties.
>
>All the low level functions in the MQD managers get both the queue_properties
>and the queue_update_info. So trying to wrap both in the same union doesn't
>make sense there either.
>
>I think you only need this because you tried to generalize pqm_update_queue to
>handle both updates to queue_properties and CU mask updates with a single
>argument. IMO this does not make the interface any clearer. I think it would be
>more straight-forward to keep a separate pqm_set_cu_mask function that takes
>a queue_update_info parameter. If you're looking for more generic names, I
>suggest the following:
>
>  * Rename pqm_update_queue to pqm_update_queue_properties
>  * Rename struct queue_update_info to struct mqd_update_info
>  * Rename pqm_set_cu_mask to pqm_update_mqd. For now this is only used
>    for CU mask (the union has only one struct member for now). It may
>    be used for other MQD properties that don't need to be stored in
>    queue_properties in the future

Got it. Thanks for your suggestions!

Regards,
Lang

>
>Regards,
>  Felix
>