lkml.org 
[lkml]   [2021]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] drm: clarify lifetime/locking for drm_master's lease fields
From
Date
On 22/7/21 6:35 pm, Daniel Vetter wrote:
> On Thu, Jul 22, 2021 at 05:29:28PM +0800, Desmond Cheong Zhi Xi wrote:
>> In particular, we 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.
>>
>> Additionally, we add descriptions for the lifetime of lessors and
>> leases to make it easier to reason about them.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>> include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>> index f99d3417f304..c978c85464fa 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,63 @@ 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 holder. The lessor does not change once it's set in
>
> Lessor is the lease grantor, lessee is the one receiving the lease. Maybe
> clarify this with
>
> "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."
>
>> + * drm_lease_create(). Each lessee holds a reference to its lessor that
>
> I also figured it'd be a good idea to link to the drm_lease docs here to
> explain the concepts, but alas we don't have those :-( Hence at least
> define what we mean with lessor, lessee, lease and owner.
>

Thanks for the suggestions, Daniel. I'll incorporate them in a v2.

Regarding the missing drm_lease docs... any reason we can't just add it
in? Seems like most of the comments in drm_lease.c are almost correctly
formatted too. I could clean them up, define the terms in a preamble,
then add it to the v2 patch.

>> + * it releases upon being destroyed in drm_lease_destroy().
>> + *
>> + * Display resource leases form a tree of &struct drm_master. All of
>
> For now we've limited the tree to a depth of 1, you can't create another
> lease if yourself are a lease. I guess another reason to update the
> drm_lease.c docs.
>
> So maybe add "Currently the lease tree depth is limited to 1."
>
>> + * these get activated simultaneously, so &drm_device.master
>> + * points at the top of the tree (for which lessor is NULL).
>> */
>> -
>> struct drm_master *lessor;
>> +
>> + /**
>> + * @lessee_id:
>> + *
>> + * ID for lessees. Owners always have ID 0. Protected by
>
> Maybe clarify to "Owners (i.e. @lessor is NULL) ..."
>
>> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>> int lessee_id;
>> +
>> + /**
>> + * @lessee_list:
>> + *
>> + * List of lessees of the same master. Protected by
>
> We try to distinguis between list entries and the list, even though it's
> the same struct. So maybe
>
> "List entry of lessees of @lessor, where they are linked to @lessee. Not
> use for owners."
>
>> + * &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 master cannot be destroyed unless this list is empty as lessors
>> + * are referenced by all their lessees.
>
> Maybe add "this list is empty of no leases have been granted."
>
>> + */
>> 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).
>
> @lessor so it's highlighted correctly
>
>> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>> struct idr lessee_idr;
>> /* private: */
>
> With the nits addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks for updating the docs!
> -Daniel
>
>> #if IS_ENABLED(CONFIG_DRM_LEGACY)
>> --
>> 2.25.1
>>
>

\
 
 \ /
  Last update: 2021-07-22 19:35    [W:0.095 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site