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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> 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@amd.com>
> > > >>> Acked-by: Tony Cheng <tony.cheng@amd.com>
> > > >>> 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 | 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.

Hmm. I think I kinda like this idea of userspace clear the state back
to good explicitly, if it happens with the same prop. So it's like
Maarten's retrain_link prop idea, but without having to add the second
prop to the mix.

It would also save me from pointing out (for the nth time) that the
link status should really be cleared to good during the commit state
swap and not at some random point during the commit ;)

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx