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