Re: [igt-dev] [PATCH i-g-t 2/2] lib/i915/intel_memory_region: Add fallback for creating gem bo

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 13 Dec 2021 09:38:36 -0800, Zbigniew Kempczyński wrote:
>
> On Mon, Dec 13, 2021 at 08:45:24AM -0800, Dixit, Ashutosh wrote:
> > On Thu, 09 Dec 2021 12:04:53 -0800, Zbigniew Kempczyński wrote:
> > >
> > > diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_region.c
> > > index fd29eec90..2263f1984 100644
> > > --- a/lib/i915/intel_memory_region.c
> > > +++ b/lib/i915/intel_memory_region.c
> > > @@ -197,8 +197,23 @@ int __gem_create_in_memory_region_list(int fd, uint32_t *handle, uint64_t *size,
> > >		.num_regions = num_regions,
> > >		.regions = to_user_pointer(mem_regions),
> > >	};
> > > +	int ret;
> > >
> > > -	return __gem_create_ext(fd, size, handle, &ext_regions.base);
> > > +	ret = __gem_create_ext(fd, size, handle, &ext_regions.base);
> > > +
> > > +	/*
> > > +	 * Provide fallback for stable kernels if region passed in the array
> > > +	 * can be system memory. In this case we get -ENODEV but still
> > > +	 * we're able to allocate gem bo in system memory using legacy call.
> > > +	 */
> > > +	if (ret == -ENODEV)
> > > +		for (int i = 0; i < num_regions; i++)
> > > +			if (mem_regions[i].memory_class == I915_MEMORY_CLASS_SYSTEM) {
> > > +				ret = __gem_create(fd, size, handle);
> > > +				break;
> > > +			}
> >
> > Shouldn't this function return success if mem_regions[] array contains only
> > a single system memory region entry (that is the sime of mem_regions[]
> > array is 1 and the only entry is system memory)? But here we are returning
> > success if /any/ of the regions passed in are system memory? Won't this
> > cause an error in the caller? I am not sure if checking for -ENODEV return
> > saves us in any way but I think the code should be changed to return
> > success if mem_regions[] array contains only a single system memory region.
>
> If user want to create in one of memory region from the list
> [device memory, system memory] I think we should create gem object in system
> memory as normally kernel would do if it couldn't create within local memory.
> That's why I decided to iterate over request regions and create within
> system memory if it is possible.
>
> Please take a look to conditional in stable kernels:
>
>	if (!IS_ENABLED(CONFIG_DRM_I915_UNSTABLE_FAKE_LMEM))
>
> So query + create_ext should be covered to avoid failures on IGT code
> which works with not drm-tip code.

OK, I think you are right, it's ok to create a smem-only object as long as
just one of the permissible regions for the object is smem. Thanks.

>
> >
> > I think the previous patch "lib/i915/intel_memory_region: Provide system
> > memory region stub" is fine and serves this purpose of returning a single
> > system memory region. Do we even need this patch?
>
> Take a look to results:
> https://intel-gfx-ci.01.org/tree/linux-stable/index-alt.html?
>
> gem_exec_basic requires properly running code:
>
> static uint32_t batch_create(int fd, uint32_t batch_size, uint32_t region)
> {
>	const uint32_t bbe = MI_BATCH_BUFFER_END;
>	uint32_t handle;
>
>	handle = gem_create_in_memory_regions(fd, batch_size, region);
>	gem_write(fd, handle, 0, &bbe, sizeof(bbe));
>
>	return handle;
> }
>
> which will fail on gem_create_in_memory_regions() with -ENODEV. I just
> want to cover also creating objects globally only when system memory
> is available.
>
> --
> Zbigniew
>
> >
> >
> > > +
> > > +	return ret;
> > >  }
> > >
> > >  /* gem_create_in_memory_region_list:
> > > --
> > > 2.26.0
> > >