* [RFC] drm: store property instead of id in obj attachment
@ 2014-11-27 0:19 Rob Clark
2014-12-02 18:47 ` Sean Paul
0 siblings, 1 reply; 4+ messages in thread
From: Rob Clark @ 2014-11-27 0:19 UTC (permalink / raw)
To: dri-devel
Keep property pointer, instead of id, in per mode-object attachments.
This will simplify things in later patches.
---
Does anyone care about the lifetime issue and dangling property ptrs?
Currently properties are destroyed during drm_mode_config_cleanup(),
so it doesn't really matter.. if someone *did* see a use for dynamically
destroying properties we'd have to do something a bit more clever and
handle properly detaching from any mode object to which that property
had been attached.
Just sending as an RFC for now, but later atomic properties/ioctl patches
will depend on this.
drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
include/drm/drm_crtc.h | 7 ++++++-
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3ba84df..3e6ef39 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2058,12 +2058,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
for (i = 0; i < connector->properties.count; i++) {
- if (put_user(connector->properties.ids[i],
- prop_ptr + copied)) {
+ struct drm_property *prop = connector->properties.properties[i];
+ if (put_user(prop->base.id, prop_ptr + copied)) {
ret = -EFAULT;
goto out;
}
-
if (put_user(connector->properties.values[i],
prop_values + copied)) {
ret = -EFAULT;
@@ -3744,7 +3743,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
return;
}
- obj->properties->ids[count] = property->base.id;
+ obj->properties->properties[count] = property;
obj->properties->values[count] = init_val;
obj->properties->count++;
}
@@ -3769,7 +3768,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
int i;
for (i = 0; i < obj->properties->count; i++) {
- if (obj->properties->ids[i] == property->base.id) {
+ if (obj->properties->properties[i] == property) {
obj->properties->values[i] = val;
return 0;
}
@@ -3799,7 +3798,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
int i;
for (i = 0; i < obj->properties->count; i++) {
- if (obj->properties->ids[i] == property->base.id) {
+ if (obj->properties->properties[i] == property) {
*val = obj->properties->values[i];
return 0;
}
@@ -4263,8 +4262,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
prop_values_ptr = (uint64_t __user *)(unsigned long)
(arg->prop_values_ptr);
for (i = 0; i < props_count; i++) {
- if (put_user(obj->properties->ids[i],
- props_ptr + copied)) {
+ struct drm_property *prop = obj->properties->properties[i];
+ if (put_user(prop->base.id, props_ptr + copied)) {
ret = -EFAULT;
goto out;
}
@@ -4322,7 +4321,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
goto out;
for (i = 0; i < arg_obj->properties->count; i++)
- if (arg_obj->properties->ids[i] == arg->prop_id)
+ if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
break;
if (i == arg_obj->properties->count)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index dd2c16e..017029d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -64,7 +64,12 @@ struct drm_mode_object {
#define DRM_OBJECT_MAX_PROPERTY 24
struct drm_object_properties {
int count;
- uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
+ /* NOTE: if we ever start dynamically destroying properties (ie.
+ * not at drm_mode_config_cleanup() time), then we'd have to do
+ * a better job of detaching property from mode objects to avoid
+ * dangling property pointers:
+ */
+ struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
uint64_t values[DRM_OBJECT_MAX_PROPERTY];
};
--
1.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC] drm: store property instead of id in obj attachment
2014-11-27 0:19 [RFC] drm: store property instead of id in obj attachment Rob Clark
@ 2014-12-02 18:47 ` Sean Paul
2014-12-02 19:09 ` Rob Clark
0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2014-12-02 18:47 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Wed, Nov 26, 2014 at 4:19 PM, Rob Clark <robdclark@gmail.com> wrote:
> Keep property pointer, instead of id, in per mode-object attachments.
> This will simplify things in later patches.
>
> ---
> Does anyone care about the lifetime issue and dangling property ptrs?
Judging by the lack of response, it seems like the answer is probably no.
That said, it doesn't seem like a whole lot of work to add refcounting
to drm_property so we don't have to worry about it. Maybe I'm missing
something.
Sean
> Currently properties are destroyed during drm_mode_config_cleanup(),
> so it doesn't really matter.. if someone *did* see a use for dynamically
> destroying properties we'd have to do something a bit more clever and
> handle properly detaching from any mode object to which that property
> had been attached.
>
> Just sending as an RFC for now, but later atomic properties/ioctl patches
> will depend on this.
>
> drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
> include/drm/drm_crtc.h | 7 ++++++-
> 2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3ba84df..3e6ef39 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2058,12 +2058,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
> prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
> for (i = 0; i < connector->properties.count; i++) {
> - if (put_user(connector->properties.ids[i],
> - prop_ptr + copied)) {
> + struct drm_property *prop = connector->properties.properties[i];
> + if (put_user(prop->base.id, prop_ptr + copied)) {
> ret = -EFAULT;
> goto out;
> }
> -
> if (put_user(connector->properties.values[i],
> prop_values + copied)) {
> ret = -EFAULT;
> @@ -3744,7 +3743,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
> return;
> }
>
> - obj->properties->ids[count] = property->base.id;
> + obj->properties->properties[count] = property;
> obj->properties->values[count] = init_val;
> obj->properties->count++;
> }
> @@ -3769,7 +3768,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
> int i;
>
> for (i = 0; i < obj->properties->count; i++) {
> - if (obj->properties->ids[i] == property->base.id) {
> + if (obj->properties->properties[i] == property) {
> obj->properties->values[i] = val;
> return 0;
> }
> @@ -3799,7 +3798,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
> int i;
>
> for (i = 0; i < obj->properties->count; i++) {
> - if (obj->properties->ids[i] == property->base.id) {
> + if (obj->properties->properties[i] == property) {
> *val = obj->properties->values[i];
> return 0;
> }
> @@ -4263,8 +4262,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> prop_values_ptr = (uint64_t __user *)(unsigned long)
> (arg->prop_values_ptr);
> for (i = 0; i < props_count; i++) {
> - if (put_user(obj->properties->ids[i],
> - props_ptr + copied)) {
> + struct drm_property *prop = obj->properties->properties[i];
> + if (put_user(prop->base.id, props_ptr + copied)) {
> ret = -EFAULT;
> goto out;
> }
> @@ -4322,7 +4321,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> goto out;
>
> for (i = 0; i < arg_obj->properties->count; i++)
> - if (arg_obj->properties->ids[i] == arg->prop_id)
> + if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
> break;
>
> if (i == arg_obj->properties->count)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index dd2c16e..017029d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -64,7 +64,12 @@ struct drm_mode_object {
> #define DRM_OBJECT_MAX_PROPERTY 24
> struct drm_object_properties {
> int count;
> - uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
> + /* NOTE: if we ever start dynamically destroying properties (ie.
> + * not at drm_mode_config_cleanup() time), then we'd have to do
> + * a better job of detaching property from mode objects to avoid
> + * dangling property pointers:
> + */
> + struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
> uint64_t values[DRM_OBJECT_MAX_PROPERTY];
> };
>
> --
> 1.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] drm: store property instead of id in obj attachment
2014-12-02 18:47 ` Sean Paul
@ 2014-12-02 19:09 ` Rob Clark
2014-12-03 13:55 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Rob Clark @ 2014-12-02 19:09 UTC (permalink / raw)
To: Sean Paul; +Cc: dri-devel
On Tue, Dec 2, 2014 at 1:47 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Wed, Nov 26, 2014 at 4:19 PM, Rob Clark <robdclark@gmail.com> wrote:
>> Keep property pointer, instead of id, in per mode-object attachments.
>> This will simplify things in later patches.
>>
>> ---
>> Does anyone care about the lifetime issue and dangling property ptrs?
>
> Judging by the lack of response, it seems like the answer is probably no.
>
> That said, it doesn't seem like a whole lot of work to add refcounting
> to drm_property so we don't have to worry about it. Maybe I'm missing
> something.
true.. but then the property would not actually go away then, it would
be kept alive by the extra ref. That said, I can't think of a valid
reason for dynamically creating/destroying properties, so I'm tempted
to just add a comment somewhere to explain that detaching from mode
objects would be required if someone did want to dynamically destroy a
property and leave it at that..
BR,
-R
> Sean
>
>
>> Currently properties are destroyed during drm_mode_config_cleanup(),
>> so it doesn't really matter.. if someone *did* see a use for dynamically
>> destroying properties we'd have to do something a bit more clever and
>> handle properly detaching from any mode object to which that property
>> had been attached.
>>
>> Just sending as an RFC for now, but later atomic properties/ioctl patches
>> will depend on this.
>>
>> drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
>> include/drm/drm_crtc.h | 7 ++++++-
>> 2 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 3ba84df..3e6ef39 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -2058,12 +2058,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
>> prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
>> for (i = 0; i < connector->properties.count; i++) {
>> - if (put_user(connector->properties.ids[i],
>> - prop_ptr + copied)) {
>> + struct drm_property *prop = connector->properties.properties[i];
>> + if (put_user(prop->base.id, prop_ptr + copied)) {
>> ret = -EFAULT;
>> goto out;
>> }
>> -
>> if (put_user(connector->properties.values[i],
>> prop_values + copied)) {
>> ret = -EFAULT;
>> @@ -3744,7 +3743,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
>> return;
>> }
>>
>> - obj->properties->ids[count] = property->base.id;
>> + obj->properties->properties[count] = property;
>> obj->properties->values[count] = init_val;
>> obj->properties->count++;
>> }
>> @@ -3769,7 +3768,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
>> int i;
>>
>> for (i = 0; i < obj->properties->count; i++) {
>> - if (obj->properties->ids[i] == property->base.id) {
>> + if (obj->properties->properties[i] == property) {
>> obj->properties->values[i] = val;
>> return 0;
>> }
>> @@ -3799,7 +3798,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
>> int i;
>>
>> for (i = 0; i < obj->properties->count; i++) {
>> - if (obj->properties->ids[i] == property->base.id) {
>> + if (obj->properties->properties[i] == property) {
>> *val = obj->properties->values[i];
>> return 0;
>> }
>> @@ -4263,8 +4262,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> prop_values_ptr = (uint64_t __user *)(unsigned long)
>> (arg->prop_values_ptr);
>> for (i = 0; i < props_count; i++) {
>> - if (put_user(obj->properties->ids[i],
>> - props_ptr + copied)) {
>> + struct drm_property *prop = obj->properties->properties[i];
>> + if (put_user(prop->base.id, props_ptr + copied)) {
>> ret = -EFAULT;
>> goto out;
>> }
>> @@ -4322,7 +4321,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>> goto out;
>>
>> for (i = 0; i < arg_obj->properties->count; i++)
>> - if (arg_obj->properties->ids[i] == arg->prop_id)
>> + if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
>> break;
>>
>> if (i == arg_obj->properties->count)
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index dd2c16e..017029d 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -64,7 +64,12 @@ struct drm_mode_object {
>> #define DRM_OBJECT_MAX_PROPERTY 24
>> struct drm_object_properties {
>> int count;
>> - uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
>> + /* NOTE: if we ever start dynamically destroying properties (ie.
>> + * not at drm_mode_config_cleanup() time), then we'd have to do
>> + * a better job of detaching property from mode objects to avoid
>> + * dangling property pointers:
>> + */
>> + struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
>> uint64_t values[DRM_OBJECT_MAX_PROPERTY];
>> };
>>
>> --
>> 1.9.3
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] drm: store property instead of id in obj attachment
2014-12-02 19:09 ` Rob Clark
@ 2014-12-03 13:55 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-12-03 13:55 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Tue, Dec 02, 2014 at 02:09:41PM -0500, Rob Clark wrote:
> On Tue, Dec 2, 2014 at 1:47 PM, Sean Paul <seanpaul@chromium.org> wrote:
> > On Wed, Nov 26, 2014 at 4:19 PM, Rob Clark <robdclark@gmail.com> wrote:
> >> Keep property pointer, instead of id, in per mode-object attachments.
> >> This will simplify things in later patches.
> >>
> >> ---
> >> Does anyone care about the lifetime issue and dangling property ptrs?
> >
> > Judging by the lack of response, it seems like the answer is probably no.
> >
> > That said, it doesn't seem like a whole lot of work to add refcounting
> > to drm_property so we don't have to worry about it. Maybe I'm missing
> > something.
>
> true.. but then the property would not actually go away then, it would
> be kept alive by the extra ref. That said, I can't think of a valid
> reason for dynamically creating/destroying properties, so I'm tempted
> to just add a comment somewhere to explain that detaching from mode
> objects would be required if someone did want to dynamically destroy a
> property and leave it at that..
I've thought we'd implement blob properties by allowing userspace to
allocate a blob prop and then pass around it's id. Similar for
framebuffers those would need to be refcounted and also cleaned up (the
public side at least) when their owning file_priv is closed.
But in general I agree with Rob, there's not really a use case for
refcounted driver-side props that I could think of.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-03 13:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-27 0:19 [RFC] drm: store property instead of id in obj attachment Rob Clark
2014-12-02 18:47 ` Sean Paul
2014-12-02 19:09 ` Rob Clark
2014-12-03 13:55 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox