[rfc] Radeon AGP support patches

[Konstantin Belousov]

On Mon, Oct 27, 2014 at 10:10:58PM +0100, Tijl Coosemans wrote:
> On Mon, 27 Oct 2014 18:27:53 +0200 Konstantin Belousov <kostikbel at gmail.com> wrote:
> > On Mon, Oct 27, 2014 at 05:00:55PM +0100, Tijl Coosemans wrote:
> >> In ttm_agp_bind the ttm->pages array is already populated.  These are
> >> the pages that need to be put into the GTT.  The patch modifies struct
> >> agp_memory in sys/dev/agp such that ttm->pages can be passed via
> >> agp_bind_memory.  Maybe it would be better to add two new functions to
> >> sys/dev/agp/agp.c: agp_bind_pages and agp_unbind_pages.  These would
> >> take a vm_page_t array as argument and bind/unbind the pages directly
> >> in the GTT.  There's no need for ttm_agp_bind to call agp_alloc_memory
> >> then and struct agp_memory would not be involved at all.  Does that
> >> sound better?
> > 
> > Yes, this approach is much better IMO.  Having discriminated storage
> > for the bound pages is too ugly;
> 
> New patch 1 & 3 attached.
> 
> > was the whole code audited for correctness after the change ?
> 
> I'm fairly confident these patches are all that's needed yes.  I made
> a first implementation on Sunday afternoon.  It got to the point that
> X showed a mouse pointer and background colour and then it crashed.
> It took the rest of the week to figure out why (NULL dereference in
> ttm_bo_vm_fault) and how to solve it (mark aperture range fictitious).
> It's hard to debug something without a screen.  I read the code front
> to back and back to front in that time and compared it with the old DRM
> code and with the Linux DRM code.  That's where patch 2 & 4 come from.
Looks good, commit. See trivial notes below.

> Index: sys/dev/agp/agp.c
> ===================================================================
> --- sys/dev/agp/agp.c	(revision 273255)
> +++ sys/dev/agp/agp.c	(working copy)
> @@ -996,3 +996,68 @@ void agp_memory_info(device_t dev, void 
>  	mi->ami_offset = mem->am_offset;
>  	mi->ami_is_bound = mem->am_is_bound;
>  }
> +
> +int agp_bind_pages(device_t dev, vm_page_t *pages, vm_size_t size,
> +		   vm_offset_t offset) {
Style:
return type of separate line, continuation line should use 4 spaces.
Opening '{' on the new line.

> +	struct agp_softc *sc = device_get_softc(dev);
Initialization in declaration.

> +	vm_offset_t i, j, k, pa;
> +	vm_page_t m;
> +	int error;
> +
> +	if ((size & (AGP_PAGE_SIZE - 1)) != 0 ||
> +	    (offset & (AGP_PAGE_SIZE - 1)) != 0)
> +		return (EINVAL);
> +
> +	mtx_lock(&sc->as_lock);
> +	for (i = 0; i < size; i += PAGE_SIZE) {
> +		m = pages[i >> PAGE_SHIFT];
Use OFF_TO_IDX().

> +
> +		/*
> +		 * Install entries in the GATT, making sure that if
> +		 * AGP_PAGE_SIZE < PAGE_SIZE and size is not
> +		 * aligned to PAGE_SIZE, we don't modify too many GATT 
> +		 * entries.
> +		 */
> +		for (j = 0; j < PAGE_SIZE && i + j < size; j += AGP_PAGE_SIZE) {
> +			pa = VM_PAGE_TO_PHYS(m) + j;
> +			AGP_DPF("binding offset %#jx to pa %#jx\n",
> +				(uintmax_t)offset + i + j, (uintmax_t)pa);
> +			error = AGP_BIND_PAGE(dev, offset + i + j, pa);
> +			if (error) {
> +				/*
> +				 * Bail out. Reverse all the mappings.
> +				 */
> +				for (k = 0; k < i + j; k += AGP_PAGE_SIZE)
> +					AGP_UNBIND_PAGE(dev, offset + k);
> +
> +				mtx_unlock(&sc->as_lock);
> +				return (error);
> +			}
> +		}
> +	}
> +
> +	agp_flush_cache();
> +	AGP_FLUSH_TLB(dev);
> +
> +	mtx_unlock(&sc->as_lock);
> +	return (0);
> +}
> +
> +int agp_unbind_pages(device_t dev, vm_size_t size, vm_offset_t offset) {
> +	struct agp_softc *sc = device_get_softc(dev);
Same style problems.
> +	vm_offset_t i;