diff mbox

drm/vmwgfx: Fix fbdev emulation using legacy functions

Message ID 20170406200256.26040-1-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter April 6, 2017, 8:02 p.m. UTC
I've broken this by removing the backoff handling from the
set_config2atomic helper in

commit 38b6441e4e75c0b319cfe4d9364c1059fc1e3c2b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Mar 22 22:50:58 2017 +0100

    drm/atomic-helper: Remove the backoff hack from set_config

Fixing this properly would mean we get to wire the acquire_ctx all the
way through vmwgfx fbdev code, and doing the same was tricky for the
shared fbdev layer. Probably much better to look into refactoring the
entire code to use the helpers, but since that's not a viable
long-term solution fix the issue by open-coding a vmwgfx version of
set_config, that does the legacy backoff dance internally.

Note: Just compile-tested. The idea is to take
drm_mode_set_config_internal(), remove the "is this a legacy driver"
check, and whack the drm_atomic_legacy_backoff trickery at the end.
Since drm_atomic_legacy_backoff is for atomic commits only we need to
open-code it.

Cc: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 58 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

Comments

Thomas Hellstrom April 7, 2017, 8:31 a.m. UTC | #1
Hi, Daniel.

It appears to work fine. Thanks!

Do you want to take it through drm-misc or want me to take it throuch
vmwgfx-next?

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
/Thomas



On 04/06/2017 10:02 PM, Daniel Vetter wrote:
> I've broken this by removing the backoff handling from the
> set_config2atomic helper in
>
> commit 38b6441e4e75c0b319cfe4d9364c1059fc1e3c2b
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Mar 22 22:50:58 2017 +0100
>
>     drm/atomic-helper: Remove the backoff hack from set_config
>
> Fixing this properly would mean we get to wire the acquire_ctx all the
> way through vmwgfx fbdev code, and doing the same was tricky for the
> shared fbdev layer. Probably much better to look into refactoring the
> entire code to use the helpers, but since that's not a viable
> long-term solution fix the issue by open-coding a vmwgfx version of
> set_config, that does the legacy backoff dance internally.
>
> Note: Just compile-tested. The idea is to take
> drm_mode_set_config_internal(), remove the "is this a legacy driver"
> check, and whack the drm_atomic_legacy_backoff trickery at the end.
> Since drm_atomic_legacy_backoff is for atomic commits only we need to
> open-code it.
>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 58 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index 09e120d50e65..6f4cb4678cbc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -418,6 +418,60 @@ static int vmw_fb_compute_depth(struct fb_var_screeninfo *var,
>  	return 0;
>  }
>  
> +static int vmwgfx_set_config_internal(struct drm_mode_set *set)
> +{
> +	struct drm_crtc *crtc = set->crtc;
> +	struct drm_framebuffer *fb;
> +	struct drm_crtc *tmp;
> +	struct drm_modeset_acquire_ctx *ctx;
> +	struct drm_device *dev = set->crtc->dev;
> +	int ret;
> +
> +	ctx = dev->mode_config.acquire_ctx;
> +
> +restart:
> +	/*
> +	 * NOTE: ->set_config can also disable other crtcs (if we steal all
> +	 * connectors from it), hence we need to refcount the fbs across all
> +	 * crtcs. Atomic modeset will have saner semantics ...
> +	 */
> +	drm_for_each_crtc(tmp, dev)
> +		tmp->primary->old_fb = tmp->primary->fb;
> +
> +	fb = set->fb;
> +
> +	ret = crtc->funcs->set_config(set, ctx);
> +	if (ret == 0) {
> +		crtc->primary->crtc = crtc;
> +		crtc->primary->fb = fb;
> +	}
> +
> +	drm_for_each_crtc(tmp, dev) {
> +		if (tmp->primary->fb)
> +			drm_framebuffer_get(tmp->primary->fb);
> +		if (tmp->primary->old_fb)
> +			drm_framebuffer_put(tmp->primary->old_fb);
> +		tmp->primary->old_fb = NULL;
> +	}
> +
> +	if (ret == -EDEADLK) {
> +		dev->mode_config.acquire_ctx = NULL;
> +
> +retry_locking:
> +		drm_modeset_backoff(ctx);
> +
> +		ret = drm_modeset_lock_all_ctx(dev, ctx);
> +		if (ret)
> +			goto retry_locking;
> +
> +		dev->mode_config.acquire_ctx = ctx;
> +
> +		goto restart;
> +	}
> +
> +	return ret;
> +}
> +
>  static int vmw_fb_kms_detach(struct vmw_fb_par *par,
>  			     bool detach_bo,
>  			     bool unref_bo)
> @@ -436,7 +490,7 @@ static int vmw_fb_kms_detach(struct vmw_fb_par *par,
>  		set.fb = NULL;
>  		set.num_connectors = 0;
>  		set.connectors = &par->con;
> -		ret = drm_mode_set_config_internal(&set);
> +		ret = vmwgfx_set_config_internal(&set);
>  		if (ret) {
>  			DRM_ERROR("Could not unset a mode.\n");
>  			return ret;
> @@ -578,7 +632,7 @@ static int vmw_fb_set_par(struct fb_info *info)
>  	set.num_connectors = 1;
>  	set.connectors = &par->con;
>  
> -	ret = drm_mode_set_config_internal(&set);
> +	ret = vmwgfx_set_config_internal(&set);
>  	if (ret)
>  		goto out_unlock;
>
Daniel Vetter April 7, 2017, 2:54 p.m. UTC | #2
On Fri, Apr 07, 2017 at 10:31:29AM +0200, Thomas Hellstrom wrote:
> Hi, Daniel.
> 
> It appears to work fine. Thanks!

Oh sweet, tbh I didn't expect this kind of blind typing without a clear
theory to work out so well :-) Did you test this with
CONFIG_DEBUG_WW_MUTEX_SLOWPATH? Just to make sure I didn't botch up the
retry path.

> Do you want to take it through drm-misc or want me to take it throuch
> vmwgfx-next?

Sean will pick it up before he does the final 4.12 -misc pull later today,
so that this vmwgfx regression is resolved.

> Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

Thanks for testing&review.
-Daniel

> /Thomas
> 
> 
> 
> On 04/06/2017 10:02 PM, Daniel Vetter wrote:
> > I've broken this by removing the backoff handling from the
> > set_config2atomic helper in
> >
> > commit 38b6441e4e75c0b319cfe4d9364c1059fc1e3c2b
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Mar 22 22:50:58 2017 +0100
> >
> >     drm/atomic-helper: Remove the backoff hack from set_config
> >
> > Fixing this properly would mean we get to wire the acquire_ctx all the
> > way through vmwgfx fbdev code, and doing the same was tricky for the
> > shared fbdev layer. Probably much better to look into refactoring the
> > entire code to use the helpers, but since that's not a viable
> > long-term solution fix the issue by open-coding a vmwgfx version of
> > set_config, that does the legacy backoff dance internally.
> >
> > Note: Just compile-tested. The idea is to take
> > drm_mode_set_config_internal(), remove the "is this a legacy driver"
> > check, and whack the drm_atomic_legacy_backoff trickery at the end.
> > Since drm_atomic_legacy_backoff is for atomic commits only we need to
> > open-code it.
> >
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 58 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > index 09e120d50e65..6f4cb4678cbc 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > @@ -418,6 +418,60 @@ static int vmw_fb_compute_depth(struct fb_var_screeninfo *var,
> >  	return 0;
> >  }
> >  
> > +static int vmwgfx_set_config_internal(struct drm_mode_set *set)
> > +{
> > +	struct drm_crtc *crtc = set->crtc;
> > +	struct drm_framebuffer *fb;
> > +	struct drm_crtc *tmp;
> > +	struct drm_modeset_acquire_ctx *ctx;
> > +	struct drm_device *dev = set->crtc->dev;
> > +	int ret;
> > +
> > +	ctx = dev->mode_config.acquire_ctx;
> > +
> > +restart:
> > +	/*
> > +	 * NOTE: ->set_config can also disable other crtcs (if we steal all
> > +	 * connectors from it), hence we need to refcount the fbs across all
> > +	 * crtcs. Atomic modeset will have saner semantics ...
> > +	 */
> > +	drm_for_each_crtc(tmp, dev)
> > +		tmp->primary->old_fb = tmp->primary->fb;
> > +
> > +	fb = set->fb;
> > +
> > +	ret = crtc->funcs->set_config(set, ctx);
> > +	if (ret == 0) {
> > +		crtc->primary->crtc = crtc;
> > +		crtc->primary->fb = fb;
> > +	}
> > +
> > +	drm_for_each_crtc(tmp, dev) {
> > +		if (tmp->primary->fb)
> > +			drm_framebuffer_get(tmp->primary->fb);
> > +		if (tmp->primary->old_fb)
> > +			drm_framebuffer_put(tmp->primary->old_fb);
> > +		tmp->primary->old_fb = NULL;
> > +	}
> > +
> > +	if (ret == -EDEADLK) {
> > +		dev->mode_config.acquire_ctx = NULL;
> > +
> > +retry_locking:
> > +		drm_modeset_backoff(ctx);
> > +
> > +		ret = drm_modeset_lock_all_ctx(dev, ctx);
> > +		if (ret)
> > +			goto retry_locking;
> > +
> > +		dev->mode_config.acquire_ctx = ctx;
> > +
> > +		goto restart;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int vmw_fb_kms_detach(struct vmw_fb_par *par,
> >  			     bool detach_bo,
> >  			     bool unref_bo)
> > @@ -436,7 +490,7 @@ static int vmw_fb_kms_detach(struct vmw_fb_par *par,
> >  		set.fb = NULL;
> >  		set.num_connectors = 0;
> >  		set.connectors = &par->con;
> > -		ret = drm_mode_set_config_internal(&set);
> > +		ret = vmwgfx_set_config_internal(&set);
> >  		if (ret) {
> >  			DRM_ERROR("Could not unset a mode.\n");
> >  			return ret;
> > @@ -578,7 +632,7 @@ static int vmw_fb_set_par(struct fb_info *info)
> >  	set.num_connectors = 1;
> >  	set.connectors = &par->con;
> >  
> > -	ret = drm_mode_set_config_internal(&set);
> > +	ret = vmwgfx_set_config_internal(&set);
> >  	if (ret)
> >  		goto out_unlock;
> >  
> 
>
Sean Paul April 7, 2017, 5:34 p.m. UTC | #3
On Fri, Apr 07, 2017 at 10:31:29AM +0200, Thomas Hellstrom wrote:
> Hi, Daniel.
> 
> It appears to work fine. Thanks!
> 
> Do you want to take it through drm-misc or want me to take it throuch
> vmwgfx-next?

Applied to drm-misc

Thanks,

Sean

> 
> Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
> /Thomas
> 
> 
> 
> On 04/06/2017 10:02 PM, Daniel Vetter wrote:
> > I've broken this by removing the backoff handling from the
> > set_config2atomic helper in
> >
> > commit 38b6441e4e75c0b319cfe4d9364c1059fc1e3c2b
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Wed Mar 22 22:50:58 2017 +0100
> >
> >     drm/atomic-helper: Remove the backoff hack from set_config
> >
> > Fixing this properly would mean we get to wire the acquire_ctx all the
> > way through vmwgfx fbdev code, and doing the same was tricky for the
> > shared fbdev layer. Probably much better to look into refactoring the
> > entire code to use the helpers, but since that's not a viable
> > long-term solution fix the issue by open-coding a vmwgfx version of
> > set_config, that does the legacy backoff dance internally.
> >
> > Note: Just compile-tested. The idea is to take
> > drm_mode_set_config_internal(), remove the "is this a legacy driver"
> > check, and whack the drm_atomic_legacy_backoff trickery at the end.
> > Since drm_atomic_legacy_backoff is for atomic commits only we need to
> > open-code it.
> >
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 58 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > index 09e120d50e65..6f4cb4678cbc 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> > @@ -418,6 +418,60 @@ static int vmw_fb_compute_depth(struct fb_var_screeninfo *var,
> >  	return 0;
> >  }
> >  
> > +static int vmwgfx_set_config_internal(struct drm_mode_set *set)
> > +{
> > +	struct drm_crtc *crtc = set->crtc;
> > +	struct drm_framebuffer *fb;
> > +	struct drm_crtc *tmp;
> > +	struct drm_modeset_acquire_ctx *ctx;
> > +	struct drm_device *dev = set->crtc->dev;
> > +	int ret;
> > +
> > +	ctx = dev->mode_config.acquire_ctx;
> > +
> > +restart:
> > +	/*
> > +	 * NOTE: ->set_config can also disable other crtcs (if we steal all
> > +	 * connectors from it), hence we need to refcount the fbs across all
> > +	 * crtcs. Atomic modeset will have saner semantics ...
> > +	 */
> > +	drm_for_each_crtc(tmp, dev)
> > +		tmp->primary->old_fb = tmp->primary->fb;
> > +
> > +	fb = set->fb;
> > +
> > +	ret = crtc->funcs->set_config(set, ctx);
> > +	if (ret == 0) {
> > +		crtc->primary->crtc = crtc;
> > +		crtc->primary->fb = fb;
> > +	}
> > +
> > +	drm_for_each_crtc(tmp, dev) {
> > +		if (tmp->primary->fb)
> > +			drm_framebuffer_get(tmp->primary->fb);
> > +		if (tmp->primary->old_fb)
> > +			drm_framebuffer_put(tmp->primary->old_fb);
> > +		tmp->primary->old_fb = NULL;
> > +	}
> > +
> > +	if (ret == -EDEADLK) {
> > +		dev->mode_config.acquire_ctx = NULL;
> > +
> > +retry_locking:
> > +		drm_modeset_backoff(ctx);
> > +
> > +		ret = drm_modeset_lock_all_ctx(dev, ctx);
> > +		if (ret)
> > +			goto retry_locking;
> > +
> > +		dev->mode_config.acquire_ctx = ctx;
> > +
> > +		goto restart;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int vmw_fb_kms_detach(struct vmw_fb_par *par,
> >  			     bool detach_bo,
> >  			     bool unref_bo)
> > @@ -436,7 +490,7 @@ static int vmw_fb_kms_detach(struct vmw_fb_par *par,
> >  		set.fb = NULL;
> >  		set.num_connectors = 0;
> >  		set.connectors = &par->con;
> > -		ret = drm_mode_set_config_internal(&set);
> > +		ret = vmwgfx_set_config_internal(&set);
> >  		if (ret) {
> >  			DRM_ERROR("Could not unset a mode.\n");
> >  			return ret;
> > @@ -578,7 +632,7 @@ static int vmw_fb_set_par(struct fb_info *info)
> >  	set.num_connectors = 1;
> >  	set.connectors = &par->con;
> >  
> > -	ret = drm_mode_set_config_internal(&set);
> > +	ret = vmwgfx_set_config_internal(&set);
> >  	if (ret)
> >  		goto out_unlock;
> >  
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter June 21, 2017, 1:30 p.m. UTC | #4
On Thu, Apr 6, 2017 at 10:02 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Fixing this properly would mean we get to wire the acquire_ctx all the
> way through vmwgfx fbdev code, and doing the same was tricky for the
> shared fbdev layer. Probably much better to look into refactoring the
> entire code to use the helpers, but since that's not a viable
> long-term solution fix the issue by open-coding a vmwgfx version of
> set_config, that does the legacy backoff dance internally.

I'm digging around in the fbev locking again, and stumbled over this.
Any plans for vmwgfx to switch over to the core fbdev emulation
helpers? If not I'll have to put another layer of hacks around this,
and it's kinda getting unsightly :(

My goal is to remove any usage of the implicit
mode_config.acquire_context since that just breaks the wait/wound
mutex locking scheme and doesn't really work together with atomic
drivers. Once fbdev helpers and the property setting are fixed this
here is the last place (minus something in i915 that I'll take care of
of course). I'm getting close to that point.

Thanks, Daniel
Thomas Hellstrom June 26, 2017, 12:25 p.m. UTC | #5
Hi, Daniel,

On 06/21/2017 03:30 PM, Daniel Vetter wrote:
> On Thu, Apr 6, 2017 at 10:02 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> Fixing this properly would mean we get to wire the acquire_ctx all the
>> way through vmwgfx fbdev code, and doing the same was tricky for the
>> shared fbdev layer. Probably much better to look into refactoring the
>> entire code to use the helpers, but since that's not a viable
>> long-term solution fix the issue by open-coding a vmwgfx version of
>> set_config, that does the legacy backoff dance internally.
> I'm digging around in the fbev locking again, and stumbled over this.
> Any plans for vmwgfx to switch over to the core fbdev emulation
> helpers? If not I'll have to put another layer of hacks around this,
> and it's kinda getting unsightly :(

It's on our todo-list but I don't think any work on this will take place 
during summer, because of vacations etc.

Thanks,
Thomas



>
> My goal is to remove any usage of the implicit
> mode_config.acquire_context since that just breaks the wait/wound
> mutex locking scheme and doesn't really work together with atomic
> drivers. Once fbdev helpers and the property setting are fixed this
> here is the last place (minus something in i915 that I'll take care of
> of course). I'm getting close to that point.
>
> Thanks, Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index 09e120d50e65..6f4cb4678cbc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -418,6 +418,60 @@  static int vmw_fb_compute_depth(struct fb_var_screeninfo *var,
 	return 0;
 }
 
+static int vmwgfx_set_config_internal(struct drm_mode_set *set)
+{
+	struct drm_crtc *crtc = set->crtc;
+	struct drm_framebuffer *fb;
+	struct drm_crtc *tmp;
+	struct drm_modeset_acquire_ctx *ctx;
+	struct drm_device *dev = set->crtc->dev;
+	int ret;
+
+	ctx = dev->mode_config.acquire_ctx;
+
+restart:
+	/*
+	 * NOTE: ->set_config can also disable other crtcs (if we steal all
+	 * connectors from it), hence we need to refcount the fbs across all
+	 * crtcs. Atomic modeset will have saner semantics ...
+	 */
+	drm_for_each_crtc(tmp, dev)
+		tmp->primary->old_fb = tmp->primary->fb;
+
+	fb = set->fb;
+
+	ret = crtc->funcs->set_config(set, ctx);
+	if (ret == 0) {
+		crtc->primary->crtc = crtc;
+		crtc->primary->fb = fb;
+	}
+
+	drm_for_each_crtc(tmp, dev) {
+		if (tmp->primary->fb)
+			drm_framebuffer_get(tmp->primary->fb);
+		if (tmp->primary->old_fb)
+			drm_framebuffer_put(tmp->primary->old_fb);
+		tmp->primary->old_fb = NULL;
+	}
+
+	if (ret == -EDEADLK) {
+		dev->mode_config.acquire_ctx = NULL;
+
+retry_locking:
+		drm_modeset_backoff(ctx);
+
+		ret = drm_modeset_lock_all_ctx(dev, ctx);
+		if (ret)
+			goto retry_locking;
+
+		dev->mode_config.acquire_ctx = ctx;
+
+		goto restart;
+	}
+
+	return ret;
+}
+
 static int vmw_fb_kms_detach(struct vmw_fb_par *par,
 			     bool detach_bo,
 			     bool unref_bo)
@@ -436,7 +490,7 @@  static int vmw_fb_kms_detach(struct vmw_fb_par *par,
 		set.fb = NULL;
 		set.num_connectors = 0;
 		set.connectors = &par->con;
-		ret = drm_mode_set_config_internal(&set);
+		ret = vmwgfx_set_config_internal(&set);
 		if (ret) {
 			DRM_ERROR("Could not unset a mode.\n");
 			return ret;
@@ -578,7 +632,7 @@  static int vmw_fb_set_par(struct fb_info *info)
 	set.num_connectors = 1;
 	set.connectors = &par->con;
 
-	ret = drm_mode_set_config_internal(&set);
+	ret = vmwgfx_set_config_internal(&set);
 	if (ret)
 		goto out_unlock;