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

[Manasi Navare <manasi.d.navare@intel.com>]

On Mon, Oct 24, 2016 at 08:33:10AM +0200, Daniel Vetter wrote:
> 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@lists.freedesktop.org
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@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

Could you share some more details on what work would need to be done
for vblank? Then I can investigate it a little bit more tomor during the day.

Manasi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx