Re: [PATCH] Always mark GEM objects as dirty when written by the CPU

From: Dave Gordon <david.s.gordon@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] Always mark GEM objects as dirty when written by the CPU
Date: Fri, 11 Dec 2015 12:19:09 +0000	[thread overview]
Message-ID: <566ABF3D.1050705@intel.com> (raw)
In-Reply-To: <20151210085822.GC20822@phenom.ffwll.local>

On 10/12/15 08:58, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 12:51:49PM +0000, Dave Gordon wrote:
>> I think I missed i915_gem_phys_pwrite().
>>
>> i915_gem_gtt_pwrite_fast() marks the object dirty for most cases (vit
>> set_to_gtt_domain(), but isn't called for all cases (or can return before
>> the set_domain). Then we try i915_gem_shmem_pwrite() for non-phys
>> objects (no check for stolen!) and that already marks the object dirty
>> [aside: we might be able to change that to page-by-page?], but
>> i915_gem_phys_pwrite() doesn't mark the object dirty, so we might lose
>> updates there?
>>
>> Or maybe we should move the marking up into i915_gem_pwrite_ioctl() instead.
>> The target object is surely going to be dirtied, whatever type it is.
>
> phys objects are special, and when binding we create allocate new
> (contiguous) storage. In put_pages_phys that gets copied back and pages
> marked as dirty. While a phys object is pinned it's a kernel bug to look
> at the shmem pages and a userspace bug to touch the cpu mmap (since that
> data will simply be overwritten whenever the kernel feels like).
>
> phys objects are only used for cursors on old crap though, so ok if we
> don't streamline this fairly quirky old ABI.
> -Daniel

So is pread broken already for 'phys' ? In the pwrite code, we have 
i915_gem_phys_pwrite() which look OK, but there isn't a corresponding 
i915_gem_phys_pread(), instead it will call i915_gem_shmem_pread(), and 
I'm not sure that will work! The question being, does the kernel have 
page table slots corresponding to the DMA area allocated, otherwise
the for_each_sg_page()/sg_page_iter_page() in i915_gem_shmem_pread() 
isn't going to give meaningful results. And I found this comment in 
drm_pci_alloc() (called from i915_gem_object_attach_phys()):

         /* XXX - Is virt_to_page() legal for consistent mem? */
         /* Reserve */
         for (addr = (unsigned long)dmah->vaddr, sz = size;
              sz > 0; addr += PAGE_SIZE, sz -= PAGE_SIZE) {
                 SetPageReserved(virt_to_page((void *)addr));
         }

(and does it depend on which memory configuration is selected?).

See also current thread on "Support for pread/pwrite from/to non shmem 
backed objects" ...

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx