On Wed, Apr 13, 2016 at 01:30:39PM +0100, Tvrtko Ursulin wrote:
> As long as it remains hidden in here, otherwise is a bit heavy and
> rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return
> pointer undocumented). And if it stays here BUG_ON is redundant.
> Hm.. leaning towards the direct container_of below to bypass all
> these considerations.
Reasonable. I do value the shorthand to_ggtt() though. Ok, the shorthand
can wait until we have more use cases.
> > static int
> > i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >
> >@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> > return obj->base.size;
> > }
> > }
> >+
> >+void *i915_vma_iomap(struct i915_vma *vma)
>
> Kerneldoc! :)
Sorry. Should have double checked with your patch.
> >+{
> >+ if (WARN_ON(!vma->obj->map_and_fenceable))
> >+ return ERR_PTR(-ENODEV);
> >+
> >+ BUG_ON(!vma->is_ggtt);
> >+ BUG_ON((vma->bound & GLOBAL_BIND) == 0);
>
> Maybe aggregate the two into a single WARN_ON and return -EINVAL ?
> Since we easily can sounds better.
>
> >+
> >+ if (vma->iomap == NULL) {
> >+ struct i915_ggtt *ggtt = to_ggtt(vma->vm);
> >+ void *ptr;
> >+
> >+ ptr = io_mapping_map_wc(ggtt->mappable,
> >+ vma->node.start,
> >+ vma->node.size);
> >+ if (ptr == NULL) {
> >+ int ret;
> >+
> >+ /* Too many areas already allocated? */
> >+ ret = i915_gem_evict_vm(vma->vm, true);
>
> I don't know much about the iomapping bussiness. Can we exhaust the
> address space similar to vmap and would then interfere with our own
> (or other?) call sites doing plain io_mapping_map* calls?
It's vmap underneath. My thinking here was that if we couldn't allocate
a new ioremap_wc, the obvious candidates were to reap everything inside
the GGTT i.e. inactive ioremap_wc.
But the question is, do we want to reap vma->iomaps upon vmap_purge.
Yes, we probably should - simplest will be to do a similar forced GGTT
eviction from vmap_purge and refine from there.
> >+ vma = i915_gem_obj_to_ggtt(obj);
> >+
> > /* setup aperture base/size for vesafb takeover */
> > info->apertures->ranges[0].base = dev->mode_config.fb_base;
> > info->apertures->ranges[0].size = ggtt->mappable_end;
> >
> >- info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> >- info->fix.smem_len = size;
> >+ info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> >+ info->fix.smem_len = vma->node.size;
> >
> >- info->screen_base =
> >- ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> >- size);
> >- if (!info->screen_base) {
> >+ info->screen_base = i915_vma_iomap(vma);
>
> We are slowly establishing that ERR_PTR should not be stored in
> structure members but I suppose here we can allow it since the
> structure is freed if it fails.
I know, I tell everyone else off for doing it like this as well.
Practice what you preach!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
