Hi all,
This patch series contains various improvements to the documentation and
comments related to atomic modesetting. Hopefully, it will ease the job
of DRM novice who want to tackle the daunting task of converting a
legacy DRM driver to atomic modesetting.
Thanks for your comments!
Geert Uytterhoeven (3):
drm/todo: Add atomic modesetting references
drm: Remove references to removed transitional helpers
drm: Fix references to drm_plane_helper_check_state()
Documentation/gpu/todo.rst | 9 ++++-
drivers/gpu/drm/drm_plane_helper.c | 8 ----
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 +-
drivers/gpu/drm/tidss/tidss_plane.c | 3 +-
include/drm/drm_crtc.h | 5 ---
include/drm/drm_modeset_helper_vtables.h | 48 +++++++++++-------------
6 files changed, 32 insertions(+), 44 deletions(-)
--
2.34.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
As of commit a01cb8ba3f628293 ("drm: Move drm_plane_helper_check_state()
into drm_atomic_helper.c"), drm_plane_helper_check_state() no longer
exists, but is part of drm_atomic_helper_check_plane_state().
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Interestingly, all these comments appeared only after the commit that
removed the function...
This is against next-20230602, which does not have the
drivers/gpu/drm/{ => renesas}/rcar-du move yet.
Biju: you're adding a new copy in
drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
---
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++-
drivers/gpu/drm/tidss/tidss_plane.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index d759e019218181ce..e445fac8e0b46c21 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -600,7 +600,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
if (!state->crtc) {
/*
* The visible field is not reset by the DRM core but only
- * updated by drm_plane_helper_check_state(), set it manually.
+ * updated by drm_atomic_helper_check_plane_state(), set it
+ * manually.
*/
state->visible = false;
*format = NULL;
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
index 6bdd6e4a955ab3cc..e1c0ef0c3894c855 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -38,7 +38,8 @@ static int tidss_plane_atomic_check(struct drm_plane *plane,
if (!new_plane_state->crtc) {
/*
* The visible field is not reset by the DRM core but only
- * updated by drm_plane_helper_check_state(), set it manually.
+ * updated by drm_atomic_helper_check_plane_state(), set it
+ * manually.
*/
new_plane_state->visible = false;
return 0;
--
2.34.1
The section about converting existing KMS drivers to atomic modesetting
mentions the existence of a conversion guide, but does not reference it.
While the guide is old and rusty, it still contains useful information,
so add a link to it. Also link to the LWN.net articles that give an
overview about the atomic mode setting design.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Documentation/gpu/todo.rst | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 68bdafa0284f55f6..51eb67f5268c5ec1 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -49,14 +49,19 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android
really want an atomic modeset interface, so this is all about the bright
future.
-There is a conversion guide for atomic and all you need is a GPU for a
+There is a conversion guide for atomic[1] and all you need is a GPU for a
non-converted driver (again virtual HW drivers for KVM are still all
-suitable).
+suitable). The "Atomic mode setting design overview" series [2][3] at
+LWN.net can also be helpful.
As part of this drivers also need to convert to universal plane (which means
exposing primary & cursor as proper plane objects). But that's much easier to
do by directly using the new atomic helper driver callbacks.
+ - [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
+ - [2] https://lwn.net/Articles/653071/
+ - [3] https://lwn.net/Articles/653466/
+
Contact: Daniel Vetter, respective driver maintainers
Level: Advanced
--
2.34.1
The transitional helpers were removed a long time ago, but some
references stuck. Remove them.
Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
It doesn't look like the drm_encoder_helper_funcs were ever used by the
transitional plane helpers?
---
drivers/gpu/drm/drm_plane_helper.c | 8 ----
include/drm/drm_crtc.h | 5 ---
include/drm/drm_modeset_helper_vtables.h | 48 +++++++++++-------------
3 files changed, 21 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index c91e454eba097942..be45bdb58d849653 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -51,14 +51,6 @@
* planes, and newly merged drivers must not rely upon these transitional
* helpers.
*
- * The second part also implements transitional helpers which allow drivers to
- * gradually switch to the atomic helper infrastructure for plane updates. Once
- * that switch is complete drivers shouldn't use these any longer, instead using
- * the proper legacy implementations for update and disable plane hooks provided
- * by the atomic helpers.
- *
- * Again drivers are strongly urged to switch to the new interfaces.
- *
* The plane helpers share the function table structures with other helpers,
* specifically also the atomic helpers. See &struct drm_plane_helper_funcs for
* the details.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8e1cbc75143ef216..8b48a1974da3143c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -77,11 +77,6 @@ struct drm_plane_helper_funcs;
* intended to indicate whether a full modeset is needed, rather than strictly
* describing what has changed in a commit. See also:
* drm_atomic_crtc_needs_modeset()
- *
- * WARNING: Transitional helpers (like drm_helper_crtc_mode_set() or
- * drm_helper_crtc_mode_set_base()) do not maintain many of the derived control
- * state like @plane_mask so drivers not converted over to atomic helpers should
- * not rely on these being accurate!
*/
struct drm_crtc_state {
/** @crtc: backpointer to the CRTC */
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 965faf082a6d1acb..e3c3ac615909474b 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -59,8 +59,8 @@ enum mode_set_atomic {
/**
* struct drm_crtc_helper_funcs - helper operations for CRTCs
*
- * These hooks are used by the legacy CRTC helpers, the transitional plane
- * helpers and the new atomic modesetting helpers.
+ * These hooks are used by the legacy CRTC helpers and the new atomic
+ * modesetting helpers.
*/
struct drm_crtc_helper_funcs {
/**
@@ -216,9 +216,7 @@ struct drm_crtc_helper_funcs {
*
* This callback is used to update the display mode of a CRTC without
* changing anything of the primary plane configuration. This fits the
- * requirement of atomic and hence is used by the atomic helpers. It is
- * also used by the transitional plane helpers to implement a
- * @mode_set hook in drm_helper_crtc_mode_set().
+ * requirement of atomic and hence is used by the atomic helpers.
*
* Note that the display pipe is completely off when this function is
* called. Atomic drivers which need hardware to be running before they
@@ -333,8 +331,8 @@ struct drm_crtc_helper_funcs {
* all updated. Again the recommendation is to just call check helpers
* until a maximal configuration is reached.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional.
*
* NOTE:
*
@@ -373,8 +371,8 @@ struct drm_crtc_helper_funcs {
* has picked. See drm_atomic_helper_commit_planes() for a discussion of
* the tradeoffs and variants of plane commit helpers.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional.
*/
void (*atomic_begin)(struct drm_crtc *crtc,
struct drm_atomic_state *state);
@@ -397,8 +395,8 @@ struct drm_crtc_helper_funcs {
* has picked. See drm_atomic_helper_commit_planes() for a discussion of
* the tradeoffs and variants of plane commit helpers.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional.
*/
void (*atomic_flush)(struct drm_crtc *crtc,
struct drm_atomic_state *state);
@@ -507,8 +505,8 @@ static inline void drm_crtc_helper_add(struct drm_crtc *crtc,
/**
* struct drm_encoder_helper_funcs - helper operations for encoders
*
- * These hooks are used by the legacy CRTC helpers, the transitional plane
- * helpers and the new atomic modesetting helpers.
+ * These hooks are used by the legacy CRTC helpers and the new atomic
+ * modesetting helpers.
*/
struct drm_encoder_helper_funcs {
/**
@@ -1185,8 +1183,7 @@ static inline void drm_connector_helper_add(struct drm_connector *connector,
/**
* struct drm_plane_helper_funcs - helper operations for planes
*
- * These functions are used by the atomic helpers and by the transitional plane
- * helpers.
+ * These functions are used by the atomic helpers.
*/
struct drm_plane_helper_funcs {
/**
@@ -1221,9 +1218,8 @@ struct drm_plane_helper_funcs {
* The helpers will call @cleanup_fb with matching arguments for every
* successful call to this hook.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional. See @begin_fb_access
- * for preparing per-commit resources.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional. See @begin_fb_access for preparing per-commit resources.
*
* RETURNS:
*
@@ -1240,8 +1236,8 @@ struct drm_plane_helper_funcs {
* This hook is called to clean up any resources allocated for the given
* framebuffer and plane configuration in @prepare_fb.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional.
*/
void (*cleanup_fb)(struct drm_plane *plane,
struct drm_plane_state *old_state);
@@ -1295,8 +1291,8 @@ struct drm_plane_helper_funcs {
* all updated. Again the recommendation is to just call check helpers
* until a maximal configuration is reached.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional.
*
* NOTE:
*
@@ -1326,8 +1322,7 @@ struct drm_plane_helper_funcs {
* has picked. See drm_atomic_helper_commit_planes() for a discussion of
* the tradeoffs and variants of plane commit helpers.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional.
+ * This callback is used by the atomic modeset helpers, but it is optional.
*/
void (*atomic_update)(struct drm_plane *plane,
struct drm_atomic_state *state);
@@ -1376,9 +1371,8 @@ struct drm_plane_helper_funcs {
* has picked. See drm_atomic_helper_commit_planes() for a discussion of
* the tradeoffs and variants of plane commit helpers.
*
- * This callback is used by the atomic modeset helpers and by the
- * transitional plane helpers, but it is optional. It's intended to
- * reverse the effects of @atomic_enable.
+ * This callback is used by the atomic modeset helpers, but it is
+ * optional. It's intended to reverse the effects of @atomic_enable.
*/
void (*atomic_disable)(struct drm_plane *plane,
struct drm_atomic_state *state);
--
2.34.1
Hi Geert,
Thanks for the feedback.
> Subject: [PATCH 3/3] drm: Fix references to
> drm_plane_helper_check_state()
>
> As of commit a01cb8ba3f628293 ("drm: Move drm_plane_helper_check_state()
> into drm_atomic_helper.c"), drm_plane_helper_check_state() no longer
> exists, but is part of drm_atomic_helper_check_plane_state().
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Interestingly, all these comments appeared only after the commit that
> removed the function...
>
> This is against next-20230602, which does not have the drivers/gpu/drm/{
> => renesas}/rcar-du move yet.
>
> Biju: you're adding a new copy in
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
OK, will update the comment in __rzg2l_du_vsp_plane_atomic_check()
as it is moved to drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.c
based on Laurent's review comment.
Cheers,
Biju
> ---
> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++-
> drivers/gpu/drm/tidss/tidss_plane.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index d759e019218181ce..e445fac8e0b46c21 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -600,7 +600,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane
> *plane,
> if (!state->crtc) {
> /*
> * The visible field is not reset by the DRM core but only
> - * updated by drm_plane_helper_check_state(), set it
> manually.
> + * updated by drm_atomic_helper_check_plane_state(), set it
> + * manually.
> */
> state->visible = false;
> *format = NULL;
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c
> b/drivers/gpu/drm/tidss/tidss_plane.c
> index 6bdd6e4a955ab3cc..e1c0ef0c3894c855 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -38,7 +38,8 @@ static int tidss_plane_atomic_check(struct drm_plane
> *plane,
> if (!new_plane_state->crtc) {
> /*
> * The visible field is not reset by the DRM core but only
> - * updated by drm_plane_helper_check_state(), set it
> manually.
> + * updated by drm_atomic_helper_check_plane_state(), set it
> + * manually.
> */
> new_plane_state->visible = false;
> return 0;
> --
> 2.34.1
Hi Geert,
Thank you for the patch.
On Fri, Jun 02, 2023 at 11:11:34AM +0200, Geert Uytterhoeven wrote:
> The section about converting existing KMS drivers to atomic modesetting
> mentions the existence of a conversion guide, but does not reference it.
> While the guide is old and rusty, it still contains useful information,
> so add a link to it. Also link to the LWN.net articles that give an
> overview about the atomic mode setting design.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Documentation/gpu/todo.rst | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 68bdafa0284f55f6..51eb67f5268c5ec1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -49,14 +49,19 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android
> really want an atomic modeset interface, so this is all about the bright
> future.
>
> -There is a conversion guide for atomic and all you need is a GPU for a
> +There is a conversion guide for atomic[1] and all you need is a GPU for a
> non-converted driver (again virtual HW drivers for KVM are still all
> -suitable).
> +suitable). The "Atomic mode setting design overview" series [2][3] at
s/ / /
Reviewed-by: Laurent Pinchart <[email protected]>
> +LWN.net can also be helpful.
>
> As part of this drivers also need to convert to universal plane (which means
> exposing primary & cursor as proper plane objects). But that's much easier to
> do by directly using the new atomic helper driver callbacks.
>
> + - [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
> + - [2] https://lwn.net/Articles/653071/
> + - [3] https://lwn.net/Articles/653466/
> +
> Contact: Daniel Vetter, respective driver maintainers
>
> Level: Advanced
--
Regards,
Laurent Pinchart
Geert Uytterhoeven <[email protected]> writes:
Hello Geert,
Thanks for your patch.
> The section about converting existing KMS drivers to atomic modesetting
> mentions the existence of a conversion guide, but does not reference it.
> While the guide is old and rusty, it still contains useful information,
> so add a link to it. Also link to the LWN.net articles that give an
> overview about the atomic mode setting design.
>
This is a great idea and agree that would be very useful to have these.
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Documentation/gpu/todo.rst | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 68bdafa0284f55f6..51eb67f5268c5ec1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -49,14 +49,19 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android
> really want an atomic modeset interface, so this is all about the bright
> future.
>
> -There is a conversion guide for atomic and all you need is a GPU for a
> +There is a conversion guide for atomic[1] and all you need is a GPU for a
> non-converted driver (again virtual HW drivers for KVM are still all
> -suitable).
Are any of the virtual drivers not yet ported to atomic? This sentence
seems to be outdated and maybe you could remove it on a following patch?
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi Geert,
Thank you for the patch.
On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> The transitional helpers were removed a long time ago, but some
> references stuck. Remove them.
>
> Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> It doesn't look like the drm_encoder_helper_funcs were ever used by the
> transitional plane helpers?
> ---
> drivers/gpu/drm/drm_plane_helper.c | 8 ----
> include/drm/drm_crtc.h | 5 ---
> include/drm/drm_modeset_helper_vtables.h | 48 +++++++++++-------------
> 3 files changed, 21 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index c91e454eba097942..be45bdb58d849653 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -51,14 +51,6 @@
> * planes, and newly merged drivers must not rely upon these transitional
> * helpers.
> *
The first paragraph starts with "This helper library has two parts.". As
you're dropping the mention of the second part, I think you should
rework the first paragraph too.
> - * The second part also implements transitional helpers which allow drivers to
> - * gradually switch to the atomic helper infrastructure for plane updates. Once
> - * that switch is complete drivers shouldn't use these any longer, instead using
> - * the proper legacy implementations for update and disable plane hooks provided
> - * by the atomic helpers.
> - *
> - * Again drivers are strongly urged to switch to the new interfaces.
> - *
> * The plane helpers share the function table structures with other helpers,
> * specifically also the atomic helpers. See &struct drm_plane_helper_funcs for
> * the details.
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8e1cbc75143ef216..8b48a1974da3143c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -77,11 +77,6 @@ struct drm_plane_helper_funcs;
> * intended to indicate whether a full modeset is needed, rather than strictly
> * describing what has changed in a commit. See also:
> * drm_atomic_crtc_needs_modeset()
> - *
> - * WARNING: Transitional helpers (like drm_helper_crtc_mode_set() or
> - * drm_helper_crtc_mode_set_base()) do not maintain many of the derived control
> - * state like @plane_mask so drivers not converted over to atomic helpers should
> - * not rely on these being accurate!
> */
> struct drm_crtc_state {
> /** @crtc: backpointer to the CRTC */
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 965faf082a6d1acb..e3c3ac615909474b 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -59,8 +59,8 @@ enum mode_set_atomic {
> /**
> * struct drm_crtc_helper_funcs - helper operations for CRTCs
> *
> - * These hooks are used by the legacy CRTC helpers, the transitional plane
> - * helpers and the new atomic modesetting helpers.
> + * These hooks are used by the legacy CRTC helpers and the new atomic
> + * modesetting helpers.
> */
> struct drm_crtc_helper_funcs {
> /**
> @@ -216,9 +216,7 @@ struct drm_crtc_helper_funcs {
> *
> * This callback is used to update the display mode of a CRTC without
> * changing anything of the primary plane configuration. This fits the
> - * requirement of atomic and hence is used by the atomic helpers. It is
> - * also used by the transitional plane helpers to implement a
> - * @mode_set hook in drm_helper_crtc_mode_set().
> + * requirement of atomic and hence is used by the atomic helpers.
> *
> * Note that the display pipe is completely off when this function is
> * called. Atomic drivers which need hardware to be running before they
> @@ -333,8 +331,8 @@ struct drm_crtc_helper_funcs {
> * all updated. Again the recommendation is to just call check helpers
> * until a maximal configuration is reached.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional.
> *
> * NOTE:
> *
> @@ -373,8 +371,8 @@ struct drm_crtc_helper_funcs {
> * has picked. See drm_atomic_helper_commit_planes() for a discussion of
> * the tradeoffs and variants of plane commit helpers.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional.
> */
> void (*atomic_begin)(struct drm_crtc *crtc,
> struct drm_atomic_state *state);
> @@ -397,8 +395,8 @@ struct drm_crtc_helper_funcs {
> * has picked. See drm_atomic_helper_commit_planes() for a discussion of
> * the tradeoffs and variants of plane commit helpers.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional.
> */
> void (*atomic_flush)(struct drm_crtc *crtc,
> struct drm_atomic_state *state);
> @@ -507,8 +505,8 @@ static inline void drm_crtc_helper_add(struct drm_crtc *crtc,
> /**
> * struct drm_encoder_helper_funcs - helper operations for encoders
> *
> - * These hooks are used by the legacy CRTC helpers, the transitional plane
> - * helpers and the new atomic modesetting helpers.
> + * These hooks are used by the legacy CRTC helpers and the new atomic
> + * modesetting helpers.
> */
> struct drm_encoder_helper_funcs {
> /**
> @@ -1185,8 +1183,7 @@ static inline void drm_connector_helper_add(struct drm_connector *connector,
> /**
> * struct drm_plane_helper_funcs - helper operations for planes
> *
> - * These functions are used by the atomic helpers and by the transitional plane
> - * helpers.
> + * These functions are used by the atomic helpers.
> */
> struct drm_plane_helper_funcs {
> /**
> @@ -1221,9 +1218,8 @@ struct drm_plane_helper_funcs {
> * The helpers will call @cleanup_fb with matching arguments for every
> * successful call to this hook.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional. See @begin_fb_access
> - * for preparing per-commit resources.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional. See @begin_fb_access for preparing per-commit resources.
> *
> * RETURNS:
> *
> @@ -1240,8 +1236,8 @@ struct drm_plane_helper_funcs {
> * This hook is called to clean up any resources allocated for the given
> * framebuffer and plane configuration in @prepare_fb.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional.
> */
> void (*cleanup_fb)(struct drm_plane *plane,
> struct drm_plane_state *old_state);
> @@ -1295,8 +1291,8 @@ struct drm_plane_helper_funcs {
> * all updated. Again the recommendation is to just call check helpers
> * until a maximal configuration is reached.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional.
> *
> * NOTE:
> *
> @@ -1326,8 +1322,7 @@ struct drm_plane_helper_funcs {
> * has picked. See drm_atomic_helper_commit_planes() for a discussion of
> * the tradeoffs and variants of plane commit helpers.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional.
> + * This callback is used by the atomic modeset helpers, but it is optional.
> */
> void (*atomic_update)(struct drm_plane *plane,
> struct drm_atomic_state *state);
> @@ -1376,9 +1371,8 @@ struct drm_plane_helper_funcs {
> * has picked. See drm_atomic_helper_commit_planes() for a discussion of
> * the tradeoffs and variants of plane commit helpers.
> *
> - * This callback is used by the atomic modeset helpers and by the
> - * transitional plane helpers, but it is optional. It's intended to
> - * reverse the effects of @atomic_enable.
> + * This callback is used by the atomic modeset helpers, but it is
> + * optional. It's intended to reverse the effects of @atomic_enable.
> */
> void (*atomic_disable)(struct drm_plane *plane,
> struct drm_atomic_state *state);
--
Regards,
Laurent Pinchart
Hi Geert,
Thank you for the patch.
On Fri, Jun 02, 2023 at 11:11:36AM +0200, Geert Uytterhoeven wrote:
> As of commit a01cb8ba3f628293 ("drm: Move drm_plane_helper_check_state()
> into drm_atomic_helper.c"), drm_plane_helper_check_state() no longer
> exists, but is part of drm_atomic_helper_check_plane_state().
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Interestingly, all these comments appeared only after the commit that
> removed the function...
>
> This is against next-20230602, which does not have the
> drivers/gpu/drm/{ => renesas}/rcar-du move yet.
>
> Biju: you're adding a new copy in
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> ---
> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++-
> drivers/gpu/drm/tidss/tidss_plane.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index d759e019218181ce..e445fac8e0b46c21 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -600,7 +600,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> if (!state->crtc) {
> /*
> * The visible field is not reset by the DRM core but only
> - * updated by drm_plane_helper_check_state(), set it manually.
> + * updated by drm_atomic_helper_check_plane_state(), set it
> + * manually.
> */
> state->visible = false;
> *format = NULL;
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index 6bdd6e4a955ab3cc..e1c0ef0c3894c855 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -38,7 +38,8 @@ static int tidss_plane_atomic_check(struct drm_plane *plane,
> if (!new_plane_state->crtc) {
> /*
> * The visible field is not reset by the DRM core but only
> - * updated by drm_plane_helper_check_state(), set it manually.
> + * updated by drm_atomic_helper_check_plane_state(), set it
> + * manually.
> */
> new_plane_state->visible = false;
> return 0;
--
Regards,
Laurent Pinchart
Hi Laurent,
On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart
<[email protected]> wrote:
> On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> > The transitional helpers were removed a long time ago, but some
> > references stuck. Remove them.
> >
> > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -51,14 +51,6 @@
> > * planes, and newly merged drivers must not rely upon these transitional
> > * helpers.
> > *
>
> The first paragraph starts with "This helper library has two parts.". As
> you're dropping the mention of the second part, I think you should
> rework the first paragraph too.
That was my initial thought, too.
However, the code still has a second part, not related to the topic of
the first part (primary plane support).
>
> > - * The second part also implements transitional helpers which allow drivers to
> > - * gradually switch to the atomic helper infrastructure for plane updates. Once
> > - * that switch is complete drivers shouldn't use these any longer, instead using
> > - * the proper legacy implementations for update and disable plane hooks provided
> > - * by the atomic helpers.
> > - *
> > - * Again drivers are strongly urged to switch to the new interfaces.
> > - *
> > * The plane helpers share the function table structures with other helpers,
> > * specifically also the atomic helpers. See &struct drm_plane_helper_funcs for
> > * the details.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Javier,
On Fri, Jun 2, 2023 at 12:39 PM Javier Martinez Canillas
<[email protected]> wrote:
> Geert Uytterhoeven <[email protected]> writes:
> > The section about converting existing KMS drivers to atomic modesetting
> > mentions the existence of a conversion guide, but does not reference it.
> > While the guide is old and rusty, it still contains useful information,
> > so add a link to it. Also link to the LWN.net articles that give an
> > overview about the atomic mode setting design.
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -49,14 +49,19 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android
> > really want an atomic modeset interface, so this is all about the bright
> > future.
> >
> > -There is a conversion guide for atomic and all you need is a GPU for a
> > +There is a conversion guide for atomic[1] and all you need is a GPU for a
> > non-converted driver (again virtual HW drivers for KVM are still all
> > -suitable).
>
> Are any of the virtual drivers not yet ported to atomic? This sentence
> seems to be outdated and maybe you could remove it on a following patch?
Good question. I'm not sure which driver(s) this refers to.
drivers/gpu/drm/vkms/ was introduced much later, and always had
DRIVER_ATOMIC. Perhaps just the boochs driver, which was converted?
> Reviewed-by: Javier Martinez Canillas <[email protected]>
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Fri, Jun 02, 2023 at 01:17:58PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart wrote:
> > On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> > > The transitional helpers were removed a long time ago, but some
> > > references stuck. Remove them.
> > >
> > > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -51,14 +51,6 @@
> > > * planes, and newly merged drivers must not rely upon these transitional
> > > * helpers.
> > > *
> >
> > The first paragraph starts with "This helper library has two parts.". As
> > you're dropping the mention of the second part, I think you should
> > rework the first paragraph too.
>
> That was my initial thought, too.
> However, the code still has a second part, not related to the topic of
> the first part (primary plane support).
How about mentioning that in the comment then ?
> > > - * The second part also implements transitional helpers which allow drivers to
> > > - * gradually switch to the atomic helper infrastructure for plane updates. Once
> > > - * that switch is complete drivers shouldn't use these any longer, instead using
> > > - * the proper legacy implementations for update and disable plane hooks provided
> > > - * by the atomic helpers.
> > > - *
> > > - * Again drivers are strongly urged to switch to the new interfaces.
> > > - *
> > > * The plane helpers share the function table structures with other helpers,
> > > * specifically also the atomic helpers. See &struct drm_plane_helper_funcs for
> > > * the details.
--
Regards,
Laurent Pinchart
Hi Laurent,
On Fri, Jun 2, 2023 at 1:36 PM Laurent Pinchart
<[email protected]> wrote:
> On Fri, Jun 02, 2023 at 01:17:58PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart wrote:
> > > On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> > > > The transitional helpers were removed a long time ago, but some
> > > > references stuck. Remove them.
> > > >
> > > > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> > > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> >
> > > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > > @@ -51,14 +51,6 @@
> > > > * planes, and newly merged drivers must not rely upon these transitional
> > > > * helpers.
> > > > *
> > >
> > > The first paragraph starts with "This helper library has two parts.". As
> > > you're dropping the mention of the second part, I think you should
> > > rework the first paragraph too.
> >
> > That was my initial thought, too.
> > However, the code still has a second part, not related to the topic of
> > the first part (primary plane support).
>
> How about mentioning that in the comment then ?
Any suggestion about the wording?
The drm novice in me would write "The second part is not about
primary plane support" ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Jun 02, 2023 at 11:11:34AM +0200, Geert Uytterhoeven wrote:
> -There is a conversion guide for atomic and all you need is a GPU for a
> +There is a conversion guide for atomic[1] and all you need is a GPU for a
> non-converted driver (again virtual HW drivers for KVM are still all
> -suitable).
> +suitable). The "Atomic mode setting design overview" series [2][3] at
> +LWN.net can also be helpful.
>
> As part of this drivers also need to convert to universal plane (which means
> exposing primary & cursor as proper plane objects). But that's much easier to
> do by directly using the new atomic helper driver callbacks.
>
> + - [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
> + - [2] https://lwn.net/Articles/653071/
> + - [3] https://lwn.net/Articles/653466/
> +
Looks like footnotes better serve these links above:
---- >8 ----
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 51eb67f5268c5e..6ea92f48a2e21c 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -49,18 +49,18 @@ converted over. Modern compositors like Wayland or Surfaceflinger on Android
really want an atomic modeset interface, so this is all about the bright
future.
-There is a conversion guide for atomic[1] and all you need is a GPU for a
+There is a conversion guide for atomic [1]_ and all you need is a GPU for a
non-converted driver (again virtual HW drivers for KVM are still all
-suitable). The "Atomic mode setting design overview" series [2][3] at
+suitable). The "Atomic mode setting design overview" series [2]_ [3]_ at
LWN.net can also be helpful.
As part of this drivers also need to convert to universal plane (which means
exposing primary & cursor as proper plane objects). But that's much easier to
do by directly using the new atomic helper driver callbacks.
- - [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
- - [2] https://lwn.net/Articles/653071/
- - [3] https://lwn.net/Articles/653466/
+ .. [1] https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
+ .. [2] https://lwn.net/Articles/653071/
+ .. [3] https://lwn.net/Articles/653466/
Contact: Daniel Vetter, respective driver maintainers
Thanks.
--
An old man doll... just what I always wanted! - Clara
On Fri, Jun 2, 2023 at 1:17 PM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart
> <[email protected]> wrote:
> > On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> > > The transitional helpers were removed a long time ago, but some
> > > references stuck. Remove them.
> > >
> > > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -51,14 +51,6 @@
> > > * planes, and newly merged drivers must not rely upon these transitional
> > > * helpers.
> > > *
> >
> > The first paragraph starts with "This helper library has two parts.". As
> > you're dropping the mention of the second part, I think you should
> > rework the first paragraph too.
>
> That was my initial thought, too.
> However, the code still has a second part, not related to the topic of
> the first part (primary plane support).
Upon deeper investigation, all of this is related to primary plane
support. Will update...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds