Re: [PATCH 3/5] drm: Add drm_object lease infrastructure [v4]

From: Sean Paul <seanpaul@chromium.org>
To: Keith Packard <keithp@keithp.com>
Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm: Add drm_object lease infrastructure [v4]
Date: Mon, 16 Oct 2017 15:44:04 -0400	[thread overview]
Message-ID: <20171016194404.ugdcgjskit44bj7v@art_vandelay> (raw)
In-Reply-To: <20171013015631.6926-4-keithp@keithp.com>

On Thu, Oct 12, 2017 at 06:56:29PM -0700, Keith Packard wrote:
> This provides new data structures to hold "lease" information about
> drm mode setting objects, and provides for creating new drm_masters
> which have access to a subset of the available drm resources.
> 
> An 'owner' is a drm_master which is not leasing the objects from
> another drm_master, and hence 'owns' them.
> 
> A 'lessee' is a drm_master which is leasing objects from some other
> drm_master. Each lessee holds the set of objects which it is leasing
> from the lessor.
> 
> A 'lessor' is a drm_master which is leasing objects to another
> drm_master. This is the same as the owner in the current code.
> 
> The set of objects any drm_master 'controls' is limited to the set of
> objects it leases (for lessees) or all objects (for owners).
> 
> Objects not controlled by a 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.
> 
> The full list of lessees leasing objects from an owner (either
> directly, or indirectly through another lessee), can be searched from
> an idr in the drm_master of the owner.
> 
> Changes for v2 as suggested by Daniel Vetter <daniel.vetter@ffwll.ch>:
> 
> * Sub-leasing has been disabled.
> 
> * BUG_ON for lock checking replaced with lockdep_assert_held
> 
> * 'change' ioctl has been removed.
> 
> * Leased objects can always be controlled by the lessor; the
>   'mask_lease' flag has been removed
> 
> * Checking for leased status has been simplified, replacing
>   the drm_lease_check function with drm_lease_held.
> 
> Changes in v3, some suggested by Dave Airlie <airlied@gmail.com>
> 
> * Add revocation. This allows leases to be effectively revoked by
>   removing all of the objects they have access to. The lease itself
>   hangs around as it's hanging off a file.
> 
> * Free the leases IDR when the master is destroyed
> 
> * _drm_lease_held should look at lessees, not lessor
> 
> * Allow non-master files to check for lease status
> 
> Changes in v4, suggested by Dave Airlie <airlied@gmail.com>
> 
> * Formatting and whitespace changes
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/Makefile      |   2 +-
>  drivers/gpu/drm/drm_auth.c    |  29 +++-
>  drivers/gpu/drm/drm_lease.c   | 339 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_auth.h        |  20 +++
>  include/drm/drm_lease.h       |  36 +++++
>  include/drm/drm_mode_object.h |   1 +
>  6 files changed, 425 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_lease.c
>  create mode 100644 include/drm/drm_lease.h
> 

<snip />

> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> new file mode 100644
> index 000000000000..6c3f2445254c
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_lease.c

<snip />

> +
> +/**
> + * _drm_lease_revoke - revoke access to all leased objects

Can you add "(idr_mutex held)" like you have in _drm_lease_held()?

> + * @master: the master losing its lease

s/master/top/

> + */
> +
> +void _drm_lease_revoke(struct drm_master *top)
> +{
> +	int object;
> +	void *entry;
> +	struct drm_master *master = top;
> +

lockdep_assert_held(&top->dev->mode_config.idr_mutex);

With these nits fixed,
Reviewed-by: Sean Paul <seanpaul@chromium.org>

> +	/*
> +	 * Walk the tree starting at 'top' emptying all leases. Because
> +	 * the tree is fully connected, we can do this without recursing
> +	 */
> +	for (;;) {
> +		DRM_DEBUG_LEASE("revoke leases for %p %d\n", master, master->lessee_id);
> +
> +		/* Evacuate the lease */
> +		idr_for_each_entry(&master->leases, entry, object)
> +			idr_remove(&master->leases, object);
> +
> +		/* Depth-first list walk */
> +
> +		/* Down */
> +		if (!list_empty(&master->lessees)) {
> +			master = list_first_entry(&master->lessees, struct drm_master, lessee_list);
> +		} else {
> +			/* Up */
> +			while (master != top && master == list_last_entry(&master->lessor->lessees, struct drm_master, lessee_list))
> +				master = master->lessor;
> +
> +			if (master == top)
> +				break;
> +
> +			/* Over */
> +			master = list_entry(master->lessee_list.next, struct drm_master, lessee_list);
> +		}
> +	}
> +}

<snip />

> -- 
> 2.15.0.rc0
> 

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS