[igt-dev] [PATCH i-g-t 1/1] lib/intel_allocator: Check validity of the file descriptor

[Zbigniew Kempczyński]

On Wed, Jun 16, 2021 at 03:05:50PM +0300, Petri Latvala wrote:
> On Wed, Jun 16, 2021 at 01:37:14PM +0200, Andrzej Turko wrote:
> > All procedures for opening particular types of allocators
> > query the gtt size. Even if the user has provided the
> > boundaries of the address space managed by the allocator,
> > the query is performed for the sake of validation.
> > 
> > gem_aperture_size() called with an invalid id returns a
> > default value instead of failing. Thus, we need to
> > additionally check whether the file descriptor is valid.
> > 
> > Signed-off-by: Andrzej Turko <andrzej.turko at linux.intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > ---
> >  lib/intel_allocator.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/lib/intel_allocator.c b/lib/intel_allocator.c
> > index 96f839d4b..c060e10c3 100644
> > --- a/lib/intel_allocator.c
> > +++ b/lib/intel_allocator.c
> > @@ -166,6 +166,15 @@ static inline void map_entry_free_func(struct igt_map_entry *entry)
> >  	free(entry->data);
> >  }
> >  
> > +static bool can_report_gtt_size(int fd)
> > +{
> > +	struct drm_i915_gem_context_param p = {
> > +		.param = I915_CONTEXT_PARAM_GTT_SIZE
> > +	};
> > +
> > +	return (__gem_context_get_param(fd, &p) == 0);
> > +}
> > +
> >  static uint64_t __handle_create(struct allocator *al)
> >  {
> >  	struct handle_entry *h = malloc(sizeof(*h));
> > @@ -271,6 +280,8 @@ static struct intel_allocator *intel_allocator_create(int fd,
> >  {
> >  	struct intel_allocator *ial = NULL;
> >  
> > +	igt_assert(can_report_gtt_size(fd));
> > +
> 
> Is igt_assert correct here (as opposed to igt_require)? Or in other
> words, is inability to report gtt size a kernel bug or not?

In this case yes, we want to assert. Let me try to explain the scenario.


1. allocator thread acts for children, for example we got 
   fd i915 == 3 and ctx == 0

2. child 1 does: 
   
   ahnd = intel_allocator_open(i915 == 3, ctx == 0);

   -> allocator (inside simple) calls gem_aperture_size(i915)
      to find out vm size - this works perfect

3. child 2 does: 
   
   i915 = gem_reopen_driver(i915);
   ahnd = intel_allocator_open(i915 == 5, ctx == 0);

   -> allocator does same what child 1, but gem_aperture_size(i915)
      is called on invalid from allocator process handle. 
   
Current implementation of gem_aperture_size() calls 
gem_global_aperture_size() what resets errno on error returning
predefined 256MB size.

We want to avoid such mistakes in the future in allocator. Plan is:

1. introduce igt_assert() on such case. Currently does nothing because
   there's no test who does child 2 scenario (but soon will be).

2. put failing test to api_intel_allocator.c (which exercise above 
   child 2 scenario).

3. fix allocator -> drop calling gem_aperture_size() on allocator
   thread and move it to the caller (it has active fd to kernel).
   Thats will drop allocator thread i915 fd dependency and this 
   is what we want.

One scenario is we cannot handle - two or more children reopens 
driver. In such case they may have same i915 fd so they starts
sharing vm instead having separate. But we still have possibility
to use (i915, vm) as a key so in such scenario we may use child
pid as vmid, then each child will have separate vm for allocations.
Or if child will call intel_allocator_init() it will totally 
drop allocator thread arbitration -> this reinitializes allocator
data within child so it comes standalone allocator.

--
Zbigniew


> 
> 
> -- 
> Petri Latvala
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev