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

Commit Message

Navare, Manasi Nov. 18, 2016, 7:13 a.m. UTC

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 <[e-mail]>
Acked-by: Tony Cheng <[e-mail]>
Cc: [e-mail]
Cc: Jani Nikula <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Ville Syrjala <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Maarten Lankhorst Nov. 18, 2016, 1:50 p.m. UTC | #1

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 <[e-mail]>
> Acked-by: Tony Cheng <[e-mail]>
> Cc: [e-mail]
> Cc: Jani Nikula <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Ville Syrjala <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  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?

~Maarten

Ville Syrjala Nov. 18, 2016, 2:11 p.m. UTC | #2

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 <[e-mail]>
> > Acked-by: Tony Cheng <[e-mail]>
> > Cc: [e-mail]
> > Cc: Jani Nikula <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Ville Syrjala <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  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?

Maarten Lankhorst Nov. 18, 2016, 2:18 p.m. UTC | #3

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 <[e-mail]>
>>> Acked-by: Tony Cheng <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: Jani Nikula <[e-mail]>
>>> Cc: Daniel Vetter <[e-mail]>
>>> Cc: Ville Syrjala <[e-mail]>
>>> Signed-off-by: Manasi Navare <[e-mail]>
>>> ---
>>>  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.

Navare, Manasi Nov. 18, 2016, 3:23 p.m. UTC | #4

On Fri, Nov 18, 2016 at 04:11:47PM +0200, Ville Syrjälä wrote:
> 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 <[e-mail]>
> > > Acked-by: Tony Cheng <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Jani Nikula <[e-mail]>
> > > Cc: Daniel Vetter <[e-mail]>
> > > Cc: Ville Syrjala <[e-mail]>
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  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?
> 
> -- 
> Ville Syrjälä
> Intel OTC

Yes I think thats a better solution, I can check ALLOW_MODESET set and only then poke the link_status and change the connectors_changed flag.
I am already checking this only if connector->state->crtc, so I thought that should avoid other atomoic commits to get into this check. But I guess
I will have to check for ALLOW MODESET as well. That should fix it.

Manasi

Ville Syrjala Nov. 18, 2016, 3:28 p.m. UTC | #5

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 <[e-mail]>
> >>> Acked-by: Tony Cheng <[e-mail]>
> >>> Cc: [e-mail]
> >>> Cc: Jani Nikula <[e-mail]>
> >>> Cc: Daniel Vetter <[e-mail]>
> >>> Cc: Ville Syrjala <[e-mail]>
> >>> Signed-off-by: Manasi Navare <[e-mail]>
> >>> ---
> >>>  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?

Daniel Vetter Nov. 18, 2016, 3:35 p.m. UTC | #6

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 <[e-mail]>
> > >>> Acked-by: Tony Cheng <[e-mail]>
> > >>> Cc: [e-mail]
> > >>> Cc: Jani Nikula <[e-mail]>
> > >>> Cc: Daniel Vetter <[e-mail]>
> > >>> Cc: Ville Syrjala <[e-mail]>
> > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > >>> ---
> > >>>  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

Ville Syrjala Nov. 18, 2016, 4:21 p.m. UTC | #7

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 <[e-mail]>
> > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > >>> Cc: [e-mail]
> > > >>> Cc: Jani Nikula <[e-mail]>
> > > >>> Cc: Daniel Vetter <[e-mail]>
> > > >>> Cc: Ville Syrjala <[e-mail]>
> > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > >>> ---
> > > >>>  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 ;)

Navare, Manasi Nov. 18, 2016, 5:44 p.m. UTC | #8

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 <[e-mail]>
> > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > >>> Cc: [e-mail]
> > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > >>> ---
> > > > >>>  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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Nov. 18, 2016, 6:13 p.m. UTC | #9

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 <[e-mail]>
> > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > >>> Cc: [e-mail]
> > > >>> Cc: Jani Nikula <[e-mail]>
> > > >>> Cc: Daniel Vetter <[e-mail]>
> > > >>> Cc: Ville Syrjala <[e-mail]>
> > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > >>> ---
> > > >>>  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, but this isnt solving the problem of drm_atomic_helper_check_modeset() 
actually gating this modeset requested by userspace. This function checks for the
connector->state in each crtc and sees if connector_changed and only if it is then it will
set needs_modeset flag and do a full modeset.
How should I solve this problem?
I am not familiar with the state magic and so I am asking for helo here since I am completely
clueless as to how to propogate this link-status property change to this helper function and
somehow indicate in the connector state that it changed so driver will do a full modeset.
Even if I add a new connector->state variable like link_status_changed, where can this be set?

Manasi

 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Nov. 21, 2016, 9:38 a.m. UTC | #10

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 <[e-mail]>
> > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > >>> Cc: [e-mail]
> > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > >>> ---
> > > > > >>>  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.

This way there's no magic auto-switching properties from an atomic pov
(which is a pet peeve of Ville, and I agree with it), legacy clients still
work, we have a clear model of when link-retrain happens (no magic
behavioural changes depending upon ALLOW_MODESET, making me happy) and DP
compliance should also still work.

So ... everyone happy with the above?
-Daniel

Chris Wilson Nov. 21, 2016, 9:42 a.m. UTC | #11

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 <[e-mail]>
> > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > >>> Cc: [e-mail]
> > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > >>> ---
> > > > > > >>>  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?
-Chris

Daniel Vetter Nov. 21, 2016, 10:10 a.m. UTC | #12

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 <[e-mail]>
> > > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > > >>> Cc: [e-mail]
> > > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > >>> ---
> > > > > > > >>>  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.
-Daniel

Daniel Vetter Nov. 21, 2016, 3:48 p.m. UTC | #13

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 <[e-mail]>
> > > > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > > > >>> Cc: [e-mail]
> > > > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > > >>> ---
> > > > > > > > >>>  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

Navare, Manasi Nov. 21, 2016, 7 p.m. UTC | #14

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 <[e-mail]>
> > > > > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > > > > >>> Cc: [e-mail]
> > > > > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > > > >>> ---
> > > > > > > > > >>>  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

Thanks Daniel for providing the solution alternatives here.
So after we make it atomic, we would solve the problem of updating the connector_changed
in atomic_helper_check_modeset function. So in this, who resets the property to GOOD?
Would this happen in drm_atomic_helper_set_config in both atomic and non atomic cases?

And in case of non atomic userspace, will it still be able to read link-status as BAD in userspace
to decide whether it needs to call setcrtc?

Chris, will any implementation in your patch for link _status change if this is made atomic?

Manasi

Chris Wilson Nov. 21, 2016, 8:46 p.m. UTC | #15

On Mon, Nov 21, 2016 at 11:00:52AM -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:
> > > > > - 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.
> 
> Thanks Daniel for providing the solution alternatives here.
> So after we make it atomic, we would solve the problem of updating the connector_changed
> in atomic_helper_check_modeset function. So in this, who resets the property to GOOD?
> Would this happen in drm_atomic_helper_set_config in both atomic and non atomic cases?
> 
> And in case of non atomic userspace, will it still be able to read link-status as BAD in userspace
> to decide whether it needs to call setcrtc?
> 
> Chris, will any implementation in your patch for link _status change if this is made atomic?

So long at the property remains visible via the GETCONNECTOR ioctl, no.
-Chris

Navare, Manasi Nov. 21, 2016, 9:07 p.m. UTC | #16

On Mon, Nov 21, 2016 at 08:46:19PM +0000, Chris Wilson wrote:
> On Mon, Nov 21, 2016 at 11:00:52AM -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:
> > > > > > - 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.
> > 
> > Thanks Daniel for providing the solution alternatives here.
> > So after we make it atomic, we would solve the problem of updating the connector_changed
> > in atomic_helper_check_modeset function. So in this, who resets the property to GOOD?
> > Would this happen in drm_atomic_helper_set_config in both atomic and non atomic cases?
> > 
> > And in case of non atomic userspace, will it still be able to read link-status as BAD in userspace
> > to decide whether it needs to call setcrtc?
> > 
> > Chris, will any implementation in your patch for link _status change if this is made atomic?
> 
> So long at the property remains visible via the GETCONNECTOR ioctl, no.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

If it is made an atomic property, will it be visible to userspace through
GETCONNECTOR?

Manasi

Navare, Manasi Nov. 23, 2016, 1:15 a.m. UTC | #17

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 <[e-mail]>
> > > > > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > > > > >>> Cc: [e-mail]
> > > > > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > > > >>> ---
> > > > > > > > > >>>  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?
-  Then in drm_connector_create_standard_properties(), when this gets created,
   it will be created with a flag DRM_MODE_PROP_ATOMIC?
-  It should still get attached in drm_connector_init right?
-  And we will still need the drm_mode_connector_set_link_status_property() function..?
-  I have wired it in drm_atomic_connector_set_property/get_property() functions? Is that
   all thats needed?

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

Regards
Manasi

Daniel Vetter Nov. 23, 2016, 7:44 a.m. UTC | #18

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 <[e-mail]>
> > > > > > > > > > >>> Acked-by: Tony Cheng <[e-mail]>
> > > > > > > > > > >>> Cc: [e-mail]
> > > > > > > > > > >>> Cc: Jani Nikula <[e-mail]>
> > > > > > > > > > >>> Cc: Daniel Vetter <[e-mail]>
> > > > > > > > > > >>> Cc: Ville Syrjala <[e-mail]>
> > > > > > > > > > >>> Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > > > > >>> ---
> > > > > > > > > > >>>  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

Patch

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;
+		}
 	}
 
 	/*