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

Noralf Trønnes
2017-09-16T14:45:42Z

Den 16.09.2017 14.37, skrev Noralf Trønnes:
>
> Den 15.09.2017 18.37, skrev 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:
>>
>
> I just remembered that Daniel said something about the helpers shouldn't
> bleed into the core, which my suggestion does :-/
>
>
>>  struct drm_device {
>> +     struct drm_fb_helper *fbdev;
>>  };
>>

However, other helpers are allowed pointers in core structures, so if we
can do that we can at least add helpers that can be used directly as
callbacks:

/**
* drm_fb_helper_lastclose - DRM driver lastclose helper for fbdev
* @dev: DRM device
*
* This function can be used as the &drm_driver->lastclose callback for
drivers
* that only need to call drm_fb_helper_restore_fbdev_mode_unlocked().
*
* Note: &drm_device->fbdev needs to be set.
*/
void drm_fb_helper_lastclose(struct drm_device *dev)
{
drm_fb_helper_restore_fbdev_mode_unlocked(dev->fbdev);
}
EXPORT_SYMBOL(drm_fb_helper_lastclose);

/**
* drm_fb_helper_output_poll_changed - DRM mode config
\.output_poll_changed
*                                     helper
* @dev: DRM device
*
* This function can be used as the
* &drm_mode_config_funcs.output_poll_changed callback for drivers that
only
* need to call drm_fb_helper_hotplug_event().
*
* Note: &drm_device->fbdev needs to be set.
*/
void drm_fb_helper_output_poll_changed(struct drm_device *dev)
{
drm_fb_helper_hotplug_event(dev->fbdev);
}
EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);

>>  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>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>