Re: [RFC 3/7] drm/amdgpu: Create MQD for userspace queue

[Shashank Sharma <shashank.sharma@amd.com>]

On 04/01/2023 15:35, Felix Kuehling wrote:
> Am 2023-01-04 um 04:23 schrieb Shashank Sharma:
>>
>> On 04/01/2023 10:17, Christian König wrote:
>>> Am 04.01.23 um 10:13 schrieb Shashank Sharma:
>>>>
>>>> On 04/01/2023 10:10, Christian König wrote:
>>>>> Am 04.01.23 um 07:21 schrieb Yadav, Arvind:
>>>>>>
>>>>>> On 1/4/2023 12:07 AM, Felix Kuehling wrote:
>>>>>>> Am 2023-01-03 um 04:36 schrieb Shashank Sharma:
>>>>>>>>>> /*MQD struct for usermode Queue*/
>>>>>>>>>> +struct amdgpu_usermode_queue_mqd
>>>>>>>>> This is specific to GC 11.  Every IP and version will have its 
>>>>>>>>> own MQD
>>>>>>>>> format.  That should live in the IP specific code, not the 
>>>>>>>>> generic
>>>>>>>>> code.  We already have the generic MQD parameters that we need 
>>>>>>>>> from
>>>>>>>>> the userq IOCTL.
>>>>>>>>
>>>>>>>> Noted, we can separate out the generic parameters from gen 
>>>>>>>> specific parameter, and will try to wrap it around the generic 
>>>>>>>> structure.
>>>>>>>>
>>>>>>>> - Shashank
>>>>>>>
>>>>>>> Is there a reason why you can't use "struct v11_compute_mqd" 
>>>>>>> from v11_structs.h?
>>>>>>
>>>>>> Hi Felix,
>>>>>>
>>>>>> Yes,  V11_compute_mqd does not have these below member which is 
>>>>>> needed for usermode queue.
>>>>>>
>>>>>>     uint32_t shadow_base_lo; // offset: 0  (0x0)
>>>>>>     uint32_t shadow_base_hi; // offset: 1  (0x1)
>>>>>>     uint32_t gds_bkup_base_lo ; // offset: 2  (0x2)
>>>>>>     uint32_t gds_bkup_base_hi ; // offset: 3  (0x3)
>>>>>>     uint32_t fw_work_area_base_lo; // offset: 4  (0x4)
>>>>>>     uint32_t fw_work_area_base_hi; // offset: 5  (0x5)
>>>>>>     uint32_t shadow_initialized; // offset: 6  (0x6)
>>>>>>     uint32_t ib_vmid; // offset: 7  (0x7)
>>>>>>
>>>>>> So we had to add new MQD structs.
>>>>>
>>>>> Would it make more sense to update the existing MQD structures 
>>>>> than adding new ones?
>>>>>
>>>> Imo, It might be a bit complicated in the bring-up state, but we 
>>>> can take a note of converting this structure into a union of two, 
>>>> or may be renaming it into a superset structure.
>>>
>>> Union? Does that mean we have stuff which is individual for both 
>>> versions of the struct?
>> So far it seems like Gfx MQD structure is a superset of two, but we 
>> have not compared them neck-to-neck yet, hence I feel like we can 
>> defer this task for sometime (but add into to-do list).
>
> v11_gfx_mqd has these fields reserved. Updating the definition with 
> the fields you need should not be a problem. v11_gfx_mqd is already 
> used in gfx_v11_0.c.
>
> The firmware shouldn't care much whether a queue is a kernel mode 
> queue or a user mode queue. The MQD layout should be the same. So 
> having two different structure definitions in two different places 
> doesn't make sense. I don't think it's wise to leave this for cleanup 
> later. That would only cause churn and ultimately more work than doing 
> the right thing in the first place.
>
> Regards,
>   Felix
>
Hey Felix, noted, we will try to reuse this same structure.

- Shashank

>
>
>>>
>>> BTW: Could we drop the "// offset:" stuff? This could cause problems 
>>> with automated checkers.
>>
>> Sure, we will do it.
>>
>> - Shashank
>>
>>>
>>> Christian.
>>>
>>>>
>>>> - Shashank
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> thanks
>>>>>>
>>>>>> ~arvind
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>>   Felix
>>>>>>>
>>>>>
>>>