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

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

On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> 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 ;)
>

Okay, so change 1 is to make the userspace clear the state back to Good for the property..
Then Change 2 is to set connector_changed flag in crtc_state to true if this property changed
from BAD to GOOD. I am not quite how and where to change this to state connector_changed to true.
Without this the full modeset will not happen and the whole design of retrianing is lost.

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