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

[Jerome Glisse]

On Mon, Nov 25, 2019 at 01:24:18PM +0000, Jason Gunthorpe wrote:
> 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

Yeah, nouveau has different code path but this is because nouveau
driver architecture allows it, i do not see any easy way to hammer
this inside i915 current architecture. I will ponder on this a bit
more.

Cheers,
Jérôme