Re: [PATCH 13/18] drm/i915: Introduce i915_gem_object_create_stolen()

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, 05 Nov 2012 16:59:44 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 5 Nov 2012 16:32:26 +0000, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Fri, 19 Oct 2012 18:03:19 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> 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 <chris@chris-wilson.co.uk>
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > 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 <ben@bwidawsk.net>

> > > +	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