[Intel-gfx] [PATCH 10/28] drm/i915/gem: Separate reloc validation into an earlier step

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-06-09 08:47:00)
> 
> On 07/06/2020 23:20, Chris Wilson wrote:
> > Over the next couple of patches, we will want to lock all the modified
> > vma for relocation processing under a single ww_mutex. We neither want
> > to have to include the vma that are skipped (due to no modifications
> > required) nor do we want those to be marked as written too. So separate
> > out the reloc validation into an early step, which we can use both to
> > reject the execbuf before committing to making our changes, and to
> > filter out the unmodified vma.
> > 
> > This does introduce a second pass through the reloc[], but only if we
> > need to emit relocations.
> > 
> > v2: reuse the outer loop, not cut'n'paste.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 145 +++++++++++-------
> >   1 file changed, 86 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 23db79b806db..01ab1e15a142 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -911,9 +911,9 @@ static void eb_destroy(const struct i915_execbuffer *eb)
> >   
> >   static inline u64
> >   relocation_target(const struct drm_i915_gem_relocation_entry *reloc,
> > -               const struct i915_vma *target)
> > +               u64 target)
> >   {
> > -     return gen8_canonical_addr((int)reloc->delta + target->node.start);
> > +     return gen8_canonical_addr((int)reloc->delta + target);
> >   }
> >   
> >   static void reloc_cache_init(struct reloc_cache *cache,
> > @@ -1292,26 +1292,11 @@ static int __reloc_entry_gpu(struct i915_execbuffer *eb,
> >       return 0;
> >   }
> >   
> > -static u64
> > -relocate_entry(struct i915_execbuffer *eb,
> > -            struct i915_vma *vma,
> > -            const struct drm_i915_gem_relocation_entry *reloc,
> > -            const struct i915_vma *target)
> > -{
> > -     u64 target_addr = relocation_target(reloc, target);
> > -     int err;
> > -
> > -     err = __reloc_entry_gpu(eb, vma, reloc->offset, target_addr);
> > -     if (err)
> > -             return err;
> > -
> > -     return target->node.start | UPDATE;
> > -}
> > -
> > -static u64
> > -eb_relocate_entry(struct i915_execbuffer *eb,
> > -               struct eb_vma *ev,
> > -               const struct drm_i915_gem_relocation_entry *reloc)
> > +static int
> > +eb_reloc_prepare(struct i915_execbuffer *eb,
> > +              struct eb_vma *ev,
> > +              const struct drm_i915_gem_relocation_entry *reloc,
> > +              struct drm_i915_gem_relocation_entry __user *user)
> >   {
> >       struct drm_i915_private *i915 = eb->i915;
> >       struct eb_vma *target;
> > @@ -1389,6 +1374,32 @@ eb_relocate_entry(struct i915_execbuffer *eb,
> >               return -EINVAL;
> >       }
> >   
> > +     return 1;
> > +}
> > +
> > +static int
> > +eb_reloc_entry(struct i915_execbuffer *eb,
> > +            struct eb_vma *ev,
> > +            const struct drm_i915_gem_relocation_entry *reloc,
> > +            struct drm_i915_gem_relocation_entry __user *user)
> > +{
> > +     struct eb_vma *target;
> > +     u64 offset;
> > +     int err;
> > +
> > +     /* we've already hold a reference to all valid objects */
> > +     target = eb_get_vma(eb, reloc->target_handle);
> > +     if (unlikely(!target))
> > +             return -ENOENT;
> > +
> > +     /*
> > +      * If the relocation already has the right value in it, no
> > +      * more work needs to be done.
> > +      */
> > +     offset = gen8_canonical_addr(target->vma->node.start);
> > +     if (offset == reloc->presumed_offset) > +               return 0;
> > +
> 
> Haven't these reloc entries been removed from the list in the prepare phase?

No, we don't adjust the user reloc arrays, we only skip entire objects
that do not require relocs.
-Chris