[PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Thomas Zimmermann]

(was: DRM driver for fbdev devices)

This is version 2 of the fbdev conversion helpers. It's more or less a
rewrite of the original patchset.

The fbdev subsystem is considered legacy and will probably be removed at
some point. This would mean the loss of a signifanct number of drivers.
Some of the affected hardware is not in use any longer, but some hardware
is still around and provides good(-enough) framebuffers.

The fbconv helpers allow for running the current DRM stack on top of fbdev
drivers. It's a set of functions that convert between fbdev interfaces and
DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
basic functionality of a framebuffer, but should be compatible with most
existing fbdev drivers.

A DRM driver using fbconv helpers consists of

  * DRM stub code that calls into fbconv helpers, and
  * the original fbdev driver code.

The fbdev driver code has to be modified to register itself with the
stub driver instead of the fbdev core framework. A tutorial on how to use
the helpers is part of this patchset. The resulting driver hybrid can be
refactored into a first-class DRM driver. The fbconv helpers contain a
number of comments, labeled 'DRM porting note', which explain the required
steps.

I tested the current patchset with the following drivers: atyfb, aty128fb,
matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
With each, I was able to successfully start with fbcon enabled, run weston and
X11. The drivers are available at [1]. For reference, the patchset includes
the Matrox stub driver.

v2:
	* rename to fbconv helpers
	* rewrite as helper library
	* switch over to simple KMS helpers
	* switch over to SHMEM
	* add documentation

[1] https://gitlab.freedesktop.org/tzimmermann/linux/commits/fbconv-plus-drivers

Thomas Zimmermann (15):
  fbdev: Export fb_check_foreignness()
  fbdev: Export FBPIXMAPSIZE
  drm/simple-kms-helper: Add mode_fixup() to simple display pipe
  drm: Add fbconv helper module
  drm/fbconv: Add DRM <-> fbdev pixel-format conversion
  drm/fbconv: Add mode conversion DRM <-> fbdev
  drm/fbconv: Add modesetting infrastructure
  drm/fbconv: Add plane-state check and update
  drm/fbconv: Mode-setting pipeline enable / disable
  drm/fbconv: Reimplement several fbdev interfaces
  drm/fbconv: Add helpers for init and cleanup of fb_info structures
  drm/fbconv: Add helper documentation
  staging: Add mgakms driver
  staging/mgakms: Import matroxfb driver source code
  staging/mgakms: Update matroxfb driver code for DRM

 Documentation/gpu/drm-kms-helpers.rst     |   12 +
 Documentation/gpu/todo.rst                |   15 +
 drivers/gpu/drm/Kconfig                   |   11 +
 drivers/gpu/drm/Makefile                  |    1 +
 drivers/gpu/drm/drm_fbconv_helper.c       | 2126 +++++++++++++++++
 drivers/gpu/drm/drm_simple_kms_helper.c   |   15 +
 drivers/staging/Kconfig                   |    2 +
 drivers/staging/Makefile                  |    1 +
 drivers/staging/mgakms/Kconfig            |   18 +
 drivers/staging/mgakms/Makefile           |   17 +
 drivers/staging/mgakms/g450_pll.c         |  539 +++++
 drivers/staging/mgakms/g450_pll.h         |   13 +
 drivers/staging/mgakms/i2c-matroxfb.c     |  238 ++
 drivers/staging/mgakms/matroxfb_DAC1064.c | 1082 +++++++++
 drivers/staging/mgakms/matroxfb_DAC1064.h |  174 ++
 drivers/staging/mgakms/matroxfb_Ti3026.c  |  746 ++++++
 drivers/staging/mgakms/matroxfb_Ti3026.h  |   10 +
 drivers/staging/mgakms/matroxfb_accel.c   |  519 +++++
 drivers/staging/mgakms/matroxfb_accel.h   |    9 +
 drivers/staging/mgakms/matroxfb_base.c    | 2592 +++++++++++++++++++++
 drivers/staging/mgakms/matroxfb_base.h    |  700 ++++++
 drivers/staging/mgakms/matroxfb_crtc2.h   |   35 +
 drivers/staging/mgakms/matroxfb_g450.c    |  640 +++++
 drivers/staging/mgakms/matroxfb_g450.h    |   10 +
 drivers/staging/mgakms/matroxfb_maven.h   |   21 +
 drivers/staging/mgakms/matroxfb_misc.c    |  815 +++++++
 drivers/staging/mgakms/matroxfb_misc.h    |   22 +
 drivers/staging/mgakms/mga_device.c       |   68 +
 drivers/staging/mgakms/mga_device.h       |   30 +
 drivers/staging/mgakms/mga_drv.c          |  129 +
 drivers/staging/mgakms/mga_drv.h          |   14 +
 drivers/video/fbdev/core/fbmem.c          |    5 +-
 include/drm/drm_fbconv_helper.h           |  150 ++
 include/drm/drm_simple_kms_helper.h       |   43 +
 include/linux/fb.h                        |    3 +
 35 files changed, 10822 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c
 create mode 100644 drivers/staging/mgakms/Kconfig
 create mode 100644 drivers/staging/mgakms/Makefile
 create mode 100644 drivers/staging/mgakms/g450_pll.c
 create mode 100644 drivers/staging/mgakms/g450_pll.h
 create mode 100644 drivers/staging/mgakms/i2c-matroxfb.c
 create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.c
 create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.h
 create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.c
 create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.h
 create mode 100644 drivers/staging/mgakms/matroxfb_accel.c
 create mode 100644 drivers/staging/mgakms/matroxfb_accel.h
 create mode 100644 drivers/staging/mgakms/matroxfb_base.c
 create mode 100644 drivers/staging/mgakms/matroxfb_base.h
 create mode 100644 drivers/staging/mgakms/matroxfb_crtc2.h
 create mode 100644 drivers/staging/mgakms/matroxfb_g450.c
 create mode 100644 drivers/staging/mgakms/matroxfb_g450.h
 create mode 100644 drivers/staging/mgakms/matroxfb_maven.h
 create mode 100644 drivers/staging/mgakms/matroxfb_misc.c
 create mode 100644 drivers/staging/mgakms/matroxfb_misc.h
 create mode 100644 drivers/staging/mgakms/mga_device.c
 create mode 100644 drivers/staging/mgakms/mga_device.h
 create mode 100644 drivers/staging/mgakms/mga_drv.c
 create mode 100644 drivers/staging/mgakms/mga_drv.h
 create mode 100644 include/drm/drm_fbconv_helper.h

--
2.23.0

[PATCH v2 01/15] fbdev: Export fb_check_foreignness()

[Thomas Zimmermann]

This function is required by DRM's fbdev conversion helpers. Export it
from the module.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 3 ++-
 include/linux/fb.h               | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 95c32952fa8a..e828fcccce40 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1481,7 +1481,7 @@ static const struct file_operations fb_fops = {
 struct class *fb_class;
 EXPORT_SYMBOL(fb_class);
 
-static int fb_check_foreignness(struct fb_info *fi)
+int fb_check_foreignness(struct fb_info *fi)
 {
 	const bool foreign_endian = fi->flags & FBINFO_FOREIGN_ENDIAN;
 
@@ -1505,6 +1505,7 @@ static int fb_check_foreignness(struct fb_info *fi)
 
 	return 0;
 }
+EXPORT_SYMBOL(fb_check_foreignness);
 
 static bool apertures_overlap(struct aperture *gen, struct aperture *hw)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 41e0069eca0a..372f1f6ae42e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -622,6 +622,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var,
 			      struct fb_fix_screeninfo *fix);
 extern int fb_get_options(const char *name, char **option);
 extern int fb_new_modelist(struct fb_info *info);
+extern int fb_check_foreignness(struct fb_info *fi);
 
 extern struct fb_info *registered_fb[FB_MAX];
 extern int num_registered_fb;
-- 
2.23.0

[PATCH v2 02/15] fbdev: Export FBPIXMAPSIZE

[Thomas Zimmermann]

This constant is required by DRM's fbdev conversion helpers. Define it in
fbdev's public header file.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 2 --
 include/linux/fb.h               | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index e828fcccce40..f02377e959dc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -43,8 +43,6 @@
      *  Frame buffer device initialization and setup routines
      */
 
-#define FBPIXMAPSIZE	(1024 * 8)
-
 static DEFINE_MUTEX(registration_lock);
 
 struct fb_info *registered_fb[FB_MAX] __read_mostly;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 372f1f6ae42e..e17d3e1d86ad 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -199,6 +199,8 @@ struct fb_pixmap {
 	void (*readio) (struct fb_info *info, void *dst, void __iomem *src, unsigned int size);
 };
 
+#define FBPIXMAPSIZE	(1024 * 8)
+
 #ifdef CONFIG_FB_DEFERRED_IO
 struct fb_deferred_io {
 	/* delay between mkwrite and deferred handler */
-- 
2.23.0

[PATCH v2 03/15] drm/simple-kms-helper: Add mode_fixup() to simple display pipe

[Thomas Zimmermann]

The mode fix-up function for simple display helpers is equivalent to the
regular pipeline's CRTC mode fix-up function. It's called to adjust the
CRTC's display mode for the encoder. Add this function for DRM fbconv
helpers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++
 include/drm/drm_simple_kms_helper.h     | 43 +++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 046055719245..acd9b79bf92a 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -46,6 +46,20 @@ drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
 	return pipe->funcs->mode_valid(crtc, mode);
 }
 
+static bool
+drm_simple_kms_crtc_mode_fixup(struct drm_crtc *crtc,
+			       const struct drm_display_mode *mode,
+			       struct drm_display_mode *adjusted_mode)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->mode_fixup)
+		return true;
+
+	return pipe->funcs->mode_fixup(crtc, mode, adjusted_mode);
+}
+
 static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 				     struct drm_crtc_state *state)
 {
@@ -87,6 +101,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
 
 static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
 	.mode_valid = drm_simple_kms_crtc_mode_valid,
+	.mode_fixup = drm_simple_kms_crtc_mode_fixup,
 	.atomic_check = drm_simple_kms_crtc_check,
 	.atomic_enable = drm_simple_kms_crtc_enable,
 	.atomic_disable = drm_simple_kms_crtc_disable,
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 4d89cd0a60db..1b975ab67144 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -52,6 +52,49 @@ struct drm_simple_display_pipe_funcs {
 	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
 					   const struct drm_display_mode *mode);
 
+	/**
+	 * @mode_fixup:
+	 *
+	 * This callback is used to validate a mode. The parameter mode is the
+	 * display mode that userspace requested, adjusted_mode is the mode the
+	 * encoders need to be fed with. Note that this is the inverse semantics
+	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
+	 * vfunc. If the CRTC of the simple display pipe cannot support the
+	 * requested conversion from mode to adjusted_mode it should reject the
+	 * modeset.
+	 *
+	 * This function is optional.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of atomic modesets, which
+	 * can be aborted for any reason (including on userspace's request to
+	 * just check whether a configuration would be possible). Atomic drivers
+	 * MUST NOT touch any persistent state (hardware or software) or data
+	 * structures except the passed in adjusted_mode parameter.
+	 *
+	 * Atomic drivers which need to inspect and adjust more state should
+	 * instead use the @atomic_check callback, but note that they're not
+	 * perfectly equivalent: @mode_valid is called from
+	 * drm_atomic_helper_check_modeset(), but @atomic_check is called from
+	 * drm_atomic_helper_check_planes(), because originally it was meant for
+	 * plane update checks only.
+	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
+	 * RETURNS:
+	 *
+	 * True if an acceptable configuration is possible, false if the modeset
+	 * operation should be rejected.
+	 */
+	bool (*mode_fixup)(struct drm_crtc *crtc,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode);
+
 	/**
 	 * @enable:
 	 *
-- 
2.23.0

[PATCH v2 04/15] drm: Add fbconv helper module

[Thomas Zimmermann]

This adds fbconv helpers for DRM to the build infrastructure. The
configuration symbol is DRM_FBCONV_HELPERS. Drivers should select
it if they wish to use fbconv.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/Kconfig             | 10 ++++++++++
 drivers/gpu/drm/Makefile            |  1 +
 drivers/gpu/drm/drm_fbconv_helper.c |  1 +
 3 files changed, 12 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 9591bd720e56..ed689201ec81 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -157,6 +157,16 @@ config DRM_DP_CEC
 	  Note: not all adapters support this feature, and even for those
 	  that do support this they often do not hook up the CEC pin.
 
+config DRM_FBCONV_HELPER
+	tristate "Enable fbdev conversion helpers"
+	depends on DRM
+	help
+	  Provides helpers for running DRM on top of fbdev drivers. Choose
+	  this option if you're converting an fbdev driver to DRM. The
+	  helpers provide conversion functions for fbdev data structures
+	  and allow to build a basic DRM driver on top of the fbdev
+	  interfaces.
+
 config DRM_TTM
 	tristate
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9f1c7c486f88..a7178245d938 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -52,6 +52,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
+drm_kms_helper-$(CONFIG_DRM_FBCONV_HELPER) += drm_fbconv_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
new file mode 100644
index 000000000000..0cb46d2c98c3
--- /dev/null
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -0,0 +1 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
-- 
2.23.0

[PATCH v2 05/15] drm/fbconv: Add DRM <-> fbdev pixel-format conversion

[Thomas Zimmermann]

DRM uses FOURCC constants to describe pixel formats, fbdev uses a
per-component bitfield structure. The functions in this patch convert
between the two.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 435 ++++++++++++++++++++++++++++
 include/drm/drm_fbconv_helper.h     |  23 ++
 2 files changed, 458 insertions(+)
 create mode 100644 include/drm/drm_fbconv_helper.h

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index 0cb46d2c98c3..af45358a156a 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -1 +1,436 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <asm/byteorder.h>
+
+#include <linux/fb.h>
+
+#include <drm/drm_fbconv_helper.h>
+
+/*
+ * Format conversion helpers
+ */
+
+static bool is_c8(const struct fb_var_screeninfo *fb_var)
+{
+	return fb_var->bits_per_pixel = 8;
+}
+
+static bool is_rgb565(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 16) &&
+	       (fb_var->red.offset = 11) &&
+	       (fb_var->red.length = 5) &&
+	       (fb_var->green.offset = 5) &&
+	       (fb_var->green.length = 6) &&
+	       (fb_var->blue.offset = 0) &&
+	       (fb_var->blue.length = 5);
+}
+
+static bool is_bgr565(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 16) &&
+	       (fb_var->red.offset = 0) &&
+	       (fb_var->red.length = 5) &&
+	       (fb_var->green.offset = 5) &&
+	       (fb_var->green.length = 6) &&
+	       (fb_var->blue.offset = 11) &&
+	       (fb_var->blue.length = 5);
+}
+
+static bool is_rgb888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 24) &&
+	       (fb_var->red.offset = 16) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 0) &&
+	       (fb_var->blue.length = 8);
+}
+
+static bool is_bgr888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 24) &&
+	       (fb_var->red.offset = 0) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 16) &&
+	       (fb_var->blue.length = 8);
+}
+
+static bool is_xrgb8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 16) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 0) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.length = 0);
+}
+
+static bool is_xbgr8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 0) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 16) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.length = 0);
+}
+
+static bool is_rgbx8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 24) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 16) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 8) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.length = 0);
+}
+
+static bool is_bgrx8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 8) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 16) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 24) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.length = 0);
+}
+
+static bool is_argb8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 16) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 0) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.offset = 24) &&
+	       (fb_var->transp.length = 8);
+}
+
+static bool is_abgr8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 0) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 8) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 16) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.offset = 24) &&
+	       (fb_var->transp.length = 8);
+}
+
+static bool is_rgba8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 24) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 16) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 8) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.offset = 0) &&
+	       (fb_var->transp.length = 8);
+}
+
+static bool is_bgra8888(const struct fb_var_screeninfo *fb_var)
+{
+	return (fb_var->bits_per_pixel = 32) &&
+	       (fb_var->red.offset = 8) &&
+	       (fb_var->red.length = 8) &&
+	       (fb_var->green.offset = 16) &&
+	       (fb_var->green.length = 8) &&
+	       (fb_var->blue.offset = 24) &&
+	       (fb_var->blue.length = 8) &&
+	       (fb_var->transp.offset = 0) &&
+	       (fb_var->transp.length = 8);
+}
+
+struct format_map {
+	bool (*is_format)(const struct fb_var_screeninfo *fb_var);
+	uint32_t format;
+};
+
+/**
+ * drm_fbconv_format_of_fb_var_screeninfo - Returns a DRM_FORMAT constant
+ *	from an fb_var_screeninfo structure
+ * @fb_info:	the fb_var_screeninfo structure
+ * Returns:
+ *	A DRM_FORMAT constant on success, or
+ *	DRM_FORMAT_INVALID otherwise.
+ */
+uint32_t
+drm_fbconv_format_of_fb_var_screeninfo(const struct fb_var_screeninfo *fb_var)
+{
+	static const struct format_map map[] = {
+		{ is_c8,       DRM_FORMAT_C8 }, /* 256-color palette */
+		{ is_rgb565,   DRM_FORMAT_RGB565 },
+		{ is_bgr565,   DRM_FORMAT_BGR565 },
+		{ is_rgb888,   DRM_FORMAT_RGB888 },
+		{ is_bgr888,   DRM_FORMAT_BGR888 },
+		{ is_xrgb8888, DRM_FORMAT_XRGB8888 },
+		{ is_xbgr8888, DRM_FORMAT_XBGR8888 },
+		{ is_rgbx8888, DRM_FORMAT_RGBX8888 },
+		{ is_bgrx8888, DRM_FORMAT_BGRX8888 },
+		{ is_argb8888, DRM_FORMAT_ARGB8888 },
+		{ is_abgr8888, DRM_FORMAT_ABGR8888 },
+		{ is_rgba8888, DRM_FORMAT_RGBA8888 },
+		{ is_bgra8888, DRM_FORMAT_BGRA8888 }
+	};
+
+	size_t i;
+
+	if (fb_var->bits_per_pixel < 8)
+		goto err; /* at least 8-bit color required */
+	if (fb_var->grayscale = 1)
+		goto err; /* grayscale output is not supported */
+
+	for (i = 0; i < ARRAY_SIZE(map); ++i) {
+		if (map[i].is_format(fb_var))
+			return map[i].format;
+	}
+
+err:
+	return DRM_FORMAT_INVALID;
+}
+EXPORT_SYMBOL(drm_fbconv_format_of_fb_var_screeninfo);
+
+/**
+ * drm_fbconv_format_of_fb_info - Returns a DRM_FORMAT constant from
+ *	an fb_info structure
+ * @fb_info:	the fb_info structure
+ * Returns:
+ *	A DRM_FORMAT constant on success, or
+ *	DRM_FORMAT_INVALID otherwise.
+ */
+uint32_t drm_fbconv_format_of_fb_info(const struct fb_info *fb_info)
+{
+	if (fb_info->fix.type != FB_TYPE_PACKED_PIXELS)
+		return DRM_FORMAT_INVALID; /* no multi-plane formats */
+
+	return drm_fbconv_format_of_fb_var_screeninfo(&fb_info->var);
+}
+EXPORT_SYMBOL(drm_fbconv_format_of_fb_info);
+
+static void set_fb_bitfield(struct fb_bitfield *bits, __u32 offset,
+			    __u32 length)
+{
+	bits->offset = offset;
+	bits->length = length;
+	bits->msb_right = 0;
+}
+
+static void set_c8(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 8;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    0, 8);
+	set_fb_bitfield(&fb_var->green,  0, 8);
+	set_fb_bitfield(&fb_var->blue,   0, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_rgb565(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 16;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,   11, 5);
+	set_fb_bitfield(&fb_var->green,  5, 6);
+	set_fb_bitfield(&fb_var->blue,   0, 5);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_bgr565(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 16;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    0, 5);
+	set_fb_bitfield(&fb_var->green,  5, 6);
+	set_fb_bitfield(&fb_var->blue,  11, 5);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_rgb888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 24;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,   16, 8);
+	set_fb_bitfield(&fb_var->green,  8, 8);
+	set_fb_bitfield(&fb_var->blue,   0, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_bgr888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 24;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    0, 8);
+	set_fb_bitfield(&fb_var->green,  8, 8);
+	set_fb_bitfield(&fb_var->blue,  16, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_xrgb8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    16, 8);
+	set_fb_bitfield(&fb_var->green,   8, 8);
+	set_fb_bitfield(&fb_var->blue,    0, 8);
+	set_fb_bitfield(&fb_var->transp, 24, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_xbgr8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,     0, 8);
+	set_fb_bitfield(&fb_var->green,   8, 8);
+	set_fb_bitfield(&fb_var->blue,   16, 8);
+	set_fb_bitfield(&fb_var->transp, 24, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_rgbx8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,   24, 8);
+	set_fb_bitfield(&fb_var->green, 16, 8);
+	set_fb_bitfield(&fb_var->blue,   8, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_bgrx8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    8, 8);
+	set_fb_bitfield(&fb_var->green, 16, 8);
+	set_fb_bitfield(&fb_var->blue,  24, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 0);
+	fb_var->nonstd = 0;
+}
+
+static void set_argb8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    16, 8);
+	set_fb_bitfield(&fb_var->green,   8, 8);
+	set_fb_bitfield(&fb_var->blue,    0, 8);
+	set_fb_bitfield(&fb_var->transp, 24, 8);
+	fb_var->nonstd = 0;
+}
+
+static void set_abgr8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,     0, 8);
+	set_fb_bitfield(&fb_var->green,   8, 8);
+	set_fb_bitfield(&fb_var->blue,   16, 8);
+	set_fb_bitfield(&fb_var->transp, 24, 8);
+	fb_var->nonstd = 0;
+}
+
+static void set_rgba8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,   24, 8);
+	set_fb_bitfield(&fb_var->green, 16, 8);
+	set_fb_bitfield(&fb_var->blue,   8, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 8);
+	fb_var->nonstd = 0;
+}
+
+static void set_bgra8888(struct fb_var_screeninfo *fb_var)
+{
+	fb_var->bits_per_pixel = 32;
+	fb_var->grayscale = 0;
+	set_fb_bitfield(&fb_var->red,    8, 8);
+	set_fb_bitfield(&fb_var->green, 16, 8);
+	set_fb_bitfield(&fb_var->blue,  24, 8);
+	set_fb_bitfield(&fb_var->transp, 0, 8);
+	fb_var->nonstd = 0;
+}
+
+/**
+ * drm_fbconv_update_fb_var_screen_info_from_format - sets the pixel format
+ *	of a fb_var_screeninfo structure from a DRM_FORMAT constant
+ * @fb_var:	the fb_var_screeninfo structure to update
+ * @format:	a DRM_FORMAT constant
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise.
+ */
+int drm_fbconv_update_fb_var_screeninfo_from_format(
+	struct fb_var_screeninfo *fb_var, uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_C8:
+		set_c8(fb_var);
+		break;
+	case DRM_FORMAT_RGB565:
+		set_rgb565(fb_var);
+		break;
+	case DRM_FORMAT_BGR565:
+		set_bgr565(fb_var);
+		break;
+	case DRM_FORMAT_RGB888:
+		set_rgb888(fb_var);
+		break;
+	case DRM_FORMAT_BGR888:
+		set_bgr888(fb_var);
+		break;
+	case DRM_FORMAT_XRGB8888:
+		set_xrgb8888(fb_var);
+		break;
+	case DRM_FORMAT_XBGR8888:
+		set_xbgr8888(fb_var);
+		break;
+	case DRM_FORMAT_RGBX8888:
+		set_rgbx8888(fb_var);
+		break;
+	case DRM_FORMAT_BGRX8888:
+		set_bgrx8888(fb_var);
+		break;
+	case DRM_FORMAT_ARGB8888:
+		set_argb8888(fb_var);
+		break;
+	case DRM_FORMAT_ABGR8888:
+		set_abgr8888(fb_var);
+		break;
+	case DRM_FORMAT_RGBA8888:
+		set_rgba8888(fb_var);
+		break;
+	case DRM_FORMAT_BGRA8888:
+		set_bgra8888(fb_var);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_update_fb_var_screeninfo_from_format);
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
new file mode 100644
index 000000000000..6b2ed12b579a
--- /dev/null
+++ b/include/drm/drm_fbconv_helper.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef DRM_FBCONV_HELPER_H
+#define DRM_FBCONV_HELPER_H
+
+#include <drm/drm_fourcc.h>
+
+struct fb_info;
+struct fb_var_screeninfo;
+
+/*
+ * Format conversion helpers
+ */
+
+uint32_t
+drm_fbconv_format_of_fb_var_screeninfo(const struct fb_var_screeninfo *fb_var);
+
+uint32_t drm_fbconv_format_of_fb_info(const struct fb_info *fb_info);
+
+int drm_fbconv_update_fb_var_screeninfo_from_format(
+	struct fb_var_screeninfo *fb_var, uint32_t format);
+
+#endif
-- 
2.23.0

[PATCH v2 06/15] drm/fbconv: Add mode conversion DRM <-> fbdev

[Thomas Zimmermann]

DRM uses struct drm_display_mode to describe a display mode. The
conversion functions fill it from fbdev data strucutures, and vice
versa.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 201 ++++++++++++++++++++++++++++
 include/drm/drm_fbconv_helper.h     |  31 +++++
 2 files changed, 232 insertions(+)

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index af45358a156a..e5a58a361ae9 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -5,6 +5,7 @@
 #include <linux/fb.h>
 
 #include <drm/drm_fbconv_helper.h>
+#include <drm/drm_modes.h>
 
 /*
  * Format conversion helpers
@@ -434,3 +435,203 @@ int drm_fbconv_update_fb_var_screeninfo_from_format(
 	return 0;
 }
 EXPORT_SYMBOL(drm_fbconv_update_fb_var_screeninfo_from_format);
+
+/*
+ * Mode conversion helpers
+ */
+
+/**
+ * drm_mode_update_from_fb_videomode - Sets a drm_display mode struecture
+ *	from an fb_videomode structure
+ * @mode:	the DRM display mode structure to update
+ * @fb_mode:	an fb_videomode structure
+ */
+void drm_mode_update_from_fb_videomode(struct drm_display_mode *mode,
+				       const struct fb_videomode *fb_mode)
+{
+	mode->type = DRM_MODE_TYPE_DRIVER;
+
+	mode->clock = PICOS2KHZ(fb_mode->pixclock);
+
+	mode->hdisplay = fb_mode->xres;
+	mode->hsync_start = mode->hdisplay + fb_mode->right_margin;
+	mode->hsync_end = mode->hsync_start + fb_mode->hsync_len;
+	mode->htotal = mode->hsync_end + fb_mode->left_margin;
+	mode->hskew = 0;
+
+	mode->vdisplay = fb_mode->yres;
+	mode->vsync_start = mode->vdisplay + fb_mode->lower_margin;
+	mode->vsync_end = mode->vsync_start + fb_mode->vsync_len;
+	mode->vtotal = mode->vsync_end + fb_mode->upper_margin;
+	mode->vscan = 0;
+
+	mode->flags = 0;
+
+	if (fb_mode->sync & FB_SYNC_HOR_HIGH_ACT)
+		mode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		mode->flags |= DRM_MODE_FLAG_NHSYNC;
+
+	if (fb_mode->sync & FB_SYNC_VERT_HIGH_ACT)
+		mode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		mode->flags |= DRM_MODE_FLAG_NVSYNC;
+
+	if (fb_mode->sync & FB_SYNC_COMP_HIGH_ACT)
+		mode->flags |= DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_PCSYNC;
+
+	if (fb_mode->vmode & FB_VMODE_INTERLACED)
+		mode->flags |= DRM_MODE_FLAG_INTERLACE;
+
+	if (fb_mode->vmode & FB_VMODE_DOUBLE)
+		mode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+	mode->width_mm = 0;
+	mode->height_mm = 0;
+
+	mode->vrefresh = fb_mode->refresh;
+	mode->hsync = mode->clock / mode->vtotal;
+
+	/* final step; depends on previous setup */
+	if (fb_mode->name) {
+		strncpy(mode->name, fb_mode->name, sizeof(mode->name) - 1);
+		mode->name[sizeof(mode->name) - 1] = '\0';
+	} else {
+		drm_mode_set_name(mode);
+	}
+}
+EXPORT_SYMBOL(drm_mode_update_from_fb_videomode);
+
+/**
+ * drm_mode_update_from_fb_var_screeninfo - Sets a drm_display mode structure
+ *	from an fb_var_screenmode structure
+ * @mode:	the DRM display mode structure to update
+ * @fb_var:	an fb_var_screeninfo structure
+ */
+void drm_mode_update_from_fb_var_screeninfo(
+	struct drm_display_mode *mode, const struct fb_var_screeninfo *fb_var)
+{
+	struct fb_videomode fb_mode;
+
+	fb_var_to_videomode(&fb_mode, fb_var);
+	drm_mode_update_from_fb_videomode(mode, &fb_mode);
+}
+EXPORT_SYMBOL(drm_mode_update_from_fb_var_screeninfo);
+
+/**
+ * drm_mode_create_from_fb_videomode - Creates a drm_display mode structure
+ *	from an fb_videomode structure
+ * @dev:	the new mode's DRM device
+ * @fb_mode:	an fb_videomode structure
+ * Returns:
+ *	A newly allocated DRM display mode structure on success, or
+ *	NULL otherwise
+ */
+struct drm_display_mode * drm_mode_create_from_fb_videomode(
+	struct drm_device *dev, const struct fb_videomode *fb_mode)
+{
+	/* cleared to '0' */
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_create(dev);
+	if (!mode)
+		return NULL;
+
+	drm_mode_update_from_fb_videomode(mode, fb_mode);
+
+	return mode;
+}
+EXPORT_SYMBOL(drm_mode_create_from_fb_videomode);
+
+/**
+ * drm_fbconv_update_fb_videomode_from_mode - updates an fb_videomode
+ *	structure from a DRM display mode
+ * @fb_mode:	the fb_videomode structure to update
+ * @mode:	a DRM display mode
+ */
+void
+drm_fbconv_update_fb_videomode_from_mode(struct fb_videomode *fb_mode,
+					 const struct drm_display_mode *mode)
+{
+	fb_mode->name = NULL;
+	fb_mode->refresh = mode->vrefresh;
+	fb_mode->xres = mode->hdisplay;
+	fb_mode->yres = mode->vdisplay;
+	fb_mode->pixclock = KHZ2PICOS(mode->clock);
+	fb_mode->left_margin = mode->htotal - mode->hsync_end;
+	fb_mode->right_margin = mode->hsync_start - mode->hdisplay;
+	fb_mode->upper_margin = mode->vtotal - mode->vsync_end;
+	fb_mode->lower_margin = mode->vsync_start - mode->vdisplay;
+	fb_mode->hsync_len = mode->hsync_end - mode->hsync_start;
+	fb_mode->vsync_len = mode->vsync_end - mode->vsync_start;
+
+	fb_mode->sync = 0;
+	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
+		fb_mode->sync |= FB_SYNC_HOR_HIGH_ACT;
+	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+		fb_mode->sync |= FB_SYNC_VERT_HIGH_ACT;
+	if (mode->flags & (DRM_MODE_FLAG_CSYNC | DRM_MODE_FLAG_PCSYNC))
+		fb_mode->sync |= FB_SYNC_COMP_HIGH_ACT;
+
+	fb_mode->vmode = 0;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		fb_mode->vmode |= FB_VMODE_INTERLACED;
+	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
+		fb_mode->vmode |= FB_VMODE_DOUBLE;
+
+	fb_mode->flag = 0;
+}
+EXPORT_SYMBOL(drm_fbconv_update_fb_videomode_from_mode);
+
+/**
+ * drm_fbconv_init_fb_videomode_from_mode - initializes an fb_videomode
+ *	structure from a DRM display mode
+ * @fb_mode:	the fb_videomode structure to update
+ * @mode:	a DRM display mode
+ *
+ * This is the same as drm_fbconv_update_fb_videomode_from_mode(), but
+ * first clears the fb_screeninfo structure to 0.
+ */
+void drm_fbconv_init_fb_videomode_from_mode(
+	struct fb_videomode *fb_mode, const struct drm_display_mode *mode)
+{
+	memset(fb_mode, 0, sizeof(*fb_mode));
+	drm_fbconv_update_fb_videomode_from_mode(fb_mode, mode);
+}
+EXPORT_SYMBOL(drm_fbconv_init_fb_videomode_from_mode);
+
+/**
+ * drm_fbconv_update_fb_var_screeninfo_from_mode - updates an
+ *	fb_var_screeninfo structure from a DRM display mode
+ * @fb_var:	the fb_var_screeninfo structure to update
+ * @mode:	a DRM display mode
+ */
+void drm_fbconv_update_fb_var_screeninfo_from_mode(
+	struct fb_var_screeninfo *fb_var, const struct drm_display_mode *mode)
+{
+	struct fb_videomode fb_mode;
+
+	drm_fbconv_init_fb_videomode_from_mode(&fb_mode, mode);
+	fb_videomode_to_var(fb_var, &fb_mode);
+
+	fb_var->height = mode->height_mm;
+	fb_var->width = mode->width_mm;
+}
+EXPORT_SYMBOL(drm_fbconv_update_fb_var_screeninfo_from_mode);
+
+/**
+ * drm_fbconv_init_fb_var_screeninfo_from_mode - initialize an
+ *	fb_var_screeninfo structure from a DRM display mode
+ * @fb_var:	the fb_var_screeninfo structure to update
+ * @mode:	a DRM display mode
+ *
+ * This is the same as drm_fbconv_update_fb_var_screeninfo_from_mode(), but
+ * first clears the fb_screeninfo structure to 0.
+ */
+void drm_fbconv_init_fb_var_screeninfo_from_mode(
+	struct fb_var_screeninfo *fb_var, const struct drm_display_mode *mode)
+{
+	memset(fb_var, 0, sizeof(*fb_var));
+	drm_fbconv_update_fb_var_screeninfo_from_mode(fb_var, mode);
+}
+EXPORT_SYMBOL(drm_fbconv_init_fb_var_screeninfo_from_mode);
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
index 6b2ed12b579a..cbb13228c76c 100644
--- a/include/drm/drm_fbconv_helper.h
+++ b/include/drm/drm_fbconv_helper.h
@@ -5,8 +5,12 @@
 
 #include <drm/drm_fourcc.h>
 
+struct drm_device;
+struct drm_display_mode;
 struct fb_info;
 struct fb_var_screeninfo;
+struct fb_var_screeninfo;
+struct fb_videomode;
 
 /*
  * Format conversion helpers
@@ -20,4 +24,31 @@ uint32_t drm_fbconv_format_of_fb_info(const struct fb_info *fb_info);
 int drm_fbconv_update_fb_var_screeninfo_from_format(
 	struct fb_var_screeninfo *fb_var, uint32_t format);
 
+/*
+ * Mode conversion helpers
+ */
+
+void drm_mode_update_from_fb_videomode(struct drm_display_mode *mode,
+				       const struct fb_videomode *fb_mode);
+
+void drm_mode_update_from_fb_var_screeninfo(
+	struct drm_display_mode *mode, const struct fb_var_screeninfo *fb_var);
+
+struct drm_display_mode * drm_mode_create_from_fb_videomode(
+	struct drm_device *dev, const struct fb_videomode *fb_mode);
+
+void
+drm_fbconv_update_fb_videomode_from_mode(struct fb_videomode *fb_mode,
+					 const struct drm_display_mode *mode);
+
+void
+drm_fbconv_init_fb_videomode_from_mode(struct fb_videomode *fb_mode,
+				       const struct drm_display_mode *mode);
+
+void drm_fbconv_update_fb_var_screeninfo_from_mode(
+	struct fb_var_screeninfo *var, const struct drm_display_mode *mode);
+
+void drm_fbconv_init_fb_var_screeninfo_from_mode(
+	struct fb_var_screeninfo *var, const struct drm_display_mode *mode);
+
 #endif
-- 
2.23.0

[PATCH v2 07/15] drm/fbconv: Add modesetting infrastructure

[Thomas Zimmermann]

Modesetting for fbconv supports a single display pipeline with CRTC,
primary plane, encoder and connector. It's implementation is based on
struct drm_simple_display_pipe, which fits this use case nicely.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 382 ++++++++++++++++++++++++++++
 include/drm/drm_fbconv_helper.h     |  78 ++++++
 2 files changed, 460 insertions(+)

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index e5a58a361ae9..4cda1f15e072 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -4,8 +4,13 @@
 
 #include <linux/fb.h>
 
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_fbconv_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modes.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
 
 /*
  * Format conversion helpers
@@ -635,3 +640,380 @@ void drm_fbconv_init_fb_var_screeninfo_from_mode(
 	drm_fbconv_update_fb_var_screeninfo_from_mode(fb_var, mode);
 }
 EXPORT_SYMBOL(drm_fbconv_init_fb_var_screeninfo_from_mode);
+
+/*
+ * Connector
+ */
+
+static int connector_helper_get_modes(struct drm_connector *connector)
+{
+	return 0;
+}
+
+static int connector_helper_detect_ctx(struct drm_connector *connector,
+				       struct drm_modeset_acquire_ctx *ctx,
+				       bool force)
+{
+	return connector_status_connected;
+}
+
+static enum drm_mode_status connector_helper_mode_valid(
+	struct drm_connector *connector, struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static int connector_helper_atomic_check(struct drm_connector *connector,
+					 struct drm_atomic_state *state)
+{
+	return 0;
+}
+
+static void connector_helper_atomic_commit(struct drm_connector *connector,
+					   struct drm_connector_state *state)
+{ }
+
+static const struct drm_connector_helper_funcs connector_helper_funcs = {
+	.get_modes = connector_helper_get_modes,
+	.detect_ctx = connector_helper_detect_ctx,
+	.mode_valid = connector_helper_mode_valid,
+	.best_encoder = NULL, /* use default */
+	.atomic_best_encoder = NULL, /* use best_encoder instead */
+	.atomic_check = connector_helper_atomic_check,
+	.atomic_commit = connector_helper_atomic_commit
+};
+
+static enum drm_connector_status connector_detect(
+	struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static void connector_force(struct drm_connector *connector)
+{ }
+
+static void connector_destroy(struct drm_connector *connector)
+{ }
+
+static int connector_atomic_set_property(struct drm_connector *connector,
+					 struct drm_connector_state *state,
+					 struct drm_property *property,
+					 uint64_t val)
+{
+	return -EINVAL;
+}
+
+static int connector_atomic_get_property(
+	struct drm_connector *connector,
+	const struct drm_connector_state *state, struct drm_property *property,
+	uint64_t *val)
+{
+	return -EINVAL;
+}
+
+static const struct drm_connector_funcs connector_funcs = {
+	.dpms = NULL, /* not used by atomic drivers */
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = connector_detect,
+	.force = connector_force,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.set_property = NULL,
+	.late_register = NULL,
+	.early_unregister = NULL,
+	.destroy = connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.atomic_set_property = connector_atomic_set_property,
+	.atomic_get_property = connector_atomic_get_property,
+	.atomic_print_state = NULL
+};
+
+/*
+ * Simple display pipe
+ */
+
+/**
+ * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
+ *	struct drm_simple_display_pipe_funcs.mode_valid
+ * @crtc:	the DRM CRTC structure
+ * @mode:	the display mode to validate
+ * Returns:
+ *	MODE_OK on success, or
+ *	a MODE constant otherwise
+ */
+enum drm_mode_status
+drm_fbconv_simple_display_pipe_mode_valid(struct drm_crtc *crtc,
+					  const struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_valid);
+
+/**
+ * drm_fbconv_simple_display_pipe_mode_fixup - default implementation for
+ *	struct drm_simple_display_pipe_funcs.mode_fixup
+ * @crtc:		the DRM CRTC structure
+ * @mode:		the display mode
+ * @adjusted_mode:	the adjusted display mode
+ * Returns:
+ *	true on success, or
+ *	false otherwise
+ */
+bool drm_fbconv_simple_display_pipe_mode_fixup(
+	struct drm_crtc *crtc, const struct drm_display_mode *mode,
+	struct drm_display_mode *adjusted_mode)
+{
+	return true;
+}
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);
+
+/**
+ * drm_fbconv_simple_display_pipe_enable - default implementation for
+ *	struct drm_simple_display_pipe_funcs.enable
+ * @pipe:		the display pipe structure
+ * @crtc_state:		the new CRTC state
+ * @plane_state:	the new plane state
+ */
+void
+drm_fbconv_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_plane_state *plane_state)
+{ }
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_enable);
+
+/**
+ * drm_fbconv_simple_display_pipe_disable - default implementation for
+ *	struct drm_simple_display_pipe_funcs.disable
+ * @pipe:		the display pipe structure
+ */
+void
+drm_fbconv_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+{ }
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_disable);
+
+/**
+ * drm_fbconv_simple_display_pipe_check - default implementation for
+ *	struct drm_simple_display_pipe_funcs.check
+ * @pipe:		the display pipe structure
+ * @plane_state:	the new plane state
+ * @crtc_state:		the new CRTC state
+ */
+int
+drm_fbconv_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
+				     struct drm_plane_state *plane_state,
+				     struct drm_crtc_state *crtc_state)
+{
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_check);
+
+/**
+ * drm_fbconv_simple_display_pipe_mode_update - default implementation for
+ *	struct drm_simple_display_pipe_funcs.update
+ * @pipe:		the display pipe structure
+ * @old_plane_state:	the old plane state
+ */
+void
+drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
+				      struct drm_plane_state *old_plane_state)
+{ }
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_update);
+
+/**
+ * drm_fbconv_simple_display_pipe_prepare_fb - default implementation for
+ *	struct drm_simple_display_pipe_funcs.prepare_fb
+ * @pipe:		the display pipe structure
+ * @plane_state:	the new plane state
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise.
+ *
+ * The implementation of struct drm_simple_display_pipe_funcs.prepare_fb
+ * maps the framebuffer's buffer object and the fbdev's screen memory, if
+ * necessary. After converting the fbdev driver to DRM, only the buffer-object
+ * mapping should remaing. See drm_fbconv_simple_display_pipe_cleanup_fb() for
+ * the corresponding clean-up helper.
+ */
+int
+drm_fbconv_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+					  struct drm_plane_state *plane_state)
+{ }
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_prepare_fb);
+
+/**
+ * drm_fbconv_simple_display_pipe_cleanup_fb - default implementation for
+ *	struct drm_simple_display_pipe_funcs.cleanup_fb
+ * @pipe:		the display pipe structure
+ * @plane_state:	the old plane state
+ *
+ * This function cleans up the framebuffer state after a plane update. See
+ * drm_fbconv_simple_display_pipe_prepare_fb() for the corresponding prepare
+ * helper.
+ */
+void
+drm_fbconv_simple_display_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
+					  struct drm_plane_state *plane_state)
+{ }
+
+static const struct drm_simple_display_pipe_funcs simple_display_pipe_funcs = {
+	.mode_valid = drm_fbconv_simple_display_pipe_mode_valid,
+	.mode_fixup = drm_fbconv_simple_display_pipe_mode_fixup,
+	.enable	    = drm_fbconv_simple_display_pipe_enable,
+	.disable    = drm_fbconv_simple_display_pipe_disable,
+	.check	    = drm_fbconv_simple_display_pipe_check,
+	.update	    = drm_fbconv_simple_display_pipe_update,
+	.prepare_fb = drm_fbconv_simple_display_pipe_prepare_fb,
+	.cleanup_fb = drm_fbconv_simple_display_pipe_cleanup_fb,
+};
+
+/*
+ * Mode config
+ */
+
+static enum drm_mode_status mode_config_mode_valid(
+	struct drm_device *dev, const struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static const struct drm_mode_config_funcs mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.get_format_info = NULL,
+	/* DRM porting notes: the output_poll_changed callback is used by
+	 * fb helpers to implement fbdev emulation. If you're porting an
+	 * fbdev driver to DRM and enable fbdev emulation, this callback
+	 * will become useful.
+	 */
+	.output_poll_changed = drm_fb_helper_output_poll_changed,
+	.mode_valid = mode_config_mode_valid,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+	.atomic_state_alloc = NULL,
+	.atomic_state_clear = NULL,
+	.atomic_state_free = NULL
+};
+
+/**
+ * drm_fbconv_modeset_init - initializes an fbconv modesetting structure
+ * @modeset:		the fbconv modesetting structure to initialize
+ * @dev:		the DRM device
+ * @fb_info:		the fbdev driver's fb_info structure
+ * @max_width:		the maximum display width that is supported by
+ *                      the device
+ * @max_height:		the maximum display height that is supported by
+ *                      the device
+ * @preferred_depth:	the device's preferred color depth
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise
+ *
+ * This function initializes an instance of struct drm_fbconv_modeset. The
+ * supplied values for max_width, max_height, and max_depth should match the
+ * devices capabilities and be supported by the fbdev driver. DRM helpers
+ * will use these to auto-configure and validate display settings.
+ */
+int drm_fbconv_modeset_init(struct drm_fbconv_modeset *modeset,
+			    struct drm_device *dev, struct fb_info *fb_info,
+			    unsigned int max_width, unsigned int max_height,
+			    unsigned int preferred_depth)
+{
+	struct drm_mode_config *mode_config = &dev->mode_config;
+
+	modeset->dev = dev;
+	modeset->fb_info = fb_info;
+
+	drm_mode_config_init(dev);
+
+	mode_config->max_width = (int)max_width;
+	mode_config->max_height = (int)max_height;
+	mode_config->fb_base = fb_info->fix.smem_start;
+	mode_config->preferred_depth = preferred_depth;
+	mode_config->prefer_shadow_fbdev = true;
+	mode_config->funcs = &mode_config_funcs;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_modeset_init);
+
+/**
+ * drm_fbconv_modeset_cleanup - Cleans up an fbconv modesetting structure
+ * @modeset:	the fbconv modesetting structure to clean up
+ */
+void drm_fbconv_modeset_cleanup(struct drm_fbconv_modeset *modeset)
+{
+	drm_mode_config_cleanup(modeset->dev);
+}
+EXPORT_SYMBOL(drm_fbconv_modeset_cleanup);
+
+/**
+ * drm_fbconv_modeset_setup_pipe - sets up the display pipeline for fbconv
+ * @modeset:		an fbconv modesetting structure
+ * @funcs:		an implementation of
+ *                      struct drm_simple_display_pipe_funcs, or NULL
+ * @formats:		the device's supported display formats
+ * @format_count:	the number of entries in @formats
+ * @format_modifiers:	DRM format modifiers, or NULL
+ * @connector:		the DRM connector, or NULL
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise
+ *
+ * This function sets up the display pipeline for an initialized instance of
+ * struct drm_fbconv_modeset. For maximum compatibility with userspace, the
+ * provided list of formats should contain at least DRM_FORMAT_XRGB8888 and
+ * DRM_FORMAT_RGB565. The necessary conversion to the hardware's actual
+ * configuration can be performed by drm_fbconv_simple_display_pipe_update().
+ *
+ * The values for @funcs, @format_modifiers, and @connector should be NULL
+ * by default. Explicitly settings these parameters will only be helpful for
+ * refactoring an fbdev driver into a DRM driver.
+ */
+int
+drm_fbconv_modeset_setup_pipe(struct drm_fbconv_modeset *modeset,
+			      const struct drm_simple_display_pipe_funcs *funcs,
+			      const uint32_t *formats,
+			      unsigned int format_count,
+			      const uint64_t *format_modifiers,
+			      struct drm_connector *connector)
+{
+	int ret;
+
+	/* DRM porting note: Now let's enable the display pipeline. If
+	 * you're porting a framebuffer driver to DRM, you may want to
+	 * set the correct connector type or replace the simple display
+	 * pipeline with something more sophisticated.
+	 */
+
+	if (!funcs)
+		funcs = &simple_display_pipe_funcs;
+
+	if (!connector) {
+		connector = &modeset->connector;
+
+		ret = drm_connector_init(modeset->dev, connector,
+					 &connector_funcs,
+					 DRM_MODE_CONNECTOR_Unknown);
+		if (ret)
+			return ret;
+		drm_connector_helper_add(connector, &connector_helper_funcs);
+
+		ret = drm_connector_register(connector);
+		if (ret < 0)
+			return ret;
+
+	}
+
+	ret = drm_simple_display_pipe_init(modeset->dev, &modeset->pipe,
+					   funcs, formats, format_count,
+					   format_modifiers, connector);
+	if (ret)
+		return ret;
+
+	/* Final step: resetting the device's mode config creates
+	 * state for all objects in the mode-setting pipeline.
+	 */
+	drm_mode_config_reset(modeset->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_modeset_setup_pipe);
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
index cbb13228c76c..79716af687c1 100644
--- a/include/drm/drm_fbconv_helper.h
+++ b/include/drm/drm_fbconv_helper.h
@@ -3,7 +3,11 @@
 #ifndef DRM_FBCONV_HELPER_H
 #define DRM_FBCONV_HELPER_H
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_simple_kms_helper.h>
 
 struct drm_device;
 struct drm_display_mode;
@@ -51,4 +55,78 @@ void drm_fbconv_update_fb_var_screeninfo_from_mode(
 void drm_fbconv_init_fb_var_screeninfo_from_mode(
 	struct fb_var_screeninfo *var, const struct drm_display_mode *mode);
 
+/*
+ * Simple display pipe
+ */
+
+enum drm_mode_status
+drm_fbconv_simple_display_pipe_mode_valid(struct drm_crtc *crtc,
+					  const struct drm_display_mode *mode);
+
+bool drm_fbconv_simple_display_pipe_mode_fixup(
+	struct drm_crtc *crtc, const struct drm_display_mode *mode,
+	struct drm_display_mode *adjusted_mode);
+
+void
+drm_fbconv_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_plane_state *plane_state);
+
+void
+drm_fbconv_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe);
+
+int
+drm_fbconv_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
+				     struct drm_plane_state *plane_state,
+				     struct drm_crtc_state *crtc_state);
+
+void
+drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
+				      struct drm_plane_state *old_plane_state);
+
+int
+drm_fbconv_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+					  struct drm_plane_state *plane_state);
+
+void
+drm_fbconv_simple_display_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
+					  struct drm_plane_state *plane_state);
+
+/*
+ * Modeset helpers
+ */
+
+/**
+ * struct drm_fbconv_modeset - contains state for fbconv modesetting
+ * @connector:	the DRM connector
+ * @pipe:	the modesetting pipeline
+ * @dev:	the DRM device
+ * @fb_info:	the fbdev driver's fb_info structure
+ */
+struct drm_fbconv_modeset {
+	struct drm_connector connector;
+	struct drm_simple_display_pipe pipe;
+
+	struct drm_device *dev;
+	struct fb_info *fb_info;
+};
+
+static inline struct drm_fbconv_modeset *drm_fbconv_modeset_of_pipe(
+	struct drm_simple_display_pipe *pipe)
+{
+	return container_of(pipe, struct drm_fbconv_modeset, pipe);
+}
+
+int drm_fbconv_modeset_init(struct drm_fbconv_modeset *modeset,
+			    struct drm_device *dev, struct fb_info *fb_info,
+			    unsigned int max_width, unsigned int max_height,
+			    unsigned int preferred_depth);
+void drm_fbconv_modeset_cleanup(struct drm_fbconv_modeset *modeset);
+
+int drm_fbconv_modeset_setup_pipe(
+	struct drm_fbconv_modeset *modeset,
+	const struct drm_simple_display_pipe_funcs *funcs,
+	const uint32_t *formats, unsigned int format_count,
+	const uint64_t *format_modifiers, struct drm_connector *connector);
+
 #endif
-- 
2.23.0

[PATCH v2 08/15] drm/fbconv: Add plane-state check and update

[Thomas Zimmermann]

For the update of the primary plane, we copy the content of a SHMEM buffer
object to the hardware's on-screen buffer; doing a format conversion if
necessary. This is able to support all combinations of framebuffers and
hardware, and should work with any fbdev driver.

Occasionally, fbdev drivers require an update of the hardware's gamma
tables to not show distorted colors. We also do this during the plane
update.

There's no support for horizontal panning, as fbdev drivers vary widely
in their capability to do this. Vertical panning is supported to the
extend allowed by available video ram. However, this whole functionality
is more interesting for porting drivers and not directly required by
fbconv itself.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/Kconfig             |   1 +
 drivers/gpu/drm/drm_fbconv_helper.c | 500 +++++++++++++++++++++++++++-
 include/drm/drm_fbconv_helper.h     |   9 +
 3 files changed, 507 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index ed689201ec81..2ce7749c3157 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -160,6 +160,7 @@ config DRM_DP_CEC
 config DRM_FBCONV_HELPER
 	tristate "Enable fbdev conversion helpers"
 	depends on DRM
+	select DRM_GEM_SHMEM_HELPER
 	help
 	  Provides helpers for running DRM on top of fbdev drivers. Choose
 	  this option if you're converting an fbdev driver to DRM. The
diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index 4cda1f15e072..cf218016ac05 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -5,12 +5,17 @@
 #include <linux/fb.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_fbconv_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_format_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>
 
 /*
  * Format conversion helpers
@@ -728,10 +733,192 @@ static const struct drm_connector_funcs connector_funcs = {
 	.atomic_print_state = NULL
 };
 
+/*
+ * Colormap updates
+ */
+
+/* provides a default colormap for palette modes */
+static int create_palette_cmap(struct fb_cmap *cmap,
+			       const struct fb_var_screeninfo *fb_var)
+{
+	__u32 len;
+	const struct fb_cmap *default_cmap;
+	int ret;
+
+	len = max3(fb_var->red.length,
+		   fb_var->green.length,
+		   fb_var->blue.length);
+	if (!len || (len > 31)) {
+		DRM_ERROR("fbconv: Gamma LUT has invalid bit count of %u\n",
+			  (unsigned int)len);
+		return -EINVAL;
+	}
+
+	default_cmap = fb_default_cmap(1ul << len);
+	if (!default_cmap) {
+		DRM_ERROR("fbconv: fb_default_cmap() failed\n");
+		return -EINVAL;
+	}
+
+	ret = fb_alloc_cmap(cmap, default_cmap->len, 0);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_alloc_cmap() failed: %d\n", ret);
+		return ret;
+	}
+	ret = fb_copy_cmap(default_cmap, cmap);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_copy_cmap() failed: %d\n", ret);
+		goto err_fb_dealloc_cmap;
+	}
+
+	return 0;
+
+err_fb_dealloc_cmap:
+	fb_dealloc_cmap(cmap);
+	return ret;
+}
+
+/* provides a linear color ramp for RGB modes */
+static int create_linear_cmap(struct fb_cmap *cmap,
+			      const struct fb_var_screeninfo *fb_var)
+{
+	int ret;
+	size_t i;
+	unsigned int j;
+	u16 *lut;
+	u16 incr;
+	u16 *gamma_lut[3];
+	__u32 len;
+	const __u32 gamma_len[3] = {
+		fb_var->red.length,
+		fb_var->green.length,
+		fb_var->blue.length
+	};
+
+	len = max3(gamma_len[0], gamma_len[1], gamma_len[2]);
+	if (!len || (len > 8)) {
+		DRM_ERROR("fbconv: gamma LUT has invalid bit count of %u\n",
+			  (unsigned int)len);
+		return -EINVAL;
+	}
+
+	ret = fb_alloc_cmap(cmap, 1ul << len, 0);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_alloc_cmap() failed: %d\n", ret);
+		return ret;
+	}
+
+	gamma_lut[0] = cmap->red;
+	gamma_lut[1] = cmap->green;
+	gamma_lut[2] = cmap->blue;
+
+	for (i = 0; i < ARRAY_SIZE(gamma_lut); ++i) {
+		lut = gamma_lut[i];
+		len = 1ul << gamma_len[i];
+		incr = 0x10000u >> gamma_len[i];
+		for (j = 0; j < len; ++j, ++lut)
+			*lut = incr * j;
+
+		/* In order to have no intensity at index 0 and full
+		 * intensity at the final index of the LUT, we fix-up the
+		 * table's final entries. The fix-up makes intensity grow
+		 * faster near the final entries of the gamma LUT. The human
+		 * eye is more sensitive to changes to the lower intensities,
+		 * so this is probably not directly perceivable.
+		 */
+		for (lut -= gamma_len[i], j = gamma_len[i]; j > 0; ++lut) {
+			--j;
+			/* subtract 1 to not overflow the LUT's final entry */
+			*lut += (incr >> j) - 1;
+		}
+	}
+
+	return 0;
+}
+
+static int set_cmap(struct fb_info *fb_info)
+{
+	struct fb_cmap cmap;
+	int ret;
+
+	memset(&cmap, 0, sizeof(cmap));
+
+	switch (fb_info->fix.visual) {
+	case FB_VISUAL_PSEUDOCOLOR:
+		ret = create_palette_cmap(&cmap, &fb_info->var);
+		break;
+	case FB_VISUAL_DIRECTCOLOR:
+		ret = create_linear_cmap(&cmap, &fb_info->var);
+		break;
+	default:
+		return 0;
+	}
+	if (ret)
+		return ret;
+
+	ret = fb_set_cmap(&cmap, fb_info);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_set_cmap() failed: %d\n", ret);
+		goto err_fb_dealloc_cmap;
+	}
+	fb_dealloc_cmap(&cmap);
+
+	return 0;
+
+err_fb_dealloc_cmap:
+	fb_dealloc_cmap(&cmap);
+	return ret;
+}
+
 /*
  * Simple display pipe
  */
 
+static void drm_fbconv_update_fb_var_screeninfo_from_crtc_state(
+	struct fb_var_screeninfo *fb_var, struct drm_crtc_state *crtc_state)
+{
+	drm_fbconv_update_fb_var_screeninfo_from_mode(
+		fb_var, &crtc_state->adjusted_mode);
+}
+
+static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
+	struct fb_var_screeninfo *fb_var, struct drm_framebuffer *fb,
+	size_t vram_size)
+{
+	unsigned int width, pitch;
+	uint64_t cpp, lines;
+	int ret;
+
+	/* Our virtual screen covers all the graphics memory (sans some
+	 * trailing bytes). This allows for setting the scanout buffer's
+	 * address with fb_pan_display().
+	 */
+
+	width = fb->pitches[0];
+	cpp = fb->format[0].cpp[0];
+	do_div(width, cpp);
+
+	if (width > (__u32)-1)
+		return -EINVAL; /* would overflow fb_var->xres_virtual */
+
+	pitch = fb->pitches[0];
+	lines = vram_size;
+	do_div(lines, pitch);
+
+	if (lines > (__u32)-1)
+		return -EINVAL; /* would overflow fb_var->yres_virtual */
+
+	fb_var->xres_virtual = width;
+	fb_var->yres_virtual = lines;
+
+	ret = drm_fbconv_update_fb_var_screeninfo_from_format(
+		fb_var, fb->format[0].format);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /**
  * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
  *	struct drm_simple_display_pipe_funcs.mode_valid
@@ -767,6 +954,52 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
 }
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);
 
+/**
+ * drm_fbconv_blit_rect - copy an area of pixel data from a framebuffer
+ *	to the hardware buffer
+ * @dst:	the on-screen hardware buffer
+ * @vaddr:	the source buffer in kernel address space
+ * @fb:		the framebuffer of the source buffer
+ * @rect:	the area to copy
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise.
+ *
+ * This function copies the pixel data from a DRM framebuffer to a hardware
+ * buffer; doing necessary format conversion in the process. Not all
+ * combinations of source and destination formats are currently supported.
+ */
+int drm_fbconv_blit_rect(void *dst, void *vaddr, struct drm_framebuffer *fb,
+			 struct drm_rect *rect)
+{
+	struct drm_device *dev = fb->dev;
+
+	if (!vaddr)
+		return 0; /* no framebuffer set for plane; no error */
+
+	if (dev->mode_config.preferred_depth = (fb->format->cpp[0] * 8))
+		drm_fb_memcpy_dstclip(dst, vaddr, fb, rect);
+
+	else if (fb->format->cpp[0] = 4 &&
+		 dev->mode_config.preferred_depth = 16)
+		drm_fb_xrgb8888_to_rgb565_dstclip(dst, fb->pitches[0],
+						  vaddr, fb, rect, false);
+
+	else if (fb->format->cpp[0] = 4 &&
+		 dev->mode_config.preferred_depth = 24)
+		drm_fb_xrgb8888_to_rgb888_dstclip(dst, fb->pitches[0],
+						  vaddr, fb, rect);
+
+	else {
+		/* TODO: add the missing conversion */
+		DRM_ERROR("fbconv: mismatching pixel formats\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_blit_rect);
+
 /**
  * drm_fbconv_simple_display_pipe_enable - default implementation for
  *	struct drm_simple_display_pipe_funcs.enable
@@ -803,6 +1036,100 @@ drm_fbconv_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
 				     struct drm_plane_state *plane_state,
 				     struct drm_crtc_state *crtc_state)
 {
+	struct drm_fbconv_modeset *modeset;
+	struct fb_videomode fb_mode, fb_var_mode;
+	int ret;
+	struct fb_var_screeninfo fb_var;
+
+	/*
+	 * CRTC check
+	 */
+
+	modeset = drm_fbconv_modeset_of_pipe(pipe);
+
+	/* DRM porting notes: when fbcon takes over the console, it regularly
+	 * changes the display mode. Where's apparently no way to detect this
+	 * directly from fbcon itself. DRM's mode information might therefore
+	 * be out of data, after it takes over the display at a later time.
+	 * Here, we test the CRTC's current mode with the fbdev state. If they
+	 * do not match, we request a mode change from DRM. If you port an
+	 * fbdev driver to DRM, you can remove this code section, DRM will
+	 * be in full control of the display device and doesn't have to react
+	 * to changes from external sources.
+	 */
+
+	if (!crtc_state->mode_changed && crtc_state->adjusted_mode.clock) {
+		drm_fbconv_init_fb_videomode_from_mode(
+			&fb_mode, &crtc_state->adjusted_mode);
+		fb_var_to_videomode(&fb_var_mode, &modeset->fb_info->var);
+		if (!fb_mode_is_equal(&fb_mode, &fb_var_mode))
+			crtc_state->mode_changed = true;
+	}
+
+	/* TODO: The vblank interrupt is currently not supported. We set
+	 * the corresponding flag as a workaround. Some fbdev drivers
+	 * support FBIO_WAITFORVSYNC, which we might use for querying
+	 * vblanks.
+	 *
+	 * DRM porting notes: if you're porting an fbdev driver to DRM,
+	 * remove this line and instead signal a vblank event from the
+	 * interrupt handler.
+	 */
+	crtc_state->no_vblank = true;
+
+	/*
+	 * Plane check
+	 */
+
+	if (!plane_state->crtc)
+		return 0;
+
+	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						  1 << 16, 1 << 16,
+						  false, true);
+	if (ret < 0)
+		return ret;
+
+	if (!plane_state->visible || !plane_state->fb)
+		return 0;
+
+	/* Virtual screen sizes are not supported.
+	 */
+
+	if (drm_rect_width(&plane_state->dst) != plane_state->fb->width ||
+	    drm_rect_height(&plane_state->dst) != plane_state->fb->height) {
+		DRM_ERROR("fbconv: virtual screen sizes not supported\n");
+		return -EINVAL;
+	}
+	if (plane_state->dst.x1 || plane_state->dst.y1) {
+		DRM_ERROR("fbconv: virtual screen offset not supported\n");
+		return -EINVAL;
+	}
+
+	/* Pixel formats have to be compatible with fbdev. This is
+	 * usually some variation of XRGB.
+	 */
+
+	if (!pipe->plane.state ||
+	    !pipe->plane.state->fb ||
+	    pipe->plane.state->fb->format[0].format !+		plane_state->fb->format[0].format) {
+
+		if (modeset->fb_info->fbops->fb_check_var) {
+			memcpy(&fb_var, &modeset->fb_info->var,
+			       sizeof(fb_var));
+			drm_fbconv_update_fb_var_screeninfo_from_crtc_state(
+				&fb_var, crtc_state);
+			drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
+				&fb_var, plane_state->fb,
+				modeset->fb_info->fix.smem_len);
+			ret = modeset->fb_info->fbops->fb_check_var(
+				&fb_var, modeset->fb_info);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_check);
@@ -816,7 +1143,119 @@ EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_check);
 void
 drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 				      struct drm_plane_state *old_plane_state)
-{ }
+{
+	struct drm_fbconv_modeset *modeset;
+	uint32_t format;
+	struct fb_var_screeninfo fb_var;
+	int ret;
+	bool do_blit;
+	struct drm_rect rect;
+	struct drm_crtc *crtc = &pipe->crtc;
+
+	/*
+	 * Plane update
+	 */
+
+	modeset = drm_fbconv_modeset_of_pipe(pipe);
+
+	format = drm_fbconv_format_of_fb_info(modeset->fb_info);
+
+	/* DRM porting notes: Some fbdev drivers report alpha channels for
+	 * their framebuffer, even though they don't support transparent
+	 * primary planes. For the format test below, we ignore the alpha
+	 * channel and use the non-transparent equivalent of the pixel format.
+	 * If you're porting an fbdev driver to DRM, remove this switch
+	 * statement and report the correct format instead.
+	 */
+	switch (format) {
+	case DRM_FORMAT_ARGB8888:
+		format = DRM_FORMAT_XRGB8888;
+		break;
+	case DRM_FORMAT_ABGR8888:
+		format = DRM_FORMAT_XBGR8888;
+		break;
+	case DRM_FORMAT_RGBA8888:
+		format = DRM_FORMAT_RGBX8888;
+		break;
+	case DRM_FORMAT_BGRA8888:
+		format = DRM_FORMAT_BGRX8888;
+		break;
+	default:
+		break;
+	}
+
+	if (!pipe->plane.state->fb) {
+		/* No framebuffer installed; blank display. */
+		fb_blank(modeset->fb_info, FB_BLANK_NORMAL);
+		return;
+	}
+
+	if ((format != pipe->plane.state->fb->format[0].format) ||
+	    (modeset->fb_info->var.xres_virtual !+	    pipe->plane.state->fb->width)) {
+
+		/* Pixel format changed, update fb_info accordingly
+		 */
+
+		memcpy(&fb_var, &modeset->fb_info->var, sizeof(fb_var));
+		ret = drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
+			&fb_var, pipe->plane.state->fb,
+			modeset->fb_info->fix.smem_len);
+		if (ret)
+			return;
+
+		fb_var.activate = FB_ACTIVATE_NOW;
+
+		ret = fb_set_var(modeset->fb_info, &fb_var);
+		if (ret) {
+			DRM_ERROR("fbconv: fb_set_var() failed: %d\n", ret);
+			return;
+		}
+	}
+
+	if (!old_plane_state->fb || /* first-time update */
+	    (format != pipe->plane.state->fb->format[0].format)) {
+
+		/* DRM porting notes: Below we set the LUTs for palette and
+		 * gamma correction. This is required by some fbdev drivers,
+		 * such as nvidiafb and atyfb, which don't initialize the
+		 * table to pass-through the framebuffer values unchanged. This
+		 * is actually CRTC state, but the respective function
+		 * crtc_helper_mode_set_nofb() is only called when a CRTC
+		 * property changes, changes in color formats are not handled
+		 * there. When you're porting a fbdev driver to DRM, remove
+		 * the call. Gamma LUTs are CRTC properties and should be
+		 * handled there. Either remove gamma correction or set up
+		 * the respective CRTC properties for userspace.
+		 */
+		set_cmap(modeset->fb_info);
+	}
+
+	memcpy(&fb_var, &modeset->fb_info->var, sizeof(fb_var));
+	fb_var.xoffset = 0;
+	fb_var.yoffset = 0;
+
+	ret = fb_pan_display(modeset->fb_info, &fb_var);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_pan_display() failed: %d\n", ret);
+		return;
+	}
+
+	do_blit = drm_atomic_helper_damage_merged(old_plane_state,
+						  pipe->plane.state,
+						  &rect);
+	if (do_blit)
+		drm_fbconv_blit_rect(modeset->blit.screen_base,
+				     modeset->blit.vmap, pipe->plane.state->fb,
+				     &rect);
+
+	if (crtc->state->event) {
+		spin_lock_irq(&crtc->dev->event_lock);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		crtc->state->event = NULL;
+		spin_unlock_irq(&crtc->dev->event_lock);
+	}
+}
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_update);
 
 /**
@@ -837,7 +1276,48 @@ EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_update);
 int
 drm_fbconv_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
 					  struct drm_plane_state *plane_state)
-{ }
+{
+	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
+	struct fb_info *fb_info = modeset->fb_info;
+	struct drm_framebuffer *fb = plane_state->fb;
+	bool unmap_screen_base = false;
+	void *screen_base;
+	void *vmap;
+	int ret;
+
+	if (!fb)
+		return 0;
+
+	screen_base = fb_info->screen_base;
+
+	if (!screen_base) {
+		screen_base = ioremap(fb_info->fix.smem_start,
+				      fb_info->fix.smem_len);
+		if (!screen_base) {
+			DRM_ERROR("fbconv: ioremap() failed\n");
+			return -ENOMEM;
+		}
+		unmap_screen_base = true;
+	}
+
+	vmap = drm_gem_shmem_vmap(fb->obj[0]);
+	if (!vmap) {
+		DRM_ERROR("fbconv: drm_gem_shmem_vmap() failed\n");
+		ret = -ENOMEM;
+		goto err_iounmap;
+	}
+
+	modeset->blit.vmap = vmap;
+	modeset->blit.screen_base = screen_base;
+	modeset->blit.unmap_screen_base = unmap_screen_base;
+
+	return 0;
+
+err_iounmap:
+	if (unmap_screen_base)
+		iounmap(screen_base);
+	return ret;
+}
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_prepare_fb);
 
 /**
@@ -853,7 +1333,21 @@ EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_prepare_fb);
 void
 drm_fbconv_simple_display_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
 					  struct drm_plane_state *plane_state)
-{ }
+{
+	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
+	struct drm_framebuffer *fb = plane_state->fb;
+
+	if (!fb)
+		return;
+
+	drm_gem_shmem_vunmap(fb->obj[0], modeset->blit.vmap);
+
+	if (modeset->blit.unmap_screen_base)
+		iounmap(modeset->blit.screen_base);
+
+	memset(&modeset->blit, 0, sizeof(modeset->blit));
+}
+EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_cleanup_fb);
 
 static const struct drm_simple_display_pipe_funcs simple_display_pipe_funcs = {
 	.mode_valid = drm_fbconv_simple_display_pipe_mode_valid,
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
index 79716af687c1..3e62b5e80af6 100644
--- a/include/drm/drm_fbconv_helper.h
+++ b/include/drm/drm_fbconv_helper.h
@@ -92,6 +92,9 @@ void
 drm_fbconv_simple_display_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
 					  struct drm_plane_state *plane_state);
 
+int drm_fbconv_blit_rect(void *dst, void *vaddr, struct drm_framebuffer *fb,
+			 struct drm_rect *rect);
+
 /*
  * Modeset helpers
  */
@@ -107,6 +110,12 @@ struct drm_fbconv_modeset {
 	struct drm_connector connector;
 	struct drm_simple_display_pipe pipe;
 
+	struct {
+		void *vmap;
+		void *screen_base;
+		bool unmap_screen_base;
+	} blit;
+
 	struct drm_device *dev;
 	struct fb_info *fb_info;
 };
-- 
2.23.0

[PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Thomas Zimmermann]

The display mode is set by converting the DRM display mode to an
fb_info state and handling it to the fbdev driver's fb_setvar()
function. This also requires a color depth, which we take from the
value of struct drm_mode_config.preferred_depth

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 113 +++++++++++++++++++++++++++-
 include/drm/drm_fbconv_helper.h     |   2 +
 2 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index cf218016ac05..ca8b43c91266 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
 	return 0;
 }
 
+static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
+	struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
+{
+	struct drm_plane *primary = pipe->crtc.primary;
+	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
+
+	if (primary && primary->state && primary->state->fb)
+		return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
+			fb_var, primary->state->fb,
+			modeset->fb_info->fix.smem_len);
+
+	fb_var->xres_virtual = fb_var->xres;
+	fb_var->yres_virtual = fb_var->yres;
+	fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;
+
+	return 0;
+}
+
 /**
  * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
  *	struct drm_simple_display_pipe_funcs.mode_valid
@@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
 	struct drm_crtc *crtc, const struct drm_display_mode *mode,
 	struct drm_display_mode *adjusted_mode)
 {
+	struct drm_simple_display_pipe *pipe +		container_of(crtc, struct drm_simple_display_pipe, crtc);
+	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
+	struct fb_var_screeninfo fb_var;
+	int ret;
+
+	if (!modeset->fb_info->fbops->fb_check_var)
+		return true;
+
+	drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
+
+	ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
+		&fb_var, &modeset->pipe);
+	if (ret)
+		return true;
+
+	ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);
+	if (ret < 0)
+		return false;
+
+	drm_mode_update_from_fb_var_screeninfo(adjusted_mode, &fb_var);
+
 	return true;
 }
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);
@@ -1000,6 +1040,32 @@ int drm_fbconv_blit_rect(void *dst, void *vaddr, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_fbconv_blit_rect);
 
+/**
+ * drm_fbconv_blit_fullscreen - copy all pixel data from a framebuffer
+ *	to the hardware buffer
+ * @dst:	the on-screen hardware buffer
+ * @vaddr:	the source buffer in kernel address space
+ * @fb:		the framebuffer of the source buffer
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise.
+ *
+ * This function is equivalent to drm_fbconv_blit_rect(), but copies the
+ * framebuffer's complete content.
+ */
+int drm_fbconv_blit_fullscreen(void *dst, void *vaddr,
+			       struct drm_framebuffer *fb)
+{
+	struct drm_rect fullscreen = {
+		.x1 = 0,
+		.x2 = fb->width,
+		.y1 = 0,
+		.y2 = fb->height,
+	};
+	return drm_fbconv_blit_rect(dst, vaddr, fb, &fullscreen);
+}
+EXPORT_SYMBOL(drm_fbconv_blit_fullscreen);
+
 /**
  * drm_fbconv_simple_display_pipe_enable - default implementation for
  *	struct drm_simple_display_pipe_funcs.enable
@@ -1011,7 +1077,46 @@ void
 drm_fbconv_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 				      struct drm_crtc_state *crtc_state,
 				      struct drm_plane_state *plane_state)
-{ }
+{
+	struct drm_fbconv_modeset *modeset;
+	struct fb_var_screeninfo fb_var;
+	int ret;
+
+	/* As this is atomic mode setting, any function call is not
+	 * allowed to fail. If it does, an additional test should be
+	 * added to simple_display_pipe_check().
+	 */
+
+	modeset = drm_fbconv_modeset_of_pipe(pipe);
+
+	drm_fbconv_init_fb_var_screeninfo_from_mode(
+		&fb_var, &crtc_state->adjusted_mode);
+
+	if (plane_state && plane_state->fb) {
+		ret = drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
+			&fb_var, plane_state->fb,
+			modeset->fb_info->fix.smem_len);
+		if (ret)
+			return;
+	} else {
+		fb_var.xres_virtual = fb_var.xres;
+		fb_var.yres_virtual = fb_var.yres;
+	}
+
+	fb_var.activate = FB_ACTIVATE_NOW;
+
+	ret = fb_set_var(modeset->fb_info, &fb_var);
+	if (ret) {
+		DRM_ERROR("fbconv: fb_set_var() failed: %d\n", ret);
+		return;
+	}
+
+	fb_blank(modeset->fb_info, FB_BLANK_UNBLANK);
+
+	drm_fbconv_blit_fullscreen(modeset->blit.screen_base,
+				   modeset->blit.vmap,
+				   plane_state->fb);
+}
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_enable);
 
 /**
@@ -1021,7 +1126,11 @@ EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_enable);
  */
 void
 drm_fbconv_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
-{ }
+{
+	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
+
+	fb_blank(modeset->fb_info, FB_BLANK_POWERDOWN);
+}
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_disable);
 
 /**
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
index 3e62b5e80af6..c7d211f40462 100644
--- a/include/drm/drm_fbconv_helper.h
+++ b/include/drm/drm_fbconv_helper.h
@@ -94,6 +94,8 @@ drm_fbconv_simple_display_pipe_cleanup_fb(struct drm_simple_display_pipe *pipe,
 
 int drm_fbconv_blit_rect(void *dst, void *vaddr, struct drm_framebuffer *fb,
 			 struct drm_rect *rect);
+int drm_fbconv_blit_fullscreen(void *dst, void *vaddr,
+			       struct drm_framebuffer *fb);
 
 /*
  * Modeset helpers
-- 
2.23.0

[PATCH v2 10/15] drm/fbconv: Reimplement several fbdev interfaces

[Thomas Zimmermann]

This patch reimplements fb_blank(), fb_pan_display(), fb_set_cmap() and
fb_set_var() for fbconv helpers. The goal is to have all calls to driver
callback functions located within fbconv and to reduce the amount of
contained work to a minimum.

Some noteable differences to fbdev include:

  * Code related to fbcon has been left out. Console support is
    emulated by DRM and the drivers don't interact directly with
    it.

  * No events are sent out. As the fbconv helpers are not part of
    the fbdev framework, there are no event listeners anyway.

  * Code related to ioctl and user-space has been left out as
    well. User-space interfaces are provided by DRM.

  * Error messages have been added.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 240 +++++++++++++++++++++++++---
 1 file changed, 220 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index ca8b43c91266..f7f247e30a3d 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -737,6 +737,55 @@ static const struct drm_connector_funcs connector_funcs = {
  * Colormap updates
  */
 
+static int drm_fbconv_set_cmap(struct fb_cmap *cmap, struct fb_info *fb_info)
+{
+	int i, start, res;
+	u16 *red, *green, *blue, *transp;
+	u_int hred, hgreen, hblue, htransp = 0xffff;
+
+	red = cmap->red;
+	green = cmap->green;
+	blue = cmap->blue;
+	transp = cmap->transp;
+	start = cmap->start;
+
+	if (start < 0 || (!fb_info->fbops->fb_setcolreg &&
+			  !fb_info->fbops->fb_setcmap)) {
+		DRM_ERROR("fbconv: Palette not supported.\n");
+		return -EINVAL;
+	}
+
+	if (fb_info->fbops->fb_setcmap) {
+		res = fb_info->fbops->fb_setcmap(cmap, fb_info);
+		if (res) {
+			DRM_ERROR("fbconv: fbops->fb_setcmap() failed: %d\n",
+				  res);
+			return res;
+		}
+	} else {
+		for (i = 0; i < cmap->len; i++) {
+			hred = *red++;
+			hgreen = *green++;
+			hblue = *blue++;
+			if (transp)
+				htransp = *transp++;
+			res = fb_info->fbops->fb_setcolreg(start++,
+							   hred, hgreen, hblue,
+							   htransp, fb_info);
+			if (res) {
+				DRM_ERROR("fbconv: fbops->fb_setcolreg() failed: %d\n",
+					  res);
+				/* cmap handling is a mess; don't err here */
+				break;
+			}
+		}
+	}
+
+	fb_copy_cmap(cmap, &fb_info->cmap);
+
+	return 0;
+}
+
 /* provides a default colormap for palette modes */
 static int create_palette_cmap(struct fb_cmap *cmap,
 			       const struct fb_var_screeninfo *fb_var)
@@ -856,11 +905,9 @@ static int set_cmap(struct fb_info *fb_info)
 	if (ret)
 		return ret;
 
-	ret = fb_set_cmap(&cmap, fb_info);
-	if (ret) {
-		DRM_ERROR("fbconv: fb_set_cmap() failed: %d\n", ret);
+	ret = drm_fbconv_set_cmap(&cmap, fb_info);
+	if (ret)
 		goto err_fb_dealloc_cmap;
-	}
 	fb_dealloc_cmap(&cmap);
 
 	return 0;
@@ -891,7 +938,7 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
 
 	/* Our virtual screen covers all the graphics memory (sans some
 	 * trailing bytes). This allows for setting the scanout buffer's
-	 * address with fb_pan_display().
+	 * address with drm_fbconv_pan_display().
 	 */
 
 	width = fb->pitches[0];
@@ -937,6 +984,165 @@ static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
 	return 0;
 }
 
+static int drm_fbconv_blank(struct fb_info *fb_info, int blank)
+{
+	int ret = -EINVAL;
+
+	if (fb_info->fbops->fb_blank) {
+		ret = fb_info->fbops->fb_blank(blank, fb_info);
+		if (ret) {
+			DRM_ERROR("fbconv: fbops->fb_blank() failed: %d\n",
+				  ret);
+		}
+	}
+	return ret;
+}
+
+static int drm_fbconv_pan_display(struct fb_info *fb_info,
+				  struct fb_var_screeninfo *var)
+{
+	struct fb_fix_screeninfo *fix = &fb_info->fix;
+	unsigned int yres = fb_info->var.yres;
+	int err;
+
+	if (var->yoffset > 0) {
+		if (var->vmode & FB_VMODE_YWRAP) {
+			if (!fix->ywrapstep ||
+			    (var->yoffset % fix->ywrapstep)) {
+				DRM_ERROR("fbconv: Invalid fix->ywrapstep: %d\n",
+					  fix->ywrapstep);
+				return -EINVAL;
+			}
+			yres = 0;
+		} else if (!fix->ypanstep || (var->yoffset % fix->ypanstep)) {
+			DRM_ERROR("fbconv: Invalid fix->ypanstep: %d\n",
+				  fix->ypanstep);
+			return -EINVAL;
+		}
+	}
+
+	if (var->xoffset > 0) {
+		if (!fix->xpanstep || (var->xoffset % fix->xpanstep)) {
+			DRM_ERROR("fbconv: Invalid fix->xpanstep: %d\n",
+				  fix->xpanstep);
+			return -EINVAL;
+		}
+	}
+
+	if (!fb_info->fbops->fb_pan_display ||
+	    var->yoffset > fb_info->var.yres_virtual - yres ||
+	    var->xoffset > fb_info->var.xres_virtual - fb_info->var.xres) {
+		DRM_ERROR("fbconv: Display panning unsupported\n");
+		return -EINVAL;
+	}
+
+	err = fb_info->fbops->fb_pan_display(var, fb_info);
+	if (err) {
+		DRM_ERROR("fbconv: fbops->pan_display() failed: %d", err);
+		return err;
+	}
+
+	fb_info->var.xoffset = var->xoffset;
+	fb_info->var.yoffset = var->yoffset;
+
+	if (var->vmode & FB_VMODE_YWRAP)
+		fb_info->var.vmode |= FB_VMODE_YWRAP;
+	else
+		fb_info->var.vmode &= ~FB_VMODE_YWRAP;
+
+	return 0;
+}
+
+static int drm_fbconv_set_var(struct fb_info *fb_info,
+			      struct fb_var_screeninfo *var)
+{
+	int ret = 0;
+	u32 activate;
+	struct fb_var_screeninfo old_var;
+	struct fb_videomode mode;
+
+	if (var->activate & FB_ACTIVATE_INV_MODE) {
+		struct fb_videomode mode1, mode2;
+
+		fb_var_to_videomode(&mode1, var);
+		fb_var_to_videomode(&mode2, &fb_info->var);
+		/* make sure we don't delete the videomode of current var */
+		ret = fb_mode_is_equal(&mode1, &mode2);
+		if (ret) {
+			DRM_ERROR("fbconv: fb_mode_is_equal() failed: %d\n",
+				  ret);
+			return -EINVAL;
+		}
+
+		fb_delete_videomode(&mode1, &fb_info->modelist);
+
+		return 0;
+	}
+
+	if (!(var->activate & FB_ACTIVATE_FORCE) &&
+	    !memcmp(&fb_info->var, var, sizeof(*var)))
+		return 0;
+
+	activate = var->activate;
+
+	/* When using FOURCC mode, make sure the red, green, blue and
+	 * transp fields are set to 0.
+	 */
+	if ((fb_info->fix.capabilities & FB_CAP_FOURCC) && var->grayscale > 1) {
+		if (var->red.offset     || var->green.offset    ||
+		    var->blue.offset    || var->transp.offset   ||
+		    var->red.length     || var->green.length    ||
+		    var->blue.length    || var->transp.length   ||
+		    var->red.msb_right  || var->green.msb_right ||
+		    var->blue.msb_right || var->transp.msb_right) {
+			DRM_ERROR("fbconv: Invalid color offsets in FOURCC mode\n");
+			return -EINVAL;
+		}
+	}
+
+	if (!fb_info->fbops->fb_check_var) {
+		*var = fb_info->var;
+		return 0;
+	}
+
+	ret = fb_info->fbops->fb_check_var(var, fb_info);
+	if (ret) {
+		DRM_ERROR("fbconv: fbops->fb_check_var() failed: %d\n", ret);
+		return ret;
+	}
+
+	if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
+		return 0;
+
+	old_var = fb_info->var;
+	fb_info->var = *var;
+
+	if (fb_info->fbops->fb_set_par) {
+		ret = fb_info->fbops->fb_set_par(fb_info);
+		if (ret) {
+			fb_info->var = old_var;
+			DRM_ERROR("fbconv: fbops->fb_set_par() failed: %d\n",
+				  ret);
+			return ret;
+		}
+	}
+
+	drm_fbconv_pan_display(fb_info, &fb_info->var);
+	drm_fbconv_set_cmap(&fb_info->cmap, fb_info);
+	fb_var_to_videomode(&mode, &fb_info->var);
+
+	if (fb_info->modelist.prev && fb_info->modelist.next &&
+	    !list_empty(&fb_info->modelist))
+		ret = fb_add_videomode(&mode, &fb_info->modelist);
+
+	if (ret) {
+		DRM_ERROR("fbconv: fb_add_videomode() failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 /**
  * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
  *	struct drm_simple_display_pipe_funcs.mode_valid
@@ -1105,13 +1311,11 @@ drm_fbconv_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	fb_var.activate = FB_ACTIVATE_NOW;
 
-	ret = fb_set_var(modeset->fb_info, &fb_var);
-	if (ret) {
-		DRM_ERROR("fbconv: fb_set_var() failed: %d\n", ret);
+	ret = drm_fbconv_set_var(modeset->fb_info, &fb_var);
+	if (ret)
 		return;
-	}
 
-	fb_blank(modeset->fb_info, FB_BLANK_UNBLANK);
+	drm_fbconv_blank(modeset->fb_info, FB_BLANK_UNBLANK);
 
 	drm_fbconv_blit_fullscreen(modeset->blit.screen_base,
 				   modeset->blit.vmap,
@@ -1129,7 +1333,7 @@ drm_fbconv_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 {
 	struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
 
-	fb_blank(modeset->fb_info, FB_BLANK_POWERDOWN);
+	drm_fbconv_blank(modeset->fb_info, FB_BLANK_POWERDOWN);
 }
 EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_disable);
 
@@ -1295,7 +1499,7 @@ drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 
 	if (!pipe->plane.state->fb) {
 		/* No framebuffer installed; blank display. */
-		fb_blank(modeset->fb_info, FB_BLANK_NORMAL);
+		drm_fbconv_blank(modeset->fb_info, FB_BLANK_NORMAL);
 		return;
 	}
 
@@ -1315,11 +1519,9 @@ drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 
 		fb_var.activate = FB_ACTIVATE_NOW;
 
-		ret = fb_set_var(modeset->fb_info, &fb_var);
-		if (ret) {
-			DRM_ERROR("fbconv: fb_set_var() failed: %d\n", ret);
+		ret = drm_fbconv_set_var(modeset->fb_info, &fb_var);
+		if (ret)
 			return;
-		}
 	}
 
 	if (!old_plane_state->fb || /* first-time update */
@@ -1344,11 +1546,9 @@ drm_fbconv_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 	fb_var.xoffset = 0;
 	fb_var.yoffset = 0;
 
-	ret = fb_pan_display(modeset->fb_info, &fb_var);
-	if (ret) {
-		DRM_ERROR("fbconv: fb_pan_display() failed: %d\n", ret);
+	ret = drm_fbconv_pan_display(modeset->fb_info, &fb_var);
+	if (ret)
 		return;
-	}
 
 	do_blit = drm_atomic_helper_damage_merged(old_plane_state,
 						  pipe->plane.state,
-- 
2.23.0

[PATCH v2 11/15] drm/fbconv: Add helpers for init and cleanup of fb_info structures

[Thomas Zimmermann]

The implementation of drm_fbconv_fill_fb_info() sets up an fbdev driver's
fb_info structure for use. It's similar to register_framebuffer(), but does
not create device files, register the driver with the fbdev code or create
a console. drm_fbconv_cleanup_fb_info() does the inverse.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbconv_helper.c | 123 ++++++++++++++++++++++++++++
 include/drm/drm_fbconv_helper.h     |   7 ++
 2 files changed, 130 insertions(+)

diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index f7f247e30a3d..7d7e4da2a29e 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -3,6 +3,7 @@
 #include <asm/byteorder.h>
 
 #include <linux/fb.h>
+#include <linux/major.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_damage_helper.h>
@@ -1722,6 +1723,9 @@ int drm_fbconv_modeset_init(struct drm_fbconv_modeset *modeset,
 {
 	struct drm_mode_config *mode_config = &dev->mode_config;
 
+	if (WARN_ON(fb_info->node != FB_MAX))
+		return -EINVAL; /* forgot to run drm_fbconv_fill_fb_info()? */
+
 	modeset->dev = dev;
 	modeset->fb_info = fb_info;
 
@@ -1820,3 +1824,122 @@ drm_fbconv_modeset_setup_pipe(struct drm_fbconv_modeset *modeset,
 	return 0;
 }
 EXPORT_SYMBOL(drm_fbconv_modeset_setup_pipe);
+
+/*
+ * Helpers for struct fb_info
+ *
+ * This is the setup and cleanup code for struct fb_info. It has been
+ * adapted from the fbdev core modules.
+ *
+ * The original implementation in fbdev also handles device files, console,
+ * and framebuffer events. As DRM drivers use DRM's framebuffer emulation,
+ * the respective code has been removed here.
+ *
+ * In contrast to the fbdev core, we don't need locking here. These don't
+ * interact with fbdev's internal state.
+ */
+
+static bool is_matroxfb(const struct fb_info *fb_info)
+{
+	return !!(fb_info->fix.accel & (FB_ACCEL_MATROX_MGA2064W |
+					FB_ACCEL_MATROX_MGA1064SG |
+					FB_ACCEL_MATROX_MGA2164W |
+					FB_ACCEL_MATROX_MGA2164W_AGP |
+					FB_ACCEL_MATROX_MGAG100 |
+					FB_ACCEL_MATROX_MGAG200 |
+					FB_ACCEL_MATROX_MGAG400));
+}
+
+/**
+ * drm_fbconv_fill_fb_info - prepares an fbdev driver's fb_info structure
+ *	for use
+ * @fb_info:	the fb_info structure to set up
+ * Returns:
+ *	0 on success, or
+ *	a negative error code otherwise.
+ *
+ * The fbdev driver provides fbconv helpers with an fb_info structure. Before
+ * use, the structure has to be set up correctly. In fbdev core,
+ * register_framebuffer() does this; here it's provided by
+ * drm_fbconv_fill_fb_info().
+ *
+ * Call drm_fbconv_cleanup_fb_info() during shutdown to clean up the fb_info
+ * structure.
+ */
+int drm_fbconv_fill_fb_info(struct fb_info *fb_info)
+{
+	struct fb_videomode mode;
+
+	/* Returing -ENOSYS on error is technically wrong, but it's
+	 * what the fbdev core code would do. So we do the same.
+	 */
+	if (fb_check_foreignness(fb_info))
+		return -ENOSYS;
+
+	fb_info->node = FB_MAX; /* not registered; filled by fbconv helpers */
+	atomic_set(&fb_info->count, 1);
+	mutex_init(&fb_info->lock);
+	mutex_init(&fb_info->mm_lock);
+
+	fb_info->dev = NULL; /* device file not needed for fbconv. */
+
+	if (fb_info->pixmap.addr = NULL) {
+		fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL);
+		if (fb_info->pixmap.addr) {
+			fb_info->pixmap.size = FBPIXMAPSIZE;
+			fb_info->pixmap.buf_align = 1;
+			fb_info->pixmap.scan_align = 1;
+			fb_info->pixmap.access_align = 32;
+			fb_info->pixmap.flags = FB_PIXMAP_DEFAULT;
+		}
+	}
+	fb_info->pixmap.offset = 0;
+
+	if (!fb_info->pixmap.blit_x)
+		fb_info->pixmap.blit_x = ~(u32)0;
+
+	if (!fb_info->pixmap.blit_y)
+		fb_info->pixmap.blit_y = ~(u32)0;
+
+	if (!fb_info->modelist.prev || !fb_info->modelist.next)
+		INIT_LIST_HEAD(&fb_info->modelist);
+
+	fb_var_to_videomode(&mode, &fb_info->var);
+	fb_add_videomode(&mode, &fb_info->modelist);
+
+	/* matroxfb: Requries a call to fb_set_par() to initialize
+	 * fbinfo->fix.{smem_start,smem_len}. Otherwise, we won't be
+	 * able to initialize framebuffer memory management.
+	 */
+	if (is_matroxfb(fb_info)) {
+		if (fb_info->fbops->fb_set_par)
+			fb_info->fbops->fb_set_par(fb_info);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fbconv_fill_fb_info);
+
+/**
+ * drm_fbconv_cleanup_fb_info - cleans up an fbdev driver's fb_info structure
+ *	after use
+ * @fb_info:	the fb_info structure to clean up
+ */
+void drm_fbconv_cleanup_fb_info(struct fb_info *fb_info)
+{
+	if (fb_info->node != FB_MAX)
+		return;
+
+	if (fb_info->pixmap.addr &&
+	    (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT))
+		kfree(fb_info->pixmap.addr);
+
+	fb_destroy_modelist(&fb_info->modelist);
+
+	if (!atomic_dec_and_test(&fb_info->count))
+		return;
+
+	if (fb_info->fbops->fb_destroy)
+		fb_info->fbops->fb_destroy(fb_info);
+}
+EXPORT_SYMBOL(drm_fbconv_cleanup_fb_info);
diff --git a/include/drm/drm_fbconv_helper.h b/include/drm/drm_fbconv_helper.h
index c7d211f40462..23d17ad14b81 100644
--- a/include/drm/drm_fbconv_helper.h
+++ b/include/drm/drm_fbconv_helper.h
@@ -140,4 +140,11 @@ int drm_fbconv_modeset_setup_pipe(
 	const uint32_t *formats, unsigned int format_count,
 	const uint64_t *format_modifiers, struct drm_connector *connector);
 
+/*
+ * Helpers for struct fb_info
+ */
+
+int drm_fbconv_fill_fb_info(struct fb_info *fb_info);
+void drm_fbconv_cleanup_fb_info(struct fb_info *fb_info);
+
 #endif
-- 
2.23.0

[PATCH v2 12/15] drm/fbconv: Add helper documentation

[Thomas Zimmermann]

There's now a tutorial on how to create a DRM driver on top of fbconv
helpers. The DRM TODO list contains an entry for converting fbdev
drivers over to DRM.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/drm-kms-helpers.rst |  12 ++
 Documentation/gpu/todo.rst            |  15 +++
 drivers/gpu/drm/drm_fbconv_helper.c   | 181 ++++++++++++++++++++++++++
 3 files changed, 208 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 9668a7fe2408..1232a3ef24ff 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -411,3 +411,15 @@ SHMEM GEM Helper Reference
 
 .. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
    :export:
+
+fbdev Conversion Helper Reference
+================+
+.. kernel-doc:: drivers/gpu/drm/drm_fbconv_helper.c
+   :doc: fbconv helpers
+
+.. kernel-doc:: include/drm/drm_fbconv_helper.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_fbconv_helper.c
+   :export:
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 79785559d711..1be44a17f3e8 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -462,3 +462,18 @@ Contact: Sam Ravnborg
 
 Outside DRM
 =====+
+Convert fbdev drivers to DRM
+----------------------------
+
+There are plenty of fbdev drivers for old hardware. With fbconv helpers, we
+have a simple and clean way of transitioning fbdev drivers to DRM. Set up a
+simple DRM driver that builds onto the fbconv helpers, copy over the fbdev
+driver and connect both. This should result in a basic DRM driver that can
+run X11 and Weston.  There's a tutorial for this process with example source
+code in the fbconv documentation.
+
+From there, refactor the driver source code into a clean DRM driver that
+requires neither fbdev nor fbconv helpers.
+
+Contact: Thomas Zimmermann
diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
index 7d7e4da2a29e..1fa240a4789f 100644
--- a/drivers/gpu/drm/drm_fbconv_helper.c
+++ b/drivers/gpu/drm/drm_fbconv_helper.c
@@ -18,6 +18,187 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
+/**
+ * DOC: fbconv helpers
+ *
+ * The Linux kernel's fbdev subsystem provides a wide range of drivers for
+ * older graphics hardware. Except for these existng drivers, fbdev is
+ * deprecated and expected to be removed at some point in the future. All new
+ * development happens in DRM. Some of the fbdev drivers are worth carrying
+ * forward. The fbconv helper functions provide a framework for porting fbdev
+ * drivers to DRM.
+ *
+ * When porting over fbdev drivers to DRM, the most significant problem is the
+ * difference in how the internal driver interfaces work. Fbdev has a single
+ * function, struct fb_ops.fb_set_par(), to set video mode and framebuffer
+ * format. DRM use a much more fine-grained interface. In fbdev, framebuffer
+ * memory is managed by a single client, while in DRM multiple clients can
+ * hold buffers with framebuffer data.
+ *
+ * The fbconv helper library provides a set of data structures and functions
+ * that connect DRM and fbdev. The resulting DRM driver maps DRM operations
+ * to fbdev interfaces and uses an fbdev driver for its hardware operations.
+ * Such a driver is not intended to be merged into DRM as-is. It does,
+ * however, provide a starting point for refactoring the fbdev driver's
+ * implementation into first-class DRM code.
+ *
+ * As an example, we create a DRM driver from vesafb, fbdev's generic
+ * vesa driver. We begin by creating a DRM stub driver vesadrm. Please keep
+ * in mind that the provided code is for illustrative purposes and requires
+ * error handling.
+ *
+ * .. code-block:: c
+ *
+ *	DEFINE_DRM_GEM_SHMEM_FOPS(vesadrm_file_operations);
+ *
+ *	static struct drm_driver vesadrm_driver = {
+ *		.major = 1,
+ *		.minor = 0,
+ *		.patchlevel = 0,
+ *		.name = "vesadrm",
+ *		.desc = "DRM VESA driver",
+ *		.date = "01/01/1970",
+ *		.driver_features = DRIVER_ATOMIC |
+ *				   DRIVER_GEM |
+ *				   DRIVER_MODESET,
+ *		.fops = &vesadrm_file_operations,
+ *		DRM_GEM_SHMEM_DRIVER_OPS,
+ *	};
+ *
+ * Fbconv uses SHMEM, so we set up the structures accordingly.
+ *
+ * The fbdev code usually calls register_framebuffer() and
+ * unregister_framebuffer() to connect and disconnect itself to the fbdev
+ * core code. In our case, we replace these calls with
+ * vesadrm_register_framebuffer() and vesadrm_unregister_framebuffer(), which
+ * serve as entry points for vesafb.
+ *
+ * .. code-block:: c
+ *
+ *	#include <drm/drm/fbconv_helper.h>
+ *
+ *	struct vesadrm_device {
+ *		struct drm_device dev;
+ *		struct fb_info *fb_info;
+ *
+ *		struct drm_fbconv_modeset modeset;
+ *	};
+ *
+ *	struct vesadrm_device* vesadrm_register_framebuffer(struct fb_info *fb_info)
+ *	{
+ *		struct vesadrm *vdev;
+ *
+ *		drm_fbconv_fill_fb_info(fb_info);
+ *
+ *		vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+ *		vesadrm_device_init(vdev, &vesadrm_driver, fb_info)
+ *
+ *		drm_dev_register(&vdev->dev, 0);
+ *
+ *		return vdev;
+ *	}
+ *
+ * Here, we have the first references to fbconf helpers. The instance
+ * of struct drm_fbconv_modeset is the central data structure for fbconv.
+ * Built upon struct drm_simple_display_pipe, it stores most state for the
+ * DRM driver.
+ *
+ * The function vesadrm_register_framebuffer() will later be called by
+ * vesafb code with the fbdev driver's fb_info structure. In core fbdev,
+ * register_framebuffer() would fill fb_info with general state and complete
+ * registration. With fbconv helpers, drm_fbconv_fill_fb_info() does this.
+ * It's a simplified version of the fbdev setup process, without device file
+ * creation, registration, or events. No console is created either.
+ * Finally vesadrm_register_framebuffer() initializes the vesadrm device and
+ * registers the DRM device. At this point, vesadrm is completely initialized.
+ *
+ * For completeness, here's the implementation of
+ * vesadrm_unregister_framebuffer(), which shuts the device down.
+ *
+ * .. code-block:: c
+ *
+ *	void vesadrm_unregister_framebuffer(struct vesadrm_device *vdev)
+ *	{
+ *		struct fb_info *fb_info = vdev->fb_info;
+ *
+ *		vesadrm_device_cleanup(vdev);
+ *		kfree(vdev);
+ *		drm_fbconv_cleanup_fb_info(fb_info);
+ *	}
+ *
+ * Next we need an implementation of vesadrm_device_init() and
+ * vesadrm_device_cleanup(). These functions handle the details of
+ * device configuration and console setup. As all this functionality
+ * is provided by helpers, the actual implementation is fairly small.
+ *
+ * .. code-block:: c
+ *
+ *	static int vesadrm_device_init(struct vesadrm_device *vdev,
+ *				       struct drm_driver *drv,
+ *				       struct fb_info *fb_info)
+ *	{
+ *		static const uint32_t formats[] = {
+ *			DRM_FORMAT_XRGB8888,
+ *			DRM_FORMAT_RGB565
+ *		};
+ *		static unsigned int max_width = 1600;
+ *		static unsigned int max_height = 1200;
+ *		static unsigned int preferred_depth = 32;
+ *
+ *		drm_dev_init(&vdev->dev, drv, fb_info->device);
+ *
+ *		vdev->dev.dev_private = vdev;
+ *		vdev->dev.pdev = container_of(fb_info->device, struct pci_dev, dev);
+ *		vdev->fb_info = fb_info;
+ *
+ *		drm_fbconv_modeset_init(&vdev->modeset, &vdev->dev, fb_info,
+ *					max_width, max_height, preferred_depth);
+ *
+ *		drm_fbconv_modeset_setup_pipe(&vdev->modeset, NULL, formats,
+ *					      ARRAY_SIZE(formats), NULL, NULL);
+ *
+ *		drm_fbdev_generic_setup(&vdev->dev, 0);
+ *
+ *		return 0;
+ *	}
+ *
+ *	static void vesadrm_device_cleanup(struct vesadrm_device *vdev)
+ *	{
+ *		struct drm_device *dev = &vdev->dev;
+ *
+ *		drm_fbconv_modeset_cleanup(&vdev->modeset);
+ *
+ *		drm_dev_fini(dev);
+ *		dev->dev_private = NULL;
+ *	}
+ *
+ * In vesadrm_device_init(), several device-specific constants are declared.
+ * Depending on the hardware, drivers should set them accordingly.
+ * The call to drm_fbconv_modeset_init() initializes fbconv modesetting
+ * helpers with these device constants.
+ *
+ * The drm_fbconv_modeset_setup_pipe() creates the simple display pipe with
+ * the specified color formats. By default, everything is set up
+ * automatically. But the function also accepts format modifiers, a DRM
+ * connector, and call-back functions for struct drm_simple_display_pipe.
+ * So each of these can be refactored individually later on.
+ *
+ * After setting up the fbconv helpers, there's is a call to
+ * drm_fbdev_generic_setup(), which set an initial mode and creates a
+ * framebuffer console.
+ *
+ * The implementation of vesadrm_device_cleanup() is the inverse of the
+ * init function. It cleans up the fbconv modesetting helper and releases
+ * the DRM device.
+ *
+ * What is left is connecting vesafb to vesadrm. As a first step, we need a
+ * copy the vesafb source files into the vesadrm driver and make them compile.
+ * Once this is done, we have to replace the call to register_framebuffer()
+ * with a call to vesadrm_register_framebuffer(), and unregister_framebuffer()
+ * with vesadrm_unregister_framebuffer(). We have now disconnected vesafb from
+ * the fbdev core and run it as part of DRM.
+ */
+
 /*
  * Format conversion helpers
  */
-- 
2.23.0

[PATCH v2 13/15] staging: Add mgakms driver

[Thomas Zimmermann]

The mgakms driver uses DRM's fbconv helpers to provide a DRM driver
for Matrox chipsets. This will allow matroxfb to be refactored into
a first-class DRM driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/staging/Kconfig             |   2 +
 drivers/staging/Makefile            |   1 +
 drivers/staging/mgakms/Kconfig      |  13 +++
 drivers/staging/mgakms/Makefile     |   6 ++
 drivers/staging/mgakms/mga_device.c |  68 +++++++++++++++
 drivers/staging/mgakms/mga_device.h |  30 +++++++
 drivers/staging/mgakms/mga_drv.c    | 129 ++++++++++++++++++++++++++++
 drivers/staging/mgakms/mga_drv.h    |  14 +++
 8 files changed, 263 insertions(+)
 create mode 100644 drivers/staging/mgakms/Kconfig
 create mode 100644 drivers/staging/mgakms/Makefile
 create mode 100644 drivers/staging/mgakms/mga_device.c
 create mode 100644 drivers/staging/mgakms/mga_device.h
 create mode 100644 drivers/staging/mgakms/mga_drv.c
 create mode 100644 drivers/staging/mgakms/mga_drv.h

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 6f1fa4c849a1..fd25596813c5 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -125,4 +125,6 @@ source "drivers/staging/exfat/Kconfig"
 
 source "drivers/staging/qlge/Kconfig"
 
+source "drivers/staging/mgakms/Kconfig"
+
 endif # STAGING
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index a90f9b308c8d..4c98b028ee99 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -53,3 +53,4 @@ obj-$(CONFIG_UWB)		+= uwb/
 obj-$(CONFIG_USB_WUSB)		+= wusbcore/
 obj-$(CONFIG_EXFAT_FS)		+= exfat/
 obj-$(CONFIG_QLGE)		+= qlge/
+obj-$(CONFIG_DRM_MGAKMS)	+= mgakms/
diff --git a/drivers/staging/mgakms/Kconfig b/drivers/staging/mgakms/Kconfig
new file mode 100644
index 000000000000..de23e76317bd
--- /dev/null
+++ b/drivers/staging/mgakms/Kconfig
@@ -0,0 +1,13 @@
+config DRM_MGAKMS
+	tristate "Matrox g200/g400"
+	depends on DRM && PCI
+	select DRM_FBCONV_HELPER
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  Choose this option if you have a Matrox Millennium,
+	  Matrox Millennium II, Matrox Mystique, Matrox Mystique 220,
+	  Matrox Productiva G100, Matrox Mystique G200,
+	  Matrox Millennium G200, Matrox Marvel G200 video, Matrox G400,
+	  G450 or G550 card. If M is selected, the module will be called mga.
+	  AGP support is required for this driver to work.
diff --git a/drivers/staging/mgakms/Makefile b/drivers/staging/mgakms/Makefile
new file mode 100644
index 000000000000..65695f04eb7f
--- /dev/null
+++ b/drivers/staging/mgakms/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+mgakms-y	:= mga_device.o \
+		   mga_drv.o
+
+obj-$(CONFIG_DRM_MGAKMS)	+= mgakms.o
diff --git a/drivers/staging/mgakms/mga_device.c b/drivers/staging/mgakms/mga_device.c
new file mode 100644
index 000000000000..34b3bb1ed8a5
--- /dev/null
+++ b/drivers/staging/mgakms/mga_device.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/fb.h>
+#include <linux/pci.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_modeset_helper.h>
+
+#include "mga_device.h"
+
+/*
+ * struct mga_device
+ */
+
+int mga_device_init(struct mga_device *mdev, struct drm_driver *drv,
+		    struct fb_info *fb_info)
+{
+	static const uint32_t formats[] = {
+		DRM_FORMAT_XRGB8888,
+		DRM_FORMAT_RGB565
+	};
+	static unsigned int max_width = 2048;
+	static unsigned int max_height = 2048;
+	static unsigned int preferred_depth = 32;
+
+	int ret;
+
+	ret = drm_dev_init(&mdev->dev, drv, fb_info->device);
+	if (ret)
+		return ret;
+	mdev->dev.dev_private = mdev;
+	mdev->dev.pdev = container_of(fb_info->device, struct pci_dev, dev);
+	mdev->fb_info = fb_info;
+
+	ret = drm_fbconv_modeset_init(&mdev->modeset, &mdev->dev, fb_info,
+				      max_width, max_height, preferred_depth);
+	if (ret)
+		goto err_drm_dev_fini;
+
+	ret = drm_fbconv_modeset_setup_pipe(&mdev->modeset, NULL, formats,
+					    ARRAY_SIZE(formats), NULL, NULL);
+	if (ret)
+		goto err_drm_fbconv_modeset_cleanup;
+
+	ret = drm_fbdev_generic_setup(&mdev->dev, 0);
+	if (ret)
+		goto err_drm_fbconv_modeset_cleanup;
+
+	return 0;
+
+err_drm_fbconv_modeset_cleanup:
+	/* cleans up all mode-setting structures */
+	drm_fbconv_modeset_cleanup(&mdev->modeset);
+err_drm_dev_fini:
+	drm_dev_fini(&mdev->dev);
+	return ret;
+}
+
+void mga_device_cleanup(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->dev;
+
+	drm_fbconv_modeset_cleanup(&mdev->modeset);
+
+	drm_dev_fini(dev);
+	dev->dev_private = NULL;
+}
diff --git a/drivers/staging/mgakms/mga_device.h b/drivers/staging/mgakms/mga_device.h
new file mode 100644
index 000000000000..442effbf37bc
--- /dev/null
+++ b/drivers/staging/mgakms/mga_device.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef MGA_DEVICE_H
+#define MGA_DEVICE_H
+
+#include <linux/kernel.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_fbconv_helper.h>
+
+struct drm_driver;
+struct fb_info;
+
+struct mga_device {
+	struct drm_device dev;
+	struct fb_info *fb_info;
+
+	struct drm_fbconv_modeset modeset;
+};
+
+static inline struct mga_device *mga_device_of_dev(struct drm_device *dev)
+{
+	return container_of(dev, struct mga_device, dev);
+}
+
+int mga_device_init(struct mga_device *mdev, struct drm_driver *drv,
+		    struct fb_info *fb_info);
+void mga_device_cleanup(struct mga_device *mdev);
+
+#endif
diff --git a/drivers/staging/mgakms/mga_drv.c b/drivers/staging/mgakms/mga_drv.c
new file mode 100644
index 000000000000..75e26d3046f3
--- /dev/null
+++ b/drivers/staging/mgakms/mga_drv.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/fb.h>
+#include <linux/pci.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_fbconv_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_file.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_ioctl.h>
+
+#include "mga_device.h"
+#include "mga_drv.h"
+
+#define DRIVER_AUTHOR		"Thomas Zimmermann <tzimmermann@suse.de>"
+#define DRIVER_NAME		"mgakms"
+#define DRIVER_DESCRIPTION	"DRM driver for Matrox graphics chipsets"
+#define DRIVER_LICENSE		"GPL"
+#define DRIVER_DATE		"20190301"
+#define DRIVER_MAJOR		0
+#define DRIVER_MINOR		0
+#define DRIVER_PATCHLEVEL	1
+
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_SHMEM_FOPS(mga_file_operations);
+
+static struct drm_driver mga_driver = {
+	.major = DRIVER_MAJOR,
+	.minor = DRIVER_MINOR,
+	.patchlevel = DRIVER_PATCHLEVEL,
+	.name = DRIVER_NAME,
+	.desc = DRIVER_DESCRIPTION,
+	.date = DRIVER_DATE,
+	.driver_features = DRIVER_ATOMIC |
+			   DRIVER_GEM |
+			   DRIVER_MODESET,
+	.fops = &mga_file_operations,
+	DRM_GEM_SHMEM_DRIVER_OPS,
+};
+
+static void mga_remove_conflicting_framebuffers(struct pci_dev *pdev)
+{
+	struct apertures_struct *ap;
+	bool primary = false;
+
+	ap = alloc_apertures(1);
+	if (!ap)
+		return;
+
+	ap->ranges[0].base = pci_resource_start(pdev, 1);
+	ap->ranges[0].size = pci_resource_len(pdev, 1);
+
+#ifdef CONFIG_X86
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
+		IORESOURCE_ROM_SHADOW;
+#endif
+	drm_fb_helper_remove_conflicting_framebuffers(ap, DRIVER_NAME,
+						      primary);
+	kfree(ap);
+}
+
+static struct mga_device *mga_create_device(struct fb_info *fb_info)
+{
+	struct mga_device *mdev;
+	int ret;
+
+	mdev = devm_kzalloc(fb_info->device, sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return ERR_PTR(-ENOMEM);
+	ret = mga_device_init(mdev, &mga_driver, fb_info);
+	if (ret)
+		goto err_devm_kfree;
+	return mdev;
+
+err_devm_kfree:
+	devm_kfree(fb_info->device, mdev);
+	return ERR_PTR(ret);
+}
+
+static void mga_destroy_device(struct mga_device *mdev)
+{
+	struct device *dev = mdev->fb_info->device;
+
+	mga_device_cleanup(mdev);
+	devm_kfree(dev, mdev);
+}
+
+struct mga_device *mga_register_framebuffer(struct fb_info *fb_info,
+					    struct pci_dev *pdev)
+{
+	int ret;
+	struct mga_device *mdev;
+
+	mga_remove_conflicting_framebuffers(pdev);
+
+	ret = drm_fbconv_fill_fb_info(fb_info);
+	if (ret)
+		return ERR_PTR(ret);
+
+	mdev = mga_create_device(fb_info);
+	if (IS_ERR(mdev)) {
+		ret = PTR_ERR(mdev);
+		goto err_drm_fbconv_cleanup_fb_info;
+	}
+
+	ret = drm_dev_register(&mdev->dev, 0);
+	if (ret)
+		goto err_mga_destroy_device;
+
+	return mdev;
+
+err_mga_destroy_device:
+	mga_destroy_device(mdev);
+err_drm_fbconv_cleanup_fb_info:
+	drm_fbconv_cleanup_fb_info(fb_info);
+	return ERR_PTR(ret);
+}
+
+void mga_unregister_framebuffer(struct mga_device *mdev)
+{
+	struct fb_info *fb_info = mdev->fb_info;
+
+	mga_destroy_device(mdev);
+	drm_fbconv_cleanup_fb_info(fb_info);
+}
diff --git a/drivers/staging/mgakms/mga_drv.h b/drivers/staging/mgakms/mga_drv.h
new file mode 100644
index 000000000000..d214719516c0
--- /dev/null
+++ b/drivers/staging/mgakms/mga_drv.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef MGA_DRV_H
+#define MGA_DRV_H
+
+struct fb_info;
+struct mga_device;
+struct pci_dev;
+
+struct mga_device *mga_register_framebuffer(struct fb_info *fb_info,
+					    struct pci_dev *pdev);
+void mga_unregister_framebuffer(struct mga_device *mdev);
+
+#endif
-- 
2.23.0

[PATCH v2 15/15] staging/mgakms: Update matroxfb driver code for DRM

[Thomas Zimmermann]

With the update, all driver code coming from matroxfb is build
unconditionally. The driver registers itself with the mgakms driver,
instead of the fbdev core.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/staging/mgakms/Kconfig            |  5 ++
 drivers/staging/mgakms/Makefile           | 11 ++++
 drivers/staging/mgakms/matroxfb_DAC1064.c | 77 +++++++----------------
 drivers/staging/mgakms/matroxfb_DAC1064.h |  6 --
 drivers/staging/mgakms/matroxfb_Ti3026.c  |  2 -
 drivers/staging/mgakms/matroxfb_Ti3026.h  |  2 -
 drivers/staging/mgakms/matroxfb_base.c    | 29 +++------
 drivers/staging/mgakms/matroxfb_base.h    | 20 ++----
 drivers/staging/mgakms/matroxfb_g450.h    |  5 --
 9 files changed, 51 insertions(+), 106 deletions(-)

diff --git a/drivers/staging/mgakms/Kconfig b/drivers/staging/mgakms/Kconfig
index de23e76317bd..66c5b3b8669f 100644
--- a/drivers/staging/mgakms/Kconfig
+++ b/drivers/staging/mgakms/Kconfig
@@ -4,6 +4,11 @@ config DRM_MGAKMS
 	select DRM_FBCONV_HELPER
 	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
+	select FB_CFB_COPYAREA
+	select FB_CFB_FILLRECT
+	select FB_CFB_IMAGEBLIT
+	select FB_MACMODES if PPC_PMAC
+	select FB_TILEBLITTING
 	help
 	  Choose this option if you have a Matrox Millennium,
 	  Matrox Millennium II, Matrox Mystique, Matrox Mystique 220,
diff --git a/drivers/staging/mgakms/Makefile b/drivers/staging/mgakms/Makefile
index 65695f04eb7f..b368f8a5e38a 100644
--- a/drivers/staging/mgakms/Makefile
+++ b/drivers/staging/mgakms/Makefile
@@ -3,4 +3,15 @@
 mgakms-y	:= mga_device.o \
 		   mga_drv.o
 
+# Old matroxfb driver code. Keep this separate and
+# remove it after conversion.
+mgakms-y	+= g450_pll.o \
+		   i2c-matroxfb.o \
+		   matroxfb_accel.o \
+		   matroxfb_base.o \
+		   matroxfb_DAC1064.o \
+		   matroxfb_g450.o \
+		   matroxfb_misc.o \
+		   matroxfb_Ti3026.o
+
 obj-$(CONFIG_DRM_MGAKMS)	+= mgakms.o
diff --git a/drivers/staging/mgakms/matroxfb_DAC1064.c b/drivers/staging/mgakms/matroxfb_DAC1064.c
index b380a393cbc3..6bb214c548af 100644
--- a/drivers/staging/mgakms/matroxfb_DAC1064.c
+++ b/drivers/staging/mgakms/matroxfb_DAC1064.c
@@ -19,7 +19,6 @@
 #include "g450_pll.h"
 #include <linux/matroxfb.h>
 
-#ifdef NEED_DAC1064
 #define outDAC1064 matroxfb_DAC_out
 #define inDAC1064 matroxfb_DAC_in
 
@@ -42,11 +41,11 @@ static void DAC1064_calcclock(const struct matrox_fb_info *minfo,
 	unsigned int p;
 
 	DBG(__func__)
-	
+
 	/* only for devices older than G450 */
 
 	fvco = PLL_calcclock(minfo, freq, fmax, in, feed, &p);
-	
+
 	p = (1 << p) - 1;
 	if (fvco <= 100000)
 		;
@@ -160,7 +159,6 @@ static void DAC1064_setmclk(struct matrox_fb_info *minfo, int oscinfo,
 	hw->MXoptionReg = mx;
 }
 
-#ifdef CONFIG_FB_MATROX_G
 static void g450_set_plls(struct matrox_fb_info *minfo)
 {
 	u_int32_t c2_ctl;
@@ -168,7 +166,7 @@ static void g450_set_plls(struct matrox_fb_info *minfo)
 	struct matrox_hw_state *hw = &minfo->hw;
 	int pixelmnp;
 	int videomnp;
-	
+
 	c2_ctl = hw->crtc2.ctl & ~0x4007;	/* Clear PLL + enable for CRTC2 */
 	c2_ctl |= 0x0001;			/* Enable CRTC2 */
 	hw->DACreg[POS1064_XPWRCTRL] &= ~0x02;	/* Stop VIDEO PLL */
@@ -191,7 +189,7 @@ static void g450_set_plls(struct matrox_fb_info *minfo)
 		}
 		c2_ctl |=  0x0006;	/* Use video PLL */
 		hw->DACreg[POS1064_XPWRCTRL] |= 0x02;
-		
+
 		outDAC1064(minfo, M1064_XPWRCTRL, hw->DACreg[POS1064_XPWRCTRL]);
 		matroxfb_g450_setpll_cond(minfo, videomnp, M_VIDEO_PLL);
 	}
@@ -199,7 +197,7 @@ static void g450_set_plls(struct matrox_fb_info *minfo)
 	hw->DACreg[POS1064_XPIXCLKCTRL] &= ~M1064_XPIXCLKCTRL_PLL_UP;
 	if (pixelmnp >= 0) {
 		hw->DACreg[POS1064_XPIXCLKCTRL] |= M1064_XPIXCLKCTRL_PLL_UP;
-		
+
 		outDAC1064(minfo, M1064_XPIXCLKCTRL, hw->DACreg[POS1064_XPIXCLKCTRL]);
 		matroxfb_g450_setpll_cond(minfo, pixelmnp, M_PIXEL_PLL_C);
 	}
@@ -251,7 +249,6 @@ static void g450_set_plls(struct matrox_fb_info *minfo)
 		}
 	}
 }
-#endif
 
 void DAC1064_global_init(struct matrox_fb_info *minfo)
 {
@@ -260,7 +257,6 @@ void DAC1064_global_init(struct matrox_fb_info *minfo)
 	hw->DACreg[POS1064_XMISCCTRL] &= M1064_XMISCCTRL_DAC_WIDTHMASK;
 	hw->DACreg[POS1064_XMISCCTRL] |= M1064_XMISCCTRL_LUT_EN;
 	hw->DACreg[POS1064_XPIXCLKCTRL] = M1064_XPIXCLKCTRL_PLL_UP | M1064_XPIXCLKCTRL_EN | M1064_XPIXCLKCTRL_SRC_PLL;
-#ifdef CONFIG_FB_MATROX_G
 	if (minfo->devflags.g450dac) {
 		hw->DACreg[POS1064_XPWRCTRL] = 0x1F;	/* powerup everything */
 		hw->DACreg[POS1064_XOUTPUTCONN] = 0x00;	/* disable outputs */
@@ -302,15 +298,14 @@ void DAC1064_global_init(struct matrox_fb_info *minfo)
 				   poweroff TMDS. But if we boot with DFP connected,
 				   TMDS generated clocks are used instead of ALL pixclocks
 				   available... If someone knows which register
-				   handles it, please reveal this secret to me... */			
+				   handles it, please reveal this secret to me... */
 				hw->DACreg[POS1064_XPWRCTRL] &= ~0x04;		/* Poweroff TMDS */
-#endif				
+#endif
 				break;
 		}
 		/* Now set timming related variables... */
 		g450_set_plls(minfo);
 	} else
-#endif
 	{
 		if (minfo->outputs[1].src = MATROXFB_SRC_CRTC1) {
 			hw->DACreg[POS1064_XPIXCLKCTRL] = M1064_XPIXCLKCTRL_PLL_UP | M1064_XPIXCLKCTRL_EN | M1064_XPIXCLKCTRL_SRC_EXT;
@@ -510,7 +505,6 @@ static struct matrox_altout m1064 = {
 	.compute = m1064_compute,
 };
 
-#ifdef CONFIG_FB_MATROX_G
 static int g450_compute(void* out, struct my_timming* m) {
 #define minfo ((struct matrox_fb_info*)out)
 	if (m->mnp < 0) {
@@ -527,11 +521,7 @@ static struct matrox_altout g450out = {
 	.name	 = "Primary output",
 	.compute = g450_compute,
 };
-#endif
 
-#endif /* NEED_DAC1064 */
-
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 static int MGA1064_init(struct matrox_fb_info *minfo, struct my_timming *m)
 {
 	struct matrox_hw_state *hw = &minfo->hw;
@@ -552,9 +542,7 @@ static int MGA1064_init(struct matrox_fb_info *minfo, struct my_timming *m)
 	if (DAC1064_init_2(minfo, m)) return 1;
 	return 0;
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_G
 static int MGAG100_init(struct matrox_fb_info *minfo, struct my_timming *m)
 {
 	struct matrox_hw_state *hw = &minfo->hw;
@@ -576,9 +564,7 @@ static int MGAG100_init(struct matrox_fb_info *minfo, struct my_timming *m)
 	if (DAC1064_init_2(minfo, m)) return 1;
 	return 0;
 }
-#endif	/* G */
 
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 static void MGA1064_ramdac_init(struct matrox_fb_info *minfo)
 {
 
@@ -596,9 +582,7 @@ static void MGA1064_ramdac_init(struct matrox_fb_info *minfo)
 	/* maybe cmdline MCLK= ?, doc says gclkDMHz, mclkfMHz... it was 55/83 with old values */
 	DAC1064_setmclk(minfo, DAC1064_OPT_MDIV2 | DAC1064_OPT_GDIV3 | DAC1064_OPT_SCLK_PLL, 133333);
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_G
 /* BIOS environ */
 static int x7AF4 = 0x10;	/* flags, maybe 0x10 = SDRAM, 0x00 = SGRAM??? */
 				/* G100 wants 0x10, G200 SGRAM does not care... */
@@ -662,9 +646,7 @@ static void MGAG100_setPixClock(const struct matrox_fb_info *minfo, int flags,
 	DAC1064_calcclock(minfo, freq, minfo->max_pixel_clock, &m, &n, &p);
 	MGAG100_progPixClock(minfo, flags, m, n, p);
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 static int MGA1064_preinit(struct matrox_fb_info *minfo)
 {
 	static const int vxres_mystique[] = { 512,        640, 768,  800,  832,  960,
@@ -710,9 +692,7 @@ static void MGA1064_reset(struct matrox_fb_info *minfo)
 
 	MGA1064_ramdac_init(minfo);
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_G
 static void g450_mclk_init(struct matrox_fb_info *minfo)
 {
 	/* switch all clocks to PCI source */
@@ -727,14 +707,14 @@ static void g450_mclk_init(struct matrox_fb_info *minfo)
 	} else {
 		unsigned long flags;
 		unsigned int pwr;
-		
+
 		matroxfb_DAC_lock_irqsave(flags);
 		pwr = inDAC1064(minfo, M1064_XPWRCTRL) & ~0x02;
 		outDAC1064(minfo, M1064_XPWRCTRL, pwr);
 		matroxfb_DAC_unlock_irqrestore(flags);
 	}
 	matroxfb_g450_setclk(minfo, minfo->values.pll.system, M_SYSTEM_PLL);
-	
+
 	/* switch clocks to their real PLL source(s) */
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION_REG, minfo->hw.MXoptionReg | 4);
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION3_REG, minfo->values.reg.opt3);
@@ -747,15 +727,15 @@ static void g450_memory_init(struct matrox_fb_info *minfo)
 	/* disable memory refresh */
 	minfo->hw.MXoptionReg &= ~0x001F8000;
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION_REG, minfo->hw.MXoptionReg);
-	
+
 	/* set memory interface parameters */
 	minfo->hw.MXoptionReg &= ~0x00207E00;
 	minfo->hw.MXoptionReg |= 0x00207E00 & minfo->values.reg.opt;
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION_REG, minfo->hw.MXoptionReg);
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION2_REG, minfo->values.reg.opt2);
-	
+
 	mga_outl(M_CTLWTST, minfo->values.reg.mctlwtst);
-	
+
 	/* first set up memory interface with disabled memory interface clocks */
 	pci_write_config_dword(minfo->pcidev, PCI_MEMMISC_REG, minfo->values.reg.memmisc & ~0x80000000U);
 	mga_outl(M_MEMRDBK, minfo->values.reg.memrdbk);
@@ -764,25 +744,25 @@ static void g450_memory_init(struct matrox_fb_info *minfo)
 	pci_write_config_dword(minfo->pcidev, PCI_MEMMISC_REG, minfo->values.reg.memmisc | 0x80000000U);
 
 	udelay(200);
-	
+
 	if (minfo->values.memory.ddr && (!minfo->values.memory.emrswen || !minfo->values.memory.dll)) {
 		mga_outl(M_MEMRDBK, minfo->values.reg.memrdbk & ~0x1000);
 	}
 	mga_outl(M_MACCESS, minfo->values.reg.maccess | 0x8000);
-	
+
 	udelay(200);
-	
+
 	minfo->hw.MXoptionReg |= 0x001F8000 & minfo->values.reg.opt;
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION_REG, minfo->hw.MXoptionReg);
-	
+
 	/* value is written to memory chips only if old != new */
 	mga_outl(M_PLNWT, 0);
 	mga_outl(M_PLNWT, ~0);
-	
+
 	if (minfo->values.reg.mctlwtst != minfo->values.reg.mctlwtst_core) {
 		mga_outl(M_CTLWTST, minfo->values.reg.mctlwtst_core);
 	}
-	
+
 }
 
 static void g450_preinit(struct matrox_fb_info *minfo)
@@ -790,7 +770,7 @@ static void g450_preinit(struct matrox_fb_info *minfo)
 	u_int32_t c2ctl;
 	u_int8_t curctl;
 	u_int8_t c1ctl;
-	
+
 	/* minfo->hw.MXoptionReg = minfo->values.reg.opt; */
 	minfo->hw.MXoptionReg &= 0xC0000100;
 	minfo->hw.MXoptionReg |= 0x00000020;
@@ -804,7 +784,7 @@ static void g450_preinit(struct matrox_fb_info *minfo)
 	pci_write_config_dword(minfo->pcidev, PCI_OPTION_REG, minfo->hw.MXoptionReg);
 
 	/* Init system clocks */
-		
+
 	/* stop crtc2 */
 	c2ctl = mga_inl(M_C2CTL);
 	mga_outl(M_C2CTL, c2ctl & ~1);
@@ -817,20 +797,20 @@ static void g450_preinit(struct matrox_fb_info *minfo)
 
 	g450_mclk_init(minfo);
 	g450_memory_init(minfo);
-	
+
 	/* set legacy VGA clock sources for DOSEmu or VMware... */
 	matroxfb_g450_setclk(minfo, 25175, M_PIXEL_PLL_A);
 	matroxfb_g450_setclk(minfo, 28322, M_PIXEL_PLL_B);
 
 	/* restore crtc1 */
 	mga_setr(M_SEQ_INDEX, 1, c1ctl);
-	
+
 	/* restore cursor */
 	outDAC1064(minfo, M1064_XCURCTRL, curctl);
 
 	/* restore crtc2 */
 	mga_outl(M_C2CTL, c2ctl);
-	
+
 	return;
 }
 
@@ -1031,9 +1011,7 @@ static void MGAG100_reset(struct matrox_fb_info *minfo)
 		}
 	}
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 static void MGA1064_restore(struct matrox_fb_info *minfo)
 {
 	int i;
@@ -1058,9 +1036,7 @@ static void MGA1064_restore(struct matrox_fb_info *minfo)
 		mga_setr(M_EXTVGA_INDEX, i, hw->CRTCEXT[i]);
 	DAC1064_restore_2(minfo);
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_G
 static void MGAG100_restore(struct matrox_fb_info *minfo)
 {
 	int i;
@@ -1084,9 +1060,7 @@ static void MGAG100_restore(struct matrox_fb_info *minfo)
 		mga_setr(M_EXTVGA_INDEX, i, hw->CRTCEXT[i]);
 	DAC1064_restore_2(minfo);
 }
-#endif
 
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 struct matrox_switch matrox_mystique = {
 	.preinit	= MGA1064_preinit,
 	.reset		= MGA1064_reset,
@@ -1094,9 +1068,7 @@ struct matrox_switch matrox_mystique = {
 	.restore	= MGA1064_restore,
 };
 EXPORT_SYMBOL(matrox_mystique);
-#endif
 
-#ifdef CONFIG_FB_MATROX_G
 struct matrox_switch matrox_G100 = {
 	.preinit	= MGAG100_preinit,
 	.reset		= MGAG100_reset,
@@ -1104,10 +1076,7 @@ struct matrox_switch matrox_G100 = {
 	.restore	= MGAG100_restore,
 };
 EXPORT_SYMBOL(matrox_G100);
-#endif
 
-#ifdef NEED_DAC1064
 EXPORT_SYMBOL(DAC1064_global_init);
 EXPORT_SYMBOL(DAC1064_global_restore);
-#endif
 MODULE_LICENSE("GPL");
diff --git a/drivers/staging/mgakms/matroxfb_DAC1064.h b/drivers/staging/mgakms/matroxfb_DAC1064.h
index 3b2a6fd35fff..3d388941b51d 100644
--- a/drivers/staging/mgakms/matroxfb_DAC1064.h
+++ b/drivers/staging/mgakms/matroxfb_DAC1064.h
@@ -5,16 +5,10 @@
 
 #include "matroxfb_base.h"
 
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 extern struct matrox_switch matrox_mystique;
-#endif
-#ifdef CONFIG_FB_MATROX_G
 extern struct matrox_switch matrox_G100;
-#endif
-#ifdef NEED_DAC1064
 void DAC1064_global_init(struct matrox_fb_info *minfo);
 void DAC1064_global_restore(struct matrox_fb_info *minfo);
-#endif
 
 #define M1064_INDEX	0x00
 #define M1064_PALWRADD	0x00
diff --git a/drivers/staging/mgakms/matroxfb_Ti3026.c b/drivers/staging/mgakms/matroxfb_Ti3026.c
index 9ff9be85759e..e82e2a3cf76a 100644
--- a/drivers/staging/mgakms/matroxfb_Ti3026.c
+++ b/drivers/staging/mgakms/matroxfb_Ti3026.c
@@ -84,7 +84,6 @@
 #include "matroxfb_accel.h"
 #include <linux/matroxfb.h>
 
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 #define outTi3026 matroxfb_DAC_out
 #define inTi3026 matroxfb_DAC_in
 
@@ -744,5 +743,4 @@ struct matrox_switch matrox_millennium = {
 	.restore	= Ti3026_restore
 };
 EXPORT_SYMBOL(matrox_millennium);
-#endif
 MODULE_LICENSE("GPL");
diff --git a/drivers/staging/mgakms/matroxfb_Ti3026.h b/drivers/staging/mgakms/matroxfb_Ti3026.h
index faee149d0ba0..2fda3b30b9e7 100644
--- a/drivers/staging/mgakms/matroxfb_Ti3026.h
+++ b/drivers/staging/mgakms/matroxfb_Ti3026.h
@@ -5,8 +5,6 @@
 
 #include "matroxfb_base.h"
 
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 extern struct matrox_switch matrox_millennium;
-#endif
 
 #endif	/* __MATROXFB_TI3026_H__ */
diff --git a/drivers/staging/mgakms/matroxfb_base.c b/drivers/staging/mgakms/matroxfb_base.c
index d11b5e6210ed..7ee0ea046a5f 100644
--- a/drivers/staging/mgakms/matroxfb_base.c
+++ b/drivers/staging/mgakms/matroxfb_base.c
@@ -114,6 +114,7 @@
 #include <linux/nvram.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include "mga_drv.h"
 
 #ifdef CONFIG_PPC_PMAC
 #include <asm/machdep.h>
@@ -368,7 +369,7 @@ static void matroxfb_remove(struct matrox_fb_info *minfo, int dummy)
 		return;
 	}
 	matroxfb_unregister_device(minfo);
-	unregister_framebuffer(&minfo->fbcon);
+	mga_unregister_framebuffer(minfo->mdev);
 	matroxfb_g450_shutdown(minfo);
 	arch_phys_wc_del(minfo->wc_cookie);
 	iounmap(minfo->mmio.vbase.vaddr);
@@ -1319,9 +1320,7 @@ static int matroxfb_getmemory(struct matrox_fb_info *minfo,
 	mga_outb(M_EXTVGA_DATA, orig);
 
 	*realSize = offs - 0x100000;
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 	minfo->interleave = !(!isMillenium(minfo) || ((offs - 0x100000) & 0x3FFFFF));
-#endif
 	return 1;
 }
 
@@ -1331,7 +1330,6 @@ struct video_board {
 	int accelID;
 	struct matrox_switch* lowlevel;
 		 };
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 static struct video_board vbMillennium = {
 	.maxvram = 0x0800000,
 	.maxdisplayable = 0x0800000,
@@ -1352,16 +1350,12 @@ static struct video_board vbMillennium2A = {
 	.accelID = FB_ACCEL_MATROX_MGA2164W_AGP,
 	.lowlevel = &matrox_millennium
 };
-#endif	/* CONFIG_FB_MATROX_MILLENIUM */
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 static struct video_board vbMystique = {
 	.maxvram = 0x0800000,
 	.maxdisplayable = 0x0800000,
 	.accelID = FB_ACCEL_MATROX_MGA1064SG,
 	.lowlevel = &matrox_mystique
 };
-#endif	/* CONFIG_FB_MATROX_MYSTIQUE */
-#ifdef CONFIG_FB_MATROX_G
 static struct video_board vbG100 = {
 	.maxvram = 0x0800000,
 	.maxdisplayable = 0x0800000,
@@ -1383,7 +1377,6 @@ static struct video_board vbG400 = {
 	.accelID = FB_ACCEL_MATROX_MGAG400,
 	.lowlevel = &matrox_G100
 };
-#endif
 
 #define DEVF_VIDEO64BIT		0x0001
 #define	DEVF_SWAPS		0x0002
@@ -1418,7 +1411,6 @@ static struct board {
 	struct video_board* base;
 	const char* name;
 		} dev_list[] = {
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MIL,	0xFF,
 		0,			0,
 		DEVF_TEXT4B,
@@ -1440,8 +1432,6 @@ static struct board {
 		MGA_2164,
 		&vbMillennium2A,
 		"Millennium II (AGP)"},
-#endif
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MYS,	0x02,
 		0,			0,
 		DEVF_VIDEO64BIT | DEVF_CROSS4MB,
@@ -1470,8 +1460,6 @@ static struct board {
 		MGA_1164,
 		&vbMystique,
 		"Mystique 220 (AGP)"},
-#endif
-#ifdef CONFIG_FB_MATROX_G
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_G100_MM,	0xFF,
 		0,			0,
 		DEVF_G100,
@@ -1563,7 +1551,6 @@ static struct board {
 		MGA_G550,
 		&vbG400,
 		"G550"},
-#endif
 	{0,			0,				0xFF,
 		0,			0,
 		0,
@@ -1622,6 +1609,7 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
 	unsigned long video_base_phys = 0;
 	unsigned int memsize;
 	int err;
+	struct mga_device *mdev;
 
 	static const struct pci_device_id intel_82437[] = {
 		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82437) },
@@ -1918,9 +1906,12 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
  * and we do not want currcon = 0 for subsequent framebuffers */
 
 	minfo->fbcon.device = &minfo->pcidev->dev;
-	if (register_framebuffer(&minfo->fbcon) < 0) {
+	mdev = mga_register_framebuffer(&minfo->fbcon, minfo->pcidev);
+	if (IS_ERR(mdev)) {
+		err = PTR_ERR(mdev);
 		goto failVideoIO;
 	}
+	minfo->mdev = mdev;
 	fb_info(&minfo->fbcon, "%s frame buffer device\n", minfo->fbcon.fix.id);
 
 	/* there is no console on this fb... but we have to initialize hardware
@@ -2116,19 +2107,14 @@ static void pci_remove_matrox(struct pci_dev* pdev) {
 }
 
 static const struct pci_device_id matroxfb_devices[] = {
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MIL,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MIL_2,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MIL_2_AGP,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
-#endif
-#ifdef CONFIG_FB_MATROX_MYSTIQUE
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_MYS,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
-#endif
-#ifdef CONFIG_FB_MATROX_G
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_G100_MM,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_G100_AGP,
@@ -2141,7 +2127,6 @@ static const struct pci_device_id matroxfb_devices[] = {
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
 	{PCI_VENDOR_ID_MATROX,	PCI_DEVICE_ID_MATROX_G550,
 		PCI_ANY_ID,	PCI_ANY_ID,	0, 0, 0},
-#endif
 	{0,			0,
 		0,		0,		0, 0, 0}
 };
diff --git a/drivers/staging/mgakms/matroxfb_base.h b/drivers/staging/mgakms/matroxfb_base.h
index f85ad25659e5..f7548549b3a6 100644
--- a/drivers/staging/mgakms/matroxfb_base.h
+++ b/drivers/staging/mgakms/matroxfb_base.h
@@ -115,11 +115,7 @@
 
 #define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)
 
-/* G-series and Mystique have (almost) same DAC */
-#undef NEED_DAC1064
-#if defined(CONFIG_FB_MATROX_MYSTIQUE) || defined(CONFIG_FB_MATROX_G)
-#define NEED_DAC1064 1
-#endif
+struct mga_device;
 
 typedef struct {
 	void __iomem*	vaddr;
@@ -282,9 +278,7 @@ struct matrox_hw_state {
 };
 
 struct matrox_accel_data {
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 	unsigned char	ramdac_rev;
-#endif
 	u_int32_t	m_dwg_rect;
 	u_int32_t	m_opmode;
 	u_int32_t	m_access;
@@ -302,9 +296,9 @@ struct matrox_altout {
 	int		(*verifymode)(void* altout_dev, u_int32_t mode);
 	int		(*getqueryctrl)(void* altout_dev,
 					struct v4l2_queryctrl* ctrl);
-	int		(*getctrl)(void* altout_dev, 
+	int		(*getctrl)(void* altout_dev,
 				   struct v4l2_control* ctrl);
-	int		(*setctrl)(void* altout_dev, 
+	int		(*setctrl)(void* altout_dev,
 				   struct v4l2_control* ctrl);
 };
 
@@ -338,6 +332,8 @@ struct matrox_vsync {
 struct matrox_fb_info {
 	struct fb_info		fbcon;
 
+	struct mga_device	*mdev;
+
 	struct list_head	next_fb;
 
 	int			dead;
@@ -676,15 +672,9 @@ void matroxfb_unregister_driver(struct matroxfb_driver* drv);
 #define WaitTillIdle()	do { mga_inl(M_STATUS); do {} while (mga_inl(M_STATUS) & 0x10000); } while (0)
 
 /* code speedup */
-#ifdef CONFIG_FB_MATROX_MILLENIUM
 #define isInterleave(x)	 (x->interleave)
 #define isMillenium(x)	 (x->millenium)
 #define isMilleniumII(x) (x->milleniumII)
-#else
-#define isInterleave(x)  (0)
-#define isMillenium(x)	 (0)
-#define isMilleniumII(x) (0)
-#endif
 
 #define matroxfb_DAC_lock()                   spin_lock(&minfo->lock.DAC)
 #define matroxfb_DAC_unlock()                 spin_unlock(&minfo->lock.DAC)
diff --git a/drivers/staging/mgakms/matroxfb_g450.h b/drivers/staging/mgakms/matroxfb_g450.h
index b5f17b86eae5..2cec778a2a81 100644
--- a/drivers/staging/mgakms/matroxfb_g450.h
+++ b/drivers/staging/mgakms/matroxfb_g450.h
@@ -4,12 +4,7 @@
 
 #include "matroxfb_base.h"
 
-#ifdef CONFIG_FB_MATROX_G
 void matroxfb_g450_connect(struct matrox_fb_info *minfo);
 void matroxfb_g450_shutdown(struct matrox_fb_info *minfo);
-#else
-static inline void matroxfb_g450_connect(struct matrox_fb_info *minfo) { };
-static inline void matroxfb_g450_shutdown(struct matrox_fb_info *minfo) { };
-#endif
 
 #endif /* __MATROXFB_G450_H__ */
-- 
2.23.0

Re: [PATCH v2 05/15] drm/fbconv: Add DRM <-> fbdev pixel-format conversion

[Sam Ravnborg]

Hi Thomas.

On Mon, Oct 14, 2019 at 04:04:06PM +0200, Thomas Zimmermann wrote:
> DRM uses FOURCC constants to describe pixel formats, fbdev uses a
> per-component bitfield structure. The functions in this patch convert
> between the two.
> 

A few nits below.


> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fbconv_helper.c | 435 ++++++++++++++++++++++++++++
>  include/drm/drm_fbconv_helper.h     |  23 ++
>  2 files changed, 458 insertions(+)
>  create mode 100644 include/drm/drm_fbconv_helper.h
> 
> diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
> index 0cb46d2c98c3..af45358a156a 100644
> --- a/drivers/gpu/drm/drm_fbconv_helper.c
> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
> @@ -1 +1,436 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <asm/byteorder.h>
> +
> +#include <linux/fb.h>

<asm/*> after <linux/*>
So we in this way pick the more general include file first.

> +
> +struct format_map {
> +	bool (*is_format)(const struct fb_var_screeninfo *fb_var);
> +	uint32_t format;
> +};
We are in the kernel - where I think u32 is preferred over the longer
uint32_t.
If I grep in drm/* then they seems be be equally popular, so feel free
to ignore this comment.


> +static void set_fb_bitfield(struct fb_bitfield *bits, __u32 offset,
> +			    __u32 length)

This is not uapi - so u32 is preferred.

Both comments apply to the whole file.

I did not see that this was wired into the kernel-doc in Documentation/
but maybe I just missed it.

With my comments considered you can add:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

All code looks sane, but as I have not grasped the bigger picture
this can hardly be a review.

	Sam

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Sam Ravnborg]

Hi Thomas.

On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> (was: DRM driver for fbdev devices)
> 
> This is version 2 of the fbdev conversion helpers. It's more or less a
> rewrite of the original patchset.
> 
> The fbdev subsystem is considered legacy and will probably be removed at
> some point. This would mean the loss of a signifanct number of drivers.
> Some of the affected hardware is not in use any longer, but some hardware
> is still around and provides good(-enough) framebuffers.
> 
> The fbconv helpers allow for running the current DRM stack on top of fbdev
> drivers. It's a set of functions that convert between fbdev interfaces and
> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> basic functionality of a framebuffer, but should be compatible with most
> existing fbdev drivers.
> 
> A DRM driver using fbconv helpers consists of
> 
>   * DRM stub code that calls into fbconv helpers, and
>   * the original fbdev driver code.
> 
> The fbdev driver code has to be modified to register itself with the
> stub driver instead of the fbdev core framework. A tutorial on how to use
> the helpers is part of this patchset. The resulting driver hybrid can be
> refactored into a first-class DRM driver. The fbconv helpers contain a
> number of comments, labeled 'DRM porting note', which explain the required
> steps.
> 
> I tested the current patchset with the following drivers: atyfb, aty128fb,
> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> With each, I was able to successfully start with fbcon enabled, run weston and
> X11. The drivers are available at [1]. For reference, the patchset includes
> the Matrox stub driver.

In general I like the idea of modernizing the existing fbdev drivers.
What I fail to read in your intro above is if this allows us to phase
out the migrated fbdev drivers sooner?
Or do we end up with two drivers to maintain?

Obviously a full migration to a DRM driver was preferred - but this may
serve as a step in that direction.
But we should not end up with two drivers doing almost the same.

Another general question. Do we want the modernized DRM drivers to end
up in staging? Why should they not go direct into drm/*
I know they are not fully atomic but this is not new drivers so maybe
they can be excused.
Problem is that drm drivers in staging live a secret nonvisible life
where they are easy to forget when we change interfaces and such.

	Sam

Re: [PATCH v2 05/15] drm/fbconv: Add DRM <-> fbdev pixel-format conversion

[Thomas Zimmermann]

Hi

Am 14.10.19 um 22:30 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Oct 14, 2019 at 04:04:06PM +0200, Thomas Zimmermann wrote:
>> DRM uses FOURCC constants to describe pixel formats, fbdev uses a
>> per-component bitfield structure. The functions in this patch convert
>> between the two.
>>
> 
> A few nits below.
> 
> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_fbconv_helper.c | 435 ++++++++++++++++++++++++++++
>>   include/drm/drm_fbconv_helper.h     |  23 ++
>>   2 files changed, 458 insertions(+)
>>   create mode 100644 include/drm/drm_fbconv_helper.h
>>
>> diff --git a/drivers/gpu/drm/drm_fbconv_helper.c b/drivers/gpu/drm/drm_fbconv_helper.c
>> index 0cb46d2c98c3..af45358a156a 100644
>> --- a/drivers/gpu/drm/drm_fbconv_helper.c
>> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
>> @@ -1 +1,436 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +#include <asm/byteorder.h>
>> +
>> +#include <linux/fb.h>
> 
> <asm/*> after <linux/*>
> So we in this way pick the more general include file first.

Ok.

>> +
>> +struct format_map {
>> +	bool (*is_format)(const struct fb_var_screeninfo *fb_var);
>> +	uint32_t format;
>> +};
> We are in the kernel - where I think u32 is preferred over the longer
> uint32_t.
> If I grep in drm/* then they seems be be equally popular, so feel free
> to ignore this comment.

I generally use types that are used by related interfaces. Here it's 
uint32_t because most other places use uint32_t for storing DRM_FORMAT 
constants.

> 
>> +static void set_fb_bitfield(struct fb_bitfield *bits, __u32 offset,
>> +			    __u32 length)
> 
> This is not uapi - so u32 is preferred.

Same as above. The __u32 comes from the fb_bitfield structure.

Best regards
Thomas

> Both comments apply to the whole file.
> 
> I did not see that this was wired into the kernel-doc in Documentation/
> but maybe I just missed it.
> 
> With my comments considered you can add:
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
> All code looks sane, but as I have not grasped the bigger picture
> this can hardly be a review.
> 
> 	Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Thomas Zimmermann]

Hi

Am 14.10.19 um 22:36 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
>> (was: DRM driver for fbdev devices)
>>
>> This is version 2 of the fbdev conversion helpers. It's more or less a
>> rewrite of the original patchset.
>>
>> The fbdev subsystem is considered legacy and will probably be removed at
>> some point. This would mean the loss of a signifanct number of drivers.
>> Some of the affected hardware is not in use any longer, but some hardware
>> is still around and provides good(-enough) framebuffers.
>>
>> The fbconv helpers allow for running the current DRM stack on top of fbdev
>> drivers. It's a set of functions that convert between fbdev interfaces and
>> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
>> basic functionality of a framebuffer, but should be compatible with most
>> existing fbdev drivers.
>>
>> A DRM driver using fbconv helpers consists of
>>
>>    * DRM stub code that calls into fbconv helpers, and
>>    * the original fbdev driver code.
>>
>> The fbdev driver code has to be modified to register itself with the
>> stub driver instead of the fbdev core framework. A tutorial on how to use
>> the helpers is part of this patchset. The resulting driver hybrid can be
>> refactored into a first-class DRM driver. The fbconv helpers contain a
>> number of comments, labeled 'DRM porting note', which explain the required
>> steps.
>>
>> I tested the current patchset with the following drivers: atyfb, aty128fb,
>> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
>> With each, I was able to successfully start with fbcon enabled, run weston and
>> X11. The drivers are available at [1]. For reference, the patchset includes
>> the Matrox stub driver.
> 
> In general I like the idea of modernizing the existing fbdev drivers.
> What I fail to read in your intro above is if this allows us to phase
> out the migrated fbdev drivers sooner?
> Or do we end up with two drivers to maintain?

The idea is that an fbdev driver is converted over to DRM and, once 
ready, the original fbdev driver gets removed. When a hybrid driver gets 
added, I'd want to see the rsp developer actually clean up and refactor 
the code. There shouldn't be multiple drivers for long.

But most of the fbdev drivers appear to be unmaintained anyway. I 
wouldn't expect having two drivers for a few releases would make much of 
a difference.

> Obviously a full migration to a DRM driver was preferred - but this may
> serve as a step in that direction.
> But we should not end up with two drivers doing almost the same.
> 
> Another general question. Do we want the modernized DRM drivers to end
> up in staging? Why should they not go direct into drm/*
> I know they are not fully atomic but this is not new drivers so maybe
> they can be excused.
> Problem is that drm drivers in staging live a secret nonvisible life
> where they are easy to forget when we change interfaces and such.

True. OTOH putting them next to the regular DRM code sends the message 
that the driver is already complete and in good shape. Those hybrid 
drivers are limited in functionality and don't really live up to 
anyone's requirements for code quality. It's the kind of code one would 
expect in staging.

Best regards
Thomas

> 	Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 08/15] drm/fbconv: Add plane-state check and update

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5472 bytes --]

Hi Thomas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/DRM-fbconv-helpers-for-converting-fbdev-drivers/20191015-152231
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/mips/include/asm/div64.h:12:0,
                    from include/linux/kernel.h:18,
                    from include/asm-generic/bug.h:19,
                    from arch/mips/include/asm/bug.h:42,
                    from include/linux/bug.h:5,
                    from arch/mips/include/asm/cmpxchg.h:11,
                    from arch/mips/include/asm/atomic.h:22,
                    from include/linux/atomic.h:7,
                    from include/linux/kgdb.h:18,
                    from include/linux/fb.h:5,
                    from drivers/gpu/drm/drm_fbconv_helper.c:5:
   drivers/gpu/drm/drm_fbconv_helper.c: In function 'drm_fbconv_update_fb_var_screeninfo_from_framebuffer':
   include/asm-generic/div64.h:226:28: warning: comparison of distinct pointer types lacks a cast
     (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
                               ^
>> drivers/gpu/drm/drm_fbconv_helper.c:899:2: note: in expansion of macro 'do_div'
     do_div(width, cpp);
     ^~~~~~
   In file included from include/uapi/linux/swab.h:6:0,
                    from include/linux/swab.h:5,
                    from include/uapi/linux/byteorder/big_endian.h:13,
                    from include/linux/byteorder/big_endian.h:5,
                    from arch/mips/include/uapi/asm/byteorder.h:13,
                    from drivers/gpu/drm/drm_fbconv_helper.c:3:
   include/asm-generic/div64.h:239:25: warning: right shift count >= width of type [-Wshift-count-overflow]
     } else if (likely(((n) >> 32) == 0)) {  \
                            ^
   include/linux/compiler.h:77:40: note: in definition of macro 'likely'
    # define likely(x) __builtin_expect(!!(x), 1)
                                           ^
>> drivers/gpu/drm/drm_fbconv_helper.c:899:2: note: in expansion of macro 'do_div'
     do_div(width, cpp);
     ^~~~~~
   In file included from arch/mips/include/asm/div64.h:12:0,
                    from include/linux/kernel.h:18,
                    from include/asm-generic/bug.h:19,
                    from arch/mips/include/asm/bug.h:42,
                    from include/linux/bug.h:5,
                    from arch/mips/include/asm/cmpxchg.h:11,
                    from arch/mips/include/asm/atomic.h:22,
                    from include/linux/atomic.h:7,
                    from include/linux/kgdb.h:18,
                    from include/linux/fb.h:5,
                    from drivers/gpu/drm/drm_fbconv_helper.c:5:
   include/asm-generic/div64.h:243:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
      __rem = __div64_32(&(n), __base); \
                         ^
>> drivers/gpu/drm/drm_fbconv_helper.c:899:2: note: in expansion of macro 'do_div'
     do_div(width, cpp);
     ^~~~~~
   include/asm-generic/div64.h:217:17: note: expected 'uint64_t * {aka long long unsigned int *}' but argument is of type 'unsigned int *'
    extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
                    ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/do_div +899 drivers/gpu/drm/drm_fbconv_helper.c

   883	
   884	static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
   885		struct fb_var_screeninfo *fb_var, struct drm_framebuffer *fb,
   886		size_t vram_size)
   887	{
   888		unsigned int width, pitch;
   889		uint64_t cpp, lines;
   890		int ret;
   891	
   892		/* Our virtual screen covers all the graphics memory (sans some
   893		 * trailing bytes). This allows for setting the scanout buffer's
   894		 * address with fb_pan_display().
   895		 */
   896	
   897		width = fb->pitches[0];
   898		cpp = fb->format[0].cpp[0];
 > 899		do_div(width, cpp);
   900	
   901		if (width > (__u32)-1)
   902			return -EINVAL; /* would overflow fb_var->xres_virtual */
   903	
   904		pitch = fb->pitches[0];
   905		lines = vram_size;
   906		do_div(lines, pitch);
   907	
   908		if (lines > (__u32)-1)
   909			return -EINVAL; /* would overflow fb_var->yres_virtual */
   910	
   911		fb_var->xres_virtual = width;
   912		fb_var->yres_virtual = lines;
   913	
   914		ret = drm_fbconv_update_fb_var_screeninfo_from_format(
   915			fb_var, fb->format[0].format);
   916		if (ret)
   917			return ret;
   918	
   919		return 0;
   920	}
   921	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62095 bytes --]

Re: [PATCH v2 12/15] drm/fbconv: Add helper documentation

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 24745 bytes --]

Hi Thomas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/DRM-fbconv-helpers-for-converting-fbdev-drivers/20191015-152231
reproduce: make htmldocs

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
   include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   drivers/net/phy/phylink.c:595: warning: Function parameter or member 'config' not described in 'phylink_create'
   drivers/net/phy/phylink.c:595: warning: Excess function parameter 'ndev' description in 'phylink_create'
   lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc' not found
   lib/genalloc.c:1: warning: 'gen_pool_free' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found
   include/linux/rculist.h:374: warning: Excess function parameter 'cond' description in 'list_for_each_entry_rcu'
   include/linux/rculist.h:651: warning: Excess function parameter 'cond' description in 'hlist_for_each_entry_rcu'
   mm/util.c:1: warning: 'get_user_pages_fast' not found
   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:335: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export'
   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:336: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:821: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2821: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:378: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found
   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found
   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: 'pp_dpm_sclk pp_dpm_mclk pp_dpm_pcie' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:132: warning: Incorrect use of kernel-doc format:          * @atomic_obj
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:238: warning: Incorrect use of kernel-doc format:          * gpu_info FW provided soc bounding box struct or 0 if not
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'soc_bounding_box' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_crtc_high_irq' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_pflip_high_irq' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'register_hpd_handlers' not found
   include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
   include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
   include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
   include/drm/drm_gem_shmem_helper.h:87: warning: Function parameter or member 'madv' not described in 'drm_gem_shmem_object'
   include/drm/drm_gem_shmem_helper.h:87: warning: Function parameter or member 'madv_list' not described in 'drm_gem_shmem_object'
   drivers/gpu/drm/drm_fbconv_helper.c:370: warning: Excess function parameter 'fb_info' description in 'drm_fbconv_format_of_fb_var_screeninfo'
>> include/drm/drm_fbconv_helper.h:124: warning: Function parameter or member 'blit' not described in 'drm_fbconv_modeset'
>> drivers/gpu/drm/drm_fbconv_helper.c:371: warning: Function parameter or member 'fb_var' not described in 'drm_fbconv_format_of_fb_var_screeninfo'
   drivers/gpu/drm/drm_fbconv_helper.c:371: warning: Excess function parameter 'fb_info' description in 'drm_fbconv_format_of_fb_var_screeninfo'
   drivers/gpu/drm/i915/display/intel_dpll_mgr.h:158: warning: Enum value 'DPLL_ID_TGL_MGPLL5' not described in enum 'intel_dpll_id'
   drivers/gpu/drm/i915/display/intel_dpll_mgr.h:158: warning: Enum value 'DPLL_ID_TGL_MGPLL6' not described in enum 'intel_dpll_id'
   drivers/gpu/drm/i915/display/intel_dpll_mgr.h:158: warning: Excess enum value 'DPLL_ID_TGL_TCPLL5' description in 'intel_dpll_id'
   drivers/gpu/drm/i915/display/intel_dpll_mgr.h:158: warning: Excess enum value 'DPLL_ID_TGL_TCPLL6' description in 'intel_dpll_id'
   drivers/gpu/drm/i915/display/intel_dpll_mgr.h:342: warning: Function parameter or member 'wakeref' not described in 'intel_shared_dpll'
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_batch_pool.c
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_batch_pool.c
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_batch_pool.c
   drivers/gpu/drm/i915/i915_drv.h:1129: warning: Incorrect use of kernel-doc format:          * The OA context specific information.
   drivers/gpu/drm/i915/i915_drv.h:1143: warning: Incorrect use of kernel-doc format:          * State of the OA buffer.
   drivers/gpu/drm/i915/i915_drv.h:1154: warning: Incorrect use of kernel-doc format:                  * Locks reads and writes to all head/tail state
   drivers/gpu/drm/i915/i915_drv.h:1176: warning: Incorrect use of kernel-doc format:                  * One 'aging' tail pointer and one 'aged' tail pointer ready to
   drivers/gpu/drm/i915/i915_drv.h:1188: warning: Incorrect use of kernel-doc format:                  * Index for the aged tail ready to read() data up to.
   drivers/gpu/drm/i915/i915_drv.h:1193: warning: Incorrect use of kernel-doc format:                  * A monotonic timestamp for when the current aging tail pointer
   drivers/gpu/drm/i915/i915_drv.h:1199: warning: Incorrect use of kernel-doc format:                  * Although we can always read back the head pointer register,
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'pinned_ctx' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'specific_ctx_id' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'specific_ctx_id_mask' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'poll_check_timer' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'poll_wq' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'pollin' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'periodic' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'period_exponent' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1207: warning: Function parameter or member 'oa_buffer' not described in 'i915_perf_stream'
   drivers/gpu/drm/i915/i915_drv.h:1129: warning: Incorrect use of kernel-doc format:          * The OA context specific information.
   drivers/gpu/drm/i915/i915_drv.h:1143: warning: Incorrect use of kernel-doc format:          * State of the OA buffer.
   drivers/gpu/drm/i915/i915_drv.h:1154: warning: Incorrect use of kernel-doc format:                  * Locks reads and writes to all head/tail state
   drivers/gpu/drm/i915/i915_drv.h:1176: warning: Incorrect use of kernel-doc format:                  * One 'aging' tail pointer and one 'aged' tail pointer ready to
   drivers/gpu/drm/i915/i915_drv.h:1188: warning: Incorrect use of kernel-doc format:                  * Index for the aged tail ready to read() data up to.
   drivers/gpu/drm/i915/i915_drv.h:1193: warning: Incorrect use of kernel-doc format:                  * A monotonic timestamp for when the current aging tail pointer
   drivers/gpu/drm/i915/i915_drv.h:1199: warning: Incorrect use of kernel-doc format:                  * Although we can always read back the head pointer register,
   drivers/gpu/drm/i915/i915_drv.h:1129: warning: Incorrect use of kernel-doc format:          * The OA context specific information.
   drivers/gpu/drm/i915/i915_drv.h:1143: warning: Incorrect use of kernel-doc format:          * State of the OA buffer.
   drivers/gpu/drm/i915/i915_drv.h:1154: warning: Incorrect use of kernel-doc format:                  * Locks reads and writes to all head/tail state
   drivers/gpu/drm/i915/i915_drv.h:1176: warning: Incorrect use of kernel-doc format:                  * One 'aging' tail pointer and one 'aged' tail pointer ready to
   drivers/gpu/drm/i915/i915_drv.h:1188: warning: Incorrect use of kernel-doc format:                  * Index for the aged tail ready to read() data up to.
   drivers/gpu/drm/i915/i915_drv.h:1193: warning: Incorrect use of kernel-doc format:                  * A monotonic timestamp for when the current aging tail pointer
   drivers/gpu/drm/i915/i915_drv.h:1199: warning: Incorrect use of kernel-doc format:                  * Although we can always read back the head pointer register,
   drivers/gpu/drm/mcde/mcde_drv.c:1: warning: 'ST-Ericsson MCDE DRM Driver' not found
   include/net/cfg80211.h:1185: warning: Function parameter or member 'txpwr' not described in 'station_parameters'
   include/net/mac80211.h:4056: warning: Function parameter or member 'sta_set_txpwr' not described in 'ieee80211_ops'
   include/net/mac80211.h:2018: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta'
   Documentation/admin-guide/perf/imx-ddr.rst:21: WARNING: Unexpected indentation.
   Documentation/admin-guide/perf/imx-ddr.rst:34: WARNING: Unexpected indentation.
   Documentation/admin-guide/perf/imx-ddr.rst:40: WARNING: Unexpected indentation.
   Documentation/admin-guide/perf/imx-ddr.rst:45: WARNING: Unexpected indentation.
   Documentation/admin-guide/perf/imx-ddr.rst:52: WARNING: Unexpected indentation.
   Documentation/admin-guide/xfs.rst:257: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/filesystems/ubifs-authentication.rst:94: WARNING: Inline interpreted text or phrase reference start-string without end-string.
   Documentation/usb/index.rst:5: WARNING: toctree contains reference to nonexisting document 'usb/rio'
   Documentation/usb/index.rst:5: WARNING: toctree contains reference to nonexisting document 'usb/wusb-design-overview'
   Documentation/usb/text_files.rst:22: WARNING: Include file 'Documentation/usb/wusb-cbaf' not found or reading it failed
   Documentation/trace/kprobetrace.rst:100: WARNING: Explicit markup ends without a blank line; unexpected unindent.
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -function Reservation Object Overview drivers/dma-buf/reservation.c' failed with return code 1
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export drivers/dma-buf/reservation.c' failed with return code 2
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -internal include/linux/reservation.h' failed with return code 2
   Documentation/translations/it_IT/process/maintainer-pgp-guide.rst:458: WARNING: Unknown target name: "nitrokey pro".
   Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
   drivers/firewire/core-transaction.c:606: WARNING: Inline strong start-string without end-string.
   drivers/ata/libata-core.c:5945: WARNING: Unknown target name: "hw".
   drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/regulator/driver.h:284: WARNING: Unknown target name: "regulator_regmap_x_voltage".
   include/linux/spi/spi.h:382: WARNING: Unexpected indentation.
   fs/seq_file.c:40: WARNING: Inline strong start-string without end-string.
   fs/seq_file.c:40: WARNING: Inline strong start-string without end-string.
   fs/seq_file.c:40: WARNING: Inline strong start-string without end-string.
   fs/seq_file.c:40: WARNING: Inline strong start-string without end-string.
   fs/posix_acl.c:636: WARNING: Inline emphasis start-string without end-string.
   fs/debugfs/inode.c:427: WARNING: Inline literal start-string without end-string.
   fs/debugfs/inode.c:506: WARNING: Inline literal start-string without end-string.
   fs/debugfs/inode.c:538: WARNING: Inline literal start-string without end-string.
   fs/debugfs/inode.c:631: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:424: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:430: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:469: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:475: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:514: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:520: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:560: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:566: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:608: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:614: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:875: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:881: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:928: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:934: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:1120: WARNING: Inline literal start-string without end-string.
   fs/debugfs/file.c:1126: WARNING: Inline literal start-string without end-string.
   Documentation/networking/device_drivers/pensando/ionic.rst:39: WARNING: Unexpected indentation.
   Documentation/networking/device_drivers/pensando/ionic.rst:43: WARNING: Unexpected indentation.
   Documentation/misc-devices/index.rst:14: WARNING: toctree contains reference to nonexisting document 'misc-devices/xilinx_sdfec'
   Documentation/driver-api/gpio/driver.rst:420: WARNING: Unexpected indentation.
   Documentation/driver-api/gpio/driver.rst:418: WARNING: Inline emphasis start-string without end-string.
   Documentation/driver-api/gpio/driver.rst:422: WARNING: Block quote ends without a blank line; unexpected unindent.

vim +124 include/drm/drm_fbconv_helper.h

87f9f686c7da49 Thomas Zimmermann 2019-10-14 @124  

:::::: The code at line 124 was first introduced by commit
:::::: 87f9f686c7da49f2b145d7c1073bb38319b31f99 drm/fbconv: Add modesetting infrastructure

:::::: TO: Thomas Zimmermann <tzimmermann@suse.de>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7279 bytes --]

Re: [PATCH v2 14/15] staging/mgakms: Import matroxfb driver source code

[Ville Syrjälä]

On Mon, Oct 14, 2019 at 04:04:15PM +0200, Thomas Zimmermann wrote:
> Only code is being copied, no functional changes are made.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/staging/mgakms/g450_pll.c         |  539 +++++a

...

Personally I would start from scratch. In fact some years (*cough*
decade) ago I did just that and started writing a new driver for
matrox stuff. Unfortunately I ran out of steam after figuring out
most of the interesting hardware quirks and whatnot, so I never
finished it. The end result is that it still runs in userspace
but kinda looks like a kernel driver if you squint a bit.

Anyways, I just slapped a MIT license on it  dumped the whole
thing here:
https://gitlab.com/syrjala/mga
The development history was, shall we say, not really useful
so I just squashed it.

You, or someone else, might find it interesting. I think in
terms of hardware support it's a superset of any other driver,
apart from the blob.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH v2 14/15] staging/mgakms: Import matroxfb driver source code

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1885 bytes --]

Hi

Am 15.10.19 um 13:48 schrieb Ville Syrjälä:
> On Mon, Oct 14, 2019 at 04:04:15PM +0200, Thomas Zimmermann wrote:
>> Only code is being copied, no functional changes are made.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/staging/mgakms/g450_pll.c         |  539 +++++a
> 
> ...
> 
> Personally I would start from scratch. In fact some years (*cough*
> decade) ago I did just that and started writing a new driver for
> matrox stuff. Unfortunately I ran out of steam after figuring out
> most of the interesting hardware quirks and whatnot, so I never
> finished it. The end result is that it still runs in userspace
> but kinda looks like a kernel driver if you squint a bit.
> 
> Anyways, I just slapped a MIT license on it  dumped the whole
> thing here:
> https://gitlab.com/syrjala/mga
> The development history was, shall we say, not really useful
> so I just squashed it.
> 
> You, or someone else, might find it interesting. I think in
> terms of hardware support it's a superset of any other driver,
> apart from the blob.
> 

Just to make this clear: I do not intend to port every single fbdev
driver to DRM. :)

I did, however, began to convert that Matrox driver. First, to see if
the approach does work in general; and because matroxfb is one of the
more complex drivers. If it can be converted, any other driver should be
convertible as well. I split up the driver code by HW generation and can
now refactor each generation on its own. I expect to end up with several
duplicated functions, which can be re-merged.

Maybe our repo can be helpful. Thanks for the link.

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Daniel Vetter]

Hi Thomas,

On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> (was: DRM driver for fbdev devices)
> 
> This is version 2 of the fbdev conversion helpers. It's more or less a
> rewrite of the original patchset.
> 
> The fbdev subsystem is considered legacy and will probably be removed at
> some point. This would mean the loss of a signifanct number of drivers.
> Some of the affected hardware is not in use any longer, but some hardware
> is still around and provides good(-enough) framebuffers.
> 
> The fbconv helpers allow for running the current DRM stack on top of fbdev
> drivers. It's a set of functions that convert between fbdev interfaces and
> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> basic functionality of a framebuffer, but should be compatible with most
> existing fbdev drivers.
> 
> A DRM driver using fbconv helpers consists of
> 
>   * DRM stub code that calls into fbconv helpers, and
>   * the original fbdev driver code.
> 
> The fbdev driver code has to be modified to register itself with the
> stub driver instead of the fbdev core framework. A tutorial on how to use
> the helpers is part of this patchset. The resulting driver hybrid can be
> refactored into a first-class DRM driver. The fbconv helpers contain a
> number of comments, labeled 'DRM porting note', which explain the required
> steps.
> 
> I tested the current patchset with the following drivers: atyfb, aty128fb,
> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> With each, I was able to successfully start with fbcon enabled, run weston and
> X11. The drivers are available at [1]. For reference, the patchset includes
> the Matrox stub driver.

So I really don't want to rain on the parade here, since if you think this
is useful when converting fbdev drivers I'll buy that, and I'm all for
getting more modern drivers into drm.

But I have a bunch of concerns with the approach you're proposing here:

- we've tried staging for drm driver refactoring, it hurts. Separate tree
  plus the quick pace in refactoring create lots of pains. And for small
  drivers refacotoring before it's not buying you anything above
  refactoring in your own personal tree. And for big drivers we're fairly
  lenient with merging drivers that aren't fully polished yet, if there's
  a team serious enough with cleaning up the mess. I think even merging
  partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
  better than staging.

- we've had conversion helpers before (for the legacy kms -> atomic
  upgrade). They constantly broke, pretty much every release when someone
  wanted to use them they first had to fix them up again. I think having
  those helpers is good, but better to just have them in some branch
  somewhere where it's clear that they might not work anymore on latest
  upstream.

- especially for some of these simple fbdev drivers I feel like just
  typing a new driver from scratch might be simpler.

A few more concerns specifically for your mga example:

- We already have a mga driver. Might be better to integrate support for
  older mgas into that than have a parallel driver.

- Your helper is based on simple display pipe, and I think for these old
  mga chips (especially the dual pipe mga 450 and 550) simple display pipe
  helper is more a hindering detour than actual help. From a quick read
  through the code (especially all the custom ioctls) you definitely want
  separate TV-out connector to expose all the tv mode properties (instead
  of the custom ioctls).

- On the topic of ioctls, looks like we could add FBIOGET_VBLANK to our
  generic implementation in the fbdev helpers.

So here's my alternative proposal:

- You push this as a branch onto a gitlab repo (freedesktop.org or
  wherever you feel like).

- You add a gitlab CI target to autobuild the very nice kerneldoc you've
  created. Feel free to also do this with anything else you're familiar
  with, it's just I know gitlab and it's real simple to get a few docs
  autogenerated and published with it.

- We add a todo.rst patch linking to your branch and the docs and a few
  lines on how to best convert an fbdev driver over to kms/atomic.

And all the drivers would land the usual way, like any of the other
drivers we've added to drivers/gpu/drm over the past few years.

Thoughts?

Cheers, Daniel
> 
> v2:
> 	* rename to fbconv helpers
> 	* rewrite as helper library
> 	* switch over to simple KMS helpers
> 	* switch over to SHMEM
> 	* add documentation
> 
> [1] https://gitlab.freedesktop.org/tzimmermann/linux/commits/fbconv-plus-drivers
> 
> Thomas Zimmermann (15):
>   fbdev: Export fb_check_foreignness()
>   fbdev: Export FBPIXMAPSIZE
>   drm/simple-kms-helper: Add mode_fixup() to simple display pipe
>   drm: Add fbconv helper module
>   drm/fbconv: Add DRM <-> fbdev pixel-format conversion
>   drm/fbconv: Add mode conversion DRM <-> fbdev
>   drm/fbconv: Add modesetting infrastructure
>   drm/fbconv: Add plane-state check and update
>   drm/fbconv: Mode-setting pipeline enable / disable
>   drm/fbconv: Reimplement several fbdev interfaces
>   drm/fbconv: Add helpers for init and cleanup of fb_info structures
>   drm/fbconv: Add helper documentation
>   staging: Add mgakms driver
>   staging/mgakms: Import matroxfb driver source code
>   staging/mgakms: Update matroxfb driver code for DRM
> 
>  Documentation/gpu/drm-kms-helpers.rst     |   12 +
>  Documentation/gpu/todo.rst                |   15 +
>  drivers/gpu/drm/Kconfig                   |   11 +
>  drivers/gpu/drm/Makefile                  |    1 +
>  drivers/gpu/drm/drm_fbconv_helper.c       | 2126 +++++++++++++++++
>  drivers/gpu/drm/drm_simple_kms_helper.c   |   15 +
>  drivers/staging/Kconfig                   |    2 +
>  drivers/staging/Makefile                  |    1 +
>  drivers/staging/mgakms/Kconfig            |   18 +
>  drivers/staging/mgakms/Makefile           |   17 +
>  drivers/staging/mgakms/g450_pll.c         |  539 +++++
>  drivers/staging/mgakms/g450_pll.h         |   13 +
>  drivers/staging/mgakms/i2c-matroxfb.c     |  238 ++
>  drivers/staging/mgakms/matroxfb_DAC1064.c | 1082 +++++++++
>  drivers/staging/mgakms/matroxfb_DAC1064.h |  174 ++
>  drivers/staging/mgakms/matroxfb_Ti3026.c  |  746 ++++++
>  drivers/staging/mgakms/matroxfb_Ti3026.h  |   10 +
>  drivers/staging/mgakms/matroxfb_accel.c   |  519 +++++
>  drivers/staging/mgakms/matroxfb_accel.h   |    9 +
>  drivers/staging/mgakms/matroxfb_base.c    | 2592 +++++++++++++++++++++
>  drivers/staging/mgakms/matroxfb_base.h    |  700 ++++++
>  drivers/staging/mgakms/matroxfb_crtc2.h   |   35 +
>  drivers/staging/mgakms/matroxfb_g450.c    |  640 +++++
>  drivers/staging/mgakms/matroxfb_g450.h    |   10 +
>  drivers/staging/mgakms/matroxfb_maven.h   |   21 +
>  drivers/staging/mgakms/matroxfb_misc.c    |  815 +++++++
>  drivers/staging/mgakms/matroxfb_misc.h    |   22 +
>  drivers/staging/mgakms/mga_device.c       |   68 +
>  drivers/staging/mgakms/mga_device.h       |   30 +
>  drivers/staging/mgakms/mga_drv.c          |  129 +
>  drivers/staging/mgakms/mga_drv.h          |   14 +
>  drivers/video/fbdev/core/fbmem.c          |    5 +-
>  include/drm/drm_fbconv_helper.h           |  150 ++
>  include/drm/drm_simple_kms_helper.h       |   43 +
>  include/linux/fb.h                        |    3 +
>  35 files changed, 10822 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c
>  create mode 100644 drivers/staging/mgakms/Kconfig
>  create mode 100644 drivers/staging/mgakms/Makefile
>  create mode 100644 drivers/staging/mgakms/g450_pll.c
>  create mode 100644 drivers/staging/mgakms/g450_pll.h
>  create mode 100644 drivers/staging/mgakms/i2c-matroxfb.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_base.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_base.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_crtc2.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_maven.h
>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.c
>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.h
>  create mode 100644 drivers/staging/mgakms/mga_device.c
>  create mode 100644 drivers/staging/mgakms/mga_device.h
>  create mode 100644 drivers/staging/mgakms/mga_drv.c
>  create mode 100644 drivers/staging/mgakms/mga_drv.h
>  create mode 100644 include/drm/drm_fbconv_helper.h
> 
> --
> 2.23.0
> 

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

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 10549 bytes --]

Hi Daniel

Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> Hi Thomas,
> 
> On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
>> (was: DRM driver for fbdev devices)
>>
>> This is version 2 of the fbdev conversion helpers. It's more or less a
>> rewrite of the original patchset.
>>
>> The fbdev subsystem is considered legacy and will probably be removed at
>> some point. This would mean the loss of a signifanct number of drivers.
>> Some of the affected hardware is not in use any longer, but some hardware
>> is still around and provides good(-enough) framebuffers.
>>
>> The fbconv helpers allow for running the current DRM stack on top of fbdev
>> drivers. It's a set of functions that convert between fbdev interfaces and
>> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
>> basic functionality of a framebuffer, but should be compatible with most
>> existing fbdev drivers.
>>
>> A DRM driver using fbconv helpers consists of
>>
>>   * DRM stub code that calls into fbconv helpers, and
>>   * the original fbdev driver code.
>>
>> The fbdev driver code has to be modified to register itself with the
>> stub driver instead of the fbdev core framework. A tutorial on how to use
>> the helpers is part of this patchset. The resulting driver hybrid can be
>> refactored into a first-class DRM driver. The fbconv helpers contain a
>> number of comments, labeled 'DRM porting note', which explain the required
>> steps.
>>
>> I tested the current patchset with the following drivers: atyfb, aty128fb,
>> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
>> With each, I was able to successfully start with fbcon enabled, run weston and
>> X11. The drivers are available at [1]. For reference, the patchset includes
>> the Matrox stub driver.
> 
> So I really don't want to rain on the parade here, since if you think this
> is useful when converting fbdev drivers I'll buy that, and I'm all for
> getting more modern drivers into drm.
> 
> But I have a bunch of concerns with the approach you're proposing here:
> 
> - we've tried staging for drm driver refactoring, it hurts. Separate tree
>   plus the quick pace in refactoring create lots of pains. And for small
>   drivers refacotoring before it's not buying you anything above
>   refactoring in your own personal tree. And for big drivers we're fairly
>   lenient with merging drivers that aren't fully polished yet, if there's
>   a team serious enough with cleaning up the mess. I think even merging
>   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
>   better than staging.

I mostly put this into staging, because it's the kind of code you'd
expect there.

> - we've had conversion helpers before (for the legacy kms -> atomic
>   upgrade). They constantly broke, pretty much every release when someone
>   wanted to use them they first had to fix them up again. I think having
>   those helpers is good, but better to just have them in some branch
>   somewhere where it's clear that they might not work anymore on latest
>   upstream.
> 
> - especially for some of these simple fbdev drivers I feel like just
>   typing a new driver from scratch might be simpler.
> 
> A few more concerns specifically for your mga example:
> 
> - We already have a mga driver. Might be better to integrate support for
>   older mgas into that than have a parallel driver.

Two colleagues of mine, Takashi and Egbert, send a patch that added
support for desktop G200s to mgag200. [1] But it was rejected because
the devices are two old and not relevant any longer. If that opinion has
changed in the meantime, I wouldn't mind adding support for desktop GPUs
to the driver.

> - Your helper is based on simple display pipe, and I think for these old
>   mga chips (especially the dual pipe mga 450 and 550) simple display pipe
>   helper is more a hindering detour than actual help. From a quick read
>   through the code (especially all the custom ioctls) you definitely want
>   separate TV-out connector to expose all the tv mode properties (instead
>   of the custom ioctls).

Around the G100, there's something like a change in generation. Before,
devices had only a single output and less than 8 MiB of RAM. This works
well with GEM SHMEM and simple KMS. Afterwards, devices have 8 MiB or
more and multiple outputs. GEM VRAM and the full set of helpers fit this
much better. Maybe having 2 drivers that share common code (or 3 with
the Server Engine chipsets) makes most sense.

> 
> - On the topic of ioctls, looks like we could add FBIOGET_VBLANK to our
>   generic implementation in the fbdev helpers.
> 
> So here's my alternative proposal:
> 
> - You push this as a branch onto a gitlab repo (freedesktop.org or
>   wherever you feel like).
> 
> - You add a gitlab CI target to autobuild the very nice kerneldoc you've
>   created. Feel free to also do this with anything else you're familiar
>   with, it's just I know gitlab and it's real simple to get a few docs
>   autogenerated and published with it.
> 
> - We add a todo.rst patch linking to your branch and the docs and a few
>   lines on how to best convert an fbdev driver over to kms/atomic.

Yes we can do that.

Best regards
Thomas

[1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147868.html

> 
> And all the drivers would land the usual way, like any of the other
> drivers we've added to drivers/gpu/drm over the past few years.
> 
> Thoughts?
> 
> Cheers, Daniel
>>
>> v2:
>> 	* rename to fbconv helpers
>> 	* rewrite as helper library
>> 	* switch over to simple KMS helpers
>> 	* switch over to SHMEM
>> 	* add documentation
>>
>> [1] https://gitlab.freedesktop.org/tzimmermann/linux/commits/fbconv-plus-drivers
>>
>> Thomas Zimmermann (15):
>>   fbdev: Export fb_check_foreignness()
>>   fbdev: Export FBPIXMAPSIZE
>>   drm/simple-kms-helper: Add mode_fixup() to simple display pipe
>>   drm: Add fbconv helper module
>>   drm/fbconv: Add DRM <-> fbdev pixel-format conversion
>>   drm/fbconv: Add mode conversion DRM <-> fbdev
>>   drm/fbconv: Add modesetting infrastructure
>>   drm/fbconv: Add plane-state check and update
>>   drm/fbconv: Mode-setting pipeline enable / disable
>>   drm/fbconv: Reimplement several fbdev interfaces
>>   drm/fbconv: Add helpers for init and cleanup of fb_info structures
>>   drm/fbconv: Add helper documentation
>>   staging: Add mgakms driver
>>   staging/mgakms: Import matroxfb driver source code
>>   staging/mgakms: Update matroxfb driver code for DRM
>>
>>  Documentation/gpu/drm-kms-helpers.rst     |   12 +
>>  Documentation/gpu/todo.rst                |   15 +
>>  drivers/gpu/drm/Kconfig                   |   11 +
>>  drivers/gpu/drm/Makefile                  |    1 +
>>  drivers/gpu/drm/drm_fbconv_helper.c       | 2126 +++++++++++++++++
>>  drivers/gpu/drm/drm_simple_kms_helper.c   |   15 +
>>  drivers/staging/Kconfig                   |    2 +
>>  drivers/staging/Makefile                  |    1 +
>>  drivers/staging/mgakms/Kconfig            |   18 +
>>  drivers/staging/mgakms/Makefile           |   17 +
>>  drivers/staging/mgakms/g450_pll.c         |  539 +++++
>>  drivers/staging/mgakms/g450_pll.h         |   13 +
>>  drivers/staging/mgakms/i2c-matroxfb.c     |  238 ++
>>  drivers/staging/mgakms/matroxfb_DAC1064.c | 1082 +++++++++
>>  drivers/staging/mgakms/matroxfb_DAC1064.h |  174 ++
>>  drivers/staging/mgakms/matroxfb_Ti3026.c  |  746 ++++++
>>  drivers/staging/mgakms/matroxfb_Ti3026.h  |   10 +
>>  drivers/staging/mgakms/matroxfb_accel.c   |  519 +++++
>>  drivers/staging/mgakms/matroxfb_accel.h   |    9 +
>>  drivers/staging/mgakms/matroxfb_base.c    | 2592 +++++++++++++++++++++
>>  drivers/staging/mgakms/matroxfb_base.h    |  700 ++++++
>>  drivers/staging/mgakms/matroxfb_crtc2.h   |   35 +
>>  drivers/staging/mgakms/matroxfb_g450.c    |  640 +++++
>>  drivers/staging/mgakms/matroxfb_g450.h    |   10 +
>>  drivers/staging/mgakms/matroxfb_maven.h   |   21 +
>>  drivers/staging/mgakms/matroxfb_misc.c    |  815 +++++++
>>  drivers/staging/mgakms/matroxfb_misc.h    |   22 +
>>  drivers/staging/mgakms/mga_device.c       |   68 +
>>  drivers/staging/mgakms/mga_device.h       |   30 +
>>  drivers/staging/mgakms/mga_drv.c          |  129 +
>>  drivers/staging/mgakms/mga_drv.h          |   14 +
>>  drivers/video/fbdev/core/fbmem.c          |    5 +-
>>  include/drm/drm_fbconv_helper.h           |  150 ++
>>  include/drm/drm_simple_kms_helper.h       |   43 +
>>  include/linux/fb.h                        |    3 +
>>  35 files changed, 10822 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c
>>  create mode 100644 drivers/staging/mgakms/Kconfig
>>  create mode 100644 drivers/staging/mgakms/Makefile
>>  create mode 100644 drivers/staging/mgakms/g450_pll.c
>>  create mode 100644 drivers/staging/mgakms/g450_pll.h
>>  create mode 100644 drivers/staging/mgakms/i2c-matroxfb.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_base.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_base.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_crtc2.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_maven.h
>>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.c
>>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.h
>>  create mode 100644 drivers/staging/mgakms/mga_device.c
>>  create mode 100644 drivers/staging/mgakms/mga_device.h
>>  create mode 100644 drivers/staging/mgakms/mga_drv.c
>>  create mode 100644 drivers/staging/mgakms/mga_drv.h
>>  create mode 100644 include/drm/drm_fbconv_helper.h
>>
>> --
>> 2.23.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 08/15] drm/fbconv: Add plane-state check and update

[kbuild test robot]

Hi Thomas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/DRM-fbconv-helpers-for-converting-fbdev-drivers/20191015-152231
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/drm_fbconv_helper.c:981:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@    expected void [noderef] <asn:2> *dst @@    got n:2> *dst @@
>> drivers/gpu/drm/drm_fbconv_helper.c:981:39: sparse:    expected void [noderef] <asn:2> *dst
>> drivers/gpu/drm/drm_fbconv_helper.c:981:39: sparse:    got void *dst
   drivers/gpu/drm/drm_fbconv_helper.c:985:51: sparse: sparse: incorrect type in argument 1 (different address spaces) @@    expected void [noderef] <asn:2> *dst @@    got n:2> *dst @@
   drivers/gpu/drm/drm_fbconv_helper.c:985:51: sparse:    expected void [noderef] <asn:2> *dst
   drivers/gpu/drm/drm_fbconv_helper.c:985:51: sparse:    got void *dst
   drivers/gpu/drm/drm_fbconv_helper.c:990:51: sparse: sparse: incorrect type in argument 1 (different address spaces) @@    expected void [noderef] <asn:2> *dst @@    got n:2> *dst @@
   drivers/gpu/drm/drm_fbconv_helper.c:990:51: sparse:    expected void [noderef] <asn:2> *dst
   drivers/gpu/drm/drm_fbconv_helper.c:990:51: sparse:    got void *dst
>> drivers/gpu/drm/drm_fbconv_helper.c:1291:21: sparse: sparse: incorrect type in assignment (different address spaces) @@    expected void *screen_base @@    got char [noderef] <asvoid *screen_base @@
>> drivers/gpu/drm/drm_fbconv_helper.c:1291:21: sparse:    expected void *screen_base
>> drivers/gpu/drm/drm_fbconv_helper.c:1291:21: sparse:    got char [noderef] <asn:2> *screen_base
>> drivers/gpu/drm/drm_fbconv_helper.c:1294:29: sparse: sparse: incorrect type in assignment (different address spaces) @@    expected void *screen_base @@    got void [noderef] <asvoid *screen_base @@
   drivers/gpu/drm/drm_fbconv_helper.c:1294:29: sparse:    expected void *screen_base
>> drivers/gpu/drm/drm_fbconv_helper.c:1294:29: sparse:    got void [noderef] <asn:2> *
>> drivers/gpu/drm/drm_fbconv_helper.c:1318:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@    expected void volatile [noderef] <asn:2> *addr @@    got n:2> *addr @@
>> drivers/gpu/drm/drm_fbconv_helper.c:1318:25: sparse:    expected void volatile [noderef] <asn:2> *addr
>> drivers/gpu/drm/drm_fbconv_helper.c:1318:25: sparse:    got void *screen_base
   drivers/gpu/drm/drm_fbconv_helper.c:1346:38: sparse: sparse: incorrect type in argument 1 (different address spaces) @@    expected void volatile [noderef] <asn:2> *addr @@    got n:2> *addr @@
   drivers/gpu/drm/drm_fbconv_helper.c:1346:38: sparse:    expected void volatile [noderef] <asn:2> *addr
   drivers/gpu/drm/drm_fbconv_helper.c:1346:38: sparse:    got void *screen_base

vim +981 drivers/gpu/drm/drm_fbconv_helper.c

   956	
   957	/**
   958	 * drm_fbconv_blit_rect - copy an area of pixel data from a framebuffer
   959	 *	to the hardware buffer
   960	 * @dst:	the on-screen hardware buffer
   961	 * @vaddr:	the source buffer in kernel address space
   962	 * @fb:		the framebuffer of the source buffer
   963	 * @rect:	the area to copy
   964	 * Returns:
   965	 *	0 on success, or
   966	 *	a negative error code otherwise.
   967	 *
   968	 * This function copies the pixel data from a DRM framebuffer to a hardware
   969	 * buffer; doing necessary format conversion in the process. Not all
   970	 * combinations of source and destination formats are currently supported.
   971	 */
   972	int drm_fbconv_blit_rect(void *dst, void *vaddr, struct drm_framebuffer *fb,
   973				 struct drm_rect *rect)
   974	{
   975		struct drm_device *dev = fb->dev;
   976	
   977		if (!vaddr)
   978			return 0; /* no framebuffer set for plane; no error */
   979	
   980		if (dev->mode_config.preferred_depth = (fb->format->cpp[0] * 8))
 > 981			drm_fb_memcpy_dstclip(dst, vaddr, fb, rect);
   982	
   983		else if (fb->format->cpp[0] = 4 &&
   984			 dev->mode_config.preferred_depth = 16)
 > 985			drm_fb_xrgb8888_to_rgb565_dstclip(dst, fb->pitches[0],
   986							  vaddr, fb, rect, false);
   987	
   988		else if (fb->format->cpp[0] = 4 &&
   989			 dev->mode_config.preferred_depth = 24)
 > 990			drm_fb_xrgb8888_to_rgb888_dstclip(dst, fb->pitches[0],
   991							  vaddr, fb, rect);
   992	
   993		else {
   994			/* TODO: add the missing conversion */
   995			DRM_ERROR("fbconv: mismatching pixel formats\n");
   996			return -EINVAL;
   997		}
   998	
   999		return 0;
  1000	}
  1001	EXPORT_SYMBOL(drm_fbconv_blit_rect);
  1002	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Daniel Vetter]

On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi Daniel
>
> Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> > Hi Thomas,
> >
> > On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> >> (was: DRM driver for fbdev devices)
> >>
> >> This is version 2 of the fbdev conversion helpers. It's more or less a
> >> rewrite of the original patchset.
> >>
> >> The fbdev subsystem is considered legacy and will probably be removed at
> >> some point. This would mean the loss of a signifanct number of drivers.
> >> Some of the affected hardware is not in use any longer, but some hardware
> >> is still around and provides good(-enough) framebuffers.
> >>
> >> The fbconv helpers allow for running the current DRM stack on top of fbdev
> >> drivers. It's a set of functions that convert between fbdev interfaces and
> >> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> >> basic functionality of a framebuffer, but should be compatible with most
> >> existing fbdev drivers.
> >>
> >> A DRM driver using fbconv helpers consists of
> >>
> >>   * DRM stub code that calls into fbconv helpers, and
> >>   * the original fbdev driver code.
> >>
> >> The fbdev driver code has to be modified to register itself with the
> >> stub driver instead of the fbdev core framework. A tutorial on how to use
> >> the helpers is part of this patchset. The resulting driver hybrid can be
> >> refactored into a first-class DRM driver. The fbconv helpers contain a
> >> number of comments, labeled 'DRM porting note', which explain the required
> >> steps.
> >>
> >> I tested the current patchset with the following drivers: atyfb, aty128fb,
> >> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> >> With each, I was able to successfully start with fbcon enabled, run weston and
> >> X11. The drivers are available at [1]. For reference, the patchset includes
> >> the Matrox stub driver.
> >
> > So I really don't want to rain on the parade here, since if you think this
> > is useful when converting fbdev drivers I'll buy that, and I'm all for
> > getting more modern drivers into drm.
> >
> > But I have a bunch of concerns with the approach you're proposing here:
> >
> > - we've tried staging for drm driver refactoring, it hurts. Separate tree
> >   plus the quick pace in refactoring create lots of pains. And for small
> >   drivers refacotoring before it's not buying you anything above
> >   refactoring in your own personal tree. And for big drivers we're fairly
> >   lenient with merging drivers that aren't fully polished yet, if there's
> >   a team serious enough with cleaning up the mess. I think even merging
> >   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
> >   better than staging.
>
> I mostly put this into staging, because it's the kind of code you'd
> expect there.

Yeah, except we tried, it's a real pain. Conclusion by everyone
involved is that staging doesn't work for the drm subsystem.

> > - we've had conversion helpers before (for the legacy kms -> atomic
> >   upgrade). They constantly broke, pretty much every release when someone
> >   wanted to use them they first had to fix them up again. I think having
> >   those helpers is good, but better to just have them in some branch
> >   somewhere where it's clear that they might not work anymore on latest
> >   upstream.
> >
> > - especially for some of these simple fbdev drivers I feel like just
> >   typing a new driver from scratch might be simpler.
> >
> > A few more concerns specifically for your mga example:
> >
> > - We already have a mga driver. Might be better to integrate support for
> >   older mgas into that than have a parallel driver.
>
> Two colleagues of mine, Takashi and Egbert, send a patch that added
> support for desktop G200s to mgag200. [1] But it was rejected because
> the devices are two old and not relevant any longer. If that opinion has
> changed in the meantime, I wouldn't mind adding support for desktop GPUs
> to the driver.

Hm that seems to have petered out inconclusive. I definitely think a
merged mga driver is better than 2 drm atomic kms drivers for roughly
the same hardware. I'm also assuming that at least for now no one
plans to resurrect the 3d acceleration support for these old chips.
But even then it's fairly easy to disable all that on the server
chips.

> > - Your helper is based on simple display pipe, and I think for these old
> >   mga chips (especially the dual pipe mga 450 and 550) simple display pipe
> >   helper is more a hindering detour than actual help. From a quick read
> >   through the code (especially all the custom ioctls) you definitely want
> >   separate TV-out connector to expose all the tv mode properties (instead
> >   of the custom ioctls).
>
> Around the G100, there's something like a change in generation. Before,
> devices had only a single output and less than 8 MiB of RAM. This works
> well with GEM SHMEM and simple KMS. Afterwards, devices have 8 MiB or
> more and multiple outputs. GEM VRAM and the full set of helpers fit this
> much better. Maybe having 2 drivers that share common code (or 3 with
> the Server Engine chipsets) makes most sense.

Yeah if that's the case maybe a mga100 and mga200g driver fits better.
Former based on simple display pipe.

You could also have them in one directory/kernel module, if there's
some low-level functions worth sharing (like clock programming), with
the low level driver probe either setting up the simple display based
mga100 support, or the fancier mga200 support using atomic directly.
If there's really no shared code at all, then separate kernel modules
sounds better.

> > - On the topic of ioctls, looks like we could add FBIOGET_VBLANK to our
> >   generic implementation in the fbdev helpers.
> >
> > So here's my alternative proposal:
> >
> > - You push this as a branch onto a gitlab repo (freedesktop.org or
> >   wherever you feel like).
> >
> > - You add a gitlab CI target to autobuild the very nice kerneldoc you've
> >   created. Feel free to also do this with anything else you're familiar
> >   with, it's just I know gitlab and it's real simple to get a few docs
> >   autogenerated and published with it.
> >
> > - We add a todo.rst patch linking to your branch and the docs and a few
> >   lines on how to best convert an fbdev driver over to kms/atomic.
>
> Yes we can do that.

Sounds like we have a rough plan?
-Daniel

>
> Best regards
> Thomas
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147868.html
>
> >
> > And all the drivers would land the usual way, like any of the other
> > drivers we've added to drivers/gpu/drm over the past few years.
> >
> > Thoughts?
> >
> > Cheers, Daniel
> >>
> >> v2:
> >>      * rename to fbconv helpers
> >>      * rewrite as helper library
> >>      * switch over to simple KMS helpers
> >>      * switch over to SHMEM
> >>      * add documentation
> >>
> >> [1] https://gitlab.freedesktop.org/tzimmermann/linux/commits/fbconv-plus-drivers
> >>
> >> Thomas Zimmermann (15):
> >>   fbdev: Export fb_check_foreignness()
> >>   fbdev: Export FBPIXMAPSIZE
> >>   drm/simple-kms-helper: Add mode_fixup() to simple display pipe
> >>   drm: Add fbconv helper module
> >>   drm/fbconv: Add DRM <-> fbdev pixel-format conversion
> >>   drm/fbconv: Add mode conversion DRM <-> fbdev
> >>   drm/fbconv: Add modesetting infrastructure
> >>   drm/fbconv: Add plane-state check and update
> >>   drm/fbconv: Mode-setting pipeline enable / disable
> >>   drm/fbconv: Reimplement several fbdev interfaces
> >>   drm/fbconv: Add helpers for init and cleanup of fb_info structures
> >>   drm/fbconv: Add helper documentation
> >>   staging: Add mgakms driver
> >>   staging/mgakms: Import matroxfb driver source code
> >>   staging/mgakms: Update matroxfb driver code for DRM
> >>
> >>  Documentation/gpu/drm-kms-helpers.rst     |   12 +
> >>  Documentation/gpu/todo.rst                |   15 +
> >>  drivers/gpu/drm/Kconfig                   |   11 +
> >>  drivers/gpu/drm/Makefile                  |    1 +
> >>  drivers/gpu/drm/drm_fbconv_helper.c       | 2126 +++++++++++++++++
> >>  drivers/gpu/drm/drm_simple_kms_helper.c   |   15 +
> >>  drivers/staging/Kconfig                   |    2 +
> >>  drivers/staging/Makefile                  |    1 +
> >>  drivers/staging/mgakms/Kconfig            |   18 +
> >>  drivers/staging/mgakms/Makefile           |   17 +
> >>  drivers/staging/mgakms/g450_pll.c         |  539 +++++
> >>  drivers/staging/mgakms/g450_pll.h         |   13 +
> >>  drivers/staging/mgakms/i2c-matroxfb.c     |  238 ++
> >>  drivers/staging/mgakms/matroxfb_DAC1064.c | 1082 +++++++++
> >>  drivers/staging/mgakms/matroxfb_DAC1064.h |  174 ++
> >>  drivers/staging/mgakms/matroxfb_Ti3026.c  |  746 ++++++
> >>  drivers/staging/mgakms/matroxfb_Ti3026.h  |   10 +
> >>  drivers/staging/mgakms/matroxfb_accel.c   |  519 +++++
> >>  drivers/staging/mgakms/matroxfb_accel.h   |    9 +
> >>  drivers/staging/mgakms/matroxfb_base.c    | 2592 +++++++++++++++++++++
> >>  drivers/staging/mgakms/matroxfb_base.h    |  700 ++++++
> >>  drivers/staging/mgakms/matroxfb_crtc2.h   |   35 +
> >>  drivers/staging/mgakms/matroxfb_g450.c    |  640 +++++
> >>  drivers/staging/mgakms/matroxfb_g450.h    |   10 +
> >>  drivers/staging/mgakms/matroxfb_maven.h   |   21 +
> >>  drivers/staging/mgakms/matroxfb_misc.c    |  815 +++++++
> >>  drivers/staging/mgakms/matroxfb_misc.h    |   22 +
> >>  drivers/staging/mgakms/mga_device.c       |   68 +
> >>  drivers/staging/mgakms/mga_device.h       |   30 +
> >>  drivers/staging/mgakms/mga_drv.c          |  129 +
> >>  drivers/staging/mgakms/mga_drv.h          |   14 +
> >>  drivers/video/fbdev/core/fbmem.c          |    5 +-
> >>  include/drm/drm_fbconv_helper.h           |  150 ++
> >>  include/drm/drm_simple_kms_helper.h       |   43 +
> >>  include/linux/fb.h                        |    3 +
> >>  35 files changed, 10822 insertions(+), 3 deletions(-)
> >>  create mode 100644 drivers/gpu/drm/drm_fbconv_helper.c
> >>  create mode 100644 drivers/staging/mgakms/Kconfig
> >>  create mode 100644 drivers/staging/mgakms/Makefile
> >>  create mode 100644 drivers/staging/mgakms/g450_pll.c
> >>  create mode 100644 drivers/staging/mgakms/g450_pll.h
> >>  create mode 100644 drivers/staging/mgakms/i2c-matroxfb.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_DAC1064.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_Ti3026.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_accel.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_base.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_base.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_crtc2.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_g450.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_maven.h
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.c
> >>  create mode 100644 drivers/staging/mgakms/matroxfb_misc.h
> >>  create mode 100644 drivers/staging/mgakms/mga_device.c
> >>  create mode 100644 drivers/staging/mgakms/mga_device.h
> >>  create mode 100644 drivers/staging/mgakms/mga_drv.c
> >>  create mode 100644 drivers/staging/mgakms/mga_drv.h
> >>  create mode 100644 include/drm/drm_fbconv_helper.h
> >>
> >> --
> >> 2.23.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Greg KH]

On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote:
> On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Hi Daniel
> >
> > Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> > > Hi Thomas,
> > >
> > > On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> > >> (was: DRM driver for fbdev devices)
> > >>
> > >> This is version 2 of the fbdev conversion helpers. It's more or less a
> > >> rewrite of the original patchset.
> > >>
> > >> The fbdev subsystem is considered legacy and will probably be removed at
> > >> some point. This would mean the loss of a signifanct number of drivers.
> > >> Some of the affected hardware is not in use any longer, but some hardware
> > >> is still around and provides good(-enough) framebuffers.
> > >>
> > >> The fbconv helpers allow for running the current DRM stack on top of fbdev
> > >> drivers. It's a set of functions that convert between fbdev interfaces and
> > >> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> > >> basic functionality of a framebuffer, but should be compatible with most
> > >> existing fbdev drivers.
> > >>
> > >> A DRM driver using fbconv helpers consists of
> > >>
> > >>   * DRM stub code that calls into fbconv helpers, and
> > >>   * the original fbdev driver code.
> > >>
> > >> The fbdev driver code has to be modified to register itself with the
> > >> stub driver instead of the fbdev core framework. A tutorial on how to use
> > >> the helpers is part of this patchset. The resulting driver hybrid can be
> > >> refactored into a first-class DRM driver. The fbconv helpers contain a
> > >> number of comments, labeled 'DRM porting note', which explain the required
> > >> steps.
> > >>
> > >> I tested the current patchset with the following drivers: atyfb, aty128fb,
> > >> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> > >> With each, I was able to successfully start with fbcon enabled, run weston and
> > >> X11. The drivers are available at [1]. For reference, the patchset includes
> > >> the Matrox stub driver.
> > >
> > > So I really don't want to rain on the parade here, since if you think this
> > > is useful when converting fbdev drivers I'll buy that, and I'm all for
> > > getting more modern drivers into drm.
> > >
> > > But I have a bunch of concerns with the approach you're proposing here:
> > >
> > > - we've tried staging for drm driver refactoring, it hurts. Separate tree
> > >   plus the quick pace in refactoring create lots of pains. And for small
> > >   drivers refacotoring before it's not buying you anything above
> > >   refactoring in your own personal tree. And for big drivers we're fairly
> > >   lenient with merging drivers that aren't fully polished yet, if there's
> > >   a team serious enough with cleaning up the mess. I think even merging
> > >   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
> > >   better than staging.
> >
> > I mostly put this into staging, because it's the kind of code you'd
> > expect there.
> 
> Yeah, except we tried, it's a real pain. Conclusion by everyone
> involved is that staging doesn't work for the drm subsystem.

As the staging maintainer, I too agree with this.  No drm drivers/code
in staging please.

thanks,

greg k-h

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Ville Syrjälä]

On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote:
> On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Hi Daniel
> >
> > Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> > > Hi Thomas,
> > >
> > > On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> > >> (was: DRM driver for fbdev devices)
> > >>
> > >> This is version 2 of the fbdev conversion helpers. It's more or less a
> > >> rewrite of the original patchset.
> > >>
> > >> The fbdev subsystem is considered legacy and will probably be removed at
> > >> some point. This would mean the loss of a signifanct number of drivers.
> > >> Some of the affected hardware is not in use any longer, but some hardware
> > >> is still around and provides good(-enough) framebuffers.
> > >>
> > >> The fbconv helpers allow for running the current DRM stack on top of fbdev
> > >> drivers. It's a set of functions that convert between fbdev interfaces and
> > >> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> > >> basic functionality of a framebuffer, but should be compatible with most
> > >> existing fbdev drivers.
> > >>
> > >> A DRM driver using fbconv helpers consists of
> > >>
> > >>   * DRM stub code that calls into fbconv helpers, and
> > >>   * the original fbdev driver code.
> > >>
> > >> The fbdev driver code has to be modified to register itself with the
> > >> stub driver instead of the fbdev core framework. A tutorial on how to use
> > >> the helpers is part of this patchset. The resulting driver hybrid can be
> > >> refactored into a first-class DRM driver. The fbconv helpers contain a
> > >> number of comments, labeled 'DRM porting note', which explain the required
> > >> steps.
> > >>
> > >> I tested the current patchset with the following drivers: atyfb, aty128fb,
> > >> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> > >> With each, I was able to successfully start with fbcon enabled, run weston and
> > >> X11. The drivers are available at [1]. For reference, the patchset includes
> > >> the Matrox stub driver.
> > >
> > > So I really don't want to rain on the parade here, since if you think this
> > > is useful when converting fbdev drivers I'll buy that, and I'm all for
> > > getting more modern drivers into drm.
> > >
> > > But I have a bunch of concerns with the approach you're proposing here:
> > >
> > > - we've tried staging for drm driver refactoring, it hurts. Separate tree
> > >   plus the quick pace in refactoring create lots of pains. And for small
> > >   drivers refacotoring before it's not buying you anything above
> > >   refactoring in your own personal tree. And for big drivers we're fairly
> > >   lenient with merging drivers that aren't fully polished yet, if there's
> > >   a team serious enough with cleaning up the mess. I think even merging
> > >   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
> > >   better than staging.
> >
> > I mostly put this into staging, because it's the kind of code you'd
> > expect there.
> 
> Yeah, except we tried, it's a real pain. Conclusion by everyone
> involved is that staging doesn't work for the drm subsystem.
> 
> > > - we've had conversion helpers before (for the legacy kms -> atomic
> > >   upgrade). They constantly broke, pretty much every release when someone
> > >   wanted to use them they first had to fix them up again. I think having
> > >   those helpers is good, but better to just have them in some branch
> > >   somewhere where it's clear that they might not work anymore on latest
> > >   upstream.
> > >
> > > - especially for some of these simple fbdev drivers I feel like just
> > >   typing a new driver from scratch might be simpler.
> > >
> > > A few more concerns specifically for your mga example:
> > >
> > > - We already have a mga driver. Might be better to integrate support for
> > >   older mgas into that than have a parallel driver.
> >
> > Two colleagues of mine, Takashi and Egbert, send a patch that added
> > support for desktop G200s to mgag200. [1] But it was rejected because
> > the devices are two old and not relevant any longer. If that opinion has
> > changed in the meantime, I wouldn't mind adding support for desktop GPUs
> > to the driver.
> 
> Hm that seems to have petered out inconclusive. I definitely think a
> merged mga driver is better than 2 drm atomic kms drivers for roughly
> the same hardware. I'm also assuming that at least for now no one
> plans to resurrect the 3d acceleration support for these old chips.
> But even then it's fairly easy to disable all that on the server
> chips.
> 
> > > - Your helper is based on simple display pipe, and I think for these old
> > >   mga chips (especially the dual pipe mga 450 and 550) simple display pipe
> > >   helper is more a hindering detour than actual help. From a quick read
> > >   through the code (especially all the custom ioctls) you definitely want
> > >   separate TV-out connector to expose all the tv mode properties (instead
> > >   of the custom ioctls).
> >
> > Around the G100, there's something like a change in generation. Before,
> > devices had only a single output and less than 8 MiB of RAM. This works
> > well with GEM SHMEM and simple KMS. Afterwards, devices have 8 MiB or
> > more and multiple outputs. GEM VRAM and the full set of helpers fit this
> > much better. Maybe having 2 drivers that share common code (or 3 with
> > the Server Engine chipsets) makes most sense.
> 
> Yeah if that's the case maybe a mga100 and mga200g driver fits better.
> Former based on simple display pipe.

The display hardware differences are quite minimal from 
1064SG to G200. G400 did add the second CRTC but essentially
nothing else changed from G200 display. G450/G550 changed
the PLLS around a bit just for the heck of it, and integrated 
the TMDS transmitter and TV encoder. And then they did even
more PLL madness with the different G200 server chip variants.

So IMO from display hw POV G100 vs. G200 split doesn't really
make sense.

And if you go to the effort of supporting dual head on G400+ then
the single head case on earlier platforms is just the same as the
single head "trivial" case on G400+, so totally different drivers
doesn't make sense there either. Also not all G400 cards have
the TVO/TMDS chip present in which case you can only use a
single head anyway (it's almost identical to a G200 card at
that point, assuming you drive the single output with the
primary CRTC).

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 00/15] DRM fbconv helpers for converting fbdev drivers

[Ville Syrjälä]

On Tue, Oct 15, 2019 at 09:13:37PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 15, 2019 at 07:48:29PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 15, 2019 at 7:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > >
> > > Hi Daniel
> > >
> > > Am 15.10.19 um 16:33 schrieb Daniel Vetter:
> > > > Hi Thomas,
> > > >
> > > > On Mon, Oct 14, 2019 at 04:04:01PM +0200, Thomas Zimmermann wrote:
> > > >> (was: DRM driver for fbdev devices)
> > > >>
> > > >> This is version 2 of the fbdev conversion helpers. It's more or less a
> > > >> rewrite of the original patchset.
> > > >>
> > > >> The fbdev subsystem is considered legacy and will probably be removed at
> > > >> some point. This would mean the loss of a signifanct number of drivers.
> > > >> Some of the affected hardware is not in use any longer, but some hardware
> > > >> is still around and provides good(-enough) framebuffers.
> > > >>
> > > >> The fbconv helpers allow for running the current DRM stack on top of fbdev
> > > >> drivers. It's a set of functions that convert between fbdev interfaces and
> > > >> DRM interfaces. Based on SHMEM and simple KMS helpers, it only offers the
> > > >> basic functionality of a framebuffer, but should be compatible with most
> > > >> existing fbdev drivers.
> > > >>
> > > >> A DRM driver using fbconv helpers consists of
> > > >>
> > > >>   * DRM stub code that calls into fbconv helpers, and
> > > >>   * the original fbdev driver code.
> > > >>
> > > >> The fbdev driver code has to be modified to register itself with the
> > > >> stub driver instead of the fbdev core framework. A tutorial on how to use
> > > >> the helpers is part of this patchset. The resulting driver hybrid can be
> > > >> refactored into a first-class DRM driver. The fbconv helpers contain a
> > > >> number of comments, labeled 'DRM porting note', which explain the required
> > > >> steps.
> > > >>
> > > >> I tested the current patchset with the following drivers: atyfb, aty128fb,
> > > >> matroxfb, pm2fb, pm3fb, rivafb, s3fb, savagefb, sisfb, tdfxfb and tridentfb.
> > > >> With each, I was able to successfully start with fbcon enabled, run weston and
> > > >> X11. The drivers are available at [1]. For reference, the patchset includes
> > > >> the Matrox stub driver.
> > > >
> > > > So I really don't want to rain on the parade here, since if you think this
> > > > is useful when converting fbdev drivers I'll buy that, and I'm all for
> > > > getting more modern drivers into drm.
> > > >
> > > > But I have a bunch of concerns with the approach you're proposing here:
> > > >
> > > > - we've tried staging for drm driver refactoring, it hurts. Separate tree
> > > >   plus the quick pace in refactoring create lots of pains. And for small
> > > >   drivers refacotoring before it's not buying you anything above
> > > >   refactoring in your own personal tree. And for big drivers we're fairly
> > > >   lenient with merging drivers that aren't fully polished yet, if there's
> > > >   a team serious enough with cleaning up the mess. I think even merging
> > > >   partial drivers directly under drivers/gpu (but behind CONFIG_BROKEN) is
> > > >   better than staging.
> > >
> > > I mostly put this into staging, because it's the kind of code you'd
> > > expect there.
> > 
> > Yeah, except we tried, it's a real pain. Conclusion by everyone
> > involved is that staging doesn't work for the drm subsystem.
> > 
> > > > - we've had conversion helpers before (for the legacy kms -> atomic
> > > >   upgrade). They constantly broke, pretty much every release when someone
> > > >   wanted to use them they first had to fix them up again. I think having
> > > >   those helpers is good, but better to just have them in some branch
> > > >   somewhere where it's clear that they might not work anymore on latest
> > > >   upstream.
> > > >
> > > > - especially for some of these simple fbdev drivers I feel like just
> > > >   typing a new driver from scratch might be simpler.
> > > >
> > > > A few more concerns specifically for your mga example:
> > > >
> > > > - We already have a mga driver. Might be better to integrate support for
> > > >   older mgas into that than have a parallel driver.
> > >
> > > Two colleagues of mine, Takashi and Egbert, send a patch that added
> > > support for desktop G200s to mgag200. [1] But it was rejected because
> > > the devices are two old and not relevant any longer. If that opinion has
> > > changed in the meantime, I wouldn't mind adding support for desktop GPUs
> > > to the driver.
> > 
> > Hm that seems to have petered out inconclusive. I definitely think a
> > merged mga driver is better than 2 drm atomic kms drivers for roughly
> > the same hardware. I'm also assuming that at least for now no one
> > plans to resurrect the 3d acceleration support for these old chips.
> > But even then it's fairly easy to disable all that on the server
> > chips.
> > 
> > > > - Your helper is based on simple display pipe, and I think for these old
> > > >   mga chips (especially the dual pipe mga 450 and 550) simple display pipe
> > > >   helper is more a hindering detour than actual help. From a quick read
> > > >   through the code (especially all the custom ioctls) you definitely want
> > > >   separate TV-out connector to expose all the tv mode properties (instead
> > > >   of the custom ioctls).
> > >
> > > Around the G100, there's something like a change in generation. Before,
> > > devices had only a single output and less than 8 MiB of RAM. This works
> > > well with GEM SHMEM and simple KMS. Afterwards, devices have 8 MiB or
> > > more and multiple outputs. GEM VRAM and the full set of helpers fit this
> > > much better. Maybe having 2 drivers that share common code (or 3 with
> > > the Server Engine chipsets) makes most sense.
> > 
> > Yeah if that's the case maybe a mga100 and mga200g driver fits better.
> > Former based on simple display pipe.
> 
> The display hardware differences are quite minimal from 
> 1064SG to G200. G400 did add the second CRTC but essentially
> nothing else changed from G200 display. G450/G550 changed
> the PLLS around a bit just for the heck of it, and integrated 
> the TMDS transmitter and TV encoder. And then they did even
> more PLL madness with the different G200 server chip variants.
> 
> So IMO from display hw POV G100 vs. G200 split doesn't really
> make sense.

Ah, I did forget that G100 and earlier don't support the
cursor 16 color mode that G200+ have. So I guess there is a
little bit of a difference there.

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 15/15] staging/mgakms: Update matroxfb driver code for DRM

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5338 bytes --]

Hi Thomas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/DRM-fbconv-helpers-for-converting-fbdev-drivers/20191015-152231
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/mgakms/g450_pll.o: In function `__crc_g450_mnp2f':
>> g450_pll.c:(*ABS*+0x3559243b): multiple definition of `__crc_g450_mnp2f'
   drivers/staging/mgakms/g450_pll.o: In function `__crc_matroxfb_g450_setclk':
>> g450_pll.c:(*ABS*+0xb4e884b9): multiple definition of `__crc_matroxfb_g450_setclk'
   drivers/staging/mgakms/g450_pll.o: In function `matroxfb_g450_setclk':
>> g450_pll.c:(.text+0x1fe8): multiple definition of `matroxfb_g450_setclk'
   drivers/video/fbdev/matrox/g450_pll.o:g450_pll.c:(.text+0x1fe8): first defined here
   drivers/staging/mgakms/g450_pll.o: In function `g450_mnp2f':
>> g450_pll.c:(.text+0x0): multiple definition of `g450_mnp2f'
   drivers/video/fbdev/matrox/g450_pll.o:g450_pll.c:(.text+0x0): first defined here
   drivers/staging/mgakms/g450_pll.o: In function `matroxfb_g450_setpll_cond':
>> g450_pll.c:(.text+0x258): multiple definition of `matroxfb_g450_setpll_cond'
   drivers/video/fbdev/matrox/g450_pll.o:g450_pll.c:(.text+0x258): first defined here
   drivers/staging/mgakms/g450_pll.o: In function `__crc_matroxfb_g450_setpll_cond':
>> g450_pll.c:(*ABS*+0x92a49e90): multiple definition of `__crc_matroxfb_g450_setpll_cond'
   drivers/staging/mgakms/matroxfb_accel.o: In function `matrox_cfbX_init':
>> matroxfb_accel.c:(.text+0x78): multiple definition of `matrox_cfbX_init'
   drivers/video/fbdev/matrox/matroxfb_accel.o:matroxfb_accel.c:(.text+0x78): first defined here
   drivers/staging/mgakms/matroxfb_accel.o: In function `__crc_matrox_cfbX_init':
>> matroxfb_accel.c:(*ABS*+0x30cc4dd3): multiple definition of `__crc_matrox_cfbX_init'
   drivers/staging/mgakms/matroxfb_base.o: In function `__crc_matroxfb_enable_irq':
>> matroxfb_base.c:(*ABS*+0xcf34e848): multiple definition of `__crc_matroxfb_enable_irq'
   drivers/staging/mgakms/matroxfb_base.o: In function `__crc_matroxfb_register_driver':
>> matroxfb_base.c:(*ABS*+0x1294ef50): multiple definition of `__crc_matroxfb_register_driver'
   drivers/staging/mgakms/matroxfb_base.o: In function `matroxfb_enable_irq':
>> matroxfb_base.c:(.text+0x1130): multiple definition of `matroxfb_enable_irq'
   drivers/video/fbdev/matrox/matroxfb_base.o:matroxfb_base.c:(.text+0x1108): first defined here
   drivers/staging/mgakms/matroxfb_base.o: In function `__crc_matroxfb_unregister_driver':
>> matroxfb_base.c:(*ABS*+0x6a5fc443): multiple definition of `__crc_matroxfb_unregister_driver'
   drivers/staging/mgakms/matroxfb_base.o: In function `matroxfb_register_driver':
>> matroxfb_base.c:(.text+0x3e8): multiple definition of `matroxfb_register_driver'
   drivers/video/fbdev/matrox/matroxfb_base.o:matroxfb_base.c:(.text+0x3c8): first defined here
   drivers/staging/mgakms/matroxfb_base.o: In function `matroxfb_wait_for_sync':
>> matroxfb_base.c:(.text+0x13a0): multiple definition of `matroxfb_wait_for_sync'
   drivers/video/fbdev/matrox/matroxfb_base.o:matroxfb_base.c:(.text+0x1378): first defined here
   drivers/staging/mgakms/matroxfb_base.o: In function `__crc_matroxfb_wait_for_sync':
>> matroxfb_base.c:(*ABS*+0x299ca154): multiple definition of `__crc_matroxfb_wait_for_sync'
   drivers/staging/mgakms/matroxfb_base.o: In function `matroxfb_unregister_driver':
>> matroxfb_base.c:(.text+0x540): multiple definition of `matroxfb_unregister_driver'
   drivers/video/fbdev/matrox/matroxfb_base.o:matroxfb_base.c:(.text+0x520): first defined here
>> drivers/staging/mgakms/matroxfb_DAC1064.o:(.data+0xc0): multiple definition of `matrox_G100'
   drivers/video/fbdev/matrox/matroxfb_DAC1064.o:(.data+0xc0): first defined here
   drivers/staging/mgakms/matroxfb_DAC1064.o: In function `__crc_DAC1064_global_init':
>> matroxfb_DAC1064.c:(*ABS*+0xd101267): multiple definition of `__crc_DAC1064_global_init'
   drivers/staging/mgakms/matroxfb_DAC1064.o: In function `__crc_DAC1064_global_restore':
>> matroxfb_DAC1064.c:(*ABS*+0xf73a4dac): multiple definition of `__crc_DAC1064_global_restore'
>> drivers/staging/mgakms/matroxfb_DAC1064.o:(.data+0x100): multiple definition of `matrox_mystique'
   drivers/video/fbdev/matrox/matroxfb_DAC1064.o:(.data+0x100): first defined here
   drivers/staging/mgakms/matroxfb_DAC1064.o: In function `__crc_matrox_mystique':

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67227 bytes --]

Re: [PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Geert Uytterhoeven]

Hi Thomas,

On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The display mode is set by converting the DRM display mode to an
> fb_info state and handling it to the fbdev driver's fb_setvar()
> function. This also requires a color depth, which we take from the
> value of struct drm_mode_config.preferred_depth
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

> --- a/drivers/gpu/drm/drm_fbconv_helper.c
> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
> @@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
>         return 0;
>  }
>
> +static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> +       struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
> +{
> +       struct drm_plane *primary = pipe->crtc.primary;
> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> +
> +       if (primary && primary->state && primary->state->fb)
> +               return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> +                       fb_var, primary->state->fb,
> +                       modeset->fb_info->fix.smem_len);
> +
> +       fb_var->xres_virtual = fb_var->xres;
> +       fb_var->yres_virtual = fb_var->yres;
> +       fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;

This looks wrong to me: IMHO bits_per_pixel should be derived from
the fourcc format of the _new_ mode to be set...

> +
> +       return 0;
> +}
> +
>  /**
>   * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
>   *     struct drm_simple_display_pipe_funcs.mode_valid
> @@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
>         struct drm_crtc *crtc, const struct drm_display_mode *mode,
>         struct drm_display_mode *adjusted_mode)
>  {
> +       struct drm_simple_display_pipe *pipe =
> +               container_of(crtc, struct drm_simple_display_pipe, crtc);
> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> +       struct fb_var_screeninfo fb_var;
> +       int ret;
> +
> +       if (!modeset->fb_info->fbops->fb_check_var)
> +               return true;
> +
> +       drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
> +
> +       ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> +               &fb_var, &modeset->pipe);
> +       if (ret)
> +               return true;
> +
> +       ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);

... hence this fails if the requested mode is valid with the new
fourcc format, but invalid with the old (but preferred) depth.
E.g. due to bandwidth limitations, a high-resolution mode is valid
with a low color depth, while a high color depth is limited to lower
resolutions.

Unfortunately we do not know the new fourcc format here, as both
drm_simple_display_pipe_funcs.mode_{valid,fixup}() are passed
the mode (from drm_mode_set.mode), but not the new format (from
drm_mode_set.fb->format).

Am I missing something? Is the new format available in some other way?
Thanks!

> +       if (ret < 0)
> +               return false;
> +
> +       drm_mode_update_from_fb_var_screeninfo(adjusted_mode, &fb_var);
> +
>         return true;
>  }
>  EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4683 bytes --]

Hi Geert,

first of all, thanks for looking at the patch.

Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> The display mode is set by converting the DRM display mode to an
>> fb_info state and handling it to the fbdev driver's fb_setvar()
>> function. This also requires a color depth, which we take from the
>> value of struct drm_mode_config.preferred_depth
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
>> --- a/drivers/gpu/drm/drm_fbconv_helper.c
>> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
>> @@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
>>          return 0;
>>   }
>>
>> +static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
>> +       struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
>> +{
>> +       struct drm_plane *primary = pipe->crtc.primary;
>> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
>> +
>> +       if (primary && primary->state && primary->state->fb)
>> +               return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
>> +                       fb_var, primary->state->fb,
>> +                       modeset->fb_info->fix.smem_len);
>> +
>> +       fb_var->xres_virtual = fb_var->xres;
>> +       fb_var->yres_virtual = fb_var->yres;
>> +       fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;
> 
> This looks wrong to me: IMHO bits_per_pixel should be derived from
> the fourcc format of the _new_ mode to be set...

Indeed, this appears to be wrong.

> 
>> +
>> +       return 0;
>> +}
>> +
>>   /**
>>    * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
>>    *     struct drm_simple_display_pipe_funcs.mode_valid
>> @@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
>>          struct drm_crtc *crtc, const struct drm_display_mode *mode,
>>          struct drm_display_mode *adjusted_mode)
>>   {
>> +       struct drm_simple_display_pipe *pipe =
>> +               container_of(crtc, struct drm_simple_display_pipe, crtc);
>> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
>> +       struct fb_var_screeninfo fb_var;
>> +       int ret;
>> +
>> +       if (!modeset->fb_info->fbops->fb_check_var)
>> +               return true;
>> +
>> +       drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
>> +
>> +       ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
>> +               &fb_var, &modeset->pipe);
>> +       if (ret)
>> +               return true;
>> +
>> +       ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);
> 
> ... hence this fails if the requested mode is valid with the new
> fourcc format, but invalid with the old (but preferred) depth.
> E.g. due to bandwidth limitations, a high-resolution mode is valid
> with a low color depth, while a high color depth is limited to lower
> resolutions.

I tested the helpers with various fbdev drivers and modified them until 
all of these drivers produced at least some output. I'm not surprised 
that there are still bugs.

> 
> Unfortunately we do not know the new fourcc format here, as both
> drm_simple_display_pipe_funcs.mode_{valid,fixup}() are passed
> the mode (from drm_mode_set.mode), but not the new format (from
> drm_mode_set.fb->format).
> 
> Am I missing something? Is the new format available in some other way?

We can always get the format from the new plane state of 
modeset->pipe->plane. We'd have this in the atomic_check call. And it 
appears that drm_fbconv_simple_display_pipe_check() is a better place 
for this code anyway.

Best regards
Thomas

> Thanks!
> 
>> +       if (ret < 0)
>> +               return false;
>> +
>> +       drm_mode_update_from_fb_var_screeninfo(adjusted_mode, &fb_var);
>> +
>>          return true;
>>   }
>>   EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Geert Uytterhoeven]

Hi Thomas,

On Mon, May 30, 2022 at 9:47 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> first of all, thanks for looking at the patch.

Thank you, your patch series helped a lot.

> Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:
> > On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> The display mode is set by converting the DRM display mode to an
> >> fb_info state and handling it to the fbdev driver's fb_setvar()
> >> function. This also requires a color depth, which we take from the
> >> value of struct drm_mode_config.preferred_depth
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >
> >> --- a/drivers/gpu/drm/drm_fbconv_helper.c
> >> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
> >> @@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> >>          return 0;
> >>   }
> >>
> >> +static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> >> +       struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
> >> +{
> >> +       struct drm_plane *primary = pipe->crtc.primary;
> >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> >> +
> >> +       if (primary && primary->state && primary->state->fb)
> >> +               return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> >> +                       fb_var, primary->state->fb,
> >> +                       modeset->fb_info->fix.smem_len);
> >> +
> >> +       fb_var->xres_virtual = fb_var->xres;
> >> +       fb_var->yres_virtual = fb_var->yres;
> >> +       fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;
> >
> > This looks wrong to me: IMHO bits_per_pixel should be derived from
> > the fourcc format of the _new_ mode to be set...
>
> Indeed, this appears to be wrong.

OK.

>
> >
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   /**
> >>    * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
> >>    *     struct drm_simple_display_pipe_funcs.mode_valid
> >> @@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
> >>          struct drm_crtc *crtc, const struct drm_display_mode *mode,
> >>          struct drm_display_mode *adjusted_mode)
> >>   {
> >> +       struct drm_simple_display_pipe *pipe =
> >> +               container_of(crtc, struct drm_simple_display_pipe, crtc);
> >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> >> +       struct fb_var_screeninfo fb_var;
> >> +       int ret;
> >> +
> >> +       if (!modeset->fb_info->fbops->fb_check_var)
> >> +               return true;
> >> +
> >> +       drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
> >> +
> >> +       ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> >> +               &fb_var, &modeset->pipe);
> >> +       if (ret)
> >> +               return true;
> >> +
> >> +       ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);
> >
> > ... hence this fails if the requested mode is valid with the new
> > fourcc format, but invalid with the old (but preferred) depth.
> > E.g. due to bandwidth limitations, a high-resolution mode is valid
> > with a low color depth, while a high color depth is limited to lower
> > resolutions.
>
> I tested the helpers with various fbdev drivers and modified them until
> all of these drivers produced at least some output. I'm not surprised
> that there are still bugs.

As usual, the devil is in the details ;-)

The other issue I was facing are the non-rounding KHZ2PICOS() and
PICOS2KHZ() macros, and the numerous back-and-forth conversions:
a valid pixclock in kHz is converted to a valid pixclock in ps,
and accepted.  The returned pixclock in ps is slightly different,
and converted to an invalid pixclock in kHz, hence rejected in the
next iteration (remember: fb_ops.fb_check_var() should only round up
values to match, never round down)...

> > Unfortunately we do not know the new fourcc format here, as both
> > drm_simple_display_pipe_funcs.mode_{valid,fixup}() are passed
> > the mode (from drm_mode_set.mode), but not the new format (from
> > drm_mode_set.fb->format).
> >
> > Am I missing something? Is the new format available in some other way?
>
> We can always get the format from the new plane state of
> modeset->pipe->plane. We'd have this in the atomic_check call. And it
> appears that drm_fbconv_simple_display_pipe_check() is a better place
> for this code anyway.

Thanks, I'll give that a try!

Anyway, I finally made some progress with KMS-style mode-setting inside
my ataridrm driver. Before, I relied solely on initial fbdev-style
mode-setting in the probe function.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Geert Uytterhoeven]

Hi Thomas,

On Mon, May 30, 2022 at 10:34 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, May 30, 2022 at 9:47 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:
> > > On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > >> The display mode is set by converting the DRM display mode to an
> > >> fb_info state and handling it to the fbdev driver's fb_setvar()
> > >> function. This also requires a color depth, which we take from the
> > >> value of struct drm_mode_config.preferred_depth
> > >>
> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >
> > >> --- a/drivers/gpu/drm/drm_fbconv_helper.c
> > >> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
> > >> @@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> > >>          return 0;
> > >>   }
> > >>
> > >> +static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> > >> +       struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
> > >> +{
> > >> +       struct drm_plane *primary = pipe->crtc.primary;
> > >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> > >> +
> > >> +       if (primary && primary->state && primary->state->fb)
> > >> +               return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> > >> +                       fb_var, primary->state->fb,
> > >> +                       modeset->fb_info->fix.smem_len);
> > >> +
> > >> +       fb_var->xres_virtual = fb_var->xres;
> > >> +       fb_var->yres_virtual = fb_var->yres;
> > >> +       fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;
> > >
> > > This looks wrong to me: IMHO bits_per_pixel should be derived from
> > > the fourcc format of the _new_ mode to be set...
> >
> > Indeed, this appears to be wrong.
>
> OK.
>
> >
> > >
> > >> +
> > >> +       return 0;
> > >> +}
> > >> +
> > >>   /**
> > >>    * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
> > >>    *     struct drm_simple_display_pipe_funcs.mode_valid
> > >> @@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
> > >>          struct drm_crtc *crtc, const struct drm_display_mode *mode,
> > >>          struct drm_display_mode *adjusted_mode)
> > >>   {
> > >> +       struct drm_simple_display_pipe *pipe =
> > >> +               container_of(crtc, struct drm_simple_display_pipe, crtc);
> > >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> > >> +       struct fb_var_screeninfo fb_var;
> > >> +       int ret;
> > >> +
> > >> +       if (!modeset->fb_info->fbops->fb_check_var)
> > >> +               return true;
> > >> +
> > >> +       drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
> > >> +
> > >> +       ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> > >> +               &fb_var, &modeset->pipe);
> > >> +       if (ret)
> > >> +               return true;
> > >> +
> > >> +       ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);
> > >
> > > ... hence this fails if the requested mode is valid with the new
> > > fourcc format, but invalid with the old (but preferred) depth.
> > > E.g. due to bandwidth limitations, a high-resolution mode is valid
> > > with a low color depth, while a high color depth is limited to lower
> > > resolutions.

> > > Unfortunately we do not know the new fourcc format here, as both
> > > drm_simple_display_pipe_funcs.mode_{valid,fixup}() are passed
> > > the mode (from drm_mode_set.mode), but not the new format (from
> > > drm_mode_set.fb->format).
> > >
> > > Am I missing something? Is the new format available in some other way?
> >
> > We can always get the format from the new plane state of
> > modeset->pipe->plane. We'd have this in the atomic_check call. And it
> > appears that drm_fbconv_simple_display_pipe_check() is a better place
> > for this code anyway.
>
> Thanks, I'll give that a try!

Getting the format from the new plane state of pipe->plane doesn't
work, as pipe->plane.state->fb = NULL.
But it is indeed available in the drm_simple_display_pipe_funcs.check()
callback, so that seems to work...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds