On Thu, Jan 23, 2014 at 01:56:51PM +0100, Laurent Pinchart wrote:
<para> Drivers must first validate the requested frame buffer parameters passed@@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis> <function>drm_framebuffer_unregister_private</function>. </sect2> <sect2>
<title>Dumb GEM Objects</title><para>- The KMS API doesn't standardize backing storage object creation and
Strictly speaking isn't it the DRM API that's responsible for memory management, not the KMS API ?
The driver's private api is responsible for memory management, but the crucial thing here is that the KMS ioctls don't mandate anything specific (beyong that it needs to use uint32_t for handles).
Sure, but my point was that I believe memory management is the responsibility of DRM, not KMS. It of course needs to be driver-specific.
Well imo the dumb ioctls are part of kms. DRM itself doesn't have any memory management interfaces (at least if you ignore all the horror stories around legacy ums/dri1 drivers). My reasons are: - If you implement a kms driver you really should implement the dumb interfaces. Even when you have almost no memory management like the simpledrm driver. - If you have a driver with memory management and command submission but no KMS, there's no reason at all to implement the dumb interfaces. - ARM people abused dumb buffers for accelaration "because it worked", so imo moving it's documentation as far away as possible from the memory management section is a feature.
So the dumb stuff is a KMS interface to abstract away the lowest common denominator between all kms drivers. Actually memory manament needs a real interface, and is obviously separate.
- leaves it to driver-specific ioctls. Furthermore actually creating a
- buffer object even for GEM-based drivers is done through a
- driver-specific ioctl - GEM only has a common userspace interface for
- sharing and destroying objects. While not an issue for full-fledged
- graphics stacks that include device-specific userspace components (in
- libdrm for instance), this limit makes DRM-based early boot graphics
- unnecessarily complex.
</para>This feels a bit out of place, can't we leave the section where it was ?
Imo the justification for why we have the dumb ioctls should be here. And I wanted to mention both that KMS doesn't mandate a particular bo interface like GEM and that on top GEM wouldn't even provide a common allocation function anyway.
I agree that a justification here is a good idea, but I would just add one paragraph that references the dumb GEM objects section instead of scattering GEM documentation around.
I've pretty much removed all mention of dumb gem objects ;-) There's one mention of dumb_create left in the GEM/memory management section, but that one is just an example for the lifetime and reference counting rules. So not relevant.
But besides that I think there's some room for improvement in the GEM section to clarify what is the actual core interfaces, what is more helper library in nature and what in GEM is more just a common design pattern for driver ioctls but not specified in a mandatory way anywhere. E.g. atm all drivers which implement a GEM interface (radeon, i915, ...) have a mostly implicitly synchronizing buffer access interface, but there's nothing in GEM mandating this. Or the usual confusing between TTM directly exposed to userspace and TTM hidden behind a GEM-based ioctl interface.
I agree, the GEM section should be improved, and TTM documentation would be nice as well. I'm lacking time though (as well as knowledge about TTM).
I have a few ideas, but I think I'll postpone this until I get around to documenting the i915 GEM code a bit. Having a concrete driver to talk about should help greatly to separate common concepts from artifacts of the i915 implementation. I guess that review will also lead to some abi cleanups to remove i915-ism from core gem. -Daniel