From 6a56d09bdab7c9d17ca2cf70c62ec162bbb972f7 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 21 Jan 2021 12:29:19 +0100 Subject: drm: Update todo.rst Internship season is starting, let's review this. One thing that's pending is Maxime's work to roll out drm_atomic_state pointers to all callbacks, he said he'll remove that entry once it's all done. v2: Fix typos (Maxime) Acked-by: Maxime Ripard Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20210121112919.1460322-1-daniel.vetter@ffwll.ch --- Documentation/gpu/todo.rst | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) (limited to 'Documentation/gpu/todo.rst') diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 009d8e6c7e3c..dea9082c0e5f 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -577,20 +577,24 @@ Contact: Daniel Vetter Level: Intermediate -KMS cleanups ------------- +Object lifetime fixes +--------------------- + +There's two related issues here + +- Cleanup up the various ->destroy callbacks, which often are all the same + simple code. -Some of these date from the very introduction of KMS in 2008 ... +- Lots of drivers erroneously allocate DRM modeset objects using devm_kzalloc, + which results in use-after free issues on driver unload. This can be serious + trouble even for drivers for hardware integrated on the SoC due to + EPROBE_DEFERRED backoff. -- Make ->funcs and ->helper_private vtables optional. There's a bunch of empty - function tables in drivers, but before we can remove them we need to make sure - that all the users in helpers and drivers do correctly check for a NULL - vtable. +Both these problems can be solved by switching over to drmm_kzalloc(), and the +various convenience wrappers provided, e.g. drmm_crtc_alloc_with_planes(), +drmm_universal_plane_alloc(), ... and so on. -- Cleanup up the various ->destroy callbacks. A lot of them just wrapt the - drm_*_cleanup implementations and can be removed. Some tack a kfree() at the - end, for which we could add drm_*_cleanup_kfree(). And then there's the (for - historical reasons) misnamed drm_primary_helper_destroy() function. +Contact: Daniel Vetter Level: Intermediate @@ -626,8 +630,6 @@ See the documentation of :ref:`VKMS ` for more details. This is an ideal internship task, since it only requires a virtual machine and can be sized to fit the available time. -Contact: Daniel Vetter - Level: See details Backlight Refactoring -- cgit From 5823cca39d585e4b4a32b1292eed0015da9c3276 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Fri, 22 Jan 2021 14:36:23 +0100 Subject: drm/todo: Add entry for moving to dma_resv_lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requested by Thomas. I think it justifies a new level, since I tried to make some forward progress on this last summer, and gave up (for now). This is very tricky. Acked-by: Thomas Zimmermann Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Cc: Sumit Semwal Cc: "Christian König" Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Link: https://patchwork.freedesktop.org/patch/msgid/20210122133624.1751802-1-daniel.vetter@ffwll.ch --- Documentation/gpu/todo.rst | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'Documentation/gpu/todo.rst') diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index dea9082c0e5f..f872d3d33218 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem and graphics topics. Generally need the relevant hardware for development and testing. +Expert: Only attempt these if you've successfully completed some tricky +refactorings already and are an expert in the specific area + Subsystem-wide refactorings =========================== @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers Level: Advanced +Move Buffer Object Locking to dma_resv_lock() +--------------------------------------------- + +Many drivers have their own per-object locking scheme, usually using +mutex_lock(). This causes all kinds of trouble for buffer sharing, since +depending which driver is the exporter and importer, the locking hierarchy is +reversed. + +To solve this we need one standard per-object locking mechanism, which is +dma_resv_lock(). This lock needs to be called as the outermost lock, with all +other driver specific per-object locks removed. The problem is tha rolling out +the actual change to the locking contract is a flag day, due to struct dma_buf +buffer sharing. + +Level: Expert + Convert logging to drm_* functions with drm_device paramater ------------------------------------------------------------ -- cgit From c129b49825530942f344ed3e0e6a72568ad3e3e2 Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Fri, 19 Feb 2021 13:00:31 +0100 Subject: drm/todo: Remove the drm_atomic_state todo item Only planes' prepare_fb and cleanup_fb, and encoders' atomic_check and atomic_mode_set hooks remain with an object state and not the global drm_atomic_state. prepare_fb and cleanup_fb operate by design on a given state and depending on the calling site can operate on either the old or new state, so it doesn't really make much sense to convert them. The encoders' atomic_check and atomic_mode_set operate on the CRTC and connector state connected to them since encoders don't have a state of their own. Without those state pointers, we would need to get the CRTC through the drm_connector_state crtc pointer. However, in order to get the drm_connector_state pointer, we would need to get the connector itself and while usually we have a single connector connected to the encoder, we can't really get it from the encoder at the moment since it could be behind any number of bridges. While this could be addressed by (for example) listing all the connectors and finding the one that has the encoder as its source, it feels like an unnecessary rework for something that is slowly getting replaced by bridges. Since all the users that matter have been converted, let's remove the TODO item. Acked-by: Daniel Vetter Signed-off-by: Maxime Ripard Acked-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20210219120032.260676-11-maxime@cerno.tech --- Documentation/gpu/todo.rst | 46 ---------------------------------------------- 1 file changed, 46 deletions(-) (limited to 'Documentation/gpu/todo.rst') diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index f872d3d33218..0631b9b323d5 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -459,52 +459,6 @@ Contact: Emil Velikov, respective driver maintainers Level: Intermediate -Plumb drm_atomic_state all over -------------------------------- - -Currently various atomic functions take just a single or a handful of -object states (eg. plane state). While that single object state can -suffice for some simple cases, we often have to dig out additional -object states for dealing with various dependencies between the individual -objects or the hardware they represent. The process of digging out the -additional states is rather non-intuitive and error prone. - -To fix that most functions should rather take the overall -drm_atomic_state as one of their parameters. The other parameters -would generally be the object(s) we mainly want to interact with. - -For example, instead of - -.. code-block:: c - - int (*atomic_check)(struct drm_plane *plane, struct drm_plane_state *state); - -we would have something like - -.. code-block:: c - - int (*atomic_check)(struct drm_plane *plane, struct drm_atomic_state *state); - -The implementation can then trivially gain access to any required object -state(s) via drm_atomic_get_plane_state(), drm_atomic_get_new_plane_state(), -drm_atomic_get_old_plane_state(), and their equivalents for -other object types. - -Additionally many drivers currently access the object->state pointer -directly in their commit functions. That is not going to work if we -eg. want to allow deeper commit pipelines as those pointers could -then point to the states corresponding to a future commit instead of -the current commit we're trying to process. Also non-blocking commits -execute locklessly so there are serious concerns with dereferencing -the object->state pointers without holding the locks that protect them. -Use of drm_atomic_get_new_plane_state(), drm_atomic_get_old_plane_state(), -etc. avoids these problems as well since they relate to a specific -commit via the passed in drm_atomic_state. - -Contact: Ville Syrjälä, Daniel Vetter - -Level: Intermediate - Use struct dma_buf_map throughout codebase ------------------------------------------ -- cgit