[RFC 06/13] drm/i915/svm: Page table mirroring support

[Niranjan Vishwanathapura]

On Sat, Nov 23, 2019 at 11:53:52PM +0000, Jason Gunthorpe wrote:
>On Fri, Nov 22, 2019 at 08:44:18PM -0800, Niranjan Vishwanathapura wrote:
>> On Fri, Nov 22, 2019 at 11:33:12PM +0000, Jason Gunthorpe wrote:
>> > 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.
>> >
>>
>> Thanks Jason for the feedback.
>> Yah, will update as per new API in the next revision.
>> The caller of this function is indeed retrying if the range is not valid.
>> But I got the point. I made changes in my local build as per hmm.rst
>> and it is working fine. Will include the change in next revision.
>
>Generally speaking this locking approach is a live-lockable
>collision-retry scheme.
>
>For maintainability it is best if the retry loop is explicit and
>doesn't unregister the range during the retry. I also encourage adding
>an absolute time bound to the retry as it *could* live lock.
>

Ok, thanks for the suggestion, will do.

>> > > +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?
>>
>> Yah, not required. In my local build I tried with proper default_flags
>> and set pfn_flags_mask to 0 and it is working fine.
>
>Sorry, I ment calling hmm_range_register during fault processing.
>
>If your driver works around user space objects that cover a VA then
>the range should be created when the object is created.
>

Oh ok. No, there is no user space object here.
Binding the user space object to device page table is handled in
patch 03 of this series (no HMM there).
This is for binding a system allocated (malloc) memory. User calls
the bind ioctl with the VA range.

>> > > +	/*
>> > > +	 * 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?
>>
>> The sgl is used to plug into the page table update function in i915.
>>
>> For the device memory in discrete card, we don't need dma map which
>> is the case here.
>
>How did we get to device memory on a card? Isn't range->pfns a CPU PFN
>at this point?
>
>I'm confused.
>

Device memory plugin is done through devm_memremap_pages() in patch 07 of
this series. In that patch, we convert the CPU PFN to device PFN before
building the sgl (this is similar to the nouveau driver).

>> > > +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 ?
>>
>> No, I don't think mmget is already done for current->mm here.
>
>I'm not certain, but I thought it is already done because it is
>'current' and current cannot begin to destroy the mm while a syscall
>is currently running.
>

Ok, I don't know, at least the driver is not calling it.
But it makes sense to me. I will remove get_task_mm and directly use current->mm.

Niranjana

>Jason