[Intel-gfx] [RFC 23/33] drm/i915: Convert i915_gem_flush_ggtt_writes to intel_gt

[Tvrtko Ursulin]

On 17/06/2019 21:01, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-17 19:12:26)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Having introduced struct intel_gt (named the anonymous structure in i915)
>> we can start using it to compartmentalize our code better. It makes more
>> sense logically to have the code internally like this and it will also
>> help with future split between gt and display in i915.
>>
>> v2:
>>   * Keep ggtt flush before fb obj flush. (Chris)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c    | 12 +++---
>>   .../drm/i915/gem/selftests/i915_gem_mman.c    |  3 +-
>>   drivers/gpu/drm/i915/gt/intel_gt.c            | 41 +++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_gt.h            |  2 +
>>   drivers/gpu/drm/i915/i915_drv.h               |  2 -
>>   drivers/gpu/drm/i915/i915_gem.c               | 40 ------------------
>>   drivers/gpu/drm/i915/i915_vma.c               |  3 +-
>>   drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |  2 +-
>>   8 files changed, 54 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index a4047a585c8b..f58d45ae10d0 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -22,6 +22,7 @@
>>    *
>>    */
>>   
>> +#include "gt/intel_gt.h"
>>   #include "i915_drv.h"
>>   #include "i915_gem_clflush.h"
>>   #include "i915_gem_context.h"
>> @@ -367,7 +368,6 @@ void
>>   i915_gem_object_flush_write_domain(struct drm_i915_gem_object *obj,
>>                                     unsigned int flush_domains)
>>   {
>> -       struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>>          struct i915_vma *vma;
>>   
>>          assert_object_held(obj);
>> @@ -377,17 +377,17 @@ i915_gem_object_flush_write_domain(struct drm_i915_gem_object *obj,
>>   
>>          switch (obj->write_domain) {
>>          case I915_GEM_DOMAIN_GTT:
>> -               i915_gem_flush_ggtt_writes(dev_priv);
>> -
>> -               intel_fb_obj_flush(obj,
>> -                                  fb_write_origin(obj, I915_GEM_DOMAIN_GTT));
>> -
>>                  for_each_ggtt_vma(vma, obj) {
>>                          if (vma->iomap)
>>                                  continue;
>>   
>> +                       intel_gt_flush_ggtt_writes(vma->vm->gt);
>>                          i915_vma_unset_ggtt_write(vma);
>> +                       break;
> 
> So we no longer flush writes from user GGTT mmaps, nor clear all the
> dirty bits? And now despite the move to be gt centric we end up assuming
> that it is still really device centric (breaking after one gt flush).

I did not realize the significance of vma->iomap and the break was not 
supposed to be there, sneaked up after some back and forth refactoring.

So it sounds like it needs to be two loops to preserve existing behaviour.

Regards,

Tvrtko