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

["Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>]

On Mon, Sep 20, 2021 at 12:32:03PM +0200, Modem, Bhanuprakash wrote:
> > From: Srinivas, Vidya <vidya.srinivas@intel.com>
> > Sent: Monday, September 20, 2021 3:43 PM
> > To: Kempczynski, Zbigniew <zbigniew.kempczynski@intel.com>; Modem,
> > Bhanuprakash <bhanuprakash.modem@intel.com>; Joshi, Kunal1
> > <kunal1.joshi@intel.com>
> > Cc: igt-dev@lists.freedesktop.org; markyacoub@google.com
> > Subject: RE: [PATCH i-g-t] lib/intel_bufops: Fix regression on 5.4 kernel
> >
> >
> >
> > > -----Original Message-----
> > > From: Kempczynski, Zbigniew <zbigniew.kempczynski@intel.com>
> > > Sent: Monday, September 20, 2021 3:17 PM
> > > To: Srinivas, Vidya <vidya.srinivas@intel.com>
> > > Cc: igt-dev@lists.freedesktop.org; Modem, Bhanuprakash
> > > <bhanuprakash.modem@intel.com>; markyacoub@google.com
> > > Subject: Re: [PATCH i-g-t] lib/intel_bufops: Fix regression on 5.4 kernel
> > >
> > > On Mon, Sep 20, 2021 at 02:54:55PM +0530, Vidya Srinivas 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
> > > > adds fall back to gem_create when __gem_create_in_memory_region_list
> > > fails.
> > > >
> > > > v2 - Addressed review comments from Zbigniew and Mark Added the fall
> > > > back in __intel_buf_init
> > > >
> > > > v3 - Fixed space issue - review comment from Zbigniew
> > > >
> > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > ---
> > > >  lib/intel_bufops.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c index
> > > > f5f67eddabd7..0236e4d919d5 100644
> > > > --- a/lib/intel_bufops.c
> > > > +++ b/lib/intel_bufops.c
> > > > @@ -818,8 +818,12 @@ static void __intel_buf_init(struct buf_ops
> > > > *bops,
> > > >
> > > >   if (handle)
> > > >           buf->handle = handle;
> > > > - else
> > > > -         buf->handle = gem_create_in_memory_regions(bops->fd,
> > > size, region);
> > > > + else {
> > > > +         if (__gem_create_in_memory_regions(bops->fd, &handle,
> > > size, region) != 0)
> > > > +                 buf->handle = gem_create(bops->fd, size);
> > > > +         else
> > > > +                 buf->handle = handle;
> 
> There is nothing wrong with this code, but I personally feels to update
> as below can be more user understandable (Just throw legacy/incompatible
> stuff to else condition)
> 
> if (!__gem_create_in_memory_regions(bops->fd, &handle, size, region))
>         buf->handle = handle;
> else
>         buf->handle = gem_create(bops->fd, size);
> 
> Acked-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> 
> - Bhanu

Hmm, after looking at wider context we could even do

if (__gem_create_in_memory_regions(bops->fd, &buf->handle, size, region))
         buf->handle = gem_create(bops->fd, size);

But this will likely obscure the intention.

--
Zbigniew

> 
> > > > + }
> > >
> > > Patchwork didn't get my previous rb, so for easier merge:
> > >
> > > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> >
> > Hello Zbigniew,
> >
> > Thank you so much. That is extremely kind of you.
> >
> > @Modem, Bhanuprakash, @Joshi, Kunal1 kindly help with merge if it passes CI.
> >
> > Regards
> > Vidya
> >
> > >
> > > --
> > > Zbigniew
> > >
> > > >
> > > >   set_hw_tiled(bops, buf);
> > > >  }
> > > > --
> > > > 2.33.0
> > > >