On Fri, Nov 22, 2019 at 12:57:27PM -0800, Niranjana Vishwanathapura wrote:
+static inline bool +i915_range_done(struct hmm_range *range) +{
- bool ret = hmm_range_valid(range);
- hmm_range_unregister(range);
- return ret;
+}
This needs to be updated to follow the new API, but this pattern is generally wrong, failure if hmm_range_valid should retry the range_fault and so forth. See the hmm.rst.
+static int +i915_range_fault(struct i915_svm *svm, struct hmm_range *range) +{
- long ret;
- range->default_flags = 0;
- range->pfn_flags_mask = -1UL;
- ret = hmm_range_register(range, &svm->mirror);
- if (ret) {
up_read(&svm->mm->mmap_sem);return (int)ret;- }
Using a temporary range is the pattern from nouveau, is it really necessary in this driver?
+static u32 i915_svm_build_sg(struct i915_address_space *vm,
struct hmm_range *range,struct sg_table *st)+{
- struct scatterlist *sg;
- u32 sg_page_sizes = 0;
- u64 i, npages;
- sg = NULL;
- st->nents = 0;
- npages = (range->end - range->start) / PAGE_SIZE;
- /*
* No needd to dma map the host pages and later unmap it, as* GPU is not allowed to access it with SVM. Hence, no need* of any intermediate data strucutre to hold the mappings.*/- for (i = 0; i < npages; i++) {
u64 addr = range->pfns[i] & ~((1UL << range->pfn_shift) - 1);if (sg && (addr == (sg_dma_address(sg) + sg->length))) {sg->length += PAGE_SIZE;sg_dma_len(sg) += PAGE_SIZE;continue;}if (sg)sg_page_sizes |= sg->length;sg = sg ? __sg_next(sg) : st->sgl;sg_dma_address(sg) = addr;sg_dma_len(sg) = PAGE_SIZE;sg->length = PAGE_SIZE;st->nents++;
It is odd to build the range into a sgl.
IMHO it is not a good idea to use the sg_dma_address like this, that should only be filled in by a dma map. Where does it end up being used?
- range.pfn_shift = PAGE_SHIFT;
- range.start = args->start;
- range.flags = i915_range_flags;
- range.values = i915_range_values;
- range.end = range.start + args->length;
- for (i = 0; i < npages; ++i) {
range.pfns[i] = range.flags[HMM_PFN_VALID];if (!(args->flags & I915_BIND_READONLY))range.pfns[i] |= range.flags[HMM_PFN_WRITE];- }
- down_read(&svm->mm->mmap_sem);
- vma = find_vma(svm->mm, range.start);
Why is find_vma needed?
- if (unlikely(!vma || vma->vm_end < range.end)) {
ret = -EFAULT;goto vma_done;- }
+again:
- ret = i915_range_fault(svm, &range);
- if (unlikely(ret))
goto vma_done;- sg_page_sizes = i915_svm_build_sg(vm, &range, &st);
Keep in mind what can be done with the range values is limited until the lock is obtained. Reading the pfns and flags is OK though.
+int i915_svm_bind_mm(struct i915_address_space *vm) +{
- struct i915_svm *svm;
- struct mm_struct *mm;
- int ret = 0;
- mm = get_task_mm(current);
I don't think the get_task_mm(current) is needed, the mmget is already done for current->mm ?
Jason