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

Wed Nov 23 07:44:52 UTC 2016

On Tue, Nov 22, 2016 at 05:15:59PM -0800, Manasi Navare wrote:
> On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 21, 2016 at 09:42:57AM +0000, Chris Wilson wrote:
> > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > > > 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 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.
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > > To make this work we need a few more bits:
> > > > > 
> > > > > - link-status needs to become a full-blown atomic property. This means we
> > > > >   need to move link_status into drm_connector_state, mark the property as
> > > > >   type ATOMIC and wire up the get/set stuff.
> > > > > 
> > > > > - once that's done it's also pretty easy to set crtc->connectors_changed
> > > > >   from the atomic helpers. You can just compare old and new link_status in
> > > > >   drm_connector_state, similar to how we compare old/new CRTC in
> > > > >   drm_connector_state->crtc already.
> > > > > 
> > > > > - Another fallout is that legacy clients will no longer see the
> > > > >   link-status property. And they won't be able to set it through the
> > > > >   SETCRTC ioctl, which would kinda defaut the point. I think the best
> > > > >   solution would be to check for link_status == BAD in
> > > > >   drm_atomic_helper_set_config, and reset it to good automatically for
> > > > >   legacy clients.
> > > > 
> > > > Then how do they know that the kernel demands the modeset? Both a legacy
> > > > and atomic property?
> > > 
> > > I guess we could avoid the filtering of the property for legacy clients.
> > > Definitely not 2 properties, that's silly. Or we teach userspace to go
> > > look for atomic properties.
> > 
> > Well, now that I flushed the gunk out of my brain with some work-out it's
> > a lot easier: ATOMIC on properties is only to hide them from legacy
> > userspace, it doesn't control how it's implement. Which means we can
> > implement it as described above, and non-atomic userspace can still read
> > it. Setting would also work, but since we want to do that as part of
> > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a
> > modeset will happen or not, automagic in there seems reasonable.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> Daniel,
> I read through the atomic property functions and how those are wired, I just
> have a few questions:
> 
> -  So to make this link-status property atomic, it would still be declared as
>    a drm_property inside mode_config struct right?

Yes. Atomic is about how the property is set/read, not how it's set up.

> -  Then in drm_connector_create_standard_properties(), when this gets created,
>    it will be created with a flag DRM_MODE_PROP_ATOMIC?

No, that was just me being confused. DRM_MODE_PROP_ATOMIC only hides a
property from old/legacy userspace, it doesn't really have an influence on
how it's internally embedded. Since we want legacy/non-atomic userspace to
be able to read the link_status property using GETCONNECTOR, we do not set
the PROP_ATOMIC flag.

> -  It should still get attached in drm_connector_init right?

Yes, that's unchanged.

> -  And we will still need the drm_mode_connector_set_link_status_property() function..?

Yes, I think that's still useful. The implementation will be a bit more
tricky though, since updating an atomic property needs different locks:
1) you need to grab drm_dev->mode_config.connection_mutex.
2) Usually properties are updated by first duplicating the existing
drm_connector_state, then changing it, and then committing it. But since
link-status going from good->bad is something that already happened, and
not something we need to commit, just updating
drm_connector->state->link_status should be fine. Protected with the lock
from 1) of course.

> -  I have wired it in drm_atomic_connector_set_property/get_property() functions? Is that
>    all thats needed?

Yup, that should be all that's needed.

> I will submit RFC patches for these above changes for you to look at.

Do they still work with the compliance tool and the sna patches? You
didn't have any questions about it, so I guess you did wire up the
explicit bad->good change in drm_atomic_helper_set_config?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch