Date: Wed, 29 Oct 2014 19:03:29 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Tijl Coosemans <tijl@FreeBSD.org> Cc: x11@FreeBSD.org, dumbbell@FreeBSD.org Subject: Re: [rfc] Radeon AGP support patches Message-ID: <20141029170329.GI53947@kib.kiev.ua> In-Reply-To: <20141027221058.23a188d0@kalimero.tijl.coosemans.org> References: <20141026162442.1330d4c3@kalimero.tijl.coosemans.org> <20141027141631.GX1877@kib.kiev.ua> <20141027170055.10af15e6@kalimero.tijl.coosemans.org> <20141027162753.GB1877@kib.kiev.ua> <20141027221058.23a188d0@kalimero.tijl.coosemans.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141029170329.GI53947>