Date: Thu, 30 Oct 2014 15:31:30 +0100 From: Tijl Coosemans <tijl@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: x11@FreeBSD.org, dumbbell@FreeBSD.org Subject: Re: [rfc] Radeon AGP support patches Message-ID: <20141030153130.4d8c441b@kalimero.tijl.coosemans.org> In-Reply-To: <20141029170329.GI53947@kib.kiev.ua> 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> <20141029170329.GI53947@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 Oct 2014 19:03:29 +0200 Konstantin Belousov <kostikbel@gmail.com> wrote: > 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. Thanks. Just to be sure, did you review 2 & 4 as well?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141030153130.4d8c441b>