[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed

[Daniel Vetter]

On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > >>> CRTC state connector_changed needs to be set to true
> > >>> if connector link status property has changed. This will tell the
> > >>> driver to do a complete modeset due to change in connector property.
> > >>>
> > >>> Acked-by: Harry Wentland <harry.wentland at amd.com>
> > >>> Acked-by: Tony Cheng <tony.cheng at amd.com>
> > >>> 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 | 7 +++++++
> > >>>  1 file changed, 7 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > >>> index 0b16587..2125fd1 100644
> > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >>> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> > >>>  					       connector_state);
> > >>>  		if (ret)
> > >>>  			return ret;
> > >>> +
> > >>> +		if (connector->state->crtc) {
> > >>> +			crtc_state = drm_atomic_get_existing_crtc_state(state,
> > >>> +									connector->state->crtc);
> > >>> +			if (connector->link_status == DRM_MODE_LINK_STATUS_BAD)
> > >>> +				crtc_state->connectors_changed = true;
> > >>> +		}
> > >>>  	}
> > >>>  
> > >>>  	/*
> > >> This will cause ordinary atomic commits that happen to change connector flags to potentially fail with -EINVAL if ALLOW_MODESET is not set.
> > >> For this reason I'm not sure this flag should be set automatically by the kernel. Could we add add a retrain link property instead, that
> > >> always return 0 when queried, but writing a 1 causing connectors_changed to be set on bad link status?
> > > Or just check for allow_modeset before setting connectors_changed=true here?
> > 
> > I don't think modesets should be done automatically like that, even if ALLOW_MODESET is set a modeset may not be expected by userspace.
> 
> Presumably userspace would want a picture on the screen using any means
> if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> probably say as much?

Yeah, agreed. Also, if the link is bad then we pretty much have to do a
modeset to recover it, otherwise you'll be forever stuck with a bad
screen.

What we could try is to gate this of whether userspace touches the mode
property on the corresponding CRTC. I.e. if that's touched (even if it's
the same mode), and a link is bad in one of the connectors in the state
then we do a full modeset to recover.

Another option would be to make the link status writeable. Trying to
change it from bad->good would force the modeset. That would be 100% clear
to userspace, not special hacks needed with checking for allow_modeset,
no magic property that auto-changes its value. And 100% backwards compat
because existing userspace should never touch properties it doesn't
understand (except when restoring a mode, and then it should allow a full
modeset). And if someone does try a good->bad transition, we just silently
keep it at good.

Definitely need to document this properly in the property docs, no matter
what we decide.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch