Re: [igt-dev] [i-g-t] lib/i915: Use FIXED mapping only for discrete memory

["Surendrakumar Upadhyay, TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com>]

> -----Original Message-----
> From: Latvala, Petri <petri.latvala@intel.com>
> Sent: 30 August 2021 14:50
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay@intel.com>
> Cc: igt-dev@lists.freedesktop.org
> Subject: Re: [igt-dev] [i-g-t] lib/i915: Use FIXED mapping only for discrete
> memory
> 
> On Mon, Aug 30, 2021 at 12:12:25PM +0300, Surendrakumar Upadhyay,
> TejaskumarX wrote:
> >
> >
> > > -----Original Message-----
> > > From: Latvala, Petri <petri.latvala@intel.com>
> > > Sent: 30 August 2021 14:07
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <tejaskumarx.surendrakumar.upadhyay@intel.com>
> > > Cc: igt-dev@lists.freedesktop.org
> > > Subject: Re: [igt-dev] [i-g-t] lib/i915: Use FIXED mapping only for
> > > discrete memory
> > >
> > > On Fri, Aug 27, 2021 at 06:30:02PM +0530, Tejas Upadhyay wrote:
> > > > The FIXED mapping is only used for discrete, and mapping type is
> > > > pre-defined. This disables the other type of mmap offsets when
> > > > discrete memory is used.
> > > >
> > > > Taken from kernel commit:
> > > > commit 7961c5b60f23 ("drm/i915: Add TTM offset argument to
> mmap.")
> > > >
> > > > Signed-off-by: Tejas Upadhyay
> > > > <tejaskumarx.surendrakumar.upadhyay@intel.com>
> > > > ---
> > > >  include/drm-uapi/i915_drm.h | 1 +
> > > >  lib/i915/gem_mman.c         | 8 +++++++-
> > > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/drm-uapi/i915_drm.h
> > > > b/include/drm-uapi/i915_drm.h index a1c0030c..b46367f2 100644
> > > > --- a/include/drm-uapi/i915_drm.h
> > > > +++ b/include/drm-uapi/i915_drm.h
> > > > @@ -871,6 +871,7 @@ struct drm_i915_gem_mmap_offset {  #define
> > > > I915_MMAP_OFFSET_WC  1  #define I915_MMAP_OFFSET_WB  2
> #define
> > > > I915_MMAP_OFFSET_UC  3
> > > > +#define I915_MMAP_OFFSET_FIXED 4
> > > >
> > > >     /*
> > > >      * Zero-terminated chain of extensions.
> > > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c index
> > > > 0406a0b9..1917c24d 100644
> > > > --- a/lib/i915/gem_mman.c
> > > > +++ b/lib/i915/gem_mman.c
> > > > @@ -75,7 +75,13 @@ bool gem_has_legacy_mmap(int fd)
> > > >
> > > >  bool gem_has_mmap_offset_type(int fd, const struct mmap_offset *t)
> {
> > > > -   return gem_has_mmap_offset(fd) || t->type ==
> > > I915_MMAP_OFFSET_GTT;
> > > > +   if (gem_has_mmap_offset(fd))
> > > > +           if (!gem_has_lmem(fd))
> > > > +                   return !(t->type == I915_MMAP_OFFSET_FIXED);
> > > > +           else
> > > > +                   return !(t->type != I915_MMAP_OFFSET_FIXED);
> > >
> > > Why the !(a == b) stuff?
> > >        ^^^
> >
> > If its discrete memory and offset type is fixed then only its valid
> combination.  Reverse if not discrete then any offset type except fixed is valid
> combination.
> 
> 
> I understand the goal, but this is imho more readable:
> 
> if (gem_has_lmem(fd))
>   return t->type == I915_MMAP_OFFSET_FIXED; else
>   return t->type != I915_MMAP_OFFSET_FIXED;
> 

I agree. Changed accordingly.

Thanks,
Tejas
> 
> 
> --
> Petri Latvala