[PATCH 0/4] drm: Add various helpers for simple drivers

[PATCH 0/4] drm: Add various helpers for simple drivers

[Noralf Trønnes]

This patchset adds various helpers that was originally part of the
tinydrm patchset.

Essentially it adds 3 functions:
- drm_fb_cma_create_with_funcs()
  CMA backed framebuffer supporting a dirty() callback.
- drm_simple_display_pipe_init()
  Plane, crtc and encoder are collapsed into one entity.
- drm_panel_connector_create()
  Create a simple connector for a panel.

Noralf Trønnes (4):
  drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  drm: Make drm_encoder_helper_funcs optional
  drm: Add helper for simple display pipeline
  drm/panel: Add helper for simple panel connector

 drivers/gpu/drm/Kconfig                 |   7 ++
 drivers/gpu/drm/Makefile                |   2 +
 drivers/gpu/drm/drm_atomic_helper.c     |  11 ++-
 drivers/gpu/drm/drm_crtc_helper.c       |  41 +++++++--
 drivers/gpu/drm/drm_fb_cma_helper.c     |  29 ++++--
 drivers/gpu/drm/drm_panel_helper.c      | 117 ++++++++++++++++++++++++
 drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/panel/Kconfig           |   7 ++
 include/drm/drm_fb_cma_helper.h         |   3 +
 include/drm/drm_panel_helper.h          |  20 ++++
 include/drm/drm_simple_kms_helper.h     |  88 ++++++++++++++++++
 11 files changed, 466 insertions(+), 16 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_panel_helper.c
 create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
 create mode 100644 include/drm/drm_panel_helper.h
 create mode 100644 include/drm/drm_simple_kms_helper.h

--
2.2.2

[PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Noralf Trønnes]

Add drm_fb_cma_create_with_funcs() for drivers that need to set the
dirty() callback.

Cc: laurent.pinchart@ideasonboard.com
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
 include/drm/drm_fb_cma_helper.h     |  3 +++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 086f600..7165209 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
 }
 
 /**
- * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
+ * drm_fb_cma_create_with_funcs() - helper function for the
+ *                                  &drm_mode_config_funcs ->fb_create
+ *                                  callback function
  *
- * If your hardware has special alignment or pitch requirements these should be
- * checked before calling this function.
+ * This can be used to set &drm_framebuffer_funcs for drivers that need the
+ * dirty() callback.
  */
-struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
-	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+	struct drm_framebuffer_funcs *funcs)
 {
 	struct drm_fb_cma *fb_cma;
 	struct drm_gem_cma_object *objs[4];
@@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 		objs[i] = to_drm_gem_cma_obj(obj);
 	}
 
-	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
+	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
 	if (IS_ERR(fb_cma)) {
 		ret = PTR_ERR(fb_cma);
 		goto err_gem_object_unreference;
@@ -217,6 +220,20 @@ err_gem_object_unreference:
 		drm_gem_object_unreference_unlocked(&objs[i]->base);
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
+
+/**
+ * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
+ *
+ * If your hardware has special alignment or pitch requirements these should be
+ * checked before calling this function.
+ */
+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
+					    &drm_fb_cma_funcs);
+}
 EXPORT_SYMBOL_GPL(drm_fb_cma_create);
 
 /**
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index c6d9c9c..1f9a8bc 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
 int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
 	struct drm_file *file_priv, unsigned int *handle);
 
+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+	struct drm_framebuffer_funcs *funcs);
 struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
 
-- 
2.2.2

[PATCH 2/4] drm: Make drm_encoder_helper_funcs optional

[Noralf Trønnes]

Make drm_encoder_helper_funcs and it's functions optional to avoid
having dummy functions.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
 drivers/gpu/drm/drm_crtc_helper.c   | 41 +++++++++++++++++++++++++++++--------
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 92e11a2..03ea049 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 			continue;
 
 		funcs = encoder->helper_private;
+		if (!funcs)
+			continue;
 
 		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 			funcs->prepare(encoder);
 		else if (funcs->disable)
 			funcs->disable(encoder);
-		else
+		else if (funcs->dpms)
 			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 
 		drm_bridge_post_disable(encoder->bridge);
@@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
+		if (!funcs)
+			continue;
+
 		new_crtc_state = connector->state->crtc->state;
 		mode = &new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
@@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
+		if (!funcs)
+			continue;
 
 		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		if (funcs->enable)
 			funcs->enable(encoder);
-		else
+		else if (funcs->commit)
 			funcs->commit(encoder);
 
 		drm_bridge_enable(encoder->bridge);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 66ca313..6e6ab38 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 
+	if (!encoder_funcs)
+		return;
+
 	drm_bridge_disable(encoder->bridge);
 
 	if (encoder_funcs->disable)
 		(*encoder_funcs->disable)(encoder);
-	else
+	else if (encoder_funcs->dpms)
 		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
 
 	drm_bridge_post_disable(encoder->bridge);
@@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
 
 	drm_for_each_encoder(encoder, dev) {
 		encoder_funcs = encoder->helper_private;
+		if (!encoder_funcs)
+			continue;
+
 		/* Disable unused encoders */
 		if (encoder->crtc == NULL)
 			drm_encoder_disable(encoder);
@@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		if (encoder->crtc != crtc)
 			continue;
 
+		encoder_funcs = encoder->helper_private;
+		if (!encoder_funcs)
+			continue;
+
 		ret = drm_bridge_mode_fixup(encoder->bridge,
 			mode, adjusted_mode);
 		if (!ret) {
@@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		if (encoder->crtc != crtc)
 			continue;
 
+		encoder_funcs = encoder->helper_private;
+		if (!encoder_funcs)
+			continue;
+
 		drm_bridge_disable(encoder->bridge);
 
-		encoder_funcs = encoder->helper_private;
 		/* Disable the encoders as the first thing we do. */
-		encoder_funcs->prepare(encoder);
+		if (encoder_funcs->prepare)
+			encoder_funcs->prepare(encoder);
 
 		drm_bridge_post_disable(encoder->bridge);
 	}
@@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		if (encoder->crtc != crtc)
 			continue;
 
+		encoder_funcs = encoder->helper_private;
+		if (!encoder_funcs)
+			continue;
+
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
 			encoder->base.id, encoder->name,
 			mode->base.id, mode->name);
-		encoder_funcs = encoder->helper_private;
-		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
+		if (encoder_funcs->mode_set)
+			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
 
 		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
 	}
@@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		if (encoder->crtc != crtc)
 			continue;
 
+		encoder_funcs = encoder->helper_private;
+		if (!encoder_funcs)
+			continue;
+
 		drm_bridge_pre_enable(encoder->bridge);
 
-		encoder_funcs = encoder->helper_private;
-		encoder_funcs->commit(encoder);
+		if (encoder_funcs->commit)
+			encoder_funcs->commit(encoder);
 
 		drm_bridge_enable(encoder->bridge);
 	}
@@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
 	struct drm_bridge *bridge = encoder->bridge;
 	const struct drm_encoder_helper_funcs *encoder_funcs;
 
+	encoder_funcs = encoder->helper_private;
+	if (!encoder_funcs)
+		return;
+
 	if (mode == DRM_MODE_DPMS_ON)
 		drm_bridge_pre_enable(bridge);
 	else
 		drm_bridge_disable(bridge);
 
-	encoder_funcs = encoder->helper_private;
 	if (encoder_funcs->dpms)
 		encoder_funcs->dpms(encoder, mode);
 
-- 
2.2.2

[PATCH 3/4] drm: Add helper for simple display pipeline

[Noralf Trønnes]

Provides helper functions for drivers that have a simple display
pipeline. Plane, crtc and encoder are collapsed into one entity.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Kconfig                 |   7 ++
 drivers/gpu/drm/Makefile                |   1 +
 drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++
 include/drm/drm_simple_kms_helper.h     |  88 ++++++++++++++++++
 4 files changed, 253 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
 create mode 100644 include/drm/drm_simple_kms_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 16e4c21..ff9f480 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -39,6 +39,13 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_SIMPLE_KMS_HELPER
+	tristate
+	depends on DRM
+	select DRM_KMS_HELPER
+	help
+	  Helpers for very simple KMS drivers.
+
 config DRM_KMS_FB_HELPER
 	bool
 	depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 43c2abf..7e99923 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
+obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o
 
 CFLAGS_drm_trace_points.o := -I$(src)
 
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
new file mode 100644
index 0000000..c8725bb
--- /dev/null
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -0,0 +1,157 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <linux/slab.h>
+
+static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->enable)
+		return;
+
+	pipe->funcs->enable(pipe, crtc->state);
+}
+
+static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->disable)
+		return;
+
+	pipe->funcs->disable(pipe);
+}
+
+static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
+	.disable = drm_simple_kms_crtc_disable,
+	.enable = drm_simple_kms_crtc_enable,
+};
+
+static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
+					struct drm_plane_state *pstate)
+{
+	struct drm_simple_display_pipe *pipe;
+	struct drm_crtc_state *cstate;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->check)
+		return 0;
+
+	cstate = drm_atomic_get_existing_crtc_state(pstate->state,
+						    &pipe->crtc);
+
+	return pipe->funcs->check(pipe, pstate, cstate);
+}
+
+static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
+					struct drm_plane_state *pstate)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->update)
+		return;
+
+	pipe->funcs->update(pipe, pstate);
+}
+
+static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
+	.atomic_check = drm_simple_kms_plane_atomic_check,
+	.atomic_update = drm_simple_kms_plane_atomic_update,
+};
+
+static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
+	.update_plane		= drm_atomic_helper_update_plane,
+	.disable_plane		= drm_atomic_helper_disable_plane,
+	.destroy		= drm_plane_cleanup,
+	.reset			= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+};
+
+/**
+ * drm_simple_display_pipe_init - Initialize a simple display pipeline
+ * @dev: DRM device
+ * @pipe: simple display pipe object to initialize
+ * @funcs: callbacks for the display pipe
+ * @formats: array of supported formats (%DRM_FORMAT_*)
+ * @format_count: number of elements in @formats
+ * @connector: connector to attach and register
+ *
+ * Sets up a display pipeline which consist of a really simple
+ * plane-crtc-encoder pipe coupled with the provided connector.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_simple_display_pipe_init(struct drm_device *dev,
+				 struct drm_simple_display_pipe *pipe,
+				 struct drm_simple_display_pipe_funcs *funcs,
+				 const uint32_t *formats,
+				 unsigned int format_count,
+				 struct drm_connector *connector)
+{
+	struct drm_encoder *encoder = &pipe->encoder;
+	struct drm_plane *plane = &pipe->plane;
+	struct drm_crtc *crtc = &pipe->crtc;
+	int ret;
+
+	pipe->funcs = funcs;
+
+	drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs);
+	ret = drm_universal_plane_init(dev, plane, 0,
+				       &drm_simple_kms_plane_funcs,
+				       formats, format_count,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+
+	drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs);
+	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
+					&drm_simple_kms_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+
+	encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+
+	ret = drm_mode_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return drm_connector_register(connector);
+}
+EXPORT_SYMBOL(drm_simple_display_pipe_init);
+
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
new file mode 100644
index 0000000..69eac94
--- /dev/null
+++ b/include/drm/drm_simple_kms_helper.h
@@ -0,0 +1,88 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_DRM_SIMPLE_KMS_HELPER_H
+#define __LINUX_DRM_SIMPLE_KMS_HELPER_H
+
+struct drm_simple_display_pipe;
+
+/**
+ * struct drm_simple_display_pipe_funcs - helper operations for a simple
+ *                                        display pipeline
+ */
+struct drm_simple_display_pipe_funcs {
+	/**
+	 * @enable:
+	 *
+	 * This function should be used to enable the pipeline.
+	 * It is called when the underlying crtc is enabled.
+	 */
+	void (*enable)(struct drm_simple_display_pipe *pipe,
+		       struct drm_crtc_state *crtc_state);
+	/**
+	 * @disable:
+	 *
+	 * This function should be used to disable the pipeline.
+	 * It is called when the underlying crtc is disabled.
+	 */
+	void (*disable)(struct drm_simple_display_pipe *pipe);
+
+	/**
+	 * @check:
+	 *
+	 * This function is called in the check phase of an atomic update,
+	 * specifically when the underlying plane is checked.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success, -EINVAL if the state or the transition can't be
+	 * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
+	 * attempt to obtain another state object ran into a &drm_modeset_lock
+	 * deadlock.
+	 */
+	int (*check)(struct drm_simple_display_pipe *pipe,
+		     struct drm_plane_state *plane_state,
+		     struct drm_crtc_state *crtc_state);
+	/**
+	 * @update:
+	 *
+	 * This function is called when the underlying plane state is updated.
+	 */
+	void (*update)(struct drm_simple_display_pipe *pipe,
+		       struct drm_plane_state *plane_state);
+};
+
+/**
+ * struct drm_simple_display_pipe - simple display pipeline
+ * @crtc: CRTC control structure
+ * @plane: Plane control structure
+ * @encoder: Encoder control structure
+ * @connector: Connector control structure
+ * @funcs: Pipeline control functions (optional)
+ *
+ * Simple display pipeline with plane, crtc and encoder collapsed into one
+ * entity.
+ */
+struct drm_simple_display_pipe {
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+	struct drm_encoder encoder;
+	struct drm_connector *connector;
+
+	struct drm_simple_display_pipe_funcs *funcs;
+};
+
+int drm_simple_display_pipe_init(struct drm_device *dev,
+				 struct drm_simple_display_pipe *pipe,
+				 struct drm_simple_display_pipe_funcs *funcs,
+				 const uint32_t *formats,
+				 unsigned int format_count,
+				 struct drm_connector *connector);
+
+#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
-- 
2.2.2

[PATCH 4/4] drm/panel: Add helper for simple panel connector

[Noralf Trønnes]

Add function to create a simple connector for a panel.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Makefile           |   1 +
 drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/panel/Kconfig      |   7 +++
 include/drm/drm_panel_helper.h     |  20 +++++++
 4 files changed, 145 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_panel_helper.c
 create mode 100644 include/drm/drm_panel_helper.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 7e99923..3f3696c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
+drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 
diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c
new file mode 100644
index 0000000..5d8b623
--- /dev/null
+++ b/drivers/gpu/drm/drm_panel_helper.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_panel.h>
+
+struct drm_panel_connector {
+	struct drm_connector base;
+	struct drm_panel *panel;
+};
+
+static inline struct drm_panel_connector *
+to_drm_panel_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct drm_panel_connector, base);
+}
+
+static int drm_panel_connector_get_modes(struct drm_connector *connector)
+{
+	return drm_panel_get_modes(to_drm_panel_connector(connector)->panel);
+}
+
+static struct drm_encoder *
+drm_panel_connector_best_encoder(struct drm_connector *connector)
+{
+	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
+}
+
+static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = {
+	.get_modes = drm_panel_connector_get_modes,
+	.best_encoder = drm_panel_connector_best_encoder,
+};
+
+static enum drm_connector_status
+drm_panel_connector_detect(struct drm_connector *connector, bool force)
+{
+	if (drm_device_is_unplugged(connector->dev))
+		return connector_status_disconnected;
+
+	return connector->status;
+}
+
+static void drm_panel_connector_destroy(struct drm_connector *connector)
+{
+	struct drm_panel_connector *panel_connector;
+
+	panel_connector = to_drm_panel_connector(connector);
+	drm_panel_detach(panel_connector->panel);
+	drm_panel_remove(panel_connector->panel);
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+	kfree(panel_connector);
+}
+
+static const struct drm_connector_funcs drm_panel_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = drm_panel_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_panel_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+/**
+ * drm_panel_connector_create - Create simple connector for panel
+ * @dev: DRM device
+ * @panel: DRM panel
+ * @connector_type: user visible type of the connector
+ *
+ * Creates a simple connector for a panel.
+ * The panel needs to provide a get_modes() function.
+ *
+ * Returns:
+ * Pointer to new connector or ERR_PTR on failure.
+ */
+struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
+						 struct drm_panel *panel,
+						 int connector_type)
+{
+	struct drm_panel_connector *panel_connector;
+	struct drm_connector *connector;
+	int ret;
+
+	panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL);
+	if (!panel_connector)
+		return ERR_PTR(-ENOMEM);
+
+	panel_connector->panel = panel;
+	connector = &panel_connector->base;
+	drm_connector_helper_add(connector, &drm_panel_connector_hfuncs);
+	ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs,
+				 connector_type);
+	if (ret) {
+		kfree(panel_connector);
+		return ERR_PTR(ret);
+	}
+
+	connector->status = connector_status_connected;
+	drm_panel_init(panel);
+	drm_panel_add(panel);
+	drm_panel_attach(panel, connector);
+
+	return connector;
+}
+EXPORT_SYMBOL(drm_panel_connector_create);
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 1500ab9..87264a3 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -4,6 +4,13 @@ config DRM_PANEL
 	help
 	  Panel registration and lookup framework.
 
+config DRM_PANEL_HELPER
+	bool
+	depends on DRM
+	select DRM_PANEL
+	help
+	  Panel helper.
+
 menu "Display Panels"
 	depends on DRM && DRM_PANEL
 
diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h
new file mode 100644
index 0000000..c3355e3
--- /dev/null
+++ b/include/drm/drm_panel_helper.h
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __DRM_PANEL_HELPER_H__
+#define __DRM_PANEL_HELPER_H__
+
+struct drm_device;
+struct drm_panel;
+
+struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
+						 struct drm_panel *panel,
+						 int connector_type);
+
+#endif
-- 
2.2.2

Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
> Make drm_encoder_helper_funcs and it's functions optional to avoid
> having dummy functions.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Please also update the kerneldoc and mention there that the enable/disable
hooks are optional. You can build the hmtl docs using

$ make htmldocs

to check the result. The vtables are documented in
include/drm/drm_helper_vtables.h

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
>  drivers/gpu/drm/drm_crtc_helper.c   | 41 +++++++++++++++++++++++++++++--------

Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
part. Old drivers should converted to atomic, new drivers should just use
atomic. No need to touch that code at all.
-Daniel

>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 92e11a2..03ea049 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  			continue;
>  
>  		funcs = encoder->helper_private;
> +		if (!funcs)
> +			continue;
>  
>  		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  			funcs->prepare(encoder);
>  		else if (funcs->disable)
>  			funcs->disable(encoder);
> -		else
> +		else if (funcs->dpms)
>  			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>  
>  		drm_bridge_post_disable(encoder->bridge);
> @@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> +		if (!funcs)
> +			continue;
> +
>  		new_crtc_state = connector->state->crtc->state;
>  		mode = &new_crtc_state->mode;
>  		adjusted_mode = &new_crtc_state->adjusted_mode;
> @@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> +		if (!funcs)
> +			continue;
>  
>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  
>  		if (funcs->enable)
>  			funcs->enable(encoder);
> -		else
> +		else if (funcs->commit)
>  			funcs->commit(encoder);
>  
>  		drm_bridge_enable(encoder->bridge);
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 66ca313..6e6ab38 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
>  {
>  	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
>  
> +	if (!encoder_funcs)
> +		return;
> +
>  	drm_bridge_disable(encoder->bridge);
>  
>  	if (encoder_funcs->disable)
>  		(*encoder_funcs->disable)(encoder);
> -	else
> +	else if (encoder_funcs->dpms)
>  		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
>  
>  	drm_bridge_post_disable(encoder->bridge);
> @@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
>  
>  	drm_for_each_encoder(encoder, dev) {
>  		encoder_funcs = encoder->helper_private;
> +		if (!encoder_funcs)
> +			continue;
> +
>  		/* Disable unused encoders */
>  		if (encoder->crtc == NULL)
>  			drm_encoder_disable(encoder);
> @@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> +		encoder_funcs = encoder->helper_private;
> +		if (!encoder_funcs)
> +			continue;
> +
>  		ret = drm_bridge_mode_fixup(encoder->bridge,
>  			mode, adjusted_mode);
>  		if (!ret) {
> @@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> +		encoder_funcs = encoder->helper_private;
> +		if (!encoder_funcs)
> +			continue;
> +
>  		drm_bridge_disable(encoder->bridge);
>  
> -		encoder_funcs = encoder->helper_private;
>  		/* Disable the encoders as the first thing we do. */
> -		encoder_funcs->prepare(encoder);
> +		if (encoder_funcs->prepare)
> +			encoder_funcs->prepare(encoder);
>  
>  		drm_bridge_post_disable(encoder->bridge);
>  	}
> @@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> +		encoder_funcs = encoder->helper_private;
> +		if (!encoder_funcs)
> +			continue;
> +
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
>  			encoder->base.id, encoder->name,
>  			mode->base.id, mode->name);
> -		encoder_funcs = encoder->helper_private;
> -		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> +		if (encoder_funcs->mode_set)
> +			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
> @@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> +		encoder_funcs = encoder->helper_private;
> +		if (!encoder_funcs)
> +			continue;
> +
>  		drm_bridge_pre_enable(encoder->bridge);
>  
> -		encoder_funcs = encoder->helper_private;
> -		encoder_funcs->commit(encoder);
> +		if (encoder_funcs->commit)
> +			encoder_funcs->commit(encoder);
>  
>  		drm_bridge_enable(encoder->bridge);
>  	}
> @@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
>  	struct drm_bridge *bridge = encoder->bridge;
>  	const struct drm_encoder_helper_funcs *encoder_funcs;
>  
> +	encoder_funcs = encoder->helper_private;
> +	if (!encoder_funcs)
> +		return;
> +
>  	if (mode == DRM_MODE_DPMS_ON)
>  		drm_bridge_pre_enable(bridge);
>  	else
>  		drm_bridge_disable(bridge);
>  
> -	encoder_funcs = encoder->helper_private;
>  	if (encoder_funcs->dpms)
>  		encoder_funcs->dpms(encoder, mode);
>  
> -- 
> 2.2.2
> 

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

Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
> Add drm_fb_cma_create_with_funcs() for drivers that need to set the
> dirty() callback.
> 
> Cc: laurent.pinchart@ideasonboard.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
>  include/drm/drm_fb_cma_helper.h     |  3 +++
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 086f600..7165209 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>  }
>  
>  /**
> - * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
> + * drm_fb_cma_create_with_funcs() - helper function for the
> + *                                  &drm_mode_config_funcs ->fb_create
> + *                                  callback function
>   *
> - * If your hardware has special alignment or pitch requirements these should be
> - * checked before calling this function.
> + * This can be used to set &drm_framebuffer_funcs for drivers that need the
> + * dirty() callback.

Please reference the other function in your kerneldoc using
drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
of functions it's always good to cross link them and explain exactly when
another one is more appropriate. E.g. here "If your driver does not need a
custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."

Similar, but other way round for the existing one.

Again please check with make htmldocs that it all looks good.

Otherwise lgtm.
-Daniel

>   */
> -struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> -	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> +	struct drm_framebuffer_funcs *funcs)
>  {
>  	struct drm_fb_cma *fb_cma;
>  	struct drm_gem_cma_object *objs[4];
> @@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>  		objs[i] = to_drm_gem_cma_obj(obj);
>  	}
>  
> -	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
> +	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
>  	if (IS_ERR(fb_cma)) {
>  		ret = PTR_ERR(fb_cma);
>  		goto err_gem_object_unreference;
> @@ -217,6 +220,20 @@ err_gem_object_unreference:
>  		drm_gem_object_unreference_unlocked(&objs[i]->base);
>  	return ERR_PTR(ret);
>  }
> +EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
> +
> +/**
> + * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
> + *
> + * If your hardware has special alignment or pitch requirements these should be
> + * checked before calling this function.
> + */
> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
> +					    &drm_fb_cma_funcs);
> +}
>  EXPORT_SYMBOL_GPL(drm_fb_cma_create);
>  
>  /**
> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> index c6d9c9c..1f9a8bc 100644
> --- a/include/drm/drm_fb_cma_helper.h
> +++ b/include/drm/drm_fb_cma_helper.h
> @@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
>  int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
>  	struct drm_file *file_priv, unsigned int *handle);
>  
> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> +	struct drm_framebuffer_funcs *funcs);
>  struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>  	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
>  
> -- 
> 2.2.2
> 

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

Re: [PATCH 3/4] drm: Add helper for simple display pipeline

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:33PM +0200, Noralf Trønnes wrote:
> Provides helper functions for drivers that have a simple display
> pipeline. Plane, crtc and encoder are collapsed into one entity.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

When resubmitting a patch please always have a per-patch changelog of what
you've done. Otherwise you force reviewers to remember they looked at this
already, dig out the old mail and compare ;-)

> ---
>  drivers/gpu/drm/Kconfig                 |   7 ++
>  drivers/gpu/drm/Makefile                |   1 +
>  drivers/gpu/drm/drm_simple_kms_helper.c | 157 ++++++++++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     |  88 ++++++++++++++++++

Please also add a new section (next to all the ones for the other kms
helpers) in Documentation/DocBook/gpu.tmpl and pull your kerneldoc in.
Please also add a short overview kernel doc section to the .c file (Using
the DOC: header) and also pull that into the overall gpu docs.

You can check the results using

$ make htmldocs

>  4 files changed, 253 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>  create mode 100644 include/drm/drm_simple_kms_helper.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 16e4c21..ff9f480 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -39,6 +39,13 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers.
>  
> +config DRM_SIMPLE_KMS_HELPER
> +	tristate
> +	depends on DRM
> +	select DRM_KMS_HELPER
> +	help
> +	  Helpers for very simple KMS drivers.
> +
>  config DRM_KMS_FB_HELPER
>  	bool
>  	depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 43c2abf..7e99923 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> +obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o
>  
>  CFLAGS_drm_trace_points.o := -I$(src)
>  
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> new file mode 100644
> index 0000000..c8725bb
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +#include <linux/slab.h>
> +
> +static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->enable)
> +		return;
> +
> +	pipe->funcs->enable(pipe, crtc->state);
> +}
> +
> +static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->disable)
> +		return;
> +
> +	pipe->funcs->disable(pipe);
> +}
> +
> +static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> +	.disable = drm_simple_kms_crtc_disable,
> +	.enable = drm_simple_kms_crtc_enable,
> +};
> +
> +static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = {
> +	.reset = drm_atomic_helper_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
> +
> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> +					struct drm_plane_state *pstate)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +	struct drm_crtc_state *cstate;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	if (!pipe->funcs || !pipe->funcs->check)
> +		return 0;
> +
> +	cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> +						    &pipe->crtc);
> +
> +	return pipe->funcs->check(pipe, pstate, cstate);
> +}
> +
> +static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
> +					struct drm_plane_state *pstate)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	if (!pipe->funcs || !pipe->funcs->update)
> +		return;
> +
> +	pipe->funcs->update(pipe, pstate);
> +}
> +
> +static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
> +	.atomic_check = drm_simple_kms_plane_atomic_check,
> +	.atomic_update = drm_simple_kms_plane_atomic_update,
> +};
> +
> +static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
> +	.update_plane		= drm_atomic_helper_update_plane,
> +	.disable_plane		= drm_atomic_helper_disable_plane,
> +	.destroy		= drm_plane_cleanup,
> +	.reset			= drm_atomic_helper_plane_reset,
> +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> +};
> +
> +/**
> + * drm_simple_display_pipe_init - Initialize a simple display pipeline
> + * @dev: DRM device
> + * @pipe: simple display pipe object to initialize
> + * @funcs: callbacks for the display pipe
> + * @formats: array of supported formats (%DRM_FORMAT_*)
> + * @format_count: number of elements in @formats
> + * @connector: connector to attach and register
> + *
> + * Sets up a display pipeline which consist of a really simple
> + * plane-crtc-encoder pipe coupled with the provided connector.

need to specify that @funcs is optional.

> + *
> + * Returns:
> + * Zero on success, error code on failure.

"_negative_ error code"
> + */
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +				 struct drm_simple_display_pipe *pipe,
> +				 struct drm_simple_display_pipe_funcs *funcs,
> +				 const uint32_t *formats,
> +				 unsigned int format_count,
> +				 struct drm_connector *connector)
> +{
> +	struct drm_encoder *encoder = &pipe->encoder;
> +	struct drm_plane *plane = &pipe->plane;
> +	struct drm_crtc *crtc = &pipe->crtc;
> +	int ret;
> +
> +	pipe->funcs = funcs;
> +
> +	drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs);
> +	ret = drm_universal_plane_init(dev, plane, 0,
> +				       &drm_simple_kms_plane_funcs,
> +				       formats, format_count,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		return ret;
> +
> +	drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs);
> +	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
> +					&drm_simple_kms_crtc_funcs, NULL);
> +	if (ret)
> +		return ret;
> +
> +	encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
> +	ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_mode_connector_attach_encoder(connector, encoder);
> +	if (ret)
> +		return ret;
> +
> +	return drm_connector_register(connector);
> +}
> +EXPORT_SYMBOL(drm_simple_display_pipe_init);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> new file mode 100644
> index 0000000..69eac94
> --- /dev/null
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __LINUX_DRM_SIMPLE_KMS_HELPER_H
> +#define __LINUX_DRM_SIMPLE_KMS_HELPER_H
> +
> +struct drm_simple_display_pipe;
> +
> +/**
> + * struct drm_simple_display_pipe_funcs - helper operations for a simple
> + *                                        display pipeline
> + */
> +struct drm_simple_display_pipe_funcs {
> +	/**
> +	 * @enable:
> +	 *
> +	 * This function should be used to enable the pipeline.
> +	 * It is called when the underlying crtc is enabled.

Please add boilerplate to all these hooks along the lines of:

"This hook is optional."

Besides that little bit of polish looks all good to me.
-Daniel

> +	 */
> +	void (*enable)(struct drm_simple_display_pipe *pipe,
> +		       struct drm_crtc_state *crtc_state);
> +	/**
> +	 * @disable:
> +	 *
> +	 * This function should be used to disable the pipeline.
> +	 * It is called when the underlying crtc is disabled.
> +	 */
> +	void (*disable)(struct drm_simple_display_pipe *pipe);
> +
> +	/**
> +	 * @check:
> +	 *
> +	 * This function is called in the check phase of an atomic update,
> +	 * specifically when the underlying plane is checked.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success, -EINVAL if the state or the transition can't be
> +	 * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
> +	 * attempt to obtain another state object ran into a &drm_modeset_lock
> +	 * deadlock.
> +	 */
> +	int (*check)(struct drm_simple_display_pipe *pipe,
> +		     struct drm_plane_state *plane_state,
> +		     struct drm_crtc_state *crtc_state);
> +	/**
> +	 * @update:
> +	 *
> +	 * This function is called when the underlying plane state is updated.
> +	 */
> +	void (*update)(struct drm_simple_display_pipe *pipe,
> +		       struct drm_plane_state *plane_state);
> +};
> +
> +/**
> + * struct drm_simple_display_pipe - simple display pipeline
> + * @crtc: CRTC control structure
> + * @plane: Plane control structure
> + * @encoder: Encoder control structure
> + * @connector: Connector control structure
> + * @funcs: Pipeline control functions (optional)
> + *
> + * Simple display pipeline with plane, crtc and encoder collapsed into one
> + * entity.
> + */
> +struct drm_simple_display_pipe {
> +	struct drm_crtc crtc;
> +	struct drm_plane plane;
> +	struct drm_encoder encoder;
> +	struct drm_connector *connector;
> +
> +	struct drm_simple_display_pipe_funcs *funcs;
> +};
> +
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +				 struct drm_simple_display_pipe *pipe,
> +				 struct drm_simple_display_pipe_funcs *funcs,
> +				 const uint32_t *formats,
> +				 unsigned int format_count,
> +				 struct drm_connector *connector);
> +
> +#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> -- 
> 2.2.2
> 

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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> Add function to create a simple connector for a panel.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Like in the previous patch please also add a new section for the panel
helpers to gpu.tmpl. I don't think this needs an overview section, it's so
simple. But adding some cross references from the drm_panel.c kerneldoc to
this and back would be real good.

> ---
>  drivers/gpu/drm/Makefile           |   1 +
>  drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/panel/Kconfig      |   7 +++
>  include/drm/drm_panel_helper.h     |  20 +++++++
>  4 files changed, 145 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_panel_helper.c
>  create mode 100644 include/drm/drm_panel_helper.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 7e99923..3f3696c 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>  drm-$(CONFIG_PCI) += ati_pcigart.o
>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  
> diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c
> new file mode 100644
> index 0000000..5d8b623
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_helper.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_panel.h>
> +
> +struct drm_panel_connector {
> +	struct drm_connector base;
> +	struct drm_panel *panel;
> +};
> +
> +static inline struct drm_panel_connector *
> +to_drm_panel_connector(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct drm_panel_connector, base);
> +}
> +
> +static int drm_panel_connector_get_modes(struct drm_connector *connector)
> +{
> +	return drm_panel_get_modes(to_drm_panel_connector(connector)->panel);
> +}
> +
> +static struct drm_encoder *
> +drm_panel_connector_best_encoder(struct drm_connector *connector)
> +{
> +	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
> +}

As mentioned in my previous review, this function would be extremely
useful to rid tons of drivers of boilerplate - most drm_connector only
support exactly 1 encoder, statically determined at driver init time.

Please put this helper into the atomic helper library, and maybe call it
something like

drm_atomic_helper_best_encoder.

To make sure it's never abused by accident please also add a 

	WARN_ON(connector->encoder_ids[1]);

to make sure that there's really only just one encoder for this connector.

If you're bored you can also go through drivers and use that everywhere it
fits, it would result in a _lot_ of deleted code. But also needs quite a
bit of audit work ...

Otherwise lgtm, but please ask Thierry for an ack as the panel maintainer.
-Daniel


> +
> +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = {
> +	.get_modes = drm_panel_connector_get_modes,
> +	.best_encoder = drm_panel_connector_best_encoder,
> +};
> +
> +static enum drm_connector_status
> +drm_panel_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	if (drm_device_is_unplugged(connector->dev))
> +		return connector_status_disconnected;
> +
> +	return connector->status;
> +}
> +
> +static void drm_panel_connector_destroy(struct drm_connector *connector)
> +{
> +	struct drm_panel_connector *panel_connector;
> +
> +	panel_connector = to_drm_panel_connector(connector);
> +	drm_panel_detach(panel_connector->panel);
> +	drm_panel_remove(panel_connector->panel);
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +	kfree(panel_connector);
> +}
> +
> +static const struct drm_connector_funcs drm_panel_connector_funcs = {
> +	.dpms = drm_atomic_helper_connector_dpms,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.detect = drm_panel_connector_detect,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_panel_connector_destroy,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +/**
> + * drm_panel_connector_create - Create simple connector for panel
> + * @dev: DRM device
> + * @panel: DRM panel
> + * @connector_type: user visible type of the connector
> + *
> + * Creates a simple connector for a panel.
> + * The panel needs to provide a get_modes() function.
> + *
> + * Returns:
> + * Pointer to new connector or ERR_PTR on failure.
> + */
> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
> +						 struct drm_panel *panel,
> +						 int connector_type)
> +{
> +	struct drm_panel_connector *panel_connector;
> +	struct drm_connector *connector;
> +	int ret;
> +
> +	panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL);
> +	if (!panel_connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	panel_connector->panel = panel;
> +	connector = &panel_connector->base;
> +	drm_connector_helper_add(connector, &drm_panel_connector_hfuncs);
> +	ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs,
> +				 connector_type);
> +	if (ret) {
> +		kfree(panel_connector);
> +		return ERR_PTR(ret);
> +	}
> +
> +	connector->status = connector_status_connected;
> +	drm_panel_init(panel);
> +	drm_panel_add(panel);
> +	drm_panel_attach(panel, connector);
> +
> +	return connector;
> +}
> +EXPORT_SYMBOL(drm_panel_connector_create);
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 1500ab9..87264a3 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -4,6 +4,13 @@ config DRM_PANEL
>  	help
>  	  Panel registration and lookup framework.
>  
> +config DRM_PANEL_HELPER
> +	bool
> +	depends on DRM
> +	select DRM_PANEL
> +	help
> +	  Panel helper.
> +
>  menu "Display Panels"
>  	depends on DRM && DRM_PANEL
>  
> diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h
> new file mode 100644
> index 0000000..c3355e3
> --- /dev/null
> +++ b/include/drm/drm_panel_helper.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2016 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __DRM_PANEL_HELPER_H__
> +#define __DRM_PANEL_HELPER_H__
> +
> +struct drm_device;
> +struct drm_panel;
> +
> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
> +						 struct drm_panel *panel,
> +						 int connector_type);
> +
> +#endif
> -- 
> 2.2.2
> 

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

Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Noralf Trønnes]

Den 05.05.2016 18:27, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
>> Add drm_fb_cma_create_with_funcs() for drivers that need to set the
>> dirty() callback.
>>
>> Cc: laurent.pinchart@ideasonboard.com
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
>>   include/drm/drm_fb_cma_helper.h     |  3 +++
>>   2 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index 086f600..7165209 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>>   }
>>   
>>   /**
>> - * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
>> + * drm_fb_cma_create_with_funcs() - helper function for the
>> + *                                  &drm_mode_config_funcs ->fb_create
>> + *                                  callback function
>>    *
>> - * If your hardware has special alignment or pitch requirements these should be
>> - * checked before calling this function.
>> + * This can be used to set &drm_framebuffer_funcs for drivers that need the
>> + * dirty() callback.
> Please reference the other function in your kerneldoc using
> drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
> of functions it's always good to cross link them and explain exactly when
> another one is more appropriate. E.g. here "If your driver does not need a
> custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."
>
> Similar, but other way round for the existing one.
>
> Again please check with make htmldocs that it all looks good.

Ok, I didn't understand this htmldocs stuff, I thought it picked up the docs
by magic or something.
It turns out that drm_fb_cma_helper isn't mentioned in gpu.tmpl so I have
to make a patch for that.
Is there an order to things where I should put it in gpu.tmpl?
(drm_simple_kms_helper also)

This is the current order:

Mode Setting Helper Functions

Atomic Modeset Helper Functions Reference
Modeset Helper Reference for Common Vtables
Legacy CRTC/Modeset Helper Functions Reference
Output Probing Helper Functions Reference
fbdev Helper Functions Reference
Display Port Helper Functions Reference
Display Port MST Helper Functions Reference
MIPI DSI Helper Functions Reference
EDID Helper Functions Reference
Rectangle Utilities Reference
Flip-work Helper Reference
HDMI Infoframes Helper Reference
Plane Helper Reference
Tile group
Bridges


And the code example I put in drm_fb_cma_helper DOC: looks
terrible, maybe it looks better in the intel augmented version?

Noralf.

> Otherwise lgtm.
> -Daniel
>
>>    */
>> -struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> -	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
>> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
>> +	struct drm_framebuffer_funcs *funcs)
>>   {
>>   	struct drm_fb_cma *fb_cma;
>>   	struct drm_gem_cma_object *objs[4];
>> @@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>   		objs[i] = to_drm_gem_cma_obj(obj);
>>   	}
>>   
>> -	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
>> +	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
>>   	if (IS_ERR(fb_cma)) {
>>   		ret = PTR_ERR(fb_cma);
>>   		goto err_gem_object_unreference;
>> @@ -217,6 +220,20 @@ err_gem_object_unreference:
>>   		drm_gem_object_unreference_unlocked(&objs[i]->base);
>>   	return ERR_PTR(ret);
>>   }
>> +EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
>> +
>> +/**
>> + * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
>> + *
>> + * If your hardware has special alignment or pitch requirements these should be
>> + * checked before calling this function.
>> + */
>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
>> +{
>> +	return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
>> +					    &drm_fb_cma_funcs);
>> +}
>>   EXPORT_SYMBOL_GPL(drm_fb_cma_create);
>>   
>>   /**
>> diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
>> index c6d9c9c..1f9a8bc 100644
>> --- a/include/drm/drm_fb_cma_helper.h
>> +++ b/include/drm/drm_fb_cma_helper.h
>> @@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
>>   int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
>>   	struct drm_file *file_priv, unsigned int *handle);
>>   
>> +struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
>> +	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
>> +	struct drm_framebuffer_funcs *funcs);
>>   struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>   	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
>>   
>> -- 
>> 2.2.2
>>

Re: [PATCH 1/4] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Daniel Vetter]

On Fri, May 06, 2016 at 03:01:37PM +0200, Noralf Trønnes wrote:
> 
> Den 05.05.2016 18:27, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:31PM +0200, Noralf Trønnes wrote:
> >>Add drm_fb_cma_create_with_funcs() for drivers that need to set the
> >>dirty() callback.
> >>
> >>Cc: laurent.pinchart@ideasonboard.com
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>---
> >>  drivers/gpu/drm/drm_fb_cma_helper.c | 29 +++++++++++++++++++++++------
> >>  include/drm/drm_fb_cma_helper.h     |  3 +++
> >>  2 files changed, 26 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> >>index 086f600..7165209 100644
> >>--- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >>+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >>@@ -161,13 +161,16 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
> >>  }
> >>  /**
> >>- * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create callback function
> >>+ * drm_fb_cma_create_with_funcs() - helper function for the
> >>+ *                                  &drm_mode_config_funcs ->fb_create
> >>+ *                                  callback function
> >>   *
> >>- * If your hardware has special alignment or pitch requirements these should be
> >>- * checked before calling this function.
> >>+ * This can be used to set &drm_framebuffer_funcs for drivers that need the
> >>+ * dirty() callback.
> >Please reference the other function in your kerneldoc using
> >drm_fb_cma_create() syntax. That will create a hyperlink. With such sets
> >of functions it's always good to cross link them and explain exactly when
> >another one is more appropriate. E.g. here "If your driver does not need a
> >custom &drm_framebuffer_funcs then just use drm_fb_cma_create() directly."
> >
> >Similar, but other way round for the existing one.
> >
> >Again please check with make htmldocs that it all looks good.
> 
> Ok, I didn't understand this htmldocs stuff, I thought it picked up the docs
> by magic or something.
> It turns out that drm_fb_cma_helper isn't mentioned in gpu.tmpl so I have
> to make a patch for that.

Oh right, cma fb helpers aren't pulled int at all. Would be great if you
can fix that.

> Is there an order to things where I should put it in gpu.tmpl?
> (drm_simple_kms_helper also)
> 
> This is the current order:
> 
> Mode Setting Helper Functions
> 
> Atomic Modeset Helper Functions Reference
> Modeset Helper Reference for Common Vtables
> Legacy CRTC/Modeset Helper Functions Reference
> Output Probing Helper Functions Reference
> fbdev Helper Functions Reference
> Display Port Helper Functions Reference
> Display Port MST Helper Functions Reference
> MIPI DSI Helper Functions Reference
> EDID Helper Functions Reference
> Rectangle Utilities Reference
> Flip-work Helper Reference
> HDMI Infoframes Helper Reference
> Plane Helper Reference
> Tile group
> Bridges

Just add it somewhere to this list of helpers where you think it fits.
We're not that structured yet.

> And the code example I put in drm_fb_cma_helper DOC: looks
> terrible, maybe it looks better in the intel augmented version?

Yeah, in upstream it's terrible. But if you base your patch on top of
drm-intel-nightly, or pull topic/kerneldoc in, and install asciidoc, then
it should be fairly pretty. Note: That asciidoc support is pretty hacked
up, it takes 10-20 minutes to build the gpu docs with that. We're working
on something much better.
-Daniel

> 
> Noralf.
> 
> >Otherwise lgtm.
> >-Daniel
> >
> >>   */
> >>-struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>-	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> >>+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> >>+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> >>+	struct drm_framebuffer_funcs *funcs)
> >>  {
> >>  	struct drm_fb_cma *fb_cma;
> >>  	struct drm_gem_cma_object *objs[4];
> >>@@ -204,7 +207,7 @@ struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>  		objs[i] = to_drm_gem_cma_obj(obj);
> >>  	}
> >>-	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, &drm_fb_cma_funcs);
> >>+	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);
> >>  	if (IS_ERR(fb_cma)) {
> >>  		ret = PTR_ERR(fb_cma);
> >>  		goto err_gem_object_unreference;
> >>@@ -217,6 +220,20 @@ err_gem_object_unreference:
> >>  		drm_gem_object_unreference_unlocked(&objs[i]->base);
> >>  	return ERR_PTR(ret);
> >>  }
> >>+EXPORT_SYMBOL_GPL(drm_fb_cma_create_with_funcs);
> >>+
> >>+/**
> >>+ * drm_fb_cma_create() - &drm_mode_config_funcs ->fb_create callback function
> >>+ *
> >>+ * If your hardware has special alignment or pitch requirements these should be
> >>+ * checked before calling this function.
> >>+ */
> >>+struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd)
> >>+{
> >>+	return drm_fb_cma_create_with_funcs(dev, file_priv, mode_cmd,
> >>+					    &drm_fb_cma_funcs);
> >>+}
> >>  EXPORT_SYMBOL_GPL(drm_fb_cma_create);
> >>  /**
> >>diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> >>index c6d9c9c..1f9a8bc 100644
> >>--- a/include/drm/drm_fb_cma_helper.h
> >>+++ b/include/drm/drm_fb_cma_helper.h
> >>@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
> >>  int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> >>  	struct drm_file *file_priv, unsigned int *handle);
> >>+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
> >>+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
> >>+	struct drm_framebuffer_funcs *funcs);
> >>  struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
> >>  	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);
> >>-- 
> >>2.2.2
> >>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Noralf Trønnes]

Den 05.05.2016 19:03, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
>> Add function to create a simple connector for a panel.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Like in the previous patch please also add a new section for the panel
> helpers to gpu.tmpl. I don't think this needs an overview section, it's so
> simple. But adding some cross references from the drm_panel.c kerneldoc to
> this and back would be real good.

drm_panel.c doesn't have any documentation and the header file has only
the drm_panel_funcs struct documented, not hooked up to gpu.tmpl.

I can make a patch documenting the functions, it looks fairly straight
forward, but I have no idea what to put in the DOC: section, except an
xref to this helper :-)

Noralf.

>> ---
>>   drivers/gpu/drm/Makefile           |   1 +
>>   drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/panel/Kconfig      |   7 +++
>>   include/drm/drm_panel_helper.h     |  20 +++++++
>>   4 files changed, 145 insertions(+)
>>   create mode 100644 drivers/gpu/drm/drm_panel_helper.c
>>   create mode 100644 include/drm/drm_panel_helper.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 7e99923..3f3696c 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>   drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>   drm-$(CONFIG_PCI) += ati_pcigart.o
>>   drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>> +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o
>>   drm-$(CONFIG_OF) += drm_of.o
>>   drm-$(CONFIG_AGP) += drm_agpsupport.o
>>   
>> diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c
>> new file mode 100644
>> index 0000000..5d8b623
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_panel_helper.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Copyright (C) 2016 Noralf Trønnes
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_panel.h>
>> +
>> +struct drm_panel_connector {
>> +	struct drm_connector base;
>> +	struct drm_panel *panel;
>> +};
>> +
>> +static inline struct drm_panel_connector *
>> +to_drm_panel_connector(struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct drm_panel_connector, base);
>> +}
>> +
>> +static int drm_panel_connector_get_modes(struct drm_connector *connector)
>> +{
>> +	return drm_panel_get_modes(to_drm_panel_connector(connector)->panel);
>> +}
>> +
>> +static struct drm_encoder *
>> +drm_panel_connector_best_encoder(struct drm_connector *connector)
>> +{
>> +	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
>> +}
> As mentioned in my previous review, this function would be extremely
> useful to rid tons of drivers of boilerplate - most drm_connector only
> support exactly 1 encoder, statically determined at driver init time.
>
> Please put this helper into the atomic helper library, and maybe call it
> something like
>
> drm_atomic_helper_best_encoder.
>
> To make sure it's never abused by accident please also add a
>
> 	WARN_ON(connector->encoder_ids[1]);
>
> to make sure that there's really only just one encoder for this connector.
>
> If you're bored you can also go through drivers and use that everywhere it
> fits, it would result in a _lot_ of deleted code. But also needs quite a
> bit of audit work ...
>
> Otherwise lgtm, but please ask Thierry for an ack as the panel maintainer.
> -Daniel
>
>
>> +
>> +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = {
>> +	.get_modes = drm_panel_connector_get_modes,
>> +	.best_encoder = drm_panel_connector_best_encoder,
>> +};
>> +
>> +static enum drm_connector_status
>> +drm_panel_connector_detect(struct drm_connector *connector, bool force)
>> +{
>> +	if (drm_device_is_unplugged(connector->dev))
>> +		return connector_status_disconnected;
>> +
>> +	return connector->status;
>> +}
>> +
>> +static void drm_panel_connector_destroy(struct drm_connector *connector)
>> +{
>> +	struct drm_panel_connector *panel_connector;
>> +
>> +	panel_connector = to_drm_panel_connector(connector);
>> +	drm_panel_detach(panel_connector->panel);
>> +	drm_panel_remove(panel_connector->panel);
>> +	drm_connector_unregister(connector);
>> +	drm_connector_cleanup(connector);
>> +	kfree(panel_connector);
>> +}
>> +
>> +static const struct drm_connector_funcs drm_panel_connector_funcs = {
>> +	.dpms = drm_atomic_helper_connector_dpms,
>> +	.reset = drm_atomic_helper_connector_reset,
>> +	.detect = drm_panel_connector_detect,
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.destroy = drm_panel_connector_destroy,
>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +/**
>> + * drm_panel_connector_create - Create simple connector for panel
>> + * @dev: DRM device
>> + * @panel: DRM panel
>> + * @connector_type: user visible type of the connector
>> + *
>> + * Creates a simple connector for a panel.
>> + * The panel needs to provide a get_modes() function.
>> + *
>> + * Returns:
>> + * Pointer to new connector or ERR_PTR on failure.
>> + */
>> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
>> +						 struct drm_panel *panel,
>> +						 int connector_type)
>> +{
>> +	struct drm_panel_connector *panel_connector;
>> +	struct drm_connector *connector;
>> +	int ret;
>> +
>> +	panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL);
>> +	if (!panel_connector)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	panel_connector->panel = panel;
>> +	connector = &panel_connector->base;
>> +	drm_connector_helper_add(connector, &drm_panel_connector_hfuncs);
>> +	ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs,
>> +				 connector_type);
>> +	if (ret) {
>> +		kfree(panel_connector);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	connector->status = connector_status_connected;
>> +	drm_panel_init(panel);
>> +	drm_panel_add(panel);
>> +	drm_panel_attach(panel, connector);
>> +
>> +	return connector;
>> +}
>> +EXPORT_SYMBOL(drm_panel_connector_create);
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 1500ab9..87264a3 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -4,6 +4,13 @@ config DRM_PANEL
>>   	help
>>   	  Panel registration and lookup framework.
>>   
>> +config DRM_PANEL_HELPER
>> +	bool
>> +	depends on DRM
>> +	select DRM_PANEL
>> +	help
>> +	  Panel helper.
>> +
>>   menu "Display Panels"
>>   	depends on DRM && DRM_PANEL
>>   
>> diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h
>> new file mode 100644
>> index 0000000..c3355e3
>> --- /dev/null
>> +++ b/include/drm/drm_panel_helper.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * Copyright (C) 2016 Noralf Trønnes
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#ifndef __DRM_PANEL_HELPER_H__
>> +#define __DRM_PANEL_HELPER_H__
>> +
>> +struct drm_device;
>> +struct drm_panel;
>> +
>> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev,
>> +						 struct drm_panel *panel,
>> +						 int connector_type);
>> +
>> +#endif
>> -- 
>> 2.2.2
>>

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Thierry Reding]

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

On Fri, May 06, 2016 at 03:39:53PM +0200, Noralf Trønnes wrote:
> 
> Den 05.05.2016 19:03, skrev Daniel Vetter:
> > On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> > > Add function to create a simple connector for a panel.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > Like in the previous patch please also add a new section for the panel
> > helpers to gpu.tmpl. I don't think this needs an overview section, it's so
> > simple. But adding some cross references from the drm_panel.c kerneldoc to
> > this and back would be real good.
> 
> drm_panel.c doesn't have any documentation and the header file has only
> the drm_panel_funcs struct documented, not hooked up to gpu.tmpl.
> 
> I can make a patch documenting the functions, it looks fairly straight
> forward, but I have no idea what to put in the DOC: section, except an
> xref to this helper :-)

Maybe now is a good time for me to post the below. I really should've
sent this out ages ago, sorry.

Thierry
--- >8 ---
From 77057510413f8ca52d37da883afeabb13031ec63 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Tue, 4 Nov 2014 15:23:10 +0100
Subject: [PATCH] drm/panel: Flesh out kerneldoc

Write more complete kerneldoc comments for the DRM panel API and
integrate the helpers in the DRM DocBook reference.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/DocBook/gpu.tmpl | 12 ++++++---
 drivers/gpu/drm/drm_panel.c    | 61 ++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_panel.h        | 59 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 1464fb2f3c46..fb4ad6945a97 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -1671,17 +1671,23 @@ void intel_crt_init(struct drm_device *dev)
 !Pdrivers/gpu/drm/drm_crtc.c Tile group
     </sect2>
     <sect2>
-	<title>Bridges</title>
+      <title>Bridges</title>
       <sect3>
-	 <title>Overview</title>
+        <title>Overview</title>
 !Pdrivers/gpu/drm/drm_bridge.c overview
       </sect3>
       <sect3>
-	 <title>Default bridge callback sequence</title>
+        <title>Default bridge callback sequence</title>
 !Pdrivers/gpu/drm/drm_bridge.c bridge callbacks
       </sect3>
 !Edrivers/gpu/drm/drm_bridge.c
     </sect2>
+    <sect2>
+      <title>Panel Helper Reference</title>
+!Iinclude/drm/drm_panel.h
+!Edrivers/gpu/drm/drm_panel.c
+!Pdrivers/gpu/drm/drm_panel.c drm panel
+    </sect2>
   </sect1>
 
   <!-- Internals: kms properties -->
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e037b7..3dfe3c886502 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -30,12 +30,36 @@
 static DEFINE_MUTEX(panel_lock);
 static LIST_HEAD(panel_list);
 
+/**
+ * DOC: drm panel
+ *
+ * The DRM panel helpers allow drivers to register panel objects with a
+ * central registry and provide functions to retrieve those panels in display
+ * drivers.
+ */
+
+/**
+ * drm_panel_init - initialize a panel
+ * @panel: DRM panel
+ *
+ * Sets up internal fields of the panel so that it can subsequently be added
+ * to the registry.
+ */
 void drm_panel_init(struct drm_panel *panel)
 {
 	INIT_LIST_HEAD(&panel->list);
 }
 EXPORT_SYMBOL(drm_panel_init);
 
+/**
+ * drm_panel_add - add a panel to the global registry
+ * @panel: panel to add
+ *
+ * Add a panel to the global registry so that it can be looked up by display
+ * drivers.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 int drm_panel_add(struct drm_panel *panel)
 {
 	mutex_lock(&panel_lock);
@@ -46,6 +70,12 @@ int drm_panel_add(struct drm_panel *panel)
 }
 EXPORT_SYMBOL(drm_panel_add);
 
+/**
+ * drm_panel_remove - remove a panel from the global registry
+ * @panel: DRM panel
+ *
+ * Removes a panel from the global registry.
+ */
 void drm_panel_remove(struct drm_panel *panel)
 {
 	mutex_lock(&panel_lock);
@@ -54,6 +84,18 @@ void drm_panel_remove(struct drm_panel *panel)
 }
 EXPORT_SYMBOL(drm_panel_remove);
 
+/**
+ * drm_panel_attach - attach a panel to a connector
+ * @panel: DRM panel
+ * @connector: DRM connector
+ *
+ * After obtaining a pointer to a DRM panel a display driver calls this
+ * function to attach a panel to a connector.
+ *
+ * An error is returned if the panel is already attached to another connector.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 {
 	if (panel->connector)
@@ -66,6 +108,15 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_panel_attach);
 
+/**
+ * drm_panel_detach - detach a panel from a connector
+ * @panel: DRM panel
+ *
+ * Detaches a panel from the connector it is attached to. If a panel is not
+ * attached to any connector this is effectively a no-op.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 int drm_panel_detach(struct drm_panel *panel)
 {
 	panel->connector = NULL;
@@ -76,6 +127,16 @@ int drm_panel_detach(struct drm_panel *panel)
 EXPORT_SYMBOL(drm_panel_detach);
 
 #ifdef CONFIG_OF
+/**
+ * of_drm_find_panel - look up a panel using a device tree node
+ * @np: device tree node of the panel
+ *
+ * Searches the set of registered panels for one that matches the given device
+ * tree node. If a matching panel is found, return a pointer to it.
+ *
+ * Return: A pointer to the panel registered for the specified device tree
+ * node or NULL if no panel matching the device tree node can be found.
+ */
 struct drm_panel *of_drm_find_panel(struct device_node *np)
 {
 	struct drm_panel *panel;
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 13ff44b28893..220d1e2b3db1 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -75,6 +75,14 @@ struct drm_panel_funcs {
 			   struct display_timing *timings);
 };
 
+/**
+ * struct drm_panel - DRM panel object
+ * @drm: DRM device owning the panel
+ * @connector: DRM connector that the panel is attached to
+ * @dev: parent device of the panel
+ * @funcs: operations that can be performed on the panel
+ * @list: panel entry in registry
+ */
 struct drm_panel {
 	struct drm_device *drm;
 	struct drm_connector *connector;
@@ -85,6 +93,17 @@ struct drm_panel {
 	struct list_head list;
 };
 
+/**
+ * drm_disable_unprepare - power off a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will completely power off a panel (assert the panel's
+ * reset, turn off power supplies, ...). After this function has completed, it
+ * is usually no longer possible to communicate with the panel until another
+ * call to drm_panel_prepare().
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 static inline int drm_panel_unprepare(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->unprepare)
@@ -93,6 +112,16 @@ static inline int drm_panel_unprepare(struct drm_panel *panel)
 	return panel ? -ENOSYS : -EINVAL;
 }
 
+/**
+ * drm_panel_disable - disable a panel
+ * @panel: DRM panel
+ *
+ * This will typically turn off the panel's backlight or disable the display
+ * drivers. For smart panels it should still be possible to communicate with
+ * the integrated circuitry via any command bus after this call.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 static inline int drm_panel_disable(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->disable)
@@ -101,6 +130,16 @@ static inline int drm_panel_disable(struct drm_panel *panel)
 	return panel ? -ENOSYS : -EINVAL;
 }
 
+/**
+ * drm_panel_prepare - power on a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will enable power and deassert any reset signals to
+ * the panel. After this has completed it is possible to communicate with any
+ * integrated circuitry via a command bus.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 static inline int drm_panel_prepare(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->prepare)
@@ -109,6 +148,16 @@ static inline int drm_panel_prepare(struct drm_panel *panel)
 	return panel ? -ENOSYS : -EINVAL;
 }
 
+/**
+ * drm_panel_enable - enable a panel
+ * @panel: DRM panel
+ *
+ * Calling this function will cause the panel display drivers to be turned on
+ * and the backlight to be enabled. Content will be visible on screen after
+ * this call completes.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 static inline int drm_panel_enable(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->enable)
@@ -117,6 +166,16 @@ static inline int drm_panel_enable(struct drm_panel *panel)
 	return panel ? -ENOSYS : -EINVAL;
 }
 
+/**
+ * drm_panel_get_modes - probe the available display modes of a panel
+ * @panel: DRM panel
+ *
+ * The modes probed from the panel are automatically added to the connector
+ * that the panel is attached to.
+ *
+ * Return: The number of modes available from the panel on success or a
+ * negative error code on failure.
+ */
 static inline int drm_panel_get_modes(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->get_modes)
-- 
2.8.2


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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Thierry Reding]

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

On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> Add function to create a simple connector for a panel.

I'm not sure I see the usefulness of this. Typically you'd attach a
panel to an encoder/connector, in which case you already have the
connector.

Perhaps it would become more obvious why we need this if you posted
patches that show where this is used?

Thierry

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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Fri, May 06, 2016 at 04:01:37PM +0200, Thierry Reding wrote:
> On Fri, May 06, 2016 at 03:39:53PM +0200, Noralf Trønnes wrote:
> > 
> > Den 05.05.2016 19:03, skrev Daniel Vetter:
> > > On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> > > > Add function to create a simple connector for a panel.
> > > > 
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > Like in the previous patch please also add a new section for the panel
> > > helpers to gpu.tmpl. I don't think this needs an overview section, it's so
> > > simple. But adding some cross references from the drm_panel.c kerneldoc to
> > > this and back would be real good.
> > 
> > drm_panel.c doesn't have any documentation and the header file has only
> > the drm_panel_funcs struct documented, not hooked up to gpu.tmpl.
> > 
> > I can make a patch documenting the functions, it looks fairly straight
> > forward, but I have no idea what to put in the DOC: section, except an
> > xref to this helper :-)
> 
> Maybe now is a good time for me to post the below. I really should've
> sent this out ages ago, sorry.
> 
> Thierry
> --- >8 ---
> From 77057510413f8ca52d37da883afeabb13031ec63 Mon Sep 17 00:00:00 2001
> From: Thierry Reding <treding@nvidia.com>
> Date: Tue, 4 Nov 2014 15:23:10 +0100
> Subject: [PATCH] drm/panel: Flesh out kerneldoc
> 
> Write more complete kerneldoc comments for the DRM panel API and
> integrate the helpers in the DRM DocBook reference.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Applied to drm-msic, thanks.

> ---
>  Documentation/DocBook/gpu.tmpl | 12 ++++++---
>  drivers/gpu/drm/drm_panel.c    | 61 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_panel.h        | 59 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1464fb2f3c46..fb4ad6945a97 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1671,17 +1671,23 @@ void intel_crt_init(struct drm_device *dev)
>  !Pdrivers/gpu/drm/drm_crtc.c Tile group
>      </sect2>
>      <sect2>
> -	<title>Bridges</title>
> +      <title>Bridges</title>
>        <sect3>
> -	 <title>Overview</title>
> +        <title>Overview</title>
>  !Pdrivers/gpu/drm/drm_bridge.c overview
>        </sect3>
>        <sect3>
> -	 <title>Default bridge callback sequence</title>
> +        <title>Default bridge callback sequence</title>
>  !Pdrivers/gpu/drm/drm_bridge.c bridge callbacks
>        </sect3>
>  !Edrivers/gpu/drm/drm_bridge.c
>      </sect2>
> +    <sect2>
> +      <title>Panel Helper Reference</title>
> +!Iinclude/drm/drm_panel.h
> +!Edrivers/gpu/drm/drm_panel.c
> +!Pdrivers/gpu/drm/drm_panel.c drm panel
> +    </sect2>

Hm, since you call this a helper, and we already have a Kconfig for it I
guess would make sense to put Noralf's connector-for-panel helper in there
too?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
> On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> > Add function to create a simple connector for a panel.
> 
> I'm not sure I see the usefulness of this. Typically you'd attach a
> panel to an encoder/connector, in which case you already have the
> connector.
> 
> Perhaps it would become more obvious why we need this if you posted
> patches that show where this is used?

The other helpers give you a simple drm pipeline with plane, crtc &
encoder all baked into on drm_simple_pipeline structure. The only thing
variable you have to hook up to that is the drm_connector. And I think for
dead-simple panels avoiding the basic boilerplate in that does indeed make
some sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Thierry Reding]

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

On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
> On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
> > On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> > > Add function to create a simple connector for a panel.
> > 
> > I'm not sure I see the usefulness of this. Typically you'd attach a
> > panel to an encoder/connector, in which case you already have the
> > connector.
> > 
> > Perhaps it would become more obvious why we need this if you posted
> > patches that show where this is used?
> 
> The other helpers give you a simple drm pipeline with plane, crtc &
> encoder all baked into on drm_simple_pipeline structure. The only thing
> variable you have to hook up to that is the drm_connector. And I think for
> dead-simple panels avoiding the basic boilerplate in that does indeed make
> some sense.

Avoiding boilerplate is good, but I have a difficult time envisioning
how you might want to use this. At the same time I'm asking myself how
we know that this helper is any good if we haven't seen it used anywhere
and actually see the boilerplate go away.

Thierry

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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Noralf Trønnes]

Den 06.05.2016 16:15, skrev Thierry Reding:
> On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
>> On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
>>> On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
>>>> Add function to create a simple connector for a panel.
>>> I'm not sure I see the usefulness of this. Typically you'd attach a
>>> panel to an encoder/connector, in which case you already have the
>>> connector.
>>>
>>> Perhaps it would become more obvious why we need this if you posted
>>> patches that show where this is used?
>> The other helpers give you a simple drm pipeline with plane, crtc &
>> encoder all baked into on drm_simple_pipeline structure. The only thing
>> variable you have to hook up to that is the drm_connector. And I think for
>> dead-simple panels avoiding the basic boilerplate in that does indeed make
>> some sense.
> Avoiding boilerplate is good, but I have a difficult time envisioning
> how you might want to use this. At the same time I'm asking myself how
> we know that this helper is any good if we haven't seen it used anywhere
> and actually see the boilerplate go away.

I pulled out the patches from the tinydrm patchset that would go
into drm core and helpers. I'm not very good at juggling many patches
around in various version and getting it right.
I'm doing development in the downstream Raspberry Pi repo
on 4.5 to get all the pi drivers, and then apply it on linux-next...

This is the tinydrm patch that will use it:
https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html

Extract:
+int tinydrm_display_pipe_init(struct tinydrm_device *tdev,
+                  const uint32_t *formats, unsigned int format_count)
+{
+    struct drm_device *dev = tdev->base;
+    struct drm_connector *connector;
+    int ret;
+
+    connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel,
+                        DRM_MODE_CONNECTOR_VIRTUAL);
+    if (IS_ERR(connector))
+        return PTR_ERR(connector);
+
+    ret = drm_simple_display_pipe_init(dev, &tdev->pipe,
+                &tinydrm_display_pipe_funcs,
+                formats, format_count,
+                connector);
+
+    return ret;
+}
+EXPORT_SYMBOL(tinydrm_display_pipe_init);

drm_simple_kms_panel_connector_create() changed name when Daniel
suggested it be moved to drm_panel.c or drm_panel_helper.c.

Noralf.

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Fri, May 06, 2016 at 04:34:08PM +0200, Noralf Trønnes wrote:
> 
> Den 06.05.2016 16:15, skrev Thierry Reding:
> >On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
> >>On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
> >>>On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> >>>>Add function to create a simple connector for a panel.
> >>>I'm not sure I see the usefulness of this. Typically you'd attach a
> >>>panel to an encoder/connector, in which case you already have the
> >>>connector.
> >>>
> >>>Perhaps it would become more obvious why we need this if you posted
> >>>patches that show where this is used?
> >>The other helpers give you a simple drm pipeline with plane, crtc &
> >>encoder all baked into on drm_simple_pipeline structure. The only thing
> >>variable you have to hook up to that is the drm_connector. And I think for
> >>dead-simple panels avoiding the basic boilerplate in that does indeed make
> >>some sense.
> >Avoiding boilerplate is good, but I have a difficult time envisioning
> >how you might want to use this. At the same time I'm asking myself how
> >we know that this helper is any good if we haven't seen it used anywhere
> >and actually see the boilerplate go away.
> 
> I pulled out the patches from the tinydrm patchset that would go
> into drm core and helpers. I'm not very good at juggling many patches
> around in various version and getting it right.
> I'm doing development in the downstream Raspberry Pi repo
> on 4.5 to get all the pi drivers, and then apply it on linux-next...
> 
> This is the tinydrm patch that will use it:
> https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html
> 
> Extract:
> +int tinydrm_display_pipe_init(struct tinydrm_device *tdev,
> +                  const uint32_t *formats, unsigned int format_count)
> +{
> +    struct drm_device *dev = tdev->base;
> +    struct drm_connector *connector;
> +    int ret;
> +
> +    connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel,
> +                        DRM_MODE_CONNECTOR_VIRTUAL);
> +    if (IS_ERR(connector))
> +        return PTR_ERR(connector);
> +
> +    ret = drm_simple_display_pipe_init(dev, &tdev->pipe,
> +                &tinydrm_display_pipe_funcs,
> +                formats, format_count,
> +                connector);
> +
> +    return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_display_pipe_init);

It's imo even more impressive when you show the entire driver, including
all the hooks. Because there's just 4 of them iirc, all optional.

And besides the tinydrm panel drivers in it's various editions we could
also use this for simpledrm (the boot-up firmware kms driver from David
Herrmann) and probably a few other super-simple display drivers.
Essentially with this I think drm is now better suited for dumb&simple
hardware than fbdev.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Thierry Reding]

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

On Fri, May 06, 2016 at 04:34:08PM +0200, Noralf Trønnes wrote:
> 
> Den 06.05.2016 16:15, skrev Thierry Reding:
> > On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
> > > On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
> > > > On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
> > > > > Add function to create a simple connector for a panel.
> > > > I'm not sure I see the usefulness of this. Typically you'd attach a
> > > > panel to an encoder/connector, in which case you already have the
> > > > connector.
> > > > 
> > > > Perhaps it would become more obvious why we need this if you posted
> > > > patches that show where this is used?
> > > The other helpers give you a simple drm pipeline with plane, crtc &
> > > encoder all baked into on drm_simple_pipeline structure. The only thing
> > > variable you have to hook up to that is the drm_connector. And I think for
> > > dead-simple panels avoiding the basic boilerplate in that does indeed make
> > > some sense.
> > Avoiding boilerplate is good, but I have a difficult time envisioning
> > how you might want to use this. At the same time I'm asking myself how
> > we know that this helper is any good if we haven't seen it used anywhere
> > and actually see the boilerplate go away.
> 
> I pulled out the patches from the tinydrm patchset that would go
> into drm core and helpers. I'm not very good at juggling many patches
> around in various version and getting it right.
> I'm doing development in the downstream Raspberry Pi repo
> on 4.5 to get all the pi drivers, and then apply it on linux-next...
> 
> This is the tinydrm patch that will use it:
> https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html
> 
> Extract:
> +int tinydrm_display_pipe_init(struct tinydrm_device *tdev,
> +                  const uint32_t *formats, unsigned int format_count)
> +{
> +    struct drm_device *dev = tdev->base;
> +    struct drm_connector *connector;
> +    int ret;
> +
> +    connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel,
> +                        DRM_MODE_CONNECTOR_VIRTUAL);
> +    if (IS_ERR(connector))
> +        return PTR_ERR(connector);
> +
> +    ret = drm_simple_display_pipe_init(dev, &tdev->pipe,
> +                &tinydrm_display_pipe_funcs,
> +                formats, format_count,
> +                connector);
> +
> +    return ret;
> +}
> +EXPORT_SYMBOL(tinydrm_display_pipe_init);
> 
> drm_simple_kms_panel_connector_create() changed name when Daniel
> suggested it be moved to drm_panel.c or drm_panel_helper.c.

Okay, that gives me a better understanding of where things are headed.
That said, why would these devices even need to deal with drm_panel in
the first place? Sounds to me like they are devices on some sort of
control bus that you talk to directly. And they will represent
essentially the panel with its built-in display controller.

drm_panel on the other hand was designed as an interface between display
controllers and panels, with the goal to let any display controller talk
to any panel.

While I'm sure you can support these types of simple panels using the
drm_panel infrastructure I'm not sure it's necessary, since your driver
will always talk to the panel directly, and hence the code to deal with
the panel specifics could be part of the display pipe functions.

Thierry

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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Noralf Trønnes]

Den 06.05.2016 16:43, skrev Thierry Reding:
> On Fri, May 06, 2016 at 04:34:08PM +0200, Noralf Trønnes wrote:
>> Den 06.05.2016 16:15, skrev Thierry Reding:
>>> On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote:
>>>> On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote:
>>>>> On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote:
>>>>>> Add function to create a simple connector for a panel.
>>>>> I'm not sure I see the usefulness of this. Typically you'd attach a
>>>>> panel to an encoder/connector, in which case you already have the
>>>>> connector.
>>>>>
>>>>> Perhaps it would become more obvious why we need this if you posted
>>>>> patches that show where this is used?
>>>> The other helpers give you a simple drm pipeline with plane, crtc &
>>>> encoder all baked into on drm_simple_pipeline structure. The only thing
>>>> variable you have to hook up to that is the drm_connector. And I think for
>>>> dead-simple panels avoiding the basic boilerplate in that does indeed make
>>>> some sense.
>>> Avoiding boilerplate is good, but I have a difficult time envisioning
>>> how you might want to use this. At the same time I'm asking myself how
>>> we know that this helper is any good if we haven't seen it used anywhere
>>> and actually see the boilerplate go away.
>> I pulled out the patches from the tinydrm patchset that would go
>> into drm core and helpers. I'm not very good at juggling many patches
>> around in various version and getting it right.
>> I'm doing development in the downstream Raspberry Pi repo
>> on 4.5 to get all the pi drivers, and then apply it on linux-next...
>>
>> This is the tinydrm patch that will use it:
>> https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html
>>
>> Extract:
>> +int tinydrm_display_pipe_init(struct tinydrm_device *tdev,
>> +                  const uint32_t *formats, unsigned int format_count)
>> +{
>> +    struct drm_device *dev = tdev->base;
>> +    struct drm_connector *connector;
>> +    int ret;
>> +
>> +    connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel,
>> +                        DRM_MODE_CONNECTOR_VIRTUAL);
>> +    if (IS_ERR(connector))
>> +        return PTR_ERR(connector);
>> +
>> +    ret = drm_simple_display_pipe_init(dev, &tdev->pipe,
>> +                &tinydrm_display_pipe_funcs,
>> +                formats, format_count,
>> +                connector);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL(tinydrm_display_pipe_init);
>>
>> drm_simple_kms_panel_connector_create() changed name when Daniel
>> suggested it be moved to drm_panel.c or drm_panel_helper.c.
> Okay, that gives me a better understanding of where things are headed.
> That said, why would these devices even need to deal with drm_panel in
> the first place? Sounds to me like they are devices on some sort of
> control bus that you talk to directly. And they will represent
> essentially the panel with its built-in display controller.
>
> drm_panel on the other hand was designed as an interface between display
> controllers and panels, with the goal to let any display controller talk
> to any panel.
>
> While I'm sure you can support these types of simple panels using the
> drm_panel infrastructure I'm not sure it's necessary, since your driver
> will always talk to the panel directly, and hence the code to deal with
> the panel specifics could be part of the display pipe functions.

In the discussion following the "no more fbdev drivers" call, someone
pointed me to drm_panel. It had function hooks that I needed, so I built
tinydrm around it. If this is misusing drm_panel, it shouldn't be too
difficult to drop it.

Actually I could just do this:

struct tinydrm_funcs {
         int (*prepare)(struct tinydrm_device *tdev);
         int (*unprepare)(struct tinydrm_device *tdev);
         int (*enable)(struct tinydrm_device *tdev);
         int (*disable)(struct tinydrm_device *tdev);
         int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned 
flags,
                      unsigned color, struct drm_clip_rect *clips,
                      unsigned num_clips);
};


When it comes to the simple connector, one solution could be to extend
drm_simple_display_pipe to embed a connector with (optional) modes:

  struct drm_simple_display_pipe {
-        struct drm_connector *connector;
+        struct drm_connector connector;
+        const struct drm_display_mode *modes;
+        unsigned int num_modes;
  };

And maybe optional detect and get_modes hooks:

  struct drm_simple_display_pipe_funcs {
+        enum drm_connector_status detect(struct drm_connector *connector,
+                                         bool force);
+        int (*get_modes)(struct drm_connector *connector);
  };

  int drm_simple_display_pipe_init(struct drm_device *dev,
                                   struct drm_simple_display_pipe *pipe,
                                   struct drm_simple_display_pipe_funcs 
*funcs,
                                   const uint32_t *formats,
                                   unsigned int format_count,
-                                 struct drm_connector *connector);
+                                 int connector_type);


Another solution is to create a simple connector with modes:

struct drm_simple_connector {
         struct drm_connector connector;
         const struct drm_display_mode *modes;
         unsigned int num_modes;
};

struct drm_connector *drm_simple_connector_create(struct drm_device *dev,
         const struct drm_display_mode *modes, unsigned int num_modes,
         int connector_type);


Noralf.

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Daniel Vetter]

On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> In the discussion following the "no more fbdev drivers" call, someone
> pointed me to drm_panel. It had function hooks that I needed, so I built
> tinydrm around it. If this is misusing drm_panel, it shouldn't be too
> difficult to drop it.
>
> Actually I could just do this:
>
> struct tinydrm_funcs {
>         int (*prepare)(struct tinydrm_device *tdev);
>         int (*unprepare)(struct tinydrm_device *tdev);
>         int (*enable)(struct tinydrm_device *tdev);
>         int (*disable)(struct tinydrm_device *tdev);
>         int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned flags,
>                      unsigned color, struct drm_clip_rect *clips,
>                      unsigned num_clips);
> };
>
>
> When it comes to the simple connector, one solution could be to extend
> drm_simple_display_pipe to embed a connector with (optional) modes:
>
>  struct drm_simple_display_pipe {
> -        struct drm_connector *connector;
> +        struct drm_connector connector;
> +        const struct drm_display_mode *modes;
> +        unsigned int num_modes;
>  };
>
> And maybe optional detect and get_modes hooks:
>
>  struct drm_simple_display_pipe_funcs {
> +        enum drm_connector_status detect(struct drm_connector *connector,
> +                                         bool force);
> +        int (*get_modes)(struct drm_connector *connector);
>  };
>
>  int drm_simple_display_pipe_init(struct drm_device *dev,
>                                   struct drm_simple_display_pipe *pipe,
>                                   struct drm_simple_display_pipe_funcs
> *funcs,
>                                   const uint32_t *formats,
>                                   unsigned int format_count,
> -                                 struct drm_connector *connector);
> +                                 int connector_type);

Tbh I really like that simple_display_pipe is split after the encoder.
This allows us to easily splice in a drm_bridge (which will register
the drm_connector itself) with very little work. And even if there's
not a full-blown bridge you might have different connectors and
things.

> Another solution is to create a simple connector with modes:
>
> struct drm_simple_connector {
>         struct drm_connector connector;
>         const struct drm_display_mode *modes;
>         unsigned int num_modes;
> };
>
> struct drm_connector *drm_simple_connector_create(struct drm_device *dev,
>         const struct drm_display_mode *modes, unsigned int num_modes,
>         int connector_type);

Yeah, this makes more sense to me, but then we're kinda reinventing
something like simple-panel.c with this. Otoh right now with
smple_display_pipe it's not possible to wire up the panel callbacks
easily, so maybe it should be a drm_bridge. Or we just leave this code
in tinydrm and extract it when someone else has a need for it?
-Daniel



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

Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

[Noralf Trønnes]

Den 07.05.2016 11:59, skrev Daniel Vetter:
> On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> In the discussion following the "no more fbdev drivers" call, someone
>> pointed me to drm_panel. It had function hooks that I needed, so I built
>> tinydrm around it. If this is misusing drm_panel, it shouldn't be too
>> difficult to drop it.
>>
>> Actually I could just do this:
>>
>> struct tinydrm_funcs {
>>          int (*prepare)(struct tinydrm_device *tdev);
>>          int (*unprepare)(struct tinydrm_device *tdev);
>>          int (*enable)(struct tinydrm_device *tdev);
>>          int (*disable)(struct tinydrm_device *tdev);
>>          int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned flags,
>>                       unsigned color, struct drm_clip_rect *clips,
>>                       unsigned num_clips);
>> };
>>
>>
>> When it comes to the simple connector, one solution could be to extend
>> drm_simple_display_pipe to embed a connector with (optional) modes:
>>
>>   struct drm_simple_display_pipe {
>> -        struct drm_connector *connector;
>> +        struct drm_connector connector;
>> +        const struct drm_display_mode *modes;
>> +        unsigned int num_modes;
>>   };
>>
>> And maybe optional detect and get_modes hooks:
>>
>>   struct drm_simple_display_pipe_funcs {
>> +        enum drm_connector_status detect(struct drm_connector *connector,
>> +                                         bool force);
>> +        int (*get_modes)(struct drm_connector *connector);
>>   };
>>
>>   int drm_simple_display_pipe_init(struct drm_device *dev,
>>                                    struct drm_simple_display_pipe *pipe,
>>                                    struct drm_simple_display_pipe_funcs
>> *funcs,
>>                                    const uint32_t *formats,
>>                                    unsigned int format_count,
>> -                                 struct drm_connector *connector);
>> +                                 int connector_type);
> Tbh I really like that simple_display_pipe is split after the encoder.
> This allows us to easily splice in a drm_bridge (which will register
> the drm_connector itself) with very little work. And even if there's
> not a full-blown bridge you might have different connectors and
> things.
>
>> Another solution is to create a simple connector with modes:
>>
>> struct drm_simple_connector {
>>          struct drm_connector connector;
>>          const struct drm_display_mode *modes;
>>          unsigned int num_modes;
>> };
>>
>> struct drm_connector *drm_simple_connector_create(struct drm_device *dev,
>>          const struct drm_display_mode *modes, unsigned int num_modes,
>>          int connector_type);
> Yeah, this makes more sense to me, but then we're kinda reinventing
> something like simple-panel.c with this. Otoh right now with
> smple_display_pipe it's not possible to wire up the panel callbacks
> easily, so maybe it should be a drm_bridge. Or we just leave this code
> in tinydrm and extract it when someone else has a need for it?

Yes, since there's no obvious use for such a simple controller I'll just 
put it in tinydrm.

Noralf.

Re: [PATCH 3/4] drm: Add helper for simple display pipeline

[Daniel Vetter]

On Thu, May 05, 2016 at 03:24:33PM +0200, Noralf Trønnes wrote:
> Provides helper functions for drivers that have a simple display
> pipeline. Plane, crtc and encoder are collapsed into one entity.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> +					struct drm_plane_state *pstate)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +	struct drm_crtc_state *cstate;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	if (!pipe->funcs || !pipe->funcs->check)
> +		return 0;
> +
> +	cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> +						    &pipe->crtc);
> +
> +	return pipe->funcs->check(pipe, pstate, cstate);
> +}

Ok one thing I've missed here is that for most drivers this is way too
simple a check function, which means we'll end up with tons of duplicated
code. Things which the drm core allows, but simple pipelines all don't
really cope with:
- plane scaling
- disabling the plane without the crtc (i.e. scan out black)
- plane not sized to fill the entire hactive/vactive

There's a helper to do most of these checks for you -
drm_plane_helper_check_update. I think it'd be good to place a call for
that in here, before we call down into the driver's ->check callback. But
ofc before we return 0; we want these checks always done. And catch all
these things so that drivers never fall over this pitfall.

Noticed while discussing tilcdc atomic patches, since tilcdc could
probably use drm_simple_display_pipe too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] drm: Add helper for simple display pipeline

[Noralf Trønnes]

Den 09.05.2016 16:46, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:33PM +0200, Noralf Trønnes wrote:
>> Provides helper functions for drivers that have a simple display
>> pipeline. Plane, crtc and encoder are collapsed into one entity.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>> +					struct drm_plane_state *pstate)
>> +{
>> +	struct drm_simple_display_pipe *pipe;
>> +	struct drm_crtc_state *cstate;
>> +
>> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>> +	if (!pipe->funcs || !pipe->funcs->check)
>> +		return 0;
>> +
>> +	cstate = drm_atomic_get_existing_crtc_state(pstate->state,
>> +						    &pipe->crtc);
>> +
>> +	return pipe->funcs->check(pipe, pstate, cstate);
>> +}
> Ok one thing I've missed here is that for most drivers this is way too
> simple a check function, which means we'll end up with tons of duplicated
> code. Things which the drm core allows, but simple pipelines all don't
> really cope with:
> - plane scaling
> - disabling the plane without the crtc (i.e. scan out black)
> - plane not sized to fill the entire hactive/vactive
>
> There's a helper to do most of these checks for you -
> drm_plane_helper_check_update. I think it'd be good to place a call for
> that in here, before we call down into the driver's ->check callback. But
> ofc before we return 0; we want these checks always done. And catch all
> these things so that drivers never fall over this pitfall.

Does this resemble what you're after? I'm just guessing here.

static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
                     struct drm_plane_state *pstate)
{
     struct drm_rect src = {
         .x1 = pstate->src_x,
         .y1 = pstate->src_y,
         .x2 = pstate->src_x + pstate->src_w,
         .y2 = pstate->src_y + pstate->src_h,
     };
     struct drm_rect dest = {
         .x1 = pstate->crtc_x,
         .y1 = pstate->crtc_y,
         .x2 = pstate->crtc_x + pstate->crtc_w,
         .y2 = pstate->crtc_y + pstate->crtc_h,
     };
     struct drm_rect clip = { 0 };
     struct drm_simple_display_pipe *pipe;
     struct drm_crtc_state *cstate;
     bool visible;
     int ret;

     pipe = container_of(plane, struct drm_simple_display_pipe, plane);
     clip.x2 = pipe->crtc.mode.hdisplay;
     clip.y2 = pipe->crtc.mode.vdisplay;
     ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
                         &src, &dest, &clip,
                         DRM_PLANE_HELPER_NO_SCALING,
                         DRM_PLANE_HELPER_NO_SCALING,
                         false, false, &visible);
     if (ret)
         return ret;

     /* How to handle !visible, is it even possible? */

     if (!pipe->funcs || !pipe->funcs->check)
         return 0;

     cstate = drm_atomic_get_existing_crtc_state(pstate->state,
                             &pipe->crtc);

     return pipe->funcs->check(pipe, pstate, cstate);
}


Noralf.

> Noticed while discussing tilcdc atomic patches, since tilcdc could
> probably use drm_simple_display_pipe too.
> -Daniel

Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional

[Noralf Trønnes]

Den 05.05.2016 18:23, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
>> Make drm_encoder_helper_funcs and it's functions optional to avoid
>> having dummy functions.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Please also update the kerneldoc and mention there that the enable/disable
> hooks are optional. You can build the hmtl docs using

AFAICT the kerneldoc already says that they are optional for atomic helpers:

struct drm_encoder_helper_funcs {
[...]
         /**
          * @disable:
[...]
          * This hook is used both by legacy CRTC helpers and atomic 
helpers.
          * Atomic drivers don't need to implement it if there's no need to
          * disable anything at the encoder level. To ensure that runtime PM
[...]
         /**
          * @enable:
[...]
          * This hook is used only by atomic helpers, for symmetry with 
@disable.
          * Atomic drivers don't need to implement it if there's no need to
          * enable anything at the encoder level. To ensure that runtime 
PM handling

Noralf.

> $ make htmldocs
>
> to check the result. The vtables are documented in
> include/drm/drm_helper_vtables.h
>
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
>>   drivers/gpu/drm/drm_crtc_helper.c   | 41 +++++++++++++++++++++++++++++--------
> Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
> part. Old drivers should converted to atomic, new drivers should just use
> atomic. No need to touch that code at all.
> -Daniel
>
>>   2 files changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 92e11a2..03ea049 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>   			continue;
>>   
>>   		funcs = encoder->helper_private;
>> +		if (!funcs)
>> +			continue;
>>   
>>   		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
>>   				 encoder->base.id, encoder->name);
>> @@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>   			funcs->prepare(encoder);
>>   		else if (funcs->disable)
>>   			funcs->disable(encoder);
>> -		else
>> +		else if (funcs->dpms)
>>   			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>   
>>   		drm_bridge_post_disable(encoder->bridge);
>> @@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>>   
>>   		encoder = connector->state->best_encoder;
>>   		funcs = encoder->helper_private;
>> +		if (!funcs)
>> +			continue;
>> +
>>   		new_crtc_state = connector->state->crtc->state;
>>   		mode = &new_crtc_state->mode;
>>   		adjusted_mode = &new_crtc_state->adjusted_mode;
>> @@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>   
>>   		encoder = connector->state->best_encoder;
>>   		funcs = encoder->helper_private;
>> +		if (!funcs)
>> +			continue;
>>   
>>   		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
>>   				 encoder->base.id, encoder->name);
>> @@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>   
>>   		if (funcs->enable)
>>   			funcs->enable(encoder);
>> -		else
>> +		else if (funcs->commit)
>>   			funcs->commit(encoder);
>>   
>>   		drm_bridge_enable(encoder->bridge);
>> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> index 66ca313..6e6ab38 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> @@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
>>   {
>>   	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
>>   
>> +	if (!encoder_funcs)
>> +		return;
>> +
>>   	drm_bridge_disable(encoder->bridge);
>>   
>>   	if (encoder_funcs->disable)
>>   		(*encoder_funcs->disable)(encoder);
>> -	else
>> +	else if (encoder_funcs->dpms)
>>   		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
>>   
>>   	drm_bridge_post_disable(encoder->bridge);
>> @@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
>>   
>>   	drm_for_each_encoder(encoder, dev) {
>>   		encoder_funcs = encoder->helper_private;
>> +		if (!encoder_funcs)
>> +			continue;
>> +
>>   		/* Disable unused encoders */
>>   		if (encoder->crtc == NULL)
>>   			drm_encoder_disable(encoder);
>> @@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>>   		if (encoder->crtc != crtc)
>>   			continue;
>>   
>> +		encoder_funcs = encoder->helper_private;
>> +		if (!encoder_funcs)
>> +			continue;
>> +
>>   		ret = drm_bridge_mode_fixup(encoder->bridge,
>>   			mode, adjusted_mode);
>>   		if (!ret) {
>> @@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>>   		if (encoder->crtc != crtc)
>>   			continue;
>>   
>> +		encoder_funcs = encoder->helper_private;
>> +		if (!encoder_funcs)
>> +			continue;
>> +
>>   		drm_bridge_disable(encoder->bridge);
>>   
>> -		encoder_funcs = encoder->helper_private;
>>   		/* Disable the encoders as the first thing we do. */
>> -		encoder_funcs->prepare(encoder);
>> +		if (encoder_funcs->prepare)
>> +			encoder_funcs->prepare(encoder);
>>   
>>   		drm_bridge_post_disable(encoder->bridge);
>>   	}
>> @@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>>   		if (encoder->crtc != crtc)
>>   			continue;
>>   
>> +		encoder_funcs = encoder->helper_private;
>> +		if (!encoder_funcs)
>> +			continue;
>> +
>>   		DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
>>   			encoder->base.id, encoder->name,
>>   			mode->base.id, mode->name);
>> -		encoder_funcs = encoder->helper_private;
>> -		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>> +		if (encoder_funcs->mode_set)
>> +			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>>   
>>   		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>>   	}
>> @@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>>   		if (encoder->crtc != crtc)
>>   			continue;
>>   
>> +		encoder_funcs = encoder->helper_private;
>> +		if (!encoder_funcs)
>> +			continue;
>> +
>>   		drm_bridge_pre_enable(encoder->bridge);
>>   
>> -		encoder_funcs = encoder->helper_private;
>> -		encoder_funcs->commit(encoder);
>> +		if (encoder_funcs->commit)
>> +			encoder_funcs->commit(encoder);
>>   
>>   		drm_bridge_enable(encoder->bridge);
>>   	}
>> @@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
>>   	struct drm_bridge *bridge = encoder->bridge;
>>   	const struct drm_encoder_helper_funcs *encoder_funcs;
>>   
>> +	encoder_funcs = encoder->helper_private;
>> +	if (!encoder_funcs)
>> +		return;
>> +
>>   	if (mode == DRM_MODE_DPMS_ON)
>>   		drm_bridge_pre_enable(bridge);
>>   	else
>>   		drm_bridge_disable(bridge);
>>   
>> -	encoder_funcs = encoder->helper_private;
>>   	if (encoder_funcs->dpms)
>>   		encoder_funcs->dpms(encoder, mode);
>>   
>> -- 
>> 2.2.2
>>

Re: [PATCH 2/4] drm: Make drm_encoder_helper_funcs optional

[Daniel Vetter]

On Mon, May 09, 2016 at 09:19:22PM +0200, Noralf Trønnes wrote:
> 
> Den 05.05.2016 18:23, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:32PM +0200, Noralf Trønnes wrote:
> >>Make drm_encoder_helper_funcs and it's functions optional to avoid
> >>having dummy functions.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >Please also update the kerneldoc and mention there that the enable/disable
> >hooks are optional. You can build the hmtl docs using
> 
> AFAICT the kerneldoc already says that they are optional for atomic helpers:
> 
> struct drm_encoder_helper_funcs {
> [...]
>         /**
>          * @disable:
> [...]
>          * This hook is used both by legacy CRTC helpers and atomic helpers.
>          * Atomic drivers don't need to implement it if there's no need to
>          * disable anything at the encoder level. To ensure that runtime PM
> [...]
>         /**
>          * @enable:
> [...]
>          * This hook is used only by atomic helpers, for symmetry with
> @disable.
>          * Atomic drivers don't need to implement it if there's no need to
>          * enable anything at the encoder level. To ensure that runtime PM
> handling

Indeed, docs not matching the code. But with your patch here will soon ;-)
I'll apply this one to drm-misc.
-Daniel

> 
> Noralf.
> 
> >$ make htmldocs
> >
> >to check the result. The vtables are documented in
> >include/drm/drm_helper_vtables.h
> >
> >>---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++--
> >>  drivers/gpu/drm/drm_crtc_helper.c   | 41 +++++++++++++++++++++++++++++--------
> >Hm, tbh I wouldn't bother at all with crtc helpers and just drop that
> >part. Old drivers should converted to atomic, new drivers should just use
> >atomic. No need to touch that code at all.
> >-Daniel
> >
> >>  2 files changed, 42 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>index 92e11a2..03ea049 100644
> >>--- a/drivers/gpu/drm/drm_atomic_helper.c
> >>+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>@@ -696,6 +696,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  			continue;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>  		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
> >>  				 encoder->base.id, encoder->name);
> >>@@ -711,7 +713,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  			funcs->prepare(encoder);
> >>  		else if (funcs->disable)
> >>  			funcs->disable(encoder);
> >>-		else
> >>+		else if (funcs->dpms)
> >>  			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> >>  		drm_bridge_post_disable(encoder->bridge);
> >>@@ -859,6 +861,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> >>  		encoder = connector->state->best_encoder;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>+
> >>  		new_crtc_state = connector->state->crtc->state;
> >>  		mode = &new_crtc_state->mode;
> >>  		adjusted_mode = &new_crtc_state->adjusted_mode;
> >>@@ -964,6 +969,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >>  		encoder = connector->state->best_encoder;
> >>  		funcs = encoder->helper_private;
> >>+		if (!funcs)
> >>+			continue;
> >>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
> >>  				 encoder->base.id, encoder->name);
> >>@@ -976,7 +983,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >>  		if (funcs->enable)
> >>  			funcs->enable(encoder);
> >>-		else
> >>+		else if (funcs->commit)
> >>  			funcs->commit(encoder);
> >>  		drm_bridge_enable(encoder->bridge);
> >>diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >>index 66ca313..6e6ab38 100644
> >>--- a/drivers/gpu/drm/drm_crtc_helper.c
> >>+++ b/drivers/gpu/drm/drm_crtc_helper.c
> >>@@ -170,11 +170,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
> >>  {
> >>  	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
> >>+	if (!encoder_funcs)
> >>+		return;
> >>+
> >>  	drm_bridge_disable(encoder->bridge);
> >>  	if (encoder_funcs->disable)
> >>  		(*encoder_funcs->disable)(encoder);
> >>-	else
> >>+	else if (encoder_funcs->dpms)
> >>  		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
> >>  	drm_bridge_post_disable(encoder->bridge);
> >>@@ -248,6 +251,9 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
> >>  	drm_for_each_encoder(encoder, dev) {
> >>  		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		/* Disable unused encoders */
> >>  		if (encoder->crtc == NULL)
> >>  			drm_encoder_disable(encoder);
> >>@@ -326,6 +332,10 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		ret = drm_bridge_mode_fixup(encoder->bridge,
> >>  			mode, adjusted_mode);
> >>  		if (!ret) {
> >>@@ -360,11 +370,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		drm_bridge_disable(encoder->bridge);
> >>-		encoder_funcs = encoder->helper_private;
> >>  		/* Disable the encoders as the first thing we do. */
> >>-		encoder_funcs->prepare(encoder);
> >>+		if (encoder_funcs->prepare)
> >>+			encoder_funcs->prepare(encoder);
> >>  		drm_bridge_post_disable(encoder->bridge);
> >>  	}
> >>@@ -385,11 +399,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] set [MODE:%d:%s]\n",
> >>  			encoder->base.id, encoder->name,
> >>  			mode->base.id, mode->name);
> >>-		encoder_funcs = encoder->helper_private;
> >>-		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >>+		if (encoder_funcs->mode_set)
> >>+			encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> >>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> >>  	}
> >>@@ -402,10 +420,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> >>  		if (encoder->crtc != crtc)
> >>  			continue;
> >>+		encoder_funcs = encoder->helper_private;
> >>+		if (!encoder_funcs)
> >>+			continue;
> >>+
> >>  		drm_bridge_pre_enable(encoder->bridge);
> >>-		encoder_funcs = encoder->helper_private;
> >>-		encoder_funcs->commit(encoder);
> >>+		if (encoder_funcs->commit)
> >>+			encoder_funcs->commit(encoder);
> >>  		drm_bridge_enable(encoder->bridge);
> >>  	}
> >>@@ -771,12 +793,15 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> >>  	struct drm_bridge *bridge = encoder->bridge;
> >>  	const struct drm_encoder_helper_funcs *encoder_funcs;
> >>+	encoder_funcs = encoder->helper_private;
> >>+	if (!encoder_funcs)
> >>+		return;
> >>+
> >>  	if (mode == DRM_MODE_DPMS_ON)
> >>  		drm_bridge_pre_enable(bridge);
> >>  	else
> >>  		drm_bridge_disable(bridge);
> >>-	encoder_funcs = encoder->helper_private;
> >>  	if (encoder_funcs->dpms)
> >>  		encoder_funcs->dpms(encoder, mode);
> >>-- 
> >>2.2.2
> >>
> 

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

Re: [PATCH 3/4] drm: Add helper for simple display pipeline

[Daniel Vetter]

On Mon, May 09, 2016 at 08:37:39PM +0200, Noralf Trønnes wrote:
> 
> Den 09.05.2016 16:46, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:33PM +0200, Noralf Trønnes wrote:
> >>Provides helper functions for drivers that have a simple display
> >>pipeline. Plane, crtc and encoder are collapsed into one entity.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>+static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> >>+					struct drm_plane_state *pstate)
> >>+{
> >>+	struct drm_simple_display_pipe *pipe;
> >>+	struct drm_crtc_state *cstate;
> >>+
> >>+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >>+	if (!pipe->funcs || !pipe->funcs->check)
> >>+		return 0;
> >>+
> >>+	cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> >>+						    &pipe->crtc);
> >>+
> >>+	return pipe->funcs->check(pipe, pstate, cstate);
> >>+}
> >Ok one thing I've missed here is that for most drivers this is way too
> >simple a check function, which means we'll end up with tons of duplicated
> >code. Things which the drm core allows, but simple pipelines all don't
> >really cope with:
> >- plane scaling
> >- disabling the plane without the crtc (i.e. scan out black)
> >- plane not sized to fill the entire hactive/vactive
> >
> >There's a helper to do most of these checks for you -
> >drm_plane_helper_check_update. I think it'd be good to place a call for
> >that in here, before we call down into the driver's ->check callback. But
> >ofc before we return 0; we want these checks always done. And catch all
> >these things so that drivers never fall over this pitfall.
> 
> Does this resemble what you're after? I'm just guessing here.
> 
> static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>                     struct drm_plane_state *pstate)
> {
>     struct drm_rect src = {
>         .x1 = pstate->src_x,
>         .y1 = pstate->src_y,
>         .x2 = pstate->src_x + pstate->src_w,
>         .y2 = pstate->src_y + pstate->src_h,
>     };
>     struct drm_rect dest = {
>         .x1 = pstate->crtc_x,
>         .y1 = pstate->crtc_y,
>         .x2 = pstate->crtc_x + pstate->crtc_w,
>         .y2 = pstate->crtc_y + pstate->crtc_h,
>     };
>     struct drm_rect clip = { 0 };

Clip rect needs to be set to crtc_state->adjusted_mode.h/vdisplay, see
rockchip or armada. Otherwise you'll clip to nothing and always fail.

>     struct drm_simple_display_pipe *pipe;
>     struct drm_crtc_state *cstate;
>     bool visible;
>     int ret;
> 
>     pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>     clip.x2 = pipe->crtc.mode.hdisplay;
>     clip.y2 = pipe->crtc.mode.vdisplay;
>     ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
>                         &src, &dest, &clip,
>                         DRM_PLANE_HELPER_NO_SCALING,
>                         DRM_PLANE_HELPER_NO_SCALING,
>                         false, false, &visible);

can_update_disabled = true should work, Only caveat is that you might get
a call to update the primary plane when the display is turned off.
Probably best though if we handle that in the simple pipe driver too. Just
call drm_atomic_helper_commit_planes with active_only = true.

>     if (ret)
>         return ret;
> 
>     /* How to handle !visible, is it even possible? */

	if (!visible)
		return -EINVAL;

You can't, so need to reject it.
> 
>     if (!pipe->funcs || !pipe->funcs->check)
>         return 0;
> 
>     cstate = drm_atomic_get_existing_crtc_state(pstate->state,
>                             &pipe->crtc);
> 
>     return pipe->funcs->check(pipe, pstate, cstate);
> }

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

Re: [PATCH 3/4] drm: Add helper for simple display pipeline

[Daniel Vetter]

On Tue, May 10, 2016 at 8:59 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>     if (ret)
>>         return ret;
>>
>>     /* How to handle !visible, is it even possible? */
>
>         if (!visible)
>                 return -EINVAL;
>
> You can't, so need to reject it.

Ok, on further thought I think we need a bit more here. We not just
need to make sure the plane is visible and not scaled or positioned,
but also that the plane is only enabled iff the crtc is.

So even before you call anything else you need to have this check:

if (crtc_state->enable != !!plane_state->crtc)
        return -EINVAL; /* plane must match crtc enable state */

if (!crtc_state->enable)
        return 0; /* nothing to check when disabling or disabled
already, calling check helpers and driver callbacks might only result
in confusion in such a case. */

Then continue with the remaining check logic that you have already,
with my coments in the earlier mail addressed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch