diff mbox series

[1/3] drm: add gem ttm helpers

Message ID 20190806133454.8254-2-kraxel@redhat.com (mailing list archive)
State New, archived
Headers show
Series drm: add gem ttm helpers | expand

Commit Message

Gerd Hoffmann Aug. 6, 2019, 1:34 p.m. UTC 
Now with ttm_buffer_object being a subclass of drm_gem_object we can
easily lookup ttm_buffer_object for a given drm_gem_object, which in
turm allows to create common helper functions.  This patch starts off
with dump mmap helpers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_gem_ttm_helper.h     | 27 +++++++++++++++
 drivers/gpu/drm/drm_gem_ttm_helper.c | 52 ++++++++++++++++++++++++++++
 drivers/gpu/drm/Kconfig              |  7 ++++
 drivers/gpu/drm/Makefile             |  3 ++
 4 files changed, 89 insertions(+)
 create mode 100644 include/drm/drm_gem_ttm_helper.h
 create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c

Comments

Daniel Vetter Aug. 6, 2019, 1:54 p.m. UTC | #1 
On Tue, Aug 06, 2019 at 03:34:52PM +0200, Gerd Hoffmann wrote:
> Now with ttm_buffer_object being a subclass of drm_gem_object we can
> easily lookup ttm_buffer_object for a given drm_gem_object, which in
> turm allows to create common helper functions.  This patch starts off
> with dump mmap helpers.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_ttm_helper.h     | 27 +++++++++++++++
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 52 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/Kconfig              |  7 ++++
>  drivers/gpu/drm/Makefile             |  3 ++
>  4 files changed, 89 insertions(+)
>  create mode 100644 include/drm/drm_gem_ttm_helper.h
>  create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c
> 
> diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
> new file mode 100644
> index 000000000000..2c6874190b17
> --- /dev/null
> +++ b/include/drm/drm_gem_ttm_helper.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef DRM_GEM_TTM_HELPER_H
> +#define DRM_GEM_TTM_HELPER_H
> +
> +#include <drm/drm_gem.h>
> +#include <drm/ttm/ttm_bo_api.h>
> +#include <linux/kernel.h> /* for container_of() */
> +
> +/**
> + * Returns the container of type &struct ttm_buffer_object
> + * for field base.
> + * @gem:	the GEM object
> + * Returns:	The containing GEM VRAM object
> + */
> +static inline struct ttm_buffer_object *drm_gem_ttm_of_gem(
> +	struct drm_gem_object *gem)
> +{
> +	return container_of(gem, struct ttm_buffer_object, base);
> +}
> +
> +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo);
> +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> +					struct drm_device *dev,
> +					uint32_t handle, uint64_t *offset);
> +
> +#endif
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> new file mode 100644
> index 000000000000..b069bd0c4c94
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <drm/drm_gem_ttm_helper.h>
> +
> +/**
> + * drm_gem_ttm_mmap_offset() - Returns a GEM ttm object's mmap offset
> + * @gbo:	the GEM ttm object
> + *
> + * See drm_vma_node_offset_addr() for more information.
> + *
> + * Returns:
> + * The buffer object's offset for userspace mappings on success, or
> + * 0 if no offset is allocated.
> + */
> +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo)
> +{
> +	return drm_vma_node_offset_addr(&bo->base.vma_node);

Why do we need a new one here, can't we use the existing gem
implementation for this (there really should only be one I hope, but I
didn't check).

> +}
> +EXPORT_SYMBOL(drm_gem_ttm_mmap_offset);
> +
> +/**
> + * drm_gem_ttm_driver_dumb_mmap_offset() - \
> +	Implements &struct drm_driver.dumb_mmap_offset
> + * @file:	DRM file pointer.
> + * @dev:	DRM device.
> + * @handle:	GEM handle
> + * @offset:	Returns the mapping's memory offset on success
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative errno code otherwise.
> + */
> +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> +					 struct drm_device *dev,
> +					 uint32_t handle, uint64_t *offset)
> +{
> +	struct drm_gem_object *gem;
> +	struct ttm_buffer_object *bo;
> +
> +	gem = drm_gem_object_lookup(file, handle);
> +	if (!gem)
> +		return -ENOENT;
> +
> +	bo = drm_gem_ttm_of_gem(gem);
> +	*offset = drm_gem_ttm_mmap_offset(bo);
> +
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_ttm_driver_dumb_mmap_offset);

Same for this, you're just upcasting to ttm_bo and then downcasting to
gem_bo again ... I think just a series to roll out the existing gem
helpers everywhere should work?
-Daniel

> +
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e6f40fb54c9a..f7b25519f95c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -172,6 +172,13 @@ config DRM_VRAM_HELPER
>  	help
>  	  Helpers for VRAM memory management
>  
> +config DRM_TTM_HELPER
> +	tristate
> +	depends on DRM
> +	select DRM_TTM
> +	help
> +	  Helpers for ttm-based gem objects
> +
>  config DRM_GEM_CMA_HELPER
>  	bool
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 10f8329a8b71..545c61d6528b 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -37,6 +37,9 @@ drm_vram_helper-y := drm_gem_vram_helper.o \
>  		     drm_vram_mm_helper.o
>  obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>  
> +drm_ttm_helper-y := drm_gem_ttm_helper.o
> +obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
> +
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> -- 
> 2.18.1
>
Sam Ravnborg Aug. 6, 2019, 3:46 p.m. UTC | #2 
Hi Gerd.

On Tue, Aug 06, 2019 at 03:34:52PM +0200, Gerd Hoffmann wrote:
> Now with ttm_buffer_object being a subclass of drm_gem_object we can
> easily lookup ttm_buffer_object for a given drm_gem_object, which in
> turm allows to create common helper functions.  This patch starts off
> with dump mmap helpers.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

One nit below.

> ---
>  include/drm/drm_gem_ttm_helper.h     | 27 +++++++++++++++
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 52 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/Kconfig              |  7 ++++
>  drivers/gpu/drm/Makefile             |  3 ++
>  4 files changed, 89 insertions(+)
>  create mode 100644 include/drm/drm_gem_ttm_helper.h
>  create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c
> 
> diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
> new file mode 100644
> index 000000000000..2c6874190b17
> --- /dev/null
> +++ b/include/drm/drm_gem_ttm_helper.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef DRM_GEM_TTM_HELPER_H
> +#define DRM_GEM_TTM_HELPER_H
> +
> +#include <drm/drm_gem.h>
> +#include <drm/ttm/ttm_bo_api.h>
> +#include <linux/kernel.h> /* for container_of() */

The typical order of include files is:

#include <linux/*>

#include <drm/*>

So space between each block of includes and sort within each block.

The comment "/* for container_of() */" is not really useful for
anyone.

	Sam
Sam Ravnborg Aug. 6, 2019, 3:51 p.m. UTC | #3 
Hi Gerd.

On Tue, Aug 06, 2019 at 03:34:52PM +0200, Gerd Hoffmann wrote:
> Now with ttm_buffer_object being a subclass of drm_gem_object we can
> easily lookup ttm_buffer_object for a given drm_gem_object, which in
> turm allows to create common helper functions.  This patch starts off
> with dump mmap helpers.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_gem_ttm_helper.h     | 27 +++++++++++++++
>  drivers/gpu/drm/drm_gem_ttm_helper.c | 52 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/Kconfig              |  7 ++++
>  drivers/gpu/drm/Makefile             |  3 ++
>  4 files changed, 89 insertions(+)
>  create mode 100644 include/drm/drm_gem_ttm_helper.h
>  create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c

You have made nice kernel-doc comments for the struct and the public
functions.
Could we plug this into the existing doc too so one can browse around.
This will also allow you to check for any syntax errors using 
"make htmldocs"
(I spotted  one gbo versus bo mismatch)

A small DOC section in the top of drm_gem_ttm_helper.c
would also be nice. Just a few lines that introduce the purpose of this
file would cut it.

	Sam


> diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
> new file mode 100644
> index 000000000000..2c6874190b17
> --- /dev/null
> +++ b/include/drm/drm_gem_ttm_helper.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef DRM_GEM_TTM_HELPER_H
> +#define DRM_GEM_TTM_HELPER_H
> +
> +#include <drm/drm_gem.h>
> +#include <drm/ttm/ttm_bo_api.h>
> +#include <linux/kernel.h> /* for container_of() */
> +
> +/**
> + * Returns the container of type &struct ttm_buffer_object
> + * for field base.
> + * @gem:	the GEM object
> + * Returns:	The containing GEM VRAM object
> + */
> +static inline struct ttm_buffer_object *drm_gem_ttm_of_gem(
> +	struct drm_gem_object *gem)
> +{
> +	return container_of(gem, struct ttm_buffer_object, base);
> +}
> +
> +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo);
> +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> +					struct drm_device *dev,
> +					uint32_t handle, uint64_t *offset);
> +
> +#endif
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
> new file mode 100644
> index 000000000000..b069bd0c4c94
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <drm/drm_gem_ttm_helper.h>
> +
> +/**
> + * drm_gem_ttm_mmap_offset() - Returns a GEM ttm object's mmap offset
> + * @gbo:	the GEM ttm object
> + *
> + * See drm_vma_node_offset_addr() for more information.
> + *
> + * Returns:
> + * The buffer object's offset for userspace mappings on success, or
> + * 0 if no offset is allocated.
> + */
> +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo)
> +{
> +	return drm_vma_node_offset_addr(&bo->base.vma_node);
> +}
> +EXPORT_SYMBOL(drm_gem_ttm_mmap_offset);
> +
> +/**
> + * drm_gem_ttm_driver_dumb_mmap_offset() - \
> +	Implements &struct drm_driver.dumb_mmap_offset
> + * @file:	DRM file pointer.
> + * @dev:	DRM device.
> + * @handle:	GEM handle
> + * @offset:	Returns the mapping's memory offset on success
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative errno code otherwise.
> + */
> +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> +					 struct drm_device *dev,
> +					 uint32_t handle, uint64_t *offset)
> +{
> +	struct drm_gem_object *gem;
> +	struct ttm_buffer_object *bo;
> +
> +	gem = drm_gem_object_lookup(file, handle);
> +	if (!gem)
> +		return -ENOENT;
> +
> +	bo = drm_gem_ttm_of_gem(gem);
> +	*offset = drm_gem_ttm_mmap_offset(bo);
> +
> +	drm_gem_object_put_unlocked(gem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_ttm_driver_dumb_mmap_offset);
> +
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e6f40fb54c9a..f7b25519f95c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -172,6 +172,13 @@ config DRM_VRAM_HELPER
>  	help
>  	  Helpers for VRAM memory management
>  
> +config DRM_TTM_HELPER
> +	tristate
> +	depends on DRM
> +	select DRM_TTM
> +	help
> +	  Helpers for ttm-based gem objects
> +
>  config DRM_GEM_CMA_HELPER
>  	bool
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 10f8329a8b71..545c61d6528b 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -37,6 +37,9 @@ drm_vram_helper-y := drm_gem_vram_helper.o \
>  		     drm_vram_mm_helper.o
>  obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>  
> +drm_ttm_helper-y := drm_gem_ttm_helper.o
> +obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
> +
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> -- 
> 2.18.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann Aug. 7, 2019, 7:26 a.m. UTC | #4 
> > +/**
> > + * drm_gem_ttm_mmap_offset() - Returns a GEM ttm object's mmap offset
> > + * @gbo:	the GEM ttm object
> > + *
> > + * See drm_vma_node_offset_addr() for more information.
> > + *
> > + * Returns:
> > + * The buffer object's offset for userspace mappings on success, or
> > + * 0 if no offset is allocated.
> > + */
> > +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo)
> > +{
> > +	return drm_vma_node_offset_addr(&bo->base.vma_node);
> 
> Why do we need a new one here, can't we use the existing gem
> implementation for this (there really should only be one I hope, but I
> didn't check).

Havn't found one.

But maybe we don't need this as separate function and can simply move
the drm_vma_node_offset_addr() call into
drm_gem_ttm_driver_dumb_mmap_offset().

> > +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> > +					 struct drm_device *dev,
> > +					 uint32_t handle, uint64_t *offset)
> > +{
> > +	struct drm_gem_object *gem;
> > +	struct ttm_buffer_object *bo;
> > +
> > +	gem = drm_gem_object_lookup(file, handle);
> > +	if (!gem)
> > +		return -ENOENT;
> > +
> > +	bo = drm_gem_ttm_of_gem(gem);
> > +	*offset = drm_gem_ttm_mmap_offset(bo);
> > +
> > +	drm_gem_object_put_unlocked(gem);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_ttm_driver_dumb_mmap_offset);
> 
> Same for this, you're just upcasting to ttm_bo and then downcasting to
> gem_bo again ... I think just a series to roll out the existing gem
> helpers everywhere should work?

I don't think so.  drm_gem_dumb_map_offset() calls
drm_gem_create_mmap_offset(), which I think is not correct for ttm
objects because ttm_bo_init() handles vma_node initialization.

cheers,
  Gerd
Daniel Vetter Aug. 7, 2019, 8:28 a.m. UTC | #5 
On Wed, Aug 7, 2019 at 9:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > +/**
> > > + * drm_gem_ttm_mmap_offset() - Returns a GEM ttm object's mmap offset
> > > + * @gbo:   the GEM ttm object
> > > + *
> > > + * See drm_vma_node_offset_addr() for more information.
> > > + *
> > > + * Returns:
> > > + * The buffer object's offset for userspace mappings on success, or
> > > + * 0 if no offset is allocated.
> > > + */
> > > +u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo)
> > > +{
> > > +   return drm_vma_node_offset_addr(&bo->base.vma_node);
> >
> > Why do we need a new one here, can't we use the existing gem
> > implementation for this (there really should only be one I hope, but I
> > didn't check).
>
> Havn't found one.

It got reverted out again:

commit 415d2e9e07574d3de63b8df77dc686e0ebf64865
Author: Rob Herring <robh@kernel.org>
Date:   Wed Jul 3 16:38:50 2019 -0600

    Revert "drm/gem: Rename drm_gem_dumb_map_offset() to drm_gem_map_offset()"


> But maybe we don't need this as separate function and can simply move
> the drm_vma_node_offset_addr() call into
> drm_gem_ttm_driver_dumb_mmap_offset().
>
> > > +int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
> > > +                                    struct drm_device *dev,
> > > +                                    uint32_t handle, uint64_t *offset)
> > > +{
> > > +   struct drm_gem_object *gem;
> > > +   struct ttm_buffer_object *bo;
> > > +
> > > +   gem = drm_gem_object_lookup(file, handle);
> > > +   if (!gem)
> > > +           return -ENOENT;
> > > +
> > > +   bo = drm_gem_ttm_of_gem(gem);
> > > +   *offset = drm_gem_ttm_mmap_offset(bo);
> > > +
> > > +   drm_gem_object_put_unlocked(gem);
> > > +
> > > +   return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_ttm_driver_dumb_mmap_offset);
> >
> > Same for this, you're just upcasting to ttm_bo and then downcasting to
> > gem_bo again ... I think just a series to roll out the existing gem
> > helpers everywhere should work?
>
> I don't think so.  drm_gem_dumb_map_offset() calls
> drm_gem_create_mmap_offset(), which I think is not correct for ttm
> objects because ttm_bo_init() handles vma_node initialization.

More code to unify first? This should work exactly the same way for
all gem based drivers I think ... Only tricky bit is making sure
vmwgfx keeps working correctly.
-Daniel

>
> cheers,
>   Gerd
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann Aug. 7, 2019, 10:36 a.m. UTC | #6 
Hi,

> > > Same for this, you're just upcasting to ttm_bo and then downcasting to
> > > gem_bo again ... I think just a series to roll out the existing gem
> > > helpers everywhere should work?
> >
> > I don't think so.  drm_gem_dumb_map_offset() calls
> > drm_gem_create_mmap_offset(), which I think is not correct for ttm
> > objects because ttm_bo_init() handles vma_node initialization.
> 
> More code to unify first? This should work exactly the same way for
> all gem based drivers I think ... Only tricky bit is making sure
> vmwgfx keeps working correctly.

Yea.  Unifying on the gem way of doing things isn't going to work very
well.  We would have to keep the current way of doing things in the ttm
code, wrapped into "if (ttm_bo_uses_embedded_gem_object()) { ... }", to
not break vmwgfx.

So adding gem ttm helpers (where gem+ttm drivers can opt-in) looked like
the better way of handling this to me ...

cheers,
  Gerd
Daniel Vetter Aug. 7, 2019, 11:40 a.m. UTC | #7 
On Wed, Aug 7, 2019 at 12:36 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > Same for this, you're just upcasting to ttm_bo and then downcasting to
> > > > gem_bo again ... I think just a series to roll out the existing gem
> > > > helpers everywhere should work?
> > >
> > > I don't think so.  drm_gem_dumb_map_offset() calls
> > > drm_gem_create_mmap_offset(), which I think is not correct for ttm
> > > objects because ttm_bo_init() handles vma_node initialization.
> >
> > More code to unify first? This should work exactly the same way for
> > all gem based drivers I think ... Only tricky bit is making sure
> > vmwgfx keeps working correctly.
>
> Yea.  Unifying on the gem way of doing things isn't going to work very
> well.  We would have to keep the current way of doing things in the ttm
> code, wrapped into "if (ttm_bo_uses_embedded_gem_object()) { ... }", to
> not break vmwgfx.
>
> So adding gem ttm helpers (where gem+ttm drivers can opt-in) looked like
> the better way of handling this to me ...

Ok I looked again, and your ttm version seems to exactly match
drm_gem_dumb_map_offset(), which we almost called
drm_gem_map_offset(). And could do that again by undoing that revert.
So I'm not seeing how a generic version for this stuff here wouldn't
also work for ttm ... Ofc if vmwgfx does something else they can keep
their own specific dumb map_offset implementation.

What am I missing?
-Daniel
Gerd Hoffmann Aug. 7, 2019, 11:51 a.m. UTC | #8 
Hi,

> > > > I don't think so.  drm_gem_dumb_map_offset() calls
> > > > drm_gem_create_mmap_offset(), which I think is not correct for ttm
> > > > objects because ttm_bo_init() handles vma_node initialization.

> Ok I looked again, and your ttm version seems to exactly match
> drm_gem_dumb_map_offset(),

No.  The difference outlined above is still there.  See also v2 which
adds an comment saying so.

cheers,
  Gerd
Daniel Vetter Aug. 7, 2019, 9:08 p.m. UTC | #9 
On Wed, Aug 07, 2019 at 01:51:33PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > I don't think so.  drm_gem_dumb_map_offset() calls
> > > > > drm_gem_create_mmap_offset(), which I think is not correct for ttm
> > > > > objects because ttm_bo_init() handles vma_node initialization.
> 
> > Ok I looked again, and your ttm version seems to exactly match
> > drm_gem_dumb_map_offset(),
> 
> No.  The difference outlined above is still there.  See also v2 which
> adds an comment saying so.

Creating an mmap offset is idempotent. Otherwise the gem version would
already blow up real bad, since it's getting called multiple times by
userspace already.

So I still think ttm isn't special here, how did this blow up when you
tried?
-Daniel
diff mbox series

Patch

diff --git a/include/drm/drm_gem_ttm_helper.h b/include/drm/drm_gem_ttm_helper.h
new file mode 100644
index 000000000000..2c6874190b17
--- /dev/null
+++ b/include/drm/drm_gem_ttm_helper.h
@@ -0,0 +1,27 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef DRM_GEM_TTM_HELPER_H
+#define DRM_GEM_TTM_HELPER_H
+
+#include <drm/drm_gem.h>
+#include <drm/ttm/ttm_bo_api.h>
+#include <linux/kernel.h> /* for container_of() */
+
+/**
+ * Returns the container of type &struct ttm_buffer_object
+ * for field base.
+ * @gem:	the GEM object
+ * Returns:	The containing GEM VRAM object
+ */
+static inline struct ttm_buffer_object *drm_gem_ttm_of_gem(
+	struct drm_gem_object *gem)
+{
+	return container_of(gem, struct ttm_buffer_object, base);
+}
+
+u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo);
+int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
+					struct drm_device *dev,
+					uint32_t handle, uint64_t *offset);
+
+#endif
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c b/drivers/gpu/drm/drm_gem_ttm_helper.c
new file mode 100644
index 000000000000..b069bd0c4c94
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -0,0 +1,52 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <drm/drm_gem_ttm_helper.h>
+
+/**
+ * drm_gem_ttm_mmap_offset() - Returns a GEM ttm object's mmap offset
+ * @gbo:	the GEM ttm object
+ *
+ * See drm_vma_node_offset_addr() for more information.
+ *
+ * Returns:
+ * The buffer object's offset for userspace mappings on success, or
+ * 0 if no offset is allocated.
+ */
+u64 drm_gem_ttm_mmap_offset(struct ttm_buffer_object *bo)
+{
+	return drm_vma_node_offset_addr(&bo->base.vma_node);
+}
+EXPORT_SYMBOL(drm_gem_ttm_mmap_offset);
+
+/**
+ * drm_gem_ttm_driver_dumb_mmap_offset() - \
+	Implements &struct drm_driver.dumb_mmap_offset
+ * @file:	DRM file pointer.
+ * @dev:	DRM device.
+ * @handle:	GEM handle
+ * @offset:	Returns the mapping's memory offset on success
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative errno code otherwise.
+ */
+int drm_gem_ttm_driver_dumb_mmap_offset(struct drm_file *file,
+					 struct drm_device *dev,
+					 uint32_t handle, uint64_t *offset)
+{
+	struct drm_gem_object *gem;
+	struct ttm_buffer_object *bo;
+
+	gem = drm_gem_object_lookup(file, handle);
+	if (!gem)
+		return -ENOENT;
+
+	bo = drm_gem_ttm_of_gem(gem);
+	*offset = drm_gem_ttm_mmap_offset(bo);
+
+	drm_gem_object_put_unlocked(gem);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_ttm_driver_dumb_mmap_offset);
+
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e6f40fb54c9a..f7b25519f95c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -172,6 +172,13 @@  config DRM_VRAM_HELPER
 	help
 	  Helpers for VRAM memory management
 
+config DRM_TTM_HELPER
+	tristate
+	depends on DRM
+	select DRM_TTM
+	help
+	  Helpers for ttm-based gem objects
+
 config DRM_GEM_CMA_HELPER
 	bool
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 10f8329a8b71..545c61d6528b 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -37,6 +37,9 @@  drm_vram_helper-y := drm_gem_vram_helper.o \
 		     drm_vram_mm_helper.o
 obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
 
+drm_ttm_helper-y := drm_gem_ttm_helper.o
+obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
+
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \