Re: [Mesa-dev] [PATCH 1/2] nouveau: Add basic memory object support

[Miguel Angel Vico]

On Fri, 22 Jun 2018 20:37:19 -0400
Ilia Mirkin  wrote:

> On Fri, Jun 22, 2018 at 8:22 PM, Miguel Angel Vico  
> wrote:
> >
> >
> > On Thu, 21 Jun 2018 22:09:14 -0400
> > Ilia Mirkin  wrote:
> >  
> >> Hi Miguel,
> >>
> >> Preface: I know little about this ext, so feel free to educate me on
> >> the wrongness of my thinking.
> >>
> >> On Thu, Jun 21, 2018 at 10:01 PM, Miguel A. Vico  
> >> wrote:  
> >> > Add memory object support for nvc0 and nv50
> >> >
> >> > Signed-off-by: Miguel A Vico Moya 
> >> > ---
> >> >  .../drivers/nouveau/nv50/nv50_miptree.c   | 49 +
> >> >  .../drivers/nouveau/nv50/nv50_resource.c  | 52 +++
> >> >  .../drivers/nouveau/nv50/nv50_resource.h  | 33 
> >> >  .../drivers/nouveau/nvc0/nvc0_resource.c  | 22 
> >> >  4 files changed, 146 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/src/gallium/drivers/nouveau/nv50/nv50_miptree.c 
> >> > b/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> >> > index f2e304fde6..91007d3dac 100644
> >> > --- a/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> >> > +++ b/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> >> > @@ -397,13 +397,13 @@ nv50_miptree_create(struct pipe_screen *pscreen,
> >> > return pt;
> >> >  }
> >> >
> >> > -struct pipe_resource *
> >> > -nv50_miptree_from_handle(struct pipe_screen *pscreen,
> >> > - const struct pipe_resource *templ,
> >> > - struct winsys_handle *whandle)
> >> > +static struct pipe_resource *
> >> > +nv50_miptree_from_bo(struct pipe_screen *pscreen,
> >> > + const struct pipe_resource *templ,
> >> > + struct nouveau_bo *bo,
> >> > + uint32_t stride)
> >> >  {
> >> > struct nv50_miptree *mt;
> >> > -   unsigned stride;
> >> >
> >> > /* only supports 2D, non-mipmapped textures for the moment */  
> >>
> >> Won't this be a drag, since you're supposed to be able to "place" 3d
> >> textures, as well as mip-mapped ones?
> >>
> >> The reason I haven't looked at doing VK for nouveau yet is that the
> >> nouveau kernel API does not allow explicit userspace-side VA
> >> management, which would be required to allow something like this. I
> >> believe it would also be required to implement this GL extension. Feel
> >> free to correct my thinking.  
> >
> > My understanding is that EXT_external_objects itself only presents a
> > generic interface for applications to feed external memory handles to
> > OpenGL. It doesn't specify what properties those handles need to
> > satisfy, or whether the memory comes from user-space or any other
> > driver component. It is up for specific extensions to define new memory
> > objects, their properties, and how they can be imported/exported.
> >
> > As I understand it, the initial motivation for putting together this
> > extension was indeed Vulkan-OpenGL interoperability, but it is not
> > limited to that.
> >
> > This initial implementation of the extension adds the logic to allow
> > applications to feed opaque handles to OpenGL, but there's no API that
> > can create compatible opaque handles for the nouveau driver yet.
> >
> > Just to add a bit more context, here's a prototype of an extension
> > defining one of such handles:
> >
> >   
> > https://gitlab.freedesktop.org/mvicomoya/mesa/tree/wip/NVX_unix_allocator_import
> >
> > It is used by the the kmscube prototype that uses the generic allocator
> > to allocate buffers:
> >
> >   https://gitlab.freedesktop.org/allocator/kmscube/merge_requests/1
> >
> > And EXT_external_objects is just a pre-requisite for that.  
> 
> So by exposing GL_EXT_memory_object, the function
> glTexStorageMem3DEXT() becomes available. I don't think that will work
> with nouveau (without further changes), so the extension can't be
> exposed. Right?

Yes, exposing EXT_memory_object, several functions such as
glTexStorageMem3DEXT() become available, but they would not be usable at
all without at least one accompanying platform-specific extension that
defines how memory objects can be imported (e.g.
https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_external_objects_win32.txt),
am I correct?

In any case, we can keep the extension disabled by not applying patch
2/2 in this series, but maybe patch 1/2 is still good as a stepping
stone?

Thanks.

> 
> [I totally get that this is not your desired use-case, but we can't
> expose half-working extensions...]
> 
>   -ilia


-- 
Miguel


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] nouveau: Add basic memory object support

[Miguel Angel Vico]

On Thu, 21 Jun 2018 22:09:14 -0400
Ilia Mirkin  wrote:

> Hi Miguel,
> 
> Preface: I know little about this ext, so feel free to educate me on
> the wrongness of my thinking.
> 
> On Thu, Jun 21, 2018 at 10:01 PM, Miguel A. Vico  wrote:
> > Add memory object support for nvc0 and nv50
> >
> > Signed-off-by: Miguel A Vico Moya 
> > ---
> >  .../drivers/nouveau/nv50/nv50_miptree.c   | 49 +
> >  .../drivers/nouveau/nv50/nv50_resource.c  | 52 +++
> >  .../drivers/nouveau/nv50/nv50_resource.h  | 33 
> >  .../drivers/nouveau/nvc0/nvc0_resource.c  | 22 
> >  4 files changed, 146 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/gallium/drivers/nouveau/nv50/nv50_miptree.c 
> > b/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> > index f2e304fde6..91007d3dac 100644
> > --- a/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> > +++ b/src/gallium/drivers/nouveau/nv50/nv50_miptree.c
> > @@ -397,13 +397,13 @@ nv50_miptree_create(struct pipe_screen *pscreen,
> > return pt;
> >  }
> >
> > -struct pipe_resource *
> > -nv50_miptree_from_handle(struct pipe_screen *pscreen,
> > - const struct pipe_resource *templ,
> > - struct winsys_handle *whandle)
> > +static struct pipe_resource *
> > +nv50_miptree_from_bo(struct pipe_screen *pscreen,
> > + const struct pipe_resource *templ,
> > + struct nouveau_bo *bo,
> > + uint32_t stride)
> >  {
> > struct nv50_miptree *mt;
> > -   unsigned stride;
> >
> > /* only supports 2D, non-mipmapped textures for the moment */  
> 
> Won't this be a drag, since you're supposed to be able to "place" 3d
> textures, as well as mip-mapped ones?
> 
> The reason I haven't looked at doing VK for nouveau yet is that the
> nouveau kernel API does not allow explicit userspace-side VA
> management, which would be required to allow something like this. I
> believe it would also be required to implement this GL extension. Feel
> free to correct my thinking.

My understanding is that EXT_external_objects itself only presents a
generic interface for applications to feed external memory handles to
OpenGL. It doesn't specify what properties those handles need to
satisfy, or whether the memory comes from user-space or any other
driver component. It is up for specific extensions to define new memory
objects, their properties, and how they can be imported/exported.

As I understand it, the initial motivation for putting together this
extension was indeed Vulkan-OpenGL interoperability, but it is not
limited to that.

This initial implementation of the extension adds the logic to allow
applications to feed opaque handles to OpenGL, but there's no API that
can create compatible opaque handles for the nouveau driver yet.

Just to add a bit more context, here's a prototype of an extension
defining one of such handles:

  
https://gitlab.freedesktop.org/mvicomoya/mesa/tree/wip/NVX_unix_allocator_import

It is used by the the kmscube prototype that uses the generic allocator
to allocate buffers:

  https://gitlab.freedesktop.org/allocator/kmscube/merge_requests/1

And EXT_external_objects is just a pre-requisite for that.

Thanks.

> 
> Cheers,
> 
>   -ilia


-- 
Miguel


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Miguel Angel Vico]

Hi,

Besides the DRM modifiers discussion in the other forks or this thread
(I should've probably started separate threads), has anyone gotten the
chance to look at least at the mesa changes and allocator changes I
shared below?

With respect to Mesa changes, I think it might be worth merging the
EXT_external_objects nouveau implementation upstream. Should I just
send the list of patches as a formal RFR?

With respect to Allocator changes, it'd be nice getting someone else's
(out of NVIDIA) feedback.

Thanks.

On Wed, 20 Dec 2017 08:51:51 -0800
Miguel Angel Vico <mvicom...@nvidia.com> wrote:

> Hi all,
> 
> As many of you already know, I've been working with James Jones on the
> Generic Device Allocator project lately. He started a discussion thread
> some weeks ago seeking feedback on the current prototype of the library
> and advice on how to move all this forward, from a prototype stage to
> production. For further reference, see:
> 
>https://lists.freedesktop.org/archives/mesa-dev/2017-November/177632.html
> 
> From the thread above, we came up with very interesting high level
> design ideas for one of the currently missing parts in the library:
> Usage transitions. That's something I'll personally work on during the
> following weeks.
> 
> 
> In the meantime, I've been working on putting together an open source
> implementation of the allocator mechanisms using the Nouveau driver for
> all to be able to play with.
> 
> Below I'm seeking feedback on a bunch of changes I had to make to
> different components of the graphics stack:
> 
> ** Allocator **
> 
>   An allocator driver implementation on top of Nouveau. The current
>   implementation only handles pitch linear layouts, but that's enough
>   to have the kmscube port working using the allocator and Nouveau
>   drivers.
> 
>   You can pull these changes from
> 
>   https://github.com/mvicomoya/allocator/tree/wip/mvicomoya/nouveau-driver
> 
> ** Mesa **
> 
>   James's kmscube port to use the allocator relies on the
>   EXT_external_objects extension to import allocator allocations to
>   OpenGL as a texture object. However, the Nouveau implementation of
>   these mechanisms is missing in Mesa, so I went ahead and added them.
> 
>   You can pull these changes from
> 
>   
> https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/EXT_external_objects-nouveau
> 
>   Also, James's kmscube port uses the NVX_unix_allocator_import
>   extension to attach allocator metadata to texture objects so the
>   driver knows how to deal with the imported memory.
> 
>   Note that there isn't a formal spec for this extension yet. For now,
>   it just serves as an experimental mechanism to import allocator
>   memory in OpenGL, and attach metadata to texture objects.
> 
>   You can pull these changes (written on top of the above) from:
> 
>   
> https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/NVX_unix_allocator_import
> 
> ** kmscube **
> 
>   Mostly minor fixes and improvements on top of James's port to use the
>   allocator. Main thing is the allocator initialization path will use
>   EGL_MESA_platform_surfaceless if EGLDevice platform isn't supported
>   by the underlying EGL implementation.
> 
>   You can pull these changes from:
> 
>   
> https://github.com/mvicomoya/kmscube/tree/wip/mvicomoya/allocator-nouveau
> 
> 
> With all the above you should be able to get kmscube working using the
> allocator on top of the Nouveau driver.
> 
> 
> Another of the missing pieces before we can move this to production is
> importing allocations to DRM FB objects. This is probably one of the
> most sensitive parts of the project as it requires modification/addition
> of kernel driver interfaces.
> 
> At XDC2017, James had several hallway conversations with several people
> about this, all having different opinions. I'd like to take this
> opportunity to also start a discussion about what's the best option to
> create a path to get allocator allocations added as DRM FB objects.
> 
> These are the few options we've considered to start with:
> 
>   A) Have vendor-private ioctls to set properties on GEM objects that
>  are inherited by the FB objects. This is how our (NVIDIA) desktop
>  DRM driver currently works. This would require every vendor to add
>  their own ioctl to process allocator metadata, but the metadata is
>  actually a vendor-agnostic object more like DRM modifiers. We'd
>  like to come up with a vendor-agnostic solutions that can be
>  integrated to core DRM.
> 
>   B) Add a new drmModeAddFBWithMetadata() command that takes allocator
>  metadata blobs for each plane of the FB. Some people in the
>  community have me

Re: [Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Miguel Angel Vico]

(Adding dri-devel back, and trying to respond to some comments from
the different forks)

James Jones wrote:

> Your worst case analysis above isn't far off from our HW, give or take
> some bits and axes here and there.  We've started an internal discussion
> about how to lay out all the bits we need.  It's hard to even enumerate
> them all without having a complete understanding of what capability sets
> are going to include, a fully-optimized implementation of the mechanism
> on our HW, and lot's of test scenarios though.  

(thanks James for most of the info below)

To elaborate a bit, if we want to share an allocation across GPUs for 3D
rendering, it seems we would need 12 bits to express our
swizzling/tiling memory layouts for fermi+. In addition to that,
maxwell uses 3 more bits for this, and we need an extra bit to identify
pre-fermi representations.

We also need one bit to differentiate between Tegra and desktop, and
another one to indicate whether the layout is otherwise linear.

Then things like whether compression is used (one more bit), and we can
probably get by with 3 bits for the type of compression if we are
creative. However, it'd be way easier to just track arch + page kind,
which would be like 32 bits on its own.

Whether Z-culling and/or zero-bandwidth-clears are used may be another 3
bits.

If device-local properties are included, we might need a couple more
bits for caching.

We may also need to express locality information, which may take at
least another 2 or 3 bits.

If we want to share array textures too, you also need to pass the array
pitch. Is it supposed to be encoded in a modifier too? That's 64 bits on
its own.

So yes, as James mentioned, with some effort, we could technically fit
our current allocation parameters in a modifier, but I'm still not
convinced this is as future proof as it could be as our hardware grows
in capabilities.


Daniel Stone wrote:

> So I reflexively
> get a bit itchy when I see the kernel being used to transit magic
> blobs of data which are supplied by userspace, and only interpreted by
> different userspace. Having tiling formats hidden away means that
> we've had real-world bugs in AMD hardware, where we end up displaying
> garbage because we cannot generically reason about the buffer
> attributes.  

I'm a bit confused. Can't modifiers be specified by vendors and only
interpreted by drivers? My understanding was that modifiers could
actually be treated as opaque 64-bit data, in which case they would
qualify as "magic blobs of data". Otherwise, it seems this wouldn't be
scalable. What am I missing?


Daniel Vetter wrote:

> I think in the interim figuring out how to expose kms capabilities
> better (and necessarily standardizing at least some of them which
> matter at the compositor level, like size limits of framebuffers)
> feels like the place to push the ecosystem forward. In some way
> Miguel's proposal looks a bit backwards, since it adds the pitch
> capabilities to addfb, but at addfb time you've allocated everything
> already, so way too late to fix things up. With modifiers we've added
> a very simple per-plane property to list which modifiers can be
> combined with which pixel formats. Tiny start, but obviously very far
> from all that we'll need.  

Not sure whether I might be misunderstanding your statement, but one of
the allocator main features is negotiation of nearly optimal allocation
parameters given a set of uses on different devices/engines by the
capability merge operation. A client should have queried what every
device/engine is capable of for the given uses, find the optimal set of
capabilities, and use it for allocating a buffer. At the moment these
parameters are given to KMS, they are expected to be good. If they
aren't, the client didn't do things right.


Rob Clark wrote:

> It does seem like, if possible, starting out with modifiers for now at
> the kernel interface would make life easier, vs trying to reinvent
> both kernel and userspace APIs at the same time.  Userspace APIs are
> easier to change or throw away.  Presumably by the time we get to the
> point of changing kernel uabi, we are already using, and pretty happy
> with, serialized liballoc data over the wire in userspace so it is
> only a matter of changing the kernel interface.  

I guess we can indeed start with modifiers for now, if that's what it
takes to get the allocator mechanisms rolling. However, it seems to me
that we won't be able to encode the same type of information included
in capability sets with modifiers in all cases. For instance, if we end
up encoding usage transition information in capability sets, how that
would translate to modifiers?

I assume display doesn't really care about a lot of the data capability
sets may encode, but is it correct to think of modifiers as things only
display needs? If we are to treat modifiers as a first-class citizen, I
would expect to use them beyond that.


Kristian Kristensen wrote:

> I agree and let me 

Re: [Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Miguel Angel Vico]

Inline.

On Wed, 20 Dec 2017 11:54:10 -0800
Kristian Høgsberg <hoegsb...@gmail.com> wrote:

> On Wed, Dec 20, 2017 at 11:51 AM, Daniel Vetter <dan...@ffwll.ch> wrote:
> > Since this also involves the kernel let's add dri-devel ...

Yeah, I forgot. Thanks Daniel!

> >
> > On Wed, Dec 20, 2017 at 5:51 PM, Miguel Angel Vico <mvicom...@nvidia.com> 
> > wrote:  
> >> Hi all,
> >>
> >> As many of you already know, I've been working with James Jones on the
> >> Generic Device Allocator project lately. He started a discussion thread
> >> some weeks ago seeking feedback on the current prototype of the library
> >> and advice on how to move all this forward, from a prototype stage to
> >> production. For further reference, see:
> >>
> >>
> >> https://lists.freedesktop.org/archives/mesa-dev/2017-November/177632.html
> >>
> >> From the thread above, we came up with very interesting high level
> >> design ideas for one of the currently missing parts in the library:
> >> Usage transitions. That's something I'll personally work on during the
> >> following weeks.
> >>
> >>
> >> In the meantime, I've been working on putting together an open source
> >> implementation of the allocator mechanisms using the Nouveau driver for
> >> all to be able to play with.
> >>
> >> Below I'm seeking feedback on a bunch of changes I had to make to
> >> different components of the graphics stack:
> >>
> >> ** Allocator **
> >>
> >>   An allocator driver implementation on top of Nouveau. The current
> >>   implementation only handles pitch linear layouts, but that's enough
> >>   to have the kmscube port working using the allocator and Nouveau
> >>   drivers.
> >>
> >>   You can pull these changes from
> >>
> >>   
> >> https://github.com/mvicomoya/allocator/tree/wip/mvicomoya/nouveau-driver
> >>
> >> ** Mesa **
> >>
> >>   James's kmscube port to use the allocator relies on the
> >>   EXT_external_objects extension to import allocator allocations to
> >>   OpenGL as a texture object. However, the Nouveau implementation of
> >>   these mechanisms is missing in Mesa, so I went ahead and added them.
> >>
> >>   You can pull these changes from
> >>
> >>   
> >> https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/EXT_external_objects-nouveau
> >>
> >>   Also, James's kmscube port uses the NVX_unix_allocator_import
> >>   extension to attach allocator metadata to texture objects so the
> >>   driver knows how to deal with the imported memory.
> >>
> >>   Note that there isn't a formal spec for this extension yet. For now,
> >>   it just serves as an experimental mechanism to import allocator
> >>   memory in OpenGL, and attach metadata to texture objects.
> >>
> >>   You can pull these changes (written on top of the above) from:
> >>
> >>   
> >> https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/NVX_unix_allocator_import
> >>
> >> ** kmscube **
> >>
> >>   Mostly minor fixes and improvements on top of James's port to use the
> >>   allocator. Main thing is the allocator initialization path will use
> >>   EGL_MESA_platform_surfaceless if EGLDevice platform isn't supported
> >>   by the underlying EGL implementation.
> >>
> >>   You can pull these changes from:
> >>
> >>   
> >> https://github.com/mvicomoya/kmscube/tree/wip/mvicomoya/allocator-nouveau
> >>
> >>
> >> With all the above you should be able to get kmscube working using the
> >> allocator on top of the Nouveau driver.
> >>
> >>
> >> Another of the missing pieces before we can move this to production is
> >> importing allocations to DRM FB objects. This is probably one of the
> >> most sensitive parts of the project as it requires modification/addition
> >> of kernel driver interfaces.
> >>
> >> At XDC2017, James had several hallway conversations with several people
> >> about this, all having different opinions. I'd like to take this
> >> opportunity to also start a discussion about what's the best option to
> >> create a path to get allocator allocations added as DRM FB objects.
> >>
> >> These are the few options we've considered to start with:
> >>
> >>   A) Have vendor-private ioctls to set properties on GEM 

[Mesa-dev] Allocator Nouveau driver, Mesa EXT_external_objects, and DRM metadata import interfaces

[Miguel Angel Vico]

Hi all,

As many of you already know, I've been working with James Jones on the
Generic Device Allocator project lately. He started a discussion thread
some weeks ago seeking feedback on the current prototype of the library
and advice on how to move all this forward, from a prototype stage to
production. For further reference, see:

   https://lists.freedesktop.org/archives/mesa-dev/2017-November/177632.html

From the thread above, we came up with very interesting high level
design ideas for one of the currently missing parts in the library:
Usage transitions. That's something I'll personally work on during the
following weeks.


In the meantime, I've been working on putting together an open source
implementation of the allocator mechanisms using the Nouveau driver for
all to be able to play with.

Below I'm seeking feedback on a bunch of changes I had to make to
different components of the graphics stack:

** Allocator **

  An allocator driver implementation on top of Nouveau. The current
  implementation only handles pitch linear layouts, but that's enough
  to have the kmscube port working using the allocator and Nouveau
  drivers.

  You can pull these changes from

  https://github.com/mvicomoya/allocator/tree/wip/mvicomoya/nouveau-driver

** Mesa **

  James's kmscube port to use the allocator relies on the
  EXT_external_objects extension to import allocator allocations to
  OpenGL as a texture object. However, the Nouveau implementation of
  these mechanisms is missing in Mesa, so I went ahead and added them.

  You can pull these changes from

  
https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/EXT_external_objects-nouveau

  Also, James's kmscube port uses the NVX_unix_allocator_import
  extension to attach allocator metadata to texture objects so the
  driver knows how to deal with the imported memory.

  Note that there isn't a formal spec for this extension yet. For now,
  it just serves as an experimental mechanism to import allocator
  memory in OpenGL, and attach metadata to texture objects.

  You can pull these changes (written on top of the above) from:

  
https://github.com/mvicomoya/mesa/tree/wip/mvicomoya/NVX_unix_allocator_import

** kmscube **

  Mostly minor fixes and improvements on top of James's port to use the
  allocator. Main thing is the allocator initialization path will use
  EGL_MESA_platform_surfaceless if EGLDevice platform isn't supported
  by the underlying EGL implementation.

  You can pull these changes from:

  https://github.com/mvicomoya/kmscube/tree/wip/mvicomoya/allocator-nouveau


With all the above you should be able to get kmscube working using the
allocator on top of the Nouveau driver.


Another of the missing pieces before we can move this to production is
importing allocations to DRM FB objects. This is probably one of the
most sensitive parts of the project as it requires modification/addition
of kernel driver interfaces.

At XDC2017, James had several hallway conversations with several people
about this, all having different opinions. I'd like to take this
opportunity to also start a discussion about what's the best option to
create a path to get allocator allocations added as DRM FB objects.

These are the few options we've considered to start with:

  A) Have vendor-private ioctls to set properties on GEM objects that
 are inherited by the FB objects. This is how our (NVIDIA) desktop
 DRM driver currently works. This would require every vendor to add
 their own ioctl to process allocator metadata, but the metadata is
 actually a vendor-agnostic object more like DRM modifiers. We'd
 like to come up with a vendor-agnostic solutions that can be
 integrated to core DRM.

  B) Add a new drmModeAddFBWithMetadata() command that takes allocator
 metadata blobs for each plane of the FB. Some people in the
 community have mentioned this is their preferred design. This,
 however, means we'd have to go through the exercise of adding
 another metadata mechanism to the whole graphics stack.

  C) Shove allocator metadata into DRM by defining it to be a separate
 plane in the image, and using the existing DRM modifiers mechanism
 to indicate there is another plane for each "real" plane added. It
 isn't clear how this scales to surfaces that already need several
 planes, but there are some people that see this as the only way
 forward. Also, we would have to create a separate GEM buffer for
 the metadatada itself, which seems excessive.

We personally like option (B) better, and have already started to
prototype the new path (which is actually very similar to the
drmModeAddFB2() one). You can take a look at the new interfaces here:


https://github.com/mvicomoya/linux/tree/wip/mvicomoya/drm_addfb_with_metadata__4.14-rc8

There may be other options that haven't been explored yet that could be
a better choice than the above, so any suggestion will be greatly
appreciated.

Re: [Mesa-dev] GBM and the Device Memory Allocator Proposals

[Miguel Angel Vico]

On Wed, 6 Dec 2017 16:57:45 -0800
James Jones  wrote:

> On 12/06/2017 03:25 AM, Nicolai Hähnle wrote:
> > On 06.12.2017 08:07, James Jones wrote:
> > [snip]  
> >> So lets say you have a setup where both display and GPU supported
> >> FOO/tiled, but only GPU supported compressed (FOO/CC) and cached
> >> (FOO/cached).  But the GPU supported the following transitions:
> >>
> >>     trans_a: FOO/CC -> null
> >>     trans_b: FOO/cached -> null
> >>
> >> Then the sets for each device (in order of preference):
> >>
> >> GPU:
> >>     1: caps(FOO/tiled, FOO/CC, FOO/cached); 
> >> constraints(alignment=32k)
> >>     2: caps(FOO/tiled, FOO/CC); constraints(alignment=32k)
> >>     3: caps(FOO/tiled); constraints(alignment=32k)
> >>
> >> Display:
> >>     1: caps(FOO/tiled); constraints(alignment=64k)
> >>
> >> Merged Result:
> >>     1: caps(FOO/tiled, FOO/CC, FOO/cached); 
> >> constraints(alignment=64k);
> >>    transition(GPU->display: trans_a, trans_b; display->GPU: none)
> >>     2: caps(FOO/tiled, FOO/CC); constraints(alignment=64k);
> >>    transition(GPU->display: trans_a; display->GPU: none)
> >>     3: caps(FOO/tiled); constraints(alignment=64k);
> >>    transition(GPU->display: none; display->GPU: none)  
> >
> >
> > We definitely don't want to expose a way of getting uncached rendering
> > surfaces for radeonsi. I mean, I think we are supposed to be able 
> > to program
> > our hardware so that the backend bypasses all caches, but (a) nobody
> > validates that and (b) it's basically suicide in terms of 
> > performance. Let's
> > build fewer footguns :)  
> 
>  sure, this was just a hypothetical example.  But to take this case as
>  another example, if you didn't want to expose uncached rendering (or
>  cached w/ cache flushes after each draw), you would exclude the entry
>  from the GPU set which didn't have FOO/cached (I'm adding back a
>  cached but not CC config just to make it interesting), and end up
>  with:
> 
>      trans_a: FOO/CC -> null
>      trans_b: FOO/cached -> null
> 
>  GPU:
>     1: caps(FOO/tiled, FOO/CC, FOO/cached); constraints(alignment=32k)
>     2: caps(FOO/tiled, FOO/cached); constraints(alignment=32k)
> 
>  Display:
>     1: caps(FOO/tiled); constraints(alignment=64k)
> 
>  Merged Result:
>     1: caps(FOO/tiled, FOO/CC, FOO/cached); constraints(alignment=64k);
>    transition(GPU->display: trans_a, trans_b; display->GPU: none)
>     2: caps(FOO/tiled, FOO/cached); constraints(alignment=64k);
>    transition(GPU->display: trans_b; display->GPU: none)
> 
>  So there isn't anything in the result set that doesn't have GPU cache,
>  and the cache-flush transition is always in the set of required
>  transitions going from GPU -> display
> 
>  Hmm, I guess this does require the concept of a required cap..  
> >>>
> >>> Which we already introduced to the allocator API when we realized we
> >>> would need them as we were prototyping.  
> >>
> >> Note I also posed the question of whether things like cached (and 
> >> similarly compression, since I view compression as roughly an 
> >> equivalent mechanism to a cache) in one of the open issues on my XDC 
> >> 2017 slides because of this very problem of over-pruning it causes.  
> >> It's on slide 15, as "No device-local capabilities".  You'll have to 
> >> listen to my coverage of it in the recorded presentation for that 
> >> slide to make any sense, but it's the same thing Nicolai has laid out 
> >> here.
> >>
> >> As I continued working through our prototype driver support, I found I 
> >> didn't actually need to include cached or compressed as capabilities: 
> >> The GPU just applies them as needed and the usage transitions make it 
> >> transparent to the non-GPU engines.  That does mean the GPU driver 
> >> currently needs to be the one to realize the allocation from the 
> >> capability set to get optimal behavior.  We could fix that by 
> >> reworking our driver though.  At this point, not including 
> >> device-local properties like on-device caching in capabilities seems 
> >> like the right solution to me.  I'm curious whether this applies 
> >> universally though, or if other hardware doesn't fit the "compression 
> >> and stuff all behaves like a cache" idiom.  
> > 
> > Compression is a part of the memory layout for us: framebuffer 
> > compression uses an additional "meta surface". At the most basic level, 
> > an allocation with loss-less compression support is by necessity bigger 
> > than an allocation without.
> > 
> > We can allocate this meta surface separately, but then we're forced to 
> > decompress when passing the surface around (e.g. to a compositor.)
> > 
> > Consider also the example I gave elsewhere, where a 

Re: [Mesa-dev] GBM and the Device Memory Allocator Proposals

[Miguel Angel Vico]

On Fri, 1 Dec 2017 13:38:41 -0500
Rob Clark  wrote:

> On Fri, Dec 1, 2017 at 12:09 PM, Nicolai Hähnle  wrote:
> > On 01.12.2017 16:06, Rob Clark wrote:  
> >>
> >> On Thu, Nov 30, 2017 at 5:43 PM, Nicolai Hähnle 
> >> wrote:  
> >>>
> >>> Hi,
> >>>
> >>> I've had a chance to look a bit more closely at the allocator prototype
> >>> repository now. There's a whole bunch of low-level API design feedback,
> >>> but
> >>> for now let's focus on the high-level stuff first.
> >>>
> >>> Going by the 4.5 major object types (as also seen on slide 5 of your
> >>> presentation [0]), assertions and usages make sense to me.
> >>>
> >>> Capabilities and capability sets should be cleaned up in my opinion, as
> >>> the
> >>> status quo is overly obfuscating things. What capability sets really
> >>> represent, as far as I understand them, is *memory layouts*, and so
> >>> that's
> >>> what they should be called.
> >>>
> >>> This conceptually simplifies `derive_capabilities` significantly without
> >>> any
> >>> loss of expressiveness as far as I can see. Given two lists of memory
> >>> layouts, we simply look for which memory layouts appear in both lists,
> >>> and
> >>> then merge their constraints and capabilities.
> >>>
> >>> Merging constraints looks good to me.
> >>>
> >>> Capabilities need some more thought. The prototype removes capabilities
> >>> when
> >>> merging layouts, but I'd argue that that is often undesirable. (In fact,
> >>> I
> >>> cannot think of capabilities which we'd always want to remove.)
> >>>
> >>> A typical example for this is compression (i.e. DCC in our case). For
> >>> rendering usage, we'd return something like:
> >>>
> >>> Memory layout: AMD/tiled; constraints(alignment=64k); caps(AMD/DCC)
> >>>
> >>> For display usage, we might return (depending on hardware):
> >>>
> >>> Memory layout: AMD/tiled; constraints(alignment=64k); caps(none)
> >>>
> >>> Merging these in the prototype would remove the DCC capability, even
> >>> though
> >>> it might well make sense to keep it there for rendering. Dealing with the
> >>> fact that display usage does not have this capability is precisely one of
> >>> the two things that transitions are about! The other thing that
> >>> transitions
> >>> are about is caches.
> >>>
> >>> I think this is kind of what Rob was saying in one of his mails.  
> >>
> >>
> >> Perhaps "layout" is a better name than "caps".. either way I think of
> >> both AMD/tiled and AMD/DCC as the same type of "thing".. the
> >> difference between AMD/tiled and AMD/DCC is that a transition can be
> >> provided for AMD/DCC.  Other than that they are both things describing
> >> the layout.  
> >
> >
> > The reason that a transition can be provided is that they aren't quite the
> > same thing, though. In a very real sense, AMD/DCC is a "child" property of
> > AMD/tiled: DCC is implemented as a meta surface whose memory layout depends
> > on the layout of the main surface.  
> 
> I suppose this is six-of-one, half-dozen of the other..
> 
> what you are calling a layout is what I'm calling a cap that just
> happens not to have an associated transition
> 
> > Although, if there are GPUs that can do an in-place "transition" between
> > different tiling layouts, then the distinction is perhaps really not as
> > clear-cut. I guess that would only apply to tiled renderers.  
> 
> I suppose the advantage of just calling both layout and caps the same
> thing, and just saying that a "cap" (or "layout" if you prefer that
> name) can optionally have one or more associated transitions, is that
> you can deal with cases where sometimes a tiled format might actually
> have an in-place transition ;-)
> 
> >  
> >> So lets say you have a setup where both display and GPU supported
> >> FOO/tiled, but only GPU supported compressed (FOO/CC) and cached
> >> (FOO/cached).  But the GPU supported the following transitions:
> >>
> >>trans_a: FOO/CC -> null
> >>trans_b: FOO/cached -> null
> >>
> >> Then the sets for each device (in order of preference):
> >>
> >> GPU:
> >>1: caps(FOO/tiled, FOO/CC, FOO/cached); constraints(alignment=32k)
> >>2: caps(FOO/tiled, FOO/CC); constraints(alignment=32k)
> >>3: caps(FOO/tiled); constraints(alignment=32k)
> >>
> >> Display:
> >>1: caps(FOO/tiled); constraints(alignment=64k)
> >>
> >> Merged Result:
> >>1: caps(FOO/tiled, FOO/CC, FOO/cached); constraints(alignment=64k);
> >>   transition(GPU->display: trans_a, trans_b; display->GPU: none)
> >>2: caps(FOO/tiled, FOO/CC); constraints(alignment=64k);
> >>   transition(GPU->display: trans_a; display->GPU: none)
> >>3: caps(FOO/tiled); constraints(alignment=64k);
> >>   transition(GPU->display: none; display->GPU: none)  
> >
> >
> > We definitely don't want to expose a way of getting uncached rendering
> > surfaces for radeonsi. I mean, I think we are supposed to be able to program
> > our hardware so that the backend 

Re: [Mesa-dev] GBM and the Device Memory Allocator Proposals

[Miguel Angel Vico]

On Wed, 29 Nov 2017 16:28:15 -0500
Rob Clark <robdcl...@gmail.com> wrote:

> On Wed, Nov 29, 2017 at 2:41 PM, Miguel Angel Vico <mvicom...@nvidia.com> 
> wrote:
> > Many of you may already know, but James is going to be out for a few
> > weeks and I'll be taking over this in the meantime.
> >
> > See inline for comments.
> >
> > On Wed, 29 Nov 2017 09:33:29 -0800
> > Jason Ekstrand <ja...@jlekstrand.net> wrote:
> >  
> >> On Sat, Nov 25, 2017 at 1:20 PM, Rob Clark <robdcl...@gmail.com> wrote:
> >>  
> >> > On Sat, Nov 25, 2017 at 12:46 PM, Jason Ekstrand <ja...@jlekstrand.net>
> >> > wrote:  
> >> > > On November 24, 2017 09:29:43 Rob Clark <robdcl...@gmail.com> wrote:  
> >> > >>
> >> > >>
> >> > >> On Mon, Nov 20, 2017 at 8:11 PM, James Jones <jajo...@nvidia.com>  
> >> > wrote:  
> >> > >>>
> >> > >>> As many here know at this point, I've been working on solving issues
> >> > >>> related
> >> > >>> to DMA-capable memory allocation for various devices for some time 
> >> > >>> now.
> >> > >>> I'd
> >> > >>> like to take this opportunity to apologize for the way I handled the 
> >> > >>>  
> >> > EGL  
> >> > >>> stream proposals.  I understand now that the development process  
> >> > followed  
> >> > >>> there was unacceptable to the community and likely offended many 
> >> > >>> great
> >> > >>> engineers.
> >> > >>>
> >> > >>> Moving forward, I attempted to reboot talks in a more constructive  
> >> > manner  
> >> > >>> with the generic allocator library proposals & discussion forum at 
> >> > >>> XDC
> >> > >>> 2016.
> >> > >>> Some great design ideas came out of that, and I've since been  
> >> > prototyping  
> >> > >>> some code to prove them out before bringing them back as official
> >> > >>> proposals.
> >> > >>> Again, I understand some people are growing concerned that I've been
> >> > >>> doing
> >> > >>> this off on the side in a github project that has primarily NVIDIA
> >> > >>> contributors.  My goal was only to avoid wasting everyone's time with
> >> > >>> unproven ideas.  The intent was never to dump the prototype code 
> >> > >>> as-is  
> >> > on  
> >> > >>> the community and presume acceptance. It's just a public research
> >> > >>> project.
> >> > >>>
> >> > >>> Now the prototyping is nearing completion, and I'd like to renew
> >> > >>> discussion
> >> > >>> on whether and how the new mechanisms can be integrated with the 
> >> > >>> Linux
> >> > >>> graphics stack.
> >> > >>>
> >> > >>> I'd be interested to know if more work is needed to demonstrate the
> >> > >>> usefulness of the new mechanisms, or whether people think they have  
> >> > value  
> >> > >>> at
> >> > >>> this point.
> >> > >>>
> >> > >>> After talking with people on the hallway track at XDC this year, I've
> >> > >>> heard
> >> > >>> several proposals for incorporating the new mechanisms:
> >> > >>>
> >> > >>> -Include ideas from the generic allocator design into GBM.  This 
> >> > >>> could
> >> > >>> take
> >> > >>> the form of designing a "GBM 2.0" API, or incrementally adding to the
> >> > >>> existing GBM API.
> >> > >>>
> >> > >>> -Develop a library to replace GBM.  The allocator prototype code 
> >> > >>> could  
> >> > be  
> >> > >>> massaged into something production worthy to jump start this process.
> >> > >>>
> >> > >>> -Develop a library that sits beside or on top of GBM, using GBM for
> >> > >>> low-level graphics buffer allocation, while supporting non-graphics
> >> > >>> kernel
> >> > >

Re: [Mesa-dev] GBM and the Device Memory Allocator Proposals

[Miguel Angel Vico]

Many of you may already know, but James is going to be out for a few
weeks and I'll be taking over this in the meantime.

See inline for comments.

On Wed, 29 Nov 2017 09:33:29 -0800
Jason Ekstrand  wrote:

> On Sat, Nov 25, 2017 at 1:20 PM, Rob Clark  wrote:
> 
> > On Sat, Nov 25, 2017 at 12:46 PM, Jason Ekstrand 
> > wrote:  
> > > On November 24, 2017 09:29:43 Rob Clark  wrote:  
> > >>
> > >>
> > >> On Mon, Nov 20, 2017 at 8:11 PM, James Jones   
> > wrote:  
> > >>>
> > >>> As many here know at this point, I've been working on solving issues
> > >>> related
> > >>> to DMA-capable memory allocation for various devices for some time now.
> > >>> I'd
> > >>> like to take this opportunity to apologize for the way I handled the  
> > EGL  
> > >>> stream proposals.  I understand now that the development process  
> > followed  
> > >>> there was unacceptable to the community and likely offended many great
> > >>> engineers.
> > >>>
> > >>> Moving forward, I attempted to reboot talks in a more constructive  
> > manner  
> > >>> with the generic allocator library proposals & discussion forum at XDC
> > >>> 2016.
> > >>> Some great design ideas came out of that, and I've since been  
> > prototyping  
> > >>> some code to prove them out before bringing them back as official
> > >>> proposals.
> > >>> Again, I understand some people are growing concerned that I've been
> > >>> doing
> > >>> this off on the side in a github project that has primarily NVIDIA
> > >>> contributors.  My goal was only to avoid wasting everyone's time with
> > >>> unproven ideas.  The intent was never to dump the prototype code as-is  
> > on  
> > >>> the community and presume acceptance. It's just a public research
> > >>> project.
> > >>>
> > >>> Now the prototyping is nearing completion, and I'd like to renew
> > >>> discussion
> > >>> on whether and how the new mechanisms can be integrated with the Linux
> > >>> graphics stack.
> > >>>
> > >>> I'd be interested to know if more work is needed to demonstrate the
> > >>> usefulness of the new mechanisms, or whether people think they have  
> > value  
> > >>> at
> > >>> this point.
> > >>>
> > >>> After talking with people on the hallway track at XDC this year, I've
> > >>> heard
> > >>> several proposals for incorporating the new mechanisms:
> > >>>
> > >>> -Include ideas from the generic allocator design into GBM.  This could
> > >>> take
> > >>> the form of designing a "GBM 2.0" API, or incrementally adding to the
> > >>> existing GBM API.
> > >>>
> > >>> -Develop a library to replace GBM.  The allocator prototype code could  
> > be  
> > >>> massaged into something production worthy to jump start this process.
> > >>>
> > >>> -Develop a library that sits beside or on top of GBM, using GBM for
> > >>> low-level graphics buffer allocation, while supporting non-graphics
> > >>> kernel
> > >>> APIs directly.  The additional cross-device negotiation and sorting of
> > >>> capabilities would be handled in this slightly higher-level API before
> > >>> handing off to GBM and other APIs for actual allocation somehow.  
> > >>
> > >>
> > >> tbh, I kinda see GBM and $new_thing sitting side by side.. GBM is
> > >> still the "winsys" for running on "bare metal" (ie. kms).  And we
> > >> don't want to saddle $new_thing with aspects of that, but rather have
> > >> it focus on being the thing that in multiple-"device"[1] scenarious
> > >> figures out what sort of buffer can be allocated by who for sharing.
> > >> Ie $new_thing should really not care about winsys level things like
> > >> cursors or surfaces.. only buffers.
> > >>
> > >> The mesa implementation of $new_thing could sit on top of GBM,
> > >> although it could also just sit on top of the same internal APIs that
> > >> GBM sits on top of.  That is an implementation detail.  It could be
> > >> that GBM grows an API to return an instance of $new_thing for
> > >> use-cases that involve sharing a buffer with the GPU.  Or perhaps that
> > >> is exposed via some sort of EGL extension.  (We probably also need a
> > >> way to get an instance from libdrm (?) for display-only KMS drivers,
> > >> to cover cases like etnaviv sharing a buffer with a separate display
> > >> driver.)
> > >>
> > >> [1] where "devices" could be multiple GPUs or multiple APIs for one or
> > >> more GPUs, but also includes non-GPU devices like camera, video
> > >> decoder, "image processor" (which may or may not be part of camera),
> > >> etc, etc  
> > >
> > >
> > > I'm not quite some sure what I think about this.  I think I would like to
> > > see $new_thing at least replace the guts of GBM. Whether GBM becomes a
> > > wrapper around $new_thing or $new_thing implements the GBM API, I'm not
> > > sure.  What I don't think I want is to see GBM development continuing on
> > > it's own so we have two competing solutions.  
> >
> > I don't really view them as 

Re: [Mesa-dev] [PATCH 7/7] wayland-egl: rework and simplify wl_egl_window initialization

[Miguel Angel Vico]

> Miguel I believe the comment correctly describes the design plan, while
> addressing Dan's comment that things look a bit ugly.

Yes. Thank you.

Also, the whole series:

Reviewed-by: Miguel A. Vico 


Thanks.

On Fri, 29 Sep 2017 14:48:58 +0100
Daniel Stone  wrote:

> On 29 September 2017 at 13:14, Emil Velikov  wrote:
> > Use calloc instead of malloc + explicitly zeroing the different fields.
> > We need special handling for the version field which if of type
> > const intptr_t.
> >
> > As we're here document why keeping the constness is a good idea.
> >
> > The wl_egl_window_resize() call is replaced with an explicit set of the
> > width/height.  
> 
> Thanks for doing this; series is:
> Reviewed-by: Daniel Stone 
> 
> Cheers,
> Daniel
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


-- 
Miguel


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] XDC 2017 feedback

[Miguel Angel Vico]

Hi,

In general, I think the organization was great. I agree having
everything happen in a single room was a good point. Here's some of my
personal feedback:

 * I didn't like the tables layout at all. I found it to be extremely
   uncomfortable to have to look sideways in order to see the screens
   and presenter.

 * There were a very few power strips, and not well distributed along
   the tables.


Also, this is what I've been able to gather from some of my colleagues
here at NVIDIA::

 * Some people watching the conference remotely complained about the
   stream quality, and the recordings wouldn't include the presenter.

   In one of the hallway conversations, Martin mentioned in XDC2016
   they had a team of camera experts doing the job, and will try to
   improve that part in future XDC's.

 * The microphone/audio situation wasn't great either.  I don't know
   how practical it is with something the size of XDC, but at Khronos
   meetings, they usually set up microphones for the audience too, with
   tap-on/tap-off switches, and a ratio of 1:2 or 1:3 for
   microphones:people.  That makes Q a lot easier.  Alternatively,
   having a question queue rather than running mics around the room can
   speed things up, but makes cross-talk harder.

 * The table/chair layout was really cumbersome. Beyond just the
   orientation, some people had a lot of trouble getting in/out to use
   the restroom, grab snacks, etc.


On a positive note:

 * More time for hallway conversations was in general a good thing.
   Some of us got a ton of useful feedback talking to others.

 * The food was great, and having food on-site gave us even more time
   for hallway-tracking.

 * Surprisingly, parking was not an issue.

 * Signage was good, and the security guards were polite/helpful. It
   was easy to find the room and get our badges.

 * The wifi worked great in general, which is a rarity at conferences.
   It was pretty spotty at XDC 2016.


Thank you.

On Tue, 26 Sep 2017 11:49:10 -0700
Manasi Navare wrote:

> Hi,
> 
> XDC was a really great conference and loved the fact that it was
> in just one room which kept all the hallway conversations in that room 
> resulting
> into more networking.
> But I agree with Andres that for the videos, it would be great to split the
> huge youtube video stream per presentation and have seperate links for each
> talk somewhere on the XDC page.
> 
> Regards
> Manasi
> 
> 
> 
> On Tue, Sep 26, 2017 at 01:25:15PM -0400, Andres Rodriguez wrote:
> > Hi,
> > 
> > A small piece of feedback from those of us watching remotely. It would be
> > nice to have a simple to access index for the long livestream videos.
> > 
> > I think the XDC 2017 wiki page would be a good place for this information. A
> > brief example:
> > 
> > Presentation Title | Presenter Name | Link with timestamp
> > ---||-
> > ...| ...| youtu.be/video-id?t=XhYmZs
> > 
> > Or alternatively, a list of hyperlinks with the presentation title as text
> > that point to the correct timestamp in the video would also be sufficient.
> > 
> > Really enjoyed the talks, and would like them to be slightly easier to
> > access and share with others.
> > 
> > Regards,
> > Andres
> > 
> > 
> > On 2017-09-26 12:57 PM, Daniel Vetter wrote:  
> > >Hi all,
> > >
> > >First again big thanks to Stéphane and Jennifer for organizing a great XDC.
> > >
> > >Like last year we'd like to hear feedback on how this year's XDC went,
> > >both the good (and what you'd like to see more of) and the not so
> > >good. Talk selection, organization, location, scheduling of talks,
> > >anything really.
> > >
> > >Here's a few things we took into account from Helsinki and tried to apply:
> > >- More breaks for more hallway track.
> > >- Try to schedule the hot topics on the first day (did we pick the
> > >right ones) for better hallway track.
> > >- More lightning talk time to give all the late/rejected submissions
> > >some place to give a quick showcase.
> > >
> > >Things that didn't work out perfectly this year and that we'll try to
> > >get better at next year:
> > >- Lots of people missed the submission deadline and their talks were
> > >rejected only because of that. We'll do better PR by sending out a
> > >pile of reminders.
> > >- Comparitively few people asked for travel assistance. No idea
> > >whether this was a year with more budget around, or whether this isn't
> > >all that well know and we need to make more PR in maybe the call for
> > >papers about it.
> > >
> > >But that's just the stuff we've gathered already, we'd like to hear
> > >more feedback. Just reply to this mail or send a mail to
> > >bo...@foundation.x.org if you don't want the entire world to read it.
> > >And if you want to send at pseudonymous feedback, drop a pastebin onto
> > >the #xf-bod channel on the OFTC irc server.
> > >
> > >Thanks, Daniel
> > >  
> > 

Re: [Mesa-dev] [PATCH mesa 5/5] wayland-egl: Update ABI checker

[Miguel Angel Vico]

On Wed, 19 Jul 2017 12:19:30 +0100
Emil Velikov  wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico  wrote:
> > This change updates wayland-egl-abi-check.c with the latest changes to
> > wl_egl_window.
> >
> > Signed-off-by: Miguel A. Vico 
> > Reviewed-by: James Jones 
> > ---
> >  .../wayland/wayland-egl/wayland-egl-abi-check.c| 78 
> > ++
> >  1 file changed, 65 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c 
> > b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > index 1962f05850..6bdd71b6e0 100644
> > --- a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > +++ b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > @@ -31,7 +31,28 @@
> >   * DO NOT EVER CHANGE!
> >   */
> >
> > +/* From: a2ab5c2588 - Miguel A. Vico : wayland-egl: Make wl_egl_window a 
> > versioned struct */  
> Please keep the sha as XXX - we'll update it as the commit lands.
> 

Done.

> > +#define WL_EGL_WINDOW_VERSION_v3 3
> > +struct wl_egl_window_v3 {
> > +const intptr_t version;
> > +
> > +int width;
> > +int height;
> > +int dx;
> > +int dy;
> > +
> > +int attached_width;
> > +int attached_height;
> > +
> > +void *private;
> > +void (*resize_callback)(struct wl_egl_window *, void *);
> > +void (*destroy_window_callback)(void *);
> > +
> > +struct wl_surface *surface;
> > +};
> > +
> >  /* From: 690ead4a13 - Stencel, Joanna : egl/wayland-egl: Fix for segfault 
> > in dri2_wl_destroy_surface. */
> > +#define WL_EGL_WINDOW_VERSION_v2 2
> >  struct wl_egl_window_v2 {
> >  struct wl_surface *surface;
> >
> > @@ -123,6 +144,20 @@ struct wl_egl_window_v0 {
> >  }  
> >  \
> >  } while (0)
> >
> > +#define CHECK_VERSION(A_VER, B_VER, MATCH) 
> >  \
> > +do {   
> >  \
> > +if (((MATCH)  && (WL_EGL_WINDOW_VERSION ## A_VER) !=   
> >  \
> > + (WL_EGL_WINDOW_VERSION ## B_VER)) ||  
> >  \
> > +(!(MATCH) && (WL_EGL_WINDOW_VERSION ## A_VER) >=   
> >  \
> > + (WL_EGL_WINDOW_VERSION ## B_VER))) {  
> >  \
> > +printf("Backards incompatible change detected!\n   "   
> >  \
> > +   "WL_EGL_WINDOW_VERSION" #A_VER " %s "   
> >  \
> > +   "WL_EGL_WINDOW_VERSION" #B_VER "\n",
> >  \
> > +   ((MATCH) ? "!=" : ">="));   
> >  \
> > +return 1;  
> >  \
> > +}  
> >  \
> > +} while (0)
> > +  
> Same crazy idea as CHECK_SIZE - worth having separate macros?

Yup. Done.

> 
> >  int main(int argc, char **argv)
> >  {
> >  /* Check wl_egl_window_v1 ABI against wl_egl_window_v0 */
> > @@ -149,19 +184,36 @@ int main(int argc, char **argv)
> >
> >  CHECK_SIZE(_v1, _v2, FALSE);
> >
> > -/* Check wl_egl_window ABI against wl_egl_window_v2 */
> > -CHECK_MEMBER(_v2,, surface, surface);
> > -CHECK_MEMBER(_v2,, width,   width);
> > -CHECK_MEMBER(_v2,, height,  height);
> > -CHECK_MEMBER(_v2,, dx,  dx);
> > -CHECK_MEMBER(_v2,, dy,  dy);
> > -CHECK_MEMBER(_v2,, attached_width,  attached_width);
> > -CHECK_MEMBER(_v2,, attached_height, attached_height);
> > -CHECK_MEMBER(_v2,, private, private);
> > -CHECK_MEMBER(_v2,, resize_callback, resize_callback);
> > -CHECK_MEMBER(_v2,, destroy_window_callback, destroy_window_callback);
> > -
> > -CHECK_SIZE(_v2,, TRUE);
> > +/* Check wl_egl_window_v3 ABI against wl_egl_window_v2 */
> > +CHECK_MEMBER(_v2, _v3, surface, version);  
> Just hit me that with the current CHECK_MEMBER macro changes like the
> above will be easier to miss. Or someone will attempt to "correct" it.
> The following seem a bit more obvious, imho.
> 
> CHECK_RENAMED_MEMBER(_v2, _v3, surface, version);
> CHECK_MEMBER(_v2, _v3, width);

Sure. Done.

> 
> > +CHECK_MEMBER(_v2, _v3, width,   width);
> > +CHECK_MEMBER(_v2, _v3, height,  height);
> > +CHECK_MEMBER(_v2, _v3, dx,  dx);
> > +CHECK_MEMBER(_v2, _v3, dy,  dy);
> > +CHECK_MEMBER(_v2, _v3, attached_width,  attached_width);
> > +CHECK_MEMBER(_v2, _v3, attached_height, attached_height);

Re: [Mesa-dev] [PATCH mesa 4/5] wayland-egl: Make wl_egl_window a versioned struct

[Miguel Angel Vico]

On Wed, 19 Jul 2017 12:06:06 +0100
Emil Velikov  wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico  wrote:
> > We need wl_egl_window to be a versioned struct in order to keep track of
> > ABI changes.
> >
> > This change makes the first member of wl_egl_window the version number.
> >
> > An heuristic in the wayland driver is added so that we don't break
> > backwards compatibility:
> >
> >  - If the first field (version) is an actual pointer, it is an old
> >implementation of wl_egl_window, and version points to the wl_surface
> >proxy.
> >
> >  - Else, the first field is the version number, and we have
> >wl_egl_window::surface pointing to the wl_surface proxy.
> >
> > Signed-off-by: Miguel A. Vico 
> > Reviewed-by: James Jones   
> 
> This commit will cause a break in the ABI checker. Yet again, I'm
> short on ideas how to avoid that :-(

Yeah... The only think I can think of is pushing both this and 5/5 as a
single commit.

I usually like to keep things separate. Is it much of a deal given that
they'll go in at the same time?

> 
> > ---
> >  src/egl/drivers/dri2/platform_wayland.c| 13 -
> >  src/egl/wayland/wayland-egl/wayland-egl-priv.h |  6 +-
> >  src/egl/wayland/wayland-egl/wayland-egl.c  |  6 +-
> >  3 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> > b/src/egl/drivers/dri2/platform_wayland.c
> > index ee68284217..0f0a12fd80 100644
> > --- a/src/egl/drivers/dri2/platform_wayland.c
> > +++ b/src/egl/drivers/dri2/platform_wayland.c
> > @@ -41,6 +41,7 @@
> >  #include "egl_dri2.h"
> >  #include "egl_dri2_fallbacks.h"
> >  #include "loader.h"
> > +#include "eglglobals.h"
> >
> >  #include 
> >  #include "wayland-drm-client-protocol.h"
> > @@ -100,6 +101,16 @@ destroy_window_callback(void *data)
> > dri2_surf->wl_win = NULL;
> >  }
> >
> > +static struct wl_surface *
> > +get_wl_surface_proxy(struct wl_egl_window *window)
> > +{
> > +   if (_eglPointerIsDereferencable((void *)(window->version))) {
> > +  /* window->version points to actual wl_surface data */
> > +  return wl_proxy_create_wrapper((void *)(window->version));
> > +   }  
> Please add a comment in there. I'm thinking about the following
> although use whatever you prefer.
> 
> Version 3 of wl_egl_window introduced a version field, at the same
> location where a pointer to wl_surface was stored.

Done. 

> 
> > +   return wl_proxy_create_wrapper(window->surface);
> > +}
> > +
> >  /**
> >   * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
> >   */
> > @@ -171,7 +182,7 @@ dri2_wl_create_window_surface(_EGLDriver *drv, 
> > _EGLDisplay *disp,
> > wl_proxy_set_queue((struct wl_proxy *)dri2_surf->wl_dpy_wrapper,
> >dri2_surf->wl_queue);
> >
> > -   dri2_surf->wl_surface_wrapper = 
> > wl_proxy_create_wrapper(window->surface);
> > +   dri2_surf->wl_surface_wrapper = get_wl_surface_proxy(window);
> > if (!dri2_surf->wl_surface_wrapper) {
> >_eglError(EGL_BAD_ALLOC, "dri2_create_surface");
> >goto cleanup_drm;
> > diff --git a/src/egl/wayland/wayland-egl/wayland-egl-priv.h 
> > b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> > index 92c31d9454..3b59908cc1 100644
> > --- a/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> > +++ b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> > @@ -41,8 +41,10 @@
> >  extern "C" {
> >  #endif
> >
> > +#define WL_EGL_WINDOW_VERSION 3
> > +
> >  struct wl_egl_window {
> > -   struct wl_surface *surface;
> > +   const intptr_t version;
> >
> > int width;
> > int height;
> > @@ -55,6 +57,8 @@ struct wl_egl_window {
> > void *private;
> > void (*resize_callback)(struct wl_egl_window *, void *);
> > void (*destroy_window_callback)(void *);
> > +
> > +   struct wl_surface *surface;
> >  };
> >
> >  #ifdef  __cplusplus
> > diff --git a/src/egl/wayland/wayland-egl/wayland-egl.c 
> > b/src/egl/wayland/wayland-egl/wayland-egl.c
> > index 4a4701a2de..02645549e0 100644
> > --- a/src/egl/wayland/wayland-egl/wayland-egl.c
> > +++ b/src/egl/wayland/wayland-egl/wayland-egl.c
> > @@ -28,6 +28,7 @@
> >   */
> >
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include "wayland-egl.h"
> > @@ -54,6 +55,7 @@ WL_EGL_EXPORT struct wl_egl_window *
> >  wl_egl_window_create(struct wl_surface *surface,
> >  int width, int height)
> >  {
> > +   struct wl_egl_window _INIT_ = { .version = WL_EGL_WINDOW_VERSION };
> > struct wl_egl_window *egl_window;
> >
> > if (width <= 0 || height <= 0)
> > @@ -63,6 +65,8 @@ wl_egl_window_create(struct wl_surface *surface,
> > if (!egl_window)
> > return NULL;
> >
> > +   memcpy(egl_window, &_INIT_, sizeof *egl_window);
> > +  
> The _INIT_ and memcpy seems like an overkill. At the same time the
> 

Re: [Mesa-dev] [PATCH mesa 3/5] egl: Fix _eglPointerIsDereferencable() to ignore page residency

[Miguel Angel Vico]

On Wed, 19 Jul 2017 11:43:43 +0100
Emil Velikov  wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico  wrote:
> > mincore() returns 0 on success, and -1 on failure.  The last parameter
> > is a vector of bytes with one entry for each page queried.  mincore
> > returns page residency information in the first bit of each byte in the
> > vector.
> >
> > Residency doesn't actually matter when determining whether a pointer is
> > dereferenceable, so the output vector can be ignored.  What matters is
> > whether mincore succeeds. See:
> >
> >   http://man7.org/linux/man-pages/man2/mincore.2.html
> >  
> Makes sense. Can you confirm that the BSD/Solaris manpages are on the same 
> page?

According to the man pages, they all seem to behave the same way in
that regard.

> 
> Considering they all agree
> Reviewed-by: Emil Velikov 
> 

Thanks. I sent the new rebased v2 version. Could you push on my behalf?

> -Emil

Thanks.

-- 
Miguel


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 2/5] egl: Move _eglPointerIsDereferencable() to eglglobals.[ch]

[Miguel Angel Vico]

On Wed, 19 Jul 2017 11:36:59 +0100
Emil Velikov  wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico  wrote:
> > More _eglPointerIsDereferencable() to eglglobals.[ch] and make it a
> > non-static function so it can be used out of egldisplay.c
> >  
> s/More/Move/

Done.

> 
> > Signed-off-by: Miguel A. Vico 
> > Reviewed-by: James Jones   
> Reviewed-by: Emil Velikov 

Thanks. I sent the new rebased v2 version. Could you push on my behalf?

> 
> -Emil

Thanks.

-- 
Miguel


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa 1/5] wayland-egl: Add wl_egl_window ABI checker

[Miguel Angel Vico]

Thanks for all the reviews, Emil.

Inline.

On Wed, 19 Jul 2017 11:35:04 +0100
Emil Velikov  wrote:

> Hi Miguel,
> 
> Thanks for looking into this. There's a couple of small nits below but
> it overall looks good.
> 
> On 18 July 2017 at 21:49, Miguel A. Vico  wrote:
> > Add a small ABI checker for wl_egl_window so that we can check for
> > backwards incompatible changes at 'make check' time.
> >
> > Signed-off-by: Miguel A. Vico 
> > Reviewed-by: James Jones 
> > ---
> >  src/egl/wayland/wayland-egl/Makefile.am|  10 +-
> >  .../wayland/wayland-egl/wayland-egl-abi-check.c| 167 
> > +
> >  2 files changed, 176 insertions(+), 1 deletion(-)
> >  create mode 100644 src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> >
> > diff --git a/src/egl/wayland/wayland-egl/Makefile.am 
> > b/src/egl/wayland/wayland-egl/Makefile.am
> > index 8c45e8e26d..74a52027c6 100644
> > --- a/src/egl/wayland/wayland-egl/Makefile.am
> > +++ b/src/egl/wayland/wayland-egl/Makefile.am
> > @@ -14,7 +14,15 @@ libwayland_egl_la_LDFLAGS = \
> > $(GC_SECTIONS) \
> > $(LD_NO_UNDEFINED)
> >
> > -TESTS = wayland-egl-symbols-check
> > +TESTS =
> > +check_PROGRAMS =
> > +
> > +TESTS += wayland-egl-symbols-check
> >  EXTRA_DIST = wayland-egl-symbols-check
> >
> > +TESTS += wayland-egl-abi-check
> > +check_PROGRAMS += wayland-egl-abi-check
> > +  
> Please add into the respective variables directly.
> 
> TESTS = \
>foo \
>bar
> 
> check_PROGRAMS = wayland-egl-abi-check
> 
> > +wayland_egl_abi_check_SOURCES = wayland-egl-abi-check.c
> > +  
> Feel free to drop this - default extension is ".c" so this will be
> picked automatically.

Done.

> 
> >  include $(top_srcdir)/install-lib-links.mk
> > diff --git a/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c 
> > b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > new file mode 100644
> > index 00..1962f05850
> > --- /dev/null
> > +++ b/src/egl/wayland/wayland-egl/wayland-egl-abi-check.c
> > @@ -0,0 +1,167 @@
> > +/*
> > + * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the 
> > "Software"),
> > + * to deal in the Software without restriction, including without 
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include  // offsetof
> > +#include   // printf
> > +
> > +#include "wayland-egl-priv.h" // Current struct wl_egl_window 
> > implementation
> > +
> > +/*
> > + * Following are previous implementations of wl_egl_window.
> > + *
> > + * DO NOT EVER CHANGE!
> > + */
> > +
> > +/* From: 690ead4a13 - Stencel, Joanna : egl/wayland-egl: Fix for segfault 
> > in dri2_wl_destroy_surface. */
> > +struct wl_egl_window_v2 {  
> 
> > +/* From: ca3ed3e024 - Ander Conselvan de Oliveira : egl/wayland: Don't 
> > invalidate drawable on swap buffers */
> > +struct wl_egl_window_v1 {  
> 
> > +/* From: 214fc6e850 - Benjamin Franzke : egl: Implement libwayland-egl */
> > +struct wl_egl_window_v0 {  
> Hats off for digging all the commits and adding references!
> 
> Can we declare the structs in the same order to how they are used - 0...
> 

Done.

> 
> > +/* This program checks we keep a backwards-compatible struct wl_egl_window
> > + * definition whenever it is modified in wayland-egl-priv.h.
> > + *
> > + * The previous definition should be added above as a new struct
> > + * wl_egl_window_vN, and the appropriate checks should be added below
> > + */
> > +
> > +#define TRUE  1
> > +#define FALSE 0
> > +  
> Use stdbool.h's true/false if the suggestion, below seem too much?
> 
> > +#define MEMBER_SIZE(TYPE, MEMBER) sizeof(((TYPE *)0)->MEMBER)
> > +
> > +#define CHECK_MEMBER(A_VER, B_VER, A_MEMBER, B_MEMBER) 
> >  \
> > +do {   
> >  \
> > +if (offsetof(struct wl_egl_window ##