[igt-dev] [RFC 5/5] igt/lib: If mappable aperture is missing return 0 size

[Chris Wilson]

Quoting Chris Wilson (2019-03-07 16:45:35)
> Quoting Antonio Argenziano (2019-03-07 16:29:35)
> > 
> > 
> > On 07/03/19 07:58, Chris Wilson wrote:
> > > Quoting Antonio Argenziano (2019-03-07 15:51:28)
> > >>
> > >>
> > >> On 21/02/19 13:51, Chris Wilson wrote:
> > >>> Quoting Antonio Argenziano (2019-02-21 21:45:46)
> > >>>>
> > >>>>
> > >>>> On 21/02/19 12:01, Chris Wilson wrote:
> > >>>>> Quoting Antonio Argenziano (2019-02-21 19:27:45)
> > >>>>>> So far the aperture size has been read directly from the bar,
> > >>>>>> in this patch we return zero if the mappable aperture is not available
> > >>>>>> as the value stored in the bar might not be accurate. The patch also adds a
> > >>>>>> 'require' when a call to gem_mappable_aperture_size() is made so that
> > >>>>>> the aperture is guaranteed to exist before checking the size.
> > >>>>>>
> > >>>>>> Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > >>>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > >>>>>> Cc: Matthew Auld <matthew.auld at intel.com>
> > >>>>>> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> > >>>>>> ---
> > >>>>>>     lib/ioctl_wrappers.c            | 28 +++++++++++++++++++---------
> > >>>>>>     lib/ioctl_wrappers.h            |  2 +-
> > >>>>>>     tests/i915/gem_concurrent_all.c | 12 ++++++------
> > >>>>>>     tests/i915/gem_cpu_reloc.c      | 14 ++++++++++----
> > >>>>>>     tests/i915/gem_mmap.c           |  4 ++--
> > >>>>>>     tests/i915/gem_mmap_gtt.c       | 10 +++++-----
> > >>>>>>     tests/i915/gem_pwrite.c         |  4 ++--
> > >>>>>>     tests/i915/gem_shrink.c         |  2 +-
> > >>>>>>     tests/i915/i915_pm_rpm.c        |  2 +-
> > >>>>>>     tests/kms_flip.c                |  2 +-
> > >>>>>>     tests/prime_mmap.c              |  7 +++++--
> > >>>>>>     11 files changed, 53 insertions(+), 34 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > >>>>>> index 25dd8ad3..ab7c7a5e 100644
> > >>>>>> --- a/lib/ioctl_wrappers.c
> > >>>>>> +++ b/lib/ioctl_wrappers.c
> > >>>>>> @@ -1440,19 +1440,14 @@ uint64_t gem_aperture_size(int fd)
> > >>>>>>            return aperture_size;
> > >>>>>>     }
> > >>>>>>     
> > >>>>>> -/**
> > >>>>>> - * gem_mappable_aperture_size:
> > >>>>>> - *
> > >>>>>> - * Feature test macro to query the kernel for the mappable gpu aperture size.
> > >>>>>> - * This is the area available for GTT memory mappings.
> > >>>>>> - *
> > >>>>>> - * Returns: The mappable gtt address space size.
> > >>>>>> - */
> > >>>>>> -uint64_t gem_mappable_aperture_size(void)
> > >>>>>> +static uint64_t __gem_mappable_aperture_size(int fd)
> > >>>>>>     {
> > >>>>>>            struct pci_device *pci_dev = intel_get_pci_device();
> > >>>>>>            int bar;
> > >>>>>>     
> > >>>>>> +       if (!gem_mmap__has_gtt(fd))
> > >>>>>> +               return 0; /* Aperture not available */
> > >>>>>> +
> > >>>>>>            if (intel_gen(pci_dev->device_id) < 3)
> > >>>>>>                    bar = 0;
> > >>>>>
> > >>>>> I swear we made this accessible via fd so that we didn't need root to
> > >>>>> poke around libpciacess.
> > >>>>
> > >>>> mhh, couldn't find it. Is it in some IOCTL?
> > >>>
> > >>> No, we extended get_aperture_ioctl to provide it, once upon a time. I
> > >>> suggest we do something similar, or we use a param to get the actual
> > >>> value. (Even if it's legacy, we can still justify a usecase as I'm
> > >>> pretty sure 256MiB is hardcoded into several legacy userspaces.)
> > >>
> > >> Is something like: https://paste.debian.net/1072096/ acceptable?
> > > 
> > > Yes.
> > > 
> > > Kernel->    old                   new
> > > Userspace
> > >     v
> > > 
> > >    old       no change             see old size, skips write
> > > 
> > >    new       reads 0               legacy HW gets mappable size
> > >                                    new HW, 0?
> > > 
> > > So for new userspace, how do we determine 0 meaning old kernel, pick a
> > > value for ourselves, or 0 meaning no aperture at all. PCI-ID? Or is it
> > > simply that new userspace (that might run on no aperture devices)
> > > doesn't care about the aperture size anyway, so it's a meaningless
> > > question?
> > 
> > Good question, I'm afraid I don't know :). There is the option of saying 
> > new userspace should check the gtt_version before using the aperture at 
> > all but I can see the appeal of just getting the size to make that decision.
> 
> One possible answer is to return
> 
>         aper.mappable_size = (u64)-ENODEV;
> 
> The errno by design are <4096 and so -errno is in the last page, and
> should always be an invalid GTT size, as the last representable value in
> this scheme is -4096ull.

I can't answer why we don't just return -ENODEV in that case anyway. I
was thinking of preventing upset applications, but those applications
don't exist yet.
-Chris