[PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w]

From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Hi,

The new Intel Arria10 SOC FPGA devkit has a Display Port IP component 
which requires a new driver. This is a virtual driver in which the
FGPA hardware would enable the Display Port based on the information
and data provided from the DRM frame buffer from the OS. Basically all
all information with reagrds to resolution and bits per pixel are
pre-configured on the FPGA design and these information are fed to
the driver via the device tree information as part of the hardware 
information.

Further information can be obtained from
https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_vip.pdf

Ong, Hean Loong (3):
  dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings
  ARM: drm: Intel FPGA VIP Frame Buffer II drm driver
  ARM: socfpga: drm driver updates in socfpga_defconfig

 .../devicetree/bindings/display/altr,vip-fb2.txt   |  30 ++++
 arch/arm/configs/socfpga_defconfig                 |   4 +
 drivers/gpu/drm/Kconfig                            |   2 +
 drivers/gpu/drm/Makefile                           |   1 +
 drivers/gpu/drm/ivip/Kconfig                       |  13 ++
 drivers/gpu/drm/ivip/Makefile                      |   9 +
 drivers/gpu/drm/ivip/intel_vip_conn.c              |  96 ++++++++++
 drivers/gpu/drm/ivip/intel_vip_core.c              | 171 ++++++++++++++++++
 drivers/gpu/drm/ivip/intel_vip_drv.h               |  55 ++++++
 drivers/gpu/drm/ivip/intel_vip_of.c                | 195 +++++++++++++++++++++
 10 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/altr,vip-fb2.txt
 create mode 100644 drivers/gpu/drm/ivip/Kconfig
 create mode 100644 drivers/gpu/drm/ivip/Makefile
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_conn.c
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_core.c
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_drv.h
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_of.c

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings

[hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w]

From: "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Device tree binding for Intel FPGA Video and Image
Processing Suite. The binding involved would be generated
from the Altera (Intel) Qsys system. The bindings would
set the max width, max height, buts per pixel and memory
port width. The device tree binding only supports the Intel
Arria10 devkit and its variants. Vendor name retained as
altr.

Signed-off-by: Ong, Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
v2:
* Moved Device Tree bindings to Documentation/devicetree/bindings/display/
* Added vendor name altr, to description
---
 .../devicetree/bindings/display/altr,vip-fb2.txt   | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/altr,vip-fb2.txt

diff --git a/Documentation/devicetree/bindings/display/altr,vip-fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
new file mode 100644
index 0000000..bdffefb
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
@@ -0,0 +1,30 @@
+Intel Video and Image Processing(VIP) Frame Buffer II bindings
+
+Supported hardware: Arria 10 and above with display port IP
+
+The drm driver for the Arria 10 devkit would require the display resolution
+and pixel information to be included as these values are generated based
+on the FPGA design that drives the video connector attached to the drm driver
+Information the FPGA video IP component can be acquired from
+https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_vip.pdf
+
+Required properties:
+
+- compatible: "altr,vip-frame-buffer-2.0"
+- reg: Physical base address and length of the framebuffer controller's
+  registers.
+- altr,max-width: The width of the framebuffer in pixels.
+- altr,max-height: The height of the framebuffer in pixels.
+- altr,bits-per-symbol: only "8" is currently supported
+- altr,mem-port-width = the bus width of the avalon master port on the frame reader
+
+Example:
+
+	dp_0_frame_buf: vip@100000280 {
+			compatible = "altr,vip-frame-buffer-2.0";
+			reg = <0x00000001 0x00000280 0x00000040>;
+			altr,max-width = <1280>;
+			altr,max-height = <720>;
+			altr,bits-per-symbol = <8>;
+			altr,mem-port-width = <128>;
+	};
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/3] ARM: drm: Intel FPGA VIP Frame Buffer II drm driver

[hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w]

From: "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Driver for Intel FPGA Video and Image Processing
Suite Frame Buffer II. The driver only supports the Intel
Arria10 devkit and its variants. This driver can be either
loaded staticlly or in modules. The OF device tree binding
is located at:
Documentation/devicetree/bindings/display/altr,vip-fb2.txt

Signed-off-by: Ong, Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
v2:
* Simplify the driver by using drm_simple_display_pipe_init.
* Cleaned up some unused codes with no-ops
* Clean up Kconfig to use only DRM_IVIP
* Use DRM_GEM_CMA_FOPS for file operations
* Removed the use of fb_info to populate DT information
---
 drivers/gpu/drm/Kconfig               |   2 +
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/ivip/Kconfig          |  13 +++
 drivers/gpu/drm/ivip/Makefile         |   9 ++
 drivers/gpu/drm/ivip/intel_vip_conn.c |  96 +++++++++++++++++
 drivers/gpu/drm/ivip/intel_vip_core.c | 171 +++++++++++++++++++++++++++++
 drivers/gpu/drm/ivip/intel_vip_drv.h  |  55 ++++++++++
 drivers/gpu/drm/ivip/intel_vip_of.c   | 195 ++++++++++++++++++++++++++++++++++
 8 files changed, 542 insertions(+)
 create mode 100644 drivers/gpu/drm/ivip/Kconfig
 create mode 100644 drivers/gpu/drm/ivip/Makefile
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_conn.c
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_core.c
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_drv.h
 create mode 100644 drivers/gpu/drm/ivip/intel_vip_of.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 78d7fc0..bc03c938 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -195,6 +195,8 @@ source "drivers/gpu/drm/nouveau/Kconfig"
 
 source "drivers/gpu/drm/i915/Kconfig"
 
+source "drivers/gpu/drm/ivip/Kconfig"
+
 config DRM_VGEM
 	tristate "Virtual GEM provider"
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 59f0f9b..7cfe899 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_DRM_AMDGPU)+= amd/amdgpu/
 obj-$(CONFIG_DRM_MGA)	+= mga/
 obj-$(CONFIG_DRM_I810)	+= i810/
 obj-$(CONFIG_DRM_I915)	+= i915/
+obj-$(CONFIG_DRM_IVIP)	+= ivip/
 obj-$(CONFIG_DRM_MGAG200) += mgag200/
 obj-$(CONFIG_DRM_VC4)  += vc4/
 obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus/
diff --git a/drivers/gpu/drm/ivip/Kconfig b/drivers/gpu/drm/ivip/Kconfig
new file mode 100644
index 0000000..9a8c5ce
--- /dev/null
+++ b/drivers/gpu/drm/ivip/Kconfig
@@ -0,0 +1,13 @@
+config DRM_IVIP
+        tristate "Intel FGPA Video and Image Processing"
+        depends on DRM && OF
+        select DRM_GEM_CMA_HELPER
+        select DRM_KMS_HELPER
+        select DRM_KMS_FB_HELPER
+        select DRM_KMS_CMA_HELPER
+        help
+            Choose this option if you have a Intel FPGA Arria 10 system
+            and above with a Display Port IP. This does not support legacy
+            Intel FPGA Cyclone V display port. Currently only single frame
+            buffer is supported. Note that ACPI and X_86 architecture is yet
+            to be supported.If M is selected the module would be called ivip.
diff --git a/drivers/gpu/drm/ivip/Makefile b/drivers/gpu/drm/ivip/Makefile
new file mode 100644
index 0000000..95291c6
--- /dev/null
+++ b/drivers/gpu/drm/ivip/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for the drm device driver.  This driver provides support for the
+# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
+
+ccflags-y := -Iinclude/drm
+
+obj-$(CONFIG_DRM_IVIP) += ivip.o
+ivip-objs := intel_vip_of.o intel_vip_core.o \
+intel_vip_conn.o
diff --git a/drivers/gpu/drm/ivip/intel_vip_conn.c b/drivers/gpu/drm/ivip/intel_vip_conn.c
new file mode 100644
index 0000000..499d3b4
--- /dev/null
+++ b/drivers/gpu/drm/ivip/intel_vip_conn.c
@@ -0,0 +1,96 @@
+/*
+ * intel_vip_conn.c -- Intel Video and Image Processing(VIP)
+ * Frame Buffer II driver
+ *
+ * This driver supports the Intel VIP Frame Reader component.
+ * More info on the hardware can be found in the Intel Video
+ * and Image Processing Suite User Guide at this address
+ * http://www.altera.com/literature/ug/ug_vip.pdf.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * Authors:
+ * Ong, Hean-Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_encoder_slave.h>
+#include <drm/drm_plane_helper.h>
+
+static enum drm_connector_status
+intelvipfb_drm_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static void intelvipfb_drm_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs intelvipfb_drm_connector_funcs = {
+	.dpms = drm_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = intelvipfb_drm_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = intelvipfb_drm_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int intelvipfb_drm_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *drm = connector->dev;
+	int count;
+
+	count = drm_add_modes_noedid(connector, drm->mode_config.max_width,
+				     drm->mode_config.max_height);
+	drm_set_preferred_mode(connector, drm->mode_config.max_width,
+			       drm->mode_config.max_height);
+	return count;
+}
+
+static const struct drm_connector_helper_funcs
+intelvipfb_drm_connector_helper_funcs = {
+	.get_modes = intelvipfb_drm_connector_get_modes,
+};
+
+struct drm_connector *
+intelvipfb_conn_setup(struct drm_device *drm)
+{
+	struct drm_connector *conn;
+	int ret;
+
+	conn = devm_kzalloc(drm->dev, sizeof(*conn), GFP_KERNEL);
+	if (IS_ERR(conn))
+		return NULL;
+
+	ret = drm_connector_init(drm, conn, &intelvipfb_drm_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret < 0) {
+		dev_err(drm->dev, "failed to initialize drm connector\n");
+		ret = -ENOMEM;
+		goto error_connector_cleanup;
+	}
+
+	drm_connector_helper_add(conn, &intelvipfb_drm_connector_helper_funcs);
+
+	return conn;
+
+error_connector_cleanup:
+	drm_connector_cleanup(conn);
+
+	return NULL;
+}
diff --git a/drivers/gpu/drm/ivip/intel_vip_core.c b/drivers/gpu/drm/ivip/intel_vip_core.c
new file mode 100644
index 0000000..4e83343
--- /dev/null
+++ b/drivers/gpu/drm/ivip/intel_vip_core.c
@@ -0,0 +1,171 @@
+/*
+ * intel_vip_core.c -- Intel Video and Image Processing(VIP)
+ * Frame Buffer II driver
+ *
+ * This driver supports the Intel VIP Frame Reader component.
+ * More info on the hardware can be found in the Intel Video
+ * and Image Processing Suite User Guide at this address
+ * http://www.altera.com/literature/ug/ug_vip.pdf.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * Authors:
+ * Ong, Hean-Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#include "intel_vip_drv.h"
+
+static const u32 fbpriv_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_RGB565
+};
+
+static void intelvipfb_start_hw(void __iomem *base, resource_size_t addr)
+{
+	/*
+	 * The frameinfo variable has to correspond to the size of the VIP Suite
+	 * Frame Reader register 7 which will determine the maximum size used
+	 * in this frameinfo
+	 */
+
+	u32 frameinfo;
+
+	frameinfo =
+		readl(base + INTELVIPFB_FRAME_READER) & 0x00ffffff;
+	writel(frameinfo, base + INTELVIPFB_FRAME_INFO);
+	writel(addr, base + INTELVIPFB_FRAME_START);
+	/* Finally set the control register to 1 to start streaming */
+	writel(1, base + INTELVIPFB_CONTROL);
+}
+
+static void intelvipfb_disable_hw(void __iomem *base)
+{
+	/* set the control register to 0 to stop streaming */
+	writel(0, base + INTELVIPFB_CONTROL);
+}
+
+static const struct drm_mode_config_funcs intelvipfb_mode_config_funcs = {
+	.fb_create = drm_fb_cma_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static struct drm_mode_config_helper_funcs intelvipfb_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail,
+};
+
+static void intelvipfb_setup_mode_config(struct drm_device *drm)
+{
+	drm_mode_config_init(drm);
+	drm->mode_config.funcs = &intelvipfb_mode_config_funcs;
+	drm->mode_config.helper_private = &intelvipfb_mode_config_helpers;
+}
+
+static int intelvipfb_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
+				      struct drm_plane_state *plane_state)
+{
+	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
+}
+
+static struct drm_simple_display_pipe_funcs fbpriv_funcs = {
+	.prepare_fb = intelvipfb_pipe_prepare_fb,
+};
+
+int intelvipfb_probe(struct device *dev, void __iomem *base)
+{
+	int retval;
+	struct drm_device *drm;
+	struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev);
+	struct drm_connector *connector;
+
+	dev_set_drvdata(dev, fbpriv);
+
+	drm = fbpriv->drm;
+
+	intelvipfb_setup_mode_config(drm);
+
+	connector = intelvipfb_conn_setup(drm);
+	if (!connector) {
+		dev_err(drm->dev, "Connector setup failed\n");
+		goto err_mode_config;
+	}
+
+	retval = drm_simple_display_pipe_init(drm, &fbpriv->pipe,
+					      &fbpriv_funcs,
+					      fbpriv_formats,
+					      ARRAY_SIZE(fbpriv_formats),
+					      connector);
+	if (retval < 0) {
+		dev_err(drm->dev, "Cannot setup simple display pipe\n");
+		goto err_mode_config;
+	}
+
+	fbpriv->fbcma = drm_fbdev_cma_init(drm, PREF_BPP,
+					   drm->mode_config.num_connector);
+	if (!fbpriv->fbcma) {
+		fbpriv->fbcma = NULL;
+		dev_err(drm->dev, "Failed to init FB CMA area\n");
+		goto err_mode_config;
+	}
+
+	drm_mode_config_reset(drm);
+
+	intelvipfb_start_hw(base, drm->mode_config.fb_base);
+
+	drm_dev_register(drm, 0);
+
+	return retval;
+
+err_mode_config:
+
+	drm_mode_config_cleanup(drm);
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(intelvipfb_probe);
+
+int intelvipfb_remove(struct device *dev)
+{
+	struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev);
+	struct drm_device *drm =  fbpriv->drm;
+
+	drm_dev_unregister(drm);
+
+	if (fbpriv->fbcma)
+		drm_fbdev_cma_fini(fbpriv->fbcma);
+
+	intelvipfb_disable_hw(fbpriv->base);
+	drm_mode_config_cleanup(drm);
+
+	drm_dev_unref(drm);
+
+	devm_kfree(dev, fbpriv);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intelvipfb_remove);
+
+MODULE_AUTHOR("Ong, Hean-Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Intel VIP Frame Buffer II driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/ivip/intel_vip_drv.h b/drivers/gpu/drm/ivip/intel_vip_drv.h
new file mode 100644
index 0000000..8ef83f59
--- /dev/null
+++ b/drivers/gpu/drm/ivip/intel_vip_drv.h
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2017 Intel Corporation.
+ *
+ * Intel Video and Image Processing(VIP) Frame Buffer II driver.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ * Ong, Hean-Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ */
+#ifndef _INTEL_VIP_DRV_H
+#define _INTEL_VIP_DRV_H
+#include <linux/io.h>
+#include <linux/fb.h>
+
+#define DRIVER_NAME	"intelvipfb"
+#define BYTES_PER_PIXEL	4
+#define PREF_BPP		32
+#define CRTC_NUM		1
+#define CONN_NUM		1
+
+/* control registers */
+#define INTELVIPFB_CONTROL		0
+#define INTELVIPFB_STATUS		0x4
+#define INTELVIPFB_INTERRUPT		0x8
+#define INTELVIPFB_FRAME_COUNTER	0xC
+#define INTELVIPFB_FRAME_DROP		0x10
+#define INTELVIPFB_FRAME_INFO		0x14
+#define INTELVIPFB_FRAME_START		0x18
+#define INTELVIPFB_FRAME_READER		0x1C
+
+int intelvipfb_probe(struct device *dev, void __iomem *base);
+int intelvipfb_remove(struct device *dev);
+int intelvipfb_setup_crtc(struct drm_device *drm);
+struct drm_connector *intelvipfb_conn_setup(struct drm_device *drm);
+
+struct intelvipfb_priv {
+	struct drm_simple_display_pipe pipe;
+	struct drm_fbdev_cma *fbcma;
+	struct drm_device *drm;
+	void	__iomem	*base;
+};
+
+#endif
diff --git a/drivers/gpu/drm/ivip/intel_vip_of.c b/drivers/gpu/drm/ivip/intel_vip_of.c
new file mode 100644
index 0000000..7e3dc39
--- /dev/null
+++ b/drivers/gpu/drm/ivip/intel_vip_of.c
@@ -0,0 +1,195 @@
+/*
+ * intel_vip_of.c -- Intel Video and Image Processing(VIP)
+ * Frame Buffer II driver
+ *
+ * This driver supports the Intel VIP Frame Reader component.
+ * More info on the hardware can be found in the Intel Video
+ * and Image Processing Suite User Guide at this address
+ * http://www.altera.com/literature/ug/ug_vip.pdf.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * Authors:
+ * Ong, Hean-Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ */
+
+#include <linux/component.h>
+#include <linux/fb.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#include "intel_vip_drv.h"
+
+DEFINE_DRM_GEM_CMA_FOPS(fops);
+
+static struct drm_driver intelvipfb_drm = {
+	.driver_features =
+	    DRIVER_GEM | DRIVER_PRIME |
+	    DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC,
+	.gem_free_object_unlocked = drm_gem_cma_free_object,
+	.gem_vm_ops = &drm_gem_cma_vm_ops,
+	.dumb_create = drm_gem_cma_dumb_create,
+	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
+	.dumb_destroy = drm_gem_dumb_destroy,
+	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
+	.gem_prime_export = drm_gem_prime_export,
+	.gem_prime_import = drm_gem_prime_import,
+	.gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
+	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
+	.gem_prime_vmap = drm_gem_cma_prime_vmap,
+	.gem_prime_vunmap = drm_gem_cma_prime_vunmap,
+	.gem_prime_mmap = drm_gem_cma_prime_mmap,
+	.name = DRIVER_NAME,
+	.date = "20170329",
+	.desc = "Intel FPGA VIP SUITE",
+	.major = 1,
+	.minor = 0,
+	.patchlevel = 0,
+	.fops = &fops,
+};
+
+/*
+ * Setting up information derived from OF Device Tree Nodes
+ * max-width, max-height, bits per pixel, memory port width
+ */
+
+static int intelvipfb_drm_setup(struct device *dev,
+				struct intelvipfb_priv *fbpriv)
+{
+	struct drm_device *drm = fbpriv->drm;
+	struct device_node *np = dev->of_node;
+	int mem_word_width;
+	int max_h, max_w;
+	int bpp;
+	int ret;
+
+	ret = of_property_read_u32(np, "altr,max-width", &max_w);
+	if (ret) {
+		dev_err(dev,
+			"Missing required parameter 'altr,max-width'");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "altr,max-height", &max_h);
+	if (ret) {
+		dev_err(dev,
+			"Missing required parameter 'altr,max-height'");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "altr,bits-per-symbol", &bpp);
+	if (ret) {
+		dev_err(dev,
+			"Missing required parameter 'altr,bits-per-symbol'");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "altr,mem-port-width", &mem_word_width);
+	if (ret) {
+		dev_err(dev, "Missing required parameter 'altr,mem-port-width '");
+		return ret;
+	}
+
+	if (!(mem_word_width >= 32 && mem_word_width % 32 == 0)) {
+		dev_err(dev,
+			"mem-word-width is set to %i. must be >= 32 and multiple of 32.",
+			 mem_word_width);
+		return -ENODEV;
+	}
+
+	drm->mode_config.min_width = 640;
+	drm->mode_config.min_height = 480;
+	drm->mode_config.max_width = max_w;
+	drm->mode_config.max_height = max_h;
+	drm->mode_config.preferred_depth = bpp * BYTES_PER_PIXEL;
+
+	return 0;
+}
+
+static int intelvipfb_of_probe(struct platform_device *pdev)
+{
+	int retval;
+	struct resource *reg_res;
+	struct intelvipfb_priv *fbpriv;
+	struct device *dev = &pdev->dev;
+	struct drm_device *drm;
+
+	fbpriv = devm_kzalloc(dev, sizeof(*fbpriv), GFP_KERNEL);
+	if (!fbpriv)
+		return -ENOMEM;
+
+	/*setup DRM */
+	drm = drm_dev_alloc(&intelvipfb_drm, dev);
+	if (IS_ERR(drm))
+		return PTR_ERR(drm);
+
+	retval = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
+	if (retval)
+		return -ENODEV;
+
+	fbpriv->drm = drm;
+
+	reg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!reg_res)
+		return -ENOMEM;
+
+	fbpriv->base = devm_ioremap_resource(dev, reg_res);
+
+	if (IS_ERR(fbpriv->base)) {
+		dev_err(dev, "devm_ioremap_resource failed\n");
+		retval = PTR_ERR(fbpriv->base);
+		return -ENOMEM;
+	}
+
+	intelvipfb_drm_setup(dev, fbpriv);
+
+	dev_set_drvdata(dev, fbpriv);
+
+	return intelvipfb_probe(dev, fbpriv->base);
+}
+
+static int intelvipfb_of_remove(struct platform_device *pdev)
+{
+	return intelvipfb_remove(&pdev->dev);
+}
+
+/*
+ * The name vip-frame-buffer-2.0 is derived from
+ * http://www.altera.com/literature/ug/ug_vip.pdf
+ * frame buffer IP cores section 14
+ */
+
+static const struct of_device_id intelvipfb_of_match[] = {
+	{ .compatible = "altr,vip-frame-buffer-2.0" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, intelvipfb_of_match);
+
+static struct platform_driver intelvipfb_driver = {
+	.probe = intelvipfb_of_probe,
+	.remove = intelvipfb_of_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = intelvipfb_of_match,
+	},
+};
+
+module_platform_driver(intelvipfb_driver);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 3/3] ARM: socfpga: drm driver updates in socfpga_defconfig

[hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w]

From: "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Intel FPGA Video and Image Processing Suite Frame Buffer II
driver config for Arria 10 devkit and its variants

Signed-off-by: Ong, Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
v2:
* Added drm frame bufferr II module support for Arria10
---
 arch/arm/configs/socfpga_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
index 030264c..49ae269 100644
--- a/arch/arm/configs/socfpga_defconfig
+++ b/arch/arm/configs/socfpga_defconfig
@@ -110,6 +110,10 @@ CONFIG_MFD_ALTERA_A10SR=y
 CONFIG_MFD_STMPE=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
+CONFIG_DRM=m
+CONFIG_DRM_IVIP=m
+CONFIG_DRM_IVIP_OF=m
+CONFIG_FB=y
 CONFIG_USB=y
 CONFIG_USB_STORAGE=y
 CONFIG_USB_DWC2=y
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/3] ARM: drm: Intel FPGA VIP Frame Buffer II drm driver

[Jani Nikula]

On Tue, 25 Apr 2017, hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> +++ b/drivers/gpu/drm/ivip/Makefile
> @@ -0,0 +1,9 @@
> +#
> +# Makefile for the drm device driver.  This driver provides support for the
> +# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> +
> +ccflags-y := -Iinclude/drm

Just a drive-by observation, there are patches on the list removing such
ccflags from existing drivers. You shouldn't need this. Just make sure
all your #includes begin with <drm/.

BR,
Jani.

> +
> +obj-$(CONFIG_DRM_IVIP) += ivip.o
> +ivip-objs := intel_vip_of.o intel_vip_core.o \
> +intel_vip_conn.o
-- 
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings

[Rob Herring]

On Tue, Apr 25, 2017 at 10:06:44AM +0800, hean.loong.ong@intel.com wrote:
> From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> 
> Device tree binding for Intel FPGA Video and Image
> Processing Suite. The binding involved would be generated
> from the Altera (Intel) Qsys system. The bindings would
> set the max width, max height, buts per pixel and memory
> port width. The device tree binding only supports the Intel
> Arria10 devkit and its variants. Vendor name retained as
> altr.
> 
> Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> ---
> v2:
> * Moved Device Tree bindings to Documentation/devicetree/bindings/display/
> * Added vendor name altr, to description
> ---
>  .../devicetree/bindings/display/altr,vip-fb2.txt   | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/altr,vip-fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> new file mode 100644
> index 0000000..bdffefb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> @@ -0,0 +1,30 @@
> +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> +
> +Supported hardware: Arria 10 and above with display port IP
> +
> +The drm driver for the Arria 10 devkit would require the display resolution

Bindings describe h/w. DRM driver is a Linux term.

> +and pixel information to be included as these values are generated based
> +on the FPGA design that drives the video connector attached to the drm driver
> +Information the FPGA video IP component can be acquired from
> +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_vip.pdf
> +
> +Required properties:
> +
> +- compatible: "altr,vip-frame-buffer-2.0"
> +- reg: Physical base address and length of the framebuffer controller's
> +  registers.
> +- altr,max-width: The width of the framebuffer in pixels.
> +- altr,max-height: The height of the framebuffer in pixels.
> +- altr,bits-per-symbol: only "8" is currently supported

Supported in the driver or IP? The former isn't relevant to the binding. 
In the latter case, you don't need it if that's the only thing 
supported.

> +- altr,mem-port-width = the bus width of the avalon master port on the frame reader

In bits or bytes?

> +
> +Example:
> +
> +	dp_0_frame_buf: vip@100000280 {
> +			compatible = "altr,vip-frame-buffer-2.0";
> +			reg = <0x00000001 0x00000280 0x00000040>;
> +			altr,max-width = <1280>;
> +			altr,max-height = <720>;
> +			altr,bits-per-symbol = <8>;
> +			altr,mem-port-width = <128>;
> +	};
> -- 
> 2.7.4
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings

[Ong, Hean Loong]

On Fri, 2017-04-28 at 13:32 -0500, Rob Herring wrote:
> On Tue, Apr 25, 2017 at 10:06:44AM +0800, hean.loong.ong@intel.com
> wrote:
> > 
> > From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> > 
> > Device tree binding for Intel FPGA Video and Image
> > Processing Suite. The binding involved would be generated
> > from the Altera (Intel) Qsys system. The bindings would
> > set the max width, max height, buts per pixel and memory
> > port width. The device tree binding only supports the Intel
> > Arria10 devkit and its variants. Vendor name retained as
> > altr.
> > 
> > Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> > ---
> > v2:
> > * Moved Device Tree bindings to
> > Documentation/devicetree/bindings/display/
> > * Added vendor name altr, to description
> > ---
> >  .../devicetree/bindings/display/altr,vip-fb2.txt   | 30
> > ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt
> > new file mode 100644
> > index 0000000..bdffefb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > @@ -0,0 +1,30 @@
> > +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> > +
> > +Supported hardware: Arria 10 and above with display port IP
> > +
> > +The drm driver for the Arria 10 devkit would require the display
> > resolution
> Bindings describe h/w. DRM driver is a Linux term.
> 
Noted.
> > 
> > +and pixel information to be included as these values are generated
> > based
> > +on the FPGA design that drives the video connector attached to the
> > drm driver
> > +Information the FPGA video IP component can be acquired from
> > +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/li
> > terature/ug/ug_vip.pdf
> > +
> > +Required properties:
> > +
> > +- compatible: "altr,vip-frame-buffer-2.0"
> > +- reg: Physical base address and length of the framebuffer
> > controller's
> > +  registers.
> > +- altr,max-width: The width of the framebuffer in pixels.
> > +- altr,max-height: The height of the framebuffer in pixels.
> > +- altr,bits-per-symbol: only "8" is currently supported
> Supported in the driver or IP? The former isn't relevant to the
> binding. 
> In the latter case, you don't need it if that's the only thing 
> supported.
> 
Since the device is an FPGA the values here are based on how the FPGA
HW design is created or programmed. The values here are the optimal
reference design proposed for the Intel Arria10 devkit. 
However anyone that uses the Intel Arria10 devkit could create a device
that runs with a different resolution that has varying values but with
the condition that they need to fill these values accordingly with
Intel Quartus Programmer tools. 
Once programmed the parameters could not be changed at runtime and the
HW rerence designs currently only support 1 type of resolution per
design. 
Therefore the driver needs to support a hardware with varying
parameters programmed specifically into the FPGA.
> > 
> > +- altr,mem-port-width = the bus width of the avalon master port on
> > the frame reader
> In bits or bytes?
> 
> > 
> > +
> > +Example:
> > +
> > +	dp_0_frame_buf: vip@100000280 {
> > +			compatible = "altr,vip-frame-buffer-2.0";
> > +			reg = <0x00000001 0x00000280 0x00000040>;
> > +			altr,max-width = <1280>;
> > +			altr,max-height = <720>;
> > +			altr,bits-per-symbol = <8>;
> > +			altr,mem-port-width = <128>;
> > +	};

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Eric Anholt]

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

hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org writes:

> From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Hi,
>
> The new Intel Arria10 SOC FPGA devkit has a Display Port IP component 
> which requires a new driver. This is a virtual driver in which the
> FGPA hardware would enable the Display Port based on the information
> and data provided from the DRM frame buffer from the OS. Basically all
> all information with reagrds to resolution and bits per pixel are
> pre-configured on the FPGA design and these information are fed to
> the driver via the device tree information as part of the hardware 
> information.

I started reviewing the code, but I want to make sure I understand
what's going on:

This IP core isn't displaying contents from system memory on some sort
of actual physical display, right?  It's a core that takes some input
video stream (not described in the DT or in this driver) and stores it
to memory?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCHv2 2/3] ARM: drm: Intel FPGA VIP Frame Buffer II drm driver

[Eric Anholt]

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

hean.loong.ong@intel.com writes:

> From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
>
> Driver for Intel FPGA Video and Image Processing
> Suite Frame Buffer II. The driver only supports the Intel
> Arria10 devkit and its variants. This driver can be either
> loaded staticlly or in modules. The OF device tree binding
> is located at:
> Documentation/devicetree/bindings/display/altr,vip-fb2.txt
>
> Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>

I'm not sure if this driver is going to make sense as-is, if it doesn't
actually do display of planes from system memory.  But in case I was
wrong, here's my review:


> diff --git a/drivers/gpu/drm/ivip/intel_vip_conn.c b/drivers/gpu/drm/ivip/intel_vip_conn.c
> new file mode 100644
> index 0000000..499d3b4
> --- /dev/null
> +++ b/drivers/gpu/drm/ivip/intel_vip_conn.c
> @@ -0,0 +1,96 @@
> +/*
> + * intel_vip_conn.c -- Intel Video and Image Processing(VIP)
> + * Frame Buffer II driver
> + *
> + * This driver supports the Intel VIP Frame Reader component.
> + * More info on the hardware can be found in the Intel Video
> + * and Image Processing Suite User Guide at this address
> + * http://www.altera.com/literature/ug/ug_vip.pdf.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * Authors:
> + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_encoder_slave.h>
> +#include <drm/drm_plane_helper.h>
> +
> +static enum drm_connector_status
> +intelvipfb_drm_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	return connector_status_connected;
> +}
> +
> +static void intelvipfb_drm_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs intelvipfb_drm_connector_funcs = {
> +	.dpms = drm_helper_connector_dpms,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.detect = intelvipfb_drm_connector_detect,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = intelvipfb_drm_connector_destroy,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int intelvipfb_drm_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *drm = connector->dev;
> +	int count;
> +
> +	count = drm_add_modes_noedid(connector, drm->mode_config.max_width,
> +				     drm->mode_config.max_height);
> +	drm_set_preferred_mode(connector, drm->mode_config.max_width,
> +			       drm->mode_config.max_height);
> +	return count;
> +}

You're adding a bunch of modes here, but I don't see anywhere in the
driver that you change the resolution being scanned out.

If you can't change resolution, then I'd probably use drm_gtf_mode() or
something to generate just the one mode (do you have a vrefresh for
it?).  Also, in the simple display pipe's atomic_check, make sure that
the mode chosen is the width/height you can support.

> +
> +static const struct drm_connector_helper_funcs
> +intelvipfb_drm_connector_helper_funcs = {
> +	.get_modes = intelvipfb_drm_connector_get_modes,
> +};
> +
> +struct drm_connector *
> +intelvipfb_conn_setup(struct drm_device *drm)
> +{
> +	struct drm_connector *conn;
> +	int ret;
> +
> +	conn = devm_kzalloc(drm->dev, sizeof(*conn), GFP_KERNEL);
> +	if (IS_ERR(conn))
> +		return NULL;
> +
> +	ret = drm_connector_init(drm, conn, &intelvipfb_drm_connector_funcs,
> +				 DRM_MODE_CONNECTOR_DisplayPort);

Are you actually outputting DisplayPort
(https://en.wikipedia.org/wiki/DisplayPort)?

You probably want something else from the DRM_MODE_CONNECTOR list, or
maybe just DRM_MODE_CONNECTOR_Unknown.


> +	if (ret < 0) {
> +		dev_err(drm->dev, "failed to initialize drm connector\n");
> +		ret = -ENOMEM;
> +		goto error_connector_cleanup;
> +	}
> +
> +	drm_connector_helper_add(conn, &intelvipfb_drm_connector_helper_funcs);
> +
> +	return conn;
> +
> +error_connector_cleanup:
> +	drm_connector_cleanup(conn);
> +
> +	return NULL;
> +}
> diff --git a/drivers/gpu/drm/ivip/intel_vip_core.c b/drivers/gpu/drm/ivip/intel_vip_core.c
> new file mode 100644
> index 0000000..4e83343
> --- /dev/null
> +++ b/drivers/gpu/drm/ivip/intel_vip_core.c
> @@ -0,0 +1,171 @@
> +/*
> + * intel_vip_core.c -- Intel Video and Image Processing(VIP)
> + * Frame Buffer II driver
> + *
> + * This driver supports the Intel VIP Frame Reader component.
> + * More info on the hardware can be found in the Intel Video
> + * and Image Processing Suite User Guide at this address
> + * http://www.altera.com/literature/ug/ug_vip.pdf.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * Authors:
> + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +#include "intel_vip_drv.h"
> +
> +static const u32 fbpriv_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_RGB565
> +};

You're registering that you support this set of formats, but I don't see
anything programming the hardware differently based on the format of the
plane.

> +static void intelvipfb_start_hw(void __iomem *base, resource_size_t addr)
> +{
> +	/*
> +	 * The frameinfo variable has to correspond to the size of the VIP Suite
> +	 * Frame Reader register 7 which will determine the maximum size used
> +	 * in this frameinfo
> +	 */
> +
> +	u32 frameinfo;
> +
> +	frameinfo =
> +		readl(base + INTELVIPFB_FRAME_READER) & 0x00ffffff;
> +	writel(frameinfo, base + INTELVIPFB_FRAME_INFO);
> +	writel(addr, base + INTELVIPFB_FRAME_START);
> +	/* Finally set the control register to 1 to start streaming */
> +	writel(1, base + INTELVIPFB_CONTROL);
> +}

The addr you're passing in here is from dev->mode_config.fb_base, which
is this weird sideband value from drm_fbdev_cma.  It's the wrong value
to use if anything else uses the KMS interfaces to change the plane --
you should be using
drm_fb_cma_get_gem_addr(drm_simple_display_pipe->plane->state, 0)
instead.

> +
> +static void intelvipfb_disable_hw(void __iomem *base)
> +{
> +	/* set the control register to 0 to stop streaming */
> +	writel(0, base + INTELVIPFB_CONTROL);
> +}

These two functions should be be called from fbpriv_funcs.enable() and
.disable(), not device load/unload.

> +static const struct drm_mode_config_funcs intelvipfb_mode_config_funcs = {
> +	.fb_create = drm_fb_cma_create,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static struct drm_mode_config_helper_funcs intelvipfb_mode_config_helpers = {
> +	.atomic_commit_tail = drm_atomic_helper_commit_tail,
> +};
> +
> +static void intelvipfb_setup_mode_config(struct drm_device *drm)
> +{
> +	drm_mode_config_init(drm);
> +	drm->mode_config.funcs = &intelvipfb_mode_config_funcs;
> +	drm->mode_config.helper_private = &intelvipfb_mode_config_helpers;
> +}
> +
> +static int intelvipfb_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
> +				      struct drm_plane_state *plane_state)
> +{
> +	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
> +}
> +
> +static struct drm_simple_display_pipe_funcs fbpriv_funcs = {
> +	.prepare_fb = intelvipfb_pipe_prepare_fb,
> +};
> +
> +int intelvipfb_probe(struct device *dev, void __iomem *base)
> +{
> +	int retval;
> +	struct drm_device *drm;
> +	struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev);
> +	struct drm_connector *connector;
> +
> +	dev_set_drvdata(dev, fbpriv);
> +
> +	drm = fbpriv->drm;
> +
> +	intelvipfb_setup_mode_config(drm);
> +
> +	connector = intelvipfb_conn_setup(drm);
> +	if (!connector) {
> +		dev_err(drm->dev, "Connector setup failed\n");
> +		goto err_mode_config;
> +	}
> +
> +	retval = drm_simple_display_pipe_init(drm, &fbpriv->pipe,
> +					      &fbpriv_funcs,
> +					      fbpriv_formats,
> +					      ARRAY_SIZE(fbpriv_formats),
> +					      connector);
> +	if (retval < 0) {
> +		dev_err(drm->dev, "Cannot setup simple display pipe\n");
> +		goto err_mode_config;
> +	}
> +
> +	fbpriv->fbcma = drm_fbdev_cma_init(drm, PREF_BPP,
> +					   drm->mode_config.num_connector);
> +	if (!fbpriv->fbcma) {
> +		fbpriv->fbcma = NULL;
> +		dev_err(drm->dev, "Failed to init FB CMA area\n");
> +		goto err_mode_config;
> +	}

On failure, drm_fbdev_cma_init() returns an ERR_PTR, not NULL.

Also, you're passing this PREF_BPP value here, ignoring
the altr,bits-per-symbol property.  That seems wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Ong, Hean Loong]

On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
> hean.loong.ong@intel.com writes:
> 
> > 
> > From: Ong Hean Loong <hean.loong.ong@intel.com>
> > 
> > Hi,
> > 
> > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
> > component 
> > which requires a new driver. This is a virtual driver in which the
> > FGPA hardware would enable the Display Port based on the
> > information
> > and data provided from the DRM frame buffer from the OS. Basically
> > all
> > all information with reagrds to resolution and bits per pixel are
> > pre-configured on the FPGA design and these information are fed to
> > the driver via the device tree information as part of the hardware 
> > information.
> I started reviewing the code, but I want to make sure I understand
> what's going on:
> 
> This IP core isn't displaying contents from system memory on some
> sort
> of actual physical display, right?  It's a core that takes some input
> video stream (not described in the DT or in this driver) and stores
> it
> to memory?

If the IP Core you are referring to is some form of GPU then in this
case we are using the Intel FPGA Display Port Framebuffer IP. It does
display contents streamed from the ARM/Linux system to the Display Port
of the physical Monitor.

Below a simple illustration of the system:

ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
				|
				|
			Physical Connection
			Display Port

Re: [PATCHv2 2/3] ARM: drm: Intel FPGA VIP Frame Buffer II drm driver

[Ong, Hean Loong]

On Wed, 2017-05-03 at 13:34 -0700, Eric Anholt wrote:
> hean.loong.ong@intel.com writes:
> 
> > 
> > From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> > 
> > Driver for Intel FPGA Video and Image Processing
> > Suite Frame Buffer II. The driver only supports the Intel
> > Arria10 devkit and its variants. This driver can be either
> > loaded staticlly or in modules. The OF device tree binding
> > is located at:
> > Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > 
> > Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> I'm not sure if this driver is going to make sense as-is, if it
> doesn't
> actually do display of planes from system memory.  But in case I was
> wrong, here's my review:
> 
> 
> > 
> > diff --git a/drivers/gpu/drm/ivip/intel_vip_conn.c
> > b/drivers/gpu/drm/ivip/intel_vip_conn.c
> > new file mode 100644
> > index 0000000..499d3b4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/ivip/intel_vip_conn.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * intel_vip_conn.c -- Intel Video and Image Processing(VIP)
> > + * Frame Buffer II driver
> > + *
> > + * This driver supports the Intel VIP Frame Reader component.
> > + * More info on the hardware can be found in the Intel Video
> > + * and Image Processing Suite User Guide at this address
> > + * http://www.altera.com/literature/ug/ug_vip.pdf.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License for
> > + * more details.
> > + *
> > + * Authors:
> > + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_encoder_slave.h>
> > +#include <drm/drm_plane_helper.h>
> > +
> > +static enum drm_connector_status
> > +intelvipfb_drm_connector_detect(struct drm_connector *connector,
> > bool force)
> > +{
> > +	return connector_status_connected;
> > +}
> > +
> > +static void intelvipfb_drm_connector_destroy(struct drm_connector
> > *connector)
> > +{
> > +	drm_connector_unregister(connector);
> > +	drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs
> > intelvipfb_drm_connector_funcs = {
> > +	.dpms = drm_helper_connector_dpms,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.detect = intelvipfb_drm_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = intelvipfb_drm_connector_destroy,
> > +	.atomic_duplicate_state =
> > drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state =
> > drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int intelvipfb_drm_connector_get_modes(struct drm_connector
> > *connector)
> > +{
> > +	struct drm_device *drm = connector->dev;
> > +	int count;
> > +
> > +	count = drm_add_modes_noedid(connector, drm-
> > >mode_config.max_width,
> > +				     drm->mode_config.max_height);
> > +	drm_set_preferred_mode(connector, drm-
> > >mode_config.max_width,
> > +			       drm->mode_config.max_height);
> > +	return count;
> > +}
> You're adding a bunch of modes here, but I don't see anywhere in the
> driver that you change the resolution being scanned out.
> 
> If you can't change resolution, then I'd probably use drm_gtf_mode()
> or
> something to generate just the one mode (do you have a vrefresh for
> it?).  Also, in the simple display pipe's atomic_check, make sure
> that
> the mode chosen is the width/height you can support.
> 
From the drivers perpective the resolution is dependant on how the FPGA
Display Port Framebuffer hardwware IP is designed. I would take a look
into how drm_gtf_mode() works compared to this.
> > 
> > +
> > +static const struct drm_connector_helper_funcs
> > +intelvipfb_drm_connector_helper_funcs = {
> > +	.get_modes = intelvipfb_drm_connector_get_modes,
> > +};
> > +
> > +struct drm_connector *
> > +intelvipfb_conn_setup(struct drm_device *drm)
> > +{
> > +	struct drm_connector *conn;
> > +	int ret;
> > +
> > +	conn = devm_kzalloc(drm->dev, sizeof(*conn), GFP_KERNEL);
> > +	if (IS_ERR(conn))
> > +		return NULL;
> > +
> > +	ret = drm_connector_init(drm, conn,
> > &intelvipfb_drm_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DisplayPort);
> Are you actually outputting DisplayPort
> (https://en.wikipedia.org/wiki/DisplayPort)?
> 
> You probably want something else from the DRM_MODE_CONNECTOR list, or
> maybe just DRM_MODE_CONNECTOR_Unknown.
> 
The Physical Connector is a Display Port connector. A simple
introduction of the product is found here.
https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literat
ure/an/an793.pdf
> 
> > 
> > +	if (ret < 0) {
> > +		dev_err(drm->dev, "failed to initialize drm
> > connector\n");
> > +		ret = -ENOMEM;
> > +		goto error_connector_cleanup;
> > +	}
> > +
> > +	drm_connector_helper_add(conn,
> > &intelvipfb_drm_connector_helper_funcs);
> > +
> > +	return conn;
> > +
> > +error_connector_cleanup:
> > +	drm_connector_cleanup(conn);
> > +
> > +	return NULL;
> > +}
> > diff --git a/drivers/gpu/drm/ivip/intel_vip_core.c
> > b/drivers/gpu/drm/ivip/intel_vip_core.c
> > new file mode 100644
> > index 0000000..4e83343
> > --- /dev/null
> > +++ b/drivers/gpu/drm/ivip/intel_vip_core.c
> > @@ -0,0 +1,171 @@
> > +/*
> > + * intel_vip_core.c -- Intel Video and Image Processing(VIP)
> > + * Frame Buffer II driver
> > + *
> > + * This driver supports the Intel VIP Frame Reader component.
> > + * More info on the hardware can be found in the Intel Video
> > + * and Image Processing Suite User Guide at this address
> > + * http://www.altera.com/literature/ug/ug_vip.pdf.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License for
> > + * more details.
> > + *
> > + * Authors:
> > + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> > +
> > +#include "intel_vip_drv.h"
> > +
> > +static const u32 fbpriv_formats[] = {
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_RGB565
> > +};
> You're registering that you support this set of formats, but I don't
> see
> anything programming the hardware differently based on the format of
> the
> plane.
> 
> > 
> > +static void intelvipfb_start_hw(void __iomem *base,
> > resource_size_t addr)
> > +{
> > +	/*
> > +	 * The frameinfo variable has to correspond to the size of
> > the VIP Suite
> > +	 * Frame Reader register 7 which will determine the
> > maximum size used
> > +	 * in this frameinfo
> > +	 */
> > +
> > +	u32 frameinfo;
> > +
> > +	frameinfo =
> > +		readl(base + INTELVIPFB_FRAME_READER) &
> > 0x00ffffff;
> > +	writel(frameinfo, base + INTELVIPFB_FRAME_INFO);
> > +	writel(addr, base + INTELVIPFB_FRAME_START);
> > +	/* Finally set the control register to 1 to start
> > streaming */
> > +	writel(1, base + INTELVIPFB_CONTROL);
> > +}
> The addr you're passing in here is from dev->mode_config.fb_base,
> which
> is this weird sideband value from drm_fbdev_cma.  It's the wrong
> value
> to use if anything else uses the KMS interfaces to change the plane
> --
> you should be using
> drm_fb_cma_get_gem_addr(drm_simple_display_pipe->plane->state, 0)
> instead.
> 
Thank you for highlighting this. Will look into using
drm_fb_cma_get_gem_addr(drm_simple_display_pipe->plane->state, 0)
> > 
> > +
> > +static void intelvipfb_disable_hw(void __iomem *base)
> > +{
> > +	/* set the control register to 0 to stop streaming */
> > +	writel(0, base + INTELVIPFB_CONTROL);
> > +}
> These two functions should be be called from fbpriv_funcs.enable()
> and
> .disable(), not device load/unload.
> 
Since I am not supporting hotplugging here would it make a difference?
>  
> > 
> > +static const struct drm_mode_config_funcs
> > intelvipfb_mode_config_funcs = {
> > +	.fb_create = drm_fb_cma_create,
> > +	.atomic_check = drm_atomic_helper_check,
> > +	.atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> > +static struct drm_mode_config_helper_funcs
> > intelvipfb_mode_config_helpers = {
> > +	.atomic_commit_tail = drm_atomic_helper_commit_tail,
> > +};
> > +
> > +static void intelvipfb_setup_mode_config(struct drm_device *drm)
> > +{
> > +	drm_mode_config_init(drm);
> > +	drm->mode_config.funcs = &intelvipfb_mode_config_funcs;
> > +	drm->mode_config.helper_private =
> > &intelvipfb_mode_config_helpers;
> > +}
> > +
> > +static int intelvipfb_pipe_prepare_fb(struct
> > drm_simple_display_pipe *pipe,
> > +				      struct drm_plane_state
> > *plane_state)
> > +{
> > +	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
> > +}
> > +
> > +static struct drm_simple_display_pipe_funcs fbpriv_funcs = {
> > +	.prepare_fb = intelvipfb_pipe_prepare_fb,
> > +};
> > +
> > +int intelvipfb_probe(struct device *dev, void __iomem *base)
> > +{
> > +	int retval;
> > +	struct drm_device *drm;
> > +	struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev);
> > +	struct drm_connector *connector;
> > +
> > +	dev_set_drvdata(dev, fbpriv);
> > +
> > +	drm = fbpriv->drm;
> > +
> > +	intelvipfb_setup_mode_config(drm);
> > +
> > +	connector = intelvipfb_conn_setup(drm);
> > +	if (!connector) {
> > +		dev_err(drm->dev, "Connector setup failed\n");
> > +		goto err_mode_config;
> > +	}
> > +
> > +	retval = drm_simple_display_pipe_init(drm, &fbpriv->pipe,
> > +					      &fbpriv_funcs,
> > +					      fbpriv_formats,
> > +					      ARRAY_SIZE(fbpriv_fo
> > rmats),
> > +					      connector);
> > +	if (retval < 0) {
> > +		dev_err(drm->dev, "Cannot setup simple display
> > pipe\n");
> > +		goto err_mode_config;
> > +	}
> > +
> > +	fbpriv->fbcma = drm_fbdev_cma_init(drm, PREF_BPP,
> > +					   drm-
> > >mode_config.num_connector);
> > +	if (!fbpriv->fbcma) {
> > +		fbpriv->fbcma = NULL;
> > +		dev_err(drm->dev, "Failed to init FB CMA area\n");
> > +		goto err_mode_config;
> > +	}
> On failure, drm_fbdev_cma_init() returns an ERR_PTR, not NULL.
> 
> Also, you're passing this PREF_BPP value here, ignoring
> the altr,bits-per-symbol property.  That seems wrong.

Noted thanks for highlighting.

Re: [PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings

[Neil Armstrong]

On 04/25/2017 04:06 AM, hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
> From: "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Device tree binding for Intel FPGA Video and Image
> Processing Suite. The binding involved would be generated
> from the Altera (Intel) Qsys system. The bindings would
> set the max width, max height, buts per pixel and memory
> port width. The device tree binding only supports the Intel
> Arria10 devkit and its variants. Vendor name retained as
> altr.
> 
> Signed-off-by: Ong, Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> v2:
> * Moved Device Tree bindings to Documentation/devicetree/bindings/display/
> * Added vendor name altr, to description
> ---
>  .../devicetree/bindings/display/altr,vip-fb2.txt   | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/altr,vip-fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> new file mode 100644
> index 0000000..bdffefb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> @@ -0,0 +1,30 @@
> +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> +
> +Supported hardware: Arria 10 and above with display port IP
> +

Hi,

> +The drm driver for the Arria 10 devkit would require the display resolution
> +and pixel information to be included as these values are generated based
> +on the FPGA design that drives the video connector attached to the drm driver
> +Information the FPGA video IP component can be acquired from
> +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_vip.pdf

The bindings should not reference the driver, but only the hardware and the way it is configured
and interconnected with the system.
Please explicit that this is an IP component that can be configured with explicit limits on
the display widths and heights and memory parameters.

But you should also indicate over what this IP is connected and add a port connected to a connector
node in case this ip is not used on a specific board like in [1] or [2] :

"""
Required nodes:

The connections to the DU output video ports are modeled using the OF graph
bindings specified in Documentation/devicetree/bindings/graph.txt.

The following table lists for each supported model the port number
corresponding to each DU output.

		Port 0		Port1		Port2		Port3
-----------------------------------------------------------------------------
 R8A7779 (H1)	DPAD 0		DPAD 1		-		-
"""

You may also need to add a "Display Port" connector binding aswell along the HDMI, VGA, ....

I know this targets an FPGA system, so you should explicit that in the description.

> +Required properties:
> +
> +- compatible: "altr,vip-frame-buffer-2.0"

You should also add model specific compatible strings for each supported FPGA devices.

> +- reg: Physical base address and length of the framebuffer controller's
> +  registers.
> +- altr,max-width: The width of the framebuffer in pixels.
> +- altr,max-height: The height of the framebuffer in pixels.
> +- altr,bits-per-symbol: only "8" is currently supported
> +- altr,mem-port-width = the bus width of the avalon master port on the frame reader

What is the avalon master port ?
Can you add a schema to explicit how this IP is connected to the system ?

> +
> +Example:
> +
> +	dp_0_frame_buf: vip@100000280 {
> +			compatible = "altr,vip-frame-buffer-2.0";
> +			reg = <0x00000001 0x00000280 0x00000040>;
> +			altr,max-width = <1280>;
> +			altr,max-height = <720>;
> +			altr,bits-per-symbol = <8>;
> +			altr,mem-port-width = <128>;
> +	};
> 


[1] Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
[2] Documentation/devicetree/bindings/display/renesas,du.txt

Thanks,

Neil

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings

[Ong, Hean Loong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4937 bytes --]

Thanks Neil. 

On Thu, 2017-05-04 at 09:55 +0200, Neil Armstrong wrote:
> On 04/25/2017 04:06 AM, hean.loong.ong@intel.com wrote:
> > 
> > From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> > 
> > Device tree binding for Intel FPGA Video and Image
> > Processing Suite. The binding involved would be generated
> > from the Altera (Intel) Qsys system. The bindings would
> > set the max width, max height, buts per pixel and memory
> > port width. The device tree binding only supports the Intel
> > Arria10 devkit and its variants. Vendor name retained as
> > altr.
> > 
> > Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> > ---
> > v2:
> > * Moved Device Tree bindings to
> > Documentation/devicetree/bindings/display/
> > * Added vendor name altr, to description
> > ---
> >  .../devicetree/bindings/display/altr,vip-fb2.txt   | 30
> > ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt
> > new file mode 100644
> > index 0000000..bdffefb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > @@ -0,0 +1,30 @@
> > +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> > +
> > +Supported hardware: Arria 10 and above with display port IP
> > +
> Hi,
> 
> > 
> > +The drm driver for the Arria 10 devkit would require the display
> > resolution
> > +and pixel information to be included as these values are generated
> > based
> > +on the FPGA design that drives the video connector attached to the
> > drm driver
> > +Information the FPGA video IP component can be acquired from
> > +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/li
> > terature/ug/ug_vip.pdf
> The bindings should not reference the driver, but only the hardware
> and the way it is configured
> and interconnected with the system.
> Please explicit that this is an IP component that can be configured
> with explicit limits on
> the display widths and heights and memory parameters.
> 
> But you should also indicate over what this IP is connected and add a
> port connected to a connector
> node in case this ip is not used on a specific board like in [1] or
> [2] :
> 
> """
> Required nodes:
> 
> The connections to the DU output video ports are modeled using the OF
> graph
> bindings specified in Documentation/devicetree/bindings/graph.txt.
> 
> The following table lists for each supported model the port number
> corresponding to each DU output.
> 
> 		Port 0		Port1		Port2	
> 	Port3
> -------------------------------------------------------------------
> ----------
>  R8A7779 (H1)	DPAD 0		DPAD 1		-	
> 	-
> """
> 
> You may also need to add a "Display Port" connector binding aswell
> along the HDMI, VGA, ....
> 
> I know this targets an FPGA system, so you should explicit that in
> the description.
> 
> > 
> > +Required properties:
> > +
> > +- compatible: "altr,vip-frame-buffer-2.0"
> You should also add model specific compatible strings for each
> supported FPGA devices.
> 
> > 
> > +- reg: Physical base address and length of the framebuffer
> > controller's
> > +  registers.
> > +- altr,max-width: The width of the framebuffer in pixels.
> > +- altr,max-height: The height of the framebuffer in pixels.
> > +- altr,bits-per-symbol: only "8" is currently supported
> > +- altr,mem-port-width = the bus width of the avalon master port on
> > the frame reader
> What is the avalon master port ?
A memory bus that on the FPGA that interfaces with the ARM processor
> Can you add a schema to explicit how this IP is connected to the
> system ?
> 
I could like to include a more detailed schema for the next updated
patch. In the mean time the details of the IP is provided on the links
below:
https://www.altera.com/products/intellectual-property/ip/interface-prot
ocols/m-alt-displayport-megacore.html

https://www.altera.com/documentation/yru1480906794402.html

but basically the idea is:
ARM/Linux -->FPGA(Avalon-MM interface)-->DisplayPort Connector

> > 
> > +
> > +Example:
> > +
> > +	dp_0_frame_buf: vip@100000280 {
> > +			compatible = "altr,vip-frame-buffer-2.0";
> > +			reg = <0x00000001 0x00000280 0x00000040>;
> > +			altr,max-width = <1280>;
> > +			altr,max-height = <720>;
> > +			altr,bits-per-symbol = <8>;
> > +			altr,mem-port-width = <128>;
> > +	};
> > 
> 
> [1] Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> [2] Documentation/devicetree/bindings/display/renesas,du.txt
> 
> Thanks,
> 
> Neil
> N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Daniel Vetter]

On Wed, May 3, 2017 at 10:28 PM, Eric Anholt <eric@anholt.net> wrote:
>> From: Ong Hean Loong <hean.loong.ong@intel.com>
>>
>> Hi,
>>
>> The new Intel Arria10 SOC FPGA devkit has a Display Port IP component
>> which requires a new driver. This is a virtual driver in which the
>> FGPA hardware would enable the Display Port based on the information
>> and data provided from the DRM frame buffer from the OS. Basically all
>> all information with reagrds to resolution and bits per pixel are
>> pre-configured on the FPGA design and these information are fed to
>> the driver via the device tree information as part of the hardware
>> information.
>
> I started reviewing the code, but I want to make sure I understand
> what's going on:
>
> This IP core isn't displaying contents from system memory on some sort
> of actual physical display, right?  It's a core that takes some input
> video stream (not described in the DT or in this driver) and stores it
> to memory?

I assumed it's the input IP core in the linked pdf, i.e. the CVI
(clocked video input). There's also a CVO (clocked video output), and
that would indeed be a misfit for drm. Definitely need to clarify this
(in the DT description).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Eric Anholt]

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

"Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
>> hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org writes:
>> 
>> > 
>> > From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > 
>> > Hi,
>> > 
>> > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
>> > component 
>> > which requires a new driver. This is a virtual driver in which the
>> > FGPA hardware would enable the Display Port based on the
>> > information
>> > and data provided from the DRM frame buffer from the OS. Basically
>> > all
>> > all information with reagrds to resolution and bits per pixel are
>> > pre-configured on the FPGA design and these information are fed to
>> > the driver via the device tree information as part of the hardware 
>> > information.
>> I started reviewing the code, but I want to make sure I understand
>> what's going on:
>> 
>> This IP core isn't displaying contents from system memory on some
>> sort
>> of actual physical display, right?  It's a core that takes some input
>> video stream (not described in the DT or in this driver) and stores
>> it
>> to memory?
>
> If the IP Core you are referring to is some form of GPU then in this
> case we are using the Intel FPGA Display Port Framebuffer IP. It does
> display contents streamed from the ARM/Linux system to the Display Port
> of the physical Monitor.
>
> Below a simple illustration of the system:
>
> ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
> 				|
> 				|
> 			Physical Connection
> 			Display Port

The "DMA" in this diagram is the frame reader IP, right?  The frame
reader, as described in the spec, sounds approximately like a DRM plane,
so if you have that in your system then that needs to be part of this
DRM driver (otherwise you won't be putting the right things on the
screen!).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Ong, Hean Loong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2552 bytes --]

On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
> "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> 
> > 
> > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
> > > 
> > > hean.loong.ong@intel.com writes:
> > > 
> > > > 
> > > > 
> > > > From: Ong Hean Loong <hean.loong.ong@intel.com>
> > > > 
> > > > Hi,
> > > > 
> > > > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
> > > > component 
> > > > which requires a new driver. This is a virtual driver in which
> > > > the
> > > > FGPA hardware would enable the Display Port based on the
> > > > information
> > > > and data provided from the DRM frame buffer from the OS.
> > > > Basically
> > > > all
> > > > all information with reagrds to resolution and bits per pixel
> > > > are
> > > > pre-configured on the FPGA design and these information are fed
> > > > to
> > > > the driver via the device tree information as part of the
> > > > hardware 
> > > > information.
> > > I started reviewing the code, but I want to make sure I
> > > understand
> > > what's going on:
> > > 
> > > This IP core isn't displaying contents from system memory on some
> > > sort
> > > of actual physical display, right?  It's a core that takes some
> > > input
> > > video stream (not described in the DT or in this driver) and
> > > stores
> > > it
> > > to memory?
> > If the IP Core you are referring to is some form of GPU then in
> > this
> > case we are using the Intel FPGA Display Port Framebuffer IP. It
> > does
> > display contents streamed from the ARM/Linux system to the Display
> > Port
> > of the physical Monitor.
> > 
> > Below a simple illustration of the system:
> > 
> > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
> > 				|
> > 				|
> > 			Physical Connection
> > 			Display Port
> The "DMA" in this diagram is the frame reader IP, right?  The frame
> reader, as described in the spec, sounds approximately like a DRM
> plane,
> so if you have that in your system then that needs to be part of this
> DRM driver (otherwise you won't be putting the right things on the
> screen!).

Would the drm_simple_display_pipe_init be able to handle this ? It
seems to be displaying the proper images on screen, based on my current
changes. There were recommendations to use the
drm_simple_display_pipe_init instead of creating the CRTC and planes
myselfN‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Eric Anholt]

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

"Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
>> "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
>> 
>> > 
>> > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
>> > > 
>> > > hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org writes:
>> > > 
>> > > > 
>> > > > 
>> > > > From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > > > 
>> > > > Hi,
>> > > > 
>> > > > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
>> > > > component 
>> > > > which requires a new driver. This is a virtual driver in which
>> > > > the
>> > > > FGPA hardware would enable the Display Port based on the
>> > > > information
>> > > > and data provided from the DRM frame buffer from the OS.
>> > > > Basically
>> > > > all
>> > > > all information with reagrds to resolution and bits per pixel
>> > > > are
>> > > > pre-configured on the FPGA design and these information are fed
>> > > > to
>> > > > the driver via the device tree information as part of the
>> > > > hardware 
>> > > > information.
>> > > I started reviewing the code, but I want to make sure I
>> > > understand
>> > > what's going on:
>> > > 
>> > > This IP core isn't displaying contents from system memory on some
>> > > sort
>> > > of actual physical display, right?  It's a core that takes some
>> > > input
>> > > video stream (not described in the DT or in this driver) and
>> > > stores
>> > > it
>> > > to memory?
>> > If the IP Core you are referring to is some form of GPU then in
>> > this
>> > case we are using the Intel FPGA Display Port Framebuffer IP. It
>> > does
>> > display contents streamed from the ARM/Linux system to the Display
>> > Port
>> > of the physical Monitor.
>> > 
>> > Below a simple illustration of the system:
>> > 
>> > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
>> > 				|
>> > 				|
>> > 			Physical Connection
>> > 			Display Port
>> The "DMA" in this diagram is the frame reader IP, right?  The frame
>> reader, as described in the spec, sounds approximately like a DRM
>> plane,
>> so if you have that in your system then that needs to be part of this
>> DRM driver (otherwise you won't be putting the right things on the
>> screen!).
>
> Would the drm_simple_display_pipe_init be able to handle this ? It
> seems to be displaying the proper images on screen, based on my current
> changes. There were recommendations to use the
> drm_simple_display_pipe_init instead of creating the CRTC and planes
> myself

The docs I've read for the Frame Buffer IP don't fit into any current
DRM component, since it's what we're currently calling a "writeback
connector".

I don't understand your system enough to answer.  You didn't answer if
the "DMA" block you diagrammed was the frame reader IP (or maybe the
frame reader IP comes after).  I also don't see how the frame buffer IP
could possibly be connected directly to a physical display connector.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Ong, Hean Loong]

On Mon, 2017-05-08 at 09:03 -0700, Eric Anholt wrote:
> "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> 
> > 
> > On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
> > > 
> > > "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> > > 
> > > > 
> > > > 
> > > > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
> > > > > 
> > > > > 
> > > > > hean.loong.ong@intel.com writes:
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Ong Hean Loong <hean.loong.ong@intel.com>
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
> > > > > > component 
> > > > > > which requires a new driver. This is a virtual driver in
> > > > > > which
> > > > > > the
> > > > > > FGPA hardware would enable the Display Port based on the
> > > > > > information
> > > > > > and data provided from the DRM frame buffer from the OS.
> > > > > > Basically
> > > > > > all
> > > > > > all information with reagrds to resolution and bits per
> > > > > > pixel
> > > > > > are
> > > > > > pre-configured on the FPGA design and these information are
> > > > > > fed
> > > > > > to
> > > > > > the driver via the device tree information as part of the
> > > > > > hardware 
> > > > > > information.
> > > > > I started reviewing the code, but I want to make sure I
> > > > > understand
> > > > > what's going on:
> > > > > 
> > > > > This IP core isn't displaying contents from system memory on
> > > > > some
> > > > > sort
> > > > > of actual physical display, right?  It's a core that takes
> > > > > some
> > > > > input
> > > > > video stream (not described in the DT or in this driver) and
> > > > > stores
> > > > > it
> > > > > to memory?
> > > > If the IP Core you are referring to is some form of GPU then in
> > > > this
> > > > case we are using the Intel FPGA Display Port Framebuffer IP.
> > > > It
> > > > does
> > > > display contents streamed from the ARM/Linux system to the
> > > > Display
> > > > Port
> > > > of the physical Monitor.
> > > > 
> > > > Below a simple illustration of the system:
> > > > 
> > > > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
> > > > 				|
> > > > 				|
> > > > 			Physical Connection
> > > > 			Display Port
> > > The "DMA" in this diagram is the frame reader IP, right?  The
> > > frame
> > > reader, as described in the spec, sounds approximately like a DRM
> > > plane,
> > > so if you have that in your system then that needs to be part of
> > > this
> > > DRM driver (otherwise you won't be putting the right things on
> > > the
> > > screen!).
> > Would the drm_simple_display_pipe_init be able to handle this ? It
> > seems to be displaying the proper images on screen, based on my
> > current
> > changes. There were recommendations to use the
> > drm_simple_display_pipe_init instead of creating the CRTC and
> > planes
> > myself
> The docs I've read for the Frame Buffer IP don't fit into any current
> DRM component, since it's what we're currently calling a "writeback
> connector".
> 
While running my own test (since the intel-gpu-tools doesn't seems
applicable.) The drm_simple_display_pipe_init seems to be simple enough
for displaying a matchbox console. The use case for this driver is only
part of the enablemennt of the reference design board so that users of
the Arria10 devkit can use this as base for their own designs.

> I don't understand your system enough to answer.  You didn't answer
> if
> the "DMA" block you diagrammed was the frame reader IP (or maybe the
> frame reader IP comes after).  I also don't see how the frame buffer
> IP
> could possibly be connected directly to a physical display connector.

The FPGA FrameBuffer 2 soft IP reads the information provided to it
from the Linux Kernel via the platform device register provided in the
DT. The Linux driver here allocates a chunk of coherent memory that
directly maps to the SDRAM. The FPGA soft IP would stream the display
out to the Display Port based on the information that was written to
the SDRAM.

The FrameBuffer 2 IP or previously used Frame Reader IP are soft IP is
programmed as part of the FPGA which is connected to the Display Port
via a native physical transceiver. The Soft IP are driven by the NIOS 2
soft core which is also the soft processor. 

Note however whatever that goes beyond the the memory bus (between ARM
and FPGA) known as Avalon is no longer under the control of the Linux
Driver (as we only establish connection on the ARM side)
  

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Eric Anholt]

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

"Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> On Mon, 2017-05-08 at 09:03 -0700, Eric Anholt wrote:
>> "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
>> 
>> > 
>> > On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
>> > > 
>> > > "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
>> > > 
>> > > > 
>> > > > 
>> > > > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
>> > > > > 
>> > > > > 
>> > > > > hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org writes:
>> > > > > 
>> > > > > > 
>> > > > > > 
>> > > > > > 
>> > > > > > From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > > > > > 
>> > > > > > Hi,
>> > > > > > 
>> > > > > > The new Intel Arria10 SOC FPGA devkit has a Display Port IP
>> > > > > > component 
>> > > > > > which requires a new driver. This is a virtual driver in
>> > > > > > which
>> > > > > > the
>> > > > > > FGPA hardware would enable the Display Port based on the
>> > > > > > information
>> > > > > > and data provided from the DRM frame buffer from the OS.
>> > > > > > Basically
>> > > > > > all
>> > > > > > all information with reagrds to resolution and bits per
>> > > > > > pixel
>> > > > > > are
>> > > > > > pre-configured on the FPGA design and these information are
>> > > > > > fed
>> > > > > > to
>> > > > > > the driver via the device tree information as part of the
>> > > > > > hardware 
>> > > > > > information.
>> > > > > I started reviewing the code, but I want to make sure I
>> > > > > understand
>> > > > > what's going on:
>> > > > > 
>> > > > > This IP core isn't displaying contents from system memory on
>> > > > > some
>> > > > > sort
>> > > > > of actual physical display, right?  It's a core that takes
>> > > > > some
>> > > > > input
>> > > > > video stream (not described in the DT or in this driver) and
>> > > > > stores
>> > > > > it
>> > > > > to memory?
>> > > > If the IP Core you are referring to is some form of GPU then in
>> > > > this
>> > > > case we are using the Intel FPGA Display Port Framebuffer IP.
>> > > > It
>> > > > does
>> > > > display contents streamed from the ARM/Linux system to the
>> > > > Display
>> > > > Port
>> > > > of the physical Monitor.
>> > > > 
>> > > > Below a simple illustration of the system:
>> > > > 
>> > > > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
>> > > > 				|
>> > > > 				|
>> > > > 			Physical Connection
>> > > > 			Display Port
>> > > The "DMA" in this diagram is the frame reader IP, right?  The
>> > > frame
>> > > reader, as described in the spec, sounds approximately like a DRM
>> > > plane,
>> > > so if you have that in your system then that needs to be part of
>> > > this
>> > > DRM driver (otherwise you won't be putting the right things on
>> > > the
>> > > screen!).
>> > Would the drm_simple_display_pipe_init be able to handle this ? It
>> > seems to be displaying the proper images on screen, based on my
>> > current
>> > changes. There were recommendations to use the
>> > drm_simple_display_pipe_init instead of creating the CRTC and
>> > planes
>> > myself
>> The docs I've read for the Frame Buffer IP don't fit into any current
>> DRM component, since it's what we're currently calling a "writeback
>> connector".
>> 
> While running my own test (since the intel-gpu-tools doesn't seems
> applicable.) The drm_simple_display_pipe_init seems to be simple enough
> for displaying a matchbox console. The use case for this driver is only
> part of the enablemennt of the reference design board so that users of
> the Arria10 devkit can use this as base for their own designs.
>
>> I don't understand your system enough to answer.  You didn't answer
>> if
>> the "DMA" block you diagrammed was the frame reader IP (or maybe the
>> frame reader IP comes after).  I also don't see how the frame buffer
>> IP
>> could possibly be connected directly to a physical display connector.
>
> The FPGA FrameBuffer 2 soft IP reads the information provided to it
> from the Linux Kernel via the platform device register provided in the
> DT. The Linux driver here allocates a chunk of coherent memory that
> directly maps to the SDRAM. The FPGA soft IP would stream the display
> out to the Display Port based on the information that was written to
> the SDRAM.

Your "FPGA soft IP" that's reading pixels from an area of memory (a DRM
plane does this) and outputting that to a display port (a DRM CRTC and
encoder does this) would make a lot of sense as a DRM driver.

However, this piece of hardware that takes in pixels in a stream from
elsewhere and stores it to memory doesn't make sense to me as a DRM
driver.

I can't really offer more advice until I understand what's generating
the pixels that the FrameBuffer IP is storing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Ong, Hean Loong]

On Tue, 2017-05-09 at 09:40 -0700, Eric Anholt wrote:
> "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> 
> > 
> > On Mon, 2017-05-08 at 09:03 -0700, Eric Anholt wrote:
> > > 
> > > "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> > > 
> > > > 
> > > > 
> > > > On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
> > > > > 
> > > > > 
> > > > > "Ong, Hean Loong" <hean.loong.ong@intel.com> writes:
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > hean.loong.ong@intel.com writes:
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: Ong Hean Loong <hean.loong.ong@intel.com>
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > The new Intel Arria10 SOC FPGA devkit has a Display
> > > > > > > > Port IP
> > > > > > > > component 
> > > > > > > > which requires a new driver. This is a virtual driver
> > > > > > > > in
> > > > > > > > which
> > > > > > > > the
> > > > > > > > FGPA hardware would enable the Display Port based on
> > > > > > > > the
> > > > > > > > information
> > > > > > > > and data provided from the DRM frame buffer from the
> > > > > > > > OS.
> > > > > > > > Basically
> > > > > > > > all
> > > > > > > > all information with reagrds to resolution and bits per
> > > > > > > > pixel
> > > > > > > > are
> > > > > > > > pre-configured on the FPGA design and these information
> > > > > > > > are
> > > > > > > > fed
> > > > > > > > to
> > > > > > > > the driver via the device tree information as part of
> > > > > > > > the
> > > > > > > > hardware 
> > > > > > > > information.
> > > > > > > I started reviewing the code, but I want to make sure I
> > > > > > > understand
> > > > > > > what's going on:
> > > > > > > 
> > > > > > > This IP core isn't displaying contents from system memory
> > > > > > > on
> > > > > > > some
> > > > > > > sort
> > > > > > > of actual physical display, right?  It's a core that
> > > > > > > takes
> > > > > > > some
> > > > > > > input
> > > > > > > video stream (not described in the DT or in this driver)
> > > > > > > and
> > > > > > > stores
> > > > > > > it
> > > > > > > to memory?
> > > > > > If the IP Core you are referring to is some form of GPU
> > > > > > then in
> > > > > > this
> > > > > > case we are using the Intel FPGA Display Port Framebuffer
> > > > > > IP.
> > > > > > It
> > > > > > does
> > > > > > display contents streamed from the ARM/Linux system to the
> > > > > > Display
> > > > > > Port
> > > > > > of the physical Monitor.
> > > > > > 
> > > > > > Below a simple illustration of the system:
> > > > > > 
> > > > > > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
> > > > > > 				|
> > > > > > 				|
> > > > > > 			Physical Connection
> > > > > > 			Display Port
> > > > > The "DMA" in this diagram is the frame reader IP, right?  The
> > > > > frame
> > > > > reader, as described in the spec, sounds approximately like a
> > > > > DRM
> > > > > plane,
> > > > > so if you have that in your system then that needs to be part
> > > > > of
> > > > > this
> > > > > DRM driver (otherwise you won't be putting the right things
> > > > > on
> > > > > the
> > > > > screen!).
> > > > Would the drm_simple_display_pipe_init be able to handle this ?
> > > > It
> > > > seems to be displaying the proper images on screen, based on my
> > > > current
> > > > changes. There were recommendations to use the
> > > > drm_simple_display_pipe_init instead of creating the CRTC and
> > > > planes
> > > > myself
> > > The docs I've read for the Frame Buffer IP don't fit into any
> > > current
> > > DRM component, since it's what we're currently calling a
> > > "writeback
> > > connector".
> > > 
> > While running my own test (since the intel-gpu-tools doesn't seems
> > applicable.) The drm_simple_display_pipe_init seems to be simple
> > enough
> > for displaying a matchbox console. The use case for this driver is
> > only
> > part of the enablemennt of the reference design board so that users
> > of
> > the Arria10 devkit can use this as base for their own designs.
> > 
> > > 
> > > I don't understand your system enough to answer.  You didn't
> > > answer
> > > if
> > > the "DMA" block you diagrammed was the frame reader IP (or maybe
> > > the
> > > frame reader IP comes after).  I also don't see how the frame
> > > buffer
> > > IP
> > > could possibly be connected directly to a physical display
> > > connector.
> > The FPGA FrameBuffer 2 soft IP reads the information provided to it
> > from the Linux Kernel via the platform device register provided in
> > the
> > DT. The Linux driver here allocates a chunk of coherent memory that
> > directly maps to the SDRAM. The FPGA soft IP would stream the
> > display
> > out to the Display Port based on the information that was written
> > to
> > the SDRAM.
> Your "FPGA soft IP" that's reading pixels from an area of memory (a
> DRM
> plane does this) and outputting that to a display port (a DRM CRTC
> and
> encoder does this) would make a lot of sense as a DRM driver.
> 
> However, this piece of hardware that takes in pixels in a stream from
> elsewhere and stores it to memory doesn't make sense to me as a DRM
> driver.
> 
> I can't really offer more advice until I understand what's generating
> the pixels that the FrameBuffer IP is storing.

My previous explanation might have been confusing here. I apologize for
that. 
To put it in simpler terms the FPGA FrameBuffer Soft IP could be seen
as the GPU and the DRM driver patch here is allocating memory for
information to be streamed from the ARM/Linux to the display port.
Basically the driver just wraps the information such as the pixels to
be drawn by the FPGA FrameBuffer 2.

The piece of hardware in discussion is the SoC FPGA where Linux runs on
the ARM chip and the FGPA is driven by its NIOS soft core with its own
proprietary firmware.

For example the application from the ARM Linux would have to write
information on the /dev/fb0 with the information stored in the SDRAM to
be fetched by the FPGA framebuffer IP and displayed on the Display Port
Monitor. 

Re: [PATCHv2 0/3] Intel FPGA VIP Frame Buffer II DRM Driver

[Daniel Vetter]

On Thu, May 11, 2017 at 02:50:41AM +0000, Ong, Hean Loong wrote:
> On Tue, 2017-05-09 at 09:40 -0700, Eric Anholt wrote:
> > "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
> > 
> > > 
> > > On Mon, 2017-05-08 at 09:03 -0700, Eric Anholt wrote:
> > > > 
> > > > "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
> > > > 
> > > > > 
> > > > > 
> > > > > On Thu, 2017-05-04 at 10:11 -0700, Eric Anholt wrote:
> > > > > > 
> > > > > > 
> > > > > > "Ong, Hean Loong" <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Wed, 2017-05-03 at 13:28 -0700, Eric Anholt wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org writes:
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > From: Ong Hean Loong <hean.loong.ong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > > > > 
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > The new Intel Arria10 SOC FPGA devkit has a Display
> > > > > > > > > Port IP
> > > > > > > > > component 
> > > > > > > > > which requires a new driver. This is a virtual driver
> > > > > > > > > in
> > > > > > > > > which
> > > > > > > > > the
> > > > > > > > > FGPA hardware would enable the Display Port based on
> > > > > > > > > the
> > > > > > > > > information
> > > > > > > > > and data provided from the DRM frame buffer from the
> > > > > > > > > OS.
> > > > > > > > > Basically
> > > > > > > > > all
> > > > > > > > > all information with reagrds to resolution and bits per
> > > > > > > > > pixel
> > > > > > > > > are
> > > > > > > > > pre-configured on the FPGA design and these information
> > > > > > > > > are
> > > > > > > > > fed
> > > > > > > > > to
> > > > > > > > > the driver via the device tree information as part of
> > > > > > > > > the
> > > > > > > > > hardware 
> > > > > > > > > information.
> > > > > > > > I started reviewing the code, but I want to make sure I
> > > > > > > > understand
> > > > > > > > what's going on:
> > > > > > > > 
> > > > > > > > This IP core isn't displaying contents from system memory
> > > > > > > > on
> > > > > > > > some
> > > > > > > > sort
> > > > > > > > of actual physical display, right?  It's a core that
> > > > > > > > takes
> > > > > > > > some
> > > > > > > > input
> > > > > > > > video stream (not described in the DT or in this driver)
> > > > > > > > and
> > > > > > > > stores
> > > > > > > > it
> > > > > > > > to memory?
> > > > > > > If the IP Core you are referring to is some form of GPU
> > > > > > > then in
> > > > > > > this
> > > > > > > case we are using the Intel FPGA Display Port Framebuffer
> > > > > > > IP.
> > > > > > > It
> > > > > > > does
> > > > > > > display contents streamed from the ARM/Linux system to the
> > > > > > > Display
> > > > > > > Port
> > > > > > > of the physical Monitor.
> > > > > > > 
> > > > > > > Below a simple illustration of the system:
> > > > > > > 
> > > > > > > ARM/Linux --DMA-->Intel FPGA Display Port Framebuffer IP
> > > > > > > 				|
> > > > > > > 				|
> > > > > > > 			Physical Connection
> > > > > > > 			Display Port
> > > > > > The "DMA" in this diagram is the frame reader IP, right?  The
> > > > > > frame
> > > > > > reader, as described in the spec, sounds approximately like a
> > > > > > DRM
> > > > > > plane,
> > > > > > so if you have that in your system then that needs to be part
> > > > > > of
> > > > > > this
> > > > > > DRM driver (otherwise you won't be putting the right things
> > > > > > on
> > > > > > the
> > > > > > screen!).
> > > > > Would the drm_simple_display_pipe_init be able to handle this ?
> > > > > It
> > > > > seems to be displaying the proper images on screen, based on my
> > > > > current
> > > > > changes. There were recommendations to use the
> > > > > drm_simple_display_pipe_init instead of creating the CRTC and
> > > > > planes
> > > > > myself
> > > > The docs I've read for the Frame Buffer IP don't fit into any
> > > > current
> > > > DRM component, since it's what we're currently calling a
> > > > "writeback
> > > > connector".
> > > > 
> > > While running my own test (since the intel-gpu-tools doesn't seems
> > > applicable.) The drm_simple_display_pipe_init seems to be simple
> > > enough
> > > for displaying a matchbox console. The use case for this driver is
> > > only
> > > part of the enablemennt of the reference design board so that users
> > > of
> > > the Arria10 devkit can use this as base for their own designs.
> > > 
> > > > 
> > > > I don't understand your system enough to answer.  You didn't
> > > > answer
> > > > if
> > > > the "DMA" block you diagrammed was the frame reader IP (or maybe
> > > > the
> > > > frame reader IP comes after).  I also don't see how the frame
> > > > buffer
> > > > IP
> > > > could possibly be connected directly to a physical display
> > > > connector.
> > > The FPGA FrameBuffer 2 soft IP reads the information provided to it
> > > from the Linux Kernel via the platform device register provided in
> > > the
> > > DT. The Linux driver here allocates a chunk of coherent memory that
> > > directly maps to the SDRAM. The FPGA soft IP would stream the
> > > display
> > > out to the Display Port based on the information that was written
> > > to
> > > the SDRAM.
> > Your "FPGA soft IP" that's reading pixels from an area of memory (a
> > DRM
> > plane does this) and outputting that to a display port (a DRM CRTC
> > and
> > encoder does this) would make a lot of sense as a DRM driver.
> > 
> > However, this piece of hardware that takes in pixels in a stream from
> > elsewhere and stores it to memory doesn't make sense to me as a DRM
> > driver.
> > 
> > I can't really offer more advice until I understand what's generating
> > the pixels that the FrameBuffer IP is storing.
> 
> My previous explanation might have been confusing here. I apologize for
> that. 
> To put it in simpler terms the FPGA FrameBuffer Soft IP could be seen
> as the GPU and the DRM driver patch here is allocating memory for
> information to be streamed from the ARM/Linux to the display port.
> Basically the driver just wraps the information such as the pixels to
> be drawn by the FPGA FrameBuffer 2.
> 
> The piece of hardware in discussion is the SoC FPGA where Linux runs on
> the ARM chip and the FGPA is driven by its NIOS soft core with its own
> proprietary firmware.
> 
> For example the application from the ARM Linux would have to write
> information on the /dev/fb0 with the information stored in the SDRAM to
> be fetched by the FPGA framebuffer IP and displayed on the Display Port
> Monitor. 

Ah ok, this sounds like it's indeed suitable for drm. Thanks for clearing
up the confusion ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/3] ARM: drm: Intel FPGA VIP Frame Buffer II drm driver

[Ong, Hean Loong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11834 bytes --]

On Wed, 2017-05-03 at 13:34 -0700, Eric Anholt wrote:
> hean.loong.ong@intel.com writes:
> 
> > 
> > From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> > 
> > Driver for Intel FPGA Video and Image Processing
> > Suite Frame Buffer II. The driver only supports the Intel
> > Arria10 devkit and its variants. This driver can be either
> > loaded staticlly or in modules. The OF device tree binding
> > is located at:
> > Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > 
> > Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> I'm not sure if this driver is going to make sense as-is, if it
> doesn't
> actually do display of planes from system memory.  But in case I was
> wrong, here's my review:
> 
> 
> > 
> > diff --git a/drivers/gpu/drm/ivip/intel_vip_conn.c
> > b/drivers/gpu/drm/ivip/intel_vip_conn.c
> > new file mode 100644
> > index 0000000..499d3b4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/ivip/intel_vip_conn.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * intel_vip_conn.c -- Intel Video and Image Processing(VIP)
> > + * Frame Buffer II driver
> > + *
> > + * This driver supports the Intel VIP Frame Reader component.
> > + * More info on the hardware can be found in the Intel Video
> > + * and Image Processing Suite User Guide at this address
> > + * http://www.altera.com/literature/ug/ug_vip.pdf.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License for
> > + * more details.
> > + *
> > + * Authors:
> > + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_encoder_slave.h>
> > +#include <drm/drm_plane_helper.h>
> > +
> > +static enum drm_connector_status
> > +intelvipfb_drm_connector_detect(struct drm_connector *connector,
> > bool force)
> > +{
> > +	return connector_status_connected;
> > +}
> > +
> > +static void intelvipfb_drm_connector_destroy(struct drm_connector
> > *connector)
> > +{
> > +	drm_connector_unregister(connector);
> > +	drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs
> > intelvipfb_drm_connector_funcs = {
> > +	.dpms = drm_helper_connector_dpms,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.detect = intelvipfb_drm_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = intelvipfb_drm_connector_destroy,
> > +	.atomic_duplicate_state =
> > drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state =
> > drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int intelvipfb_drm_connector_get_modes(struct drm_connector
> > *connector)
> > +{
> > +	struct drm_device *drm = connector->dev;
> > +	int count;
> > +
> > +	count = drm_add_modes_noedid(connector, drm-
> > >mode_config.max_width,
> > +				     drm->mode_config.max_height);
> > +	drm_set_preferred_mode(connector, drm-
> > >mode_config.max_width,
> > +			       drm->mode_config.max_height);
> > +	return count;
> > +}
> You're adding a bunch of modes here, but I don't see anywhere in the
> driver that you change the resolution being scanned out.
> 
> If you can't change resolution, then I'd probably use drm_gtf_mode()
> or
> something to generate just the one mode (do you have a vrefresh for
> it?).  Also, in the simple display pipe's atomic_check, make sure
> that
> the mode chosen is the width/height you can support.
> 
I can see the point in the proposed approach.Based on the comment for
get_modes in drm_connector_helper_funcs the recommended approach was
using drm_add_modes_noedid and drm_set_preferred_mode. Not many drivers
uses the drm_gtf_mode directly therefore I am not inclined to this
approach.
> > 
> > +
> > +static const struct drm_connector_helper_funcs
> > +intelvipfb_drm_connector_helper_funcs = {
> > +	.get_modes = intelvipfb_drm_connector_get_modes,
> > +};
> > +
> > +struct drm_connector *
> > +intelvipfb_conn_setup(struct drm_device *drm)
> > +{
> > +	struct drm_connector *conn;
> > +	int ret;
> > +
> > +	conn = devm_kzalloc(drm->dev, sizeof(*conn), GFP_KERNEL);
> > +	if (IS_ERR(conn))
> > +		return NULL;
> > +
> > +	ret = drm_connector_init(drm, conn,
> > &intelvipfb_drm_connector_funcs,
> > +				 DRM_MODE_CONNECTOR_DisplayPort);
> Are you actually outputting DisplayPort
> (https://en.wikipedia.org/wiki/DisplayPort)?
> 
> You probably want something else from the DRM_MODE_CONNECTOR list, or
> maybe just DRM_MODE_CONNECTOR_Unknown.
> 
> 
> > 
> > +	if (ret < 0) {
> > +		dev_err(drm->dev, "failed to initialize drm
> > connector\n");
> > +		ret = -ENOMEM;
> > +		goto error_connector_cleanup;
> > +	}
> > +
> > +	drm_connector_helper_add(conn,
> > &intelvipfb_drm_connector_helper_funcs);
> > +
> > +	return conn;
> > +
> > +error_connector_cleanup:
> > +	drm_connector_cleanup(conn);
> > +
> > +	return NULL;
> > +}
> > diff --git a/drivers/gpu/drm/ivip/intel_vip_core.c
> > b/drivers/gpu/drm/ivip/intel_vip_core.c
> > new file mode 100644
> > index 0000000..4e83343
> > --- /dev/null
> > +++ b/drivers/gpu/drm/ivip/intel_vip_core.c
> > @@ -0,0 +1,171 @@
> > +/*
> > + * intel_vip_core.c -- Intel Video and Image Processing(VIP)
> > + * Frame Buffer II driver
> > + *
> > + * This driver supports the Intel VIP Frame Reader component.
> > + * More info on the hardware can be found in the Intel Video
> > + * and Image Processing Suite User Guide at this address
> > + * http://www.altera.com/literature/ug/ug_vip.pdf.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License for
> > + * more details.
> > + *
> > + * Authors:
> > + * Ong, Hean-Loong <hean.loong.ong@intel.com>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_helper.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> > +
> > +#include "intel_vip_drv.h"
> > +
> > +static const u32 fbpriv_formats[] = {
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_RGB565
> > +};
> You're registering that you support this set of formats, but I don't
> see
> anything programming the hardware differently based on the format of
> the
> plane.
> 
> > 
> > +static void intelvipfb_start_hw(void __iomem *base,
> > resource_size_t addr)
> > +{
> > +	/*
> > +	 * The frameinfo variable has to correspond to the size of
> > the VIP Suite
> > +	 * Frame Reader register 7 which will determine the
> > maximum size used
> > +	 * in this frameinfo
> > +	 */
> > +
> > +	u32 frameinfo;
> > +
> > +	frameinfo =
> > +		readl(base + INTELVIPFB_FRAME_READER) &
> > 0x00ffffff;
> > +	writel(frameinfo, base + INTELVIPFB_FRAME_INFO);
> > +	writel(addr, base + INTELVIPFB_FRAME_START);
> > +	/* Finally set the control register to 1 to start
> > streaming */
> > +	writel(1, base + INTELVIPFB_CONTROL);
> > +}
> The addr you're passing in here is from dev->mode_config.fb_base,
> which
> is this weird sideband value from drm_fbdev_cma.  It's the wrong
> value
> to use if anything else uses the KMS interfaces to change the plane
> --
> you should be using
> drm_fb_cma_get_gem_addr(drm_simple_display_pipe->plane->state, 0)
> instead.
> 
The function drm_fb_cma_get_gem_addr(drm_simple_display_pipe,
drm_simple_display_pipe->plane->state, 0) is causing an exception in
git tag next-20170526. I have confirmed that the pipe is okay. What
other configuration should I be looking into before using this function
> > 
> > +
> > +static void intelvipfb_disable_hw(void __iomem *base)
> > +{
> > +	/* set the control register to 0 to stop streaming */
> > +	writel(0, base + INTELVIPFB_CONTROL);
> > +}
> These two functions should be be called from fbpriv_funcs.enable()
> and
> .disable(), not device load/unload.
> 
I agree to this. However in my recent test (git tag next-20170526) it
does not seem to be triggered when the driver is loaded

> > 
> > +static const struct drm_mode_config_funcs
> > intelvipfb_mode_config_funcs = {
> > +	.fb_create = drm_fb_cma_create,
> > +	.atomic_check = drm_atomic_helper_check,
> > +	.atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> > +static struct drm_mode_config_helper_funcs
> > intelvipfb_mode_config_helpers = {
> > +	.atomic_commit_tail = drm_atomic_helper_commit_tail,
> > +};
> > +
> > +static void intelvipfb_setup_mode_config(struct drm_device *drm)
> > +{
> > +	drm_mode_config_init(drm);
> > +	drm->mode_config.funcs = &intelvipfb_mode_config_funcs;
> > +	drm->mode_config.helper_private =
> > &intelvipfb_mode_config_helpers;
> > +}
> > +
> > +static int intelvipfb_pipe_prepare_fb(struct
> > drm_simple_display_pipe *pipe,
> > +				      struct drm_plane_state
> > *plane_state)
> > +{
> > +	return drm_fb_cma_prepare_fb(&pipe->plane, plane_state);
> > +}
> > +
> > +static struct drm_simple_display_pipe_funcs fbpriv_funcs = {
> > +	.prepare_fb = intelvipfb_pipe_prepare_fb,
> > +};
> > +
> > +int intelvipfb_probe(struct device *dev, void __iomem *base)
> > +{
> > +	int retval;
> > +	struct drm_device *drm;
> > +	struct intelvipfb_priv *fbpriv = dev_get_drvdata(dev);
> > +	struct drm_connector *connector;
> > +
> > +	dev_set_drvdata(dev, fbpriv);
> > +
> > +	drm = fbpriv->drm;
> > +
> > +	intelvipfb_setup_mode_config(drm);
> > +
> > +	connector = intelvipfb_conn_setup(drm);
> > +	if (!connector) {
> > +		dev_err(drm->dev, "Connector setup failed\n");
> > +		goto err_mode_config;
> > +	}
> > +
> > +	retval = drm_simple_display_pipe_init(drm, &fbpriv->pipe,
> > +					      &fbpriv_funcs,
> > +					      fbpriv_formats,
> > +					      ARRAY_SIZE(fbpriv_fo
> > rmats),
> > +					      connector);
> > +	if (retval < 0) {
> > +		dev_err(drm->dev, "Cannot setup simple display
> > pipe\n");
> > +		goto err_mode_config;
> > +	}
> > +
> > +	fbpriv->fbcma = drm_fbdev_cma_init(drm, PREF_BPP,
> > +					   drm-
> > >mode_config.num_connector);
> > +	if (!fbpriv->fbcma) {
> > +		fbpriv->fbcma = NULL;
> > +		dev_err(drm->dev, "Failed to init FB CMA area\n");
> > +		goto err_mode_config;
> > +	}
> On failure, drm_fbdev_cma_init() returns an ERR_PTR, not NULL.
> 
> Also, you're passing this PREF_BPP value here, ignoring
> the altr,bits-per-symbol property.  That seems wrong.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i