Re: [PATCH 3/3] drm/i915: mark GEM objects as dirty when written by the CPU

[Dave Gordon <david.s.gordon@intel.com>]

On 08/12/15 17:03, Chris Wilson wrote:
> On Tue, Dec 08, 2015 at 04:51:18PM +0000, Dave Gordon wrote:
>> This patch covers a couple more places where a GEM object is (or may be)
>> modified by means of CPU writes, and should therefore be marked dirty to
>> ensure that the changes are not lost in the evenof of the object is
>> evicted under memory pressure.
>>
>> It may be possible to optimise these paths later, by marking only
>> specific pages of the object as dirty (for objects backed by shmfs
>> pages); but for now let's ensure correctness by dirtying the whole
>> object.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c        | 4 +++-
>>   drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 ++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 12f68f4..36b9539 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -937,7 +937,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
>>   	i915_gem_object_pin_pages(obj);
>>
>>   	offset = args->offset;
>> -	obj->dirty = 1;
>>
>>   	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents,
>>   			 offset >> PAGE_SHIFT) {
>> @@ -1074,6 +1073,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>>   		goto out;
>>   	}
>>
>> +	/* Object backing store will be out of date hereafter */
>> +	obj->dirty = 1;
>
> Possibly. I'd rather just have shmem_pwrite be consistent and use
> set_page_dirty. It is baked into the code that it doesn't access every
> page.

It wasn't the shmem path that was the problem; this line was previously 
inside i915_gem_shmem_pwrite() above. But i915_gem_phys_pwrite() was 
missing the corresponding line, so it was simpler to move marking the 
object dirty up to the top level of the ioctl for now, especially as 
i915_gem_gtt_pwrite_fast() might or might not have marked the object in 
the case where it returns early.

We could at some time in the future devolve object marking to a 
class-specific vfunc, at which point this line would disappear again; 
but we'd have to implement it in each class, or at least the ones that 
users can call pwrite on (shmem, phys, and eventually stolen?). Of 
those, shmem can do per-page dirtying, but phys can stolen can't (stolen 
doesn't even have "struct page" entries available).

Which is why it's simpler to just mark the whole object here and let 
put_pages() deal with it later (if ever -- if the object is never 
actually swapped out then marking the object incurs LESS overhead than 
marking all the pages).

>>   	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>>
>>   	ret = -EFAULT;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> index e9c2bfd..49a74c6 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> @@ -208,6 +208,8 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size
>>   		return ret;
>>
>>   	ret = i915_gem_object_set_to_cpu_domain(obj, write);
>> +	if (write)
>> +		obj->dirty = 1;
>
> No. The accessor here should already be using set_page_dirty.
> -Chris

What function would that be? I can't find any calls to set_page_dirty() 
in this source file. OTOH, does a dmabuf object have shmfs backing store 
anyway?

.Dave.

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