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

[Felix Kuehling <felix.kuehling@amd.com>]

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



>>
>> 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
>>>>>>
>>>>
>>