diff mbox series

drm/vkms: prime import support

Message ID 20190704085410.12749-1-oleg.vasilev@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/vkms: prime import support | expand

Commit Message

Oleg Vasilev July 4, 2019, 8:54 a.m. UTC
Bring dmabuf sharing through implementing prime_import_sg_table callback.
This will help to validate userspace conformance in prime configurations
without using any actual hardware (e.g. in the cloud).

Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c |  6 +++++
 drivers/gpu/drm/vkms/vkms_drv.h |  9 +++++++
 drivers/gpu/drm/vkms/vkms_gem.c | 46 +++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

Comments

Rodrigo Siqueira July 9, 2019, 1:39 p.m. UTC | #1
Hi Oleg,

First of all, thank you for your patch and for working in this issue.

A few comments inline.

On Thu, Jul 4, 2019 at 5:54 AM Oleg Vasilev <oleg.vasilev@intel.com> wrote:
>
> Bring dmabuf sharing through implementing prime_import_sg_table callback.
> This will help to validate userspace conformance in prime configurations
> without using any actual hardware (e.g. in the cloud).
>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c |  6 +++++
>  drivers/gpu/drm/vkms/vkms_drv.h |  9 +++++++
>  drivers/gpu/drm/vkms/vkms_gem.c | 46 +++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index cc53ef88a331..b71c16d9ca09 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -10,6 +10,7 @@
>   */
>
>  #include <linux/module.h>
> +#include <linux/dma-buf.h>

Maybe I missed something, but I think you don't need to add this
include here since you already included it on vkms_gem.

>  #include <drm/drm_gem.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -96,6 +97,8 @@ static struct drm_driver vkms_driver = {
>         .gem_vm_ops             = &vkms_gem_vm_ops,
>         .gem_free_object_unlocked = vkms_gem_free_object,
>         .get_vblank_timestamp   = vkms_get_vblank_timestamp,
> +       .prime_fd_to_handle     = drm_gem_prime_fd_to_handle,
> +       .gem_prime_import_sg_table = vkms_prime_import_sg_table,
>
>         .name                   = DRIVER_NAME,
>         .desc                   = DRIVER_DESC,
> @@ -147,6 +150,9 @@ static int __init vkms_init(void)
>
>         ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
>                            &vkms_device->platform->dev);
> +
> +       dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> +                                    DMA_BIT_MASK(64));

How about capture the return value from dma_coerce_mask_and_coherent()
and warn the user if something wrong happened? Something like:
ret = dma_coerce_mask_and_coherent(..);

if (ret)
  DRM_WARN("Failed to set dma mask");

Additionally, I would like to suggest you move this code above
drm_dev_init() since there's a return validation of ret in the below
if.

>         if (ret)
>                 goto out_unregister;
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 12b4db7ac641..fb15101c8f3e 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -126,6 +126,9 @@ struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
>                                        u32 *handle,
>                                        u64 size);
>
> +struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
> +                                               u64 size);
> +
>  vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
>
>  int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> @@ -137,6 +140,12 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
>
>  void vkms_gem_vunmap(struct drm_gem_object *obj);
>
> +/* Prime */
> +struct drm_gem_object *
> +vkms_prime_import_sg_table(struct drm_device *dev,
> +                          struct dma_buf_attachment *attach,
> +                          struct sg_table *sg);
> +
>  /* CRC Support */
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
>                                         size_t *count);
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index 69048e73377d..a1b837460f63 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
>
> +#include <linux/dma-buf.h>
>  #include <linux/shmem_fs.h>
>
>  #include "vkms_drv.h"
> @@ -117,6 +118,25 @@ struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
>         return &obj->gem;
>  }
>
> +struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
> +                                               u64 size)
> +{

I did not fully get the idea behind this function in this patch, and
it looks like this function is never invoked. Am I right or I missed
something?

> +       struct vkms_gem_object *obj;
> +
> +       obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> +
> +       if (!obj)
> +               return ERR_PTR(-ENOMEM);
> +
> +       size = roundup(size, PAGE_SIZE);
> +
> +       drm_gem_private_object_init(dev, &obj->gem, size);

Please, correct if I'm wrong, I'm trying to understand where this
function should be placed and when it is invoked. If I correctly
understood, you want to use this function for creating a separate gem
object to be used as a private area to be shared with another device,
am I right? If I am right, I'm wondering if we could reuse or update
some of the functions available in the vkms_gem. If I am wrong, I'm
wondering if we may have synchronization problems for handling
multiples mutex for accessing pages.  Could you help me here?

> +
> +       mutex_init(&obj->pages_lock);
> +
> +       return obj;
> +}
> +
>  int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
>                      struct drm_mode_create_dumb *args)
>  {
> @@ -217,3 +237,29 @@ int vkms_gem_vmap(struct drm_gem_object *obj)
>         mutex_unlock(&vkms_obj->pages_lock);
>         return ret;
>  }
> +
> +struct drm_gem_object *
> +vkms_prime_import_sg_table(struct drm_device *dev,
> +                          struct dma_buf_attachment *attach,
> +                          struct sg_table *sg)
> +{
> +       struct vkms_gem_object *obj;
> +       int npages;
> +
> +       obj = __vkms_gem_create(dev, attach->dmabuf->size);
> +       if (IS_ERR(obj))
> +               return ERR_CAST(obj);
> +
> +       npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> +       DRM_DEBUG_PRIME("Importing %d  pages\n", npages);
> +
> +       obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);

One question, why not use _get_pages() instead of using
kvmalloc_array? Use _get_pages has the advantage of already handling
some errors which may reduce the amount of code in this function.

> +       if (!obj->pages) {
> +               vkms_gem_free_object(&obj->gem);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL,
> +                                        npages);

Why not add npage in the same line?

Thanks you again.

> +       return &obj->gem;
> +}
> --
> 2.22.0
>
Oleg Vasilev July 10, 2019, 8:53 a.m. UTC | #2
Hi Rodrigo,

Thanks for the review.

On Tue, 2019-07-09 at 10:39 -0300, Rodrigo Siqueira wrote:
> Hi Oleg,
> 
> First of all, thank you for your patch and for working in this issue.
> 
> A few comments inline.
> 
> On Thu, Jul 4, 2019 at 5:54 AM Oleg Vasilev <oleg.vasilev@intel.com>
> wrote:
> > Bring dmabuf sharing through implementing prime_import_sg_table
> > callback.
> > This will help to validate userspace conformance in prime
> > configurations
> > without using any actual hardware (e.g. in the cloud).
> > 
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c |  6 +++++
> >  drivers/gpu/drm/vkms/vkms_drv.h |  9 +++++++
> >  drivers/gpu/drm/vkms/vkms_gem.c | 46
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c
> > b/drivers/gpu/drm/vkms/vkms_drv.c
> > index cc53ef88a331..b71c16d9ca09 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -10,6 +10,7 @@
> >   */
> > 
> >  #include <linux/module.h>
> > +#include <linux/dma-buf.h>
> 
> Maybe I missed something, but I think you don't need to add this
> include here since you already included it on vkms_gem.

You are right, we need 

#include <linux/dma-mapping.h>

instead.
 
> 
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> > @@ -96,6 +97,8 @@ static struct drm_driver vkms_driver = {
> >         .gem_vm_ops             = &vkms_gem_vm_ops,
> >         .gem_free_object_unlocked = vkms_gem_free_object,
> >         .get_vblank_timestamp   = vkms_get_vblank_timestamp,
> > +       .prime_fd_to_handle     = drm_gem_prime_fd_to_handle,
> > +       .gem_prime_import_sg_table = vkms_prime_import_sg_table,
> > 
> >         .name                   = DRIVER_NAME,
> >         .desc                   = DRIVER_DESC,
> > @@ -147,6 +150,9 @@ static int __init vkms_init(void)
> > 
> >         ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
> >                            &vkms_device->platform->dev);
> > +
> > +       dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> > +                                    DMA_BIT_MASK(64));
> 
> How about capture the return value from
> dma_coerce_mask_and_coherent()
> and warn the user if something wrong happened? 

I've copypasted this from vgem, where they don't check return value. 
But I guess, DRM_WARN won't hurt.

> Something like:
> ret = dma_coerce_mask_and_coherent(..);
> 
> if (ret)
>   DRM_WARN("Failed to set dma mask");
> 
> Additionally, I would like to suggest you move this code above
> drm_dev_init() since there's a return validation of ret in the below
> if.

Sure.

> 
> >         if (ret)
> >                 goto out_unregister;
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h
> > b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 12b4db7ac641..fb15101c8f3e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -126,6 +126,9 @@ struct drm_gem_object *vkms_gem_create(struct
> > drm_device *dev,
> >                                        u32 *handle,
> >                                        u64 size);
> > 
> > +struct vkms_gem_object *vkms_gem_create_private(struct drm_device
> > *dev,
> > +                                               u64 size);
> > +
> >  vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
> > 
> >  int vkms_dumb_create(struct drm_file *file, struct drm_device
> > *dev,
> > @@ -137,6 +140,12 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
> > 
> >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> > 
> > +/* Prime */
> > +struct drm_gem_object *
> > +vkms_prime_import_sg_table(struct drm_device *dev,
> > +                          struct dma_buf_attachment *attach,
> > +                          struct sg_table *sg);
> > +
> >  /* CRC Support */
> >  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> >                                         size_t *count);
> > diff --git a/drivers/gpu/drm/vkms/vkms_gem.c
> > b/drivers/gpu/drm/vkms/vkms_gem.c
> > index 69048e73377d..a1b837460f63 100644
> > --- a/drivers/gpu/drm/vkms/vkms_gem.c
> > +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> > 
> > +#include <linux/dma-buf.h>
> >  #include <linux/shmem_fs.h>
> > 
> >  #include "vkms_drv.h"
> > @@ -117,6 +118,25 @@ struct drm_gem_object *vkms_gem_create(struct
> > drm_device *dev,
> >         return &obj->gem;
> >  }
> > 
> > +struct vkms_gem_object *vkms_gem_create_private(struct drm_device
> > *dev,
> > +                                               u64 size)
> > +{
> 
> I did not fully get the idea behind this function in this patch, and
> it looks like this function is never invoked. Am I right or I missed
> something?

That's true. I've initially intended to use it, but haven't. I keep it
in the patch for the sake of consistency, since other drivers have it.
I am not sure we have any usage for it at the moment. Should I remove
it from the patch?

> 
> > +       struct vkms_gem_object *obj;
> > +
> > +       obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > +
> > +       if (!obj)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       size = roundup(size, PAGE_SIZE);
> > +
> > +       drm_gem_private_object_init(dev, &obj->gem, size);
> 
> Please, correct if I'm wrong, I'm trying to understand where this
> function should be placed and when it is invoked. If I correctly
> understood, you want to use this function for creating a separate gem
> object to be used as a private area to be shared with another device,
> am I right? If I am right, I'm wondering if we could reuse or update
> some of the functions available in the vkms_gem. If I am wrong, I'm
> wondering if we may have synchronization problems for handling
> multiples mutex for accessing pages.  Could you help me here?

This private objects are used in i915 to handle prime import. It
creates a private object, and then overwrites get/put_pages functions
so that pages would come from the right place.  

> > +       mutex_init(&obj->pages_lock);
> > +
> > +       return obj;
> > +}
> > +
> >  int vkms_dumb_create(struct drm_file *file, struct drm_device
> > *dev,
> >                      struct drm_mode_create_dumb *args)
> >  {
> > @@ -217,3 +237,29 @@ int vkms_gem_vmap(struct drm_gem_object *obj)
> >         mutex_unlock(&vkms_obj->pages_lock);
> >         return ret;
> >  }
> > +
> > +struct drm_gem_object *
> > +vkms_prime_import_sg_table(struct drm_device *dev,
> > +                          struct dma_buf_attachment *attach,
> > +                          struct sg_table *sg)
> > +{
> > +       struct vkms_gem_object *obj;
> > +       int npages;
> > +
> > +       obj = __vkms_gem_create(dev, attach->dmabuf->size);
> > +       if (IS_ERR(obj))
> > +               return ERR_CAST(obj);
> > +
> > +       npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> > +       DRM_DEBUG_PRIME("Importing %d  pages\n", npages);
> > +
> > +       obj->pages = kvmalloc_array(npages, sizeof(struct page *),
> > GFP_KERNEL);
> 
> One question, why not use _get_pages() instead of using
> kvmalloc_array? Use _get_pages has the advantage of already handling
> some errors which may reduce the amount of code in this function.

drm_gem_get_pages also allocates the pages itself. Here we need only
the array of pointers, since pages come from the imported object.  

> 
> > +       if (!obj->pages) {
> > +               vkms_gem_free_object(&obj->gem);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> > +       drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL,
> > +                                        npages);
> 
> Why not add npage in the same line?

You are right.

Oleg

> 
> Thanks you again.
> 
> > +       return &obj->gem;
> > +}
> > --
> > 2.22.0
> >
Daniel Vetter July 10, 2019, 3:31 p.m. UTC | #3
On Thu, Jul 04, 2019 at 11:54:10AM +0300, Oleg Vasilev wrote:
> Bring dmabuf sharing through implementing prime_import_sg_table callback.
> This will help to validate userspace conformance in prime configurations
> without using any actual hardware (e.g. in the cloud).
> 
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>

Btw which igt testcases does this enable? Are the igt patches already
merged (I think as-is the igt prime tests won't run on vkms).

Imo for vkms we really want to make sure there's tests for everything,
otherwise a fake driver for testing/validation is kinda pointless ...
-Daniel

> ---
>  drivers/gpu/drm/vkms/vkms_drv.c |  6 +++++
>  drivers/gpu/drm/vkms/vkms_drv.h |  9 +++++++
>  drivers/gpu/drm/vkms/vkms_gem.c | 46 +++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index cc53ef88a331..b71c16d9ca09 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/dma-buf.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -96,6 +97,8 @@ static struct drm_driver vkms_driver = {
>  	.gem_vm_ops		= &vkms_gem_vm_ops,
>  	.gem_free_object_unlocked = vkms_gem_free_object,
>  	.get_vblank_timestamp	= vkms_get_vblank_timestamp,
> +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> +	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
>  
>  	.name			= DRIVER_NAME,
>  	.desc			= DRIVER_DESC,
> @@ -147,6 +150,9 @@ static int __init vkms_init(void)
>  
>  	ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
>  			   &vkms_device->platform->dev);
> +
> +	dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> +				     DMA_BIT_MASK(64));
>  	if (ret)
>  		goto out_unregister;
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 12b4db7ac641..fb15101c8f3e 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -126,6 +126,9 @@ struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
>  				       u32 *handle,
>  				       u64 size);
>  
> +struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
> +						u64 size);
> +
>  vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
>  
>  int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> @@ -137,6 +140,12 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
>  
>  void vkms_gem_vunmap(struct drm_gem_object *obj);
>  
> +/* Prime */
> +struct drm_gem_object *
> +vkms_prime_import_sg_table(struct drm_device *dev,
> +			   struct dma_buf_attachment *attach,
> +			   struct sg_table *sg);
> +
>  /* CRC Support */
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
>  					size_t *count);
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index 69048e73377d..a1b837460f63 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
> +#include <linux/dma-buf.h>
>  #include <linux/shmem_fs.h>
>  
>  #include "vkms_drv.h"
> @@ -117,6 +118,25 @@ struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
>  	return &obj->gem;
>  }
>  
> +struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
> +						u64 size)
> +{
> +	struct vkms_gem_object *obj;
> +
> +	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> +
> +	if (!obj)
> +		return ERR_PTR(-ENOMEM);
> +
> +	size = roundup(size, PAGE_SIZE);
> +
> +	drm_gem_private_object_init(dev, &obj->gem, size);
> +
> +	mutex_init(&obj->pages_lock);
> +
> +	return obj;
> +}
> +
>  int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		     struct drm_mode_create_dumb *args)
>  {
> @@ -217,3 +237,29 @@ int vkms_gem_vmap(struct drm_gem_object *obj)
>  	mutex_unlock(&vkms_obj->pages_lock);
>  	return ret;
>  }
> +
> +struct drm_gem_object *
> +vkms_prime_import_sg_table(struct drm_device *dev,
> +			   struct dma_buf_attachment *attach,
> +			   struct sg_table *sg)
> +{
> +	struct vkms_gem_object *obj;
> +	int npages;
> +
> +	obj = __vkms_gem_create(dev, attach->dmabuf->size);
> +	if (IS_ERR(obj))
> +		return ERR_CAST(obj);
> +
> +	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> +	DRM_DEBUG_PRIME("Importing %d  pages\n", npages);
> +
> +	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> +	if (!obj->pages) {
> +		vkms_gem_free_object(&obj->gem);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL,
> +					 npages);
> +	return &obj->gem;
> +}
> -- 
> 2.22.0
>
Oleg Vasilev July 10, 2019, 3:35 p.m. UTC | #4
On Wed, 2019-07-10 at 17:31 +0200, Daniel Vetter wrote:
> On Thu, Jul 04, 2019 at 11:54:10AM +0300, Oleg Vasilev wrote:
> > Bring dmabuf sharing through implementing prime_import_sg_table
> > callback.
> > This will help to validate userspace conformance in prime
> > configurations
> > without using any actual hardware (e.g. in the cloud).
> > 
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> 
> Btw which igt testcases does this enable? Are the igt patches already
> merged (I think as-is the igt prime tests won't run on vkms).
> 
> Imo for vkms we really want to make sure there's tests for
> everything,
> otherwise a fake driver for testing/validation is kinda pointless ...
> -Daniel

I've submitted a patch to IGT, but it is not merged yet:
https://patchwork.freedesktop.org/series/63213/

Oleg
> 
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c |  6 +++++
> >  drivers/gpu/drm/vkms/vkms_drv.h |  9 +++++++
> >  drivers/gpu/drm/vkms/vkms_gem.c | 46
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c
> > b/drivers/gpu/drm/vkms/vkms_drv.c
> > index cc53ef88a331..b71c16d9ca09 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -10,6 +10,7 @@
> >   */
> >  
> >  #include <linux/module.h>
> > +#include <linux/dma-buf.h>
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> > @@ -96,6 +97,8 @@ static struct drm_driver vkms_driver = {
> >  	.gem_vm_ops		= &vkms_gem_vm_ops,
> >  	.gem_free_object_unlocked = vkms_gem_free_object,
> >  	.get_vblank_timestamp	= vkms_get_vblank_timestamp,
> > +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> > +	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
> >  
> >  	.name			= DRIVER_NAME,
> >  	.desc			= DRIVER_DESC,
> > @@ -147,6 +150,9 @@ static int __init vkms_init(void)
> >  
> >  	ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
> >  			   &vkms_device->platform->dev);
> > +
> > +	dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> > +				     DMA_BIT_MASK(64));
> >  	if (ret)
> >  		goto out_unregister;
> >  
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h
> > b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 12b4db7ac641..fb15101c8f3e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -126,6 +126,9 @@ struct drm_gem_object *vkms_gem_create(struct
> > drm_device *dev,
> >  				       u32 *handle,
> >  				       u64 size);
> >  
> > +struct vkms_gem_object *vkms_gem_create_private(struct drm_device
> > *dev,
> > +						u64 size);
> > +
> >  vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
> >  
> >  int vkms_dumb_create(struct drm_file *file, struct drm_device
> > *dev,
> > @@ -137,6 +140,12 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
> >  
> >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> >  
> > +/* Prime */
> > +struct drm_gem_object *
> > +vkms_prime_import_sg_table(struct drm_device *dev,
> > +			   struct dma_buf_attachment *attach,
> > +			   struct sg_table *sg);
> > +
> >  /* CRC Support */
> >  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> >  					size_t *count);
> > diff --git a/drivers/gpu/drm/vkms/vkms_gem.c
> > b/drivers/gpu/drm/vkms/vkms_gem.c
> > index 69048e73377d..a1b837460f63 100644
> > --- a/drivers/gpu/drm/vkms/vkms_gem.c
> > +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >  
> > +#include <linux/dma-buf.h>
> >  #include <linux/shmem_fs.h>
> >  
> >  #include "vkms_drv.h"
> > @@ -117,6 +118,25 @@ struct drm_gem_object *vkms_gem_create(struct
> > drm_device *dev,
> >  	return &obj->gem;
> >  }
> >  
> > +struct vkms_gem_object *vkms_gem_create_private(struct drm_device
> > *dev,
> > +						u64 size)
> > +{
> > +	struct vkms_gem_object *obj;
> > +
> > +	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > +
> > +	if (!obj)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	size = roundup(size, PAGE_SIZE);
> > +
> > +	drm_gem_private_object_init(dev, &obj->gem, size);
> > +
> > +	mutex_init(&obj->pages_lock);
> > +
> > +	return obj;
> > +}
> > +
> >  int vkms_dumb_create(struct drm_file *file, struct drm_device
> > *dev,
> >  		     struct drm_mode_create_dumb *args)
> >  {
> > @@ -217,3 +237,29 @@ int vkms_gem_vmap(struct drm_gem_object *obj)
> >  	mutex_unlock(&vkms_obj->pages_lock);
> >  	return ret;
> >  }
> > +
> > +struct drm_gem_object *
> > +vkms_prime_import_sg_table(struct drm_device *dev,
> > +			   struct dma_buf_attachment *attach,
> > +			   struct sg_table *sg)
> > +{
> > +	struct vkms_gem_object *obj;
> > +	int npages;
> > +
> > +	obj = __vkms_gem_create(dev, attach->dmabuf->size);
> > +	if (IS_ERR(obj))
> > +		return ERR_CAST(obj);
> > +
> > +	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
> > +	DRM_DEBUG_PRIME("Importing %d  pages\n", npages);
> > +
> > +	obj->pages = kvmalloc_array(npages, sizeof(struct page *),
> > GFP_KERNEL);
> > +	if (!obj->pages) {
> > +		vkms_gem_free_object(&obj->gem);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL,
> > +					 npages);
> > +	return &obj->gem;
> > +}
> > -- 
> > 2.22.0
> >
Oleg Vasilev July 10, 2019, 3:38 p.m. UTC | #5
On Wed, 2019-07-10 at 18:35 +0300, Oleg Vasilev wrote:
> On Wed, 2019-07-10 at 17:31 +0200, Daniel Vetter wrote:
> > On Thu, Jul 04, 2019 at 11:54:10AM +0300, Oleg Vasilev wrote:
> > > Bring dmabuf sharing through implementing prime_import_sg_table
> > > callback.
> > > This will help to validate userspace conformance in prime
> > > configurations
> > > without using any actual hardware (e.g. in the cloud).
> > > 
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > 
> > Btw which igt testcases does this enable? Are the igt patches
> > already
> > merged (I think as-is the igt prime tests won't run on vkms).
> > 
> > Imo for vkms we really want to make sure there's tests for
> > everything,
> > otherwise a fake driver for testing/validation is kinda pointless
> > ...
> > -Daniel
> 
> I've submitted a patch to IGT, but it is not merged yet:
> https://patchwork.freedesktop.org/series/63213/

Oops, this one: 
https://patchwork.freedesktop.org/patch/315659/?series=63216&rev=1

> Oleg
Daniel Vetter July 10, 2019, 4:39 p.m. UTC | #6
On Wed, Jul 10, 2019 at 03:38:13PM +0000, Vasilev, Oleg wrote:
> On Wed, 2019-07-10 at 18:35 +0300, Oleg Vasilev wrote:
> > On Wed, 2019-07-10 at 17:31 +0200, Daniel Vetter wrote:
> > > On Thu, Jul 04, 2019 at 11:54:10AM +0300, Oleg Vasilev wrote:
> > > > Bring dmabuf sharing through implementing prime_import_sg_table
> > > > callback.
> > > > This will help to validate userspace conformance in prime
> > > > configurations
> > > > without using any actual hardware (e.g. in the cloud).
> > > > 
> > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > > 
> > > Btw which igt testcases does this enable? Are the igt patches
> > > already
> > > merged (I think as-is the igt prime tests won't run on vkms).
> > > 
> > > Imo for vkms we really want to make sure there's tests for
> > > everything,
> > > otherwise a fake driver for testing/validation is kinda pointless
> > > ...
> > > -Daniel
> > 
> > I've submitted a patch to IGT, but it is not merged yet:
> > https://patchwork.freedesktop.org/series/63213/
> 
> Oops, this one: 
> https://patchwork.freedesktop.org/patch/315659/?series=63216&rev=1

Ah, replied there. Let's polish the testcase first, then land the kernel
side.
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index cc53ef88a331..b71c16d9ca09 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -10,6 +10,7 @@ 
  */
 
 #include <linux/module.h>
+#include <linux/dma-buf.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -96,6 +97,8 @@  static struct drm_driver vkms_driver = {
 	.gem_vm_ops		= &vkms_gem_vm_ops,
 	.gem_free_object_unlocked = vkms_gem_free_object,
 	.get_vblank_timestamp	= vkms_get_vblank_timestamp,
+	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
+	.gem_prime_import_sg_table = vkms_prime_import_sg_table,
 
 	.name			= DRIVER_NAME,
 	.desc			= DRIVER_DESC,
@@ -147,6 +150,9 @@  static int __init vkms_init(void)
 
 	ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
 			   &vkms_device->platform->dev);
+
+	dma_coerce_mask_and_coherent(vkms_device->drm.dev,
+				     DMA_BIT_MASK(64));
 	if (ret)
 		goto out_unregister;
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 12b4db7ac641..fb15101c8f3e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -126,6 +126,9 @@  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
 				       u32 *handle,
 				       u64 size);
 
+struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
+						u64 size);
+
 vm_fault_t vkms_gem_fault(struct vm_fault *vmf);
 
 int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
@@ -137,6 +140,12 @@  int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* Prime */
+struct drm_gem_object *
+vkms_prime_import_sg_table(struct drm_device *dev,
+			   struct dma_buf_attachment *attach,
+			   struct sg_table *sg);
+
 /* CRC Support */
 const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
 					size_t *count);
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
index 69048e73377d..a1b837460f63 100644
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ b/drivers/gpu/drm/vkms/vkms_gem.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <linux/dma-buf.h>
 #include <linux/shmem_fs.h>
 
 #include "vkms_drv.h"
@@ -117,6 +118,25 @@  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
 	return &obj->gem;
 }
 
+struct vkms_gem_object *vkms_gem_create_private(struct drm_device *dev,
+						u64 size)
+{
+	struct vkms_gem_object *obj;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	size = roundup(size, PAGE_SIZE);
+
+	drm_gem_private_object_init(dev, &obj->gem, size);
+
+	mutex_init(&obj->pages_lock);
+
+	return obj;
+}
+
 int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
 		     struct drm_mode_create_dumb *args)
 {
@@ -217,3 +237,29 @@  int vkms_gem_vmap(struct drm_gem_object *obj)
 	mutex_unlock(&vkms_obj->pages_lock);
 	return ret;
 }
+
+struct drm_gem_object *
+vkms_prime_import_sg_table(struct drm_device *dev,
+			   struct dma_buf_attachment *attach,
+			   struct sg_table *sg)
+{
+	struct vkms_gem_object *obj;
+	int npages;
+
+	obj = __vkms_gem_create(dev, attach->dmabuf->size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
+
+	npages = PAGE_ALIGN(attach->dmabuf->size) / PAGE_SIZE;
+	DRM_DEBUG_PRIME("Importing %d  pages\n", npages);
+
+	obj->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+	if (!obj->pages) {
+		vkms_gem_free_object(&obj->gem);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	drm_prime_sg_to_page_addr_arrays(sg, obj->pages, NULL,
+					 npages);
+	return &obj->gem;
+}