Messages in this thread | | | Subject | Re: [PATCH v2 2/3] drm: clarify usage of drm leases | From | Desmond Cheong Zhi Xi <> | Date | Wed, 28 Jul 2021 18:01:03 +0800 |
| |
On 27/7/21 9:04 pm, Daniel Vetter wrote: > On Sat, Jul 24, 2021 at 07:18:23PM +0800, Desmond Cheong Zhi Xi wrote: >> We make the following changes to the documentation of drm leases to >> make it easier to reason about their usage. In particular, we clarify >> the lifetime and locking rules of lease fields in drm_master: >> >> 1. Make it clear that &drm_device.mode_config.idr_mutex protects the >> lease idr and list structures for drm_master. The lessor field itself >> doesn't need to be protected as it doesn't change after it's set in >> drm_lease_create. >> >> 2. Add descriptions for the lifetime of lessors and leases. >> >> 3. Add an overview DOC: section in drm-uapi.rst that defines the >> terminology for drm leasing, and explains how leases work and why >> they're used. >> >> 4. Clean up function documentation in drm_lease.c to use kernel-doc >> formatting. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> >> Hi, >> >> After I updated the formatting for comments in drm_lease.c, I noticed >> that none of these were driver interfaces (i.e. no structs/inline >> functions declared in headers, and no exported symbols in .c files). >> >> I left the kernel-doc links inside drm-uapi.rst so that if any such >> interfaces are defined in the future, they'll go to the appropriate >> place. But if these should be removed, or if the formatting changes for >> function comments should be removed, please let me know. > > > Hm indeed, so there's not really any need to either include the > drm_lease.c or drm_lease.h kerneldoc. The DOC section itself is still > useful. > > For the internal pieces usually what we do is remove the comment outright > if it doesn't provide anything useful (like just repeats what the function > name says already). If there's something interesting in the comment then > we leave it that sentence in there as a normal comment, but without any of > the structured comment stuff (so no /**, nor @arguments, or the function > summary). > > > >> >> Best wishes, >> Desmond >> >> Documentation/gpu/drm-uapi.rst | 15 +++ >> drivers/gpu/drm/drm_lease.c | 182 ++++++++++++++++++++------------- >> include/drm/drm_auth.h | 67 ++++++++++-- >> 3 files changed, 180 insertions(+), 84 deletions(-) >> >> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst >> index 7e51dd40bf6e..6d7233a9fb14 100644 >> --- a/Documentation/gpu/drm-uapi.rst >> +++ b/Documentation/gpu/drm-uapi.rst >> @@ -37,6 +37,21 @@ Primary Nodes, DRM Master and Authentication >> .. kernel-doc:: include/drm/drm_auth.h >> :internal: >> >> + >> +.. _drm_leasing: >> + >> +DRM Display Resource Leasing >> +============================ >> + >> +.. kernel-doc:: drivers/gpu/drm/drm_lease.c >> + :doc: drm leasing >> + >> +.. kernel-doc:: drivers/gpu/drm/drm_lease.c >> + :export: >> + >> +.. kernel-doc:: include/drm/drm_lease.h >> + :internal: >> + >> Open-Source Userspace Requirements >> ================================== >> >> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c >> index 92eac73d9001..9b68617840ed 100644 >> --- a/drivers/gpu/drm/drm_lease.c >> +++ b/drivers/gpu/drm/drm_lease.c >> @@ -15,18 +15,67 @@ >> #include "drm_crtc_internal.h" >> #include "drm_internal.h" >> >> +/** >> + * DOC: drm leasing >> + * >> + * DRM leases provide information about whether a DRM master may control a DRM >> + * mode setting object. This enables the creation of multiple DRM masters that >> + * manage subsets of display resources. >> + * >> + * The original DRM master of a device 'owns' the available drm resources. It >> + * may create additional DRM masters and 'lease' resources which it controls >> + * to the new DRM master. This gives the new DRM master control over the >> + * leased resources until the owner revokes the lease, or the new DRM master >> + * is closed. Some helpful terminology: >> + * >> + * - An 'owner' is a &struct drm_master that is not leasing objects from >> + * another &struct drm_master, and hence 'owns' the objects. The owner can be >> + * identified as the &struct drm_master for which &drm_master.lessor is NULL. >> + * >> + * - A 'lessor' is a &struct drm_master which is leasing objects to one or more >> + * other &struct drm_master. Currently, lessees are not allowed to >> + * create sub-leases, hence the lessor is the same as the owner. >> + * >> + * - A 'lessee' is a &struct drm_master which is leasing objects from some >> + * other &struct drm_master. Each lessee only leases resources from a single >> + * lessor recorded in &drm_master.lessor, and holds the set of objects that >> + * it is leasing in &drm_master.leases. >> + * >> + * - A 'lease' is a contract between the lessor and lessee that identifies >> + * which resources may be controlled by the lessee. All of the resources >> + * that are leased must be owned by or leased to the lessor, and lessors are >> + * not permitted to lease the same object to multiple lessees. >> + * >> + * The set of objects any &struct drm_master 'controls' is limited to the set >> + * of objects it leases (for lessees) or all objects (for owners). >> + * >> + * Objects not controlled by a &struct drm_master cannot be modified through >> + * the various state manipulating ioctls, and any state reported back to user >> + * space will be edited to make them appear idle and/or unusable. For >> + * instance, connectors always report 'disconnected', while encoders >> + * report no possible crtcs or clones. >> + * >> + * Since each lessee may lease objects from a single lessor, display resource >> + * leases form a tree of &struct drm_master. As lessees are currently not >> + * allowed to create sub-leases, the tree depth is limited to 1. All of >> + * these get activated simultaneously, so &drm_device.master points to the > > I think it would be good to clarify that this happens when the top level > owner changes through the SETMASTER/DROPMASTER IOCTL. Otherwise this all > looks good, thanks for typing it up. > >> + * owner at the top of the lease tree (i.e. the &struct drm_master for which >> + * &drm_master.lessor is NULL). The full list of lessees that are leasing >> + * objects from the owner can be searched via the owner's >> + * &drm_master.lessee_idr. >> + */ >> + >> #define drm_for_each_lessee(lessee, lessor) \ >> list_for_each_entry((lessee), &(lessor)->lessees, lessee_list) >> >> static uint64_t drm_lease_idr_object; >> >> /** >> - * drm_lease_owner - return ancestor owner drm_master >> - * @master: drm_master somewhere within tree of lessees and lessors >> - * >> - * RETURN: >> + * drm_lease_owner - return the device owner in @master's lease tree >> + * @master: &struct drm_master somewhere within tree of lessees and lessors >> * >> - * drm_master at the top of the tree (i.e, with lessor NULL >> + * Returns the &struct drm_master that owns the device, i.e. the &struct >> + * drm_master at the top of the tree for which &drm_master.lessor is NULL. >> */ >> struct drm_master *drm_lease_owner(struct drm_master *master) >> { >> @@ -37,12 +86,10 @@ struct drm_master *drm_lease_owner(struct drm_master *master) >> >> /** >> * _drm_find_lessee - find lessee by id (idr_mutex held) >> - * @master: drm_master of lessor >> - * @lessee_id: id >> + * @master: &struct drm_master of lessor >> + * @lessee_id: &drm_master.lessee_id of lessee >> * >> - * RETURN: >> - * >> - * drm_master of the lessee if valid, NULL otherwise >> + * Returns &struct drm_master of the lessee if the ID is valid, NULL otherwise. >> */ >> >> static struct drm_master* >> @@ -53,15 +100,12 @@ _drm_find_lessee(struct drm_master *master, int lessee_id) >> } >> >> /** >> - * _drm_lease_held_master - check to see if an object is leased (or owned) by master (idr_mutex held) >> - * @master: the master to check the lease status of >> - * @id: the id to check >> - * >> - * Checks if the specified master holds a lease on the object. Return >> - * value: >> + * _drm_lease_held_master - check to see if an object is leased (or owned) by >> + * @master (idr_mutex held) >> + * @master: the &struct drm_master to check the lease status of >> + * @id: the object ID to check >> * >> - * true 'master' holds a lease on (or owns) the object >> - * false 'master' does not hold a lease. >> + * Returns true if @master holds a lease on (or owns) the object, false if not. >> */ >> static int _drm_lease_held_master(struct drm_master *master, int id) >> { >> @@ -73,14 +117,11 @@ static int _drm_lease_held_master(struct drm_master *master, int id) >> >> /** >> * _drm_has_leased - check to see if an object has been leased (idr_mutex held) >> - * @master: the master to check the lease status of >> - * @id: the id to check >> + * @master: the &struct drm_master to check the lease status of >> + * @id: the object ID to check >> * >> - * Checks if any lessee of 'master' holds a lease on 'id'. Return >> - * value: >> - * >> - * true Some lessee holds a lease on the object. >> - * false No lessee has a lease on the object. >> + * Returns true if any lessee of @master holds a lease on the given object, >> + * false if no lessee has a lease on the object. >> */ >> static bool _drm_has_leased(struct drm_master *master, int id) >> { >> @@ -95,14 +136,11 @@ static bool _drm_has_leased(struct drm_master *master, int id) >> >> /** >> * _drm_lease_held - check drm_mode_object lease status (idr_mutex held) >> - * @file_priv: the master drm_file >> - * @id: the object id >> - * >> - * Checks if the specified master holds a lease on the object. Return >> - * value: >> + * @file_priv: the master &struct drm_file >> + * @id: the object ID >> * >> - * true 'master' holds a lease on (or owns) the object >> - * false 'master' does not hold a lease. >> + * Returns true if @file_priv's &drm_file.master holds a lease on (or owns) the >> + * object, false if it does not. >> */ >> bool _drm_lease_held(struct drm_file *file_priv, int id) >> { >> @@ -123,14 +161,11 @@ bool _drm_lease_held(struct drm_file *file_priv, int id) >> >> /** >> * drm_lease_held - check drm_mode_object lease status (idr_mutex not held) >> - * @file_priv: the master drm_file >> - * @id: the object id >> + * @file_priv: the master &struct drm_file >> + * @id: the object ID >> * >> - * Checks if the specified master holds a lease on the object. Return >> - * value: >> - * >> - * true 'master' holds a lease on (or owns) the object >> - * false 'master' does not hold a lease. >> + * Returns true if @file_priv's &drm_file.master holds a lease on (or owns) the >> + * object, false if it does not. >> */ >> bool drm_lease_held(struct drm_file *file_priv, int id) >> { >> @@ -157,7 +192,8 @@ bool drm_lease_held(struct drm_file *file_priv, int id) >> } >> >> /** >> - * drm_lease_filter_crtcs - restricted crtc set to leased values (idr_mutex not held) >> + * drm_lease_filter_crtcs - restricted crtc set to leased values >> + * (idr_mutex not held) >> * @file_priv: requestor file >> * @crtcs_in: bitmask of crtcs to check >> * >> @@ -207,13 +243,14 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) >> } >> >> /* >> - * drm_lease_create - create a new drm_master with leased objects (idr_mutex not held) >> + * drm_lease_create - create a new &struct drm_master with leased objects >> + * (idr_mutex not held) >> * @lessor: lease holder (or owner) of objects >> * @leases: objects to lease to the new drm_master >> * >> - * Uses drm_master_create to allocate a new drm_master, then checks to >> - * make sure all of the desired objects can be leased, atomically >> - * leasing them to the new drmmaster. >> + * Uses drm_master_create() to allocate a new &struct drm_master, then checks >> + * to make sure all of the desired objects can be leased, atomically >> + * leasing them to the new &struct drm_master. >> * >> * ERR_PTR(-EACCES) some other master holds the title to any object >> * ERR_PTR(-ENOENT) some object is not a valid DRM object for this device >> @@ -280,13 +317,13 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr >> } >> >> /** >> - * drm_lease_destroy - a master is going away (idr_mutex not held) >> - * @master: the drm_master being destroyed >> + * drm_lease_destroy - destroy a master (idr_mutex not held) >> + * @master: the &struct drm_master being destroyed >> * >> - * All lessees will have been destroyed as they >> - * hold a reference on their lessor. Notify any >> - * lessor for this master so that it can check >> - * the list of lessees. >> + * Destroys @master, then notifies any lessor for this master so that it can >> + * check the list of lessees. Note that all lessees of @master will have been >> + * destroyed for this function to be called as they hold a reference on their >> + * lessor. >> */ >> void drm_lease_destroy(struct drm_master *master) >> { >> @@ -323,7 +360,7 @@ void drm_lease_destroy(struct drm_master *master) >> >> /** >> * _drm_lease_revoke - revoke access to all leased objects (idr_mutex held) >> - * @top: the master losing its lease >> + * @top: the &struct drm_master losing its lease >> */ >> static void _drm_lease_revoke(struct drm_master *top) >> { >> @@ -365,7 +402,7 @@ static void _drm_lease_revoke(struct drm_master *top) >> >> /** >> * drm_lease_revoke - revoke access to all leased objects (idr_mutex not held) >> - * @top: the master losing its lease >> + * @top: the &struct drm_master losing its lease >> */ >> void drm_lease_revoke(struct drm_master *top) >> { >> @@ -500,14 +537,16 @@ static int fill_object_idr(struct drm_device *dev, >> >> /** >> * drm_mode_create_lease_ioctl - create a new lease >> - * @dev: the drm device >> - * @data: pointer to struct drm_mode_create_lease >> - * @lessor_priv: the file being manipulated >> + * @dev: the &struct drm_device >> + * @data: pointer to &struct drm_mode_create_lease >> + * @lessor_priv: the &struct drm_file being manipulated >> * >> - * The master associated with the specified file will have a lease >> + * The &struct drm_master associated with @lessor_priv will have a lease >> * created containing the objects specified in the ioctl structure. >> * A file descriptor will be allocated for that and returned to the >> * application. >> + * >> + * Note that creating recursive sub-leases for existing lessees is not allowed. >> */ >> int drm_mode_create_lease_ioctl(struct drm_device *dev, >> void *data, struct drm_file *lessor_priv) >> @@ -626,15 +665,13 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, >> } >> >> /** >> - * drm_mode_list_lessees_ioctl - list lessee ids >> - * @dev: the drm device >> - * @data: pointer to struct drm_mode_list_lessees >> - * @lessor_priv: the file being manipulated >> + * drm_mode_list_lessees_ioctl - list lessee IDs >> + * @dev: the &struct drm_device >> + * @data: pointer to &struct drm_mode_list_lessees >> + * @lessor_priv: the &struct drm_file being manipulated >> * >> - * Starting from the master associated with the specified file, >> - * the master with the provided lessee_id is found, and then >> - * an array of lessee ids associated with leases from that master >> - * are returned. >> + * Returns an array of &drm_master.lessee_id associated with lessees of >> + * @lessor_priv's &drm_file.master that have unrevoked leases. >> */ >> >> int drm_mode_list_lessees_ioctl(struct drm_device *dev, >> @@ -685,9 +722,9 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, >> >> /** >> * drm_mode_get_lease_ioctl - list leased objects >> - * @dev: the drm device >> - * @data: pointer to struct drm_mode_get_lease >> - * @lessee_priv: the file being manipulated >> + * @dev: the &struct drm_device >> + * @data: pointer to &struct drm_mode_get_lease >> + * @lessee_priv: the &struct drm_file being manipulated >> * >> * Return the list of leased objects for the specified lessee >> */ >> @@ -747,13 +784,12 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, >> >> /** >> * drm_mode_revoke_lease_ioctl - revoke lease >> - * @dev: the drm device >> - * @data: pointer to struct drm_mode_revoke_lease >> - * @lessor_priv: the file being manipulated >> + * @dev: the &struct drm_device >> + * @data: pointer to &struct drm_mode_revoke_lease >> + * @lessor_priv: the &struct drm_file being manipulated >> * >> - * This removes all of the objects from the lease without >> - * actually getting rid of the lease itself; that way all >> - * references to it still work correctly >> + * Removes all of the objects from the lessee without actually getting rid >> + * of the lessee itself; that way all references to it still work correctly. >> */ >> int drm_mode_revoke_lease_ioctl(struct drm_device *dev, >> void *data, struct drm_file *lessor_priv) >> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h >> index f99d3417f304..ba248ca8866f 100644 >> --- a/include/drm/drm_auth.h >> +++ b/include/drm/drm_auth.h >> @@ -58,12 +58,6 @@ struct drm_lock_data { >> * @refcount: Refcount for this master object. >> * @dev: Link back to the DRM device >> * @driver_priv: Pointer to driver-private information. >> - * @lessor: Lease holder >> - * @lessee_id: id for lessees. Owners always have id 0 >> - * @lessee_list: other lessees of the same master >> - * @lessees: drm_masters leasing from this one >> - * @leases: Objects leased to this drm_master. >> - * @lessee_idr: All lessees under this owner (only used where lessor == NULL) >> * >> * Note that master structures are only relevant for the legacy/primary device >> * nodes, hence there can only be one per device, not one per drm_minor. >> @@ -88,17 +82,68 @@ struct drm_master { >> struct idr magic_map; >> void *driver_priv; >> >> - /* Tree of display resource leases, each of which is a drm_master struct >> - * All of these get activated simultaneously, so drm_device master points >> - * at the top of the tree (for which lessor is NULL). Protected by >> - * &drm_device.mode_config.idr_mutex. >> + /** >> + * @lessor: >> + * >> + * Lease grantor, only set if this &struct drm_master represents a >> + * lessee holding a lease of objects from @lessor. Full owners of the >> + * device have this set to NULL. >> + * >> + * The lessor does not change once it's set in drm_lease_create(), and >> + * each lessee holds a reference to its lessor that it releases upon >> + * being destroyed in drm_lease_destroy(). >> + * >> + * See also the :ref:`section on display resource leasing >> + * <drm_leasing>`. >> */ >> - >> struct drm_master *lessor; >> + >> + /** >> + * @lessee_id: >> + * >> + * ID for lessees. Owners (i.e. @lessor is NULL) always have ID 0. >> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex. >> + */ >> int lessee_id; >> + >> + /** >> + * @lessee_list: >> + * >> + * List entry of lessees of @lessor, where they are linked to @lessees. >> + * Not used for owners. Protected by &drm_device.mode_config's >> + * &drm_mode_config.idr_mutex. >> + */ >> struct list_head lessee_list; >> + >> + /** >> + * @lessees: >> + * >> + * List of drm_masters leasing from this one. Protected by >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex. >> + * >> + * This list is empty if no leases have been granted, or if all lessees >> + * have been destroyed. Since lessors are referenced by all their >> + * lessees, this master cannot be destroyed unless the list is empty. >> + */ >> struct list_head lessees; >> + >> + /** >> + * @leases: >> + * >> + * Objects leased to this drm_master. Protected by >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex. >> + * >> + * Objects are leased all together in drm_lease_create(), and are >> + * removed all together when the lease is revoked. >> + */ >> struct idr leases; >> + >> + /** >> + * @lessee_idr: >> + * >> + * All lessees under this owner (only used where @lessor is NULL). >> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex. >> + */ > > This also looks great now. With the one improvement addressed and I guess > the includes for drm_lease.[hc] and those parts of the patch removed > again: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > If you want perhaps do a follow-up patch to clean up the comments in > drm_lease.c and remove the outdated/unused kerneldoc? It tends to not be > maintained when it's not included, so short comments where they provide > real insight is imo best. > -Daniel >
Thanks for the feedback, Daniel.
Sounds good, I'll send out the updated patch with the suggested changes, followed by a clean up of drm_lease.c.
Best wishes, Desmond
>> struct idr lessee_idr; >> /* private: */ >> #if IS_ENABLED(CONFIG_DRM_LEGACY) >> -- >> 2.25.1 >> >
| |