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

[Jason Gunthorpe]

On Sun, Nov 24, 2019 at 01:12:47PM -0800, Niranjan Vishwanathapura wrote:

> > > > 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).

But earlier just called hmm_range_fault(), it can return all kinds of
pages. If these are only allowed to be device pages here then that
must be checked (under lock)

And putting the cpu PFN of a ZONE_DEVICE device page into
sg_dma_address still looks very wrong to me

Jason