[RFC 0/3] drm/gem: Add drm_gem_object_funcs

[RFC 0/3] drm/gem: Add drm_gem_object_funcs

[Noralf Trønnes]

Hi,

I've found it odd that the GEM object has its callbacks on drm_driver
and not a vtable of its own. But something being odd isn't enough to
make a change (me thinks).

After working on the GEM shmem helper I saw that a few drivers have
runtime support for 2 memory types for their buffers (shmem,vram,cma).
I have realised that if the shmem helper was self contained wrt the
callbacks, it would be easier for these types of drivers to use the
helper. All they needed to do was to determine the buffer type on GEM
object creation time and let the helper handle the rest of the
callbacks.

No sure if this makes sense or if the approach is to simplistic. Hence
the RFC.

I've added a patch to give an example of how this would look for the CMA
helper and vc4 (I'm not volunteering to refactor the CMA helper and
drivers).

Noralf.

Noralf Trønnes (3):
  drm/driver: Add defaults for .gem_prime_export/import callbacks
  drm/gem: Add drm_gem_object_funcs
  drm/cma: Use drm_gem_object_funcs

 Documentation/gpu/todo.rst           |   7 ++
 drivers/gpu/drm/drm_client.c         |  12 ++-
 drivers/gpu/drm/drm_fb_helper.c      |   8 +-
 drivers/gpu/drm/drm_gem.c            | 108 +++++++++++++++++++++++++--
 drivers/gpu/drm/drm_gem_cma_helper.c |  99 +++++++++----------------
 drivers/gpu/drm/drm_prime.c          |  50 +++++++------
 drivers/gpu/drm/vc4/vc4_bo.c         |  46 ++++++------
 drivers/gpu/drm/vc4/vc4_drv.c        |  26 +------
 drivers/gpu/drm/vc4/vc4_drv.h        |   6 +-
 include/drm/drm_drv.h                |   4 +
 include/drm/drm_gem.h                | 138 +++++++++++++++++++++++++++++++++++
 include/drm/drm_gem_cma_helper.h     |  17 +----
 12 files changed, 350 insertions(+), 171 deletions(-)

-- 
2.15.1

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

[RFC 1/3] drm/driver: Add defaults for .gem_prime_export/import callbacks

[Noralf Trønnes]

The majority of drivers use drm_gem_prime_export() and
drm_gem_prime_import() for these callbacks so let's make them the
default.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/gpu/todo.rst  |  7 +++++++
 drivers/gpu/drm/drm_prime.c | 10 ++++++++--
 include/drm/drm_drv.h       |  4 ++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 77c2b3c25565..39748e22dea8 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -234,6 +234,13 @@ efficient.
 
 Contact: Daniel Vetter
 
+Defaults for .gem_prime_import and export
+-----------------------------------------
+
+Most drivers don't need to set drm_driver->gem_prime_import and
+->gem_prime_export now that drm_gem_prime_import() and drm_gem_prime_export()
+are the default.
+
 Core refactorings
 =================
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 3f0205fc0a1a..6721571749ff 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -559,7 +559,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 		return dmabuf;
 	}
 
-	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	if (dev->driver->gem_prime_export)
+		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	else
+		dmabuf = drm_gem_prime_export(dev, obj, flags);
 	if (IS_ERR(dmabuf)) {
 		/* normally the created dma-buf takes ownership of the ref,
 		 * but if that fails then drop the ref
@@ -792,7 +795,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
 	/* never seen this one, need to import */
 	mutex_lock(&dev->object_name_lock);
-	obj = dev->driver->gem_prime_import(dev, dma_buf);
+	if (dev->driver->gem_prime_import)
+		obj = dev->driver->gem_prime_import(dev, dma_buf);
+	else
+		obj = drm_gem_prime_import(dev, dma_buf);
 	if (IS_ERR(obj)) {
 		ret = PTR_ERR(obj);
 		goto out_unlock;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 8830e3de3a86..1162145f7f68 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -471,6 +471,8 @@ struct drm_driver {
 	 * @gem_prime_export:
 	 *
 	 * export GEM -> dmabuf
+	 *
+	 * This defaults to drm_gem_prime_export() if not set.
 	 */
 	struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
 				struct drm_gem_object *obj, int flags);
@@ -478,6 +480,8 @@ struct drm_driver {
 	 * @gem_prime_import:
 	 *
 	 * import dmabuf -> GEM
+	 *
+	 * This defaults to drm_gem_prime_import() if not set.
 	 */
 	struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 				struct dma_buf *dma_buf);
-- 
2.15.1

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

Re: [RFC 1/3] drm/driver: Add defaults for .gem_prime_export/import callbacks

[Daniel Vetter]

On Fri, Sep 21, 2018 at 06:42:28PM +0200, Noralf Trønnes wrote:
> The majority of drivers use drm_gem_prime_export() and
> drm_gem_prime_import() for these callbacks so let's make them the
> default.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/gpu/todo.rst  |  7 +++++++
>  drivers/gpu/drm/drm_prime.c | 10 ++++++++--
>  include/drm/drm_drv.h       |  4 ++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 77c2b3c25565..39748e22dea8 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -234,6 +234,13 @@ efficient.
>  
>  Contact: Daniel Vetter
>  
> +Defaults for .gem_prime_import and export
> +-----------------------------------------
> +
> +Most drivers don't need to set drm_driver->gem_prime_import and
> +->gem_prime_export now that drm_gem_prime_import() and drm_gem_prime_export()
> +are the default.
> +
>  Core refactorings
>  =================
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 3f0205fc0a1a..6721571749ff 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -559,7 +559,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>  		return dmabuf;
>  	}
>  
> -	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> +	if (dev->driver->gem_prime_export)
> +		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> +	else
> +		dmabuf = drm_gem_prime_export(dev, obj, flags);

Hm, this one here operates on a drm_gem_object (the dev parameter is kinda
redundant). I think this would look neat in the callback structure you're
adding in the next patch. There's also a bunch of gem drivers overwriting
this one, so would make your callbacks structure more generally useful.

Otherwise lgtm.
-Daniel

>  	if (IS_ERR(dmabuf)) {
>  		/* normally the created dma-buf takes ownership of the ref,
>  		 * but if that fails then drop the ref
> @@ -792,7 +795,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>  	/* never seen this one, need to import */
>  	mutex_lock(&dev->object_name_lock);
> -	obj = dev->driver->gem_prime_import(dev, dma_buf);
> +	if (dev->driver->gem_prime_import)
> +		obj = dev->driver->gem_prime_import(dev, dma_buf);
> +	else
> +		obj = drm_gem_prime_import(dev, dma_buf);
>  	if (IS_ERR(obj)) {
>  		ret = PTR_ERR(obj);
>  		goto out_unlock;
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 8830e3de3a86..1162145f7f68 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -471,6 +471,8 @@ struct drm_driver {
>  	 * @gem_prime_export:
>  	 *
>  	 * export GEM -> dmabuf
> +	 *
> +	 * This defaults to drm_gem_prime_export() if not set.
>  	 */
>  	struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
>  				struct drm_gem_object *obj, int flags);
> @@ -478,6 +480,8 @@ struct drm_driver {
>  	 * @gem_prime_import:
>  	 *
>  	 * import dmabuf -> GEM
> +	 *
> +	 * This defaults to drm_gem_prime_import() if not set.
>  	 */
>  	struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>  				struct dma_buf *dma_buf);
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: [RFC 1/3] drm/driver: Add defaults for .gem_prime_export/import callbacks

[Noralf Trønnes]

Den 22.09.2018 12.10, skrev Daniel Vetter:
> On Fri, Sep 21, 2018 at 06:42:28PM +0200, Noralf Trønnes wrote:
>> The majority of drivers use drm_gem_prime_export() and
>> drm_gem_prime_import() for these callbacks so let's make them the
>> default.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   Documentation/gpu/todo.rst  |  7 +++++++
>>   drivers/gpu/drm/drm_prime.c | 10 ++++++++--
>>   include/drm/drm_drv.h       |  4 ++++
>>   3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>> index 77c2b3c25565..39748e22dea8 100644
>> --- a/Documentation/gpu/todo.rst
>> +++ b/Documentation/gpu/todo.rst
>> @@ -234,6 +234,13 @@ efficient.
>>   
>>   Contact: Daniel Vetter
>>   
>> +Defaults for .gem_prime_import and export
>> +-----------------------------------------
>> +
>> +Most drivers don't need to set drm_driver->gem_prime_import and
>> +->gem_prime_export now that drm_gem_prime_import() and drm_gem_prime_export()
>> +are the default.
>> +
>>   Core refactorings
>>   =================
>>   
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 3f0205fc0a1a..6721571749ff 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -559,7 +559,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>>   		return dmabuf;
>>   	}
>>   
>> -	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
>> +	if (dev->driver->gem_prime_export)
>> +		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
>> +	else
>> +		dmabuf = drm_gem_prime_export(dev, obj, flags);
> Hm, this one here operates on a drm_gem_object (the dev parameter is kinda
> redundant). I think this would look neat in the callback structure you're
> adding in the next patch. There's also a bunch of gem drivers overwriting
> this one, so would make your callbacks structure more generally useful.

I'm not following you here.
What I do is to add a default for the 29 drivers that have this set to
drm_gem_prime_export(). For the 8 drivers that have their own thing, I
have the funcs->export callback that they can use.

Combined with the next patch it looks like this:

     if (dev->driver->gem_prime_export)
         dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
     else if (obj->funcs && obj->funcs->export)
         dmabuf = obj->funcs->export(obj, flags);
     else
         dmabuf = drm_gem_prime_export(dev, obj, flags);

Noralf.

> Otherwise lgtm.
> -Daniel
>
>>   	if (IS_ERR(dmabuf)) {
>>   		/* normally the created dma-buf takes ownership of the ref,
>>   		 * but if that fails then drop the ref
>> @@ -792,7 +795,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>>   
>>   	/* never seen this one, need to import */
>>   	mutex_lock(&dev->object_name_lock);
>> -	obj = dev->driver->gem_prime_import(dev, dma_buf);
>> +	if (dev->driver->gem_prime_import)
>> +		obj = dev->driver->gem_prime_import(dev, dma_buf);
>> +	else
>> +		obj = drm_gem_prime_import(dev, dma_buf);
>>   	if (IS_ERR(obj)) {
>>   		ret = PTR_ERR(obj);
>>   		goto out_unlock;
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 8830e3de3a86..1162145f7f68 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -471,6 +471,8 @@ struct drm_driver {
>>   	 * @gem_prime_export:
>>   	 *
>>   	 * export GEM -> dmabuf
>> +	 *
>> +	 * This defaults to drm_gem_prime_export() if not set.
>>   	 */
>>   	struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
>>   				struct drm_gem_object *obj, int flags);
>> @@ -478,6 +480,8 @@ struct drm_driver {
>>   	 * @gem_prime_import:
>>   	 *
>>   	 * import dmabuf -> GEM
>> +	 *
>> +	 * This defaults to drm_gem_prime_import() if not set.
>>   	 */
>>   	struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>>   				struct dma_buf *dma_buf);
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [RFC 1/3] drm/driver: Add defaults for .gem_prime_export/import callbacks

[Daniel Vetter]

On Sat, Sep 22, 2018 at 05:58:30PM +0200, Noralf Trønnes wrote:
> 
> Den 22.09.2018 12.10, skrev Daniel Vetter:
> > On Fri, Sep 21, 2018 at 06:42:28PM +0200, Noralf Trønnes wrote:
> > > The majority of drivers use drm_gem_prime_export() and
> > > drm_gem_prime_import() for these callbacks so let's make them the
> > > default.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   Documentation/gpu/todo.rst  |  7 +++++++
> > >   drivers/gpu/drm/drm_prime.c | 10 ++++++++--
> > >   include/drm/drm_drv.h       |  4 ++++
> > >   3 files changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 77c2b3c25565..39748e22dea8 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -234,6 +234,13 @@ efficient.
> > >   Contact: Daniel Vetter
> > > +Defaults for .gem_prime_import and export
> > > +-----------------------------------------
> > > +
> > > +Most drivers don't need to set drm_driver->gem_prime_import and
> > > +->gem_prime_export now that drm_gem_prime_import() and drm_gem_prime_export()
> > > +are the default.
> > > +
> > >   Core refactorings
> > >   =================
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 3f0205fc0a1a..6721571749ff 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -559,7 +559,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> > >   		return dmabuf;
> > >   	}
> > > -	dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> > > +	if (dev->driver->gem_prime_export)
> > > +		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> > > +	else
> > > +		dmabuf = drm_gem_prime_export(dev, obj, flags);
> > Hm, this one here operates on a drm_gem_object (the dev parameter is kinda
> > redundant). I think this would look neat in the callback structure you're
> > adding in the next patch. There's also a bunch of gem drivers overwriting
> > this one, so would make your callbacks structure more generally useful.
> 
> I'm not following you here.
> What I do is to add a default for the 29 drivers that have this set to
> drm_gem_prime_export(). For the 8 drivers that have their own thing, I
> have the funcs->export callback that they can use.
> 
> Combined with the next patch it looks like this:
> 
>     if (dev->driver->gem_prime_export)
>         dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
>     else if (obj->funcs && obj->funcs->export)
>         dmabuf = obj->funcs->export(obj, flags);
>     else
>         dmabuf = drm_gem_prime_export(dev, obj, flags);

Oh, I missed that you're adding the funcs->export thing already, that's
what I wanted to suggested.
-Daniel

> 
> Noralf.
> 
> > Otherwise lgtm.
> > -Daniel
> > 
> > >   	if (IS_ERR(dmabuf)) {
> > >   		/* normally the created dma-buf takes ownership of the ref,
> > >   		 * but if that fails then drop the ref
> > > @@ -792,7 +795,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> > >   	/* never seen this one, need to import */
> > >   	mutex_lock(&dev->object_name_lock);
> > > -	obj = dev->driver->gem_prime_import(dev, dma_buf);
> > > +	if (dev->driver->gem_prime_import)
> > > +		obj = dev->driver->gem_prime_import(dev, dma_buf);
> > > +	else
> > > +		obj = drm_gem_prime_import(dev, dma_buf);
> > >   	if (IS_ERR(obj)) {
> > >   		ret = PTR_ERR(obj);
> > >   		goto out_unlock;
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index 8830e3de3a86..1162145f7f68 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -471,6 +471,8 @@ struct drm_driver {
> > >   	 * @gem_prime_export:
> > >   	 *
> > >   	 * export GEM -> dmabuf
> > > +	 *
> > > +	 * This defaults to drm_gem_prime_export() if not set.
> > >   	 */
> > >   	struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
> > >   				struct drm_gem_object *obj, int flags);
> > > @@ -478,6 +480,8 @@ struct drm_driver {
> > >   	 * @gem_prime_import:
> > >   	 *
> > >   	 * import dmabuf -> GEM
> > > +	 *
> > > +	 * This defaults to drm_gem_prime_import() if not set.
> > >   	 */
> > >   	struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> > >   				struct dma_buf *dma_buf);
> > > -- 
> > > 2.15.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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

[RFC 2/3] drm/gem: Add drm_gem_object_funcs

[Noralf Trønnes]

This adds an optional function table on GEM objects.
The main benefit is for drivers that support more than one type of
memory (shmem,vram,cma) for their buffers depending on the hardware it
runs on. With the callbacks attached to the GEM object itself, it is
easier to have core helpers for the the various buffer types. The driver
only has to make the decision about buffer type on GEM object creation
and all other callbacks can be handled by the chosen helper.

drm_driver->gem_prime_res_obj has not been added since there's a todo to
put a reservation_object into drm_gem_object.

The individual drm_driver callbacks take priority over the GEM attached
callbacks.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_client.c    |  12 ++--
 drivers/gpu/drm/drm_fb_helper.c |   8 ++-
 drivers/gpu/drm/drm_gem.c       | 108 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_prime.c     |  40 ++++++------
 include/drm/drm_gem.h           | 138 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 272 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 17d9a64e885e..eca7331762e4 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -80,8 +80,7 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
 {
 	int ret;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
-	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
+	if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create)
 		return -EOPNOTSUPP;
 
 	if (funcs && !try_module_get(funcs->owner))
@@ -212,8 +211,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
 {
 	struct drm_device *dev = buffer->client->dev;
 
-	if (buffer->vaddr && dev->driver->gem_prime_vunmap)
-		dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
+	drm_gem_vunmap(buffer->gem, buffer->vaddr);
 
 	if (buffer->gem)
 		drm_gem_object_put_unlocked(buffer->gem);
@@ -266,9 +264,9 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
 	 * fd_install step out of the driver backend hooks, to make that
 	 * final step optional for internal users.
 	 */
-	vaddr = dev->driver->gem_prime_vmap(obj);
-	if (!vaddr) {
-		ret = -ENOMEM;
+	vaddr = drm_gem_vmap(obj);
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
 		goto err_delete;
 	}
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8e95d0f7c71d..66969f0facbb 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -38,6 +38,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_gem.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -3010,9 +3011,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
 static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct drm_fb_helper *fb_helper = info->par;
+	struct drm_gem_object *obj = fb_helper->buffer->gem;
 
-	if (fb_helper->dev->driver->gem_prime_mmap)
-		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
+	if (obj->dev->driver->gem_prime_mmap)
+		return obj->dev->driver->gem_prime_mmap(obj, vma);
+	else if (obj->funcs && obj->funcs->mmap)
+		return drm_gem_mmap_obj(obj, obj->size, vma);
 	else
 		return -ENODEV;
 }
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 512078ebd97b..7da2549667ce 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -259,6 +259,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 
 	if (dev->driver->gem_close_object)
 		dev->driver->gem_close_object(obj, file_priv);
+	else if (obj->funcs && obj->funcs->close)
+		obj->funcs->close(obj, file_priv);
 
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_gem_remove_prime_handles(obj, file_priv);
@@ -414,6 +416,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 		ret = dev->driver->gem_open_object(obj, file_priv);
 		if (ret)
 			goto err_revoke;
+	} else if (obj->funcs && obj->funcs->open) {
+		ret = obj->funcs->open(obj, file_priv);
+		if (ret)
+			goto err_revoke;
 	}
 
 	*handlep = handle;
@@ -841,6 +847,8 @@ drm_gem_object_free(struct kref *kref)
 		WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 		dev->driver->gem_free_object(obj);
+	} else if (obj->funcs) {
+		obj->funcs->free(obj);
 	}
 }
 EXPORT_SYMBOL(drm_gem_object_free);
@@ -864,13 +872,13 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
 
 	dev = obj->dev;
 
-	if (dev->driver->gem_free_object_unlocked) {
-		kref_put(&obj->refcount, drm_gem_object_free);
-	} else {
+	if (dev->driver->gem_free_object) {
 		might_lock(&dev->struct_mutex);
 		if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
 				&dev->struct_mutex))
 			mutex_unlock(&dev->struct_mutex);
+	} else {
+		kref_put(&obj->refcount, drm_gem_object_free);
 	}
 }
 EXPORT_SYMBOL(drm_gem_object_put_unlocked);
@@ -955,20 +963,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 		     struct vm_area_struct *vma)
 {
 	struct drm_device *dev = obj->dev;
+	int ret;
 
 	/* Check for valid size. */
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
 
-	if (!dev->driver->gem_vm_ops)
+	if (dev->driver->gem_vm_ops)
+		vma->vm_ops = dev->driver->gem_vm_ops;
+	else if (obj->funcs && obj->funcs->vm_ops)
+		vma->vm_ops = obj->funcs->vm_ops;
+	else
 		return -EINVAL;
 
 	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
-	vma->vm_ops = dev->driver->gem_vm_ops;
 	vma->vm_private_data = obj;
 	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
 	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
 
+	if (obj->funcs && obj->funcs->mmap) {
+		ret = obj->funcs->mmap(obj, vma);
+		if (ret)
+			return ret;
+	}
+
 	/* Take a ref for this mapping of the object, so that the fault
 	 * handler can dereference the mmap offset's pointer to the object.
 	 * This reference is cleaned up by the corresponding vm_close
@@ -1068,4 +1086,84 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
 
 	if (obj->dev->driver->gem_print_info)
 		obj->dev->driver->gem_print_info(p, indent, obj);
+	else if (obj->funcs && obj->funcs->print_info)
+		obj->funcs->print_info(p, indent, obj);
 }
+
+/**
+ * drm_gem_pin - Pin backing buffer in memory
+ * @obj: GEM object
+ *
+ * Make sure the backing buffer is pinned in memory.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_pin(struct drm_gem_object *obj)
+{
+	if (obj->dev->driver->gem_prime_pin)
+		return obj->dev->driver->gem_prime_pin(obj);
+	else if (obj->funcs && obj->funcs->pin)
+		return obj->funcs->pin(obj);
+	else
+		return 0;
+}
+EXPORT_SYMBOL(drm_gem_pin);
+
+/**
+ * drm_gem_pin - Unpin backing buffer from memory
+ * @obj: GEM object
+ *
+ * Relax the requirement that the backing buffer is pinned in memory.
+ */
+void drm_gem_unpin(struct drm_gem_object *obj)
+{
+	if (obj->dev->driver->gem_prime_unpin)
+		obj->dev->driver->gem_prime_unpin(obj);
+	else if (obj->funcs && obj->funcs->unpin)
+		obj->funcs->unpin(obj);
+}
+EXPORT_SYMBOL(drm_gem_unpin);
+
+/**
+ * drm_gem_vmap - Map buffer into kernel virtual address space
+ * @obj: GEM object
+ *
+ * Returns:
+ * A virtual pointer to a newly created GEM object or an ERR_PTR-encoded negative
+ * error code on failure.
+ */
+void *drm_gem_vmap(struct drm_gem_object *obj)
+{
+	void *vaddr;
+
+	if (obj->dev->driver->gem_prime_vmap)
+		vaddr = obj->dev->driver->gem_prime_vmap(obj);
+	else if (obj->funcs && obj->funcs->vmap)
+		vaddr = obj->funcs->vmap(obj);
+	else
+		vaddr = ERR_PTR(-EOPNOTSUPP);
+
+	if (!vaddr)
+		vaddr = ERR_PTR(-ENOMEM);
+
+	return vaddr;
+}
+EXPORT_SYMBOL(drm_gem_vmap);
+
+/**
+ * drm_gem_vunmap - Remove buffer mapping from kernel virtual address space
+ * @obj: GEM object
+ * @vaddr: Virtual address (can be NULL)
+ */
+void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
+{
+	if (!vaddr)
+		return;
+
+	if (obj->dev->driver->gem_prime_vunmap)
+		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
+	else if (obj->funcs && obj->funcs->vunmap)
+		obj->funcs->vunmap(obj, vaddr);
+}
+EXPORT_SYMBOL(drm_gem_vunmap);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 6721571749ff..7f5c9500136b 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -199,7 +199,6 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
 {
 	struct drm_prime_attachment *prime_attach;
 	struct drm_gem_object *obj = dma_buf->priv;
-	struct drm_device *dev = obj->dev;
 
 	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
 	if (!prime_attach)
@@ -208,10 +207,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
 	prime_attach->dir = DMA_NONE;
 	attach->priv = prime_attach;
 
-	if (!dev->driver->gem_prime_pin)
-		return 0;
-
-	return dev->driver->gem_prime_pin(obj);
+	return drm_gem_pin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_attach);
 
@@ -228,7 +224,6 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
 {
 	struct drm_prime_attachment *prime_attach = attach->priv;
 	struct drm_gem_object *obj = dma_buf->priv;
-	struct drm_device *dev = obj->dev;
 
 	if (prime_attach) {
 		struct sg_table *sgt = prime_attach->sgt;
@@ -247,8 +242,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
 		attach->priv = NULL;
 	}
 
-	if (dev->driver->gem_prime_unpin)
-		dev->driver->gem_prime_unpin(obj);
+	drm_gem_unpin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_detach);
 
@@ -310,7 +304,10 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
 	if (WARN_ON(prime_attach->dir != DMA_NONE))
 		return ERR_PTR(-EBUSY);
 
-	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+	if (obj->dev->driver->gem_prime_get_sg_table)
+		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+	else
+		sgt = obj->funcs->get_sg_table(obj);
 
 	if (!IS_ERR(sgt)) {
 		if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
@@ -406,12 +403,13 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
 void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
-	struct drm_device *dev = obj->dev;
+	void *vaddr;
 
-	if (dev->driver->gem_prime_vmap)
-		return dev->driver->gem_prime_vmap(obj);
-	else
-		return NULL;
+	vaddr = drm_gem_vmap(obj);
+	if (IS_ERR(vaddr))
+		vaddr = NULL;
+
+	return vaddr;
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
 
@@ -426,10 +424,8 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
 void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
-	struct drm_device *dev = obj->dev;
 
-	if (dev->driver->gem_prime_vunmap)
-		dev->driver->gem_prime_vunmap(obj, vaddr);
+	drm_gem_vunmap(obj, vaddr);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
 
@@ -476,10 +472,12 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct drm_device *dev = obj->dev;
 
-	if (!dev->driver->gem_prime_mmap)
+	if (dev->driver->gem_prime_mmap)
+		return dev->driver->gem_prime_mmap(obj, vma);
+	else if (obj->funcs && obj->funcs->mmap)
+		return drm_gem_mmap_obj(obj, obj->size, vma);
+	else
 		return -ENOSYS;
-
-	return dev->driver->gem_prime_mmap(obj, vma);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
 
@@ -561,6 +559,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 
 	if (dev->driver->gem_prime_export)
 		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
+	else if (obj->funcs && obj->funcs->export)
+		dmabuf = obj->funcs->export(obj, flags);
 	else
 		dmabuf = drm_gem_prime_export(dev, obj, flags);
 	if (IS_ERR(dmabuf)) {
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 3583b98a1718..3ae3665bf4e7 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -38,6 +38,130 @@
 
 #include <drm/drm_vma_manager.h>
 
+struct drm_gem_object;
+
+/**
+ * struct drm_gem_object_funcs - GEM object functions
+ */
+struct drm_gem_object_funcs {
+	/**
+	 * @free:
+	 *
+	 * Deconstructor for drm_gem_objects.
+	 *
+	 * This callback is mandatory.
+	 */
+	void (*free)(struct drm_gem_object *obj);
+
+	/**
+	 * @open:
+	 *
+	 * Called upon GEM handle creation.
+	 *
+	 * This callback is optional.
+	 */
+	int (*open)(struct drm_gem_object *obj, struct drm_file *file);
+
+	/**
+	 * @close_object:
+	 *
+	 * Called upon GEM handle release.
+	 *
+	 * This callback is optional.
+	 */
+	void (*close)(struct drm_gem_object *obj, struct drm_file *file);
+
+	/**
+	 * @print_info:
+	 *
+	 * If driver subclasses struct &drm_gem_object, it can implement this
+	 * optional hook for printing additional driver specific info.
+	 *
+	 * drm_printf_indent() should be used in the callback passing it the
+	 * indent argument.
+	 *
+	 * This callback is called from drm_gem_print_info().
+	 *
+	 * This callback is optional.
+	 */
+	void (*print_info)(struct drm_printer *p, unsigned int indent,
+			   const struct drm_gem_object *obj);
+
+	/**
+	 * @export:
+	 *
+	 * Export backing buffer as a &dma_buf.
+	 * If this is not set drm_gem_prime_export() is used.
+	 *
+	 * This callback is optional.
+	 */
+	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
+
+	/*
+	 * @pin:
+	 *
+	 * Pin backing buffer in memory.
+	 *
+	 * This callback is optional.
+	 */
+	int (*pin)(struct drm_gem_object *obj);
+
+	/*
+	 * @unpin:
+	 *
+	 * Unpin backing buffer.
+	 *
+	 * This callback is optional.
+	 */
+	void (*unpin)(struct drm_gem_object *obj);
+
+	/*
+	 * @get_sg_table:
+	 *
+	 * Returns a Scatter-Gather table representation of the buffer.
+	 * Used when exporting a buffer.
+	 *
+	 * This callback is mandatory if buffer export is supported.
+	 */
+	struct sg_table *(*get_sg_table)(struct drm_gem_object *obj);
+
+	/*
+	 * @vmap:
+	 *
+	 * Returns a virtual address for the buffer.
+	 *
+	 * This callback is optional.
+	 */
+	void *(*vmap)(struct drm_gem_object *obj);
+
+	/*
+	 * @vunmap:
+	 *
+	 * Releases the the address previously returned by @vmap.
+	 *
+	 * This callback is optional.
+	 */
+	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
+
+	/*
+	 * @mmap:
+	 *
+	 * Setup userspace mapping with the given vma.
+	 *
+	 * This callback is optional.
+	 */
+	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
+
+	/**
+	 * @vm_ops:
+	 *
+	 * Virtual memory operations used with mmap.
+	 *
+	 * This is optional but necessary for mmap support.
+	 */
+	const struct vm_operations_struct *vm_ops;
+};
+
 /**
  * struct drm_gem_object - GEM buffer object
  *
@@ -146,6 +270,15 @@ struct drm_gem_object {
 	 * simply leave it as NULL.
 	 */
 	struct dma_buf_attachment *import_attach;
+
+	/*
+	 * @funcs:
+	 *
+	 * Optional GEM object functions. If this is set, it will be used if the
+	 * corresponding &drm_driver GEM callback is not set.
+	 *
+	 */
+	const struct drm_gem_object_funcs *funcs;
 };
 
 /**
@@ -293,4 +426,9 @@ int drm_gem_dumb_destroy(struct drm_file *file,
 			 struct drm_device *dev,
 			 uint32_t handle);
 
+int drm_gem_pin(struct drm_gem_object *obj);
+void drm_gem_unpin(struct drm_gem_object *obj);
+void *drm_gem_vmap(struct drm_gem_object *obj);
+void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
+
 #endif /* __DRM_GEM_H__ */
-- 
2.15.1

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

Re: [RFC 2/3] drm/gem: Add drm_gem_object_funcs

[Daniel Vetter]

On Fri, Sep 21, 2018 at 06:42:29PM +0200, Noralf Trønnes wrote:
> This adds an optional function table on GEM objects.
> The main benefit is for drivers that support more than one type of
> memory (shmem,vram,cma) for their buffers depending on the hardware it
> runs on. With the callbacks attached to the GEM object itself, it is
> easier to have core helpers for the the various buffer types. The driver
> only has to make the decision about buffer type on GEM object creation
> and all other callbacks can be handled by the chosen helper.
> 
> drm_driver->gem_prime_res_obj has not been added since there's a todo to
> put a reservation_object into drm_gem_object.
> 
> The individual drm_driver callbacks take priority over the GEM attached
> callbacks.

I'd do this the other way round, make the new gem callbacks the clearly
favoured option.

> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_client.c    |  12 ++--
>  drivers/gpu/drm/drm_fb_helper.c |   8 ++-
>  drivers/gpu/drm/drm_gem.c       | 108 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_prime.c     |  40 ++++++------
>  include/drm/drm_gem.h           | 138 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 272 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 17d9a64e885e..eca7331762e4 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -80,8 +80,7 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
>  {
>  	int ret;
>  
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
> -	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create)
>  		return -EOPNOTSUPP;

I guess the ->gem_prime_vmap check got lost, needs to be readded somewhere
I think.

>  
>  	if (funcs && !try_module_get(funcs->owner))
> @@ -212,8 +211,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>  {
>  	struct drm_device *dev = buffer->client->dev;
>  
> -	if (buffer->vaddr && dev->driver->gem_prime_vunmap)
> -		dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
> +	drm_gem_vunmap(buffer->gem, buffer->vaddr);
>  
>  	if (buffer->gem)
>  		drm_gem_object_put_unlocked(buffer->gem);
> @@ -266,9 +264,9 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>  	 * fd_install step out of the driver backend hooks, to make that
>  	 * final step optional for internal users.
>  	 */
> -	vaddr = dev->driver->gem_prime_vmap(obj);
> -	if (!vaddr) {
> -		ret = -ENOMEM;
> +	vaddr = drm_gem_vmap(obj);
> +	if (IS_ERR(vaddr)) {
> +		ret = PTR_ERR(vaddr);
>  		goto err_delete;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8e95d0f7c71d..66969f0facbb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -38,6 +38,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -3010,9 +3011,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>  static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_gem_object *obj = fb_helper->buffer->gem;
>  
> -	if (fb_helper->dev->driver->gem_prime_mmap)
> -		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
> +	if (obj->dev->driver->gem_prime_mmap)
> +		return obj->dev->driver->gem_prime_mmap(obj, vma);
> +	else if (obj->funcs && obj->funcs->mmap)
> +		return drm_gem_mmap_obj(obj, obj->size, vma);
>  	else
>  		return -ENODEV;
>  }
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 512078ebd97b..7da2549667ce 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -259,6 +259,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  
>  	if (dev->driver->gem_close_object)
>  		dev->driver->gem_close_object(obj, file_priv);
> +	else if (obj->funcs && obj->funcs->close)
> +		obj->funcs->close(obj, file_priv);
>  
>  	if (drm_core_check_feature(dev, DRIVER_PRIME))
>  		drm_gem_remove_prime_handles(obj, file_priv);
> @@ -414,6 +416,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  		ret = dev->driver->gem_open_object(obj, file_priv);
>  		if (ret)
>  			goto err_revoke;
> +	} else if (obj->funcs && obj->funcs->open) {
> +		ret = obj->funcs->open(obj, file_priv);
> +		if (ret)
> +			goto err_revoke;
>  	}
>  
>  	*handlep = handle;
> @@ -841,6 +847,8 @@ drm_gem_object_free(struct kref *kref)
>  		WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  		dev->driver->gem_free_object(obj);
> +	} else if (obj->funcs) {
> +		obj->funcs->free(obj);
>  	}
>  }
>  EXPORT_SYMBOL(drm_gem_object_free);
> @@ -864,13 +872,13 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
>  
>  	dev = obj->dev;
>  
> -	if (dev->driver->gem_free_object_unlocked) {
> -		kref_put(&obj->refcount, drm_gem_object_free);
> -	} else {
> +	if (dev->driver->gem_free_object) {
>  		might_lock(&dev->struct_mutex);
>  		if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
>  				&dev->struct_mutex))
>  			mutex_unlock(&dev->struct_mutex);
> +	} else {
> +		kref_put(&obj->refcount, drm_gem_object_free);
>  	}
>  }
>  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> @@ -955,20 +963,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		     struct vm_area_struct *vma)
>  {
>  	struct drm_device *dev = obj->dev;
> +	int ret;
>  
>  	/* Check for valid size. */
>  	if (obj_size < vma->vm_end - vma->vm_start)
>  		return -EINVAL;
>  
> -	if (!dev->driver->gem_vm_ops)
> +	if (dev->driver->gem_vm_ops)
> +		vma->vm_ops = dev->driver->gem_vm_ops;
> +	else if (obj->funcs && obj->funcs->vm_ops)
> +		vma->vm_ops = obj->funcs->vm_ops;
> +	else
>  		return -EINVAL;

A bit a bikeshed, but if we go with these new ops, I'd put them first in
all the if ladders. Makes it a bit clearer while reading that they're the
recommended option.
>  
>  	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> -	vma->vm_ops = dev->driver->gem_vm_ops;
>  	vma->vm_private_data = obj;
>  	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>  	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>  
> +	if (obj->funcs && obj->funcs->mmap) {
> +		ret = obj->funcs->mmap(obj, vma);
> +		if (ret)
> +			return ret;
> +	}

I'm confused about this one here ... this is for prime mmap only iirc.

> +
>  	/* Take a ref for this mapping of the object, so that the fault
>  	 * handler can dereference the mmap offset's pointer to the object.
>  	 * This reference is cleaned up by the corresponding vm_close
> @@ -1068,4 +1086,84 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>  
>  	if (obj->dev->driver->gem_print_info)
>  		obj->dev->driver->gem_print_info(p, indent, obj);
> +	else if (obj->funcs && obj->funcs->print_info)
> +		obj->funcs->print_info(p, indent, obj);
>  }
> +
> +/**
> + * drm_gem_pin - Pin backing buffer in memory
> + * @obj: GEM object
> + *
> + * Make sure the backing buffer is pinned in memory.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_pin(struct drm_gem_object *obj)
> +{
> +	if (obj->dev->driver->gem_prime_pin)
> +		return obj->dev->driver->gem_prime_pin(obj);
> +	else if (obj->funcs && obj->funcs->pin)
> +		return obj->funcs->pin(obj);
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_pin);
> +
> +/**
> + * drm_gem_pin - Unpin backing buffer from memory
> + * @obj: GEM object
> + *
> + * Relax the requirement that the backing buffer is pinned in memory.
> + */
> +void drm_gem_unpin(struct drm_gem_object *obj)
> +{
> +	if (obj->dev->driver->gem_prime_unpin)
> +		obj->dev->driver->gem_prime_unpin(obj);
> +	else if (obj->funcs && obj->funcs->unpin)
> +		obj->funcs->unpin(obj);
> +}
> +EXPORT_SYMBOL(drm_gem_unpin);
> +
> +/**
> + * drm_gem_vmap - Map buffer into kernel virtual address space
> + * @obj: GEM object
> + *
> + * Returns:
> + * A virtual pointer to a newly created GEM object or an ERR_PTR-encoded negative
> + * error code on failure.
> + */
> +void *drm_gem_vmap(struct drm_gem_object *obj)
> +{
> +	void *vaddr;
> +
> +	if (obj->dev->driver->gem_prime_vmap)
> +		vaddr = obj->dev->driver->gem_prime_vmap(obj);
> +	else if (obj->funcs && obj->funcs->vmap)
> +		vaddr = obj->funcs->vmap(obj);
> +	else
> +		vaddr = ERR_PTR(-EOPNOTSUPP);
> +
> +	if (!vaddr)
> +		vaddr = ERR_PTR(-ENOMEM);
> +
> +	return vaddr;
> +}
> +EXPORT_SYMBOL(drm_gem_vmap);
> +
> +/**
> + * drm_gem_vunmap - Remove buffer mapping from kernel virtual address space
> + * @obj: GEM object
> + * @vaddr: Virtual address (can be NULL)
> + */
> +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
> +{
> +	if (!vaddr)
> +		return;
> +
> +	if (obj->dev->driver->gem_prime_vunmap)
> +		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
> +	else if (obj->funcs && obj->funcs->vunmap)
> +		obj->funcs->vunmap(obj, vaddr);
> +}
> +EXPORT_SYMBOL(drm_gem_vunmap);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 6721571749ff..7f5c9500136b 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -199,7 +199,6 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>  {
>  	struct drm_prime_attachment *prime_attach;
>  	struct drm_gem_object *obj = dma_buf->priv;
> -	struct drm_device *dev = obj->dev;
>  
>  	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
>  	if (!prime_attach)
> @@ -208,10 +207,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>  	prime_attach->dir = DMA_NONE;
>  	attach->priv = prime_attach;
>  
> -	if (!dev->driver->gem_prime_pin)
> -		return 0;
> -
> -	return dev->driver->gem_prime_pin(obj);
> +	return drm_gem_pin(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_map_attach);
>  
> @@ -228,7 +224,6 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
>  {
>  	struct drm_prime_attachment *prime_attach = attach->priv;
>  	struct drm_gem_object *obj = dma_buf->priv;
> -	struct drm_device *dev = obj->dev;
>  
>  	if (prime_attach) {
>  		struct sg_table *sgt = prime_attach->sgt;
> @@ -247,8 +242,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
>  		attach->priv = NULL;
>  	}
>  
> -	if (dev->driver->gem_prime_unpin)
> -		dev->driver->gem_prime_unpin(obj);
> +	drm_gem_unpin(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_map_detach);
>  
> @@ -310,7 +304,10 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>  	if (WARN_ON(prime_attach->dir != DMA_NONE))
>  		return ERR_PTR(-EBUSY);
>  
> -	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);

I think if we just check for obj->funcs here and then unconditionally
call, we avoid a bunch of pointer chasing for the new recommended path :-)

> +	if (obj->dev->driver->gem_prime_get_sg_table)
> +		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> +	else
> +		sgt = obj->funcs->get_sg_table(obj);
>  
>  	if (!IS_ERR(sgt)) {
>  		if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
> @@ -406,12 +403,13 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
>  void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>  {
>  	struct drm_gem_object *obj = dma_buf->priv;
> -	struct drm_device *dev = obj->dev;
> +	void *vaddr;
>  
> -	if (dev->driver->gem_prime_vmap)
> -		return dev->driver->gem_prime_vmap(obj);
> -	else
> -		return NULL;
> +	vaddr = drm_gem_vmap(obj);
> +	if (IS_ERR(vaddr))
> +		vaddr = NULL;
> +
> +	return vaddr;
>  }
>  EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
>  
> @@ -426,10 +424,8 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
>  void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>  {
>  	struct drm_gem_object *obj = dma_buf->priv;
> -	struct drm_device *dev = obj->dev;
>  
> -	if (dev->driver->gem_prime_vunmap)
> -		dev->driver->gem_prime_vunmap(obj, vaddr);
> +	drm_gem_vunmap(obj, vaddr);
>  }
>  EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
>  
> @@ -476,10 +472,12 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  	struct drm_gem_object *obj = dma_buf->priv;
>  	struct drm_device *dev = obj->dev;
>  
> -	if (!dev->driver->gem_prime_mmap)
> +	if (dev->driver->gem_prime_mmap)
> +		return dev->driver->gem_prime_mmap(obj, vma);
> +	else if (obj->funcs && obj->funcs->mmap)
> +		return drm_gem_mmap_obj(obj, obj->size, vma);
> +	else
>  		return -ENOSYS;
> -
> -	return dev->driver->gem_prime_mmap(obj, vma);
>  }
>  EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>  
> @@ -561,6 +559,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>  
>  	if (dev->driver->gem_prime_export)
>  		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> +	else if (obj->funcs && obj->funcs->export)
> +		dmabuf = obj->funcs->export(obj, flags);
>  	else
>  		dmabuf = drm_gem_prime_export(dev, obj, flags);
>  	if (IS_ERR(dmabuf)) {
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 3583b98a1718..3ae3665bf4e7 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -38,6 +38,130 @@
>  
>  #include <drm/drm_vma_manager.h>
>  
> +struct drm_gem_object;
> +
> +/**
> + * struct drm_gem_object_funcs - GEM object functions
> + */
> +struct drm_gem_object_funcs {
> +	/**
> +	 * @free:
> +	 *
> +	 * Deconstructor for drm_gem_objects.
> +	 *
> +	 * This callback is mandatory.
> +	 */
> +	void (*free)(struct drm_gem_object *obj);
> +
> +	/**
> +	 * @open:
> +	 *
> +	 * Called upon GEM handle creation.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	int (*open)(struct drm_gem_object *obj, struct drm_file *file);
> +
> +	/**
> +	 * @close_object:

@close:

> +	 *
> +	 * Called upon GEM handle release.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void (*close)(struct drm_gem_object *obj, struct drm_file *file);
> +
> +	/**
> +	 * @print_info:
> +	 *
> +	 * If driver subclasses struct &drm_gem_object, it can implement this
> +	 * optional hook for printing additional driver specific info.
> +	 *
> +	 * drm_printf_indent() should be used in the callback passing it the
> +	 * indent argument.
> +	 *
> +	 * This callback is called from drm_gem_print_info().
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void (*print_info)(struct drm_printer *p, unsigned int indent,
> +			   const struct drm_gem_object *obj);
> +
> +	/**
> +	 * @export:
> +	 *
> +	 * Export backing buffer as a &dma_buf.
> +	 * If this is not set drm_gem_prime_export() is used.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
> +
> +	/*
> +	 * @pin:
> +	 *
> +	 * Pin backing buffer in memory.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	int (*pin)(struct drm_gem_object *obj);
> +
> +	/*
> +	 * @unpin:
> +	 *
> +	 * Unpin backing buffer.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void (*unpin)(struct drm_gem_object *obj);
> +
> +	/*
> +	 * @get_sg_table:
> +	 *
> +	 * Returns a Scatter-Gather table representation of the buffer.
> +	 * Used when exporting a buffer.
> +	 *
> +	 * This callback is mandatory if buffer export is supported.
> +	 */
> +	struct sg_table *(*get_sg_table)(struct drm_gem_object *obj);
> +
> +	/*
> +	 * @vmap:
> +	 *
> +	 * Returns a virtual address for the buffer.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void *(*vmap)(struct drm_gem_object *obj);
> +
> +	/*
> +	 * @vunmap:
> +	 *
> +	 * Releases the the address previously returned by @vmap.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
> +
> +	/*
> +	 * @mmap:
> +	 *
> +	 * Setup userspace mapping with the given vma.
> +	 *
> +	 * This callback is optional.
> +	 */
> +	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
> +
> +	/**
> +	 * @vm_ops:
> +	 *
> +	 * Virtual memory operations used with mmap.
> +	 *
> +	 * This is optional but necessary for mmap support.
> +	 */
> +	const struct vm_operations_struct *vm_ops;
> +};

I think fleshing out the docs a bit is a good opportunity here (and adding
links to the old place that the new ones recommended). But probably best
in follow-up patches.

> +
>  /**
>   * struct drm_gem_object - GEM buffer object
>   *
> @@ -146,6 +270,15 @@ struct drm_gem_object {
>  	 * simply leave it as NULL.
>  	 */
>  	struct dma_buf_attachment *import_attach;
> +
> +	/*
> +	 * @funcs:
> +	 *
> +	 * Optional GEM object functions. If this is set, it will be used if the
> +	 * corresponding &drm_driver GEM callback is not set.

Maybe also here a note that this is recommended over the callbacks in
&drm_driver.

Aside from all the tiny style suggestions, I really like this.
-Daniel

> +	 *
> +	 */
> +	const struct drm_gem_object_funcs *funcs;
>  };
>  
>  /**
> @@ -293,4 +426,9 @@ int drm_gem_dumb_destroy(struct drm_file *file,
>  			 struct drm_device *dev,
>  			 uint32_t handle);
>  
> +int drm_gem_pin(struct drm_gem_object *obj);
> +void drm_gem_unpin(struct drm_gem_object *obj);
> +void *drm_gem_vmap(struct drm_gem_object *obj);
> +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
> +
>  #endif /* __DRM_GEM_H__ */
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: [RFC 2/3] drm/gem: Add drm_gem_object_funcs

[Noralf Trønnes]

Den 22.09.2018 12.41, skrev Daniel Vetter:
> On Fri, Sep 21, 2018 at 06:42:29PM +0200, Noralf Trønnes wrote:
>> This adds an optional function table on GEM objects.
>> The main benefit is for drivers that support more than one type of
>> memory (shmem,vram,cma) for their buffers depending on the hardware it
>> runs on. With the callbacks attached to the GEM object itself, it is
>> easier to have core helpers for the the various buffer types. The driver
>> only has to make the decision about buffer type on GEM object creation
>> and all other callbacks can be handled by the chosen helper.
>>
>> drm_driver->gem_prime_res_obj has not been added since there's a todo to
>> put a reservation_object into drm_gem_object.
>>
>> The individual drm_driver callbacks take priority over the GEM attached
>> callbacks.
> I'd do this the other way round, make the new gem callbacks the clearly
> favoured option.

The reason I did it like this is that I thought it would be easier to
convert the CMA helper this way. I've taken a closer look at this, and
the solution is to convert the drivers that override the defaults first,
and then convert the helper as a whole when that's done.

I'll flip it around.

>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_client.c    |  12 ++--
>>   drivers/gpu/drm/drm_fb_helper.c |   8 ++-
>>   drivers/gpu/drm/drm_gem.c       | 108 +++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/drm_prime.c     |  40 ++++++------
>>   include/drm/drm_gem.h           | 138 ++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 272 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 17d9a64e885e..eca7331762e4 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -80,8 +80,7 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
>>   {
>>   	int ret;
>>   
>> -	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
>> -	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
>> +	if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create)
>>   		return -EOPNOTSUPP;
> I guess the ->gem_prime_vmap check got lost, needs to be readded somewhere
> I think.

It happens in drm_client_buffer_create() when drm_gem_vmap() is called.
It returns -EOPNOTSUPP if no callback is set.

>
>>   
>>   	if (funcs && !try_module_get(funcs->owner))
>> @@ -212,8 +211,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>>   {
>>   	struct drm_device *dev = buffer->client->dev;
>>   
>> -	if (buffer->vaddr && dev->driver->gem_prime_vunmap)
>> -		dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
>> +	drm_gem_vunmap(buffer->gem, buffer->vaddr);
>>   
>>   	if (buffer->gem)
>>   		drm_gem_object_put_unlocked(buffer->gem);
>> @@ -266,9 +264,9 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
>>   	 * fd_install step out of the driver backend hooks, to make that
>>   	 * final step optional for internal users.
>>   	 */
>> -	vaddr = dev->driver->gem_prime_vmap(obj);
>> -	if (!vaddr) {
>> -		ret = -ENOMEM;
>> +	vaddr = drm_gem_vmap(obj);
>> +	if (IS_ERR(vaddr)) {
>> +		ret = PTR_ERR(vaddr);
>>   		goto err_delete;
>>   	}
>>   
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 8e95d0f7c71d..66969f0facbb 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -38,6 +38,7 @@
>>   #include <drm/drmP.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem.h>
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>> @@ -3010,9 +3011,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>>   static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>>   {
>>   	struct drm_fb_helper *fb_helper = info->par;
>> +	struct drm_gem_object *obj = fb_helper->buffer->gem;
>>   
>> -	if (fb_helper->dev->driver->gem_prime_mmap)
>> -		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
>> +	if (obj->dev->driver->gem_prime_mmap)
>> +		return obj->dev->driver->gem_prime_mmap(obj, vma);
>> +	else if (obj->funcs && obj->funcs->mmap)
>> +		return drm_gem_mmap_obj(obj, obj->size, vma);
>>   	else
>>   		return -ENODEV;
>>   }
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 512078ebd97b..7da2549667ce 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -259,6 +259,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>>   
>>   	if (dev->driver->gem_close_object)
>>   		dev->driver->gem_close_object(obj, file_priv);
>> +	else if (obj->funcs && obj->funcs->close)
>> +		obj->funcs->close(obj, file_priv);
>>   
>>   	if (drm_core_check_feature(dev, DRIVER_PRIME))
>>   		drm_gem_remove_prime_handles(obj, file_priv);
>> @@ -414,6 +416,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>>   		ret = dev->driver->gem_open_object(obj, file_priv);
>>   		if (ret)
>>   			goto err_revoke;
>> +	} else if (obj->funcs && obj->funcs->open) {
>> +		ret = obj->funcs->open(obj, file_priv);
>> +		if (ret)
>> +			goto err_revoke;
>>   	}
>>   
>>   	*handlep = handle;
>> @@ -841,6 +847,8 @@ drm_gem_object_free(struct kref *kref)
>>   		WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>   
>>   		dev->driver->gem_free_object(obj);
>> +	} else if (obj->funcs) {
>> +		obj->funcs->free(obj);
>>   	}
>>   }
>>   EXPORT_SYMBOL(drm_gem_object_free);
>> @@ -864,13 +872,13 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
>>   
>>   	dev = obj->dev;
>>   
>> -	if (dev->driver->gem_free_object_unlocked) {
>> -		kref_put(&obj->refcount, drm_gem_object_free);
>> -	} else {
>> +	if (dev->driver->gem_free_object) {
>>   		might_lock(&dev->struct_mutex);
>>   		if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
>>   				&dev->struct_mutex))
>>   			mutex_unlock(&dev->struct_mutex);
>> +	} else {
>> +		kref_put(&obj->refcount, drm_gem_object_free);
>>   	}
>>   }
>>   EXPORT_SYMBOL(drm_gem_object_put_unlocked);
>> @@ -955,20 +963,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>>   		     struct vm_area_struct *vma)
>>   {
>>   	struct drm_device *dev = obj->dev;
>> +	int ret;
>>   
>>   	/* Check for valid size. */
>>   	if (obj_size < vma->vm_end - vma->vm_start)
>>   		return -EINVAL;
>>   
>> -	if (!dev->driver->gem_vm_ops)
>> +	if (dev->driver->gem_vm_ops)
>> +		vma->vm_ops = dev->driver->gem_vm_ops;
>> +	else if (obj->funcs && obj->funcs->vm_ops)
>> +		vma->vm_ops = obj->funcs->vm_ops;
>> +	else
>>   		return -EINVAL;
> A bit a bikeshed, but if we go with these new ops, I'd put them first in
> all the if ladders. Makes it a bit clearer while reading that they're the
> recommended option.
>>   
>>   	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
>> -	vma->vm_ops = dev->driver->gem_vm_ops;
>>   	vma->vm_private_data = obj;
>>   	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>   	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>>   
>> +	if (obj->funcs && obj->funcs->mmap) {
>> +		ret = obj->funcs->mmap(obj, vma);
>> +		if (ret)
>> +			return ret;
>> +	}
> I'm confused about this one here ... this is for prime mmap only iirc.

The smaller drivers that I've looked at combine the DRM mmap and PRIME
mmap paths going into a shared *_mmap_obj() function.

This is what I'm doing:

DRM fd: fops->mmap = drm_gem_mmap
     -> drm_gem_mmap_obj
         -> gem->funcs->mmap

dmabuf fd: fops->mmap = dma_buf_mmap_internal
     -> dmabuf->ops->mmap = drm_gem_dmabuf_mmap
         -> drm_gem_mmap_obj
             -> gem->funcs->mmap

Is there a difference to the mapping operation itself whether the mmap
happens through DRM dev or through dmabuf?

Noralf.

>> +
>>   	/* Take a ref for this mapping of the object, so that the fault
>>   	 * handler can dereference the mmap offset's pointer to the object.
>>   	 * This reference is cleaned up by the corresponding vm_close
>> @@ -1068,4 +1086,84 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>>   
>>   	if (obj->dev->driver->gem_print_info)
>>   		obj->dev->driver->gem_print_info(p, indent, obj);
>> +	else if (obj->funcs && obj->funcs->print_info)
>> +		obj->funcs->print_info(p, indent, obj);
>>   }
>> +
>> +/**
>> + * drm_gem_pin - Pin backing buffer in memory
>> + * @obj: GEM object
>> + *
>> + * Make sure the backing buffer is pinned in memory.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_pin(struct drm_gem_object *obj)
>> +{
>> +	if (obj->dev->driver->gem_prime_pin)
>> +		return obj->dev->driver->gem_prime_pin(obj);
>> +	else if (obj->funcs && obj->funcs->pin)
>> +		return obj->funcs->pin(obj);
>> +	else
>> +		return 0;
>> +}
>> +EXPORT_SYMBOL(drm_gem_pin);
>> +
>> +/**
>> + * drm_gem_pin - Unpin backing buffer from memory
>> + * @obj: GEM object
>> + *
>> + * Relax the requirement that the backing buffer is pinned in memory.
>> + */
>> +void drm_gem_unpin(struct drm_gem_object *obj)
>> +{
>> +	if (obj->dev->driver->gem_prime_unpin)
>> +		obj->dev->driver->gem_prime_unpin(obj);
>> +	else if (obj->funcs && obj->funcs->unpin)
>> +		obj->funcs->unpin(obj);
>> +}
>> +EXPORT_SYMBOL(drm_gem_unpin);
>> +
>> +/**
>> + * drm_gem_vmap - Map buffer into kernel virtual address space
>> + * @obj: GEM object
>> + *
>> + * Returns:
>> + * A virtual pointer to a newly created GEM object or an ERR_PTR-encoded negative
>> + * error code on failure.
>> + */
>> +void *drm_gem_vmap(struct drm_gem_object *obj)
>> +{
>> +	void *vaddr;
>> +
>> +	if (obj->dev->driver->gem_prime_vmap)
>> +		vaddr = obj->dev->driver->gem_prime_vmap(obj);
>> +	else if (obj->funcs && obj->funcs->vmap)
>> +		vaddr = obj->funcs->vmap(obj);
>> +	else
>> +		vaddr = ERR_PTR(-EOPNOTSUPP);
>> +
>> +	if (!vaddr)
>> +		vaddr = ERR_PTR(-ENOMEM);
>> +
>> +	return vaddr;
>> +}
>> +EXPORT_SYMBOL(drm_gem_vmap);
>> +
>> +/**
>> + * drm_gem_vunmap - Remove buffer mapping from kernel virtual address space
>> + * @obj: GEM object
>> + * @vaddr: Virtual address (can be NULL)
>> + */
>> +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
>> +{
>> +	if (!vaddr)
>> +		return;
>> +
>> +	if (obj->dev->driver->gem_prime_vunmap)
>> +		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
>> +	else if (obj->funcs && obj->funcs->vunmap)
>> +		obj->funcs->vunmap(obj, vaddr);
>> +}
>> +EXPORT_SYMBOL(drm_gem_vunmap);
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 6721571749ff..7f5c9500136b 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -199,7 +199,6 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>>   {
>>   	struct drm_prime_attachment *prime_attach;
>>   	struct drm_gem_object *obj = dma_buf->priv;
>> -	struct drm_device *dev = obj->dev;
>>   
>>   	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
>>   	if (!prime_attach)
>> @@ -208,10 +207,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>>   	prime_attach->dir = DMA_NONE;
>>   	attach->priv = prime_attach;
>>   
>> -	if (!dev->driver->gem_prime_pin)
>> -		return 0;
>> -
>> -	return dev->driver->gem_prime_pin(obj);
>> +	return drm_gem_pin(obj);
>>   }
>>   EXPORT_SYMBOL(drm_gem_map_attach);
>>   
>> @@ -228,7 +224,6 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
>>   {
>>   	struct drm_prime_attachment *prime_attach = attach->priv;
>>   	struct drm_gem_object *obj = dma_buf->priv;
>> -	struct drm_device *dev = obj->dev;
>>   
>>   	if (prime_attach) {
>>   		struct sg_table *sgt = prime_attach->sgt;
>> @@ -247,8 +242,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
>>   		attach->priv = NULL;
>>   	}
>>   
>> -	if (dev->driver->gem_prime_unpin)
>> -		dev->driver->gem_prime_unpin(obj);
>> +	drm_gem_unpin(obj);
>>   }
>>   EXPORT_SYMBOL(drm_gem_map_detach);
>>   
>> @@ -310,7 +304,10 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>>   	if (WARN_ON(prime_attach->dir != DMA_NONE))
>>   		return ERR_PTR(-EBUSY);
>>   
>> -	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> I think if we just check for obj->funcs here and then unconditionally
> call, we avoid a bunch of pointer chasing for the new recommended path :-)
>
>> +	if (obj->dev->driver->gem_prime_get_sg_table)
>> +		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> +	else
>> +		sgt = obj->funcs->get_sg_table(obj);
>>   
>>   	if (!IS_ERR(sgt)) {
>>   		if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
>> @@ -406,12 +403,13 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
>>   void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>>   {
>>   	struct drm_gem_object *obj = dma_buf->priv;
>> -	struct drm_device *dev = obj->dev;
>> +	void *vaddr;
>>   
>> -	if (dev->driver->gem_prime_vmap)
>> -		return dev->driver->gem_prime_vmap(obj);
>> -	else
>> -		return NULL;
>> +	vaddr = drm_gem_vmap(obj);
>> +	if (IS_ERR(vaddr))
>> +		vaddr = NULL;
>> +
>> +	return vaddr;
>>   }
>>   EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
>>   
>> @@ -426,10 +424,8 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
>>   void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>>   {
>>   	struct drm_gem_object *obj = dma_buf->priv;
>> -	struct drm_device *dev = obj->dev;
>>   
>> -	if (dev->driver->gem_prime_vunmap)
>> -		dev->driver->gem_prime_vunmap(obj, vaddr);
>> +	drm_gem_vunmap(obj, vaddr);
>>   }
>>   EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
>>   
>> @@ -476,10 +472,12 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>>   	struct drm_gem_object *obj = dma_buf->priv;
>>   	struct drm_device *dev = obj->dev;
>>   
>> -	if (!dev->driver->gem_prime_mmap)
>> +	if (dev->driver->gem_prime_mmap)
>> +		return dev->driver->gem_prime_mmap(obj, vma);
>> +	else if (obj->funcs && obj->funcs->mmap)
>> +		return drm_gem_mmap_obj(obj, obj->size, vma);
>> +	else
>>   		return -ENOSYS;
>> -
>> -	return dev->driver->gem_prime_mmap(obj, vma);
>>   }
>>   EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
>>   
>> @@ -561,6 +559,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>>   
>>   	if (dev->driver->gem_prime_export)
>>   		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
>> +	else if (obj->funcs && obj->funcs->export)
>> +		dmabuf = obj->funcs->export(obj, flags);
>>   	else
>>   		dmabuf = drm_gem_prime_export(dev, obj, flags);
>>   	if (IS_ERR(dmabuf)) {
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index 3583b98a1718..3ae3665bf4e7 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -38,6 +38,130 @@
>>   
>>   #include <drm/drm_vma_manager.h>
>>   
>> +struct drm_gem_object;
>> +
>> +/**
>> + * struct drm_gem_object_funcs - GEM object functions
>> + */
>> +struct drm_gem_object_funcs {
>> +	/**
>> +	 * @free:
>> +	 *
>> +	 * Deconstructor for drm_gem_objects.
>> +	 *
>> +	 * This callback is mandatory.
>> +	 */
>> +	void (*free)(struct drm_gem_object *obj);
>> +
>> +	/**
>> +	 * @open:
>> +	 *
>> +	 * Called upon GEM handle creation.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	int (*open)(struct drm_gem_object *obj, struct drm_file *file);
>> +
>> +	/**
>> +	 * @close_object:
> @close:
>
>> +	 *
>> +	 * Called upon GEM handle release.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	void (*close)(struct drm_gem_object *obj, struct drm_file *file);
>> +
>> +	/**
>> +	 * @print_info:
>> +	 *
>> +	 * If driver subclasses struct &drm_gem_object, it can implement this
>> +	 * optional hook for printing additional driver specific info.
>> +	 *
>> +	 * drm_printf_indent() should be used in the callback passing it the
>> +	 * indent argument.
>> +	 *
>> +	 * This callback is called from drm_gem_print_info().
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	void (*print_info)(struct drm_printer *p, unsigned int indent,
>> +			   const struct drm_gem_object *obj);
>> +
>> +	/**
>> +	 * @export:
>> +	 *
>> +	 * Export backing buffer as a &dma_buf.
>> +	 * If this is not set drm_gem_prime_export() is used.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
>> +
>> +	/*
>> +	 * @pin:
>> +	 *
>> +	 * Pin backing buffer in memory.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	int (*pin)(struct drm_gem_object *obj);
>> +
>> +	/*
>> +	 * @unpin:
>> +	 *
>> +	 * Unpin backing buffer.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	void (*unpin)(struct drm_gem_object *obj);
>> +
>> +	/*
>> +	 * @get_sg_table:
>> +	 *
>> +	 * Returns a Scatter-Gather table representation of the buffer.
>> +	 * Used when exporting a buffer.
>> +	 *
>> +	 * This callback is mandatory if buffer export is supported.
>> +	 */
>> +	struct sg_table *(*get_sg_table)(struct drm_gem_object *obj);
>> +
>> +	/*
>> +	 * @vmap:
>> +	 *
>> +	 * Returns a virtual address for the buffer.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	void *(*vmap)(struct drm_gem_object *obj);
>> +
>> +	/*
>> +	 * @vunmap:
>> +	 *
>> +	 * Releases the the address previously returned by @vmap.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
>> +
>> +	/*
>> +	 * @mmap:
>> +	 *
>> +	 * Setup userspace mapping with the given vma.
>> +	 *
>> +	 * This callback is optional.
>> +	 */
>> +	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
>> +
>> +	/**
>> +	 * @vm_ops:
>> +	 *
>> +	 * Virtual memory operations used with mmap.
>> +	 *
>> +	 * This is optional but necessary for mmap support.
>> +	 */
>> +	const struct vm_operations_struct *vm_ops;
>> +};
> I think fleshing out the docs a bit is a good opportunity here (and adding
> links to the old place that the new ones recommended). But probably best
> in follow-up patches.
>
>> +
>>   /**
>>    * struct drm_gem_object - GEM buffer object
>>    *
>> @@ -146,6 +270,15 @@ struct drm_gem_object {
>>   	 * simply leave it as NULL.
>>   	 */
>>   	struct dma_buf_attachment *import_attach;
>> +
>> +	/*
>> +	 * @funcs:
>> +	 *
>> +	 * Optional GEM object functions. If this is set, it will be used if the
>> +	 * corresponding &drm_driver GEM callback is not set.
> Maybe also here a note that this is recommended over the callbacks in
> &drm_driver.
>
> Aside from all the tiny style suggestions, I really like this.
> -Daniel
>
>> +	 *
>> +	 */
>> +	const struct drm_gem_object_funcs *funcs;
>>   };
>>   
>>   /**
>> @@ -293,4 +426,9 @@ int drm_gem_dumb_destroy(struct drm_file *file,
>>   			 struct drm_device *dev,
>>   			 uint32_t handle);
>>   
>> +int drm_gem_pin(struct drm_gem_object *obj);
>> +void drm_gem_unpin(struct drm_gem_object *obj);
>> +void *drm_gem_vmap(struct drm_gem_object *obj);
>> +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
>> +
>>   #endif /* __DRM_GEM_H__ */
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [RFC 2/3] drm/gem: Add drm_gem_object_funcs

[Daniel Vetter]

On Sat, Sep 22, 2018 at 06:01:55PM +0200, Noralf Trønnes wrote:
> 
> Den 22.09.2018 12.41, skrev Daniel Vetter:
> > On Fri, Sep 21, 2018 at 06:42:29PM +0200, Noralf Trønnes wrote:
> > > This adds an optional function table on GEM objects.
> > > The main benefit is for drivers that support more than one type of
> > > memory (shmem,vram,cma) for their buffers depending on the hardware it
> > > runs on. With the callbacks attached to the GEM object itself, it is
> > > easier to have core helpers for the the various buffer types. The driver
> > > only has to make the decision about buffer type on GEM object creation
> > > and all other callbacks can be handled by the chosen helper.
> > > 
> > > drm_driver->gem_prime_res_obj has not been added since there's a todo to
> > > put a reservation_object into drm_gem_object.
> > > 
> > > The individual drm_driver callbacks take priority over the GEM attached
> > > callbacks.
> > I'd do this the other way round, make the new gem callbacks the clearly
> > favoured option.
> 
> The reason I did it like this is that I thought it would be easier to
> convert the CMA helper this way. I've taken a closer look at this, and
> the solution is to convert the drivers that override the defaults first,
> and then convert the helper as a whole when that's done.
> 
> I'll flip it around.

If it's easier for conversion, then keep as-is (but probably with a plan
to change the defaults later on). Up to you.

> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   drivers/gpu/drm/drm_client.c    |  12 ++--
> > >   drivers/gpu/drm/drm_fb_helper.c |   8 ++-
> > >   drivers/gpu/drm/drm_gem.c       | 108 +++++++++++++++++++++++++++++--
> > >   drivers/gpu/drm/drm_prime.c     |  40 ++++++------
> > >   include/drm/drm_gem.h           | 138 ++++++++++++++++++++++++++++++++++++++++
> > >   5 files changed, 272 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > index 17d9a64e885e..eca7331762e4 100644
> > > --- a/drivers/gpu/drm/drm_client.c
> > > +++ b/drivers/gpu/drm/drm_client.c
> > > @@ -80,8 +80,7 @@ int drm_client_new(struct drm_device *dev, struct drm_client_dev *client,
> > >   {
> > >   	int ret;
> > > -	if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
> > > -	    !dev->driver->dumb_create || !dev->driver->gem_prime_vmap)
> > > +	if (!drm_core_check_feature(dev, DRIVER_MODESET) || !dev->driver->dumb_create)
> > >   		return -EOPNOTSUPP;
> > I guess the ->gem_prime_vmap check got lost, needs to be readded somewhere
> > I think.
> 
> It happens in drm_client_buffer_create() when drm_gem_vmap() is called.
> It returns -EOPNOTSUPP if no callback is set.

Ah, missed that.

> > 
> > >   	if (funcs && !try_module_get(funcs->owner))
> > > @@ -212,8 +211,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
> > >   {
> > >   	struct drm_device *dev = buffer->client->dev;
> > > -	if (buffer->vaddr && dev->driver->gem_prime_vunmap)
> > > -		dev->driver->gem_prime_vunmap(buffer->gem, buffer->vaddr);
> > > +	drm_gem_vunmap(buffer->gem, buffer->vaddr);
> > >   	if (buffer->gem)
> > >   		drm_gem_object_put_unlocked(buffer->gem);
> > > @@ -266,9 +264,9 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
> > >   	 * fd_install step out of the driver backend hooks, to make that
> > >   	 * final step optional for internal users.
> > >   	 */
> > > -	vaddr = dev->driver->gem_prime_vmap(obj);
> > > -	if (!vaddr) {
> > > -		ret = -ENOMEM;
> > > +	vaddr = drm_gem_vmap(obj);
> > > +	if (IS_ERR(vaddr)) {
> > > +		ret = PTR_ERR(vaddr);
> > >   		goto err_delete;
> > >   	}
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 8e95d0f7c71d..66969f0facbb 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -38,6 +38,7 @@
> > >   #include <drm/drmP.h>
> > >   #include <drm/drm_crtc.h>
> > >   #include <drm/drm_fb_helper.h>
> > > +#include <drm/drm_gem.h>
> > >   #include <drm/drm_crtc_helper.h>
> > >   #include <drm/drm_atomic.h>
> > >   #include <drm/drm_atomic_helper.h>
> > > @@ -3010,9 +3011,12 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
> > >   static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > >   {
> > >   	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct drm_gem_object *obj = fb_helper->buffer->gem;
> > > -	if (fb_helper->dev->driver->gem_prime_mmap)
> > > -		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
> > > +	if (obj->dev->driver->gem_prime_mmap)
> > > +		return obj->dev->driver->gem_prime_mmap(obj, vma);
> > > +	else if (obj->funcs && obj->funcs->mmap)
> > > +		return drm_gem_mmap_obj(obj, obj->size, vma);
> > >   	else
> > >   		return -ENODEV;
> > >   }
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 512078ebd97b..7da2549667ce 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -259,6 +259,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
> > >   	if (dev->driver->gem_close_object)
> > >   		dev->driver->gem_close_object(obj, file_priv);
> > > +	else if (obj->funcs && obj->funcs->close)
> > > +		obj->funcs->close(obj, file_priv);
> > >   	if (drm_core_check_feature(dev, DRIVER_PRIME))
> > >   		drm_gem_remove_prime_handles(obj, file_priv);
> > > @@ -414,6 +416,10 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > >   		ret = dev->driver->gem_open_object(obj, file_priv);
> > >   		if (ret)
> > >   			goto err_revoke;
> > > +	} else if (obj->funcs && obj->funcs->open) {
> > > +		ret = obj->funcs->open(obj, file_priv);
> > > +		if (ret)
> > > +			goto err_revoke;
> > >   	}
> > >   	*handlep = handle;
> > > @@ -841,6 +847,8 @@ drm_gem_object_free(struct kref *kref)
> > >   		WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > >   		dev->driver->gem_free_object(obj);
> > > +	} else if (obj->funcs) {
> > > +		obj->funcs->free(obj);
> > >   	}
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_object_free);
> > > @@ -864,13 +872,13 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
> > >   	dev = obj->dev;
> > > -	if (dev->driver->gem_free_object_unlocked) {
> > > -		kref_put(&obj->refcount, drm_gem_object_free);
> > > -	} else {
> > > +	if (dev->driver->gem_free_object) {
> > >   		might_lock(&dev->struct_mutex);
> > >   		if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> > >   				&dev->struct_mutex))
> > >   			mutex_unlock(&dev->struct_mutex);
> > > +	} else {
> > > +		kref_put(&obj->refcount, drm_gem_object_free);
> > >   	}
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> > > @@ -955,20 +963,30 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> > >   		     struct vm_area_struct *vma)
> > >   {
> > >   	struct drm_device *dev = obj->dev;
> > > +	int ret;
> > >   	/* Check for valid size. */
> > >   	if (obj_size < vma->vm_end - vma->vm_start)
> > >   		return -EINVAL;
> > > -	if (!dev->driver->gem_vm_ops)
> > > +	if (dev->driver->gem_vm_ops)
> > > +		vma->vm_ops = dev->driver->gem_vm_ops;
> > > +	else if (obj->funcs && obj->funcs->vm_ops)
> > > +		vma->vm_ops = obj->funcs->vm_ops;
> > > +	else
> > >   		return -EINVAL;
> > A bit a bikeshed, but if we go with these new ops, I'd put them first in
> > all the if ladders. Makes it a bit clearer while reading that they're the
> > recommended option.
> > >   	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> > > -	vma->vm_ops = dev->driver->gem_vm_ops;
> > >   	vma->vm_private_data = obj;
> > >   	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > >   	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > > +	if (obj->funcs && obj->funcs->mmap) {
> > > +		ret = obj->funcs->mmap(obj, vma);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > I'm confused about this one here ... this is for prime mmap only iirc.
> 
> The smaller drivers that I've looked at combine the DRM mmap and PRIME
> mmap paths going into a shared *_mmap_obj() function.
> 
> This is what I'm doing:
> 
> DRM fd: fops->mmap = drm_gem_mmap
>     -> drm_gem_mmap_obj
>         -> gem->funcs->mmap
> 
> dmabuf fd: fops->mmap = dma_buf_mmap_internal
>     -> dmabuf->ops->mmap = drm_gem_dmabuf_mmap
>         -> drm_gem_mmap_obj
>             -> gem->funcs->mmap
> 
> Is there a difference to the mapping operation itself whether the mmap
> happens through DRM dev or through dmabuf?

Prime gives you the additional sync ioctl stuff. Which isn't wired up at
all for this gem midlayer, so dunno. Tbh I wouldn't add more functions to
gem_funcs that map to dma_buf functions 1:1, that's just indirection for no
gain (the only reason we have these is EXPORT_SYMBOL_GPL not working well
with the nvidia blob, which I still have a grudge with Dave Airlie about).

I think a default implementation that just remaps dma-buf mmap to gem mmap
for everyone is perfectly fine. For anyone doing write-combined mmap (or
dumb buffers in general) it's the right thing to do.

The problem I'm seeing here is that we now have 2 ways to do gem mmap:
Through your gem_funcs->mmap callback, and through the page fault handling
callback (in vm_ops). That's a bit confusion ... Maybe worth it to tackle
the mmap story separately (both gem_funcs->mmap and dma-buf mmap), so
that it's clearer what you're aiming for, and clearer that it's a win?

On the gem_funcs->mmap itself: Can't you use fops->mmap, with adjusted
offsets? That's what e.g. msm does, afaiui. More callbacks doesn't sound
like a great idea to me ...

Cheers, Daniel

> Noralf.
> 
> > > +
> > >   	/* Take a ref for this mapping of the object, so that the fault
> > >   	 * handler can dereference the mmap offset's pointer to the object.
> > >   	 * This reference is cleaned up by the corresponding vm_close
> > > @@ -1068,4 +1086,84 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
> > >   	if (obj->dev->driver->gem_print_info)
> > >   		obj->dev->driver->gem_print_info(p, indent, obj);
> > > +	else if (obj->funcs && obj->funcs->print_info)
> > > +		obj->funcs->print_info(p, indent, obj);
> > >   }
> > > +
> > > +/**
> > > + * drm_gem_pin - Pin backing buffer in memory
> > > + * @obj: GEM object
> > > + *
> > > + * Make sure the backing buffer is pinned in memory.
> > > + *
> > > + * Returns:
> > > + * 0 on success or a negative error code on failure.
> > > + */
> > > +int drm_gem_pin(struct drm_gem_object *obj)
> > > +{
> > > +	if (obj->dev->driver->gem_prime_pin)
> > > +		return obj->dev->driver->gem_prime_pin(obj);
> > > +	else if (obj->funcs && obj->funcs->pin)
> > > +		return obj->funcs->pin(obj);
> > > +	else
> > > +		return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_pin);
> > > +
> > > +/**
> > > + * drm_gem_pin - Unpin backing buffer from memory
> > > + * @obj: GEM object
> > > + *
> > > + * Relax the requirement that the backing buffer is pinned in memory.
> > > + */
> > > +void drm_gem_unpin(struct drm_gem_object *obj)
> > > +{
> > > +	if (obj->dev->driver->gem_prime_unpin)
> > > +		obj->dev->driver->gem_prime_unpin(obj);
> > > +	else if (obj->funcs && obj->funcs->unpin)
> > > +		obj->funcs->unpin(obj);
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_unpin);
> > > +
> > > +/**
> > > + * drm_gem_vmap - Map buffer into kernel virtual address space
> > > + * @obj: GEM object
> > > + *
> > > + * Returns:
> > > + * A virtual pointer to a newly created GEM object or an ERR_PTR-encoded negative
> > > + * error code on failure.
> > > + */
> > > +void *drm_gem_vmap(struct drm_gem_object *obj)
> > > +{
> > > +	void *vaddr;
> > > +
> > > +	if (obj->dev->driver->gem_prime_vmap)
> > > +		vaddr = obj->dev->driver->gem_prime_vmap(obj);
> > > +	else if (obj->funcs && obj->funcs->vmap)
> > > +		vaddr = obj->funcs->vmap(obj);
> > > +	else
> > > +		vaddr = ERR_PTR(-EOPNOTSUPP);
> > > +
> > > +	if (!vaddr)
> > > +		vaddr = ERR_PTR(-ENOMEM);
> > > +
> > > +	return vaddr;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_vmap);
> > > +
> > > +/**
> > > + * drm_gem_vunmap - Remove buffer mapping from kernel virtual address space
> > > + * @obj: GEM object
> > > + * @vaddr: Virtual address (can be NULL)
> > > + */
> > > +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
> > > +{
> > > +	if (!vaddr)
> > > +		return;
> > > +
> > > +	if (obj->dev->driver->gem_prime_vunmap)
> > > +		obj->dev->driver->gem_prime_vunmap(obj, vaddr);
> > > +	else if (obj->funcs && obj->funcs->vunmap)
> > > +		obj->funcs->vunmap(obj, vaddr);
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_vunmap);
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 6721571749ff..7f5c9500136b 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -199,7 +199,6 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
> > >   {
> > >   	struct drm_prime_attachment *prime_attach;
> > >   	struct drm_gem_object *obj = dma_buf->priv;
> > > -	struct drm_device *dev = obj->dev;
> > >   	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
> > >   	if (!prime_attach)
> > > @@ -208,10 +207,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
> > >   	prime_attach->dir = DMA_NONE;
> > >   	attach->priv = prime_attach;
> > > -	if (!dev->driver->gem_prime_pin)
> > > -		return 0;
> > > -
> > > -	return dev->driver->gem_prime_pin(obj);
> > > +	return drm_gem_pin(obj);
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_map_attach);
> > > @@ -228,7 +224,6 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
> > >   {
> > >   	struct drm_prime_attachment *prime_attach = attach->priv;
> > >   	struct drm_gem_object *obj = dma_buf->priv;
> > > -	struct drm_device *dev = obj->dev;
> > >   	if (prime_attach) {
> > >   		struct sg_table *sgt = prime_attach->sgt;
> > > @@ -247,8 +242,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
> > >   		attach->priv = NULL;
> > >   	}
> > > -	if (dev->driver->gem_prime_unpin)
> > > -		dev->driver->gem_prime_unpin(obj);
> > > +	drm_gem_unpin(obj);
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_map_detach);
> > > @@ -310,7 +304,10 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> > >   	if (WARN_ON(prime_attach->dir != DMA_NONE))
> > >   		return ERR_PTR(-EBUSY);
> > > -	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > I think if we just check for obj->funcs here and then unconditionally
> > call, we avoid a bunch of pointer chasing for the new recommended path :-)
> > 
> > > +	if (obj->dev->driver->gem_prime_get_sg_table)
> > > +		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > > +	else
> > > +		sgt = obj->funcs->get_sg_table(obj);
> > >   	if (!IS_ERR(sgt)) {
> > >   		if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
> > > @@ -406,12 +403,13 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
> > >   void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > >   {
> > >   	struct drm_gem_object *obj = dma_buf->priv;
> > > -	struct drm_device *dev = obj->dev;
> > > +	void *vaddr;
> > > -	if (dev->driver->gem_prime_vmap)
> > > -		return dev->driver->gem_prime_vmap(obj);
> > > -	else
> > > -		return NULL;
> > > +	vaddr = drm_gem_vmap(obj);
> > > +	if (IS_ERR(vaddr))
> > > +		vaddr = NULL;
> > > +
> > > +	return vaddr;
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
> > > @@ -426,10 +424,8 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
> > >   void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> > >   {
> > >   	struct drm_gem_object *obj = dma_buf->priv;
> > > -	struct drm_device *dev = obj->dev;
> > > -	if (dev->driver->gem_prime_vunmap)
> > > -		dev->driver->gem_prime_vunmap(obj, vaddr);
> > > +	drm_gem_vunmap(obj, vaddr);
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
> > > @@ -476,10 +472,12 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
> > >   	struct drm_gem_object *obj = dma_buf->priv;
> > >   	struct drm_device *dev = obj->dev;
> > > -	if (!dev->driver->gem_prime_mmap)
> > > +	if (dev->driver->gem_prime_mmap)
> > > +		return dev->driver->gem_prime_mmap(obj, vma);
> > > +	else if (obj->funcs && obj->funcs->mmap)
> > > +		return drm_gem_mmap_obj(obj, obj->size, vma);
> > > +	else
> > >   		return -ENOSYS;
> > > -
> > > -	return dev->driver->gem_prime_mmap(obj, vma);
> > >   }
> > >   EXPORT_SYMBOL(drm_gem_dmabuf_mmap);
> > > @@ -561,6 +559,8 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> > >   	if (dev->driver->gem_prime_export)
> > >   		dmabuf = dev->driver->gem_prime_export(dev, obj, flags);
> > > +	else if (obj->funcs && obj->funcs->export)
> > > +		dmabuf = obj->funcs->export(obj, flags);
> > >   	else
> > >   		dmabuf = drm_gem_prime_export(dev, obj, flags);
> > >   	if (IS_ERR(dmabuf)) {
> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index 3583b98a1718..3ae3665bf4e7 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -38,6 +38,130 @@
> > >   #include <drm/drm_vma_manager.h>
> > > +struct drm_gem_object;
> > > +
> > > +/**
> > > + * struct drm_gem_object_funcs - GEM object functions
> > > + */
> > > +struct drm_gem_object_funcs {
> > > +	/**
> > > +	 * @free:
> > > +	 *
> > > +	 * Deconstructor for drm_gem_objects.
> > > +	 *
> > > +	 * This callback is mandatory.
> > > +	 */
> > > +	void (*free)(struct drm_gem_object *obj);
> > > +
> > > +	/**
> > > +	 * @open:
> > > +	 *
> > > +	 * Called upon GEM handle creation.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	int (*open)(struct drm_gem_object *obj, struct drm_file *file);
> > > +
> > > +	/**
> > > +	 * @close_object:
> > @close:
> > 
> > > +	 *
> > > +	 * Called upon GEM handle release.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	void (*close)(struct drm_gem_object *obj, struct drm_file *file);
> > > +
> > > +	/**
> > > +	 * @print_info:
> > > +	 *
> > > +	 * If driver subclasses struct &drm_gem_object, it can implement this
> > > +	 * optional hook for printing additional driver specific info.
> > > +	 *
> > > +	 * drm_printf_indent() should be used in the callback passing it the
> > > +	 * indent argument.
> > > +	 *
> > > +	 * This callback is called from drm_gem_print_info().
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	void (*print_info)(struct drm_printer *p, unsigned int indent,
> > > +			   const struct drm_gem_object *obj);
> > > +
> > > +	/**
> > > +	 * @export:
> > > +	 *
> > > +	 * Export backing buffer as a &dma_buf.
> > > +	 * If this is not set drm_gem_prime_export() is used.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
> > > +
> > > +	/*
> > > +	 * @pin:
> > > +	 *
> > > +	 * Pin backing buffer in memory.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	int (*pin)(struct drm_gem_object *obj);
> > > +
> > > +	/*
> > > +	 * @unpin:
> > > +	 *
> > > +	 * Unpin backing buffer.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	void (*unpin)(struct drm_gem_object *obj);
> > > +
> > > +	/*
> > > +	 * @get_sg_table:
> > > +	 *
> > > +	 * Returns a Scatter-Gather table representation of the buffer.
> > > +	 * Used when exporting a buffer.
> > > +	 *
> > > +	 * This callback is mandatory if buffer export is supported.
> > > +	 */
> > > +	struct sg_table *(*get_sg_table)(struct drm_gem_object *obj);
> > > +
> > > +	/*
> > > +	 * @vmap:
> > > +	 *
> > > +	 * Returns a virtual address for the buffer.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	void *(*vmap)(struct drm_gem_object *obj);
> > > +
> > > +	/*
> > > +	 * @vunmap:
> > > +	 *
> > > +	 * Releases the the address previously returned by @vmap.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	void (*vunmap)(struct drm_gem_object *obj, void *vaddr);
> > > +
> > > +	/*
> > > +	 * @mmap:
> > > +	 *
> > > +	 * Setup userspace mapping with the given vma.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 */
> > > +	int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > > +
> > > +	/**
> > > +	 * @vm_ops:
> > > +	 *
> > > +	 * Virtual memory operations used with mmap.
> > > +	 *
> > > +	 * This is optional but necessary for mmap support.
> > > +	 */
> > > +	const struct vm_operations_struct *vm_ops;
> > > +};
> > I think fleshing out the docs a bit is a good opportunity here (and adding
> > links to the old place that the new ones recommended). But probably best
> > in follow-up patches.
> > 
> > > +
> > >   /**
> > >    * struct drm_gem_object - GEM buffer object
> > >    *
> > > @@ -146,6 +270,15 @@ struct drm_gem_object {
> > >   	 * simply leave it as NULL.
> > >   	 */
> > >   	struct dma_buf_attachment *import_attach;
> > > +
> > > +	/*
> > > +	 * @funcs:
> > > +	 *
> > > +	 * Optional GEM object functions. If this is set, it will be used if the
> > > +	 * corresponding &drm_driver GEM callback is not set.
> > Maybe also here a note that this is recommended over the callbacks in
> > &drm_driver.
> > 
> > Aside from all the tiny style suggestions, I really like this.
> > -Daniel
> > 
> > > +	 *
> > > +	 */
> > > +	const struct drm_gem_object_funcs *funcs;
> > >   };
> > >   /**
> > > @@ -293,4 +426,9 @@ int drm_gem_dumb_destroy(struct drm_file *file,
> > >   			 struct drm_device *dev,
> > >   			 uint32_t handle);
> > > +int drm_gem_pin(struct drm_gem_object *obj);
> > > +void drm_gem_unpin(struct drm_gem_object *obj);
> > > +void *drm_gem_vmap(struct drm_gem_object *obj);
> > > +void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
> > > +
> > >   #endif /* __DRM_GEM_H__ */
> > > -- 
> > > 2.15.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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

[RFC 3/3] drm/cma: Use drm_gem_object_funcs

[Noralf Trønnes]

This is just a showcase for the drm/gem patch, not to be applied.
vc4 has it's own mmap function so I just bundled it in for a quick hack.

The drivers that use the CMA helper should in theory be able to drop it's
drm_driver GEM callbacks (not dumb_create and import) and use the GEM
attached vtable.

There might be driver quirks like for vc4 that results in breakage, so
there's most likely problems lurking here wrt converting the CMA helper.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 99 +++++++++++++-----------------------
 drivers/gpu/drm/vc4/vc4_bo.c         | 46 ++++++++---------
 drivers/gpu/drm/vc4/vc4_drv.c        | 26 +---------
 drivers/gpu/drm/vc4/vc4_drv.h        |  6 +--
 include/drm/drm_gem_cma_helper.h     | 17 +------
 5 files changed, 59 insertions(+), 135 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 1d2ced882b66..950ebf9fb482 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -40,6 +40,36 @@
  * display drivers that are unable to map scattered buffers via an IOMMU.
  */
 
+static int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
+{
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+	int ret;
+
+	/*
+	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
+	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
+	 * the whole buffer.
+	 */
+	vma->vm_flags &= ~VM_PFNMAP;
+	vma->vm_pgoff = 0;
+
+	ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
+			  cma_obj->paddr, vma->vm_end - vma->vm_start);
+	if (ret)
+		drm_gem_vm_close(vma);
+
+	return ret;
+}
+
+static const struct drm_gem_object_funcs drm_gem_cma_funcs = {
+	.free = drm_gem_cma_free_object,
+	.print_info = drm_gem_cma_print_info,
+	.get_sg_table = drm_gem_cma_prime_get_sg_table,
+	.vmap = drm_gem_cma_prime_vmap,
+	.mmap = drm_gem_cma_mmap,
+	.vm_ops = &drm_gem_cma_vm_ops,
+};
+
 /**
  * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
  * @drm: DRM device
@@ -67,6 +97,9 @@ __drm_gem_cma_create(struct drm_device *drm, size_t size)
 		return ERR_PTR(-ENOMEM);
 	cma_obj = container_of(gem_obj, struct drm_gem_cma_object, base);
 
+	if (!gem_obj->funcs)
+		gem_obj->funcs = &drm_gem_cma_funcs;
+
 	ret = drm_gem_object_init(drm, gem_obj, size);
 	if (ret)
 		goto error;
@@ -270,62 +303,6 @@ const struct vm_operations_struct drm_gem_cma_vm_ops = {
 };
 EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
 
-static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
-				struct vm_area_struct *vma)
-{
-	int ret;
-
-	/*
-	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
-	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
-	 * the whole buffer.
-	 */
-	vma->vm_flags &= ~VM_PFNMAP;
-	vma->vm_pgoff = 0;
-
-	ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
-			  cma_obj->paddr, vma->vm_end - vma->vm_start);
-	if (ret)
-		drm_gem_vm_close(vma);
-
-	return ret;
-}
-
-/**
- * drm_gem_cma_mmap - memory-map a CMA GEM object
- * @filp: file object
- * @vma: VMA for the area to be mapped
- *
- * This function implements an augmented version of the GEM DRM file mmap
- * operation for CMA objects: In addition to the usual GEM VMA setup it
- * immediately faults in the entire object instead of using on-demaind
- * faulting. Drivers which employ the CMA helpers should use this function
- * as their ->mmap() handler in the DRM device file's file_operations
- * structure.
- *
- * Instead of directly referencing this function, drivers should use the
- * DEFINE_DRM_GEM_CMA_FOPS().macro.
- *
- * Returns:
- * 0 on success or a negative error code on failure.
- */
-int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	struct drm_gem_cma_object *cma_obj;
-	struct drm_gem_object *gem_obj;
-	int ret;
-
-	ret = drm_gem_mmap(filp, vma);
-	if (ret)
-		return ret;
-
-	gem_obj = vma->vm_private_data;
-	cma_obj = to_drm_gem_cma_obj(gem_obj);
-
-	return drm_gem_cma_mmap_obj(cma_obj, vma);
-}
-EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
-
 #ifndef CONFIG_MMU
 /**
  * drm_gem_cma_get_unmapped_area - propose address for mapping in noMMU cases
@@ -525,15 +502,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
 int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 			   struct vm_area_struct *vma)
 {
-	struct drm_gem_cma_object *cma_obj;
-	int ret;
-
-	ret = drm_gem_mmap_obj(obj, obj->size, vma);
-	if (ret < 0)
-		return ret;
-
-	cma_obj = to_drm_gem_cma_obj(obj);
-	return drm_gem_cma_mmap_obj(cma_obj, vma);
+	return drm_gem_mmap_obj(obj, obj->size, vma);
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
 
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 8dcce7182bb7..e65f4bb70ba0 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -402,6 +402,22 @@ static struct vc4_bo *vc4_bo_get_from_cache(struct drm_device *dev,
 	return bo;
 }
 
+static const struct vm_operations_struct vc4_vm_ops = {
+	.fault = vc4_fault,
+	.open = drm_gem_vm_open,
+	.close = drm_gem_vm_close,
+};
+
+static const struct drm_gem_object_funcs vc4_gem_funcs = {
+	.free = vc4_free_object,
+	.print_info = drm_gem_cma_print_info,
+	.export = vc4_prime_export,
+	.get_sg_table = drm_gem_cma_prime_get_sg_table,
+	.vmap = vc4_prime_vmap,
+	.mmap = vc4_mmap,
+	.vm_ops = &vc4_vm_ops,
+};
+
 /**
  * vc4_gem_create_object - Implementation of driver->gem_create_object.
  * @dev: DRM device
@@ -429,6 +445,7 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size)
 	mutex_unlock(&vc4->bo_lock);
 	bo->resv = &bo->_resv;
 	reservation_object_init(bo->resv);
+	bo->base.base.funcs = &vc4_gem_funcs;
 
 	return &bo->base.base;
 }
@@ -691,8 +708,7 @@ struct reservation_object *vc4_prime_res_obj(struct drm_gem_object *obj)
 	return bo->resv;
 }
 
-struct dma_buf *
-vc4_prime_export(struct drm_device *dev, struct drm_gem_object *obj, int flags)
+struct dma_buf *vc4_prime_export(struct drm_gem_object *obj, int flags)
 {
 	struct vc4_bo *bo = to_vc4_bo(obj);
 	struct dma_buf *dmabuf;
@@ -714,7 +730,7 @@ vc4_prime_export(struct drm_device *dev, struct drm_gem_object *obj, int flags)
 		return ERR_PTR(ret);
 	}
 
-	dmabuf = drm_gem_prime_export(dev, obj, flags);
+	dmabuf = drm_gem_prime_export(obj->dev, obj, flags);
 	if (IS_ERR(dmabuf))
 		vc4_bo_dec_usecnt(bo);
 
@@ -737,20 +753,12 @@ vm_fault_t vc4_fault(struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
-int vc4_mmap(struct file *filp, struct vm_area_struct *vma)
+int vc4_mmap(struct drm_gem_object *gem_obj, struct vm_area_struct *vma)
 {
-	struct drm_gem_object *gem_obj;
 	unsigned long vm_pgoff;
-	struct vc4_bo *bo;
+	struct vc4_bo *bo = to_vc4_bo(gem_obj);
 	int ret;
 
-	ret = drm_gem_mmap(filp, vma);
-	if (ret)
-		return ret;
-
-	gem_obj = vma->vm_private_data;
-	bo = to_vc4_bo(gem_obj);
-
 	if (bo->validated_shader && (vma->vm_flags & VM_WRITE)) {
 		DRM_DEBUG("mmaping of shader BOs for writing not allowed.\n");
 		return -EINVAL;
@@ -792,18 +800,6 @@ int vc4_mmap(struct file *filp, struct vm_area_struct *vma)
 	return ret;
 }
 
-int vc4_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
-{
-	struct vc4_bo *bo = to_vc4_bo(obj);
-
-	if (bo->validated_shader && (vma->vm_flags & VM_WRITE)) {
-		DRM_DEBUG("mmaping of shader BOs for writing not allowed.\n");
-		return -EINVAL;
-	}
-
-	return drm_gem_cma_prime_mmap(obj, vma);
-}
-
 void *vc4_prime_vmap(struct drm_gem_object *obj)
 {
 	struct vc4_bo *bo = to_vc4_bo(obj);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index e2a15c63a81f..dca115234ec4 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -133,23 +133,7 @@ static void vc4_close(struct drm_device *dev, struct drm_file *file)
 	kfree(vc4file);
 }
 
-static const struct vm_operations_struct vc4_vm_ops = {
-	.fault = vc4_fault,
-	.open = drm_gem_vm_open,
-	.close = drm_gem_vm_close,
-};
-
-static const struct file_operations vc4_drm_fops = {
-	.owner = THIS_MODULE,
-	.open = drm_open,
-	.release = drm_release,
-	.unlocked_ioctl = drm_ioctl,
-	.mmap = vc4_mmap,
-	.poll = drm_poll,
-	.read = drm_read,
-	.compat_ioctl = drm_compat_ioctl,
-	.llseek = noop_llseek,
-};
+DEFINE_DRM_GEM_FOPS(vc4_drm_fops);
 
 static const struct drm_ioctl_desc vc4_drm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(VC4_SUBMIT_CL, vc4_submit_cl_ioctl, DRM_RENDER_ALLOW),
@@ -194,19 +178,11 @@ static struct drm_driver vc4_drm_driver = {
 #endif
 
 	.gem_create_object = vc4_create_object,
-	.gem_free_object_unlocked = vc4_free_object,
-	.gem_vm_ops = &vc4_vm_ops,
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
-	.gem_prime_import = drm_gem_prime_import,
-	.gem_prime_export = vc4_prime_export,
 	.gem_prime_res_obj = vc4_prime_res_obj,
-	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
 	.gem_prime_import_sg_table = vc4_prime_import_sg_table,
-	.gem_prime_vmap = vc4_prime_vmap,
-	.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
-	.gem_prime_mmap = vc4_prime_mmap,
 
 	.dumb_create = vc4_dumb_create,
 
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bd6ef1f31822..6ffebc81290c 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -660,8 +660,7 @@ struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t size,
 int vc4_dumb_create(struct drm_file *file_priv,
 		    struct drm_device *dev,
 		    struct drm_mode_create_dumb *args);
-struct dma_buf *vc4_prime_export(struct drm_device *dev,
-				 struct drm_gem_object *obj, int flags);
+struct dma_buf *vc4_prime_export(struct drm_gem_object *obj, int flags);
 int vc4_create_bo_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
@@ -677,9 +676,8 @@ int vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
 int vc4_label_bo_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv);
 vm_fault_t vc4_fault(struct vm_fault *vmf);
-int vc4_mmap(struct file *filp, struct vm_area_struct *vma);
+int vc4_mmap(struct drm_gem_object *gem_obj, struct vm_area_struct *vma);
 struct reservation_object *vc4_prime_res_obj(struct drm_gem_object *obj);
-int vc4_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
 struct drm_gem_object *vc4_prime_import_sg_table(struct drm_device *dev,
 						 struct dma_buf_attachment *attach,
 						 struct sg_table *sgt);
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 19777145cf8e..d87ed8ba3d23 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -46,19 +46,7 @@ struct drm_gem_cma_object {
  * non-static version of this you're probably doing it wrong and will break the
  * THIS_MODULE reference by accident.
  */
-#define DEFINE_DRM_GEM_CMA_FOPS(name) \
-	static const struct file_operations name = {\
-		.owner		= THIS_MODULE,\
-		.open		= drm_open,\
-		.release	= drm_release,\
-		.unlocked_ioctl	= drm_ioctl,\
-		.compat_ioctl	= drm_compat_ioctl,\
-		.poll		= drm_poll,\
-		.read		= drm_read,\
-		.llseek		= noop_llseek,\
-		.mmap		= drm_gem_cma_mmap,\
-		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
-	}
+#define DEFINE_DRM_GEM_CMA_FOPS(name) DEFINE_DRM_GEM_FOPS(name)
 
 /* free GEM object */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
@@ -73,9 +61,6 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 			    struct drm_device *drm,
 			    struct drm_mode_create_dumb *args);
 
-/* set vm_flags and we can change the VM attribute to other one at here */
-int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
-
 /* allocate physical memory */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 					      size_t size);
-- 
2.15.1

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

Re: [RFC 0/3] drm/gem: Add drm_gem_object_funcs

[Daniel Vetter]

On Fri, Sep 21, 2018 at 06:42:27PM +0200, Noralf Trønnes wrote:
> Hi,
> 
> I've found it odd that the GEM object has its callbacks on drm_driver
> and not a vtable of its own. But something being odd isn't enough to
> make a change (me thinks).
> 
> After working on the GEM shmem helper I saw that a few drivers have
> runtime support for 2 memory types for their buffers (shmem,vram,cma).
> I have realised that if the shmem helper was self contained wrt the
> callbacks, it would be easier for these types of drivers to use the
> helper. All they needed to do was to determine the buffer type on GEM
> object creation time and let the helper handle the rest of the
> callbacks.
> 
> No sure if this makes sense or if the approach is to simplistic. Hence
> the RFC.
> 
> I've added a patch to give an example of how this would look for the CMA
> helper and vc4 (I'm not volunteering to refactor the CMA helper and
> drivers).

I very much like, the drm_driver dumping ground has been a sore spot for
me for a long time, and the per-object callback structures look so much
nicer in kms. That's why I also really liked moving all the vblank stuff
from drm_driver to the drm_crtc_(helper_)funcs struct.

Cheers, Daniel
> 
> Noralf.
> 
> Noralf Trønnes (3):
>   drm/driver: Add defaults for .gem_prime_export/import callbacks
>   drm/gem: Add drm_gem_object_funcs
>   drm/cma: Use drm_gem_object_funcs
> 
>  Documentation/gpu/todo.rst           |   7 ++
>  drivers/gpu/drm/drm_client.c         |  12 ++-
>  drivers/gpu/drm/drm_fb_helper.c      |   8 +-
>  drivers/gpu/drm/drm_gem.c            | 108 +++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_gem_cma_helper.c |  99 +++++++++----------------
>  drivers/gpu/drm/drm_prime.c          |  50 +++++++------
>  drivers/gpu/drm/vc4/vc4_bo.c         |  46 ++++++------
>  drivers/gpu/drm/vc4/vc4_drv.c        |  26 +------
>  drivers/gpu/drm/vc4/vc4_drv.h        |   6 +-
>  include/drm/drm_drv.h                |   4 +
>  include/drm/drm_gem.h                | 138 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_gem_cma_helper.h     |  17 +----
>  12 files changed, 350 insertions(+), 171 deletions(-)
> 
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: [RFC 0/3] drm/gem: Add drm_gem_object_funcs

[Noralf Trønnes]

Den 22.09.2018 12.04, skrev Daniel Vetter:
> On Fri, Sep 21, 2018 at 06:42:27PM +0200, Noralf Trønnes wrote:
>> Hi,
>>
>> I've found it odd that the GEM object has its callbacks on drm_driver
>> and not a vtable of its own. But something being odd isn't enough to
>> make a change (me thinks).
>>
>> After working on the GEM shmem helper I saw that a few drivers have
>> runtime support for 2 memory types for their buffers (shmem,vram,cma).
>> I have realised that if the shmem helper was self contained wrt the
>> callbacks, it would be easier for these types of drivers to use the
>> helper. All they needed to do was to determine the buffer type on GEM
>> object creation time and let the helper handle the rest of the
>> callbacks.
>>
>> No sure if this makes sense or if the approach is to simplistic. Hence
>> the RFC.
>>
>> I've added a patch to give an example of how this would look for the CMA
>> helper and vc4 (I'm not volunteering to refactor the CMA helper and
>> drivers).
> I very much like, the drm_driver dumping ground has been a sore spot for
> me for a long time, and the per-object callback structures look so much
> nicer in kms. That's why I also really liked moving all the vblank stuff
> from drm_driver to the drm_crtc_(helper_)funcs struct.

Cool, let's make this happen :-)

Noralf.

> Cheers, Daniel
>> Noralf.
>>
>> Noralf Trønnes (3):
>>    drm/driver: Add defaults for .gem_prime_export/import callbacks
>>    drm/gem: Add drm_gem_object_funcs
>>    drm/cma: Use drm_gem_object_funcs
>>
>>   Documentation/gpu/todo.rst           |   7 ++
>>   drivers/gpu/drm/drm_client.c         |  12 ++-
>>   drivers/gpu/drm/drm_fb_helper.c      |   8 +-
>>   drivers/gpu/drm/drm_gem.c            | 108 +++++++++++++++++++++++++--
>>   drivers/gpu/drm/drm_gem_cma_helper.c |  99 +++++++++----------------
>>   drivers/gpu/drm/drm_prime.c          |  50 +++++++------
>>   drivers/gpu/drm/vc4/vc4_bo.c         |  46 ++++++------
>>   drivers/gpu/drm/vc4/vc4_drv.c        |  26 +------
>>   drivers/gpu/drm/vc4/vc4_drv.h        |   6 +-
>>   include/drm/drm_drv.h                |   4 +
>>   include/drm/drm_gem.h                | 138 +++++++++++++++++++++++++++++++++++
>>   include/drm/drm_gem_cma_helper.h     |  17 +----
>>   12 files changed, 350 insertions(+), 171 deletions(-)
>>
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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