[PATCH v3 2/3] drm: simpledrm: add fbdev fallback support
Noralf Trønnes
noralf at tronnes.org
Tue Aug 16 13:10:06 UTC 2016
Den 15.08.2016 08:48, skrev Daniel Vetter:
> On Sun, Aug 14, 2016 at 06:52:05PM +0200, Noralf Trønnes wrote:
>> Create a simple fbdev device during SimpleDRM setup so legacy user-space
>> and fbcon can use it.
>>
>> Original work by David Herrmann.
>>
>> Cc: dh.herrmann at gmail.com
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>>
>> Changes from version 2:
>> - Switch to using drm_fb_helper in preparation for future panic handling
>> which needs an enabled pipeline.
>>
>> Changes from version 1:
>> No changes
>>
>> Changes from previous version:
>> - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
>> - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
>> - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
>>
>> drivers/gpu/drm/simpledrm/Kconfig | 3 +
>> drivers/gpu/drm/simpledrm/Makefile | 1 +
>> drivers/gpu/drm/simpledrm/simpledrm.h | 32 ++++-
>> drivers/gpu/drm/simpledrm/simpledrm_drv.c | 4 +
>> drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 201 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/simpledrm/simpledrm_kms.c | 10 +-
>> 6 files changed, 249 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>
>> diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
>> index f45b25d..9454536 100644
>> --- a/drivers/gpu/drm/simpledrm/Kconfig
>> +++ b/drivers/gpu/drm/simpledrm/Kconfig
>> @@ -13,6 +13,9 @@ config DRM_SIMPLEDRM
>> SimpleDRM supports "simple-framebuffer" DeviceTree objects and
>> compatible platform framebuffers.
>>
>> + If fbdev support is enabled, this driver will also provide an fbdev
>> + compatibility layer.
>> +
>> If unsure, say Y.
>>
>> To compile this driver as a module, choose M here: the
>> diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
>> index f6a62dc..7087245 100644
>> --- a/drivers/gpu/drm/simpledrm/Makefile
>> +++ b/drivers/gpu/drm/simpledrm/Makefile
>> @@ -1,4 +1,5 @@
>> simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
>> simpledrm_damage.o
>> +simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
>>
>> obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
>> index 481acc2..f01b75d 100644
>> --- a/drivers/gpu/drm/simpledrm/simpledrm.h
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm.h
>> @@ -17,13 +17,13 @@
>>
>> struct simplefb_format;
>> struct regulator;
>> -struct fb_info;
>> struct clk;
>>
>> struct sdrm_device {
>> struct drm_device *ddev;
>> struct drm_simple_display_pipe pipe;
>> struct drm_connector conn;
>> + struct sdrm_fbdev *fbdev;
>>
>> /* framebuffer information */
>> const struct simplefb_format *fb_sformat;
>> @@ -46,6 +46,7 @@ struct sdrm_device {
>> #endif
>> };
>>
>> +void sdrm_lastclose(struct drm_device *ddev);
>> int sdrm_drm_modeset_init(struct sdrm_device *sdrm);
>> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma);
>>
>> @@ -87,4 +88,33 @@ struct sdrm_framebuffer {
>>
>> #define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
>>
>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>> +
>> +void sdrm_fbdev_init(struct sdrm_device *sdrm);
>> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
>> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb);
>> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
>> +
>> +#else
>> +
>> +static inline void sdrm_fbdev_init(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +#endif
> Why do we need the #ifdefs here? Imo those few bytes of codes we can save
> aren't worth the complexity ...
This one is needed to make fbdev optional, which I assume is what we want?
Actually I can drop sdrm_fbdev_display_pipe_update() and
sdrm_fbdev_restore_mode() if I use drm_fb_helper_set_suspend() as you
remineded me of further down.
Or do you actually refer to the #ifdef's around clk and regulator in
struct sdrm_device? I lifted that as-is from simplefb.c, but I wondered
if I shouldn't just have dropped them since it was only 2 pointers and
2 ints.
<snip>
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>> new file mode 100644
>> index 0000000..4038dd9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * SimpleDRM firmware framebuffer driver
>> + * Copyright (c) 2012-2014 David Herrmann <dh.herrmann at gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <linux/console.h>
>> +#include <linux/fb.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/platform_data/simplefb.h>
>> +
>> +#include "simpledrm.h"
>> +
>> +struct sdrm_fbdev {
>> + struct drm_fb_helper fb_helper;
>> + struct drm_framebuffer fb;
>> +};
>> +
>> +static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
>> +{
>> + return container_of(helper, struct sdrm_fbdev, fb_helper);
>> +}
>> +
>> +static struct fb_ops sdrm_fbdev_ops = {
>> + .owner = THIS_MODULE,
>> + .fb_fillrect = drm_fb_helper_cfb_fillrect,
>> + .fb_copyarea = drm_fb_helper_cfb_copyarea,
>> + .fb_imageblit = drm_fb_helper_cfb_imageblit,
>> + .fb_check_var = drm_fb_helper_check_var,
>> + .fb_set_par = drm_fb_helper_set_par,
>> + .fb_setcmap = drm_fb_helper_setcmap,
>> +};
>> +
>> +static struct drm_framebuffer_funcs sdrm_fb_funcs = {
>> + .destroy = drm_framebuffer_cleanup,
>> +};
>> +
>> +static int sdrm_fbdev_create(struct drm_fb_helper *helper,
>> + struct drm_fb_helper_surface_size *sizes)
>> +{
>> + struct sdrm_fbdev *fbdev = to_sdrm_fbdev(helper);
>> + struct drm_device *ddev = helper->dev;
>> + struct sdrm_device *sdrm = ddev->dev_private;
>> + struct drm_framebuffer *fb = &fbdev->fb;
>> + struct drm_mode_fb_cmd2 mode_cmd = {
>> + .width = sdrm->fb_width,
>> + .height = sdrm->fb_height,
>> + .pitches[0] = sdrm->fb_stride,
>> + .pixel_format = sdrm->fb_format,
>> + };
>> + struct fb_info *fbi;
>> + int ret;
>> +
>> + fbi = drm_fb_helper_alloc_fbi(helper);
>> + if (IS_ERR(fbi))
>> + return PTR_ERR(fbi);
>> +
>> + drm_helper_mode_fill_fb_struct(fb, &mode_cmd);
>> +
>> + ret = drm_framebuffer_init(ddev, fb, &sdrm_fb_funcs);
>> + if (ret) {
>> + dev_err(ddev->dev, "Failed to initialize framebuffer: %d\n", ret);
>> + goto err_fb_info_destroy;
>> + }
>> +
>> + helper->fb = fb;
>> + fbi->par = helper;
>> +
>> + fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
>> + FBINFO_CAN_FORCE_OUTPUT;
>> + fbi->fbops = &sdrm_fbdev_ops;
>> +
>> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
>> + drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
>> +
>> + strncpy(fbi->fix.id, "simpledrmfb", 15);
>> + fbi->fix.smem_start = (unsigned long)sdrm->fb_base;
>> + fbi->fix.smem_len = sdrm->fb_size;
>> + fbi->screen_base = sdrm->fb_map;
>> +
>> + return 0;
>> +
>> +err_fb_info_destroy:
>> + drm_fb_helper_release_fbi(helper);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct drm_fb_helper_funcs sdrm_fb_helper_funcs = {
>> + .fb_probe = sdrm_fbdev_create,
>> +};
>> +
>> +void sdrm_fbdev_init(struct sdrm_device *sdrm)
>> +{
>> + struct drm_device *ddev = sdrm->ddev;
>> + struct drm_fb_helper *fb_helper;
>> + struct sdrm_fbdev *fbdev;
>> + int ret;
>> +
>> + fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>> + if (!fbdev) {
>> + dev_err(ddev->dev, "Failed to allocate drm fbdev.\n");
>> + return;
>> + }
>> +
>> + fb_helper = &fbdev->fb_helper;
>> +
>> + drm_fb_helper_prepare(ddev, fb_helper, &sdrm_fb_helper_funcs);
>> +
>> + ret = drm_fb_helper_init(ddev, fb_helper, 1, 1);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to initialize drm fb helper.\n");
>> + goto err_free;
>> + }
>> +
>> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to add connectors.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + ret = drm_fb_helper_initial_config(fb_helper,
>> + ddev->mode_config.preferred_depth);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to set initial hw configuration.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + sdrm->fbdev = fbdev;
>> +
>> + return;
>> +
>> +err_drm_fb_helper_fini:
>> + drm_fb_helper_fini(fb_helper);
>> +err_free:
>> + kfree(fbdev);
>> +}
>> +
>> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> + struct drm_fb_helper *fb_helper;
>> +
>> + if (!fbdev)
>> + return;
>> +
>> + sdrm->fbdev = NULL;
>> + fb_helper = &fbdev->fb_helper;
>> +
>> + drm_fb_helper_unregister_fbi(fb_helper);
>> + drm_fb_helper_release_fbi(fb_helper);
>> +
>> + drm_framebuffer_unregister_private(&fbdev->fb);
>> + drm_framebuffer_cleanup(&fbdev->fb);
>> +
>> + drm_fb_helper_fini(fb_helper);
>> + kfree(fbdev);
>> +}
>> +
>> +static void sdrm_fbdev_set_suspend(struct fb_info *fbi, int state)
>> +{
>> + console_lock();
>> + fb_set_suspend(fbi, state);
> Pls use the drm_fb_helper.c wrapper for this. Did you check that sdrm
> still compiles even with CONFIG_FB=n?
This file is only built if CONFIG_DRM_FBDEV_EMULATION=y, so it's safe,
I've tested that. I forgot about that wrapper, so this function can be
dropped, and I'll just open code it in pipe_update() and lastclose().
In the remove_conflicting_framebuffers patch I switch to using CONFIG_FB
instead of CONFIG_DRM_FBDEV_EMULATION to be on the safe side since I don't
know if all drivers use CONFIG_DRM_FBDEV_EMULATION.
But now I remember that CONFIG_DRM_FBDEV_EMULATION is default yes, so
I think I'll stick to that instead of switching to CONFIG_FB.
Noralf.
>> + console_unlock();
>> +}
>> +
>> +/*
>> + * Since fbdev is using the native framebuffer, fbcon has to be disabled
>> + * when the drm stack is used.
>> + */
> Tbh I wonder whether this really is still worth it, after all your work to
> make fbdev defio work smoothly. I think it'd be better if we give fbdev
> normal framebuffers too and just depend upon all the defio/dirty handling
> that's not wired up. Less complexity ftw ;-)
> -Daniel
>
>> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> +
>> + if (!fbdev || fbdev->fb_helper.fb == fb)
>> + return;
>> +
>> + if (fbdev->fb_helper.fbdev->state == FBINFO_STATE_RUNNING)
>> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 1);
>> +}
>> +
>> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> +
>> + if (!fbdev)
>> + return;
>> +
>> + drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->fb_helper);
>> +
>> + if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
>> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
>> +}
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> index 00e50d9..92ddc116 100644
>> --- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> @@ -24,6 +24,13 @@ static const uint32_t sdrm_formats[] = {
>> DRM_FORMAT_XRGB8888,
>> };
>>
>> +void sdrm_lastclose(struct drm_device *ddev)
>> +{
>> + struct sdrm_device *sdrm = ddev->dev_private;
>> +
>> + sdrm_fbdev_restore_mode(sdrm);
>> +}
>> +
>> static int sdrm_conn_get_modes(struct drm_connector *conn)
>> {
>> struct sdrm_device *sdrm = conn->dev->dev_private;
>> @@ -91,8 +98,9 @@ void sdrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>> struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
>>
>> sdrm_crtc_send_vblank_event(&pipe->crtc);
>> + sdrm_fbdev_display_pipe_update(sdrm, fb);
>>
>> - if (fb) {
>> + if (fb && fb->funcs->dirty) {
>> pipe->plane.fb = fb;
>> sdrm_dirty_all_locked(sdrm);
>> }
>> --
>> 2.8.2
>>
>> _______________________________________________
>> 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