[PATCH 1/2] drm/amdgpu: Moved fault hash table to amdgpu vm

Christian König ckoenig.leichtzumerken at gmail.com
Mon Sep 10 09:42:32 UTC 2018


Hi Felix & Oak,

over the weekend I had the idea that we could just use the shadow BOs to 
have the current state in a page fault. They are GTT BOs and CPU 
accessible anyway.

Regards,
Christian.

Am 08.09.2018 um 09:34 schrieb Christian König:
> Hi Felix,
>
>> But why do you want to update page tables when there is no more user
>> mode context that cares about them? Is this just to allow pending work
>> to complete after the user mode context is gone? To prevent hangs?
>
> The problem I'm see is that we still don't have a reliable way of 
> killing a command submission while making sure that no more faults of 
> it are in the flight.
>
> E.g. the HWS has a function to kill a running job on the hardware, but 
> that only works for compute and there is no way of telling when that 
> operation has finished.
>
> But Oak has already convinced me that we should probably work on that 
> instead of trying to keep the VM alive as long as possible.
>
> Regards,
> Christian.
>
> Am 08.09.2018 um 02:27 schrieb Felix Kuehling:
>> Hi Christian,
>>
>> I'm not sure I get your point here.
>>
>> As I understand it, the amdgpu_vm structure represents the user mode VM
>> context. It has the pointers to the root page directory and the rest of
>> the page table hierarchy. If there is no amdgpu_vm, there is no user
>> mode context that cares about the page tables.
>>
>> If we leave the page table pointers in the amdgpu_vm, then handling
>> faults after the VM is destroyed is pointless. We can't find the page
>> tables and we can't update them, so there is nothing we can do in
>> response to VM faults.
>>
>> So I'm guessing that you want to move the page table pointers into a
>> different structure that exists half-independently of the VM. It would
>> be created when the VM is created (that's where we currently allocate
>> the PASID) but can be destroyed slightly later.
>>
>> But why do you want to update page tables when there is no more user
>> mode context that cares about them? Is this just to allow pending work
>> to complete after the user mode context is gone? To prevent hangs?
>>
>> Regards,
>>    Felix
>>
>> On 2018-09-10 07:20 AM, Christian König wrote:
>>>> Am I right that the handling of page fault need a valid VM?
>>> No, exactly that view is incorrect.
>>>
>>> The VM is meant to be a memory management structure of page tables and
>>> is completely pointless fault processing because it represent the
>>> future state of the page tables and not the current one.
>>>
>>> What we need for fault processing is a new structure build around
>>> PASIDs which is feed by the with addresses when page tables are moved
>>> around.
>>>
>>> Alternatively I hope that we can use the SDMA to walk the page tables
>>> and update the required entries by just using the address.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 07.09.2018 um 16:55 schrieb Zeng, Oak:
>>>> Hi Christian,
>>>>
>>>>
>>>> What is the value of delay the destroy of hash to after vm is
>>>> destroyed? Since vm is destroyed, you basically don’t have enough
>>>> information to paging in the correct page to gpuvm. Am I right that
>>>> the handling of page fault need a valid VM? For example, you need the
>>>> VM to get the corresponding physical address of the faulty page.
>>>>
>>>>
>>>> After vm is destroyed, the retry interrupt can be directly discard as
>>>> you can’t find vm through pasid. You can think this is also one kind
>>>> of prescreen.
>>>>
>>>>
>>>> So I am stand for the point that, there is no need to delay the
>>>> destroy of hash to after vm is destroyed. Prescreening hash can be
>>>> destroyed at the time of vm_fini.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Oak
>>>>
>>>>
>>>> *From:*Christian König <ckoenig.leichtzumerken at gmail.com>
>>>> *Sent:* Friday, September 07, 2018 4:39 AM
>>>> *To:* Zeng, Oak <Oak.Zeng at amd.com>; Oak Zeng <zengshanjun at gmail.com>;
>>>> amd-gfx at lists.freedesktop.org
>>>> *Cc:* Zeng, Oak <Oak.Zeng at amd.com>
>>>> *Subject:* Re: [PATCH 1/2] drm/amdgpu: Moved fault hash table to
>>>> amdgpu vm
>>>>
>>>>
>>>> Hi Oak,
>>>>
>>>> yeah, but this is still a NAK. Making the hash per PASID was not a
>>>> suggestion but rather a must have.
>>>>
>>>> The VM structures must be destroyed while the processing is still
>>>> ongoing, or otherwise we would not have a clean OOM handling.
>>>>
>>>> The IDR for PASID lockup can be put into amdgpu_ids.c, you can just
>>>> replace the IDA already used there with an IDR.
>>>>
>>>> Since the PASID is already freed up delayed we should have the grace
>>>> period for interrupt processing included. If that still isn't
>>>> sufficient we can still add some delayed work for this.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 07.09.2018 um 06:16 schrieb ozeng:
>>>>
>>>>      Hi Christian,
>>>>
>>>>      In this implementation, fault hash is made per vm, not per pasid
>>>>      as suggested, based on below considerations:
>>>>
>>>>        * Delay the destroy of hash introduces more effort like how to
>>>>          set the proper grace period after which no retry interrupt
>>>>          will be generated. We want to avoid those complication if we
>>>>          can.
>>>>        * The  problem of the per vm implementation is that it is hard
>>>>          to delay the destroy of fault hash, because when vm is
>>>>          destroyed, prescreen function won't be able to retrieve the
>>>>          fault hash. But in this case, the prescreen function can
>>>>          either let the interrupt go through (to gmc) or ignore it.
>>>>          From this perspective, it is not very necessary to delay the
>>>>          destroy of hash table.
>>>>        * On the other hand, since ASICs after vega10 have the HW CAM
>>>>          feature. So the SW IV prescreen is only useful for vega10.
>>>>          For all the HW implemented CAM, we can consider the delayed
>>>>          CAM acknowledgment.
>>>>        * Making it per pasid need to introduce another idr to
>>>>          correlate pasid and hash table - where to put the idr? You
>>>>          will have to make it a global variable which is not very
>>>>          desirable.
>>>>
>>>>      The main purpose of this patch is to solve the inter-process lock
>>>>      issue (when hash table is full), while still keep codes simple.
>>>>
>>>>      Also with this patch, the faults kfifo is not necessary any more.
>>>>      See patch 2.
>>>>
>>>>      Regards,
>>>>
>>>>      Oak
>>>>
>>>>
>>>>      On 09/06/2018 11:28 PM, Oak Zeng wrote:
>>>>
>>>>          In stead of share one fault hash table per device, make it
>>>>
>>>>          per vm. This can avoid inter-process lock issue when fault
>>>>
>>>>          hash table is full.
>>>>
>>>>
>>>>          Change-Id: I5d1281b7c41eddc8e26113e010516557588d3708
>>>>
>>>>          Signed-off-by: Oak Zeng <Oak.Zeng at amd.com> 
>>>> <mailto:Oak.Zeng at amd.com>
>>>>
>>>>          Suggested-by: Christian Konig <Christian.Koenig at amd.com> 
>>>> <mailto:Christian.Koenig at amd.com>
>>>>
>>>>          Suggested-by: Felix Kuehling <Felix.Kuehling at amd.com> 
>>>> <mailto:Felix.Kuehling at amd.com>
>>>>
>>>>          ---
>>>>
>>>>           drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c |  75 
>>>> ------------------------
>>>>
>>>>           drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h |  10 ----
>>>>
>>>>           drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 102 
>>>> ++++++++++++++++++++++++++++++++-
>>>>
>>>>           drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  12 ++++
>>>>
>>>>           drivers/gpu/drm/amd/amdgpu/vega10_ih.c |  38 +++++-------
>>>>
>>>>           5 files changed, 127 insertions(+), 110 deletions(-)
>>>>
>>>>
>>>>          diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
>>>>
>>>>          index 06373d4..4ed8621 100644
>>>>
>>>>          --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
>>>>
>>>>          +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
>>>>
>>>>          @@ -197,78 +197,3 @@ int amdgpu_ih_process(struct 
>>>> amdgpu_device *adev)
>>>>
>>>>             return IRQ_HANDLED;
>>>>
>>>>           }
>>>>
>>>>
>>>>          -/**
>>>>
>>>>          - * amdgpu_ih_add_fault - Add a page fault record
>>>>
>>>>          - *
>>>>
>>>>          - * @adev: amdgpu device pointer
>>>>
>>>>          - * @key: 64-bit encoding of PASID and address
>>>>
>>>>          - *
>>>>
>>>>          - * This should be called when a retry page fault 
>>>> interrupt is
>>>>
>>>>          - * received. If this is a new page fault, it will be 
>>>> added to a hash
>>>>
>>>>          - * table. The return value indicates whether this is a 
>>>> new fault, or
>>>>
>>>>          - * a fault that was already known and is already being 
>>>> handled.
>>>>
>>>>          - *
>>>>
>>>>          - * If there are too many pending page faults, this will 
>>>> fail. Retry
>>>>
>>>>          - * interrupts should be ignored in this case until there 
>>>> is enough
>>>>
>>>>          - * free space.
>>>>
>>>>          - *
>>>>
>>>>          - * Returns 0 if the fault was added, 1 if the fault was 
>>>> already known,
>>>>
>>>>          - * -ENOSPC if there are too many pending faults.
>>>>
>>>>          - */
>>>>
>>>>          -int amdgpu_ih_add_fault(struct amdgpu_device *adev, u64 key)
>>>>
>>>>          -{
>>>>
>>>>          -  unsigned long flags;
>>>>
>>>>          -  int r = -ENOSPC;
>>>>
>>>>          -
>>>>
>>>>          -  if (WARN_ON_ONCE(!adev->irq.ih.faults))
>>>>
>>>>          -          /* Should be allocated in <IP>_ih_sw_init on 
>>>> GPUs that
>>>>
>>>>          -           * support retry faults and require retry 
>>>> filtering.
>>>>
>>>>          -           */
>>>>
>>>>          -          return r;
>>>>
>>>>          -
>>>>
>>>>          - spin_lock_irqsave(&adev->irq.ih.faults->lock, flags);
>>>>
>>>>          -
>>>>
>>>>          -  /* Only let the hash table fill up to 50% for best 
>>>> performance */
>>>>
>>>>          -  if (adev->irq.ih.faults->count >= (1 << 
>>>> (AMDGPU_PAGEFAULT_HASH_BITS-1)))
>>>>
>>>>          -          goto unlock_out;
>>>>
>>>>          -
>>>>
>>>>          -  r = chash_table_copy_in(&adev->irq.ih.faults->hash, 
>>>> key, NULL);
>>>>
>>>>          -  if (!r)
>>>>
>>>>          -          adev->irq.ih.faults->count++;
>>>>
>>>>          -
>>>>
>>>>          -  /* chash_table_copy_in should never fail unless we're 
>>>> losing count */
>>>>
>>>>          -  WARN_ON_ONCE(r < 0);
>>>>
>>>>          -
>>>>
>>>>          -unlock_out:
>>>>
>>>>          - spin_unlock_irqrestore(&adev->irq.ih.faults->lock, flags);
>>>>
>>>>          -  return r;
>>>>
>>>>          -}
>>>>
>>>>          -
>>>>
>>>>          -/**
>>>>
>>>>          - * amdgpu_ih_clear_fault - Remove a page fault record
>>>>
>>>>          - *
>>>>
>>>>          - * @adev: amdgpu device pointer
>>>>
>>>>          - * @key: 64-bit encoding of PASID and address
>>>>
>>>>          - *
>>>>
>>>>          - * This should be called when a page fault has been 
>>>> handled. Any
>>>>
>>>>          - * future interrupt with this key will be processed as a new
>>>>
>>>>          - * page fault.
>>>>
>>>>          - */
>>>>
>>>>          -void amdgpu_ih_clear_fault(struct amdgpu_device *adev, 
>>>> u64 key)
>>>>
>>>>          -{
>>>>
>>>>          -  unsigned long flags;
>>>>
>>>>          -  int r;
>>>>
>>>>          -
>>>>
>>>>          -  if (!adev->irq.ih.faults)
>>>>
>>>>          -          return;
>>>>
>>>>          -
>>>>
>>>>          - spin_lock_irqsave(&adev->irq.ih.faults->lock, flags);
>>>>
>>>>          -
>>>>
>>>>          -  r = chash_table_remove(&adev->irq.ih.faults->hash, key, 
>>>> NULL);
>>>>
>>>>          -  if (!WARN_ON_ONCE(r < 0)) {
>>>>
>>>>          -          adev->irq.ih.faults->count--;
>>>>
>>>>          - WARN_ON_ONCE(adev->irq.ih.faults->count < 0);
>>>>
>>>>          -  }
>>>>
>>>>          -
>>>>
>>>>          - spin_unlock_irqrestore(&adev->irq.ih.faults->lock, flags);
>>>>
>>>>          -}
>>>>
>>>>          diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>>
>>>>          index a23e1c0..f411ffb 100644
>>>>
>>>>          --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>>
>>>>          +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>>
>>>>          @@ -32,13 +32,6 @@ struct amdgpu_device;
>>>>
>>>>           #define AMDGPU_IH_CLIENTID_LEGACY 0
>>>>
>>>>           #define AMDGPU_IH_CLIENTID_MAX SOC15_IH_CLIENTID_MAX
>>>>
>>>>
>>>>          -#define AMDGPU_PAGEFAULT_HASH_BITS 8
>>>>
>>>>          -struct amdgpu_retryfault_hashtable {
>>>>
>>>>          -  DECLARE_CHASH_TABLE(hash, AMDGPU_PAGEFAULT_HASH_BITS, 
>>>> 8, 0);
>>>>
>>>>          -  spinlock_t     lock;
>>>>
>>>>          -  int            count;
>>>>
>>>>          -};
>>>>
>>>>          -
>>>>
>>>>           /*
>>>>
>>>>            * R6xx+ IH ring
>>>>
>>>>            */
>>>>
>>>>          @@ -57,7 +50,6 @@ struct amdgpu_ih_ring {
>>>>
>>>>             bool                   use_doorbell;
>>>>
>>>>             bool                   use_bus_addr;
>>>>
>>>>             dma_addr_t             rb_dma_addr; /* only used when 
>>>> use_bus_addr = true */
>>>>
>>>>          -  struct amdgpu_retryfault_hashtable *faults;
>>>>
>>>>           };
>>>>
>>>>
>>>>           #define AMDGPU_IH_SRC_DATA_MAX_SIZE_DW 4
>>>>
>>>>          @@ -95,7 +87,5 @@ int amdgpu_ih_ring_init(struct 
>>>> amdgpu_device *adev, unsigned ring_size,
>>>>
>>>>                             bool use_bus_addr);
>>>>
>>>>           void amdgpu_ih_ring_fini(struct amdgpu_device *adev);
>>>>
>>>>           int amdgpu_ih_process(struct amdgpu_device *adev);
>>>>
>>>>          -int amdgpu_ih_add_fault(struct amdgpu_device *adev, u64 
>>>> key);
>>>>
>>>>          -void amdgpu_ih_clear_fault(struct amdgpu_device *adev, 
>>>> u64 key);
>>>>
>>>>
>>>>           #endif
>>>>
>>>>          diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>
>>>>          index 1d7e3c1..8b220e9 100644
>>>>
>>>>          --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>
>>>>          +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>
>>>>          @@ -2692,6 +2692,22 @@ void amdgpu_vm_adjust_size(struct 
>>>> amdgpu_device *adev, uint32_t min_vm_size,
>>>>
>>>>                      adev->vm_manager.fragment_size);
>>>>
>>>>           }
>>>>
>>>>
>>>>          +static struct amdgpu_retryfault_hashtable 
>>>> *init_fault_hash(void)
>>>>
>>>>          +{
>>>>
>>>>          +  struct amdgpu_retryfault_hashtable *fault_hash;
>>>>
>>>>          +
>>>>
>>>>          +  fault_hash = kmalloc(sizeof(*fault_hash), GFP_KERNEL);
>>>>
>>>>          +  if (!fault_hash)
>>>>
>>>>          +          return fault_hash;
>>>>
>>>>          +
>>>>
>>>>          +  INIT_CHASH_TABLE(fault_hash->hash,
>>>>
>>>>          +                  AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);
>>>>
>>>>          +  spin_lock_init(&fault_hash->lock);
>>>>
>>>>          +  fault_hash->count = 0;
>>>>
>>>>          +
>>>>
>>>>          +  return fault_hash;
>>>>
>>>>          +}
>>>>
>>>>          +
>>>>
>>>>           /**
>>>>
>>>>            * amdgpu_vm_init - initialize a vm instance
>>>>
>>>>            *
>>>>
>>>>          @@ -2780,6 +2796,12 @@ int amdgpu_vm_init(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>
>>>>                     vm->pasid = pasid;
>>>>
>>>>             }
>>>>
>>>>
>>>>          +  vm->fault_hash = init_fault_hash();
>>>>
>>>>          +  if (!vm->fault_hash) {
>>>>
>>>>          +          r = -ENOMEM;
>>>>
>>>>          +          goto error_free_root;
>>>>
>>>>          +  }
>>>>
>>>>          +
>>>>
>>>>             INIT_KFIFO(vm->faults);
>>>>
>>>>             vm->fault_credit = 16;
>>>>
>>>>
>>>>          @@ -2973,7 +2995,7 @@ void amdgpu_vm_fini(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm)
>>>>
>>>>
>>>>             /* Clear pending page faults from IH when the VM is 
>>>> destroyed */
>>>>
>>>>             while (kfifo_get(&vm->faults, &fault))
>>>>
>>>>          -          amdgpu_ih_clear_fault(adev, fault);
>>>>
>>>>          +          amdgpu_vm_clear_fault(vm->fault_hash, fault);
>>>>
>>>>
>>>>             if (vm->pasid) {
>>>>
>>>>                     unsigned long flags;
>>>>
>>>>          @@ -2983,6 +3005,9 @@ void amdgpu_vm_fini(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm)
>>>>
>>>> spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>
>>>>             }
>>>>
>>>>
>>>>          +  kfree(vm->fault_hash);
>>>>
>>>>          +  vm->fault_hash = NULL;
>>>>
>>>>          +
>>>>
>>>>             drm_sched_entity_destroy(&vm->entity);
>>>>
>>>>
>>>>             if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
>>>>
>>>>          @@ -3183,3 +3208,78 @@ void amdgpu_vm_set_task_info(struct 
>>>> amdgpu_vm *vm)
>>>>
>>>>                     }
>>>>
>>>>             }
>>>>
>>>>           }
>>>>
>>>>          +
>>>>
>>>>          +/**
>>>>
>>>>          + * amdgpu_vm_add_fault - Add a page fault record to fault 
>>>> hash table
>>>>
>>>>          + *
>>>>
>>>>          + * @fault_hash: fault hash table
>>>>
>>>>          + * @key: 64-bit encoding of PASID and address
>>>>
>>>>          + *
>>>>
>>>>          + * This should be called when a retry page fault 
>>>> interrupt is
>>>>
>>>>          + * received. If this is a new page fault, it will be 
>>>> added to a hash
>>>>
>>>>          + * table. The return value indicates whether this is a 
>>>> new fault, or
>>>>
>>>>          + * a fault that was already known and is already being 
>>>> handled.
>>>>
>>>>          + *
>>>>
>>>>          + * If there are too many pending page faults, this will 
>>>> fail. Retry
>>>>
>>>>          + * interrupts should be ignored in this case until there 
>>>> is enough
>>>>
>>>>          + * free space.
>>>>
>>>>          + *
>>>>
>>>>          + * Returns 0 if the fault was added, 1 if the fault was 
>>>> already known,
>>>>
>>>>          + * -ENOSPC if there are too many pending faults.
>>>>
>>>>          + */
>>>>
>>>>          +int amdgpu_vm_add_fault(struct 
>>>> amdgpu_retryfault_hashtable *fault_hash, u64 key)
>>>>
>>>>          +{
>>>>
>>>>          +  unsigned long flags;
>>>>
>>>>          +  int r = -ENOSPC;
>>>>
>>>>          +
>>>>
>>>>          +  if (WARN_ON_ONCE(!fault_hash))
>>>>
>>>>          +          /* Should be allocated in amdgpu_vm_init
>>>>
>>>>          +           */
>>>>
>>>>          +          return r;
>>>>
>>>>          +
>>>>
>>>>          +  spin_lock_irqsave(&fault_hash->lock, flags);
>>>>
>>>>          +
>>>>
>>>>          +  /* Only let the hash table fill up to 50% for best 
>>>> performance */
>>>>
>>>>          +  if (fault_hash->count >= (1 << 
>>>> (AMDGPU_PAGEFAULT_HASH_BITS-1)))
>>>>
>>>>          +          goto unlock_out;
>>>>
>>>>          +
>>>>
>>>>          +  r = chash_table_copy_in(&fault_hash->hash, key, NULL);
>>>>
>>>>          +  if (!r)
>>>>
>>>>          +          fault_hash->count++;
>>>>
>>>>          +
>>>>
>>>>          +  /* chash_table_copy_in should never fail unless we're 
>>>> losing count */
>>>>
>>>>          +  WARN_ON_ONCE(r < 0);
>>>>
>>>>          +
>>>>
>>>>          +unlock_out:
>>>>
>>>>          +  spin_unlock_irqrestore(&fault_hash->lock, flags);
>>>>
>>>>          +  return r;
>>>>
>>>>          +}
>>>>
>>>>          +
>>>>
>>>>          +/**
>>>>
>>>>          + * amdgpu_vm_clear_fault - Remove a page fault record
>>>>
>>>>          + *
>>>>
>>>>          + * @fault_hash: fault hash table
>>>>
>>>>          + * @key: 64-bit encoding of PASID and address
>>>>
>>>>          + *
>>>>
>>>>          + * This should be called when a page fault has been 
>>>> handled. Any
>>>>
>>>>          + * future interrupt with this key will be processed as a new
>>>>
>>>>          + * page fault.
>>>>
>>>>          + */
>>>>
>>>>          +void amdgpu_vm_clear_fault(struct 
>>>> amdgpu_retryfault_hashtable *fault_hash, u64 key)
>>>>
>>>>          +{
>>>>
>>>>          +  unsigned long flags;
>>>>
>>>>          +  int r;
>>>>
>>>>          +
>>>>
>>>>          +  if (!fault_hash)
>>>>
>>>>          +          return;
>>>>
>>>>          +
>>>>
>>>>          +  spin_lock_irqsave(&fault_hash->lock, flags);
>>>>
>>>>          +
>>>>
>>>>          +  r = chash_table_remove(&fault_hash->hash, key, NULL);
>>>>
>>>>          +  if (!WARN_ON_ONCE(r < 0)) {
>>>>
>>>>          +          fault_hash->count--;
>>>>
>>>>          +          WARN_ON_ONCE(fault_hash->count < 0);
>>>>
>>>>          +  }
>>>>
>>>>          +
>>>>
>>>>          +  spin_unlock_irqrestore(&fault_hash->lock, flags);
>>>>
>>>>          +}
>>>>
>>>>          diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>
>>>>          index e275ee7..6eb1da1 100644
>>>>
>>>>          --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>
>>>>          +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>
>>>>          @@ -178,6 +178,13 @@ struct amdgpu_task_info {
>>>>
>>>>             pid_t   tgid;
>>>>
>>>>           };
>>>>
>>>>
>>>>          +#define AMDGPU_PAGEFAULT_HASH_BITS 8
>>>>
>>>>          +struct amdgpu_retryfault_hashtable {
>>>>
>>>>          +  DECLARE_CHASH_TABLE(hash, AMDGPU_PAGEFAULT_HASH_BITS, 
>>>> 8, 0);
>>>>
>>>>          +  spinlock_t     lock;
>>>>
>>>>          +  int            count;
>>>>
>>>>          +};
>>>>
>>>>          +
>>>>
>>>>           struct amdgpu_vm {
>>>>
>>>>             /* tree of virtual addresses mapped */
>>>>
>>>>             struct rb_root_cached  va;
>>>>
>>>>          @@ -240,6 +247,7 @@ struct amdgpu_vm {
>>>>
>>>>             struct ttm_lru_bulk_move lru_bulk_move;
>>>>
>>>>             /* mark whether can do the bulk move */
>>>>
>>>>             bool                   bulk_moveable;
>>>>
>>>>          +  struct amdgpu_retryfault_hashtable *fault_hash;
>>>>
>>>>           };
>>>>
>>>>
>>>>           struct amdgpu_vm_manager {
>>>>
>>>>          @@ -355,4 +363,8 @@ void amdgpu_vm_set_task_info(struct 
>>>> amdgpu_vm *vm);
>>>>
>>>>           void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>>>>
>>>>                                    struct amdgpu_vm *vm);
>>>>
>>>>
>>>>          +int amdgpu_vm_add_fault(struct 
>>>> amdgpu_retryfault_hashtable *fault_hash, u64 key);
>>>>
>>>>          +
>>>>
>>>>          +void amdgpu_vm_clear_fault(struct 
>>>> amdgpu_retryfault_hashtable *fault_hash, u64 key);
>>>>
>>>>          +
>>>>
>>>>           #endif
>>>>
>>>>          diff --git a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>>
>>>>          index 5ae5ed2..acbe5a7 100644
>>>>
>>>>          --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>>
>>>>          +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>>
>>>>          @@ -265,35 +265,36 @@ static bool 
>>>> vega10_ih_prescreen_iv(struct amdgpu_device *adev)
>>>>
>>>>                     return true;
>>>>
>>>>             }
>>>>
>>>>
>>>>          -  addr = ((u64)(dw5 & 0xf) << 44) | ((u64)dw4 << 12);
>>>>
>>>>          -  key = AMDGPU_VM_FAULT(pasid, addr);
>>>>
>>>>          -  r = amdgpu_ih_add_fault(adev, key);
>>>>
>>>>          -
>>>>
>>>>          -  /* Hash table is full or the fault is already being 
>>>> processed,
>>>>
>>>>          -   * ignore further page faults
>>>>
>>>>          -   */
>>>>
>>>>          -  if (r != 0)
>>>>
>>>>          -          goto ignore_iv;
>>>>
>>>>          -
>>>>
>>>>             /* Track retry faults in per-VM fault FIFO. */
>>>>
>>>>             spin_lock(&adev->vm_manager.pasid_lock);
>>>>
>>>>             vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>
>>>>          +  addr = ((u64)(dw5 & 0xf) << 44) | ((u64)dw4 << 12);
>>>>
>>>>          +  key = AMDGPU_VM_FAULT(pasid, addr);
>>>>
>>>>             if (!vm) {
>>>>
>>>>                     /* VM not found, process it normally */
>>>>
>>>> spin_unlock(&adev->vm_manager.pasid_lock);
>>>>
>>>>          -          amdgpu_ih_clear_fault(adev, key);
>>>>
>>>>                     return true;
>>>>
>>>>          +  } else {
>>>>
>>>>          +          r = amdgpu_vm_add_fault(vm->fault_hash, key);
>>>>
>>>>          +
>>>>
>>>>          +          /* Hash table is full or the fault is already 
>>>> being processed,
>>>>
>>>>          +           * ignore further page faults
>>>>
>>>>          +           */
>>>>
>>>>          +          if (r != 0) {
>>>>
>>>>          + spin_unlock(&adev->vm_manager.pasid_lock);
>>>>
>>>>          +                  goto ignore_iv;
>>>>
>>>>          +          }
>>>>
>>>>             }
>>>>
>>>>             /* No locking required with single writer and single 
>>>> reader */
>>>>
>>>>             r = kfifo_put(&vm->faults, key);
>>>>
>>>>             if (!r) {
>>>>
>>>>                     /* FIFO is full. Ignore it until there is space */
>>>>
>>>>          +          amdgpu_vm_clear_fault(vm->fault_hash, key);
>>>>
>>>> spin_unlock(&adev->vm_manager.pasid_lock);
>>>>
>>>>          -          amdgpu_ih_clear_fault(adev, key);
>>>>
>>>>                     goto ignore_iv;
>>>>
>>>>             }
>>>>
>>>>          - spin_unlock(&adev->vm_manager.pasid_lock);
>>>>
>>>>
>>>>          + spin_unlock(&adev->vm_manager.pasid_lock);
>>>>
>>>>             /* It's the first fault for this address, process it 
>>>> normally */
>>>>
>>>>             return true;
>>>>
>>>>
>>>>          @@ -386,14 +387,6 @@ static int vega10_ih_sw_init(void 
>>>> *handle)
>>>>
>>>>             adev->irq.ih.use_doorbell = true;
>>>>
>>>>             adev->irq.ih.doorbell_index = AMDGPU_DOORBELL64_IH << 1;
>>>>
>>>>
>>>>          -  adev->irq.ih.faults = 
>>>> kmalloc(sizeof(*adev->irq.ih.faults), GFP_KERNEL);
>>>>
>>>>          -  if (!adev->irq.ih.faults)
>>>>
>>>>          -          return -ENOMEM;
>>>>
>>>>          - INIT_CHASH_TABLE(adev->irq.ih.faults->hash,
>>>>
>>>>          -                   AMDGPU_PAGEFAULT_HASH_BITS, 8, 0);
>>>>
>>>>          - spin_lock_init(&adev->irq.ih.faults->lock);
>>>>
>>>>          -  adev->irq.ih.faults->count = 0;
>>>>
>>>>          -
>>>>
>>>>             r = amdgpu_irq_init(adev);
>>>>
>>>>
>>>>             return r;
>>>>
>>>>          @@ -406,9 +399,6 @@ static int vega10_ih_sw_fini(void 
>>>> *handle)
>>>>
>>>>             amdgpu_irq_fini(adev);
>>>>
>>>>             amdgpu_ih_ring_fini(adev);
>>>>
>>>>
>>>>          -  kfree(adev->irq.ih.faults);
>>>>
>>>>          -  adev->irq.ih.faults = NULL;
>>>>
>>>>          -
>>>>
>>>>             return 0;
>>>>
>>>>           }
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>      _______________________________________________
>>>>
>>>>      amd-gfx mailing list
>>>>
>>>>      amd-gfx at lists.freedesktop.org 
>>>> <mailto:amd-gfx at lists.freedesktop.org>
>>>>
>>>>      https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



More information about the amd-gfx mailing list