diff mbox

[v3,2/3] drm: simpledrm: add fbdev fallback support

Message ID 1471193526-22844-3-git-send-email-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show

Commit Message

Noralf Trønnes Aug. 14, 2016, 4:52 p.m. UTC
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@gmail.com
Signed-off-by: Noralf Trønnes <noralf@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

--
2.8.2

Comments

Daniel Vetter Aug. 15, 2016, 6:48 a.m. UTC | #1
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@gmail.com
> Signed-off-by: Noralf Trønnes <noralf@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 ...

> +
>  #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@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?

> +	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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Noralf Trønnes Aug. 16, 2016, 1:10 p.m. UTC | #2
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@gmail.com
>> Signed-off-by: Noralf Trønnes <noralf@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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Aug. 16, 2016, 3:31 p.m. UTC | #3
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@gmail.com
> > > Signed-off-by: Noralf Trønnes <noralf@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.

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@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().

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
diff mbox

Patch

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
+
 #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@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);
+	console_unlock();
+}
+
+/*
+ * Since fbdev is using the native framebuffer, fbcon has to be disabled
+ * when the drm stack is used.
+ */
+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);
 	}