[PATCH 2/2] drm: add docs for standard CRTC properties

[PATCH 2/2] drm: add docs for standard CRTC properties

[Simon Ser]

This patch adds docs for the ACTIVE and MODE_ID CRTC properties.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Michel Dänzer <michel@daenzer.net>
---

Let me know if you think of other things to add.

Should we refer to ALLOW_MODESET in the MODE_ID docs?

 Documentation/gpu/drm-kms.rst |  5 +++++
 drivers/gpu/drm/drm_crtc.c    | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 397314d08f77..4289b7205696 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -460,6 +460,11 @@ HDMI Specific Connector Properties
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
    :doc: HDMI connector properties
 
+Standard CRTC Properties
+
+.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
+   :doc: standard CRTC properties
+
 Plane Composition Properties
 ----------------------------
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4936e1080e41..c28e662c1e93 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -204,6 +204,22 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
 	return fence;
 }
 
+/**
+ * DOC: standard CRTC properties
+ *
+ * DRM CRTCs have a few standardized properties:
+ *
+ * ACTIVE:
+ * 	Atomic property for setting the power state of the CRTC. When set to 1 the
+ * 	CRTC will actively display content. When set to 0 the CRTC will be powered
+ * 	off. There is no expectation that user-space will reset CRTC resources like
+ * 	the mode and planes when setting ACTIVE to 0.
+ * MODE_ID:
+ * 	Atomic property for setting the CRTC display timings. The value is the ID
+ * 	of a blob containing the DRM mode info. To disable the CRTC, user-space
+ * 	must set this property to 0.
+ */
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
-- 
2.26.2


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

Re: [PATCH 2/2] drm: add docs for standard CRTC properties

[Daniel Vetter]

On Mon, May 18, 2020 at 02:22:47PM +0000, Simon Ser wrote:
> This patch adds docs for the ACTIVE and MODE_ID CRTC properties.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Cc: Michel Dänzer <michel@daenzer.net>
> ---
> 
> Let me know if you think of other things to add.
> 
> Should we refer to ALLOW_MODESET in the MODE_ID docs?
> 
>  Documentation/gpu/drm-kms.rst |  5 +++++
>  drivers/gpu/drm/drm_crtc.c    | 16 ++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 397314d08f77..4289b7205696 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -460,6 +460,11 @@ HDMI Specific Connector Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
>     :doc: HDMI connector properties
>  
> +Standard CRTC Properties

Missing the 
--------------------------
line here so this reanders neatly as a title and works with the sidebar
toc. Please check the output to make sure that all looks good.


> +
> +.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
> +   :doc: standard CRTC properties
> +
>  Plane Composition Properties
>  ----------------------------
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4936e1080e41..c28e662c1e93 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -204,6 +204,22 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
>  	return fence;
>  }
>  
> +/**
> + * DOC: standard CRTC properties
> + *
> + * DRM CRTCs have a few standardized properties:
> + *
> + * ACTIVE:
> + * 	Atomic property for setting the power state of the CRTC. When set to 1 the
> + * 	CRTC will actively display content. When set to 0 the CRTC will be powered
> + * 	off. There is no expectation that user-space will reset CRTC resources like
> + * 	the mode and planes when setting ACTIVE to 0.

Maybe add "Note that the legacy DPMS property on connectors is internally
routed to control this property for atomic drivers."


Also I think we should add the dpms guarantee here.

"Userspace can rely on an change to ACTIVE always suceeding as long as no
other property has changed. If a change to ACTIVE only fails an atomic
test, this is a driver bug."

Kinda important part of the uapi here :-) Also the dpms emulation relies
on this.

> + * MODE_ID:
> + * 	Atomic property for setting the CRTC display timings. The value is the ID
> + * 	of a blob containing the DRM mode info. To disable the CRTC, user-space
> + * 	must set this property to 0.

Maybe we should add: "Note that only disabling by setting MODE_ID to 0
will release all internal resources (like reserved memory bandwidth or
clock generators). Only setting ACTIVE to 0 does not release reserved
resources, but only shuts down the hardware."

With these clarifications all added:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for doing this, I think really important we start properly
documenting these uapi details at least somewhere!

Cheers, Daniel

> + */
> +
>  /**
>   * drm_crtc_init_with_planes - Initialise a new CRTC object with
>   *    specified primary and cursor planes.
> -- 
> 2.26.2
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH v2] drm: add docs for standard CRTC properties

[Simon Ser]

This patch adds docs for the ACTIVE and MODE_ID CRTC properties.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Daniel Stone <daniel@fooishbar.org>
---

Thanks for the review Daniel! I rejiggered your suggestions a little to
move some ACTIVE bits into the ACTIVE description.

 Documentation/gpu/drm-kms.rst |  6 ++++++
 drivers/gpu/drm/drm_crtc.c    | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 397314d08f77..975cfeb8a353 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -460,6 +460,12 @@ HDMI Specific Connector Properties
 .. kernel-doc:: drivers/gpu/drm/drm_connector.c
    :doc: HDMI connector properties
 
+Standard CRTC Properties
+------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
+   :doc: standard CRTC properties
+
 Plane Composition Properties
 ----------------------------
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4936e1080e41..fafb8d3c3235 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -204,6 +204,33 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
 	return fence;
 }
 
+/**
+ * DOC: standard CRTC properties
+ *
+ * DRM CRTCs have a few standardized properties:
+ *
+ * ACTIVE:
+ * 	Atomic property for setting the power state of the CRTC. When set to 1 the
+ * 	CRTC will actively display content. When set to 0 the CRTC will be powered
+ * 	off. There is no expectation that user-space will reset CRTC resources like
+ * 	the mode and planes when setting ACTIVE to 0.
+ *
+ * 	User-space can rely on an ACTIVE change to 1 to never fail an atomic test
+ * 	as long as no other property has changed. If a change to ACTIVE fails an
+ * 	atomic test, this is a driver bug. For this reason setting ACTIVE to 0 must
+ * 	not release internal resources (like reserved memory bandwidth or clock
+ * 	generators).
+ *
+ * 	Note that the legacy DPMS property on connectors is internally routed to
+ * 	control this property for atomic drivers.
+ * MODE_ID:
+ * 	Atomic property for setting the CRTC display timings. The value is the ID
+ * 	of a blob containing the DRM mode info. To disable the CRTC, user-space
+ * 	must set this property to 0.
+ *
+ * 	Setting MODE_ID to 0 will release reserved resources for the CRTC.
+ */
+
 /**
  * drm_crtc_init_with_planes - Initialise a new CRTC object with
  *    specified primary and cursor planes.
-- 
2.26.2


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

Re: [PATCH v2] drm: add docs for standard CRTC properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3271 bytes --]

On Thu, 21 May 2020 11:09:31 +0000
Simon Ser <contact@emersion.fr> wrote:

> This patch adds docs for the ACTIVE and MODE_ID CRTC properties.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Daniel Stone <daniel@fooishbar.org>
> ---
> 
> Thanks for the review Daniel! I rejiggered your suggestions a little to
> move some ACTIVE bits into the ACTIVE description.
> 
>  Documentation/gpu/drm-kms.rst |  6 ++++++
>  drivers/gpu/drm/drm_crtc.c    | 27 +++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 397314d08f77..975cfeb8a353 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -460,6 +460,12 @@ HDMI Specific Connector Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
>     :doc: HDMI connector properties
>  
> +Standard CRTC Properties
> +------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_crtc.c
> +   :doc: standard CRTC properties
> +
>  Plane Composition Properties
>  ----------------------------
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4936e1080e41..fafb8d3c3235 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -204,6 +204,33 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
>  	return fence;
>  }
>  
> +/**
> + * DOC: standard CRTC properties
> + *
> + * DRM CRTCs have a few standardized properties:
> + *
> + * ACTIVE:
> + * 	Atomic property for setting the power state of the CRTC. When set to 1 the
> + * 	CRTC will actively display content. When set to 0 the CRTC will be powered
> + * 	off. There is no expectation that user-space will reset CRTC resources like
> + * 	the mode and planes when setting ACTIVE to 0.
> + *
> + * 	User-space can rely on an ACTIVE change to 1 to never fail an atomic test
> + * 	as long as no other property has changed. If a change to ACTIVE fails an
> + * 	atomic test, this is a driver bug. For this reason setting ACTIVE to 0 must
> + * 	not release internal resources (like reserved memory bandwidth or clock
> + * 	generators).
> + *
> + * 	Note that the legacy DPMS property on connectors is internally routed to
> + * 	control this property for atomic drivers.
> + * MODE_ID:
> + * 	Atomic property for setting the CRTC display timings. The value is the ID
> + * 	of a blob containing the DRM mode info. To disable the CRTC, user-space
> + * 	must set this property to 0.
> + *
> + * 	Setting MODE_ID to 0 will release reserved resources for the CRTC.
> + */
> +
>  /**
>   * drm_crtc_init_with_planes - Initialise a new CRTC object with
>   *    specified primary and cursor planes.

Really nice.

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>

And the patch 1/2 R-b as well.

FWIW, as I cannot really check the kernel code is like this, but it
totally makes sense from userspace side. I didn't even know these
little details before.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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