Hi Oleg, First of all, thank you for keeping working on this feature. You did a great job here. I just have a comments inline.
On 08/21, 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).
>
> This enables kms_prime on vkms.
>
> V2:
> - Rodrigo: styleguide + return code check
>
> Cc: Rodrigo Siqueira <rodrigosiqueiram...@gmail.com>
> Cc: Haneen Mohammed <hamohammed...@gmail.com>
> Cc: Daniel Vetter <dan...@ffwll.ch>
> Cc: Simon Ser <simon....@intel.com>
> Signed-off-by: Oleg Vasilev <oleg.vasi...@intel.com>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 12 +++++++++
> drivers/gpu/drm/vkms/vkms_drv.h | 9 +++++++
> drivers/gpu/drm/vkms/vkms_gem.c | 46 +++++++++++++++++++++++++++++++++
> 3 files changed, 67 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 44ab9f8ef8be..09598f949b9b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -11,7 +11,9 @@
>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
>
> +#include <drm/drm_gem.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_drv.h>
> @@ -103,6 +105,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,
> @@ -157,6 +161,14 @@ static int __init vkms_init(void)
> if (ret)
> goto out_unregister;
>
> + ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> + DMA_BIT_MASK(64));
> +
> + if (ret) {
> + DRM_ERROR("Could not initialize DMA support\n");
> + goto out_fini;
> + }
> +
> vkms_device->drm.irq_enabled = true;
>
> ret = drm_vblank_init(&vkms_device->drm, 1);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 5a95100fa18b..6bf195c91097 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 6489bfe0a149..b7565f48e054 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -1,7 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> +#include <linux/dma-buf.h>
> #include <linux/shmem_fs.h>
> #include <linux/vmalloc.h>
> +#include <drm/drm_prime.h>
>
> #include "vkms_drv.h"
>
> @@ -118,6 +120,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;
> +}
> +
I suppose that any other function or helper do not invoke this function,
am I right?
If so, I recommend you to remove it from this patch. IMHO, add a
function that is not used yet may make the code hard to understand and
maintain.
Thanks
> int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
> struct drm_mode_create_dumb *args)
> {
> @@ -218,3 +239,28 @@ 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.23.0
>
--
Rodrigo Siqueira
Software Engineer, Advanced Micro Devices (AMD)
https://siqueira.tech
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
