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

[PATCH v4 0/3] 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 2 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.


Changes since v3:
- Add patch 'drm/fb-cma-helper: Use const for drm_framebuffer_funcs argument'
- drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  - funcs argument should be const
- drm: Add helper for simple display pipeline
  - (struct drm_simple_display_pipe *)->funcs should be const

Changes since v2:
- drm: Add helper for simple display pipeline
  - Drop Kconfig knob DRM_KMS_HELPER
  - Expand documentation

Changes since v1:
- Drop patch: drm/panel: Add helper for simple panel connector
- Add fb-helper and fb-cma-helper doc patches
- Add drm/atomic: Don't skip drm_bridge_*() calls if !drm_encoder_helper_funcs
- Add drm_atomic_helper_best_encoder()
- drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  - Expand docs
- drm: Add helper for simple display pipeline
  - Add DOC header and add to gpu.tmpl
  - Fix docs: @funcs is optional, "negative error code",
    "This hook is optional."
  - Add checks to drm_simple_kms_plane_atomic_check()


Noralf Trønnes (3):
  drm/fb-cma-helper: Use const for drm_framebuffer_funcs argument
  drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  drm: Add helper for simple display pipeline

 Documentation/DocBook/gpu.tmpl          |   6 +
 drivers/gpu/drm/Makefile                |   2 +-
 drivers/gpu/drm/drm_fb_cma_helper.c     |  35 ++++--
 drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
 include/drm/drm_fb_cma_helper.h         |   5 +-
 include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
 6 files changed, 340 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
 create mode 100644 include/drm/drm_simple_kms_helper.h

--
2.8.2

[PATCH v4 1/3] drm/fb-cma-helper: Use const for drm_framebuffer_funcs argument

[Noralf Trønnes]

drm_framebuffer_init() uses const for the drm_framebuffer_funcs
argument so use that on drm_fb_cma_alloc() and
drm_fbdev_cma_create_with_funcs() as well.

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

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 3165ac0..815c72f 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -133,7 +133,7 @@ static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
 static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
 	const struct drm_mode_fb_cmd2 *mode_cmd,
 	struct drm_gem_cma_object **obj,
-	unsigned int num_planes, struct drm_framebuffer_funcs *funcs)
+	unsigned int num_planes, const struct drm_framebuffer_funcs *funcs)
 {
 	struct drm_fb_cma *fb_cma;
 	int ret;
@@ -350,7 +350,7 @@ static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
  */
 int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
 	struct drm_fb_helper_surface_size *sizes,
-	struct drm_framebuffer_funcs *funcs)
+	const struct drm_framebuffer_funcs *funcs)
 {
 	struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index c6d9c9c..ac38a05 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -25,7 +25,7 @@ void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
 void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
 int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
 	struct drm_fb_helper_surface_size *sizes,
-	struct drm_framebuffer_funcs *funcs);
+	const struct drm_framebuffer_funcs *funcs);
 
 void drm_fb_cma_destroy(struct drm_framebuffer *fb);
 int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
-- 
2.8.2

[PATCH v4 2/3] 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.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Changes since v3:
- funcs argument should be const

Changes since v1:
- Expand docs

 drivers/gpu/drm/drm_fb_cma_helper.c | 31 +++++++++++++++++++++++++------
 include/drm/drm_fb_cma_helper.h     |  3 +++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 815c72f..2248c32 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -159,13 +159,17 @@ 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. Use drm_fb_cma_create() if you don't need to change
+ * &drm_framebuffer_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)
+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,
+	const struct drm_framebuffer_funcs *funcs)
 {
 	struct drm_fb_cma *fb_cma;
 	struct drm_gem_cma_object *objs[4];
@@ -202,7 +206,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;
@@ -215,6 +219,21 @@ 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. Use drm_fb_cma_create_with_funcs() if
+ * you need to set &drm_framebuffer_funcs ->dirty.
+ */
+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 ac38a05..fd0dde9 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,
+	const 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.8.2

Re: [PATCH v4 2/3] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Daniel Vetter]

On Thu, May 12, 2016 at 08:25:22PM +0200, Noralf Trønnes wrote:
> Add drm_fb_cma_create_with_funcs() for drivers that need to set the
> dirty() callback.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Merged the first 2 patches to drm-misc, thanks.
-Daniel

> ---
> 
> Changes since v3:
> - funcs argument should be const
> 
> Changes since v1:
> - Expand docs
> 
>  drivers/gpu/drm/drm_fb_cma_helper.c | 31 +++++++++++++++++++++++++------
>  include/drm/drm_fb_cma_helper.h     |  3 +++
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 815c72f..2248c32 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -159,13 +159,17 @@ 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. Use drm_fb_cma_create() if you don't need to change
> + * &drm_framebuffer_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)
> +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,
> +	const struct drm_framebuffer_funcs *funcs)
>  {
>  	struct drm_fb_cma *fb_cma;
>  	struct drm_gem_cma_object *objs[4];
> @@ -202,7 +206,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;
> @@ -215,6 +219,21 @@ 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. Use drm_fb_cma_create_with_funcs() if
> + * you need to set &drm_framebuffer_funcs ->dirty.
> + */
> +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 ac38a05..fd0dde9 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,
> +	const 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.8.2
> 

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

[PATCH v4 3/3] 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.

Cc: jsarha@ti.com
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Changes since v3:
- (struct drm_simple_display_pipe *)->funcs should be const

Changes since v2:
- Drop Kconfig knob DRM_KMS_HELPER
- Expand documentation

Changes since v1:
- Add DOC header and add to gpu.tmpl
- Fix docs: @funcs is optional, "negative error code",
  "This hook is optional."
- Add checks to drm_simple_kms_plane_atomic_check()

 Documentation/DocBook/gpu.tmpl          |   6 +
 drivers/gpu/drm/Makefile                |   2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
 include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
 4 files changed, 309 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
 create mode 100644 include/drm/drm_simple_kms_helper.h

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 4a0c599..cf3f5a8 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
 !Edrivers/gpu/drm/drm_panel.c
 !Pdrivers/gpu/drm/drm_panel.c drm panel
     </sect2>
+    <sect2>
+      <title>Simple KMS Helper Reference</title>
+!Iinclude/drm/drm_simple_kms_helper.h
+!Edrivers/gpu/drm/drm_simple_kms_helper.c
+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
+    </sect2>
   </sect1>

   <!-- Internals: kms properties -->
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2bd3e5a..31b85df5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o

 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
-		drm_kms_helper_common.o
+		drm_kms_helper_common.o drm_simple_kms_helper.o

 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
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..d45417a
--- /dev/null
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -0,0 +1,208 @@
+/*
+ * 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>
+
+/**
+ * DOC: overview
+ *
+ * This helper library provides helpers for drivers for simple display
+ * hardware.
+ *
+ * drm_simple_display_pipe_init() initializes a simple display pipeline
+ * which has only one full-screen scanout buffer feeding one output. The
+ * pipeline is represented by struct &drm_simple_display_pipe and binds
+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
+ * entity. Some flexibility for code reuse is provided through a separately
+ * allocated &drm_connector object and supporting optional &drm_bridge
+ * encoder drivers.
+ */
+
+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 *plane_state)
+{
+	struct drm_rect src = {
+		.x1 = plane_state->src_x,
+		.y1 = plane_state->src_y,
+		.x2 = plane_state->src_x + plane_state->src_w,
+		.y2 = plane_state->src_y + plane_state->src_h,
+	};
+	struct drm_rect dest = {
+		.x1 = plane_state->crtc_x,
+		.y1 = plane_state->crtc_y,
+		.x2 = plane_state->crtc_x + plane_state->crtc_w,
+		.y2 = plane_state->crtc_y + plane_state->crtc_h,
+	};
+	struct drm_rect clip = { 0 };
+	struct drm_simple_display_pipe *pipe;
+	struct drm_crtc_state *crtc_state;
+	bool visible;
+	int ret;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
+							&pipe->crtc);
+	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 */
+
+	clip.x2 = crtc_state->adjusted_mode.hdisplay;
+	clip.y2 = crtc_state->adjusted_mode.vdisplay;
+	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
+					    plane_state->fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, true, &visible);
+	if (ret)
+		return ret;
+
+	if (!visible)
+		return -EINVAL;
+
+	if (!pipe->funcs || !pipe->funcs->check)
+		return 0;
+
+	return pipe->funcs->check(pipe, plane_state, crtc_state);
+}
+
+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 (optional)
+ * @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.
+ * Teardown of a simple display pipe is all handled automatically by the drm
+ * core through calling drm_mode_config_cleanup(). Drivers afterwards need to
+ * release the memory for the structure themselves.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int drm_simple_display_pipe_init(struct drm_device *dev,
+			struct drm_simple_display_pipe *pipe,
+			const 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..2690397
--- /dev/null
+++ b/include/drm/drm_simple_kms_helper.h
@@ -0,0 +1,94 @@
+/*
+ * 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.
+	 * This hook is optional.
+	 */
+	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.
+	 * This hook is optional.
+	 */
+	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.
+	 * The simple display pipeline helpers already check that the plane is
+	 * not scaled, fills the entire visible area and is always enabled
+	 * when the crtc is also enabled.
+	 * This hook is optional.
+	 *
+	 * 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.
+	 * This hook is optional.
+	 */
+	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. It should be initialized by calling drm_simple_display_pipe_init().
+ */
+struct drm_simple_display_pipe {
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+	struct drm_encoder encoder;
+	struct drm_connector *connector;
+
+	const struct drm_simple_display_pipe_funcs *funcs;
+};
+
+int drm_simple_display_pipe_init(struct drm_device *dev,
+			struct drm_simple_display_pipe *pipe,
+			const 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.8.2

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

[Ville Syrjälä]

On Thu, May 12, 2016 at 08:25:23PM +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.
> 
> Cc: jsarha@ti.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> Changes since v3:
> - (struct drm_simple_display_pipe *)->funcs should be const
> 
> Changes since v2:
> - Drop Kconfig knob DRM_KMS_HELPER
> - Expand documentation
> 
> Changes since v1:
> - Add DOC header and add to gpu.tmpl
> - Fix docs: @funcs is optional, "negative error code",
>   "This hook is optional."
> - Add checks to drm_simple_kms_plane_atomic_check()
> 
>  Documentation/DocBook/gpu.tmpl          |   6 +
>  drivers/gpu/drm/Makefile                |   2 +-
>  drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>  4 files changed, 309 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>  create mode 100644 include/drm/drm_simple_kms_helper.h
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 4a0c599..cf3f5a8 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
>  !Edrivers/gpu/drm/drm_panel.c
>  !Pdrivers/gpu/drm/drm_panel.c drm panel
>      </sect2>
> +    <sect2>
> +      <title>Simple KMS Helper Reference</title>
> +!Iinclude/drm/drm_simple_kms_helper.h
> +!Edrivers/gpu/drm/drm_simple_kms_helper.c
> +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> +    </sect2>
>    </sect1>
> 
>    <!-- Internals: kms properties -->
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2bd3e5a..31b85df5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> 
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> -		drm_kms_helper_common.o
> +		drm_kms_helper_common.o drm_simple_kms_helper.o
> 
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> 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..d45417a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,208 @@
> +/*
> + * 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>
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides helpers for drivers for simple display
> + * hardware.
> + *
> + * drm_simple_display_pipe_init() initializes a simple display pipeline
> + * which has only one full-screen scanout buffer feeding one output. The
> + * pipeline is represented by struct &drm_simple_display_pipe and binds
> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> + * entity. Some flexibility for code reuse is provided through a separately
> + * allocated &drm_connector object and supporting optional &drm_bridge
> + * encoder drivers.
> + */
> +
> +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 *plane_state)
> +{
> +	struct drm_rect src = {
> +		.x1 = plane_state->src_x,
> +		.y1 = plane_state->src_y,
> +		.x2 = plane_state->src_x + plane_state->src_w,
> +		.y2 = plane_state->src_y + plane_state->src_h,
> +	};
> +	struct drm_rect dest = {
> +		.x1 = plane_state->crtc_x,
> +		.y1 = plane_state->crtc_y,
> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> +	};
> +	struct drm_rect clip = { 0 };
> +	struct drm_simple_display_pipe *pipe;
> +	struct drm_crtc_state *crtc_state;
> +	bool visible;
> +	int ret;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> +							&pipe->crtc);
> +	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 */
> +
> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> +					    plane_state->fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    false, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	if (!visible)
> +		return -EINVAL;
> +
> +	if (!pipe->funcs || !pipe->funcs->check)
> +		return 0;
> +
> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> +}

What's anyone supposed to do with this when the clipped coordinates
aren't even passed/stored anywhere?

> +
> +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 (optional)
> + * @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.
> + * Teardown of a simple display pipe is all handled automatically by the drm
> + * core through calling drm_mode_config_cleanup(). Drivers afterwards need to
> + * release the memory for the structure themselves.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +			struct drm_simple_display_pipe *pipe,
> +			const 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..2690397
> --- /dev/null
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -0,0 +1,94 @@
> +/*
> + * 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.
> +	 * This hook is optional.
> +	 */
> +	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.
> +	 * This hook is optional.
> +	 */
> +	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.
> +	 * The simple display pipeline helpers already check that the plane is
> +	 * not scaled, fills the entire visible area and is always enabled
> +	 * when the crtc is also enabled.
> +	 * This hook is optional.
> +	 *
> +	 * 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.
> +	 * This hook is optional.
> +	 */
> +	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. It should be initialized by calling drm_simple_display_pipe_init().
> + */
> +struct drm_simple_display_pipe {
> +	struct drm_crtc crtc;
> +	struct drm_plane plane;
> +	struct drm_encoder encoder;
> +	struct drm_connector *connector;
> +
> +	const struct drm_simple_display_pipe_funcs *funcs;
> +};
> +
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +			struct drm_simple_display_pipe *pipe,
> +			const 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.8.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> On Thu, May 12, 2016 at 08:25:23PM +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.
> > 
> > Cc: jsarha@ti.com
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> > 
> > Changes since v3:
> > - (struct drm_simple_display_pipe *)->funcs should be const
> > 
> > Changes since v2:
> > - Drop Kconfig knob DRM_KMS_HELPER
> > - Expand documentation
> > 
> > Changes since v1:
> > - Add DOC header and add to gpu.tmpl
> > - Fix docs: @funcs is optional, "negative error code",
> >   "This hook is optional."
> > - Add checks to drm_simple_kms_plane_atomic_check()
> > 
> >  Documentation/DocBook/gpu.tmpl          |   6 +
> >  drivers/gpu/drm/Makefile                |   2 +-
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> >  include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> >  4 files changed, 309 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> >  create mode 100644 include/drm/drm_simple_kms_helper.h
> > 
> > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > index 4a0c599..cf3f5a8 100644
> > --- a/Documentation/DocBook/gpu.tmpl
> > +++ b/Documentation/DocBook/gpu.tmpl
> > @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> >  !Edrivers/gpu/drm/drm_panel.c
> >  !Pdrivers/gpu/drm/drm_panel.c drm panel
> >      </sect2>
> > +    <sect2>
> > +      <title>Simple KMS Helper Reference</title>
> > +!Iinclude/drm/drm_simple_kms_helper.h
> > +!Edrivers/gpu/drm/drm_simple_kms_helper.c
> > +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> > +    </sect2>
> >    </sect1>
> > 
> >    <!-- Internals: kms properties -->
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 2bd3e5a..31b85df5 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> > 
> >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > -		drm_kms_helper_common.o
> > +		drm_kms_helper_common.o drm_simple_kms_helper.o
> > 
> >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > 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..d45417a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -0,0 +1,208 @@
> > +/*
> > + * 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>
> > +
> > +/**
> > + * DOC: overview
> > + *
> > + * This helper library provides helpers for drivers for simple display
> > + * hardware.
> > + *
> > + * drm_simple_display_pipe_init() initializes a simple display pipeline
> > + * which has only one full-screen scanout buffer feeding one output. The
> > + * pipeline is represented by struct &drm_simple_display_pipe and binds
> > + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> > + * entity. Some flexibility for code reuse is provided through a separately
> > + * allocated &drm_connector object and supporting optional &drm_bridge
> > + * encoder drivers.
> > + */
> > +
> > +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 *plane_state)
> > +{
> > +	struct drm_rect src = {
> > +		.x1 = plane_state->src_x,
> > +		.y1 = plane_state->src_y,
> > +		.x2 = plane_state->src_x + plane_state->src_w,
> > +		.y2 = plane_state->src_y + plane_state->src_h,
> > +	};
> > +	struct drm_rect dest = {
> > +		.x1 = plane_state->crtc_x,
> > +		.y1 = plane_state->crtc_y,
> > +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> > +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> > +	};
> > +	struct drm_rect clip = { 0 };
> > +	struct drm_simple_display_pipe *pipe;
> > +	struct drm_crtc_state *crtc_state;
> > +	bool visible;
> > +	int ret;
> > +
> > +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> > +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > +							&pipe->crtc);
> > +	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 */
> > +
> > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> > +					    plane_state->fb,
> > +					    &src, &dest, &clip,
> > +					    DRM_PLANE_HELPER_NO_SCALING,
> > +					    DRM_PLANE_HELPER_NO_SCALING,
> > +					    false, true, &visible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!visible)
> > +		return -EINVAL;
> > +
> > +	if (!pipe->funcs || !pipe->funcs->check)
> > +		return 0;
> > +
> > +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> > +}
> 
> What's anyone supposed to do with this when the clipped coordinates
> aren't even passed/stored anywhere?

It disallows positioning and scaling, so shouldn't ever need to have the
clipped area?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> > On Thu, May 12, 2016 at 08:25:23PM +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.
> > > 
> > > Cc: jsarha@ti.com
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > > 
> > > Changes since v3:
> > > - (struct drm_simple_display_pipe *)->funcs should be const
> > > 
> > > Changes since v2:
> > > - Drop Kconfig knob DRM_KMS_HELPER
> > > - Expand documentation
> > > 
> > > Changes since v1:
> > > - Add DOC header and add to gpu.tmpl
> > > - Fix docs: @funcs is optional, "negative error code",
> > >   "This hook is optional."
> > > - Add checks to drm_simple_kms_plane_atomic_check()
> > > 
> > >  Documentation/DocBook/gpu.tmpl          |   6 +
> > >  drivers/gpu/drm/Makefile                |   2 +-
> > >  drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> > >  include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> > >  4 files changed, 309 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> > >  create mode 100644 include/drm/drm_simple_kms_helper.h
> > > 
> > > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > > index 4a0c599..cf3f5a8 100644
> > > --- a/Documentation/DocBook/gpu.tmpl
> > > +++ b/Documentation/DocBook/gpu.tmpl
> > > @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> > >  !Edrivers/gpu/drm/drm_panel.c
> > >  !Pdrivers/gpu/drm/drm_panel.c drm panel
> > >      </sect2>
> > > +    <sect2>
> > > +      <title>Simple KMS Helper Reference</title>
> > > +!Iinclude/drm/drm_simple_kms_helper.h
> > > +!Edrivers/gpu/drm/drm_simple_kms_helper.c
> > > +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> > > +    </sect2>
> > >    </sect1>
> > > 
> > >    <!-- Internals: kms properties -->
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 2bd3e5a..31b85df5 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> > > 
> > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > > -		drm_kms_helper_common.o
> > > +		drm_kms_helper_common.o drm_simple_kms_helper.o
> > > 
> > >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > > 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..d45417a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > @@ -0,0 +1,208 @@
> > > +/*
> > > + * 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>
> > > +
> > > +/**
> > > + * DOC: overview
> > > + *
> > > + * This helper library provides helpers for drivers for simple display
> > > + * hardware.
> > > + *
> > > + * drm_simple_display_pipe_init() initializes a simple display pipeline
> > > + * which has only one full-screen scanout buffer feeding one output. The
> > > + * pipeline is represented by struct &drm_simple_display_pipe and binds
> > > + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> > > + * entity. Some flexibility for code reuse is provided through a separately
> > > + * allocated &drm_connector object and supporting optional &drm_bridge
> > > + * encoder drivers.
> > > + */
> > > +
> > > +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 *plane_state)
> > > +{
> > > +	struct drm_rect src = {
> > > +		.x1 = plane_state->src_x,
> > > +		.y1 = plane_state->src_y,
> > > +		.x2 = plane_state->src_x + plane_state->src_w,
> > > +		.y2 = plane_state->src_y + plane_state->src_h,
> > > +	};
> > > +	struct drm_rect dest = {
> > > +		.x1 = plane_state->crtc_x,
> > > +		.y1 = plane_state->crtc_y,
> > > +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> > > +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> > > +	};
> > > +	struct drm_rect clip = { 0 };
> > > +	struct drm_simple_display_pipe *pipe;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	bool visible;
> > > +	int ret;
> > > +
> > > +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> > > +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > > +							&pipe->crtc);
> > > +	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 */
> > > +
> > > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > > +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> > > +					    plane_state->fb,
> > > +					    &src, &dest, &clip,
> > > +					    DRM_PLANE_HELPER_NO_SCALING,
> > > +					    DRM_PLANE_HELPER_NO_SCALING,
> > > +					    false, true, &visible);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!visible)
> > > +		return -EINVAL;
> > > +
> > > +	if (!pipe->funcs || !pipe->funcs->check)
> > > +		return 0;
> > > +
> > > +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> > > +}
> > 
> > What's anyone supposed to do with this when the clipped coordinates
> > aren't even passed/stored anywhere?
> 
> It disallows positioning and scaling, so shouldn't ever need to have the
> clipped area?

You can still configure a larger area that gets clipped to the
fullscreen dimensions.

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> > On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> > > On Thu, May 12, 2016 at 08:25:23PM +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.
> > > > 
> > > > Cc: jsarha@ti.com
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > ---
> > > > 
> > > > Changes since v3:
> > > > - (struct drm_simple_display_pipe *)->funcs should be const
> > > > 
> > > > Changes since v2:
> > > > - Drop Kconfig knob DRM_KMS_HELPER
> > > > - Expand documentation
> > > > 
> > > > Changes since v1:
> > > > - Add DOC header and add to gpu.tmpl
> > > > - Fix docs: @funcs is optional, "negative error code",
> > > >   "This hook is optional."
> > > > - Add checks to drm_simple_kms_plane_atomic_check()
> > > > 
> > > >  Documentation/DocBook/gpu.tmpl          |   6 +
> > > >  drivers/gpu/drm/Makefile                |   2 +-
> > > >  drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> > > >  4 files changed, 309 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> > > >  create mode 100644 include/drm/drm_simple_kms_helper.h
> > > > 
> > > > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > > > index 4a0c599..cf3f5a8 100644
> > > > --- a/Documentation/DocBook/gpu.tmpl
> > > > +++ b/Documentation/DocBook/gpu.tmpl
> > > > @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> > > >  !Edrivers/gpu/drm/drm_panel.c
> > > >  !Pdrivers/gpu/drm/drm_panel.c drm panel
> > > >      </sect2>
> > > > +    <sect2>
> > > > +      <title>Simple KMS Helper Reference</title>
> > > > +!Iinclude/drm/drm_simple_kms_helper.h
> > > > +!Edrivers/gpu/drm/drm_simple_kms_helper.c
> > > > +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> > > > +    </sect2>
> > > >    </sect1>
> > > > 
> > > >    <!-- Internals: kms properties -->
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 2bd3e5a..31b85df5 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> > > > 
> > > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > > >  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > > > -		drm_kms_helper_common.o
> > > > +		drm_kms_helper_common.o drm_simple_kms_helper.o
> > > > 
> > > >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > > > 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..d45417a
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > @@ -0,0 +1,208 @@
> > > > +/*
> > > > + * 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>
> > > > +
> > > > +/**
> > > > + * DOC: overview
> > > > + *
> > > > + * This helper library provides helpers for drivers for simple display
> > > > + * hardware.
> > > > + *
> > > > + * drm_simple_display_pipe_init() initializes a simple display pipeline
> > > > + * which has only one full-screen scanout buffer feeding one output. The
> > > > + * pipeline is represented by struct &drm_simple_display_pipe and binds
> > > > + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> > > > + * entity. Some flexibility for code reuse is provided through a separately
> > > > + * allocated &drm_connector object and supporting optional &drm_bridge
> > > > + * encoder drivers.
> > > > + */
> > > > +
> > > > +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 *plane_state)
> > > > +{
> > > > +	struct drm_rect src = {
> > > > +		.x1 = plane_state->src_x,
> > > > +		.y1 = plane_state->src_y,
> > > > +		.x2 = plane_state->src_x + plane_state->src_w,
> > > > +		.y2 = plane_state->src_y + plane_state->src_h,
> > > > +	};
> > > > +	struct drm_rect dest = {
> > > > +		.x1 = plane_state->crtc_x,
> > > > +		.y1 = plane_state->crtc_y,
> > > > +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> > > > +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> > > > +	};
> > > > +	struct drm_rect clip = { 0 };
> > > > +	struct drm_simple_display_pipe *pipe;
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	bool visible;
> > > > +	int ret;
> > > > +
> > > > +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> > > > +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > > > +							&pipe->crtc);
> > > > +	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 */
> > > > +
> > > > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > > > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > > > +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> > > > +					    plane_state->fb,
> > > > +					    &src, &dest, &clip,
> > > > +					    DRM_PLANE_HELPER_NO_SCALING,
> > > > +					    DRM_PLANE_HELPER_NO_SCALING,
> > > > +					    false, true, &visible);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (!visible)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!pipe->funcs || !pipe->funcs->check)
> > > > +		return 0;
> > > > +
> > > > +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> > > > +}
> > > 
> > > What's anyone supposed to do with this when the clipped coordinates
> > > aren't even passed/stored anywhere?
> > 
> > It disallows positioning and scaling, so shouldn't ever need to have the
> > clipped area?
> 
> You can still configure a larger area that gets clipped to the
> fullscreen dimensions.

Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
Probably best if we add clipped plane coordinates to drm_plane_state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Noralf Trønnes]

Den 17.05.2016 09:59, skrev Daniel Vetter:
> On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
>> On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
>>> On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
>>>> On Thu, May 12, 2016 at 08:25:23PM +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.
>>>>>
>>>>> Cc: jsarha@ti.com
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>
>>>>> Changes since v3:
>>>>> - (struct drm_simple_display_pipe *)->funcs should be const
>>>>>
>>>>> Changes since v2:
>>>>> - Drop Kconfig knob DRM_KMS_HELPER
>>>>> - Expand documentation
>>>>>
>>>>> Changes since v1:
>>>>> - Add DOC header and add to gpu.tmpl
>>>>> - Fix docs: @funcs is optional, "negative error code",
>>>>>    "This hook is optional."
>>>>> - Add checks to drm_simple_kms_plane_atomic_check()
>>>>>
>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
>>>>>   drivers/gpu/drm/Makefile                |   2 +-
>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
>>>>>
>>>>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>>>>> index 4a0c599..cf3f5a8 100644
>>>>> --- a/Documentation/DocBook/gpu.tmpl
>>>>> +++ b/Documentation/DocBook/gpu.tmpl
>>>>> @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
>>>>>   !Edrivers/gpu/drm/drm_panel.c
>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
>>>>>       </sect2>
>>>>> +    <sect2>
>>>>> +      <title>Simple KMS Helper Reference</title>
>>>>> +!Iinclude/drm/drm_simple_kms_helper.h
>>>>> +!Edrivers/gpu/drm/drm_simple_kms_helper.c
>>>>> +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
>>>>> +    </sect2>
>>>>>     </sect1>
>>>>>
>>>>>     <!-- Internals: kms properties -->
>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>>> index 2bd3e5a..31b85df5 100644
>>>>> --- a/drivers/gpu/drm/Makefile
>>>>> +++ b/drivers/gpu/drm/Makefile
>>>>> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>>>>>
>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>>>>>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>>>>> -		drm_kms_helper_common.o
>>>>> +		drm_kms_helper_common.o drm_simple_kms_helper.o
>>>>>
>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>>>> 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..d45417a
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>>>> @@ -0,0 +1,208 @@
>>>>> +/*
>>>>> + * 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>
>>>>> +
>>>>> +/**
>>>>> + * DOC: overview
>>>>> + *
>>>>> + * This helper library provides helpers for drivers for simple display
>>>>> + * hardware.
>>>>> + *
>>>>> + * drm_simple_display_pipe_init() initializes a simple display pipeline
>>>>> + * which has only one full-screen scanout buffer feeding one output. The
>>>>> + * pipeline is represented by struct &drm_simple_display_pipe and binds
>>>>> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
>>>>> + * entity. Some flexibility for code reuse is provided through a separately
>>>>> + * allocated &drm_connector object and supporting optional &drm_bridge
>>>>> + * encoder drivers.
>>>>> + */
>>>>> +
>>>>> +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 *plane_state)
>>>>> +{
>>>>> +	struct drm_rect src = {
>>>>> +		.x1 = plane_state->src_x,
>>>>> +		.y1 = plane_state->src_y,
>>>>> +		.x2 = plane_state->src_x + plane_state->src_w,
>>>>> +		.y2 = plane_state->src_y + plane_state->src_h,
>>>>> +	};
>>>>> +	struct drm_rect dest = {
>>>>> +		.x1 = plane_state->crtc_x,
>>>>> +		.y1 = plane_state->crtc_y,
>>>>> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
>>>>> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
>>>>> +	};
>>>>> +	struct drm_rect clip = { 0 };
>>>>> +	struct drm_simple_display_pipe *pipe;
>>>>> +	struct drm_crtc_state *crtc_state;
>>>>> +	bool visible;
>>>>> +	int ret;
>>>>> +
>>>>> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>>>>> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
>>>>> +							&pipe->crtc);
>>>>> +	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 */
>>>>> +
>>>>> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
>>>>> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
>>>>> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
>>>>> +					    plane_state->fb,
>>>>> +					    &src, &dest, &clip,
>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>> +					    false, true, &visible);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (!visible)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!pipe->funcs || !pipe->funcs->check)
>>>>> +		return 0;
>>>>> +
>>>>> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
>>>>> +}
>>>> What's anyone supposed to do with this when the clipped coordinates
>>>> aren't even passed/stored anywhere?
>>> It disallows positioning and scaling, so shouldn't ever need to have the
>>> clipped area?
>> You can still configure a larger area that gets clipped to the
>> fullscreen dimensions.
> Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> Probably best if we add clipped plane coordinates to drm_plane_state.

Both src and crtc or only src?

      if (!visible)
          return -EINVAL;
+
+    plane_state->src_x = src.x1;
+    plane_state->src_y = src.y1;
+    plane_state->src_w = drm_rect_width(&src);
+    plane_state->src_h = drm_rect_height(&src);
+
+    plane_state->crtc_x = dest.x1;
+    plane_state->crtc_y = dest.y1;
+    plane_state->crtc_w = drm_rect_width(&dest);
+    plane_state->crtc_h = drm_rect_height(&dest);

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

Noralf.

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

[Ville Syrjälä]

On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
> 
> Den 17.05.2016 09:59, skrev Daniel Vetter:
> > On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> >> On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> >>> On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> >>>> On Thu, May 12, 2016 at 08:25:23PM +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.
> >>>>>
> >>>>> Cc: jsarha@ti.com
> >>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>> ---
> >>>>>
> >>>>> Changes since v3:
> >>>>> - (struct drm_simple_display_pipe *)->funcs should be const
> >>>>>
> >>>>> Changes since v2:
> >>>>> - Drop Kconfig knob DRM_KMS_HELPER
> >>>>> - Expand documentation
> >>>>>
> >>>>> Changes since v1:
> >>>>> - Add DOC header and add to gpu.tmpl
> >>>>> - Fix docs: @funcs is optional, "negative error code",
> >>>>>    "This hook is optional."
> >>>>> - Add checks to drm_simple_kms_plane_atomic_check()
> >>>>>
> >>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
> >>>>>   drivers/gpu/drm/Makefile                |   2 +-
> >>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> >>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> >>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
> >>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> >>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
> >>>>>
> >>>>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> >>>>> index 4a0c599..cf3f5a8 100644
> >>>>> --- a/Documentation/DocBook/gpu.tmpl
> >>>>> +++ b/Documentation/DocBook/gpu.tmpl
> >>>>> @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> >>>>>   !Edrivers/gpu/drm/drm_panel.c
> >>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
> >>>>>       </sect2>
> >>>>> +    <sect2>
> >>>>> +      <title>Simple KMS Helper Reference</title>
> >>>>> +!Iinclude/drm/drm_simple_kms_helper.h
> >>>>> +!Edrivers/gpu/drm/drm_simple_kms_helper.c
> >>>>> +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> >>>>> +    </sect2>
> >>>>>     </sect1>
> >>>>>
> >>>>>     <!-- Internals: kms properties -->
> >>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >>>>> index 2bd3e5a..31b85df5 100644
> >>>>> --- a/drivers/gpu/drm/Makefile
> >>>>> +++ b/drivers/gpu/drm/Makefile
> >>>>> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> >>>>>
> >>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >>>>>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >>>>> -		drm_kms_helper_common.o
> >>>>> +		drm_kms_helper_common.o drm_simple_kms_helper.o
> >>>>>
> >>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >>>>> 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..d45417a
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >>>>> @@ -0,0 +1,208 @@
> >>>>> +/*
> >>>>> + * 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>
> >>>>> +
> >>>>> +/**
> >>>>> + * DOC: overview
> >>>>> + *
> >>>>> + * This helper library provides helpers for drivers for simple display
> >>>>> + * hardware.
> >>>>> + *
> >>>>> + * drm_simple_display_pipe_init() initializes a simple display pipeline
> >>>>> + * which has only one full-screen scanout buffer feeding one output. The
> >>>>> + * pipeline is represented by struct &drm_simple_display_pipe and binds
> >>>>> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> >>>>> + * entity. Some flexibility for code reuse is provided through a separately
> >>>>> + * allocated &drm_connector object and supporting optional &drm_bridge
> >>>>> + * encoder drivers.
> >>>>> + */
> >>>>> +
> >>>>> +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 *plane_state)
> >>>>> +{
> >>>>> +	struct drm_rect src = {
> >>>>> +		.x1 = plane_state->src_x,
> >>>>> +		.y1 = plane_state->src_y,
> >>>>> +		.x2 = plane_state->src_x + plane_state->src_w,
> >>>>> +		.y2 = plane_state->src_y + plane_state->src_h,
> >>>>> +	};
> >>>>> +	struct drm_rect dest = {
> >>>>> +		.x1 = plane_state->crtc_x,
> >>>>> +		.y1 = plane_state->crtc_y,
> >>>>> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> >>>>> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> >>>>> +	};
> >>>>> +	struct drm_rect clip = { 0 };
> >>>>> +	struct drm_simple_display_pipe *pipe;
> >>>>> +	struct drm_crtc_state *crtc_state;
> >>>>> +	bool visible;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >>>>> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> >>>>> +							&pipe->crtc);
> >>>>> +	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 */
> >>>>> +
> >>>>> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> >>>>> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> >>>>> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> >>>>> +					    plane_state->fb,
> >>>>> +					    &src, &dest, &clip,
> >>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
> >>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
> >>>>> +					    false, true, &visible);
> >>>>> +	if (ret)
> >>>>> +		return ret;
> >>>>> +
> >>>>> +	if (!visible)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	if (!pipe->funcs || !pipe->funcs->check)
> >>>>> +		return 0;
> >>>>> +
> >>>>> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> >>>>> +}
> >>>> What's anyone supposed to do with this when the clipped coordinates
> >>>> aren't even passed/stored anywhere?
> >>> It disallows positioning and scaling, so shouldn't ever need to have the
> >>> clipped area?
> >> You can still configure a larger area that gets clipped to the
> >> fullscreen dimensions.
> > Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> > Probably best if we add clipped plane coordinates to drm_plane_state.
> 
> Both src and crtc or only src?
> 
>       if (!visible)
>           return -EINVAL;
> +
> +    plane_state->src_x = src.x1;
> +    plane_state->src_y = src.y1;
> +    plane_state->src_w = drm_rect_width(&src);
> +    plane_state->src_h = drm_rect_height(&src);
> +
> +    plane_state->crtc_x = dest.x1;
> +    plane_state->crtc_y = dest.y1;
> +    plane_state->crtc_w = drm_rect_width(&dest);
> +    plane_state->crtc_h = drm_rect_height(&dest);

You aren't allowed clobber the user provided coordinates like this.
What you need to do is store the clipped coordinates in the plane
state in addition to the user coordinates.

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

-- 
Ville Syrjälä
Intel OTC

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

[Noralf Trønnes]

Den 17.05.2016 14:12, skrev Ville Syrjälä:
> On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
>> Den 17.05.2016 09:59, skrev Daniel Vetter:
>>> On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
>>>> On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
>>>>> On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
>>>>>> On Thu, May 12, 2016 at 08:25:23PM +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.
>>>>>>>
>>>>>>> Cc: jsarha@ti.com
>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes since v3:
>>>>>>> - (struct drm_simple_display_pipe *)->funcs should be const
>>>>>>>
>>>>>>> Changes since v2:
>>>>>>> - Drop Kconfig knob DRM_KMS_HELPER
>>>>>>> - Expand documentation
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>> - Add DOC header and add to gpu.tmpl
>>>>>>> - Fix docs: @funcs is optional, "negative error code",
>>>>>>>     "This hook is optional."
>>>>>>> - Add checks to drm_simple_kms_plane_atomic_check()
>>>>>>>
>>>>>>>    Documentation/DocBook/gpu.tmpl          |   6 +
>>>>>>>    drivers/gpu/drm/Makefile                |   2 +-
>>>>>>>    drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
>>>>>>>    include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>>>>>>>    4 files changed, 309 insertions(+), 1 deletion(-)
>>>>>>>    create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>>>>>>>    create mode 100644 include/drm/drm_simple_kms_helper.h
>>>>>>>
>>>>>>> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>>>>>>> index 4a0c599..cf3f5a8 100644
>>>>>>> --- a/Documentation/DocBook/gpu.tmpl
>>>>>>> +++ b/Documentation/DocBook/gpu.tmpl
>>>>>>> @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
>>>>>>>    !Edrivers/gpu/drm/drm_panel.c
>>>>>>>    !Pdrivers/gpu/drm/drm_panel.c drm panel
>>>>>>>        </sect2>
>>>>>>> +    <sect2>
>>>>>>> +      <title>Simple KMS Helper Reference</title>
>>>>>>> +!Iinclude/drm/drm_simple_kms_helper.h
>>>>>>> +!Edrivers/gpu/drm/drm_simple_kms_helper.c
>>>>>>> +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
>>>>>>> +    </sect2>
>>>>>>>      </sect1>
>>>>>>>
>>>>>>>      <!-- Internals: kms properties -->
>>>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>>>>> index 2bd3e5a..31b85df5 100644
>>>>>>> --- a/drivers/gpu/drm/Makefile
>>>>>>> +++ b/drivers/gpu/drm/Makefile
>>>>>>> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>>>>>>>
>>>>>>>    drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>>>>>>>    		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>>>>>>> -		drm_kms_helper_common.o
>>>>>>> +		drm_kms_helper_common.o drm_simple_kms_helper.o
>>>>>>>
>>>>>>>    drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>>>>>>    drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>>>>>> 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..d45417a
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>>>>>> @@ -0,0 +1,208 @@
>>>>>>> +/*
>>>>>>> + * 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>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * DOC: overview
>>>>>>> + *
>>>>>>> + * This helper library provides helpers for drivers for simple display
>>>>>>> + * hardware.
>>>>>>> + *
>>>>>>> + * drm_simple_display_pipe_init() initializes a simple display pipeline
>>>>>>> + * which has only one full-screen scanout buffer feeding one output. The
>>>>>>> + * pipeline is represented by struct &drm_simple_display_pipe and binds
>>>>>>> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
>>>>>>> + * entity. Some flexibility for code reuse is provided through a separately
>>>>>>> + * allocated &drm_connector object and supporting optional &drm_bridge
>>>>>>> + * encoder drivers.
>>>>>>> + */
>>>>>>> +
>>>>>>> +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 *plane_state)
>>>>>>> +{
>>>>>>> +	struct drm_rect src = {
>>>>>>> +		.x1 = plane_state->src_x,
>>>>>>> +		.y1 = plane_state->src_y,
>>>>>>> +		.x2 = plane_state->src_x + plane_state->src_w,
>>>>>>> +		.y2 = plane_state->src_y + plane_state->src_h,
>>>>>>> +	};
>>>>>>> +	struct drm_rect dest = {
>>>>>>> +		.x1 = plane_state->crtc_x,
>>>>>>> +		.y1 = plane_state->crtc_y,
>>>>>>> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
>>>>>>> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
>>>>>>> +	};
>>>>>>> +	struct drm_rect clip = { 0 };
>>>>>>> +	struct drm_simple_display_pipe *pipe;
>>>>>>> +	struct drm_crtc_state *crtc_state;
>>>>>>> +	bool visible;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>>>>>>> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
>>>>>>> +							&pipe->crtc);
>>>>>>> +	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 */
>>>>>>> +
>>>>>>> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
>>>>>>> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
>>>>>>> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
>>>>>>> +					    plane_state->fb,
>>>>>>> +					    &src, &dest, &clip,
>>>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>>>> +					    DRM_PLANE_HELPER_NO_SCALING,
>>>>>>> +					    false, true, &visible);
>>>>>>> +	if (ret)
>>>>>>> +		return ret;
>>>>>>> +
>>>>>>> +	if (!visible)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	if (!pipe->funcs || !pipe->funcs->check)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
>>>>>>> +}
>>>>>> What's anyone supposed to do with this when the clipped coordinates
>>>>>> aren't even passed/stored anywhere?
>>>>> It disallows positioning and scaling, so shouldn't ever need to have the
>>>>> clipped area?
>>>> You can still configure a larger area that gets clipped to the
>>>> fullscreen dimensions.
>>> Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
>>> Probably best if we add clipped plane coordinates to drm_plane_state.
>> Both src and crtc or only src?
>>
>>        if (!visible)
>>            return -EINVAL;
>> +
>> +    plane_state->src_x = src.x1;
>> +    plane_state->src_y = src.y1;
>> +    plane_state->src_w = drm_rect_width(&src);
>> +    plane_state->src_h = drm_rect_height(&src);
>> +
>> +    plane_state->crtc_x = dest.x1;
>> +    plane_state->crtc_y = dest.y1;
>> +    plane_state->crtc_w = drm_rect_width(&dest);
>> +    plane_state->crtc_h = drm_rect_height(&dest);
> You aren't allowed clobber the user provided coordinates like this.
> What you need to do is store the clipped coordinates in the plane
> state in addition to the user coordinates.

How do I do that?

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

[Daniel Vetter]

On Tue, May 17, 2016 at 02:22:26PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 17.05.2016 14:12, skrev Ville Syrjälä:
> >On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
> >>Den 17.05.2016 09:59, skrev Daniel Vetter:
> >>>On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> >>>>On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> >>>>>On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> >>>>>>On Thu, May 12, 2016 at 08:25:23PM +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.
> >>>>>>>
> >>>>>>>Cc: jsarha@ti.com
> >>>>>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>>>>>---
> >>>>>>>
> >>>>>>>Changes since v3:
> >>>>>>>- (struct drm_simple_display_pipe *)->funcs should be const
> >>>>>>>
> >>>>>>>Changes since v2:
> >>>>>>>- Drop Kconfig knob DRM_KMS_HELPER
> >>>>>>>- Expand documentation
> >>>>>>>
> >>>>>>>Changes since v1:
> >>>>>>>- Add DOC header and add to gpu.tmpl
> >>>>>>>- Fix docs: @funcs is optional, "negative error code",
> >>>>>>>    "This hook is optional."
> >>>>>>>- Add checks to drm_simple_kms_plane_atomic_check()
> >>>>>>>
> >>>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
> >>>>>>>   drivers/gpu/drm/Makefile                |   2 +-
> >>>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> >>>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> >>>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
> >>>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> >>>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
> >>>>>>>
> >>>>>>>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> >>>>>>>index 4a0c599..cf3f5a8 100644
> >>>>>>>--- a/Documentation/DocBook/gpu.tmpl
> >>>>>>>+++ b/Documentation/DocBook/gpu.tmpl
> >>>>>>>@@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> >>>>>>>   !Edrivers/gpu/drm/drm_panel.c
> >>>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
> >>>>>>>       </sect2>
> >>>>>>>+    <sect2>
> >>>>>>>+      <title>Simple KMS Helper Reference</title>
> >>>>>>>+!Iinclude/drm/drm_simple_kms_helper.h
> >>>>>>>+!Edrivers/gpu/drm/drm_simple_kms_helper.c
> >>>>>>>+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> >>>>>>>+    </sect2>
> >>>>>>>     </sect1>
> >>>>>>>
> >>>>>>>     <!-- Internals: kms properties -->
> >>>>>>>diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >>>>>>>index 2bd3e5a..31b85df5 100644
> >>>>>>>--- a/drivers/gpu/drm/Makefile
> >>>>>>>+++ b/drivers/gpu/drm/Makefile
> >>>>>>>@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> >>>>>>>
> >>>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >>>>>>>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >>>>>>>-		drm_kms_helper_common.o
> >>>>>>>+		drm_kms_helper_common.o drm_simple_kms_helper.o
> >>>>>>>
> >>>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >>>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >>>>>>>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..d45417a
> >>>>>>>--- /dev/null
> >>>>>>>+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >>>>>>>@@ -0,0 +1,208 @@
> >>>>>>>+/*
> >>>>>>>+ * 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>
> >>>>>>>+
> >>>>>>>+/**
> >>>>>>>+ * DOC: overview
> >>>>>>>+ *
> >>>>>>>+ * This helper library provides helpers for drivers for simple display
> >>>>>>>+ * hardware.
> >>>>>>>+ *
> >>>>>>>+ * drm_simple_display_pipe_init() initializes a simple display pipeline
> >>>>>>>+ * which has only one full-screen scanout buffer feeding one output. The
> >>>>>>>+ * pipeline is represented by struct &drm_simple_display_pipe and binds
> >>>>>>>+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> >>>>>>>+ * entity. Some flexibility for code reuse is provided through a separately
> >>>>>>>+ * allocated &drm_connector object and supporting optional &drm_bridge
> >>>>>>>+ * encoder drivers.
> >>>>>>>+ */
> >>>>>>>+
> >>>>>>>+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 *plane_state)
> >>>>>>>+{
> >>>>>>>+	struct drm_rect src = {
> >>>>>>>+		.x1 = plane_state->src_x,
> >>>>>>>+		.y1 = plane_state->src_y,
> >>>>>>>+		.x2 = plane_state->src_x + plane_state->src_w,
> >>>>>>>+		.y2 = plane_state->src_y + plane_state->src_h,
> >>>>>>>+	};
> >>>>>>>+	struct drm_rect dest = {
> >>>>>>>+		.x1 = plane_state->crtc_x,
> >>>>>>>+		.y1 = plane_state->crtc_y,
> >>>>>>>+		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> >>>>>>>+		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> >>>>>>>+	};
> >>>>>>>+	struct drm_rect clip = { 0 };
> >>>>>>>+	struct drm_simple_display_pipe *pipe;
> >>>>>>>+	struct drm_crtc_state *crtc_state;
> >>>>>>>+	bool visible;
> >>>>>>>+	int ret;
> >>>>>>>+
> >>>>>>>+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >>>>>>>+	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> >>>>>>>+							&pipe->crtc);
> >>>>>>>+	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 */
> >>>>>>>+
> >>>>>>>+	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> >>>>>>>+	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> >>>>>>>+	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> >>>>>>>+					    plane_state->fb,
> >>>>>>>+					    &src, &dest, &clip,
> >>>>>>>+					    DRM_PLANE_HELPER_NO_SCALING,
> >>>>>>>+					    DRM_PLANE_HELPER_NO_SCALING,
> >>>>>>>+					    false, true, &visible);
> >>>>>>>+	if (ret)
> >>>>>>>+		return ret;
> >>>>>>>+
> >>>>>>>+	if (!visible)
> >>>>>>>+		return -EINVAL;
> >>>>>>>+
> >>>>>>>+	if (!pipe->funcs || !pipe->funcs->check)
> >>>>>>>+		return 0;
> >>>>>>>+
> >>>>>>>+	return pipe->funcs->check(pipe, plane_state, crtc_state);
> >>>>>>>+}
> >>>>>>What's anyone supposed to do with this when the clipped coordinates
> >>>>>>aren't even passed/stored anywhere?
> >>>>>It disallows positioning and scaling, so shouldn't ever need to have the
> >>>>>clipped area?
> >>>>You can still configure a larger area that gets clipped to the
> >>>>fullscreen dimensions.
> >>>Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> >>>Probably best if we add clipped plane coordinates to drm_plane_state.
> >>Both src and crtc or only src?
> >>
> >>       if (!visible)
> >>           return -EINVAL;
> >>+
> >>+    plane_state->src_x = src.x1;
> >>+    plane_state->src_y = src.y1;
> >>+    plane_state->src_w = drm_rect_width(&src);
> >>+    plane_state->src_h = drm_rect_height(&src);
> >>+
> >>+    plane_state->crtc_x = dest.x1;
> >>+    plane_state->crtc_y = dest.y1;
> >>+    plane_state->crtc_w = drm_rect_width(&dest);
> >>+    plane_state->crtc_h = drm_rect_height(&dest);
> >You aren't allowed clobber the user provided coordinates like this.
> >What you need to do is store the clipped coordinates in the plane
> >state in addition to the user coordinates.
> 
> How do I do that?

Add new set of plane_state->clipped_src/dst_x/y/h/w I think, and suggest
to drivers to use that if they need clipped coordinates. I think at least,
all these clip rects are a bit too confusing to me. Ville?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Tue, May 17, 2016 at 03:04:52PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 02:22:26PM +0200, Noralf Trønnes wrote:
> > 
> > 
> > Den 17.05.2016 14:12, skrev Ville Syrjälä:
> > >On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
> > >>Den 17.05.2016 09:59, skrev Daniel Vetter:
> > >>>On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> > >>>>On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> > >>>>>On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> > >>>>>>On Thu, May 12, 2016 at 08:25:23PM +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.
> > >>>>>>>
> > >>>>>>>Cc: jsarha@ti.com
> > >>>>>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > >>>>>>>---
> > >>>>>>>
> > >>>>>>>Changes since v3:
> > >>>>>>>- (struct drm_simple_display_pipe *)->funcs should be const
> > >>>>>>>
> > >>>>>>>Changes since v2:
> > >>>>>>>- Drop Kconfig knob DRM_KMS_HELPER
> > >>>>>>>- Expand documentation
> > >>>>>>>
> > >>>>>>>Changes since v1:
> > >>>>>>>- Add DOC header and add to gpu.tmpl
> > >>>>>>>- Fix docs: @funcs is optional, "negative error code",
> > >>>>>>>    "This hook is optional."
> > >>>>>>>- Add checks to drm_simple_kms_plane_atomic_check()
> > >>>>>>>
> > >>>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
> > >>>>>>>   drivers/gpu/drm/Makefile                |   2 +-
> > >>>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> > >>>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> > >>>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
> > >>>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> > >>>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
> > >>>>>>>
> > >>>>>>>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> > >>>>>>>index 4a0c599..cf3f5a8 100644
> > >>>>>>>--- a/Documentation/DocBook/gpu.tmpl
> > >>>>>>>+++ b/Documentation/DocBook/gpu.tmpl
> > >>>>>>>@@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> > >>>>>>>   !Edrivers/gpu/drm/drm_panel.c
> > >>>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
> > >>>>>>>       </sect2>
> > >>>>>>>+    <sect2>
> > >>>>>>>+      <title>Simple KMS Helper Reference</title>
> > >>>>>>>+!Iinclude/drm/drm_simple_kms_helper.h
> > >>>>>>>+!Edrivers/gpu/drm/drm_simple_kms_helper.c
> > >>>>>>>+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> > >>>>>>>+    </sect2>
> > >>>>>>>     </sect1>
> > >>>>>>>
> > >>>>>>>     <!-- Internals: kms properties -->
> > >>>>>>>diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > >>>>>>>index 2bd3e5a..31b85df5 100644
> > >>>>>>>--- a/drivers/gpu/drm/Makefile
> > >>>>>>>+++ b/drivers/gpu/drm/Makefile
> > >>>>>>>@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> > >>>>>>>
> > >>>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > >>>>>>>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > >>>>>>>-		drm_kms_helper_common.o
> > >>>>>>>+		drm_kms_helper_common.o drm_simple_kms_helper.o
> > >>>>>>>
> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > >>>>>>>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..d45417a
> > >>>>>>>--- /dev/null
> > >>>>>>>+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > >>>>>>>@@ -0,0 +1,208 @@
> > >>>>>>>+/*
> > >>>>>>>+ * 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>
> > >>>>>>>+
> > >>>>>>>+/**
> > >>>>>>>+ * DOC: overview
> > >>>>>>>+ *
> > >>>>>>>+ * This helper library provides helpers for drivers for simple display
> > >>>>>>>+ * hardware.
> > >>>>>>>+ *
> > >>>>>>>+ * drm_simple_display_pipe_init() initializes a simple display pipeline
> > >>>>>>>+ * which has only one full-screen scanout buffer feeding one output. The
> > >>>>>>>+ * pipeline is represented by struct &drm_simple_display_pipe and binds
> > >>>>>>>+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> > >>>>>>>+ * entity. Some flexibility for code reuse is provided through a separately
> > >>>>>>>+ * allocated &drm_connector object and supporting optional &drm_bridge
> > >>>>>>>+ * encoder drivers.
> > >>>>>>>+ */
> > >>>>>>>+
> > >>>>>>>+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 *plane_state)
> > >>>>>>>+{
> > >>>>>>>+	struct drm_rect src = {
> > >>>>>>>+		.x1 = plane_state->src_x,
> > >>>>>>>+		.y1 = plane_state->src_y,
> > >>>>>>>+		.x2 = plane_state->src_x + plane_state->src_w,
> > >>>>>>>+		.y2 = plane_state->src_y + plane_state->src_h,
> > >>>>>>>+	};
> > >>>>>>>+	struct drm_rect dest = {
> > >>>>>>>+		.x1 = plane_state->crtc_x,
> > >>>>>>>+		.y1 = plane_state->crtc_y,
> > >>>>>>>+		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> > >>>>>>>+		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> > >>>>>>>+	};
> > >>>>>>>+	struct drm_rect clip = { 0 };
> > >>>>>>>+	struct drm_simple_display_pipe *pipe;
> > >>>>>>>+	struct drm_crtc_state *crtc_state;
> > >>>>>>>+	bool visible;
> > >>>>>>>+	int ret;
> > >>>>>>>+
> > >>>>>>>+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> > >>>>>>>+	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > >>>>>>>+							&pipe->crtc);
> > >>>>>>>+	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 */
> > >>>>>>>+
> > >>>>>>>+	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > >>>>>>>+	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > >>>>>>>+	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> > >>>>>>>+					    plane_state->fb,
> > >>>>>>>+					    &src, &dest, &clip,
> > >>>>>>>+					    DRM_PLANE_HELPER_NO_SCALING,
> > >>>>>>>+					    DRM_PLANE_HELPER_NO_SCALING,
> > >>>>>>>+					    false, true, &visible);
> > >>>>>>>+	if (ret)
> > >>>>>>>+		return ret;
> > >>>>>>>+
> > >>>>>>>+	if (!visible)
> > >>>>>>>+		return -EINVAL;
> > >>>>>>>+
> > >>>>>>>+	if (!pipe->funcs || !pipe->funcs->check)
> > >>>>>>>+		return 0;
> > >>>>>>>+
> > >>>>>>>+	return pipe->funcs->check(pipe, plane_state, crtc_state);
> > >>>>>>>+}
> > >>>>>>What's anyone supposed to do with this when the clipped coordinates
> > >>>>>>aren't even passed/stored anywhere?
> > >>>>>It disallows positioning and scaling, so shouldn't ever need to have the
> > >>>>>clipped area?
> > >>>>You can still configure a larger area that gets clipped to the
> > >>>>fullscreen dimensions.
> > >>>Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> > >>>Probably best if we add clipped plane coordinates to drm_plane_state.
> > >>Both src and crtc or only src?
> > >>
> > >>       if (!visible)
> > >>           return -EINVAL;
> > >>+
> > >>+    plane_state->src_x = src.x1;
> > >>+    plane_state->src_y = src.y1;
> > >>+    plane_state->src_w = drm_rect_width(&src);
> > >>+    plane_state->src_h = drm_rect_height(&src);
> > >>+
> > >>+    plane_state->crtc_x = dest.x1;
> > >>+    plane_state->crtc_y = dest.y1;
> > >>+    plane_state->crtc_w = drm_rect_width(&dest);
> > >>+    plane_state->crtc_h = drm_rect_height(&dest);
> > >You aren't allowed clobber the user provided coordinates like this.
> > >What you need to do is store the clipped coordinates in the plane
> > >state in addition to the user coordinates.
> > 
> > How do I do that?
> 
> Add new set of plane_state->clipped_src/dst_x/y/h/w I think, and suggest
> to drivers to use that if they need clipped coordinates. I think at least,
> all these clip rects are a bit too confusing to me. Ville?

Basically everyone should use the clipped coords. There must be
something very special going on if anyone wants to use the raw user
coords.

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

On Tue, May 17, 2016 at 3:14 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, May 17, 2016 at 03:04:52PM +0200, Daniel Vetter wrote:
>> On Tue, May 17, 2016 at 02:22:26PM +0200, Noralf Trønnes wrote:
>> >
>> >
>> > Den 17.05.2016 14:12, skrev Ville Syrjälä:
>> > >On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
>> > >>Den 17.05.2016 09:59, skrev Daniel Vetter:
>> > >>>On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
>> > >>>>On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
>> > >>>>>On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
>> > >>>>>>On Thu, May 12, 2016 at 08:25:23PM +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.
>> > >>>>>>>
>> > >>>>>>>Cc: jsarha@ti.com
>> > >>>>>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> > >>>>>>>---
>> > >>>>>>>
>> > >>>>>>>Changes since v3:
>> > >>>>>>>- (struct drm_simple_display_pipe *)->funcs should be const
>> > >>>>>>>
>> > >>>>>>>Changes since v2:
>> > >>>>>>>- Drop Kconfig knob DRM_KMS_HELPER
>> > >>>>>>>- Expand documentation
>> > >>>>>>>
>> > >>>>>>>Changes since v1:
>> > >>>>>>>- Add DOC header and add to gpu.tmpl
>> > >>>>>>>- Fix docs: @funcs is optional, "negative error code",
>> > >>>>>>>    "This hook is optional."
>> > >>>>>>>- Add checks to drm_simple_kms_plane_atomic_check()
>> > >>>>>>>
>> > >>>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
>> > >>>>>>>   drivers/gpu/drm/Makefile                |   2 +-
>> > >>>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
>> > >>>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>> > >>>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
>> > >>>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>> > >>>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
>> > >>>>>>>
>> > >>>>>>>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
>> > >>>>>>>index 4a0c599..cf3f5a8 100644
>> > >>>>>>>--- a/Documentation/DocBook/gpu.tmpl
>> > >>>>>>>+++ b/Documentation/DocBook/gpu.tmpl
>> > >>>>>>>@@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
>> > >>>>>>>   !Edrivers/gpu/drm/drm_panel.c
>> > >>>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
>> > >>>>>>>       </sect2>
>> > >>>>>>>+    <sect2>
>> > >>>>>>>+      <title>Simple KMS Helper Reference</title>
>> > >>>>>>>+!Iinclude/drm/drm_simple_kms_helper.h
>> > >>>>>>>+!Edrivers/gpu/drm/drm_simple_kms_helper.c
>> > >>>>>>>+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
>> > >>>>>>>+    </sect2>
>> > >>>>>>>     </sect1>
>> > >>>>>>>
>> > >>>>>>>     <!-- Internals: kms properties -->
>> > >>>>>>>diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> > >>>>>>>index 2bd3e5a..31b85df5 100644
>> > >>>>>>>--- a/drivers/gpu/drm/Makefile
>> > >>>>>>>+++ b/drivers/gpu/drm/Makefile
>> > >>>>>>>@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>> > >>>>>>>
>> > >>>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>> > >>>>>>>             drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>> > >>>>>>>-            drm_kms_helper_common.o
>> > >>>>>>>+            drm_kms_helper_common.o drm_simple_kms_helper.o
>> > >>>>>>>
>> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>> > >>>>>>>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..d45417a
>> > >>>>>>>--- /dev/null
>> > >>>>>>>+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>> > >>>>>>>@@ -0,0 +1,208 @@
>> > >>>>>>>+/*
>> > >>>>>>>+ * 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>
>> > >>>>>>>+
>> > >>>>>>>+/**
>> > >>>>>>>+ * DOC: overview
>> > >>>>>>>+ *
>> > >>>>>>>+ * This helper library provides helpers for drivers for simple display
>> > >>>>>>>+ * hardware.
>> > >>>>>>>+ *
>> > >>>>>>>+ * drm_simple_display_pipe_init() initializes a simple display pipeline
>> > >>>>>>>+ * which has only one full-screen scanout buffer feeding one output. The
>> > >>>>>>>+ * pipeline is represented by struct &drm_simple_display_pipe and binds
>> > >>>>>>>+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
>> > >>>>>>>+ * entity. Some flexibility for code reuse is provided through a separately
>> > >>>>>>>+ * allocated &drm_connector object and supporting optional &drm_bridge
>> > >>>>>>>+ * encoder drivers.
>> > >>>>>>>+ */
>> > >>>>>>>+
>> > >>>>>>>+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 *plane_state)
>> > >>>>>>>+{
>> > >>>>>>>+    struct drm_rect src = {
>> > >>>>>>>+            .x1 = plane_state->src_x,
>> > >>>>>>>+            .y1 = plane_state->src_y,
>> > >>>>>>>+            .x2 = plane_state->src_x + plane_state->src_w,
>> > >>>>>>>+            .y2 = plane_state->src_y + plane_state->src_h,
>> > >>>>>>>+    };
>> > >>>>>>>+    struct drm_rect dest = {
>> > >>>>>>>+            .x1 = plane_state->crtc_x,
>> > >>>>>>>+            .y1 = plane_state->crtc_y,
>> > >>>>>>>+            .x2 = plane_state->crtc_x + plane_state->crtc_w,
>> > >>>>>>>+            .y2 = plane_state->crtc_y + plane_state->crtc_h,
>> > >>>>>>>+    };
>> > >>>>>>>+    struct drm_rect clip = { 0 };
>> > >>>>>>>+    struct drm_simple_display_pipe *pipe;
>> > >>>>>>>+    struct drm_crtc_state *crtc_state;
>> > >>>>>>>+    bool visible;
>> > >>>>>>>+    int ret;
>> > >>>>>>>+
>> > >>>>>>>+    pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>> > >>>>>>>+    crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
>> > >>>>>>>+                                                    &pipe->crtc);
>> > >>>>>>>+    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 */
>> > >>>>>>>+
>> > >>>>>>>+    clip.x2 = crtc_state->adjusted_mode.hdisplay;
>> > >>>>>>>+    clip.y2 = crtc_state->adjusted_mode.vdisplay;
>> > >>>>>>>+    ret = drm_plane_helper_check_update(plane, &pipe->crtc,
>> > >>>>>>>+                                        plane_state->fb,
>> > >>>>>>>+                                        &src, &dest, &clip,
>> > >>>>>>>+                                        DRM_PLANE_HELPER_NO_SCALING,
>> > >>>>>>>+                                        DRM_PLANE_HELPER_NO_SCALING,
>> > >>>>>>>+                                        false, true, &visible);
>> > >>>>>>>+    if (ret)
>> > >>>>>>>+            return ret;
>> > >>>>>>>+
>> > >>>>>>>+    if (!visible)
>> > >>>>>>>+            return -EINVAL;
>> > >>>>>>>+
>> > >>>>>>>+    if (!pipe->funcs || !pipe->funcs->check)
>> > >>>>>>>+            return 0;
>> > >>>>>>>+
>> > >>>>>>>+    return pipe->funcs->check(pipe, plane_state, crtc_state);
>> > >>>>>>>+}
>> > >>>>>>What's anyone supposed to do with this when the clipped coordinates
>> > >>>>>>aren't even passed/stored anywhere?
>> > >>>>>It disallows positioning and scaling, so shouldn't ever need to have the
>> > >>>>>clipped area?
>> > >>>>You can still configure a larger area that gets clipped to the
>> > >>>>fullscreen dimensions.
>> > >>>Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
>> > >>>Probably best if we add clipped plane coordinates to drm_plane_state.
>> > >>Both src and crtc or only src?
>> > >>
>> > >>       if (!visible)
>> > >>           return -EINVAL;
>> > >>+
>> > >>+    plane_state->src_x = src.x1;
>> > >>+    plane_state->src_y = src.y1;
>> > >>+    plane_state->src_w = drm_rect_width(&src);
>> > >>+    plane_state->src_h = drm_rect_height(&src);
>> > >>+
>> > >>+    plane_state->crtc_x = dest.x1;
>> > >>+    plane_state->crtc_y = dest.y1;
>> > >>+    plane_state->crtc_w = drm_rect_width(&dest);
>> > >>+    plane_state->crtc_h = drm_rect_height(&dest);
>> > >You aren't allowed clobber the user provided coordinates like this.
>> > >What you need to do is store the clipped coordinates in the plane
>> > >state in addition to the user coordinates.
>> >
>> > How do I do that?
>>
>> Add new set of plane_state->clipped_src/dst_x/y/h/w I think, and suggest
>> to drivers to use that if they need clipped coordinates. I think at least,
>> all these clip rects are a bit too confusing to me. Ville?
>
> Basically everyone should use the clipped coords. There must be
> something very special going on if anyone wants to use the raw user
> coords.

Hm, maybe it would be better then to add raw_ versions, and clip to
src/dst in the atomic helpers for everyone? Or would that break some
drivers? This seems to get a bit out of hand ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Ville Syrjälä]

On Tue, May 17, 2016 at 03:19:20PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 3:14 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Tue, May 17, 2016 at 03:04:52PM +0200, Daniel Vetter wrote:
> >> On Tue, May 17, 2016 at 02:22:26PM +0200, Noralf Trønnes wrote:
> >> >
> >> >
> >> > Den 17.05.2016 14:12, skrev Ville Syrjälä:
> >> > >On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
> >> > >>Den 17.05.2016 09:59, skrev Daniel Vetter:
> >> > >>>On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> >> > >>>>On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> >> > >>>>>On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> >> > >>>>>>On Thu, May 12, 2016 at 08:25:23PM +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.
> >> > >>>>>>>
> >> > >>>>>>>Cc: jsarha@ti.com
> >> > >>>>>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> > >>>>>>>---
> >> > >>>>>>>
> >> > >>>>>>>Changes since v3:
> >> > >>>>>>>- (struct drm_simple_display_pipe *)->funcs should be const
> >> > >>>>>>>
> >> > >>>>>>>Changes since v2:
> >> > >>>>>>>- Drop Kconfig knob DRM_KMS_HELPER
> >> > >>>>>>>- Expand documentation
> >> > >>>>>>>
> >> > >>>>>>>Changes since v1:
> >> > >>>>>>>- Add DOC header and add to gpu.tmpl
> >> > >>>>>>>- Fix docs: @funcs is optional, "negative error code",
> >> > >>>>>>>    "This hook is optional."
> >> > >>>>>>>- Add checks to drm_simple_kms_plane_atomic_check()
> >> > >>>>>>>
> >> > >>>>>>>   Documentation/DocBook/gpu.tmpl          |   6 +
> >> > >>>>>>>   drivers/gpu/drm/Makefile                |   2 +-
> >> > >>>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> >> > >>>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> >> > >>>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
> >> > >>>>>>>   create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
> >> > >>>>>>>   create mode 100644 include/drm/drm_simple_kms_helper.h
> >> > >>>>>>>
> >> > >>>>>>>diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> >> > >>>>>>>index 4a0c599..cf3f5a8 100644
> >> > >>>>>>>--- a/Documentation/DocBook/gpu.tmpl
> >> > >>>>>>>+++ b/Documentation/DocBook/gpu.tmpl
> >> > >>>>>>>@@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev)
> >> > >>>>>>>   !Edrivers/gpu/drm/drm_panel.c
> >> > >>>>>>>   !Pdrivers/gpu/drm/drm_panel.c drm panel
> >> > >>>>>>>       </sect2>
> >> > >>>>>>>+    <sect2>
> >> > >>>>>>>+      <title>Simple KMS Helper Reference</title>
> >> > >>>>>>>+!Iinclude/drm/drm_simple_kms_helper.h
> >> > >>>>>>>+!Edrivers/gpu/drm/drm_simple_kms_helper.c
> >> > >>>>>>>+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview
> >> > >>>>>>>+    </sect2>
> >> > >>>>>>>     </sect1>
> >> > >>>>>>>
> >> > >>>>>>>     <!-- Internals: kms properties -->
> >> > >>>>>>>diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> > >>>>>>>index 2bd3e5a..31b85df5 100644
> >> > >>>>>>>--- a/drivers/gpu/drm/Makefile
> >> > >>>>>>>+++ b/drivers/gpu/drm/Makefile
> >> > >>>>>>>@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> >> > >>>>>>>
> >> > >>>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >> > >>>>>>>             drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >> > >>>>>>>-            drm_kms_helper_common.o
> >> > >>>>>>>+            drm_kms_helper_common.o drm_simple_kms_helper.o
> >> > >>>>>>>
> >> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >> > >>>>>>>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..d45417a
> >> > >>>>>>>--- /dev/null
> >> > >>>>>>>+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >> > >>>>>>>@@ -0,0 +1,208 @@
> >> > >>>>>>>+/*
> >> > >>>>>>>+ * 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>
> >> > >>>>>>>+
> >> > >>>>>>>+/**
> >> > >>>>>>>+ * DOC: overview
> >> > >>>>>>>+ *
> >> > >>>>>>>+ * This helper library provides helpers for drivers for simple display
> >> > >>>>>>>+ * hardware.
> >> > >>>>>>>+ *
> >> > >>>>>>>+ * drm_simple_display_pipe_init() initializes a simple display pipeline
> >> > >>>>>>>+ * which has only one full-screen scanout buffer feeding one output. The
> >> > >>>>>>>+ * pipeline is represented by struct &drm_simple_display_pipe and binds
> >> > >>>>>>>+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> >> > >>>>>>>+ * entity. Some flexibility for code reuse is provided through a separately
> >> > >>>>>>>+ * allocated &drm_connector object and supporting optional &drm_bridge
> >> > >>>>>>>+ * encoder drivers.
> >> > >>>>>>>+ */
> >> > >>>>>>>+
> >> > >>>>>>>+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 *plane_state)
> >> > >>>>>>>+{
> >> > >>>>>>>+    struct drm_rect src = {
> >> > >>>>>>>+            .x1 = plane_state->src_x,
> >> > >>>>>>>+            .y1 = plane_state->src_y,
> >> > >>>>>>>+            .x2 = plane_state->src_x + plane_state->src_w,
> >> > >>>>>>>+            .y2 = plane_state->src_y + plane_state->src_h,
> >> > >>>>>>>+    };
> >> > >>>>>>>+    struct drm_rect dest = {
> >> > >>>>>>>+            .x1 = plane_state->crtc_x,
> >> > >>>>>>>+            .y1 = plane_state->crtc_y,
> >> > >>>>>>>+            .x2 = plane_state->crtc_x + plane_state->crtc_w,
> >> > >>>>>>>+            .y2 = plane_state->crtc_y + plane_state->crtc_h,
> >> > >>>>>>>+    };
> >> > >>>>>>>+    struct drm_rect clip = { 0 };
> >> > >>>>>>>+    struct drm_simple_display_pipe *pipe;
> >> > >>>>>>>+    struct drm_crtc_state *crtc_state;
> >> > >>>>>>>+    bool visible;
> >> > >>>>>>>+    int ret;
> >> > >>>>>>>+
> >> > >>>>>>>+    pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >> > >>>>>>>+    crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> >> > >>>>>>>+                                                    &pipe->crtc);
> >> > >>>>>>>+    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 */
> >> > >>>>>>>+
> >> > >>>>>>>+    clip.x2 = crtc_state->adjusted_mode.hdisplay;
> >> > >>>>>>>+    clip.y2 = crtc_state->adjusted_mode.vdisplay;
> >> > >>>>>>>+    ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> >> > >>>>>>>+                                        plane_state->fb,
> >> > >>>>>>>+                                        &src, &dest, &clip,
> >> > >>>>>>>+                                        DRM_PLANE_HELPER_NO_SCALING,
> >> > >>>>>>>+                                        DRM_PLANE_HELPER_NO_SCALING,
> >> > >>>>>>>+                                        false, true, &visible);
> >> > >>>>>>>+    if (ret)
> >> > >>>>>>>+            return ret;
> >> > >>>>>>>+
> >> > >>>>>>>+    if (!visible)
> >> > >>>>>>>+            return -EINVAL;
> >> > >>>>>>>+
> >> > >>>>>>>+    if (!pipe->funcs || !pipe->funcs->check)
> >> > >>>>>>>+            return 0;
> >> > >>>>>>>+
> >> > >>>>>>>+    return pipe->funcs->check(pipe, plane_state, crtc_state);
> >> > >>>>>>>+}
> >> > >>>>>>What's anyone supposed to do with this when the clipped coordinates
> >> > >>>>>>aren't even passed/stored anywhere?
> >> > >>>>>It disallows positioning and scaling, so shouldn't ever need to have the
> >> > >>>>>clipped area?
> >> > >>>>You can still configure a larger area that gets clipped to the
> >> > >>>>fullscreen dimensions.
> >> > >>>Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> >> > >>>Probably best if we add clipped plane coordinates to drm_plane_state.
> >> > >>Both src and crtc or only src?
> >> > >>
> >> > >>       if (!visible)
> >> > >>           return -EINVAL;
> >> > >>+
> >> > >>+    plane_state->src_x = src.x1;
> >> > >>+    plane_state->src_y = src.y1;
> >> > >>+    plane_state->src_w = drm_rect_width(&src);
> >> > >>+    plane_state->src_h = drm_rect_height(&src);
> >> > >>+
> >> > >>+    plane_state->crtc_x = dest.x1;
> >> > >>+    plane_state->crtc_y = dest.y1;
> >> > >>+    plane_state->crtc_w = drm_rect_width(&dest);
> >> > >>+    plane_state->crtc_h = drm_rect_height(&dest);
> >> > >You aren't allowed clobber the user provided coordinates like this.
> >> > >What you need to do is store the clipped coordinates in the plane
> >> > >state in addition to the user coordinates.
> >> >
> >> > How do I do that?
> >>
> >> Add new set of plane_state->clipped_src/dst_x/y/h/w I think, and suggest
> >> to drivers to use that if they need clipped coordinates. I think at least,
> >> all these clip rects are a bit too confusing to me. Ville?
> >
> > Basically everyone should use the clipped coords. There must be
> > something very special going on if anyone wants to use the raw user
> > coords.
> 
> Hm, maybe it would be better then to add raw_ versions, and clip to
> src/dst in the atomic helpers for everyone? Or would that break some
> drivers? This seems to get a bit out of hand ...

I suppose we might be able to do the basic stuff in some common code.
The driver may need to adjust the coordinates a bit further to deal
with hardware specific limits and whatnot.

One issue is that I originally designed the rect stuff for nice
hardware that can eg. deal with sub-pixel coordinates. But there's
lots of not so nice hardware around and the current code may not be
the best fit there.

We should also come to some kind of decisions on what kind of
tolerances we allow in deviating from the user coordinates and
whanot.

-- 
Ville Syrjälä
Intel OTC

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

[Noralf Trønnes]

Den 12.05.2016 20:25, skrev Noralf Trønnes:
> Provides helper functions for drivers that have a simple display
> pipeline. Plane, crtc and encoder are collapsed into one entity.
>
> Cc: jsarha@ti.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

[...]

> 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..d45417a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,208 @@
> +/*
> + * 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>
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides helpers for drivers for simple display
> + * hardware.
> + *
> + * drm_simple_display_pipe_init() initializes a simple display pipeline
> + * which has only one full-screen scanout buffer feeding one output. The
> + * pipeline is represented by struct &drm_simple_display_pipe and binds
> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> + * entity. Some flexibility for code reuse is provided through a separately
> + * allocated &drm_connector object and supporting optional &drm_bridge
> + * encoder drivers.
> + */
> +
> +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 *plane_state)
> +{
> +	struct drm_rect src = {
> +		.x1 = plane_state->src_x,
> +		.y1 = plane_state->src_y,
> +		.x2 = plane_state->src_x + plane_state->src_w,
> +		.y2 = plane_state->src_y + plane_state->src_h,
> +	};
> +	struct drm_rect dest = {
> +		.x1 = plane_state->crtc_x,
> +		.y1 = plane_state->crtc_y,
> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> +	};
> +	struct drm_rect clip = { 0 };
> +	struct drm_simple_display_pipe *pipe;
> +	struct drm_crtc_state *crtc_state;
> +	bool visible;
> +	int ret;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> +							&pipe->crtc);
> +	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 */
> +
> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> +					    plane_state->fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    false, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	if (!visible)
> +		return -EINVAL;
> +
> +	if (!pipe->funcs || !pipe->funcs->check)
> +		return 0;
> +
> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> +}
> +
> +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 (optional)
> + * @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.
> + * Teardown of a simple display pipe is all handled automatically by the drm
> + * core through calling drm_mode_config_cleanup(). Drivers afterwards need to
> + * release the memory for the structure themselves.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +			struct drm_simple_display_pipe *pipe,
> +			const 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);

I'm wondering if we shouldn't remove drm_connector_register() here,
because it doesn't fit the pattern I've seen in the latest drivers:

         drm = drm_dev_alloc(...);

         /* init */

         ret = drm_dev_register(drm, 0);
         ret = drm_connector_register_all(drm);


Noralf.

> +}
> +EXPORT_SYMBOL(drm_simple_display_pipe_init);
> +
> +MODULE_LICENSE("GPL");

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

[Daniel Vetter]

On Sun, May 29, 2016 at 05:38:39PM +0200, Noralf Trønnes wrote:
> 
> Den 12.05.2016 20:25, skrev Noralf Trønnes:
> >Provides helper functions for drivers that have a simple display
> >pipeline. Plane, crtc and encoder are collapsed into one entity.
> >
> >Cc: jsarha@ti.com
> >Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >---
> 
> [...]
> 
> >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..d45417a
> >--- /dev/null
> >+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >@@ -0,0 +1,208 @@
> >+/*
> >+ * 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>
> >+
> >+/**
> >+ * DOC: overview
> >+ *
> >+ * This helper library provides helpers for drivers for simple display
> >+ * hardware.
> >+ *
> >+ * drm_simple_display_pipe_init() initializes a simple display pipeline
> >+ * which has only one full-screen scanout buffer feeding one output. The
> >+ * pipeline is represented by struct &drm_simple_display_pipe and binds
> >+ * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> >+ * entity. Some flexibility for code reuse is provided through a separately
> >+ * allocated &drm_connector object and supporting optional &drm_bridge
> >+ * encoder drivers.
> >+ */
> >+
> >+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 *plane_state)
> >+{
> >+	struct drm_rect src = {
> >+		.x1 = plane_state->src_x,
> >+		.y1 = plane_state->src_y,
> >+		.x2 = plane_state->src_x + plane_state->src_w,
> >+		.y2 = plane_state->src_y + plane_state->src_h,
> >+	};
> >+	struct drm_rect dest = {
> >+		.x1 = plane_state->crtc_x,
> >+		.y1 = plane_state->crtc_y,
> >+		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> >+		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> >+	};
> >+	struct drm_rect clip = { 0 };
> >+	struct drm_simple_display_pipe *pipe;
> >+	struct drm_crtc_state *crtc_state;
> >+	bool visible;
> >+	int ret;
> >+
> >+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >+	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> >+							&pipe->crtc);
> >+	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 */
> >+
> >+	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> >+	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> >+	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> >+					    plane_state->fb,
> >+					    &src, &dest, &clip,
> >+					    DRM_PLANE_HELPER_NO_SCALING,
> >+					    DRM_PLANE_HELPER_NO_SCALING,
> >+					    false, true, &visible);
> >+	if (ret)
> >+		return ret;
> >+
> >+	if (!visible)
> >+		return -EINVAL;
> >+
> >+	if (!pipe->funcs || !pipe->funcs->check)
> >+		return 0;
> >+
> >+	return pipe->funcs->check(pipe, plane_state, crtc_state);
> >+}
> >+
> >+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 (optional)
> >+ * @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.
> >+ * Teardown of a simple display pipe is all handled automatically by the drm
> >+ * core through calling drm_mode_config_cleanup(). Drivers afterwards need to
> >+ * release the memory for the structure themselves.
> >+ *
> >+ * Returns:
> >+ * Zero on success, negative error code on failure.
> >+ */
> >+int drm_simple_display_pipe_init(struct drm_device *dev,
> >+			struct drm_simple_display_pipe *pipe,
> >+			const 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);
> 
> I'm wondering if we shouldn't remove drm_connector_register() here,
> because it doesn't fit the pattern I've seen in the latest drivers:
> 
>         drm = drm_dev_alloc(...);
> 
>         /* init */
> 
>         ret = drm_dev_register(drm, 0);
>         ret = drm_connector_register_all(drm);

Excellent point, this should indeed be removed.
drm_simple_display_pipe_init should just wire up the connector to the
pipeline, not init/register it at all imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Noralf Trønnes]

Den 12.05.2016 20:25, skrev Noralf Trønnes:
> Provides helper functions for drivers that have a simple display
> pipeline. Plane, crtc and encoder are collapsed into one entity.
>
> Cc: jsarha@ti.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

[...]

> 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..d45417a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,208 @@
> +/*
> + * 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>
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides helpers for drivers for simple display
> + * hardware.
> + *
> + * drm_simple_display_pipe_init() initializes a simple display pipeline
> + * which has only one full-screen scanout buffer feeding one output. The
> + * pipeline is represented by struct &drm_simple_display_pipe and binds
> + * together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed
> + * entity. Some flexibility for code reuse is provided through a separately
> + * allocated &drm_connector object and supporting optional &drm_bridge
> + * encoder drivers.
> + */
> +
> +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 *plane_state)
> +{
> +	struct drm_rect src = {
> +		.x1 = plane_state->src_x,
> +		.y1 = plane_state->src_y,
> +		.x2 = plane_state->src_x + plane_state->src_w,
> +		.y2 = plane_state->src_y + plane_state->src_h,
> +	};
> +	struct drm_rect dest = {
> +		.x1 = plane_state->crtc_x,
> +		.y1 = plane_state->crtc_y,
> +		.x2 = plane_state->crtc_x + plane_state->crtc_w,
> +		.y2 = plane_state->crtc_y + plane_state->crtc_h,
> +	};
> +	struct drm_rect clip = { 0 };
> +	struct drm_simple_display_pipe *pipe;
> +	struct drm_crtc_state *crtc_state;
> +	bool visible;
> +	int ret;
> +
> +	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> +	crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> +							&pipe->crtc);
> +	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 */
> +
> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> +	ret = drm_plane_helper_check_update(plane, &pipe->crtc,
> +					    plane_state->fb,
> +					    &src, &dest, &clip,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    DRM_PLANE_HELPER_NO_SCALING,
> +					    false, true, &visible);
> +	if (ret)
> +		return ret;
> +
> +	if (!visible)
> +		return -EINVAL;
> +
> +	if (!pipe->funcs || !pipe->funcs->check)
> +		return 0;
> +
> +	return pipe->funcs->check(pipe, plane_state, crtc_state);
> +}
> +
> +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,

Most of the displays I will use tinydrm for does support hardware rotation.
I have support for initial rotation in the driver since the 
panel/display can
be mounted in all orientations.
I'm wondering if I should add support for the plane rotation property in
tinydrm. That would require this addition here:

         .set_property           = drm_atomic_helper_plane_set_property,

Will this affect the drm_simple_kms_plane_atomic_check() function also?
Is it even possible to support plane rotation in this simple pipe?

How is userspace actually doing a 90/270 degree rotation?
How is swapping width/height handled?
AFAICT xf86-video-modesetting doesn't support the rotation property.

Noralf.

> +	.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 (optional)
> + * @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.
> + * Teardown of a simple display pipe is all handled automatically by the drm
> + * core through calling drm_mode_config_cleanup(). Drivers afterwards need to
> + * release the memory for the structure themselves.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +			struct drm_simple_display_pipe *pipe,
> +			const 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..2690397
> --- /dev/null
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -0,0 +1,94 @@
> +/*
> + * 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.
> +	 * This hook is optional.
> +	 */
> +	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.
> +	 * This hook is optional.
> +	 */
> +	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.
> +	 * The simple display pipeline helpers already check that the plane is
> +	 * not scaled, fills the entire visible area and is always enabled
> +	 * when the crtc is also enabled.
> +	 * This hook is optional.
> +	 *
> +	 * 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.
> +	 * This hook is optional.
> +	 */
> +	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. It should be initialized by calling drm_simple_display_pipe_init().
> + */
> +struct drm_simple_display_pipe {
> +	struct drm_crtc crtc;
> +	struct drm_plane plane;
> +	struct drm_encoder encoder;
> +	struct drm_connector *connector;
> +
> +	const struct drm_simple_display_pipe_funcs *funcs;
> +};
> +
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +			struct drm_simple_display_pipe *pipe,
> +			const 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.8.2
>

Re: [PATCH v4 0/3] drm: Add various helpers for simple drivers

[Laurent Pinchart]

Hi Noralf,

Thank you for the patches.

For 1/3 and 2/3,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Thursday 12 May 2016 20:25:20 Noralf Trønnes wrote:
> This patchset adds various helpers that was originally part of the
> tinydrm patchset.
> 
> Essentially it adds 2 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.
> 
> 
> Changes since v3:
> - Add patch 'drm/fb-cma-helper: Use const for drm_framebuffer_funcs
> argument' - drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
>   - funcs argument should be const
> - drm: Add helper for simple display pipeline
>   - (struct drm_simple_display_pipe *)->funcs should be const
> 
> Changes since v2:
> - drm: Add helper for simple display pipeline
>   - Drop Kconfig knob DRM_KMS_HELPER
>   - Expand documentation
> 
> Changes since v1:
> - Drop patch: drm/panel: Add helper for simple panel connector
> - Add fb-helper and fb-cma-helper doc patches
> - Add drm/atomic: Don't skip drm_bridge_*() calls if
> !drm_encoder_helper_funcs - Add drm_atomic_helper_best_encoder()
> - drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
>   - Expand docs
> - drm: Add helper for simple display pipeline
>   - Add DOC header and add to gpu.tmpl
>   - Fix docs: @funcs is optional, "negative error code",
>     "This hook is optional."
>   - Add checks to drm_simple_kms_plane_atomic_check()
> 
> 
> Noralf Trønnes (3):
>   drm/fb-cma-helper: Use const for drm_framebuffer_funcs argument
>   drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
>   drm: Add helper for simple display pipeline
> 
>  Documentation/DocBook/gpu.tmpl          |   6 +
>  drivers/gpu/drm/Makefile                |   2 +-
>  drivers/gpu/drm/drm_fb_cma_helper.c     |  35 ++++--
>  drivers/gpu/drm/drm_simple_kms_helper.c | 208
> ++++++++++++++++++++++++++++++++ include/drm/drm_fb_cma_helper.h         | 
>  5 +-
>  include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
>  6 files changed, 340 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>  create mode 100644 include/drm/drm_simple_kms_helper.h
> 
> --
> 2.8.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Regards,

Laurent Pinchart