[PATCH 0/7] some cleanups and uapi clarification for leases

[PATCH 0/7] some cleanups and uapi clarification for leases

[Daniel Vetter]

Hi all,

Nothing too major, only things I did find in all my igt test extending for
drm lease is some corner cases around implicit planes and atomic target
crtcs. Review and comments very much appreciated.

Cheers, Daniel

Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch

Daniel Vetter (7):
  drm/leases: Drop object_id validation for negative ids
  drm/lease: Drop recursive leads checks
  drm/leases: Don't init to 0 in drm_master_create
  drm/lease: Check for lessor outside of locks
  drm/lease: Make sure implicit planes are leased
  drm/atomic: Wire file_priv through for property changes
  drm/atomic: -EACCESS for lease-denied crtc lookup

 drivers/gpu/drm/drm_atomic_uapi.c   | 36 +++++++++++++++++++++++-------------
 drivers/gpu/drm/drm_auth.c          |  2 --
 drivers/gpu/drm/drm_crtc.c          |  4 ++++
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_lease.c         | 13 +++----------
 drivers/gpu/drm/drm_mode_object.c   |  5 +++--
 drivers/gpu/drm/drm_plane.c         |  8 ++++++++
 7 files changed, 42 insertions(+), 27 deletions(-)

-- 
2.14.4

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

[PATCH 1/7] drm/leases: Drop object_id validation for negative ids

[Daniel Vetter]

Not exactly sure what's the aim here, but the canonical nil object has
id == 0, we don't use negative object ids for anything. Plus all
object_id are valided by the object_idr, there's nothing we need to do
on top of that ENOENT check a bit further down.

Spotted while typing exhaustive igt coverage for all these
corner-cases.

Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_lease.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 603b0bd9c5ce..1176d814cf7f 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev,
 	/* step one - get references to all the mode objects
 	   and check for validity. */
 	for (o = 0; o < object_count; o++) {
-		if ((int) object_ids[o] < 0) {
-			ret = -EINVAL;
-			goto out_free_objects;
-		}
-
 		objects[o] = drm_mode_object_find(dev, lessor_priv,
 						  object_ids[o],
 						  DRM_MODE_OBJECT_ANY);
-- 
2.14.4

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

[PATCH 2/7] drm/lease: Drop recursive leads checks

[Daniel Vetter]

We disallow subleasing, so no point checking whether the master holds
all the leases - it will.

Spotted while typing exhaustive igt coverage for all these corner
cases.

Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_lease.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 1176d814cf7f..cce5d9dd52ff 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -220,8 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
 		error = 0;
 		if (!idr_find(&dev->mode_config.object_idr, object))
 			error = -ENOENT;
-		else if (!_drm_lease_held_master(lessor, object))
-			error = -EACCES;
 		else if (_drm_has_leased(lessor, object))
 			error = -EBUSY;
 
-- 
2.14.4

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

[PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create

[Daniel Vetter]

We kzalloc.

Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_auth.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 1669c42c40ed..bcf0a5a1018f 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev)
 	master->dev = dev;
 
 	/* initialize the tree of output resource lessees */
-	master->lessor = NULL;
-	master->lessee_id = 0;
 	INIT_LIST_HEAD(&master->lessees);
 	INIT_LIST_HEAD(&master->lessee_list);
 	idr_init(&master->leases);
-- 
2.14.4

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

[PATCH 4/7] drm/lease: Check for lessor outside of locks

[Daniel Vetter]

The lessor is invariant over a lifetime of a lease, we don't have to
grab any locks for that. Speeds up the common case of not being a lease.

Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_lease.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index cce5d9dd52ff..694ff363a90b 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
  */
 bool _drm_lease_held(struct drm_file *file_priv, int id)
 {
-	if (file_priv == NULL || file_priv->master == NULL)
+	if (!file_priv || !file_priv->master)
 		return true;
 
 	return _drm_lease_held_master(file_priv->master, id);
@@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
 	struct drm_master *master;
 	bool ret;
 
-	if (file_priv == NULL || file_priv->master == NULL)
+	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
 		return true;
 
 	master = file_priv->master;
@@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
 	int count_in, count_out;
 	uint32_t crtcs_out = 0;
 
-	if (file_priv == NULL || file_priv->master == NULL)
+	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
 		return crtcs_in;
 
 	master = file_priv->master;
-- 
2.14.4

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

[PATCH 5/7] drm/lease: Make sure implicit planes are leased

[Daniel Vetter]

If userspace doesn't enable universal planes, then we automatically
add the primary and cursor planes. But for universal userspace there's
no such check (and maybe we only want to give the lessee one plane,
maybe not even the primary one), hence we need to check for the
implied plane.

v2: don't forget setcrtc ioctl.

v3: Still allow disabling of the crtc in SETCRTC.

Cc: stable@vger.kernel.org
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 4 ++++
 drivers/gpu/drm/drm_plane.c | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7dabbaf033a1..790ba5941954 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -559,6 +559,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 	plane = crtc->primary;
 
+	/* allow disabling with the primary plane leased */
+	if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id))
+		return -EACCES;
+
 	mutex_lock(&crtc->dev->mode_config.mutex);
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
 				   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 4cfb56893b7f..d6ad60ab0d38 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -960,6 +960,11 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		if (ret)
 			goto out;
 
+		if (!drm_lease_held(file_priv, crtc->cursor->base.id)) {
+			ret = -EACCES;
+			goto out;
+		}
+
 		ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx);
 		goto out;
 	}
@@ -1062,6 +1067,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 	plane = crtc->primary;
 
+	if (!drm_lease_held(file_priv, plane->base.id))
+		return -EACCES;
+
 	if (crtc->funcs->page_flip_target) {
 		u32 current_vblank;
 		int r;
-- 
2.14.4

[PATCH 6/7] drm/atomic: Wire file_priv through for property changes

[Daniel Vetter]

We need this to make sure lessees can only connect their
plane/connectors to crtc objects they own. And note that this is
irrespective of whether the lessor is atomic or not, lessor cannot
prevent lessees from enabling atomic.

Cc: stable@vger.kernel.org
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c   | 32 +++++++++++++++++++-------------
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_mode_object.c   |  5 +++--
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 4eb81f10bc54..f0dbfeb00926 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -512,8 +512,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 }
 
 static int drm_atomic_plane_set_property(struct drm_plane *plane,
-		struct drm_plane_state *state, struct drm_property *property,
-		uint64_t val)
+		struct drm_plane_state *state, struct drm_file *file_priv,
+		struct drm_property *property, uint64_t val)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_mode_config *config = &dev->mode_config;
@@ -521,7 +521,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 	int ret;
 
 	if (property == config->prop_fb_id) {
-		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
+		struct drm_framebuffer *fb;
+		fb = drm_framebuffer_lookup(dev, file_priv, val);
 		drm_atomic_set_fb_for_plane(state, fb);
 		if (fb)
 			drm_framebuffer_put(fb);
@@ -537,7 +538,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 			return -EINVAL;
 
 	} else if (property == config->prop_crtc_id) {
-		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val);
+		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
 		return drm_atomic_set_crtc_for_plane(state, crtc);
 	} else if (property == config->prop_crtc_x) {
 		state->crtc_x = U642I64(val);
@@ -681,14 +682,14 @@ static int drm_atomic_set_writeback_fb_for_connector(
 }
 
 static int drm_atomic_connector_set_property(struct drm_connector *connector,
-		struct drm_connector_state *state, struct drm_property *property,
-		uint64_t val)
+		struct drm_connector_state *state, struct drm_file *file_priv,
+		struct drm_property *property, uint64_t val)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 
 	if (property == config->prop_crtc_id) {
-		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val);
+		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
 		return drm_atomic_set_crtc_for_connector(state, crtc);
 	} else if (property == config->dpms_property) {
 		/* setting DPMS property requires special handling, which
@@ -749,8 +750,10 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 	} else if (property == connector->colorspace_property) {
 		state->colorspace = val;
 	} else if (property == config->writeback_fb_id_property) {
-		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
-		int ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
+		struct drm_framebuffer *fb;
+		int ret;
+		fb = drm_framebuffer_lookup(dev, file_priv, val);
+		ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
 		if (fb)
 			drm_framebuffer_put(fb);
 		return ret;
@@ -947,6 +950,7 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 }
 
 int drm_atomic_set_property(struct drm_atomic_state *state,
+			    struct drm_file *file_priv,
 			    struct drm_mode_object *obj,
 			    struct drm_property *prop,
 			    uint64_t prop_value)
@@ -969,7 +973,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		}
 
 		ret = drm_atomic_connector_set_property(connector,
-				connector_state, prop, prop_value);
+				connector_state, file_priv,
+				prop, prop_value);
 		break;
 	}
 	case DRM_MODE_OBJECT_CRTC: {
@@ -997,7 +1002,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		}
 
 		ret = drm_atomic_plane_set_property(plane,
-				plane_state, prop, prop_value);
+				plane_state, file_priv,
+				prop, prop_value);
 		break;
 	}
 	default:
@@ -1369,8 +1375,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 				goto out;
 			}
 
-			ret = drm_atomic_set_property(state, obj, prop,
-						      prop_value);
+			ret = drm_atomic_set_property(state, file_priv,
+						      obj, prop, prop_value);
 			if (ret) {
 				drm_mode_object_put(obj);
 				goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 216f2a9ee3d4..0719a235d6cc 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -214,6 +214,7 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 				     struct drm_connector *connector,
 				     int mode);
 int drm_atomic_set_property(struct drm_atomic_state *state,
+			    struct drm_file *file_priv,
 			    struct drm_mode_object *obj,
 			    struct drm_property *prop,
 			    uint64_t prop_value);
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index a9005c1c2384..f32507e65b79 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -451,6 +451,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
 }
 
 static int set_property_atomic(struct drm_mode_object *obj,
+			       struct drm_file *file_priv,
 			       struct drm_property *prop,
 			       uint64_t prop_value)
 {
@@ -477,7 +478,7 @@ static int set_property_atomic(struct drm_mode_object *obj,
 						       obj_to_connector(obj),
 						       prop_value);
 	} else {
-		ret = drm_atomic_set_property(state, obj, prop, prop_value);
+		ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
 		if (ret)
 			goto out;
 		ret = drm_atomic_commit(state);
@@ -520,7 +521,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		goto out_unref;
 
 	if (drm_drv_uses_atomic_modeset(property->dev))
-		ret = set_property_atomic(arg_obj, property, arg->value);
+		ret = set_property_atomic(arg_obj, file_priv, property, arg->value);
 	else
 		ret = set_property_legacy(arg_obj, property, arg->value);
 
-- 
2.14.4

[PATCH 7/7] drm/atomic: -EACCESS for lease-denied crtc lookup

[Daniel Vetter]

With the previous patch drm_crtc_find will return NULL when the crtc
isn't in our lease, which will then disable the plane/connector. No
longer an issue since the lessor can't escape their lease terms
anymore, but not quite great semantics yet either.

Catch this and return -EACCES, so that at least evil test cases have a
better chance of making sure the kernel works correctly.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index f0dbfeb00926..6687215cc188 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -539,6 +539,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 
 	} else if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+		if (val && !crtc)
+			return -EACCES;
 		return drm_atomic_set_crtc_for_plane(state, crtc);
 	} else if (property == config->prop_crtc_x) {
 		state->crtc_x = U642I64(val);
@@ -690,6 +692,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 
 	if (property == config->prop_crtc_id) {
 		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+		if (val && !crtc)
+			return -EACCES;
 		return drm_atomic_set_crtc_for_connector(state, crtc);
 	} else if (property == config->dpms_property) {
 		/* setting DPMS property requires special handling, which
-- 
2.14.4

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

Re: [PATCH 5/7] drm/lease: Make sure implicit planes are leased

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v4.20.13, v4.19.26, v4.14.104, v4.9.161, v4.4.176, v3.18.136.

v4.20.13: Build OK!
v4.19.26: Build OK!
v4.14.104: Failed to apply! Possible dependencies:
    23163a7d4b03 ("drm: Check that the plane supports the request format+modifier combo")
    64c32b490333 ("drm: Add local 'plane' variable for primary/cursor planes")
    ce0769e0ea4b ("drm/plane: Make framebuffer refcounting the responsibility of setplane_internal callers")

v4.9.161: Failed to apply! Possible dependencies:
    13b55664eec7 ("drm/atomic: add drm_atomic_set_fence_for_plane()")
    1cec20f0ea0e ("dma-buf: Restart reservation_object_wait_timeout_rcu() after writes")
    2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl")
    2ceb585a956c ("drm: Add explicit acquire ctx handling around ->set_config")
    53552d5df669 ("drm: Take mode_config.mutex in setcrtc ioctl")
    64c32b490333 ("drm: Add local 'plane' variable for primary/cursor planes")
    78010cd9736e ("dma-buf/fence: add an lockdep_assert_held()")
    d49473a53aec ("drm: Restrict drm_mode_set_config_internal to non-atomic drivers")
    d574528a64c3 ("drm/kms-core: Use recommened kerneldoc for struct member refs")
    de7b6be7f300 ("drm: Use atomic state for FB in legacy ioctls")
    f54d1867005c ("dma-buf: Rename struct fence to dma_fence")
    fedf54132d24 ("dma-buf: Restart reservation_object_get_fences_rcu() after writes")

v4.4.176: Failed to apply! Possible dependencies:
    1d657c58dd16 ("drm/etnaviv: Use lockless gem BO free callback")
    22554020409f ("Documentation/gpu: use recommended order of heading markers")
    22cba31bae9d ("Documentation/sphinx: add basic working Sphinx configuration and build")
    23e7b2ab9a8f ("drm/hisilicon: Add hisilicon kirin drm master driver")
    2fa91d15588c ("Documentation/gpu: split up mm, kms and kms-helpers from internals")
    311b62d94c0b ("drm/doc: Reorg for drm-kms.rst")
    321a95ae35f2 ("drm: Extract drm_encoder.[hc]")
    34a839c689b0 ("drm: Link directly from drm_master to drm_device")
    3b96a0b1407e ("drm: document drm_auth.c")
    40647e45b92b ("drm: Hide master MAP cleanup in drm_bufs.c")
    43968d7b806d ("drm: Extract drm_plane.[hc]")
    4b193663dbc9 ("drm/imx: Use lockless gem BO free callback")
    51dacf208988 ("drm: Add support of ARC PGU display controller")
    522171951761 ("drm: Extract drm_connector.[hc]")
    5443ce86fa37 ("drm: virtio-gpu: set atomic flag")
    64c32b490333 ("drm: Add local 'plane' variable for primary/cursor planes")
    6548f4e7a3ba ("drm: Move master functions into drm_auth.c")
    7520a277d97b ("drm: Extract drm_framebuffer.[hc]")
    9b20fa08d3fd ("Documentation/gpu: convert the KMS properties table to CSV")
    a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci drivers")
    a8c21a5451d8 ("drm/etnaviv: add initial etnaviv DRM driver")
    ca00c2b986ea ("Documentation/gpu: split up the gpu documentation")
    cb597fcea5c2 ("Documentation/gpu: add new gpu.rst converted from DocBook gpu.tmpl")
    d6ed682eba54 ("drm: Refactor drop/set master code a bit")
    de7b6be7f300 ("drm: Use atomic state for FB in legacy ioctls")

v3.18.136: Failed to apply! Possible dependencies:
    22554020409f ("Documentation/gpu: use recommended order of heading markers")
    22cba31bae9d ("Documentation/sphinx: add basic working Sphinx configuration and build")
    2fa91d15588c ("Documentation/gpu: split up mm, kms and kms-helpers from internals")
    311b62d94c0b ("drm/doc: Reorg for drm-kms.rst")
    321a95ae35f2 ("drm: Extract drm_encoder.[hc]")
    43968d7b806d ("drm: Extract drm_plane.[hc]")
    522171951761 ("drm: Extract drm_connector.[hc]")
    5699f871d2d5 ("scripts/kernel-doc: Adding cross-reference links to html documentation.")
    64c32b490333 ("drm: Add local 'plane' variable for primary/cursor planes")
    9b20fa08d3fd ("Documentation/gpu: convert the KMS properties table to CSV")
    b479bfd00e46 ("DocBook: Use a fixed encoding for output")
    ca00c2b986ea ("Documentation/gpu: split up the gpu documentation")
    cb597fcea5c2 ("Documentation/gpu: add new gpu.rst converted from DocBook gpu.tmpl")
    de7b6be7f300 ("drm: Use atomic state for FB in legacy ioctls")


How should we proceed with this patch?

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

Re: [PATCH 1/7] drm/leases: Drop object_id validation for negative ids

[Boris Brezillon]

On Thu, 28 Feb 2019 15:49:04 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Not exactly sure what's the aim here, but the canonical nil object has
> id == 0, we don't use negative object ids for anything. Plus all
> object_id are valided by the object_idr, there's nothing we need to do
> on top of that ENOENT check a bit further down.
> 
> Spotted while typing exhaustive igt coverage for all these
> corner-cases.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/drm_lease.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 603b0bd9c5ce..1176d814cf7f 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev,
>  	/* step one - get references to all the mode objects
>  	   and check for validity. */
>  	for (o = 0; o < object_count; o++) {
> -		if ((int) object_ids[o] < 0) {
> -			ret = -EINVAL;
> -			goto out_free_objects;
> -		}
> -
>  		objects[o] = drm_mode_object_find(dev, lessor_priv,
>  						  object_ids[o],
>  						  DRM_MODE_OBJECT_ANY);

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

Re: [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create

[Boris Brezillon]

On Thu, 28 Feb 2019 15:49:06 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We kzalloc.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/drm_auth.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1669c42c40ed..bcf0a5a1018f 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev)
>  	master->dev = dev;
>  
>  	/* initialize the tree of output resource lessees */
> -	master->lessor = NULL;
> -	master->lessee_id = 0;
>  	INIT_LIST_HEAD(&master->lessees);
>  	INIT_LIST_HEAD(&master->lessee_list);
>  	idr_init(&master->leases);

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

Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks

[Boris Brezillon]

On Thu, 28 Feb 2019 15:49:07 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The lessor is invariant over a lifetime of a lease, we don't have to
> grab any locks for that. Speeds up the common case of not being a lease.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_lease.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index cce5d9dd52ff..694ff363a90b 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>   */
>  bool _drm_lease_held(struct drm_file *file_priv, int id)
>  {
> -	if (file_priv == NULL || file_priv->master == NULL)
> +	if (!file_priv || !file_priv->master)

Looks like you're doing unrelated cosmetic changes in the same patch.
Maybe mention that in the commit message, or move that to a separate
patch.

>  		return true;
>  
>  	return _drm_lease_held_master(file_priv->master, id);
> @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>  	struct drm_master *master;
>  	bool ret;
>  
> -	if (file_priv == NULL || file_priv->master == NULL)
> +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>  		return true;
>  
>  	master = file_priv->master;
> @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>  	int count_in, count_out;
>  	uint32_t crtcs_out = 0;
>  
> -	if (file_priv == NULL || file_priv->master == NULL)
> +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
>  		return crtcs_in;
>  
>  	master = file_priv->master;

Couldn't we also remove the if (master->lessor) check done in
_drm_lease_held_master()?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/lease: Drop recursive leads checks

[Boris Brezillon]

On Thu, 28 Feb 2019 15:49:05 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We disallow subleasing, so no point checking whether the master holds
> all the leases - it will.
> 
> Spotted while typing exhaustive igt coverage for all these corner
> cases.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/drm_lease.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 1176d814cf7f..cce5d9dd52ff 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -220,8 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>  		error = 0;
>  		if (!idr_find(&dev->mode_config.object_idr, object))
>  			error = -ENOENT;
> -		else if (!_drm_lease_held_master(lessor, object))
> -			error = -EACCES;
>  		else if (_drm_has_leased(lessor, object))
>  			error = -EBUSY;
>  

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

Re: [PATCH 0/7] some cleanups and uapi clarification for leases

[Boris Brezillon]

On Thu, 28 Feb 2019 15:49:03 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Hi all,
> 
> Nothing too major, only things I did find in all my igt test extending for
> drm lease is some corner cases around implicit planes and atomic target
> crtcs. Review and comments very much appreciated.
> 
> Cheers, Daniel
> 
> Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch
> 
> Daniel Vetter (7):
>   drm/leases: Drop object_id validation for negative ids
>   drm/lease: Drop recursive leads checks
>   drm/leases: Don't init to 0 in drm_master_create
>   drm/lease: Check for lessor outside of locks
>   drm/lease: Make sure implicit planes are leased
>   drm/atomic: Wire file_priv through for property changes
>   drm/atomic: -EACCESS for lease-denied crtc lookup

I had a look at patches 5-7 and they seem to do the right thing, but my
knowledge on DRM leases is quite limited, and given all the corner
cases of legacy CRTC/plane updates, I'm not comfortable adding my R-b
on these.

> 
>  drivers/gpu/drm/drm_atomic_uapi.c   | 36 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/drm_auth.c          |  2 --
>  drivers/gpu/drm/drm_crtc.c          |  4 ++++
>  drivers/gpu/drm/drm_crtc_internal.h |  1 +
>  drivers/gpu/drm/drm_lease.c         | 13 +++----------
>  drivers/gpu/drm/drm_mode_object.c   |  5 +++--
>  drivers/gpu/drm/drm_plane.c         |  8 ++++++++
>  7 files changed, 42 insertions(+), 27 deletions(-)
> 

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

Re: [PATCH 1/7] drm/leases: Drop object_id validation for negative ids

[Dave Airlie]

commit 2e1c9b2867656ff9a469d23e1dfe90cf77ec0c72
Author: Tejun Heo <tj@kernel.org>
Date:   Fri Mar 8 12:43:30 2013 -0800

    idr: remove WARN_ON_ONCE() on negative IDs

We used to WARN_ON if we hit a negative id, it appears we don't
anymore, so just update the commit msg to reflect that info on where
the code came from originally.

You had me wondering if I'd been dreaming up reasons for Keith to add code :-P

Dave.

On Thu, 14 Mar 2019 at 17:54, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Thu, 28 Feb 2019 15:49:04 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > Not exactly sure what's the aim here, but the canonical nil object has
> > id == 0, we don't use negative object ids for anything. Plus all
> > object_id are valided by the object_idr, there's nothing we need to do
> > on top of that ENOENT check a bit further down.
> >
> > Spotted while typing exhaustive igt coverage for all these
> > corner-cases.
> >
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
>
> > ---
> >  drivers/gpu/drm/drm_lease.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > index 603b0bd9c5ce..1176d814cf7f 100644
> > --- a/drivers/gpu/drm/drm_lease.c
> > +++ b/drivers/gpu/drm/drm_lease.c
> > @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev,
> >       /* step one - get references to all the mode objects
> >          and check for validity. */
> >       for (o = 0; o < object_count; o++) {
> > -             if ((int) object_ids[o] < 0) {
> > -                     ret = -EINVAL;
> > -                     goto out_free_objects;
> > -             }
> > -
> >               objects[o] = drm_mode_object_find(dev, lessor_priv,
> >                                                 object_ids[o],
> >                                                 DRM_MODE_OBJECT_ANY);
>
> _______________________________________________
> 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 3/7] drm/leases: Don't init to 0 in drm_master_create

[Dave Airlie]

On Thu, 14 Mar 2019 at 17:56, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Thu, 28 Feb 2019 15:49:06 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > We kzalloc.
> >
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> > ---
> >  drivers/gpu/drm/drm_auth.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > index 1669c42c40ed..bcf0a5a1018f 100644
> > --- a/drivers/gpu/drm/drm_auth.c
> > +++ b/drivers/gpu/drm/drm_auth.c
> > @@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev)
> >       master->dev = dev;
> >
> >       /* initialize the tree of output resource lessees */
> > -     master->lessor = NULL;
> > -     master->lessee_id = 0;
> >       INIT_LIST_HEAD(&master->lessees);
> >       INIT_LIST_HEAD(&master->lessee_list);
> >       idr_init(&master->leases);
>
> _______________________________________________
> 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/7] drm/leases: Drop object_id validation for negative ids

[Daniel Vetter]

On Fri, Mar 29, 2019 at 02:46:55PM +1000, Dave Airlie wrote:
> commit 2e1c9b2867656ff9a469d23e1dfe90cf77ec0c72
> Author: Tejun Heo <tj@kernel.org>
> Date:   Fri Mar 8 12:43:30 2013 -0800
> 
>     idr: remove WARN_ON_ONCE() on negative IDs
> 
> We used to WARN_ON if we hit a negative id, it appears we don't
> anymore, so just update the commit msg to reflect that info on where
> the code came from originally.
> 
> You had me wondering if I'd been dreaming up reasons for Keith to add code :-P

Hm right, looking around I think another relevant commit is

commit 322d884ba731e05ca79ae58e9dee1ef7dc4de504
Author: Matthew Wilcox <mawilcox@microsoft.com>
Date:   Tue Nov 28 10:01:24 2017 -0500

    idr: Delete idr_find_ext function
    
    Simply changing idr_remove's 'id' argument to 'unsigned long' works
    for all callers.
    
    Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

I don't remember any such checks for the kms or gem idr lookups, so still
no idea why Keith typed this. The warning is gone since 5+ years after all
(and drm lease isn't that old).
-Daniel

> 
> Dave.
> 
> On Thu, 14 Mar 2019 at 17:54, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Thu, 28 Feb 2019 15:49:04 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > > Not exactly sure what's the aim here, but the canonical nil object has
> > > id == 0, we don't use negative object ids for anything. Plus all
> > > object_id are valided by the object_idr, there's nothing we need to do
> > > on top of that ENOENT check a bit further down.
> > >
> > > Spotted while typing exhaustive igt coverage for all these
> > > corner-cases.
> > >
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > > ---
> > >  drivers/gpu/drm/drm_lease.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index 603b0bd9c5ce..1176d814cf7f 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev,
> > >       /* step one - get references to all the mode objects
> > >          and check for validity. */
> > >       for (o = 0; o < object_count; o++) {
> > > -             if ((int) object_ids[o] < 0) {
> > > -                     ret = -EINVAL;
> > > -                     goto out_free_objects;
> > > -             }
> > > -
> > >               objects[o] = drm_mode_object_find(dev, lessor_priv,
> > >                                                 object_ids[o],
> > >                                                 DRM_MODE_OBJECT_ANY);
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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/7] drm/lease: Check for lessor outside of locks

[Dave Airlie]

On Thu, 14 Mar 2019 at 18:07, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Thu, 28 Feb 2019 15:49:07 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > The lessor is invariant over a lifetime of a lease, we don't have to
> > grab any locks for that. Speeds up the common case of not being a lease.
> >
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_lease.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > index cce5d9dd52ff..694ff363a90b 100644
> > --- a/drivers/gpu/drm/drm_lease.c
> > +++ b/drivers/gpu/drm/drm_lease.c
> > @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> >   */
> >  bool _drm_lease_held(struct drm_file *file_priv, int id)
> >  {
> > -     if (file_priv == NULL || file_priv->master == NULL)
> > +     if (!file_priv || !file_priv->master)
>
> Looks like you're doing unrelated cosmetic changes in the same patch.
> Maybe mention that in the commit message, or move that to a separate
> patch.
>
> >               return true;
> >
> >       return _drm_lease_held_master(file_priv->master, id);
> > @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> >       struct drm_master *master;
> >       bool ret;
> >
> > -     if (file_priv == NULL || file_priv->master == NULL)
> > +     if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >               return true;
> >
> >       master = file_priv->master;
> > @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> >       int count_in, count_out;
> >       uint32_t crtcs_out = 0;
> >
> > -     if (file_priv == NULL || file_priv->master == NULL)
> > +     if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >               return crtcs_in;
> >
> >       master = file_priv->master;
>
> Couldn't we also remove the if (master->lessor) check done in
> _drm_lease_held_master()?

Daniel, answers for these? ^^

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

Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks

[Daniel Vetter]

On Thu, Mar 14, 2019 at 09:07:25AM +0100, Boris Brezillon wrote:
> On Thu, 28 Feb 2019 15:49:07 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > The lessor is invariant over a lifetime of a lease, we don't have to
> > grab any locks for that. Speeds up the common case of not being a lease.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_lease.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > index cce5d9dd52ff..694ff363a90b 100644
> > --- a/drivers/gpu/drm/drm_lease.c
> > +++ b/drivers/gpu/drm/drm_lease.c
> > @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> >   */
> >  bool _drm_lease_held(struct drm_file *file_priv, int id)
> >  {
> > -	if (file_priv == NULL || file_priv->master == NULL)
> > +	if (!file_priv || !file_priv->master)
> 
> Looks like you're doing unrelated cosmetic changes in the same patch.
> Maybe mention that in the commit message, or move that to a separate
> patch.

The next hunk would have broken the 80 limit with the == NULL check, so I
changed those for consistency too because ocd. I can mention that in the
commit message.
> 
> >  		return true;
> >  
> >  	return _drm_lease_held_master(file_priv->master, id);
> > @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> >  	struct drm_master *master;
> >  	bool ret;
> >  
> > -	if (file_priv == NULL || file_priv->master == NULL)
> > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >  		return true;
> >  
> >  	master = file_priv->master;
> > @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> >  	int count_in, count_out;
> >  	uint32_t crtcs_out = 0;
> >  
> > -	if (file_priv == NULL || file_priv->master == NULL)
> > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> >  		return crtcs_in;
> >  
> >  	master = file_priv->master;
> 
> Couldn't we also remove the if (master->lessor) check done in
> _drm_lease_held_master()?

How? For !master->lessor (which is the case for all top-level masters,
i.e. one created by opening the /dev node and not through the create lease
ioctl) there's no lease idr. These are handled by the unconditional return
true. And there's some call chains leading to this which don't first check
for master->lessor (the object find stuff through _drm_lease_held).

Also confused by "also remove", this patch doesn't drop any checks, just
adds them outside of the lock to extend existing fastpaths.
-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/7] drm/lease: Check for lessor outside of locks

[Boris Brezillon]

On Wed, 3 Apr 2019 09:04:03 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Mar 14, 2019 at 09:07:25AM +0100, Boris Brezillon wrote:
> > On Thu, 28 Feb 2019 15:49:07 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >   
> > > The lessor is invariant over a lifetime of a lease, we don't have to
> > > grab any locks for that. Speeds up the common case of not being a lease.
> > > 
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_lease.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index cce5d9dd52ff..694ff363a90b 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> > >   */
> > >  bool _drm_lease_held(struct drm_file *file_priv, int id)
> > >  {
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master)  
> > 
> > Looks like you're doing unrelated cosmetic changes in the same patch.
> > Maybe mention that in the commit message, or move that to a separate
> > patch.  
> 
> The next hunk would have broken the 80 limit with the == NULL check, so I
> changed those for consistency too because ocd. I can mention that in the
> commit message.

Okay.

> >   
> > >  		return true;
> > >  
> > >  	return _drm_lease_held_master(file_priv->master, id);
> > > @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> > >  	struct drm_master *master;
> > >  	bool ret;
> > >  
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > >  		return true;
> > >  
> > >  	master = file_priv->master;
> > > @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> > >  	int count_in, count_out;
> > >  	uint32_t crtcs_out = 0;
> > >  
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > >  		return crtcs_in;
> > >  
> > >  	master = file_priv->master;  
> > 
> > Couldn't we also remove the if (master->lessor) check done in
> > _drm_lease_held_master()?  
> 
> How? For !master->lessor (which is the case for all top-level masters,
> i.e. one created by opening the /dev node and not through the create lease
> ioctl) there's no lease idr. These are handled by the unconditional return
> true. And there's some call chains leading to this which don't first check
> for master->lessor (the object find stuff through _drm_lease_held).

I had the impression that all callers of _drm_lease_held_master() were
now doing the necessary checks to guarantee that master->lessor == NULL
cannot happen in _drm_lease_held_master(). But it seems that
_drm_lease_held() and drm_lease_create() do not check the value of
master->lessor.

> 
> Also confused by "also remove", this patch doesn't drop any checks, just
> adds them outside of the lock to extend existing fastpaths.

Yes, I meant "remove" not "also remove". Sorry for the confusion.

FWIW, here is my

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] some cleanups and uapi clarification for leases

[Dave Airlie]

On Thu, 14 Mar 2019 at 18:58, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Thu, 28 Feb 2019 15:49:03 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> > Hi all,
> >
> > Nothing too major, only things I did find in all my igt test extending for
> > drm lease is some corner cases around implicit planes and atomic target
> > crtcs. Review and comments very much appreciated.
> >
> > Cheers, Daniel
> >
> > Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch
> >
> > Daniel Vetter (7):
> >   drm/leases: Drop object_id validation for negative ids
> >   drm/lease: Drop recursive leads checks
> >   drm/leases: Don't init to 0 in drm_master_create
> >   drm/lease: Check for lessor outside of locks
> >   drm/lease: Make sure implicit planes are leased
> >   drm/atomic: Wire file_priv through for property changes
> >   drm/atomic: -EACCESS for lease-denied crtc lookup
>
> I had a look at patches 5-7 and they seem to do the right thing, but my
> knowledge on DRM leases is quite limited, and given all the corner
> cases of legacy CRTC/plane updates, I'm not comfortable adding my R-b
> on these.

For the series:

Reviewed-by: Dave Airlie <airlied@redhat.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] some cleanups and uapi clarification for leases

[Daniel Vetter]

On Fri, Apr 05, 2019 at 12:40:14PM +1000, Dave Airlie wrote:
> On Thu, 14 Mar 2019 at 18:58, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Thu, 28 Feb 2019 15:49:03 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > > Hi all,
> > >
> > > Nothing too major, only things I did find in all my igt test extending for
> > > drm lease is some corner cases around implicit planes and atomic target
> > > crtcs. Review and comments very much appreciated.
> > >
> > > Cheers, Daniel
> > >
> > > Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch
> > >
> > > Daniel Vetter (7):
> > >   drm/leases: Drop object_id validation for negative ids
> > >   drm/lease: Drop recursive leads checks
> > >   drm/leases: Don't init to 0 in drm_master_create
> > >   drm/lease: Check for lessor outside of locks
> > >   drm/lease: Make sure implicit planes are leased
> > >   drm/atomic: Wire file_priv through for property changes
> > >   drm/atomic: -EACCESS for lease-denied crtc lookup
> >
> > I had a look at patches 5-7 and they seem to do the right thing, but my
> > knowledge on DRM leases is quite limited, and given all the corner
> > cases of legacy CRTC/plane updates, I'm not comfortable adding my R-b
> > on these.
> 
> For the series:
> 
> Reviewed-by: Dave Airlie <airlied@redhat.com>

Finally gotten around to pulling it all in, thanks for all your reviews.
-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

[PATCH 0/7] some cleanups and uapi clarification for leases

[Daniel Vetter]

[resend with the right mailing lists]

Hi all,

Nothing too major, only things I did find in all my igt test extending for
drm lease is some corner cases around implicit planes and atomic target
crtcs. Review and comments very much appreciated.

Cheers, Daniel

Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch

Daniel Vetter (7):
  drm/leases: Drop object_id validation for negative ids
  drm/lease: Drop recursive leads checks
  drm/leases: Don't init to 0 in drm_master_create
  drm/lease: Check for lessor outside of locks
  drm/lease: Make sure implicit planes are leased
  drm/atomic: Wire file_priv through for property changes
  drm/atomic: -EACCESS for lease-denied crtc lookup

 drivers/gpu/drm/drm_atomic_uapi.c   | 36 +++++++++++++++++++++++-------------
 drivers/gpu/drm/drm_auth.c          |  2 --
 drivers/gpu/drm/drm_crtc.c          |  4 ++++
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_lease.c         | 13 +++----------
 drivers/gpu/drm/drm_mode_object.c   |  5 +++--
 drivers/gpu/drm/drm_plane.c         |  8 ++++++++
 7 files changed, 42 insertions(+), 27 deletions(-)

-- 
2.14.4

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