[PATCH 2/4] drm/fbdev-helper: don't force restores

[Daniel Vetter]

On Tue, Jan 28, 2020 at 12:52:05PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 28.01.2020 11.45, skrev Daniel Vetter:
> > Instead check for master status, in case we've raced.
> > 
> > This is the last exception to the general rule that we restore fbcon
> > only when there's no master active. Compositors are supposed to drop
> > their master status before they switch to a different console back to
> > text mode (or just switch to text mode directly, without a vt switch).
> > 
> > This is known to break some subtests of kms_fbcon_fbt in igt, but they're
> > just wrong - it does a graphics/text mode switch for the vt without
> > updating the master status.
> > 
> > Also add a comment to the drm_client->restore hook that this is expected
> > going forward from all clients (there's currently just one).
> > 
> > v2: Also drop the force in pan_display
> > 
> > Cc: Noralf Trønnes <noralf at tronnes.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 14 ++------------
> >  include/drm/drm_client.h        |  5 +++++
> >  2 files changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 4c7cbce7bae7..926187a82255 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -250,17 +250,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> >  		return 0;
> >  
> >  	mutex_lock(&fb_helper->lock);
> > -	/*
> > -	 * TODO:
> > -	 * We should bail out here if there is a master by dropping _force.
> > -	 * Currently these igt tests fail if we do that:
> > -	 * - kms_fbcon_fbt at psr
> > -	 * - kms_fbcon_fbt at psr-suspend
> > -	 *
> > -	 * So first these tests need to be fixed so they drop master or don't
> > -	 * have an fd open.
> > -	 */
> > -	ret = drm_client_modeset_commit_force(&fb_helper->client);
> > +	ret = drm_client_modeset_commit(&fb_helper->client);
> >  
> >  	do_delayed = fb_helper->delayed_hotplug;
> >  	if (do_delayed)
> > @@ -1357,7 +1347,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
> >  
> >  	pan_set(fb_helper, var->xoffset, var->yoffset);
> >  
> > -	ret = drm_client_modeset_commit_force(&fb_helper->client);
> > +	ret = drm_client_modeset_commit(&fb_helper->client);
> 
> This needs _force because drm_fb_helper_pan_display() already holds the
> locks.

Geez, now I remember again why I did _not_ include this in v1 :-/

> With that fixed:
> Reviewed-by: Noralf Trønnes <noralf at tronnes.org>
> 
> Maybe a better and more descriptive name would have been
> drm_client_modeset_commit_locked().

This sounds like a good idea, I'll do a patch for this. I'll need to
resend the series anyway to be able to co-test it with the igt side fix.

Thanks for your review.
-Daniel

> 
> Noralf.
> 
> >  	if (!ret) {
> >  		info->var.xoffset = var->xoffset;
> >  		info->var.yoffset = var->yoffset;
> > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> > index 5cf2c5dd8b1e..d01d311023ac 100644
> > --- a/include/drm/drm_client.h
> > +++ b/include/drm/drm_client.h
> > @@ -44,6 +44,11 @@ struct drm_client_funcs {
> >  	 * returns zero gets the privilege to restore and no more clients are
> >  	 * called. This callback is not called after @unregister has been called.
> >  	 *
> > +	 * Note that the core does not guarantee exclusion against concurrent
> > +	 * drm_open(). Clients need to ensure this themselves, for example by
> > +	 * using drm_master_internal_acquire() and
> > +	 * drm_master_internal_release().
> > +	 *
> >  	 * This callback is optional.
> >  	 */
> >  	int (*restore)(struct drm_client_dev *client);
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch