[igt-dev] [PATCH i-g-t] lib/i915/intel_memory_region: Fix regression on 5.4 kernel

[Srinivas, Vidya]

> -----Original Message-----
> From: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>
> Sent: Friday, September 17, 2021 12:52 PM
> To: Mark Yacoub <markyacoub at chromium.org>
> Cc: Srinivas, Vidya <vidya.srinivas at intel.com>; Development mailing list for
> IGT GPU Tools <igt-dev at lists.freedesktop.org>; Modem, Bhanuprakash
> <bhanuprakash.modem at intel.com>; Mark Yacoub
> <markyacoub at google.com>
> Subject: Re: [igt-dev] [PATCH i-g-t] lib/i915/intel_memory_region: Fix
> regression on 5.4 kernel
> 
> On Thu, Sep 16, 2021 at 11:25:30AM -0400, Mark Yacoub wrote:
> > On Thu, Sep 16, 2021 at 10:24 AM Vidya Srinivas
> > <vidya.srinivas at intel.com> wrote:
> > >
> > > Starting commit 8759c4a3020ce4 "Add intel_buf_init_in_region"
> > > __intel_buf_init uses gem_create_in_memory_regions instead of
> > > gem_create. Older kernels like 5.4 still dont have support for
> > > I915_GEM_CREATE_EXT_MEMORY_REGIONS (i915_gem_create_ext_ioctl)
> from
> > > kernel commit
> > > (https://patchwork.freedesktop.org/patch/431581/?series=89648&rev=1)
> > > Due to this, the flip-vs-fences tests are failing on kernel 5.4
> > > Patch add roll back to gem_create when
> __gem_create_in_memory_region_list fails.
> > >
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
> > > ---
> > >  lib/i915/intel_memory_region.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/i915/intel_memory_region.c
> > > b/lib/i915/intel_memory_region.c index 3de40549319b..323b96bad232
> > > 100644
> > > --- a/lib/i915/intel_memory_region.c
> > > +++ b/lib/i915/intel_memory_region.c
> > > @@ -209,7 +209,9 @@ uint32_t gem_create_in_memory_region_list(int
> fd, uint64_t size,
> > >         uint32_t handle;
> > >         int ret = __gem_create_in_memory_region_list(fd, &handle, size,
> > >                                                      mem_regions, num_regions);
> > > -       igt_assert_eq(ret, 0);
> > > +       if (ret != 0)
> > > +               handle = gem_create(fd, size);
> 
> > I'm not sure if this change should be here or should be at the caller
> > `void __intel_buf_init(...).
> 
> I agree, silent fallback to system memory when mem_regions could contain
> only device memory is definitely not we want.
> 
> You may leave this function intact and call underscored (not-asserting)
> version of __gem_create_in_memory_regions() in __intel_buf_init(), then
> react to result. I mean if region _is_ system memory fallback to gem_create()
> is possible, otherwise assert.
> 
> --
> Zbigniew

Thank you very much Zbigniew and Mark. I have made the suggested modification and submitted
Kindly have a check https://patchwork.freedesktop.org/patch/454570/?series=94761&rev=3
I guess I should've passed buf->handle directly to __gem_create_in_memory_regions. 
As of now, I just passed &handle.

Regards
Vidya

> 
> > > +
> > Let's have a check before we return, parallel to the removed `
> > igt_assert_eq(ret, 0);`, Something like `igt_assert(!ret || handle).`
> > >         return handle;
> > >  }
> > >
> > > --
> > > 2.33.0
> > >