[Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Sun, Oct 23, 2016 at 11:12:31PM -0700, Manasi Navare wrote:
> On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote:
> > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote:
> > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > > > > This function provides a way for the driver to redo a
> > > > > > modeset on the current mode and retry the link training
> > > > > > at a lower link rate/lane count/bpp. This will get called
> > > > > > incase the link training fails during the current modeset.
> > > > > > 
> > > > > > Cc: dri-devel at lists.freedesktop.org
> > > > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > > > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > > > >  2 files changed, 59 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > index f936276..0c1614e 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > > > > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > > > > >  
> > > > > >  /**
> > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > > > > + * @connector: DRM connector
> > > > > > + *
> > > > > > + * Provides a way to redo a modeset with the current mode so that it can
> > > > > > + * drop the bpp, link rate/lane count and retry the link training.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * Returns 0 on success, negative errno numbers on failure.
> > > > > > + */
> > > > > > +int
> > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > > > > +{
> > > > > > +	struct drm_device *dev = connector->dev;
> > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > +	struct drm_atomic_state *state;
> > > > > > +	struct drm_connector_state *connector_state;
> > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > > > +	state = drm_atomic_state_alloc(dev);
> > > > > > +	if (!state) {
> > > > > > +		ret = -ENOMEM;
> > > > > > +		goto fail;
> > > > > > +	}
> > > > > > +	state->acquire_ctx = &ctx;
> > > > > > +retry:
> > > > > > +	ret = 0;
> > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > +		ret = PTR_ERR(connector_state);
> > > > > > +		goto fail;
> > > > > > +	}
> > > > > > +	if (!connector_state->crtc)
> > > > > > +		goto fail;
> > > > > > +
> > > > > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > > > > +							connector_state->crtc);
> > > > > > +	crtc_state->connectors_changed = true;
> > > > > 
> > > > > This line here only works if the driver uses the helpers for checking and
> > > > > commit, and when it doesn't overwrite connectors_changed. I think the
> > > > > kerneldoc should mention this.
> > > > > 
> > > > > The other issue: You cannot call this from modeset code itself because of
> > > > > recursion. I think this also must be mentioned.
> 
> I will add this to the kernel doc.
> In our case, we call this in a work func that will get scheduled after
> the current modeset is completed.
> 
> > > > 
> > > > And if this indeed does a modeset then we have again the same issue as
> > > > with upfront link training when the pipe is supposed to be on: Userspace
> > > > can observe the kernel playing tricks because the vblank support will be
> > > > temporarily disabled.
> > > > 
> > > > Not sure how to best fix this, but might be good to grab the crtc->mutex
> > > > in the vblank code for stuff like this.
> > > 
> > > We're going to have the same problem with async modeset as well.
> > 
> > Async modeset is ok because userspace expects that the pipe will go
> > off/on, and the kernel will send out an event to signal when the pipe is
> > guaranteed to be on and can be started to be used. It's random modesets
> > afterwards I'm concerned about.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> These wont be random modesets, this will occur only in case of link training
> failure in which case if the mode has not changed/pruned, it will attempt
> modeset at lower link rate.

"Cat ate the cable" is very much random from userspace's pov. It's not
random from the kernel's pov, because the kernel is aware of what's going
on - it just tried to (re)train the link. But userspace has no idea at
all. Note that link training failures can not just happen right after a
modeset, but also:
- anytime the sink feels like the link is bad and asks us to retrain (yay,
  same issue there with having to stop the pipe).
- system suspend/resume might also result in link train fail (the screen
  might not even be there any more), but the link should still keep
  running.

I guess we just need to do some additional work on top to make sure the
vblank ioctl can't see invalid state. Which would then again make upfront
link trainig possible ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch