* Re: [PATCH] drm/vmwgfx: fix lock breakage
2014-10-30 17:39 [PATCH] drm/vmwgfx: fix lock breakage Rob Clark
@ 2014-10-30 17:40 ` Rob Clark
2014-10-31 0:19 ` Jakob Bornecrantz
2014-10-31 8:24 ` Thomas Hellstrom
2014-10-31 17:35 ` Daniel Vetter
2 siblings, 1 reply; 5+ messages in thread
From: Rob Clark @ 2014-10-30 17:40 UTC (permalink / raw)
To: dri-devel@lists.freedesktop.org; +Cc: Thomas Hellstrom
On Thu, Oct 30, 2014 at 1:39 PM, Rob Clark <robdclark@gmail.com> wrote:
> After:
>
> commit d059f652e73c35678d28d4cd09ab2cec89696af9
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> AuthorDate: Fri Jul 25 18:07:40 2014 +0200
>
> drm: Handle legacy per-crtc locking with full acquire ctx
>
> drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc()
> which uses full aquire ctx. So dropping/reaquiring the lock via
> drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep
> kindly points out.
>
> The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all
> for cursor updates still apply.
>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1155825
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index d2bc2b0..8fc1e38 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
>
> /* A lot of the code assumes this */
> @@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> ret = 0;
> out:
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return ret;
> }
> @@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
>
> vmw_cursor_update_position(dev_priv, shown,
> @@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> du->cursor_y + du->hotspot_y);
>
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return 0;
> }
> --
> 1.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vmwgfx: fix lock breakage
2014-10-30 17:40 ` Rob Clark
@ 2014-10-31 0:19 ` Jakob Bornecrantz
0 siblings, 0 replies; 5+ messages in thread
From: Jakob Bornecrantz @ 2014-10-31 0:19 UTC (permalink / raw)
To: Rob Clark; +Cc: Thomas Hellstrom, dri-devel@lists.freedesktop.org
On Thu, Oct 30, 2014 at 6:40 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Oct 30, 2014 at 1:39 PM, Rob Clark <robdclark@gmail.com> wrote:
>> After:
>>
>> commit d059f652e73c35678d28d4cd09ab2cec89696af9
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> AuthorDate: Fri Jul 25 18:07:40 2014 +0200
>>
>> drm: Handle legacy per-crtc locking with full acquire ctx
>>
>> drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc()
>> which uses full aquire ctx. So dropping/reaquiring the lock via
>> drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep
>> kindly points out.
>>
>> The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all
>> for cursor updates still apply.
>>
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1155825
>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Jakob Bornecrantz <jakob@vmware.com>
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> index d2bc2b0..8fc1e38 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
>> @@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>> * can do this since the caller in the drm core doesn't check anything
>> * which is protected by any looks.
>> */
>> - drm_modeset_unlock(&crtc->mutex);
>> + drm_modeset_unlock_crtc(crtc);
>> drm_modeset_lock_all(dev_priv->dev);
>>
>> /* A lot of the code assumes this */
>> @@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>> ret = 0;
>> out:
>> drm_modeset_unlock_all(dev_priv->dev);
>> - drm_modeset_lock(&crtc->mutex, NULL);
>> + drm_modeset_lock_crtc(crtc);
>>
>> return ret;
>> }
>> @@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>> * can do this since the caller in the drm core doesn't check anything
>> * which is protected by any looks.
>> */
>> - drm_modeset_unlock(&crtc->mutex);
>> + drm_modeset_unlock_crtc(crtc);
>> drm_modeset_lock_all(dev_priv->dev);
>>
>> vmw_cursor_update_position(dev_priv, shown,
>> @@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>> du->cursor_y + du->hotspot_y);
>>
>> drm_modeset_unlock_all(dev_priv->dev);
>> - drm_modeset_lock(&crtc->mutex, NULL);
>> + drm_modeset_lock_crtc(crtc);
>>
>> return 0;
>> }
>> --
>> 1.9.3
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vmwgfx: fix lock breakage
2014-10-30 17:39 [PATCH] drm/vmwgfx: fix lock breakage Rob Clark
2014-10-30 17:40 ` Rob Clark
@ 2014-10-31 8:24 ` Thomas Hellstrom
2014-10-31 17:35 ` Daniel Vetter
2 siblings, 0 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2014-10-31 8:24 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On 10/30/2014 06:39 PM, Rob Clark wrote:
> After:
>
> commit d059f652e73c35678d28d4cd09ab2cec89696af9
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> AuthorDate: Fri Jul 25 18:07:40 2014 +0200
>
> drm: Handle legacy per-crtc locking with full acquire ctx
>
> drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc()
> which uses full aquire ctx. So dropping/reaquiring the lock via
> drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep
> kindly points out.
>
> The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all
> for cursor updates still apply.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
Tested-by: Thomas Hellstrom <thellstrom@vmware.com>
Thanks, Rob.
/Thomas
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index d2bc2b0..8fc1e38 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
>
> /* A lot of the code assumes this */
> @@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> ret = 0;
> out:
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return ret;
> }
> @@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
>
> vmw_cursor_update_position(dev_priv, shown,
> @@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> du->cursor_y + du->hotspot_y);
>
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return 0;
> }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vmwgfx: fix lock breakage
2014-10-30 17:39 [PATCH] drm/vmwgfx: fix lock breakage Rob Clark
2014-10-30 17:40 ` Rob Clark
2014-10-31 8:24 ` Thomas Hellstrom
@ 2014-10-31 17:35 ` Daniel Vetter
2 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-10-31 17:35 UTC (permalink / raw)
To: Rob Clark; +Cc: Thomas Hellstrom, dri-devel
On Thu, Oct 30, 2014 at 01:39:04PM -0400, Rob Clark wrote:
> After:
>
> commit d059f652e73c35678d28d4cd09ab2cec89696af9
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> AuthorDate: Fri Jul 25 18:07:40 2014 +0200
>
> drm: Handle legacy per-crtc locking with full acquire ctx
>
> drm_mode_cursor_common() was switched to use drm_modeset_(un)lock_crtc()
> which uses full aquire ctx. So dropping/reaquiring the lock via
> drm_modeset_(un)lock() directly isn't the right thing to do, as lockdep
> kindly points out.
>
> The 'FIXME's about sorting out whether vmwgfx *really* needs to lock-all
> for cursor updates still apply.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index d2bc2b0..8fc1e38 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
Oh right this is simpler, but it also grabs a new acquire context. So not
too terribly fair really, but otoh neither was the old one.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> /* A lot of the code assumes this */
> @@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> ret = 0;
> out:
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return ret;
> }
> @@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> * can do this since the caller in the drm core doesn't check anything
> * which is protected by any looks.
> */
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_unlock_crtc(crtc);
> drm_modeset_lock_all(dev_priv->dev);
>
> vmw_cursor_update_position(dev_priv, shown,
> @@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> du->cursor_y + du->hotspot_y);
>
> drm_modeset_unlock_all(dev_priv->dev);
> - drm_modeset_lock(&crtc->mutex, NULL);
> + drm_modeset_lock_crtc(crtc);
>
> return 0;
> }
> --
> 1.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread