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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
