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@xxxxxxxxx>
> 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@xxxxxxxxx>
> > Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Signed-off-by: Oleg Vasilev <oleg.vasilev@xxxxxxxxx>
> > ---
> > 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
> >
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
