| Message ID | 1390467164-951-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > - This is _not_ a generic interface to create gem objects, but just an > interface to make early boot services (like boot splash) with a > generic KMS userspace driver possible. Hence it's better to move > the documentation for this from the GEM section to the KMS section, > next to the creation of framebuffer objects. > > - Make it really clear that the returned handle isn't necessarily a > GEM object (it can also be e.g. a TTM handle when running on top of > vmwgfx). > > - Add a paragraph to make it clear that this is just for unaccelarated > userspace - gpu drivers need to have their own buffer object > creation ioctl which is hardware specific. > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++++------------------- > 1 file changed, 68 insertions(+), 57 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index ed1d6d289022..9c3fdd59c995 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -830,62 +830,6 @@ char *date;</synopsis> > </para> > </sect3> > <sect3> > - <title>Dumb GEM Objects</title> > - <para> > - The GEM API doesn't standardize GEM objects creation and leaves it to > - driver-specific ioctls. 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> > - <para> > - Dumb GEM objects partly alleviate the problem by providing a standard > - API to create dumb buffers suitable for scanout, which can then be used > - to create KMS frame buffers. > - </para> > - <para> > - To support dumb GEM objects drivers must implement the > - <methodname>dumb_create</methodname>, > - <methodname>dumb_destroy</methodname> and > - <methodname>dumb_map_offset</methodname> operations. > - </para> > - <itemizedlist> > - <listitem> > - <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev, > - struct drm_mode_create_dumb *args);</synopsis> > - <para> > - The <methodname>dumb_create</methodname> operation creates a GEM > - object suitable for scanout based on the width, height and depth > - from the struct <structname>drm_mode_create_dumb</structname> > - argument. It fills the argument's <structfield>handle</structfield>, > - <structfield>pitch</structfield> and <structfield>size</structfield> > - fields with a handle for the newly created GEM object and its line > - pitch and size in bytes. > - </para> > - </listitem> > - <listitem> > - <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev, > - uint32_t handle);</synopsis> > - <para> > - The <methodname>dumb_destroy</methodname> operation destroys a dumb > - GEM object created by <methodname>dumb_create</methodname>. > - </para> > - </listitem> > - <listitem> > - <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev, > - uint32_t handle, uint64_t *offset);</synopsis> > - <para> > - The <methodname>dumb_map_offset</methodname> operation associates an > - mmap fake offset with the GEM object given by the handle and returns > - it. Drivers must use the > - <function>drm_gem_create_mmap_offset</function> function to > - associate the fake offset as described in > - <xref linkend="drm-gem-objects-mapping"/>. > - </para> > - </listitem> > - </itemizedlist> > - </sect3> > - <sect3> > <title>Memory Coherency</title> > <para> > When mapped to the device or used in a command buffer, backing pages > @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> > handle (or a list of memory handles for multi-planar formats) through > the <parameter>drm_mode_fb_cmd2</parameter> argument. This document > assumes that the driver uses GEM, those handles thus reference GEM > - objects. > + objects. But drivers are free to use their own backing storage object > + handles, e.g. vmwgfx directly exposes special TTM handles to userspace > + and so expects TTM handles in the create ioctl and not GEM objects. Maybe remove the sentence saying "this document assumes that the driver uses GEM". I don't see where we explicitly do that. Otherwise the patch looks fine. Thanks David > </para> > <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 > + 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> > + <para> > + Dumb objects partly alleviate the problem by providing a standard > + API to create dumb buffers suitable for scanout, which can then be used > + to create KMS frame buffers. > + </para> > + <para> > + To support dumb objects drivers must implement the > + <methodname>dumb_create</methodname>, > + <methodname>dumb_destroy</methodname> and > + <methodname>dumb_map_offset</methodname> operations. > + </para> > + <itemizedlist> > + <listitem> > + <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev, > + struct drm_mode_create_dumb *args);</synopsis> > + <para> > + The <methodname>dumb_create</methodname> operation creates a driver > + object (GEM or TTM handle) object suitable for scanout based on the > + width, height and depth from the struct > + <structname>drm_mode_create_dumb</structname> argument. It fills the > + argument's <structfield>handle</structfield>, > + <structfield>pitch</structfield> and <structfield>size</structfield> > + fields with a handle for the newly created object and its line > + pitch and size in bytes. > + </para> > + </listitem> > + <listitem> > + <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev, > + uint32_t handle);</synopsis> > + <para> > + The <methodname>dumb_destroy</methodname> operation destroys a dumb > + object created by <methodname>dumb_create</methodname>. > + </para> > + </listitem> > + <listitem> > + <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev, > + uint32_t handle, uint64_t *offset);</synopsis> > + <para> > + The <methodname>dumb_map_offset</methodname> operation associates an > + mmap fake offset with the object given by the handle and returns > + it. Drivers must use the > + <function>drm_gem_create_mmap_offset</function> function to > + associate the fake offset as described in > + <xref linkend="drm-gem-objects-mapping"/>. > + </para> > + </listitem> > + </itemizedlist> > + <para> > + Note that dumb objects may not be used for gpu accelaration, as has been > + attempted on some ARM embedded platforms. Such drivers really must have > + a hardware-specific ioctl to allocate suitable objects. > + </para> > + </sect2> > + <sect2> > <title>Output Polling</title> > <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis> > <para> > -- > 1.8.5.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, Thank you for the patch. On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote: > - This is _not_ a generic interface to create gem objects, but just an > interface to make early boot services (like boot splash) with a > generic KMS userspace driver possible. Hence it's better to move > the documentation for this from the GEM section to the KMS section, > next to the creation of framebuffer objects. > > - Make it really clear that the returned handle isn't necessarily a > GEM object (it can also be e.g. a TTM handle when running on top of > vmwgfx). > > - Add a paragraph to make it clear that this is just for unaccelarated > userspace - gpu drivers need to have their own buffer object > creation ioctl which is hardware specific. > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++------------------ > 1 file changed, 68 insertions(+), 57 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index ed1d6d289022..9c3fdd59c995 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -830,62 +830,6 @@ char *date;</synopsis> > </para> > </sect3> > <sect3> > - <title>Dumb GEM Objects</title> > - <para> > - The GEM API doesn't standardize GEM objects creation and leaves > it to - driver-specific ioctls. 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> > - <para> > - Dumb GEM objects partly alleviate the problem by providing a > standard - API to create dumb buffers suitable for scanout, which > can then be used - to create KMS frame buffers. > - </para> > - <para> > - To support dumb GEM objects drivers must implement the > - <methodname>dumb_create</methodname>, > - <methodname>dumb_destroy</methodname> and > - <methodname>dumb_map_offset</methodname> operations. > - </para> > - <itemizedlist> > - <listitem> > - <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct > drm_device *dev, - struct drm_mode_create_dumb > *args);</synopsis> - <para> > - The <methodname>dumb_create</methodname> operation creates a > GEM - object suitable for scanout based on the width, height > and depth - from the struct > <structname>drm_mode_create_dumb</structname> - argument. It > fills the argument's <structfield>handle</structfield>, - > <structfield>pitch</structfield> and <structfield>size</structfield> - > fields with a handle for the newly created GEM object and its line > - pitch and size in bytes. > - </para> > - </listitem> > - <listitem> > - <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, > struct drm_device *dev, - uint32_t handle);</synopsis> > - <para> > - The <methodname>dumb_destroy</methodname> operation destroys > a dumb - GEM object created by > <methodname>dumb_create</methodname>. - </para> > - </listitem> > - <listitem> > - <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, > struct drm_device *dev, - uint32_t handle, uint64_t > *offset);</synopsis> - <para> > - The <methodname>dumb_map_offset</methodname> operation > associates an - mmap fake offset with the GEM object given by > the handle and returns - it. Drivers must use the > - <function>drm_gem_create_mmap_offset</function> function to > - associate the fake offset as described in > - <xref linkend="drm-gem-objects-mapping"/>. > - </para> > - </listitem> > - </itemizedlist> > - </sect3> > - <sect3> > <title>Memory Coherency</title> > <para> > When mapped to the device or used in a command buffer, backing > pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> > handle (or a list of memory handles for multi-planar formats) > through the <parameter>drm_mode_fb_cmd2</parameter> argument. This document > assumes that the driver uses GEM, those handles thus reference GEM - > objects. > + objects. But drivers are free to use their own backing storage > object > + handles, e.g. vmwgfx directly exposes special TTM handles to userspace > + and so expects TTM handles in the create ioctl and not GEM objects. Nitpicking, I would say "Drivers are however free to use their own backing storage object handles, e.g. vmwgfx directly exposes special TTM handles to userspace and so expects TTM handles in the create ioctl, not GEM objects." > <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 ? > + 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 ? > + <para> > + Dumb objects partly alleviate the problem by providing a standard > + API to create dumb buffers suitable for scanout, which can then be > used + to create KMS frame buffers. > + </para> > + <para> > + To support dumb objects drivers must implement the > + <methodname>dumb_create</methodname>, > + <methodname>dumb_destroy</methodname> and > + <methodname>dumb_map_offset</methodname> operations. > + </para> > + <itemizedlist> > + <listitem> > + <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct > drm_device *dev, + struct drm_mode_create_dumb > *args);</synopsis> + <para> > + The <methodname>dumb_create</methodname> operation creates a > driver + object (GEM or TTM handle) object suitable for scanout based > on the + width, height and depth from the struct > + <structname>drm_mode_create_dumb</structname> argument. It fills the > + argument's <structfield>handle</structfield>, > + <structfield>pitch</structfield> and <structfield>size</structfield> > + fields with a handle for the newly created object and its line > + pitch and size in bytes. > + </para> > + </listitem> > + <listitem> > + <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct > drm_device *dev, + uint32_t handle);</synopsis> > + <para> > + The <methodname>dumb_destroy</methodname> operation destroys a > dumb + object created by <methodname>dumb_create</methodname>. + > </para> > + </listitem> > + <listitem> > + <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, > struct drm_device *dev, + uint32_t handle, uint64_t > *offset);</synopsis> + <para> > + The <methodname>dumb_map_offset</methodname> operation > associates an + mmap fake offset with the object given by the > handle and returns + it. Drivers must use the > + <function>drm_gem_create_mmap_offset</function> function to > + associate the fake offset as described in > + <xref linkend="drm-gem-objects-mapping"/>. > + </para> > + </listitem> > + </itemizedlist> > + <para> > + Note that dumb objects may not be used for gpu accelaration, as has > been + attempted on some ARM embedded platforms. Such drivers really must > have + a hardware-specific ioctl to allocate suitable objects. > + </para> > + </sect2> > + <sect2> > <title>Output Polling</title> > <synopsis>void (*output_poll_changed)(struct drm_device > *dev);</synopsis> <para>
Hi David, On Thursday 23 January 2014 10:14:35 David Herrmann wrote: > On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter wrote: > > - This is _not_ a generic interface to create gem objects, but just an > > interface to make early boot services (like boot splash) with a > > generic KMS userspace driver possible. Hence it's better to move > > the documentation for this from the GEM section to the KMS section, > > next to the creation of framebuffer objects. > > > > - Make it really clear that the returned handle isn't necessarily a > > GEM object (it can also be e.g. a TTM handle when running on top of > > vmwgfx). > > > > - Add a paragraph to make it clear that this is just for unaccelarated > > userspace - gpu drivers need to have their own buffer object > > creation ioctl which is hardware specific. > > > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > > > Documentation/DocBook/drm.tmpl | 125 +++++++++++++++++++----------------- > > 1 file changed, 68 insertions(+), 57 deletions(-) > > > > diff --git a/Documentation/DocBook/drm.tmpl > > b/Documentation/DocBook/drm.tmpl index ed1d6d289022..9c3fdd59c995 100644 > > --- a/Documentation/DocBook/drm.tmpl > > +++ b/Documentation/DocBook/drm.tmpl [snip] > > @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> > > > > handle (or a list of memory handles for multi-planar formats) > > through the <parameter>drm_mode_fb_cmd2</parameter> argument. > > This document assumes that the driver uses GEM, those handles > > thus reference GEM > > - objects. > > + objects. But drivers are free to use their own backing storage > > object > > + handles, e.g. vmwgfx directly exposes special TTM handles to > > userspace > > + and so expects TTM handles in the create ioctl and not GEM > > objects. > > Maybe remove the sentence saying "this document assumes that the > driver uses GEM". I don't see where we explicitly do that. We do in one place, in the documentation of the create_handle operation. We could apply the following change and replace the above sentence with "For drivers using GEM those handles reference GEM objects. Drivers are however free to use their own backing storage object handles, e.g. vmwgfx directly exposes special TTM handles to userspace and so expects TTM handles in the create ioctl, not GEM object handles." @@ -1010,8 +1010,8 @@ int max_width, max_height;</synopsis> reference the memory object associated with the first plane. </para> <para> - Drivers call <function>drm_gem_handle_create</function> to create - the handle. + GEM-aware drivers call <function>drm_gem_handle_create</function> to + create the handle. </para> </listitem> <listitem> > Otherwise the patch looks fine.
On Thu, Jan 23, 2014 at 12:21:42PM +0100, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote: > > - This is _not_ a generic interface to create gem objects, but just an > > interface to make early boot services (like boot splash) with a > > generic KMS userspace driver possible. Hence it's better to move > > the documentation for this from the GEM section to the KMS section, > > next to the creation of framebuffer objects. > > > > - Make it really clear that the returned handle isn't necessarily a > > GEM object (it can also be e.g. a TTM handle when running on top of > > vmwgfx). > > > > - Add a paragraph to make it clear that this is just for unaccelarated > > userspace - gpu drivers need to have their own buffer object > > creation ioctl which is hardware specific. > > > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++------------------ > > 1 file changed, 68 insertions(+), 57 deletions(-) > > > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > > index ed1d6d289022..9c3fdd59c995 100644 > > --- a/Documentation/DocBook/drm.tmpl > > +++ b/Documentation/DocBook/drm.tmpl > > @@ -830,62 +830,6 @@ char *date;</synopsis> > > </para> > > </sect3> > > <sect3> > > - <title>Dumb GEM Objects</title> > > - <para> > > - The GEM API doesn't standardize GEM objects creation and leaves > > it to - driver-specific ioctls. 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> > > - <para> > > - Dumb GEM objects partly alleviate the problem by providing a > > standard - API to create dumb buffers suitable for scanout, which > > can then be used - to create KMS frame buffers. > > - </para> > > - <para> > > - To support dumb GEM objects drivers must implement the > > - <methodname>dumb_create</methodname>, > > - <methodname>dumb_destroy</methodname> and > > - <methodname>dumb_map_offset</methodname> operations. > > - </para> > > - <itemizedlist> > > - <listitem> > > - <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct > > drm_device *dev, - struct drm_mode_create_dumb > > *args);</synopsis> - <para> > > - The <methodname>dumb_create</methodname> operation creates a > > GEM - object suitable for scanout based on the width, height > > and depth - from the struct > > <structname>drm_mode_create_dumb</structname> - argument. It > > fills the argument's <structfield>handle</structfield>, - > > <structfield>pitch</structfield> and <structfield>size</structfield> - > > fields with a handle for the newly created GEM object and its line > > - pitch and size in bytes. > > - </para> > > - </listitem> > > - <listitem> > > - <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, > > struct drm_device *dev, - uint32_t handle);</synopsis> > > - <para> > > - The <methodname>dumb_destroy</methodname> operation destroys > > a dumb - GEM object created by > > <methodname>dumb_create</methodname>. - </para> > > - </listitem> > > - <listitem> > > - <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, > > struct drm_device *dev, - uint32_t handle, uint64_t > > *offset);</synopsis> - <para> > > - The <methodname>dumb_map_offset</methodname> operation > > associates an - mmap fake offset with the GEM object given by > > the handle and returns - it. Drivers must use the > > - <function>drm_gem_create_mmap_offset</function> function to > > - associate the fake offset as described in > > - <xref linkend="drm-gem-objects-mapping"/>. > > - </para> > > - </listitem> > > - </itemizedlist> > > - </sect3> > > - <sect3> > > <title>Memory Coherency</title> > > <para> > > When mapped to the device or used in a command buffer, backing > > pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> > > handle (or a list of memory handles for multi-planar formats) > > through the <parameter>drm_mode_fb_cmd2</parameter> argument. This document > > assumes that the driver uses GEM, those handles thus reference GEM - > > objects. > > + objects. But drivers are free to use their own backing storage > > object > > + handles, e.g. vmwgfx directly exposes special TTM handles to userspace > > + and so expects TTM handles in the create ioctl and not GEM objects. > > Nitpicking, I would say > > "Drivers are however free to use their own backing storage object handles, > e.g. vmwgfx directly exposes special TTM handles to userspace and so expects > TTM handles in the create ioctl, not GEM objects." I've already adjusted this a bit but haven't yet sent out the new version of the patch. Slightly different wording now. > > <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). > > + 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. 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. -Daniel > > > + <para> > > + Dumb objects partly alleviate the problem by providing a standard > > + API to create dumb buffers suitable for scanout, which can then be > > used + to create KMS frame buffers. > > + </para> > > + <para> > > + To support dumb objects drivers must implement the > > + <methodname>dumb_create</methodname>, > > + <methodname>dumb_destroy</methodname> and > > + <methodname>dumb_map_offset</methodname> operations. > > + </para> > > + <itemizedlist> > > + <listitem> > > + <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct > > drm_device *dev, + struct drm_mode_create_dumb > > *args);</synopsis> + <para> > > + The <methodname>dumb_create</methodname> operation creates a > > driver + object (GEM or TTM handle) object suitable for scanout based > > on the + width, height and depth from the struct > > + <structname>drm_mode_create_dumb</structname> argument. It fills the > > + argument's <structfield>handle</structfield>, > > + <structfield>pitch</structfield> and <structfield>size</structfield> > > + fields with a handle for the newly created object and its line > > + pitch and size in bytes. > > + </para> > > + </listitem> > > + <listitem> > > + <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct > > drm_device *dev, + uint32_t handle);</synopsis> > > + <para> > > + The <methodname>dumb_destroy</methodname> operation destroys a > > dumb + object created by <methodname>dumb_create</methodname>. + > > </para> > > + </listitem> > > + <listitem> > > + <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, > > struct drm_device *dev, + uint32_t handle, uint64_t > > *offset);</synopsis> + <para> > > + The <methodname>dumb_map_offset</methodname> operation > > associates an + mmap fake offset with the object given by the > > handle and returns + it. Drivers must use the > > + <function>drm_gem_create_mmap_offset</function> function to > > + associate the fake offset as described in > > + <xref linkend="drm-gem-objects-mapping"/>. > > + </para> > > + </listitem> > > + </itemizedlist> > > + <para> > > + Note that dumb objects may not be used for gpu accelaration, as has > > been + attempted on some ARM embedded platforms. Such drivers really must > > have + a hardware-specific ioctl to allocate suitable objects. > > + </para> > > + </sect2> > > + <sect2> > > <title>Output Polling</title> > > <synopsis>void (*output_poll_changed)(struct drm_device > > *dev);</synopsis> <para> > > -- > Regards, > > Laurent Pinchart >
Hi Daniel, On Thursday 23 January 2014 13:47:31 Daniel Vetter wrote: > On Thu, Jan 23, 2014 at 12:21:42PM +0100, Laurent Pinchart wrote: > > On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote: > > > - This is _not_ a generic interface to create gem objects, but just an > > > interface to make early boot services (like boot splash) with a > > > generic KMS userspace driver possible. Hence it's better to move > > > the documentation for this from the GEM section to the KMS section, > > > next to the creation of framebuffer objects. > > > > > > - Make it really clear that the returned handle isn't necessarily a > > > GEM object (it can also be e.g. a TTM handle when running on top of > > > vmwgfx). > > > > > > - Add a paragraph to make it clear that this is just for unaccelarated > > > userspace - gpu drivers need to have their own buffer object > > > creation ioctl which is hardware specific. > > > > > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > > > > Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++---------------- > > > 1 file changed, 68 insertions(+), 57 deletions(-) > > > > > > diff --git a/Documentation/DocBook/drm.tmpl > > > b/Documentation/DocBook/drm.tmpl index ed1d6d289022..9c3fdd59c995 > > > 100644 > > > --- a/Documentation/DocBook/drm.tmpl > > > +++ b/Documentation/DocBook/drm.tmpl [snip] > > > pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> > > > handle (or a list of memory handles for multi-planar formats) > > > through the <parameter>drm_mode_fb_cmd2</parameter> argument. This > > > document assumes that the driver uses GEM, those handles thus reference > > > GEM > > > - objects. > > > + objects. But drivers are free to use their own backing storage > > > object > > > + handles, e.g. vmwgfx directly exposes special TTM handles to > > > userspace > > > + and so expects TTM handles in the create ioctl and not GEM objects. > > > > Nitpicking, I would say > > > > "Drivers are however free to use their own backing storage object handles, > > e.g. vmwgfx directly exposes special TTM handles to userspace and so > > expects TTM handles in the create ioctl, not GEM objects." > > I've already adjusted this a bit but haven't yet sent out the new version > of the patch. Slightly different wording now. OK. Please also see my reply to David. > > > <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. > > > + 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. > 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).
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
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index ed1d6d289022..9c3fdd59c995 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -830,62 +830,6 @@ char *date;</synopsis> </para> </sect3> <sect3> - <title>Dumb GEM Objects</title> - <para> - The GEM API doesn't standardize GEM objects creation and leaves it to - driver-specific ioctls. 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> - <para> - Dumb GEM objects partly alleviate the problem by providing a standard - API to create dumb buffers suitable for scanout, which can then be used - to create KMS frame buffers. - </para> - <para> - To support dumb GEM objects drivers must implement the - <methodname>dumb_create</methodname>, - <methodname>dumb_destroy</methodname> and - <methodname>dumb_map_offset</methodname> operations. - </para> - <itemizedlist> - <listitem> - <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev, - struct drm_mode_create_dumb *args);</synopsis> - <para> - The <methodname>dumb_create</methodname> operation creates a GEM - object suitable for scanout based on the width, height and depth - from the struct <structname>drm_mode_create_dumb</structname> - argument. It fills the argument's <structfield>handle</structfield>, - <structfield>pitch</structfield> and <structfield>size</structfield> - fields with a handle for the newly created GEM object and its line - pitch and size in bytes. - </para> - </listitem> - <listitem> - <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev, - uint32_t handle);</synopsis> - <para> - The <methodname>dumb_destroy</methodname> operation destroys a dumb - GEM object created by <methodname>dumb_create</methodname>. - </para> - </listitem> - <listitem> - <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev, - uint32_t handle, uint64_t *offset);</synopsis> - <para> - The <methodname>dumb_map_offset</methodname> operation associates an - mmap fake offset with the GEM object given by the handle and returns - it. Drivers must use the - <function>drm_gem_create_mmap_offset</function> function to - associate the fake offset as described in - <xref linkend="drm-gem-objects-mapping"/>. - </para> - </listitem> - </itemizedlist> - </sect3> - <sect3> <title>Memory Coherency</title> <para> When mapped to the device or used in a command buffer, backing pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis> handle (or a list of memory handles for multi-planar formats) through the <parameter>drm_mode_fb_cmd2</parameter> argument. This document assumes that the driver uses GEM, those handles thus reference GEM - objects. + objects. But drivers are free to use their own backing storage object + handles, e.g. vmwgfx directly exposes special TTM handles to userspace + and so expects TTM handles in the create ioctl and not GEM objects. </para> <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 + 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> + <para> + Dumb objects partly alleviate the problem by providing a standard + API to create dumb buffers suitable for scanout, which can then be used + to create KMS frame buffers. + </para> + <para> + To support dumb objects drivers must implement the + <methodname>dumb_create</methodname>, + <methodname>dumb_destroy</methodname> and + <methodname>dumb_map_offset</methodname> operations. + </para> + <itemizedlist> + <listitem> + <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev, + struct drm_mode_create_dumb *args);</synopsis> + <para> + The <methodname>dumb_create</methodname> operation creates a driver + object (GEM or TTM handle) object suitable for scanout based on the + width, height and depth from the struct + <structname>drm_mode_create_dumb</structname> argument. It fills the + argument's <structfield>handle</structfield>, + <structfield>pitch</structfield> and <structfield>size</structfield> + fields with a handle for the newly created object and its line + pitch and size in bytes. + </para> + </listitem> + <listitem> + <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev, + uint32_t handle);</synopsis> + <para> + The <methodname>dumb_destroy</methodname> operation destroys a dumb + object created by <methodname>dumb_create</methodname>. + </para> + </listitem> + <listitem> + <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev, + uint32_t handle, uint64_t *offset);</synopsis> + <para> + The <methodname>dumb_map_offset</methodname> operation associates an + mmap fake offset with the object given by the handle and returns + it. Drivers must use the + <function>drm_gem_create_mmap_offset</function> function to + associate the fake offset as described in + <xref linkend="drm-gem-objects-mapping"/>. + </para> + </listitem> + </itemizedlist> + <para> + Note that dumb objects may not be used for gpu accelaration, as has been + attempted on some ARM embedded platforms. Such drivers really must have + a hardware-specific ioctl to allocate suitable objects. + </para> + </sect2> + <sect2> <title>Output Polling</title> <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis> <para>
- This is _not_ a generic interface to create gem objects, but just an interface to make early boot services (like boot splash) with a generic KMS userspace driver possible. Hence it's better to move the documentation for this from the GEM section to the KMS section, next to the creation of framebuffer objects. - Make it really clear that the returned handle isn't necessarily a GEM object (it can also be e.g. a TTM handle when running on top of vmwgfx). - Add a paragraph to make it clear that this is just for unaccelarated userspace - gpu drivers need to have their own buffer object creation ioctl which is hardware specific. Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 57 deletions(-)