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

[Antonio Argenziano <antonio.argenziano@intel.com>]

On 07/03/19 08:50, Chris Wilson wrote:
> 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@intel.com>
>>>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>>>>>> Signed-off-by: Antonio Argenziano <antonio.argenziano@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.

Not sure if in this last reply you mean return ENODEV from the IOCTL or 
just in drm_i915_gem_get_aperture.mappable_aperture_size.

Antonio

> -Chris
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev