Message-ID: <20160816153138.GR6232@phenom.ffwll.local>
Date:	Tue, 16 Aug 2016 17:31:38 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Noralf Trønnes <noralf@...nnes.org>
Cc:	Daniel Vetter <daniel@...ll.ch>, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support

On Tue, Aug 16, 2016 at 03:10:06PM +0200, Noralf Trønnes wrote:
> 
> 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@...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 ...
> 
> 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.

Yeah those ifdefs seem overkill, they make the code ugly for miniscule
gain. But I meant the above ifdef - if you drop them (and unconditionally
compile simpledrm_fbdev.c) then sure there's a bit of code size overhead
for the FB=n case, but not much. But the clear upside is less confusion in
the code. I know that i915 makes the entire file optional, but that's
because there's still a few space hacks in i915 that haven't been ported
to the core helpers yet, and because the optional i915 fbdev support
started before we had it in the core (that's only very recently).

But today I don't think the ifdef are worth it, the compile-out support
from the core should be sufficient.
> 
> <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@...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?
> 
> 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().

See above, but I think excluding files like that and trading it in for a
pile of ifdef isn't really worth it. Very little code is left as-is, since
the compiler will short-circuit most of it. It should even notice that the
fbdev emulation vfunc is unused and garbage-collect that + all the hooks,
leaving nothing behind.

> 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.

See latest drm-misc, there's a wrapper called
drm_fbdev_remove_conflicting_framebuffers. Just use that and drop the
ifdef.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists