* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-29 8:57 ` Jani Nikula
0 siblings, 0 replies; 34+ messages in thread
From: Jani Nikula @ 2020-04-29 8:57 UTC (permalink / raw)
To: Michal Orzel, maarten.lankhorst, mripard, tzimmermann, airlied, daniel
Cc: intel-gfx, Michal Orzel, linux-kernel, dri-devel
On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> As suggested by the TODO list for the kernel DRM subsystem, replace
> the deprecated functions that take/drop modeset locks with new helpers.
>
> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> ---
> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 35c2719..901b078 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_mode_obj_get_properties *arg = data;
> struct drm_mode_object *obj;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = 0;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EOPNOTSUPP;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
DRM_MODESET_LOCK_ALL_END macros. :(
Currently only six users... but there are ~60 calls to
drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
if this will come back and haunt us.
BR,
Jani.
>
> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> if (!obj) {
> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> out_unref:
> drm_mode_object_put(obj);
> out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> return ret;
> }
>
> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> {
> struct drm_device *dev = prop->dev;
> struct drm_mode_object *ref;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = -EINVAL;
>
> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> return -EINVAL;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> switch (obj->type) {
> case DRM_MODE_OBJECT_CONNECTOR:
> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> break;
> }
> drm_property_change_valid_put(prop, ref);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>
> return ret;
> }
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-29 8:57 ` Jani Nikula
0 siblings, 0 replies; 34+ messages in thread
From: Jani Nikula @ 2020-04-29 8:57 UTC (permalink / raw)
To: Michal Orzel, maarten.lankhorst, mripard, tzimmermann, airlied, daniel
Cc: intel-gfx, Michal Orzel, linux-kernel, dri-devel
On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> As suggested by the TODO list for the kernel DRM subsystem, replace
> the deprecated functions that take/drop modeset locks with new helpers.
>
> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> ---
> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 35c2719..901b078 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_mode_obj_get_properties *arg = data;
> struct drm_mode_object *obj;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = 0;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EOPNOTSUPP;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
DRM_MODESET_LOCK_ALL_END macros. :(
Currently only six users... but there are ~60 calls to
drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
if this will come back and haunt us.
BR,
Jani.
>
> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> if (!obj) {
> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> out_unref:
> drm_mode_object_put(obj);
> out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> return ret;
> }
>
> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> {
> struct drm_device *dev = prop->dev;
> struct drm_mode_object *ref;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = -EINVAL;
>
> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> return -EINVAL;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> switch (obj->type) {
> case DRM_MODE_OBJECT_CONNECTOR:
> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> break;
> }
> drm_property_change_valid_put(prop, ref);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>
> return ret;
> }
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-04-29 8:57 ` Jani Nikula
(?)
@ 2020-04-30 5:52 ` Michał Orzeł
-1 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-04-30 5:52 UTC (permalink / raw)
To: Jani Nikula, maarten.lankhorst, mripard, tzimmermann, airlied, daniel
Cc: intel-gfx, linux-kernel, dri-devel
On 29.04.2020 10:57, Jani Nikula wrote:
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>> As suggested by the TODO list for the kernel DRM subsystem, replace
>> the deprecated functions that take/drop modeset locks with new helpers.
>>
>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>> ---
>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>> index 35c2719..901b078 100644
>> --- a/drivers/gpu/drm/drm_mode_object.c
>> +++ b/drivers/gpu/drm/drm_mode_object.c
>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> {
>> struct drm_mode_obj_get_properties *arg = data;
>> struct drm_mode_object *obj;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = 0;
>>
>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>> return -EOPNOTSUPP;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
> BR,
> Jani.
Hm, so we can either replace all of these calls(I think it's a better option) or abandon the idea of removing this deprecated function.
In the latter scenario, it'd be beneficial to remove this from TODO.
Best regards
Michal
>
>
>>
>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>> if (!obj) {
>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> out_unref:
>> drm_mode_object_put(obj);
>> out:
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>> return ret;
>> }
>>
>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> {
>> struct drm_device *dev = prop->dev;
>> struct drm_mode_object *ref;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = -EINVAL;
>>
>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>> return -EINVAL;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>> switch (obj->type) {
>> case DRM_MODE_OBJECT_CONNECTOR:
>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> break;
>> }
>> drm_property_change_valid_put(prop, ref);
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>
>> return ret;
>> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 5:52 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-04-30 5:52 UTC (permalink / raw)
To: Jani Nikula, maarten.lankhorst, mripard, tzimmermann, airlied, daniel
Cc: intel-gfx, linux-kernel, dri-devel
On 29.04.2020 10:57, Jani Nikula wrote:
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>> As suggested by the TODO list for the kernel DRM subsystem, replace
>> the deprecated functions that take/drop modeset locks with new helpers.
>>
>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>> ---
>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>> index 35c2719..901b078 100644
>> --- a/drivers/gpu/drm/drm_mode_object.c
>> +++ b/drivers/gpu/drm/drm_mode_object.c
>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> {
>> struct drm_mode_obj_get_properties *arg = data;
>> struct drm_mode_object *obj;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = 0;
>>
>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>> return -EOPNOTSUPP;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
> BR,
> Jani.
Hm, so we can either replace all of these calls(I think it's a better option) or abandon the idea of removing this deprecated function.
In the latter scenario, it'd be beneficial to remove this from TODO.
Best regards
Michal
>
>
>>
>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>> if (!obj) {
>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> out_unref:
>> drm_mode_object_put(obj);
>> out:
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>> return ret;
>> }
>>
>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> {
>> struct drm_device *dev = prop->dev;
>> struct drm_mode_object *ref;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = -EINVAL;
>>
>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>> return -EINVAL;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>> switch (obj->type) {
>> case DRM_MODE_OBJECT_CONNECTOR:
>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> break;
>> }
>> drm_property_change_valid_put(prop, ref);
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>
>> return ret;
>> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 5:52 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-04-30 5:52 UTC (permalink / raw)
To: Jani Nikula, maarten.lankhorst, mripard, tzimmermann, airlied, daniel
Cc: intel-gfx, linux-kernel, dri-devel
On 29.04.2020 10:57, Jani Nikula wrote:
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>> As suggested by the TODO list for the kernel DRM subsystem, replace
>> the deprecated functions that take/drop modeset locks with new helpers.
>>
>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>> ---
>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>> index 35c2719..901b078 100644
>> --- a/drivers/gpu/drm/drm_mode_object.c
>> +++ b/drivers/gpu/drm/drm_mode_object.c
>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> {
>> struct drm_mode_obj_get_properties *arg = data;
>> struct drm_mode_object *obj;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = 0;
>>
>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>> return -EOPNOTSUPP;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
> BR,
> Jani.
Hm, so we can either replace all of these calls(I think it's a better option) or abandon the idea of removing this deprecated function.
In the latter scenario, it'd be beneficial to remove this from TODO.
Best regards
Michal
>
>
>>
>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>> if (!obj) {
>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>> out_unref:
>> drm_mode_object_put(obj);
>> out:
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>> return ret;
>> }
>>
>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> {
>> struct drm_device *dev = prop->dev;
>> struct drm_mode_object *ref;
>> + struct drm_modeset_acquire_ctx ctx;
>> int ret = -EINVAL;
>>
>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>> return -EINVAL;
>>
>> - drm_modeset_lock_all(dev);
>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>> switch (obj->type) {
>> case DRM_MODE_OBJECT_CONNECTOR:
>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>> break;
>> }
>> drm_property_change_valid_put(prop, ref);
>> - drm_modeset_unlock_all(dev);
>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>
>> return ret;
>> }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-04-29 8:57 ` Jani Nikula
(?)
@ 2020-04-30 15:37 ` Sean Paul
-1 siblings, 0 replies; 34+ messages in thread
From: Sean Paul @ 2020-04-30 15:37 UTC (permalink / raw)
To: Jani Nikula
Cc: Michal Orzel, Maarten Lankhorst, mripard, tzimmermann,
Dave Airlie, Daniel Vetter, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel
On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > As suggested by the TODO list for the kernel DRM subsystem, replace
> > the deprecated functions that take/drop modeset locks with new helpers.
> >
> > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > ---
> > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index 35c2719..901b078 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > {
> > struct drm_mode_obj_get_properties *arg = data;
> > struct drm_mode_object *obj;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = 0;
> >
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > return -EOPNOTSUPP;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
What's the alternative? Seems like the options without the macros is
to use incorrect scope or have a bunch of retry/backoff cargo-cult
everywhere (and hope the copy source is done correctly).
Sean
> BR,
> Jani.
>
>
> >
> > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > if (!obj) {
> > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > out_unref:
> > drm_mode_object_put(obj);
> > out:
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > return ret;
> > }
> >
> > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > {
> > struct drm_device *dev = prop->dev;
> > struct drm_mode_object *ref;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = -EINVAL;
> >
> > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > return -EINVAL;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > switch (obj->type) {
> > case DRM_MODE_OBJECT_CONNECTOR:
> > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > break;
> > }
> > drm_property_change_valid_put(prop, ref);
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >
> > return ret;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 15:37 ` Sean Paul
0 siblings, 0 replies; 34+ messages in thread
From: Sean Paul @ 2020-04-30 15:37 UTC (permalink / raw)
To: Jani Nikula
Cc: tzimmermann, Dave Airlie, Michal Orzel,
Intel Graphics Development, Linux Kernel Mailing List, mripard,
dri-devel
On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > As suggested by the TODO list for the kernel DRM subsystem, replace
> > the deprecated functions that take/drop modeset locks with new helpers.
> >
> > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > ---
> > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index 35c2719..901b078 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > {
> > struct drm_mode_obj_get_properties *arg = data;
> > struct drm_mode_object *obj;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = 0;
> >
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > return -EOPNOTSUPP;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
What's the alternative? Seems like the options without the macros is
to use incorrect scope or have a bunch of retry/backoff cargo-cult
everywhere (and hope the copy source is done correctly).
Sean
> BR,
> Jani.
>
>
> >
> > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > if (!obj) {
> > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > out_unref:
> > drm_mode_object_put(obj);
> > out:
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > return ret;
> > }
> >
> > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > {
> > struct drm_device *dev = prop->dev;
> > struct drm_mode_object *ref;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = -EINVAL;
> >
> > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > return -EINVAL;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > switch (obj->type) {
> > case DRM_MODE_OBJECT_CONNECTOR:
> > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > break;
> > }
> > drm_property_change_valid_put(prop, ref);
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >
> > return ret;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 15:37 ` Sean Paul
0 siblings, 0 replies; 34+ messages in thread
From: Sean Paul @ 2020-04-30 15:37 UTC (permalink / raw)
To: Jani Nikula
Cc: tzimmermann, Dave Airlie, Michal Orzel,
Intel Graphics Development, Linux Kernel Mailing List, dri-devel
On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > As suggested by the TODO list for the kernel DRM subsystem, replace
> > the deprecated functions that take/drop modeset locks with new helpers.
> >
> > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > ---
> > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index 35c2719..901b078 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > {
> > struct drm_mode_obj_get_properties *arg = data;
> > struct drm_mode_object *obj;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = 0;
> >
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > return -EOPNOTSUPP;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> DRM_MODESET_LOCK_ALL_END macros. :(
>
> Currently only six users... but there are ~60 calls to
> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> if this will come back and haunt us.
>
What's the alternative? Seems like the options without the macros is
to use incorrect scope or have a bunch of retry/backoff cargo-cult
everywhere (and hope the copy source is done correctly).
Sean
> BR,
> Jani.
>
>
> >
> > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > if (!obj) {
> > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > out_unref:
> > drm_mode_object_put(obj);
> > out:
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > return ret;
> > }
> >
> > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > {
> > struct drm_device *dev = prop->dev;
> > struct drm_mode_object *ref;
> > + struct drm_modeset_acquire_ctx ctx;
> > int ret = -EINVAL;
> >
> > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > return -EINVAL;
> >
> > - drm_modeset_lock_all(dev);
> > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > switch (obj->type) {
> > case DRM_MODE_OBJECT_CONNECTOR:
> > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > break;
> > }
> > drm_property_change_valid_put(prop, ref);
> > - drm_modeset_unlock_all(dev);
> > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >
> > return ret;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-04-30 15:37 ` Sean Paul
(?)
@ 2020-04-30 18:30 ` Daniel Vetter
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-04-30 18:30 UTC (permalink / raw)
To: Sean Paul
Cc: Jani Nikula, Michal Orzel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel
On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>
> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > > As suggested by the TODO list for the kernel DRM subsystem, replace
> > > the deprecated functions that take/drop modeset locks with new helpers.
> > >
> > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > > ---
> > > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > > index 35c2719..901b078 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > {
> > > struct drm_mode_obj_get_properties *arg = data;
> > > struct drm_mode_object *obj;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = 0;
> > >
> > > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > return -EOPNOTSUPP;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> > DRM_MODESET_LOCK_ALL_END macros. :(
> >
> > Currently only six users... but there are ~60 calls to
> > drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> > if this will come back and haunt us.
> >
>
> What's the alternative? Seems like the options without the macros is
> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> everywhere (and hope the copy source is done correctly).
Yeah Sean & me had a bunch of bikesheds and this is the least worst
option we could come up with. You can't make it a function because of
the control flow. You don't want to open code this because it's tricky
to get right, if all you want is to just grab all locks. But it is
magic hidden behind a macro, which occasionally ends up hurting.
-Daniel
> Sean
>
> > BR,
> > Jani.
> >
> >
> > >
> > > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > > if (!obj) {
> > > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > out_unref:
> > > drm_mode_object_put(obj);
> > > out:
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > > return ret;
> > > }
> > >
> > > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > {
> > > struct drm_device *dev = prop->dev;
> > > struct drm_mode_object *ref;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = -EINVAL;
> > >
> > > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > > return -EINVAL;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > > switch (obj->type) {
> > > case DRM_MODE_OBJECT_CONNECTOR:
> > > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > break;
> > > }
> > > drm_property_change_valid_put(prop, ref);
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > >
> > > return ret;
> > > }
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 18:30 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-04-30 18:30 UTC (permalink / raw)
To: Sean Paul
Cc: dri-devel, Dave Airlie, Michal Orzel, Intel Graphics Development,
Linux Kernel Mailing List, Maxime Ripard, Thomas Zimmermann
On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>
> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > > As suggested by the TODO list for the kernel DRM subsystem, replace
> > > the deprecated functions that take/drop modeset locks with new helpers.
> > >
> > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > > ---
> > > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > > index 35c2719..901b078 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > {
> > > struct drm_mode_obj_get_properties *arg = data;
> > > struct drm_mode_object *obj;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = 0;
> > >
> > > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > return -EOPNOTSUPP;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> > DRM_MODESET_LOCK_ALL_END macros. :(
> >
> > Currently only six users... but there are ~60 calls to
> > drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> > if this will come back and haunt us.
> >
>
> What's the alternative? Seems like the options without the macros is
> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> everywhere (and hope the copy source is done correctly).
Yeah Sean & me had a bunch of bikesheds and this is the least worst
option we could come up with. You can't make it a function because of
the control flow. You don't want to open code this because it's tricky
to get right, if all you want is to just grab all locks. But it is
magic hidden behind a macro, which occasionally ends up hurting.
-Daniel
> Sean
>
> > BR,
> > Jani.
> >
> >
> > >
> > > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > > if (!obj) {
> > > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > out_unref:
> > > drm_mode_object_put(obj);
> > > out:
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > > return ret;
> > > }
> > >
> > > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > {
> > > struct drm_device *dev = prop->dev;
> > > struct drm_mode_object *ref;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = -EINVAL;
> > >
> > > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > > return -EINVAL;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > > switch (obj->type) {
> > > case DRM_MODE_OBJECT_CONNECTOR:
> > > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > break;
> > > }
> > > drm_property_change_valid_put(prop, ref);
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > >
> > > return ret;
> > > }
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-04-30 18:30 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-04-30 18:30 UTC (permalink / raw)
To: Sean Paul
Cc: dri-devel, Dave Airlie, Michal Orzel, Intel Graphics Development,
Linux Kernel Mailing List, Thomas Zimmermann
On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>
> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> > > As suggested by the TODO list for the kernel DRM subsystem, replace
> > > the deprecated functions that take/drop modeset locks with new helpers.
> > >
> > > Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> > > ---
> > > drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > > index 35c2719..901b078 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > {
> > > struct drm_mode_obj_get_properties *arg = data;
> > > struct drm_mode_object *obj;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = 0;
> > >
> > > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > return -EOPNOTSUPP;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> > DRM_MODESET_LOCK_ALL_END macros. :(
> >
> > Currently only six users... but there are ~60 calls to
> > drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> > if this will come back and haunt us.
> >
>
> What's the alternative? Seems like the options without the macros is
> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> everywhere (and hope the copy source is done correctly).
Yeah Sean & me had a bunch of bikesheds and this is the least worst
option we could come up with. You can't make it a function because of
the control flow. You don't want to open code this because it's tricky
to get right, if all you want is to just grab all locks. But it is
magic hidden behind a macro, which occasionally ends up hurting.
-Daniel
> Sean
>
> > BR,
> > Jani.
> >
> >
> > >
> > > obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > > if (!obj) {
> > > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > > out_unref:
> > > drm_mode_object_put(obj);
> > > out:
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > > return ret;
> > > }
> > >
> > > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > {
> > > struct drm_device *dev = prop->dev;
> > > struct drm_mode_object *ref;
> > > + struct drm_modeset_acquire_ctx ctx;
> > > int ret = -EINVAL;
> > >
> > > if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > > return -EINVAL;
> > >
> > > - drm_modeset_lock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > > switch (obj->type) {
> > > case DRM_MODE_OBJECT_CONNECTOR:
> > > ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > > break;
> > > }
> > > drm_property_change_valid_put(prop, ref);
> > > - drm_modeset_unlock_all(dev);
> > > + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > >
> > > return ret;
> > > }
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
--
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
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-04-30 18:30 ` Daniel Vetter
(?)
@ 2020-05-01 15:49 ` Michał Orzeł
-1 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-01 15:49 UTC (permalink / raw)
To: Daniel Vetter, Sean Paul
Cc: Jani Nikula, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel
On 30.04.2020 20:30, Daniel Vetter wrote:
> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>
>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>
>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>
>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>> ---
>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>> index 35c2719..901b078 100644
>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> {
>>>> struct drm_mode_obj_get_properties *arg = data;
>>>> struct drm_mode_object *obj;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = 0;
>>>>
>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>> return -EOPNOTSUPP;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>
>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>
>>> Currently only six users... but there are ~60 calls to
>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>> if this will come back and haunt us.
>>>
>>
>> What's the alternative? Seems like the options without the macros is
>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>> everywhere (and hope the copy source is done correctly).
>
> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> option we could come up with. You can't make it a function because of
> the control flow. You don't want to open code this because it's tricky
> to get right, if all you want is to just grab all locks. But it is
> magic hidden behind a macro, which occasionally ends up hurting.
> -Daniel
So what are we doing with this problem? Should we replace at once approx. 60 calls?
Michal
>
>> Sean
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>
>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>> if (!obj) {
>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> out_unref:
>>>> drm_mode_object_put(obj);
>>>> out:
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> {
>>>> struct drm_device *dev = prop->dev;
>>>> struct drm_mode_object *ref;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = -EINVAL;
>>>>
>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>> return -EINVAL;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>> switch (obj->type) {
>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> break;
>>>> }
>>>> drm_property_change_valid_put(prop, ref);
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>
>>>> return ret;
>>>> }
>>>
>>> --
>>> Jani Nikula, Intel Open Source Graphics Center
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-01 15:49 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-01 15:49 UTC (permalink / raw)
To: Daniel Vetter, Sean Paul
Cc: dri-devel, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, Maxime Ripard, Thomas Zimmermann
On 30.04.2020 20:30, Daniel Vetter wrote:
> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>
>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>
>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>
>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>> ---
>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>> index 35c2719..901b078 100644
>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> {
>>>> struct drm_mode_obj_get_properties *arg = data;
>>>> struct drm_mode_object *obj;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = 0;
>>>>
>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>> return -EOPNOTSUPP;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>
>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>
>>> Currently only six users... but there are ~60 calls to
>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>> if this will come back and haunt us.
>>>
>>
>> What's the alternative? Seems like the options without the macros is
>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>> everywhere (and hope the copy source is done correctly).
>
> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> option we could come up with. You can't make it a function because of
> the control flow. You don't want to open code this because it's tricky
> to get right, if all you want is to just grab all locks. But it is
> magic hidden behind a macro, which occasionally ends up hurting.
> -Daniel
So what are we doing with this problem? Should we replace at once approx. 60 calls?
Michal
>
>> Sean
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>
>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>> if (!obj) {
>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> out_unref:
>>>> drm_mode_object_put(obj);
>>>> out:
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> {
>>>> struct drm_device *dev = prop->dev;
>>>> struct drm_mode_object *ref;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = -EINVAL;
>>>>
>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>> return -EINVAL;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>> switch (obj->type) {
>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> break;
>>>> }
>>>> drm_property_change_valid_put(prop, ref);
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>
>>>> return ret;
>>>> }
>>>
>>> --
>>> Jani Nikula, Intel Open Source Graphics Center
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-01 15:49 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-01 15:49 UTC (permalink / raw)
To: Daniel Vetter, Sean Paul
Cc: dri-devel, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, Thomas Zimmermann
On 30.04.2020 20:30, Daniel Vetter wrote:
> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>
>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>
>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>
>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>> ---
>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>> index 35c2719..901b078 100644
>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> {
>>>> struct drm_mode_obj_get_properties *arg = data;
>>>> struct drm_mode_object *obj;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = 0;
>>>>
>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>> return -EOPNOTSUPP;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>
>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>
>>> Currently only six users... but there are ~60 calls to
>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>> if this will come back and haunt us.
>>>
>>
>> What's the alternative? Seems like the options without the macros is
>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>> everywhere (and hope the copy source is done correctly).
>
> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> option we could come up with. You can't make it a function because of
> the control flow. You don't want to open code this because it's tricky
> to get right, if all you want is to just grab all locks. But it is
> magic hidden behind a macro, which occasionally ends up hurting.
> -Daniel
So what are we doing with this problem? Should we replace at once approx. 60 calls?
Michal
>
>> Sean
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>
>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>> if (!obj) {
>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>> out_unref:
>>>> drm_mode_object_put(obj);
>>>> out:
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> {
>>>> struct drm_device *dev = prop->dev;
>>>> struct drm_mode_object *ref;
>>>> + struct drm_modeset_acquire_ctx ctx;
>>>> int ret = -EINVAL;
>>>>
>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>> return -EINVAL;
>>>>
>>>> - drm_modeset_lock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>> switch (obj->type) {
>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>> break;
>>>> }
>>>> drm_property_change_valid_put(prop, ref);
>>>> - drm_modeset_unlock_all(dev);
>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>
>>>> return ret;
>>>> }
>>>
>>> --
>>> Jani Nikula, Intel Open Source Graphics Center
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-05-01 15:49 ` Michał Orzeł
(?)
@ 2020-05-04 11:53 ` Daniel Vetter
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-04 11:53 UTC (permalink / raw)
To: Michał Orzeł
Cc: Daniel Vetter, Sean Paul, Jani Nikula, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Dave Airlie,
Intel Graphics Development, Linux Kernel Mailing List, dri-devel
On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>
>
> On 30.04.2020 20:30, Daniel Vetter wrote:
> > On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>
> >> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>
> >>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>
> >>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>> ---
> >>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>> index 35c2719..901b078 100644
> >>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> {
> >>>> struct drm_mode_obj_get_properties *arg = data;
> >>>> struct drm_mode_object *obj;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = 0;
> >>>>
> >>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>> return -EOPNOTSUPP;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>
> >>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>
> >>> Currently only six users... but there are ~60 calls to
> >>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>> if this will come back and haunt us.
> >>>
> >>
> >> What's the alternative? Seems like the options without the macros is
> >> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >> everywhere (and hope the copy source is done correctly).
> >
> > Yeah Sean & me had a bunch of bikesheds and this is the least worst
> > option we could come up with. You can't make it a function because of
> > the control flow. You don't want to open code this because it's tricky
> > to get right, if all you want is to just grab all locks. But it is
> > magic hidden behind a macro, which occasionally ends up hurting.
> > -Daniel
> So what are we doing with this problem? Should we replace at once approx. 60 calls?
I'm confused by your question - dradual conversion is entirely orthogonal
to what exactly we're converting too. All I added here is that we've
discussed this at length, and the macro is the best thing we've come up
with. I still think it's the best compromise.
Flag-day conversion for over 60 calls doesn't work, no matter what.
-Daniel
>
> Michal
> >
> >> Sean
> >>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>>>
> >>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>> if (!obj) {
> >>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> out_unref:
> >>>> drm_mode_object_put(obj);
> >>>> out:
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>> return ret;
> >>>> }
> >>>>
> >>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> {
> >>>> struct drm_device *dev = prop->dev;
> >>>> struct drm_mode_object *ref;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = -EINVAL;
> >>>>
> >>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>> return -EINVAL;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>> switch (obj->type) {
> >>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> break;
> >>>> }
> >>>> drm_property_change_valid_put(prop, ref);
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>
> >>>> return ret;
> >>>> }
> >>>
> >>> --
> >>> Jani Nikula, Intel Open Source Graphics Center
> >
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-04 11:53 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-04 11:53 UTC (permalink / raw)
To: Michał Orzeł
Cc: dri-devel, Thomas Zimmermann, Dave Airlie,
Intel Graphics Development, Linux Kernel Mailing List,
Maxime Ripard, Sean Paul
On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>
>
> On 30.04.2020 20:30, Daniel Vetter wrote:
> > On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>
> >> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>
> >>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>
> >>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>> ---
> >>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>> index 35c2719..901b078 100644
> >>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> {
> >>>> struct drm_mode_obj_get_properties *arg = data;
> >>>> struct drm_mode_object *obj;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = 0;
> >>>>
> >>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>> return -EOPNOTSUPP;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>
> >>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>
> >>> Currently only six users... but there are ~60 calls to
> >>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>> if this will come back and haunt us.
> >>>
> >>
> >> What's the alternative? Seems like the options without the macros is
> >> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >> everywhere (and hope the copy source is done correctly).
> >
> > Yeah Sean & me had a bunch of bikesheds and this is the least worst
> > option we could come up with. You can't make it a function because of
> > the control flow. You don't want to open code this because it's tricky
> > to get right, if all you want is to just grab all locks. But it is
> > magic hidden behind a macro, which occasionally ends up hurting.
> > -Daniel
> So what are we doing with this problem? Should we replace at once approx. 60 calls?
I'm confused by your question - dradual conversion is entirely orthogonal
to what exactly we're converting too. All I added here is that we've
discussed this at length, and the macro is the best thing we've come up
with. I still think it's the best compromise.
Flag-day conversion for over 60 calls doesn't work, no matter what.
-Daniel
>
> Michal
> >
> >> Sean
> >>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>>>
> >>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>> if (!obj) {
> >>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> out_unref:
> >>>> drm_mode_object_put(obj);
> >>>> out:
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>> return ret;
> >>>> }
> >>>>
> >>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> {
> >>>> struct drm_device *dev = prop->dev;
> >>>> struct drm_mode_object *ref;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = -EINVAL;
> >>>>
> >>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>> return -EINVAL;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>> switch (obj->type) {
> >>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> break;
> >>>> }
> >>>> drm_property_change_valid_put(prop, ref);
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>
> >>>> return ret;
> >>>> }
> >>>
> >>> --
> >>> Jani Nikula, Intel Open Source Graphics Center
> >
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-04 11:53 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-04 11:53 UTC (permalink / raw)
To: Michał Orzeł
Cc: dri-devel, Thomas Zimmermann, Dave Airlie,
Intel Graphics Development, Linux Kernel Mailing List, Sean Paul
On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>
>
> On 30.04.2020 20:30, Daniel Vetter wrote:
> > On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>
> >> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>
> >>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>
> >>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>> ---
> >>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>> index 35c2719..901b078 100644
> >>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> {
> >>>> struct drm_mode_obj_get_properties *arg = data;
> >>>> struct drm_mode_object *obj;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = 0;
> >>>>
> >>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>> return -EOPNOTSUPP;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>
> >>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>
> >>> Currently only six users... but there are ~60 calls to
> >>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>> if this will come back and haunt us.
> >>>
> >>
> >> What's the alternative? Seems like the options without the macros is
> >> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >> everywhere (and hope the copy source is done correctly).
> >
> > Yeah Sean & me had a bunch of bikesheds and this is the least worst
> > option we could come up with. You can't make it a function because of
> > the control flow. You don't want to open code this because it's tricky
> > to get right, if all you want is to just grab all locks. But it is
> > magic hidden behind a macro, which occasionally ends up hurting.
> > -Daniel
> So what are we doing with this problem? Should we replace at once approx. 60 calls?
I'm confused by your question - dradual conversion is entirely orthogonal
to what exactly we're converting too. All I added here is that we've
discussed this at length, and the macro is the best thing we've come up
with. I still think it's the best compromise.
Flag-day conversion for over 60 calls doesn't work, no matter what.
-Daniel
>
> Michal
> >
> >> Sean
> >>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>>>
> >>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>> if (!obj) {
> >>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>> out_unref:
> >>>> drm_mode_object_put(obj);
> >>>> out:
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>> return ret;
> >>>> }
> >>>>
> >>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> {
> >>>> struct drm_device *dev = prop->dev;
> >>>> struct drm_mode_object *ref;
> >>>> + struct drm_modeset_acquire_ctx ctx;
> >>>> int ret = -EINVAL;
> >>>>
> >>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>> return -EINVAL;
> >>>>
> >>>> - drm_modeset_lock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>> switch (obj->type) {
> >>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>> break;
> >>>> }
> >>>> drm_property_change_valid_put(prop, ref);
> >>>> - drm_modeset_unlock_all(dev);
> >>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>
> >>>> return ret;
> >>>> }
> >>>
> >>> --
> >>> Jani Nikula, Intel Open Source Graphics Center
> >
> >
> >
--
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] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-05-04 11:53 ` Daniel Vetter
(?)
@ 2020-05-05 5:55 ` Michał Orzeł
-1 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-05 5:55 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Daniel Vetter
Cc: Maxime Ripard, Thomas Zimmermann, Dave Airlie,
Intel Graphics Development, Linux Kernel Mailing List, dri-devel
On 04.05.2020 13:53, Daniel Vetter wrote:
> On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>>
>>
>> On 30.04.2020 20:30, Daniel Vetter wrote:
>>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>>>
>>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>>
>>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>>>> index 35c2719..901b078 100644
>>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> {
>>>>>> struct drm_mode_obj_get_properties *arg = data;
>>>>>> struct drm_mode_object *obj;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = 0;
>>>>>>
>>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>>> return -EOPNOTSUPP;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>
>>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>>>
>>>>> Currently only six users... but there are ~60 calls to
>>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>>>> if this will come back and haunt us.
>>>>>
>>>>
>>>> What's the alternative? Seems like the options without the macros is
>>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>>>> everywhere (and hope the copy source is done correctly).
>>>
>>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
>>> option we could come up with. You can't make it a function because of
>>> the control flow. You don't want to open code this because it's tricky
>>> to get right, if all you want is to just grab all locks. But it is
>>> magic hidden behind a macro, which occasionally ends up hurting.
>>> -Daniel
>> So what are we doing with this problem? Should we replace at once approx. 60 calls?
>
> I'm confused by your question - dradual conversion is entirely orthogonal
> to what exactly we're converting too. All I added here is that we've
> discussed this at length, and the macro is the best thing we've come up
> with. I still think it's the best compromise.
>
> Flag-day conversion for over 60 calls doesn't work, no matter what.
> -Daniel
>
I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Thanks,
Michal
>>
>> Michal
>>>
>>>> Sean
>>>>
>>>>> BR,
>>>>> Jani.
>>>>>
>>>>>
>>>>>>
>>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>>>> if (!obj) {
>>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> out_unref:
>>>>>> drm_mode_object_put(obj);
>>>>>> out:
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> {
>>>>>> struct drm_device *dev = prop->dev;
>>>>>> struct drm_mode_object *ref;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = -EINVAL;
>>>>>>
>>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>> switch (obj->type) {
>>>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> break;
>>>>>> }
>>>>>> drm_property_change_valid_put(prop, ref);
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>
>>>>> --
>>>>> Jani Nikula, Intel Open Source Graphics Center
>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-05 5:55 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-05 5:55 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Daniel Vetter
Cc: Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel, Maxime Ripard,
Thomas Zimmermann
On 04.05.2020 13:53, Daniel Vetter wrote:
> On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>>
>>
>> On 30.04.2020 20:30, Daniel Vetter wrote:
>>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>>>
>>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>>
>>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>>>> index 35c2719..901b078 100644
>>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> {
>>>>>> struct drm_mode_obj_get_properties *arg = data;
>>>>>> struct drm_mode_object *obj;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = 0;
>>>>>>
>>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>>> return -EOPNOTSUPP;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>
>>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>>>
>>>>> Currently only six users... but there are ~60 calls to
>>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>>>> if this will come back and haunt us.
>>>>>
>>>>
>>>> What's the alternative? Seems like the options without the macros is
>>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>>>> everywhere (and hope the copy source is done correctly).
>>>
>>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
>>> option we could come up with. You can't make it a function because of
>>> the control flow. You don't want to open code this because it's tricky
>>> to get right, if all you want is to just grab all locks. But it is
>>> magic hidden behind a macro, which occasionally ends up hurting.
>>> -Daniel
>> So what are we doing with this problem? Should we replace at once approx. 60 calls?
>
> I'm confused by your question - dradual conversion is entirely orthogonal
> to what exactly we're converting too. All I added here is that we've
> discussed this at length, and the macro is the best thing we've come up
> with. I still think it's the best compromise.
>
> Flag-day conversion for over 60 calls doesn't work, no matter what.
> -Daniel
>
I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Thanks,
Michal
>>
>> Michal
>>>
>>>> Sean
>>>>
>>>>> BR,
>>>>> Jani.
>>>>>
>>>>>
>>>>>>
>>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>>>> if (!obj) {
>>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> out_unref:
>>>>>> drm_mode_object_put(obj);
>>>>>> out:
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> {
>>>>>> struct drm_device *dev = prop->dev;
>>>>>> struct drm_mode_object *ref;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = -EINVAL;
>>>>>>
>>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>> switch (obj->type) {
>>>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> break;
>>>>>> }
>>>>>> drm_property_change_valid_put(prop, ref);
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>
>>>>> --
>>>>> Jani Nikula, Intel Open Source Graphics Center
>>>
>>>
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-05 5:55 ` Michał Orzeł
0 siblings, 0 replies; 34+ messages in thread
From: Michał Orzeł @ 2020-05-05 5:55 UTC (permalink / raw)
To: Sean Paul, Jani Nikula, Daniel Vetter
Cc: Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel, Thomas Zimmermann
On 04.05.2020 13:53, Daniel Vetter wrote:
> On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
>>
>>
>> On 30.04.2020 20:30, Daniel Vetter wrote:
>>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
>>>>
>>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>>
>>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
>>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
>>>>>> the deprecated functions that take/drop modeset locks with new helpers.
>>>>>>
>>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
>>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>>>>>> index 35c2719..901b078 100644
>>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
>>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
>>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> {
>>>>>> struct drm_mode_obj_get_properties *arg = data;
>>>>>> struct drm_mode_object *obj;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = 0;
>>>>>>
>>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>>> return -EOPNOTSUPP;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>
>>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
>>>>> DRM_MODESET_LOCK_ALL_END macros. :(
>>>>>
>>>>> Currently only six users... but there are ~60 calls to
>>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
>>>>> if this will come back and haunt us.
>>>>>
>>>>
>>>> What's the alternative? Seems like the options without the macros is
>>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
>>>> everywhere (and hope the copy source is done correctly).
>>>
>>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
>>> option we could come up with. You can't make it a function because of
>>> the control flow. You don't want to open code this because it's tricky
>>> to get right, if all you want is to just grab all locks. But it is
>>> magic hidden behind a macro, which occasionally ends up hurting.
>>> -Daniel
>> So what are we doing with this problem? Should we replace at once approx. 60 calls?
>
> I'm confused by your question - dradual conversion is entirely orthogonal
> to what exactly we're converting too. All I added here is that we've
> discussed this at length, and the macro is the best thing we've come up
> with. I still think it's the best compromise.
>
> Flag-day conversion for over 60 calls doesn't work, no matter what.
> -Daniel
>
I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Thanks,
Michal
>>
>> Michal
>>>
>>>> Sean
>>>>
>>>>> BR,
>>>>> Jani.
>>>>>
>>>>>
>>>>>>
>>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
>>>>>> if (!obj) {
>>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>>>>> out_unref:
>>>>>> drm_mode_object_put(obj);
>>>>>> out:
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> {
>>>>>> struct drm_device *dev = prop->dev;
>>>>>> struct drm_mode_object *ref;
>>>>>> + struct drm_modeset_acquire_ctx ctx;
>>>>>> int ret = -EINVAL;
>>>>>>
>>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - drm_modeset_lock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>>>>>> switch (obj->type) {
>>>>>> case DRM_MODE_OBJECT_CONNECTOR:
>>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
>>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>>>>> break;
>>>>>> }
>>>>>> drm_property_change_valid_put(prop, ref);
>>>>>> - drm_modeset_unlock_all(dev);
>>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>
>>>>> --
>>>>> Jani Nikula, Intel Open Source Graphics Center
>>>
>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
2020-05-05 5:55 ` Michał Orzeł
(?)
@ 2020-05-05 8:51 ` Daniel Vetter
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-05 8:51 UTC (permalink / raw)
To: Michał Orzeł
Cc: Sean Paul, Jani Nikula, Daniel Vetter, Maxime Ripard,
Thomas Zimmermann, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, dri-devel
On Tue, May 05, 2020 at 07:55:00AM +0200, Michał Orzeł wrote:
>
>
> On 04.05.2020 13:53, Daniel Vetter wrote:
> > On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
> >>
> >>
> >> On 30.04.2020 20:30, Daniel Vetter wrote:
> >>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>>>
> >>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>>>
> >>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>>>
> >>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>>>> ---
> >>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> index 35c2719..901b078 100644
> >>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> {
> >>>>>> struct drm_mode_obj_get_properties *arg = data;
> >>>>>> struct drm_mode_object *obj;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = 0;
> >>>>>>
> >>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>>>> return -EOPNOTSUPP;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>
> >>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>>>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>>>
> >>>>> Currently only six users... but there are ~60 calls to
> >>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>>>> if this will come back and haunt us.
> >>>>>
> >>>>
> >>>> What's the alternative? Seems like the options without the macros is
> >>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >>>> everywhere (and hope the copy source is done correctly).
> >>>
> >>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> >>> option we could come up with. You can't make it a function because of
> >>> the control flow. You don't want to open code this because it's tricky
> >>> to get right, if all you want is to just grab all locks. But it is
> >>> magic hidden behind a macro, which occasionally ends up hurting.
> >>> -Daniel
> >> So what are we doing with this problem? Should we replace at once approx. 60 calls?
> >
> > I'm confused by your question - dradual conversion is entirely orthogonal
> > to what exactly we're converting too. All I added here is that we've
> > discussed this at length, and the macro is the best thing we've come up
> > with. I still think it's the best compromise.
> >
> > Flag-day conversion for over 60 calls doesn't work, no matter what.
> > -Daniel
> >
> I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Patch looks good and passed CI, so I went ahead and applied it.
Thanks, Daniel
>
> Thanks,
> Michal
> >>
> >> Michal
> >>>
> >>>> Sean
> >>>>
> >>>>> BR,
> >>>>> Jani.
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>>>> if (!obj) {
> >>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> out_unref:
> >>>>>> drm_mode_object_put(obj);
> >>>>>> out:
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>> return ret;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> {
> >>>>>> struct drm_device *dev = prop->dev;
> >>>>>> struct drm_mode_object *ref;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = -EINVAL;
> >>>>>>
> >>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>> switch (obj->type) {
> >>>>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> break;
> >>>>>> }
> >>>>>> drm_property_change_valid_put(prop, ref);
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>>
> >>>>>> return ret;
> >>>>>> }
> >>>>>
> >>>>> --
> >>>>> Jani Nikula, Intel Open Source Graphics Center
> >>>
> >>>
> >>>
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Intel-gfx] [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-05 8:51 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-05 8:51 UTC (permalink / raw)
To: Michał Orzeł
Cc: dri-devel, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, Maxime Ripard, Sean Paul,
Thomas Zimmermann
On Tue, May 05, 2020 at 07:55:00AM +0200, Michał Orzeł wrote:
>
>
> On 04.05.2020 13:53, Daniel Vetter wrote:
> > On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
> >>
> >>
> >> On 30.04.2020 20:30, Daniel Vetter wrote:
> >>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>>>
> >>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>>>
> >>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>>>
> >>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>>>> ---
> >>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> index 35c2719..901b078 100644
> >>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> {
> >>>>>> struct drm_mode_obj_get_properties *arg = data;
> >>>>>> struct drm_mode_object *obj;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = 0;
> >>>>>>
> >>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>>>> return -EOPNOTSUPP;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>
> >>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>>>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>>>
> >>>>> Currently only six users... but there are ~60 calls to
> >>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>>>> if this will come back and haunt us.
> >>>>>
> >>>>
> >>>> What's the alternative? Seems like the options without the macros is
> >>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >>>> everywhere (and hope the copy source is done correctly).
> >>>
> >>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> >>> option we could come up with. You can't make it a function because of
> >>> the control flow. You don't want to open code this because it's tricky
> >>> to get right, if all you want is to just grab all locks. But it is
> >>> magic hidden behind a macro, which occasionally ends up hurting.
> >>> -Daniel
> >> So what are we doing with this problem? Should we replace at once approx. 60 calls?
> >
> > I'm confused by your question - dradual conversion is entirely orthogonal
> > to what exactly we're converting too. All I added here is that we've
> > discussed this at length, and the macro is the best thing we've come up
> > with. I still think it's the best compromise.
> >
> > Flag-day conversion for over 60 calls doesn't work, no matter what.
> > -Daniel
> >
> I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Patch looks good and passed CI, so I went ahead and applied it.
Thanks, Daniel
>
> Thanks,
> Michal
> >>
> >> Michal
> >>>
> >>>> Sean
> >>>>
> >>>>> BR,
> >>>>> Jani.
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>>>> if (!obj) {
> >>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> out_unref:
> >>>>>> drm_mode_object_put(obj);
> >>>>>> out:
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>> return ret;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> {
> >>>>>> struct drm_device *dev = prop->dev;
> >>>>>> struct drm_mode_object *ref;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = -EINVAL;
> >>>>>>
> >>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>> switch (obj->type) {
> >>>>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> break;
> >>>>>> }
> >>>>>> drm_property_change_valid_put(prop, ref);
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>>
> >>>>>> return ret;
> >>>>>> }
> >>>>>
> >>>>> --
> >>>>> Jani Nikula, Intel Open Source Graphics Center
> >>>
> >>>
> >>>
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
@ 2020-05-05 8:51 ` Daniel Vetter
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2020-05-05 8:51 UTC (permalink / raw)
To: Michał Orzeł
Cc: dri-devel, Dave Airlie, Intel Graphics Development,
Linux Kernel Mailing List, Sean Paul, Thomas Zimmermann
On Tue, May 05, 2020 at 07:55:00AM +0200, Michał Orzeł wrote:
>
>
> On 04.05.2020 13:53, Daniel Vetter wrote:
> > On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote:
> >>
> >>
> >> On 30.04.2020 20:30, Daniel Vetter wrote:
> >>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@chromium.org> wrote:
> >>>>
> >>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>>>
> >>>>> On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@gmail.com> wrote:
> >>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace
> >>>>>> the deprecated functions that take/drop modeset locks with new helpers.
> >>>>>>
> >>>>>> Signed-off-by: Michal Orzel <michalorzel.eng@gmail.com>
> >>>>>> ---
> >>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> index 35c2719..901b078 100644
> >>>>>> --- a/drivers/gpu/drm/drm_mode_object.c
> >>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c
> >>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> {
> >>>>>> struct drm_mode_obj_get_properties *arg = data;
> >>>>>> struct drm_mode_object *obj;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = 0;
> >>>>>>
> >>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>>>> return -EOPNOTSUPP;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>
> >>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> >>>>> DRM_MODESET_LOCK_ALL_END macros. :(
> >>>>>
> >>>>> Currently only six users... but there are ~60 calls to
> >>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> >>>>> if this will come back and haunt us.
> >>>>>
> >>>>
> >>>> What's the alternative? Seems like the options without the macros is
> >>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> >>>> everywhere (and hope the copy source is done correctly).
> >>>
> >>> Yeah Sean & me had a bunch of bikesheds and this is the least worst
> >>> option we could come up with. You can't make it a function because of
> >>> the control flow. You don't want to open code this because it's tricky
> >>> to get right, if all you want is to just grab all locks. But it is
> >>> magic hidden behind a macro, which occasionally ends up hurting.
> >>> -Daniel
> >> So what are we doing with this problem? Should we replace at once approx. 60 calls?
> >
> > I'm confused by your question - dradual conversion is entirely orthogonal
> > to what exactly we're converting too. All I added here is that we've
> > discussed this at length, and the macro is the best thing we've come up
> > with. I still think it's the best compromise.
> >
> > Flag-day conversion for over 60 calls doesn't work, no matter what.
> > -Daniel
> >
> I agree with that. All I wanted to ask was whether I should add something additional to this patch or not.
Patch looks good and passed CI, so I went ahead and applied it.
Thanks, Daniel
>
> Thanks,
> Michal
> >>
> >> Michal
> >>>
> >>>> Sean
> >>>>
> >>>>> BR,
> >>>>> Jani.
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> >>>>>> if (!obj) {
> >>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> >>>>>> out_unref:
> >>>>>> drm_mode_object_put(obj);
> >>>>>> out:
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>> return ret;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> {
> >>>>>> struct drm_device *dev = prop->dev;
> >>>>>> struct drm_mode_object *ref;
> >>>>>> + struct drm_modeset_acquire_ctx ctx;
> >>>>>> int ret = -EINVAL;
> >>>>>>
> >>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> - drm_modeset_lock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >>>>>> switch (obj->type) {
> >>>>>> case DRM_MODE_OBJECT_CONNECTOR:
> >>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> >>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> >>>>>> break;
> >>>>>> }
> >>>>>> drm_property_change_valid_put(prop, ref);
> >>>>>> - drm_modeset_unlock_all(dev);
> >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret);
> >>>>>>
> >>>>>> return ret;
> >>>>>> }
> >>>>>
> >>>>> --
> >>>>> Jani Nikula, Intel Open Source Graphics Center
> >>>
> >>>
> >>>
> >
--
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] 34+ messages in thread