On Wed, 27 Sep 2017 10:15:23 -0700 Eric Anholt <[email protected]> wrote:
> Boris Brezillon <[email protected]> writes: > > > On Wed, 27 Sep 2017 15:24:16 +0100 > > Chris Wilson <[email protected]> wrote: > > > >> Quoting Boris Brezillon (2017-09-27 15:06:53) > >> > On Wed, 27 Sep 2017 14:50:10 +0100 > >> > Chris Wilson <[email protected]> wrote: > >> > > >> > > Quoting Boris Brezillon (2017-09-27 14:45:17) > >> > > > static struct vc4_bo * > >> > > > vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const > >> > > > char *name) > >> > > > { > >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, > >> > > > uint32_t size, const char *name) > >> > > > return NULL; > >> > > > } > >> > > > > >> > > > + if (vc4_bo_purgeable(bo, false)) { > >> > > > + mtx_unlock(&cache->lock); > >> > > > + return NULL; > >> > > > >> > > So this would just mean that the bo was purged in the meantime. Why not > >> > > just try to use the next one in the cache or allocate afresh? > >> > > >> > No, this means the BO was purged and the kernel failed to allocate the > >> > memory back. We don't care about the retained status here, because we > >> > don't need to restore BO's content, that's why we're not checking > >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to > >> > fail with the same ENOMEM error because both path use the CMA mem. > >> > >> Hmm, you don't treat purging as permanent. But you do track the lose of > >> contents, so retained is false? > > > > vc4_bo_purgeable() is not reporting the retained status, it just > > reports whether the BO can be used or not. I can change > > vc4_bo_purgeable() semantic to return 1 if the BO content was retained, > > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM) > > if you prefer, but in the end, all I'll check here is > > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained > > status in this specific use case, all I care about is whether the BO can > > be re-used or not (IOW, is there a valid CMA region attached to the BO). > > > >> > >> I took a harder line, and said that userspace should recreate the object > >> from scratch after it was purged. I thought that would be easier > >> overall. But maybe not.:) > > > > Well, maybe I'm wrong in how I implemented this > > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been > > purged and someone marks it back as unpurgeable I'm trying to > > re-allocate BO's buffer in the ioctl path, and if the CMA allocation > > fails I return -ENOMEM. I could move the allocation in the fault > > handler, but this would result in pretty much the same behavior except > > it would require an extra page-fault to realize the memory is not > > available or force us to check the retained status and decide to > > release the BO object from the BO cache. > > Hmm. The downside I see to this plan is if we eventually decide to have > the purge operation not clear all the BOs, then we would probably rather > have userspace freeing objects that had been purged until it finds one > in the cache that hadn't been purged, rather than forcing reallocation > of this BO now (and possibly then purging something from elsewhere in > the cache). Okay, that's a good reason to move dma_alloc_wc() in the page-fault path. I need to change a bit the implementation to check cma_gem->vaddr value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we might pass a non-allocated BO to the GPU/Display-Engine. _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
