From owner-freebsd-x11@FreeBSD.ORG Wed Oct 29 17:03:35 2014 Return-Path: Delivered-To: x11@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AD1FF427; Wed, 29 Oct 2014 17:03:35 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 361547E9; Wed, 29 Oct 2014 17:03:35 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s9TH3UWe008728 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 29 Oct 2014 19:03:30 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s9TH3UWe008728 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s9TH3TM4008727; Wed, 29 Oct 2014 19:03:29 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 29 Oct 2014 19:03:29 +0200 From: Konstantin Belousov To: Tijl Coosemans Subject: Re: [rfc] Radeon AGP support patches Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141027221058.23a188d0@kalimero.tijl.coosemans.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: x11@FreeBSD.org, dumbbell@FreeBSD.org X-BeenThere: freebsd-x11@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: X11 on FreeBSD -- maintaining and support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Oct 2014 17:03:35 -0000 On Mon, Oct 27, 2014 at 10:10:58PM +0100, Tijl Coosemans wrote: > On Mon, 27 Oct 2014 18:27:53 +0200 Konstantin Belousov 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;