Date: Mon, 27 Oct 2014 17:00:55 +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: <20141027170055.10af15e6@kalimero.tijl.coosemans.org> In-Reply-To: <20141027141631.GX1877@kib.kiev.ua> References: <20141026162442.1330d4c3@kalimero.tijl.coosemans.org> <20141027141631.GX1877@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 27 Oct 2014 16:16:31 +0200 Konstantin Belousov <kostikbel@gmail.com> wrote: > On Sun, Oct 26, 2014 at 04:24:42PM +0100, Tijl Coosemans wrote: >> I worked on AGP support for Radeon cards this week. Please take a look >> at the attached patches. >> >> Patch 1: >> Adds support for AGP_USER_TYPES to sys/dev/agp. For normal memory types >> a vm_object is allocated, for user types only a vm_page array. It is >> then up to the caller (e.g. TTM code) to manage this array. Arbitrary >> pages can be mapped into the GTT this way. >> >> Patch 3: >> Enable AGP support in sys/dev/drm2. In PCI mode the GTT exists on the >> graphics card so when accessing system memory it already does its own >> virtual address translation and only physical addresses appear on the >> system bus. The CPU can access the same addresses with its own VM >> system like it always does. In AGP mode, translation is done by the >> AGP chipset so fictitious addresses appear on the system bus. For the >> CPU cache management to work correctly it needs to use these same >> fictitious addresses instead of using the real physical addresses >> directly. The patch marks the AGP aperture range fictitious in >> radeon_device.c where the VRAM aperture is also marked fictitious such >> that PHYS_TO_VM_PAGE in ttm_bo_vm_fault works for addresses in this >> range. >> >> The rest of the patch is mostly porting to our agp_* API. It also >> fixes two memory leaks in ttm_agp_backend.c. One is a missing free in >> ttm_agp_tt_create. The other is because ttm_agp_bind allocates an >> agp_memory struct but ttm_agp_unbind does not free it. So when calling >> ttm_agp_bind a second time the reference to the struct is lost. The >> patch changes ttm_agp_bind so the allocation only happens in the first >> call. The struct is released in ttm_agp_destroy. > Looking at the combination of patch 1 + 3. > > Do you really need to change the container for the AGP_USER_MEMORY ? > Wouldn't it be enough to allocate array in ttm_agp_bind() and copy > pointers to pages from the agp backing object to the array ? 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?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141027170055.10af15e6>