Date: Mon, 27 Oct 2014 22:10:58 +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: <20141027221058.23a188d0@kalimero.tijl.coosemans.org> In-Reply-To: <20141027162753.GB1877@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>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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.
[-- Attachment #2 --]
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) {
+ struct agp_softc *sc = device_get_softc(dev);
+ 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];
+
+ /*
+ * 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);
+ vm_offset_t i;
+
+ 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 += AGP_PAGE_SIZE)
+ AGP_UNBIND_PAGE(dev, offset + i);
+
+ agp_flush_cache();
+ AGP_FLUSH_TLB(dev);
+
+ mtx_unlock(&sc->as_lock);
+ return (0);
+}
Index: sys/dev/agp/agpvar.h
===================================================================
--- sys/dev/agp/agpvar.h (revision 273255)
+++ sys/dev/agp/agpvar.h (working copy)
@@ -122,6 +122,19 @@ int agp_unbind_memory(device_t dev, void
*/
void agp_memory_info(device_t dev, void *handle, struct agp_memory_info *mi);
+/*
+ * Bind a set of pages at a given offset within the AGP aperture.
+ * Returns EINVAL if the given size or offset is not at an AGP page boundary.
+ */
+int agp_bind_pages(device_t dev, vm_page_t *pages, vm_size_t size,
+ vm_offset_t offset);
+
+/*
+ * Unbind a set of pages from the AGP aperture.
+ * Returns EINVAL if the given size or offset is not at an AGP page boundary.
+ */
+int agp_unbind_pages(device_t dev, vm_size_t size, vm_offset_t offset);
+
#define AGP_NORMAL_MEMORY 0
#define AGP_USER_TYPES (1 << 16)
[-- Attachment #3 --]
Index: sys/dev/drm2/drm_agpsupport.c
===================================================================
--- sys/dev/drm2/drm_agpsupport.c (revision 273255)
+++ sys/dev/drm2/drm_agpsupport.c (working copy)
@@ -396,7 +396,7 @@ void *drm_agp_allocate_memory(size_t pag
if (!agpdev)
return NULL;
- return agp_alloc_memory(agpdev, type, pages << AGP_PAGE_SHIFT);
+ return agp_alloc_memory(agpdev, type, pages << PAGE_SHIFT);
}
int drm_agp_free_memory(void *handle)
Index: sys/dev/drm2/radeon/radeon.h
===================================================================
--- sys/dev/drm2/radeon/radeon.h (revision 273255)
+++ sys/dev/drm2/radeon/radeon.h (working copy)
@@ -1618,6 +1618,7 @@ struct radeon_device {
bool need_dma32;
bool accel_working;
bool fictitious_range_registered;
+ bool fictitious_agp_range_registered;
struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
const struct firmware *me_fw; /* all family ME firmware */
const struct firmware *pfp_fw; /* r6/700 PFP firmware */
Index: sys/dev/drm2/radeon/radeon_device.c
===================================================================
--- sys/dev/drm2/radeon/radeon_device.c (revision 273255)
+++ sys/dev/drm2/radeon/radeon_device.c (working copy)
@@ -1014,6 +1014,7 @@ int radeon_device_init(struct radeon_dev
rdev->mc.gtt_size = radeon_gart_size * 1024 * 1024;
rdev->accel_working = false;
rdev->fictitious_range_registered = false;
+ rdev->fictitious_agp_range_registered = false;
/* set up ring ids */
for (i = 0; i < RADEON_NUM_RINGS; i++) {
rdev->ring[i].idx = i;
@@ -1168,6 +1169,24 @@ int radeon_device_init(struct radeon_dev
return (-r);
}
rdev->fictitious_range_registered = true;
+#if __OS_HAS_AGP
+ if (rdev->flags & RADEON_IS_AGP) {
+ DRM_INFO("%s: Taking over the fictitious range 0x%jx-0x%jx\n",
+ __func__, (uintmax_t)rdev->mc.agp_base,
+ (uintmax_t)rdev->mc.agp_base + rdev->mc.gtt_size);
+ r = vm_phys_fictitious_reg_range(
+ rdev->mc.agp_base,
+ rdev->mc.agp_base + rdev->mc.gtt_size,
+ VM_MEMATTR_WRITE_COMBINING);
+ if (r != 0) {
+ DRM_ERROR("Failed to register fictitious range "
+ "0x%jx-0x%jx (%d).\n", (uintmax_t)rdev->mc.agp_base,
+ (uintmax_t)rdev->mc.agp_base + rdev->mc.gtt_size, r);
+ return (-r);
+ }
+ rdev->fictitious_agp_range_registered = true;
+ }
+#endif
if ((radeon_testing & 1)) {
radeon_test_moves(rdev);
@@ -1205,6 +1224,13 @@ void radeon_device_fini(struct radeon_de
rdev->mc.aper_base,
rdev->mc.aper_base + rdev->mc.visible_vram_size);
}
+#if __OS_HAS_AGP
+ if (rdev->fictitious_agp_range_registered) {
+ vm_phys_fictitious_unreg_range(
+ rdev->mc.agp_base,
+ rdev->mc.agp_base + rdev->mc.gtt_size);
+ }
+#endif
radeon_fini(rdev);
#ifdef DUMBBELL_WIP
Index: sys/dev/drm2/radeon/radeon_ttm.c
===================================================================
--- sys/dev/drm2/radeon/radeon_ttm.c (revision 273255)
+++ sys/dev/drm2/radeon/radeon_ttm.c (working copy)
@@ -560,12 +560,10 @@ static struct ttm_tt *radeon_ttm_tt_crea
rdev = radeon_get_rdev(bdev);
#if __OS_HAS_AGP
-#ifdef DUMBBELL_WIP
if (rdev->flags & RADEON_IS_AGP) {
return ttm_agp_tt_create(bdev, rdev->ddev->agp->agpdev,
size, page_flags, dummy_read_page);
}
-#endif /* DUMBBELL_WIP */
#endif
gtt = malloc(sizeof(struct radeon_ttm_tt),
@@ -610,11 +608,9 @@ static int radeon_ttm_tt_populate(struct
rdev = radeon_get_rdev(ttm->bdev);
#if __OS_HAS_AGP
-#ifdef DUMBBELL_WIP
if (rdev->flags & RADEON_IS_AGP) {
return ttm_agp_tt_populate(ttm);
}
-#endif /* DUMBBELL_WIP */
#endif
#ifdef CONFIG_SWIOTLB
@@ -660,12 +656,10 @@ static void radeon_ttm_tt_unpopulate(str
rdev = radeon_get_rdev(ttm->bdev);
#if __OS_HAS_AGP
-#ifdef DUMBBELL_WIP
if (rdev->flags & RADEON_IS_AGP) {
ttm_agp_tt_unpopulate(ttm);
return;
}
-#endif /* DUMBBELL_WIP */
#endif
#ifdef CONFIG_SWIOTLB
Index: sys/dev/drm2/ttm/ttm_agp_backend.c
===================================================================
--- sys/dev/drm2/ttm/ttm_agp_backend.c (revision 273255)
+++ sys/dev/drm2/ttm/ttm_agp_backend.c (working copy)
@@ -41,7 +41,8 @@ __FBSDID("$FreeBSD$");
struct ttm_agp_backend {
struct ttm_tt ttm;
- struct agp_memory *mem;
+ vm_offset_t offset;
+ vm_page_t *pages;
device_t bridge;
};
@@ -51,31 +52,23 @@ static int ttm_agp_bind(struct ttm_tt *t
{
struct ttm_agp_backend *agp_be = container_of(ttm, struct ttm_agp_backend, ttm);
struct drm_mm_node *node = bo_mem->mm_node;
- struct agp_memory *mem;
- int ret, cached = (bo_mem->placement & TTM_PL_FLAG_CACHED);
+ int ret;
unsigned i;
- mem = agp_alloc_memory(agp_be->bridge, AGP_USER_MEMORY, ttm->num_pages);
- if (unlikely(mem == NULL))
- return -ENOMEM;
-
- mem->page_count = 0;
for (i = 0; i < ttm->num_pages; i++) {
vm_page_t page = ttm->pages[i];
if (!page)
page = ttm->dummy_read_page;
- mem->pages[mem->page_count++] = page;
+ agp_be->pages[i] = page;
}
- agp_be->mem = mem;
-
- mem->is_flushed = 1;
- mem->type = (cached) ? AGP_USER_CACHED_MEMORY : AGP_USER_MEMORY;
- ret = agp_bind_memory(mem, node->start);
+ agp_be->offset = node->start * PAGE_SIZE;
+ ret = -agp_bind_pages(agp_be->bridge, agp_be->pages,
+ ttm->num_pages << PAGE_SHIFT, agp_be->offset);
if (ret)
- pr_err("AGP Bind memory failed\n");
+ printf("[TTM] AGP Bind memory failed\n");
return ret;
}
@@ -84,22 +77,16 @@ static int ttm_agp_unbind(struct ttm_tt
{
struct ttm_agp_backend *agp_be = container_of(ttm, struct ttm_agp_backend, ttm);
- if (agp_be->mem) {
- if (agp_be->mem->is_bound)
- return agp_unbind_memory(agp_be->mem);
- agp_free_memory(agp_be->mem);
- agp_be->mem = NULL;
- }
- return 0;
+ return -agp_unbind_pages(agp_be->bridge, ttm->num_pages << PAGE_SHIFT,
+ agp_be->offset);
}
static void ttm_agp_destroy(struct ttm_tt *ttm)
{
struct ttm_agp_backend *agp_be = container_of(ttm, struct ttm_agp_backend, ttm);
- if (agp_be->mem)
- ttm_agp_unbind(ttm);
ttm_tt_fini(ttm);
+ free(agp_be->pages, M_TTM_AGP);
free(agp_be, M_TTM_AGP);
}
@@ -118,14 +105,18 @@ struct ttm_tt *ttm_agp_tt_create(struct
agp_be = malloc(sizeof(*agp_be), M_TTM_AGP, M_WAITOK | M_ZERO);
- agp_be->mem = NULL;
agp_be->bridge = bridge;
agp_be->ttm.func = &ttm_agp_func;
if (ttm_tt_init(&agp_be->ttm, bdev, size, page_flags, dummy_read_page)) {
+ free(agp_be, M_TTM_AGP);
return NULL;
}
+ agp_be->offset = 0;
+ agp_be->pages = malloc(agp_be->ttm.num_pages * sizeof(*agp_be->pages),
+ M_TTM_AGP, M_WAITOK);
+
return &agp_be->ttm;
}
Index: sys/dev/drm2/ttm/ttm_bo_driver.h
===================================================================
--- sys/dev/drm2/ttm/ttm_bo_driver.h (revision 273255)
+++ sys/dev/drm2/ttm/ttm_bo_driver.h (working copy)
@@ -990,9 +990,8 @@ extern vm_memattr_t ttm_io_prot(uint32_t
extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;
-#if (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && defined(MODULE)))
+#if __OS_HAS_AGP
#define TTM_HAS_AGP
-#include <linux/agp_backend.h>
/**
* ttm_agp_tt_create
@@ -1009,7 +1008,7 @@ extern const struct ttm_mem_type_manager
* bind and unbind memory backing a ttm_tt.
*/
extern struct ttm_tt *ttm_agp_tt_create(struct ttm_bo_device *bdev,
- struct agp_bridge_data *bridge,
+ device_t bridge,
unsigned long size, uint32_t page_flags,
struct vm_page *dummy_read_page);
int ttm_agp_tt_populate(struct ttm_tt *ttm);
Index: sys/dev/drm2/ttm/ttm_page_alloc.c
===================================================================
--- sys/dev/drm2/ttm/ttm_page_alloc.c (revision 273255)
+++ sys/dev/drm2/ttm/ttm_page_alloc.c (working copy)
@@ -45,10 +45,6 @@ __FBSDID("$FreeBSD$");
#include <dev/drm2/ttm/ttm_bo_driver.h>
#include <dev/drm2/ttm/ttm_page_alloc.h>
-#ifdef TTM_HAS_AGP
-#include <asm/agp.h>
-#endif
-
#define NUM_PAGES_TO_ALLOC (PAGE_SIZE/sizeof(vm_page_t))
#define SMALL_ALLOCATION 16
#define FREE_ALL_PAGES (~0U)
@@ -220,46 +216,34 @@ static struct ttm_pool_manager *_manager
static int set_pages_array_wb(vm_page_t *pages, int addrinarray)
{
- vm_page_t m;
+#ifdef TTM_HAS_AGP
int i;
- for (i = 0; i < addrinarray; i++) {
- m = pages[i];
-#ifdef TTM_HAS_AGP
- unmap_page_from_agp(m);
+ for (i = 0; i < addrinarray; i++)
+ pmap_page_set_memattr(pages[i], VM_MEMATTR_WRITE_BACK);
#endif
- pmap_page_set_memattr(m, VM_MEMATTR_WRITE_BACK);
- }
return 0;
}
static int set_pages_array_wc(vm_page_t *pages, int addrinarray)
{
- vm_page_t m;
+#ifdef TTM_HAS_AGP
int i;
- for (i = 0; i < addrinarray; i++) {
- m = pages[i];
-#ifdef TTM_HAS_AGP
- map_page_into_agp(pages[i]);
+ for (i = 0; i < addrinarray; i++)
+ pmap_page_set_memattr(pages[i], VM_MEMATTR_WRITE_COMBINING);
#endif
- pmap_page_set_memattr(m, VM_MEMATTR_WRITE_COMBINING);
- }
return 0;
}
static int set_pages_array_uc(vm_page_t *pages, int addrinarray)
{
- vm_page_t m;
+#ifdef TTM_HAS_AGP
int i;
- for (i = 0; i < addrinarray; i++) {
- m = pages[i];
-#ifdef TTM_HAS_AGP
- map_page_into_agp(pages[i]);
+ for (i = 0; i < addrinarray; i++)
+ pmap_page_set_memattr(pages[i], VM_MEMATTR_UNCACHEABLE);
#endif
- pmap_page_set_memattr(m, VM_MEMATTR_UNCACHEABLE);
- }
return 0;
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141027221058.23a188d0>