[PATCH v2] drm/amdgpu: Default disable GDS for compute VMIDs

[Koenig, Christian]

Am 18.07.19 um 22:46 schrieb Greathouse, Joseph:
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>> Sent: Thursday, July 18, 2019 3:14 AM
>> To: Kuehling, Felix <Felix.Kuehling at amd.com>; Greathouse, Joseph
>> <Joseph.Greathouse at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH v2] drm/amdgpu: Default disable GDS for compute
>> VMIDs
>>
>> Am 17.07.19 um 22:09 schrieb Kuehling, Felix:
>>> On 2019-07-17 14:23, Greathouse, Joseph wrote:
>>>> The GDS and GWS blocks default to allowing all VMIDs to
>>>> access all entries. Graphics VMIDs can handle setting
>>>> these limits when the driver launches work. However,
>>>> compute workloads under HWS control don't go through the
>>>> kernel driver. Instead, HWS firmware should set these
>>>> limits when a process is put into a VMID slot.
>>>>
>>>> Disable access to these devices by default by turning off
>>>> all mask bits (for OA) and setting BASE=SIZE=0 (for GDS
>>>> and GWS) for all compute VMIDs. If a process wants to use
>>>> these resources, they can request this from the HWS
>>>> firmware (when such capabilities are enabled). HWS will
>>>> then handle setting the base and limit for the process when
>>>> it is assigned to a VMID.
>>>>
>>>> This will also prevent user kernels from getting 'stuck' in
>>>> GWS by accident if they write GWS-using code but HWS
>>>> firmware is not set up to handle GWS reset. Until HWS is
>>>> enabled to handle GWS properly, all GWS accesses will
>>>> MEM_VIOL fault the kernel.
>>>>
>>>> v2: Move initialization outside of SRBM mutex
>>>>
>>>> Change-Id: I8edcea9d0b14d16a7444bcf9fbf9451aef8b707d
>>>> Signed-off-by: Joseph Greathouse <Joseph.Greathouse at amd.com>
>>> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>> Might be a good idea to do this for all VMIDs during initialization and
>> not just for the ones used for compute.
>>
>> But anyway patch is Reviewed-by: Christian König
>> <christian.koenig at amd.com>.
> Hmm, good point. It looks like graphics jobs will eventually call through to emit_gds_switch() to set these when launching a job, but it may be worthwhile to set these to zero as a default. I didn't want to step on any toes on the graphics side without checking first.
>
> Do you have opinions on the most reasonable location to do this? early_init(), late_init()? The various gfx_v*_set_gds_init() might be a good place -- a quick test of setting all 16 VMIDs in gfx_v9_0_set_gds_init() appears to work fine on my Vega 20.

With the exception of the GMC all hw initialization should be in 
hw_init. So set_gds_init might work, but is certainly the wrong place.

I wouldn't mind renaming gfx_*_init_compute_vmid() into 
gfx_*_init_vmid() and setting reasonable defaults for both compute as 
well as gfx VMIDs.

Christian.

>
> -Joe