* [PATCH v3 0/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*()
@ 2021-04-21 11:37 ` Fabio M. De Francesco
0 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem. The new API has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context. This has the advantage of reducing boilerplate.
The work is carried out in two consecutive logical steps: the first patch
converts the code to use drm_modeset_lock_all(), then the second does the
final replace with DRM_MODESET_LOCK_ALL_*().
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
Fabio M. De Francesco (2):
drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with
drm_modeset_lock_all_ctx()
drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with
DRM_MODESET_LOCK_ALL_*()
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--
2.31.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 0/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*()
@ 2021-04-21 11:37 ` Fabio M. De Francesco
0 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
Replace the deprecated API with new helpers, according to the TODO list
of the DRM subsystem. The new API has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context. This has the advantage of reducing boilerplate.
The work is carried out in two consecutive logical steps: the first patch
converts the code to use drm_modeset_lock_all(), then the second does the
final replace with DRM_MODESET_LOCK_ALL_*().
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
Fabio M. De Francesco (2):
drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with
drm_modeset_lock_all_ctx()
drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with
DRM_MODESET_LOCK_ALL_*()
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()
2021-04-21 11:37 ` Fabio M. De Francesco
@ 2021-04-21 11:37 ` Fabio M. De Francesco
-1 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
Replace the deprecated API with new helpers, according to the
TODO list of the DRM subsystem.
In this first patch drm_modeset_lock_all() is replaced with
drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(),
the latter doesn’t take the global drm_mode_config.mutex
since that lock isn’t required for modeset state changes.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..856903db34c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
-
- drm_modeset_lock_all(drm_dev);
+ struct drm_modeset_acquire_ctx ctx;
+ int ret_lock = 0;
+
+retry:
+ drm_modeset_lock_all_ctx(drm_dev, &ctx);
+ if(ret_lock == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
+ }
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_unlock_all(drm_dev);
+ drm_modeset_drop_locks(&ctx);
} else {
struct drm_connector *list_connector;
--
2.31.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()
@ 2021-04-21 11:37 ` Fabio M. De Francesco
0 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
Replace the deprecated API with new helpers, according to the
TODO list of the DRM subsystem.
In this first patch drm_modeset_lock_all() is replaced with
drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(),
the latter doesn’t take the global drm_mode_config.mutex
since that lock isn’t required for modeset state changes.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 671ec1002230..856903db34c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
if (amdgpu_device_has_dc_support(adev)) {
struct drm_crtc *crtc;
-
- drm_modeset_lock_all(drm_dev);
+ struct drm_modeset_acquire_ctx ctx;
+ int ret_lock = 0;
+
+retry:
+ drm_modeset_lock_all_ctx(drm_dev, &ctx);
+ if(ret_lock == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
+ }
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_unlock_all(drm_dev);
+ drm_modeset_drop_locks(&ctx);
} else {
struct drm_connector *list_connector;
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*()
2021-04-21 11:37 ` Fabio M. De Francesco
@ 2021-04-21 11:37 ` Fabio M. De Francesco
-1 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
This second patch makes use of the API that has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 856903db34c5..43dd77c227ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1441,12 +1441,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
struct drm_modeset_acquire_ctx ctx;
int ret_lock = 0;
-retry:
- drm_modeset_lock_all_ctx(drm_dev, &ctx);
- if(ret_lock == -EDEADLK) {
- drm_modeset_backoff(&ctx);
- goto retry;
- }
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1455,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_drop_locks(&ctx);
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else {
struct drm_connector *list_connector;
--
2.31.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*()
@ 2021-04-21 11:37 ` Fabio M. De Francesco
0 siblings, 0 replies; 8+ messages in thread
From: Fabio M. De Francesco @ 2021-04-21 11:37 UTC (permalink / raw
To: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
Cc: Fabio M. De Francesco
This second patch makes use of the API that has been introduced with commit
b7ea04d299c7: DRM_MODESET_LOCK_ALL_BEGIN() simplifies grabbing all modeset
locks using a local context and has the advantage of reducing boilerplate.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
Changes from v2: The work is split in two consecutive logical steps.
Changes from v1: Added further information to the commit message.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 856903db34c5..43dd77c227ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1441,12 +1441,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
struct drm_modeset_acquire_ctx ctx;
int ret_lock = 0;
-retry:
- drm_modeset_lock_all_ctx(drm_dev, &ctx);
- if(ret_lock == -EDEADLK) {
- drm_modeset_backoff(&ctx);
- goto retry;
- }
+ DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret_lock);
drm_for_each_crtc(crtc, drm_dev) {
if (crtc->state->active) {
@@ -1455,7 +1450,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
}
}
- drm_modeset_drop_locks(&ctx);
+ DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret_lock);
} else {
struct drm_connector *list_connector;
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()
2021-04-21 11:37 ` Fabio M. De Francesco
@ 2021-04-22 9:46 ` Daniel Vetter
-1 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2021-04-22 9:46 UTC (permalink / raw
To: Fabio M. De Francesco
Cc: David Airlie, dri-devel, Melissa Wen, outreachy-kernel,
Alex Deucher, Christian König
On Wed, Apr 21, 2021 at 01:37:15PM +0200, Fabio M. De Francesco wrote:
> Replace the deprecated API with new helpers, according to the
> TODO list of the DRM subsystem.
>
> In this first patch drm_modeset_lock_all() is replaced with
> drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(),
> the latter doesn’t take the global drm_mode_config.mutex
> since that lock isn’t required for modeset state changes.
So this is only true for core modeset code, not necessarily for drivers.
So needs to be audited in each case.
Now loocking at the precise code you're touching here the situation is a
lot more specific:
- We are looping over all the crtc. This list never changes, so no locks
needed.
- Then we look at crtc->state, which is proteced by crtc->mutex. So that's
the only lock we need, and we only need to hold a single one (so no
EDEADLCK retry loop needed due to multiple lock acquisition).
So I think the right fix here is to just grab the crtc->mutex lock right
around the access to crtc->state with drm_modeset_lock(). And ditch the
lock_all and all its complexity completely.
tldr; locking is complicated :-)
Can you pls respin?
Thanks, Daniel
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>
> Changes from v2: The work is split in two consecutive logical steps.
> Changes from v1: Added further information to the commit message.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 671ec1002230..856903db34c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>
> if (amdgpu_device_has_dc_support(adev)) {
> struct drm_crtc *crtc;
> -
> - drm_modeset_lock_all(drm_dev);
> + struct drm_modeset_acquire_ctx ctx;
> + int ret_lock = 0;
> +
> +retry:
> + drm_modeset_lock_all_ctx(drm_dev, &ctx);
> + if(ret_lock == -EDEADLK) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> + }
>
> drm_for_each_crtc(crtc, drm_dev) {
> if (crtc->state->active) {
> @@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
> }
> }
>
> - drm_modeset_unlock_all(drm_dev);
> + drm_modeset_drop_locks(&ctx);
>
> } else {
> struct drm_connector *list_connector;
> --
> 2.31.1
>
--
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] 8+ messages in thread
* Re: [PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx()
@ 2021-04-22 9:46 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2021-04-22 9:46 UTC (permalink / raw
To: Fabio M. De Francesco
Cc: outreachy-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie, Daniel Vetter, Melissa Wen
On Wed, Apr 21, 2021 at 01:37:15PM +0200, Fabio M. De Francesco wrote:
> Replace the deprecated API with new helpers, according to the
> TODO list of the DRM subsystem.
>
> In this first patch drm_modeset_lock_all() is replaced with
> drm_modeset_lock_all_ctx(). Unlike drm_modeset_lock_all(),
> the latter doesn’t take the global drm_mode_config.mutex
> since that lock isn’t required for modeset state changes.
So this is only true for core modeset code, not necessarily for drivers.
So needs to be audited in each case.
Now loocking at the precise code you're touching here the situation is a
lot more specific:
- We are looping over all the crtc. This list never changes, so no locks
needed.
- Then we look at crtc->state, which is proteced by crtc->mutex. So that's
the only lock we need, and we only need to hold a single one (so no
EDEADLCK retry loop needed due to multiple lock acquisition).
So I think the right fix here is to just grab the crtc->mutex lock right
around the access to crtc->state with drm_modeset_lock(). And ditch the
lock_all and all its complexity completely.
tldr; locking is complicated :-)
Can you pls respin?
Thanks, Daniel
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>
> Changes from v2: The work is split in two consecutive logical steps.
> Changes from v1: Added further information to the commit message.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 671ec1002230..856903db34c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1438,8 +1438,15 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>
> if (amdgpu_device_has_dc_support(adev)) {
> struct drm_crtc *crtc;
> -
> - drm_modeset_lock_all(drm_dev);
> + struct drm_modeset_acquire_ctx ctx;
> + int ret_lock = 0;
> +
> +retry:
> + drm_modeset_lock_all_ctx(drm_dev, &ctx);
> + if(ret_lock == -EDEADLK) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> + }
>
> drm_for_each_crtc(crtc, drm_dev) {
> if (crtc->state->active) {
> @@ -1448,7 +1455,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
> }
> }
>
> - drm_modeset_unlock_all(drm_dev);
> + drm_modeset_drop_locks(&ctx);
>
> } else {
> struct drm_connector *list_connector;
> --
> 2.31.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-22 9:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-21 11:37 [PATCH v3 0/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all() with DRM_MODESET_LOCK_ALL_*() Fabio M. De Francesco
2021-04-21 11:37 ` Fabio M. De Francesco
2021-04-21 11:37 ` [PATCH v3 1/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_lock_all() with drm_modeset_lock_all_ctx() Fabio M. De Francesco
2021-04-21 11:37 ` Fabio M. De Francesco
2021-04-22 9:46 ` Daniel Vetter
2021-04-22 9:46 ` Daniel Vetter
2021-04-21 11:37 ` [PATCH v3 2/2] drm/amd/amdgpu/amdgpu_drv.c: Replace drm_modeset_*_all_ctx() with DRM_MODESET_LOCK_ALL_*() Fabio M. De Francesco
2021-04-21 11:37 ` Fabio M. De Francesco
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.