[AMD Official Use Only]
>-----Original Message-----
>From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
>Sent: Friday, October 22, 2021 1:11 AM
>To: Yu, Lang <Lang.Yu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
><Ray.Huang@xxxxxxx>
>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
>
