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

Noralf Trønnes
2017-09-15T16:37:09Z

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>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org