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

[Noralf Trønnes]

Den 19.07.2017 23.01, skrev Eric Anholt:
> Noralf Trønnes <noralf at tronnes.org> writes:
>
>> Add a common drm_driver.dumb_map_offset function for GEM backed drivers.
>>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> 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.