Re: [RFC 1/7] drm/gem: Add drm_gem_dumb_map_offset()

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 19.07.2017 23.01, skrev Eric Anholt:
> Noralf Trønnes <noralf@xxxxxxxxxxx> writes:
>
> > Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> Instead of just introducing the new code, could we replace CMA's code
> with calls to this at the same time?

I have gone through all the drm_driver->dumb_map_offset
implementations and found 23 drivers that could use it:
- 18 drivers use drm_gem_cma_dumb_map_offset()
- exynos_drm_gem_dumb_map_offset()
- mtk_drm_gem_dumb_map_offset()
- psb_gem_dumb_map_gtt()
- rockchip_gem_dumb_map_offset()
- tegra_bo_dumb_map_offset()

vgem_gem_dumb_map() can't use it because of this check:
    if (!obj->filp) {
        ret = -EINVAL;
        goto unref;
    }

armada_gem_dumb_map_offset() can't use it because of this check:
    /* Don't allow imported objects to be mapped */
    if (obj->obj.import_attach) {
        ret = -EINVAL;
        goto err_unref;
    }

I see that drivers must implement all 3 .dumb_* callbacks:

 * To support dumb objects drivers must implement the 
&drm_driver.dumb_create,
 * &drm_driver.dumb_destroy and &drm_driver.dumb_map_offset operations. See
 * there for further details.

I'm a fan of defaults, is there any reason we can't do this:

 int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
                  void *data, struct drm_file *file_priv)
 {
     struct drm_mode_map_dumb *args = data;

     /* call driver ioctl to get mmap offset */
-     if (!dev->driver->dumb_map_offset)
+    if (!dev->driver->dumb_create)
        return -ENOSYS;

-     return dev->driver->dumb_map_offset(file_priv, dev, args->handle, 
&args->offset);
+     if (dev->driver->dumb_map_offset)
+        return dev->driver->dumb_map_offset(file_priv, dev, 
args->handle, &args->offset);
+    else
+        return drm_gem_dumb_map_offset(file_priv, dev, args->handle, 
&args->offset);
 }

 int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
                 void *data, struct drm_file *file_priv)
 {
     struct drm_mode_destroy_dumb *args = data;

-     if (!dev->driver->dumb_destroy)
+    if (!dev->driver->dumb_create)
         return -ENOSYS;

-    return dev->driver->dumb_destroy(file_priv, dev, args->handle);
+     if (dev->driver->dumb_destroy)
+        return dev->driver->dumb_destroy(file_priv, dev, args->handle);
+    else
+        return drm_gem_dumb_destroy(file_priv, dev, args->handle);
 }

There are 36 drivers that use drm_gem_dumb_destroy() directly.
vgem violates the docs and doesn't set .dumb_destroy.

> I suspect that if we had CMA subclass from drm_fb_gem (or we move the
> gem objects to the base class), we could remove a lot of its code that
> you're copying in patch 2, as well.

Yes, that was the intention.

Noralf.

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