Subject: Reworking our support for platform specific functions

From: Alexandros Frantzis <[email-masked]>
Date: Mon, 13 Oct 2014 14:37:04 +0300

Hi all,

the need has arisen to add another platform-specific function, in order
to resolve bug LP: #1379266. This addition triggered some concerns about
the way we implement such functions.

The problem
-----------

In our current implementation, adding a platform-specific function
causes platform-specific details to leak into many layers from client to
server. For example, for the existing drm_auth_magic() call we have:

Client API: mir_connection_drm_auth_magic(int magic, ...)
Client    : MirConnection::drm_auth_magic(...)
Protobuf  : rpc drm_auth_magic(DRMMagic) returns (DRMAuthMagicStatus)
Server    : SessionMediator::drm_auth_magic(), mg::DRMAuthenticator

This approach has a few drawbacks:

1. Platform-specific details leak in code that should be platform
   agnostic.
2. It can't support platform-specific functions for 3rd party platforms.
3. It's burdensome to add a new platform-specific functions (I guess
   this is a problem for our API functions in general).

Proposed solution
-----------------

To solve the issues mentioned above we need to provide a way to make an
opaque (as far as the rest of Mir is concerned) request to the platform:

Client API: mir_connection_platform_operation(type, DataAndFds request, ...);
Client    : MirConnection::platform_operation(type, ...)
Protobuf  : rpc platform_operation(type, DataAndFds) returns (DataAndFds)
Server    : SessionMediator::platform_operation()
Platform  : DataAndFds PlatformIpcOperations::platform_operation(type, DataAndFds)

Each platform should expose a header with its operation types and
expected parameters types. For mesa it could be something like:

enum MesaPlatformOperationType { drm_auth_magic, drm_get_auth_fd };
struct DRMAuthMagicRequest { int magic; }
struct DRMAuthMagicReply { int status; }
...

And a request would be:

DRMAuthMagicRequest request = { magic };

void platform_operation_callback(
    MirConnection* connection, DataAndFds const* reply, void* context)
{
    struct DRMAuthMagicReply const* magic_reply = reply->data;
    ...
}

mir_connection_platform_operation(
    drm_auth_magic, request, platform_operation_callback);

With this approach:

1. No platform-specific details leak into unrelated layers. Only the
   platform knows about these details and exposes them to other interested
   parties.

2+3. It's trivial for 3rd party platforms to expose platform-specific operations

This proposal involves changes and ABI bumps to a few layers, so I would
appreciate some early feedback and discussion.

Thoughts?

Thanks,
Alexandros

---

From: Daniel van Vugt <[email-masked]>
Date: Tue, 14 Oct 2014 09:57:45 +0800

Liking the idea of a generalised approach... Although on first glance 
this does not sound like a real function name:
    mir_connection_platform_operation()
I know we want to be generic, but that sounds somehow overly generic 
with "platform operation". Maybe we need a better function name there. 
Not "ioctl" :)

Also, to generalise you would probably not use a parameter named 
"DataAndFds" as "Fds" is too specific. The structure would need to 
contain a size and pointer (void*) field, although they may as well be 
parameters instead of a struct. And Protobuf could use opaque type "bytes".

Finally instead of MesaPlatformOperationType, you could just make "type" 
a UUID string. That has a few advantages:
   1. Adding new (private-ish) operations does not require public header 
changes.
   2. An operation can be made portable and understood by multiple 
platforms if need be.
   3. Operation types can be removed and retired without lingering or 
leaving a hole in any enum definition.

Then again, enum/ints are beautifully simple.

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Tue, 14 Oct 2014 18:14:52 +1100

I, too, like the idea of a generalised approach. I just think that this 
is insufficiently generalised :).

We're talking about extensions here, but again we're talking about 
throwing void* around. It's entirely possible to add a mechanism for 
the platform (or other code) to register extra protobuf handlers.

I look forward to discussing this in Washington :)

On Tue, Oct 14, 2014 at 12:57 PM, Daniel van Vugt 
<[email-masked]> wrote:
> with "platform operation". Maybe we need a better function name 
> there. Not "ioctl" :)
> 
> contain a size and pointer (void*) field, although they may as well 
> be parameters instead of a struct. And Protobuf could use opaque type 
> "bytes".
> 
> Finally instead of MesaPlatformOperationType, you could just make 
> "type" a UUID string. That has a few advantages:
>   1. Adding new (private-ish) operations does not require public 
> header changes.
>> Platform  : DataAndFds 
>> PlatformIpcOperations::platform_operation(type, DataAndFds)
>> 
> --
> Mir-devel mailing list
> [email-masked]
> Modify settings or unsubscribe at: 
> https://lists.ubuntu.com/mailman/listinfo/mir-devel

---

From: Alan Griffiths <[email-masked]>
Date: Tue, 14 Oct 2014 09:14:46 +0100

> the platform (or other code) to register extra protobuf handlers.

Not only is it possible, it has been done. We're currently talking of
withdrawing that feature:

https://code.launchpad.net/~alan-griffiths/mir/privatize-PrivateProtobuf/+merge/237436

---

From: Alexandros Frantzis <[email-masked]>
Date: Tue, 14 Oct 2014 11:45:19 +0300

> https://code.launchpad.net/~alan-griffiths/mir/privatize-PrivateProtobuf/+merge/237436

I'd rather we didn't go down the direct protobuf path. Protobuf is an
implementation detail which, IMO, would be a mistake to officially
expose outside our RPC layers. We could potentially add an abstraction
around the protobuf functionality we want to expose, but it remains to
be seen if such an effort is worth it.

Thanks,
Alexandros

---

From: Daniel van Vugt <[email-masked]>
Date: Tue, 14 Oct 2014 16:46:55 +0800

Remember a few of us at least aspire to retiring Protobuf eventually. 
One way or the other. Don't get too tied to it.

---

From: Alan Griffiths <[email-masked]>
Date: Tue, 14 Oct 2014 12:00:59 +0100

> the way we implement such functions.

I like the general approach, but have concerns about a bit of the detail.

1. Is there any point to having separating "type" from "data" and "fds"?
Or should the various data structures combine all three.
2. In the client API would be good to use an opaque "PlatformMessage"
type, rather than a DataAndFds struct.

E.g.

   DRMAuthMagicReply const* magic_reply = (DRMAuthMagicReply const*)mir_platform_get_data(reply);

3. With a C API we have to be explicit about ownership. E.g. does the client have to release 
magic_reply or does it become invalid after the call?
4. Do we need to address byte encoding, sizes and alignment of platform data?

---

From: Alexandros Frantzis <[email-masked]>
Date: Tue, 14 Oct 2014 16:16:38 +0300

> On 13/10/14 12:37, Alexandros Frantzis wrote:
<snip>
> >     drm_auth_magic, request, platform_operation_callback);
<snip>
> I like the general approach, but have concerns about a bit of the detail.

A caveat: the original email was meant mainly to illustrate the overall
approach, I didn't focus on the details much (e.g. names and types are
all tentative).

> Or should the various data structures combine all three.

We could avoid exposing any structure, apart from data vs fds (see
below).

> type, rather than a DataAndFds struct.

We need to be able to separate the fds from the rest of the data, so
that our RPC layer can send the fds properly. That's what the
(tentative) name was trying to convey.

> magic_reply or does it become invalid after the call?

We could go either way, weighing the cost of copying data (and dup()-ing
fds) vs the potential for memory/fd leaks. Let's be consistent with how
we handle other similar cases in the client API (do we have other such
cases?).

> 4. Do we need to address byte encoding, sizes and alignment of platform data?

I would say not in Mir. If a message has special needs, the sender
should take care to serialize/deserialize it properly.

Thanks,
Alexandros

---

From: Kevin DuBois <[email-masked]>
Date: Tue, 14 Oct 2014 16:27:09 -0400

Thinking from a different angle,

Some of this has to do with the fact that the mesa native window and native
display interface and the mir client api is blended together. If we
separated them out and moved the mesa stuff to native window headers, we'd
have a cleaner api.

Like, I consider these functions platform-specific:
mir_surface_get_platform_type
mir_surface_get_current_buffer
mir_connection_drm_auth_magic
mir_connection_drm_set_gbm_device
because they're used by mesa's egl and android (and android clients) don't
really use them, just mesa egl needs them for various reasons.

If our 'upstream' mesa would define a native buffer and native display
interface (or if certain mir headers would just migrate upstream), this
would let mir's mesa platform implement these interfaces. Android is forced
into doing this because we can't move the egl libraries, and must rely on
<system/window.h> for the native window type (android doesn't have a native
display type really)

Then, we could have a smaller client api with the sole role of supporting
people writing mir clients, instead of supporting both the people writing
mir clients and one of the platform's egl implementations.

Cheers,
Kevin

P.S.
Same line-of-thinking, but slightly different... MirNativeBuffer isn't a
convenient buffer type for android (have to unpack the ANativeWindowBuffer
into mesa's MirNativeBuffer)