Message-ID: <20160815064855.GA6232@phenom.ffwll.local>
Date:	Mon, 15 Aug 2016 08:48:55 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Noralf Trønnes <noralf@...nnes.org>
Cc:	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support

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@...il.com
> Signed-off-by: Noralf Trønnes <noralf@...nnes.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 ...

> +
>  #endif /* SDRM_DRV_H */
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> index e5b6f75..a4e6566 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> @@ -42,6 +42,7 @@ static struct drm_driver sdrm_drm_driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
>  			   DRIVER_ATOMIC,
>  	.fops = &sdrm_drm_fops,
> +	.lastclose = sdrm_lastclose,
> 
>  	.gem_free_object = sdrm_gem_free_object,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> @@ -448,6 +449,8 @@ static int sdrm_simplefb_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_regulators;
> 
> +	sdrm_fbdev_init(ddev->dev_private);
> +
>  	DRM_INFO("Initialized %s on minor %d\n", ddev->driver->name,
>  		 ddev->primary->index);
> 
> @@ -473,6 +476,7 @@ static int sdrm_simplefb_remove(struct platform_device *pdev)
>  	struct drm_device *ddev = platform_get_drvdata(pdev);
>  	struct sdrm_device *sdrm = ddev->dev_private;
> 
> +	sdrm_fbdev_cleanup(sdrm);
>  	drm_dev_unregister(ddev);
>  	drm_mode_config_cleanup(ddev);
> 
> 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@...il.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?

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists