| Message ID | 20190704085410.12749-1-oleg.vasilev@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: prime import support | expand |
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 >
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 > >
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 >
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 > >
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
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 --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; +}
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(+)