* [PATCH] drm/kms/plane: Avoiding redundant checks in case of EDEADLK retries
[not found] <CGME20180730055515epcas5p3358ce6d5e80b66256ea92aa5659f74bb@epcas5p3.samsung.com>
@ 2018-07-30 5:18 ` Satendra Singh Thakur
2018-07-30 7:53 ` Maarten Lankhorst
0 siblings, 1 reply; 2+ messages in thread
From: Satendra Singh Thakur @ 2018-07-30 5:18 UTC (permalink / raw)
To: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: vineet.j, hemanshu.s, nishant.y08, sst2005, Satendra Singh Thakur
1. Currently, if modeset_lock is re-tried many times in case of EDEADLK
error, then this will be the code flow
retry:
ret = drm_modeset_lock();
if (ret)-->this is true
goto out;
out:
if (fb)
if (plane->old_fb)
if (ret == -EDEADLK)
goto retry;
It can be observed that checks on fb, old_fb are redundant
in retry-case.
If we keep if (ret == -EDEADLK) right after the out label,
that will avoid redundant checks.
It won't affect normal scenario (non-retry case).
2. Moved NULL assignment inside if statement related to NULL check
Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
---
drivers/gpu/drm/drm_plane.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0350544..ed42cd4 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -658,9 +658,10 @@ static int __setplane_internal(struct drm_plane *plane,
}
out:
- if (plane->old_fb)
+ if (plane->old_fb) {
drm_framebuffer_put(plane->old_fb);
- plane->old_fb = NULL;
+ plane->old_fb = NULL;
+ }
return ret;
}
@@ -1097,17 +1098,17 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
}
out:
- if (fb)
- drm_framebuffer_put(fb);
- if (plane->old_fb)
- drm_framebuffer_put(plane->old_fb);
- plane->old_fb = NULL;
-
if (ret == -EDEADLK) {
ret = drm_modeset_backoff(&ctx);
if (!ret)
goto retry;
}
+ if (fb)
+ drm_framebuffer_put(fb);
+ if (plane->old_fb) {
+ drm_framebuffer_put(plane->old_fb);
+ plane->old_fb = NULL;
+ }
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drm/kms/plane: Avoiding redundant checks in case of EDEADLK retries
2018-07-30 5:18 ` [PATCH] drm/kms/plane: Avoiding redundant checks in case of EDEADLK retries Satendra Singh Thakur
@ 2018-07-30 7:53 ` Maarten Lankhorst
0 siblings, 0 replies; 2+ messages in thread
From: Maarten Lankhorst @ 2018-07-30 7:53 UTC (permalink / raw)
To: Satendra Singh Thakur, Gustavo Padovan, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: vineet.j, hemanshu.s, nishant.y08, sst2005
Op 30-07-18 om 07:18 schreef Satendra Singh Thakur:
> 1. Currently, if modeset_lock is re-tried many times in case of EDEADLK
> error, then this will be the code flow
>
> retry:
> ret = drm_modeset_lock();
>
> if (ret)-->this is true
> goto out;
>
> out:
> if (fb)
> if (plane->old_fb)
> if (ret == -EDEADLK)
> goto retry;
> It can be observed that checks on fb, old_fb are redundant
> in retry-case.
> If we keep if (ret == -EDEADLK) right after the out label,
> that will avoid redundant checks.
> It won't affect normal scenario (non-retry case).
> 2. Moved NULL assignment inside if statement related to NULL check
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> ---
> drivers/gpu/drm/drm_plane.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0350544..ed42cd4 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -658,9 +658,10 @@ static int __setplane_internal(struct drm_plane *plane,
> }
>
> out:
> - if (plane->old_fb)
> + if (plane->old_fb) {
> drm_framebuffer_put(plane->old_fb);
> - plane->old_fb = NULL;
> + plane->old_fb = NULL;
> + }
>
> return ret;
> }
> @@ -1097,17 +1098,17 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
>
> out:
> - if (fb)
> - drm_framebuffer_put(fb);
> - if (plane->old_fb)
> - drm_framebuffer_put(plane->old_fb);
> - plane->old_fb = NULL;
> -
> if (ret == -EDEADLK) {
> ret = drm_modeset_backoff(&ctx);
> if (!ret)
> goto retry;
> }
> + if (fb)
> + drm_framebuffer_put(fb);
> + if (plane->old_fb) {
> + drm_framebuffer_put(plane->old_fb);
> + plane->old_fb = NULL;
> + }
>
> drm_modeset_drop_locks(&ctx);
> drm_modeset_acquire_fini(&ctx);
It's not redundant. plane->old_fb is protected by the plane_mutex lock and should be
null before dropping the locks in drm_modeset_backoff(). Else other code will trip
with a WARN_ON(plane->old_fb != NULL);
Before sending any more patches I would strongly suggest setting
CONFIG_DEBUG_WW_MUTEX_SLOWPATH and CONFIG_PROVE_LOCKING in your kernel, and try to
run the IGT testsuite.
~Maarten
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-07-30 7:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180730055515epcas5p3358ce6d5e80b66256ea92aa5659f74bb@epcas5p3.samsung.com>
2018-07-30 5:18 ` [PATCH] drm/kms/plane: Avoiding redundant checks in case of EDEADLK retries Satendra Singh Thakur
2018-07-30 7:53 ` Maarten Lankhorst
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.