[PATCH 0/4] Convert VRAM helpers to GEM object functions

[PATCH 0/4] Convert VRAM helpers to GEM object functions

[Thomas Zimmermann]

The PRIME callback functions in struct drm_driver are deprecated in
favor of their counterparts in struct drm_gem_object_funcs.

This patch set introduces GEM object functions for VRAM helpers and
converts over the free and PRIME functions. Drivers affected by this
change, namely bochs and hibmc, are adapted accordingly.

Thomas Zimmermann (4):
  drm/vram: Set GEM object functions for PRIME
  drm/bochs: Remove PRIME helpers from driver structure
  drm/hibmc: Leave struct drm_driver.gem_free_object_unlocked to NULL
  drm/vram: Remove driver callback functions for PRIME

 Documentation/gpu/todo.rst                    |   4 +-
 drivers/gpu/drm/bochs/bochs_drv.c             |   1 -
 drivers/gpu/drm/drm_gem_vram_helper.c         | 188 +++++++-----------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |   2 -
 include/drm/drm_gem_vram_helper.h             |  25 +--
 5 files changed, 73 insertions(+), 147 deletions(-)

--
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Thomas Zimmermann]

PRIME functionality is now provided via the callback functions in
struct drm_gem_object_funcs. The driver-structure functions are obsolete.
As a side effect of this patch, VRAM-based drivers get basic PRIME
support automatically without having to set any flags or additional
fields.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 70 +++++++++++++++++++++++++++
 include/drm/drm_gem_vram_helper.h     |  3 +-
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 4de782ca26b2..dd220795e725 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -14,6 +14,73 @@
  * (VRAM). It can be used for framebuffer devices with dedicated memory.
  */
 
+/*
+ * GEM object funcs
+ */
+
+static void drm_gem_vram_object_free(struct drm_gem_object *gem)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
+
+	drm_gem_vram_put(gbo);
+}
+
+static int drm_gem_vram_object_funcs_pin(struct drm_gem_object *gem)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
+
+	/* Fbdev console emulation is the use case of these PRIME
+	 * helpers. This may involve updating a hardware buffer from
+	 * a shadow FB. We pin the buffer to it's current location
+	 * (either video RAM or system memory) to prevent it from
+	 * being relocated during the update operation. If you require
+	 * the buffer to be pinned to VRAM, implement a callback that
+	 * sets the flags accordingly.
+	 */
+	return drm_gem_vram_pin(gbo, 0);
+}
+
+static void drm_gem_vram_object_funcs_unpin(struct drm_gem_object *gem)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
+
+	drm_gem_vram_unpin(gbo);
+}
+
+static void *drm_gem_vram_object_funcs_vmap(struct drm_gem_object *gem)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
+	int ret;
+	void *base;
+
+	ret = drm_gem_vram_pin(gbo, 0);
+	if (ret)
+		return NULL;
+	base = drm_gem_vram_kmap(gbo, true, NULL);
+	if (IS_ERR(base)) {
+		drm_gem_vram_unpin(gbo);
+		return NULL;
+	}
+	return base;
+}
+
+static void drm_gem_vram_object_funcs_vunmap(struct drm_gem_object *gem,
+					     void *vaddr)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
+
+	drm_gem_vram_kunmap(gbo);
+	drm_gem_vram_unpin(gbo);
+}
+
+static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
+	.free	= drm_gem_vram_object_free,
+	.pin	= drm_gem_vram_object_funcs_pin,
+	.unpin	= drm_gem_vram_object_funcs_unpin,
+	.vmap	= drm_gem_vram_object_funcs_vmap,
+	.vunmap	= drm_gem_vram_object_funcs_vunmap
+};
+
 /*
  * Buffer-objects helpers
  */
@@ -80,6 +147,9 @@ static int drm_gem_vram_init(struct drm_device *dev,
 	int ret;
 	size_t acc_size;
 
+	if (!gbo->gem.funcs)
+		gbo->gem.funcs = &drm_gem_vram_object_funcs;
+
 	ret = drm_gem_object_init(dev, &gbo->gem, size);
 	if (ret)
 		return ret;
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 1a0ea18e7a74..bc8fe9feee3b 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -127,7 +127,8 @@ int drm_gem_vram_driver_dumb_mmap_offset(struct drm_file *file,
 	.gem_free_object_unlocked = \
 		drm_gem_vram_driver_gem_free_object_unlocked, \
 	.dumb_create		  = drm_gem_vram_driver_dumb_create, \
-	.dumb_map_offset	  = drm_gem_vram_driver_dumb_mmap_offset
+	.dumb_map_offset	  = drm_gem_vram_driver_dumb_mmap_offset, \
+	.gem_prime_mmap		  = drm_gem_prime_mmap
 
 /*
  * PRIME helpers for struct drm_driver
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/bochs: Remove PRIME helpers from driver structure

[Thomas Zimmermann]

VRAM PRIME helpers are now called through GEM object functions. The
driver callback functions are obsolete.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/bochs/bochs_drv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 78ad6c98861d..f48b212a2535 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -73,7 +73,6 @@ static struct drm_driver bochs_driver = {
 	.major			= 1,
 	.minor			= 0,
 	DRM_GEM_VRAM_DRIVER,
-	DRM_GEM_VRAM_DRIVER_PRIME,
 };
 
 /* ---------------------------------------------------------------------- */
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/hibmc: Leave struct drm_driver.gem_free_object_unlocked to NULL

[Thomas Zimmermann]

The GEM object's free function is now called through
struct drm_gem_object_funcs.free.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index ce89e56937b0..0efccf365101 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -51,8 +51,6 @@ static struct drm_driver hibmc_driver = {
 	.desc			= "hibmc drm driver",
 	.major			= 1,
 	.minor			= 0,
-	.gem_free_object_unlocked =
-		drm_gem_vram_driver_gem_free_object_unlocked,
 	.dumb_create            = hibmc_dumb_create,
 	.dumb_map_offset        = drm_gem_vram_driver_dumb_mmap_offset,
 	.irq_handler		= hibmc_drm_interrupt,
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/vram: Remove driver callback functions for PRIME

[Thomas Zimmermann]

PRIME functionality is now provided by GEM object functions. The driver
callback functions are obsolete.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/todo.rst            |   4 +-
 drivers/gpu/drm/drm_gem_vram_helper.c | 118 --------------------------
 include/drm/drm_gem_vram_helper.h     |  22 -----
 3 files changed, 1 insertion(+), 143 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index d49c1cc6dc28..3f6ecf846263 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -221,9 +221,7 @@ struct drm_gem_object_funcs
 GEM objects can now have a function table instead of having the callbacks on the
 DRM driver struct. This is now the preferred way and drivers can be moved over.
 
-DRM_GEM_CMA_VMAP_DRIVER_OPS, DRM_GEM_SHMEM_DRIVER_OPS already support this, but
-DRM_GEM_VRAM_DRIVER_PRIME does not yet and needs to be aligned with the previous
-two. We also need a 2nd version of the CMA define that doesn't require the
+We also need a 2nd version of the CMA define that doesn't require the
 vmapping to be present (different hook for prime importing). Plus this needs to
 be rolled out to all drivers using their own implementations, too.
 
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index dd220795e725..f00764934fb6 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -533,19 +533,6 @@ EXPORT_SYMBOL(drm_gem_vram_mm_funcs);
  * Helpers for struct drm_driver
  */
 
-/**
- * drm_gem_vram_driver_gem_free_object_unlocked() - \
-	Implements &struct drm_driver.gem_free_object_unlocked
- * @gem:	GEM object. Refers to &struct drm_gem_vram_object.gem
- */
-void drm_gem_vram_driver_gem_free_object_unlocked(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	drm_gem_vram_put(gbo);
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_free_object_unlocked);
-
 /**
  * drm_gem_vram_driver_create_dumb() - \
 	Implements &struct drm_driver.dumb_create
@@ -604,108 +591,3 @@ int drm_gem_vram_driver_dumb_mmap_offset(struct drm_file *file,
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_dumb_mmap_offset);
-
-/*
- * PRIME helpers for struct drm_driver
- */
-
-/**
- * drm_gem_vram_driver_gem_prime_pin() - \
-	Implements &struct drm_driver.gem_prime_pin
- * @gem:	The GEM object to pin
- *
- * Returns:
- * 0 on success, or
- * a negative errno code otherwise.
- */
-int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	/* Fbdev console emulation is the use case of these PRIME
-	 * helpers. This may involve updating a hardware buffer from
-	 * a shadow FB. We pin the buffer to it's current location
-	 * (either video RAM or system memory) to prevent it from
-	 * being relocated during the update operation. If you require
-	 * the buffer to be pinned to VRAM, implement a callback that
-	 * sets the flags accordingly.
-	 */
-	return drm_gem_vram_pin(gbo, 0);
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_pin);
-
-/**
- * drm_gem_vram_driver_gem_prime_unpin() - \
-	Implements &struct drm_driver.gem_prime_unpin
- * @gem:	The GEM object to unpin
- */
-void drm_gem_vram_driver_gem_prime_unpin(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	drm_gem_vram_unpin(gbo);
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_unpin);
-
-/**
- * drm_gem_vram_driver_gem_prime_vmap() - \
-	Implements &struct drm_driver.gem_prime_vmap
- * @gem:	The GEM object to map
- *
- * Returns:
- * The buffers virtual address on success, or
- * NULL otherwise.
- */
-void *drm_gem_vram_driver_gem_prime_vmap(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-	int ret;
-	void *base;
-
-	ret = drm_gem_vram_pin(gbo, 0);
-	if (ret)
-		return NULL;
-	base = drm_gem_vram_kmap(gbo, true, NULL);
-	if (IS_ERR(base)) {
-		drm_gem_vram_unpin(gbo);
-		return NULL;
-	}
-	return base;
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_vmap);
-
-/**
- * drm_gem_vram_driver_gem_prime_vunmap() - \
-	Implements &struct drm_driver.gem_prime_vunmap
- * @gem:	The GEM object to unmap
- * @vaddr:	The mapping's base address
- */
-void drm_gem_vram_driver_gem_prime_vunmap(struct drm_gem_object *gem,
-					  void *vaddr)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	drm_gem_vram_kunmap(gbo);
-	drm_gem_vram_unpin(gbo);
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_vunmap);
-
-/**
- * drm_gem_vram_driver_gem_prime_mmap() - \
-	Implements &struct drm_driver.gem_prime_mmap
- * @gem:	The GEM object to map
- * @vma:	The VMA describing the mapping
- *
- * Returns:
- * 0 on success, or
- * a negative errno code otherwise.
- */
-int drm_gem_vram_driver_gem_prime_mmap(struct drm_gem_object *gem,
-				       struct vm_area_struct *vma)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	gbo->gem.vma_node.vm_node.start = gbo->bo.vma_node.vm_node.start;
-	return drm_gem_prime_mmap(gem, vma);
-}
-EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_mmap);
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index bc8fe9feee3b..b41d932eb53a 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -108,7 +108,6 @@ extern const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs;
  * Helpers for struct drm_driver
  */
 
-void drm_gem_vram_driver_gem_free_object_unlocked(struct drm_gem_object *gem);
 int drm_gem_vram_driver_dumb_create(struct drm_file *file,
 				    struct drm_device *dev,
 				    struct drm_mode_create_dumb *args);
@@ -124,29 +123,8 @@ int drm_gem_vram_driver_dumb_mmap_offset(struct drm_file *file,
  * &struct drm_driver with default functions.
  */
 #define DRM_GEM_VRAM_DRIVER \
-	.gem_free_object_unlocked = \
-		drm_gem_vram_driver_gem_free_object_unlocked, \
 	.dumb_create		  = drm_gem_vram_driver_dumb_create, \
 	.dumb_map_offset	  = drm_gem_vram_driver_dumb_mmap_offset, \
 	.gem_prime_mmap		  = drm_gem_prime_mmap
 
-/*
- * PRIME helpers for struct drm_driver
- */
-
-int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *obj);
-void drm_gem_vram_driver_gem_prime_unpin(struct drm_gem_object *obj);
-void *drm_gem_vram_driver_gem_prime_vmap(struct drm_gem_object *obj);
-void drm_gem_vram_driver_gem_prime_vunmap(struct drm_gem_object *obj,
-					  void *vaddr);
-int drm_gem_vram_driver_gem_prime_mmap(struct drm_gem_object *obj,
-				       struct vm_area_struct *vma);
-
-#define DRM_GEM_VRAM_DRIVER_PRIME \
-	.gem_prime_pin	  = drm_gem_vram_driver_gem_prime_pin, \
-	.gem_prime_unpin  = drm_gem_vram_driver_gem_prime_unpin, \
-	.gem_prime_vmap	  = drm_gem_vram_driver_gem_prime_vmap, \
-	.gem_prime_vunmap = drm_gem_vram_driver_gem_prime_vunmap, \
-	.gem_prime_mmap	  = drm_gem_vram_driver_gem_prime_mmap
-
 #endif
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hibmc: Leave struct drm_driver.gem_free_object_unlocked to NULL

[Daniel Vetter]

On Fri, Jun 28, 2019 at 02:26:58PM +0200, Thomas Zimmermann wrote:
> The GEM object's free function is now called through
> struct drm_gem_object_funcs.free.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index ce89e56937b0..0efccf365101 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -51,8 +51,6 @@ static struct drm_driver hibmc_driver = {
>  	.desc			= "hibmc drm driver",
>  	.major			= 1,
>  	.minor			= 0,
> -	.gem_free_object_unlocked =
> -		drm_gem_vram_driver_gem_free_object_unlocked,
>  	.dumb_create            = hibmc_dumb_create,
>  	.dumb_map_offset        = drm_gem_vram_driver_dumb_mmap_offset,

This one looks like it could removed too? And then you can ditch the
EXPORT_SYMBOL and kerneldoc for that one too I think. Anyway good
follow-up. On the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But it's too hot here, so maybe only count that as an ack :-)

Cheers, Daniel

>  	.irq_handler		= hibmc_drm_interrupt,
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Gerd Hoffmann]

On Fri, Jun 28, 2019 at 02:26:56PM +0200, Thomas Zimmermann wrote:
> PRIME functionality is now provided via the callback functions in
> struct drm_gem_object_funcs. The driver-structure functions are obsolete.
> As a side effect of this patch, VRAM-based drivers get basic PRIME
> support automatically without having to set any flags or additional
> fields.

> +static void drm_gem_vram_object_free(struct drm_gem_object *gem)
> +static int drm_gem_vram_object_funcs_pin(struct drm_gem_object *gem)
> +static void drm_gem_vram_object_funcs_unpin(struct drm_gem_object *gem)
> +static void *drm_gem_vram_object_funcs_vmap(struct drm_gem_object *gem)
> +static void drm_gem_vram_object_funcs_vunmap(struct drm_gem_object *gem,
> +					     void *vaddr)

> +static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
> +	.free	= drm_gem_vram_object_free,
> +	.pin	= drm_gem_vram_object_funcs_pin,
> +	.unpin	= drm_gem_vram_object_funcs_unpin,
> +	.vmap	= drm_gem_vram_object_funcs_vmap,
> +	.vunmap	= drm_gem_vram_object_funcs_vunmap
> +};

Why new functions?  Can't you just hook up the existing prime functions?

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1570 bytes --]

Hi

Am 01.07.19 um 08:32 schrieb Gerd Hoffmann:
> On Fri, Jun 28, 2019 at 02:26:56PM +0200, Thomas Zimmermann wrote:
>> PRIME functionality is now provided via the callback functions in
>> struct drm_gem_object_funcs. The driver-structure functions are obsolete.
>> As a side effect of this patch, VRAM-based drivers get basic PRIME
>> support automatically without having to set any flags or additional
>> fields.
> 
>> +static void drm_gem_vram_object_free(struct drm_gem_object *gem)
>> +static int drm_gem_vram_object_funcs_pin(struct drm_gem_object *gem)
>> +static void drm_gem_vram_object_funcs_unpin(struct drm_gem_object *gem)
>> +static void *drm_gem_vram_object_funcs_vmap(struct drm_gem_object *gem)
>> +static void drm_gem_vram_object_funcs_vunmap(struct drm_gem_object *gem,
>> +					     void *vaddr)
> 
>> +static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
>> +	.free	= drm_gem_vram_object_free,
>> +	.pin	= drm_gem_vram_object_funcs_pin,
>> +	.unpin	= drm_gem_vram_object_funcs_unpin,
>> +	.vmap	= drm_gem_vram_object_funcs_vmap,
>> +	.vunmap	= drm_gem_vram_object_funcs_vunmap
>> +};
> 
> Why new functions?  Can't you just hook up the existing prime functions?

The final patch will remove the existing functions, so drivers won't use
them accidentally.

Best regards
Thomas

> 
> cheers,
>   Gerd
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Gerd Hoffmann]

On Mon, Jul 01, 2019 at 09:28:59AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 01.07.19 um 08:32 schrieb Gerd Hoffmann:
> > On Fri, Jun 28, 2019 at 02:26:56PM +0200, Thomas Zimmermann wrote:
> >> PRIME functionality is now provided via the callback functions in
> >> struct drm_gem_object_funcs. The driver-structure functions are obsolete.
> >> As a side effect of this patch, VRAM-based drivers get basic PRIME
> >> support automatically without having to set any flags or additional
> >> fields.
> > 
> >> +static void drm_gem_vram_object_free(struct drm_gem_object *gem)
> >> +static int drm_gem_vram_object_funcs_pin(struct drm_gem_object *gem)
> >> +static void drm_gem_vram_object_funcs_unpin(struct drm_gem_object *gem)
> >> +static void *drm_gem_vram_object_funcs_vmap(struct drm_gem_object *gem)
> >> +static void drm_gem_vram_object_funcs_vunmap(struct drm_gem_object *gem,
> >> +					     void *vaddr)
> > 
> >> +static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
> >> +	.free	= drm_gem_vram_object_free,
> >> +	.pin	= drm_gem_vram_object_funcs_pin,
> >> +	.unpin	= drm_gem_vram_object_funcs_unpin,
> >> +	.vmap	= drm_gem_vram_object_funcs_vmap,
> >> +	.vunmap	= drm_gem_vram_object_funcs_vunmap
> >> +};
> > 
> > Why new functions?  Can't you just hook up the existing prime functions?
> 
> The final patch will remove the existing functions, so drivers won't use
> them accidentally.

But the new and the old ones are identical, right?  So why add/remove?
Why not just rename them?

I'd also suggest to name them consistently (free has no _funcs, all
others have).  I'd drop _funcs from all function names.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2317 bytes --]

Hi

Am 01.07.19 um 10:48 schrieb Gerd Hoffmann:
> On Mon, Jul 01, 2019 at 09:28:59AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 01.07.19 um 08:32 schrieb Gerd Hoffmann:
>>> On Fri, Jun 28, 2019 at 02:26:56PM +0200, Thomas Zimmermann wrote:
>>>> PRIME functionality is now provided via the callback functions in
>>>> struct drm_gem_object_funcs. The driver-structure functions are obsolete.
>>>> As a side effect of this patch, VRAM-based drivers get basic PRIME
>>>> support automatically without having to set any flags or additional
>>>> fields.
>>>
>>>> +static void drm_gem_vram_object_free(struct drm_gem_object *gem)
>>>> +static int drm_gem_vram_object_funcs_pin(struct drm_gem_object *gem)
>>>> +static void drm_gem_vram_object_funcs_unpin(struct drm_gem_object *gem)
>>>> +static void *drm_gem_vram_object_funcs_vmap(struct drm_gem_object *gem)
>>>> +static void drm_gem_vram_object_funcs_vunmap(struct drm_gem_object *gem,
>>>> +					     void *vaddr)
>>>
>>>> +static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
>>>> +	.free	= drm_gem_vram_object_free,
>>>> +	.pin	= drm_gem_vram_object_funcs_pin,
>>>> +	.unpin	= drm_gem_vram_object_funcs_unpin,
>>>> +	.vmap	= drm_gem_vram_object_funcs_vmap,
>>>> +	.vunmap	= drm_gem_vram_object_funcs_vunmap
>>>> +};
>>>
>>> Why new functions?  Can't you just hook up the existing prime functions?
>>
>> The final patch will remove the existing functions, so drivers won't use
>> them accidentally.
> 
> But the new and the old ones are identical, right?  So why add/remove?
> Why not just rename them?

Hmm, OK. Does that somehow make a difference (e.g., easier backporting
or maintenance)?

> I'd also suggest to name them consistently (free has no _funcs, all
> others have).  I'd drop _funcs from all function names.

This inconsistency is actually an error in the patch. Thanks

Best regards
Thomas

> cheers,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram: Set GEM object functions for PRIME

[Gerd Hoffmann]

  Hi,

> > But the new and the old ones are identical, right?  So why add/remove?
> > Why not just rename them?
> 
> Hmm, OK. Does that somehow make a difference (e.g., easier backporting
> or maintenance)?

Easier patch review (it is obvious then you only change the way the
functions are hooked up, not the actual code).  A bit less code churn.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/hibmc: Leave struct drm_driver.gem_free_object_unlocked to NULL

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1747 bytes --]

Hi

Am 28.06.19 um 18:55 schrieb Daniel Vetter:
> On Fri, Jun 28, 2019 at 02:26:58PM +0200, Thomas Zimmermann wrote:
>> The GEM object's free function is now called through
>> struct drm_gem_object_funcs.free.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index ce89e56937b0..0efccf365101 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -51,8 +51,6 @@ static struct drm_driver hibmc_driver = {
>>  	.desc			= "hibmc drm driver",
>>  	.major			= 1,
>>  	.minor			= 0,
>> -	.gem_free_object_unlocked =
>> -		drm_gem_vram_driver_gem_free_object_unlocked,
>>  	.dumb_create            = hibmc_dumb_create,
>>  	.dumb_map_offset        = drm_gem_vram_driver_dumb_mmap_offset,
> 
> This one looks like it could removed too?

Removing it requires a bit more work on the related fop's mmap function.
Probably something for a separate patch.

Best regards
Thomas

> And then you can ditch the
> EXPORT_SYMBOL and kerneldoc for that one too I think. Anyway good
> follow-up. On the series:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But it's too hot here, so maybe only count that as an ack :-)
> 
> Cheers, Daniel
> 
>>  	.irq_handler		= hibmc_drm_interrupt,
>> -- 
>> 2.21.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel