Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Chris Wilson 
On Fri, Apr 29, 2016 at 11:56:28AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:39, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 29/04/16 11:18, Chris Wilson wrote:
> >>>On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> I don't get it - if we are adding something why not add it in a way
> that makes it clear and self-contained - what is the downside of
> what I propose to meet such resistance?
> >>>
> >>>You're suggesting to add a field I'm not going to use. Is any one?
> >>>Until there is an actual use (which would afaict be if one of the
> >>>existing values changed meaning, which itself would be an abi break) I'm
> >>>not convinced we should be designing for the unknowable. If we did need
> >>>to version would we not be better off with a new ioctl?
> >>
> >>I am suggesting if we are adding aper_total_size, or whatever it is
> >>called, to make it usable in an self-contained matter.
> >>
> >>My impression was you want aper_total_size so I was simply objecting
> >>to adding it without being able to directly tell if kernel actually
> >>supports that extension.
> >
> >The two additions that I thought we have reduced it to:
> >
> > u64 mappable_aperture_size
> > u64 stolen_size;
> >
> >Of which I agree that the first has some ambiguity over 0. But I don't
> >think it makes a difference in practice. I for one would not bother with
> >checking a version. I already have a cascade here to deal with not being
> >able to use various probes, with an eventual fallback to a safe value.
> >
> >We know the mappable_aperture_size cannot be zero with the exception
> >that the device is disabled - but we have opened the device already.
> 
> Since you agree there is ambiguity lets just add a version. You
> don't have to use it, but someone will.
> 
>   u32 version; // == 1
>   u64 mappable_aperture_size;
> 
> And then it is clear and self-contained.

u64 version;

or

u32 version;
u32 flags;

uABI fields need to be naturally aligned.

Hah, you didn't mention the trump card you pulled on IRC. In light of
that issue ever being a problem (that we may be faced with 0 aperture),
yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Tvrtko Ursulin 


On 29/04/16 11:39, Chris Wilson wrote:

On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:


On 29/04/16 11:18, Chris Wilson wrote:

On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:

I don't get it - if we are adding something why not add it in a way
that makes it clear and self-contained - what is the downside of
what I propose to meet such resistance?


You're suggesting to add a field I'm not going to use. Is any one?
Until there is an actual use (which would afaict be if one of the
existing values changed meaning, which itself would be an abi break) I'm
not convinced we should be designing for the unknowable. If we did need
to version would we not be better off with a new ioctl?


I am suggesting if we are adding aper_total_size, or whatever it is
called, to make it usable in an self-contained matter.

My impression was you want aper_total_size so I was simply objecting
to adding it without being able to directly tell if kernel actually
supports that extension.


The two additions that I thought we have reduced it to:

u64 mappable_aperture_size
u64 stolen_size;

Of which I agree that the first has some ambiguity over 0. But I don't
think it makes a difference in practice. I for one would not bother with
checking a version. I already have a cascade here to deal with not being
able to use various probes, with an eventual fallback to a safe value.

We know the mappable_aperture_size cannot be zero with the exception
that the device is disabled - but we have opened the device already.


Since you agree there is ambiguity lets just add a version. You don't 
have to use it, but someone will.


u32 version; // == 1
u64 mappable_aperture_size;

And then it is clear and self-contained.

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Chris Wilson 
On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:18, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> >>I don't get it - if we are adding something why not add it in a way
> >>that makes it clear and self-contained - what is the downside of
> >>what I propose to meet such resistance?
> >
> >You're suggesting to add a field I'm not going to use. Is any one?
> >Until there is an actual use (which would afaict be if one of the
> >existing values changed meaning, which itself would be an abi break) I'm
> >not convinced we should be designing for the unknowable. If we did need
> >to version would we not be better off with a new ioctl?
> 
> I am suggesting if we are adding aper_total_size, or whatever it is
> called, to make it usable in an self-contained matter.
> 
> My impression was you want aper_total_size so I was simply objecting
> to adding it without being able to directly tell if kernel actually
> supports that extension.

The two additions that I thought we have reduced it to:

u64 mappable_aperture_size
u64 stolen_size;

Of which I agree that the first has some ambiguity over 0. But I don't
think it makes a difference in practice. I for one would not bother with
checking a version. I already have a cascade here to deal with not being
able to use various probes, with an eventual fallback to a safe value.

We know the mappable_aperture_size cannot be zero with the exception
that the device is disabled - but we have opened the device already.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Tvrtko Ursulin 


On 29/04/16 11:18, Chris Wilson wrote:

On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:

I don't get it - if we are adding something why not add it in a way
that makes it clear and self-contained - what is the downside of
what I propose to meet such resistance?


You're suggesting to add a field I'm not going to use. Is any one?
Until there is an actual use (which would afaict be if one of the
existing values changed meaning, which itself would be an abi break) I'm
not convinced we should be designing for the unknowable. If we did need
to version would we not be better off with a new ioctl?


I am suggesting if we are adding aper_total_size, or whatever it is 
called, to make it usable in an self-contained matter.


My impression was you want aper_total_size so I was simply objecting to 
adding it without being able to directly tell if kernel actually 
supports that extension.


Regards,

Tvrtko

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Chris Wilson 
On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> I don't get it - if we are adding something why not add it in a way
> that makes it clear and self-contained - what is the downside of
> what I propose to meet such resistance?

You're suggesting to add a field I'm not going to use. Is any one?
Until there is an actual use (which would afaict be if one of the
existing values changed meaning, which itself would be an abi break) I'm
not convinced we should be designing for the unknowable. If we did need
to version would we not be better off with a new ioctl? 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-29 Thread Tvrtko Ursulin 


On 28/04/16 11:24, Chris Wilson wrote:

On Thu, Apr 28, 2016 at 10:30:32AM +0100, Tvrtko Ursulin wrote:


On 26/04/16 10:44, Chris Wilson wrote:

On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:


On 25/04/16 11:35, Ankitprasad Sharma wrote:

On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:

On 21/04/16 15:46, Chris Wilson wrote:

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:


Hi,

On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:

+   mutex_unlock(>struct_mutex);
+
+   seq_printf(m, "Total size of the GTT: %llu bytes\n",
+  arg.aper_size);
+   seq_printf(m, "Available space in the GTT: %llu bytes\n",
+  arg.aper_available_size);
+   seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
+  arg.map_total_size);
+   seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
+  map_space);
+   seq_printf(m, "Single largest space in the mappable aperture: %llu 
bytes\n",
+  map_largest);
+   seq_printf(m, "Available space for fences: %llu bytes\n",
+  fence_space);
+   seq_printf(m, "Single largest fence available: %llu bytes\n",
+  fence_largest);
+
+   return 0;
+}
+


In general I find this a lot of code for a feature of questionable
utility. As such I would prefer someone really stated the need for
this and explained how it really is useful - even though whetever
number they get from this may be completely irrelevant by the time
it is acted upon.


Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.


Aperture size in the ioctl is fine I think, just that detection caveat
what I asked in the other reply.

Here I wanted to suggest dropping all the non-trivial debugfs stuff and
just leave the info queried via i915_gem_get_aperture ioctl. So
effectively dropping the list traversal and vma sorting bits.


I think, debug info regarding the mappable space is good to have for
debugging purpose as Chris mentioned.
Also, the list traversal and the vma sorting stuff will only be called
for debugging purposes, not slowing anything down or so.


I am pretty indifferent on the topic of debugfs edition.

But for the ioctl extension, how about adding a version field as the
first one in the extended area?


A version number only makes sense when you are changing the meaning of
an existing field. Adding one implies that we are planning to do so, are
we?

In the scenarios, I've run through I haven't found one where a caller
would behave any differently faced with "0 - ioctl version not
supported" and "0 - no available space (mappable/stolen)". Adding a
version doesn't help using the new fields afaict. The argument is the
same as whether a flags field is forward thinking or unthinkingly
forward.


I was thinking that if 0 = no aperture or ioctl not supported
userspace has to try one mmap_gtt to find out which is true, will it
be ENODEV or ENOSPC (assuming, haven't checked). If we put a version
in there then it can avoid doing that. Sounds like a better
interface to me and I don't see any downsides to it.


I was thinking either userspace already cares and has a method for
finding the size of the PCI memory region or it doesn't. If it doesn't
and the ioctl reports 0 and its older second method fails with EPERM/EACCESS
then it is no worse off than before.


I don't get it - if we are adding something why not add it in a way that 
makes it clear and self-contained - what is the downside of what I 
propose to meet such resistance?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-28 Thread Chris Wilson 
On Thu, Apr 28, 2016 at 10:30:32AM +0100, Tvrtko Ursulin wrote:
> 
> On 26/04/16 10:44, Chris Wilson wrote:
> >On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 25/04/16 11:35, Ankitprasad Sharma wrote:
> >>>On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> On 21/04/16 15:46, Chris Wilson wrote:
> >On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> >>
> >>Hi,
> >>
> >>On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:
> >>>+  mutex_unlock(>struct_mutex);
> >>>+
> >>>+  seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >>>+ arg.aper_size);
> >>>+  seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >>>+ arg.aper_available_size);
> >>>+  seq_printf(m, "Total space in the mappable aperture: %llu 
> >>>bytes\n",
> >>>+ arg.map_total_size);
> >>>+  seq_printf(m, "Available space in the mappable aperture: %llu 
> >>>bytes\n",
> >>>+ map_space);
> >>>+  seq_printf(m, "Single largest space in the mappable aperture: 
> >>>%llu bytes\n",
> >>>+ map_largest);
> >>>+  seq_printf(m, "Available space for fences: %llu bytes\n",
> >>>+ fence_space);
> >>>+  seq_printf(m, "Single largest fence available: %llu bytes\n",
> >>>+ fence_largest);
> >>>+
> >>>+  return 0;
> >>>+}
> >>>+
> >>
> >>In general I find this a lot of code for a feature of questionable
> >>utility. As such I would prefer someone really stated the need for
> >>this and explained how it really is useful - even though whetever
> >>number they get from this may be completely irrelevant by the time
> >>it is acted upon.
> >
> >Yes, with the exception of the size of the mappable aperture, this is
> >really is debug info. It will get automatically dumped by userspace
> >when it sees an ENOSPC, and that may prove enough to solve the riddle of
> >why it failed. However, this information is terrible outdated and now
> >longer of such relevance.
> >
> >As for the mappable aperture size, there has been a request many years
> >ago! could we provide it without resorting to a privilege operation. I
> >guess by know that request has died out - but there is still the issue
> >with libpciassess that make it unsuitable for use inside a library where
> >one may want to avoid it and use a simple ioctl on the device you
> >already have open.
> >
> >Yes, it is meh.
> 
> Aperture size in the ioctl is fine I think, just that detection caveat
> what I asked in the other reply.
> 
> Here I wanted to suggest dropping all the non-trivial debugfs stuff and
> just leave the info queried via i915_gem_get_aperture ioctl. So
> effectively dropping the list traversal and vma sorting bits.
> 
> >>>I think, debug info regarding the mappable space is good to have for
> >>>debugging purpose as Chris mentioned.
> >>>Also, the list traversal and the vma sorting stuff will only be called
> >>>for debugging purposes, not slowing anything down or so.
> >>
> >>I am pretty indifferent on the topic of debugfs edition.
> >>
> >>But for the ioctl extension, how about adding a version field as the
> >>first one in the extended area?
> >
> >A version number only makes sense when you are changing the meaning of
> >an existing field. Adding one implies that we are planning to do so, are
> >we?
> >
> >In the scenarios, I've run through I haven't found one where a caller
> >would behave any differently faced with "0 - ioctl version not
> >supported" and "0 - no available space (mappable/stolen)". Adding a
> >version doesn't help using the new fields afaict. The argument is the
> >same as whether a flags field is forward thinking or unthinkingly
> >forward.
> 
> I was thinking that if 0 = no aperture or ioctl not supported
> userspace has to try one mmap_gtt to find out which is true, will it
> be ENODEV or ENOSPC (assuming, haven't checked). If we put a version
> in there then it can avoid doing that. Sounds like a better
> interface to me and I don't see any downsides to it.

I was thinking either userspace already cares and has a method for
finding the size of the PCI memory region or it doesn't. If it doesn't
and the ioctl reports 0 and its older second method fails with EPERM/EACCESS
then it is no worse off than before.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-28 Thread Tvrtko Ursulin 


On 26/04/16 10:44, Chris Wilson wrote:

On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:


On 25/04/16 11:35, Ankitprasad Sharma wrote:

On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:

On 21/04/16 15:46, Chris Wilson wrote:

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:


Hi,

On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:

+   mutex_unlock(>struct_mutex);
+
+   seq_printf(m, "Total size of the GTT: %llu bytes\n",
+  arg.aper_size);
+   seq_printf(m, "Available space in the GTT: %llu bytes\n",
+  arg.aper_available_size);
+   seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
+  arg.map_total_size);
+   seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
+  map_space);
+   seq_printf(m, "Single largest space in the mappable aperture: %llu 
bytes\n",
+  map_largest);
+   seq_printf(m, "Available space for fences: %llu bytes\n",
+  fence_space);
+   seq_printf(m, "Single largest fence available: %llu bytes\n",
+  fence_largest);
+
+   return 0;
+}
+


In general I find this a lot of code for a feature of questionable
utility. As such I would prefer someone really stated the need for
this and explained how it really is useful - even though whetever
number they get from this may be completely irrelevant by the time
it is acted upon.


Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.


Aperture size in the ioctl is fine I think, just that detection caveat
what I asked in the other reply.

Here I wanted to suggest dropping all the non-trivial debugfs stuff and
just leave the info queried via i915_gem_get_aperture ioctl. So
effectively dropping the list traversal and vma sorting bits.


I think, debug info regarding the mappable space is good to have for
debugging purpose as Chris mentioned.
Also, the list traversal and the vma sorting stuff will only be called
for debugging purposes, not slowing anything down or so.


I am pretty indifferent on the topic of debugfs edition.

But for the ioctl extension, how about adding a version field as the
first one in the extended area?


A version number only makes sense when you are changing the meaning of
an existing field. Adding one implies that we are planning to do so, are
we?

In the scenarios, I've run through I haven't found one where a caller
would behave any differently faced with "0 - ioctl version not
supported" and "0 - no available space (mappable/stolen)". Adding a
version doesn't help using the new fields afaict. The argument is the
same as whether a flags field is forward thinking or unthinkingly
forward.


I was thinking that if 0 = no aperture or ioctl not supported userspace 
has to try one mmap_gtt to find out which is true, will it be ENODEV or 
ENOSPC (assuming, haven't checked). If we put a version in there then it 
can avoid doing that. Sounds like a better interface to me and I don't 
see any downsides to it.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-26 Thread Chris Wilson 
On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
> 
> On 25/04/16 11:35, Ankitprasad Sharma wrote:
> >On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> >>On 21/04/16 15:46, Chris Wilson wrote:
> >>>On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:
> >+mutex_unlock(>struct_mutex);
> >+
> >+seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >+   arg.aper_size);
> >+seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >+   arg.aper_available_size);
> >+seq_printf(m, "Total space in the mappable aperture: %llu 
> >bytes\n",
> >+   arg.map_total_size);
> >+seq_printf(m, "Available space in the mappable aperture: %llu 
> >bytes\n",
> >+   map_space);
> >+seq_printf(m, "Single largest space in the mappable aperture: 
> >%llu bytes\n",
> >+   map_largest);
> >+seq_printf(m, "Available space for fences: %llu bytes\n",
> >+   fence_space);
> >+seq_printf(m, "Single largest fence available: %llu bytes\n",
> >+   fence_largest);
> >+
> >+return 0;
> >+}
> >+
> 
> In general I find this a lot of code for a feature of questionable
> utility. As such I would prefer someone really stated the need for
> this and explained how it really is useful - even though whetever
> number they get from this may be completely irrelevant by the time
> it is acted upon.
> >>>
> >>>Yes, with the exception of the size of the mappable aperture, this is
> >>>really is debug info. It will get automatically dumped by userspace
> >>>when it sees an ENOSPC, and that may prove enough to solve the riddle of
> >>>why it failed. However, this information is terrible outdated and now
> >>>longer of such relevance.
> >>>
> >>>As for the mappable aperture size, there has been a request many years
> >>>ago! could we provide it without resorting to a privilege operation. I
> >>>guess by know that request has died out - but there is still the issue
> >>>with libpciassess that make it unsuitable for use inside a library where
> >>>one may want to avoid it and use a simple ioctl on the device you
> >>>already have open.
> >>>
> >>>Yes, it is meh.
> >>
> >>Aperture size in the ioctl is fine I think, just that detection caveat
> >>what I asked in the other reply.
> >>
> >>Here I wanted to suggest dropping all the non-trivial debugfs stuff and
> >>just leave the info queried via i915_gem_get_aperture ioctl. So
> >>effectively dropping the list traversal and vma sorting bits.
> >>
> >I think, debug info regarding the mappable space is good to have for
> >debugging purpose as Chris mentioned.
> >Also, the list traversal and the vma sorting stuff will only be called
> >for debugging purposes, not slowing anything down or so.
> 
> I am pretty indifferent on the topic of debugfs edition.
> 
> But for the ioctl extension, how about adding a version field as the
> first one in the extended area?

A version number only makes sense when you are changing the meaning of
an existing field. Adding one implies that we are planning to do so, are
we?

In the scenarios, I've run through I haven't found one where a caller
would behave any differently faced with "0 - ioctl version not
supported" and "0 - no available space (mappable/stolen)". Adding a
version doesn't help using the new fields afaict. The argument is the
same as whether a flags field is forward thinking or unthinkingly
forward.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-25 Thread Tvrtko Ursulin 


On 25/04/16 11:35, Ankitprasad Sharma wrote:

On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:

On 21/04/16 15:46, Chris Wilson wrote:

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:


Hi,

On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:

+   mutex_unlock(>struct_mutex);
+
+   seq_printf(m, "Total size of the GTT: %llu bytes\n",
+  arg.aper_size);
+   seq_printf(m, "Available space in the GTT: %llu bytes\n",
+  arg.aper_available_size);
+   seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
+  arg.map_total_size);
+   seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
+  map_space);
+   seq_printf(m, "Single largest space in the mappable aperture: %llu 
bytes\n",
+  map_largest);
+   seq_printf(m, "Available space for fences: %llu bytes\n",
+  fence_space);
+   seq_printf(m, "Single largest fence available: %llu bytes\n",
+  fence_largest);
+
+   return 0;
+}
+


In general I find this a lot of code for a feature of questionable
utility. As such I would prefer someone really stated the need for
this and explained how it really is useful - even though whetever
number they get from this may be completely irrelevant by the time
it is acted upon.


Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.


Aperture size in the ioctl is fine I think, just that detection caveat
what I asked in the other reply.

Here I wanted to suggest dropping all the non-trivial debugfs stuff and
just leave the info queried via i915_gem_get_aperture ioctl. So
effectively dropping the list traversal and vma sorting bits.


I think, debug info regarding the mappable space is good to have for
debugging purpose as Chris mentioned.
Also, the list traversal and the vma sorting stuff will only be called
for debugging purposes, not slowing anything down or so.


I am pretty indifferent on the topic of debugfs edition.

But for the ioctl extension, how about adding a version field as the 
first one in the extended area?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-25 Thread Ankitprasad Sharma 
On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> On 21/04/16 15:46, Chris Wilson wrote:
> > On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> >>
> >> Hi,
> >>
> >> On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:
> >>> + mutex_unlock(>struct_mutex);
> >>> +
> >>> + seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >>> +arg.aper_size);
> >>> + seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >>> +arg.aper_available_size);
> >>> + seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >>> +arg.map_total_size);
> >>> + seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >>> +map_space);
> >>> + seq_printf(m, "Single largest space in the mappable aperture: %llu 
> >>> bytes\n",
> >>> +map_largest);
> >>> + seq_printf(m, "Available space for fences: %llu bytes\n",
> >>> +fence_space);
> >>> + seq_printf(m, "Single largest fence available: %llu bytes\n",
> >>> +fence_largest);
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>
> >> In general I find this a lot of code for a feature of questionable
> >> utility. As such I would prefer someone really stated the need for
> >> this and explained how it really is useful - even though whetever
> >> number they get from this may be completely irrelevant by the time
> >> it is acted upon.
> >
> > Yes, with the exception of the size of the mappable aperture, this is
> > really is debug info. It will get automatically dumped by userspace
> > when it sees an ENOSPC, and that may prove enough to solve the riddle of
> > why it failed. However, this information is terrible outdated and now
> > longer of such relevance.
> >
> > As for the mappable aperture size, there has been a request many years
> > ago! could we provide it without resorting to a privilege operation. I
> > guess by know that request has died out - but there is still the issue
> > with libpciassess that make it unsuitable for use inside a library where
> > one may want to avoid it and use a simple ioctl on the device you
> > already have open.
> >
> > Yes, it is meh.
> 
> Aperture size in the ioctl is fine I think, just that detection caveat 
> what I asked in the other reply.
> 
> Here I wanted to suggest dropping all the non-trivial debugfs stuff and 
> just leave the info queried via i915_gem_get_aperture ioctl. So 
> effectively dropping the list traversal and vma sorting bits.
> 
I think, debug info regarding the mappable space is good to have for
debugging purpose as Chris mentioned.
Also, the list traversal and the vma sorting stuff will only be called
for debugging purposes, not slowing anything down or so.

Thanks,
Ankit

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-21 Thread Tvrtko Ursulin 


On 21/04/16 15:46, Chris Wilson wrote:

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:


Hi,

On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:

+   mutex_unlock(>struct_mutex);
+
+   seq_printf(m, "Total size of the GTT: %llu bytes\n",
+  arg.aper_size);
+   seq_printf(m, "Available space in the GTT: %llu bytes\n",
+  arg.aper_available_size);
+   seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
+  arg.map_total_size);
+   seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
+  map_space);
+   seq_printf(m, "Single largest space in the mappable aperture: %llu 
bytes\n",
+  map_largest);
+   seq_printf(m, "Available space for fences: %llu bytes\n",
+  fence_space);
+   seq_printf(m, "Single largest fence available: %llu bytes\n",
+  fence_largest);
+
+   return 0;
+}
+


In general I find this a lot of code for a feature of questionable
utility. As such I would prefer someone really stated the need for
this and explained how it really is useful - even though whetever
number they get from this may be completely irrelevant by the time
it is acted upon.


Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.


Aperture size in the ioctl is fine I think, just that detection caveat 
what I asked in the other reply.


Here I wanted to suggest dropping all the non-trivial debugfs stuff and 
just leave the info queried via i915_gem_get_aperture ioctl. So 
effectively dropping the list traversal and vma sorting bits.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-21 Thread Chris Wilson 
On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:
> >+mutex_unlock(>struct_mutex);
> >+
> >+seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >+   arg.aper_size);
> >+seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >+   arg.aper_available_size);
> >+seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >+   arg.map_total_size);
> >+seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >+   map_space);
> >+seq_printf(m, "Single largest space in the mappable aperture: %llu 
> >bytes\n",
> >+   map_largest);
> >+seq_printf(m, "Available space for fences: %llu bytes\n",
> >+   fence_space);
> >+seq_printf(m, "Single largest fence available: %llu bytes\n",
> >+   fence_largest);
> >+
> >+return 0;
> >+}
> >+
> 
> In general I find this a lot of code for a feature of questionable
> utility. As such I would prefer someone really stated the need for
> this and explained how it really is useful - even though whetever
> number they get from this may be completely irrelevant by the time
> it is acted upon.

Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-21 Thread Tvrtko Ursulin 


Hi,

On 20/04/16 12:17, ankitprasad.r.sha...@intel.com wrote:

From: Ankitprasad Sharma 


Patch is mostly Rodrigo's, right? So I assumed he approved the 
authorship transfer.



When constructing a batchbuffer, it is sometimes crucial to know the
largest hole into which we can fit a fenceable buffer (for example when
handling very large objects on gen2 and gen3). This depends on the
fragmentation of pinned buffers inside the aperture, a question only the
kernel can easily answer.

This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
include a couple of new fields in its reply to userspace - the total
amount of space available in the mappable region of the aperture and
also the single largest block available.

This is not quite what userspace wants to answer the question of whether
this batch will fit as fences are also required to meet severe alignment
constraints within the batch. For this purpose, a third conservative
estimate of largest fence available is also provided. For when userspace
needs more than one batch, we also provide the culmulative space
available for fences such that it has some additional guidance to how
much space it could allocate to fences. Conservatism still wins.

The patch also adds a debugfs file for convenient testing and reporting.

v2: The first object cannot end at offset 0, so we can use last==0 to
detect the empty list.

v3: Expand all values to 64bit, just in case.
 Report total mappable aperture size for userspace that cannot easily
 determine it by inspecting the PCI device.

v4: (Rodrigo) Fixed rebase conflicts.

v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
debugfs, Addressed comments (Chris/Tvrtko)


It is confusing that you already posted a different v5 on 1st of July 2015.


Signed-off-by: Chris Wilson 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: Ankitprasad Sharma 
---
  drivers/gpu/drm/i915/i915_debugfs.c | 133 
  drivers/gpu/drm/i915/i915_gem.c |   1 +
  include/uapi/drm/i915_drm.h |   5 ++
  3 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7f94c6a..d8d7994 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -524,6 +524,138 @@ static int i915_gem_object_info(struct seq_file *m, void* 
data)
return 0;
  }

+static int vma_rank_by_ggtt(void *priv,
+   struct list_head *A,
+   struct list_head *B)
+{
+   struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
+   struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
+
+   return a->node.start - b->node.start;
+}
+
+static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
+{
+   u32 size = end - start;
+   u32 fence_size;
+
+   if (INTEL_INFO(dev_priv)->gen < 4) {
+   u32 fence_max;
+   u32 fence_next;
+
+   if (IS_GEN3(dev_priv)) {
+   fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
+   fence_next = 1024*1024;
+   } else {
+   fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
+   fence_next = 512*1024;
+   }
+
+   fence_max = min(fence_max, size);
+   fence_size = 0;
+   /* Find fence_size less than fence_max and power of 2 */
+   while (fence_next <= fence_max) {
+   u32 base = ALIGN(start, fence_next);
+   if (base + fence_next > end)
+   break;
+
+   fence_size = fence_next;
+   fence_next <<= 1;
+   }
+   } else {
+   fence_size = size;
+   }
+
+   return fence_size;
+}
+
+static int i915_gem_aperture_info(struct seq_file *m, void *data)
+{
+   struct drm_info_node *node = m->private;
+   struct drm_device *dev = node->minor->dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct i915_ggtt *ggtt = _priv->ggtt;
+   struct drm_i915_gem_get_aperture arg;
+   struct i915_vma *vma;
+   struct list_head map_list;
+   const uint64_t map_limit = ggtt->mappable_end;
+   uint64_t map_space, map_largest, fence_space, fence_largest;
+   uint64_t last, size;
+   int ret;
+
+   INIT_LIST_HEAD(_list);
+
+   map_space = map_largest = 0;
+   fence_space = fence_largest = 0;
+
+   ret = i915_gem_get_aperture_ioctl(node->minor->dev, , NULL);
+   if (ret)
+   return ret;
+
+   mutex_lock(>struct_mutex);
+   list_for_each_entry(vma, >base.active_list, vm_link)
+   if (vma->pin_count &&
+   (vma->node.start + vma->node.size) <= map_limit)
+   list_add(>exec_list, _list);
+

Re: [Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-20 Thread kbuild test robot 
Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160420]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/ankitprasad-r-sharma-intel-com/Support-for-creating-using-Stolen-memory-backed-objects/20160420-194608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/i915_gem.c:168:43-44: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

2016-04-20 Thread ankitprasad . r . sharma 
From: Ankitprasad Sharma 

When constructing a batchbuffer, it is sometimes crucial to know the
largest hole into which we can fit a fenceable buffer (for example when
handling very large objects on gen2 and gen3). This depends on the
fragmentation of pinned buffers inside the aperture, a question only the
kernel can easily answer.

This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
include a couple of new fields in its reply to userspace - the total
amount of space available in the mappable region of the aperture and
also the single largest block available.

This is not quite what userspace wants to answer the question of whether
this batch will fit as fences are also required to meet severe alignment
constraints within the batch. For this purpose, a third conservative
estimate of largest fence available is also provided. For when userspace
needs more than one batch, we also provide the culmulative space
available for fences such that it has some additional guidance to how
much space it could allocate to fences. Conservatism still wins.

The patch also adds a debugfs file for convenient testing and reporting.

v2: The first object cannot end at offset 0, so we can use last==0 to
detect the empty list.

v3: Expand all values to 64bit, just in case.
Report total mappable aperture size for userspace that cannot easily
determine it by inspecting the PCI device.

v4: (Rodrigo) Fixed rebase conflicts.

v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
debugfs, Addressed comments (Chris/Tvrtko)

Signed-off-by: Chris Wilson 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: Ankitprasad Sharma 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 133 
 drivers/gpu/drm/i915/i915_gem.c |   1 +
 include/uapi/drm/i915_drm.h |   5 ++
 3 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7f94c6a..d8d7994 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -524,6 +524,138 @@ static int i915_gem_object_info(struct seq_file *m, void* 
data)
return 0;
 }
 
+static int vma_rank_by_ggtt(void *priv,
+   struct list_head *A,
+   struct list_head *B)
+{
+   struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
+   struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
+
+   return a->node.start - b->node.start;
+}
+
+static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
+{
+   u32 size = end - start;
+   u32 fence_size;
+
+   if (INTEL_INFO(dev_priv)->gen < 4) {
+   u32 fence_max;
+   u32 fence_next;
+
+   if (IS_GEN3(dev_priv)) {
+   fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
+   fence_next = 1024*1024;
+   } else {
+   fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
+   fence_next = 512*1024;
+   }
+
+   fence_max = min(fence_max, size);
+   fence_size = 0;
+   /* Find fence_size less than fence_max and power of 2 */
+   while (fence_next <= fence_max) {
+   u32 base = ALIGN(start, fence_next);
+   if (base + fence_next > end)
+   break;
+
+   fence_size = fence_next;
+   fence_next <<= 1;
+   }
+   } else {
+   fence_size = size;
+   }
+
+   return fence_size;
+}
+
+static int i915_gem_aperture_info(struct seq_file *m, void *data)
+{
+   struct drm_info_node *node = m->private;
+   struct drm_device *dev = node->minor->dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct i915_ggtt *ggtt = _priv->ggtt;
+   struct drm_i915_gem_get_aperture arg;
+   struct i915_vma *vma;
+   struct list_head map_list;
+   const uint64_t map_limit = ggtt->mappable_end;
+   uint64_t map_space, map_largest, fence_space, fence_largest;
+   uint64_t last, size;
+   int ret;
+
+   INIT_LIST_HEAD(_list);
+
+   map_space = map_largest = 0;
+   fence_space = fence_largest = 0;
+
+   ret = i915_gem_get_aperture_ioctl(node->minor->dev, , NULL);
+   if (ret)
+   return ret;
+
+   mutex_lock(>struct_mutex);
+   list_for_each_entry(vma, >base.active_list, vm_link)
+   if (vma->pin_count &&
+   (vma->node.start + vma->node.size) <= map_limit)
+   list_add(>exec_list, _list);
+   list_for_each_entry(vma, >base.inactive_list, vm_link)
+   if (vma->pin_count &&
+   (vma->node.start + vma->node.size) <= map_limit)
+   list_add(>exec_list, _list);
+
+   last = 0;