Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1399307 > unrolled thread

[PATCH v2 6/6] drm: Add helper for simple display pipeline

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 6/6] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-11 18:20 +0200
    Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-11 19:10 +0200
      Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Paul Bolle <pebolle@tiscali.nl> - 2016-05-11 21:20 +0200
        Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-11 21:40 +0200
      Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-11 21:30 +0200
      Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-12 10:20 +0200
        Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Noralf Trønnes <noralf@tronnes.org> - 2016-05-12 12:20 +0200
          Re: [PATCH v2 6/6] drm: Add helper for simple display pipeline Daniel Vetter <daniel@ffwll.ch> - 2016-05-12 12:50 +0200

#1399307 — [PATCH v2 6/6] drm: Add helper for simple display pipeline

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

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

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/Kconfig                 |   7 ++
 drivers/gpu/drm/Makefile                |   1 +
 drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++
 include/drm/drm_simple_kms_helper.h     |  92 +++++++++++++++
 5 files changed, 306 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
 create mode 100644 include/drm/drm_simple_kms_helper.h

diff --git a/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/Kconfig b/drivers/gpu/drm/Kconfig
index 16e4c21..ff9f480 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -39,6 +39,13 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.

+config DRM_SIMPLE_KMS_HELPER
+	tristate
+	depends on DRM
+	select DRM_KMS_HELPER
+	help
+	  Helpers for very simple KMS drivers.
+
 config DRM_KMS_FB_HELPER
 	bool
 	depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 43c2abf..7e99923 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o

 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
+obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o

 CFLAGS_drm_trace_points.o := -I$(src)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
new file mode 100644
index 0000000..64bf0f2
--- /dev/null
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -0,0 +1,200 @@
+/*
+ * 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 simple drivers.
+ *
+ * drm_simple_display_pipe_init() initializes a simple display pipeline
+ * consisting of a plane-crtc-encoder pipe coupled with a connector.
+ */
+
+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.
+ *
+ * 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,
+				 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..ef578f4
--- /dev/null
+++ b/include/drm/drm_simple_kms_helper.h
@@ -0,0 +1,92 @@
+/*
+ * 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.
+	 * 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.
+ */
+struct drm_simple_display_pipe {
+	struct drm_crtc crtc;
+	struct drm_plane plane;
+	struct drm_encoder encoder;
+	struct drm_connector *connector;
+
+	struct drm_simple_display_pipe_funcs *funcs;
+};
+
+int drm_simple_display_pipe_init(struct drm_device *dev,
+				 struct drm_simple_display_pipe *pipe,
+				 struct drm_simple_display_pipe_funcs *funcs,
+				 const uint32_t *formats,
+				 unsigned int format_count,
+				 struct drm_connector *connector);
+
+#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
--
2.8.2

[toc] | [next] | [standalone]

#1399374

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

Looks really nice, just a few suggestions for the kerneldoc. Would be
awesome if we could get an ack on this from Jyri for tilcdc, but even
without that I think I'll just pull in the next iteration. Still please cc
him.

Thanks, Daniel

> ---
> 
> 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/Kconfig                 |   7 ++
>  drivers/gpu/drm/Makefile                |   1 +
>  drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     |  92 +++++++++++++++
>  5 files changed, 306 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c
>  create mode 100644 include/drm/drm_simple_kms_helper.h
> 
> diff --git a/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/Kconfig b/drivers/gpu/drm/Kconfig
> index 16e4c21..ff9f480 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -39,6 +39,13 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers.
> 
> +config DRM_SIMPLE_KMS_HELPER
> +	tristate
> +	depends on DRM
> +	select DRM_KMS_HELPER
> +	help
> +	  Helpers for very simple KMS drivers.

Personally not sold on piles of Kconfig knobs for tiny amounts of code
like this one. I'd just drop it. For a more elaborate argument see

http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html

Note that almost all the other helpers do not have a Kconfig option, the
only real exception is the fbdev helpers. And those have a good reason:
They'd drag in all of fbdev, and that is actually a pile of code.

> +
>  config DRM_KMS_FB_HELPER
>  	bool
>  	depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 43c2abf..7e99923 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> 
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> +obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o
> 
>  CFLAGS_drm_trace_points.o := -I$(src)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> new file mode 100644
> index 0000000..64bf0f2
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,200 @@
> +/*
> + * 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 simple drivers.

"drivers for simple display hardware" is imo clearer.

> + *
> + * drm_simple_display_pipe_init() initializes a simple display pipeline
> + * consisting of a plane-crtc-encoder pipe coupled with a connector.

Hm, I think a bit more text here would be useful.

"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;

I think the logic above looks correct now, but might be worth checking
with your driver that it doesn't let something silly through. I.e. a small
test app that tries to reposition the primary plane, or tries to disable
it while the crtc is on. We should have that somewhere in libdrm I think.

> +
> +	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.

How are drivers supposed to release this stuff again? Maybe add:

"Teardown of a simple display pipe is all handled automatically by the drm
core through calling drm_mode_config_cleanup()."

> + *
> + * 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,
> +				 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..ef578f4
> --- /dev/null
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -0,0 +1,92 @@
> +/*
> + * 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.

I think we need to clarify a bit more what the helpers check already:

"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.

Maybe add: "It should be initialized by calling
drm_simple_display_pipe_init()". Imo never hurts to have a few too many
cross references ;-)

> + */
> +struct drm_simple_display_pipe {
> +	struct drm_crtc crtc;
> +	struct drm_plane plane;
> +	struct drm_encoder encoder;
> +	struct drm_connector *connector;
> +
> +	struct drm_simple_display_pipe_funcs *funcs;
> +};
> +
> +int drm_simple_display_pipe_init(struct drm_device *dev,
> +				 struct drm_simple_display_pipe *pipe,
> +				 struct drm_simple_display_pipe_funcs *funcs,
> +				 const uint32_t *formats,
> +				 unsigned int format_count,
> +				 struct drm_connector *connector);
> +
> +#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.8.2
> 

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

[toc] | [prev] | [next] | [standalone]

#1399439

On wo, 2016-05-11 at 19:09 +0200, Daniel Vetter wrote:
> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:

> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig

> > +config DRM_SIMPLE_KMS_HELPER
> > +	tristate
> > +	depends on DRM
> > +	select DRM_KMS_HELPER
> > +	help
> > +	  Helpers for very simple KMS drivers.
> 
> Personally not sold on piles of Kconfig knobs for tiny amounts of code
> like this one. I'd just drop it. For a more elaborate argument see
> http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html
> 
> Note that almost all the other helpers do not have a Kconfig option, the
> only real exception is the fbdev helpers. And those have a good reason:
> They'd drag in all of fbdev, and that is actually a pile of code.

Moreover, this adds a kconfig entry without a prompt. The entry also
doesn't have a "default". And I didn't spot a patch adding a select for
this symbol. So, based on just this series, I think that
DRM_SIMPLE_KMS_HELPER can't actually be set.

I didn't test this, so perhaps I missed something. Did I, Noralf?


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1399451

Den 11.05.2016 21:10, skrev Paul Bolle:
> On wo, 2016-05-11 at 19:09 +0200, Daniel Vetter wrote:
>> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:
>>> --- a/drivers/gpu/drm/Kconfig
>>> +++ b/drivers/gpu/drm/Kconfig
>>> +config DRM_SIMPLE_KMS_HELPER
>>> +	tristate
>>> +	depends on DRM
>>> +	select DRM_KMS_HELPER
>>> +	help
>>> +	  Helpers for very simple KMS drivers.
>> Personally not sold on piles of Kconfig knobs for tiny amounts of code
>> like this one. I'd just drop it. For a more elaborate argument see
>> http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html
>>
>> Note that almost all the other helpers do not have a Kconfig option, the
>> only real exception is the fbdev helpers. And those have a good reason:
>> They'd drag in all of fbdev, and that is actually a pile of code.
> Moreover, this adds a kconfig entry without a prompt. The entry also
> doesn't have a "default". And I didn't spot a patch adding a select for
> this symbol. So, based on just this series, I think that
> DRM_SIMPLE_KMS_HELPER can't actually be set.
>
> I didn't test this, so perhaps I missed something. Did I, Noralf?

It would have been selected in the tinydrm follow-up patchset, but
I'll do as Daniel suggests and add it to drm_kms_helper-y.

Noralf.

[toc] | [prev] | [next] | [standalone]

#1399447

Den 11.05.2016 19:09, skrev Daniel Vetter:
> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:
>> Provides helper functions for drivers that have a simple display
>> pipeline. Plane, crtc and encoder are collapsed into one entity.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Looks really nice, just a few suggestions for the kerneldoc. Would be
> awesome if we could get an ack on this from Jyri for tilcdc, but even
> without that I think I'll just pull in the next iteration. Still please cc
> him.
>
> Thanks, Daniel

Thanks for helping me with the docs.

[...]

>> +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;
> I think the logic above looks correct now, but might be worth checking
> with your driver that it doesn't let something silly through. I.e. a small
> test app that tries to reposition the primary plane, or tries to disable
> it while the crtc is on. We should have that somewhere in libdrm I think.

I hacked libdrm tests/kms/kms-universal-planes to position the plane
at (1,1) which failed (I added some debug output to the driver):

[  885.906697] [drm:drm_atomic_set_fb_for_plane] Set [FB:25] for plane 
state b84aec40
[  885.906707] [drm:drm_atomic_check_only] checking b84aeec0
[  885.906738] [drm:drm_plane_helper_check_update] Plane must cover 
entire CRTC
[  885.906748] [drm:drm_rect_debug_print] dst: 319x239+1+1
[  885.906757] [drm:drm_rect_debug_print] clip: 320x240+0+0
[  885.906765] drm_simple_kms_plane_atomic_check: ret=-22, visible=1
[  885.906775] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0] 
atomic driver check failed

Then I changed the test to pass 0 for fb id which also failed:

[ 3144.599790] [drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane 
state b87805c0
[ 3144.599799] [drm:drm_atomic_check_only] checking b8780d80
[ 3144.599816] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0] 
atomic driver check failed


Noralf.

[toc] | [prev] | [next] | [standalone]

#1399741

On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote:
> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:
> > +/**
> > + * 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.
> 
> How are drivers supposed to release this stuff again? Maybe add:
> 
> "Teardown of a simple display pipe is all handled automatically by the drm
> core through calling drm_mode_config_cleanup()."

Thought a bit more about this, maybe we should also add "Drivers
afterwards need to release the memory for the structure themselves."

Btw one other thing I realized is that there's no atomic_commit for this.
How do you plane to implement async commit? No need to address this right
away, we can discuss it when you've rebased tinydrm and submit that for
review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1399855

Den 12.05.2016 10:11, skrev Daniel Vetter:
> On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote:
>> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:
>>> +/**
>>> + * 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.
>> How are drivers supposed to release this stuff again? Maybe add:
>>
>> "Teardown of a simple display pipe is all handled automatically by the drm
>> core through calling drm_mode_config_cleanup()."
> Thought a bit more about this, maybe we should also add "Drivers
> afterwards need to release the memory for the structure themselves."
>
> Btw one other thing I realized is that there's no atomic_commit for this.
> How do you plane to implement async commit? No need to address this right
> away, we can discuss it when you've rebased tinydrm and submit that for
> review.
> -Daniel

I don't follow you here. Isn't this atomic_commit:

drm_atomic_helper_commit => drm_atomic_helper_commit_planes =>
drm_simple_kms_plane_atomic_update => pipe->funcs->update

Noralf.

[toc] | [prev] | [next] | [standalone]

#1399909

On Thu, May 12, 2016 at 12:18 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> Den 12.05.2016 10:11, skrev Daniel Vetter:
>>
>> On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote:
>>>
>>> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote:
>>>>
>>>> +/**
>>>> + * 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.
>>>
>>> How are drivers supposed to release this stuff again? Maybe add:
>>>
>>> "Teardown of a simple display pipe is all handled automatically by the
>>> drm
>>> core through calling drm_mode_config_cleanup()."
>>
>> Thought a bit more about this, maybe we should also add "Drivers
>> afterwards need to release the memory for the structure themselves."
>>
>> Btw one other thing I realized is that there's no atomic_commit for this.
>> How do you plane to implement async commit? No need to address this right
>> away, we can discuss it when you've rebased tinydrm and submit that for
>> review.
>> -Daniel
>
>
> I don't follow you here. Isn't this atomic_commit:
>
> drm_atomic_helper_commit => drm_atomic_helper_commit_planes =>
> drm_simple_kms_plane_atomic_update => pipe->funcs->update

drm_atomic_helper_commit does not implement nonblocking commits,
because that's a bit tricky when there's more than 1 crtc. But if you
only have 1 crtc it's easy. And without nonblocking commit the legacy
pageflip stuff will also not work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web