| Message ID | 20181031205737.cingeaqget7hkbs6@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Rename crtc_idr as object_idr to KMS cleanups | expand |
On Wed, 31 Oct 2018, Shayenne da Luz Moura wrote: > 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 > > Signed-off-by: Shayenne da Luz Moura <shayenneluzmoura@gmail.com> > --- > 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(-) > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index f4702f23c11d..4f8de2217049 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -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; > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..ab553b6465e2 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -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); > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > index cd9bc0ce9be0..bb1dd46496cd 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -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 > > -- > 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 outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > 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. >
On 10/31, Julia Lawall wrote: > > > On Wed, 31 Oct 2018, Shayenne da Luz Moura wrote: > > > 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 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
On Wed, 31 Oct 2018, Shayenne Moura wrote: > On 10/31, Julia Lawall wrote: > > > > > > On Wed, 31 Oct 2018, Shayenne da Luz Moura wrote: > > > > > 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 > > 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? 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 > > 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 outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > 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. >
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: On Wed, 31 Oct 2018 at 20:58, Shayenne da Luz Moura <shayenneluzmoura@gmail.com> wrote: > 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 ;-) > 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. > Signed-off-by: Shayenne da Luz Moura <shayenneluzmoura@gmail.com> > --- > 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
On 10/31, Julia Lawall wrote: > > > On Wed, 31 Oct 2018, Shayenne Moura wrote: > > > On 10/31, Julia Lawall wrote: > > > > > > > > > On Wed, 31 Oct 2018, Shayenne da Luz Moura wrote: > > > > > > > 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 > > > > 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? > > 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
On Wed, 31 Oct 2018, Shayenne Moura wrote: > On 10/31, Julia Lawall wrote: > > > > > > On Wed, 31 Oct 2018, Shayenne Moura wrote: > > > > > On 10/31, Julia Lawall wrote: > > > > > > > > > > > > On Wed, 31 Oct 2018, Shayenne da Luz Moura wrote: > > > > > > > > > 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 > > > > > > 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? > > > > 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
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index f4702f23c11d..4f8de2217049 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -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; diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index ee80788f2c40..ab553b6465e2 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -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); diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index cd9bc0ce9be0..bb1dd46496cd 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -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)
Rename 'drm_mode_config.crtc_idr' as 'drm_mode_config.object_idr', as proposed in the task description in TODO list for KMS cleanups. Signed-off-by: Shayenne da Luz Moura <shayenneluzmoura@gmail.com> --- 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(-)