Prepare GPU session for ported drivers

[cnuke]

The following commit series prepares the current Gpu session for being used with GPU drivers based on contrib code, i.e., Linux DRM drivers.

[cnuke]

(See comment below.)

[cnuke]

• 225c3b2 - gpu_session: use Buffer_id to identify buffers this commit introduces local buffer identifiers that makes it easier later on to match the buffer-objects in a driver component where the id is part of the execution buffer

• cff8e91 - gpu_session: move exec seqno to Gpu namespace promote the execution sequence number type to the Gpu namespace (this a preparation commit for 3f09582 and eventually 680a107)

• 3f09582 - gpu_session: introduce information dataspace to accommodate different drivers where the exposed information differs and to lift the size limit imposed by the RPC interface export the GPU information as dataspace

• 680a107 - gpu_session: add 'complete' RPC call introduce a call for checking the completion state of an execution buffer rather than looking into the GPU information and hide the— order of sequence numbers from the client

• f91625d - gpu_session: introduce mapping type propagate the type of access from the CPU side for a mapped dataspace

[nfeske]

Thank you for the commit series.

Regarding the "gpu_session: use Buffer_id to identify buffers" commit, have you considered the use of Id_space::Id instead of defining a custom Buffer_id struct in gpu_session.h?

With a Buffer type defined (a forward declaration is enough), an ID referring to a buffer can be expressed as Id_space<Buffer>::Id. So the Buffer_id type can be used directly with the Id_space instead. As a rule of thumb, the value of an ID should at best not be visible at the API at all.

typedef  Id_space<Buffer>::Id Buffer_id;

I think this would reduce redundancy like your custom _apply_buffer implementation, and would bind the lifetime of the ID to an ID element thereby reinforcing robustness regarding the lifetime of IDs.

As a minor style remark, please remove the braces around single statements. Also put statements that alter the control flow (like return, break, continue) at the beginning of a new line (https://genode.org/documentation/developer-resources/coding_style#Indentation). E.g.,

if (!buffer.map.cap.valid()) { return; }

should be written as

if (!buffer.map.cap.valid())
  return;

[nfeske]

Now that you are touching the Gpu::Execution_buffer_sequence in "gpu_session: move exec seqno to Gpu namespace", how do you think about renaming the type to Gpu::Sequence_number? This would improve the intuitive meaning of the type and make it shorter.

[nfeske]

The purpose of the Mapping_type should better be clarified in gpu_session.h. To me, the meaning of the four values is vague, making me question the need for them. E.g., what does SYNC, READ, and WRITE mean in this context? What happens when passing IGNORE? I'm a little afraid that this change adds fuzziness to the interface.

[cnuke]

Thanks for your review. I tried to keep the changes as non-invasive as possible which arguebly lead to the commits being more raw than neccessary.

Regarding the "gpu_session: use Buffer_id to identify buffers" commit, have you considered the use of Id_space::Id instead of defining a custom Buffer_id struct in gpu_session.h? […]

Yes, I am pretty much in line with your reasoning and thought about that as well. I will adapt the commit accordingly. Eventually the Buffer and in return the Id_space could be part of the Gpu::Connection.

Now that you are touching the Gpu::Execution_buffer_sequence in "gpu_session: move exec seqno to Gpu namespace", how do you think about renaming the type to Gpu::Sequence_number? This would improve the intuitive meaning of the type and make it shorter.

Fine with me.

The purpose of the Mapping_type should better be clarified in gpu_session.h. To me, the meaning of the four values is vague, making me question the need for them. E.g., what does SYNC, READ, and WRITE mean in this context? What happens when passing IGNORE? I'm a little afraid that this change adds fuzziness to the interface.

That is true and was somewhat intentional. I like the result of our (offline) discussion where the fuzzy interface is replace by a structured Attributes object.

[cnuke]

• ca8b97f436 - gpu: use Buffer_id to identify buffers
• a8e4edc10e - gpu: move exec sequence number to Gpu namespace
• 272555132d - gpu: introduce information dataspace
• 1fc28f7a07 - gpu: add 'complete' RPC call
• 4cd4c44a3b - gpu: introduce mapping attributes

contain the suggest changes (I tried not to introduce any new guiding-style violation but refrained from change olds ones at this point). For the time being I kept the Gpu::Buffer implementation as well as the Id_space local to the client.

[nfeske]

Thank you @cnuke for the quick and thorough rework!

The commit "gpu: use Buffer_id to identify buffers" looks excellent to me now.

Down the road, we may consider using an Id_space (instead of the registry) in the Intel GPU multiplexer as well by supplying the client-provided ID to the Id_space::Element constructor (relieving _apply_buffer from iterating through all buffers). But that's for another day.

Thanks for applying the suggestion for the naming of Sequence_number in commit "gpu: move exec sequence number to Gpu namespace" . That's much easier on the eyes.

With the added Mapping_attributes type, the commit "gpu: introduce mapping attributes" makes perfect sense now. Thank you for applying the pattern that is already established (in the VFS).

I merged the new commit series to staging.

I tried not to introduce any new guiding-style violation but refrained from change olds ones at this point

That's sensible. I'd appreciate a separate style-update commit (w/o semantic changes) - maybe in tandem with a commit that imports the Genode namespace into the Gpu namespace, alleviating most Genode:: prefixes.