[PATCH 0/5] bunch of drm kernel-doc patches

[PATCH 0/5] bunch of drm kernel-doc patches

[Daniel Vetter]

Hi all,

Originally I wanted to include the atomic helper split-up here, but once
more I failed to get that done. So just a few patches to clean up
accumlated kernel-doc warnings, plus some real docs for the new best
practices for handling driver private objects. That was motivated by a
recent discussion.

Cheers, Daniel

Daniel Vetter (5):
  drm/edid: kerneldoc for is_hdmi2_sink
  drm/print: Unconfuse kerneldoc
  drm/syncobj: some kerneldoc polish
  drm/atomic: document how to handle driver private objects
  drm/doc: Move legacy kms helpers to the very end

 Documentation/gpu/drm-kms-helpers.rst | 36 ++++++++++++++--------------
 Documentation/gpu/drm-kms.rst         | 14 +++++++----
 drivers/gpu/drm/drm_atomic.c          | 45 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_edid.c            |  5 ++++
 drivers/gpu/drm/drm_syncobj.c         | 45 +++++++++++++++++++++++++++++++----
 include/drm/drm_atomic.h              | 32 +++++++++++++++++++++++++
 include/drm/drm_mode_config.h         |  9 +++++++
 include/drm/drm_print.h               |  2 +-
 include/drm/drm_syncobj.h             | 32 +++++++++++++++----------
 9 files changed, 177 insertions(+), 43 deletions(-)

-- 
2.15.1

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

[PATCH 1/5] drm/edid: kerneldoc for is_hdmi2_sink

[Daniel Vetter]

Also some breadcrumbs for how exactly to find this. Probably should
pass drm_connector * or at least drm_display_info * to that function
instead. But drm_hdmi_avi_infoframe_quant_range probably also wants
drm_connector_state (and the hdmi stuff moved into that), so this is a
bit more work.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: 9271c0ca573e ("drm/edid: Don't send non-zero YQ in AVI infoframe for HDMI 1.x sinks")
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 365901c1c33c..56ebef0c5fad 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4871,6 +4871,11 @@ EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
  * @mode: DRM display mode
  * @rgb_quant_range: RGB quantization range (Q)
  * @rgb_quant_range_selectable: Sink support selectable RGB quantization range (QS)
+ * @is_hdmi2_sink: HDMI 2.0 sink, which has different default recommendations
+ *
+ * Note that @is_hdmi2_sink can be derrived by looking at the
+ * &drm_scdc.supported flag stored in &drm_hdmi_info.scdc,
+ * &drm_display_info.hdmi, which can be found in &drm_connector.display_info.
  */
 void
 drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
-- 
2.15.1

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

[PATCH 2/5] drm/print: Unconfuse kerneldoc

[Daniel Vetter]

It thinks we want to document the __printf(2,0) annotion. Not sure we
want to teach it about all possible gcc-only flags, hence why I opted
for the cheap trick of just moving it ahead of the kerneldoc.

This is only a problem for static inline functions, since for
non-inline function the kerneldoc is in the .c file, but the special
annotations are all in the header.

Cc'ing kernel-doc maintainers as fyi.

Cc: linux-doc@vger.kernel.org
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_print.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 5f9932e2246e..2a4a42e59a47 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -80,13 +80,13 @@ void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
 __printf(2, 3)
 void drm_printf(struct drm_printer *p, const char *f, ...);
 
+__printf(2, 0)
 /**
  * drm_vprintf - print to a &drm_printer stream
  * @p: the &drm_printer
  * @fmt: format string
  * @va: the va_list
  */
-__printf(2, 0)
 static inline void
 drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)
 {
-- 
2.15.1

[PATCH 3/5] drm/syncobj: some kerneldoc polish

[Daniel Vetter]

Complete a few missing bits, fix up the existing xcross-references and
add a bunch more.

Cc: Dave Airlie <airlied@gmail.com> via lists.freedesktop.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_syncobj.c | 45 +++++++++++++++++++++++++++++++++++++++----
 include/drm/drm_syncobj.h     | 32 +++++++++++++++++-------------
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 9b733c510cbf..b6d0c2c400ec 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -29,9 +29,9 @@
 /**
  * DOC: Overview
  *
- * DRM synchronisation objects (syncobj) are a persistent objects,
- * that contain an optional fence. The fence can be updated with a new
- * fence, or be NULL.
+ * DRM synchronisation objects (syncobj, see struct &drm_syncobj) are a
+ * persistent objects, that contain an optional fence. The fence can be updated
+ * with a new fence, or be NULL.
  *
  * syncobj's can be waited upon, where it will wait for the underlying
  * fence.
@@ -61,7 +61,8 @@
  * @file_private: drm file private pointer
  * @handle: sync object handle to lookup.
  *
- * Returns a reference to the syncobj pointed to by handle or NULL.
+ * Returns a reference to the syncobj pointed to by handle or NULL. The
+ * reference must be released by calling drm_syncobj_put().
  */
 struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
 				     u32 handle)
@@ -229,6 +230,19 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
 	return 0;
 }
 
+/**
+ * drm_syncobj_find_fence - lookup and reference the fence in a sync object
+ * @file_private: drm file private pointer
+ * @handle: sync object handle to lookup.
+ * @fence: out parameter for the fence
+ *
+ * This is just a convenience function that combines drm_syncobj_find() and
+ * drm_syncobj_fence_get().
+ *
+ * Returns 0 on success or a negative error value on failure. On success @fence
+ * contains a reference to the fence, which mus be released by calling
+ * dma_fence_put().
+ */
 int drm_syncobj_find_fence(struct drm_file *file_private,
 			   u32 handle,
 			   struct dma_fence **fence)
@@ -269,6 +283,12 @@ EXPORT_SYMBOL(drm_syncobj_free);
  * @out_syncobj: returned syncobj
  * @flags: DRM_SYNCOBJ_* flags
  * @fence: if non-NULL, the syncobj will represent this fence
+ *
+ * This is the first function to create a sync object. After creating drivers
+ * probably want to make it available to userspace, either through
+ * drm_syncobj_get_handle() or drm_syncobj_get_fd().
+ *
+ * Returns 0 on success or a negative error value on failure.
  */
 int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
 		       struct dma_fence *fence)
@@ -302,6 +322,14 @@ EXPORT_SYMBOL(drm_syncobj_create);
 
 /**
  * drm_syncobj_get_handle - get a handle from a syncobj
+ * @file_private: drm file private pointer
+ * @syncobj: Sync object to export
+ * @handle: out parameter with the new handle
+ *
+ * Exports a sync object created with drm_syncobj_create() as a handle on
+ * @file_private to userspace.
+ *
+ * Returns 0 on success or a negative error value on failure.
  */
 int drm_syncobj_get_handle(struct drm_file *file_private,
 			   struct drm_syncobj *syncobj, u32 *handle)
@@ -388,6 +416,15 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
 	return 0;
 }
 
+/**
+ * drm_syncobj_get_fd - get a file descriptor from a syncobj
+ * @syncobj: Sync object to export
+ * @p_fd: out parameter with the new file descriptor
+ *
+ * Exports a sync object created with drm_syncobj_create() as a file descriptor.
+ *
+ * Returns 0 on success or a negative error value on failure.
+ */
 int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
 {
 	int ret;
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 9e8ba90c6784..87865774bdaa 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -33,36 +33,31 @@ struct drm_syncobj_cb;
 /**
  * struct drm_syncobj - sync object.
  *
- * This structure defines a generic sync object which wraps a dma fence.
+ * This structure defines a generic sync object which wraps a &dma_fence.
  */
 struct drm_syncobj {
 	/**
-	 * @refcount:
-	 *
-	 * Reference count of this object.
+	 * @refcount: Reference count of this object.
 	 */
 	struct kref refcount;
 	/**
 	 * @fence:
 	 * NULL or a pointer to the fence bound to this object.
 	 *
-	 * This field should not be used directly.  Use drm_syncobj_fence_get
-	 * and drm_syncobj_replace_fence instead.
+	 * This field should not be used directly. Use drm_syncobj_fence_get()
+	 * and drm_syncobj_replace_fence() instead.
 	 */
 	struct dma_fence __rcu *fence;
 	/**
-	 * @cb_list:
-	 * List of callbacks to call when the fence gets replaced
+	 * @cb_list: List of callbacks to call when the &fence gets replaced.
 	 */
 	struct list_head cb_list;
 	/**
-	 * @lock:
-	 * locks cb_list and write-locks fence.
+	 * @lock: Protects &cb_list and write-locks &fence.
 	 */
 	spinlock_t lock;
 	/**
-	 * @file:
-	 * a file backing for this syncobj.
+	 * @file: A file backing for this syncobj.
 	 */
 	struct file *file;
 };
@@ -73,7 +68,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
 /**
  * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
  * @node: used by drm_syncob_add_callback to append this struct to
- *	  syncobj::cb_list
+ *	  &drm_syncobj.cb_list
  * @func: drm_syncobj_func_t to call
  *
  * This struct will be initialized by drm_syncobj_add_callback, additional
@@ -111,6 +106,17 @@ drm_syncobj_put(struct drm_syncobj *obj)
 	kref_put(&obj->refcount, drm_syncobj_free);
 }
 
+/**
+ * drm_syncobj_fence_get - get a reference to a fence in a sync object
+ * @syncobj: sync object.
+ *
+ * This acquires additional reference to &drm_syncobj.fence contained in @obj,
+ * if not NULL. It is illegal to call this without already holding a reference.
+ * No locks required.
+ *
+ * Returns:
+ * Either the fence of @obj or NULL if there's none.
+ */
 static inline struct dma_fence *
 drm_syncobj_fence_get(struct drm_syncobj *syncobj)
 {
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/atomic: document how to handle driver private objects

[Daniel Vetter]

DK put some nice docs into the commit introducing driver private
state, but in the git history alone it'll be lost.

Also, since Ville remove the void* usage it's a good opportunity to
give the driver private stuff some tlc on the doc front.

Finally try to explain why the "let's just subclass drm_atomic_state"
approach wasn't the greatest, and annotate all those functions as
deprecated in favour of more standardized driver private states. Also
note where we could/should extend driver private states going forward
(atm neither locking nor synchronization is handled in core/helpers,
which isn't really all that great).

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/drm-kms.rst |  6 ++++++
 drivers/gpu/drm/drm_atomic.c  | 45 ++++++++++++++++++++++++++++++++++++++++---
 include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
 include/drm/drm_mode_config.h |  9 +++++++++
 4 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 307284125d7a..420025bd6a9b 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -271,6 +271,12 @@ Taken all together there's two consequences for the atomic design:
 Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
 coverage of specific topics.
 
+Handling Driver Private State
+-----------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
+   :doc: handling driver private state
+
 Atomic Mode Setting Function Reference
 --------------------------------------
 
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 37445d50816a..15e1a35c74a8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
  * @state: atomic state
  *
  * Free all the memory allocated by drm_atomic_state_init.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * &drm_atomic_state and haven't switched to &drm_private_state yet.
  */
 void drm_atomic_state_default_release(struct drm_atomic_state *state)
 {
@@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
  * @state: atomic state
  *
  * Default implementation for filling in a new atomic state.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * &drm_atomic_state and haven't switched to &drm_private_state yet.
  */
 int
 drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
@@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
  * @state: atomic state
  *
  * Default implementation for clearing atomic state.
- * This is useful for drivers that subclass the atomic state.
+ * This should only be used by drivers which are still subclassing
+ * &drm_atomic_state and haven't switched to &drm_private_state yet.
  */
 void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 {
@@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
 		plane->funcs->atomic_print_state(p, state);
 }
 
+/**
+ * DOC: handling driver private state
+ *
+ * Very often the DRM objects exposed to userspace in the atomic modeset api
+ * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
+ * underlying hardware. Especially for any kind of shared resources (e.g. shared
+ * clocks, scaler units, bandwidth and fifo limits shared among a group of
+ * planes or CRTCs, and so on) it makes sense to model these as independent
+ * objects. Drivers then need to similar state tracking and commit ordering for
+ * such private (since not exposed to userpace) objects as the atomic core and
+ * helpers already provide for connectors, planes and CRTCs.
+ *
+ * To make this easier on drivers the atomic core provides some support to track
+ * driver private state objects using struct &drm_private_obj, with the
+ * associated state struct &drm_private_state.
+ *
+ * Similar to userspace-exposed objects, state structures can be acquired by
+ * calling drm_atomic_get_private_obj_state(). Since this function does not take
+ * care of locking, drivers should wrap it for each type of private state object
+ * they have with the required call to drm_modeset_lock() for the corresponding
+ * &drm_modeset_lock.
+ *
+ * All private state structures contained in a &drm_atomic_state update can be
+ * iterated using for_each_oldnew_private_obj_in_state(),
+ * for_each_old_private_obj_in_state() and for_each_old_private_obj_in_state().
+ * Drivers are recommend to wrap these for each type of driver private state
+ * object they have, filtering on &drm_private_obj.funcs using for_each_if(), at
+ * least if they want to iterate over all objects of a given type.
+ *
+ * An earlier way to handle driver private state was by subclassing struct
+ * &drm_atomic_state. But since that encourages non-standard ways to implement
+ * the check/commit split atomic requires (by using e.g. "check and rollback or
+ * commit instead" of "duplicate state, check, then either commit or release
+ * duplicated state) it is deprecated in favour of using &drm_private_state.
+ */
+
 /**
  * drm_atomic_private_obj_init - initialize private object
  * @obj: private object
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 5afd6e364fb6..8e4829a25c1b 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -189,12 +189,40 @@ struct drm_private_state_funcs {
 				     struct drm_private_state *state);
 };
 
+/**
+ * struct drm_private_obj - base struct for driver private atomic object
+ *
+ * A driver private object is initialized by calling
+ * drm_atomic_private_obj_init() and cleaned up by calling
+ * drm_atomic_private_obj_fini().
+ *
+ * Currently only tracks the state update functions and the opaque driver
+ * private state itself, but in the future might also track which
+ * &drm_modeset_lock is required to duplicate and update this object's state.
+ */
 struct drm_private_obj {
+	/**
+	 * @state: Current atomic state for this driver private object.
+	 */
 	struct drm_private_state *state;
 
+	/**
+	 * @funcs:
+	 *
+	 * Functions to manipulate the state of this driver private object, see
+	 * &drm_private_state_funcs.
+	 */
 	const struct drm_private_state_funcs *funcs;
 };
 
+/**
+ * struct drm_private_state - base struct for driver private object state
+ * @state: backpointer to global drm_atomic_state
+ *
+ * Currently only contains a backpointer to the overall atomic upated, but in
+ * the future also might hold synchronization information similar to e.g.
+ * &drm_crtc.commit.
+ */
 struct drm_private_state {
 	struct drm_atomic_state *state;
 };
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index c6639e8e5007..2cb6f02df64a 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -269,6 +269,9 @@ struct drm_mode_config_funcs {
 	 * state easily. If this hook is implemented, drivers must also
 	 * implement @atomic_state_clear and @atomic_state_free.
 	 *
+	 * Subclassing of &drm_atomic_state is deprecated in favour of using
+	 * &drm_private_state and &drm_private_obj.
+	 *
 	 * RETURNS:
 	 *
 	 * A new &drm_atomic_state on success or NULL on failure.
@@ -290,6 +293,9 @@ struct drm_mode_config_funcs {
 	 *
 	 * Drivers that implement this must call drm_atomic_state_default_clear()
 	 * to clear common state.
+	 *
+	 * Subclassing of &drm_atomic_state is deprecated in favour of using
+	 * &drm_private_state and &drm_private_obj.
 	 */
 	void (*atomic_state_clear)(struct drm_atomic_state *state);
 
@@ -302,6 +308,9 @@ struct drm_mode_config_funcs {
 	 *
 	 * Drivers that implement this must call
 	 * drm_atomic_state_default_release() to release common resources.
+	 *
+	 * Subclassing of &drm_atomic_state is deprecated in favour of using
+	 * &drm_private_state and &drm_private_obj.
 	 */
 	void (*atomic_state_free)(struct drm_atomic_state *state);
 };
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/doc: Move legacy kms helpers to the very end

[Daniel Vetter]

We don't want people to accidentally stumble over there.

Also rename the plane helpers to legacy plane helpers. After Ville's
patch to make the clipping helper atomic and move it to
drm_atomic_helper.c there's nothing left in there that should be
useful for modern drivers.

v2: Laurent had a few questions around how state is added to
drm_atomic_state, tried to clarify that. And spotted another sentence
where the docs suggested subclassing.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/drm-kms-helpers.rst | 36 +++++++++++++++++------------------
 Documentation/gpu/drm-kms.rst         |  8 ++++----
 include/drm/drm_atomic.h              |  4 ++++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 3ea622876b67..e37557b30f62 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -74,15 +74,6 @@ Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
    :export:
 
-Legacy CRTC/Modeset Helper Functions Reference
-==============================================
-
-.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
-   :doc: overview
-
-.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
-   :export:
-
 Simple KMS Helper Reference
 ===========================
 
@@ -282,15 +273,6 @@ Flip-work Helper Reference
 .. kernel-doc:: drivers/gpu/drm/drm_flip_work.c
    :export:
 
-Plane Helper Reference
-======================
-
-.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
-   :doc: overview
-
-.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
-   :export:
-
 Auxiliary Modeset Helpers
 =========================
 
@@ -308,3 +290,21 @@ Framebuffer GEM Helper Reference
 
 .. kernel-doc:: drivers/gpu/drm/drm_gem_framebuffer_helper.c
    :export:
+
+Legacy Plane Helper Reference
+=============================
+
+.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
+   :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
+   :export:
+
+Legacy CRTC/Modeset Helper Functions Reference
+==============================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
+   :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
+   :export:
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 420025bd6a9b..cbba93483aec 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -263,10 +263,10 @@ Taken all together there's two consequences for the atomic design:
 
 - An atomic update is assembled and validated as an entirely free-standing pile
   of structures within the :c:type:`drm_atomic_state <drm_atomic_state>`
-  container. Again drivers can subclass that container for their own state
-  structure tracking needs. Only when a state is committed is it applied to the
-  driver and modeset objects. This way rolling back an update boils down to
-  releasing memory and unreferencing objects like framebuffers.
+  container. Driver private state structures are also tracked in the same
+  structure, see the next chapter.  Only when a state is committed is it applied
+  to the driver and modeset objects. This way rolling back an update boils down
+  to releasing memory and unreferencing objects like framebuffers.
 
 Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
 coverage of specific topics.
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 8e4829a25c1b..e7f25e342cc8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -246,6 +246,10 @@ struct __drm_private_objs_state {
  * @num_private_objs: size of the @private_objs array
  * @private_objs: pointer to array of private object pointers
  * @acquire_ctx: acquire context for this atomic modeset state update
+ *
+ * States are added to an atomic update by calling drm_atomic_get_crtc_state(),
+ * drm_atomic_get_plane_state(), drm_atomic_get_connector_state(), or for
+ * private state structures, drm_atomic_get_private_obj_state().
  */
 struct drm_atomic_state {
 	struct kref ref;
-- 
2.15.1

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

✓ Fi.CI.BAT: success for bunch of drm kernel-doc patches

[Patchwork]

== Series Details ==

Series: bunch of drm kernel-doc patches
URL   : https://patchwork.freedesktop.org/series/35375/
State : success

== Summary ==

Series 35375v1 bunch of drm kernel-doc patches
https://patchwork.freedesktop.org/api/1.0/series/35375/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
                incomplete -> PASS       (fi-snb-2520m) fdo#103713 +1
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test gem_sync:
        Subgroup basic-each:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-many-each:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-store-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-store-each:
                skip       -> PASS       (fi-blb-e6850)
Test gem_tiled_blits:
        Subgroup basic:
                skip       -> PASS       (fi-blb-e6850)
Test gem_tiled_fence_blits:
        Subgroup basic:
                skip       -> PASS       (fi-blb-e6850)
Test gem_wait:
        Subgroup basic-busy-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-wait-all:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-await-all:
                skip       -> PASS       (fi-blb-e6850)
Test kms_busy:
        Subgroup basic-flip-a:
                skip       -> PASS       (fi-blb-e6850)
        Subgroup basic-flip-b:
                skip       -> PASS       (fi-blb-e6850)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                skip       -> PASS       (fi-blb-e6850)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-kbl-r) fdo#104172 +1

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:436s
fi-blb-e6850     total:288  pass:222  dwarn:1   dfail:1   fail:0   skip:64  time:395s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:499s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:497s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:496s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:477s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:466s
fi-elk-e7500     total:224  pass:164  dwarn:14  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:1   dfail:0   fail:0   skip:108 time:264s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:408s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:412s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:396s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:473s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:419s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:519s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:522s
fi-pnv-d510      total:288  pass:221  dwarn:1   dfail:0   fail:1   skip:65  time:594s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:442s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:532s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:556s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:411s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:583s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:614s
fi-glk-dsi       total:288  pass:257  dwarn:0   dfail:0   fail:1   skip:30  time:495s

ad43db157c69bf7311ba5a0278f282796242f34a drm-tip: 2017y-12m-14d-20h-09m-53s UTC integration manifest
100f623a35f3 drm/doc: Move legacy kms helpers to the very end
61d5e0c046cc drm/atomic: document how to handle driver private objects
f7c33af2792c drm/syncobj: some kerneldoc polish
54eb7e75904f drm/print: Unconfuse kerneldoc
0df6e16851eb drm/edid: kerneldoc for is_hdmi2_sink

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7497/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for bunch of drm kernel-doc patches

[Patchwork]

== Series Details ==

Series: bunch of drm kernel-doc patches
URL   : https://patchwork.freedesktop.org/series/35375/
State : success

== Summary ==

Test kms_flip:
        Subgroup vblank-vs-suspend:
                incomplete -> PASS       (shard-hsw) fdo#103375
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test gem_tiled_swapping:
        Subgroup non-threaded:
                dmesg-warn -> INCOMPLETE (shard-hsw) fdo#104218
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                fail       -> PASS       (shard-snb) fdo#101623

fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#104218 https://bugs.freedesktop.org/show_bug.cgi?id=104218
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-hsw        total:2645 pass:1495 dwarn:1   dfail:0   fail:9   skip:1139 time:9165s
shard-snb        total:2712 pass:1309 dwarn:1   dfail:0   fail:11  skip:1391 time:8035s
Blacklisted hosts:
shard-apl        total:2712 pass:1682 dwarn:1   dfail:0   fail:27  skip:1001 time:13939s
shard-kbl        total:2712 pass:1801 dwarn:1   dfail:0   fail:32  skip:878 time:10997s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7497/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Alex Deucher]

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.
>
> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
>
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++++++
>  drivers/gpu/drm/drm_atomic.c  | 45 ++++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h |  9 +++++++++
>  4 files changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the atomic design:
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
>  coverage of specific topics.
>
> +Handling Driver Private State
> +-----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --------------------------------------
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>                 plane->funcs->atomic_print_state(p, state);
>  }
>
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset api
> + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g. shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering for
> + * such private (since not exposed to userpace) objects as the atomic core and
> + * helpers already provide for connectors, planes and CRTCs.

This last sentence doesn't quite parse.  I think it should be as follows:

Drivers then need to do similar state tracking and commit ordering for
such private (since not exposed to userpace) objects as the atomic core that
helpers already provide for DRM objects (connectors, planes and CRTCs).

Feel free to adjust as you see fit.  With that fixed up:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> + *
> + * To make this easier on drivers the atomic core provides some support to track
> + * driver private state objects using struct &drm_private_obj, with the
> + * associated state struct &drm_private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired by
> + * calling drm_atomic_get_private_obj_state(). Since this function does not take
> + * care of locking, drivers should wrap it for each type of private state object
> + * they have with the required call to drm_modeset_lock() for the corresponding
> + * &drm_modeset_lock.
> + *
> + * All private state structures contained in a &drm_atomic_state update can be
> + * iterated using for_each_oldnew_private_obj_in_state(),
> + * for_each_old_private_obj_in_state() and for_each_old_private_obj_in_state().
> + * Drivers are recommend to wrap these for each type of driver private state
> + * object they have, filtering on &drm_private_obj.funcs using for_each_if(), at
> + * least if they want to iterate over all objects of a given type.
> + *
> + * An earlier way to handle driver private state was by subclassing struct
> + * &drm_atomic_state. But since that encourages non-standard ways to implement
> + * the check/commit split atomic requires (by using e.g. "check and rollback or
> + * commit instead" of "duplicate state, check, then either commit or release
> + * duplicated state) it is deprecated in favour of using &drm_private_state.
> + */
> +
>  /**
>   * drm_atomic_private_obj_init - initialize private object
>   * @obj: private object
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 5afd6e364fb6..8e4829a25c1b 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -189,12 +189,40 @@ struct drm_private_state_funcs {
>                                      struct drm_private_state *state);
>  };
>
> +/**
> + * struct drm_private_obj - base struct for driver private atomic object
> + *
> + * A driver private object is initialized by calling
> + * drm_atomic_private_obj_init() and cleaned up by calling
> + * drm_atomic_private_obj_fini().
> + *
> + * Currently only tracks the state update functions and the opaque driver
> + * private state itself, but in the future might also track which
> + * &drm_modeset_lock is required to duplicate and update this object's state.
> + */
>  struct drm_private_obj {
> +       /**
> +        * @state: Current atomic state for this driver private object.
> +        */
>         struct drm_private_state *state;
>
> +       /**
> +        * @funcs:
> +        *
> +        * Functions to manipulate the state of this driver private object, see
> +        * &drm_private_state_funcs.
> +        */
>         const struct drm_private_state_funcs *funcs;
>  };
>
> +/**
> + * struct drm_private_state - base struct for driver private object state
> + * @state: backpointer to global drm_atomic_state
> + *
> + * Currently only contains a backpointer to the overall atomic upated, but in
> + * the future also might hold synchronization information similar to e.g.
> + * &drm_crtc.commit.
> + */
>  struct drm_private_state {
>         struct drm_atomic_state *state;
>  };
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index c6639e8e5007..2cb6f02df64a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -269,6 +269,9 @@ struct drm_mode_config_funcs {
>          * state easily. If this hook is implemented, drivers must also
>          * implement @atomic_state_clear and @atomic_state_free.
>          *
> +        * Subclassing of &drm_atomic_state is deprecated in favour of using
> +        * &drm_private_state and &drm_private_obj.
> +        *
>          * RETURNS:
>          *
>          * A new &drm_atomic_state on success or NULL on failure.
> @@ -290,6 +293,9 @@ struct drm_mode_config_funcs {
>          *
>          * Drivers that implement this must call drm_atomic_state_default_clear()
>          * to clear common state.
> +        *
> +        * Subclassing of &drm_atomic_state is deprecated in favour of using
> +        * &drm_private_state and &drm_private_obj.
>          */
>         void (*atomic_state_clear)(struct drm_atomic_state *state);
>
> @@ -302,6 +308,9 @@ struct drm_mode_config_funcs {
>          *
>          * Drivers that implement this must call
>          * drm_atomic_state_default_release() to release common resources.
> +        *
> +        * Subclassing of &drm_atomic_state is deprecated in favour of using
> +        * &drm_private_state and &drm_private_obj.
>          */
>         void (*atomic_state_free)(struct drm_atomic_state *state);
>  };
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/edid: kerneldoc for is_hdmi2_sink

[Alex Deucher]

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Also some breadcrumbs for how exactly to find this. Probably should
> pass drm_connector * or at least drm_display_info * to that function
> instead. But drm_hdmi_avi_infoframe_quant_range probably also wants
> drm_connector_state (and the hdmi stuff moved into that), so this is a
> bit more work.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: 9271c0ca573e ("drm/edid: Don't send non-zero YQ in AVI infoframe for HDMI 1.x sinks")
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 365901c1c33c..56ebef0c5fad 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4871,6 +4871,11 @@ EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>   * @mode: DRM display mode
>   * @rgb_quant_range: RGB quantization range (Q)
>   * @rgb_quant_range_selectable: Sink support selectable RGB quantization range (QS)
> + * @is_hdmi2_sink: HDMI 2.0 sink, which has different default recommendations
> + *
> + * Note that @is_hdmi2_sink can be derrived by looking at the

typo : derived
With that fixed:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> + * &drm_scdc.supported flag stored in &drm_hdmi_info.scdc,
> + * &drm_display_info.hdmi, which can be found in &drm_connector.display_info.
>   */
>  void
>  drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame,
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/print: Unconfuse kerneldoc

[Alex Deucher]

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It thinks we want to document the __printf(2,0) annotion. Not sure we
> want to teach it about all possible gcc-only flags, hence why I opted
> for the cheap trick of just moving it ahead of the kerneldoc.
>
> This is only a problem for static inline functions, since for
> non-inline function the kerneldoc is in the .c file, but the special
> annotations are all in the header.
>
> Cc'ing kernel-doc maintainers as fyi.
>
> Cc: linux-doc@vger.kernel.org
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  include/drm/drm_print.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 5f9932e2246e..2a4a42e59a47 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -80,13 +80,13 @@ void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
>  __printf(2, 3)
>  void drm_printf(struct drm_printer *p, const char *f, ...);
>
> +__printf(2, 0)
>  /**
>   * drm_vprintf - print to a &drm_printer stream
>   * @p: the &drm_printer
>   * @fmt: format string
>   * @va: the va_list
>   */
> -__printf(2, 0)
>  static inline void
>  drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)
>  {
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/syncobj: some kerneldoc polish

[Alex Deucher]

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Complete a few missing bits, fix up the existing xcross-references and
> add a bunch more.
>
> Cc: Dave Airlie <airlied@gmail.com> via lists.freedesktop.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 45 +++++++++++++++++++++++++++++++++++++++----
>  include/drm/drm_syncobj.h     | 32 +++++++++++++++++-------------
>  2 files changed, 60 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 9b733c510cbf..b6d0c2c400ec 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -29,9 +29,9 @@
>  /**
>   * DOC: Overview
>   *
> - * DRM synchronisation objects (syncobj) are a persistent objects,
> - * that contain an optional fence. The fence can be updated with a new
> - * fence, or be NULL.
> + * DRM synchronisation objects (syncobj, see struct &drm_syncobj) are a
> + * persistent objects, that contain an optional fence. The fence can be updated

grammer:
are persistent objects that contain an optional fence


> + * with a new fence, or be NULL.
>   *
>   * syncobj's can be waited upon, where it will wait for the underlying
>   * fence.
> @@ -61,7 +61,8 @@
>   * @file_private: drm file private pointer
>   * @handle: sync object handle to lookup.
>   *
> - * Returns a reference to the syncobj pointed to by handle or NULL.
> + * Returns a reference to the syncobj pointed to by handle or NULL. The
> + * reference must be released by calling drm_syncobj_put().
>   */
>  struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>                                      u32 handle)
> @@ -229,6 +230,19 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>         return 0;
>  }
>
> +/**
> + * drm_syncobj_find_fence - lookup and reference the fence in a sync object
> + * @file_private: drm file private pointer
> + * @handle: sync object handle to lookup.
> + * @fence: out parameter for the fence
> + *
> + * This is just a convenience function that combines drm_syncobj_find() and
> + * drm_syncobj_fence_get().
> + *
> + * Returns 0 on success or a negative error value on failure. On success @fence
> + * contains a reference to the fence, which mus be released by calling

typo: must

> + * dma_fence_put().
> + */
>  int drm_syncobj_find_fence(struct drm_file *file_private,
>                            u32 handle,
>                            struct dma_fence **fence)
> @@ -269,6 +283,12 @@ EXPORT_SYMBOL(drm_syncobj_free);
>   * @out_syncobj: returned syncobj
>   * @flags: DRM_SYNCOBJ_* flags
>   * @fence: if non-NULL, the syncobj will represent this fence
> + *
> + * This is the first function to create a sync object. After creating drivers
> + * probably want to make it available to userspace, either through
> + * drm_syncobj_get_handle() or drm_syncobj_get_fd().

grammer:
After creating, drivers probably want to make it available to userspace

> + *
> + * Returns 0 on success or a negative error value on failure.
>   */
>  int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>                        struct dma_fence *fence)
> @@ -302,6 +322,14 @@ EXPORT_SYMBOL(drm_syncobj_create);
>
>  /**
>   * drm_syncobj_get_handle - get a handle from a syncobj
> + * @file_private: drm file private pointer
> + * @syncobj: Sync object to export
> + * @handle: out parameter with the new handle
> + *
> + * Exports a sync object created with drm_syncobj_create() as a handle on
> + * @file_private to userspace.
> + *
> + * Returns 0 on success or a negative error value on failure.
>   */
>  int drm_syncobj_get_handle(struct drm_file *file_private,
>                            struct drm_syncobj *syncobj, u32 *handle)
> @@ -388,6 +416,15 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
>         return 0;
>  }
>
> +/**
> + * drm_syncobj_get_fd - get a file descriptor from a syncobj
> + * @syncobj: Sync object to export
> + * @p_fd: out parameter with the new file descriptor
> + *
> + * Exports a sync object created with drm_syncobj_create() as a file descriptor.
> + *
> + * Returns 0 on success or a negative error value on failure.
> + */
>  int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
>  {
>         int ret;
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 9e8ba90c6784..87865774bdaa 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -33,36 +33,31 @@ struct drm_syncobj_cb;
>  /**
>   * struct drm_syncobj - sync object.
>   *
> - * This structure defines a generic sync object which wraps a dma fence.
> + * This structure defines a generic sync object which wraps a &dma_fence.
>   */
>  struct drm_syncobj {
>         /**
> -        * @refcount:
> -        *
> -        * Reference count of this object.
> +        * @refcount: Reference count of this object.
>          */
>         struct kref refcount;
>         /**
>          * @fence:
>          * NULL or a pointer to the fence bound to this object.
>          *
> -        * This field should not be used directly.  Use drm_syncobj_fence_get
> -        * and drm_syncobj_replace_fence instead.
> +        * This field should not be used directly. Use drm_syncobj_fence_get()
> +        * and drm_syncobj_replace_fence() instead.
>          */
>         struct dma_fence __rcu *fence;
>         /**
> -        * @cb_list:
> -        * List of callbacks to call when the fence gets replaced
> +        * @cb_list: List of callbacks to call when the &fence gets replaced.
>          */
>         struct list_head cb_list;
>         /**
> -        * @lock:
> -        * locks cb_list and write-locks fence.
> +        * @lock: Protects &cb_list and write-locks &fence.
>          */
>         spinlock_t lock;
>         /**
> -        * @file:
> -        * a file backing for this syncobj.
> +        * @file: A file backing for this syncobj.
>          */
>         struct file *file;
>  };
> @@ -73,7 +68,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
>  /**
>   * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>   * @node: used by drm_syncob_add_callback to append this struct to
> - *       syncobj::cb_list
> + *       &drm_syncobj.cb_list
>   * @func: drm_syncobj_func_t to call
>   *
>   * This struct will be initialized by drm_syncobj_add_callback, additional
> @@ -111,6 +106,17 @@ drm_syncobj_put(struct drm_syncobj *obj)
>         kref_put(&obj->refcount, drm_syncobj_free);
>  }
>
> +/**
> + * drm_syncobj_fence_get - get a reference to a fence in a sync object
> + * @syncobj: sync object.
> + *
> + * This acquires additional reference to &drm_syncobj.fence contained in @obj,

grammer:
This acquires an additional reference

With those fixed up:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> + * if not NULL. It is illegal to call this without already holding a reference.
> + * No locks required.
> + *
> + * Returns:
> + * Either the fence of @obj or NULL if there's none.
> + */
>  static inline struct dma_fence *
>  drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>  {
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/doc: Move legacy kms helpers to the very end

[Alex Deucher]

On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We don't want people to accidentally stumble over there.
>
> Also rename the plane helpers to legacy plane helpers. After Ville's
> patch to make the clipping helper atomic and move it to
> drm_atomic_helper.c there's nothing left in there that should be
> useful for modern drivers.
>
> v2: Laurent had a few questions around how state is added to
> drm_atomic_state, tried to clarify that. And spotted another sentence
> where the docs suggested subclassing.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst | 36 +++++++++++++++++------------------
>  Documentation/gpu/drm-kms.rst         |  8 ++++----
>  include/drm/drm_atomic.h              |  4 ++++
>  3 files changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 3ea622876b67..e37557b30f62 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -74,15 +74,6 @@ Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
>     :export:
>
> -Legacy CRTC/Modeset Helper Functions Reference
> -==============================================
> -
> -.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
> -   :doc: overview
> -
> -.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
> -   :export:
> -
>  Simple KMS Helper Reference
>  ===========================
>
> @@ -282,15 +273,6 @@ Flip-work Helper Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_flip_work.c
>     :export:
>
> -Plane Helper Reference
> -======================
> -
> -.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
> -   :doc: overview
> -
> -.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
> -   :export:
> -
>  Auxiliary Modeset Helpers
>  =========================
>
> @@ -308,3 +290,21 @@ Framebuffer GEM Helper Reference
>
>  .. kernel-doc:: drivers/gpu/drm/drm_gem_framebuffer_helper.c
>     :export:
> +
> +Legacy Plane Helper Reference
> +=============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_plane_helper.c
> +   :export:
> +
> +Legacy CRTC/Modeset Helper Functions Reference
> +==============================================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
> +   :export:
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 420025bd6a9b..cbba93483aec 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -263,10 +263,10 @@ Taken all together there's two consequences for the atomic design:
>
>  - An atomic update is assembled and validated as an entirely free-standing pile
>    of structures within the :c:type:`drm_atomic_state <drm_atomic_state>`
> -  container. Again drivers can subclass that container for their own state
> -  structure tracking needs. Only when a state is committed is it applied to the
> -  driver and modeset objects. This way rolling back an update boils down to
> -  releasing memory and unreferencing objects like framebuffers.
> +  container. Driver private state structures are also tracked in the same
> +  structure, see the next chapter.  Only when a state is committed is it applied

I think it would be clearer as:
structure (see the next chapter).
or
structure; see the next chapter.

Either way:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> +  to the driver and modeset objects. This way rolling back an update boils down
> +  to releasing memory and unreferencing objects like framebuffers.
>
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
>  coverage of specific topics.
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8e4829a25c1b..e7f25e342cc8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -246,6 +246,10 @@ struct __drm_private_objs_state {
>   * @num_private_objs: size of the @private_objs array
>   * @private_objs: pointer to array of private object pointers
>   * @acquire_ctx: acquire context for this atomic modeset state update
> + *
> + * States are added to an atomic update by calling drm_atomic_get_crtc_state(),
> + * drm_atomic_get_plane_state(), drm_atomic_get_connector_state(), or for
> + * private state structures, drm_atomic_get_private_obj_state().
>   */
>  struct drm_atomic_state {
>         struct kref ref;
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Pandiyan, Dhinakaran]

On Thu, 2017-12-14 at 21:30 +0100, Daniel Vetter wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.
> 
> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
> 
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
> 

I have pointed out a couple of typos below, other than lgtm
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++++++
>  drivers/gpu/drm/drm_atomic.c  | 45 ++++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h |  9 +++++++++
>  4 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the atomic design:
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
>  coverage of specific topics.
>  
> +Handling Driver Private State
> +-----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --------------------------------------
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct drm_printer *p,
>  		plane->funcs->atomic_print_state(p, state);
>  }
>  
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset api
> + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g. shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering for
> + * such private (since not exposed to userpace) objects as the atomic core and
> + * helpers already provide for connectors, planes and CRTCs.
> + *
> + * To make this easier on drivers the atomic core provides some support to track
> + * driver private state objects using struct &drm_private_obj, with the
> + * associated state struct &drm_private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired by
					  ^private

> + * calling drm_atomic_get_private_obj_state(). Since this function does not take
> + * care of locking, drivers should wrap it for each type of private state object
> + * they have with the required call to drm_modeset_lock() for the corresponding
> + * &drm_modeset_lock.
> + *
> + * All private state structures contained in a &drm_atomic_state update can be
> + * iterated using for_each_oldnew_private_obj_in_state(),
> + * for_each_old_private_obj_in_state() and for_each_old_private_obj_in_state().
     ^for_each_new_private_obj_in_state

> + * Drivers are recommend to wrap these for each type of driver private state
		 recommended
> + * object they have, filtering on &drm_private_obj.funcs using for_each_if(), at
> + * least if they want to iterate over all objects of a given type.
> + *
> + * An earlier way to handle driver private state was by subclassing struct
> + * &drm_atomic_state. But since that encourages non-standard ways to implement
> + * the check/commit split atomic requires (by using e.g. "check and rollback or
> + * commit instead" of "duplicate state, check, then either commit or release
> + * duplicated state) it is deprecated in favour of using &drm_private_state.
> + */
> +
>  /**
>   * drm_atomic_private_obj_init - initialize private object
>   * @obj: private object
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 5afd6e364fb6..8e4829a25c1b 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -189,12 +189,40 @@ struct drm_private_state_funcs {
>  				     struct drm_private_state *state);
>  };
>  
> +/**
> + * struct drm_private_obj - base struct for driver private atomic object
> + *
> + * A driver private object is initialized by calling
> + * drm_atomic_private_obj_init() and cleaned up by calling
> + * drm_atomic_private_obj_fini().
> + *
> + * Currently only tracks the state update functions and the opaque driver
> + * private state itself, but in the future might also track which
> + * &drm_modeset_lock is required to duplicate and update this object's state.
> + */
>  struct drm_private_obj {
> +	/**
> +	 * @state: Current atomic state for this driver private object.
> +	 */
>  	struct drm_private_state *state;
>  
> +	/**
> +	 * @funcs:
> +	 *
> +	 * Functions to manipulate the state of this driver private object, see
> +	 * &drm_private_state_funcs.
> +	 */
>  	const struct drm_private_state_funcs *funcs;
>  };
>  
> +/**
> + * struct drm_private_state - base struct for driver private object state
> + * @state: backpointer to global drm_atomic_state
> + *
> + * Currently only contains a backpointer to the overall atomic upated, but in
								^updated
> + * the future also might hold synchronization information similar to e.g.
> + * &drm_crtc.commit.
> + */
>  struct drm_private_state {
>  	struct drm_atomic_state *state;
>  };
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index c6639e8e5007..2cb6f02df64a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -269,6 +269,9 @@ struct drm_mode_config_funcs {
>  	 * state easily. If this hook is implemented, drivers must also
>  	 * implement @atomic_state_clear and @atomic_state_free.
>  	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
> +	 *
>  	 * RETURNS:
>  	 *
>  	 * A new &drm_atomic_state on success or NULL on failure.
> @@ -290,6 +293,9 @@ struct drm_mode_config_funcs {
>  	 *
>  	 * Drivers that implement this must call drm_atomic_state_default_clear()
>  	 * to clear common state.
> +	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
>  	 */
>  	void (*atomic_state_clear)(struct drm_atomic_state *state);
>  
> @@ -302,6 +308,9 @@ struct drm_mode_config_funcs {
>  	 *
>  	 * Drivers that implement this must call
>  	 * drm_atomic_state_default_release() to release common resources.
> +	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
>  	 */
>  	void (*atomic_state_free)(struct drm_atomic_state *state);
>  };
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/doc: Move legacy kms helpers to the very end

[Daniel Vetter]

On Thu, Dec 14, 2017 at 09:11:01PM -0500, Alex Deucher wrote:
> On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 420025bd6a9b..cbba93483aec 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -263,10 +263,10 @@ Taken all together there's two consequences for the atomic design:
> >
> >  - An atomic update is assembled and validated as an entirely free-standing pile
> >    of structures within the :c:type:`drm_atomic_state <drm_atomic_state>`
> > -  container. Again drivers can subclass that container for their own state
> > -  structure tracking needs. Only when a state is committed is it applied to the
> > -  driver and modeset objects. This way rolling back an update boils down to
> > -  releasing memory and unreferencing objects like framebuffers.
> > +  container. Driver private state structures are also tracked in the same
> > +  structure, see the next chapter.  Only when a state is committed is it applied
> 
> I think it would be clearer as:
> structure (see the next chapter).
> or
> structure; see the next chapter.
> 
> Either way:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks for all the review from you and DK, all taken into account and
merged.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Laurent Pinchart]

Hi Alex,

On Friday, 15 December 2017 03:57:48 EET Alex Deucher wrote:
> On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter wrote:
> > DK put some nice docs into the commit introducing driver private
> > state, but in the git history alone it'll be lost.
> > 
> > Also, since Ville remove the void* usage it's a good opportunity to
> > give the driver private stuff some tlc on the doc front.
> > 
> > Finally try to explain why the "let's just subclass drm_atomic_state"
> > approach wasn't the greatest, and annotate all those functions as
> > deprecated in favour of more standardized driver private states. Also
> > note where we could/should extend driver private states going forward
> > (atm neither locking nor synchronization is handled in core/helpers,
> > which isn't really all that great).
> > 
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > 
> >  Documentation/gpu/drm-kms.rst |  6 ++++++
> >  drivers/gpu/drm/drm_atomic.c  | 45 +++++++++++++++++++++++++++++++++++---
> >  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
> >  include/drm/drm_mode_config.h |  9 +++++++++
> >  4 files changed, 85 insertions(+), 3 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 37445d50816a..15e1a35c74a8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> > +/**
> > + * DOC: handling driver private state
> > + *
> > + * Very often the DRM objects exposed to userspace in the atomic modeset
> > api + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to
> > the + * underlying hardware. Especially for any kind of shared resources
> > (e.g. shared + * clocks, scaler units, bandwidth and fifo limits shared
> > among a group of + * planes or CRTCs, and so on) it makes sense to model
> > these as independent + * objects. Drivers then need to similar state
> > tracking and commit ordering for + * such private (since not exposed to
> > userpace) objects as the atomic core and + * helpers already provide for
> > connectors, planes and CRTCs.
> 
> This last sentence doesn't quite parse.  I think it should be as follows:
> 
> Drivers then need to do similar state tracking and commit ordering for
> such private (since not exposed to userpace) objects as the atomic core that
> helpers already provide for DRM objects (connectors, planes and CRTCs).

I think Daniel meant

"Drivers then need similar state tracking and commit ordering for such private 
(since not exposed to userpace) objects as the atomic core and helpers already 
provide for connectors, planes and CRTCs."

[snip]

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> DK put some nice docs into the commit introducing driver private
> state, but in the git history alone it'll be lost.

You might want to spell DK's name fully.

> Also, since Ville remove the void* usage it's a good opportunity to
> give the driver private stuff some tlc on the doc front.
> 
> Finally try to explain why the "let's just subclass drm_atomic_state"
> approach wasn't the greatest, and annotate all those functions as
> deprecated in favour of more standardized driver private states. Also
> note where we could/should extend driver private states going forward
> (atm neither locking nor synchronization is handled in core/helpers,
> which isn't really all that great).
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms.rst |  6 ++++++
>  drivers/gpu/drm/drm_atomic.c  | 45 +++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h |  9 +++++++++
>  4 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 307284125d7a..420025bd6a9b 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -271,6 +271,12 @@ Taken all together there's two consequences for the
> atomic design: Read on in this chapter, and also in
> :ref:`drm_atomic_helper` for more detailed coverage of specific topics.
> 
> +Handling Driver Private State
> +-----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: handling driver private state
> +
>  Atomic Mode Setting Function Reference
>  --------------------------------------
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..15e1a35c74a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
>   * @state: atomic state
>   *
>   * Free all the memory allocated by drm_atomic_state_init.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_release(struct drm_atomic_state *state)
>  {
> @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
>   * @state: atomic state
>   *
>   * Default implementation for filling in a new atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  int
>  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state
> *state)
> @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
>   * @state: atomic state
>   *
>   * Default implementation for clearing atomic state.
> - * This is useful for drivers that subclass the atomic state.
> + * This should only be used by drivers which are still subclassing
> + * &drm_atomic_state and haven't switched to &drm_private_state yet.
>   */
>  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  {
> @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct
> drm_printer *p, plane->funcs->atomic_print_state(p, state);
>  }
> 
> +/**
> + * DOC: handling driver private state
> + *
> + * Very often the DRM objects exposed to userspace in the atomic modeset
> api

s/api/API/

> + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> + * underlying hardware. Especially for any kind of shared resources (e.g.
> shared
> + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> + * planes or CRTCs, and so on) it makes sense to model these as independent
> + * objects. Drivers then need to similar state tracking and commit ordering
> for
> + * such private (since not exposed to userpace) objects as the atomic core
> and
> + * helpers already provide for connectors, planes and CRTCs.
> 
> + * To make this easier on drivers the atomic core provides some support to
> track
> + * driver private state objects using struct &drm_private_obj, with the
> + * associated state struct &drm_private_state.
> + *
> + * Similar to userspace-exposed objects, state structures can be acquired
> by
> + * calling drm_atomic_get_private_obj_state(). Since this function does not
> take
> + * care of locking, drivers should wrap it for each type of private state
> object
> + * they have with the required call to drm_modeset_lock() for the
> corresponding
> + * &drm_modeset_lock.

This paragraph could benefit from an explanation of what the corresponding 
drm_modeset_lock is. The rest of the document is pretty clear.

> + * All private state structures contained in a &drm_atomic_state update can
> be
> + * iterated using for_each_oldnew_private_obj_in_state(),
> + * for_each_old_private_obj_in_state() and
> for_each_old_private_obj_in_state().

I think one of those two was meant to be for_each_new_private_obj_in_state().

> + * Drivers are recommend to wrap these for each type of driver private
> state
> + * object they have, filtering on &drm_private_obj.funcs using
> for_each_if(), at
> + * least if they want to iterate over all objects of a given type.
> + *
> + * An earlier way to handle driver private state was by subclassing struct
> + * &drm_atomic_state. But since that encourages non-standard ways to
> implement
> + * the check/commit split atomic requires (by using e.g. "check and
> rollback or
> + * commit instead" of "duplicate state, check, then either commit or
> release
> + * duplicated state) it is deprecated in favour of using
> &drm_private_state.

This I still don't agree with. I think it still makes sense to subclass the 
global state object when you have true global state data. How about starting 
by making it a recommendation instead, moving state data related to driver-
specific objects to the new framework, and keeping global data in the 
drm_atomic_state subclass ?

> + */
> +
>  /**
>   * drm_atomic_private_obj_init - initialize private object
>   * @obj: private object
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 5afd6e364fb6..8e4829a25c1b 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -189,12 +189,40 @@ struct drm_private_state_funcs {
>  				     struct drm_private_state *state);
>  };
> 
> +/**
> + * struct drm_private_obj - base struct for driver private atomic object
> + *
> + * A driver private object is initialized by calling
> + * drm_atomic_private_obj_init() and cleaned up by calling
> + * drm_atomic_private_obj_fini().
> + *
> + * Currently only tracks the state update functions and the opaque driver
> + * private state itself, but in the future might also track which
> + * &drm_modeset_lock is required to duplicate and update this object's
> state. + */
>  struct drm_private_obj {
> +	/**
> +	 * @state: Current atomic state for this driver private object.
> +	 */
>  	struct drm_private_state *state;
> 
> +	/**
> +	 * @funcs:
> +	 *
> +	 * Functions to manipulate the state of this driver private object, see
> +	 * &drm_private_state_funcs.
> +	 */
>  	const struct drm_private_state_funcs *funcs;
>  };
> 
> +/**
> + * struct drm_private_state - base struct for driver private object state
> + * @state: backpointer to global drm_atomic_state
> + *
> + * Currently only contains a backpointer to the overall atomic upated, but
> in + * the future also might hold synchronization information similar to
> e.g. + * &drm_crtc.commit.
> + */
>  struct drm_private_state {
>  	struct drm_atomic_state *state;
>  };
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index c6639e8e5007..2cb6f02df64a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -269,6 +269,9 @@ struct drm_mode_config_funcs {
>  	 * state easily. If this hook is implemented, drivers must also
>  	 * implement @atomic_state_clear and @atomic_state_free.
>  	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
> +	 *
>  	 * RETURNS:
>  	 *
>  	 * A new &drm_atomic_state on success or NULL on failure.
> @@ -290,6 +293,9 @@ struct drm_mode_config_funcs {
>  	 *
>  	 * Drivers that implement this must call drm_atomic_state_default_clear()
>  	 * to clear common state.
> +	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
>  	 */
>  	void (*atomic_state_clear)(struct drm_atomic_state *state);
> 
> @@ -302,6 +308,9 @@ struct drm_mode_config_funcs {
>  	 *
>  	 * Drivers that implement this must call
>  	 * drm_atomic_state_default_release() to release common resources.
> +	 *
> +	 * Subclassing of &drm_atomic_state is deprecated in favour of using
> +	 * &drm_private_state and &drm_private_obj.
>  	 */
>  	void (*atomic_state_free)(struct drm_atomic_state *state);
>  };

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Daniel Vetter]

On Mon, Dec 18, 2017 at 06:09:20PM +0200, Laurent Pinchart wrote:
> Hi Alex,
> 
> On Friday, 15 December 2017 03:57:48 EET Alex Deucher wrote:
> > On Thu, Dec 14, 2017 at 3:30 PM, Daniel Vetter wrote:
> > > DK put some nice docs into the commit introducing driver private
> > > state, but in the git history alone it'll be lost.
> > > 
> > > Also, since Ville remove the void* usage it's a good opportunity to
> > > give the driver private stuff some tlc on the doc front.
> > > 
> > > Finally try to explain why the "let's just subclass drm_atomic_state"
> > > approach wasn't the greatest, and annotate all those functions as
> > > deprecated in favour of more standardized driver private states. Also
> > > note where we could/should extend driver private states going forward
> > > (atm neither locking nor synchronization is handled in core/helpers,
> > > which isn't really all that great).
> > > 
> > > Cc: Harry Wentland <harry.wentland@amd.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > > 
> > >  Documentation/gpu/drm-kms.rst |  6 ++++++
> > >  drivers/gpu/drm/drm_atomic.c  | 45 +++++++++++++++++++++++++++++++++++---
> > >  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
> > >  include/drm/drm_mode_config.h |  9 +++++++++
> > >  4 files changed, 85 insertions(+), 3 deletions(-)
> 
> [snip]
> 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 37445d50816a..15e1a35c74a8 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> 
> [snip]
> 
> > > +/**
> > > + * DOC: handling driver private state
> > > + *
> > > + * Very often the DRM objects exposed to userspace in the atomic modeset
> > > api + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to
> > > the + * underlying hardware. Especially for any kind of shared resources
> > > (e.g. shared + * clocks, scaler units, bandwidth and fifo limits shared
> > > among a group of + * planes or CRTCs, and so on) it makes sense to model
> > > these as independent + * objects. Drivers then need to similar state
> > > tracking and commit ordering for + * such private (since not exposed to
> > > userpace) objects as the atomic core and + * helpers already provide for
> > > connectors, planes and CRTCs.
> > 
> > This last sentence doesn't quite parse.  I think it should be as follows:
> > 
> > Drivers then need to do similar state tracking and commit ordering for
> > such private (since not exposed to userpace) objects as the atomic core that
> > helpers already provide for DRM objects (connectors, planes and CRTCs).
> 
> I think Daniel meant
> 
> "Drivers then need similar state tracking and commit ordering for such private 
> (since not exposed to userpace) objects as the atomic core and helpers already 
> provide for connectors, planes and CRTCs."

Already applied before I spotted your reply, but yeah Alex' wasn't
entirely correct either, so I picked a mix.

Thanks for feedback anyway.
-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 4/5] drm/atomic: document how to handle driver private objects

[Daniel Vetter]

On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > DK put some nice docs into the commit introducing driver private
> > state, but in the git history alone it'll be lost.
> 
> You might want to spell DK's name fully.
> 
> > Also, since Ville remove the void* usage it's a good opportunity to
> > give the driver private stuff some tlc on the doc front.
> > 
> > Finally try to explain why the "let's just subclass drm_atomic_state"
> > approach wasn't the greatest, and annotate all those functions as
> > deprecated in favour of more standardized driver private states. Also
> > note where we could/should extend driver private states going forward
> > (atm neither locking nor synchronization is handled in core/helpers,
> > which isn't really all that great).
> > 
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Ben Skeggs <bskeggs@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/gpu/drm-kms.rst |  6 ++++++
> >  drivers/gpu/drm/drm_atomic.c  | 45 +++++++++++++++++++++++++++++++++++++---
> >  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
> >  include/drm/drm_mode_config.h |  9 +++++++++
> >  4 files changed, 85 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 307284125d7a..420025bd6a9b 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -271,6 +271,12 @@ Taken all together there's two consequences for the
> > atomic design: Read on in this chapter, and also in
> > :ref:`drm_atomic_helper` for more detailed coverage of specific topics.
> > 
> > +Handling Driver Private State
> > +-----------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> > +   :doc: handling driver private state
> > +
> >  Atomic Mode Setting Function Reference
> >  --------------------------------------
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 37445d50816a..15e1a35c74a8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -50,7 +50,8 @@ EXPORT_SYMBOL(__drm_crtc_commit_free);
> >   * @state: atomic state
> >   *
> >   * Free all the memory allocated by drm_atomic_state_init.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> >   */
> >  void drm_atomic_state_default_release(struct drm_atomic_state *state)
> >  {
> > @@ -67,7 +68,8 @@ EXPORT_SYMBOL(drm_atomic_state_default_release);
> >   * @state: atomic state
> >   *
> >   * Default implementation for filling in a new atomic state.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> >   */
> >  int
> >  drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state
> > *state)
> > @@ -132,7 +134,8 @@ EXPORT_SYMBOL(drm_atomic_state_alloc);
> >   * @state: atomic state
> >   *
> >   * Default implementation for clearing atomic state.
> > - * This is useful for drivers that subclass the atomic state.
> > + * This should only be used by drivers which are still subclassing
> > + * &drm_atomic_state and haven't switched to &drm_private_state yet.
> >   */
> >  void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >  {
> > @@ -946,6 +949,42 @@ static void drm_atomic_plane_print_state(struct
> > drm_printer *p, plane->funcs->atomic_print_state(p, state);
> >  }
> > 
> > +/**
> > + * DOC: handling driver private state
> > + *
> > + * Very often the DRM objects exposed to userspace in the atomic modeset
> > api
> 
> s/api/API/
> 
> > + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> > + * underlying hardware. Especially for any kind of shared resources (e.g.
> > shared
> > + * clocks, scaler units, bandwidth and fifo limits shared among a group of
> > + * planes or CRTCs, and so on) it makes sense to model these as independent
> > + * objects. Drivers then need to similar state tracking and commit ordering
> > for
> > + * such private (since not exposed to userpace) objects as the atomic core
> > and
> > + * helpers already provide for connectors, planes and CRTCs.
> > 
> > + * To make this easier on drivers the atomic core provides some support to
> > track
> > + * driver private state objects using struct &drm_private_obj, with the
> > + * associated state struct &drm_private_state.
> > + *
> > + * Similar to userspace-exposed objects, state structures can be acquired
> > by
> > + * calling drm_atomic_get_private_obj_state(). Since this function does not
> > take
> > + * care of locking, drivers should wrap it for each type of private state
> > object
> > + * they have with the required call to drm_modeset_lock() for the
> > corresponding
> > + * &drm_modeset_lock.
> 
> This paragraph could benefit from an explanation of what the corresponding 
> drm_modeset_lock is. The rest of the document is pretty clear.

Hm yeah ... This is also one of those things I'd like to improve in the
private state stuff: If we add a filed for the lock (a pointer, not the
lock itself) we could simplify this stuff a lot.

> > + * All private state structures contained in a &drm_atomic_state update can
> > be
> > + * iterated using for_each_oldnew_private_obj_in_state(),
> > + * for_each_old_private_obj_in_state() and
> > for_each_old_private_obj_in_state().
> 
> I think one of those two was meant to be for_each_new_private_obj_in_state().

Fixed.

> > + * Drivers are recommend to wrap these for each type of driver private
> > state
> > + * object they have, filtering on &drm_private_obj.funcs using
> > for_each_if(), at
> > + * least if they want to iterate over all objects of a given type.
> > + *
> > + * An earlier way to handle driver private state was by subclassing struct
> > + * &drm_atomic_state. But since that encourages non-standard ways to
> > implement
> > + * the check/commit split atomic requires (by using e.g. "check and
> > rollback or
> > + * commit instead" of "duplicate state, check, then either commit or
> > release
> > + * duplicated state) it is deprecated in favour of using
> > &drm_private_state.
> 
> This I still don't agree with. I think it still makes sense to subclass the 
> global state object when you have true global state data. How about starting 
> by making it a recommendation instead, moving state data related to driver-
> specific objects to the new framework, and keeping global data in the 
> drm_atomic_state subclass ?

Converting all the existing drivers over is somewhere on my todo. I'm also
not really clear what you mean with global data compared to
driver-specific objects ... And see the evolution of the i915 state stuff,
imo it's a bit too easy to get the drm_atomic_state subclassing wrong.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Laurent Pinchart]

Hi Daniel,

On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> >> DK put some nice docs into the commit introducing driver private
> >> state, but in the git history alone it'll be lost.
> > 
> > You might want to spell DK's name fully.
> > 
> >> Also, since Ville remove the void* usage it's a good opportunity to
> >> give the driver private stuff some tlc on the doc front.
> >> 
> >> Finally try to explain why the "let's just subclass drm_atomic_state"
> >> approach wasn't the greatest, and annotate all those functions as
> >> deprecated in favour of more standardized driver private states. Also
> >> note where we could/should extend driver private states going forward
> >> (atm neither locking nor synchronization is handled in core/helpers,
> >> which isn't really all that great).
> >> 
> >> Cc: Harry Wentland <harry.wentland@amd.com>
> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Ben Skeggs <bskeggs@redhat.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >> ---
> >> 
> >>  Documentation/gpu/drm-kms.rst |  6 ++++++
> >>  drivers/gpu/drm/drm_atomic.c  | 45 ++++++++++++++++++++++++++++++++++---
> >>  include/drm/drm_atomic.h      | 28 +++++++++++++++++++++++++++
> >>  include/drm/drm_mode_config.h |  9 +++++++++
> >>  4 files changed, 85 insertions(+), 3 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 37445d50816a..15e1a35c74a8 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c

[snip]

> >> +/**
> >> + * DOC: handling driver private state
> >> + *
> >> + * Very often the DRM objects exposed to userspace in the atomic
> >> modeset api
> > 
> > s/api/API/
> > 
> >> + * (&drm_connector, &drm_crtc and &drm_plane) do not map neatly to the
> >> + * underlying hardware. Especially for any kind of shared resources
> >> (e.g. shared
> >> + * clocks, scaler units, bandwidth and fifo limits shared among a group
> >> of
> >> + * planes or CRTCs, and so on) it makes sense to model these as
> >> independent
> >> + * objects. Drivers then need to similar state tracking and commit
> >> ordering for
> >> + * such private (since not exposed to userpace) objects as the atomic
> >> core and
> >> + * helpers already provide for connectors, planes and CRTCs.
> >> 
> >> + * To make this easier on drivers the atomic core provides some support
> >> to track
> >> + * driver private state objects using struct &drm_private_obj, with the
> >> + * associated state struct &drm_private_state.
> >> + *
> >> + * Similar to userspace-exposed objects, state structures can be
> >> acquired by
> >> + * calling drm_atomic_get_private_obj_state(). Since this function does
> >> not take
> >> + * care of locking, drivers should wrap it for each type of private
> >> state object
> >> + * they have with the required call to drm_modeset_lock() for the
> >> corresponding
> >> + * &drm_modeset_lock.
> > 
> > This paragraph could benefit from an explanation of what the corresponding
> > drm_modeset_lock is. The rest of the document is pretty clear.
> 
> Hm yeah ... This is also one of those things I'd like to improve in the
> private state stuff: If we add a filed for the lock (a pointer, not the
> lock itself) we could simplify this stuff a lot.

We don't have to fix everything in one go of course, but a small explanation 
of what drivers are supposed to do would be helpful.

> >> + * All private state structures contained in a &drm_atomic_state update
> >> can be
> >> + * iterated using for_each_oldnew_private_obj_in_state(),
> >> + * for_each_old_private_obj_in_state() and
> >> for_each_old_private_obj_in_state().
> > 
> > I think one of those two was meant to be
> > for_each_new_private_obj_in_state().
> 
> Fixed.
> 
> >> + * Drivers are recommend to wrap these for each type of driver private
> >> state
> >> + * object they have, filtering on &drm_private_obj.funcs using
> >> for_each_if(), at
> >> + * least if they want to iterate over all objects of a given type.
> >> + *
> >> + * An earlier way to handle driver private state was by subclassing
> >> struct
> >> + * &drm_atomic_state. But since that encourages non-standard ways to
> >> implement
> >> + * the check/commit split atomic requires (by using e.g. "check and
> >> rollback or
> >> + * commit instead" of "duplicate state, check, then either commit or
> >> release
> >> + * duplicated state) it is deprecated in favour of using
> >> &drm_private_state.
> > 
> > This I still don't agree with. I think it still makes sense to subclass
> > the global state object when you have true global state data. How about
> > starting by making it a recommendation instead, moving state data related
> > to driver- specific objects to the new framework, and keeping global data
> > in the drm_atomic_state subclass ?
> 
> Converting all the existing drivers over is somewhere on my todo. I'm also
> not really clear what you mean with global data compared to
> driver-specific objects ...

I'll take an example related to the rcar-du driver. The hardware groups CRTCs 
by two and share resources (such as planes) between CRTCs in a group. This is 
something I currently implement in a convoluted way, and using private objects 
to handle groups (I already have a group object in my driver) will likely help 
to model the group state.

On the other hand, if the hardware didn't have groups but shared planes 
between all CRTCs, the shared resources would be global, and it would make 
sense to store them in the global state.

> And see the evolution of the i915 state stuff,
> imo it's a bit too easy to get the drm_atomic_state subclassing wrong.

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Daniel Vetter]

On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote:
> On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > >> + * Drivers are recommend to wrap these for each type of driver private
> > >> state
> > >> + * object they have, filtering on &drm_private_obj.funcs using
> > >> for_each_if(), at
> > >> + * least if they want to iterate over all objects of a given type.
> > >> + *
> > >> + * An earlier way to handle driver private state was by subclassing
> > >> struct
> > >> + * &drm_atomic_state. But since that encourages non-standard ways to
> > >> implement
> > >> + * the check/commit split atomic requires (by using e.g. "check and
> > >> rollback or
> > >> + * commit instead" of "duplicate state, check, then either commit or
> > >> release
> > >> + * duplicated state) it is deprecated in favour of using
> > >> &drm_private_state.
> > > 
> > > This I still don't agree with. I think it still makes sense to subclass
> > > the global state object when you have true global state data. How about
> > > starting by making it a recommendation instead, moving state data related
> > > to driver- specific objects to the new framework, and keeping global data
> > > in the drm_atomic_state subclass ?
> > 
> > Converting all the existing drivers over is somewhere on my todo. I'm also
> > not really clear what you mean with global data compared to
> > driver-specific objects ...
> 
> I'll take an example related to the rcar-du driver. The hardware groups CRTCs 
> by two and share resources (such as planes) between CRTCs in a group. This is 
> something I currently implement in a convoluted way, and using private objects 
> to handle groups (I already have a group object in my driver) will likely help 
> to model the group state.
> 
> On the other hand, if the hardware didn't have groups but shared planes 
> between all CRTCs, the shared resources would be global, and it would make 
> sense to store them in the global state.

Yeah the private stuff should probably get a hole lot better for singleton
objects. I still think one global thing overall (and with a state handling
pattern that's different from everything else) is not a good idea.

Now it would be fairly easy to generate all the silly boilerplate with a
macro, but that tends to wreak havoc with cscope and friends, so I'm not
sure it's a great idea really. I'll probably have better ideas once the
i915 conversion exists ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/atomic: document how to handle driver private objects

[Laurent Pinchart]

Hi Daniel,

On Tuesday, 19 December 2017 16:00:36 EET Daniel Vetter wrote:
> On Tue, Dec 19, 2017 at 03:33:57PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 19 December 2017 12:03:55 EET Daniel Vetter wrote:
> > > On Mon, Dec 18, 2017 at 06:13:46PM +0200, Laurent Pinchart wrote:
> > > > On Thursday, 14 December 2017 22:30:53 EET Daniel Vetter wrote:
> > > >> + * Drivers are recommend to wrap these for each type of driver
> > > >> private
> > > >> state
> > > >> + * object they have, filtering on &drm_private_obj.funcs using
> > > >> for_each_if(), at
> > > >> + * least if they want to iterate over all objects of a given type.
> > > >> + *
> > > >> + * An earlier way to handle driver private state was by subclassing
> > > >> struct
> > > >> + * &drm_atomic_state. But since that encourages non-standard ways to
> > > >> implement
> > > >> + * the check/commit split atomic requires (by using e.g. "check and
> > > >> rollback or
> > > >> + * commit instead" of "duplicate state, check, then either commit or
> > > >> release
> > > >> + * duplicated state) it is deprecated in favour of using
> > > >> &drm_private_state.
> > > > 
> > > > This I still don't agree with. I think it still makes sense to
> > > > subclass
> > > > the global state object when you have true global state data. How
> > > > about
> > > > starting by making it a recommendation instead, moving state data
> > > > related
> > > > to driver- specific objects to the new framework, and keeping global
> > > > data
> > > > in the drm_atomic_state subclass ?
> > > 
> > > Converting all the existing drivers over is somewhere on my todo. I'm
> > > also
> > > not really clear what you mean with global data compared to
> > > driver-specific objects ...
> > 
> > I'll take an example related to the rcar-du driver. The hardware groups
> > CRTCs by two and share resources (such as planes) between CRTCs in a
> > group. This is something I currently implement in a convoluted way, and
> > using private objects to handle groups (I already have a group object in
> > my driver) will likely help to model the group state.
> > 
> > On the other hand, if the hardware didn't have groups but shared planes
> > between all CRTCs, the shared resources would be global, and it would make
> > sense to store them in the global state.
> 
> Yeah the private stuff should probably get a hole lot better for singleton
> objects. I still think one global thing overall (and with a state handling
> pattern that's different from everything else) is not a good idea.
> 
> Now it would be fairly easy to generate all the silly boilerplate with a
> macro, but that tends to wreak havoc with cscope and friends, so I'm not
> sure it's a great idea really. I'll probably have better ideas once the
> i915 conversion exists ...

So how about splitting this in two steps then, first deprecating subclassing 
drm_atomic_state to store private object state, and only in a second step also 
deprecating subclassing the structure for global state ?

-- 
Regards,

Laurent Pinchart

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