[PATCH 0/8] drm/fb-helper: Use drm_file to get a dumb framebuffer

[Noralf Trønnes]

Den 15.09.2017 00.29, skrev Laurent Pinchart:
> Hi Noralf,
>
> On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
>> Den 13.09.2017 07.09, skrev Laurent Pinchart:
>>> On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
>>>> Hi,
>>>>
>>>> I want to start out by saying that this patchset is low priority for me
>>>> and if no one has interest or time to review this, that is just fine. I
>>>> was in the flow and just typed it out.
>>>>
>>>> This patchset adds a way for fbdev emulation code to create a
>>>> framebuffer that is backed by a dumb buffer. drm_fb_helper gets a
>>>> drm_file to hang the objects on, drm_framebuffer_create_dumb() creates
>>>> the framebuffer and drm_fb_helper_fini() destroys it.
>>>> I have verified that all cma drivers supports dumb buffers, so
>>>> converting the library should be fine for all.
>>> Stupid question, what does this give us ? The series makes the call stack
>>> more complex (up to a point where I'm getting trouble just following it),
>>> what's the advantage that offsets that ?
>> The short answer is that it avoids the need for special fbdev
>> _with_funcs() functions in libraries like cma for framebuffers with the
>> dirty callback set.
>>
>> I'm suprised that you think this more complex, I find it more coherent
>> when fbdev userspace is doing things more like DRM userspace, like
>> hanging the objects on a drm_file and cleaning up the objects when done.
> Maybe moving to a new code base gave me the wrong feeling that the result is
> more complex. The current implementation is certainly not simple.
>
>> The longer and more diffuse answer is that it's annoying me that many
>> drivers carry the burden of fbdev emulation just to get a console!
> I totally agree with that. Ideally I'd like to remove 100% of fbdev-related
> code from drivers. This includes
>
> - initialization and cleanup of fbdev helpers
> - fbdev restore in last_close()
> - forwarding of hotplug events to fbdev compatibility layer
>
> In practice we'll likely need to keep one initialization function (or a
> drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest
> should go away. Or maybe we could even enable fbdev compatibility in all
> drivers unconditionally.

Interesting idea, maybe something like this:

  struct drm_device {
+     struct drm_fb_helper *fbdev;
  };

  void drm_lastclose(struct drm_device * dev)
  {
      DRM_DEBUG("\n");

      if (dev->driver->lastclose)
          dev->driver->lastclose(dev);
+    else if (dev->fbdev)
+        drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev);
      DRM_DEBUG("driver lastclose completed\n");

      if (drm_core_check_feature(dev, DRIVER_LEGACY))
          drm_legacy_dev_reinit(dev);
  }

  void drm_kms_helper_hotplug_event(struct drm_device *dev)
  {
      /* send a uevent + call fbdev */
      drm_sysfs_hotplug_event(dev);
      if (dev->mode_config.funcs->output_poll_changed)
          dev->mode_config.funcs->output_poll_changed(dev);
+    else if (dev->fbdev)
+        drm_fb_helper_hotplug_event(dev->fbdev);
  }

  void drm_dev_unregister(struct drm_device *dev)
  {
     struct drm_map_list *r_list, *list_temp;

+    if (dev->fbdev)
+        drm_fb_helper_unregister_fbi(dev->fbdev);
+
      drm_lastclose(dev);
  ...
  }

  static void drm_dev_release(struct kref *ref)
  {
      struct drm_device *dev = container_of(ref, struct drm_device, ref);

+    drm_fb_helper_simple_fini(dev);
+
      if (dev->driver->release) {
          dev->driver->release(dev);
      } else {
          drm_dev_fini(dev);
          kfree(dev);
      }
  }

int driver_probe()
{
     ret = drm_fb_helper_simple_init(dev, ...);
     if (ret)
         DRM_WARN("Oh well fbdev init failed, but don't let that stop 
us\n");
}

I've slightly adjusted some code I have in the pipeline:

/**
  * drm_fb_helper_simple_init - Simple fbdev emulation init
  * @dev: DRM device
  * @bpp_sel: bpp value to use for the framebuffer configuration
  * @max_conn: max connector count
  * @funcs: pointer to structure of functions associated with this helper
  *
  * Simple fbdev emulation initialization. This function allocates a
  * &drm_fb_helper structure and calls:
  * drm_fb_helper_prepare(), drm_fb_helper_init(),
  * drm_fb_helper_single_add_all_connectors() and
  * drm_fb_helper_initial_config().
  *
  * fbdev deferred I/O users should use drm_fb_helper_defio_init().
  *
  * Returns:
  * Zero on success, negative error code on failure.
  */
int drm_fb_helper_simple_init(struct drm_device *dev, int bpp_sel, int 
max_conn,
                   const struct drm_fb_helper_funcs *funcs)
{
     struct drm_fb_helper *fb_helper;
     int ret;

     fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
     if (!fb_helper)
         return -ENOMEM;

     drm_fb_helper_prepare(dev, fb_helper, funcs);

     ret = drm_fb_helper_init(dev, fb_helper, max_conn);
     if (ret < 0) {
         DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
         goto err_drm_fb_helper_free;
     }

     ret = drm_fb_helper_single_add_all_connectors(fb_helper);
     if (ret < 0) {
         DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
         goto err_drm_fb_helper_fini;

     }

     ret = drm_fb_helper_initial_config(fb_helper, bpp_sel);
     if (ret < 0) {
         DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
         goto err_drm_fb_helper_fini;
     }

     dev->fbdev = fb_helper;

     return 0;

err_drm_fb_helper_fini:
     drm_fb_helper_fini(fb_helper);
err_drm_fb_helper_free:
     kfree(fb_helper);

     return ret;
}
EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);

/**
  * drm_fb_helper_simple_fini - Simple fbdev cleanup
  * @dev: DRM device
  *
  * Simple fbdev emulation cleanup. This function unregisters fbdev if 
it is not
  * done, cleans up deferred IO if necessary, removes framebuffer, finalizes
  * @fb_helper and frees the structure.
  */
void drm_fb_helper_simple_fini(struct drm_device *dev)
{
     struct drm_fb_helper *fb_helper = dev->fbdev;
     struct fb_ops *fbops = NULL;
     struct fb_info *info;

     if (!fb_helper)
         return;

     info = fb_helper->fbdev;

     /* Make sure it hasn't been unregistered already */
     if (info && info->dev)
         drm_fb_helper_unregister_fbi(fb_helper);

     if (info && info->fbdefio) {
         fb_deferred_io_cleanup(info);
         kfree(info->fbdefio);
         info->fbdefio = NULL;
         fbops = info->fbops;
     }

     drm_fb_helper_fini(fb_helper);
     kfree(fbops);

     if (fb_helper->fb)
         drm_framebuffer_remove(fb_helper->fb);

     kfree(fb_helper);
}
EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);

<snip>