From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 13/18] drm/i915: Introduce i915_gem_object_create_stolen() Date: Mon, 5 Nov 2012 17:34:58 +0000 Message-ID: <20121105173458.06094b18@bwidawsk.net> References: <1350666204-8101-1-git-send-email-chris@chris-wilson.co.uk> <1350666204-8101-13-git-send-email-chris@chris-wilson.co.uk> <20121105163226.4a38c7c7@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.chad-versace.us (209-20-75-48.static.cloud-ips.com [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id EFA5F9ECF7 for ; Mon, 5 Nov 2012 09:34:37 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, 05 Nov 2012 16:59:44 +0000 Chris Wilson wrote: > On Mon, 5 Nov 2012 16:32:26 +0000, Ben Widawsky wrote: > > On Fri, 19 Oct 2012 18:03:19 +0100 > > Chris Wilson wrote: > > > > > Allow for the creation of GEM objects backed by stolen memory. As these > > > are not backed by ordinary pages, we create a fake dma mapping and store > > > the address in the scatterlist rather than obj->pages. > > > > > > v2: Mark _i915_gem_object_create_stolen() as static, as noticed by Jesse > > > Barnes. > > > > > > Signed-off-by: Chris Wilson > > > Reviewed-by: Jesse Barnes > > > > Deferring on an r-b for now until I understand the point of most of this > > patch. > > The stolen support is a precursor for fastboot, where we need to wrap > the allocations made by the BIOS from the stolen memory and reuse that > for our own framebuffers. > For some reason I thought this should be simpler than it is, but Jesse has successfully convinced me otherwise. Reviewed-by: Ben Widawsky > > > + struct scatterlist *sg; > > > + > > > > BUG_ON(offset + size <= dev_priv->mm.gtt->stolen_size); > > Done with a minor amendment. > > > > + /* We hide that we have no struct page backing our stolen object > > > + * by wrapping the contiguous physical allocation with a fake > > > + * dma mapping in a single scatterlist. > > > + */ > > > + > > > + st = kmalloc(sizeof(*st), GFP_KERNEL); > > > + if (st == NULL) > > > + return NULL; > > > + > > > + if (!sg_alloc_table(st, 1, GFP_KERNEL)) { > Fixed. > > > > + kfree(st); > > > + return NULL; > > > + } > > > + > > > + sg = st->sgl; > > > + sg->offset = offset; > > > + sg->length = size; > > > + > > > + sg_dma_address(sg) = dev_priv->mm.stolen_base + offset; > > > + sg_dma_len(sg) = size; > > > + > > > > Do we want to make stolen_base a dma_addr_t (or at least typecast it)? > > Interesting enough, the current FBC registers are limited to only using > 32bit addresses, so stolen_base atm is not technically a dma_addr_t. > Maybe I'm picking hairs. :) > > > > + return st; > > > +} > > > + > > > +static int i915_gem_object_get_pages_stolen(struct drm_i915_gem_object *obj) > > > +{ > > > + BUG(); > > > + return -EINVAL; > > > +} > > > + > > > > __noreturn, or maybe just make .get_pages = NULL, and do the check in > > the upper layer get_pages? > > I refer you to http://lwn.net/Articles/336262/ where the argument is put > forth that default no-op functions are preferrable in most cases to > interpretting special NULL vfuncs. We have adopted this elsewhere in > i915.ko to good effect. > > > > + stolen = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096, 0); > > > + if (stolen) > > > + stolen = drm_mm_get_block(stolen, size, 4096); > > > + if (stolen == NULL) > > > + return NULL; > > > > Could probably do slightly better here with ERR_PTR(-ENOMEM) but since > > we don't do that elsewhere, I guess it doesn't matter. > > I was tempted - it would have just looked odd as being the only create > routine to do so. :) > -Chris > -- Ben Widawsky, Intel Open Source Technology Center