* [PATCH 1/7] drm/leases: Drop object_id validation for negative ids
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-14 7:54 ` Boris Brezillon
2019-02-28 14:49 ` [PATCH 2/7] drm/lease: Drop recursive leads checks Daniel Vetter
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development
Cc: IGT development, Daniel Vetter, Keith Packard, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/7] drm/leases: Drop object_id validation for negative ids
2019-02-28 14:49 ` [PATCH 1/7] drm/leases: Drop object_id validation for negative ids Daniel Vetter
@ 2019-03-14 7:54 ` Boris Brezillon
2019-03-29 4:46 ` Dave Airlie
0 siblings, 1 reply; 23+ messages in thread
From: Boris Brezillon @ 2019-03-14 7:54 UTC (permalink / raw)
To: Daniel Vetter
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/7] drm/leases: Drop object_id validation for negative ids
2019-03-14 7:54 ` Boris Brezillon
@ 2019-03-29 4:46 ` Dave Airlie
2019-03-29 8:28 ` Daniel Vetter
0 siblings, 1 reply; 23+ messages in thread
From: Dave Airlie @ 2019-03-29 4:46 UTC (permalink / raw)
To: Boris Brezillon
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development,
Daniel Vetter
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/7] drm/leases: Drop object_id validation for negative ids
2019-03-29 4:46 ` Dave Airlie
@ 2019-03-29 8:28 ` Daniel Vetter
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2019-03-29 8:28 UTC (permalink / raw)
To: Dave Airlie
Cc: Keith Packard, Daniel Vetter, DRI Development, IGT development,
Boris Brezillon, 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] drm/lease: Drop recursive leads checks
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
2019-02-28 14:49 ` [PATCH 1/7] drm/leases: Drop object_id validation for negative ids Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-14 8:44 ` Boris Brezillon
2019-02-28 14:49 ` [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create Daniel Vetter
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development
Cc: IGT development, Daniel Vetter, Keith Packard, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/7] drm/lease: Drop recursive leads checks
2019-02-28 14:49 ` [PATCH 2/7] drm/lease: Drop recursive leads checks Daniel Vetter
@ 2019-03-14 8:44 ` Boris Brezillon
0 siblings, 0 replies; 23+ messages in thread
From: Boris Brezillon @ 2019-03-14 8:44 UTC (permalink / raw)
To: Daniel Vetter
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
2019-02-28 14:49 ` [PATCH 1/7] drm/leases: Drop object_id validation for negative ids Daniel Vetter
2019-02-28 14:49 ` [PATCH 2/7] drm/lease: Drop recursive leads checks Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-14 7:56 ` Boris Brezillon
2019-02-28 14:49 ` [PATCH 4/7] drm/lease: Check for lessor outside of locks Daniel Vetter
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development; +Cc: IGT development, Daniel Vetter, Keith Packard
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create
2019-02-28 14:49 ` [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create Daniel Vetter
@ 2019-03-14 7:56 ` Boris Brezillon
2019-03-29 4:47 ` Dave Airlie
0 siblings, 1 reply; 23+ messages in thread
From: Boris Brezillon @ 2019-03-14 7:56 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create
2019-03-14 7:56 ` Boris Brezillon
@ 2019-03-29 4:47 ` Dave Airlie
0 siblings, 0 replies; 23+ messages in thread
From: Dave Airlie @ 2019-03-29 4:47 UTC (permalink / raw)
To: Boris Brezillon
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/7] drm/lease: Check for lessor outside of locks
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
` (2 preceding siblings ...)
2019-02-28 14:49 ` [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-14 8:07 ` Boris Brezillon
2019-02-28 14:49 ` [PATCH 5/7] drm/lease: Make sure implicit planes are leased Daniel Vetter
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development; +Cc: IGT development, Daniel Vetter, Keith Packard
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks
2019-02-28 14:49 ` [PATCH 4/7] drm/lease: Check for lessor outside of locks Daniel Vetter
@ 2019-03-14 8:07 ` Boris Brezillon
2019-04-03 1:33 ` Dave Airlie
2019-04-03 7:04 ` Daniel Vetter
0 siblings, 2 replies; 23+ messages in thread
From: Boris Brezillon @ 2019-03-14 8:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks
2019-03-14 8:07 ` Boris Brezillon
@ 2019-04-03 1:33 ` Dave Airlie
2019-04-03 7:04 ` Daniel Vetter
1 sibling, 0 replies; 23+ messages in thread
From: Dave Airlie @ 2019-04-03 1:33 UTC (permalink / raw)
To: Boris Brezillon
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks
2019-03-14 8:07 ` Boris Brezillon
2019-04-03 1:33 ` Dave Airlie
@ 2019-04-03 7:04 ` Daniel Vetter
2019-04-03 7:50 ` Boris Brezillon
1 sibling, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-04-03 7:04 UTC (permalink / raw)
To: Boris Brezillon
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks
2019-04-03 7:04 ` Daniel Vetter
@ 2019-04-03 7:50 ` Boris Brezillon
0 siblings, 0 replies; 23+ messages in thread
From: Boris Brezillon @ 2019-04-03 7:50 UTC (permalink / raw)
To: Daniel Vetter
Cc: IGT development, Daniel Vetter, Keith Packard, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/7] drm/lease: Make sure implicit planes are leased
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
` (3 preceding siblings ...)
2019-02-28 14:49 ` [PATCH 4/7] drm/lease: Check for lessor outside of locks Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-05 13:35 ` Sasha Levin
2019-02-28 14:49 ` [PATCH 6/7] drm/atomic: Wire file_priv through for property changes Daniel Vetter
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development
Cc: IGT development, Daniel Vetter, stable, Keith Packard, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 5/7] drm/lease: Make sure implicit planes are leased
2019-02-28 14:49 ` [PATCH 5/7] drm/lease: Make sure implicit planes are leased Daniel Vetter
@ 2019-03-05 13:35 ` Sasha Levin
0 siblings, 0 replies; 23+ messages in thread
From: Sasha Levin @ 2019-03-05 13:35 UTC (permalink / raw)
To: Sasha Levin, Daniel Vetter, DRI Development
Cc: IGT development, Keith Packard, stable
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/7] drm/atomic: Wire file_priv through for property changes
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
` (4 preceding siblings ...)
2019-02-28 14:49 ` [PATCH 5/7] drm/lease: Make sure implicit planes are leased Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-02-28 14:49 ` [PATCH 7/7] drm/atomic: -EACCESS for lease-denied crtc lookup Daniel Vetter
2019-03-14 8:58 ` [PATCH 0/7] some cleanups and uapi clarification for leases Boris Brezillon
7 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development
Cc: IGT development, Daniel Vetter, stable, Keith Packard, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 7/7] drm/atomic: -EACCESS for lease-denied crtc lookup
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
` (5 preceding siblings ...)
2019-02-28 14:49 ` [PATCH 6/7] drm/atomic: Wire file_priv through for property changes Daniel Vetter
@ 2019-02-28 14:49 ` Daniel Vetter
2019-03-14 8:58 ` [PATCH 0/7] some cleanups and uapi clarification for leases Boris Brezillon
7 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2019-02-28 14:49 UTC (permalink / raw)
To: DRI Development; +Cc: IGT development, Daniel Vetter, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 0/7] some cleanups and uapi clarification for leases
2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter
` (6 preceding siblings ...)
2019-02-28 14:49 ` [PATCH 7/7] drm/atomic: -EACCESS for lease-denied crtc lookup Daniel Vetter
@ 2019-03-14 8:58 ` Boris Brezillon
2019-04-05 2:40 ` Dave Airlie
7 siblings, 1 reply; 23+ messages in thread
From: Boris Brezillon @ 2019-03-14 8:58 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/7] some cleanups and uapi clarification for leases
2019-03-14 8:58 ` [PATCH 0/7] some cleanups and uapi clarification for leases Boris Brezillon
@ 2019-04-05 2:40 ` Dave Airlie
2019-04-24 9:31 ` Daniel Vetter
0 siblings, 1 reply; 23+ messages in thread
From: Dave Airlie @ 2019-04-05 2:40 UTC (permalink / raw)
To: Boris Brezillon; +Cc: IGT development, Daniel Vetter, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/7] some cleanups and uapi clarification for leases
2019-04-05 2:40 ` Dave Airlie
@ 2019-04-24 9:31 ` Daniel Vetter
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2019-04-24 9:31 UTC (permalink / raw)
To: Dave Airlie
Cc: IGT development, Daniel Vetter, Boris Brezillon, DRI Development
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
^ permalink raw reply [flat|nested] 23+ messages in thread