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@xxxxxxx> > > > > > > > > >>> Acked-by: Tony Cheng <tony.cheng@xxxxxxx> > > > > > > > > >>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > > > > > > > >>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > > > > > > > >>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > > > > > > > >>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > >>> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > > > > > > > >>> --- > > > > > > > > >>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel