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

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

[Noralf Trønnes]

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

Essentially it adds 3 functions:
- drm_fb_cma_create_with_funcs()
  CMA backed framebuffer supporting a dirty() callback.
- drm_atomic_helper_best_encoder()
  (struct drm_connector_helper_funcs *)->best_encoder callback helper.
- drm_simple_display_pipe_init()
  Plane, crtc and encoder are collapsed into one entity.

Plus it has gained a couple of documentation patches since v1.

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 (6):
  drm/fb-helper: Remove mention of CONFIG_FB_DEFERRED_IO in docs
  drm/fb-cma-helper: Hook up to DocBook and fix some docs
  drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  drm/atomic: Don't skip drm_bridge_*() calls if
    !drm_encoder_helper_funcs
  drm/atomic: Add drm_atomic_helper_best_encoder()
  drm: Add helper for simple display pipeline

 Documentation/DocBook/gpu.tmpl          |  11 ++
 drivers/gpu/drm/Kconfig                 |   7 ++
 drivers/gpu/drm/Makefile                |   1 +
 drivers/gpu/drm/drm_atomic_helper.c     |  56 +++++----
 drivers/gpu/drm/drm_fb_cma_helper.c     |  39 +++++--
 drivers/gpu/drm/drm_fb_helper.c         |  16 +--
 drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h         |   2 +
 include/drm/drm_fb_cma_helper.h         |   3 +
 include/drm/drm_simple_kms_helper.h     |  92 +++++++++++++++
 10 files changed, 386 insertions(+), 41 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 v2 0/6] drm: Add various helpers for simple drivers

[Noralf Trønnes]

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

Essentially it adds 3 functions:
- drm_fb_cma_create_with_funcs()
  CMA backed framebuffer supporting a dirty() callback.
- drm_atomic_helper_best_encoder()
  (struct drm_connector_helper_funcs *)->best_encoder callback helper.
- drm_simple_display_pipe_init()
  Plane, crtc and encoder are collapsed into one entity.

Plus it has gained a couple of documentation patches since v1.

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 (6):
  drm/fb-helper: Remove mention of CONFIG_FB_DEFERRED_IO in docs
  drm/fb-cma-helper: Hook up to DocBook and fix some docs
  drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()
  drm/atomic: Don't skip drm_bridge_*() calls if
    !drm_encoder_helper_funcs
  drm/atomic: Add drm_atomic_helper_best_encoder()
  drm: Add helper for simple display pipeline

 Documentation/DocBook/gpu.tmpl          |  11 ++
 drivers/gpu/drm/Kconfig                 |   7 ++
 drivers/gpu/drm/Makefile                |   1 +
 drivers/gpu/drm/drm_atomic_helper.c     |  56 +++++----
 drivers/gpu/drm/drm_fb_cma_helper.c     |  39 +++++--
 drivers/gpu/drm/drm_fb_helper.c         |  16 +--
 drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h         |   2 +
 include/drm/drm_fb_cma_helper.h         |   3 +
 include/drm/drm_simple_kms_helper.h     |  92 +++++++++++++++
 10 files changed, 386 insertions(+), 41 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

[PATCH v2 1/6] drm/fb-helper: Remove mention of CONFIG_FB_DEFERRED_IO in docs

[Noralf Trønnes]

This was forgotten to fixup in the latest version of the deferred_io
patch which made FB_DEFERRED_IO mandatory.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 385284b..e03f8ad 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -85,14 +85,14 @@ static LIST_HEAD(kernel_fb_helper_list);
  * should call drm_fb_helper_single_add_all_connectors() followed by
  * drm_fb_helper_initial_config().
  *
- * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
- * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
- * functions will accumulate changes and schedule &fb_helper .dirty_work to run
- * right away. This worker then calls the dirty() function ensuring that it
- * will always run in process context since the fb_*() function could be
- * running in atomic context. If drm_fb_helper_deferred_io() is used as the
- * deferred_io callback it will also schedule dirty_work with the damage
- * collected from the mmap page writes.
+ * If &drm_framebuffer_funcs ->dirty is set, the
+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions will
+ * accumulate changes and schedule &drm_fb_helper ->dirty_work to run right
+ * away. This worker then calls the dirty() function ensuring that it will
+ * always run in process context since the fb_*() function could be running in
+ * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
+ * callback it will also schedule dirty_work with the damage collected from the
+ * mmap page writes.
  */
 
 /**
-- 
2.8.2

[PATCH v2 1/6] drm/fb-helper: Remove mention of CONFIG_FB_DEFERRED_IO in docs

[Noralf Trønnes]

This was forgotten to fixup in the latest version of the deferred_io
patch which made FB_DEFERRED_IO mandatory.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 385284b..e03f8ad 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -85,14 +85,14 @@ static LIST_HEAD(kernel_fb_helper_list);
  * should call drm_fb_helper_single_add_all_connectors() followed by
  * drm_fb_helper_initial_config().
  *
- * If CONFIG_FB_DEFERRED_IO is enabled and &drm_framebuffer_funcs ->dirty is
- * set, the drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit}
- * functions will accumulate changes and schedule &fb_helper .dirty_work to run
- * right away. This worker then calls the dirty() function ensuring that it
- * will always run in process context since the fb_*() function could be
- * running in atomic context. If drm_fb_helper_deferred_io() is used as the
- * deferred_io callback it will also schedule dirty_work with the damage
- * collected from the mmap page writes.
+ * If &drm_framebuffer_funcs ->dirty is set, the
+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions will
+ * accumulate changes and schedule &drm_fb_helper ->dirty_work to run right
+ * away. This worker then calls the dirty() function ensuring that it will
+ * always run in process context since the fb_*() function could be running in
+ * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
+ * callback it will also schedule dirty_work with the damage collected from the
+ * mmap page writes.
  */
 
 /**
-- 
2.8.2

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

[PATCH v2 2/6] drm/fb-cma-helper: Hook up to DocBook and fix some docs

[Noralf Trønnes]

Hook up fb_cma_helper to DocBook. Remove mention of
CONFIG_FB_DEFERRED_IO in the docs, which was forgotten in the latest
version of the deferred_io patch.
Use & when referencing drm_mode_config_funcs in docs.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/DocBook/gpu.tmpl      | 5 +++++
 drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 9dd48f7..4a0c599 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -1617,6 +1617,11 @@ void intel_crt_init(struct drm_device *dev)
 !Iinclude/drm/drm_fb_helper.h
     </sect2>
     <sect2>
+      <title>Framebuffer CMA Helper Functions Reference</title>
+!Pdrivers/gpu/drm/drm_fb_cma_helper.c framebuffer cma helper functions
+!Edrivers/gpu/drm/drm_fb_cma_helper.c
+    </sect2>
+    <sect2>
       <title>Display Port Helper Functions Reference</title>
 !Pdrivers/gpu/drm/drm_dp_helper.c dp helpers
 !Iinclude/drm/drm_dp_helper.h
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 086f600..3165ac0 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -43,14 +43,12 @@ struct drm_fbdev_cma {
  * Provides helper functions for creating a cma (contiguous memory allocator)
  * backed framebuffer.
  *
- * drm_fb_cma_create() is used in the
- * (struct drm_mode_config_funcs *)->fb_create callback function to create the
- * cma backed framebuffer.
+ * drm_fb_cma_create() is used in the &drm_mode_config_funcs ->fb_create
+ * callback function to create a cma backed framebuffer.
  *
  * An fbdev framebuffer backed by cma is also available by calling
  * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
- * If CONFIG_FB_DEFERRED_IO is enabled and the callback
- * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
+ * If the &drm_framebuffer_funcs ->dirty callback is set, fb_deferred_io
  * will be set up automatically. dirty() is called by
  * drm_fb_helper_deferred_io() in process context (struct delayed_work).
  *
-- 
2.8.2

[PATCH v2 2/6] drm/fb-cma-helper: Hook up to DocBook and fix some docs

[Noralf Trønnes]

Hook up fb_cma_helper to DocBook. Remove mention of
CONFIG_FB_DEFERRED_IO in the docs, which was forgotten in the latest
version of the deferred_io patch.
Use & when referencing drm_mode_config_funcs in docs.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/DocBook/gpu.tmpl      | 5 +++++
 drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 9dd48f7..4a0c599 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -1617,6 +1617,11 @@ void intel_crt_init(struct drm_device *dev)
 !Iinclude/drm/drm_fb_helper.h
     </sect2>
     <sect2>
+      <title>Framebuffer CMA Helper Functions Reference</title>
+!Pdrivers/gpu/drm/drm_fb_cma_helper.c framebuffer cma helper functions
+!Edrivers/gpu/drm/drm_fb_cma_helper.c
+    </sect2>
+    <sect2>
       <title>Display Port Helper Functions Reference</title>
 !Pdrivers/gpu/drm/drm_dp_helper.c dp helpers
 !Iinclude/drm/drm_dp_helper.h
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 086f600..3165ac0 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -43,14 +43,12 @@ struct drm_fbdev_cma {
  * Provides helper functions for creating a cma (contiguous memory allocator)
  * backed framebuffer.
  *
- * drm_fb_cma_create() is used in the
- * (struct drm_mode_config_funcs *)->fb_create callback function to create the
- * cma backed framebuffer.
+ * drm_fb_cma_create() is used in the &drm_mode_config_funcs ->fb_create
+ * callback function to create a cma backed framebuffer.
  *
  * An fbdev framebuffer backed by cma is also available by calling
  * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
- * If CONFIG_FB_DEFERRED_IO is enabled and the callback
- * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
+ * If the &drm_framebuffer_funcs ->dirty callback is set, fb_deferred_io
  * will be set up automatically. dirty() is called by
  * drm_fb_helper_deferred_io() in process context (struct delayed_work).
  *
-- 
2.8.2

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

Re: [PATCH v2 2/6] drm/fb-cma-helper: Hook up to DocBook and fix some docs

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:18PM +0200, Noralf Trønnes wrote:
> Hook up fb_cma_helper to DocBook. Remove mention of
> CONFIG_FB_DEFERRED_IO in the docs, which was forgotten in the latest
> version of the deferred_io patch.
> Use & when referencing drm_mode_config_funcs in docs.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

First two patches applied to drm-misc, thanks.
-Daniel

> ---
>  Documentation/DocBook/gpu.tmpl      | 5 +++++
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 9dd48f7..4a0c599 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1617,6 +1617,11 @@ void intel_crt_init(struct drm_device *dev)
>  !Iinclude/drm/drm_fb_helper.h
>      </sect2>
>      <sect2>
> +      <title>Framebuffer CMA Helper Functions Reference</title>
> +!Pdrivers/gpu/drm/drm_fb_cma_helper.c framebuffer cma helper functions
> +!Edrivers/gpu/drm/drm_fb_cma_helper.c
> +    </sect2>
> +    <sect2>
>        <title>Display Port Helper Functions Reference</title>
>  !Pdrivers/gpu/drm/drm_dp_helper.c dp helpers
>  !Iinclude/drm/drm_dp_helper.h
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 086f600..3165ac0 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -43,14 +43,12 @@ struct drm_fbdev_cma {
>   * Provides helper functions for creating a cma (contiguous memory allocator)
>   * backed framebuffer.
>   *
> - * drm_fb_cma_create() is used in the
> - * (struct drm_mode_config_funcs *)->fb_create callback function to create the
> - * cma backed framebuffer.
> + * drm_fb_cma_create() is used in the &drm_mode_config_funcs ->fb_create
> + * callback function to create a cma backed framebuffer.
>   *
>   * An fbdev framebuffer backed by cma is also available by calling
>   * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
> - * If CONFIG_FB_DEFERRED_IO is enabled and the callback
> - * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
> + * If the &drm_framebuffer_funcs ->dirty callback is set, fb_deferred_io
>   * will be set up automatically. dirty() is called by
>   * drm_fb_helper_deferred_io() in process context (struct delayed_work).
>   *
> -- 
> 2.8.2
> 

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

Re: [PATCH v2 2/6] drm/fb-cma-helper: Hook up to DocBook and fix some docs

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:18PM +0200, Noralf Trønnes wrote:
> Hook up fb_cma_helper to DocBook. Remove mention of
> CONFIG_FB_DEFERRED_IO in the docs, which was forgotten in the latest
> version of the deferred_io patch.
> Use & when referencing drm_mode_config_funcs in docs.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

First two patches applied to drm-misc, thanks.
-Daniel

> ---
>  Documentation/DocBook/gpu.tmpl      | 5 +++++
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 9dd48f7..4a0c599 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1617,6 +1617,11 @@ void intel_crt_init(struct drm_device *dev)
>  !Iinclude/drm/drm_fb_helper.h
>      </sect2>
>      <sect2>
> +      <title>Framebuffer CMA Helper Functions Reference</title>
> +!Pdrivers/gpu/drm/drm_fb_cma_helper.c framebuffer cma helper functions
> +!Edrivers/gpu/drm/drm_fb_cma_helper.c
> +    </sect2>
> +    <sect2>
>        <title>Display Port Helper Functions Reference</title>
>  !Pdrivers/gpu/drm/drm_dp_helper.c dp helpers
>  !Iinclude/drm/drm_dp_helper.h
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 086f600..3165ac0 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -43,14 +43,12 @@ struct drm_fbdev_cma {
>   * Provides helper functions for creating a cma (contiguous memory allocator)
>   * backed framebuffer.
>   *
> - * drm_fb_cma_create() is used in the
> - * (struct drm_mode_config_funcs *)->fb_create callback function to create the
> - * cma backed framebuffer.
> + * drm_fb_cma_create() is used in the &drm_mode_config_funcs ->fb_create
> + * callback function to create a cma backed framebuffer.
>   *
>   * An fbdev framebuffer backed by cma is also available by calling
>   * drm_fbdev_cma_init(). drm_fbdev_cma_fini() tears it down.
> - * If CONFIG_FB_DEFERRED_IO is enabled and the callback
> - * (struct drm_framebuffer_funcs)->dirty is set, fb_deferred_io
> + * If the &drm_framebuffer_funcs ->dirty callback is set, fb_deferred_io
>   * will be set up automatically. dirty() is called by
>   * drm_fb_helper_deferred_io() in process context (struct delayed_work).
>   *
> -- 
> 2.8.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Noralf Trønnes]

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

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

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 3165ac0..ede77c9 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,
+	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 c6d9c9c..1f9a8bc 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
 int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
 	struct drm_file *file_priv, unsigned int *handle);

+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+	struct drm_framebuffer_funcs *funcs);
 struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);

--
2.8.2

[PATCH v2 3/6] drm/fb-cma-helper: Add function drm_fb_cma_create_with_funcs()

[Noralf Trønnes]

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

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

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 3165ac0..ede77c9 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,
+	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 c6d9c9c..1f9a8bc 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -31,6 +31,9 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb);
 int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
 	struct drm_file *file_priv, unsigned int *handle);

+struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct drm_device *dev,
+	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd,
+	struct drm_framebuffer_funcs *funcs);
 struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 	struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd);

--
2.8.2

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

[PATCH v2 4/6] drm/atomic: Don't skip drm_bridge_*() calls if !drm_encoder_helper_funcs

[Noralf Trønnes]

Don't skip drm_bridge_*() calls if encoder->helper_private is NULL.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 027b227..46a3201 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -384,8 +384,6 @@ mode_fixup(struct drm_atomic_state *state)
 		 */
 		encoder = conn_state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
 				&crtc_state->adjusted_mode);
@@ -394,7 +392,7 @@ mode_fixup(struct drm_atomic_state *state)
 			return -EINVAL;
 		}
 
-		if (funcs->atomic_check) {
+		if (funcs && funcs->atomic_check) {
 			ret = funcs->atomic_check(encoder, crtc_state,
 						  conn_state);
 			if (ret) {
@@ -402,7 +400,7 @@ mode_fixup(struct drm_atomic_state *state)
 						 encoder->base.id, encoder->name);
 				return ret;
 			}
-		} else if (funcs->mode_fixup) {
+		} else if (funcs && funcs->mode_fixup) {
 			ret = funcs->mode_fixup(encoder, &crtc_state->mode,
 						&crtc_state->adjusted_mode);
 			if (!ret) {
@@ -696,8 +694,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 			continue;
 
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -709,12 +705,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		drm_bridge_disable(encoder->bridge);
 
 		/* Right function depends upon target state. */
-		if (connector->state->crtc && funcs->prepare)
-			funcs->prepare(encoder);
-		else if (funcs->disable)
-			funcs->disable(encoder);
-		else if (funcs->dpms)
-			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (connector->state->crtc && funcs->prepare)
+				funcs->prepare(encoder);
+			else if (funcs->disable)
+				funcs->disable(encoder);
+			else if (funcs->dpms)
+				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+		}
 
 		drm_bridge_post_disable(encoder->bridge);
 	}
@@ -861,9 +859,6 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
-
 		new_crtc_state = connector->state->crtc->state;
 		mode = &new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
@@ -878,7 +873,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call mode_set hooks twice.
 		 */
-		if (funcs->mode_set)
+		if (funcs && funcs->mode_set)
 			funcs->mode_set(encoder, mode, adjusted_mode);
 
 		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
@@ -969,8 +964,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -981,10 +974,12 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		 */
 		drm_bridge_pre_enable(encoder->bridge);
 
-		if (funcs->enable)
-			funcs->enable(encoder);
-		else if (funcs->commit)
-			funcs->commit(encoder);
+		if (funcs) {
+			if (funcs->enable)
+				funcs->enable(encoder);
+			else if (funcs->commit)
+				funcs->commit(encoder);
+		}
 
 		drm_bridge_enable(encoder->bridge);
 	}
-- 
2.8.2

[PATCH v2 4/6] drm/atomic: Don't skip drm_bridge_*() calls if !drm_encoder_helper_funcs

[Noralf Trønnes]

Don't skip drm_bridge_*() calls if encoder->helper_private is NULL.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 027b227..46a3201 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -384,8 +384,6 @@ mode_fixup(struct drm_atomic_state *state)
 		 */
 		encoder = conn_state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
 				&crtc_state->adjusted_mode);
@@ -394,7 +392,7 @@ mode_fixup(struct drm_atomic_state *state)
 			return -EINVAL;
 		}
 
-		if (funcs->atomic_check) {
+		if (funcs && funcs->atomic_check) {
 			ret = funcs->atomic_check(encoder, crtc_state,
 						  conn_state);
 			if (ret) {
@@ -402,7 +400,7 @@ mode_fixup(struct drm_atomic_state *state)
 						 encoder->base.id, encoder->name);
 				return ret;
 			}
-		} else if (funcs->mode_fixup) {
+		} else if (funcs && funcs->mode_fixup) {
 			ret = funcs->mode_fixup(encoder, &crtc_state->mode,
 						&crtc_state->adjusted_mode);
 			if (!ret) {
@@ -696,8 +694,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 			continue;
 
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -709,12 +705,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		drm_bridge_disable(encoder->bridge);
 
 		/* Right function depends upon target state. */
-		if (connector->state->crtc && funcs->prepare)
-			funcs->prepare(encoder);
-		else if (funcs->disable)
-			funcs->disable(encoder);
-		else if (funcs->dpms)
-			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+		if (funcs) {
+			if (connector->state->crtc && funcs->prepare)
+				funcs->prepare(encoder);
+			else if (funcs->disable)
+				funcs->disable(encoder);
+			else if (funcs->dpms)
+				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+		}
 
 		drm_bridge_post_disable(encoder->bridge);
 	}
@@ -861,9 +859,6 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
-
 		new_crtc_state = connector->state->crtc->state;
 		mode = &new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
@@ -878,7 +873,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call mode_set hooks twice.
 		 */
-		if (funcs->mode_set)
+		if (funcs && funcs->mode_set)
 			funcs->mode_set(encoder, mode, adjusted_mode);
 
 		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
@@ -969,8 +964,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
-		if (!funcs)
-			continue;
 
 		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
 				 encoder->base.id, encoder->name);
@@ -981,10 +974,12 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		 */
 		drm_bridge_pre_enable(encoder->bridge);
 
-		if (funcs->enable)
-			funcs->enable(encoder);
-		else if (funcs->commit)
-			funcs->commit(encoder);
+		if (funcs) {
+			if (funcs->enable)
+				funcs->enable(encoder);
+			else if (funcs->commit)
+				funcs->commit(encoder);
+		}
 
 		drm_bridge_enable(encoder->bridge);
 	}
-- 
2.8.2

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

Re: [PATCH v2 4/6] drm/atomic: Don't skip drm_bridge_*() calls if !drm_encoder_helper_funcs

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:20PM +0200, Noralf Trønnes wrote:
> Don't skip drm_bridge_*() calls if encoder->helper_private is NULL.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 027b227..46a3201 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -384,8 +384,6 @@ mode_fixup(struct drm_atomic_state *state)
>  		 */
>  		encoder = conn_state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
>  				&crtc_state->adjusted_mode);
> @@ -394,7 +392,7 @@ mode_fixup(struct drm_atomic_state *state)
>  			return -EINVAL;
>  		}
>  
> -		if (funcs->atomic_check) {
> +		if (funcs && funcs->atomic_check) {
>  			ret = funcs->atomic_check(encoder, crtc_state,
>  						  conn_state);
>  			if (ret) {
> @@ -402,7 +400,7 @@ mode_fixup(struct drm_atomic_state *state)
>  						 encoder->base.id, encoder->name);
>  				return ret;
>  			}
> -		} else if (funcs->mode_fixup) {
> +		} else if (funcs && funcs->mode_fixup) {
>  			ret = funcs->mode_fixup(encoder, &crtc_state->mode,
>  						&crtc_state->adjusted_mode);
>  			if (!ret) {
> @@ -696,8 +694,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  			continue;
>  
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -709,12 +705,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		drm_bridge_disable(encoder->bridge);
>  
>  		/* Right function depends upon target state. */
> -		if (connector->state->crtc && funcs->prepare)
> -			funcs->prepare(encoder);
> -		else if (funcs->disable)
> -			funcs->disable(encoder);
> -		else if (funcs->dpms)
> -			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +		if (funcs) {
> +			if (connector->state->crtc && funcs->prepare)
> +				funcs->prepare(encoder);
> +			else if (funcs->disable)
> +				funcs->disable(encoder);
> +			else if (funcs->dpms)
> +				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +		}
>  
>  		drm_bridge_post_disable(encoder->bridge);
>  	}
> @@ -861,9 +859,6 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
> -
>  		new_crtc_state = connector->state->crtc->state;
>  		mode = &new_crtc_state->mode;
>  		adjusted_mode = &new_crtc_state->adjusted_mode;
> @@ -878,7 +873,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call mode_set hooks twice.
>  		 */
> -		if (funcs->mode_set)
> +		if (funcs && funcs->mode_set)
>  			funcs->mode_set(encoder, mode, adjusted_mode);
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> @@ -969,8 +964,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -981,10 +974,12 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  		 */
>  		drm_bridge_pre_enable(encoder->bridge);
>  
> -		if (funcs->enable)
> -			funcs->enable(encoder);
> -		else if (funcs->commit)
> -			funcs->commit(encoder);
> +		if (funcs) {
> +			if (funcs->enable)
> +				funcs->enable(encoder);
> +			else if (funcs->commit)
> +				funcs->commit(encoder);
> +		}
>  
>  		drm_bridge_enable(encoder->bridge);
>  	}
> -- 
> 2.8.2
> 

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

Re: [PATCH v2 4/6] drm/atomic: Don't skip drm_bridge_*() calls if !drm_encoder_helper_funcs

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:20PM +0200, Noralf Trønnes wrote:
> Don't skip drm_bridge_*() calls if encoder->helper_private is NULL.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 027b227..46a3201 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -384,8 +384,6 @@ mode_fixup(struct drm_atomic_state *state)
>  		 */
>  		encoder = conn_state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
>  				&crtc_state->adjusted_mode);
> @@ -394,7 +392,7 @@ mode_fixup(struct drm_atomic_state *state)
>  			return -EINVAL;
>  		}
>  
> -		if (funcs->atomic_check) {
> +		if (funcs && funcs->atomic_check) {
>  			ret = funcs->atomic_check(encoder, crtc_state,
>  						  conn_state);
>  			if (ret) {
> @@ -402,7 +400,7 @@ mode_fixup(struct drm_atomic_state *state)
>  						 encoder->base.id, encoder->name);
>  				return ret;
>  			}
> -		} else if (funcs->mode_fixup) {
> +		} else if (funcs && funcs->mode_fixup) {
>  			ret = funcs->mode_fixup(encoder, &crtc_state->mode,
>  						&crtc_state->adjusted_mode);
>  			if (!ret) {
> @@ -696,8 +694,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  			continue;
>  
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		DRM_DEBUG_ATOMIC("disabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -709,12 +705,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		drm_bridge_disable(encoder->bridge);
>  
>  		/* Right function depends upon target state. */
> -		if (connector->state->crtc && funcs->prepare)
> -			funcs->prepare(encoder);
> -		else if (funcs->disable)
> -			funcs->disable(encoder);
> -		else if (funcs->dpms)
> -			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +		if (funcs) {
> +			if (connector->state->crtc && funcs->prepare)
> +				funcs->prepare(encoder);
> +			else if (funcs->disable)
> +				funcs->disable(encoder);
> +			else if (funcs->dpms)
> +				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +		}
>  
>  		drm_bridge_post_disable(encoder->bridge);
>  	}
> @@ -861,9 +859,6 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
> -
>  		new_crtc_state = connector->state->crtc->state;
>  		mode = &new_crtc_state->mode;
>  		adjusted_mode = &new_crtc_state->adjusted_mode;
> @@ -878,7 +873,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call mode_set hooks twice.
>  		 */
> -		if (funcs->mode_set)
> +		if (funcs && funcs->mode_set)
>  			funcs->mode_set(encoder, mode, adjusted_mode);
>  
>  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> @@ -969,8 +964,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
> -		if (!funcs)
> -			continue;
>  
>  		DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
>  				 encoder->base.id, encoder->name);
> @@ -981,10 +974,12 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  		 */
>  		drm_bridge_pre_enable(encoder->bridge);
>  
> -		if (funcs->enable)
> -			funcs->enable(encoder);
> -		else if (funcs->commit)
> -			funcs->commit(encoder);
> +		if (funcs) {
> +			if (funcs->enable)
> +				funcs->enable(encoder);
> +			else if (funcs->commit)
> +				funcs->commit(encoder);
> +		}
>  
>  		drm_bridge_enable(encoder->bridge);
>  	}
> -- 
> 2.8.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/atomic: Add drm_atomic_helper_best_encoder()

[Noralf Trønnes]

Add (struct drm_connector_helper_funcs *)->best_encoder callback helper
for connectors that support exactly 1 encoder, statically determined at
driver init time.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 46a3201..43a0b3d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2483,6 +2483,23 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
 
 /**
+ * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
+ *                                  ->best_encoder callback
+ * @connector: Connector control structure
+ *
+ * This is a &drm_connector_helper_funcs ->best_encoder callback helper for
+ * connectors that support exactly 1 encoder, statically determined at driver
+ * init time.
+ */
+struct drm_encoder *
+drm_atomic_helper_best_encoder(struct drm_connector *connector)
+{
+	WARN_ON(connector->encoder_ids[1]);
+	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
+}
+EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
+
+/**
  * DOC: atomic state reset and initialization
  *
  * Both the drm core and the atomic helpers assume that there is always the full
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 0364287..ccca709 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -110,6 +110,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				uint32_t flags);
 int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 				     int mode);
+struct drm_encoder *
+drm_atomic_helper_best_encoder(struct drm_connector *connector);
 
 /* default implementations for state handling */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
-- 
2.8.2

[PATCH v2 5/6] drm/atomic: Add drm_atomic_helper_best_encoder()

[Noralf Trønnes]

Add (struct drm_connector_helper_funcs *)->best_encoder callback helper
for connectors that support exactly 1 encoder, statically determined at
driver init time.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 46a3201..43a0b3d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2483,6 +2483,23 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
 
 /**
+ * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
+ *                                  ->best_encoder callback
+ * @connector: Connector control structure
+ *
+ * This is a &drm_connector_helper_funcs ->best_encoder callback helper for
+ * connectors that support exactly 1 encoder, statically determined at driver
+ * init time.
+ */
+struct drm_encoder *
+drm_atomic_helper_best_encoder(struct drm_connector *connector)
+{
+	WARN_ON(connector->encoder_ids[1]);
+	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
+}
+EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
+
+/**
  * DOC: atomic state reset and initialization
  *
  * Both the drm core and the atomic helpers assume that there is always the full
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 0364287..ccca709 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -110,6 +110,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				uint32_t flags);
 int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 				     int mode);
+struct drm_encoder *
+drm_atomic_helper_best_encoder(struct drm_connector *connector);
 
 /* default implementations for state handling */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
-- 
2.8.2

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

Re: [PATCH v2 5/6] drm/atomic: Add drm_atomic_helper_best_encoder()

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:21PM +0200, Noralf Trønnes wrote:
> Add (struct drm_connector_helper_funcs *)->best_encoder callback helper
> for connectors that support exactly 1 encoder, statically determined at
> driver init time.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 46a3201..43a0b3d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2483,6 +2483,23 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>  
>  /**
> + * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
> + *                                  ->best_encoder callback
> + * @connector: Connector control structure
> + *
> + * This is a &drm_connector_helper_funcs ->best_encoder callback helper for
> + * connectors that support exactly 1 encoder, statically determined at driver
> + * init time.
> + */
> +struct drm_encoder *
> +drm_atomic_helper_best_encoder(struct drm_connector *connector)
> +{
> +	WARN_ON(connector->encoder_ids[1]);
> +	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
> +
> +/**
>   * DOC: atomic state reset and initialization
>   *
>   * Both the drm core and the atomic helpers assume that there is always the full
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 0364287..ccca709 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -110,6 +110,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				uint32_t flags);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
> +struct drm_encoder *
> +drm_atomic_helper_best_encoder(struct drm_connector *connector);
>  
>  /* default implementations for state handling */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
> -- 
> 2.8.2
> 

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

Re: [PATCH v2 5/6] drm/atomic: Add drm_atomic_helper_best_encoder()

[Daniel Vetter]

On Wed, May 11, 2016 at 06:09:21PM +0200, Noralf Trønnes wrote:
> Add (struct drm_connector_helper_funcs *)->best_encoder callback helper
> for connectors that support exactly 1 encoder, statically determined at
> driver init time.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 46a3201..43a0b3d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2483,6 +2483,23 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>  
>  /**
> + * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
> + *                                  ->best_encoder callback
> + * @connector: Connector control structure
> + *
> + * This is a &drm_connector_helper_funcs ->best_encoder callback helper for
> + * connectors that support exactly 1 encoder, statically determined at driver
> + * init time.
> + */
> +struct drm_encoder *
> +drm_atomic_helper_best_encoder(struct drm_connector *connector)
> +{
> +	WARN_ON(connector->encoder_ids[1]);
> +	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
> +
> +/**
>   * DOC: atomic state reset and initialization
>   *
>   * Both the drm core and the atomic helpers assume that there is always the full
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 0364287..ccca709 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -110,6 +110,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				uint32_t flags);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
> +struct drm_encoder *
> +drm_atomic_helper_best_encoder(struct drm_connector *connector);
>  
>  /* default implementations for state handling */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
> -- 
> 2.8.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

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

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

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

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

[Noralf Trønnes]

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

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

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

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

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

[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

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

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

[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

> ---
> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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?


Paul Bolle

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

[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?


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

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

[Noralf Trønnes]

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.

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

[Noralf Trønnes]

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.

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

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

[Noralf Trønnes]

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.

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

[Noralf Trønnes]

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.

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

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

[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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

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.

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

[Noralf Trønnes]

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.

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

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

[Daniel Vetter]

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

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

[Daniel Vetter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel