[PATCH 0/8] drm/fb-helper: Use drm_file to get a dumb framebuffer
Noralf Trønnes
noralf at tronnes.org
Sat Sep 16 14:45:42 UTC 2017
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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list