Skip to contentSkip to conversation

drm: Rename crtc_idr as object_idr to KMS cleanups

PATCH Superseded Shayenne da Luz Moura 3 participants 7 messages Oct 31, 2018 – Nov 1, 2018

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drm: Rename crtc_idr as object_idr to KMS cleanups

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

---
 drivers/gpu/drm/drm_lease.c       | 6 +++---
 drivers/gpu/drm/drm_mode_config.c | 4 ++--
 drivers/gpu/drm/drm_mode_object.c | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)
drivers/gpu/drm/drm_lease.c +3 -3
index f4702f23c11d..4f8de2217049 100644
@@ -222,7 +222,7 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
 
 	idr_for_each_entry(leases, entry, object) {
 		error = 0;
-		if (!idr_find(&dev->mode_config.crtc_idr, object))
+		if (!idr_find(&dev->mode_config.object_idr, object))
 			error = -ENOENT;
 		else if (!_drm_lease_held_master(lessor, object))
 			error = -EACCES;
@@ -438,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
 		/*
 		 * We're using an IDR to hold the set of leased
 		 * objects, but we don't need to point at the object's
-		 * data structure from the lease as the main crtc_idr
+		 * data structure from the lease as the main object_idr
 		 * will be used to actually find that. Instead, all we
 		 * really want is a 'leased/not-leased' result, for
 		 * which any non-NULL pointer will work fine.
@@ -679,7 +679,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 
 	if (lessee->lessor == NULL)
 		/* owner can use all objects */
-		object_idr = &lessee->dev->mode_config.crtc_idr;
+		object_idr = &lessee->dev->mode_config.object_idr;
 	else
 		/* lessee can only use allowed object */
 		object_idr = &lessee->leases;
drivers/gpu/drm/drm_mode_config.c +2 -2
index ee80788f2c40..ab553b6465e2 100644
@@ -381,7 +381,7 @@ void drm_mode_config_init(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev->mode_config.property_list);
 	INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
 	INIT_LIST_HEAD(&dev->mode_config.plane_list);
-	idr_init(&dev->mode_config.crtc_idr);
+	idr_init(&dev->mode_config.object_idr);
 	idr_init(&dev->mode_config.tile_idr);
 	ida_init(&dev->mode_config.connector_ida);
 	spin_lock_init(&dev->mode_config.connector_list_lock);
@@ -484,7 +484,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 
 	ida_destroy(&dev->mode_config.connector_ida);
 	idr_destroy(&dev->mode_config.tile_idr);
-	idr_destroy(&dev->mode_config.crtc_idr);
+	idr_destroy(&dev->mode_config.object_idr);
 	drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
drivers/gpu/drm/drm_mode_object.c +4 -4
index cd9bc0ce9be0..bb1dd46496cd 100644
@@ -38,7 +38,7 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj,
 	int ret;
 
 	mutex_lock(&dev->mode_config.idr_mutex);
-	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL,
+	ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
 			1, 0, GFP_KERNEL);
 	if (ret >= 0) {
 		/*
@@ -79,7 +79,7 @@ void drm_mode_object_register(struct drm_device *dev,
 			      struct drm_mode_object *obj)
 {
 	mutex_lock(&dev->mode_config.idr_mutex);
-	idr_replace(&dev->mode_config.crtc_idr, obj, obj->id);
+	idr_replace(&dev->mode_config.object_idr, obj, obj->id);
 	mutex_unlock(&dev->mode_config.idr_mutex);
 }
 
@@ -99,7 +99,7 @@ void drm_mode_object_unregister(struct drm_device *dev,
 {
 	mutex_lock(&dev->mode_config.idr_mutex);
 	if (object->id) {
-		idr_remove(&dev->mode_config.crtc_idr, object->id);
+		idr_remove(&dev->mode_config.object_idr, object->id);
 		object->id = 0;
 	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
@@ -131,7 +131,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
 	struct drm_mode_object *obj = NULL;
 
 	mutex_lock(&dev->mode_config.idr_mutex);
-	obj = idr_find(&dev->mode_config.crtc_idr, id);
+	obj = idr_find(&dev->mode_config.object_idr, id);
 	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
 		obj = NULL;
 	if (obj && obj->id != id)
Signature · 1 line
2.19.1
#1 lore raw
Quoted 2 lines · Shayenne da Luz Moura

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Is object_idr a field that already exists? If so, "Rename" is not the best choice of words. It should be something like "use the object_idr field instead of the crtc_idr field" and then explain why. "task description in TODO list for KMS cleanups" isn't very helpful to understand why the change should be made.

julia

Quoted 104 lines
---
 drivers/gpu/drm/drm_lease.c       | 6 +++---
 drivers/gpu/drm/drm_mode_config.c | 4 ++--
 drivers/gpu/drm/drm_mode_object.c | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)
drivers/gpu/drm/drm_lease.c +3 -3
index f4702f23c11d..4f8de2217049 100644
@@ -222,7 +222,7 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
 
 	idr_for_each_entry(leases, entry, object) {
 		error = 0;
-		if (!idr_find(&dev->mode_config.crtc_idr, object))
+		if (!idr_find(&dev->mode_config.object_idr, object))
 			error = -ENOENT;
 		else if (!_drm_lease_held_master(lessor, object))
 			error = -EACCES;
@@ -438,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
 		/*
 		 * We're using an IDR to hold the set of leased
 		 * objects, but we don't need to point at the object's
-		 * data structure from the lease as the main crtc_idr
+		 * data structure from the lease as the main object_idr
 		 * will be used to actually find that. Instead, all we
 		 * really want is a 'leased/not-leased' result, for
 		 * which any non-NULL pointer will work fine.
@@ -679,7 +679,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 
 	if (lessee->lessor == NULL)
 		/* owner can use all objects */
-		object_idr = &lessee->dev->mode_config.crtc_idr;
+		object_idr = &lessee->dev->mode_config.object_idr;
 	else
 		/* lessee can only use allowed object */
 		object_idr = &lessee->leases;
drivers/gpu/drm/drm_mode_config.c +2 -2
index ee80788f2c40..ab553b6465e2 100644
@@ -381,7 +381,7 @@ void drm_mode_config_init(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev->mode_config.property_list);
 	INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
 	INIT_LIST_HEAD(&dev->mode_config.plane_list);
-	idr_init(&dev->mode_config.crtc_idr);
+	idr_init(&dev->mode_config.object_idr);
 	idr_init(&dev->mode_config.tile_idr);
 	ida_init(&dev->mode_config.connector_ida);
 	spin_lock_init(&dev->mode_config.connector_list_lock);
@@ -484,7 +484,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 
 	ida_destroy(&dev->mode_config.connector_ida);
 	idr_destroy(&dev->mode_config.tile_idr);
-	idr_destroy(&dev->mode_config.crtc_idr);
+	idr_destroy(&dev->mode_config.object_idr);
 	drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
drivers/gpu/drm/drm_mode_object.c +4 -5
index cd9bc0ce9be0..bb1dd46496cd 100644
@@ -38,7 +38,7 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj,
 	int ret;
 
 	mutex_lock(&dev->mode_config.idr_mutex);
-	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL,
+	ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
 			1, 0, GFP_KERNEL);
 	if (ret >= 0) {
 		/*
@@ -79,7 +79,7 @@ void drm_mode_object_register(struct drm_device *dev,
 			      struct drm_mode_object *obj)
 {
 	mutex_lock(&dev->mode_config.idr_mutex);
-	idr_replace(&dev->mode_config.crtc_idr, obj, obj->id);
+	idr_replace(&dev->mode_config.object_idr, obj, obj->id);
 	mutex_unlock(&dev->mode_config.idr_mutex);
 }
 
@@ -99,7 +99,7 @@ void drm_mode_object_unregister(struct drm_device *dev,
 {
 	mutex_lock(&dev->mode_config.idr_mutex);
 	if (object->id) {
-		idr_remove(&dev->mode_config.crtc_idr, object->id);
+		idr_remove(&dev->mode_config.object_idr, object->id);
 		object->id = 0;
 	}
 	mutex_unlock(&dev->mode_config.idr_mutex);
@@ -131,7 +131,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
 	struct drm_mode_object *obj = NULL;
 
 	mutex_lock(&dev->mode_config.idr_mutex);
-	obj = idr_find(&dev->mode_config.crtc_idr, id);
+	obj = idr_find(&dev->mode_config.object_idr, id);
 	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
 		obj = NULL;
 	if (obj && obj->id != id)
--

2.19.1

Quoted 7 lines

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181031205737.cingeaqget7hkbs6%40smtp.gmail.com.
For more options, visit https://groups.google.com/d/optout.

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#2 lore raw
Quoted 7 lines · Julia Lawall
Quoted 2 lines · Shayenne da Luz Moura

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Quoted 7 lines

Is object_idr a field that already exists? If so, "Rename" is not the best choice of words. It should be something like "use the object_idr field instead of the crtc_idr field" and then explain why. "task description in TODO list for KMS cleanups" isn't very helpful to understand why the change should be made.

julia

Hi Julia,

Thank you for your review!

This patch is to solve this TODO task:
drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should be renamed to drm_mode_config.object_idr.

Do you think I need to use this description in my commit message?

Best,
Shayenne

#3 lore raw
Quoted 18 lines · Shayenne Moura
Quoted 7 lines · Julia Lawall
Quoted 2 lines · Shayenne da Luz Moura

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Quoted 7 lines

Is object_idr a field that already exists? If so, "Rename" is not the best choice of words. It should be something like "use the object_idr field instead of the crtc_idr field" and then explain why. "task description in TODO list for KMS cleanups" isn't very helpful to understand why the change should be made.

julia

Hi Julia,

Quoted 7 lines

Thank you for your review!

This patch is to solve this TODO task:
drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should be renamed to drm_mode_config.object_idr.

Do you think I need to use this description in my commit message?

That seems more helpful.

But it seems that the name should actually be changed. Which means that the structure definition should be changed too. Was that done?

julia

Quoted 11 lines

Best,
Shayenne

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181031211935.p2pxr7fh26m5dc7v%40smtp.gmail.com.
For more options, visit https://groups.google.com/d/optout.

#6 lore raw
In reply to Shayenne da Luz Moura

Hi Shayenne,

Welcome to DRM.

As far as I can see you're a newcomer to kernel development, so I'd recommend watch a recent talk from Marc [1]
He provides a very good introduction, both for newbies and for people willing the know the deeper reasons behind.

That said, here's some suggestions:

Quoted 1 line · Shayenne da Luz Moura

I'd rename the title to "drm: rename drm_mode_config::crtc_idr to object_idr" The "... as ... to KMS cleanups" translation is very strange in English. It confused me and I've read the TODO over a dozen times ;-)

Quoted 3 lines

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Similarly here.

Quoted 7 lines
---
 drivers/gpu/drm/drm_lease.c       | 6 +++---
 drivers/gpu/drm/drm_mode_config.c | 4 ++--
 drivers/gpu/drm/drm_mode_object.c | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

As pointed out in the talk - always self review and ensure patches don't break things.
Here, DRM doesn't build which is obviously not correct and breaks things.

HTH
Emil
[1] https://www.youtube.com/watch?v=LIdznotOxvg

#4 lore raw
In reply to Julia Lawall
Quoted 28 lines · Julia Lawall
Quoted 24 lines · Shayenne Moura
Quoted 7 lines · Julia Lawall
Quoted 2 lines · Shayenne da Luz Moura

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Quoted 7 lines

Is object_idr a field that already exists? If so, "Rename" is not the best choice of words. It should be something like "use the object_idr field instead of the crtc_idr field" and then explain why. "task description in TODO list for KMS cleanups" isn't very helpful to understand why the change should be made.

julia

Hi Julia,

Thank you for your review!

This patch is to solve this TODO task:
drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should be renamed to drm_mode_config.object_idr.

Quoted 8 lines
Quoted 1 line

Do you think I need to use this description in my commit message?

That seems more helpful.

But it seems that the name should actually be changed. Which means that the structure definition should be changed too. Was that done?

julia

Yes, you are right. I did not add the header file.

Thanks again!

Shayenne
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#5 lore raw
Quoted 31 lines · Shayenne Moura
Quoted 30 lines · Julia Lawall
Quoted 25 lines · Shayenne Moura
Quoted 7 lines · Julia Lawall
Quoted 2 lines · Shayenne da Luz Moura

Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups.

Quoted 7 lines

Is object_idr a field that already exists? If so, "Rename" is not the best choice of words. It should be something like "use the object_idr field instead of the crtc_idr field" and then explain why. "task description in TODO list for KMS cleanups" isn't very helpful to understand why the change should be made.

julia

Hi Julia,

Thank you for your review!

This patch is to solve this TODO task:
drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should be renamed to drm_mode_config.object_idr.

Do you think I need to use this description in my commit message?

Quoted 8 lines
Quoted 6 lines

That seems more helpful.

But it seems that the name should actually be changed. Which means that the structure definition should be changed too. Was that done?

julia

Yes, you are right. I did not add the header file.

Compilation should have given an error message. If you thought you compiled, then you don't have the right configuration to actually compile the code that you changed.

julia